🎮 GameObject Toolset Redesign and Streamlining#123
Conversation
## New Tools
- find_gameobjects: Search GameObjects, returns paginated instance IDs only
- manage_components: Component lifecycle (add, remove, set_property)
## New Resources
- unity://scene/gameobject/{id}: Single GameObject data (no component serialization)
- unity://scene/gameobject/{id}/components: All components (paginated)
- unity://scene/gameobject/{id}/component/{name}: Single component by type
## Updated
- manage_scene get_hierarchy: Now includes componentTypes array
- manage_gameobject: Slimmed to lifecycle only (create, modify, delete)
- Legacy actions (find, get_components, etc.) log deprecation warnings
## Extracted Utilities
- ParamCoercion: Centralized int/bool/float/string coercion
- VectorParsing: Vector3/Vector2/Quaternion/Color parsing
- GameObjectLookup: Centralized GameObject search logic
## Test Coverage
- 76 new Unity EditMode tests for ManageGameObject actions
- 21 new pytest tests for Python tools/resources
- New NL/T CI suite for GameObject API (GO-0 to GO-5)
Addresses LLM confusion with parameter overload by splitting into
focused tools and read-only resources.
Adds unity://scene/gameobject-api resource that: - Shows in Cursor's resource list UI (no parameters needed) - Documents the parameterized gameobject resources - Explains the workflow: find_gameobjects → read resource - Lists examples and related tools
- Adds GO pass (GO-0 to GO-5) after T pass in claude-nl-suite.yml - Includes retry logic for incomplete GO tests - Updates all regex patterns to recognize GO-* test IDs - Updates DESIRED lists to include all 21 tests (NL-0..4, T-A..J, GO-0..5) - Updates default_titles for GO tests in markdown summary - Keeps separate claude-gameobject-suite.yml for standalone runs
WalkthroughAdds a GameObject API surface across Unity editor tools/resources, server-side MCP tools/resources, CI test-suite integration, helpers for JSON/vector coercion, extensive Unity editor tests, and CI workflow updates to run GO-0..GO-10 fragments via Claude. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (Client)
participant Server as MCP Server
participant Transport as Transport Layer
participant Unity as Unity Editor (MCP Bridge)
rect rgba(200,230,255,0.5)
Dev->>Server: call tool (e.g., find_gameobjects / manage_components)
Server->>Server: validate & coerce params
Server->>Transport: async_send_command_with_retry(command, params)
end
rect rgba(220,255,220,0.5)
Transport->>Unity: send MCP command (e.g., "find_gameobjects")
Unity->>Unity: perform search/manage, use Editor helpers/resources
Unity-->>Transport: result / success or error
end
rect rgba(255,245,200,0.5)
Transport->>Server: deliver response
Server->>Dev: return structured MCP response (success/message/data)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
Greptile SummaryThis PR implements a significant architectural improvement to the GameObject API by splitting monolithic tools into focused, single-responsibility components. The refactoring separates read operations (resources) from write operations (tools), improving LLM ergonomics and API clarity. Major Changes:
Key Improvements:
Architecture: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant LLM as LLM Client
participant PyServer as Python Server
participant CSharp as C# Bridge
participant Unity as Unity Editor
Note over LLM,Unity: Finding and Reading GameObject Data
LLM->>PyServer: find_gameobjects(search_term="Player", search_method="by_name")
PyServer->>CSharp: send_command("find_gameobjects", {searchTerm, searchMethod})
CSharp->>Unity: GameObjectLookup.SearchGameObjects()
Unity-->>CSharp: [12345, 67890]
CSharp-->>PyServer: {success, data: {instanceIDs, pageSize, cursor, totalCount}}
PyServer-->>LLM: {success, data: {instanceIDs: [12345]}}
Note over LLM,Unity: Reading Full GameObject Data
LLM->>PyServer: read_resource(uri="unity://scene/gameobject/12345")
PyServer->>CSharp: get_gameobject({instanceID: 12345})
CSharp->>Unity: GameObjectResource.HandleCommand()
Unity-->>CSharp: {name, tag, layer, transform, componentTypes, path}
CSharp-->>PyServer: MCPResponse(data={...})
PyServer-->>LLM: {instanceID, name, tag, layer, transform, componentTypes, path}
Note over LLM,Unity: Reading Components (Paginated)
LLM->>PyServer: read_resource(uri="unity://scene/gameobject/12345/components?page_size=10")
PyServer->>CSharp: get_gameobject_components({instanceID, pageSize, cursor})
CSharp->>Unity: GameObjectComponentsResource.HandleCommand()
Unity-->>CSharp: [components] with pagination metadata
CSharp-->>PyServer: {components, cursor, pageSize, nextCursor, totalCount}
PyServer-->>LLM: {data: {components: [...], nextCursor, hasMore}}
Note over LLM,Unity: Managing Components
LLM->>PyServer: manage_components(action="add", target="Player", component_type="Rigidbody")
PyServer->>CSharp: send_command("manage_components", {action, target, componentType})
CSharp->>Unity: ManageComponents.HandleCommand()
alt Find Target
CSharp->>Unity: GameObjectLookup.FindByTarget()
Unity-->>CSharp: GameObject reference
end
CSharp->>Unity: Undo.AddComponent(type)
Unity-->>CSharp: Component instance
CSharp-->>PyServer: {success, data: {instanceID, componentInstanceID}}
PyServer-->>LLM: {success, message, data}
|
![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
🧹 Nitpick comments (20)
.claude/prompts/nl-gameobject-suite.md (2)
16-28: XML format requirements are very strict — confirm Claude prompt clarity.The XML output format is rigidly specified (no prologue, epilogue, code fences; exactly one
<testcase>root element per file). While the spec is clear, this is fragile if Claude adds extraneous output. Consider:
- Adding an example XML file to the repo showing the exact expected format.
- Adding a validation script in CI to verify XML format before results are processed.
- Providing a template or code snippet Claude can copy-paste to ensure compliance.
103-106: Transcript minimization rules are clear but may conflict with debugging needs.Limiting per-test output to ≤400 chars and console evidence to ≤3 lines will improve CI report brevity, but may make post-mortem debugging difficult if a test fails. Consider:
- Logging full output to a separate artifact (e.g.,
reports/<TESTID>_full_log.txt) outside the XML summary.- Documenting fallback procedures (e.g., "re-run test locally with full logging enabled").
.github/workflows/claude-gameobject-suite.yml (2)
218-248: Consider adding a small delay before the first TCP probe.The bridge wait loop is robust with deadline/fatal patterns and redaction, but the immediate TCP probe at line 224 may fail if the port was just written to the status file but the socket isn't fully ready. The 2-second sleep at line 244 only occurs after a failed iteration.
This is a minor edge case since the loop will retry, but adding an initial small delay could reduce log noise.
470-491: Silent exception swallowing in fragment processing.The
except Exception: passat lines 476-477 silently ignores all parse errors. While this prevents workflow failures from malformed fragments, it could mask issues during debugging.🔎 Suggested improvement
try: froot = ET.parse(frag).getroot() if localname(froot.tag) == 'testcase': suite.append(froot) added += 1 - except Exception: - pass + except Exception as e: + print(f"Warning: failed to parse {frag}: {e}")MCPForUnity/Editor/Helpers/VectorParsing.cs (2)
106-167: Quaternion parsing may produce non-normalized quaternions.When parsing raw quaternion components
[x, y, z, w]at lines 116-124, the values are used directly without normalization. Unity'sQuaternionstruct can handle non-normalized quaternions, but they may cause unexpected behavior in some operations (e.g., interpolation, comparison).Consider normalizing when parsing raw components, or document that callers should normalize if needed.
🔎 Suggested improvement
// Quaternion components: [x, y, z, w] if (array.Count >= 4) { - return new Quaternion( + var q = new Quaternion( array[0].ToObject<float>(), array[1].ToObject<float>(), array[2].ToObject<float>(), array[3].ToObject<float>() ); + return q.normalized; }
271-291: ParseBounds silently defaults to zero vectors on parse failure.At lines 280-281, if
ParseVector3returns null for center or size, the code defaults toVector3.zero. This could mask malformed input data. Consider logging a warning when falling back to defaults.🔎 Suggested improvement
if (token is JObject obj && obj.ContainsKey("center") && obj.ContainsKey("size")) { - var center = ParseVector3(obj["center"]) ?? Vector3.zero; - var size = ParseVector3(obj["size"]) ?? Vector3.zero; + var center = ParseVector3(obj["center"]); + var size = ParseVector3(obj["size"]); + if (center == null || size == null) + { + Debug.LogWarning($"[VectorParsing] Bounds has invalid center/size: center={center}, size={size}"); + return null; + } - return new Bounds(center, size); + return new Bounds(center.Value, size.Value); }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (2)
286-304: Consider using try-finally to ensureLogAssert.ignoreFailingMessagesis reset.If the test throws an exception before line 300,
ignoreFailingMessagesremainstrue, which could mask failures in subsequent tests running in the same session.🔎 Proposed fix
[Test] public void Find_ByTag_InvalidTag_ReturnsSuccessWithEmptyResult() { // Current behavior: returns success=true even for invalid tag // Ignore expected warning logs about invalid tag UnityEngine.TestTools.LogAssert.ignoreFailingMessages = true; - - var p = new JObject - { - ["action"] = "find", - ["target"] = "NonExistentTag12345", - ["searchMethod"] = "by_tag" - }; - - var result = ManageGameObject.HandleCommand(p); - var resultObj = result as JObject ?? JObject.FromObject(result); - - UnityEngine.TestTools.LogAssert.ignoreFailingMessages = false; - - // Current behavior returns success even when nothing found - Assert.IsTrue(resultObj.Value<bool>("success"), "Current behavior returns success for not found"); + try + { + var p = new JObject + { + ["action"] = "find", + ["target"] = "NonExistentTag12345", + ["searchMethod"] = "by_tag" + }; + + var result = ManageGameObject.HandleCommand(p); + var resultObj = result as JObject ?? JObject.FromObject(result); + + // Current behavior returns success even when nothing found + Assert.IsTrue(resultObj.Value<bool>("success"), "Current behavior returns success for not found"); + } + finally + { + UnityEngine.TestTools.LogAssert.ignoreFailingMessages = false; + } }
562-566: Fragile assertion - consider verifying the actual key name.Checking for substring matches across multiple possible key formats (
instanceID,InstanceID,instance) is brittle and could pass incorrectly if any of these strings appear elsewhere in the response.🔎 Proposed fix
// Verify instanceID is present in response - var dataStr = resultObj["data"]?.ToString() ?? ""; - Assert.IsTrue(dataStr.Contains("instanceID") || dataStr.Contains("InstanceID") || dataStr.Contains("instance"), - "Response should include instance ID"); + var data = resultObj["data"] as JObject; + Assert.IsNotNull(data, "Should return data object"); + Assert.IsTrue(data["instanceID"] != null || data["InstanceID"] != null, + "Response should include instance ID field");Server/tests/integration/test_find_gameobjects.py (1)
176-200: Consider adding a test for the missingsearch_termerror case.The
find_gameobjectsfunction returns an error whensearch_termis missing. A test verifying this behavior would improve coverage.🔎 Suggested test
@pytest.mark.asyncio async def test_find_gameobjects_missing_search_term(): """Test error when search_term is missing.""" resp = await find_go_mod.find_gameobjects( ctx=DummyContext(), search_term="", # Empty search term search_method="by_name", ) assert resp.get("success") is False assert "search_term" in resp.get("message", "").lower()Server/src/services/tools/find_gameobjects.py (1)
58-84: Consider logging exceptions before returning error response.While catching broad
Exceptionis acceptable here as a last-resort handler, logging the full exception (including traceback) would aid debugging in production.🔎 Proposed improvement
+ import logging + logger = logging.getLogger(__name__) + try: params = { # ... } # ... except Exception as e: + logger.exception("Error searching GameObjects") return {"success": False, "message": f"Error searching GameObjects: {e!s}"}Server/src/services/tools/manage_components.py (1)
124-125: Same exception handling pattern as other tools - consider adding logging.Similar to
find_gameobjects.py, consider adding exception logging before returning the error response to aid debugging.🔎 Proposed improvement
except Exception as e: + import logging + logging.getLogger(__name__).exception("Error managing component") return {"success": False, "message": f"Error managing component: {e!s}"}MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
875-905: Defensive coding for error aggregation, but reflection access could be fragile.The error aggregation uses reflection to access
data.errorsfrom error objects (lines 883-896). This approach is fragile since it relies on the internal structure ofErrorResponse. Consider using a more type-safe approach or documenting thatErrorResponse.datashould contain anerrorsproperty.🔎 Consider type-safe error extraction
foreach (var errorObj in componentErrors) { - try - { - var dataProp = errorObj?.GetType().GetProperty("data"); - var dataVal = dataProp?.GetValue(errorObj); - if (dataVal != null) - { - var errorsProp = dataVal.GetType().GetProperty("errors"); - var errorsEnum = errorsProp?.GetValue(dataVal) as System.Collections.IEnumerable; - if (errorsEnum != null) - { - foreach (var item in errorsEnum) - { - var s = item?.ToString(); - if (!string.IsNullOrEmpty(s)) aggregatedErrors.Add(s); - } - } - } - } - catch { } + if (errorObj is ErrorResponse errResp && errResp.Data is IDictionary<string, object> dataDict) + { + if (dataDict.TryGetValue("errors", out var errorsVal) && errorsVal is IEnumerable<string> errs) + { + aggregatedErrors.AddRange(errs.Where(s => !string.IsNullOrEmpty(s))); + } + } }MCPForUnity/Editor/Helpers/ParamCoercion.cs (1)
89-112: Consider culture-invariant float parsing.The
float.TryParseon line 103 uses the current culture by default. In locales where the decimal separator is,instead of., parsing"3.14"would fail. Consider usingCultureInfo.InvariantCulturefor consistent behavior across environments.🔎 Proposed fix for culture-invariant parsing
+using System.Globalization; + public static float CoerceFloat(JToken token, float defaultValue) { if (token == null || token.Type == JTokenType.Null) return defaultValue; try { if (token.Type == JTokenType.Float || token.Type == JTokenType.Integer) return token.Value<float>(); var s = token.ToString().Trim(); if (s.Length == 0) return defaultValue; - if (float.TryParse(s, out var f)) + if (float.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out var f)) return f; } catch { // Swallow and return default } return defaultValue; }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (2)
124-150: Tag test may be flaky due to using built-in "MainCamera" tag.Using
"MainCamera"as a test tag could inadvertently match existing scene objects (e.g., the default camera in a test scene), leading to flaky tests. Also, lines 144-145 only assertIsNotNull(result)without verifying success or the deletion count, which weakens the test.🔎 Consider using a less common tag or creating a custom one
[Test] public void Delete_ByTag_DeletesMatchingObjects() { var target1 = CreateTestObject("DeleteByTag1"); var target2 = CreateTestObject("DeleteByTag2"); - // Use built-in tag - target1.tag = "MainCamera"; - target2.tag = "MainCamera"; + // Use a less commonly used built-in tag to reduce conflicts + target1.tag = "EditorOnly"; + target2.tag = "EditorOnly"; var p = new JObject { ["action"] = "delete", - ["target"] = "MainCamera", + ["target"] = "EditorOnly", ["searchMethod"] = "by_tag" }; var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); - // Capture current behavior - may delete one or all - Assert.IsNotNull(result, "Should return a result"); + Assert.IsTrue(resultObj.Value<bool>("success"), "Should successfully delete tagged objects"); // Clean up tracking testObjects.Remove(target1); testObjects.Remove(target2); }
291-309: Inactive object deletion test has weak assertion.The test
Delete_InactiveObject_StillDeletesonly checks that a result is returned (line 306) without verifying that the inactive object was actually deleted or that the operation succeeded. This doesn't validate the intended behavior.🔎 Strengthen the assertion
[Test] public void Delete_InactiveObject_StillDeletes() { var target = CreateTestObject("InactiveDeleteTarget"); target.SetActive(false); var p = new JObject { ["action"] = "delete", ["target"] = "InactiveDeleteTarget", ["searchMethod"] = "by_name" }; var result = ManageGameObject.HandleCommand(p); - // Capture current behavior for inactive objects - Assert.IsNotNull(result, "Should return a result"); + var resultObj = result as JObject ?? JObject.FromObject(result); + Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); + + // Verify the inactive object was actually deleted + // Note: GameObject.Find won't find inactive objects, so this verifies deletion + var allObjects = Object.FindObjectsByType<GameObject>(FindObjectsInactive.Include, FindObjectsSortMode.None); + Assert.IsFalse(allObjects.Any(go => go.name == "InactiveDeleteTarget"), "Inactive object should be deleted"); testObjects.Remove(target); }MCPForUnity/Editor/Tools/ManageComponents.cs (2)
110-120: Consider using typed response classes for consistency.The method returns anonymous objects (
new { success = true, ... }), while other parts of the codebase useSuccessResponseandErrorResponse. This inconsistency could cause serialization issues or make client-side parsing harder.🔎 Use typed response for consistency
-return new -{ - success = true, - message = $"Component '{componentType}' added to '{targetGo.name}'.", - data = new - { - instanceID = targetGo.GetInstanceID(), - componentType = type.FullName, - componentInstanceID = newComponent.GetInstanceID() - } -}; +return new SuccessResponse( + $"Component '{componentType}' added to '{targetGo.name}'.", + new + { + instanceID = targetGo.GetInstanceID(), + componentType = type.FullName, + componentInstanceID = newComponent.GetInstanceID() + } +);
149-159: Transform removal check should happen before GetComponent.The check for
typeof(Transform)on line 156 occurs after attemptingGetComponent(type)on line 149. While this works, checking earlier avoids unnecessary component lookups and provides a clearer error message flow.🔎 Move Transform check earlier
// Resolve component type Type type = FindComponentType(componentType); if (type == null) { return new ErrorResponse($"Component type '{componentType}' not found."); } +// Prevent removal of Transform +if (type == typeof(Transform)) +{ + return new ErrorResponse("Cannot remove the Transform component."); +} + Component component = targetGo.GetComponent(type); if (component == null) { return new ErrorResponse($"Component '{componentType}' not found on '{targetGo.name}'."); } -// Prevent removal of Transform -if (type == typeof(Transform)) -{ - return new ErrorResponse("Cannot remove the Transform component."); -}MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
258-272: Recursive traversal may cause stack overflow on very deep hierarchies.
GetObjectAndDescendantsuses recursion which could overflow the stack on extremely deep hierarchies (thousands of levels). While rare in practice, consider an iterative approach using an explicit stack for robustness.Server/src/services/resources/gameobject.py (2)
29-69: Thectxparameter is unused inget_gameobject_api_docs.The static analysis correctly identifies that
ctxis not used. Since this is a documentation-only resource that returns static content, consider either prefixing with underscore to indicate intentional non-use, or removing if the decorator allows.🔎 Prefix unused parameter
@mcp_for_unity_resource( uri="unity://scene/gameobject-api", name="gameobject_api", description="Documentation for GameObject resources. Use find_gameobjects tool to get instance IDs, then access resources below." ) -async def get_gameobject_api_docs(ctx: Context) -> MCPResponse: +async def get_gameobject_api_docs(_ctx: Context) -> MCPResponse:
125-129: Response handling pattern is duplicated across endpoints.The same response handling logic appears in all three resource functions (lines 125-129, 182-186, 229-233). Consider extracting to a helper function for DRY compliance.
🔎 Extract response handling helper
def _normalize_response(response: Any) -> MCPResponse: """Normalize Unity transport response to MCPResponse.""" if isinstance(response, dict): if not response.get("success", True): return MCPResponse(**response) return MCPResponse(**response) return responseThen use in each endpoint:
- if isinstance(response, dict): - if not response.get("success", True): - return MCPResponse(**response) - return MCPResponse(**response) - return response + return _normalize_response(response)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.claude/prompts/nl-gameobject-suite.md.github/workflows/claude-gameobject-suite.yml.github/workflows/claude-nl-suite.ymlMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/GameObjectLookup.cs.metaMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Helpers/ParamCoercion.cs.metaMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Helpers/VectorParsing.cs.metaMCPForUnity/Editor/Resources/Scene.metaMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.cs.metaMCPForUnity/Editor/Tools/FindGameObjects.csMCPForUnity/Editor/Tools/FindGameObjects.cs.metaMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageComponents.cs.metaMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageScene.csServer/src/services/resources/gameobject.pyServer/src/services/tools/find_gameobjects.pyServer/src/services/tools/manage_components.pyServer/tests/integration/test_find_gameobjects.pyServer/tests/integration/test_gameobject_resources.pyServer/tests/integration/test_manage_components.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs.meta
🧰 Additional context used
🧬 Code graph analysis (9)
Server/src/services/tools/find_gameobjects.py (5)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/utils.py (2)
coerce_bool(11-24)coerce_int(63-77)Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
Server/tests/integration/test_manage_components.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/tools/manage_components.py (1)
manage_components(19-125)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (18)
GameObject(30-38)TearDown(17-28)Test(42-59)Test(61-73)Test(75-88)Test(94-114)Test(116-134)Test(136-154)Test(156-173)Test(175-192)Test(194-211)Test(213-226)Test(232-250)Test(252-271)Test(273-291)Test(293-314)Test(320-342)Test(344-357)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (2)
GameObject(30-35)TearDown(17-28)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (2)
GameObject(41-46)TearDown(28-39)
Server/tests/integration/test_gameobject_resources.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/resources/gameobject.py (3)
get_gameobject(109-129)get_gameobject_components(155-186)get_gameobject_component(206-233)
Server/src/services/resources/gameobject.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
Server/tests/integration/test_find_gameobjects.py (2)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/services/tools/find_gameobjects.py (1)
find_gameobjects(19-84)
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (3)
MCPForUnity/Editor/Tools/ManageGameObject.cs (10)
UnityEngine(2438-2535)GameObject(1589-1612)List(1617-1776)List(2624-2661)List(2684-2702)List(2707-2746)List(2754-2789)IEnumerable(1779-1792)IEnumerable(2664-2671)Type(2542-2556)MCPForUnity/Editor/Tools/ManageScene.cs (1)
GameObject(577-622)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
MCPForUnity/Editor/Tools/ManageComponents.cs (3)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(10-157)CoerceString(120-127)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (6)
GameObject(57-64)GameObject(69-72)Type(277-311)List(82-86)List(96-135)GameObjectLookup(15-332)MCPForUnity/Editor/Helpers/VectorParsing.cs (4)
Vector3(18-51)Vector3(56-59)VectorParsing(11-292)Vector2(66-97)
MCPForUnity/Editor/Tools/ManageGameObject.cs (3)
MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)MCPForUnity/Editor/Helpers/Response.cs (2)
ErrorResponse(35-67)ErrorResponse(61-66)MCPForUnity/Editor/Helpers/ParamCoercion.cs (3)
ParamCoercion(10-157)CoerceInt(18-44)CoerceBool(52-81)
🪛 Ruff (0.14.10)
Server/src/services/tools/find_gameobjects.py
83-83: Do not catch blind exception: Exception
(BLE001)
Server/src/services/tools/manage_components.py
124-124: Do not catch blind exception: Exception
(BLE001)
Server/tests/integration/test_manage_components.py
17-17: Unused function argument: kwargs
(ARG001)
52-52: Unused function argument: cmd
(ARG001)
52-52: Unused function argument: kwargs
(ARG001)
79-79: Unused function argument: cmd
(ARG001)
79-79: Unused function argument: kwargs
(ARG001)
109-109: Unused function argument: cmd
(ARG001)
109-109: Unused function argument: kwargs
(ARG001)
137-137: Unused function argument: cmd
(ARG001)
137-137: Unused function argument: kwargs
(ARG001)
164-164: Unused function argument: cmd
(ARG001)
164-164: Unused function argument: kwargs
(ARG001)
194-194: Unused function argument: cmd
(ARG001)
194-194: Unused function argument: kwargs
(ARG001)
221-221: Unused function argument: cmd
(ARG001)
221-221: Unused function argument: kwargs
(ARG001)
Server/tests/integration/test_gameobject_resources.py
20-20: Unused function argument: kwargs
(ARG001)
58-58: Unused function argument: kwargs
(ARG001)
97-97: Unused function argument: cmd
(ARG001)
97-97: Unused function argument: kwargs
(ARG001)
135-135: Unused function argument: cmd
(ARG001)
135-135: Unused function argument: kwargs
(ARG001)
173-173: Unused function argument: kwargs
(ARG001)
210-210: Unused function argument: cmd
(ARG001)
210-210: Unused function argument: params
(ARG001)
210-210: Unused function argument: kwargs
(ARG001)
235-235: Unused function argument: cmd
(ARG001)
235-235: Unused function argument: params
(ARG001)
235-235: Unused function argument: kwargs
(ARG001)
Server/src/services/resources/gameobject.py
29-29: Unused function argument: ctx
(ARG001)
Server/tests/integration/test_find_gameobjects.py
17-17: Unused function argument: kwargs
(ARG001)
54-54: Unused function argument: cmd
(ARG001)
54-54: Unused function argument: kwargs
(ARG001)
89-89: Unused function argument: cmd
(ARG001)
89-89: Unused function argument: kwargs
(ARG001)
128-128: Unused function argument: cmd
(ARG001)
128-128: Unused function argument: kwargs
(ARG001)
155-155: Unused function argument: cmd
(ARG001)
155-155: Unused function argument: kwargs
(ARG001)
181-181: Unused function argument: cmd
(ARG001)
181-181: 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). (2)
- GitHub Check: nl-suite
- GitHub Check: Greptile Review
🔇 Additional comments (53)
MCPForUnity/Editor/Helpers/ParamCoercion.cs.meta (1)
1-11: LGTM!Unity meta file is correctly formatted with standard MonoImporter configuration. No issues identified.
MCPForUnity/Editor/Helpers/GameObjectLookup.cs.meta (1)
1-11: LGTM!Unity meta file is correctly formatted with standard MonoImporter configuration. No issues identified.
.claude/prompts/nl-gameobject-suite.md (1)
77-83: UseDebug.LogWarning()instead ofDebug.Log()for the deprecation message.The deprecation warning for
manage_gameobject(action="find")exists and correctly mentionsfind_gameobjectsas the replacement (ManageGameObject.cs:183). However, it's logged usingDebug.Log()(Info level) instead ofDebug.LogWarning()(Warning level). For test GO-5 to properly detect this as a deprecation warning, change the logging call toDebug.LogWarning()so the message appears with appropriate severity in the console.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs.meta (1)
1-11: Standard Unity metadata file; no review needed.This is auto-generated boilerplate metadata that Unity requires for asset tracking. No functional or structural issues.
MCPForUnity/Editor/Tools/ManageComponents.cs.meta (1)
1-11: Standard Unity metadata file; no review needed.This is auto-generated boilerplate metadata that Unity requires for asset tracking. No functional or structural issues.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs.meta (1)
1-11: Standard Unity metadata file; no review needed.This is auto-generated boilerplate metadata that Unity requires for asset tracking. No functional or structural issues.
.github/workflows/claude-gameobject-suite.yml (4)
1-11: Well-structured workflow configuration.The workflow setup with
workflow_dispatch, appropriate permissions (read contents, write checks), and concurrency control withcancel-in-progress: truefollows GitHub Actions best practices.
24-41: Robust secret detection with clear output flags.The secret detection step properly checks for required credentials and sets clear output flags for downstream conditional steps. The logic correctly handles the OR condition for Unity licensing (either ULF or email/password).
400-435: Backfill script handles missing tests correctly.The Python script properly identifies missing GO fragments and generates placeholder testcases with appropriate failure messages. The regex parsing and XML generation logic is sound.
622-636: Proper cleanup and license return handling.The cleanup steps correctly use
if: always()to ensure they run regardless of prior step outcomes. The license return step appropriately usescontinue-on-error: trueto prevent cleanup failures from marking the job as failed.MCPForUnity/Editor/Helpers/VectorParsing.cs (2)
18-51: Solid Vector3 parsing with appropriate null handling.The implementation correctly handles both array
[x, y, z]and object{x, y, z}formats with null-safe checks and exception logging. The use of nullable return type is appropriate for the parsing scenario.
175-223: Color parsing assumes normalized 0-1 range.The implementation treats color values as floats in the 0-1 range (Unity's
Colorconvention). If the input uses 0-255 byte values (common in web/JSON contexts), colors will be incorrect. Consider documenting this assumption or adding detection logic.Verify that all JSON sources producing color data use the 0-1 range convention expected by Unity's
Colorstruct.MCPForUnity/Editor/Helpers/VectorParsing.cs.meta (1)
1-11: Standard Unity meta file.Auto-generated Unity metadata with unique GUID and default MonoImporter settings.
.github/workflows/claude-nl-suite.yml (7)
754-780: GO pass integration follows established NL/T pattern.The Claude GO pass step correctly mirrors the structure of existing NL and T passes, with appropriate model selection, prompt file, and system prompt specifying GO-0 through GO-5 fragments.
782-821: GO coverage check and retry logic is consistent with T pass.The pre-retry coverage check and Sonnet retry step for GO fragments follow the same robust pattern used for T fragments, ensuring test completeness.
868-873: GO canonicalization rules added correctly.The RULES list is properly extended with GO-0 through GO-5 patterns matching the expected test titles.
900-903: Regex updates consistently handle GO- prefix.The
id_from_filenamefunction and title extraction regex are correctly updated to recognizeGO-\d+pattern alongside existing NL and T patterns.Also applies to: 918-918
941-955: DESIRED list and backfill logic include GO tests.The backfill script correctly includes GO-0 through GO-5 in the DESIRED list, and the
id_from_filenamefunction handles GO fragment detection.
1167-1190: Markdown summary includes GO tests with proper titles.The
desiredlist anddefault_titlesdictionary are correctly synchronized to include all GO tests with appropriate human-readable descriptions.
1282-1282: CI gate includes GO tests in required coverage.The final CI gate correctly requires all GO tests (GO-0 through GO-5) to be present and passing alongside NL and T tests.
MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs.meta (1)
1-11: Standard Unity meta file.Auto-generated Unity metadata with unique GUID and default MonoImporter settings.
MCPForUnity/Editor/Resources/Scene.meta (1)
1-8: Standard Unity folder meta file.Auto-generated Unity metadata for the Scene directory with unique GUID and default DefaultImporter settings.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs.meta (1)
1-11: Standard Unity meta file for test class.Auto-generated Unity metadata with unique GUID and default MonoImporter settings for the ManageGameObjectModifyTests test class.
MCPForUnity/Editor/Tools/ManageScene.cs (1)
632-648: LGTM! Well-integrated component type enumeration.The addition of
componentTypesenhances the GameObject summary with useful metadata while remaining lightweight. The implementation follows best practices:
- Defensive null checks on components
- Try-catch wrapping for safety
- Consistent with existing code patterns
- Only collects type names without expensive serialization
Also applies to: 664-664
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (3)
15-38: LGTM! Clean test infrastructure.The setup/teardown pattern with tracked object cleanup ensures tests don't pollute the scene. The
FindAndTrackhelper provides a convenient way to manage test objects while maintaining the cleanup list.
403-423: Verify Unity error message format across versions.The
LogAssert.Expectregex pattern assumes a specific Unity error message format for invalid tags. While the wildcards (.*) provide some flexibility, Unity's error messages can vary across versions.Consider documenting the Unity version(s) this test is validated against, or make the pattern more lenient if the test fails unexpectedly in different Unity versions:
// Alternative: More lenient pattern UnityEngine.TestTools.LogAssert.Expect(LogType.Error, new System.Text.RegularExpressions.Regex("(Tag|tag).*NonExistentTag12345"));
428-479: LGTM! Flexible response validation.The response structure tests are appropriately defensive, checking multiple field name variations (instanceID/InstanceID, name/Name) and using string containment as a fallback. This flexibility is valuable for baseline tests that capture existing behavior before API redesign.
Server/tests/integration/test_manage_components.py (1)
1-242: LGTM! Comprehensive test coverage.The test suite thoroughly validates the manage_components tool with good coverage of:
- Component lifecycle operations (add, remove, set_property)
- Single and multiple property handling
- JSON string property parsing
- Search method passthrough
- Targeting by ID and other methods
The mock function signatures intentionally match the function being patched, so unused arguments (cmd, kwargs) are expected and the static analysis warnings can be safely ignored.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (1)
17-28: LGTM! Clean test lifecycle management.The TearDown method properly cleans up test objects with null checks and clearing the list.
Server/tests/integration/test_gameobject_resources.py (2)
15-51: Well-structured integration tests with good coverage.The tests properly mock
async_send_command_with_retry, capture parameters to verify they're correctly propagated, and assert on response structure.
207-254: Good error case coverage.Both component-not-found and GameObject-not-found scenarios are tested, verifying that error messages include the relevant identifiers.
MCPForUnity/Editor/Tools/FindGameObjects.cs (2)
24-48: Clean parameter handling with good defaults and validation.Good defensive coding with null check, multiple parameter name aliases for compatibility, and proper bounds clamping on pageSize.
50-81: Well-implemented pagination logic.The Skip/Take pattern correctly implements cursor-based pagination, and the response includes all metadata needed for clients to request subsequent pages.
Server/tests/integration/test_find_gameobjects.py (2)
12-47: Good basic test verifying command construction.Properly validates that the command name is
find_gameobjectsand parameters are correctly mapped.
123-148: Good coercion test for boolean strings.This verifies that the
coerce_boolutility correctly handles string"true"input, which is important for robustness with various client inputs.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (3)
17-46: Good test setup with baseline object creation.Creating a standard test object in SetUp ensures consistent initial state across tests, and TearDown properly cleans up all created objects.
142-211: Thorough transform modification testing.Tests cover position, rotation, scale individually and combined, with proper assertions on actual GameObject state (not just command success).
394-412: Correct use ofLogAssert.Expectfor expected error logs.Unlike
ignoreFailingMessages,LogAssert.Expectproperly validates that the expected log message occurs, making this a proper assertion rather than just suppression.Server/src/services/tools/find_gameobjects.py (1)
16-45: Well-designed tool with clear documentation and typing.The function signature uses
Annotatedtypes with descriptive strings, and the docstring provides helpful guidance on using the returned IDs with related resources.Server/src/services/tools/manage_components.py (3)
46-59: Excellent docstring with practical examples.The examples clearly demonstrate each action type with realistic parameters, making the API easy to understand and use.
66-71: Redundant validation -actionis already constrained byLiteraltype.The
if not actioncheck can never be true whenactionis required and typed asLiteral["add", "remove", "set_property"]. However, keeping it provides defensive protection against runtime type violations.
99-107: Correct handling of property value assignment.Using
value is not Nonecorrectly allows setting falsy values like0,False, or empty strings. The conditional logic properly separates single property vs. batch properties scenarios.MCPForUnity/Editor/Tools/ManageGameObject.cs (2)
169-180: LGTM on primary lifecycle actions structure.The organization of actions into primary lifecycle operations (
create,modify,delete,duplicate,move_relative) kept in this tool is well-structured and aligns with the API redesign goals.
181-198: Deprecation notices and centralized coercion look good.The deprecation messages are clear and provide actionable guidance to the newer APIs. The switch to
Helpers.ParamCoercion.CoerceIntandCoerceBoolcentralizes parameter handling and improves consistency across the codebase.MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
18-44: CoerceInt implementation is solid.The method handles multiple input scenarios cleanly: null tokens, integer tokens, numeric strings, and even floats truncated to int. The defensive try/catch ensures robustness.
52-81: CoerceBool handles common string representations well.Good coverage of boolean-like strings (
"1","yes","on", etc.). This provides flexibility for various API callers.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (1)
13-35: Test structure follows established patterns.The
TearDownandCreateTestObjectmethods match the patterns used inManageGameObjectFindTestsandManageGameObjectModifyTests, ensuring consistency across the test suite. Based on learnings from relevant code snippets.MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs (3)
17-64: GameObjectResource implementation is well-structured.The resource handler properly validates parameters, uses
EditorUtility.InstanceIDToObjectfor efficient lookup, and returns comprehensive GameObject data without over-serializing components. The error handling is consistent.
144-149: Pagination parameters are properly clamped.Good defensive coding by clamping
pageSizeto a reasonable range (1-100). Consider documenting this limit in the resource description or making it configurable.
243-259: Component lookup uses case-insensitive comparison.The component name matching uses
StringComparison.OrdinalIgnoreCase, which is user-friendly and allows callers to use either"Camera"or"camera". Good UX consideration.MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
316-331: GetGameObjectPath implementation is clean and correct.The path building logic properly traverses up the hierarchy and constructs a valid path string. Good utility method.
Server/src/services/resources/gameobject.py (2)
72-97: Pydantic models provide good type safety.The
TransformData,GameObjectData, and related models with sensible defaults ensure consistent response structures. This is good API design.
155-180: Components endpoint with pagination is well-designed.The pagination parameters (
page_size,cursor,include_properties) with appropriate defaults provide flexible access to component data without overwhelming responses.
Stress Tests (12 new tests): - BulkCreate small/medium batches - FindGameObjects pagination with by_component search - AddComponents to single object - GetComponents with full serialization - SetComponentProperties (complex Rigidbody) - Deep hierarchy creation and path lookup - GetHierarchy with large scenes - Resource read performance tests - RapidFire create-modify-delete cycles NL/T Suite Updates: - Added GO-0..GO-10 tests in nl-gameobject-suite.md - Fixed tool naming: mcp__unity__ → mcp__UnityMCP__ Other: - Fixed LongUnityScriptClaudeTest.cs compilation errors - Added reports/, .claude/local/, scripts/local-test/ to .gitignore All 254 EditMode tests pass (250 run, 4 explicit skips)
- ParamCoercion: Use CultureInfo.InvariantCulture for float parsing - ManageComponents: Move Transform removal check before GetComponent - ManageGameObjectFindTests: Use try-finally for LogAssert.ignoreFailingMessages - VectorParsing: Document that quaternions are not auto-normalized - gameobject.py: Prefix unused ctx parameter with underscore
dsarno
changed the title
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
MCPForUnity/Editor/Tools/ManageComponents.cs (1)
312-347: FindComponentType duplicates logic from GameObjectLookup.This method is nearly identical to
GameObjectLookup.FindComponentType. Consider reusing the existing implementation to avoid divergence and reduce maintenance burden..claude/prompts/nl-gameobject-suite.md (1)
50-66: Tests GO-2 and GO-3 depend on GO-1 success.If GO-1 fails or returns no results (e.g., no Camera component in the test scene), GO-2 and GO-3 will fail with confusing errors due to an invalid instance ID. Consider documenting scene prerequisites or adding a fallback mechanism.
🧹 Nitpick comments (25)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (4)
87-87: Consider extracting performance thresholds to constants.The hard-coded time limits (5000ms, 15000ms) may be too strict for slower CI environments or under-resourced machines. Consider extracting these to named constants at the class level or making them configurable via environment variables.
🔎 Example refactor
At the class level:
+private const int SMALL_BATCH_TIMEOUT_MS = 5000; +private const int MEDIUM_BATCH_TIMEOUT_MS = 15000;Then use:
-Assert.Less(sw.ElapsedMilliseconds, 5000, "Bulk create took too long"); +Assert.Less(sw.ElapsedMilliseconds, SMALL_BATCH_TIMEOUT_MS, "Bulk create took too long");Also applies to: 115-115
538-546: Verify that layer modification works correctly.The comment indicates layer modification was replaced with name modification to "avoid layer name issues." This workaround might be hiding a bug in the underlying layer modification logic. Consider adding a separate test that explicitly validates layer changes to ensure that functionality works correctly.
Do you want me to create a separate test case that verifies layer modification functionality?
549-556: Consider verifying objects are actually destroyed.After the delete operation succeeds, consider adding an assertion to verify the object is actually destroyed (e.g.,
EditorUtility.InstanceIDToObject(instanceId)returns null). This would catch cases where the delete operation reports success but doesn't actually remove the object.🔎 Suggested addition
Assert.IsTrue(deleteResult["success"]?.Value<bool>() ?? false, $"Delete failed: {deleteResult["message"]}"); + +// Verify object is actually destroyed +var destroyed = EditorUtility.InstanceIDToObject(instanceId) as GameObject; +Assert.IsNull(destroyed, $"Object {instanceId} should be destroyed but still exists");
561-561: Performance threshold follows same pattern as earlier tests.Like the bulk creation tests, this uses a hard-coded timeout. Consider applying the same refactoring suggestion to extract this to a named constant for easier adjustment.
MCPForUnity/Editor/Tools/ManageComponents.cs (3)
340-343: Empty catch block swallows all exceptions silently.While skipping problematic assemblies is reasonable, consider logging at a verbose/debug level for troubleshooting purposes, or catching only the specific exception types expected (e.g.,
ReflectionTypeLoadException,FileNotFoundException).🔎 Proposed fix
catch + catch (Exception ex) { - // Skip assemblies that can't be searched + // Skip assemblies that can't be searched (e.g., dynamic assemblies) + // Debug.Log($"[ManageComponents] Skipping assembly {assembly.GetName().Name}: {ex.Message}"); }
349-358: SetPropertiesOnComponent silently discards errors.Unlike
SetPropertywhich collects errors and returns them, this helper method ignores the return value ofTrySetProperty. This inconsistency means property-setting failures duringAddComponentgo unreported.🔎 Consider logging or collecting errors
private static void SetPropertiesOnComponent(Component component, JObject properties) { if (component == null || properties == null) return; + var errors = new List<string>(); foreach (var prop in properties.Properties()) { - TrySetProperty(component, prop.Name, prop.Value); + var error = TrySetProperty(component, prop.Name, prop.Value); + if (error != null) + errors.Add(error); } + if (errors.Count > 0) + Debug.LogWarning($"[ManageComponents] Some properties failed to set: {string.Join(", ", errors)}"); }
405-430: ConvertValue returns null for null tokens, which may cause issues with value types.When
tokenis null/JTokenType.Null, this returnsnull. If the target type is a value type (e.g.,int,float,bool), this could cause issues when the caller attempts to set the property. The callers do handle exceptions, but returning a proper default for value types would be cleaner.🔎 Consider returning default for value types
private static object ConvertValue(JToken token, Type targetType) { if (token == null || token.Type == JTokenType.Null) - return null; + return targetType.IsValueType ? Activator.CreateInstance(targetType) : null;MCPForUnity/Editor/Helpers/VectorParsing.cs (1)
269-293: Consider adding array format support for Bounds parsing.The docstring mentions only object format, but
ParseRectsupports both array and object formats. For API consistency, consider adding array format[cx, cy, cz, sx, sy, sz]support forParseBoundsas well, or document why it's intentionally omitted.Server/src/services/resources/gameobject.py (3)
99-102: Response wrapper classes are defined but never used.
GameObjectResponse,ComponentsResponse, andSingleComponentResponseare defined but the endpoint functions returnMCPResponsedirectly. Either use these typed response classes for better type safety, or remove them to reduce dead code.🔎 Option 1: Use the typed response classes
async def get_gameobject(ctx: Context, instance_id: str) -> MCPResponse: +async def get_gameobject(ctx: Context, instance_id: str) -> GameObjectResponse: """Get GameObject data by instance ID.""" # ... existing code ... if isinstance(response, dict): if not response.get("success", True): - return MCPResponse(**response) - return MCPResponse(**response) + return GameObjectResponse(**response) + return GameObjectResponse(**response) return response🔎 Option 2: Remove unused classes
Remove the
GameObjectResponse,ComponentsResponse, andSingleComponentResponseclasses if you don't need the extra type information.Also applies to: 145-148, 196-199
125-129: Duplicate response handling pattern across all endpoints.The same response normalization logic is repeated in
get_gameobject,get_gameobject_components, andget_gameobject_component. Consider extracting to a helper function.🔎 Proposed helper function
def _normalize_response(response: dict | MCPResponse) -> MCPResponse: """Normalize Unity response to MCPResponse.""" if isinstance(response, dict): return MCPResponse(**response) return responseThen use in each endpoint:
- if isinstance(response, dict): - if not response.get("success", True): - return MCPResponse(**response) - return MCPResponse(**response) - return response + return _normalize_response(response)Also applies to: 182-186, 229-233
72-96: Consider using Pydantic'sFieldfor vector defaults.Using mutable dict literals as defaults (e.g.,
{"x": 0.0, "y": 0.0, "z": 0.0}) can sometimes cause issues. While Pydantic handles this correctly by copying defaults, usingField(default_factory=...)is the more explicit pattern.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (14)
40-58: Strengthen assertion to verify correct object is returned.The test only checks that data is present but doesn't verify that the returned object is actually the one created. Consider asserting on the instanceID or verifying the returned data contains the expected GameObject.
🔎 Example of stronger assertion
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); var data = resultObj["data"]; Assert.IsNotNull(data, "Should return data"); + // Verify it's the correct object + var instanceIds = data["instanceIDs"]?.ToObject<List<int>>(); + Assert.IsNotNull(instanceIds, "Should have instanceIDs"); + Assert.Contains(target.GetInstanceID(), instanceIds, "Should contain the target object's ID");
61-79: Verify the count of objects returned.When testing multiple objects with the same name, the test should verify that all three created objects are actually found in the results.
🔎 Proposed enhancement
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); + + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.AreEqual(3, instanceIds?.Count ?? 0, "Should find all 3 objects with duplicate name");
100-115: Clarify expected behavior for partial matching.The assertion
Assert.IsNotNull(result)is too weak and the comment indicates uncertainty about whether partial matching is supported. Based on the implementation in GameObjectLookup.cs (exact name matching), this test should verify that partial matches are NOT found.🔎 Proposed fix to verify exact-match behavior
var result = ManageGameObject.HandleCommand(p); - // Capture current behavior - may do partial match or exact only - Assert.IsNotNull(result, "Should return a result"); + var resultObj = result as JObject ?? JObject.FromObject(result); + + // Verify exact-match behavior: "Findable" should not match "FindableObject" + Assert.IsTrue(resultObj.Value<bool>("success"), "Should return success"); + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.That(instanceIds == null || instanceIds.Count == 0, + "Partial name 'Findable' should not match 'FindableObject' or 'FindableObjectTwo'");
122-138: Verify the correct object is returned by ID.Similar to the by-name test, this should verify that the returned instanceID matches the target object's ID.
🔎 Proposed enhancement
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); + + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.IsNotNull(instanceIds, "Should have instanceIDs"); + Assert.Contains(instanceID, instanceIds, "Should return the requested object by ID");
159-171: Strengthen assertion for non-numeric ID error case.The assertion
Assert.IsNotNull(result)doesn't verify the actual behavior. The test should check whether this returns an error or succeeds with empty results.🔎 Proposed enhancement
var result = ManageGameObject.HandleCommand(p); - // Should fail gracefully - Assert.IsNotNull(result, "Should return a result"); + var resultObj = result as JObject ?? JObject.FromObject(result); + + // Verify graceful handling: either error or empty results + bool success = resultObj.Value<bool>("success"); + if (success) + { + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.That(instanceIds == null || instanceIds.Count == 0, + "Non-numeric ID should return empty results"); + } + // If it fails, that's also acceptable behavior + Assert.IsNotNull(resultObj, "Should return a structured response");
178-195: Verify the correct nested object is found.Both path tests should verify that the returned object is actually the intended child/grandchild, not just that something was found.
🔎 Example for nested object verification
For the PathChild test:
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); + + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.Contains(child.GetInstanceID(), instanceIds, "Should find the nested child object");Also applies to: 198-218
262-281: Verify the count of tagged objects returned.The test creates two objects with the "Player" tag but doesn't verify both are found.
🔎 Proposed enhancement
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); + + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.GreaterOrEqual(instanceIds?.Count ?? 0, 2, + "Should find at least the 2 created objects with Player tag");
334-353: Verify the count of layered objects returned.Similar to other multi-object tests, verify that both created objects are found.
379-398: Verify the count of objects with the component.The test creates two objects with BoxCollider but doesn't verify both are found.
🔎 Proposed enhancement
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); + + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.GreaterOrEqual(instanceIds?.Count ?? 0, 2, + "Should find at least the 2 created objects with BoxCollider");
443-459: Strengthen assertion for inactive exclusion test.The assertion
Assert.IsNotNull(result)doesn't verify that inactive objects are actually excluded. The test should verify that the result is either empty or doesn't contain the inactive object's ID.🔎 Proposed enhancement
var result = ManageGameObject.HandleCommand(p); - // Capture current behavior - Assert.IsNotNull(result, "Should return a result"); + var resultObj = result as JObject ?? JObject.FromObject(result); + + Assert.IsTrue(resultObj.Value<bool>("success"), "Should return success"); + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.That(instanceIds == null || !instanceIds.Contains(target.GetInstanceID()), + "Should not find inactive object when searchInactive=false");
466-487: Consider verifying the correct grandchild object is found.The test should verify that the returned object is actually the grandchild that was searched for, not just that something was returned successfully.
Also, verify that the
searchInChildrenparameter name matches the actual implementation (similar concern as withsearchInactive).
494-521: Verify pagination fields in the response.The test creates a perfect pagination scenario (10 objects with pageSize=5) but doesn't verify any pagination behavior. According to the implementation in
FindGameObjects.cs, the response should includepageSize,cursor,nextCursor,totalCount, andhasMorefields.🔎 Proposed enhancement to verify pagination
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); - // Check for pagination info in response var data = resultObj["data"]; Assert.IsNotNull(data, "Should return data"); + + // Verify pagination fields + var instanceIds = data["instanceIDs"]?.ToObject<List<int>>(); + Assert.AreEqual(5, instanceIds?.Count ?? 0, "Page should contain exactly 5 objects"); + + Assert.AreEqual(5, data.Value<int>("pageSize"), "Should return pageSize"); + Assert.AreEqual(0, data.Value<int>("cursor"), "Should return cursor"); + Assert.AreEqual(5, data.Value<int>("nextCursor"), "nextCursor should be 5"); + Assert.GreaterOrEqual(data.Value<int>("totalCount"), 10, "totalCount should be at least 10"); + Assert.IsTrue(data.Value<bool>("hasMore"), "hasMore should be true when more results exist");
550-570: Replace string matching with proper JSON field verification.Using string contains to check for instanceID is fragile and could give false positives (e.g., matching "instance" in a comment or different field). Parse the JSON structure and verify the field exists properly.
🔎 Proposed fix for robust verification
var result = ManageGameObject.HandleCommand(p); var resultObj = result as JObject ?? JObject.FromObject(result); Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); - // Verify instanceID is present in response - var dataStr = resultObj["data"]?.ToString() ?? ""; - Assert.IsTrue(dataStr.Contains("instanceID") || dataStr.Contains("InstanceID") || dataStr.Contains("instance"), - "Response should include instance ID"); + // Verify instanceID field is present in data + var data = resultObj["data"]; + var instanceIds = data?["instanceIDs"]?.ToObject<List<int>>(); + Assert.IsNotNull(instanceIds, "Response should include instanceIDs array"); + Assert.Greater(instanceIds.Count, 0, "Should contain at least one instance ID"); + Assert.Contains(target.GetInstanceID(), instanceIds, "Should include the target object's instance ID");
9-12: Consider adding integration with the API redesign tracking.The summary mentions these tests "capture existing behavior before API redesign." Consider adding comments or markers that indicate which behaviors are expected to change during the redesign, so the tests can be updated accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.claude/prompts/nl-gameobject-suite.md.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.md.gitignoreMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Tools/ManageComponents.csServer/src/services/resources/gameobject.pyTestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs
✅ Files skipped from review due to trivial changes (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs.meta
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Helpers/ParamCoercion.cs
🧰 Additional context used
🧬 Code graph analysis (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (2)
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
List(82-86)List(96-135)GameObject(57-64)GameObject(69-72)MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-81)
Server/src/services/resources/gameobject.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (5)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
Stop(415-484)MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
FindGameObjects(1186-1209)MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)MCPForUnity/Editor/Tools/ManageComponents.cs (1)
AddComponent(68-126)TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (1)
Vector3(100-107)
🔇 Additional comments (21)
.gitignore (1)
45-53: Appropriate additions aligned with new CI/testing infrastructure.The three new ignore entries are well-organized, clearly commented, and appropriately scoped to avoid unintended exclusions:
reports/for generated CI test artifacts.claude/local/for local configurationscripts/local-test/for the local testing harnessThese entries support the new GameObject API tooling and testing workflows introduced in this PR.
.claude/prompts/nl-unity-suite-nl.md (2)
6-6: Tool identifier renaming applied consistently.All UnityMCP-prefixed tool names are used consistently throughout the prompt. Cross-verified against the AllowedTools list:
validate_script,find_in_file,read_resource,apply_text_edits,script_apply_edits,get_sha, andread_consoleare all correctly referenced in test steps, tool mapping, and state verification patterns. Naming updates align with the broader UnityMCP namespace redesign.Also applies to: 14-14, 52-53, 62-62, 65-65, 68-68, 70-72, 86-86
15-15: Clear output paths and CI contract.The distinction between individual test fragment files (
reports/<TESTID>_results.xml) and the pre-created JUnit aggregator ($JUNIT_OUT) is clearly communicated. The instruction not to read/edit$JUNIT_OUTis explicit, and the hardcodedproject_rootand environment variable references are appropriate for a CI-scoped prompt.Also applies to: 34-34, 44-46
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (3)
26-28: Good practice: precomputing animator parameter hashes.Using static readonly hash fields for animator parameters is a Unity best practice that improves performance by avoiding repeated string-to-hash conversions at runtime.
762-774: Test scaffolding for NL editing validation.The added local functions and test markers within
Pad0240()serve as test scaffolding for validating natural language editing capabilities. While the functions are never called, they appear intentionally placed to verify the tooling can handle local functions and test markers correctly.
109-114: The animator parameters are correctly named and match the hash field definitions. Lines 27-28 showBlendXHash = Animator.StringToHash("BlendX")andBlendYHash = Animator.StringToHash("BlendY"), which theApplyBlendmethod uses correctly. The code is safe and requires no changes.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (6)
198-233: Good isolation pattern using HashSet to track created objects.This test correctly handles the issue of searching by Transform (which matches all GameObjects) by tracking only the created objects in a
HashSetand counting how many of those appear in the results. This is a good pattern that mitigates test isolation concerns.
239-273: Component addition test covers good variety.The test correctly uses the
componentTypeparameter and verifies all components are added by checking the final count. Good coverage of multiple component types.
312-342: Excellent property verification test.This test thoroughly verifies that all Rigidbody properties are correctly set and persisted. The explicit assertions on each property value provide clear failure points.
348-379: Good test coverage for deep hierarchy path finding.This test effectively validates that objects can be found by their full hierarchical path, even when nested several levels deep. The explicit verification of the instance ID confirms the correct object was found.
415-461: Comprehensive test of GameObject serialization.This test thoroughly validates that all GameObject properties (tag, layer, isStatic, components, children) are correctly serialized and retrievable. Good use of explicit assertions for each expected property.
463-514: Thorough serialization verification with property inspection.The test not only verifies component count but also dives into the serialized properties structure to confirm the Rigidbody mass value is correctly serialized. This level of verification helps catch serialization bugs early.
MCPForUnity/Editor/Tools/ManageComponents.cs (2)
27-64: Well-structured command handler with comprehensive error handling.The switch expression pattern for action routing is clean, and the try-catch at the top level ensures errors don't propagate unexpectedly. Good use of
ParamCoercionfor flexible parameter handling with both camelCase and snake_case support.
92-126: Proper Undo support for component operations.Good use of
Undo.RecordObjectandUndo.AddComponentfor editor undo support, andEditorUtility.SetDirtyto mark the object for saving.MCPForUnity/Editor/Helpers/VectorParsing.cs (2)
1-51: Clean, well-documented utility class.Good separation of concerns for JSON-to-Unity type conversion. The null-safe pattern with try-catch and warnings is consistent throughout.
99-169: Quaternion parsing note about normalization is helpful.The docstring correctly notes that raw quaternion components are NOT normalized. This is important context for callers who may need normalized quaternions for interpolation.
.claude/prompts/nl-unity-suite-t.md (1)
5-5: Consistent namespace update to UnityMCP prefix.The AllowedTools line and all tool references throughout the file have been updated to use the
mcp__UnityMCP__prefix consistently.Server/src/services/resources/gameobject.py (1)
24-69: Comprehensive API documentation resource.The
get_gameobject_api_docsendpoint provides excellent self-documenting API with examples, parameter descriptions, and related tools. This is a good pattern for discoverability.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (3)
1-35: LGTM! Clean test setup and teardown.The test infrastructure is well-designed with proper cleanup using
DestroyImmediate(appropriate for editor tests) and a helper method that automatically tracks created objects for disposal.
284-308: Good use of try-finally for LogAssert flag.The try-finally pattern ensures
ignoreFailingMessagesis reset even if the test throws an exception. This prevents test pollution.
433-433: Parameter name is correct — no issue found.The tests correctly use
searchInactive, which matches the parameter name expected byManageGameObject.HandleCommand(verified at line 1627 of ManageGameObject.cs). The implementation extractsfindParams?["searchInactive"]?.ToObject<bool>(), confirming the test parameter names are accurate. TheincludeInactivenaming that appeared in the original analysis refers only to a local variable in theGetAllSceneObjectshelper method, not the incoming parameter.
- ManageComponents: Reuse GameObjectLookup.FindComponentType instead of duplicate - ManageComponents: Log warnings when SetPropertiesOnComponent fails - GameObjectLookup: Make FindComponentType public for reuse - gameobject.py: Extract _normalize_response helper to reduce duplication - gameobject.py: Add TODO comment for unused typed response classes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
148-156: SearchByPath limitation with includeInactive remains unresolved.As noted in the previous review,
GameObject.Find(path)only finds active GameObjects. TheincludeInactiveparameter has no effect for path-based searches due to Unity API limitations. This creates an API inconsistency where the parameter is accepted but ignored.The previous review suggested either documenting the limitation or implementing a manual traversal for inactive objects. This concern still applies.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ManageComponents.cs (1)
349-363: Case-insensitive reflection improves usability but consider documenting the behavior.Using
BindingFlags.IgnoreCasemakes the API more forgiving for property names, which is helpful for external callers. However, this could theoretically cause ambiguity if a component has properties differing only in case (rare but possible in user code).Consider adding a brief XML comment noting the case-insensitive behavior
private static string TrySetProperty(Component component, string propertyName, JToken value) { + // Note: Property/field lookup is case-insensitive for better usability if (component == null || string.IsNullOrEmpty(propertyName)) return $"Invalid component or property name";Also applies to: 366-380
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Tools/ManageComponents.csServer/src/services/resources/gameobject.py
🧰 Additional context used
🧬 Code graph analysis (1)
Server/src/services/resources/gameobject.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
🔇 Additional comments (13)
MCPForUnity/Editor/Tools/ManageComponents.cs (4)
315-320: Past duplication concern has been resolved.The previous review flagged this method for duplicating logic from
GameObjectLookup.FindComponentType. The current implementation now properly delegates to the shared utility, eliminating the duplication.
27-64: Well-structured command dispatcher with comprehensive validation.The method properly validates required parameters, normalizes action names, and handles errors gracefully. The switch expression provides clean action routing.
149-153: Excellent safety check preventing Transform removal.Checking early (before
GetComponent) is efficient and prevents a common mistake that would break GameObjects. The error message is clear.
386-411: Unity type conversion covers common cases with reasonable fallback.The method handles the most frequently used Unity types (Vector3, Vector2, Quaternion, Color) with special parsing, then falls back to Newtonsoft for other types. While additional Unity types like
Vector4,Rect, orBoundsaren't explicitly handled, the Newtonsoft fallback should handle most cases reasonably.Server/src/services/resources/gameobject.py (4)
20-24: Clean response normalization without redundant checks.The past review comment mentioned a redundant success check, but the current implementation is streamlined and correct. The function simply wraps dict responses in
MCPResponseand passes through other types.
31-76: Excellent API documentation resource for discoverability.Providing a static documentation resource that explains the workflow (search → get instance ID → access detailed resources) is very helpful for API consumers. The examples and parameter descriptions are clear.
118-134: Consistent validation and error handling pattern.All three resource endpoints follow the same pattern: validate instance_id, delegate to Unity transport, normalize response. This consistency makes the code predictable and maintainable.
160-187: Pagination support with sensible defaults.The components endpoint properly supports pagination with reasonable defaults (page_size=25, cursor=0) and an include_properties flag for controlling response size.
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (5)
33-48: Robust search method parsing with sensible default.The method normalizes various input formats and defaults to
ByNamefor unrecognized values, which provides a good fallback behavior rather than throwing exceptions.
158-188: Excellent handling of includeInactive for tag-based search.Unlike the path-based search, this method properly honors the
includeInactiveparameter by falling back to manual iteration when needed, sinceFindGameObjectsWithTagonly finds active objects.
190-212: Flexible layer search supporting both names and numbers.The method tries to resolve the layer by name first, then falls back to parsing as an integer. The validation for layer range (0-31) prevents invalid layer numbers.
274-311: Comprehensive type resolution with multiple fallback strategies.The method tries:
- Direct type lookup
- UnityEngine namespace prefix
- Search across all loaded assemblies with both exact and prefixed names
This thorough approach maximizes the chances of finding the correct type while safely handling assembly search failures.
313-331: Clean hierarchical path construction.The method correctly traverses up the parent chain to build the full path. The null checks prevent errors with root objects.
NL/T Prompt Fixes: - nl-gameobject-suite.md: Remove non-existent list_resources/read_resource from AllowedTools - nl-gameobject-suite.md: Fix parameter names (component_type, properties) - nl-unity-suite-nl.md: Remove unused manage_editor from AllowedTools Test Fixes: - GameObjectAPIStressTests: Add null check to ToJObject helper - GameObjectAPIStressTests: Clarify AudioSource usage comment - ManageGameObjectFindTests: Use built-in 'UI' layer instead of 'Water' - LongUnityScriptClaudeTest: Clean up NL/T test artifacts (Counte42 typo, HasTarget)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (2)
150-172: Test isolation concern remains: Transform search includes all scene objects.This issue was previously flagged. Searching for the
Transformcomponent (line 152) matches every GameObject in the scene, making the assertion at line 165 dependent on scene state. While you've added a descriptive comment at line 147, the test is still fragile if the scene contains other objects.Consider using a unique tag or custom component that only your test objects have, ensuring the test validates only the objects it creates.
240-274: Inconsistent AudioSource usage: missing justification.This was previously flagged. Line 249 adds
AudioSourceto the test without explanation, but line 281's comment states AudioSource is skipped due to deprecated API warnings. While line 424-427 includes a justifying comment for its AudioSource usage, line 249 does not.For consistency, either:
- Remove
"AudioSource"from thecomponentTypeNamesarray at line 249- Add a comment similar to line 424 explaining why it's acceptable in this specific context
🔎 Proposed fix
string[] componentTypeNames = new[] { "BoxCollider", "Rigidbody", - "AudioSource", "Light", "Camera" };
🧹 Nitpick comments (6)
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (1)
760-772: Test scaffolding with unused local functions.The test markers and local functions (
TestHelper,IncrementCounter) are clearly part of the test infrastructure mentioned in the PR objectives. However, note that these local functions are declared but never invoked withinPad0240.If these are intentional placeholders for future test modifications, this is acceptable. If they're meant to be called, consider adding invocations or removing them to keep the test code clean.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (5)
13-35: Excellent test setup and cleanup pattern.The
TearDownmethod properly cleans up test GameObjects, and theCreateTestObjecthelper encapsulates object creation and tracking. The null check in the cleanup loop prevents potential errors.Optional: Add [TestFixture] attribute for clarity
While NUnit 3+ doesn't strictly require it, adding
[TestFixture]makes the test class more explicit:+ [TestFixture] public class ManageGameObjectFindTests
37-117: Well-structured baseline tests for find-by-name functionality.These tests appropriately document existing behavior, including the current pattern of returning success even when no objects are found (line 96). The coverage includes single/multiple objects, not found scenarios, and partial matching.
Optional: Strengthen assertion in partial match test
Line 114 could verify more than just non-null result:
var result = ManageGameObject.HandleCommand(p); - // Capture current behavior - may do partial match or exact only - Assert.IsNotNull(result, "Should return a result"); + var resultObj = result as JObject ?? JObject.FromObject(result); + Assert.IsTrue(resultObj.Value<bool>("success"), "Should return success"); + // Document whether partial matching is supported or exact-only
119-173: Good coverage of ID-based search scenarios.The tests properly use
GetInstanceID()and cover valid IDs, invalid numeric IDs, and non-numeric input. The documentation of current behavior (returning success even for invalid IDs) is valuable for baseline comparison.Optional: Clarify expected behavior for non-numeric ID
Line 170 could document whether this should succeed or fail:
var result = ManageGameObject.HandleCommand(p); - // Should fail gracefully - Assert.IsNotNull(result, "Should return a result"); + var resultObj = result as JObject ?? JObject.FromObject(result); + // Document: current behavior returns success/failure for parse errors + Assert.IsNotNull(resultObj, "Should return a result object");
423-464: Inactive object handling is properly tested.Both inclusion and exclusion of inactive objects are covered. The tests correctly use
SetActive(false)to create inactive test objects.Optional: Document expected behavior when excluding inactive objects
Line 461 could clarify whether the search should succeed or return empty results:
var result = ManageGameObject.HandleCommand(p); - // Capture current behavior - Assert.IsNotNull(result, "Should return a result"); + var resultObj = result as JObject ?? JObject.FromObject(result); + Assert.IsTrue(resultObj.Value<bool>("success"), "Should return success"); + // Document: inactive object should not be found when searchInactive=false
528-575: Response structure tests verify key data elements.The tests appropriately verify that data is returned and includes instance ID information. These validate the response structure for downstream consumers.
Optional: Make instanceID check more robust
Lines 570-572 use a fragile string-based check that could match unrelated fields. Consider checking the JObject structure directly:
- // Verify instanceID is present in response - var dataStr = resultObj["data"]?.ToString() ?? ""; - Assert.IsTrue(dataStr.Contains("instanceID") || dataStr.Contains("InstanceID") || dataStr.Contains("instance"), - "Response should include instance ID"); + // Verify instanceID is present in response + var data = resultObj["data"] as JObject; + Assert.IsNotNull(data, "Should return data object"); + var hasInstanceId = data["instanceID"] != null || data["InstanceID"] != null || data["id"] != null; + Assert.IsTrue(hasInstanceId, "Response should include instance ID field");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.claude/prompts/nl-gameobject-suite.md.claude/prompts/nl-unity-suite-nl.mdTestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (6)
MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (18)
GameObject(30-35)TearDown(17-28)Test(39-58)Test(60-79)Test(81-97)Test(99-115)Test(121-138)Test(140-156)Test(158-171)Test(177-195)Test(197-218)Test(220-236)Test(242-259)Test(261-281)Test(283-308)Test(314-331)Test(333-356)Test(362-379)MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (1)
Stop(415-484)MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
FindGameObjects(1186-1209)MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)MCPForUnity/Editor/Tools/ManageComponents.cs (1)
AddComponent(68-126)
🔇 Additional comments (14)
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (2)
23-26: Good optimization: Precomputed animator parameter hashes.The static readonly hash fields follow Unity best practices by avoiding repeated string-to-hash conversions at runtime. This is the recommended pattern for animator parameter access.
107-112: Correct implementation of hash-based animator parameter updates.The method correctly uses the precomputed hash values instead of string parameter names, following the optimization pattern established in lines 25-26. The null check is properly preserved.
.claude/prompts/nl-gameobject-suite.md (1)
54-54: read_resource is invoked as a tool but should use MCP resource protocol instead.Lines 54, 62, and 97 invoke
mcp__UnityMCP__read_resource()as a callable tool, butread_resourceis not listed in AllowedTools (line 6). More importantly, the "New Resources" section (lines 134–138) documents these as MCP resource endpoints (unity://scene/gameobject/{instanceID}, etc.), not as tools.In MCP protocol, resources are accessed via resource URIs, not tool invocation. The tests should either:
- Use the MCP resource protocol directly to read these resources, or
- If a tool wrapper is required, add
read_resourceto AllowedTools and document it in the Tool Reference section (lines 131–132).TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (5)
54-58: LGTM! Null check properly implemented.The helper method now correctly handles null results as suggested in the previous review.
62-117: Performance thresholds are appropriate for CI stress testing.The generous timing thresholds (5s for 10 objects, 15s for 50 objects) are well-suited for stress tests that need to account for CI environment variability. The tests properly track created objects for cleanup and validate success at each step.
175-234: Well-designed pagination verification test.This test cleverly creates objects, tracks their IDs, and iterates through all pages to verify that all created objects are found. The safety limit at line 229 prevents infinite loops, and the final assertion confirms all objects were discovered across pages.
349-380: Deep hierarchy test validates path search effectively.The test constructs a 7-level hierarchy and successfully searches by the full path, confirming that the FindGameObjects path search works correctly for deeply nested structures.
521-563: Good coverage of rapid create-modify-delete cycles.This test effectively simulates high-frequency operations and includes appropriate workarounds (name modification instead of layer at line 545) with explanatory comments. The performance threshold accounts for stress test variability.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (6)
175-238: Path-based search tests are well-structured.The tests properly construct hierarchies using
SetParentand verify both simple nesting and deep hierarchies. The path format follows Unity conventions, and the invalid path test documents baseline behavior.
240-310: Tag-based search tests use appropriate built-in tags.Excellent use of built-in Unity tags ("MainCamera", "Player") that exist in all projects. The proper use of
LogAssert.ignoreFailingMessagesin a try-finally block (lines 288-307) cleanly handles expected warnings from invalid tags.
333-356: Past review feedback addressed: now using built-in "UI" layer.The test correctly uses the built-in "UI" layer (lines 340-342) with an explicit comment documenting the choice. This resolves the previous concern about the "Water" layer not being available in all projects.
360-421: Component search tests use reliable built-in components.The tests appropriately use
RigidbodyandBoxCollider, which are available in all Unity projects. Good coverage of single, multiple, and invalid component scenarios.
466-492: Search-in-children test validates deep hierarchy traversal.The test properly constructs a three-level hierarchy and verifies that grandchildren can be found when
searchInChildrenis enabled. Good validation of recursive search behavior.
494-526: Pagination test uses appropriate setup and built-in tags.Creating 10 test objects with the built-in "Respawn" tag is a good approach for testing pagination. The test verifies that paginated results are returned with appropriate pageSize and cursor parameters.
| ### GO-1. Find GameObjects Tool | ||
| **Goal**: Test the new find_gameobjects tool | ||
| **Actions**: | ||
| - Call `mcp__UnityMCP__find_gameobjects(search_term="Camera", search_method="by_component")` | ||
| - Verify response contains `instanceIDs` array in `data` | ||
| - Verify response contains pagination info (`pageSize`, `cursor`, `totalCount`) | ||
| - **Pass criteria**: Returns at least one instance ID |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "nl-gameobject-suite.md" -type fRepository: dsarno/unity-mcp
Length of output: 100
🏁 Script executed:
cat -n ./.claude/prompts/nl-gameobject-suite.md | head -100Repository: dsarno/unity-mcp
Length of output: 5676
🏁 Script executed:
cat -n ./.claude/prompts/nl-gameobject-suite.md | sed -n '100,150p'Repository: dsarno/unity-mcp
Length of output: 2599
Resolve GO-1 and GO-0 Camera dependency: add test scene setup or use fallback search.
GO-1 and GO-0 both assume a Camera component exists in the test scene. If no Camera is found, GO-1 will fail and cascade to GO-2 and GO-3 (which depend on GO-1's instanceID). Consider one of:
- Document scene requirement: Add a setup note stating the test scene must have at least one Camera component.
- Use guaranteed search in GO-1: Change
search_term="Camera"to a more specific term (e.g.,search_term="Main Camera", search_method="by_name") or a guaranteed component present in a default scene. - Add scene setup step: Insert a GO step before GO-0 to create a Camera if one doesn't exist.
This will eliminate brittle environment assumptions and prevent cascading test failures.
- GameObjectLookup.SearchByPath: Document and warn that includeInactive has no effect (Unity API limitation) - ManageComponents.TrySetProperty: Document case-insensitive lookup behavior
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @.github/workflows/claude-nl-suite.yml:
- Around line 782-822: Add a post-retry assertion step after the "Retry GO pass
(Sonnet)" step named something like "Check GO coverage incomplete (post-retry)"
with id go_cov_post that runs with shell: bash and if: always(); reuse the same
script from the pre-retry "Check GO coverage incomplete (pre-retry)" step (set
-euo pipefail, build missing array by checking reports/${ID}_results.xml and
reports/_staging/${ID}_results.xml for GO-0..GO-10, echo missing and list to
$GITHUB_OUTPUT) and if any missing remain exit non-zero (e.g., echo an error and
exit 1) so the workflow fails fast when the retry didn’t produce all GO
fragments.
In @Server/src/services/tools/manage_material.py:
- Around line 105-106: The manage_material function is missing the preflight
gate; update the beginning of manage_material to call preflight(...) (same
signature used by manage_asset/manage_gameobject/manage_components) before
proceeding (i.e., before executing any Unity actions or before
get_unity_instance_from_context(ctx)) so Unity compile/reload is waited for;
reference the manage_material function and the preflight() helper when making
the change.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/claude-nl-suite.yml (1)
1230-1238: Fix sorting for GO tests to handle numeric order correctly.The
order_keyfunction doesn't handle the GO- prefix, so GO tests will be sorted alphabetically as strings (GO-10 before GO-2). Add a case for GO tests to ensure GO-10 sorts after GO-9.🔎 Proposed fix
def order_key(n: str): if n.startswith('NL-'): try: return (0, int(n.split('-')[1])) except: return (0, 999) if n.startswith('T-') and len(n) > 2: return (1, ord(n[2])) + if n.startswith('GO-'): + try: + return (2, int(n.split('-')[1])) + except: + return (2, 999) - return (2, n) + return (3, n)Server/src/services/tools/manage_gameobject.py (1)
269-276: Potential issue:prefab_foldermay beNone.At line 274,
prefab_folderis used to construct the prefab path, butprefab_folderhas no default value in the function signature (line 112-113). IfsaveAsPrefab=Trueis passed withoutprefab_pathorprefab_folder, this will create a path likeNone/ObjectName.prefab.#!/bin/bash # Check if prefab_folder has a default value elsewhere or if there's upstream handling rg -n "prefab_folder" --type py -A2 -B2 Server/src/services/tools/manage_gameobject.py
🧹 Nitpick comments (11)
.github/workflows/claude-nl-suite.yml (1)
754-781: Consider timeout alignment with test count.The GO pass has a 20-minute timeout for 11 tests, while the T pass (lines 668-694) has a 30-minute timeout for 10 tests. If GO tests have similar complexity to T tests, consider increasing the timeout to 30 minutes or more to allow adequate execution time.
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (2)
266-280: Consider iterative traversal for very deep hierarchies.The recursive
GetObjectAndDescendantsworks well for typical scene hierarchies. For extremely deep hierarchies (hundreds of levels), an iterative approach with an explicit stack would prevent potential stack overflow, though this is uncommon in practice.
324-339: Consider using StringBuilder for deep hierarchies.The current string concatenation works, but for deeply nested objects, a
StringBuilderwould be more efficient. This is a minor optimization concern.🔎 Optional optimization
public static string GetGameObjectPath(GameObject obj) { if (obj == null) return string.Empty; - var path = obj.name; - var parent = obj.transform.parent; - - while (parent != null) - { - path = parent.name + "/" + path; - parent = parent.parent; - } - - return path; + var parts = new List<string>(); + var current = obj.transform; + while (current != null) + { + parts.Add(current.name); + current = current.parent; + } + parts.Reverse(); + return string.Join("/", parts); }MCPForUnity/Editor/Tools/ManageComponents.cs (1)
390-415: Consider adding support for Rect and Bounds types.The
VectorParsingutility also supportsParseRectandParseBounds(as mentioned in the relevant code snippets), but these aren't handled inConvertValue. If these types are used on components, they'll fall through totoken.ToObject()which may not parse them correctly from the same JSON format.🔎 Optional: Add Rect and Bounds handling
if (targetType == typeof(Color)) { return VectorParsing.ParseColor(token) ?? Color.white; } +if (targetType == typeof(Rect)) +{ + return VectorParsing.ParseRect(token) ?? Rect.zero; +} +if (targetType == typeof(Bounds)) +{ + return VectorParsing.ParseBounds(token) ?? new Bounds(); +} // Use Newtonsoft for other types return token.ToObject(targetType);TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (1)
521-563: Consider tracking objects for cleanup in case of partial failure.If the delete operation fails mid-loop, the already-created objects won't be cleaned up since they're not added to
_createdObjects. While the test asserts success, a test framework failure or exception could leave orphaned objects.🔎 Optional: Track for cleanup
// Create var createResult = ToJObject(ManageGameObject.HandleCommand(new JObject { ["action"] = "create", ["name"] = $"RapidFire_{i}" })); Assert.IsTrue(createResult["success"]?.Value<bool>() ?? false, $"Create failed: {createResult["message"]}"); int instanceId = createResult["data"]?["instanceID"]?.Value<int>() ?? 0; Assert.AreNotEqual(0, instanceId, "Instance ID should not be 0"); + +// Track for cleanup in case later operations fail +var go = EditorUtility.InstanceIDToObject(instanceId) as GameObject; +if (go != null) _createdObjects.Add(go);Server/src/services/tools/manage_asset.py (2)
23-24: Inconsistent return value forNoneinput.This helper returns
{}(empty dict) whenvalue is None, but the equivalent_normalize_propertiesinmanage_material.py(line 52-53) andmanage_components.py(line 21-22) returnNone, None. This inconsistency could lead to subtle behavioral differences—an empty dict{}will be included in the payload (since it's truthy), whereasNonewould be filtered out by the{k: v for k, v in params_dict.items() if v is not None}pattern at line 141.If the intent is to always include
propertiesin the payload (even when empty), this is fine but should be documented. Otherwise, consider aligning with the other helpers.🔎 Proposed fix to align with other helpers
if value is None: - return {}, None + return None, None
41-48: Python dict literal fallback is unique to this file.The
ast.literal_evalfallback for Python dict literals (e.g.,{'key': 'value'}) is not present in the other_normalize_propertiesimplementations (manage_material.py,manage_components.py). This creates an inconsistency wheremanage_assetaccepts Python syntax but other tools don't.Consider either:
- Removing this fallback for consistency, or
- Extracting a shared
_normalize_propertiesutility toservices/tools/utils.pythat all tools can useGiven the
astimport is only used here, option 1 keeps things simpler unless there's a specific requirement for Python literal support in asset management.Server/src/services/tools/manage_gameobject.py (2)
64-66: Comment doesn't match implementation.The comment says "validate structure" but the code only checks
isinstance(value, dict)without validating that values are themselves dicts (as suggested by the type hintdict[str, dict[str, Any]]). Either update the comment to say "return as-is" or add shallow structure validation if strict typing is intended.🔎 Option: Update comment to match behavior
- # Already a dict - validate structure + # Already a dict - return as-is (Unity validates structure) if isinstance(value, dict): return value, None
196-199: Consider logging error viactx.errorfor consistency.
manage_asset.pylogs parse errors viaawait ctx.error(...)before returning the failure payload (line 95). For consistency, consider adding similar logging here.🔎 Proposed addition
component_properties, comp_props_error = _normalize_component_properties(component_properties) if comp_props_error: + await ctx.error(f"manage_gameobject: {comp_props_error}") return {"success": False, "message": comp_props_error}Server/src/services/tools/manage_components.py (1)
155-156: Consider narrowing exception handling.Static analysis flags the broad
Exceptioncatch. While this provides a graceful fallback, it could mask programming errors. Consider catching more specific exceptions (e.g.,TypeError,ValueError,ConnectionError) or at minimum logging the exception type for debugging.🔎 Option: Log exception type for debugging
except Exception as e: - return {"success": False, "message": f"Error managing component: {e!s}"} + return {"success": False, "message": f"Error managing component ({type(e).__name__}): {e!s}"}Server/src/services/tools/manage_material.py (1)
4-4: Remove unusedjsonimport.The
jsonmodule imported at line 4 is not directly used in this file—all JSON parsing is delegated toparse_json_payloadfrom utils. Remove the import.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.claude/prompts/nl-gameobject-suite.md.claude/prompts/nl-unity-suite-nl.md.github/workflows/claude-nl-suite.ymlMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Tools/ManageComponents.csServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude/prompts/nl-gameobject-suite.md
- .claude/prompts/nl-unity-suite-nl.md
🧰 Additional context used
🧬 Code graph analysis (5)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (4)
MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)MCPForUnity/Editor/Tools/ManageComponents.cs (1)
AddComponent(68-126)MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
FindGameObjects(1186-1209)MCPForUnity/Editor/Tools/BatchExecute.cs (1)
JArray(153-161)
Server/src/services/tools/manage_asset.py (2)
Server/src/services/tools/manage_components.py (1)
_normalize_properties(16-40)Server/src/services/tools/manage_material.py (1)
_normalize_properties(48-67)
Server/src/services/tools/manage_components.py (5)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/utils.py (1)
parse_json_payload(27-60)Server/src/services/tools/preflight.py (1)
preflight(27-105)Server/src/services/tools/manage_material.py (1)
_normalize_properties(48-67)
MCPForUnity/Editor/Tools/ManageComponents.cs (6)
MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-81)MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs (3)
HandleCommand(20-64)HandleCommand(130-205)HandleCommand(216-282)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-158)CoerceString(121-128)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
GameObject(57-64)GameObject(69-72)Type(285-319)GameObjectLookup(15-340)MCPForUnity/Editor/Resources/Tests/GetTests.cs (1)
TryParse(79-104)MCPForUnity/Editor/Helpers/VectorParsing.cs (6)
Vector3(18-51)Vector3(56-59)VectorParsing(11-294)Vector2(66-97)Quaternion(108-169)Color(177-225)
Server/src/services/tools/manage_gameobject.py (1)
Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)coerce_int(63-77)
🪛 Ruff (0.14.10)
Server/src/services/tools/manage_components.py
155-155: Do not catch blind exception: Exception
(BLE001)
⏰ 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: nl-suite
🔇 Additional comments (35)
.github/workflows/claude-nl-suite.yml (5)
868-878: LGTM: GO canonicalization patterns integrated correctly.The GO test patterns follow the same structure as existing NL/T patterns and provide appropriate name matching rules for each GO test.
905-907: LGTM: GO ID parsing integrated consistently.All regex patterns for parsing and matching GO test IDs are consistent with the existing NL/T patterns and correctly handle the GO-\d+ format throughout the workflow.
Also applies to: 923-923, 1052-1055, 1060-1060, 1097-1097, 1103-1103
946-946: LGTM: GO tests properly integrated into backfill.The GO tests are correctly added to the DESIRED list and the filename parsing logic properly handles the GO-\d+ format.
Also applies to: 957-959
1172-1172: LGTM: GO tests integrated into markdown summary with appropriate titles.The GO tests are properly added to the summary with descriptive default titles.
Also applies to: 1189-1199
1292-1292: LGTM: GO tests properly integrated into CI gate.The GO tests are correctly included in the CI gate checks, ensuring all GO tests must be present and passing for the workflow to succeed. The regex patterns for ID extraction are consistent with existing NL/T handling.
Also applies to: 1309-1309, 1314-1314
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (9)
1-28: LGTM!The imports, namespace structure, class declaration, and
SearchMethodenum are well-organized. The enum provides a clear, extensible set of search options.
33-48: LGTM!Defensive handling with a sensible default. The case-insensitive matching via
ToLowerInvariant()is appropriate for API flexibility.
57-64: LGTM!Clean delegation pattern. The method correctly limits results to 1 for single-object lookup and properly handles null returns.
96-135: LGTM!The switch expression properly routes to specialized search methods. The
ByIdcase correctly respects theincludeInactiveflag by checkingactiveInHierarchy.
148-164: Previous concern addressed.The limitation of
GameObject.Find()not supporting inactive objects is now properly documented with a runtime warning whenincludeInactive=true, guiding users to useby_namesearch as an alternative.
166-196: LGTM!Proper handling of both active-only and include-inactive scenarios. The try-catch correctly handles invalid tag names that would throw
UnityExceptionfrom bothFindGameObjectsWithTagandCompareTag.
198-220: LGTM!Flexible input handling supporting both layer names and numeric layer indices, with proper bounds validation (0-31).
222-245: LGTM!Proper component type resolution with user-friendly warning on failure. The iteration correctly respects
maxResultslimits.
285-319: LGTM!Good fallback search strategy: direct type lookup → UnityEngine namespace → all loaded assemblies. The empty catch block is acceptable here since some assemblies may not be searchable due to various runtime constraints.
MCPForUnity/Editor/Tools/ManageComponents.cs (7)
1-64: LGTM!The
HandleCommandentry point has proper null checks, parameter extraction with fallback for both camelCase and snake_case parameter names, and clean routing to action handlers via switch expression.
92-126: LGTM!Proper use of Unity's Undo system for editor operations. The structured response with
instanceID,componentType, andcomponentInstanceIDprovides good feedback for callers.
149-180: LGTM!Good safety check preventing Transform removal. The use of
Undo.DestroyObjectImmediateis appropriate for editor component removal with undo support.
221-264: Good partial failure handling.The approach of collecting all errors rather than failing fast allows callers to see all property-setting failures at once, which is user-friendly for bulk operations. The response correctly differentiates between full success and partial failure.
286-310: LGTM!The target resolution logic properly handles both numeric instance IDs and string-based searches. Always including inactive objects in the fallback search is appropriate since component operations commonly need to work on inactive GameObjects.
315-320: Previous duplication concern resolved.The method now properly delegates to
GameObjectLookup.FindComponentType, eliminating code duplication as suggested in the previous review.
345-388: LGTM!The case-insensitive reflection lookup is a good usability choice for external callers. The fallback from property to field covers both common Unity component patterns.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (7)
23-52: LGTM!Good test infrastructure: proper setup/teardown for test isolation, helper methods for object creation with automatic cleanup tracking, and the null-safe
ToJObjecthelper (addressing previous review feedback).
62-118: LGTM!Well-structured bulk creation tests with reasonable timeout assertions. The cleanup tracking ensures test isolation.
123-235: Previous test isolation concern resolved.Excellent use of the
GameObjectAPIStressTestMarkercomponent to isolate test searches from other scene objects. The pagination tests are thorough with proper boundary checking and a safety limit to prevent infinite loops.
276-311: Previous AudioSource inconsistency addressed.The comment at line 281 clearly explains why AudioSource is skipped in this test. The test uses AudioListener as a valid alternative without deprecated API warnings.
349-410: LGTM!Good coverage of hierarchy operations. The deep hierarchy test validates path-based search through multiple levels. The pagination test verifies response structure includes
componentTypes.
416-515: Previous AudioSource concern addressed with clear comments.The comments at lines 424 and 477 clearly explain when AudioSource is acceptable (reading component types only) versus when it's skipped (full property serialization). This provides good documentation for future maintainers.
568-572: LGTM!Clean marker component pattern for test isolation. Making it
sealedis appropriate since it's not intended for inheritance.Server/src/services/tools/manage_asset.py (1)
92-96: LGTM!Good error handling pattern—logging the error via
ctx.errorbefore returning the failure payload provides useful debugging context.Server/src/services/tools/manage_gameobject.py (1)
14-53: LGTM!The
_normalize_vectorhelper is well-designed:
- Handles multiple input formats (list/tuple, JSON string, comma/space-delimited strings)
- Uses
math.isfiniteto reject NaN/Infinity values- Silent failure returning
defaultis appropriate for optional transform parametersServer/src/services/tools/manage_components.py (3)
16-40: LGTM!The
_normalize_propertieshelper is consistent with the implementation inmanage_material.pyand provides clear error messages for invalid inputs.
130-138: Clarify whether both single and multi-property updates can be combined.For
set_property, the code allows settingproperty/value(lines 131-133) ANDpropertiesdict (lines 134-135) simultaneously. Both are added to params if present. Is this intentional behavior (allowing both to be combined), or should it be mutually exclusive? The docstring examples show them as separate use cases.
43-91: LGTM!The tool follows established patterns: preflight checks, input normalization, validation, and graceful error handling. The docstring provides clear examples for each action.
Server/src/services/tools/manage_material.py (2)
15-45: LGTM!The
_normalize_colorhelper properly handles both RGB[r, g, b]and RGBA[r, g, b, a]formats with clear error messages for invalid inputs.
108-124: LGTM!Clean normalization flow with early returns on validation errors. The pattern is consistent with the other tools.
| - name: Check GO coverage incomplete (pre-retry) | ||
| id: go_cov | ||
| if: always() | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| missing=() | ||
| for id in GO-0 GO-1 GO-2 GO-3 GO-4 GO-5 GO-6 GO-7 GO-8 GO-9 GO-10; do | ||
| if [[ ! -s "reports/${id}_results.xml" && ! -s "reports/_staging/${id}_results.xml" ]]; then | ||
| missing+=("$id") | ||
| fi | ||
| done | ||
| echo "missing=${#missing[@]}" >> "$GITHUB_OUTPUT" | ||
| if (( ${#missing[@]} )); then | ||
| echo "list=${missing[*]}" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| - name: Retry GO pass (Sonnet) if incomplete | ||
| if: steps.go_cov.outputs.missing != '0' | ||
| uses: anthropics/claude-code-base-action@beta | ||
| with: | ||
| use_node_cache: false | ||
| prompt_file: .claude/prompts/nl-gameobject-suite.md | ||
| mcp_config: .claude/mcp.json | ||
| settings: .claude/settings.json | ||
| allowed_tools: "mcp__unity,Edit(reports/**),MultiEdit(reports/**)" | ||
| disallowed_tools: "Bash,WebFetch,WebSearch,Task,TodoWrite,NotebookEdit,NotebookRead" | ||
| model: claude-sonnet-4-5-20250929 | ||
| fallback_model: claude-haiku-4-5-20251001 | ||
| append_system_prompt: | | ||
| You are running the GO pass only. | ||
| Output requirements: | ||
| - Emit exactly 11 test fragments: GO-0, GO-1, GO-2, GO-3, GO-4, GO-5, GO-6, GO-7, GO-8, GO-9, GO-10. | ||
| - Write each fragment to reports/${ID}_results.xml (e.g., GO-0_results.xml). | ||
| - Prefer a single MultiEdit(reports/**) call that writes all eleven files in one batch. | ||
| - Do not emit any NL-* or T-* fragments. | ||
| Stop condition: | ||
| - After GO-10_results.xml is written, stop. | ||
| timeout_minutes: "20" | ||
| anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add post-retry assertion for GO coverage.
The T pass includes a re-assertion step after the retry (lines 740-752) that fails fast if tests are still missing. The GO pass should have the same pattern to maintain consistency and fail fast if the retry doesn't complete all GO tests.
🔎 Proposed addition after line 822
+ - name: Re-assert GO coverage (post-retry)
+ if: always()
+ shell: bash
+ run: |
+ set -euo pipefail
+ missing=()
+ for id in GO-0 GO-1 GO-2 GO-3 GO-4 GO-5 GO-6 GO-7 GO-8 GO-9 GO-10; do
+ [[ -s "reports/${id}_results.xml" ]] || missing+=("$id")
+ done
+ if (( ${#missing[@]} )); then
+ echo "::error::Still missing GO fragments: ${missing[*]}"
+ exit 1
+ fi🤖 Prompt for AI Agents
In @.github/workflows/claude-nl-suite.yml around lines 782-822, Add a post-retry
assertion step after the "Retry GO pass (Sonnet)" step named something like
"Check GO coverage incomplete (post-retry)" with id go_cov_post that runs with
shell: bash and if: always(); reuse the same script from the pre-retry "Check GO
coverage incomplete (pre-retry)" step (set -euo pipefail, build missing array by
checking reports/${ID}_results.xml and reports/_staging/${ID}_results.xml for
GO-0..GO-10, echo missing and list to $GITHUB_OUTPUT) and if any missing remain
exit non-zero (e.g., echo an error and exit 1) so the workflow fails fast when
the retry didn’t produce all GO fragments.
| ) -> dict[str, Any]: | ||
| unity_instance = get_unity_instance_from_context(ctx) |
There was a problem hiding this comment.
Missing preflight check.
Unlike manage_asset, manage_gameobject, and manage_components, this tool doesn't call preflight() before executing. This could cause issues if Unity is compiling or reloading. Consider adding the preflight gate for consistency.
🔎 Proposed fix
+from services.tools.preflight import preflight
+
+
@mcp_for_unity_tool(
description="Manages Unity materials (set properties, colors, shaders, etc)."
)
async def manage_material(
...
) -> dict[str, Any]:
unity_instance = get_unity_instance_from_context(ctx)
+
+ gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True)
+ if gate is not None:
+ return gate.model_dump()
# --- Normalize color with validation ---📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) -> dict[str, Any]: | |
| unity_instance = get_unity_instance_from_context(ctx) | |
| ) -> dict[str, Any]: | |
| unity_instance = get_unity_instance_from_context(ctx) | |
| gate = await preflight(ctx, wait_for_no_compile=True, refresh_if_dirty=True) | |
| if gate is not None: | |
| return gate.model_dump() |
🤖 Prompt for AI Agents
In @Server/src/services/tools/manage_material.py around lines 105-106, The
manage_material function is missing the preflight gate; update the beginning of
manage_material to call preflight(...) (same signature used by
manage_asset/manage_gameobject/manage_components) before proceeding (i.e.,
before executing any Unity actions or before
get_unity_instance_from_context(ctx)) so Unity compile/reload is waited for;
reference the manage_material function and the preflight() helper when making
the change.
…layDev#517) * fix: Multi-session UI improvements and HTTP instance recognition - Separate server and session lifecycle in HTTP Local mode - Show 'Start Server' / 'Stop Server' button for server control - Show 'Start Session' / 'End Session' button when server is running - No auto-join on server start (requires manual session start) - Show instance name instead of port in session status (e.g. 'Session Active (ramble)') - Use native project_hash for HTTP instance recognition instead of computed SHA256 - Fix test expectations for manage_asset JSON parsing error messages * fix: Multi-session UI improvements and HTTP instance recognition - Separate server and session lifecycle in HTTP Local mode - Show 'Start Server' / 'Stop Server' button for server control - Show 'Start Session' / 'End Session' button when server is running - Auto-start session when THIS instance starts the server - Require manual session start when connecting to external server - Auto-detect and end orphaned sessions when server stops - Show instance name instead of port in session status - Use native project_hash for HTTP instance recognition - Guard against empty hash with warning log - Remove dead code (duplicate _safe_response) - Add defensive path handling for instance name extraction
…yDev#480) * feat: Add tool annotations for improved LLM tool understanding Add readOnlyHint and destructiveHint annotations to all 23 tools to help LLMs better understand tool behavior and make safer decisions. Changes: - Added readOnlyHint: true to 5 read-only tools: - debug_request_context, find_in_file, validate_script, manage_script_capabilities, get_sha - Added destructiveHint: true to 18 tools that modify data: - All CRUD tools, execution tools, and state-modifying tools - Added title annotations for human-readable display - Imported ToolAnnotations from mcp.types in all tool files This improves tool safety metadata for MCP clients. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Clarify dual-purpose tool behavior in annotations Address Sourcery AI review feedback by updating descriptions for batch_execute and manage_* tools to clarify their safety characteristics depend on the specific actions/operations being performed. Updated tools: - batch_execute: clarifies safety depends on contained tools - read_console: clarifies 'get' is read-only, 'clear' is destructive - manage_asset: lists read-only vs destructive actions - manage_gameobject: lists read-only vs destructive actions - manage_scene: lists read-only vs destructive actions - manage_editor: lists read-only vs destructive actions - manage_material: lists read-only vs destructive actions - manage_shader: lists read-only vs destructive actions - manage_script: lists read-only vs destructive actions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Remove destructiveHint from mixed-behavior tools Address maintainer feedback on annotation semantics. Tools that have both read-only and modifying actions based on the 'action' parameter should not be marked as destructive. Changed to title-only annotations: - manage_gameobject (find/get vs create/modify/delete) - manage_scene (get_hierarchy vs create/load/save) - manage_prefabs (open_stage vs save/create) - manage_material (ping/get vs create/set) - manage_shader (read vs create/update/delete) - manage_editor (telemetry_status vs play/pause) - manage_script router (read vs create/delete) - batch_execute (depends on contained operations) - execute_menu_item (behavior varies by menu item) - execute_custom_tool (behavior varies by custom tool) - read_console (get vs clear - ephemeral UI state) Kept destructiveHint=True for always-destructive tools: - apply_text_edits (always writes files) - create_script (always creates files) - delete_script (always deletes files) - script_apply_edits (always writes files) - run_tests (always executes tests) - set_active_instance (always modifies session) * fix: Remove destructiveHint from set_active_instance set_active_instance is a session-scoped operation that switches which Unity instance is active. It doesn't persist data and is easily reversible, so it should not be marked as destructive. * fix: Re-apply destructiveHint per maintainer guidance on MCP spec Per maintainer feedback referencing MCP docs, tools should have destructiveHint=True if they MAY perform a destructive action, even if they also support read-only operations. Re-applied to tools that can destroy Unity resources: - manage_gameobject, manage_scene, manage_prefabs - manage_material, manage_shader, manage_script - manage_asset, batch_execute - execute_menu_item, execute_custom_tool Kept as-is (no destructiveHint): - read_console (clearing is ephemeral UI state) - set_active_instance (session toggle, not destructive) - manage_editor (no destructive actions) 🤖 Generated with [Claude Code](https://claude.com/claude-code) * feat: Add tool annotations to manage_scriptable_object Per reviewer request (@dsarno), add ToolAnnotations with destructiveHint=True to manage_scriptable_object tool since it can create/modify ScriptableObject assets. 🤖 Generated with [Claude Code](https://claude.com/claude-code) * feat: Add tool annotations to new async test and refresh tools Add readOnlyHint and destructiveHint annotations to tools added since the last commit: - refresh_unity: destructiveHint=True (triggers asset DB refresh) - run_tests_async: destructiveHint=True (starts test execution) - get_test_job: readOnlyHint=True (queries job status only) Also regenerate uv.lock after rebase. --------- Co-authored-by: triepod-ai <noreply@github.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: triepod-ai <199543909+triepod-ai@users.noreply.github.com>
- Add missing tools: manage_components, batch_execute, find_gameobjects, refresh_unity - Add missing resources: gameobject_api, editor_state_v2 - Make descriptions more concise across all tools and resources - Ensure documentation matches current MCP server functionality
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @commit.sh:
- Around line 1-9: commit.sh is a personal utility containing a hardcoded path
and should be removed from version control; remove the committed commit.sh file
from the repo, add "commit.sh" to .gitignore to prevent future accidental
commits, and commit the .gitignore change; optionally keep the script locally
and, if you intend to keep a shared script, update the cd invocation inside
commit.sh to handle failures (e.g., change the bare "cd
/Users/davidsarno/unity-mcp" to a safe check like "cd <path> || exit 1") before
re-adding a sanitized/shared version.
In @Server/src/services/custom_tool_service.py:
- Around line 271-276: The compute_project_id function is deprecated and not
referenced anywhere; remove the function definition from
Server/src/services/custom_tool_service.py to eliminate dead code, or if you
prefer a transitional approach keep the function but add a runtime
DeprecationWarning at the start of compute_project_id (using warnings.warn(...,
DeprecationWarning)) so callers are notified before you delete it in a future
PR; update any tests or imports that reference it accordingly.
In @TestProjects/UnityMCPTests/Assets/Scripts/BounceCubeSetup.cs:
- Around line 131-138: The reflection fallback in BounceCubeSetup.cs is using
BindingFlags.NonPublic|Instance to find the Bounce fields but Bounce.height and
Bounce.speed are public, so the GetField calls will return null; change the
logic to either set the fields directly on the bounce instance (e.g. assign
bounce.height = heights[i] and bounce.speed = speeds[i]) or update the
reflection to use BindingFlags.Public | BindingFlags.Instance when calling
typeof(Bounce).GetField("height", ...) and GetField("speed", ...), and keep the
null checks to preserve safety.
In @TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.cs:
- Around line 84-96: The code currently does mat.color = materialColors[i] after
AssetDatabase.LoadAssetAtPath, which mutates the on-disk Material asset; instead
create a runtime instance before changing color and assigning to the renderer
(e.g., instantiate the loaded Material and set its color, then assign that
instance to the renderer), or if the intent is to permanently modify the asset,
call EditorUtility.SetDirty(mat) after changing mat.color to ensure the change
is saved; update the block around AssetDatabase.LoadAssetAtPath, mat.color, and
renderer.sharedMaterial to use the chosen approach.
🧹 Nitpick comments (8)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
545-577: Consider differentiating retry behavior.The current logic retries both "server not ready" and "bridge start failed" scenarios with the same delay. If the server is confirmed running (line 556) but
StartAsyncfails, continuing to retry might not help and delays user feedback.🔎 Optional: Fail faster when bridge start fails with server running
for (int attempt = 0; attempt < maxAttempts; attempt++) { // Check if server is actually accepting connections if (!MCPServiceLocator.Server.IsLocalHttpServerRunning()) { await Task.Delay(delay); continue; } bool started = await bridgeService.StartAsync(); if (started) { await VerifyBridgeConnectionAsync(); UpdateConnectionStatus(); return; } - if (attempt < maxAttempts - 1) - { - await Task.Delay(delay); - } + // Server is running but bridge failed - log and break rather than retry + McpLog.Warn($"Bridge start failed on attempt {attempt + 1} with server running"); + break; }Server/src/services/custom_tool_service.py (1)
228-260: Consider refactoring to remove unreachable code.The
_normalize_responsemethod has duplicate dict handling. Lines 231-238 checkisinstance(response, dict)and return anMCPResponse, making the second dict check at lines 245-256 unreachable. This appears to be pre-existing technical debt rather than introduced by the current changes.🔎 Suggested refactor to consolidate dict handling
def _normalize_response(self, response) -> MCPResponse: if isinstance(response, MCPResponse): return response if isinstance(response, dict): return MCPResponse( success=response.get("success", True), message=response.get("message"), error=response.get("error"), data=response.get( "data", response) if "data" not in response else response["data"], ) - - success = True - message = None - error = None - data = None - - if isinstance(response, dict): - success = response.get("success", True) - if "_mcp_status" in response and response["_mcp_status"] == "error": - success = False - message = str(response.get("message")) if response.get( - "message") else None - error = str(response.get("error")) if response.get( - "error") else None - data = response.get("data") - if "success" not in response and "_mcp_status" not in response: - data = response - else: - success = False - message = str(response) - - return MCPResponse(success=success, message=message, error=error, data=data) + + # Handle non-dict, non-MCPResponse cases + return MCPResponse(success=False, message=str(response), error=None, data=None)TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounce.cs (1)
35-40: Consider using absolute sine for one-sided bounce.The current implementation oscillates both above and below
startPosition. If the intent is a typical "bounce" that stays above the starting point, consider usingMathf.Abs():-float bounceAmount = Mathf.Sin((Time.time * speed) + timeOffset) * height; +float bounceAmount = Mathf.Abs(Mathf.Sin((Time.time * speed) + timeOffset)) * height;If bidirectional oscillation is intentional, this can be ignored.
TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.cs (4)
6-12: Consider guarding against repeated setup calls.With
[ExecuteInEditMode],Start()is called when the script is enabled in the editor and when entering play mode. This could cause repeated setup operations. Consider adding a guard or removing the automatic call fromStart()since[ContextMenu]already provides manual invocation.🔎 Suggested approach
void Start() { - SetupBounceCylinders(); + // Removed auto-setup; use ContextMenu "Setup Bounce Cylinders" instead + // SetupBounceCylinders(); }Alternatively, add a guard flag if automatic setup is needed.
17-29: Consider using a loop to reduce repetition.The 10
GameObject.Findcalls can be simplified with a loop:🔎 Suggested refactor
-GameObject[] cylinders = { - GameObject.Find("BounceCylinder_1"), - GameObject.Find("BounceCylinder_2"), - GameObject.Find("BounceCylinder_3"), - GameObject.Find("BounceCylinder_4"), - GameObject.Find("BounceCylinder_5"), - GameObject.Find("BounceCylinder_6"), - GameObject.Find("BounceCylinder_7"), - GameObject.Find("BounceCylinder_8"), - GameObject.Find("BounceCylinder_9"), - GameObject.Find("BounceCylinder_10") -}; +GameObject[] cylinders = new GameObject[10]; +for (int i = 0; i < 10; i++) +{ + cylinders[i] = GameObject.Find($"BounceCylinder_{i + 1}"); +}
65-77:materialColorsarray is only used in editor mode.The
materialColorsarray is defined outside the#if UNITY_EDITORblock but is only used within it (line 89). This is fine but could be moved inside the preprocessor block or documented for clarity.
99-104: Consider usingUndo.AddComponentin editor mode for undo support.In edit mode,
AddComponentwithoutUndoregistration means the operation cannot be undone. Based on similar patterns inManageComponents.cs, wrap withUndo:🔎 Suggested fix
CylinderBounce bounce = cylinders[i].GetComponent<CylinderBounce>(); if (bounce == null) { +#if UNITY_EDITOR + bounce = Undo.AddComponent<CylinderBounce>(cylinders[i]); +#else bounce = cylinders[i].AddComponent<CylinderBounce>(); +#endif }TestProjects/UnityMCPTests/Assets/Scripts/BounceScript.cs (1)
5-10: Nice use of Unity Inspector attributes!The Header and Tooltip attributes make this script user-friendly in the Unity Inspector. The default values are sensible for a bounce effect.
Optionally, consider adding validation to ensure
bounceHeightandbounceSpeedremain positive, as negative or zero values could produce unexpected results:🔎 Optional: Add validation in Start or use property setters
void Start() { + bounceHeight = Mathf.Max(0f, bounceHeight); + bounceSpeed = Mathf.Max(0.01f, bounceSpeed); startPosition = transform.position; // Random offset so spheres don't all bounce in sync timeOffset = Random.Range(0f, Mathf.PI * 2f); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (46)
Server/uv.lockis excluded by!**/*.lockTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Blue.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Cyan.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Green.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Magenta.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Orange.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Pink.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Purple.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Red.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Teal.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Yellow.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Blue.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Cyan.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Emerald.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Gold.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Green.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Orange.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Pink.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Purple.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Red.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Yellow.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat1.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat10.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat2.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat3.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat4.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat5.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat6.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat7.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat8.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CubeMat9.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_10_Gold.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_1_Red.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_2_Blue.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_3_Green.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_4_Yellow.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_5_Purple.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_6_Orange.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_7_Cyan.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_8_Pink.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_9_Emerald.matis excluded by!**/*.matTestProjects/UnityMCPTests/Assets/Shaders/HolographicNoiseShader.shaderis excluded by!**/*.shaderTestProjects/UnityMCPTests/Assets/Shaders/NoiseDisplacementShader.shaderis excluded by!**/*.shaderTestProjects/UnityMCPTests/Assets/Shaders/PulsingNoiseShader.shaderis excluded by!**/*.shaderTestProjects/UnityMCPTests/Assets/Shaders/SpikyNoiseShader.shaderis excluded by!**/*.shaderTestProjects/UnityMCPTests/Assets/Shaders/WavyDistortionShader.shaderis excluded by!**/*.shader
📒 Files selected for processing (67)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csREADME.mdServer/src/services/custom_tool_service.pyServer/tests/integration/test_manage_asset_json_parsing.pyTestProjects/UnityMCPTests/Assets/Materials.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Blue.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Cyan.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Green.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Magenta.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Orange.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Pink.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Purple.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Red.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Teal.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceCube_Yellow.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Blue.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Cyan.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Emerald.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Gold.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Green.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Orange.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Pink.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Purple.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Red.mat.metaTestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Yellow.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat1.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat10.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat2.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat3.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat4.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat5.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat6.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat7.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat8.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CubeMat9.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_10_Gold.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_1_Red.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_2_Blue.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_3_Green.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_4_Yellow.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_5_Purple.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_6_Orange.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_7_Cyan.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_8_Pink.mat.metaTestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_9_Emerald.mat.metaTestProjects/UnityMCPTests/Assets/Scripts/Bounce.csTestProjects/UnityMCPTests/Assets/Scripts/Bounce.cs.metaTestProjects/UnityMCPTests/Assets/Scripts/BounceCubeSetup.csTestProjects/UnityMCPTests/Assets/Scripts/BounceCubeSetup.cs.metaTestProjects/UnityMCPTests/Assets/Scripts/BounceScript.csTestProjects/UnityMCPTests/Assets/Scripts/BounceScript.cs.metaTestProjects/UnityMCPTests/Assets/Scripts/CylinderBounce.csTestProjects/UnityMCPTests/Assets/Scripts/CylinderBounce.cs.metaTestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.csTestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.cs.metaTestProjects/UnityMCPTests/Assets/Scripts/Editor.metaTestProjects/UnityMCPTests/Assets/Scripts/Editor/BounceCubeSetupEditor.csTestProjects/UnityMCPTests/Assets/Scripts/Editor/BounceCubeSetupEditor.cs.metaTestProjects/UnityMCPTests/Assets/Scripts/TestScriptableObjectInstance.asset.metaTestProjects/UnityMCPTests/Assets/Shaders.metaTestProjects/UnityMCPTests/Assets/Shaders/HolographicNoiseShader.shader.metaTestProjects/UnityMCPTests/Assets/Shaders/NoiseDisplacementShader.shader.metaTestProjects/UnityMCPTests/Assets/Shaders/PulsingNoiseShader.shader.metaTestProjects/UnityMCPTests/Assets/Shaders/SpikyNoiseShader.shader.metaTestProjects/UnityMCPTests/Assets/Shaders/WavyDistortionShader.shader.metaTestProjects/UnityMCPTests/Assets/Temp.metacommit.sh
✅ Files skipped from review due to trivial changes (46)
- TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Red.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_6_Orange.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Orange.mat.meta
- TestProjects/UnityMCPTests/Assets/Scripts/Bounce.cs.meta
- TestProjects/UnityMCPTests/Assets/Scripts/TestScriptableObjectInstance.asset.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Gold.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Cyan.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Green.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat8.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_7_Cyan.mat.meta
- TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.cs.meta
- TestProjects/UnityMCPTests/Assets/Scripts/Editor.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Emerald.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_5_Purple.mat.meta
- TestProjects/UnityMCPTests/Assets/Shaders/NoiseDisplacementShader.shader.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat7.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_2_Blue.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Yellow.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_1_Red.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Pink.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Orange.mat.meta
- TestProjects/UnityMCPTests/Assets/Scripts/BounceScript.cs.meta
- TestProjects/UnityMCPTests/Assets/Shaders/SpikyNoiseShader.shader.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_4_Yellow.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Blue.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_3_Green.mat.meta
- TestProjects/UnityMCPTests/Assets/Shaders.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Purple.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Magenta.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Purple.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_10_Gold.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Cyan.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat4.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat9.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Red.mat.meta
- TestProjects/UnityMCPTests/Assets/Shaders/WavyDistortionShader.shader.meta
- TestProjects/UnityMCPTests/Assets/Shaders/PulsingNoiseShader.shader.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat3.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_8_Pink.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat6.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat10.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Green.mat.meta
- TestProjects/UnityMCPTests/Assets/Materials/CubeMat2.mat.meta
- TestProjects/UnityMCPTests/Assets/Scripts/Editor/BounceCubeSetupEditor.cs.meta
- TestProjects/UnityMCPTests/Assets/Materials.meta
- TestProjects/UnityMCPTests/Assets/Temp.meta
🧰 Additional context used
🧬 Code graph analysis (4)
Server/src/services/custom_tool_service.py (1)
Server/tests/integration/test_helpers.py (1)
warning(39-40)
TestProjects/UnityMCPTests/Assets/Scripts/BounceCubeSetup.cs (2)
TestProjects/UnityMCPTests/Assets/Scripts/Bounce.cs (1)
Bounce(3-24)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
SerializedObject(349-370)
TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.cs (4)
TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounce.cs (1)
CylinderBounce(3-41)MCPForUnity/Editor/Tools/ManageComponents.cs (1)
AddComponent(68-126)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
SerializedObject(349-370)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
TestProjects/UnityMCPTests/Assets/Scripts/Editor/BounceCubeSetupEditor.cs (1)
MCPForUnity/Editor/Tools/ManageComponents.cs (1)
AddComponent(68-126)
🪛 Shellcheck (0.11.0)
commit.sh
[warning] 2-2: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (32)
TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Teal.mat.meta (1)
1-8: LGTM!This is a standard Unity-generated meta file for a material asset. The GUID is valid, the
NativeFormatImporteris the correct importer type for.matfiles, andmainObjectFileID: 2100000is the standard identifier for Material objects.TestProjects/UnityMCPTests/Assets/Materials/BounceSphere_Blue.mat.meta (1)
1-8: LGTM! Standard Unity material metadata.This is a properly formatted Unity meta file for a material asset. The structure, GUID, and mainObjectFileID (2100000 for materials) are all standard and correct. These auto-generated files are essential for Unity's asset tracking and should be committed to version control.
TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Pink.mat.meta (1)
1-8: LGTM! Standard Unity meta file with correct formatting.This is a properly formatted Unity-generated meta file for the BounceCube_Pink material asset. The structure follows Unity conventions with the correct
mainObjectFileID: 2100000for Material assets and a valid GUID for asset reference tracking.TestProjects/UnityMCPTests/Assets/Materials/BounceCube_Yellow.mat.meta (1)
1-8: LGTM! Standard Unity meta file.This meta file is correctly formatted with standard Unity configuration for a material asset. The structure, GUID, and NativeFormatImporter settings are all appropriate.
TestProjects/UnityMCPTests/Assets/Shaders/HolographicNoiseShader.shader.meta (1)
1-10: Standard Unity shader meta structure - GUID is unique.The shader meta file structure is correct and follows Unity's standard format with appropriate default values. The GUID
1698d9b3ded69439b81fc03672d74ac2is unique across the project.TestProjects/UnityMCPTests/Assets/Materials/CubeMat5.mat.meta (1)
1-8: Standard Unity material metadata - correctly formatted.The metadata file is properly structured with correct field values for a Material asset. The corresponding
CubeMat5.matfile exists as expected.TestProjects/UnityMCPTests/Assets/Materials/CylinderBounce_9_Emerald.mat.meta (1)
1-8: LGTM! Standard Unity material metadata.The file structure is correct and follows Unity's standard format for material asset metadata. The
mainObjectFileID: 2100000is appropriate for Material assets, and all fields are properly configured. The GUID is unique across the project with no conflicts.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (6)
276-283: Orphaned session detection looks good.The guard
!connectionToggleInProgressprevents re-entrant cleanup, and settingisRunning = falselocally ensures consistent UI state for the remainder of this method. The fire-and-forget pattern is appropriate here sinceEndOrphanedSessionAsynchas its own error handling.
285-292: LGTM!The visibility logic correctly separates server lifecycle from session lifecycle, showing the session toggle only when relevant.
311-318: Good defensive handling for instance name extraction.The fallback chain ensures a sensible display name even for edge cases. The approach of using the project folder name for multi-instance identification is clear and user-friendly.
464-472: LGTM!Server button state is now correctly decoupled from session state, making the UI more intuitive for multi-instance workflows.
500-530: Clean refactor of the toggle handler.The stop-before-server pattern correctly prevents orphaned sessions. The distinction between "we started the server" (auto-start session) vs "detected external server" (manual session start) is a good UX decision.
660-680: LGTM!Proper cleanup with flag guarding, error handling, and guaranteed UI restoration in the finally block. The fire-and-forget usage is appropriate for this "best-effort cleanup" scenario.
TestProjects/UnityMCPTests/Assets/Scripts/BounceCubeSetup.cs.meta (1)
1-11: LGTM!Standard Unity meta file with valid MonoImporter settings.
TestProjects/UnityMCPTests/Assets/Scripts/Editor/BounceCubeSetupEditor.cs (1)
1-28: LGTM!Clean editor utility implementation. The
InitializeOnLoadpattern withdelayCallensures setup runs after the editor is fully initialized, and the temporary GameObject approach is appropriate for invoking the MonoBehaviour setup method from editor code.TestProjects/UnityMCPTests/Assets/Scripts/Bounce.cs (1)
1-24: LGTM!Simple and effective bounce implementation. The random
timeOffsetnicely desynchronizes multiple instances, and the sine wave provides smooth vertical motion.TestProjects/UnityMCPTests/Assets/Scripts/BounceCubeSetup.cs (3)
100-113: Modifying shared material assets directly.Setting
mat.colormodifies the material asset on disk, which persists across sessions. This is fine for initial setup, but be aware thatExecuteInEditModecombined withStart()means this runs each time the component activates in the editor, potentially re-applying colors repeatedly.
91-98: LGTM!Good defensive null check for missing cubes, and the transform setup is clean.
122-130: LGTM!Correct use of
SerializedObjectandApplyModifiedProperties()to ensure proper serialization of the Bounce component fields in the editor.Server/tests/integration/test_manage_asset_json_parsing.py (3)
35-37: LGTM! Test is less coupled to implementation details.Removing the logging assertion makes the test more focused on behavior rather than implementation. The test still validates that JSON string properties are successfully parsed and processed.
58-60: LGTM! Error message expectation updated.The updated error message format is clearer and follows standard capitalization conventions.
112-164: LGTM! Comprehensive test coverage for component_properties parsing.The two-test approach is excellent:
- First test validates end-to-end success with JSON string input
- Second test verifies the actual transformation from JSON string to dict before backend communication
The verification test at lines 141-164 is particularly valuable as it proves the parsing happens correctly, not just that the operation succeeds.
README.md (2)
44-70: LGTM! Documentation accurately reflects expanded tooling.The additions clearly document new capabilities:
manage_componentsfor component operationsbatch_executefor parallel command executionfind_gameobjectswith pagination support- Enhanced descriptions for
apply_text_editswith precise line/column rangesAll descriptions are concise and informative.
79-92: LGTM! Resources section expanded appropriately.The new resource entries provide valuable documentation:
gameobject_apiguides users on GameObject resources and searcheditor_state_v2offers canonical readiness snapshot- Enhanced descriptions clarify resource capabilities
All additions support the GameObject toolset redesign objectives.
Server/src/services/custom_tool_service.py (1)
309-314: LGTM! Hash resolution correctly uses Unity's native project hash.The change appropriately returns Unity's native
project_hashdirectly instead of computing a SHA256-based ID. The error handling for empty hashes with a warning log is a good defensive practice.TestProjects/UnityMCPTests/Assets/Materials/CubeMat1.mat.meta (1)
1-8: No action needed. All material metadata files in the directory already have unique GUIDs, including CubeMat1.mat.meta (f280a7af5c1be48228e98129c01a8939). This is a standard Unity-generated metadata file with correct structure and formatting.TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounce.cs.meta (1)
1-11: LGTM!Standard Unity-generated metadata file with default MonoImporter settings. The GUID ensures proper asset referencing across the project.
TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounce.cs (2)
3-16: LGTM!Clean field declarations with appropriate use of
[Header]and[Tooltip]attributes for editor usability. Default values are sensible.
18-33: LGTM!Solid initialization logic with proper handling of near-zero direction vectors. The random
timeOffsetensures visual variety when multiple cylinders bounce.TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.cs (1)
125-126: LGTM!The completion log is helpful. For more actionable feedback, you could optionally log the count of successfully configured cylinders.
TestProjects/UnityMCPTests/Assets/Scripts/BounceScript.cs (2)
15-20: Excellent initialization logic!Storing the start position and using a random time offset is the right approach to desynchronize multiple bouncing objects. The use of
Mathf.PI * 2fensures full phase coverage across the sine wave.
22-27: Sine wave implementation is correct and frame-rate independent.The bounce animation logic is well-implemented for test purposes. The use of
Time.timeensures frame-rate independence, andstartPositionis properly cached inStart()to avoid redundant calculations.Note: The performance optimization suggestion in the original review is not valid—both the original code (
startPosition + new Vector3(0, yOffset, 0)) and the suggested alternative allocate a newVector3each frame, so there is no meaningful performance improvement. Since this is test/demo code andBounceScriptis not currently used in any scenes or prefabs in the project, the current implementation is acceptable.
| """ | ||
| DEPRECATED: Computes a SHA256-based project ID. | ||
| This function is no longer used as of the multi-session fix. | ||
| Unity instances now use their native project_hash (SHA1-based) for consistency | ||
| across stdio and WebSocket transports. | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to compute_project_id to confirm it's truly deprecated
rg -nP --type=py -C3 '\bcompute_project_id\s*\(' -g '!**/test/**'Repository: dsarno/unity-mcp
Length of output: 626
Deprecation notice is accurate; consider removing the function or adding a runtime warning.
The codebase search confirms compute_project_id is indeed unused—no calls to this function exist in production code. Since it's no longer referenced, either remove the function entirely or add a DeprecationWarning to alert any future callers before full removal.
🤖 Prompt for AI Agents
In @Server/src/services/custom_tool_service.py around lines 271-276, The
compute_project_id function is deprecated and not referenced anywhere; remove
the function definition from Server/src/services/custom_tool_service.py to
eliminate dead code, or if you prefer a transitional approach keep the function
but add a runtime DeprecationWarning at the start of compute_project_id (using
warnings.warn(..., DeprecationWarning)) so callers are notified before you
delete it in a future PR; update any tests or imports that reference it
accordingly.
| #else | ||
| // Runtime fallback - make fields public temporarily or use reflection | ||
| var heightField = typeof(Bounce).GetField("height", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); | ||
| var speedField = typeof(Bounce).GetField("speed", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); | ||
|
|
||
| if (heightField != null) heightField.SetValue(bounce, heights[i]); | ||
| if (speedField != null) speedField.SetValue(bounce, speeds[i]); | ||
| #endif |
There was a problem hiding this comment.
Runtime fallback reflection uses wrong binding flags.
The height and speed fields in Bounce.cs are declared as public, but the reflection here searches for NonPublic | Instance fields. This will return null and silently fail to set the values at runtime.
🔎 Proposed fix
#else
// Runtime fallback - make fields public temporarily or use reflection
- var heightField = typeof(Bounce).GetField("height", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
- var speedField = typeof(Bounce).GetField("speed", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
+ var heightField = typeof(Bounce).GetField("height", System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance);
+ var speedField = typeof(Bounce).GetField("speed", System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance);
if (heightField != null) heightField.SetValue(bounce, heights[i]);
if (speedField != null) speedField.SetValue(bounce, speeds[i]);
#endifAlternatively, since the fields are public, you can set them directly without reflection:
#else
- // Runtime fallback - make fields public temporarily or use reflection
- var heightField = typeof(Bounce).GetField("height", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
- var speedField = typeof(Bounce).GetField("speed", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
-
- if (heightField != null) heightField.SetValue(bounce, heights[i]);
- if (speedField != null) speedField.SetValue(bounce, speeds[i]);
+ // Runtime fallback - set public fields directly
+ bounce.height = heights[i];
+ bounce.speed = speeds[i];
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #else | |
| // Runtime fallback - make fields public temporarily or use reflection | |
| var heightField = typeof(Bounce).GetField("height", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); | |
| var speedField = typeof(Bounce).GetField("speed", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); | |
| if (heightField != null) heightField.SetValue(bounce, heights[i]); | |
| if (speedField != null) speedField.SetValue(bounce, speeds[i]); | |
| #endif | |
| #else | |
| // Runtime fallback - make fields public temporarily or use reflection | |
| var heightField = typeof(Bounce).GetField("height", System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance); | |
| var speedField = typeof(Bounce).GetField("speed", System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance); | |
| if (heightField != null) heightField.SetValue(bounce, heights[i]); | |
| if (speedField != null) speedField.SetValue(bounce, speeds[i]); | |
| #endif |
🤖 Prompt for AI Agents
In @TestProjects/UnityMCPTests/Assets/Scripts/BounceCubeSetup.cs around lines
131-138, The reflection fallback in BounceCubeSetup.cs is using
BindingFlags.NonPublic|Instance to find the Bounce fields but Bounce.height and
Bounce.speed are public, so the GetField calls will return null; change the
logic to either set the fields directly on the bounce instance (e.g. assign
bounce.height = heights[i] and bounce.speed = speeds[i]) or update the
reflection to use BindingFlags.Public | BindingFlags.Instance when calling
typeof(Bounce).GetField("height", ...) and GetField("speed", ...), and keep the
null checks to preserve safety.
| #if UNITY_EDITOR | ||
| Material mat = AssetDatabase.LoadAssetAtPath<Material>(materialPaths[i]); | ||
| if (mat != null) | ||
| { | ||
| // Set material color | ||
| mat.color = materialColors[i]; | ||
|
|
||
| var renderer = cylinders[i].GetComponent<MeshRenderer>(); | ||
| if (renderer != null) | ||
| { | ||
| renderer.sharedMaterial = mat; | ||
| } | ||
| } |
There was a problem hiding this comment.
Setting mat.color modifies the asset on disk.
AssetDatabase.LoadAssetAtPath returns the actual asset, so mat.color = materialColors[i] permanently modifies the material asset. If this is unintended, create a runtime instance or ensure the colors are pre-baked in the materials.
🔎 Option: Use material instance to avoid asset modification
Material mat = AssetDatabase.LoadAssetAtPath<Material>(materialPaths[i]);
if (mat != null)
{
- // Set material color
- mat.color = materialColors[i];
-
var renderer = cylinders[i].GetComponent<MeshRenderer>();
if (renderer != null)
{
- renderer.sharedMaterial = mat;
+ // Create instance to avoid modifying the asset
+ renderer.material = new Material(mat);
+ renderer.material.color = materialColors[i];
}
}If modifying the asset is intentional (one-time setup), consider adding EditorUtility.SetDirty(mat) to ensure changes are saved.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if UNITY_EDITOR | |
| Material mat = AssetDatabase.LoadAssetAtPath<Material>(materialPaths[i]); | |
| if (mat != null) | |
| { | |
| // Set material color | |
| mat.color = materialColors[i]; | |
| var renderer = cylinders[i].GetComponent<MeshRenderer>(); | |
| if (renderer != null) | |
| { | |
| renderer.sharedMaterial = mat; | |
| } | |
| } | |
| #if UNITY_EDITOR | |
| Material mat = AssetDatabase.LoadAssetAtPath<Material>(materialPaths[i]); | |
| if (mat != null) | |
| { | |
| var renderer = cylinders[i].GetComponent<MeshRenderer>(); | |
| if (renderer != null) | |
| { | |
| // Create instance to avoid modifying the asset | |
| renderer.material = new Material(mat); | |
| renderer.material.color = materialColors[i]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @TestProjects/UnityMCPTests/Assets/Scripts/CylinderBounceSetup.cs around
lines 84-96, The code currently does mat.color = materialColors[i] after
AssetDatabase.LoadAssetAtPath, which mutates the on-disk Material asset; instead
create a runtime instance before changing color and assigning to the renderer
(e.g., instantiate the loaded Material and set its color, then assign that
instance to the renderer), or if the intent is to permanently modify the asset,
call EditorUtility.SetDirty(mat) after changing mat.color to ensure the change
is saved; update the block around AssetDatabase.LoadAssetAtPath, mat.color, and
renderer.sharedMaterial to use the chosen approach.
- ParamCoercion: Use InvariantCulture for int/double parsing consistency - ManageComponents: Remove redundant Undo.RecordObject (AddComponent handles undo) - ManageScene: Replace deprecated FindObjectsOfType with FindObjectsByType - GameObjectLookup: Add explanatory comment to empty catch block - gameobject.py: Extract _validate_instance_id helper to reduce duplication - Tests: Fix assertion for instanceID (Unity IDs can be negative)
- Remove Materials folder (40 .mat files from interactive testing) - Remove Shaders folder (5 noise shaders from testing) - Remove test scripts (Bounce*, CylinderBounce* from testing) - Remove Temp.meta and commit.sh
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs:
- Around line 151-169: The test currently only asserts that
ManageGameObject.HandleCommand(p) returns a result but doesn’t verify that the
GameObject was actually deleted; call ManageGameObject.HandleCommand(p) and then
assert the deletion by checking that the target is no longer present in the
scene (e.g. Assert.IsNull(GameObject.Find(target.name)) or Assert.IsTrue(target
== null / !target using UnityEngine.Object semantics) and that testObjects no
longer contains the target (Assert.IsFalse(testObjects.Contains(target))); move
or keep testObjects.Remove(target) only after those assertions. Ensure you
reference the existing symbols: CreateTestObject, target,
ManageGameObject.HandleCommand, and testObjects.Remove when making the change.
- Around line 123-149: The test Delete_ByTag_DeletesMatchingObjects currently
only asserts that ManageGameObject.HandleCommand(p) returns a non-null result;
update the test to assert actual deletion by (1) checking that target1 and
target2 are destroyed or absent from scene (or not present in testObjects) after
calling ManageGameObject.HandleCommand, (2) inspect resultObj (the JObject
result) for a deletion count or list and assert it matches the number of
expected deletions, and (3) ensure you only remove surviving objects from
testObjects during cleanup so the assertions reflect real deletions; reference
ManageGameObject.HandleCommand, resultObj, target1, target2, and testObjects
when adding these assertions.
- Around line 310-329: The test Delete_MultipleObjectsSameName_DeletesCorrectly
currently only asserts a non-null result; make the behavior deterministic by
deciding the expected semantics (delete first match or delete all) and asserting
accordingly: after calling ManageGameObject.HandleCommand(p) use
CreateTestObject/Test helper state and Unity APIs (e.g., GameObject.Find or
counting live objects named "DuplicateName") to assert the expected number of
remaining/deleted objects, update testObjects cleanup to only remove survivors,
and add a clear comment documenting the chosen expected behavior so
Delete_MultipleObjectsSameName_DeletesCorrectly explicitly verifies which
objects were deleted.
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/ManageComponents.cs (1)
390-415: Consider extensibility for additional Unity types.The current implementation handles the most common Unity types (Vector2/3, Quaternion, Color). As usage expands, consider adding support for other types like
Vector4,Rect, orBoundsif they're frequently manipulated via this API.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (1)
262-284: Verify the actual deleted count value, not just data presence.The test name suggests it validates "ReturnsDeletedCount", but line 281 only checks that the
datafield exists without verifying its content. This doesn't fully validate the response structure.🔎 Suggested improvement
Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString()); // Check for deleted count in response var data = resultObj["data"]; Assert.IsNotNull(data, "Response should include data"); + + // Verify the actual count + if (data is JObject dataObj && dataObj.ContainsKey("deletedCount")) + { + Assert.AreEqual(1, dataObj.Value<int>("deletedCount"), "Should report 1 deleted object"); + } testObjects.Remove(target);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageScene.csServer/src/services/resources/gameobject.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- MCPForUnity/Editor/Helpers/GameObjectLookup.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs
🧰 Additional context used
🧬 Code graph analysis (4)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (6)
UnityEngine(2438-2535)List(1617-1776)List(2624-2661)List(2684-2702)List(2707-2746)List(2754-2789)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (7)
List(1617-1776)List(2624-2661)List(2684-2702)List(2707-2746)List(2754-2789)GameObject(1589-1612)HandleCommand(42-234)MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-81)
Server/src/services/resources/gameobject.py (3)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/tests/integration/test_helpers.py (1)
error(46-47)
MCPForUnity/Editor/Tools/ManageComponents.cs (3)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-158)CoerceString(121-128)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
GameObject(57-64)GameObject(69-72)Type(285-320)GameObjectLookup(15-341)MCPForUnity/Editor/Helpers/VectorParsing.cs (5)
Vector3(18-51)Vector3(56-59)VectorParsing(11-294)Vector2(66-97)Color(177-225)
🔇 Additional comments (26)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (6)
1-11: LGTM! Well-documented utility class.The class structure, XML documentation, and namespace organization are clean and appropriate for a helper utility.
19-45: LGTM!The integer coercion handles multiple input formats correctly, including float-to-int truncation for inputs like
"3.14".
53-82: LGTM!Comprehensive boolean coercion with support for various string representations (
"1","yes","on", etc.) in addition to standard boolean parsing.
90-113: LGTM!Float coercion correctly handles both numeric token types and culture-invariant string parsing.
121-128: LGTM!Simple and effective string coercion with proper null handling.
137-157: LGTM!Case-insensitive enum coercion with proper generic constraints.
MCPForUnity/Editor/Tools/ManageComponents.cs (7)
27-64: LGTM!Well-structured command handler with proper validation, case-insensitive action matching, and exception handling around action dispatch.
68-126: LGTM!AddComponent implementation is thorough with proper undo support, optional property initialization, and detailed success responses.
128-180: LGTM!RemoveComponent correctly protects against Transform removal and uses proper undo/destroy patterns.
182-280: LGTM!SetProperty supports flexible input modes (single property or batch) with error aggregation, enabling partial success feedback for batch operations.
286-310: LGTM!FindTarget efficiently prioritizes instance ID lookup before falling back to name/path-based search.
315-320: LGTM!Clean delegation to
GameObjectLookup.FindComponentType, eliminating the previously flagged code duplication.
341-388: LGTM!Case-insensitive property/field lookup improves usability with external API callers, and the error handling pattern is consistent.
MCPForUnity/Editor/Tools/ManageScene.cs (3)
399-400: LGTM!Good update from the deprecated
FindObjectsOfType<T>()to the newerFindObjectsByType<T>(FindObjectsSortMode.None)API.
632-648: LGTM!Lightweight component type collection with proper null-safety and exception handling. Using
GetType().Nameprovides concise type names suitable for the summary payload.
664-664: LGTM!The
componentTypesfield is correctly named and populated with the lightweight component type list.Server/src/services/resources/gameobject.py (6)
20-35: LGTM!Helper functions are clean and follow a consistent pattern. The tuple-based error handling in
_validate_instance_idenables clean early-return error propagation.
42-87: LGTM!Excellent self-documenting API resource. The structured documentation with workflow steps, resource descriptions, and related tools improves discoverability.
90-121: LGTM!Well-structured Pydantic models with sensible defaults. The
childrenaslist[int](instance IDs) keeps payloads lightweight while enabling traversal.
124-144: LGTM!Clean endpoint implementation with proper validation and async command handling.
165-196: LGTM!Pagination support with configurable
page_sizeandcursorenables handling GameObjects with many components. Parameter naming follows Python conventions while correctly mapping to Unity's camelCase.
211-238: LGTM!Single component retrieval endpoint is well-structured with clear URI pattern. Using
Anyfor the component type is appropriate given the dynamic nature of component data.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (4)
15-35: LGTM! Clean test infrastructure.The test setup, teardown, and helper method are well-structured. The null check in TearDown prevents errors if an object was already deleted during the test, and the tracking list ensures proper cleanup.
39-117: LGTM! Solid baseline delete tests.The basic delete tests properly verify both success and error paths. The pattern of removing successfully deleted objects from the tracking list is correct.
196-256: LGTM! Excellent hierarchical deletion tests.These tests properly verify cascading deletion behavior with comprehensive assertions. The Delete_Parent_DeletesChildren test confirms all descendants are removed, while Delete_Child_DoesNotDeleteParent ensures proper isolation.
1-6: Add UnityEditor using directive for suggested improvements.Several review comments suggest using
EditorUtility.InstanceIDToObjectfor more reliable object existence checks. This requires adding the UnityEditor namespace.using System.Collections.Generic; using NUnit.Framework; using UnityEngine; using Newtonsoft.Json.Linq; using MCPForUnity.Editor.Tools; +using UnityEditor;Note: This addition is only needed if you implement the suggested improvements in other comments for verifying inactive object deletion and duplicate name handling.
- Delete_ByTag_DeletesMatchingObjects: Verify objects are actually destroyed - Delete_ByLayer_DeletesMatchingObjects: Assert deletion using Unity null check - Delete_MultipleObjectsSameName_DeletesCorrectly: Document first-match behavior - Delete_Success_ReturnsDeletedCount: Verify count value if present All tests now verify deletion occurred rather than just checking for a result.
- Remove deprecated switch cases: find, get_components, get_component, add_component, remove_component, set_component_property - Remove deprecated wrapper methods (423 lines deleted from ManageGameObject.cs) - Delete ManageGameObjectFindTests.cs (tests deprecated 'find' action) - Remove deprecated test methods from ManageGameObjectTests.cs - Add GameObject resource URIs to README documentation - Add batch_execute performance tips to README, tool description, and gameobject_api resource - Enhance batch_execute description to emphasize 10-100x performance gains Total: ~1200 lines removed. New API (find_gameobjects, manage_components, resources) is the recommended path forward.
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs:
- Around line 372-375: The cleanup logic in ManageGameObjectDeleteTests is
inverted: it currently removes survivors from testObjects instead of removing
the deleted objects, which prevents TearDown from cleaning up remaining
GameObjects; change the conditions so that you call testObjects.Remove(target1)
and testObjects.Remove(target2) only when target1Deleted and target2Deleted are
true (i.e., remove deleted objects, keep survivors), updating the block that
references target1Deleted/target2Deleted and testObjects accordingly so TearDown
can clean up remaining objects.
- Around line 156-159: The cleanup logic in ManageGameObjectDeleteTests.cs is
inverted: instead of removing surviving objects you must remove deleted ones
from the testObjects collection so TearDown doesn't try to destroy them later;
change the conditional checks to remove target1 when target1Deleted is true and
target2 when target2Deleted is true, and update the comment to reflect "Clean up
only deleted objects from tracking"; reference the variables target1Deleted,
target2Deleted, testObjects, target1, target2 and the TearDown behavior when
making the change.
- Around line 310-312: The cleanup condition is inverted: currently it removes
the survivor when targetDeleted is false; change it to remove the deleted object
instead by inverting the check so that when targetDeleted is true you call
testObjects.Remove(target) (i.e., replace "if (!targetDeleted)
testObjects.Remove(target);" with a condition that removes target when
targetDeleted is true).
- Around line 202-207: The test only asserts that
ManageGameObject.HandleCommand(p) returned a non-null result but does not verify
that the child GameObject was actually deleted; update the test after calling
ManageGameObject.HandleCommand(p) to assert the child is removed/destroyed
(e.g., confirm testObjects no longer contains child and/or
GameObject.Find(child.name) returns null) instead of only using
Assert.IsNotNull(result) so the test fails if deletion does not occur. Ensure
you reference the existing result variable and the child object used earlier and
remove or adjust the testObjects.Remove(child) step to reflect the true deletion
check.
- Around line 184-186: The cleanup currently removes the wrong item: it calls
testObjects.Remove(target) only when targetDeleted is false, which removes the
survivor instead of the deleted object; change the logic in the block using the
targetDeleted flag so the deleted object is removed from tracking (either remove
the condition and call testObjects.Remove(target) unconditionally, or check if
(targetDeleted) testObjects.Remove(target)); update the code around the variable
names targetDeleted and testObjects in ManageGameObjectDeleteTests.cs
accordingly to match other tests' behavior.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (2)
826-858: Empty catch block silently swallows exceptions during error aggregation.Lines 833-851 use a try-catch block with an empty catch clause while extracting error information via reflection. This silently swallows any exceptions that occur during error data extraction, making debugging difficult and potentially hiding malformed error response objects.
🔎 Proposed fix to log swallowed exceptions
} - catch { } + catch (Exception ex) + { + Debug.LogWarning($"[ManageGameObject] Failed to extract error details from component error response: {ex.Message}"); + } }
1123-1129: Simplify fragile reflection for accessing deleted object data.Lines 1126-1128 use reflection (
GetProperty("name").GetValue()) to access thenameproperty from an anonymous type created earlier. This is unnecessarily complex since the anonymous type is known at compile time.🔎 Proposed simplification using typed approach
- List<object> deletedObjects = new List<object>(); + var deletedObjects = new List<(string name, int instanceID)>(); foreach (var targetGo in targets) { if (targetGo != null) { string goName = targetGo.name; int goId = targetGo.GetInstanceID(); // Use Undo.DestroyObjectImmediate for undo support Undo.DestroyObjectImmediate(targetGo); - deletedObjects.Add(new { name = goName, instanceID = goId }); + deletedObjects.Add((goName, goId)); } } if (deletedObjects.Count > 0) { string message = targets.Count == 1 - ? $"GameObject '{deletedObjects[0].GetType().GetProperty("name").GetValue(deletedObjects[0])}' deleted successfully." + ? $"GameObject '{deletedObjects[0].name}' deleted successfully." : $"{deletedObjects.Count} GameObjects deleted successfully."; return new SuccessResponse(message, deletedObjects); }
🧹 Nitpick comments (6)
README.md (1)
357-357: Add language identifier to fenced code block.The code block at line 357 is missing a language specification, which violates markdown linting rules (MD040).
🔎 Proposed fix
-``` +``` plaintext ❌ Slow: Create 5 cubes → 5 separate manage_gameobject calls ✅ Fast: Create 5 cubes → 1 batch_execute call with 5 commandsTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (1)
182-182: Simplify redundant assertion.The compound condition
testObjects.Contains(target) && target != nullis confusing. After a Unity Object is destroyed,target == nullevaluates to true (Unity's custom null check), but the reference remains intestObjects. This assertion doesn't meaningfully verify deletion beyond what line 181 already checks.🔎 Proposed simplification
bool targetDeleted = target == null; // Unity Object == null check Assert.IsTrue(targetDeleted, "Object on UI layer should be deleted"); - Assert.IsFalse(testObjects.Contains(target) && target != null, "Deleted object should not be findable");MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
324-324: Consider using Undo.DestroyObjectImmediate for consistency.Line 324 (and similar lines 380, 424, 432, 541, 555) use
UnityEngine.Object.DestroyImmediatefor cleanup in error paths. Since other parts of the code useUndo.DestroyObjectImmediatefor undo support (lines 508, 1118, 1464), consider using the Undo version consistently to ensure proper undo behavior if users attempt to undo failed operations.🔎 Proposed consistency fix
- UnityEngine.Object.DestroyImmediate(newGo); // cleanup leak + Undo.DestroyObjectImmediate(newGo); // cleanup with undo supportApply this pattern to lines 324, 380, 424, 432, 502, 541, 555 for consistency.
564-570: Simplify complex nested expression for prefab path retrieval.Lines 564-570 contain a complex nested ternary and null-coalescing expression that's difficult to read and maintain. Consider breaking this into multiple clear steps with intermediate variables.
🔎 Proposed simplification
- string messagePrefabPath = - finalInstance == null - ? originalPrefabPath - : AssetDatabase.GetAssetPath( - PrefabUtility.GetCorrespondingObjectFromSource(finalInstance) - ?? (UnityEngine.Object)finalInstance - ); + string messagePrefabPath = originalPrefabPath; + if (finalInstance != null) + { + var prefabSource = PrefabUtility.GetCorrespondingObjectFromSource(finalInstance); + var assetTarget = prefabSource ?? (UnityEngine.Object)finalInstance; + messagePrefabPath = AssetDatabase.GetAssetPath(assetTarget); + }
1915-2008: Remove redundant parse helper methods.Lines 1918-2007 contain parse helper methods (ParseJTokenToVector3, ParseJTokenToVector2, etc.) that are acknowledged as "likely redundant" (line 1915-1916) since the InputSerializer with custom converters handles these conversions. These methods duplicate functionality and increase maintenance burden.
Consider removing these methods entirely since:
- The InputSerializer (lines 26-38) already has converters for all these types
- The comment explicitly states they're redundant (line 1915)
- Keeping them creates potential inconsistencies if one implementation is updated but not the other
If you need fallback logic for some edge cases, document why it's necessary rather than leaving "likely redundant" comments.
2289-2292: Consider cache key performance for large property lists.Line 2291 constructs the cache key by joining all available properties into a single string:
string.Join(",", availableProperties). For components with many properties, this string concatenation could become expensive, especially since the cache is never cleared and could grow unbounded during an editing session.Consider one of these approaches:
- Use a hash of the property list instead of the full concatenated string
- Cache by component type rather than the full property list
- Implement cache eviction (LRU or size-based) to prevent unbounded growth
🔎 Example using hash-based cache key
- var cacheKey = $"{userInput.ToLowerInvariant()}:{string.Join(",", availableProperties)}"; + // Use hash code for properties list to avoid expensive string concatenation + var propertiesHash = string.Join(",", availableProperties).GetHashCode(); + var cacheKey = $"{userInput.ToLowerInvariant()}:{propertiesHash}";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MCPForUnity/Editor/Tools/ManageGameObject.csREADME.mdServer/src/services/resources/gameobject.pyServer/src/services/tools/batch_execute.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
💤 Files with no reviewable changes (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
Server/src/services/resources/gameobject.py (3)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/tests/integration/test_helpers.py (1)
error(46-47)
🪛 markdownlint-cli2 (0.18.1)
README.md
357-357: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (15)
Server/src/services/tools/batch_execute.py (1)
19-22: Excellent documentation improvement.The expanded description effectively communicates the performance benefits of batching and provides a concrete, actionable example. This will help users understand when and how to leverage
batch_executefor optimal performance.Server/src/services/resources/gameobject.py (5)
20-35: Well-structured validation and normalization helpers.Both helper functions follow clean patterns:
_normalize_response: Simple, consistent response wrapping_validate_instance_id: Clear tuple-based error handling that simplifies the calling code
42-92: Comprehensive API documentation resource.The static documentation provides excellent guidance:
- Clear workflow (find → read)
- Performance best practices with batch_execute examples
- Concrete URI patterns with examples
- Related tools cross-reference
This significantly improves API discoverability and usability.
95-119: Well-designed data models with appropriate defaults.The Pydantic models effectively represent Unity data structures:
TransformData: Covers all transform variants with proper defaultsGameObjectData: Comprehensive GameObject representationComponentsData: Complete pagination supportSingleComponentData: Flexible component typing withAny(necessary for varied component types)Also applies to: 152-162, 204-208
122-126: Future type safety enhancement acknowledged.The typed response classes are defined but not yet utilized. The TODO comment appropriately documents this as future work for stricter response validation.
134-149: Consistent and well-structured resource endpoints.All three endpoints follow a robust pattern:
- Consistent validation and error handling
- Proper async/await usage
- Security: Instance IDs validated as integers
- Appropriate parameter mapping (Python snake_case → Unity camelCase)
- Clear URI patterns and descriptions
The implementation demonstrates good separation of concerns with transport abstraction and centralized response normalization.
Also applies to: 175-201, 221-243
MCPForUnity/Editor/Tools/ManageGameObject.cs (9)
888-975: LGTM - DuplicateGameObject implementation is solid.The duplicate functionality is well-implemented with:
- Proper Undo integration (line 906)
- Clear name handling that removes Unity's "(Clone)" suffix (line 916)
- Flexible positioning (absolute or offset)
- Proper null parent handling (lines 935-952)
- Scene marking and selection
981-1096: LGTM - Relative movement implementation is clean and comprehensive.The relative movement functionality provides:
- Clear separation between custom offset and directional movement
- Support for both world space and local space transformations
- Multiple direction aliases (forward/front, back/backward/behind)
- Sensible default (forward) with warning for unknown directions
- Proper Undo integration
1399-1432: Excellent 2D/3D physics component conflict detection.The physics component validation (lines 1400-1432) prevents a common Unity pitfall where mixing 2D and 3D physics components on the same GameObject causes unexpected behavior. This proactive validation provides clear error messages and improves the developer experience significantly.
1562-1568: Nice UX: AI-powered property name suggestions.Lines 1562-1568 provide intelligent property suggestions when a property isn't found, using the
ComponentResolver.GetAIPropertySuggestionsmethod. This helps developers quickly correct typos and understand the available properties, improving the developer experience.
1778-1781: Smart delegation to specialized Material property handler.Line 1778-1781 correctly identify shader properties (starting with "_") and delegate to
MaterialOps.TrySetShaderProperty. This is the right design as shader properties require special handling (GetFloat, GetColor, GetTexture, etc.) that differs from standard reflection-based property setting.
2015-2112: LGTM - FindObjectByInstruction properly handles assets and scene objects.The method correctly:
- Distinguishes between asset types and scene objects (lines 2031-2040)
- Tries direct path loading before falling back to search (lines 2043-2063)
- Handles ambiguous asset matches with clear warnings (lines 2058-2062)
- Properly resolves components from GameObjects (lines 2084-2105)
- Uses flexible default search method (line 2028)
2371-2394: LGTM - Levenshtein distance implementation is correct.The Levenshtein distance implementation (lines 2371-2394) uses the standard dynamic programming approach with proper initialization and cost calculation. This provides effective fuzzy matching for property name suggestions.
114-128: The parameter structure at line 117 is correct and intentional.The
componentPropertiesparameter is designed as a nested object where component names are keys and property objects are values:{ "ComponentName": { propertyName: value, ... } }. This is confirmed by test cases inManageGameObjectTests.cs(lines 212–217, 306–310) and the component-setting logic at lines 801–806, which both use the same nested structure by iterating over component names as keys. The code at line 117 correctly accesses this structure, and the comment "Handle potential nesting" reflects this intentional design.Likely an incorrect or invalid review comment.
163-179: Removed actions are properly routed to separate tools.The removed actions (
find_gameobjects,get_components,add_component,remove_component) are correctly handled by independently registered tools:
find_gameobjects→FindGameObjects.cs([McpForUnityTool("find_gameobjects")])manage_components(coversget_components,add_component,remove_component) →ManageComponents.cs([McpForUnityTool("manage_components")])No outdated references to these actions via
manage_gameobjectwere found in the codebase. TheCommandRegistryauto-discovery mechanism properly routes each tool independently, andManageGameObjectis correctly marked withAutoRegister = false, preventing conflicts.
| // Clean up only surviving objects from tracking | ||
| if (!target1Deleted) testObjects.Remove(target1); | ||
| if (!target2Deleted) testObjects.Remove(target2); | ||
| } |
There was a problem hiding this comment.
Fix backwards cleanup logic and misleading comment.
The conditional removal logic is inverted. The code removes survivors from tracking (line 157-158) when it should remove deleted objects. The comment "Clean up only surviving objects from tracking" compounds the confusion.
Correct logic: Remove deleted objects from testObjects so TearDown doesn't attempt to destroy them again. While TearDown has a null check (line 22) that makes this safe, the current logic contradicts the stated intent and creates maintainability confusion.
🔎 Proposed fix
- // Clean up only surviving objects from tracking
- if (!target1Deleted) testObjects.Remove(target1);
- if (!target2Deleted) testObjects.Remove(target2);
+ // Remove deleted objects from tracking (TearDown will clean up survivors)
+ if (target1Deleted) testObjects.Remove(target1);
+ if (target2Deleted) testObjects.Remove(target2);🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
around lines 156-159, The cleanup logic in ManageGameObjectDeleteTests.cs is
inverted: instead of removing surviving objects you must remove deleted ones
from the testObjects collection so TearDown doesn't try to destroy them later;
change the conditional checks to remove target1 when target1Deleted is true and
target2 when target2Deleted is true, and update the comment to reflect "Clean up
only deleted objects from tracking"; reference the variables target1Deleted,
target2Deleted, testObjects, target1, target2 and the TearDown behavior when
making the change.
| // Only remove from tracking if not already destroyed | ||
| if (!targetDeleted) testObjects.Remove(target); | ||
| } |
There was a problem hiding this comment.
Fix backwards cleanup logic.
Lines 185 removes the survivor instead of the deleted object. This is inconsistent with other tests (e.g., lines 61, 86, 243-246) that unconditionally remove deleted objects from tracking.
🔎 Proposed fix
- // Only remove from tracking if not already destroyed
- if (!targetDeleted) testObjects.Remove(target);
+ // Remove deleted object from tracking (TearDown will clean up if it survived)
+ if (targetDeleted) testObjects.Remove(target);🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
around lines 184-186, The cleanup currently removes the wrong item: it calls
testObjects.Remove(target) only when targetDeleted is false, which removes the
survivor instead of the deleted object; change the logic in the block using the
targetDeleted flag so the deleted object is removed from tracking (either remove
the condition and call testObjects.Remove(target) unconditionally, or check if
(targetDeleted) testObjects.Remove(target)); update the code around the variable
names targetDeleted and testObjects in ManageGameObjectDeleteTests.cs
accordingly to match other tests' behavior.
| var result = ManageGameObject.HandleCommand(p); | ||
| // Capture current behavior | ||
| Assert.IsNotNull(result, "Should return a result"); | ||
|
|
||
| testObjects.Remove(child); | ||
| } |
There was a problem hiding this comment.
Strengthen assertion to verify actual deletion (past review not addressed).
This test still only checks Assert.IsNotNull(result) without verifying that the child object was actually deleted. The past review comment flagged this, but the issue remains unresolved. Weak assertions won't catch regressions where deletion fails silently.
🔎 Proposed fix
var result = ManageGameObject.HandleCommand(p);
- // Capture current behavior
- Assert.IsNotNull(result, "Should return a result");
+ var resultObj = result as JObject ?? JObject.FromObject(result);
+
+ Assert.IsTrue(resultObj.Value<bool>("success"), resultObj.ToString());
+
+ // Verify child was deleted (child is not a root object, so can't use Find)
+ bool childDeleted = child == null;
+ Assert.IsTrue(childDeleted, "Child should be deleted");
+
+ // Verify parent still exists
+ Assert.IsNotNull(GameObject.Find("DeleteParent"), "Parent should still exist");
testObjects.Remove(child);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
around lines 202-207, The test only asserts that
ManageGameObject.HandleCommand(p) returned a non-null result but does not verify
that the child GameObject was actually deleted; update the test after calling
ManageGameObject.HandleCommand(p) to assert the child is removed/destroyed
(e.g., confirm testObjects no longer contains child and/or
GameObject.Find(child.name) returns null) instead of only using
Assert.IsNotNull(result) so the test fails if deletion does not occur. Ensure
you reference the existing result variable and the child object used earlier and
remove or adjust the testObjects.Remove(child) step to reflect the true deletion
check.
| // Only remove from tracking if not already destroyed | ||
| if (!targetDeleted) testObjects.Remove(target); | ||
| } |
There was a problem hiding this comment.
Fix backwards cleanup logic.
Line 311 removes the survivor instead of the deleted object, inconsistent with the test's assertions. If targetDeleted is true (which is asserted on line 298), the object should be removed from tracking.
🔎 Proposed fix
- // Only remove from tracking if not already destroyed
- if (!targetDeleted) testObjects.Remove(target);
+ // Remove deleted object from tracking
+ if (targetDeleted) testObjects.Remove(target);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only remove from tracking if not already destroyed | |
| if (!targetDeleted) testObjects.Remove(target); | |
| } | |
| // Remove deleted object from tracking | |
| if (targetDeleted) testObjects.Remove(target); | |
| } |
🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
around lines 310-312, The cleanup condition is inverted: currently it removes
the survivor when targetDeleted is false; change it to remove the deleted object
instead by inverting the check so that when targetDeleted is true you call
testObjects.Remove(target) (i.e., replace "if (!targetDeleted)
testObjects.Remove(target);" with a condition that removes target when
targetDeleted is true).
| // Clean up only survivors from tracking | ||
| if (!target1Deleted) testObjects.Remove(target1); | ||
| if (!target2Deleted) testObjects.Remove(target2); | ||
| } |
There was a problem hiding this comment.
Fix backwards cleanup logic.
Lines 373-374 remove survivors from tracking instead of deleted objects. Survivors must remain in testObjects so TearDown can clean them up. This error could leave test objects persisted in the scene.
🔎 Proposed fix
- // Clean up only survivors from tracking
- if (!target1Deleted) testObjects.Remove(target1);
- if (!target2Deleted) testObjects.Remove(target2);
+ // Remove deleted objects from tracking (TearDown will clean up survivors)
+ if (target1Deleted) testObjects.Remove(target1);
+ if (target2Deleted) testObjects.Remove(target2);🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
around lines 372-375, The cleanup logic in ManageGameObjectDeleteTests is
inverted: it currently removes survivors from testObjects instead of removing
the deleted objects, which prevents TearDown from cleaning up remaining
GameObjects; change the conditions so that you call testObjects.Remove(target1)
and testObjects.Remove(target2) only when target1Deleted and target2Deleted are
true (i.e., remove deleted objects, keep survivors), updating the block that
references target1Deleted/target2Deleted and testObjects accordingly so TearDown
can clean up remaining objects.
Keep our improved performance-focused description while retaining ToolAnnotations from upstream.
Starlette is now a proper dependency via the mcp package, so we don't need to stub it anymore. The real package handles all HTTP transport needs.
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @Server/src/services/tools/manage_asset.py:
- Around line 38-50: After calling parse_json_payload(value) in the properties
parsing logic, add an explicit check for the case where parsed is not a dict
(e.g., a JSON array) and return a clear error instead of falling through to
ast.literal_eval; specifically, update the block using parsed (from
parse_json_payload) to: if isinstance(parsed, dict): return parsed, None; if
parsed is not None: return None, f"properties must be a JSON object/dict, got
{type(parsed).__name__}"; then keep the existing ast.literal_eval fallback for
when parse_json_payload returned None or failed.
In @Server/src/services/tools/manage_gameobject.py:
- Around line 105-110: Function signatures for parameters position, rotation,
scale and component_properties declare list[float] / dict[str, dict[str, Any]]
but the normalization helpers (which call parse_json_payload and accept
JSON/comma-separated strings) actually accept string inputs; update the type
hints to reflect runtime behavior by changing the annotated types to include str
(e.g., list[float] | str for position/rotation/scale and dict[str, dict[str,
Any]] | str for component_properties) or alternatively augment the Annotated
descriptions to explicitly state supported string formats (JSON or CSV) and
reference parse_json_payload usage; ensure you update all occurrences
(parameters named position, rotation, scale and component_properties in the same
function signatures mentioned) so static checkers and docs match runtime
parsing.
In @Server/src/services/tools/manage_material.py:
- Around line 97-108: The type annotations for parameters are too narrow
compared to runtime handling: update the annotations for properties, color, and
slot to include the string types your normalizers accept (e.g., change
properties: dict[str, Any] | None to dict[str, Any] | str | None; color:
list[float] | str | None; slot: int | str | None) so static types match runtime
behavior, or alternatively widen them to Union[str, dict[str, Any], None] etc.;
adjust any related Annotated declarations and keep references to the
normalization helpers (_normalize_properties, _normalize_color and coerce_int)
intact so callers and static analysis reflect actual accepted inputs.
In @Server/src/services/tools/run_tests.py:
- Around line 46-52: The decorator on the run_tests tool incorrectly marks it as
destructive; update the mcp_for_unity_tool usage so the ToolAnnotations passed
to it set destructiveHint=False (i.e., change ToolAnnotations(...,
destructiveHint=True) to ToolAnnotations(..., destructiveHint=False)) for the
function decorated by @mcp_for_unity_tool so test execution is correctly marked
non-destructive.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Server/src/services/tools/manage_script.py (3)
354-369: Async function passed toThreadtarget will not execute.
_flip_asyncis anasync deffunction, but it's passed directly tothreading.Thread(target=...). When the thread calls this function, it returns a coroutine object that is never awaited, so the actual logic inside_flip_asyncwill never run. Additionally,time.sleep(0.1)inside an async function blocks the event loop.🔎 Proposed fix using synchronous approach or proper async execution
- async def _flip_async(): + def _flip_sync(): try: time.sleep(0.1) st = _latest_status() if st and st.get("reloading"): return - await transport.legacy.unity_connection.async_send_command_with_retry( + import asyncio + asyncio.run(transport.legacy.unity_connection.async_send_command_with_retry( "execute_menu_item", {"menuPath": "MCP/Flip Reload Sentinel"}, max_retries=0, retry_ms=0, instance_id=unity_instance, - ) + )) except Exception: pass - threading.Thread(target=_flip_async, daemon=True).start() + threading.Thread(target=_flip_sync, daemon=True).start()
554-559: PotentialKeyErrorifencodedContentsis missing.The code checks for
contentsEncodedbeing truthy but assumesencodedContentsexists. If the response hascontentsEncoded: truebut noencodedContentskey, line 556 will raise aKeyError.🔎 Proposed fix with safer access
- if response.get("data", {}).get("contentsEncoded"): - decoded_contents = base64.b64decode( - response["data"]["encodedContents"]).decode('utf-8') - response["data"]["contents"] = decoded_contents - del response["data"]["encodedContents"] - del response["data"]["contentsEncoded"] + data = response.get("data", {}) + if data.get("contentsEncoded") and "encodedContents" in data: + decoded_contents = base64.b64decode( + data["encodedContents"]).decode('utf-8') + data["contents"] = decoded_contents + data.pop("encodedContents", None) + data.pop("contentsEncoded", None)
629-631: MissingAssets/path validation.Unlike
delete_scriptandvalidate_script, this function doesn't validate that the URI resolves underAssets/. For consistency and to prevent potential misuse, consider adding the same validation.🔎 Proposed fix to add path validation
try: name, directory = _split_uri(uri) + if not directory or directory.split("/")[0].lower() != "assets": + return {"success": False, "code": "path_outside_assets", "message": "URI must resolve under 'Assets/'."} params = {"action": "get_sha", "name": name, "path": directory}Server/src/services/tools/manage_gameobject.py (1)
273-288: Critical bug: prefab_folder can be None when constructing default path.Line 280 constructs the prefab path using
prefab_folder, but this parameter has no default value (line 118-119). If a caller setssaveAsPrefab=Truewithout providingprefabPathorprefab_folder, line 280 will produce"None/ObjectName.prefab", creating an invalid path.🔎 Proposed fix: Add default value and validation
Option 1: Add default value in signature (recommended):
prefab_path: Annotated[str, "Path for prefab creation"] | None = None, -prefab_folder: Annotated[str, - "Folder for prefab creation"] | None = None, +prefab_folder: Annotated[str, + "Folder for prefab creation (default: Assets/Prefabs)"] | None = "Assets/Prefabs",Option 2: Add validation before constructing path:
if action == "create" and params.get("saveAsPrefab"): if "prefabPath" not in params: + if not prefab_folder: + prefab_folder = "Assets/Prefabs" if "name" not in params or not params["name"]: return {"success": False, "message": "Cannot create default prefab path: 'name' parameter is missing."}
🧹 Nitpick comments (6)
Server/src/services/tools/manage_editor.py (1)
52-55: Consider removing commented-out parameter placeholders.The commented-out width, height, and other parameter references don't align with the "streamlining" objective mentioned in the PR title. If these are planned future extensions, consider tracking them in a TODO comment or separate issue instead.
🔎 Proposed cleanup
params = { "action": action, "waitForCompletion": wait_for_completion, "toolName": tool_name, # Corrected parameter name to match C# "tagName": tag_name, # Pass tag name "layerName": layer_name, # Pass layer name - # Add other parameters based on the action being performed - # "width": width, - # "height": height, - # etc. }Server/src/services/tools/manage_material.py (2)
4-4: Unused import.The
jsonmodule is imported but not used directly in this file; JSON parsing is handled byparse_json_payloadfrom utils.🔎 Proposed fix
-import json
50-69: Code duplication withmanage_components.py.This
_normalize_propertieshelper is nearly identical to the one inmanage_components.py(lines 15-39). Consider extracting this toutils.pyalongsideparse_json_payloadto avoid maintaining duplicate logic.Server/src/services/tools/manage_script.py (2)
295-312: Dead code in debug preview branch.The
difflibimport and initial variables (lines,prev) are never used. The function immediately returns a preview response without performing any actual diff computation.🔎 Proposed fix to remove unused code
# Support optional debug preview for span-by-span simulation without write if opts.get("debug_preview"): - try: - import difflib - # Apply locally to preview final result - lines = [] - # Build an indexable original from a read if we normalized from read; otherwise skip - prev = "" - # We cannot guarantee file contents here without a read; return normalized spans only - return { - "success": True, - "message": "Preview only (no write)", - "data": { - "normalizedEdits": normalized_edits, - "preview": True - } - } - except Exception as e: - return {"success": False, "code": "preview_failed", "message": f"debug_preview failed: {e}", "data": {"normalizedEdits": normalized_edits}} + return { + "success": True, + "message": "Preview only (no write)", + "data": { + "normalizedEdits": normalized_edits, + "preview": True + } + }
477-478: Redundant validation for type-constrained parameter.The
levelparameter is already constrained toLiteral['basic', 'standard']by its type annotation. This runtime check is defensive but redundant if type checking is enforced.This is a minor nit - keeping it for runtime safety against untyped callers is acceptable.
Server/src/services/tools/find_in_file.py (1)
79-79: Consider removing the unusedproject_rootparameter.The
project_rootparameter is declared but never used (line 84 explicitly notes this). Unused parameters in public APIs can confuse callers and add maintenance burden.If the parameter is truly needed for "interface consistency" across multiple tools in this PR, consider:
- Documenting this design decision in the PR description or architecture docs
- Implementing the parameter if it serves a purpose
- Removing it if it's not needed
Also applies to: 84-84
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Server/src/services/tools/batch_execute.pyServer/src/services/tools/debug_request_context.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/tools/execute_menu_item.pyServer/src/services/tools/find_in_file.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_prefabs.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_scriptable_object.pyServer/src/services/tools/manage_shader.pyServer/src/services/tools/read_console.pyServer/src/services/tools/refresh_unity.pyServer/src/services/tools/run_tests.pyServer/src/services/tools/script_apply_edits.pyServer/src/services/tools/set_active_instance.pyServer/src/services/tools/test_jobs.pyServer/tests/integration/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/services/tools/batch_execute.py
🧰 Additional context used
🧬 Code graph analysis (4)
Server/src/services/tools/debug_request_context.py (1)
Server/src/transport/unity_instance_middleware.py (1)
get_unity_instance_middleware(22-31)
Server/src/services/tools/manage_shader.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)
Server/src/services/tools/manage_gameobject.py (1)
Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)coerce_int(63-77)
Server/src/services/tools/manage_material.py (4)
Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)coerce_int(63-77)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(79-113)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(838-870)Server/src/services/tools/manage_components.py (1)
_normalize_properties(16-40)
🔇 Additional comments (41)
Server/src/services/tools/execute_menu_item.py (2)
7-7: LGTM!The ToolAnnotations import is correctly added and properly utilized in the decorator below.
16-22: Excellent addition of destructive hint.The
destructiveHint=Trueannotation is well-placed here. Executing arbitrary Unity menu items can perform destructive operations (saving, deleting assets, clearing data, etc.), so flagging this tool as potentially destructive appropriately warns users and downstream systems.Server/src/services/tools/read_console.py (1)
7-7: LGTM! Consistent metadata pattern applied.The addition of
ToolAnnotationswith a descriptive title enhances tool discoverability. Correctly omitsdestructiveHintsince the tool only reads console messages or clears ephemeral UI state without modifying project data.Also applies to: 23-28
Server/src/services/tools/manage_scene.py (1)
14-20: Verify tool-level destructiveHint behavior for mixed read/write operations.The
destructiveHint=Trueflag is set at the tool level, which means it applies to all actions including read-only operations (get_hierarchy, get_active, get_build_settings, screenshot). While the expanded description correctly distinguishes read-only from modifying actions, the hint may trigger unnecessary warnings for safe operations.Consider whether the MCP client handles this appropriately, or if future iterations might benefit from action-level hints rather than tool-level hints.
Server/src/services/tools/script_apply_edits.py (1)
7-7: LGTM! Appropriate destructive hint for script editing tool.The
ToolAnnotationsmetadata correctly includesdestructiveHint=Truesince this tool modifies C# script files. The comprehensive description with canonical field examples helps users understand the expected payload structure.Also applies to: 313-367
Server/src/services/tools/manage_shader.py (1)
5-5: LGTM! Consistent metadata pattern with appropriate destructive hint.The addition of
ToolAnnotationswithdestructiveHint=Trueis appropriate since the tool's primary purpose is creating, updating, and deleting shader files. The expanded description helpfully distinguishes the read-only 'read' action from modifying operations.Also applies to: 13-19
Server/src/services/tools/manage_editor.py (2)
4-4: LGTM: Import added for ToolAnnotations.The import is necessary for the decorator enhancement below and correctly references the mcp.types module.
14-19: LGTM: Enhanced tool metadata with accurate action categorization.The expanded description clearly separates read-only telemetry actions from modifying editor actions, and the ToolAnnotations title improves tool discoverability. The categorization accurately reflects the implementation logic.
Server/src/services/tools/debug_request_context.py (2)
15-21: LGTM! Metadata enhancement is appropriate.The addition of
ToolAnnotationswith a descriptive title andreadOnlyHint=Trueis semantically correct. The function is indeed read-only (it only returns context information without modifying state), making the readOnlyHint appropriate for tooling UIs.
8-8: The import and usage ofToolAnnotationswithtitleandreadOnlyHintfields are both supported in the mcp library. No changes needed.Server/src/services/tools/manage_asset.py (3)
61-64: LGTM!The
destructiveHint=Trueannotation is appropriate since this tool supports destructive actions likedelete,modify, andmove.
72-73: Type annotation doesn't match actual runtime behavior.The type annotation declares
dict[str, Any] | None, but_normalize_propertiesstill handles string inputs (JSON and Python dict literals). If strings are still accepted at runtime, consider keepingstrin the union type for accurate documentation and type checking. If string support is being deprecated, consider logging a deprecation warning when a string is received.Is the intent to deprecate string inputs? If so, add a deprecation warning:
# Try parsing as string if isinstance(value, str): + # TODO: Log deprecation warning - string inputs will be removed in future versionOr if strings should still be officially supported, update the annotation:
- properties: Annotated[dict[str, Any], - "Dictionary of properties for 'create'/'modify'. Keys are property names, values are property values."] | None = None, + properties: Annotated[dict[str, Any] | str, + "Dictionary or JSON string of properties for 'create'/'modify'. Keys are property names, values are property values."] | None = None,
98-102: LGTM!Clean refactor to use the centralized
_normalize_propertieshelper with proper error handling and logging.Server/src/services/tools/manage_material.py (3)
17-47: LGTM!The
_normalize_colorhelper provides robust validation with clear error messages, handles edge cases like sentinel strings from serialization bugs, and correctly validates both length and numeric type constraints.
114-130: LGTM!The normalization sequence is well-structured: each input is validated with clear error messages, and the function returns early on validation failures. The sentinel value checks provide good protection against serialization bugs.
132-158: LGTM!The parameter construction and command execution follow established patterns. The None-filtering and response handling are appropriate.
Server/src/services/tools/test_jobs.py (3)
7-7: LGTM: Import added for tool metadata enrichment.The import is correctly structured and used in the decorators below.
76-82: LGTM: Read-only hint correctly applied.The
readOnlyHint=Trueannotation is appropriate for a polling operation that retrieves job status without side effects. The title is clear and descriptive.
17-23: The ToolAnnotations API correctly supports bothdestructiveHintandreadOnlyHintparameters. The code in test_jobs.py is properly implemented and consistent with usage patterns throughout the codebase.destructiveHint=Trueis semantically appropriate forrun_tests_asyncas the function modifies Unity editor state during test execution.Server/src/services/tools/execute_custom_tool.py (2)
16-19: LGTM! Appropriate use of destructiveHint for custom tool execution.The annotations correctly mark this tool as potentially destructive, which is appropriate since it executes arbitrary custom tools registered by Unity. The conservative
destructiveHint=Trueprovides proper user warning without knowing the specific behavior of each custom tool.
2-2: No issues found with the ToolAnnotations import and API usage.The import
from mcp.types import ToolAnnotationsand thedestructiveHintparameter are valid in mcp 1.16.0, as confirmed by consistent usage across 20+ tool files in the codebase. The annotations applied to this tool correctly identify it as a destructive operation.Server/src/services/tools/refresh_unity.py (2)
18-24: Appropriate use of destructiveHint for this operation.The addition of
ToolAnnotationswithdestructiveHint=Trueis correct and appropriate. This tool performs potentially disruptive operations including asset database refresh, script compilation, and domain reload, which can cause Unity to disconnect or become temporarily unavailable. The warning hint will help users understand the nature of this operation.
8-8: The import is correct. ToolAnnotations is properly imported from themcp.typesmodule, which is declared in dependencies (mcp>=1.16.0), and is consistently used across the codebase.Server/src/services/tools/manage_script.py (5)
1-64: LGTM!The imports and
_split_urihelper function are well-implemented. The URI parsing correctly handlesunity://,file://, and plain paths with proper normalization for both Windows and POSIX systems. The case-insensitive Assets segment detection is appropriate for cross-platform compatibility.
377-425: LGTM!The
create_scriptfunction has solid input validation: it verifies the path is underAssets/, rejects path traversal attempts, validates the.csextension, and properly Base64-encodes contents to avoid JSON escaping issues.
428-453: LGTM!The
delete_scriptfunction correctly validates that the URI resolves underAssets/before proceeding with deletion, using the shared_split_urihelper for consistent URI parsing.
591-612: LGTM with sync caveat.The hardcoded capabilities are appropriate for client discovery. The comment at line 594 correctly notes the need to keep values in sync with the Unity-side
ManageScriptimplementation.Consider adding a TODO or mechanism to validate these capabilities against the actual Unity server at runtime, or document the sync requirement more prominently.
67-86: Good use ofToolAnnotationsfor MCP tool metadata.The consistent application of
destructiveHintandreadOnlyHintacross all tool functions correctly signals their behavior to MCP clients. This enables clients to make informed decisions about tool invocation (e.g., confirmation prompts for destructive operations).Also applies to: 377-383, 428-434, 456-462, 503-509, 577-590, 615-621
Server/src/services/tools/manage_prefabs.py (1)
4-4: LGTM! The addition ofToolAnnotationswithdestructiveHint=Trueis appropriate and properly supported.This tool correctly marks its operations as destructive since it can save, create, and overwrite prefab files (save_open_stage, create_from_gameobject, close_stage). The
ToolAnnotationsAPI in mcp supports both thetitleanddestructiveHintparameters.Server/tests/integration/conftest.py (1)
93-94: Correct the Starlette dependency source attribution.The removal of Starlette stubs is a valid architectural improvement. However, the comment incorrectly states that Starlette is available via the
mcppackage. Starlette is actually provided as a transitive dependency through FastAPI (which is a direct project dependency:fastapi>=0.104.0). FastAPI explicitly depends on Starlette, not the mcp package.Update the comment to reflect the correct dependency chain:
# Note: starlette is now a proper dependency (via fastapi), so we don't stub it anymore. # The real starlette package will be imported when needed.Likely an incorrect or invalid review comment.
Server/src/services/tools/manage_scriptable_object.py (2)
25-31: LGTM: Appropriate use of destructiveHint.The
destructiveHint=Trueis correctly set given this tool can create new assets with optional overwrite capability and modify existing ScriptableObject assets. The title is clear and descriptive.
16-16: TheToolAnnotationsimport and API usage are correct. Thetitleparameter is a valid optional string for display titles, anddestructiveHint=Trueis appropriate for this tool since it creates, modifies, and can overwrite ScriptableObject assets.Server/src/services/tools/manage_gameobject.py (3)
16-55: Robust vector normalization with excellent edge case handling.The
_normalize_vectorhelper is well-designed with comprehensive input handling (lists, tuples, JSON strings, comma-separated strings) and proper validation usingmath.isfiniteto guard against NaN and infinity values. The backward compatibility support for legacy formats is also appreciated.
58-82: Well-structured component properties normalization.The tuple return pattern
(parsed_dict, error_message)provides clear error handling, and the validation for common invalid values like"[object Object]"prevents confusing runtime failures. The error messages are actionable and guide users toward the correct format.
213-230: Excellent parameter validation prevents silent failures.The action-specific parameter validation (preventing
namewithfind, requiringsearch_termforfind, preventingsearch_termwithcreate/modify) guards against common mistakes and provides clear, actionable error messages. This significantly improves the API's usability and reduces debugging time.Server/src/services/tools/run_tests.py (1)
5-5: No issues found. TheToolAnnotationsimport frommcp.typesis correct, and the API usage withtitleanddestructiveHintparameters matches the MCP library's current specification. The pattern is consistently applied across the codebase for state-modifying tools.Server/src/services/tools/set_active_instance.py (2)
14-19: LGTM! Tool metadata enhancement improves discoverability.The addition of
ToolAnnotationswith a descriptive title aligns with the broader PR pattern of augmenting Unity-related tools with metadata. The title "Set Active Instance" clearly describes the tool's purpose without altering its runtime behavior.
5-5: Import addition correctly enables the metadata enhancement.The
ToolAnnotationsimport frommcp.typesis properly supported in mcp 1.16.0, including thetitleparameter used in the decorator. The implementation follows the documented API pattern for adding tool metadata.Server/src/services/tools/find_in_file.py (3)
117-180: LGTM! Solid regex search implementation.The search implementation is well-structured:
- Proper regex compilation with error handling
- Correct line number calculation by counting newlines
- Appropriate use of
max_resultsto cap output while reportingtotal_matches- Safe base64 decoding fallback for encoded content
The approach of finding all matches before capping is necessary to accurately report the total count, which provides useful feedback to users.
81-81: The string handling forignore_caseis necessary and correct.The type hint and conversion logic (lines 81, 119-124) properly account for MCP clients that send boolean parameters as strings. This is a documented pattern across the codebase—the code comment explicitly states "Handle ignore_case which can be boolean or string from some clients," and similar
bool | strtype hints are used throughout other tools for the same reason. No changes needed.
8-8: ToolAnnotations API usage is correct.The
ToolAnnotationsimport and usage in the decorator is valid. BothtitleandreadOnlyHintare supported properties from the MCP SDK, andreadOnlyHint=Trueappropriately indicates this is a read-only search operation with no side effects.
| # Try JSON parsing first | ||
| parsed = parse_json_payload(value) | ||
| if isinstance(parsed, dict): | ||
| return parsed, None | ||
|
|
||
| # Fallback to ast.literal_eval for Python dict literals | ||
| try: | ||
| parsed = ast.literal_eval(value) | ||
| if isinstance(parsed, dict): | ||
| return parsed, None | ||
| return None, f"properties must evaluate to a dict, got {type(parsed).__name__}" | ||
| except (ValueError, SyntaxError) as e: | ||
| return None, f"Failed to parse properties: {e}" |
There was a problem hiding this comment.
Missing error message when JSON parses to a non-dict type.
If parse_json_payload successfully parses the string but returns a non-dict (e.g., a JSON array like [1, 2, 3]), the code falls through to ast.literal_eval, which will fail with a confusing error message. Consider adding an explicit check after JSON parsing.
🔎 Proposed fix
# Try JSON parsing first
parsed = parse_json_payload(value)
if isinstance(parsed, dict):
return parsed, None
+ if parsed is not None:
+ # JSON parsed successfully but to a non-dict type
+ return None, f"properties must be a dict, got JSON {type(parsed).__name__}"
# Fallback to ast.literal_eval for Python dict literals📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Try JSON parsing first | |
| parsed = parse_json_payload(value) | |
| if isinstance(parsed, dict): | |
| return parsed, None | |
| # Fallback to ast.literal_eval for Python dict literals | |
| try: | |
| parsed = ast.literal_eval(value) | |
| if isinstance(parsed, dict): | |
| return parsed, None | |
| return None, f"properties must evaluate to a dict, got {type(parsed).__name__}" | |
| except (ValueError, SyntaxError) as e: | |
| return None, f"Failed to parse properties: {e}" | |
| # Try JSON parsing first | |
| parsed = parse_json_payload(value) | |
| if isinstance(parsed, dict): | |
| return parsed, None | |
| if parsed is not None: | |
| # JSON parsed successfully but to a non-dict type | |
| return None, f"properties must be a dict, got JSON {type(parsed).__name__}" | |
| # Fallback to ast.literal_eval for Python dict literals | |
| try: | |
| parsed = ast.literal_eval(value) | |
| if isinstance(parsed, dict): | |
| return parsed, None | |
| return None, f"properties must evaluate to a dict, got {type(parsed).__name__}" | |
| except (ValueError, SyntaxError) as e: | |
| return None, f"Failed to parse properties: {e}" |
🤖 Prompt for AI Agents
In @Server/src/services/tools/manage_asset.py around lines 38-50, After calling
parse_json_payload(value) in the properties parsing logic, add an explicit check
for the case where parsed is not a dict (e.g., a JSON array) and return a clear
error instead of falling through to ast.literal_eval; specifically, update the
block using parsed (from parse_json_payload) to: if isinstance(parsed, dict):
return parsed, None; if parsed is not None: return None, f"properties must be a
JSON object/dict, got {type(parsed).__name__}"; then keep the existing
ast.literal_eval fallback for when parse_json_payload returned None or failed.
| position: Annotated[list[float], | ||
| "Position as [x, y, z] array"] | None = None, | ||
| rotation: Annotated[list[float], | ||
| "Rotation as [x, y, z] euler angles array"] | None = None, | ||
| scale: Annotated[list[float], | ||
| "Scale as [x, y, z] array"] | None = None, |
There was a problem hiding this comment.
Type hints don't reflect runtime string handling.
The signatures declare list[float] for vectors and dict[str, dict[str, Any]] for component_properties, but the normalization helpers (lines 182-186, 203) are designed to accept and parse string inputs (JSON strings, comma-separated strings). This creates a mismatch where:
- Static type checkers will reject valid string inputs
- The API documentation suggests strings aren't supported, despite robust string parsing
Consider either:
- Updating type hints to
list[float] | strto reflect actual runtime behavior - Adding explicit documentation in the
Annotateddescriptions mentioning string format support
Based on the context from Server/src/services/tools/utils.py, the parse_json_payload function is specifically designed for "MCP clients or LLMs serialize complex objects into strings", confirming that string inputs are an expected use case.
Also applies to: 126-126, 155-155
🤖 Prompt for AI Agents
In @Server/src/services/tools/manage_gameobject.py around lines 105-110,
Function signatures for parameters position, rotation, scale and
component_properties declare list[float] / dict[str, dict[str, Any]] but the
normalization helpers (which call parse_json_payload and accept
JSON/comma-separated strings) actually accept string inputs; update the type
hints to reflect runtime behavior by changing the annotated types to include str
(e.g., list[float] | str for position/rotation/scale and dict[str, dict[str,
Any]] | str for component_properties) or alternatively augment the Annotated
descriptions to explicitly state supported string formats (JSON or CSV) and
reference parse_json_payload usage; ensure you update all occurrences
(parameters named position, rotation, scale and component_properties in the same
function signatures mentioned) so static checkers and docs match runtime
parsing.
| properties: Annotated[dict[str, Any], "Initial properties to set as {name: value} dict."] | None = None, | ||
|
|
||
| # set_material_shader_property | ||
| value: Annotated[Union[list, float, int, str, bool, None], "Value to set (color array, float, texture path/instruction)"] | None = None, | ||
| value: Annotated[list | float | int | str | bool | None, "Value to set (color array, float, texture path/instruction)"] | None = None, | ||
|
|
||
| # set_material_color / set_renderer_color | ||
| color: Annotated[Union[list[float], list[int], str], "Color as [r,g,b] or [r,g,b,a]."] | None = None, | ||
| color: Annotated[list[float], "Color as [r, g, b] or [r, g, b, a] array."] | None = None, | ||
|
|
||
| # assign_material_to_renderer / set_renderer_color | ||
| target: Annotated[str, "Target GameObject (name, path, or find instruction)"] | None = None, | ||
| search_method: Annotated[Literal["by_name", "by_path", "by_tag", "by_layer", "by_component"], "Search method for target"] | None = None, | ||
| slot: Annotated[int | str, "Material slot index"] | None = None, | ||
| slot: Annotated[int, "Material slot index (0-based)"] | None = None, |
There was a problem hiding this comment.
Type annotations don't match runtime behavior.
The type hints are stricter than what the function actually accepts:
properties: dict[str, Any]but_normalize_propertieshandlesstrinputcolor: list[float]but_normalize_colorhandlesstrinputslot: intbutcoerce_inthandlesstrinput
This mismatch can confuse callers and cause static analysis false positives. Consider using Union types to reflect actual acceptance, e.g., dict[str, Any] | str for properties.
🔎 Proposed fix
- properties: Annotated[dict[str, Any], "Initial properties to set as {name: value} dict."] | None = None,
+ properties: Annotated[dict[str, Any] | str, "Initial properties to set as {name: value} dict or JSON string."] | None = None,
# set_material_shader_property
value: Annotated[list | float | int | str | bool | None, "Value to set (color array, float, texture path/instruction)"] | None = None,
# set_material_color / set_renderer_color
- color: Annotated[list[float], "Color as [r, g, b] or [r, g, b, a] array."] | None = None,
+ color: Annotated[list[float] | str, "Color as [r, g, b] or [r, g, b, a] array, or JSON string."] | None = None,
# assign_material_to_renderer / set_renderer_color
target: Annotated[str, "Target GameObject (name, path, or find instruction)"] | None = None,
search_method: Annotated[Literal["by_name", "by_path", "by_tag", "by_layer", "by_component"], "Search method for target"] | None = None,
- slot: Annotated[int, "Material slot index (0-based)"] | None = None,
+ slot: Annotated[int | str, "Material slot index (0-based), accepts string."] | None = None,🤖 Prompt for AI Agents
In @Server/src/services/tools/manage_material.py around lines 97-108, The type
annotations for parameters are too narrow compared to runtime handling: update
the annotations for properties, color, and slot to include the string types your
normalizers accept (e.g., change properties: dict[str, Any] | None to dict[str,
Any] | str | None; color: list[float] | str | None; slot: int | str | None) so
static types match runtime behavior, or alternatively widen them to Union[str,
dict[str, Any], None] etc.; adjust any related Annotated declarations and keep
references to the normalization helpers (_normalize_properties, _normalize_color
and coerce_int) intact so callers and static analysis reflect actual accepted
inputs.
| @mcp_for_unity_tool( | ||
| description="Runs Unity tests synchronously (blocks until complete). Prefer run_tests_async for non-blocking execution with progress polling." | ||
| description="Runs Unity tests synchronously (blocks until complete). Prefer run_tests_async for non-blocking execution with progress polling.", | ||
| annotations=ToolAnnotations( | ||
| title="Run Tests", | ||
| destructiveHint=True, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MCP ToolAnnotations destructiveHint parameter meaning
💡 Result:
"destructiveHint" indicates whether a tool may perform destructive updates to its environment. If true, the tool can delete or otherwise irreversibly change state; if false, the tool should perform only additive/non‑destructive updates. This annotation is only meaningful for non‑read‑only tools (i.e., when readOnlyHint == false). Default is true. [1][2][3]
Change destructiveHint to False for test execution.
The destructiveHint=True annotation is incorrect for this tool. According to MCP semantics, destructiveHint indicates whether a tool may perform destructive updates to its environment (i.e., delete or irreversibly change state). Running tests should be non-destructive and self-cleaning—they should not permanently alter state. Set destructiveHint=False to accurately reflect that test execution is a safe, non-destructive operation.
🤖 Prompt for AI Agents
In @Server/src/services/tools/run_tests.py around lines 46-52, The decorator on
the run_tests tool incorrectly marks it as destructive; update the
mcp_for_unity_tool usage so the ToolAnnotations passed to it set
destructiveHint=False (i.e., change ToolAnnotations(..., destructiveHint=True)
to ToolAnnotations(..., destructiveHint=False)) for the function decorated by
@mcp_for_unity_tool so test execution is correctly marked non-destructive.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.