Fix read_console crash on Unity 6.5 and batch-mode test stability (#761)#762
Merged
dsarno merged 3 commits intoCoplayDev:betafrom Feb 16, 2026
Merged
Conversation
Contributor
Reviewer's GuideImplements robust UnityEvent setting via Unity SerializedProperty instead of reflection, ensures batch_execute only normalizes top-level parameter keys, removes fragile LogEntry.instanceID reflection, skips a crashy Collider.GeometryHolder property during serialization, relaxes CodexConfigHelper tests to handle both release and prerelease argument layouts, and adds com.unity.test-framework as an explicit package dependency. Sequence diagram for UnityEvent setting via batch_execute and ComponentOpssequenceDiagram
actor Client
participant BatchExecute
participant ComponentOps
participant SerializedObject
participant SerializedProperty
participant UnityEventTestComponent
Client->>BatchExecute: ExecuteCommand(params JSON)
Note over BatchExecute: NormalizeParameterKeys
BatchExecute->>BatchExecute: ToCamelCase on top-level keys
BatchExecute->>ComponentOps: SetProperty(component, propertyName, value, out error)
activate ComponentOps
ComponentOps->>ComponentOps: ResolveMemberType(componentType, propertyName, normalizedName)
ComponentOps-->>ComponentOps: memberType
alt memberType is UnityEventBase-derived
ComponentOps->>ComponentOps: SetViaSerializedProperty(component, propertyName, normalizedName, value, out error)
ComponentOps->>SerializedObject: new SerializedObject(component)
SerializedObject-->>ComponentOps: serializedObject
ComponentOps->>SerializedObject: FindProperty(propertyName)
SerializedObject-->>ComponentOps: SerializedProperty
alt SerializedProperty found
loop Recursive set
ComponentOps->>ComponentOps: SetSerializedPropertyRecursive(prop, token, error, depth)
alt prop is array and token is JArray
ComponentOps->>SerializedProperty: set arraySize
ComponentOps->>SerializedProperty: GetArrayElementAtIndex(i)
else prop is generic and token is JObject
ComponentOps->>ComponentOps: FindPropertyRelativeFuzzy(parent, key)
else prop is ObjectReference
ComponentOps->>ComponentOps: SetObjectReference(prop, token, out error)
else leaf type
ComponentOps->>ComponentOps: Coerce value via ParamCoercion
end
end
ComponentOps->>SerializedObject: ApplyModifiedProperties
SerializedObject-->>UnityEventTestComponent: updated UnityEvent backing data
else SerializedProperty not found
ComponentOps-->>BatchExecute: error
end
ComponentOps-->>BatchExecute: success
else memberType is not UnityEvent
ComponentOps->>ComponentOps: Reflective property/field set
ComponentOps-->>BatchExecute: success
end
deactivate ComponentOps
BatchExecute-->>Client: Command result
Updated class diagram for ComponentOps UnityEvent support and test componentclassDiagram
class ComponentOps {
+bool SetProperty(Component component, string propertyName, JToken value, string error)
-Type ResolveMemberType(Type componentType, string propertyName, string normalizedName)
-bool SetViaSerializedProperty(Component component, string propertyName, string normalizedName, JToken value, string error)
-bool SetSerializedPropertyRecursive(SerializedProperty prop, JToken value, string error, int depth)
-SerializedProperty FindPropertyRelativeFuzzy(SerializedProperty parent, string key)
-bool SetObjectReference(SerializedProperty prop, JToken value, string error)
-bool SetEnum(SerializedProperty prop, JToken value, string error)
}
class UnityEventBase {
}
class UnityEventTestComponent {
+UnityEvent onSimpleEvent
+UnityEvent~float~ onFloatEvent
-UnityEvent _onPrivateEvent
}
class SerializedObject {
+SerializedObject(Object targetObject)
+SerializedProperty FindProperty(string propertyPath)
+void ApplyModifiedProperties()
+void Update()
}
class SerializedProperty {
+bool isArray
+int arraySize
+SerializedPropertyType propertyType
+string propertyPath
+string[] enumNames
+int enumValueIndex
+int depth
+string name
+SerializedObject serializedObject
+SerializedProperty GetArrayElementAtIndex(int index)
+SerializedProperty FindPropertyRelative(string relativePropertyPath)
+bool Next(bool enterChildren)
+SerializedProperty GetEndProperty()
+static bool EqualContents(SerializedProperty x, SerializedProperty y)
+int intValue
+bool boolValue
+float floatValue
+string stringValue
+Object objectReferenceValue
}
class Collider {
}
class GameObjectSerializer {
+object GetComponentData(Component c, bool includeNonPublicSerialized, bool includeNonPublicNonSerialized)
}
ComponentOps ..> UnityEventBase : uses
ComponentOps ..> SerializedObject : uses
ComponentOps ..> SerializedProperty : uses
ComponentOps ..> GameObjectSerializer : coordinates via Component operations
UnityEventTestComponent --|> MonoBehaviour
UnityEventTestComponent ..> UnityEventBase : UnityEvent fields derive from
GameObjectSerializer ..> Collider : inspects type for GeometryHolder
GameObjectSerializer ..> ComponentOps : uses SetProperty for component data
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR fixes Unity 6.5 compatibility by removing deprecated instanceID reflection in ReadConsole, adds skip logic for Collider.GeometryHolder to prevent crashes, improves resource management in ComponentOps, adds test-framework dependency, and refactors console validation test helpers. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 517-519: The branch handling SerializedPropertyType.String should
assign an empty string rather than null to prop.stringValue; change the fallback
from null to string.Empty (or "") when value is null or value.Type ==
JTokenType.Null so prop.stringValue = value == null || value.Type ==
JTokenType.Null ? string.Empty : value.ToString();—update the code in the case
for SerializedPropertyType.String that sets prop.stringValue.
- Around line 412-430: The SetViaSerializedProperty method creates a
SerializedObject (var so = new SerializedObject(component)) but never disposes
it; wrap the SerializedObject in a using block so it is always disposed even on
early returns, e.g. change the method to create the SerializedObject inside a
using (...) { ... } scope, keep the FindProperty/SetSerializedPropertyRecursive
calls and so.ApplyModifiedProperties() inside that scope, and ensure out error
returns still occur inside the using so disposal is guaranteed (referencing
SetViaSerializedProperty, the local variable so, and
SetSerializedPropertyRecursive).
- Around line 484-496: The integer coercion uses int.MinValue as a failure
sentinel which wrongly rejects legitimate int.MinValue assignments; update
ParamCoercion.CoerceInt to return a nullable int (int?) or a (bool success, int
value) tuple, then change the SerializedPropertyType.Integer branch in
ComponentOps (the switch handling the int case) to call the new API, check the
explicit success flag/null before running the existing JToken-type checks, and
only set prop.intValue when the coercion succeeded; preserve the existing error
string logic when coercion fails.
- Around line 606-623: The heuristic that treats any string containing '/' as an
asset path is too permissive; update the branch in the value.Type ==
JTokenType.String handling so it only treats a string as a path when it clearly
looks like one (e.g., strVal.StartsWith("Assets/",
StringComparison.OrdinalIgnoreCase) || strVal.StartsWith("Packages/",
StringComparison.OrdinalIgnoreCase)) instead of using strVal.Contains("/"); keep
using AssetPathUtility.SanitizeAssetPath and
AssetDatabase.LoadAssetAtPath<UnityEngine.Object> to resolve, set
prop.objectReferenceValue on success and return false with the same error
message on failure.
- Around line 629-659: Add a brief inline comment inside
FindPropertyRelativeFuzzy explaining that the underscore-insensitive matching
can produce false positives when two sibling properties differ only by
underscores (e.g., "m_Value" vs "mValue"), that this is an intentional pragmatic
fallback for transport-stripped underscores, and that the code assumes such
collisions are unlikely in Unity serialized data; reference the method name
FindPropertyRelativeFuzzy and the normalizedKey/normalizedName comparison to
indicate where the comment should be placed.
In `@MCPForUnity/Editor/Tools/ReadConsole.cs`:
- Around line 514-531: Remove the dead private static method
GetRemappedTypeForFiltering from the ReadConsole class: delete the entire method
definition (the switch block returning remapped LogType values) since it is
unused anywhere in the codebase; after removal, run a quick compile to ensure no
callers exist and remove any now-unused usings only if the compiler flags them.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs`:
- Around line 20-38: In AssertValidUvxArgs, make the TomlString cast and
prerelease lookup defensive: when building argValues from args.Children, check
each child is a TomlString (use the result of the cast != null) and call
Assert.Fail with a clear message if a non-string node is encountered; likewise,
before checking argValues[prereleaseIndex + 1], ensure prereleaseIndex + 1 <
argValues.Count and Assert.Fail with a descriptive message if it isn’t,
otherwise perform the equality assertion. This keeps AssertValidUvxArgs robust
and yields clear diagnostics on failure.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BatchExecuteKeyPreservationTests.cs`:
- Line 15: Add the NUnit [TestFixture] attribute to the
BatchExecuteKeyPreservationTests class to make the test intent explicit; ensure
NUnit.Framework is imported (using NUnit.Framework) if not already, then
annotate the class declaration public class BatchExecuteKeyPreservationTests
with [TestFixture].
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentOpsUnityEventTests.cs`:
- Around line 35-50: Extract a small helper method (e.g.,
MakePersistentCallJson(int targetId, string methodName, bool boolArg, int mode =
6, int callState = 2)) that builds and returns the JObject/string for the
m_PersistentCalls -> m_Calls entry used in ComponentOpsUnityEventTests; replace
the repeated inline JSON blocks (the JObject.Parse(...) that contains
"m_PersistentCalls" and the inner "m_Calls" object) in the tests with calls to
this helper, and for tests that need multiple entries create a small wrapper
like MakePersistentCallsArray(...) or call the helper multiple times to assemble
the array, keeping parameter names targetId, methodName, and boolArg to match
existing usage.
🧹 Nitpick comments (7)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`: - Around line 484-496: The integer coercion uses int.MinValue as a failure sentinel which wrongly rejects legitimate int.MinValue assignments; update ParamCoercion.CoerceInt to return a nullable int (int?) or a (bool success, int value) tuple, then change the SerializedPropertyType.Integer branch in ComponentOps (the switch handling the int case) to call the new API, check the explicit success flag/null before running the existing JToken-type checks, and only set prop.intValue when the coercion succeeded; preserve the existing error string logic when coercion fails. - Around line 606-623: The heuristic that treats any string containing '/' as an asset path is too permissive; update the branch in the value.Type == JTokenType.String handling so it only treats a string as a path when it clearly looks like one (e.g., strVal.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase) || strVal.StartsWith("Packages/", StringComparison.OrdinalIgnoreCase)) instead of using strVal.Contains("/"); keep using AssetPathUtility.SanitizeAssetPath and AssetDatabase.LoadAssetAtPath<UnityEngine.Object> to resolve, set prop.objectReferenceValue on success and return false with the same error message on failure. - Around line 629-659: Add a brief inline comment inside FindPropertyRelativeFuzzy explaining that the underscore-insensitive matching can produce false positives when two sibling properties differ only by underscores (e.g., "m_Value" vs "mValue"), that this is an intentional pragmatic fallback for transport-stripped underscores, and that the code assumes such collisions are unlikely in Unity serialized data; reference the method name FindPropertyRelativeFuzzy and the normalizedKey/normalizedName comparison to indicate where the comment should be placed. In `@MCPForUnity/Editor/Tools/ReadConsole.cs`: - Around line 514-531: Remove the dead private static method GetRemappedTypeForFiltering from the ReadConsole class: delete the entire method definition (the switch block returning remapped LogType values) since it is unused anywhere in the codebase; after removal, run a quick compile to ensure no callers exist and remove any now-unused usings only if the compiler flags them. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs`: - Around line 20-38: In AssertValidUvxArgs, make the TomlString cast and prerelease lookup defensive: when building argValues from args.Children, check each child is a TomlString (use the result of the cast != null) and call Assert.Fail with a clear message if a non-string node is encountered; likewise, before checking argValues[prereleaseIndex + 1], ensure prereleaseIndex + 1 < argValues.Count and Assert.Fail with a descriptive message if it isn’t, otherwise perform the equality assertion. This keeps AssertValidUvxArgs robust and yields clear diagnostics on failure. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BatchExecuteKeyPreservationTests.cs`: - Line 15: Add the NUnit [TestFixture] attribute to the BatchExecuteKeyPreservationTests class to make the test intent explicit; ensure NUnit.Framework is imported (using NUnit.Framework) if not already, then annotate the class declaration public class BatchExecuteKeyPreservationTests with [TestFixture]. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentOpsUnityEventTests.cs`: - Around line 35-50: Extract a small helper method (e.g., MakePersistentCallJson(int targetId, string methodName, bool boolArg, int mode = 6, int callState = 2)) that builds and returns the JObject/string for the m_PersistentCalls -> m_Calls entry used in ComponentOpsUnityEventTests; replace the repeated inline JSON blocks (the JObject.Parse(...) that contains "m_PersistentCalls" and the inner "m_Calls" object) in the tests with calls to this helper, and for tests that need multiple entries create a small wrapper like MakePersistentCallsArray(...) or call the helper multiple times to assemble the array, keeping parameter names targetId, methodName, and boolArg to match existing usage.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
20-38: Good extraction of shared validation logic.The helper cleanly consolidates repeated assertions and correctly handles both release and prerelease arg orderings, matching the
GetBetaServerFromArgsListcontract inAssetPathUtility.cs.Two minor robustness notes for test diagnostics:
- Line 24 —
(child as TomlString).Valuewill throwNullReferenceExceptionif any child isn't aTomlString, producing a confusing failure instead of a clear assertion message.- Line 36 —
argValues[prereleaseIndex + 1]has no bounds guard if--prereleasehappens to be the last element.Neither is likely to bite given the controlled inputs, but a small tweak would yield better diagnostics on failure:
💡 Optional: safer casting with a clearer failure message
var argValues = new List<string>(); foreach (TomlNode child in args.Children) - argValues.Add((child as TomlString).Value); + { + Assert.IsInstanceOf<TomlString>(child, $"Expected TomlString but got {child?.GetType().Name}"); + argValues.Add(((TomlString)child).Value); + }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs` around lines 20 - 38, In AssertValidUvxArgs, make the TomlString cast and prerelease lookup defensive: when building argValues from args.Children, check each child is a TomlString (use the result of the cast != null) and call Assert.Fail with a clear message if a non-string node is encountered; likewise, before checking argValues[prereleaseIndex + 1], ensure prereleaseIndex + 1 < argValues.Count and Assert.Fail with a descriptive message if it isn’t, otherwise perform the equality assertion. This keeps AssertValidUvxArgs robust and yields clear diagnostics on failure.MCPForUnity/Editor/Helpers/ComponentOps.cs (3)
484-496:int.MinValueas a sentinel for coercion failure is fragile.If a caller legitimately wants to set a serialized int property to
int.MinValue, this logic would reject it. The NaN sentinel used for float (line 508) is better because NaN is inherently "not a valid number." Consider returning a success/failure tuple fromParamCoercion.CoerceIntor using a nullable return instead, though the risk here is very low in practice.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 484 - 496, The integer coercion uses int.MinValue as a failure sentinel which wrongly rejects legitimate int.MinValue assignments; update ParamCoercion.CoerceInt to return a nullable int (int?) or a (bool success, int value) tuple, then change the SerializedPropertyType.Integer branch in ComponentOps (the switch handling the int case) to call the new API, check the explicit success flag/null before running the existing JToken-type checks, and only set prop.intValue when the coercion succeeded; preserve the existing error string logic when coercion fails.
606-623: Broad heuristic for path detection — any string containing/is treated as an asset path.The check
strVal.Contains("/")(line 609) is very permissive. Strings like"on/off","true/false", or component type names with namespaces could inadvertently match. Consider tightening the check, e.g., also verifying the string doesn't start with common non-path patterns, or only accepting strings that start with"Assets/"or"Packages/".♻️ Suggested tightening
-if (strVal.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase) || strVal.Contains("/")) +if (strVal.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase) || + strVal.StartsWith("Packages/", StringComparison.OrdinalIgnoreCase))🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 606 - 623, The heuristic that treats any string containing '/' as an asset path is too permissive; update the branch in the value.Type == JTokenType.String handling so it only treats a string as a path when it clearly looks like one (e.g., strVal.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase) || strVal.StartsWith("Packages/", StringComparison.OrdinalIgnoreCase)) instead of using strVal.Contains("/"); keep using AssetPathUtility.SanitizeAssetPath and AssetDatabase.LoadAssetAtPath<UnityEngine.Object> to resolve, set prop.objectReferenceValue on success and return false with the same error message on failure.
629-659:FindPropertyRelativeFuzzyis a pragmatic workaround — document the risk of false matches.The underscore-insensitive fuzzy matching could collide if two sibling properties differ only by underscores (e.g.,
m_ValuevsmValue). This is unlikely in Unity's serialized data but worth a brief inline comment noting the assumption.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 629 - 659, Add a brief inline comment inside FindPropertyRelativeFuzzy explaining that the underscore-insensitive matching can produce false positives when two sibling properties differ only by underscores (e.g., "m_Value" vs "mValue"), that this is an intentional pragmatic fallback for transport-stripped underscores, and that the code assumes such collisions are unlikely in Unity serialized data; reference the method name FindPropertyRelativeFuzzy and the normalizedKey/normalizedName comparison to indicate where the comment should be placed.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BatchExecuteKeyPreservationTests.cs (1)
15-15: Consider adding[TestFixture]attribute for convention.While NUnit discovers test methods without it, adding
[TestFixture]on the class is conventional and makes the intent explicit.♻️ Suggested addition
+ [TestFixture] public class BatchExecuteKeyPreservationTests🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/BatchExecuteKeyPreservationTests.cs` at line 15, Add the NUnit [TestFixture] attribute to the BatchExecuteKeyPreservationTests class to make the test intent explicit; ensure NUnit.Framework is imported (using NUnit.Framework) if not already, then annotate the class declaration public class BatchExecuteKeyPreservationTests with [TestFixture].MCPForUnity/Editor/Tools/ReadConsole.cs (1)
514-531: Remove unusedGetRemappedTypeForFilteringmethod.This private static method at lines 514–531 is never called anywhere in the codebase and can be safely removed to eliminate dead code.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@MCPForUnity/Editor/Tools/ReadConsole.cs` around lines 514 - 531, Remove the dead private static method GetRemappedTypeForFiltering from the ReadConsole class: delete the entire method definition (the switch block returning remapped LogType values) since it is unused anywhere in the codebase; after removal, run a quick compile to ensure no callers exist and remove any now-unused usings only if the compiler flags them.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentOpsUnityEventTests.cs (1)
35-50: Consider extracting a helper to build the UnityEvent JSON payload.The same JSON structure for a persistent call (target, assembly type name, method name, mode, arguments, call state) is repeated across five tests with only minor variations (different
m_BoolArgumentvalues, different counts). A small helper likeMakePersistentCallJson(int targetId, string method, bool boolArg)would reduce duplication and make the tests easier to maintain if Unity's serialization format ever changes.Also applies to: 75-96, 117-130, 155-168, 204-217
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentOpsUnityEventTests.cs` around lines 35 - 50, Extract a small helper method (e.g., MakePersistentCallJson(int targetId, string methodName, bool boolArg, int mode = 6, int callState = 2)) that builds and returns the JObject/string for the m_PersistentCalls -> m_Calls entry used in ComponentOpsUnityEventTests; replace the repeated inline JSON blocks (the JObject.Parse(...) that contains "m_PersistentCalls" and the inner "m_Calls" object) in the tests with calls to this helper, and for tests that need multiple entries create a small wrapper like MakePersistentCallsArray(...) or call the helper multiple times to assemble the array, keeping parameter names targetId, methodName, and boolArg to match existing usage.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 484-496: The code treats int.MinValue as an error sentinel which
collides with a legitimate value; update the logic to use an explicit success
flag instead: change ParamCoercion.CoerceInt to a TryCoerceInt(out int result,
JToken value, int default) or add an out/bool overload and call that from the
SerializedPropertyType.Integer branch, rely on the returned bool to decide
success/failure (set error = "Expected integer value." and return false on
failure) and assign prop.intValue = result when true; alternatively, if you
prefer not to change ParamCoercion, first inspect value.Type
(JTokenType.Integer/String/Null) to decide failure vs. accepting int.MinValue,
but prefer the TryCoerceInt(out ...) approach for clarity.
- Around line 575-586: The GUID resolution code using guidToken,
AssetDatabase.GUIDToAssetPath and AssetDatabase.LoadAssetAtPath can silently
assign null to prop.objectReferenceValue if the path exists but the asset fails
to load; modify the logic in the GUID-handling block to check the result of
LoadAssetAtPath for null, and if null set error (including the GUID and resolved
path) and return false instead of assigning null to prop.objectReferenceValue;
keep the existing checks for empty path and only assign to
prop.objectReferenceValue after verifying the loaded asset is non-null.
Comment on lines
575
to
586
| var guidToken = jObj["guid"]; | ||
| if (guidToken != null) | ||
| { | ||
| string path = AssetDatabase.GUIDToAssetPath(guidToken.ToString()); | ||
| if (string.IsNullOrEmpty(path)) | ||
| { | ||
| error = $"No asset found for GUID '{guidToken}'."; | ||
| return false; | ||
| } | ||
| prop.objectReferenceValue = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
GUID resolution may silently assign null if the asset exists in the database but fails to load.
GUIDToAssetPath can return a non-empty path for an asset that was deleted from disk but still tracked. LoadAssetAtPath would then return null, and prop.objectReferenceValue would be silently set to null without an error.
Suggested guard
string path = AssetDatabase.GUIDToAssetPath(guidToken.ToString());
if (string.IsNullOrEmpty(path))
{
error = $"No asset found for GUID '{guidToken}'.";
return false;
}
- prop.objectReferenceValue = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path);
- return true;
+ var loaded = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path);
+ if (loaded == null)
+ {
+ error = $"Asset at path '{path}' (GUID '{guidToken}') could not be loaded.";
+ return false;
+ }
+ prop.objectReferenceValue = loaded;
+ return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var guidToken = jObj["guid"]; | |
| if (guidToken != null) | |
| { | |
| string path = AssetDatabase.GUIDToAssetPath(guidToken.ToString()); | |
| if (string.IsNullOrEmpty(path)) | |
| { | |
| error = $"No asset found for GUID '{guidToken}'."; | |
| return false; | |
| } | |
| prop.objectReferenceValue = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path); | |
| return true; | |
| } | |
| var guidToken = jObj["guid"]; | |
| if (guidToken != null) | |
| { | |
| string path = AssetDatabase.GUIDToAssetPath(guidToken.ToString()); | |
| if (string.IsNullOrEmpty(path)) | |
| { | |
| error = $"No asset found for GUID '{guidToken}'."; | |
| return false; | |
| } | |
| var loaded = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path); | |
| if (loaded == null) | |
| { | |
| error = $"Asset at path '{path}' (GUID '{guidToken}') could not be loaded."; | |
| return false; | |
| } | |
| prop.objectReferenceValue = loaded; | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 575 - 586, The GUID
resolution code using guidToken, AssetDatabase.GUIDToAssetPath and
AssetDatabase.LoadAssetAtPath can silently assign null to
prop.objectReferenceValue if the path exists but the asset fails to load; modify
the logic in the GUID-handling block to check the result of LoadAssetAtPath for
null, and if null set error (including the GUID and resolved path) and return
false instead of assigning null to prop.objectReferenceValue; keep the existing
checks for empty path and only assign to prop.objectReferenceValue after
verifying the loaded asset is non-null.
…ash (CoplayDev#761) - Remove unused LogEntry.instanceID reflection that fatally blocked read_console initialization on Unity 6000.5.0a6 - Skip Collider.GeometryHolder in GameObjectSerializer to prevent native PhysX SEGV in batch-mode test runs - Fix CodexConfigHelperTests to work with both release and prerelease package versions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The package has a hard compile dependency on Test Framework (RunTests, TestJobManager, TestRunnerService, etc.) but did not declare it in package.json. Projects without Test Framework installed would get CS0234/CS0246 errors on import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e dead method - Wrap SerializedObject in using block in SetViaSerializedProperty - Use string.Empty instead of null for SerializedProperty.stringValue - Remove unused GetRemappedTypeForFiltering from ReadConsole Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dsarno
force-pushed
the
fix/read-console-instanceid-761
branch
from
93241f5 to
3f95d6c
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Generated with Claude Code
Summary by Sourcery
Improve Unity integration stability by fixing console reflection issues, ensuring safe collider serialization, and supporting UnityEvent configuration via serialized properties and batch commands.
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests
Chores