🔧 Clean up & Consolidate Shared Services Across MCP Tools #519
Conversation
- find_gameobjects: Search GameObjects, returns paginated instance IDs only
- manage_components: Component lifecycle (add, remove, set_property)
- 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
- 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
- ParamCoercion: Centralized int/bool/float/string coercion
- VectorParsing: Vector3/Vector2/Quaternion/Color parsing
- GameObjectLookup: Centralized GameObject search logic
- 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.
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
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)
- 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
- 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
- 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.
Major architectural improvements: - Create UnityJsonSerializer for shared JSON/Unity type conversion - Create ObjectResolver for unified object resolution (GameObjects, Components, Assets) - Create UnityTypeResolver for consolidated type resolution with caching - Create PropertyConversion for unified JSON→Unity property conversion - Create ComponentOps for low-level component operations - Create Pagination helpers for standardized pagination across tools Tool simplifications: - ManageGameObject: Remove 68-line prefab redirect anti-pattern, delegate to helpers - ManageAsset: Remove ~80 lines duplicate ConvertJTokenToType - ManageScriptableObject: Remove ~40 lines duplicate ResolveType - ManageComponents: Use ComponentOps, UnityTypeResolver (~90 lines saved) - ManageMaterial: Standardize to SuccessResponse/ErrorResponse patterns - FindGameObjects: Use PaginationRequest/PaginationResponse - GameObjectLookup: FindComponentType delegates to UnityTypeResolver Tests: 242/246 passed, 4 skipped (expected)
…bility Python Server: - Extract normalize_properties() to shared utils.py (removes duplication) - Move search_term validation before preflight() for fail-fast - Fix manage_script.py documentation (remove incorrect 'update' reference) - Remove stale comments in execute_menu_item.py, manage_editor.py - Remove misleading destructiveHint from manage_shader.py C# Unity: - Add Vector4Converter (commonly used, was missing) - Fix Unity 2021 compatibility: replace FindObjectsByType with FindObjectsOfType - Add path normalization in ObjectResolver before StartsWith check - Improve ComponentOps.SetProperty conversion error detection - Add Undo.RecordObject in ManageComponents before property modifications - Improve error message clarity in ManageMaterial.cs - Add defensive error handling to stress test ToJObject helper - Increase CI timeout thresholds for test stability GitHub Workflows: - Fix GO test sorting in markdown output (GO-10 now sorts after GO-9) - Add warning logging for fragment parsing errors
BlendXHash/BlendYHash now use 'reachX'/'reachY' to match the actual animator parameter names.
- Fix netstat detection on Windows by running netstat.exe directly instead of piping through findstr (findstr returns exit code 1 when no matches, causing false detection failures) - Increase auto-start retry attempts (20→30) and delays (2s→3s) to handle slow server starts during first install, version upgrades, and dev mode - Only attempt blind connection after 20 failed detection attempts to reduce connection error spam during server startup - Remove verbose debug logs that were spamming the console every frame
- ManageGameObject.cs: Check tag existence before setting; auto-create undefined tags using InternalEditorUtility.AddTag() instead of relying on exception handling (Unity logs warning, doesn't throw) - manage_gameobject.py: Remove deprecated actions (find, get_components, add_component, remove_component, set_component_property, get_component) from Literal type - these are now handled by find_gameobjects and manage_components tools - Update test suite and unit tests to reflect new auto-create behavior
Reviewer's GuideRefactors shared functionality across Unity MCP tools into centralized helper classes for serialization, type/object resolution, component operations, property conversion, and pagination, while fixing tag auto-creation, tightening prefab handling, standardizing responses, and cleaning up Python tool APIs and process detection behavior. Sequence diagram for manage_components add flow using shared helperssequenceDiagram
actor User
participant LLMClient
participant ToolServer as ManageComponents_py
participant UnityEditor
participant ManageComponents_cs as ManageComponents
participant UnityTypeResolver
participant ComponentOps
User->>LLMClient: Request add Rigidbody to Player
LLMClient->>ToolServer: manage_components(action="add", target, componentType)
ToolServer->>UnityEditor: async_send_command_with_retry(manage_components, params)
UnityEditor->>ManageComponents_cs: HandleCommand(params)
ManageComponents_cs->>ManageComponents_cs: FindTarget(targetToken, searchMethod)
ManageComponents_cs->>UnityTypeResolver: ResolveComponent(componentTypeName)
UnityTypeResolver-->>ManageComponents_cs: Type or null
alt type not found
ManageComponents_cs-->>UnityEditor: ErrorResponse("Component type not found")
else type resolved
ManageComponents_cs->>ComponentOps: AddComponent(targetGo, type, out error)
alt physics or Transform conflict
ComponentOps-->>ManageComponents_cs: null, error
ManageComponents_cs-->>UnityEditor: ErrorResponse(error)
else component added
ComponentOps-->>ManageComponents_cs: newComponent, null
ManageComponents_cs->>ManageComponents_cs: SetPropertiesOnComponent(newComponent, properties)
ManageComponents_cs-->>UnityEditor: success payload (instanceID, componentType, componentInstanceID)
end
end
UnityEditor-->>ToolServer: JSON response
ToolServer-->>LLMClient: success/error result
LLMClient-->>User: Summarized outcome
Class diagram for new shared helper architecture across Unity MCP toolsclassDiagram
class UnityJsonSerializer {
+JsonSerializer Instance
}
class PropertyConversion {
+object ConvertToType(JToken token, Type targetType)
+object TryConvertToType(JToken token, Type targetType)
+T ConvertTo~T~(JToken token)
+UnityEngine_Object LoadAssetFromToken(JToken token, Type targetType)
}
class UnityTypeResolver {
+bool TryResolve(string typeName, out Type type, out string error, Type requiredBaseType)
+Type ResolveComponent(string typeName)
+Type ResolveScriptableObject(string typeName)
+Type ResolveAny(string typeName)
}
class ObjectResolver {
+T Resolve~T~(JObject instruction)
+UnityEngine_Object Resolve(JObject instruction, Type targetType)
+GameObject ResolveGameObject(JToken target, string searchMethod)
+Material ResolveMaterial(string pathOrName)
+Texture ResolveTexture(string pathOrName)
}
class ComponentOps {
+Component AddComponent(GameObject target, Type componentType, out string error)
+bool RemoveComponent(GameObject target, Type componentType, out string error)
+bool SetProperty(Component component, string propertyName, JToken value, out string error)
+List~string~ GetAccessibleMembers(Type componentType)
}
class PaginationRequest {
+int PageSize
+int Cursor
+static PaginationRequest FromParams(JObject params, int defaultPageSize)
}
class PaginationResponse~T~ {
+List~T~ Items
+int Cursor
+int? NextCursor
+int TotalCount
+int PageSize
+bool HasMore
+static PaginationResponse~T~ Create(IList~T~ allItems, PaginationRequest request)
}
class ParamCoercion {
+int CoerceInt(JToken token, int defaultValue)
+bool CoerceBool(JToken token, bool defaultValue)
+string NormalizePropertyName(string input)
}
class AssetPathUtility {
+string NormalizeSeparators(string path)
+string SanitizeAssetPath(string path)
}
class ManageGameObject {
+object HandleCommand(JObject params)
+UnityEngine_Object FindObjectByInstruction(JObject instruction, Type targetType)
}
class ManageComponents {
+object HandleCommand(JObject params)
}
class ManageMaterial {
+object HandleCommand(JObject params)
}
class ManageAsset {
+object HandleCommand(JObject params)
}
class ManageScriptableObject {
+object HandleCommand(JObject params)
}
class FindGameObjects {
+object HandleCommand(JObject params)
}
class GameObjectLookup {
+GameObject FindByTarget(JToken target, string searchMethod, bool includeInactive)
+Type FindComponentType(string typeName)
}
class SuccessResponse {
}
class ErrorResponse {
}
UnityJsonSerializer <.. PropertyConversion : uses
UnityJsonSerializer <.. ManageGameObject : InputSerializer
UnityJsonSerializer <.. ManageMaterial : uses
UnityJsonSerializer <.. ManageAsset : uses
PropertyConversion <.. ComponentOps : uses
AssetPathUtility <.. PropertyConversion : uses
AssetPathUtility <.. ObjectResolver : uses
UnityTypeResolver <.. ManageComponents : ResolveComponent
UnityTypeResolver <.. ManageScriptableObject : ResolveAny
UnityTypeResolver <.. GameObjectLookup : ResolveComponent
ObjectResolver <.. ManageMaterial : resolve Material,Texture
ObjectResolver <.. ManageGameObject : FindObjectByInstruction
ComponentOps <.. ManageComponents : Add,Remove,SetProperty
ComponentOps <.. ManageGameObject : SetComponentPropertiesInternal
ParamCoercion <.. ComponentOps : NormalizePropertyName
ParamCoercion <.. PaginationRequest : coerce values
PaginationRequest <.. FindGameObjects : FromParams
PaginationResponse~int~ <.. FindGameObjects : Create
SuccessResponse <.. ManageMaterial : standardized responses
ErrorResponse <.. ManageMaterial : standardized responses
ErrorResponse <.. ManageGameObject : prefab,tag errors
ErrorResponse <.. ManageComponents : component errors
ErrorResponse <.. ManageAsset : property conversion errors
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughCentralizes type resolution, JSON-to-Unity conversion, object resolution, component ops, and pagination; refactors editor tools to use these helpers; tightens Windows server detection and connection retry logic; changes server tool parameter normalization and narrows manage_gameobject actions; updates tests and serializers accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManageTool as ManageGameObject/ManageMaterial
participant ObjectResolver
participant AssetDB as AssetDatabase
participant SceneLookup as GameObjectLookup/TypeResolver
participant ComponentOps
Client->>ManageTool: request (instruction with target, component, properties)
ManageTool->>ObjectResolver: resolve target (path/name/type)
ObjectResolver->>AssetDB: try load asset (if path)
alt asset found
AssetDB-->>ObjectResolver: asset
ObjectResolver-->>ManageTool: resolved asset
else not asset
ObjectResolver->>SceneLookup: search scene (method)
SceneLookup-->>ObjectResolver: GameObject / component
ObjectResolver-->>ManageTool: resolved scene object
end
ManageTool->>ComponentOps: add/set/remove component or set property (with JToken)
ComponentOps->>PropertyConversion: convert token -> target type
PropertyConversion-->>ComponentOps: converted value
ComponentOps-->>ManageTool: success / error
ManageTool-->>Client: response (SuccessResponse / ErrorResponse)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Suggested reviewers
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 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/ManageGameObject.cs:1196-1205` </location>
<code_context>
+ // Use FindObjectsOfType for Unity 2021 compatibility
</code_context>
<issue_to_address>
**issue (bug_risk):** searchInactive flag is ignored when rootSearchObject is null, so inactive objects are never returned
In the `rootSearchObject == null` path, `FindObjectsOfType(componentType)` on 2021 never returns inactive objects, so `searchInactive` is effectively ignored. The `.Where(go => searchInactive || (go != null && go.activeInHierarchy))` can only remove inactive objects when `searchInactive` is false; it can’t add them back when `searchInactive` is true.
To match the previous `FindObjectsByType(..., FindObjectsInactive.Include)` behavior, use the `FindObjectsOfType(Type, bool includeInactive)` overload instead, e.g.:
```csharp
bool includeInactive = searchInactive;
searchPoolComp = UnityEngine.Object
.FindObjectsOfType(componentType, includeInactive)
.Cast<Component>()
.Select(c => c.gameObject);
```
After that, you only need a null check, if any, rather than the current `Where` filter.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Helpers/ComponentOps.cs:150-153` </location>
<code_context>
+
+ Type type = component.GetType();
+ BindingFlags flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase;
+ string normalizedName = ParamCoercion.NormalizePropertyName(propertyName);
+
+ // Try property first
+ PropertyInfo propInfo = type.GetProperty(normalizedName, flags);
+ if (propInfo != null && propInfo.CanWrite)
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** Only using the normalized property name can break previously working property lookups
`SetProperty` now normalizes `propertyName` up front and only uses `normalizedName` for lookups. Previously (e.g. `ManageGameObject.SetProperty`) we tried both the original and normalized names.
This changes behavior and can break existing callers that pass the exact C# member name:
- `propertyName = "UseGravity"` → `NormalizePropertyName` → `"usegravity"`, which doesn’t match `"useGravity"` even with `IgnoreCase`.
To preserve backwards compatibility while still supporting normalized inputs, consider:
```csharp
string normalizedName = ParamCoercion.NormalizePropertyName(propertyName);
PropertyInfo propInfo = type.GetProperty(propertyName, flags)
?? type.GetProperty(normalizedName, flags);
```
and apply the same pattern for fields / serialized fields.
</issue_to_address>
### Comment 3
<location> `MCPForUnity/Editor/Helpers/ObjectResolver.cs:64` </location>
<code_context>
+ }
+
+ // --- Scene Object Search ---
+ GameObject foundGo = GameObjectLookup.FindByTarget(new JValue(findTerm), searchMethodToUse, includeInactive: false);
+
+ if (foundGo == null)
</code_context>
<issue_to_address>
**question (bug_risk):** Inconsistent includeInactive behavior between instruction-based and direct GameObject resolution
This path hardcodes `includeInactive: false` when calling `GameObjectLookup.FindByTarget`, while `ResolveGameObject(JToken target, ...)` relies on the default (`includeInactive: true`). As a result, identical search terms behave differently depending on whether they come from an instruction or a raw token.
If this discrepancy isn’t intentional, consider either exposing an `includeInactive` parameter on `Resolve`/`ResolveGameObject` and threading it through, or picking a single default for both call paths and documenting it so lookup behavior is consistent and predictable.
</issue_to_address>
### Comment 4
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs:404-413` </location>
<code_context>
+ public void Create_WithNewTag_AutoCreatesTag()
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that the newly created tag exists in `InternalEditorUtility.tags` to validate tag auto-creation behavior.
This will better validate the regression fix by ensuring the tag asset is actually added, not just that the GameObject ends up with the expected tag value by coincidence.
Suggested implementation:
```csharp
using NUnit.Framework;
using UnityEditorInternal;
```
To fully implement the suggestion in `Create_WithNewTag_AutoCreatesTag`, add an assertion that the auto-created tag exists in `InternalEditorUtility.tags`.
Inside the `Create_WithNewTag_AutoCreatesTag` test, after the code that triggers the create action and after you assert the GameObject's tag, add something equivalent to:
```csharp
// Assert tag was actually added to the tag manager asset
var createdTag = p["tag"]?.ToString(); // or use whatever variable holds the new tag name
Assert.That(InternalEditorUtility.tags, Does.Contain(createdTag));
```
Adjust `createdTag` to match how the test currently specifies the new tag (e.g., if you already have a `const string newTagName = "SomeTag";`, use that instead of `p["tag"]?.ToString()`).
This ensures the regression test verifies both:
1. The GameObject's tag is set correctly.
2. The tag asset was actually created in Unity’s tag manager (`InternalEditorUtility.tags`).
</issue_to_address>
### Comment 5
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs:124-126` </location>
<code_context>
Assert.Contains("useGravity", properties, "Rigidbody should have useGravity property");
// Test AI suggestions
- var suggestions = ComponentResolver.GetAIPropertySuggestions("Use Gravity", properties);
+ var suggestions = ComponentResolver.GetFuzzyPropertySuggestions("Use Gravity", properties);
Assert.Contains("useGravity", suggestions, "Should suggest useGravity for 'Use Gravity'");
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that actually set properties using human/LLM-style names (e.g. "Use Gravity", "is_kinematic") to validate the new NormalizePropertyName-based setters.
The new `NormalizePropertyName` logic is now used by `ManageGameObject` and `ComponentOps.SetProperty`, so inputs like "Use Gravity" or "is_kinematic" should map to `useGravity` / `isKinematic`. The current tests cover fuzzy suggestions only, not the actual property-setting path. Please add end-to-end tests that:
- Create a `Rigidbody` via `manage_components` or `manage_gameobject`.
- Call `set_property` / `set_component_property` with names like "Use Gravity" or "is_kinematic".
- Assert the corresponding strongly-typed properties on the component are updated.
This will verify the normalization works in real tool usage, not just suggestion generation.
Suggested implementation:
```csharp
// Test AI suggestions
var suggestions = ComponentResolver.GetFuzzyPropertySuggestions("Use Gravity", properties);
Assert.Contains("useGravity", suggestions, "Should suggest useGravity for 'Use Gravity'");
}
[Test]
public void ManageGameObject_SetsRigidbodyUseGravity_WithHumanReadableName()
{
// Arrange
var gameObject = new GameObject("TestObject_UseGravity");
var rigidbody = gameObject.AddComponent<Rigidbody>();
// Precondition: Unity's default is true, but assert for clarity
Assert.IsTrue(rigidbody.useGravity, "Precondition: Rigidbody.useGravity should start as true");
// Act: simulate tool usage with human-style property name
// This test validates that NormalizePropertyName maps "Use Gravity" -> "useGravity"
var request = new ManageGameObjectRequest
{
gameObjectName = gameObject.name,
operation = "set_component_property",
componentType = "Rigidbody",
propertyName = "Use Gravity",
propertyValue = false
};
Tools.ManageGameObject.ManageGameObject(request);
// Assert
Assert.IsFalse(rigidbody.useGravity,
"useGravity should be updated when setting property via 'Use Gravity' name");
}
[Test]
public void ManageGameObject_SetsRigidbodyIsKinematic_WithSnakeCaseName()
{
// Arrange
var gameObject = new GameObject("TestObject_IsKinematic");
var rigidbody = gameObject.AddComponent<Rigidbody>();
rigidbody.isKinematic = false;
Assert.IsFalse(rigidbody.isKinematic, "Precondition: isKinematic should start as false");
// Act: simulate tool usage with snake_case property name
// This test validates that NormalizePropertyName maps "is_kinematic" -> "isKinematic"
var request = new ManageGameObjectRequest
{
gameObjectName = gameObject.name,
operation = "set_component_property",
componentType = "Rigidbody",
propertyName = "is_kinematic",
propertyValue = true
};
Tools.ManageGameObject.ManageGameObject(request);
// Assert
Assert.IsTrue(rigidbody.isKinematic,
"isKinematic should be updated when setting property via 'is_kinematic' name");
}
foreach (var (input, expected) in testCases)
{
```
These tests assume the following, which you should verify and adjust to match your existing code:
1. **Types / namespaces**:
- `ManageGameObjectRequest` exists in the test assembly’s scope. If it lives in a namespace (e.g. `UnityMcp.Tools`), add the appropriate `using` or fully qualify it.
- `Tools.ManageGameObject.ManageGameObject(...)` is the correct entry point for the tool. If your tool is exposed differently (e.g. `ManageGameObjectTool.Execute(...)` or via a static `ToolRunner.RunManageGameObject(...)`), update the call sites accordingly.
2. **Request shape**:
- If your manage-gameobject tool uses different property names (e.g. `operationType` instead of `operation`, `component` instead of `componentType`, or wraps arguments in a nested object), align the request object initialization with your actual DTO.
3. **Rigidbody defaults**:
- If your Unity version or test setup results in different default values (e.g. `useGravity` default false in some custom configuration), you can explicitly set the starting state before assertions to keep the tests deterministic.
4. **Test framework attributes**:
- The tests use `[Test]` from NUnit. Ensure `using NUnit.Framework;` and `using UnityEngine;` are present at the top of the file if not already included.
</issue_to_address>
### Comment 6
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs:54-69` </location>
<code_context>
private static JObject ToJObject(object result)
{
if (result == null) return new JObject();
- return result as JObject ?? JObject.FromObject(result);
+ if (result is JObject jobj) return jobj;
+ try
+ {
+ return JObject.FromObject(result);
+ }
+ catch (Exception ex)
+ {
+ Debug.LogWarning($"[ToJObject] Failed to convert result: {ex.Message}");
+ return new JObject { ["error"] = ex.Message };
+ }
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a small unit test exercising the new ToJObject error path to ensure non-serializable results are handled gracefully.
Right now this new behavior isn’t asserted anywhere. Please add a targeted test that passes a non-serializable object (or one that throws during `JObject.FromObject`) and verifies that `ToJObject` returns a `JObject` containing the `"error"` field, so regressions don’t slip in unnoticed.
```suggestion
private static JObject ToJObject(object result)
{
if (result == null) return new JObject();
if (result is JObject jobj) return jobj;
try
{
return JObject.FromObject(result);
}
catch (Exception ex)
{
Debug.LogWarning($"[ToJObject] Failed to convert result: {ex.Message}");
return new JObject { ["error"] = ex.Message };
}
}
// Helper type that throws during JSON.NET serialization to exercise the ToJObject error path.
private class NonSerializableType
{
public int SafeValue => 42;
public int ThrowingProperty => throw new InvalidOperationException("Boom");
}
[Test]
public void ToJObject_ReturnsErrorField_WhenSerializationFails()
{
// Arrange
var nonSerializable = new NonSerializableType();
// Act
var jObject = ToJObject(nonSerializable);
// Assert
Assert.IsNotNull(jObject);
Assert.IsTrue(jObject.ContainsKey("error"),
"Expected JObject to contain an 'error' field when serialization fails.");
var errorValue = jObject["error"];
Assert.IsNotNull(errorValue);
Assert.IsInstanceOf<JValue>(errorValue);
Assert.IsNotEmpty(errorValue.ToString());
}
#region Bulk GameObject Creation
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| // --- Scene Object Search --- | ||
| GameObject foundGo = GameObjectLookup.FindByTarget(new JValue(findTerm), searchMethodToUse, includeInactive: false); |
There was a problem hiding this comment.
question (bug_risk): Inconsistent includeInactive behavior between instruction-based and direct GameObject resolution
This path hardcodes includeInactive: false when calling GameObjectLookup.FindByTarget, while ResolveGameObject(JToken target, ...) relies on the default (includeInactive: true). As a result, identical search terms behave differently depending on whether they come from an instruction or a raw token.
If this discrepancy isn’t intentional, consider either exposing an includeInactive parameter on Resolve/ResolveGameObject and threading it through, or picking a single default for both call paths and documenting it so lookup behavior is consistent and predictable.
| // Test AI suggestions | ||
| var suggestions = ComponentResolver.GetAIPropertySuggestions("Use Gravity", properties); | ||
| var suggestions = ComponentResolver.GetFuzzyPropertySuggestions("Use Gravity", properties); | ||
| Assert.Contains("useGravity", suggestions, "Should suggest useGravity for 'Use Gravity'"); |
There was a problem hiding this comment.
suggestion (testing): Add tests that actually set properties using human/LLM-style names (e.g. "Use Gravity", "is_kinematic") to validate the new NormalizePropertyName-based setters.
The new NormalizePropertyName logic is now used by ManageGameObject and ComponentOps.SetProperty, so inputs like "Use Gravity" or "is_kinematic" should map to useGravity / isKinematic. The current tests cover fuzzy suggestions only, not the actual property-setting path. Please add end-to-end tests that:
- Create a
Rigidbodyviamanage_componentsormanage_gameobject. - Call
set_property/set_component_propertywith names like "Use Gravity" or "is_kinematic". - Assert the corresponding strongly-typed properties on the component are updated.
This will verify the normalization works in real tool usage, not just suggestion generation.
Suggested implementation:
// Test AI suggestions
var suggestions = ComponentResolver.GetFuzzyPropertySuggestions("Use Gravity", properties);
Assert.Contains("useGravity", suggestions, "Should suggest useGravity for 'Use Gravity'");
}
[Test]
public void ManageGameObject_SetsRigidbodyUseGravity_WithHumanReadableName()
{
// Arrange
var gameObject = new GameObject("TestObject_UseGravity");
var rigidbody = gameObject.AddComponent<Rigidbody>();
// Precondition: Unity's default is true, but assert for clarity
Assert.IsTrue(rigidbody.useGravity, "Precondition: Rigidbody.useGravity should start as true");
// Act: simulate tool usage with human-style property name
// This test validates that NormalizePropertyName maps "Use Gravity" -> "useGravity"
var request = new ManageGameObjectRequest
{
gameObjectName = gameObject.name,
operation = "set_component_property",
componentType = "Rigidbody",
propertyName = "Use Gravity",
propertyValue = false
};
Tools.ManageGameObject.ManageGameObject(request);
// Assert
Assert.IsFalse(rigidbody.useGravity,
"useGravity should be updated when setting property via 'Use Gravity' name");
}
[Test]
public void ManageGameObject_SetsRigidbodyIsKinematic_WithSnakeCaseName()
{
// Arrange
var gameObject = new GameObject("TestObject_IsKinematic");
var rigidbody = gameObject.AddComponent<Rigidbody>();
rigidbody.isKinematic = false;
Assert.IsFalse(rigidbody.isKinematic, "Precondition: isKinematic should start as false");
// Act: simulate tool usage with snake_case property name
// This test validates that NormalizePropertyName maps "is_kinematic" -> "isKinematic"
var request = new ManageGameObjectRequest
{
gameObjectName = gameObject.name,
operation = "set_component_property",
componentType = "Rigidbody",
propertyName = "is_kinematic",
propertyValue = true
};
Tools.ManageGameObject.ManageGameObject(request);
// Assert
Assert.IsTrue(rigidbody.isKinematic,
"isKinematic should be updated when setting property via 'is_kinematic' name");
}
foreach (var (input, expected) in testCases)
{These tests assume the following, which you should verify and adjust to match your existing code:
-
Types / namespaces:
ManageGameObjectRequestexists in the test assembly’s scope. If it lives in a namespace (e.g.UnityMcp.Tools), add the appropriateusingor fully qualify it.Tools.ManageGameObject.ManageGameObject(...)is the correct entry point for the tool. If your tool is exposed differently (e.g.ManageGameObjectTool.Execute(...)or via a staticToolRunner.RunManageGameObject(...)), update the call sites accordingly.
-
Request shape:
- If your manage-gameobject tool uses different property names (e.g.
operationTypeinstead ofoperation,componentinstead ofcomponentType, or wraps arguments in a nested object), align the request object initialization with your actual DTO.
- If your manage-gameobject tool uses different property names (e.g.
-
Rigidbody defaults:
- If your Unity version or test setup results in different default values (e.g.
useGravitydefault false in some custom configuration), you can explicitly set the starting state before assertions to keep the tests deterministic.
- If your Unity version or test setup results in different default values (e.g.
-
Test framework attributes:
- The tests use
[Test]from NUnit. Ensureusing NUnit.Framework;andusing UnityEngine;are present at the top of the file if not already included.
- The tests use
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/manage_gameobject.py (1)
175-179: Error message lists deprecated actions that are no longer valid.The error message at line 178 still references
find,add_component,remove_component,set_component_property,get_components, andget_component, but these actions were removed from theLiteraltype at line 94. This will confuse users.🔎 Proposed fix to align error message with valid actions
if action is None: return { "success": False, - "message": "Missing required parameter 'action'. Valid actions: create, modify, delete, find, add_component, remove_component, set_component_property, get_components, get_component, duplicate, move_relative" + "message": "Missing required parameter 'action'. Valid actions: create, modify, delete, duplicate, move_relative. For finding GameObjects use find_gameobjects tool. For component operations use manage_components tool." }
🤖 Fix all issues with AI Agents
In @MCPForUnity/Editor/Tools/ManageGameObject.cs:
- Around line 336-363: The ternary assignment to tagToSet is redundant because
the outer condition guarantees tag is non-empty; replace the conditional
assignment in the Create block by using tag directly (or initialize tagToSet =
tag once before the outer check), keeping the existing tag existence check
(InternalEditorUtility.tags), AddTag call, error handling that destroys newGo
and returns ErrorResponse, and the subsequent newGo.tag assignment and its
try/catch unchanged.
- Around line 1787-1795: The wrapper ConvertJTokenToType currently ignores its
inputSerializer parameter while callers pass a serializer expecting it to be
used; either (A) update the five callers' inline comments to state that the
serializer is ignored and remove the unused parameter from ConvertJTokenToType,
or (B) propagate the serializer into PropertyConversion by adding an
overload/parameter to PropertyConversion.ConvertToType that accepts
JsonSerializer, update ConvertJTokenToType to call that new overload (passing
the inputSerializer), and update PropertyConversion implementation to honor the
serializer during conversion; ensure you update all callers that currently pass
a serializer to remain consistent.
In @Server/src/services/tools/manage_shader.py:
- Around line 15-18: The ToolAnnotations for the manage_shader tool is missing
the destructiveHint flag; restore destructiveHint=True in the annotations for
manage_shader (i.e., update the ToolAnnotations(...) used when declaring the
manage_shader tool) so the framework and MCP clients will show
destructive-operation warnings; keep the existing comment about 'read' vs
'create/update/delete' but add destructiveHint=True to the ToolAnnotations
instantiation.
In
@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs:
- Around line 395-410: The test Modify_NewTag_AutoCreatesTag auto-creates a tag
via ManageGameObject.HandleCommand but TearDown does not remove it, polluting
the TagManager; update the TearDown() method to remove any tags created by tests
(at minimum remove "AutoModifyTag12345") by tracking created test tags in the
test and removing them from the editor TagManager (e.g., via
UnityEditor/UnityEditorInternal tag APIs or resetting tags to their prior state)
so each test run leaves TagManager unchanged.
🧹 Nitpick comments (10)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (1)
97-98: Performance threshold increased for CI stability.Doubling the threshold to 10 seconds is a reasonable accommodation for CI environment variability. The explanatory comment is helpful.
Consider tracking actual performance metrics over time (e.g., in CI logs or a dashboard) to detect gradual performance degradation that might be masked by generous thresholds.
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
1033-1055: Consider using Get-CimInstance for consistency with line 170.The logic is sound and the two-step validation (tasklist → wmic) is a good approach. However:
wmic is deprecated: Line 169-170 in
TryProcessCommandLineContainsInstanceTokenusesGet-CimInstance Win32_Processspecifically because "wmic is deprecated." For consistency and future-proofing, consider using the same approach here.Minor pattern inconsistency: Lines 1040-1044 use
wmicCompactfor matching, but line 1045 useswmicCombined. While "uvicorn" doesn't contain characters thatNormalizeForMatchwould affect, usingwmicCompactconsistently would make the code more uniform.🔎 Suggested alignment with PowerShell approach
- // Step 2: Try to get command line with wmic for better validation - ExecPath.TryRun("cmd.exe", $"/c wmic process where \"ProcessId={pid}\" get CommandLine /value", Application.dataPath, out var wmicOut, out var wmicErr, 5000); - string wmicCombined = ((wmicOut ?? string.Empty) + "\n" + (wmicErr ?? string.Empty)).ToLowerInvariant(); - string wmicCompact = NormalizeForMatch(wmicOut ?? string.Empty); - - // If we can see the command line, validate it's our server - if (!string.IsNullOrEmpty(wmicCombined) && wmicCombined.Contains("commandline=")) + // Step 2: Try to get command line with Get-CimInstance for better validation (wmic is deprecated) + string ps = $"(Get-CimInstance Win32_Process -Filter \\\"ProcessId={pid}\\\").CommandLine"; + ExecPath.TryRun("powershell", $"-NoProfile -Command \"{ps}\"", Application.dataPath, out var cmdLineOut, out var cmdLineErr, 5000); + string cmdLineCombined = ((cmdLineOut ?? string.Empty) + "\n" + (cmdLineErr ?? string.Empty)).ToLowerInvariant(); + string cmdLineCompact = NormalizeForMatch(cmdLineOut ?? string.Empty); + + // If we can see the command line, validate it's our server + if (!string.IsNullOrWhiteSpace(cmdLineOut)) { - bool mentionsMcp = wmicCompact.Contains("mcp-for-unity") - || wmicCompact.Contains("mcp_for_unity") - || wmicCompact.Contains("mcpforunity") - || wmicCompact.Contains("mcpforunityserver"); - bool mentionsTransport = wmicCompact.Contains("--transporthttp") || (wmicCompact.Contains("--transport") && wmicCompact.Contains("http")); - bool mentionsUvicorn = wmicCombined.Contains("uvicorn"); + bool mentionsMcp = cmdLineCompact.Contains("mcp-for-unity") + || cmdLineCompact.Contains("mcp_for_unity") + || cmdLineCompact.Contains("mcpforunity") + || cmdLineCompact.Contains("mcpforunityserver"); + bool mentionsTransport = cmdLineCompact.Contains("--transporthttp") || (cmdLineCompact.Contains("--transport") && cmdLineCompact.Contains("http")); + bool mentionsUvicorn = cmdLineCompact.Contains("uvicorn");MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
550-594: Retry logic improvements look good, with one minor concern.The tiered delay strategy and increased retry count are sensible for Windows/dev environments with slower startup. The early-return-on-success pattern improves clarity.
However, the fallback path (lines 574-586) will attempt
bridgeService.StartAsync()on every iteration from attempt 20 to 29, potentially logging multiple connection errors if the server truly isn't responding. Consider adding a return or break after a few failed fallback attempts to avoid spamming errors:🔎 Suggested improvement to limit fallback attempts
else if (attempt >= 20) { // After many attempts without detection, try connecting anyway as a last resort. // This handles cases where process detection fails but the server is actually running. - // Only try a few times to avoid spamming connection errors. + // Only try once every few attempts to avoid spamming connection errors. + if ((attempt - 20) % 3 != 0) continue; // Try at attempts 20, 23, 26, 29 bool started = await bridgeService.StartAsync(); if (started) { await VerifyBridgeConnectionAsync(); UpdateConnectionStatus(); return; } }Server/src/services/tools/manage_gameobject.py (1)
209-223: Dead code: validation for removed "find" action.Since
"find"is no longer a valid action (removed from theLiteralat line 94), this validation block will never execute. Consider removing it to avoid confusion during maintenance.🔎 Proposed removal of dead code
- # Map tag to search_term when search_method is by_tag for backward compatibility - if action == "find" and search_method == "by_tag" and tag is not None and search_term is None: - search_term = tag - - # Validate parameter usage to prevent silent failures - if action == "find": - if name is not None: - return { - "success": False, - "message": "For 'find' action, use 'search_term' parameter, not 'name'. Remove 'name' parameter. Example: search_term='Player', search_method='by_name'" - } - if search_term is None: - return { - "success": False, - "message": "For 'find' action, 'search_term' parameter is required. Use search_term (not 'name') to specify what to find." - }MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
163-189: Implementation looks correct; consider grouping with other vector converters.The
Vector4Converterimplementation correctly mirrors the pattern used inVector2ConverterandVector3Converter, with proper serialization/deserialization of x, y, z, w components.However, for better code organization, consider placing this converter near the other vector types (after
Vector3Converterat line 34 or beforeQuaternionConverterat line 58) rather than afterBoundsConverter. This would improve maintainability by keeping related converters grouped together.📍 Suggested placement
Move the
Vector4Converterclass to appear right afterVector3Converter(after line 34) to maintain logical grouping of vector types.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
404-422: Test correctly validates the new auto-tag creation behavior.The test accurately reflects the updated functionality where tags are auto-created instead of failing on missing tags. The assertions verify both command success and that the tag is properly assigned.
Consider tag cleanup: The
TearDownmethod cleans up created GameObjects but doesn't remove auto-created tags from the project's tag list. If tests run repeatedly, this could accumulate orphan tags inInternalEditorUtility.tags. This is a minor concern since test tags use unique names and won't affect production.MCPForUnity/Editor/Tools/ManageComponents.cs (1)
107-117: Consider using SuccessResponse for consistency.Success returns use anonymous objects while errors use
ErrorResponse. For consistency withManageMaterial.cswhich usesSuccessResponse/ErrorResponsethroughout, consider wrapping success returns:- return new - { - success = true, - message = $"Component '{componentTypeName}' added to '{targetGo.name}'.", - data = new { ... } - }; + return new SuccessResponse( + $"Component '{componentTypeName}' added to '{targetGo.name}'.", + new { ... } + );This is a minor consistency suggestion that could be deferred.
Also applies to: 150-158
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (1)
194-207: Duplicate CompilationPipeline call could be optimized.
PreferPlayercallsCompilationPipeline.GetAssemblies(AssembliesType.Player)again afterFindCandidatesalready computed the player assembly names. For typical usage viaTryResolve, this results in redundant work when the TypeCache path (lines 76-91) is reached.Consider caching the player assembly names or passing them as a parameter if this becomes a performance concern. Given type resolution is typically cached, this is a minor optimization.
MCPForUnity/Editor/Helpers/PropertyConversion.cs (1)
33-53: Consider consolidating exception handlers.All three catch blocks log and rethrow. Since the handling logic is identical (log + rethrow), you could simplify to a single catch block:
-catch (JsonSerializationException jsonEx) -{ - Debug.LogError($"JSON Deserialization Error converting token to {targetType.FullName}: {jsonEx.Message}\nToken: {token.ToString(Formatting.None)}"); - throw; -} -catch (ArgumentException argEx) -{ - Debug.LogError($"Argument Error converting token to {targetType.FullName}: {argEx.Message}\nToken: {token.ToString(Formatting.None)}"); - throw; -} -catch (Exception ex) -{ - Debug.LogError($"Unexpected error converting token to {targetType.FullName}: {ex}\nToken: {token.ToString(Formatting.None)}"); - throw; -} +catch (Exception ex) +{ + Debug.LogError($"Error converting token to {targetType.FullName}: {ex.Message}\nToken: {token.ToString(Formatting.None)}"); + throw; +}MCPForUnity/Editor/Helpers/ObjectResolver.cs (1)
64-64: Consider exposingincludeInactiveas a configurable instruction parameter.The
includeInactiveparameter is hardcoded tofalseon line 64. While the underlyingGameObjectLookup.FindByTargetmethod supports this parameter,ObjectResolver.Resolve()does not expose it as a configurable instruction field. Other similar utilities in the codebase (e.g.,FindGameObjects,ManageGameObject,ManagePrefabs) allow callers to control this behavior. For consistency and flexibility, consider acceptingincludeInactiveas an optional instruction parameter and passing it through to the lookup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
.claude/prompts/nl-gameobject-suite.md.github/workflows/claude-gameobject-suite.yml.github/workflows/claude-nl-suite.ymlMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/ComponentOps.cs.metaMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/ObjectResolver.cs.metaMCPForUnity/Editor/Helpers/Pagination.csMCPForUnity/Editor/Helpers/Pagination.cs.metaMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/PropertyConversion.cs.metaMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.cs.metaMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Helpers/UnityTypeResolver.cs.metaMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Tools/FindGameObjects.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.csREADME.mdServer/src/services/tools/batch_execute.pyServer/src/services/tools/execute_menu_item.pyServer/src/services/tools/find_gameobjects.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_shader.pyServer/src/services/tools/utils.pyServer/tests/integration/test_manage_asset_json_parsing.pyTestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.csTestProjects/UnityMCPTests/Assets/Scripts/TestScriptableObjectInstance.asset.metaTestProjects/UnityMCPTests/Assets/Temp.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs
💤 Files with no reviewable changes (1)
- Server/src/services/tools/execute_menu_item.py
🧰 Additional context used
🧠 Learnings (8)
📓 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.
📚 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/PropertyConversion.cs.metaMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.cs.metaMCPForUnity/Editor/Helpers/Pagination.cs.metaMCPForUnity/Editor/Helpers/ObjectResolver.cs.metaMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageComponents.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:
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs.metaMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Helpers/ComponentOps.cs.metaMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Helpers/ObjectResolver.cs.metaMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/GameObjectLookup.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/Helpers/ParamCoercion.csMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/FindGameObjects.csMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/Pagination.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/GameObjectLookup.cs
📚 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:
TestProjects/UnityMCPTests/Assets/Temp.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:
Server/src/services/tools/manage_material.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_components.py
📚 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/ManageGameObjectTests.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/manage_asset.py
🧬 Code graph analysis (21)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(30-122)
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (3)
MCPForUnity/Editor/Tools/ManageGameObject.cs (2)
Type(1910-1924)TryResolve(1937-1940)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
Type(288-291)MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
Type(893-896)
Server/src/services/tools/manage_material.py (1)
Server/src/services/tools/utils.py (3)
parse_json_payload(27-60)coerce_int(63-77)normalize_properties(80-112)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs (1)
MCPForUnity/Editor/Helpers/Pagination.cs (4)
PaginationRequest(11-66)PaginationRequest(28-65)PaginationResponse(73-147)PaginationResponse(117-146)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
ComponentResolver(1931-2067)
MCPForUnity/Editor/Helpers/Pagination.cs (1)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (3)
ParamCoercion(11-200)CoerceInt(19-45)T(137-157)
MCPForUnity/Editor/Tools/ManageMaterial.cs (3)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (4)
Material(133-140)ObjectResolver(12-199)Texture(145-152)GameObject(109-128)MCPForUnity/Editor/Helpers/MaterialOps.cs (4)
MaterialOps(12-396)TrySetShaderProperty(207-340)Color(345-395)ApplyProperties(17-155)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)
Server/src/services/tools/find_gameobjects.py (1)
Server/tests/integration/test_helpers.py (1)
model_dump(10-13)
Server/src/services/tools/manage_asset.py (1)
Server/src/services/tools/utils.py (3)
parse_json_payload(27-60)coerce_int(63-77)normalize_properties(80-112)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs (5)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs (1)
JObject(38-41)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs (1)
JObject(40-43)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (1)
JObject(67-70)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (1)
JObject(79-82)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
JObject(81-90)
Server/src/services/tools/manage_components.py (1)
Server/src/services/tools/utils.py (2)
parse_json_payload(27-60)normalize_properties(80-112)
MCPForUnity/Editor/Helpers/PropertyConversion.cs (4)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
UnityEngine(31-104)UnityEngine(169-198)T(20-23)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (3)
Type(103-108)Type(113-118)Type(123-128)MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-212)SanitizeAssetPath(31-45)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (8)
Vector2Converter(36-56)Vector3Converter(11-34)Vector4Converter(163-189)QuaternionConverter(58-84)ColorConverter(86-112)RectConverter(114-140)BoundsConverter(142-161)UnityEngineObjectConverter(253-354)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
MCPForUnity/Editor/Helpers/ExecPath.cs (2)
ExecPath(12-272)TryRun(161-227)
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/PropertyConversion.cs (2)
PropertyConversion(13-100)TryConvertToType(59-69)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/ServerManagementService.cs (1)
IsLocalHttpServerRunning(538-598)MCPForUnity/Editor/Services/IServerManagementService.cs (1)
IsLocalHttpServerRunning(36-36)
MCPForUnity/Editor/Tools/ManageGameObject.cs (6)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (6)
Type(103-108)Type(113-118)Type(123-128)TryResolve(32-98)UnityTypeResolver(18-215)List(150-185)MCPForUnity/Editor/Helpers/ObjectResolver.cs (4)
UnityEngine(31-104)UnityEngine(169-198)GameObject(109-128)ObjectResolver(12-199)MCPForUnity/Editor/Helpers/PropertyConversion.cs (3)
UnityEngine(85-99)PropertyConversion(13-100)ConvertToType(21-53)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)NormalizePropertyName(170-199)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
JObject(98-149)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(30-122)
MCPForUnity/Editor/Tools/ManageComponents.cs (4)
MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)CoerceString(121-128)MCPForUnity/Editor/Tools/ManageGameObject.cs (2)
Type(1910-1924)SetProperty(1493-1575)MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (4)
Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)MCPForUnity/Editor/Helpers/ComponentOps.cs (4)
Component(23-73)ComponentOps(14-302)RemoveComponent(82-122)SetProperty(132-223)
MCPForUnity/Editor/Helpers/ObjectResolver.cs (3)
MCPForUnity/Editor/Tools/ManageGameObject.cs (3)
UnityEngine(1900-1903)Type(1910-1924)GameObject(1071-1094)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
JObject(98-149)AssetPathUtility(15-212)NormalizeSeparators(21-26)MCPForUnity/Editor/Helpers/GameObjectLookup.cs (4)
Type(288-291)GameObject(57-64)GameObject(69-72)GameObjectLookup(15-312)
MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (4)
Type(103-108)Type(113-118)Type(123-128)UnityTypeResolver(18-215)
⏰ 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 (69)
TestProjects/UnityMCPTests/Assets/Scripts/TestScriptableObjectInstance.asset.meta (1)
1-8: Standard Unity meta file—no review needed.This is an auto-generated Unity metadata file that accompanies the test asset. The format is correct and should be committed to version control alongside its corresponding
.assetfile.Server/src/services/tools/manage_script.py (1)
515-516: Documentation update accurately reflects action deprecation.The change from "C# code for 'create'/'update'" to "C# code for 'create' action" correctly aligns with the removal of the 'update' action from the
manage_scriptAPI. Theactionparameter (line 512) only accepts'create','read', or'delete', and thecontentsparameter is only used whenaction == 'create'(line 536), so the updated annotation is now accurate..github/workflows/claude-gameobject-suite.yml (1)
476-477: LGTM! Improved error visibility for test result parsing.Changing from silent exception suppression to logging warnings improves observability when test result fragments fail to parse, while still allowing the script to continue processing remaining fragments. This aligns well with the PR's broader goal of explicit error reporting.
.claude/prompts/nl-gameobject-suite.md (1)
118-123: LGTM! Documentation accurately reflects the API cleanup.The section rename and updated flow correctly document that legacy actions have been removed (not just deprecated), with clear pass criteria expecting error responses. This aligns with the PR's objective to remove deprecated manage_gameobject actions.
.github/workflows/claude-nl-suite.yml (1)
238-243: LGTM! Correct numeric sorting for GO- test IDs.*The new sorting branch ensures GO-* tests are ordered numerically (GO-0, GO-1, ..., GO-10) rather than lexically. The fallback to 999 on parse failure is appropriate for error handling.
TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs (1)
24-26: LGTM! Improved naming consistency.The animator parameter names now align better with the "reach" theme used throughout the script (e.g.,
reachOriginfield on Line 9). The comment correctly documents the new parameter mapping, and the usage inApplyBlend()remains consistent.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectAPIStressTests.cs (1)
57-66: LGTM! Robust error handling for test utility.The try-catch wrapper around
JObject.FromObjectwith fallback error logging is a solid improvement. This prevents test failures from propagating as unhandled exceptions while still providing diagnostic information through the warning log and error object.MCPForUnity/Editor/Services/ServerManagementService.cs (1)
948-977: LGTM! Robust Windows port discovery.Running
netstat.exe -anodirectly and filtering in C# is a solid improvement. The previous pipe tofindstrcaused exit code 1 when no matches were found, whichExecPath.TryRuninterpreted as failure.Key improvements:
- Validating
localAddr.EndsWith(portSuffix)prevents false matches (e.g., port 80 matching:8080).- Processing stdout regardless of the success flag handles edge cases where netstat still produces valid output.
Server/src/services/tools/batch_execute.py (1)
23-24: LGTM! Clear documentation of batch size limit.The explicit mention of the 25-command limit in the description improves user experience by setting expectations upfront, consistent with the
MAX_COMMANDS_PER_BATCHconstant and validation logic.MCPForUnity/Editor/Helpers/UnityTypeResolver.cs.meta (1)
1-11: Standard Unity asset metadata.This metadata file accompanies the new
UnityTypeResolver.cshelper class. Standard UnityMonoImporterconfiguration with default settings.Server/src/services/tools/manage_editor.py (1)
49-51: LGTM! Enables required parameters for editor actions.The inclusion of
toolName,tagName, andlayerNamein the params dictionary enables the tool to properly support actions likeset_active_tool,add_tag,remove_tag,add_layer, andremove_layer. The None-filtering on line 53 ensures only provided parameters are sent.TestProjects/UnityMCPTests/Assets/Temp.meta (1)
1-8: Standard Unity folder metadata.Standard Unity metadata file for a temporary assets folder. No issues.
MCPForUnity/Editor/Tools/ManageScene.cs (1)
399-400: No action needed. The code change correctly usesFindObjectsOfTypebecauseFindObjectsByTypewas not introduced until Unity 2022; it is not available in Unity 2021. The comment accurately reflects this compatibility requirement.MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)
17-26: LGTM! Good refactoring for path normalization.Extracting the separator normalization logic into a dedicated method improves code reusability and maintainability. The early return for null/empty input and the use of forward slashes align with Unity's path conventions.
Server/src/services/tools/find_gameobjects.py (1)
43-52: LGTM! Better error handling with early parameter validation.Moving the
search_termvalidation before the preflight I/O operations is a good practice. This provides faster feedback for missing required parameters and avoids unnecessary I/O when the request is invalid.MCPForUnity/Editor/Helpers/ParamCoercion.cs (1)
159-199: LGTM! Robust property name normalization for fuzzy matching.The implementation correctly handles common naming variations (snake_case, kebab-case, space-separated) and converts them to camelCase. The use of
RemoveEmptyEntriesprevents edge cases with consecutive separators, and theStringBuilderensures efficient string concatenation.Note that this normalization assumes camelCase is the target format for all inputs, which is appropriate for Unity's serialized property naming conventions and aligns with the fuzzy property matching use case described in the PR.
README.md (1)
355-355: The documentation is accurate. The batch_execute implementation confirms thatMAX_COMMANDS_PER_BATCH = 25(line 14 ofServer/src/services/tools/batch_execute.py), and this limit is both documented in the tool description and enforced in the validation logic (lines 44-46). No changes needed.Server/src/services/tools/manage_gameobject.py (1)
86-94: API cleanup looks good - actions consolidated appropriately.The description update and narrowed action set align with the PR's goal of separating concerns into dedicated tools (
find_gameobjects,manage_components).Server/src/services/tools/manage_material.py (2)
12-12: Good consolidation using shared utility.Replacing the local
_normalize_propertieswith the sharednormalize_propertiesfromutils.pyreduces duplication and ensures consistent validation behavior across tools.
98-100: LGTM!The call site correctly uses the shared utility with the same tuple return pattern for error handling.
Server/tests/integration/test_manage_asset_json_parsing.py (1)
58-60: Test assertion correctly updated to match new error format.The assertion now aligns with the error message from the shared
normalize_propertiesutility inutils.py, which returns"properties must be..."for invalid inputs.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (1)
146-147: LGTM!The assertion correctly updated to use the boolean
successfield, aligning with the standardized response pattern (SuccessResponse/ErrorResponse) introduced across the codebase.MCPForUnity/Editor/Helpers/ObjectResolver.cs.meta (1)
1-11: Standard Unity meta file for new helper class.No issues with this auto-generated meta file.
MCPForUnity/Editor/Helpers/PropertyConversion.cs.meta (1)
1-11: Standard Unity meta file for new helper class.No issues with this auto-generated meta file.
Server/src/services/tools/manage_components.py (1)
12-12: LGTM! Good consolidation of shared utility.The refactoring to use the centralized
normalize_propertiesutility fromservices.tools.utilsaligns well with the PR's goal of reducing duplication. The function signature and error handling pattern are correct.Also applies to: 85-85
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs (2)
87-87: LGTM: Response format migration to boolean success flag.The assertions correctly check the new boolean
successfield and include helpful debugging context viaresult.ToString(). This is cleaner than the previous string-based status checks.Also applies to: 112-112, 143-143, 184-184
218-223: LGTM: Correct handling of new data wrapper pattern.The test properly validates the presence of the
dataobject before accessing nested properties, with appropriate null assertions.MCPForUnity/Editor/Tools/ManageScriptableObject.cs (1)
890-896: LGTM: Good refactoring to centralize type resolution.Delegating to
UnityTypeResolver.ResolveAny()reduces duplication and ensures consistent type resolution across the codebase. The XML documentation clearly indicates the delegation.Server/src/services/tools/manage_asset.py (1)
13-13: LGTM: Refactoring to use centralized property normalization.The change delegates to the shared
normalize_propertiesutility fromutils.py, reducing code duplication. The error handling pattern is preserved.Also applies to: 63-63
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
1-32: LGTM: Well-designed centralized serializer for Unity types.The shared
JsonSerializerinstance with Unity type converters is appropriate for the Unity Editor environment (single-threaded). This eliminates duplicate serializer configurations across tools and ensures consistent JSON handling for Unity types.MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
285-291: LGTM: Consistent refactoring to centralize component type resolution.Delegating to
UnityTypeResolver.ResolveComponent()is consistent with the broader refactoring pattern and reduces code duplication. The XML remarks appropriately document this delegation.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs (1)
64-69: LGTM!The assertions correctly align with the new response schema using boolean
successand stringerrorfields. The test properly validates both the failure condition and the error message content.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (2)
83-84: LGTM!The assertions correctly validate the new response schema with boolean
successand stringerrorfields.
94-95: Consistent response schema updates.All remaining assertions correctly use the boolean
successfield anderrorstring, maintaining test coverage while aligning with the new response format.Also applies to: 111-112, 140-140, 171-171, 186-186
Server/src/services/tools/utils.py (1)
80-112: Well-designed utility function with robust error handling.The
normalize_propertiesfunction handles the common edge cases from MCP clients/LLMs effectively:
- Explicit checks for serialization artifacts (
"[object Object]","undefined") provide clear error messages.- The tuple return pattern makes error handling explicit at call sites.
- Good documentation of the expected behavior.
The implementation correctly covers dict, string, and invalid type inputs.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
125-126: Method rename aligns with refactor.The change from
GetAIPropertySuggestionstoGetFuzzyPropertySuggestionscorrectly reflects the transition to rule-based fuzzy matching as shown in the relevant code snippet fromManageGameObject.cs.
559-633: Good test coverage for prefab asset handling.The new prefab handling tests thoroughly validate the expected behavior:
modifyanddeleteactions on prefab paths return guidance errors pointing tomanage_asset/manage_prefabs.createaction withprefab_pathis not blocked by the prefab check.The test at line 605-631 correctly uses a defensive assertion pattern - it accepts either success or a non-redirection error, which handles the case where the prefab doesn't exist.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs (4)
57-59: LGTM!The assertion correctly uses the boolean
successfield and includes the full result in the failure message for debugging.
78-80: Consistent response schema updates.Both assertions correctly validate success using the boolean field.
Also applies to: 96-97
115-122: Good error validation.The test correctly checks for failure via boolean
successand reads the error message from theerrorfield, maintaining the validation that the error contains exception details.
143-146: Appropriate handling for nullable success field.Using
Value<bool?>("success")correctly handles edge cases where the response might have different structures. The assertion validates the response format without being overly strict about the outcome.MCPForUnity/Editor/Tools/FindGameObjects.cs (1)
40-63: Good adoption of centralized pagination utilities.The refactor correctly uses
PaginationRequest.FromParamsandPaginationResponse<T>.Createfor standardized pagination handling. The response structure includes all expected fields for consistent API behavior. The clamping ofPageSizeto the valid range (1-500) is appropriate and safe.MCPForUnity/Editor/Tools/ManageMaterial.cs (4)
57-71: Path normalization implementation is sound.The
NormalizePathmethod correctly sanitizes paths and ensures.matextension. This is a good pattern for consistent asset path handling.
485-490: Defensive path validation is reasonable.The explicit check for
Assets/prefix after sanitization provides defense-in-depth. WhileSanitizeAssetPathshould guarantee this, the redundant check guards against future regressions in the utility or unexpected edge cases.
73-126: Clean refactor using unified helpers.The method correctly delegates material resolution to
ObjectResolverand property setting toMaterialOps. The special handling for texture instructions (lines 99-112) preserves ManageMaterial's advanced feature while standardizing the rest.
326-342: Instance mode includes appropriate user warning.The warning at line 339 correctly informs users that material instantiation cannot be fully reverted by Undo. This transparency about the limitation is good practice.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PaginationTests.cs (2)
1-89: Comprehensive PaginationRequest tests.The test suite thoroughly covers:
- Both naming conventions (
page_size/pageSize,cursor/page_number)- Cursor precedence over page_number
- Default value handling for null/empty params
- Custom default page size
- String value coercion
This provides good regression protection for the pagination API.
95-204: Thorough PaginationResponse edge case coverage.Tests validate correct behavior for:
- First, middle, and last page extraction
- Empty input lists
- Cursor clamping (both negative and beyond-bounds)
- HasMore/NextCursor consistency
- TotalCount accuracy
These tests will catch regressions in the pagination slicing logic.
MCPForUnity/Editor/Tools/ManageAsset.cs (3)
218-218: Correct serializer usage for material properties.The switch to
UnityJsonSerializer.Instanceensures consistent handling of Unity types (Vector3, Color, etc.) when applying material properties during asset creation.
356-412: Well-structured component modification for GameObject assets.The implementation correctly:
- Iterates properties as
{ComponentName: {prop: value}}structure- Resolves component types via
ComponentResolver.TryResolve- Only warns when both type resolution fails AND component not found
- Accumulates modification results with
|=operatorThis enables prefab asset modification through the unified
manage_assettool.
992-1016: Property conversion with null-safety check.The
TryConvertToTypeusage correctly handles conversion failures by checking for null results. The comparison with current value (!object.Equals(...)) prevents unnecessary property writes.Minor note: If a property legitimately needs to be set to
null, this logic would skip it. For most Unity types this is fine, but serializable reference fields might requirenullassignment. This appears to be intentional defensive behavior.MCPForUnity/Editor/Tools/ManageComponents.cs (3)
76-117: Clean refactor to use centralized component operations.The AddComponent flow correctly:
- Uses
UnityTypeResolver.ResolveComponentfor type lookup- Delegates creation to
ComponentOps.AddComponentwith physics conflict checks- Applies properties after creation with proper Undo recording
- Returns consistent response structure
128-158: RemoveComponent follows consistent patterns.The removal flow mirrors the add flow: type resolution via
UnityTypeResolver, operation viaComponentOps.RemoveComponent, and proper dirty marking. Error handling is clear and descriptive.
310-326: TrySetProperty cleanly delegates to ComponentOps.The refactored method properly delegates to
ComponentOps.SetPropertyand preserves the error logging behavior for debugging. This eliminates duplicate reflection-based property setting logic.MCPForUnity/Editor/Helpers/Pagination.cs (2)
11-66: Well-designed PaginationRequest with flexible parameter handling.The implementation correctly:
- Accepts both
page_size/pageSizeandcursor/page_numbernaming- Gives cursor precedence when both provided
- Converts 1-based page_number to 0-based cursor
- Validates PageSize > 0 with fallback to default
This provides a robust API for paginated tool operations.
117-146: PaginationResponse.Create handles edge cases correctly.The slicing logic:
- Clamps cursor to
[0, totalCount]range (line 124-125)- Uses explicit loop for clarity
- Computes NextCursor as
nullfor last pageThe choice to clamp
cursor > totalCounttototalCount(nottotalCount - 1) means out-of-range cursors return an empty page rather than the last item. This matches the test expectations inPaginationTests.cs(line 191 assertsCursor = 3for a 3-item list with cursor=100).MCPForUnity/Editor/Helpers/UnityTypeResolver.cs (3)
18-48: Solid type resolution with caching and constraint checking.The implementation provides efficient caching with two lookup paths (FQN and short name). The constraint check via
PassesConstraintensures cached types still match required base types.Note on short name caching (line 47): If two different types share the same short name (e.g.,
Foo.ComponentandBar.Component), the cache will store whichever is resolved first. Subsequent lookups for the same short name with different constraints might return the wrong cached type. However, the constraint check at line 47 (PassesConstraint(type, requiredBaseType)) mitigates this by falling through to full resolution if the cached type doesn't match.
150-185: Player assembly preference correctly prioritized.The
FindCandidatesmethod:
- Separates Player (runtime) and Editor assemblies via
CompilationPipeline- Searches Player assemblies first
- Falls back to Editor assemblies only if no Player match found
This prevents accidentally resolving Editor-only types when runtime types are available, which is important for prefab/asset modifications that need to work in builds.
187-192: SafeGetTypes handles partial assembly loading gracefully.The exception handling for
ReflectionTypeLoadExceptioncorrectly extracts the successfully loaded types fromrtle.Types. The generic catch prevents crashes from other assembly inspection failures.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (1)
50-166: LGTM! Clean API rename.The systematic renaming from
GetAIPropertySuggestionstoGetFuzzyPropertySuggestionsis consistent across all test methods and call sites. The test logic remains intact, ensuring continuity in test coverage.MCPForUnity/Editor/Tools/ManageGameObject.cs (9)
25-26: LGTM: Clean delegation to centralized serializerThe backward-compatible alias properly delegates to
UnityJsonSerializer.Instance, maintaining existing code while centralizing serialization logic.
79-95: LGTM: Clear prefab asset handling with guidanceThe early check correctly prevents modification of prefab assets while allowing instantiation via 'create'. The error message provides clear guidance to users on the appropriate tools.
587-618: LGTM: Robust tag handling with auto-creationThe tag modification logic correctly handles:
- Empty string as "Untagged" (line 593)
- Tag existence validation before assignment
- Automatic tag creation for missing tags
- Exception handling with clear error messages
Unlike the create action, cleanup is appropriately not attempted here since we're modifying an existing object.
1196-1211: LGTM: Unity 2021 compatibility preservedThe conditional approach correctly handles both scoped (children) and global searches while maintaining compatibility with Unity 2021's API. The manual filtering for
activeInHierarchycompensates forFindObjectsOfTypenot supporting anincludeInactiveparameter.
1499-1500: LGTM: User-friendly property name normalizationThe fallback pattern (original → normalized) provides excellent UX by accepting natural language property names like "Use Gravity" while maintaining backward compatibility. The normalization is consistently applied across all member types.
Also applies to: 1515-1517, 1534-1536, 1553-1555
1468-1468: LGTM: Accurate rename from AI to fuzzy matchingThe rename from
GetAIPropertySuggestionstoGetFuzzyPropertySuggestionscorrectly reflects the rule-based implementation using Levenshtein distance and heuristics. Caching is preserved, improving transparency.Also applies to: 1969-1969
1896-1903: LGTM: Clean delegation to ObjectResolverThe delegation maintains backward compatibility for existing callers while centralizing object resolution logic. Documentation clearly explains the architectural improvement.
1927-1940: LGTM: Consistent delegation to UnityTypeResolverThe
ComponentResolvernow properly delegates toUnityTypeResolver.TryResolvewith theComponentconstraint, centralizing type resolution while maintaining the existing API surface. This aligns with the PR's goal of consolidating shared services.Based on learnings, ComponentResolver is a nested static class within ManageGameObject and the delegation pattern correctly leverages the new centralized
UnityTypeResolver.
1582-1745: LGTM: Robust nested property handlingThe method correctly handles:
- Complex property paths with dot notation and array indexing
- Null checks for intermediate objects
- Array bounds validation
- Special shader property handling via
MaterialOps.TrySetShaderProperty- Clear error messages for failures
The structure is defensive and well-designed for handling Unity's diverse property types.
| /// <remarks> | ||
| /// Delegates to PropertyConversion.ConvertToType for unified type handling. | ||
| /// The inputSerializer parameter is kept for backwards compatibility but is ignored | ||
| /// as PropertyConversion uses UnityJsonSerializer.Instance internally. | ||
| /// </remarks> | ||
| private static object ConvertJTokenToType(JToken token, Type targetType, JsonSerializer inputSerializer) | ||
| { | ||
| if (token == null || token.Type == JTokenType.Null) | ||
| { | ||
| if (targetType.IsValueType && Nullable.GetUnderlyingType(targetType) == null) | ||
| { | ||
| Debug.LogWarning($"Cannot assign null to non-nullable value type {targetType.Name}. Returning default value."); | ||
| return Activator.CreateInstance(targetType); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // Use the provided serializer instance which includes our custom converters | ||
| return token.ToObject(targetType, inputSerializer); | ||
| } | ||
| catch (JsonSerializationException jsonEx) | ||
| { | ||
| Debug.LogError($"JSON Deserialization Error converting token to {targetType.FullName}: {jsonEx.Message}\nToken: {token.ToString(Formatting.None)}"); | ||
| // Optionally re-throw or return null/default | ||
| // return targetType.IsValueType ? Activator.CreateInstance(targetType) : null; | ||
| throw; // Re-throw to indicate failure higher up | ||
| } | ||
| catch (ArgumentException argEx) | ||
| { | ||
| Debug.LogError($"Argument Error converting token to {targetType.FullName}: {argEx.Message}\nToken: {token.ToString(Formatting.None)}"); | ||
| throw; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Debug.LogError($"Unexpected error converting token to {targetType.FullName}: {ex}\nToken: {token.ToString(Formatting.None)}"); | ||
| throw; | ||
| } | ||
| // If ToObject succeeded, it would have returned. If it threw, we wouldn't reach here. | ||
| // This fallback logic is likely unreachable if ToObject covers all cases or throws on failure. | ||
| // Debug.LogWarning($"Conversion failed for token to {targetType.FullName}. Token: {token.ToString(Formatting.None)}"); | ||
| // return targetType.IsValueType ? Activator.CreateInstance(targetType) : null; | ||
| return PropertyConversion.ConvertToType(token, targetType); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of ConvertJTokenToType
rg -nP '\bConvertJTokenToType\s*\(' --type=cs -C3Repository: CoplayDev/unity-mcp
Length of output: 4972
🏁 Script executed:
#!/bin/bash
# Check the PropertyConversion.ConvertToType implementation
rg -nP 'class PropertyConversion|def ConvertToType|ConvertToType\s*\(' --type=cs -A10 | head -100Repository: CoplayDev/unity-mcp
Length of output: 9027
🏁 Script executed:
#!/bin/bash
# Look for the actual PropertyConversion.ConvertToType method definition
fd PropertyConversion --type cs -x cat -n {}Repository: CoplayDev/unity-mcp
Length of output: 232
Misleading callers expect custom serializer usage
The inputSerializer parameter is indeed kept for backward compatibility but ignored since PropertyConversion.ConvertToType uses only the token and target type. However, all five call sites in this file (lines 1521, 1540, 1558, 1701, 1718) explicitly pass the serializer with comments stating "Use the inputSerializer for conversion"—a direct mismatch with the actual behavior.
Either update those comments to reflect that the serializer is ignored, or refactor PropertyConversion.ConvertToType to accept and use the serializer parameter for the intended behavior.
🤖 Prompt for AI Agents
In @MCPForUnity/Editor/Tools/ManageGameObject.cs around lines 1787 - 1795, The
wrapper ConvertJTokenToType currently ignores its inputSerializer parameter
while callers pass a serializer expecting it to be used; either (A) update the
five callers' inline comments to state that the serializer is ignored and remove
the unused parameter from ConvertJTokenToType, or (B) propagate the serializer
into PropertyConversion by adding an overload/parameter to
PropertyConversion.ConvertToType that accepts JsonSerializer, update
ConvertJTokenToType to call that new overload (passing the inputSerializer), and
update PropertyConversion implementation to honor the serializer during
conversion; ensure you update all callers that currently pass a serializer to
remain consistent.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs
Show resolved
Hide resolved
Bug fixes: - Fix searchInactive flag ignored in FindObjectsOfType (use includeInactive overload) - Fix property lookup to try both original and normalized names for backwards compat - Remove dead code for deprecated 'find' action validation - Update error message to list only valid actions Improvements: - Add destructiveHint=True to manage_shader tool - Limit fallback connection attempts (every 3rd attempt) to avoid spamming errors - Consolidate PropertyConversion exception handlers to single catch block - Add tag existence assertion and cleanup in tag auto-creation tests Test fixes: - Update SetComponentProperties_ContinuesAfterException log regex for new error format - Update test_manage_gameobject_param_coercion to test valid actions only
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
396-420: Test correctly validates auto-tag creation with inline cleanup.The test properly verifies:
- Command succeeds
- Tag is assigned to the object
- Tag exists in Unity's TagManager
The inline cleanup addresses test isolation. However, the empty
catch { }silently swallows cleanup failures, which could mask issues in CI.Consider logging cleanup failures for debugging
// Clean up the created tag - try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } catch { } + try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } + catch (System.Exception ex) { Debug.LogWarning($"Tag cleanup failed: {ex.Message}"); }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
405-432: Test correctly validates tag auto-creation on create action.The test mirrors the pattern in
ManageGameObjectModifyTests, verifying:
- Create command succeeds
- Object is created with the auto-created tag
- Tag exists in Unity's TagManager
Using a distinct tag name (
AutoCreatedTag12345) from the modify test prevents collision.Consider logging cleanup failures (same as modify test)
// Clean up the created tag - try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } catch { } + try { UnityEditorInternal.InternalEditorUtility.RemoveTag(testTag); } + catch (System.Exception ex) { Debug.LogWarning($"Tag cleanup failed: {ex.Message}"); }MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
558-558: Consider extracting magic numbers to named constants.The thresholds and intervals (6 for delay switch, 20 for fallback start, 3 for fallback interval) would be clearer as named constants:
const int quickRetryAttempts = 6; const int fallbackThreshold = 20; const int fallbackInterval = 3;This would make the retry strategy more explicit and easier to tune.
🔎 Proposed refactor
+ private const int QuickRetryAttempts = 6; + private const int FallbackDetectionThreshold = 20; + private const int FallbackAttemptInterval = 3; + private async Task TryAutoStartSessionAsync() { // Wait briefly for the HTTP server to become ready, then start the session. // This is called when THIS instance starts the server (not when detecting an external server). var bridgeService = MCPServiceLocator.Bridge; // Windows/dev mode may take much longer due to uv package resolution, fresh downloads, antivirus scans, etc. const int maxAttempts = 30; // Use shorter delays initially, then longer delays to allow server startup var shortDelay = TimeSpan.FromMilliseconds(500); var longDelay = TimeSpan.FromSeconds(3); for (int attempt = 0; attempt < maxAttempts; attempt++) { - var delay = attempt < 6 ? shortDelay : longDelay; + var delay = attempt < QuickRetryAttempts ? shortDelay : longDelay; // Check if server is actually accepting connections bool serverDetected = MCPServiceLocator.Server.IsLocalHttpServerRunning(); if (serverDetected) { // Server detected - try to connect bool started = await bridgeService.StartAsync(); if (started) { await VerifyBridgeConnectionAsync(); UpdateConnectionStatus(); return; } } - else if (attempt >= 20) + else if (attempt >= FallbackDetectionThreshold) { // After many attempts without detection, try connecting anyway as a last resort. // This handles cases where process detection fails but the server is actually running. - // Only try once every 3 attempts to avoid spamming connection errors (at attempts 20, 23, 26, 29). - if ((attempt - 20) % 3 != 0) continue; + // Only try once every N attempts to avoid spamming connection errors. + if ((attempt - FallbackDetectionThreshold) % FallbackAttemptInterval != 0) continue; bool started = await bridgeService.StartAsync(); if (started) { await VerifyBridgeConnectionAsync(); UpdateConnectionStatus(); return; } } if (attempt < maxAttempts - 1) { await Task.Delay(delay); } } McpLog.Warn("Failed to auto-start session after launching the HTTP server."); }Also applies to: 574-579
MCPForUnity/Editor/Helpers/PropertyConversion.cs (1)
49-59: Log caught exceptions for easier debugging.The
catchblock silently swallows exceptions, making it difficult to diagnose conversion failures. Consider logging the exception atDebug.LogWarninglevel to aid troubleshooting while still returning null.🔎 Suggested improvement
public static object TryConvertToType(JToken token, Type targetType) { try { return ConvertToType(token, targetType); } - catch + catch (Exception ex) { + Debug.LogWarning($"[PropertyConversion] Failed to convert token to {targetType?.Name}: {ex.Message}"); return null; } }MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
1516-1517: Update misleading comments at call sites.Lines 1516, 1535, and 1553 have comments stating "Use the inputSerializer for conversion", but the
ConvertJTokenToTypewrapper now ignores the serializer parameter (as documented in lines 1783-1787). Update these comments to reflect that the sharedUnityJsonSerializer.Instanceis used instead.🔎 Suggested comment updates
-// Use the inputSerializer for conversion +// Delegates to PropertyConversion.ConvertToType (uses UnityJsonSerializer.Instance) object convertedValue = ConvertJTokenToType(value, propInfo.PropertyType, inputSerializer);Apply similar changes at lines 1535 and 1553.
Also applies to: 1535-1536, 1553-1554
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_shader.pyServer/tests/integration/test_manage_gameobject_param_coercion.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
🧰 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.
📚 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/ManageGameObjectTests.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/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Tools/ManageGameObject.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/ManageGameObjectCreateTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/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/manage_shader.py
🧬 Code graph analysis (5)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
HandleCommand(30-122)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
RemoveTag(193-219)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
MCPForUnity/Editor/Tools/ManageEditor.cs (1)
RemoveTag(193-219)
MCPForUnity/Editor/Helpers/PropertyConversion.cs (2)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-212)SanitizeAssetPath(31-45)
Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(92-289)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
MCPForUnity/Editor/Tools/ManageGameObject.cs (3)
MCPForUnity/Editor/Helpers/UnityJsonSerializer.cs (1)
UnityJsonSerializer(11-31)MCPForUnity/Editor/Helpers/PropertyConversion.cs (3)
UnityEngine(75-89)PropertyConversion(13-90)ConvertToType(21-43)MCPForUnity/Editor/Helpers/ParamCoercion.cs (2)
ParamCoercion(11-200)NormalizePropertyName(170-199)
⏰ 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 (34)
Server/src/services/tools/manage_shader.py (1)
17-17: LGTM! Clarifying comment improves documentation.The added comment effectively documents that the tool has mixed destructive/non-destructive actions, which is helpful context for maintainers. This addresses the previous review feedback while acknowledging the tool-level nature of
destructiveHint.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectModifyTests.cs (1)
4-4: LGTM!The
UnityEditorInternalimport is correctly added to supportInternalEditorUtility.tagsandRemoveTag()used in the tag auto-creation test.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (8)
125-126: LGTM!Method renamed from
GetAIPropertySuggestionstoGetFuzzyPropertySuggestions, aligning with the broader refactor to use fuzzy matching terminology.
156-157: Consistent rename applied.
166-173: Consistent rename applied across error handling tests.
184-190: Consistent rename applied in caching performance test.
318-321: Log expectations updated to match consolidated error handling.The test now correctly expects:
- PropertyConversion error when converting invalid type to Vector3
- SetProperty error when the property set fails
- Warning for property not found
This aligns with the consolidated
PropertyConversionexception handling mentioned in the PR objectives.
562-583: Well-structured test for prefab modify action guidance.The test correctly verifies that modifying a prefab asset path returns an
ErrorResponsewith guidance to usemanage_assetormanage_prefabsinstead. The assertions align with the error message format inManageGameObject.HandleCommand(from relevant code snippet lines 79-87).
585-604: Test validates delete action also returns prefab guidance.
606-632: Good negative test for create action.This test ensures the create (instantiate) action is not blocked by the prefab check, verifying the
action != "create"condition in the prefab asset check. The assertion correctly verifies the error (if any) is NOT the prefab redirection error.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectCreateTests.cs (1)
4-4: LGTM!Import added for
InternalEditorUtilityaccess in the tag auto-creation test.Server/tests/integration/test_manage_gameobject_param_coercion.py (2)
8-34: LGTM! Boolean coercion test is well-structured.The test correctly validates that string boolean values (e.g.,
"true") are properly coerced to Python booleans when passed to themanage_gameobjectfunction. The assertions confirm that thesetActiveparameter is converted from the string"true"to the booleanTruebefore being forwarded to the Unity command layer.
38-65: LGTM! Create action parameter forwarding is correctly tested.The test validates that the
createaction properly forwards thename,tag, andpositionparameters to the Unity command layer. This ensures the Python-side parameter normalization is working correctly.Note: This test verifies parameter forwarding but does not test the Unity-side tag auto-creation logic mentioned in the PR summary (checking
InternalEditorUtility.tagsbefore assignment). That behavior would need to be validated by Unity-side tests or end-to-end integration tests.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
545-597: Improved connection retry logic with selective attempts.The two-phase delay strategy and conditional connection attempts are solid improvements:
- Shorter delays (500ms) for the first 6 attempts allow quick connection to fast-starting servers
- Longer delays (3s) for subsequent attempts accommodate Windows/dev mode scenarios with package resolution
- Early returns on successful connection avoid unnecessary waiting
- Server detection check before attempting
StartAsyncreduces failed connection attempts- Defensive fallback after 20 failed detection attempts handles edge cases where process detection fails but the server is actually running
MCPForUnity/Editor/Helpers/PropertyConversion.cs (3)
21-43: LGTM: Robust null handling and error reporting.The method properly handles nullable value types and provides clear error messages with token context before re-throwing exceptions.
64-67: LGTM: Clean generic wrapper.
75-89: LGTM: Proper validation and clear error messaging.The method correctly validates token type, sanitizes paths, and provides helpful warnings when assets aren't found.
MCPForUnity/Editor/Helpers/ComponentOps.cs (6)
23-73: LGTM: Comprehensive validation and error handling.The method properly validates inputs, prevents invalid operations (duplicate Transform), checks for physics conflicts, and integrates with Unity's Undo system.
82-122: LGTM: Consistent validation pattern.The method mirrors the validation approach of
AddComponentand properly prevents removing essential components like Transform.
132-227: LGTM: Thorough property lookup with backward compatibility.The method properly tries both original and normalized property names, handles various member types (properties, fields, SerializeField), and has good error handling. The conversion failure detection (lines 161-165, 185-189, 210-214) catches cases where conversion returns null unexpectedly.
232-266: LGTM: Comprehensive member enumeration.The method correctly includes all Unity-serializable members, including private
[SerializeField]fields, and returns a sorted list for predictable output.
270-296: LGTM: Proper physics conflict detection.The method correctly prevents mixing 2D and 3D physics components on the same GameObject, with clear error messages explaining the conflict.
298-305: LGTM: Sensible default values.Setting newly added Lights to Directional is a reasonable default, and the pattern is easy to extend for other component types.
Server/src/services/tools/manage_gameobject.py (3)
85-94: LGTM: Clear scope definition with helpful guidance.The updated description and narrowed action literal clearly define the tool's scope and guide users to appropriate tools for finding GameObjects and managing components.
175-179: LGTM: Consistent error messaging.The error message matches the updated action set and provides helpful guidance to users.
257-271: LGTM: Robust prefab path handling.The logic correctly constructs and validates prefab paths, with clear error messages when required parameters are missing or invalid. Path separator normalization ensures Unity compatibility.
MCPForUnity/Editor/Tools/ManageGameObject.cs (8)
79-95: LGTM: Clear prefab handling with helpful guidance.The prefab asset check properly prevents direct modification of prefab assets while allowing instantiation, and provides clear guidance to users on which tools to use for prefab modifications.
334-361: LGTM: Proper tag validation and auto-creation.The tag existence check using
InternalEditorUtility.tagsbefore assignment is correct, and the auto-creation logic with proper error handling and cleanup addresses the concerns from previous reviews. The redundant ternary assignment has been removed.
584-616: LGTM: Consistent tag handling pattern.The tag validation and auto-creation logic mirrors the CreateGameObject implementation, with appropriate error handling. The ternary at line 591 is correct here since empty strings should be converted to "Untagged".
1194-1207: LGTM: Properly respects searchInactive flag.The fix correctly uses
FindObjectsOfType(Type, bool includeInactive)overload to honor thesearchInactiveparameter, addressing the issue from previous reviews where inactive objects were never returned.
1495-1496: LGTM: Consistent property name normalization.The backward-compatible approach of trying both original and normalized names ensures existing code continues to work while supporting user-friendly property names like "Use Gravity" → "useGravity".
Also applies to: 1511-1513, 1530-1532, 1549-1551
1892-1899: LGTM: Clean delegation to centralized resolver.The method properly delegates to
ObjectResolver.Resolve()while maintaining backward compatibility, with clear documentation of the architectural change.
1927-1936: LGTM: Clean delegation with backward compatibility.The ComponentResolver now properly delegates to the centralized
UnityTypeResolverwhile maintaining its public API for backward compatibility.
1965-1986: LGTM: Improved naming and caching.The rename from
GetAIPropertySuggestionstoGetFuzzyPropertySuggestionsmore accurately describes the rule-based approach. The simple caching mechanism will help performance for repeated lookups.
1 participant
Summary
This PR extracts common functionality from
ManageGameObject,ManageAsset,ManageComponents, and other tools into reusable helper classes. The result is cleaner architecture, reduced code duplication, and a more maintainable codebase.Also includes bug fixes for tag assignment and API cleanup for the new Tool/Resource separation.
Net change: 37 files, +1,700 / -900 lines (~860 lines of duplicate code eliminated)
New Helper Classes
UnityJsonSerializerJsonSerializerwith Unity type converters (Vector3, Color, etc.)ObjectResolverUnityTypeResolverPropertyConversionComponentOpsPaginationPaginationRequest/PaginationResponse<T>classesKey Changes
1. Removed Prefab Redirect Anti-Pattern
ManageGameObjectpreviously had 68 lines of code that redirected prefab operations toManageAsset. Now it returns helpful guidance instead:2. Standardized Response Patterns
ManageMaterialnow usesSuccessResponse/ErrorResponseconsistently (42 response sites updated).3. Unified Type Resolution
Three duplicate implementations consolidated:
ComponentResolver(ManageGameObject)FindComponentType(GameObjectLookup)ResolveType(ManageScriptableObject)Now all delegate to
UnityTypeResolverwith caching.4. Physics Conflict Detection in ComponentOps
2D/3D physics conflict checking moved to shared
ComponentOps.AddComponent():5. Standardized Pagination
FindGameObjectsnow usesPaginationRequest.FromParams()andPaginationResponse<T>.Create().6. Tag Auto-Creation Fix
ManageGameObjectnow properly auto-creates tags that don't exist. Previously, the code assumed Unity throws exceptions for undefined tags, but Unity only logs a warning and silently ignores the assignment. Now it checks tag existence viaInternalEditorUtility.tagsbefore assignment and creates missing tags automatically.7. Cleaned Up manage_gameobject API
Removed deprecated actions from Python tool definition that were already removed from C#:
find→ Usefind_gameobjectstool insteadget_components,get_component→ Useunity://scene/gameobject/{id}/componentsresourceadd_component,remove_component,set_component_property→ Usemanage_componentstoolTesting
Migration Guide
No breaking changes for scene operations. All existing tool calls work identically.
API Cleanup: If using deprecated
manage_gameobjectactions, migrate to new tools:action="find"find_gameobjectstoolaction="get_components"unity://scene/gameobject/{id}/componentsresourceaction="add_component"manage_components(action="add")action="remove_component"manage_components(action="remove")action="set_component_property"manage_components(action="set_property")For contributors working on new tools:
Files Changed
New files (6)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/ObjectResolver.csMCPForUnity/Editor/Helpers/Pagination.csMCPForUnity/Editor/Helpers/PropertyConversion.csMCPForUnity/Editor/Helpers/UnityJsonSerializer.csMCPForUnity/Editor/Helpers/UnityTypeResolver.csModified tools (7)
ManageGameObject.cs- Delegates to helpers, prefab redirect removed, tag auto-creation fixManageAsset.cs- Uses PropertyConversionManageComponents.cs- Uses ComponentOps, UnityTypeResolverManageMaterial.cs- Uses SuccessResponse/ErrorResponse, UnityJsonSerializerManageScriptableObject.cs- Uses UnityTypeResolverFindGameObjects.cs- Uses Pagination helpersServer/src/services/tools/manage_gameobject.py- Removed deprecated actions from APIModified helpers (2)
GameObjectLookup.cs- FindComponentType delegates to UnityTypeResolverAssetPathUtility.cs- Added NormalizeSeparators()Modified tests (2)
ManageGameObjectCreateTests.cs- Updated for tag auto-creation behaviorManageGameObjectModifyTests.cs- Updated for tag auto-creation behaviorChecklist
Summary by Sourcery
Extract shared Unity serialization, type resolution, component operations, object resolution, and pagination into reusable helpers, while standardizing responses and tightening behavior across editor tools and server-side MCP tools.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.