🎮 GameObject Toolset Redesign and Streamlining #518
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
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
- 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
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)
- GameObjectLookup.SearchByPath: Document and warn that includeInactive has no effect (Unity API limitation) - ManageComponents.TrySetProperty: Document case-insensitive lookup behavior
Reviewer's GuideRedesigns the GameObject API into focused tools/resources (find_gameobjects, manage_components, read-only GameObject resources), introduces shared lookup/coercion helpers, tightens parameter normalization for existing tools, and wires a dedicated CI GameObject NL test suite plus extensive Unity/pytest coverage to lock in behavior and deprecation paths. Sequence diagram for searching and then reading GameObject datasequenceDiagram
actor LLM as "LLM_or_Claude"
participant PyFindTool as "find_gameobjects_tool"
participant UnityTransport as "unity_transport"
participant CSFind as "FindGameObjects_tool"
participant Lookup as "GameObjectLookup"
participant PyGORes as "gameobject_resource"
participant CSGORes as "GameObjectResource"
LLM->>PyFindTool: find_gameobjects(search_term, search_method)
PyFindTool->>UnityTransport: send_with_unity_instance("find_gameobjects", params)
UnityTransport->>CSFind: HandleCommand(params)
CSFind->>Lookup: SearchGameObjects(searchMethod, searchTerm, includeInactive, maxResults)
Lookup-->>CSFind: List instanceIDs
CSFind-->>UnityTransport: {success, data.instanceIDs, pagination}
UnityTransport-->>PyFindTool: response
PyFindTool-->>LLM: instanceIDs
LLM->>PyGORes: read_resource unity://scene/gameobject/{instance_id}
PyGORes->>UnityTransport: send_with_unity_instance("get_gameobject", {instanceID})
UnityTransport->>CSGORes: HandleCommand(params)
CSGORes-->>UnityTransport: {success, data GameObject summary}
UnityTransport-->>PyGORes: response
PyGORes-->>LLM: GameObjectData (name, tag, layer, transform, componentTypes)
Sequence diagram for adding and modifying a component with manage_componentssequenceDiagram
actor LLM as "LLM_or_Claude"
participant PyCompTool as "manage_components_tool"
participant UnityTransport as "unity_transport"
participant CSManageComp as "ManageComponents_tool"
participant Lookup as "GameObjectLookup"
participant Param as "ParamCoercion"
participant VecParse as "VectorParsing"
LLM->>PyCompTool: manage_components(action="add", target, component_type, properties)
PyCompTool->>UnityTransport: send_with_unity_instance("manage_components", params)
UnityTransport->>CSManageComp: HandleCommand(params)
CSManageComp->>Lookup: FindTarget(targetToken, searchMethod)
Lookup-->>CSManageComp: GameObject
CSManageComp->>Lookup: FindComponentType(componentType)
CSManageComp-->>UnityTransport: {success, data.componentInstanceID}
UnityTransport-->>PyCompTool: response
PyCompTool-->>LLM: component added summary
LLM->>PyCompTool: manage_components(action="set_property", properties={mass:5.0})
PyCompTool->>UnityTransport: send_with_unity_instance("manage_components", params)
UnityTransport->>CSManageComp: HandleCommand(params)
CSManageComp->>Lookup: FindTarget(targetToken, searchMethod)
Lookup-->>CSManageComp: GameObject
CSManageComp->>Param: CoerceString(propertyName)
CSManageComp->>VecParse: ConvertValue for Vector or Color properties
CSManageComp-->>UnityTransport: {success, message}
UnityTransport-->>PyCompTool: response
PyCompTool-->>LLM: property set summary
Class diagram for new and updated Unity C# GameObject API typesclassDiagram
%% Tools
class ManageGameObject {
+HandleCommand(params JObject) object
-CreateGameObject(params JObject) object
-ModifyGameObject(params JObject, JToken, string) object
-DeleteGameObject(JToken, string) object
-DuplicateGameObject(params JObject, JToken, string) object
-MoveRelativeToObject(params JObject, JToken, string) object
-FindGameObjects(params JObject, JToken, string) object
-GetComponentsFromTarget(string, string, bool, int, int, int, bool) object
-GetSingleComponentFromTarget(string, string, string, bool) object
-AddComponentToTarget(JObject, JToken, string) object
-RemoveComponentFromTarget(JObject, JToken, string) object
-SetComponentPropertyOnTarget(JObject, JToken, string) object
}
class FindGameObjects {
+HandleCommand(params JObject) object
}
class ManageComponents {
+HandleCommand(params JObject) object
-AddComponent(JObject, JToken, string) object
-RemoveComponent(JObject, JToken, string) object
-SetProperty(JObject, JToken, string) object
-FindTarget(JToken, string) GameObject
-FindComponentType(string) Type
-SetPropertiesOnComponent(Component, JObject) void
-TrySetProperty(Component, string, JToken) string
-ConvertValue(JToken, Type) object
}
%% Resources
class GameObjectResource {
+HandleCommand(params JObject) object
+SerializeGameObject(GameObject) object
-SerializeVector3(Vector3) object
}
class GameObjectComponentsResource {
+HandleCommand(params JObject) object
}
class GameObjectComponentResource {
+HandleCommand(params JObject) object
}
class GameObjectSerializer {
}
%% Helpers
class GameObjectLookup {
<<static>>
+SearchMethod
+ParseSearchMethod(string) SearchMethod
+FindByTarget(JToken, string, bool) GameObject
+FindById(int) GameObject
+SearchGameObjects(string, string, bool, int) List~int~
+SearchGameObjects(SearchMethod, string, bool, int) List~int~
+GetAllSceneObjects(bool) IEnumerable~GameObject~
+GetGameObjectPath(GameObject) string
+FindComponentType(string) Type
}
class ParamCoercion {
<<static>>
+CoerceInt(JToken, int) int
+CoerceBool(JToken, bool) bool
+CoerceFloat(JToken, float) float
+CoerceString(JToken, string) string
+CoerceEnum~T~(JToken, T) T
}
class VectorParsing {
<<static>>
+ParseVector3(JToken) Vector3?
+ParseVector3OrDefault(JToken, Vector3) Vector3
+ParseVector2(JToken) Vector2?
+ParseQuaternion(JToken, bool) Quaternion?
+ParseColor(JToken) Color?
+ParseRect(JToken) Rect?
+ParseBounds(JToken) Bounds?
}
class ManageScene {
-BuildGameObjectSummary(GameObject, bool, int) object
}
%% Relationships
ManageGameObject ..> GameObjectLookup : uses
ManageGameObject ..> ParamCoercion : uses
FindGameObjects ..> GameObjectLookup : uses
FindGameObjects ..> ParamCoercion : uses
ManageComponents ..> GameObjectLookup : uses
ManageComponents ..> ParamCoercion : uses
ManageComponents ..> VectorParsing : uses
GameObjectResource ..> GameObjectLookup : uses
GameObjectComponentsResource ..> GameObjectSerializer : uses
GameObjectComponentResource ..> GameObjectSerializer : uses
ManageScene ..> ParamCoercion : uses
ManageScene ..> GameObjectResource : exposes_componentTypes
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds a comprehensive GameObject API test suite and management infrastructure, introducing new tools for GameObject search (find_gameobjects) and component management (manage_components), along with corresponding read-only resources (gameobject, gameobject_components, gameobject_component). Includes C# editor utilities, Python server implementations, CI/CD workflow orchestration, and extensive test coverage. Updates tool identifiers from legacy Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Claude
participant PyServer as Python Server<br/>(MCP Dispatcher)
participant FindGO as find_gameobjects<br/>Tool
participant UnityTransport as Unity Transport<br/>(MCP Bridge)
participant CSharpLookup as C# GameObjectLookup<br/>Editor Tool
participant Scene as Unity Scene<br/>(Active)
Client->>PyServer: search_term, search_method,<br/>pagination (cursor, pageSize)
PyServer->>FindGO: validate inputs,<br/>coerce parameters
FindGO->>UnityTransport: async_send_command<br/>("find_gameobjects", params)
UnityTransport->>CSharpLookup: HandleCommand(params)
CSharpLookup->>Scene: GetAllSceneObjects()<br/>SearchByName/Tag/Layer/etc.
Scene-->>CSharpLookup: List of<br/>matching GameObjects
CSharpLookup->>CSharpLookup: Extract instanceIDs,<br/>apply pagination
CSharpLookup-->>UnityTransport: { instanceIDs,<br/>pageSize, nextCursor,<br/>totalCount, hasMore }
UnityTransport-->>FindGO: response dict
FindGO->>FindGO: normalize response,<br/>apply error handling
FindGO-->>PyServer: success payload<br/>or error
PyServer-->>Client: structured result<br/>(instanceIDs + pagination)
Key flows:
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the Python tool signatures (e.g. manage_gameobject.position/rotation/scale and manage_material.color), the type hints now advertise only list[...] inputs but the normalization helpers still accept strings; consider widening the annotations back to Union[...] to reflect actual accepted inputs and avoid schema/validation mismatches for existing clients.
- The _normalize_properties helper logic is now duplicated (and slightly diverging) across manage_asset, manage_material, and manage_components; consider centralizing this into a shared utility to keep behavior consistent and reduce future maintenance overhead.
- In ParamCoercion.CoerceInt/CoerceBool/CoerceFloat you silently swallow all exceptions and return defaults; it might be worth at least logging unexpected parse failures (at debug level) so mis-shaped parameters are diagnosable without relying solely on higher-level error messages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Python tool signatures (e.g. manage_gameobject.position/rotation/scale and manage_material.color), the type hints now advertise only list[...] inputs but the normalization helpers still accept strings; consider widening the annotations back to Union[...] to reflect actual accepted inputs and avoid schema/validation mismatches for existing clients.
- The _normalize_properties helper logic is now duplicated (and slightly diverging) across manage_asset, manage_material, and manage_components; consider centralizing this into a shared utility to keep behavior consistent and reduce future maintenance overhead.
- In ParamCoercion.CoerceInt/CoerceBool/CoerceFloat you silently swallow all exceptions and return defaults; it might be worth at least logging unexpected parse failures (at debug level) so mis-shaped parameters are diagnosable without relying solely on higher-level error messages.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/manage_material.py:102-111` </location>
<code_context>
+ slot: Annotated[int, "Material slot index (0-based)"] | None = None,
</code_context>
<issue_to_address>
**issue:** Coercing `slot` without an explicit default may turn a missing slot into index 0 unintentionally.
Previously, omitting `slot` or passing `None` preserved `None`, allowing Unity to distinguish “no slot specified” from “use slot 0”. With `slot = coerce_int(slot)`, `None` will likely become `0`, unintentionally using the first material slot. Please ensure `None` stays `None` (e.g. `coerce_int(slot, default=None)` and adjust `coerce_int` if needed) so only explicit values are coerced.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| slot: Annotated[int, "Material slot index (0-based)"] | None = None, | ||
| mode: Annotated[Literal["shared", "instance", "property_block"], "Assignment/modification mode"] | None = None, | ||
|
|
||
| ) -> dict[str, Any]: | ||
| unity_instance = get_unity_instance_from_context(ctx) | ||
|
|
||
| # Parse inputs that might be stringified JSON | ||
| color = parse_json_payload(color) | ||
| properties = parse_json_payload(properties) | ||
| # --- Normalize color with validation --- | ||
| color, color_error = _normalize_color(color) | ||
| if color_error: | ||
| return {"success": False, "message": color_error} |
There was a problem hiding this comment.
issue: Coercing slot without an explicit default may turn a missing slot into index 0 unintentionally.
Previously, omitting slot or passing None preserved None, allowing Unity to distinguish “no slot specified” from “use slot 0”. With slot = coerce_int(slot), None will likely become 0, unintentionally using the first material slot. Please ensure None stays None (e.g. coerce_int(slot, default=None) and adjust coerce_int if needed) so only explicit values are coerced.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @.github/workflows/claude-gameobject-suite.yml:
- Around line 390-391: The workflow uses invalid model identifiers; replace the
incorrect values "claude-haiku-4-5-20251001" and "claude-sonnet-4-5-20250929"
with valid Anthropic model names; update the model key to a valid identifier
such as "claude-haiku-4.5" and the fallback_model key to "claude-sonnet-4.5" (or
other confirmed Anthropic versioned names) so the model and fallback_model
entries use the proper period-delimited version format.
🧹 Nitpick comments (17)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (1)
400-410: Consider tightening the assertion for better pagination validation.The assertion
Assert.GreaterOrEqual(items.Count, 1)is quite loose given you create 50 objects and requestpageSize = 20. This could mask pagination bugs where only a single item is returned. Consider a more meaningful lower bound based on expected scene content.🔎 Suggested improvement
Assert.IsNotNull(items); - Assert.GreaterOrEqual(items.Count, 1); + // With pageSize=20 and 50+ created objects, first page should be full + Assert.AreEqual(20, items.Count, "First page should contain pageSize items when more exist");MCPForUnity/Editor/Helpers/ParamCoercion.cs (1)
33-37: Consider using InvariantCulture for double parsing consistency.
int.TryParseanddouble.TryParseat lines 33-37 use the current culture, whileCoerceFloat(line 104) correctly usesCultureInfo.InvariantCulture. For consistency and to avoid locale-dependent parsing issues (e.g., comma vs. period decimal separators), consider:🔎 Proposed fix
- if (int.TryParse(s, out var i)) + if (int.TryParse(s, NumberStyles.Integer, CultureInfo.InvariantCulture, out var i)) return i; - if (double.TryParse(s, out var d)) + if (double.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out var d)) return (int)d;Server/src/services/tools/manage_asset.py (1)
18-50: Consider consolidating_normalize_propertiesinto a shared utility.This helper is duplicated across
manage_asset.py,manage_components.py, andmanage_material.pywith subtle differences:
- This version returns
{}forNone(line 24), while others returnNone- This version includes an
ast.literal_evalfallback (lines 42-48), others don'tConsider extracting to
services/tools/utils.pyfor consistency and maintainability.🔎 Proposed centralized implementation
Add to
Server/src/services/tools/utils.py:def normalize_properties( value: Any, *, default_on_none: dict[str, Any] | None = None, allow_ast_eval: bool = False, ) -> tuple[dict[str, Any] | None, str | None]: """ Robustly normalize properties parameter to a dict. Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. """ if value is None: return default_on_none, None if isinstance(value, dict): return value, None if isinstance(value, str): if value in ("[object Object]", "undefined", "null", ""): return None, f"properties received invalid value: '{value}'. Expected a JSON object" parsed = parse_json_payload(value) if isinstance(parsed, dict): return parsed, None if allow_ast_eval: try: import ast parsed = ast.literal_eval(value) if isinstance(parsed, dict): return parsed, None except (ValueError, SyntaxError): pass return None, f"properties must be a dict or JSON string, got {type(parsed).__name__}" return None, f"properties must be a dict or JSON string, got {type(value).__name__}"Server/src/services/tools/manage_components.py (1)
155-156: Consider narrowing the exception type.The static analyzer flags the blind
Exceptioncatch. While defensive error handling is appropriate here, consider catching more specific exceptions or at minimum logging the exception type for debugging:🔎 Proposed improvement
except Exception as e: - return {"success": False, "message": f"Error managing component: {e!s}"} + import logging + logging.getLogger(__name__).exception("manage_components failed") + return {"success": False, "message": f"Error managing component: {e!s}"}Or narrow to expected exceptions if the failure modes are known.
Server/src/services/tools/manage_material.py (1)
48-67: Duplicate_normalize_propertiesimplementation.This is the third copy of
_normalize_properties(also inmanage_asset.pyandmanage_components.py). As noted in themanage_asset.pyreview, consider consolidating intoservices/tools/utils.py.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (2)
42-43: Unused variableinstanceID.The variable
instanceIDis declared but never used in this test method. Consider removing it or using it in an assertion if it was intended for verification.public void Delete_ByName_DeletesObject() { var target = CreateTestObject("DeleteTargetByName"); - int instanceID = target.GetInstanceID(); var p = new JObject
124-150: Consider adding explicit assertion for delete behavior with multiple objects.The test
Delete_ByTag_DeletesMatchingObjectscreates two objects with the same tag but only asserts that a result is returned. The comment notes "may delete one or all" — consider adding a follow-up assertion to verify the actual count deleted, which would make the test more valuable for catching behavioral regressions.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
447-449: Simplify the instanceID assertion logic.The current assertion has redundant conditions. If
instanceID.HasValueis false, theninstanceID > 0will also be false (comparing null). The fallback to string matching is a reasonable safety net, but the logic can be cleaner:// Check that instanceID is returned var instanceID = data["instanceID"]?.Value<int>() ?? data["InstanceID"]?.Value<int>(); - Assert.IsTrue(instanceID.HasValue || instanceID > 0 || data.ToString().Contains("instanceID") || data.ToString().Contains("InstanceID"), + Assert.IsTrue(instanceID.HasValue && instanceID > 0 || data.ToString().Contains("instanceID") || data.ToString().Contains("InstanceID"), "Response should include instanceID in some form");Or consider simplifying to just check if instanceID exists and is positive:
Assert.IsTrue(instanceID > 0, "Response should include a positive instanceID");Server/src/services/tools/find_gameobjects.py (1)
83-84: Broad exception handling is acceptable here.While the static analysis tool flags
Exceptionas too broad, this is a reasonable pattern for a top-level tool handler that needs to return a structured error response rather than propagate exceptions. The exception message is included in the response, preserving debugging information.For more detailed diagnostics, consider logging the full traceback while still returning the user-friendly error:
except Exception as e: + import logging + logging.exception("Error searching GameObjects") return {"success": False, "message": f"Error searching GameObjects: {e!s}"}MCPForUnity/Editor/Tools/ManageComponents.cs (2)
92-106: Undo.RecordObject before Undo.AddComponent is redundant.
Undo.AddComponentalready creates a proper undo record for the added component. TheUndo.RecordObjectcall on line 94 is unnecessary and records the GameObject state before the component is added, which doesn't contribute to the undo operation.Additionally,
SetPropertiesOnComponentsilently logs warnings for failed property assignments but doesn't propagate errors back to the caller. Consider returning a result or accumulating errors to include in the response.🔎 Suggested improvement
try { - Undo.RecordObject(targetGo, $"Add {componentType}"); Component newComponent = Undo.AddComponent(targetGo, type); if (newComponent == null) { return new ErrorResponse($"Failed to add component '{componentType}' to '{targetGo.name}'."); } // Set properties if provided + List<string> propertyErrors = null; if (properties != null && properties.HasValues) { - SetPropertiesOnComponent(newComponent, properties); + propertyErrors = SetPropertiesOnComponent(newComponent, properties); } EditorUtility.SetDirty(targetGo); return new { success = true, message = $"Component '{componentType}' added to '{targetGo.name}'.", data = new { instanceID = targetGo.GetInstanceID(), componentType = type.FullName, - componentInstanceID = newComponent.GetInstanceID() + componentInstanceID = newComponent.GetInstanceID(), + propertyWarnings = propertyErrors?.Count > 0 ? propertyErrors : null } };
390-415: Inconsistent null-coalescing for parsed Unity types may mask failures.When parsing fails,
Vector2,Quaternion, andColorsilently fall back to default values (Vector2.zero,Quaternion.identity,Color.white), while anulltoken returnsnull. This inconsistency could mask parsing errors - the caller won't know if the default value was intentional or a parsing failure.Consider either:
- Returning
nullon parse failure and letting the caller decide the default, or- Logging a warning when falling back to defaults
MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
50-74: Performance consideration: fetching all results before pagination.The current implementation fetches all matching IDs (
maxResults = 0) before applying pagination. For large scenes with many matching objects (e.g., searching by a common tag or layer), this loads the entire result set into memory just to return a single page.This is acceptable for typical use cases, but consider documenting this behavior or adding a warning if
totalCountexceeds a threshold. Alternatively, for very large result sets, you could stream results with early termination oncecursor + pageSizeis reached, though this would sacrifice the accuratetotalCount.MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs (1)
244-259: Consider documenting behavior when multiple components of the same type exist.The current implementation returns the first matching component. If a GameObject has multiple components of the same type (e.g., multiple
AudioSourcecomponents), only the first one is returned. This is reasonable behavior, but consider adding a note in the response or documentation.Server/src/services/tools/manage_gameobject.py (1)
99-104: Type annotations may mislead callers.The parameter type annotations (
list[float]) now reflect the normalized output type rather than the accepted input types. The function still accepts string inputs (JSON strings, comma-separated values) that_normalize_vectorparses, but the type hints suggest onlylist[float]is valid.This could confuse callers and type checkers. Consider using
Union[list[float], str]to accurately document accepted inputs, or add a docstring clarifying the normalization behavior.Also applies to: 149-150
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
297-316: Empty catch block could hide unexpected errors.The empty catch block on lines 312-315 suppresses all exceptions during assembly type lookup. While this is necessary to handle assemblies that throw during reflection (e.g., dynamic assemblies, unloadable assemblies), catching all exceptions could hide unexpected issues.
Consider catching specific exceptions or adding a trace-level log for debugging purposes.
🔎 Suggested improvement
try { // Try exact match type = assembly.GetType(typeName); if (type != null && typeof(Component).IsAssignableFrom(type)) return type; // Try with UnityEngine prefix type = assembly.GetType($"UnityEngine.{typeName}"); if (type != null && typeof(Component).IsAssignableFrom(type)) return type; } - catch + catch (Exception) { - // Skip assemblies that can't be searched + // Skip assemblies that can't be searched (e.g., dynamic, reflection-only) + // This is expected for some assemblies in Unity's domain }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
238-254: Consider strengthening assertions in "handles gracefully" tests.Tests like
Modify_NameToEmpty_HandlesGracefully,Modify_ParentToNull_UnparentsObject, andModify_ParentToNonExistent_HandlesGracefullyonly assert that a result is returned (Assert.IsNotNull). While the comments indicate these capture current behavior, consider adding assertions for the expected success/failure state to prevent regressions.Also applies to: 279-311
Server/src/services/resources/gameobject.py (1)
118-134: Consider extracting instance_id validation to reduce duplication.The instance_id validation logic (try/except ValueError with error MCPResponse) is identical in all three endpoints (lines 122-125, 170-173, 215-218). Extracting this to a shared helper function would improve maintainability.
🔎 Proposed refactor to extract validation helper
Add a validation helper at the module level:
+def _validate_instance_id(instance_id: str) -> tuple[int | None, MCPResponse | None]: + """ + Validate and convert instance_id string to int. + Returns (id_int, None) on success or (None, error_response) on failure. + """ + try: + return int(instance_id), None + except ValueError: + return None, MCPResponse(success=False, error=f"Invalid instance ID: {instance_id}") + + def _normalize_response(response: dict | Any) -> MCPResponse:Then simplify each endpoint:
async def get_gameobject(ctx: Context, instance_id: str) -> MCPResponse: """Get GameObject data by instance ID.""" unity_instance = get_unity_instance_from_context(ctx) - try: - id_int = int(instance_id) - except ValueError: - return MCPResponse(success=False, error=f"Invalid instance ID: {instance_id}") + id_int, error = _validate_instance_id(instance_id) + if error: + return error response = await send_with_unity_instance(Apply the same pattern to
get_gameobject_componentsandget_gameobject_component.Also applies to: 160-187, 207-230
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
.claude/prompts/nl-gameobject-suite.md.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.md.github/workflows/claude-gameobject-suite.yml.github/workflows/claude-nl-suite.yml.gitignoreMCPForUnity/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_asset.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/tests/integration/test_find_gameobjects.pyServer/tests/integration/test_gameobject_resources.pyServer/tests/integration/test_manage_components.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/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
🧠 Learnings (9)
📓 Common learnings
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs.metaMCPForUnity/Editor/Tools/ManageComponents.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Tools/ManageComponents.cs.metaMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Tools/FindGameObjects.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs
📚 Learning: 2025-12-29T15:23:17.871Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs.claude/prompts/nl-unity-suite-t.md
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Helpers/VectorParsing.csMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Tools/FindGameObjects.cs
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Helpers/ParamCoercion.cs.metaMCPForUnity/Editor/Tools/ManageComponents.cs.meta.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.mdMCPForUnity/Editor/Helpers/VectorParsing.cs.metaMCPForUnity/Editor/Helpers/GameObjectLookup.cs.meta
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Tools/ManageComponents.csServer/src/services/tools/manage_components.pyMCPForUnity/Editor/Tools/ManageComponents.cs.meta.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.md
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
Applied to files:
MCPForUnity/Editor/Tools/ManageComponents.cs.meta.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.mdMCPForUnity/Editor/Helpers/VectorParsing.cs.meta
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
.claude/prompts/nl-unity-suite-nl.md.claude/prompts/nl-unity-suite-t.mdServer/src/services/resources/gameobject.py
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
.claude/prompts/nl-unity-suite-nl.md
🧬 Code graph analysis (11)
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(118-134)get_gameobject_components(160-187)get_gameobject_component(207-230)
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(46-156)
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_asset.py (1)
_normalize_properties(18-50)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (3)
ParamCoercion(11-158)CoerceInt(19-45)CoerceBool(53-82)
Server/src/services/tools/manage_material.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/src/services/tools/manage_components.py (1)
_normalize_properties(16-40)
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/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)
Server/src/services/tools/find_gameobjects.py (4)
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/src/services/tools/manage_asset.py (3)
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/utils.py (1)
parse_json_payload(27-60)
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/ManageGameObjectFindTests.cs (6)
MCPForUnity/Editor/Tools/ManageGameObject.cs (11)
UnityEngine(2438-2535)List(1617-1776)List(2624-2661)List(2684-2702)List(2707-2746)List(2754-2789)GameObject(1589-1612)HandleCommand(42-234)Vector3(1110-1144)Vector3(1566-1584)Vector3(2341-2356)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
List(82-86)List(96-135)GameObject(57-64)GameObject(69-72)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (2)
GameObject(30-35)TearDown(17-28)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (2)
GameObject(41-46)TearDown(28-39)MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
HandleCommand(24-81)MCPForUnity/Editor/Helpers/VectorParsing.cs (2)
Vector3(18-51)Vector3(56-59)
🪛 Ruff (0.14.10)
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/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/src/services/tools/manage_components.py
155-155: Do not catch blind exception: Exception
(BLE001)
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)
Server/src/services/tools/find_gameobjects.py
83-83: 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: Sourcery review
🔇 Additional comments (68)
.gitignore (1)
45-53: LGTM! ✓The three new ignore entries are appropriately scoped and well-documented. They align with the PR's introduction of new CI workflows (test reports), local Claude configurations, and testing infrastructure. The patterns are narrow, consistent with the file's existing style, and unlikely to inadvertently suppress legitimate tracked files.
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (3)
23-26: LGTM! Performance optimization for animator parameters.Using pre-computed hashes for animator parameters is a Unity best practice that eliminates repeated string-to-hash conversions at runtime.
107-112: LGTM! Correctly applies the hash-based parameter access.The method now uses the pre-computed BlendXHash and BlendYHash fields, which improves performance by avoiding string-to-hash conversion on each call. The safety null-check is appropriately retained.
760-773: LGTM! Test scaffolding for NL/T editing tests.The test markers and local helper functions are appropriate additions for testing Claude's code editing capabilities. The local functions (C# 7.0+) are supported in modern Unity versions.
MCPForUnity/Editor/Tools/ManageScene.cs (2)
632-649: LGTM! Component type collection is well-implemented.The implementation correctly collects component type names with appropriate null checks and error handling. The lightweight approach (type names only) aligns well with the PR's goal of reducing payload sizes while providing useful metadata for LLM tool-calling.
664-664: LGTM! Field addition is consistent and well-placed.The
componentTypesfield is appropriately added to the GameObject summary payload, following the established naming conventions and structure. This provides useful lightweight metadata that complements the existing fields.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (8)
1-58: LGTM - Well-structured test infrastructure.The test setup is clean with proper lifecycle management via
SetUp/TearDown, a helper for object tracking, and appropriate imports. TheCreateTestObjecthelper ensures all created objects are tracked for cleanup.
62-117: LGTM - Bulk creation tests with reasonable performance thresholds.The bulk creation tests properly track objects for cleanup and validate success. The timing thresholds (5s for 10 objects, 15s for 50 objects) are generous enough to accommodate CI variability while still catching severe performance regressions.
123-235: LGTM - Solid pagination tests with good isolation strategy.Using the
GameObjectAPIStressTestMarkercomponent to isolate searches ensures tests don't interfere with pre-existing scene objects. The safety limit on page iteration (line 230) prevents infinite loops. Cursor tracking and validation logic is correct.
241-343: LGTM - Component operations thoroughly tested.Good coverage of add, get, and set operations. Float comparisons correctly use tolerance values (0.01f). The comment explaining the AudioSource skip due to deprecated API warnings is helpful documentation.
349-380: LGTM - Deep hierarchy path search test.Correctly creates a 7-level hierarchy (root + 5 levels + target) and validates the full path search returns the expected target object by instance ID.
416-515: LGTM - Resource read tests with thorough property validation.Both tests validate the structure of returned data including nested properties. The
ComponentsResource_ReadAllWithFullSerializationtest properly verifies the serialized Rigidbody mass value, confirming the full serialization pipeline works.
521-563: LGTM - Rapid-fire lifecycle test.Good coverage of the create-modify-delete cycle. Since objects are deleted within the loop, they won't accumulate, which is appropriate for this stress test pattern.
568-572: LGTM - Marker component is well-documented.The sealed marker component pattern effectively isolates component-based searches to test-created objects, preventing interference from other scene content.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs.meta (1)
1-11: LGTM - Standard Unity asset metadata.Auto-generated Unity metadata file for the test script with default settings.
MCPForUnity/Editor/Resources/Scene.meta (1)
1-8: Meta file format is correct.The folder metadata follows standard Unity conventions with proper DefaultImporter configuration. Format and structure are valid.
MCPForUnity/Editor/Helpers/ParamCoercion.cs.meta (1)
1-11: Meta file format is correct.The MonoImporter metadata conforms to Unity standards with proper serialization version and execution order for a utility class. Format and structure are valid.
MCPForUnity/Editor/Tools/ManageComponents.cs.meta (1)
1-11: Meta file format is correct.The MonoImporter metadata conforms to Unity standards with appropriate serialization version and execution order for an editor tool class. Format and structure are valid.
MCPForUnity/Editor/Helpers/GameObjectLookup.cs.meta (1)
1-11: LGTM!Standard Unity meta file for the new
GameObjectLookup.cshelper. The GUID is unique and the MonoImporter settings are appropriate defaults.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs.meta (1)
1-11: LGTM!Standard Unity meta file for the new test class. No issues.
MCPForUnity/Editor/Helpers/ParamCoercion.cs (1)
1-159: Well-structured utility class.The
ParamCoercionclass provides robust, defensive coercion methods with proper null handling, culture-aware float parsing, and case-insensitive enum support. The XML documentation is comprehensive..github/workflows/claude-nl-suite.yml (2)
754-821: GO pass integration follows established patterns.The new GO (GameObject API) test pass cleanly mirrors the existing NL/T pass structure with appropriate timeout (20 min), coverage checks, and retry logic using Sonnet as fallback. Good consistency.
868-878: GO test canonicalization rules are comprehensive.The regex patterns for GO-0 through GO-10 are properly added alongside NL/T patterns, enabling consistent test ID extraction and canonicalization.
Server/src/services/tools/manage_asset.py (1)
92-96: Good error handling with context logging.The integration of
_normalize_propertieswith error logging viactx.errorbefore returning the failure response is appropriate for debugging.Server/src/services/tools/manage_components.py (3)
16-40: LGTM!The
_normalize_propertieshelper provides robust input validation with clear error messages. The check for serialization bugs ("[object Object]","undefined") is particularly useful for handling LLM-generated inputs.
46-86: Well-documented tool with clear examples.The docstring provides excellent usage examples covering all three actions. The
Annotatedtype hints with descriptions will help LLM tool-calling accuracy.
130-135: Document the dual-mode behavior ofset_propertyor enforce mutual exclusivity.The C# handler processes both
property/valueandpropertiesdict when both are provided. Single property is applied first, then the properties dict. If the same property appears in both, the dict value overwrites the single value. Consider either:
- Document this behavior clearly in the Python docstring/comments
- Enforce mutual exclusivity in the Python layer to prevent accidental double-setting
Server/src/services/tools/manage_material.py (2)
15-45: Good color normalization with comprehensive validation.The
_normalize_colorhelper properly handles multiple input formats (list, tuple, JSON string) and validates component count (3 or 4). The serialization bug detection ("[object Object]") is consistent with other normalizers.
108-124: Clean validation flow with early returns.The normalization and validation of
color,properties,value, andslotwith early error returns follows a clear pattern. Usingcoerce_intfor slot ensures robust handling of various input types.MCPForUnity/Editor/Helpers/VectorParsing.cs.meta (1)
1-11: All three GUIDs are unique within the MCPForUnity project—no collisions detected.The verification confirms that each GUID (4511082b395b14922b34e90f7a23027e, 5ee79050d9f6d42798a0757cc7672517, and ca2205caede3744aebda9f6da2fa2c22) appears exactly once in its respective file, with no duplicates across the 184 total GUIDs in the project.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs.meta (1)
1-11: Metadata file is properly formatted.Standard Unity MonoImporter metadata with unique GUID and correct structure.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs.meta (1)
1-11: Metadata file is properly formatted.Standard Unity MonoImporter metadata with unique GUID and correct structure.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs.meta (1)
1-11: Metadata file is properly formatted.Standard Unity MonoImporter metadata with unique GUID and correct structure.
Server/tests/integration/test_manage_components.py (1)
1-242: LGTM! Comprehensive test coverage for manage_components tool.The test suite thoroughly exercises the manage_components tool across all actions (add, remove, set_property) with various parameter combinations. The mocking strategy correctly captures command construction and validates parameter forwarding.
Note on static analysis hints: The unused
cmdandkwargsarguments infake_sendfunctions are intentional—these mocks must match the signature ofasync_send_command_with_retryeven though they don't use all parameters. This is a standard testing pattern..claude/prompts/nl-gameobject-suite.md (1)
1-150: LGTM! Clear and well-structured test specification.The test suite documentation provides comprehensive specifications for GO-0 through GO-10, with explicit XML format requirements, clear pass criteria, and helpful tool references. The structure supports autonomous CI execution with proper guidance on transcript minimization and evidence collection.
Server/tests/integration/test_find_gameobjects.py (1)
1-200: LGTM! Solid test coverage for find_gameobjects tool.The test suite validates search functionality across multiple methods (by_name, by_component, by_tag, by_layer, by_path) and verifies proper parameter handling including pagination and boolean coercion. The mocking approach correctly captures command construction for integration validation.
Note on static analysis hints: Similar to other test files, the unused arguments in
fake_sendfunctions are intentional to match theasync_send_command_with_retrysignature.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectFindTests.cs (1)
1-578: LGTM! Comprehensive Unity EditMode test coverage.This test class provides excellent baseline coverage for the ManageGameObject find action, exercising all search methods (by_name, by_id, by_path, by_tag, by_layer, by_component) with proper edge case handling. The setup/teardown pattern ensures clean test isolation, and the tests appropriately document current behavior for future API migration.
The test organization by region improves readability, and the helper method
CreateTestObjectfollows the established pattern from other test files in the codebase.Server/tests/integration/test_gameobject_resources.py (1)
1-254: LGTM! Complete test coverage for GameObject resource endpoints.The test suite validates all GameObject resource endpoints with appropriate success and error scenarios. Tests cover:
- Single GameObject retrieval
- Component listing with pagination and property inclusion controls
- Single component retrieval
- Error handling for missing components and GameObjects
The mocking strategy consistently captures parameter passing and validates response structures.
Note on static analysis hints: The unused arguments in
fake_sendfunctions are false positives—they're required to match the real function signature for proper mocking.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (1)
14-35: Well-structured test setup and teardown.The test class properly implements cleanup via
TearDownusingObject.DestroyImmediatefor EditMode tests, and theCreateTestObjecthelper ensures all created objects are tracked for cleanup. This prevents test pollution across test runs..claude/prompts/nl-unity-suite-nl.md (1)
6-6: Tool naming migration looks consistent.The update from
mcp__unity__*tomcp__UnityMCP__*prefix is applied consistently across the AllowedTools declaration and all subsequent references in the document. This aligns with the API redesign mentioned in the PR objectives..claude/prompts/nl-unity-suite-t.md (1)
5-5: Tool naming migration consistent with NL suite.The
mcp__UnityMCP__*prefix updates are consistently applied throughout this T-suite prompt file, matching the changes in the NL suite file.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
17-38: Good test infrastructure with proper cleanup.The
TearDownmethod andFindAndTrackhelper ensure proper cleanup of GameObjects created during tests, preventing test pollution. UsingObject.DestroyImmediateis correct for EditMode tests..github/workflows/claude-gameobject-suite.yml (2)
234-237: Good sensitive data redaction in logs.The redaction pattern
sed -E 's/((email|serial|license|password|token)[^[:space:]]*)/[REDACTED]/Ig'properly sanitizes sensitive information from logs before output, which is important for security in CI environments.
216-248: Robust Unity bridge readiness check.The wait loop with configurable deadline (600s), fatal detection after warm-up (120s), and multiple readiness indicators (port check, log markers) provides a resilient startup sequence. The license error detection and container state monitoring are well-implemented.
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
169-184: Clear deprecation warnings guide users to new APIs.The deprecation warnings with specific guidance (e.g., "Use the 'find_gameobjects' tool instead") provide a clear migration path. This approach allows backwards compatibility while encouraging adoption of the redesigned API.
194-198: Good use of centralized ParamCoercion helpers.Using
Helpers.ParamCoercion.CoerceIntandCoerceBoolfor parameter handling ensures consistent coercion logic across the codebase. The support for both camelCase and snake_case parameter names (pageSize/page_size) improves API ergonomics.
936-1023: Well-implemented DuplicateGameObject action.The implementation handles all expected scenarios:
- Optional new name with sensible default (removing "(Clone)" suffix)
- Absolute position or offset-based positioning
- Flexible parent handling (explicit null for root, specified parent, or same as source)
- Proper Undo registration and scene dirty marking
This is a valuable addition to the GameObject lifecycle operations.
1029-1105: MoveRelativeToObject provides intuitive spatial operations.The implementation supports both world-space and local-space relative movements with named directions (left, right, up, down, forward, back) or custom offset vectors. This is particularly useful for LLM-driven scene manipulation where natural language descriptions of positioning are common.
Server/src/services/tools/find_gameobjects.py (2)
43-51: Good preflight check for compilation state.The preflight gate with
wait_for_no_compile=Trueandrefresh_if_dirty=Trueensures the tool waits for Unity to finish compiling before executing searches, preventing inconsistent results during domain reloads.
54-66: Parameter coercion handles flexible inputs well.The use of
coerce_boolandcoerce_intallows the tool to accept both typed values and string representations, improving robustness when called from various clients.Note: Line 66's filtering for
Nonevalues may be redundant since the coercion functions provide defaults, so no values should beNoneat this point. However, keeping it as a defensive measure is reasonable.MCPForUnity/Editor/Tools/ManageComponents.cs (1)
182-280: LGTM!The
SetPropertyaction handles both single property mode and multiple properties mode well, with proper error accumulation and partial success reporting. The undo recording before property changes is correct.MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs (3)
17-64: LGTM!The
GameObjectResourcehandler correctly validates the instance ID parameter with fallback aliases, handles the -1 sentinel for invalid coercion, and properly serializes the GameObject with lightweight metadata.
70-120: LGTM!The
SerializeGameObjectmethod provides a clean, lightweight serialization that includes essential metadata without full component serialization. The null filtering for components handles edge cases with destroyed components.
127-206: LGTM!The
GameObjectComponentsResourcecorrectly implements pagination with configurable property inclusion. The dual-mode serialization (lightweight vs full) provides flexibility for different use cases.MCPForUnity/Editor/Helpers/VectorParsing.cs (4)
1-59: LGTM!The
ParseVector3andParseVector3OrDefaultmethods correctly handle both array and object formats with proper null checks and error logging. The implementation is clean and follows consistent patterns.
99-169: LGTM!The
ParseQuaternionimplementation correctly handles multiple input formats with clear precedence (4-element quaternion before 3-element euler). The documentation about non-normalized quaternions is helpful for callers.
171-225: LGTM!The
ParseColormethod handles RGB and RGBA formats well, with sensible alpha defaults.
269-293: LGTM with a minor note.
ParseBoundssilently defaults toVector3.zeroifcenterorsizeparsing fails. This is consistent with the pattern used elsewhere but could create unexpected zero-sized bounds at origin if the input is malformed. The warning logging inParseVector3will surface these issues.Server/src/services/tools/manage_gameobject.py (2)
14-53: LGTM!The
_normalize_vectorhelper is robust, handling multiple input formats (list/tuple, JSON string, comma-separated strings) with proper validation includingmath.isfinitechecks to reject NaN/Inf values. The fallback chain is well-structured.
56-80: LGTM!The
_normalize_component_propertieshelper provides excellent error handling with clear, actionable error messages. The check for"[object Object]"is a smart catch for JavaScript serialization bugs from clients.MCPForUnity/Editor/Helpers/GameObjectLookup.cs (2)
148-164: Good documentation of Unity API limitation.The warning about
GameObject.Find()not finding inactive objects is helpful. This is a well-known Unity limitation that's properly documented here.
250-280: LGTM!The
GetAllSceneObjectsandGetObjectAndDescendantsmethods use lazy evaluation viayield return, which is memory-efficient for large scenes. The recursive traversal is appropriate for typical Unity hierarchy depths.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (2)
15-46: LGTM!The test setup and teardown correctly manage test object lifecycle, ensuring proper cleanup with
DestroyImmediatefor editor mode tests. TheCreateTestObjecthelper simplifies test authoring.
394-411: LGTM!Good use of
LogAssert.Expectwith regex to handle Unity's error logging for invalid tags. This ensures the test doesn't fail due to expected error logs while still verifying the operation completes.Server/src/services/resources/gameobject.py (4)
1-24: LGTM! Clean imports and helper function.The module structure is clear, imports are appropriate, and the
_normalize_responsehelper correctly handles both dict and MCPResponse types.
31-110: LGTM! Well-structured data models and documentation resource.The static documentation resource provides clear guidance for users, and the Pydantic models are well-designed with appropriate defaults. The TODO comment on lines 106-107 acknowledges that typed response validation could be stricter in the future, which is fine for incremental improvement.
113-134: LGTM! Correct implementation of the gameobject resource endpoint.The endpoint properly validates the instance_id parameter, handles errors gracefully, and uses the Unity transport pattern correctly.
137-187: LGTM! Proper pagination implementation.The ComponentsData model includes all necessary pagination fields, and the endpoint correctly forwards pagination parameters to Unity. The default values (page_size=25, include_properties=True) are reasonable.
| model: claude-haiku-4-5-20251001 | ||
| fallback_model: claude-sonnet-4-5-20250929 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Anthropic Claude valid model identifiers January 2026 haiku sonnet
💡 Result:
Short list of Anthropic Claude model identifiers that were published/used around January 2026 (representative, not exhaustive):
- claude-sonnet-4.5 / claude-haiku-4.5 / claude-opus-4.1 (Claude 4.x family). [1][2]
- claude-3-5-sonnet-20241022, claude-3-5-haiku-20241022 (Claude 3.5 variants). [1][2]
- claude-3-opus-20240229, claude-3-sonnet-20240229, claude-3-haiku-20240307 (Claude 3 family). [1][3]
- claude-2, claude-2.1 (legacy Claude 2.x identifiers). [3]
Sources: Humanloop model list and docs (shows many Anthropic IDs) [1]; ActiveAgent / Anthropic provider docs (Claude 4.x and 3.x naming) [2]; litellm / provider lists (additional model-ID examples) [3].
Haiku
tokens in a stream
claude-sonnet hums, haiku quick—
names map thinking’s reach
Sonnet (14 lines)
In racks of code the model names appear,
claude-sonnet, claude-haiku, opus, three;
each tag a door that brings the mind more near,
a labeled key to scale and memory.
From two to three to four the versions climb,
timestamps and suffixes keep time precise;
a versioned string that holds a shape of rhyme,
and trades in speed, in depth, in measured price.
Use sonnet when you ask for thoughtful art,
haiku when you need a fast and nimble mind;
opus for the problems piercing to the heart,
old tags remain so reproducible you’ll find.
Model IDs: the paths for prompt and plan—
choose name, invoke, and let the system span.
If you want a complete machine-readable list (JSON or exact strings) for a specific API/region (Anthropic, Bedrock, OpenRouter, etc.), tell me which target and I’ll fetch the authoritative current list and links.
These model identifiers are invalid and will cause the workflow to fail.
The model identifiers claude-haiku-4-5-20251001 and claude-sonnet-4-5-20250929 do not match valid Anthropic Claude API model identifiers as of January 2026. Valid models use formats like claude-sonnet-4.5, claude-haiku-4.5, or claude-3-5-sonnet-20241022. The identifiers here use incorrect formatting (hyphens instead of periods in the version) and date suffixes that are not documented in Anthropic's official model naming conventions. Update to use valid model identifiers such as claude-sonnet-4.5 and claude-haiku-4.5 or confirm the correct versioned identifiers from Anthropic's API documentation.
🤖 Prompt for AI Agents
In @.github/workflows/claude-gameobject-suite.yml around lines 390-391, The
workflow uses invalid model identifiers; replace the incorrect values
"claude-haiku-4-5-20251001" and "claude-sonnet-4-5-20250929" with valid
Anthropic model names; update the model key to a valid identifier such as
"claude-haiku-4.5" and the fallback_model key to "claude-sonnet-4.5" (or other
confirmed Anthropic versioned names) so the model and fallback_model entries use
the proper period-delimited version format.
- 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
- 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: 6
Fix all issues with AI Agents 🤖
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs:
- Around line 151-169: The test Delete_ByLayer_DeletesMatchingObjects currently
only asserts a non-null result; update it to verify the target GameObject was
actually removed after calling ManageGameObject.HandleCommand(p) by asserting
the object no longer exists (e.g., GameObject.Find or checking target == null or
target.activeInHierarchy as appropriate) before removing it from testObjects;
keep the call to CreateTestObject("DeleteByLayer") and the same payload, then
replace or extend the Assert.IsNotNull(result, ...) with an assertion that the
created target is gone and only remove target from testObjects if the deletion
succeeded.
- Around line 171-190: Update the Delete_ByPath_DeletesObject test to actually
verify the GameObject was removed: after calling
ManageGameObject.HandleCommand(p) assert that
GameObject.Find("DeleteParent/DeleteChild") returns null (or use Assert.IsNull)
instead of only asserting the result is non-null; then adjust cleanup to remove
the parent from testObjects (and only remove the child if it still exists) so
you don't reference a destroyed object. Use the test method
Delete_ByPath_DeletesObject, the ManageGameObject.HandleCommand call, and
GameObject.Find to locate the target for verification.
- Around line 123-149: The test Delete_ByTag_DeletesMatchingObjects currently
only checks that ManageGameObject.HandleCommand returned a non-null result;
update it to explicitly verify the two created GameObjects (target1 and target2)
were deleted by calling GameObject.Find with their names (or checking target1 ==
null / target2 == null after the command) and asserting they are null (or
destroyed), mirroring the pattern used in Delete_ByName_DeletesObject; also
adjust cleanup so you only call testObjects.Remove(...) if the objects still
exist or skip removal for destroyed objects to avoid false failures.
- Around line 290-308: The test Delete_InactiveObject_StillDeletes currently
only asserts a non-null result; update it to explicitly verify the inactive
GameObject was removed by asserting GameObject.Find("InactiveDeleteTarget")
returns null (or Assert.IsFalse(GameObject.Find(...) != null) with a clear
message) after calling ManageGameObject.HandleCommand(p), and also ensure
testObjects no longer contains the target (or only call
testObjects.Remove(target) if the object still exists) to avoid leaving stale
references; reference the test method Delete_InactiveObject_StillDeletes, the
helper CreateTestObject, and ManageGameObject.HandleCommand when making this
change.
- Around line 262-284: In the Delete_Success_ReturnsDeletedCount test the code
only checks that resultObj["data"] exists but doesn't verify the deleted count;
after obtaining var data = resultObj["data"], add an assertion that the deleted
count equals 1 by asserting data.Value<int>("deleted") (or the equivalent
accessor) is 1 so the test actually validates the expected deletion count
returned by ManageGameObject.HandleCommand.
- Around line 310-329: The test Delete_MultipleObjectsSameName_DeletesCorrectly
lacks assertions about whether ManageGameObject.HandleCommand deletes only the
first matching GameObject or all matches; decide the intended behavior and
update the test to assert it explicitly: if the expected behavior is "delete
first match" then after calling ManageGameObject.HandleCommand(p) assert that
one of the two created objects (from CreateTestObject("DuplicateName")) was
destroyed and the other still exists (use testObjects list or UnityEngine.Object
references to check null/active state) and remove only the destroyed reference
from testObjects; if the expected behavior is "delete all matches" then assert
both objects are destroyed and clear both from testObjects. Ensure assertions
reference ManageGameObject.HandleCommand, CreateTestObject, and the testObjects
collection so reviewers can find the code to verify behavior.
🧹 Nitpick comments (2)
Server/src/services/resources/gameobject.py (1)
117-122: Clarify TODO: Timeline for stricter type validationThe TODO comment suggests future migration to typed response classes for "better type safety." Since the endpoints already use Pydantic models for the
datafield schema, clarify:
- What additional validation is missing?
- Is this about enforcing response structure at the Unity→Python boundary?
- Should this be tracked in an issue?
If the TODO refers to validating Unity's response structure matches the Pydantic models, consider opening an issue to track this work and link it in the comment for traceability.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (1)
61-61: Consider removing redundant manual cleanup.The manual removal of deleted objects from
testObjects(lines 61, 86, and elsewhere) is redundant becauseTearDownalready performs a null check before callingDestroyImmediate. While not harmful, removing these lines would simplify the tests.Example for Delete_ByName_DeletesObject
// Verify object is deleted var found = GameObject.Find("DeleteTargetByName"); Assert.IsNull(found, "Object should be deleted"); - - // Remove from our tracking list since it's deleted - testObjects.Remove(target);Also applies to: 86-86
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
MCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageScene.csREADME.mdServer/src/services/resources/gameobject.pyServer/tests/integration/test_manage_asset_json_parsing.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 (3)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs
- MCPForUnity/Editor/Helpers/GameObjectLookup.cs
- MCPForUnity/Editor/Helpers/ParamCoercion.cs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageScene.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Tools/ManageComponents.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageScene.cs
📚 Learning: 2025-12-29T15:23:17.871Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
README.md
🧬 Code graph analysis (2)
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)
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)
⏰ 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: Sourcery review
🔇 Additional comments (19)
Server/tests/integration/test_manage_asset_json_parsing.py (1)
40-60: LGTM! Test assertion correctly updated to match backend error message.The assertion update aligns with the backend error message capitalization change mentioned in the PR objectives. The test continues to validate invalid JSON handling appropriately.
MCPForUnity/Editor/Tools/ManageScene.cs (2)
399-399: LGTM: Modern Unity API usageThe replacement of the deprecated
FindObjectsOfTypewithFindObjectsByTypeis correct and follows Unity's recommended migration path. UsingFindObjectsSortMode.Noneis appropriate for this fallback camera discovery scenario.
632-664: LGTM: Well-implemented component type enumerationThe new
componentTypescollection provides a lightweight list of component type names without full serialization. The implementation correctly:
- Filters out null components
- Extracts
Type.Namefor each component- Wraps enumeration in try-catch for resilience
- Adds the list to the summary dictionary
This aligns well with the broader PR objective of separating lightweight metadata (resources) from heavy operations (tools).
MCPForUnity/Editor/Tools/ManageComponents.cs (6)
27-64: LGTM: Robust command handling with proper validationThe entry point correctly:
- Validates required parameters (action, target)
- Normalizes action to lowercase for consistency
- Routes to appropriate handlers using pattern matching
- Wraps execution in try-catch with structured error responses
- Logs errors for debugging
68-126: LGTM: Proper component addition with Undo supportThe
AddComponentimplementation correctly:
- Validates target GameObject and component type
- Uses
Undo.AddComponentfor editor undo support- Optionally sets initial properties after creation
- Marks GameObject dirty for proper serialization
- Returns structured data with instance IDs for further operations
149-153: Good safety check: Transform removal preventionPreventing Transform removal is essential since every GameObject must have a Transform component. The early check before
GetComponentis efficient and provides a clear error message.
225-248: Verify error accumulation behavior matches user expectationsThe
SetPropertyaction accumulates errors when setting multiple properties but still returnssuccess: falseif any property fails. This means:
- Partial success scenarios return
success: falsewith an error list- Users receive all validation errors at once (good for batch operations)
Consider whether callers expect
success: truewith a warning list when some properties succeed. The current behavior is safer (fail-fast for any error), but document this in the tool description if not already covered.Review the tool's public description (likely in a decorator or schema elsewhere) to ensure it clarifies that
set_propertyreturnssuccess: falseif any property fails, even if others succeed. This helps LLM callers understand the partial-success semantics.
353-353: Case-insensitive property lookup improves usabilityUsing
BindingFlags.IgnoreCasefor both properties and fields (lines 353, 370) is excellent for external tool callers who may not know the exact casing of Unity component properties. This reduces friction for LLM-generated calls.
390-415: LGTM: Unity type conversion with safe defaultsThe
ConvertValuemethod correctly:
- Handles Unity-specific types (Vector3, Vector2, Quaternion, Color) via VectorParsing helpers
- Uses safe defaults (Vector2.zero, Quaternion.identity, Color.white)
- Falls back to Newtonsoft.Json for other types
- Returns null for null tokens
Server/src/services/resources/gameobject.py (6)
27-35: LGTM: Clean instance ID validation with early returnThe
_validate_instance_idhelper correctly:
- Returns a tuple of (parsed_int, None) on success or (None, error_response) on failure
- Provides clear error messages for invalid instance IDs
- Avoids exceptions for control flow
This pattern is idiomatic and easy to use at call sites.
42-87: Excellent: Self-documenting API resourceThe
get_gameobject_api_docsresource provides:
- Clear workflow guidance (find → read pattern)
- Examples for each resource URI
- Parameter documentation for paginated endpoints
- Links to related tools
This significantly improves discoverability for LLM clients and human developers.
90-114: LGTM: Well-structured data models with safe defaultsThe Pydantic models (
TransformData,GameObjectData) correctly:
- Provide default values for all optional fields
- Use appropriate types (int, str, bool, dict, list)
- Include both instance data and relationship data (parent, children)
- Match the Unity serialization format
129-144: LGTM: Proper async resource patternThe
get_gameobjectendpoint correctly:
- Extracts Unity instance from context
- Validates instance_id before sending the command
- Uses
send_with_unity_instancewrapper for transport abstraction- Normalizes the response
170-196: LGTM: Pagination parameters properly forwardedThe
get_gameobject_componentsendpoint correctly:
- Accepts pagination parameters (page_size, cursor, include_properties)
- Provides sensible defaults (page_size=25, cursor=0, include_properties=True)
- Forwards all parameters to Unity command
216-238: LGTM: Single component retrievalThe
get_gameobject_componentendpoint correctly retrieves a specific component by type name, following the same validation and transport pattern as the other endpoints.README.md (2)
44-70: LGTM: Comprehensive tool documentation updatesThe Tools section accurately reflects the PR changes:
manage_componentsadded with correct description (add, remove, set properties)find_gameobjectsadded with search criteria listedmanage_gameobjectupdated to mention duplicate and move operationsmanage_sceneupdated to include screenshot capability- Other tools have refined descriptions aligning with actual behavior
The documentation correctly guides users toward preferred tools (e.g., suggesting
apply_text_editsover legacymanage_script).
79-92: LGTM: Resources section accurately documents new endpointsThe Resources section correctly adds:
gameobject_api: Documentation resource for GameObject endpoints- Enhanced descriptions for existing resources (unity_instances, tests, editor_state)
editor_state_v2: New canonical readiness snapshotThe descriptions accurately reflect the functionality implemented in the codebase.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (2)
15-35: Well-structured test infrastructure.The testObjects tracking list with null-safe cleanup in TearDown is a solid pattern for managing test GameObject lifecycle in edit mode tests.
196-256: Excellent hierarchy deletion tests.Both tests properly verify the expected deletion behavior with explicit
GameObject.Findassertions for all relevant objects. The parent/child relationship tests cover important edge cases.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
Show resolved
Hide resolved
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
Show resolved
Hide resolved
| [Test] | ||
| public void Delete_ByPath_DeletesObject() | ||
| { | ||
| var parent = CreateTestObject("DeleteParent"); | ||
| var child = CreateTestObject("DeleteChild"); | ||
| child.transform.SetParent(parent.transform); | ||
|
|
||
| var p = new JObject | ||
| { | ||
| ["action"] = "delete", | ||
| ["target"] = "DeleteParent/DeleteChild", | ||
| ["searchMethod"] = "by_path" | ||
| }; | ||
|
|
||
| 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.
🛠️ Refactor suggestion | 🟠 Major
Strengthen deletion verification for path-based deletion.
This test should verify that the child GameObject was actually deleted via GameObject.Find, not just that a result was returned.
Suggested verification approach
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());
+ Assert.IsNull(GameObject.Find("DeleteChild"), "Child should be deleted");
+ Assert.IsNotNull(GameObject.Find("DeleteParent"), "Parent should still exist");
testObjects.Remove(child);🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
around lines 171-190, Update the Delete_ByPath_DeletesObject test to actually
verify the GameObject was removed: after calling
ManageGameObject.HandleCommand(p) assert that
GameObject.Find("DeleteParent/DeleteChild") returns null (or use Assert.IsNull)
instead of only asserting the result is non-null; then adjust cleanup to remove
the parent from testObjects (and only remove the child if it still exists) so
you don't reference a destroyed object. Use the test method
Delete_ByPath_DeletesObject, the ManageGameObject.HandleCommand call, and
GameObject.Find to locate the target for verification.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
Show resolved
Hide resolved
| [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"); | ||
|
|
||
| testObjects.Remove(target); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen verification for inactive object deletion.
The test only verifies that a result was returned, not that the inactive GameObject was actually deleted. Add explicit verification of the deletion.
Suggested verification
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());
+ Assert.IsNull(GameObject.Find("InactiveDeleteTarget"), "Inactive object should be deleted");
testObjects.Remove(target);🤖 Prompt for AI Agents
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
around lines 290-308, The test Delete_InactiveObject_StillDeletes currently only
asserts a non-null result; update it to explicitly verify the inactive
GameObject was removed by asserting GameObject.Find("InactiveDeleteTarget")
returns null (or Assert.IsFalse(GameObject.Find(...) != null) with a clear
message) after calling ManageGameObject.HandleCommand(p), and also ensure
testObjects no longer contains the target (or only call
testObjects.Remove(target) if the object still exists) to avoid leaving stale
references; reference the test method Delete_InactiveObject_StillDeletes, the
helper CreateTestObject, and ManageGameObject.HandleCommand when making this
change.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
Show resolved
Hide resolved
- 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.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (1)
182-182: Simplify redundant assertion.This assertion checks
testObjects.Contains(target) && target != nullafter already verifyingtarget == nullon line 180. If the target is deleted (null), this condition is always false anyway, making the assertion redundant.🔎 Suggested simplification
- Assert.IsFalse(testObjects.Contains(target) && target != null, "Deleted object should not be findable");Since deletion is already verified on line 180-181, this line can be removed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-29T15:23:17.871Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:17.871Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs
⏰ 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: Sourcery review
🔇 Additional comments (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectDeleteTests.cs (4)
1-117: LGTM! Solid test foundation.The test setup, teardown, and basic delete tests are well-structured with proper deletion verification using
GameObject.Findand Unity null checks. Error handling for non-existent and missing targets is appropriately tested.
213-273: LGTM! Comprehensive hierarchy testing.Both tests properly verify GameObject deletion in hierarchy contexts using
GameObject.Findto confirm parent/child relationships are handled correctly during deletion operations.
279-312: LGTM! Proper deletion and response verification.The test correctly verifies actual object destruction using Unity null checks and conditionally validates the
deletedCountfield when present in the response data.
338-375: LGTM! Excellent duplicate-name handling.The test properly verifies deletion behavior using Unity null checks and clearly documents the first-match deletion behavior with appropriate comments and assertions.
- 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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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: 1
Fix all issues with AI Agents 🤖
In @Server/src/services/tools/manage_asset.py:
- Around line 20-53: The _normalize_properties function currently returns an
empty dict for value is None and also uses ast.literal_eval fallback, which
diverges from sibling normalizers; change the function so that when value is
None it returns (None, None) to match manage_material/manage_components
behavior, and remove the ast.literal_eval fallback so parsing only uses
parse_json_payload and JSON error handling; if you intentionally need
Python-literal support keep ast.literal_eval but add a short comment above
_normalize_properties explaining why this tool accepts Python dict literals and
why None maps to {} (or conversely adjust to return None and document that
choice).
🧹 Nitpick comments (3)
Server/src/services/tools/batch_execute.py (1)
20-23: Consider mentioning the batch size limit in the description.The updated description effectively emphasizes performance benefits and provides clear guidance for LLMs. However, since
MAX_COMMANDS_PER_BATCHis set to 25 (line 14), consider adding this constraint to the description to prevent the LLM from attempting to batch more commands than allowed.🔎 Suggested enhancement
description=( "Executes multiple MCP commands in a single batch for dramatically better performance. " "STRONGLY RECOMMENDED when creating/modifying multiple objects, adding components to multiple targets, " "or performing any repetitive operations. Reduces latency and token costs by 10-100x compared to " - "sequential tool calls. Example: creating 5 cubes → use 1 batch_execute with 5 create commands instead of 5 separate calls." + "sequential tool calls. Supports up to 25 commands per batch. " + "Example: creating 5 cubes → use 1 batch_execute with 5 create commands instead of 5 separate calls." ),README.md (1)
357-357: Add language identifier to fenced code block.Line 357 uses a plain fenced code block without a language specifier, which triggered a static analysis warning. While this is minor, adding a language identifier improves rendering and accessibility.
🔎 Proposed fix
-``` +```text ❌ Slow: Create 5 cubes → 5 separate manage_gameobject calls ✅ Fast: Create 5 cubes → 1 batch_execute call with 5 commandsAlternatively, use
markdownor leave the content as regular text without a code fence if it's meant as a visual comparison.MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
2284-2323: Consider renaming method to reflect actual implementation.
GetAIPropertySuggestionsuses rule-based fuzzy matching (Levenshtein distance, substring matching) rather than AI. The unused prompt string (lines 2296-2309) suggests AI integration was planned but not implemented.Recommendations:
- Rename to
GetPropertySuggestionsorGetFuzzyPropertyMatchesto reflect the actual implementation- Remove the unused prompt string (lines 2296-2309) to reduce confusion
- If AI integration is planned for the future, add a TODO comment
The current implementation works well, but the naming is misleading.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
MCPForUnity/Editor/Tools/ManageGameObject.csREADME.mdServer/src/services/resources/gameobject.pyServer/src/services/tools/batch_execute.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/tests/integration/conftest.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
💤 Files with no reviewable changes (1)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/services/resources/gameobject.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
📚 Learning: 2025-12-29T04:54:25.306Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 490
File: Server/pyproject.toml:33-33
Timestamp: 2025-12-29T04:54:25.306Z
Learning: For fastmcp dependency in Server/pyproject.toml: prefer exact version pinning (e.g., fastmcp==2.14.1) rather than range pins like >=2.13.0, due to past issues where fastmcp introduced breaking changes that affected all MCP tools.
Applied to files:
Server/tests/integration/conftest.py
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Tools/ManageGameObject.cs
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Tools/ManageGameObject.cs
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
Server/src/services/tools/batch_execute.py
🧬 Code graph analysis (3)
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)
Server/src/services/tools/manage_asset.py (3)
Server/src/services/tools/manage_material.py (1)
_normalize_properties(50-69)Server/src/services/tools/manage_components.py (1)
_normalize_properties(16-40)Server/src/services/tools/utils.py (1)
parse_json_payload(27-60)
🪛 markdownlint-cli2 (0.18.1)
README.md
357-357: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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: Sourcery review
🔇 Additional comments (10)
Server/src/services/tools/batch_execute.py (1)
25-28: Appropriate use of destructiveHint flag.Adding
destructiveHint=Trueis correct sincebatch_executecan execute any command including destructive operations (delete, modify, etc.). This properly signals the potential risk to MCP clients.Server/src/services/tools/manage_gameobject.py (2)
16-56: Robust vector normalization with comprehensive format support.The
_normalize_vectorhelper handles multiple input formats (list, tuple, JSON string, comma-separated strings) and includesmath.isfinitechecks (lines 29, 39, 51) to prevent NaN/Infinity values. This is excellent defensive programming for Unity transform data.
195-195: Verifyworld_spacedefault behavior is documented.Line 195 sets
world_space = coerce_bool(world_space, default=True), making world-space mode the default when the parameter is omitted. Ensure this default is clearly documented in the parameter annotation (line 164-165) and aligns with Unity's expected behavior for relative movement operations. If users expect local-space by default, this could be surprising.Server/src/services/tools/manage_material.py (2)
17-48: Color normalization handles RGB and RGBA formats correctly.The helper validates that color arrays have exactly 3 or 4 components (line 27) and converts all values to floats. The error messages are clear and actionable.
129-130: Verifyslotcoercion preservesNoneas intended.Line 130 calls
slot = coerce_int(slot)without an explicitdefaultparameter. Based on thecoerce_intimplementation (Server/src/services/tools/utils.py, lines 62-76), omittingdefaultshould returnNonewhenslotisNone(since the function signature hasdefault: int | None = None). However, a previous review comment flagged concern about accidentally coercingNoneto0.Please verify that:
coerce_int(slot)correctly preservesNonewhenslotisNone(expected behavior based on the function signature).- Unity's C# handler distinguishes "no slot specified" (None) from "use slot 0" (explicit 0).
If the current behavior is correct, consider adding
default=Noneexplicitly for clarity:slot = coerce_int(slot, default=None).Based on past review comments suggesting potential for unintentional coercion.
README.md (1)
353-366: Excellent performance guidance forbatch_execute.The side-by-side comparison (❌ Slow vs. ✅ Fast) clearly illustrates when to batch operations. This will help users avoid common performance pitfalls.
Server/tests/integration/conftest.py (1)
93-94: Clean removal of starlette stubs is correct and well-documented.The removal of starlette stub code is justified and complete. The mcp package version 1.16.0 includes starlette>=0.27 as a direct dependency, confirming the comment that "starlette is now a proper dependency (via mcp package)."
MCPForUnity/Editor/Tools/ManageGameObject.cs (3)
76-89: Good defensive parsing, but consider the failure path.The string-to-JObject coercion for
componentPropertiesis well-handled with appropriate error logging. However, when parsing fails (line 87), the code logs a warning but doesn't update@params["componentProperties"], meaning downstream code may still receive an unparsable string.Consider whether this silent continuation is intentional—if components expect a JObject and receive a string, they may fail later with less context.
165-176: LGTM—clear documentation of refactored scope.The comment clarifies that only lifecycle actions remain in
manage_gameobjectafter the refactoring. This is consistent with the PR objectives stating that find/component operations were moved to dedicated tools.
114-128: No issue found—line 117 correctly assumes the nested format.The code at line 117 properly expects
componentPropertiesto be nested by component name ({"Transform": {...}}) rather than flat. This is the consistent input format across all test cases and the codebase:["componentProperties"] = new JObject { ["Transform"] = new JObject { ["localPosition"] = ... }, ["Rigidbody"] = new JObject { ["mass"] = ... } }Both the
"modify"and"set_component_property"actions use this same nested structure, so the code at lines 114–128 is correctly implemented.Likely an incorrect or invalid review comment.
| def _normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]: | ||
| """ | ||
| Robustly normalize properties parameter to a dict. | ||
| Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. | ||
| """ | ||
| if value is None: | ||
| return {}, None | ||
|
|
||
| # Already a dict - return as-is | ||
| if isinstance(value, dict): | ||
| return value, None | ||
|
|
||
| # Try parsing as string | ||
| if isinstance(value, str): | ||
| # Check for obviously invalid values from serialization bugs | ||
| if value in ("[object Object]", "undefined", "null", ""): | ||
| return None, f"properties received invalid value: '{value}'. Expected a JSON object like {{\"key\": value}}" | ||
|
|
||
| # 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}" | ||
|
|
||
| return None, f"properties must be a dict or JSON string, got {type(value).__name__}" | ||
|
|
There was a problem hiding this comment.
Inconsistent handling of None input across normalization helpers.
Line 26 returns {} (empty dict) when value is None, while the parallel _normalize_properties helpers in Server/src/services/tools/manage_material.py (line 54-55) and Server/src/services/tools/manage_components.py (line 16-17) return None. This inconsistency may cause confusion or unintended behavior if callers expect None to remain None.
Additionally, lines 44-50 introduce ast.literal_eval as a fallback parser, which is not present in the other normalization helpers. While this adds robustness for Python dict literals, it introduces complexity and behavioral divergence.
🔎 Recommended alignment
Option 1 (align with sibling files): Return None for None input to match the pattern in manage_material.py and manage_components.py.
def _normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]:
"""
Robustly normalize properties parameter to a dict.
Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None.
"""
if value is None:
- return {}, None
+ return None, NoneOption 2 (document rationale): If returning {} is intentional for this tool (e.g., because Unity expects non-null properties), add a comment explaining why this differs from the other normalizers.
For ast.literal_eval: If Python dict literals are expected inputs for manage_asset, keep it; otherwise, consider removing to match sibling patterns.
📝 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.
| def _normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]: | |
| """ | |
| Robustly normalize properties parameter to a dict. | |
| Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. | |
| """ | |
| if value is None: | |
| return {}, None | |
| # Already a dict - return as-is | |
| if isinstance(value, dict): | |
| return value, None | |
| # Try parsing as string | |
| if isinstance(value, str): | |
| # Check for obviously invalid values from serialization bugs | |
| if value in ("[object Object]", "undefined", "null", ""): | |
| return None, f"properties received invalid value: '{value}'. Expected a JSON object like {{\"key\": value}}" | |
| # 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}" | |
| return None, f"properties must be a dict or JSON string, got {type(value).__name__}" | |
| def _normalize_properties(value: Any) -> tuple[dict[str, Any] | None, str | None]: | |
| """ | |
| Robustly normalize properties parameter to a dict. | |
| Returns (parsed_dict, error_message). If error_message is set, parsed_dict is None. | |
| """ | |
| if value is None: | |
| return None, None | |
| # Already a dict - return as-is | |
| if isinstance(value, dict): | |
| return value, None | |
| # Try parsing as string | |
| if isinstance(value, str): | |
| # Check for obviously invalid values from serialization bugs | |
| if value in ("[object Object]", "undefined", "null", ""): | |
| return None, f"properties received invalid value: '{value}'. Expected a JSON object like {{\"key\": value}}" | |
| # 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}" | |
| return None, f"properties must be a dict or JSON string, got {type(value).__name__}" |
🤖 Prompt for AI Agents
In @Server/src/services/tools/manage_asset.py around lines 20-53, The
_normalize_properties function currently returns an empty dict for value is None
and also uses ast.literal_eval fallback, which diverges from sibling
normalizers; change the function so that when value is None it returns (None,
None) to match manage_material/manage_components behavior, and remove the
ast.literal_eval fallback so parsing only uses parse_json_payload and JSON error
handling; if you intentionally need Python-literal support keep ast.literal_eval
but add a short comment above _normalize_properties explaining why this tool
accepts Python dict literals and why None maps to {} (or conversely adjust to
return None and document that choice).
|
This is definitely a major version bump, and it's exactly the update I think will really take this tool to the next level. Particularly when working with larger projects and getting LLMs to do the right things. Top tier work @dsarno 🚀 🚀 🚀 |
上游變更 (v7.0.0 → v9.0.3)： - GameObject 工具集重構 (CoplayDev#518) - VFX 管理功能 (CoplayDev#520) - 批次執行錯誤處理改進 (CoplayDev#531) - Windows 長路徑問題修復 (CoplayDev#534) - HTTP/Stdio 傳輸 UX 改進 (CoplayDev#530) - LLM 工具註解功能 (CoplayDev#480) 衝突解決： - modify/delete：接受刪除（架構已重構） - content：接受 v9.0.3 版本（2020 相容性修正將另行處理）
2 participants
Summary
This PR fundamentally redesigns the GameObject API to improve LLM tool-calling accuracy and reduce payload sizes. The existing
manage_gameobjecttool suffered from parameter overload—too many optional parameters for different purposes caused LLMs to frequently confuse which parameters to use for which actions.The solution: Split responsibilities into focused, single-purpose tools and resources.
🆕 New Tools
find_gameobjectsmanage_componentsadd,remove,set_property. Clear, focused parameters.🆕 New Resources (Read-Only)
unity://scene/gameobject/{id}unity://scene/gameobject/{id}/componentsunity://scene/gameobject/{id}/component/{name}unity://scene/gameobject-api🔄 Updated Existing Tools
manage_scene(get_hierarchy)componentTypesarray in each hierarchy itemmanage_gameobjectcreate,modify,delete,duplicate,move_relativefind,get_components,get_component,add_component,remove_component,set_component_property🧹 Extracted Utilities
Refactored duplicate code into reusable helpers:
GameObjectLookupParamCoercionVectorParsing📊 Test Coverage
claude-gameobject-suite.ymlwith GO-0 through GO-10Unity Test Files:
ManageGameObjectCreateTests.cs- Create action edge casesManageGameObjectModifyTests.cs- Modify action variationsManageGameObjectDeleteTests.cs- Delete action with proper deletion verificationGameObjectAPIStressTests.cs- Pagination, large payloads, new API tools/resourcesPython Test Files:
test_find_gameobjects.py- New search tooltest_manage_components.py- New component tooltest_gameobject_resources.py- New resource endpoints🎯 Design Principles
batch_executeemphasized for 10-100x performance gains📝 Migration Guide
Unknown actionerrors:manage_gameobject(action="find", ...)find_gameobjects(...)manage_gameobject(action="get_components", ...)unity://scene/gameobject/{id}/componentsmanage_gameobject(action="get_component", ...)unity://scene/gameobject/{id}/component/{name}manage_gameobject(action="add_component", ...)manage_components(action="add", ...)manage_gameobject(action="remove_component", ...)manage_components(action="remove", ...)manage_gameobject(action="set_component_property", ...)manage_components(action="set_property", ...)✅ Checklist
batch_executerecommendations added to README and API docsKey changes from earlier version:
batch_executeperformance recommendations as a design principleSummary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.