Skip to content

TryPatch descends into JSON-object leaf nodes instead of delegating to the registered converter (object-refs unresolvable via merge-patch) #84

Description

@IvanMurzak

Summary

Reflector.TryPatch (the JSON Merge Patch path) cannot apply a value whose JSON encoding is an object handled atomically by a registered converter — e.g. a Unity object reference {"instanceID":"…"}. It structurally descends into the node as if it were a bag of members to navigate, instead of delegating to the converter registered for the target member type.

Reported via the Unity-MCP tool gameobject-component-modify:

jsonPatch = { "sharedMaterial": { "instanceID": "568105589213746962" } }
=> "Segment 'instanceID' not found on type 'Material'"

Affects both the object and stringified forms of jsonPatch equally. (Distinct from #82, which fixed the string-vs-object schema/validation; this is the apply-time layer.)

Root cause

src/Reflector/Reflector.Patch.csTryPatchInternal branches purely on JSON ValueKind:

  • A non-object leaf (string/number/bool/null) is delegated to TryModify → the converter chain → resolves correctly (Reflector.Patch.cs:109-114).
  • An object node is always treated as navigable members and recursed via TryPatchMember (Reflector.Patch.cs:116+, 194-206). For {"instanceID":"…"} against a Material property it enumerates key instanceID, finds no such member on Material, and throws Segment '<key>' not found on type '<T>' (Reflector.Patch.cs:318).

The converter registry is never consulted for a ValueKind == Object node, so a reference encoded as a JSON object bypasses the converter that knows how to resolve it.

Why the other surfaces work

TryModify (componentDiff/SerializedMember) and TryModifyAt (pathPatches) both funnel the {instanceID:…} object into Converters.GetConverter(type) → the converter's SetValue (Reflector.Modify.cs:70-92). For Unity types that converter (UnityEngine_Object_ReflectionConverter, registered by Unity-MCP) resolves the ref via ToAssetObjectRef().FindAssetObject(type). The merge-patch path never reaches that resolver.

Proposed fix (ReflectorNet)

Add a converter capability flag, e.g. bool TreatJsonObjectAsAtomicValue(Type type) on IReflectionConverter (default false in BaseReflectionConverter). In TryPatchInternal, before the structural key-enumeration block and after the leaf checks (~Reflector.Patch.cs:114): when patch.ValueKind == Object, the target type has a converter, that flag is true, and the node has no $type key → route the node through TryModify (the same path the other two surfaces use). Plain POCO sub-objects keep the flag false and descend exactly as today. Keep $type polymorphic replacement flowing through the existing structural path.

The Unity UnityEngine_Object_ReflectionConverter overrides the new flag to true (one line, in Unity-MCP).

Blast radius / tests

  • Callers of TryPatch: 5 Unity-MCP modify tools (gameobject-component-modify, object-modify, gameobject-modify, assets-modify); ReflectorNet AtomicModifyTests; Unity-MCP TryPatchTests + TestToolGameObject.JsonPatchObjectOrString (the latter deliberately punted on the object-ref case). No direct callers in MCP-Plugin-dotnet/extensions.
  • Risk to plain-POCO patching: low when gated on a default-false capability flag — existing TryPatch_* tests (multi-field, deep, null, $type) unaffected.
  • Add a ReflectorNet test using a stub converter whose flag is true + SetValue consumes a {ref:…} object (proves routing with no Unity dependency).
  • Foundational lib: a fix here implies a NuGet bump + downstream consumption (Unity-MCP, MCP-Plugin-dotnet) — a deliberate release cascade.

Acceptance criteria

  • TryPatch routes converter-atomic JSON-object nodes (e.g. object-refs) through the registered converter instead of descending structurally.
  • Plain POCO sub-object patching unchanged; $type replacement unchanged; all existing TryPatch_* tests green (net8.0 + net9.0).
  • New ReflectorNet test covers the converter-atomic object-node routing (Unity-agnostic stub converter).
  • Downstream: gameobject-component-modify with jsonPatch={"sharedMaterial":{"instanceID":…}} resolves the Material (tracked in the Unity-MCP cross-ref issue).

Discovered while verifying #82 (PR #83) end-to-end against the live gameobject-component-modify tool.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions