From 17ad011b42595616abab35b3884f3e3f9a6e0f69 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Wed, 3 Sep 2025 08:27:34 -0700 Subject: [PATCH] fix: Address CodeRabbit review issues and improve robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical Bug Fixes: - Fix operator precedence bug in ManageAsset.cs that could cause null reference exceptions - Fix GameObject memory leak in primitive creation when name validation fails - Add proper cleanup with DestroyImmediate when primitive creation fails ComponentResolver Integration: - Replace fragile string-based GetComponent() calls with robust ComponentResolver - Add ComponentResolver integration in ManageAsset.cs for component lookups - Add fallback to string-based lookup in ManageGameObject.cs for compatibility Enhanced Error Handling: - Surface specific ComponentResolver error context in ScriptableObject creation failures - Add support for setting private [SerializeField] fields in property matching - Improve debugging with detailed error messages Assembly Definition Fixes: - Configure TestAsmdef as Editor-only to prevent build bloat - Add explicit TestAsmdef reference to test assembly for proper component resolution - Fix ComponentResolverTests to use accessible CustomComponent instead of TicTacToe3D Code Quality: - Disable nullable reference types for legacy codebase to eliminate 100+ warnings - Maintain backward compatibility while improving reliability All 45 unit tests pass, ensuring no regressions while significantly improving robustness. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../Scripts/TestAsmdef/TestAsmdef.asmdef | 4 +- .../EditMode/MCPForUnityTests.Editor.asmdef | 1 + .../EditMode/Tools/ComponentResolverTests.cs | 6 +-- UnityMcpBridge/Editor/Tools/ManageAsset.cs | 28 +++++++++----- .../Editor/Tools/ManageGameObject.cs | 38 +++++++++++++++++-- 5 files changed, 59 insertions(+), 18 deletions(-) diff --git a/TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef b/TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef index 04cfdce..a5833da 100644 --- a/TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef +++ b/TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/TestAsmdef.asmdef @@ -2,12 +2,12 @@ "name": "TestAsmdef", "rootNamespace": "TestNamespace", "references": [], - "includePlatforms": [], + "includePlatforms": ["Editor"], "excludePlatforms": [], "allowUnsafeCode": false, "overrideReferences": false, "precompiledReferences": [], - "autoReferenced": true, + "autoReferenced": false, "defineConstraints": [], "versionDefines": [], "noEngineReferences": false diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPForUnityTests.Editor.asmdef b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPForUnityTests.Editor.asmdef index 864596e..1207b26 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPForUnityTests.Editor.asmdef +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPForUnityTests.Editor.asmdef @@ -3,6 +3,7 @@ "rootNamespace": "", "references": [ "MCPForUnity.Editor", + "TestAsmdef", "UnityEngine.TestRunner", "UnityEditor.TestRunner" ], diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs index 0465ac6..d19afbb 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs @@ -31,11 +31,11 @@ namespace MCPForUnityTests.Editor.Tools [Test] public void TryResolve_ReturnsTrue_ForCustomComponentShortName() { - bool result = ComponentResolver.TryResolve("TicTacToe3D", out Type type, out string error); + bool result = ComponentResolver.TryResolve("CustomComponent", out Type type, out string error); - Assert.IsTrue(result, "Should resolve TicTacToe3D component"); + Assert.IsTrue(result, "Should resolve CustomComponent"); Assert.IsNotNull(type, "Should return valid type"); - Assert.AreEqual("TicTacToe3D", type.Name, "Should have correct type name"); + Assert.AreEqual("CustomComponent", type.Name, "Should have correct type name"); Assert.IsTrue(typeof(Component).IsAssignableFrom(type), "Should be a Component type"); Assert.IsEmpty(error, "Should have no error message"); } diff --git a/UnityMcpBridge/Editor/Tools/ManageAsset.cs b/UnityMcpBridge/Editor/Tools/ManageAsset.cs index a05931f..c223f8c 100644 --- a/UnityMcpBridge/Editor/Tools/ManageAsset.cs +++ b/UnityMcpBridge/Editor/Tools/ManageAsset.cs @@ -7,6 +7,7 @@ using Newtonsoft.Json.Linq; using UnityEditor; using UnityEngine; using MCPForUnity.Editor.Helpers; // For Response class +using static MCPForUnity.Editor.Tools.ManageGameObject; // For ComponentResolver #if UNITY_6000_0_OR_NEWER using PhysicsMaterialType = UnityEngine.PhysicsMaterial; @@ -207,9 +208,10 @@ namespace MCPForUnity.Editor.Tools || !typeof(ScriptableObject).IsAssignableFrom(scriptType) ) { - return Response.Error( - $"Script class '{scriptClassName}' not found or does not inherit from ScriptableObject." - ); + var reason = scriptType == null + ? (string.IsNullOrEmpty(error) ? "Type not found." : error) + : "Type found but does not inherit from ScriptableObject."; + return Response.Error($"Script class '{scriptClassName}' invalid: {reason}"); } ScriptableObject so = ScriptableObject.CreateInstance(scriptType); @@ -353,10 +355,18 @@ namespace MCPForUnity.Editor.Tools && componentProperties.HasValues ) // e.g., {"bobSpeed": 2.0} { - // Find the component on the GameObject using the name from the JSON key - // Using GetComponent(string) is convenient but might require exact type name or be ambiguous. - // Consider using ComponentResolver if needed for more complex scenarios. - Component targetComponent = gameObject.GetComponent(componentName); + // Resolve component type via ComponentResolver, then fetch by Type + Component targetComponent = null; + if (ComponentResolver.TryResolve(componentName, out var compType, out var compError)) + { + targetComponent = gameObject.GetComponent(compType); + } + else + { + Debug.LogWarning( + $"[ManageAsset.ModifyAsset] Failed to resolve component '{componentName}' on '{gameObject.name}': {compError}" + ); + } if (targetComponent != null) { @@ -937,8 +947,8 @@ namespace MCPForUnity.Editor.Tools { string propName = floatProps["name"]?.ToString(); if ( - !string.IsNullOrEmpty(propName) && floatProps["value"]?.Type == JTokenType.Float - || floatProps["value"]?.Type == JTokenType.Integer + !string.IsNullOrEmpty(propName) && + (floatProps["value"]?.Type == JTokenType.Float || floatProps["value"]?.Type == JTokenType.Integer) ) { try diff --git a/UnityMcpBridge/Editor/Tools/ManageGameObject.cs b/UnityMcpBridge/Editor/Tools/ManageGameObject.cs index 5a86701..f3cbe6f 100644 --- a/UnityMcpBridge/Editor/Tools/ManageGameObject.cs +++ b/UnityMcpBridge/Editor/Tools/ManageGameObject.cs @@ -1,4 +1,4 @@ -#nullable enable +#nullable disable using System; using System.Collections.Generic; using System.Linq; @@ -289,11 +289,16 @@ namespace MCPForUnity.Editor.Tools newGo = GameObject.CreatePrimitive(type); // Set name *after* creation for primitives if (!string.IsNullOrEmpty(name)) + { newGo.name = name; + } else + { + UnityEngine.Object.DestroyImmediate(newGo); // cleanup leak return Response.Error( "'name' parameter is required when creating a primitive." - ); // Name is essential + ); + } createdNewObject = true; } catch (ArgumentException) @@ -1493,7 +1498,18 @@ namespace MCPForUnity.Editor.Tools Component targetComponentInstance = null ) { - Component targetComponent = targetComponentInstance ?? targetGo.GetComponent(compName); + Component targetComponent = targetComponentInstance; + if (targetComponent == null) + { + if (ComponentResolver.TryResolve(compName, out var compType, out var compError)) + { + targetComponent = targetGo.GetComponent(compType); + } + else + { + targetComponent = targetGo.GetComponent(compName); // fallback to string-based lookup + } + } if (targetComponent == null) { return Response.Error( @@ -1627,6 +1643,20 @@ namespace MCPForUnity.Editor.Tools Debug.LogWarning($"[SetProperty] Conversion failed for field '{memberName}' (Type: {fieldInfo.FieldType.Name}) from token: {value.ToString(Formatting.None)}"); } } + else + { + // Try NonPublic [SerializeField] fields + var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase); + if (npField != null && npField.GetCustomAttribute() != null) + { + object convertedValue = ConvertJTokenToType(value, npField.FieldType, inputSerializer); + if (convertedValue != null || value.Type == JTokenType.Null) + { + npField.SetValue(target, convertedValue); + return true; + } + } + } } } catch (Exception ex) @@ -2199,7 +2229,7 @@ namespace MCPForUnity.Editor.Tools t.Name.Equals(q, StringComparison.Ordinal) || (t.FullName?.Equals(q, StringComparison.Ordinal) ?? false); - private static bool IsValidComponent(Type? t) => + private static bool IsValidComponent(Type t) => t != null && typeof(Component).IsAssignableFrom(t); private static void Cache(Type t)