From 7a0d507b58ada9208e604de4ce61774f383adaa3 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 16 Feb 2026 18:48:22 -0800 Subject: [PATCH 1/4] Fix misleading 'Property not found' error for conversion failures and add SerializedProperty fallback (#765) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When setting component properties via reflection failed (e.g. array types, UdonSharp proxies), the error message falsely reported "Property 'X' not found. Did you mean: X?" — even though the property existed. The root cause was that SetProperty returned a bare bool with no error detail, so the caller interpreted every failure as "not found." Changes: - GameObjectComponentHelpers.SetProperty now returns error details via out parameter, distinguishing conversion failures from missing properties - ComponentOps.SetProperty falls back to SetViaSerializedProperty when reflection-based conversion fails, handling arrays and custom serialization - GameObjectComponentHelpers falls back to ComponentOps.SetProperty when its local reflection fails, gaining the same SerializedProperty support Co-Authored-By: Claude Opus 4.6 --- MCPForUnity/Editor/Helpers/ComponentOps.cs | 38 ++++-- .../GameObjects/GameObjectComponentHelpers.cs | 76 +++++++----- .../GameObjectComponentHelpersErrorTests.cs | 110 ++++++++++++++++++ ...meObjectComponentHelpersErrorTests.cs.meta | 11 ++ 4 files changed, 196 insertions(+), 39 deletions(-) create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs.meta diff --git a/MCPForUnity/Editor/Helpers/ComponentOps.cs b/MCPForUnity/Editor/Helpers/ComponentOps.cs index 33a911a1d..755525d39 100644 --- a/MCPForUnity/Editor/Helpers/ComponentOps.cs +++ b/MCPForUnity/Editor/Helpers/ComponentOps.cs @@ -174,15 +174,37 @@ public static bool SetProperty(Component component, string propertyName, JToken return SetViaSerializedProperty(component, propertyName, normalizedName, value, out error); } - // Try property first - check both original and normalized names for backwards compatibility - PropertyInfo propInfo = type.GetProperty(propertyName, flags) + // Try reflection first (property, field, then non-public serialized field) + if (TrySetViaReflection(component, type, propertyName, normalizedName, flags, value, out error)) + return true; + + // Reflection failed — fall back to SerializedProperty which handles arrays, + // custom serialization (e.g. UdonSharp), and types reflection can't convert. + string reflectionError = error; + if (SetViaSerializedProperty(component, propertyName, normalizedName, value, out error)) + return true; + + // Both paths failed. If reflection found the member but couldn't convert, + // report that (more useful than the SerializedProperty error). + // If reflection didn't find it at all, report the SerializedProperty error. + if (reflectionError != null && !reflectionError.Contains("not found")) + error = reflectionError; + + return false; + } + + private static bool TrySetViaReflection(object component, Type type, string propertyName, string normalizedName, BindingFlags flags, JToken value, out string error) + { + error = null; + + // Try property first + PropertyInfo propInfo = type.GetProperty(propertyName, flags) ?? type.GetProperty(normalizedName, flags); if (propInfo != null && propInfo.CanWrite) { try { object convertedValue = PropertyConversion.ConvertToType(value, propInfo.PropertyType); - // Detect conversion failure: null result when input wasn't null if (convertedValue == null && value.Type != JTokenType.Null) { error = $"Failed to convert value for property '{propertyName}' to type '{propInfo.PropertyType.Name}'."; @@ -198,15 +220,14 @@ public static bool SetProperty(Component component, string propertyName, JToken } } - // Try field - check both original and normalized names for backwards compatibility - FieldInfo fieldInfo = type.GetField(propertyName, flags) + // Try field + FieldInfo fieldInfo = type.GetField(propertyName, flags) ?? type.GetField(normalizedName, flags); if (fieldInfo != null && !fieldInfo.IsInitOnly) { try { object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType); - // Detect conversion failure: null result when input wasn't null if (convertedValue == null && value.Type != JTokenType.Null) { error = $"Failed to convert value for field '{propertyName}' to type '{fieldInfo.FieldType.Name}'."; @@ -222,9 +243,7 @@ public static bool SetProperty(Component component, string propertyName, JToken } } - // Try non-public serialized fields - traverse inheritance hierarchy - // Type.GetField() with NonPublic only finds fields declared directly on that type, - // so we need to walk up the inheritance chain manually + // Try non-public serialized fields — traverse inheritance hierarchy fieldInfo = FindSerializedFieldInHierarchy(type, propertyName) ?? FindSerializedFieldInHierarchy(type, normalizedName); if (fieldInfo != null) @@ -232,7 +251,6 @@ public static bool SetProperty(Component component, string propertyName, JToken try { object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType); - // Detect conversion failure: null result when input wasn't null if (convertedValue == null && value.Type != JTokenType.Null) { error = $"Failed to convert value for serialized field '{propertyName}' to type '{fieldInfo.FieldType.Name}'."; diff --git a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs index 8e35508c9..eec9374a8 100644 --- a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs +++ b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs @@ -172,14 +172,26 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin try { - bool setResult = SetProperty(targetComponent, propName, propValue); + bool setResult = SetProperty(targetComponent, propName, propValue, out string setError); if (!setResult) { - var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType()); - var suggestions = ComponentResolver.GetFuzzyPropertySuggestions(propName, availableProperties); - var msg = suggestions.Any() - ? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]" - : $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]"; + // Local reflection failed — fall back to ComponentOps which + // tries SerializedProperty (handles arrays, UdonSharp, etc.) + if (ComponentOps.SetProperty(targetComponent, propName, propValue, out string opsError)) + { + continue; + } + + // Both paths failed. Prefer the more specific error. + string msg = setError ?? opsError; + if (msg == null || msg.Contains("not found")) + { + var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType()); + var suggestions = ComponentResolver.GetFuzzyPropertySuggestions(propName, availableProperties); + msg = suggestions.Any() + ? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]" + : $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]"; + } McpLog.Warn($"[ManageGameObject] {msg}"); failures.Add(msg); } @@ -199,8 +211,9 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin private static JsonSerializer InputSerializer => UnityJsonSerializer.Instance; - private static bool SetProperty(object target, string memberName, JToken value) + private static bool SetProperty(object target, string memberName, JToken value, out string error) { + error = null; Type type = target.GetType(); BindingFlags flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase; @@ -223,39 +236,44 @@ private static bool SetProperty(object target, string memberName, JToken value) propInfo.SetValue(target, convertedValue); return true; } + error = $"Failed to convert value for property '{memberName}' to type '{propInfo.PropertyType.Name}'."; + return false; } - else + + FieldInfo fieldInfo = type.GetField(memberName, flags) ?? type.GetField(normalizedName, flags); + if (fieldInfo != null) { - FieldInfo fieldInfo = type.GetField(memberName, flags) ?? type.GetField(normalizedName, flags); - if (fieldInfo != null) + object convertedValue = ConvertJTokenToType(value, fieldInfo.FieldType, inputSerializer); + if (convertedValue != null || value.Type == JTokenType.Null) { - object convertedValue = ConvertJTokenToType(value, fieldInfo.FieldType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - fieldInfo.SetValue(target, convertedValue); - return true; - } + fieldInfo.SetValue(target, convertedValue); + return true; } - else + error = $"Failed to convert value for field '{memberName}' to type '{fieldInfo.FieldType.Name}'."; + return false; + } + + var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase) + ?? type.GetField(normalizedName, 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) { - var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase) - ?? type.GetField(normalizedName, 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; - } - } + npField.SetValue(target, convertedValue); + return true; } + error = $"Failed to convert value for serialized field '{memberName}' to type '{npField.FieldType.Name}'."; + return false; } } catch (Exception ex) { - McpLog.Error($"[SetProperty] Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}"); + error = $"Failed to set '{memberName}' on {type.Name}: {ex.Message}"; + return false; } + + // Property/field not found — caller will generate "not found" with suggestions return false; } diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs new file mode 100644 index 000000000..f51a08e0f --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs @@ -0,0 +1,110 @@ +using System.Text.RegularExpressions; +using NUnit.Framework; +using UnityEngine; +using UnityEngine.TestTools; +using Newtonsoft.Json.Linq; +using MCPForUnity.Editor.Helpers; +using MCPForUnity.Editor.Tools; +using MCPForUnity.Editor.Tools.GameObjects; + +namespace MCPForUnityTests.Editor.Tools +{ + /// + /// Tests for GameObjectComponentHelpers.SetComponentPropertiesInternal error reporting. + /// Reproduces issue #765: conversion failures incorrectly reported as "Property not found". + /// + public class GameObjectComponentHelpersErrorTests + { + private GameObject testGo; + + [SetUp] + public void SetUp() + { + testGo = new GameObject("ErrorTestGO"); + CommandRegistry.Initialize(); + } + + [TearDown] + public void TearDown() + { + if (testGo != null) + Object.DestroyImmediate(testGo); + } + + /// + /// When a property exists but conversion fails, the error should say + /// "Failed to convert" rather than "Property not found. Did you mean: X?" + /// + [Test] + public void SetComponentProperties_ConversionFailure_ReportsConversionError_NotPropertyNotFound() + { + // Expect conversion error logs from PropertyConversion (once from local SetProperty, + // once from ComponentOps fallback trying reflection before SerializedProperty) + LogAssert.Expect(LogType.Error, new Regex("Error converting token")); + LogAssert.Expect(LogType.Error, new Regex("Error converting token")); + // Expect the warning log from SetComponentPropertiesInternal + LogAssert.Expect(LogType.Warning, new Regex("Failed to set")); + + var audioSource = testGo.AddComponent(); + + // spatialBlend is a float property — passing an array triggers conversion failure + var props = new JObject { ["spatialBlend"] = JArray.Parse("[1, 2, 3]") }; + + var result = GameObjectComponentHelpers.SetComponentPropertiesInternal( + testGo, "AudioSource", props, audioSource); + + Assert.IsNotNull(result, "Should return an error response"); + Assert.IsInstanceOf(result); + + var errorResponse = (ErrorResponse)result; + + // The error message must NOT say "not found" for a property that exists + Assert.IsFalse( + errorResponse.Error.Contains("not found"), + $"Error should report conversion failure, not 'not found'. Got: {errorResponse.Error}"); + } + + /// + /// When a property genuinely doesn't exist, the error should still say "not found" with suggestions. + /// + [Test] + public void SetComponentProperties_NonexistentProperty_ReportsNotFound() + { + // Expect the "not found" warning + LogAssert.Expect(LogType.Warning, new Regex("not found")); + + var audioSource = testGo.AddComponent(); + + var props = new JObject { ["totallyFakeProperty"] = 42 }; + + var result = GameObjectComponentHelpers.SetComponentPropertiesInternal( + testGo, "AudioSource", props, audioSource); + + Assert.IsNotNull(result); + Assert.IsInstanceOf(result); + + var errorResponse = (ErrorResponse)result; + + Assert.IsTrue( + errorResponse.Error.Contains("not found") || errorResponse.Error.Contains("failed"), + $"Error for nonexistent property should say 'not found'. Got: {errorResponse.Error}"); + } + + /// + /// Valid property setting should still succeed. + /// + [Test] + public void SetComponentProperties_ValidProperty_Succeeds() + { + var audioSource = testGo.AddComponent(); + + var props = new JObject { ["volume"] = 0.42f }; + + var result = GameObjectComponentHelpers.SetComponentPropertiesInternal( + testGo, "AudioSource", props, audioSource); + + Assert.IsNull(result, "Should return null on success (no errors)"); + Assert.AreEqual(0.42f, audioSource.volume, 0.001f); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs.meta new file mode 100644 index 000000000..cf28b93b3 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 51225acc846dc412b82750041eb0ee61 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: From 2f5963a80b7b14e81e4bd0ad5dd8f629e3c574e2 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 16 Feb 2026 18:52:58 -0800 Subject: [PATCH 2/4] Include token context in SetProperty error message for debuggability Co-Authored-By: Claude Opus 4.6 --- .../Editor/Tools/GameObjects/GameObjectComponentHelpers.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs index eec9374a8..2c60524fe 100644 --- a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs +++ b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs @@ -269,7 +269,7 @@ private static bool SetProperty(object target, string memberName, JToken value, } catch (Exception ex) { - error = $"Failed to set '{memberName}' on {type.Name}: {ex.Message}"; + error = $"Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}"; return false; } From aace07320601e932195f39577624d2c0d1cc3c31 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 16 Feb 2026 19:06:10 -0800 Subject: [PATCH 3/4] Remove redundant local SetProperty; delegate to ComponentOps directly Address CodeRabbit review feedback: - Remove duplicated reflection logic from GameObjectComponentHelpers - Route non-nested property paths directly through ComponentOps.SetProperty - Add out string error to SetNestedProperty for proper error propagation - Fix MaterialOps.TrySetShaderProperty call to set error on failure Co-Authored-By: Claude Opus 4.6 --- .../GameObjects/GameObjectComponentHelpers.cs | 137 ++++++------------ .../GameObjectComponentHelpersErrorTests.cs | 4 +- 2 files changed, 46 insertions(+), 95 deletions(-) diff --git a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs index 2c60524fe..c618ae4e5 100644 --- a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs +++ b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs @@ -172,18 +172,24 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin try { - bool setResult = SetProperty(targetComponent, propName, propValue, out string setError); - if (!setResult) + bool setResult; + string setError; + + // Nested paths (e.g. "transform.position") need local handling + // since ComponentOps doesn't support dot/bracket notation. + if (propName.Contains('.') || propName.Contains('[')) { - // Local reflection failed — fall back to ComponentOps which - // tries SerializedProperty (handles arrays, UdonSharp, etc.) - if (ComponentOps.SetProperty(targetComponent, propName, propValue, out string opsError)) - { - continue; - } + setResult = SetNestedProperty(targetComponent, propName, propValue, InputSerializer, out setError); + } + else + { + // ComponentOps handles reflection + SerializedProperty fallback + setResult = ComponentOps.SetProperty(targetComponent, propName, propValue, out setError); + } - // Both paths failed. Prefer the more specific error. - string msg = setError ?? opsError; + if (!setResult) + { + string msg = setError; if (msg == null || msg.Contains("not found")) { var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType()); @@ -211,79 +217,17 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin private static JsonSerializer InputSerializer => UnityJsonSerializer.Instance; - private static bool SetProperty(object target, string memberName, JToken value, out string error) + private static bool SetNestedProperty(object target, string path, JToken value, JsonSerializer inputSerializer, out string error) { error = null; - Type type = target.GetType(); - BindingFlags flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase; - - string normalizedName = Helpers.ParamCoercion.NormalizePropertyName(memberName); - var inputSerializer = InputSerializer; - - try - { - if (memberName.Contains('.') || memberName.Contains('[')) - { - return SetNestedProperty(target, memberName, value, inputSerializer); - } - - PropertyInfo propInfo = type.GetProperty(memberName, flags) ?? type.GetProperty(normalizedName, flags); - if (propInfo != null && propInfo.CanWrite) - { - object convertedValue = ConvertJTokenToType(value, propInfo.PropertyType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - propInfo.SetValue(target, convertedValue); - return true; - } - error = $"Failed to convert value for property '{memberName}' to type '{propInfo.PropertyType.Name}'."; - return false; - } - - FieldInfo fieldInfo = type.GetField(memberName, flags) ?? type.GetField(normalizedName, flags); - if (fieldInfo != null) - { - object convertedValue = ConvertJTokenToType(value, fieldInfo.FieldType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - fieldInfo.SetValue(target, convertedValue); - return true; - } - error = $"Failed to convert value for field '{memberName}' to type '{fieldInfo.FieldType.Name}'."; - return false; - } - - var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase) - ?? type.GetField(normalizedName, 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; - } - error = $"Failed to convert value for serialized field '{memberName}' to type '{npField.FieldType.Name}'."; - return false; - } - } - catch (Exception ex) - { - error = $"Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}"; - return false; - } - - // Property/field not found — caller will generate "not found" with suggestions - return false; - } - - private static bool SetNestedProperty(object target, string path, JToken value, JsonSerializer inputSerializer) - { try { string[] pathParts = SplitPropertyPath(path); if (pathParts.Length == 0) + { + error = $"Invalid nested property path '{path}'."; return false; + } object currentObject = target; Type currentType = currentObject.GetType(); @@ -317,7 +261,7 @@ private static bool SetNestedProperty(object target, string path, JToken value, fieldInfo = currentType.GetField(part, flags); if (fieldInfo == null) { - McpLog.Warn($"[SetNestedProperty] Could not find property or field '{part}' on type '{currentType.Name}'"); + error = $"Could not find property or field '{part}' on type '{currentType.Name}' in path '{path}'."; return false; } } @@ -325,7 +269,7 @@ private static bool SetNestedProperty(object target, string path, JToken value, currentObject = propInfo != null ? propInfo.GetValue(currentObject) : fieldInfo.GetValue(currentObject); if (currentObject == null) { - McpLog.Warn($"[SetNestedProperty] Property '{part}' is null, cannot access nested properties."); + error = $"Property '{part}' is null in path '{path}', cannot access nested properties."; return false; } @@ -336,7 +280,7 @@ private static bool SetNestedProperty(object target, string path, JToken value, var materials = currentObject as Material[]; if (arrayIndex < 0 || arrayIndex >= materials.Length) { - McpLog.Warn($"[SetNestedProperty] Material index {arrayIndex} out of range (0-{materials.Length - 1})"); + error = $"Material index {arrayIndex} out of range (0-{materials.Length - 1}) in path '{path}'."; return false; } currentObject = materials[arrayIndex]; @@ -346,14 +290,14 @@ private static bool SetNestedProperty(object target, string path, JToken value, var list = currentObject as System.Collections.IList; if (arrayIndex < 0 || arrayIndex >= list.Count) { - McpLog.Warn($"[SetNestedProperty] Index {arrayIndex} out of range (0-{list.Count - 1})"); + error = $"Index {arrayIndex} out of range (0-{list.Count - 1}) in path '{path}'."; return false; } currentObject = list[arrayIndex]; } else { - McpLog.Warn($"[SetNestedProperty] Property '{part}' is not an array or list, cannot access by index."); + error = $"Property '{part}' is not an array or list in path '{path}', cannot access by index."; return false; } } @@ -365,7 +309,12 @@ private static bool SetNestedProperty(object target, string path, JToken value, if (currentObject is Material material && finalPart.StartsWith("_")) { - return MaterialOps.TrySetShaderProperty(material, finalPart, value, inputSerializer); + if (!MaterialOps.TrySetShaderProperty(material, finalPart, value, inputSerializer)) + { + error = $"Failed to set shader property '{finalPart}' on material '{material.name}' in path '{path}'."; + return false; + } + return true; } PropertyInfo finalPropInfo = currentType.GetProperty(finalPart, flags); @@ -377,24 +326,28 @@ private static bool SetNestedProperty(object target, string path, JToken value, finalPropInfo.SetValue(currentObject, convertedValue); return true; } + error = $"Failed to convert value for '{finalPart}' to type '{finalPropInfo.PropertyType.Name}' in path '{path}'."; + return false; } - else + + FieldInfo finalFieldInfo = currentType.GetField(finalPart, flags); + if (finalFieldInfo != null) { - FieldInfo finalFieldInfo = currentType.GetField(finalPart, flags); - if (finalFieldInfo != null) + object convertedValue = ConvertJTokenToType(value, finalFieldInfo.FieldType, inputSerializer); + if (convertedValue != null || value.Type == JTokenType.Null) { - object convertedValue = ConvertJTokenToType(value, finalFieldInfo.FieldType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - finalFieldInfo.SetValue(currentObject, convertedValue); - return true; - } + finalFieldInfo.SetValue(currentObject, convertedValue); + return true; } + error = $"Failed to convert value for '{finalPart}' to type '{finalFieldInfo.FieldType.Name}' in path '{path}'."; + return false; } + + error = $"Property or field '{finalPart}' not found on type '{currentType.Name}' in path '{path}'."; } catch (Exception ex) { - McpLog.Error($"[SetNestedProperty] Error setting nested property '{path}': {ex.Message}\nToken: {value.ToString(Formatting.None)}"); + error = $"Error setting nested property '{path}': {ex.Message}"; } return false; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs index f51a08e0f..f8b09f5e9 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs @@ -38,9 +38,7 @@ public void TearDown() [Test] public void SetComponentProperties_ConversionFailure_ReportsConversionError_NotPropertyNotFound() { - // Expect conversion error logs from PropertyConversion (once from local SetProperty, - // once from ComponentOps fallback trying reflection before SerializedProperty) - LogAssert.Expect(LogType.Error, new Regex("Error converting token")); + // Expect conversion error log from PropertyConversion (ComponentOps reflection attempt) LogAssert.Expect(LogType.Error, new Regex("Error converting token")); // Expect the warning log from SetComponentPropertiesInternal LogAssert.Expect(LogType.Warning, new Regex("Failed to set")); From bad1057607244da542e3ebf4b7d4fdea86db53d3 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 16 Feb 2026 19:11:48 -0800 Subject: [PATCH 4/4] Fix empty array error message and support [SerializeField] in nested paths Address CodeRabbit review round 2: - Handle empty arrays/lists separately to avoid nonsensical "out of range (0--1)" - Add [SerializeField] private field lookup for final segment of nested paths - Expose FindSerializedFieldInHierarchy as internal for cross-class reuse Co-Authored-By: Claude Opus 4.6 --- MCPForUnity/Editor/Helpers/ComponentOps.cs | 2 +- .../GameObjects/GameObjectComponentHelpers.cs | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/MCPForUnity/Editor/Helpers/ComponentOps.cs b/MCPForUnity/Editor/Helpers/ComponentOps.cs index 755525d39..5b19bb8a0 100644 --- a/MCPForUnity/Editor/Helpers/ComponentOps.cs +++ b/MCPForUnity/Editor/Helpers/ComponentOps.cs @@ -325,7 +325,7 @@ public static List GetAccessibleMembers(Type componentType) /// Type.GetField() with NonPublic only returns fields declared directly on that type, /// so this method walks up the chain to find inherited private serialized fields. /// - private static FieldInfo FindSerializedFieldInHierarchy(Type type, string fieldName) + internal static FieldInfo FindSerializedFieldInHierarchy(Type type, string fieldName) { if (type == null || string.IsNullOrEmpty(fieldName)) return null; diff --git a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs index c618ae4e5..1e7063ae6 100644 --- a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs +++ b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs @@ -278,6 +278,11 @@ private static bool SetNestedProperty(object target, string path, JToken value, if (currentObject is Material[]) { var materials = currentObject as Material[]; + if (materials.Length == 0) + { + error = $"Material array is empty in path '{path}', cannot access index {arrayIndex}."; + return false; + } if (arrayIndex < 0 || arrayIndex >= materials.Length) { error = $"Material index {arrayIndex} out of range (0-{materials.Length - 1}) in path '{path}'."; @@ -288,6 +293,11 @@ private static bool SetNestedProperty(object target, string path, JToken value, else if (currentObject is System.Collections.IList) { var list = currentObject as System.Collections.IList; + if (list.Count == 0) + { + error = $"List is empty in path '{path}', cannot access index {arrayIndex}."; + return false; + } if (arrayIndex < 0 || arrayIndex >= list.Count) { error = $"Index {arrayIndex} out of range (0-{list.Count - 1}) in path '{path}'."; @@ -343,6 +353,20 @@ private static bool SetNestedProperty(object target, string path, JToken value, return false; } + // Try non-public [SerializeField] fields (nested paths need this too) + FieldInfo serializedField = ComponentOps.FindSerializedFieldInHierarchy(currentType, finalPart); + if (serializedField != null) + { + object convertedValue = ConvertJTokenToType(value, serializedField.FieldType, inputSerializer); + if (convertedValue != null || value.Type == JTokenType.Null) + { + serializedField.SetValue(currentObject, convertedValue); + return true; + } + error = $"Failed to convert value for '{finalPart}' to type '{serializedField.FieldType.Name}' in path '{path}'."; + return false; + } + error = $"Property or field '{finalPart}' not found on type '{currentType.Name}' in path '{path}'."; } catch (Exception ex)