bindings/go/runtime/registry_convert_test.go (1)

267-279: Good documentation of unsupported conversion.

This test correctly documents that Typed → Unstructured conversion is not supported. Consider adding a code comment or updating the Convert method's documentation to explicitly mention this limitation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/registry_convert_test.go` around lines 267 - 279, Add
explicit documentation and an inline code comment to clarify that converting a
typed object to runtime.Unstructured via scheme.Convert is unsupported; update
the Convert method's docstring (the Convert function on the scheme type) to
state this limitation and reference the TestConvert_TypedToUnstructured test as
an example, and add a short comment above the test or the scheme.MustRegister
call explaining that Typed → Unstructured conversion fails by design due to
reflection assignment limitations.

bindings/go/runtime/unstructured.go (1)

46-63: Stale TODO comment should be removed or clarified.

The TODO on line 47 says "check if string, parse type otherwise use below" but the code already implements this logic. Either remove the TODO or clarify what remains to be done.

Also, there's a subtle edge case: when Get[Type] succeeds but returns a zero-value Type (e.g., if someone explicitly stored runtime.Type{} in the map), the function returns that zero value without attempting string parsing. This seems intentional but worth noting.

🧹 Remove stale TODO

 func (u *Unstructured) GetType() Type {
-	// TODO check if string, parse type otherwise use below
 	v, ok := Get[Type](u, IdentityAttributeType)
 	if !ok {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/unstructured.go` around lines 46 - 63, Remove the stale
TODO in Unstructured.GetType and replace it with a short clarifying comment that
documents the intended behavior: that the function first attempts to
Get[Type](u, IdentityAttributeType) and will return that value (including a
zero-value Type if present), and only if that lookup fails will it attempt to
Get[string], parse via TypeFromString and fall back to NewUnversionedType;
explicitly note the zero-value edge case so future readers know this is
intentional.

bindings/go/runtime/registry_convert_test.go (1)

229-236: Assert raw.Type in the Unstructured → Raw test.

The JSON round-trip proves the payload still contains the type string, but it will not catch a regression where Raw.UnmarshalJSON stops populating raw.Type—the field Scheme.Convert uses on the next hop.

✅ Small test hardening

 	raw := &runtime.Raw{}
 	err := scheme.Convert(un, raw)
 	require.NoError(t, err)
+	assert.Equal(t, typ, raw.Type)

 	var parsed TestType
 	require.NoError(t, json.Unmarshal(raw.Data, &parsed))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/registry_convert_test.go` around lines 229 - 236, Add an
assertion that raw.Type is populated after converting the Unstructured to
runtime.Raw: after calling scheme.Convert(un, raw) (and require.NoError), assert
that raw.Type equals the expected typ (the same variable used later) to catch
regressions in Raw.UnmarshalJSON; reference runtime.Raw, scheme.Convert and
Raw.UnmarshalJSON and place the check before unmarshalling raw.Data into
TestType.

bindings/go/runtime/registry.go (1)

415-421: ⚠️ Potential issue | 🟠 Major

Reset the destination before JSON decode.

Both decode helpers still call json.Unmarshal on the existing target, so a reused into can retain omitted struct fields or map keys from a previous conversion. That makes Raw -> Typed and Unstructured -> Typed behave like patches instead of replacements.

💡 Suggested fix

 func (r *Scheme) convertFromRaw(from *Raw, into Typed, fromType Type) error {
 	// Raw → Raw: Deep copy the underlying data.
 	if rawInto, ok := into.(*Raw); ok {
 		from.DeepCopyInto(rawInto)
 		return nil
 	}

 	// Raw → Typed: Unmarshal the Raw.Data into the target.
 	if !r.IsRegistered(fromType) && !r.allowUnknown {
 		return fmt.Errorf("cannot decode from unregistered type: %s", fromType)
 	}
+	intoVal := reflect.ValueOf(into)
+	if intoVal.Kind() != reflect.Ptr || intoVal.IsNil() {
+		return fmt.Errorf("'into' must be a non-nil pointer")
+	}
+	intoVal.Elem().Set(reflect.Zero(intoVal.Elem().Type()))
 	if err := json.Unmarshal(from.Data, into); err != nil {
 		return fmt.Errorf("failed to unmarshal from raw: %w", err)
 	}
 	return nil
 }
@@
 	data, err := json.Marshal(from.Data)
 	if err != nil {
 		return fmt.Errorf("failed to marshal unstructured data: %w", err)
 	}
+	intoVal := reflect.ValueOf(into)
+	if intoVal.Kind() != reflect.Ptr || intoVal.IsNil() {
+		return fmt.Errorf("'into' must be a non-nil pointer")
+	}
+	intoVal.Elem().Set(reflect.Zero(intoVal.Elem().Type()))
 	if err := json.Unmarshal(data, into); err != nil {
 		return fmt.Errorf("failed to unmarshal from unstructured: %w", err)
 	}

In Go's encoding/json package, when json.Unmarshal decodes into a non-zero struct or map, do omitted struct fields and pre-existing map keys remain unchanged?

Also applies to: 433-442

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/registry.go` around lines 415 - 421, The decode path that
does "json.Unmarshal(from.Data, into)" can leave previous values in the target
when reused; before calling json.Unmarshal in the Raw -> Typed block (the code
using variables from/into and the r.allowUnknown check) and the analogous
Unstructured -> Typed section, reset the destination pointed to by into to its
zero value via reflection (handle pointers by dereferencing/allocating as
needed, and clear maps/slices) so json.Unmarshal writes a fresh value rather
than merging with prior contents. Ensure you use reflect to set into to
reflect.Zero(reflect.TypeOf(...)) or recreate the pointed value for
pointer-to-struct/map/slice cases before calling json.Unmarshal.

bindings/go/runtime/registry_convert_test.go (1)

373-387: Assert the parsed type in the allowUnknown case.

This test currently only checks Foo, so it would still pass if the conversion started dropping or zeroing the unknown Type value. Since this PR is about type handling, it is worth locking that in explicitly.

✅ Suggested test tightening

 func TestConvert_UnstructuredToTyped_AllowUnknown(t *testing.T) {
 	scheme := runtime.NewScheme(runtime.WithAllowUnknown())
+	typ := runtime.NewVersionedType("UnknownType", "v1")

 	un := &runtime.Unstructured{
 		Data: map[string]any{
-			"type": "UnknownType/v1",
+			"type": typ.String(),
 			"foo":  "bar",
 		},
 	}

 	out := &TestType{}
 	err := scheme.Convert(un, out)
 	require.NoError(t, err)
 	assert.Equal(t, "bar", out.Foo)
+	assert.Equal(t, typ, out.Type)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/registry_convert_test.go` around lines 373 - 387, The
test TestConvert_UnstructuredToTyped_AllowUnknown only asserts out.Foo; add an
assertion that the converted object's type field is preserved by checking the
TestType instance (out) has its Type (or equivalent type-identifying field)
equal to the original "UnknownType/v1" after scheme.Convert; update the
assertions in the test to require no error and assert both out.Foo == "bar" and
out.Type == "UnknownType/v1" (use the actual field name on TestType).

bindings/go/runtime/registry.go (1)

361-367: Update the conversion comments to match the refactor.

The public Convert doc still omits Unstructured -> Raw and still scopes registration failures to Raw conversions, and convertFromTyped still says it returns a boolean even though it now returns only error.

📝 Suggested wording update

 // Special Cases:
 //   - Raw → Raw: performs a deep copy of the underlying []byte data.
 //   - Raw → Typed: unmarshals Raw.Data JSON via json.Unmarshal into the Typed object (if Typed.GetType is registered).
 //   - Typed → Raw: marshals the Typed with json.Marshal, applies canonicalization, and stores the result in Raw.Data.
 //     (See Raw.UnmarshalJSON for equivalent behavior)
+//   - Unstructured → Raw: marshals the Unstructured, applies canonicalization, and stores the result in Raw.Data.
 //   - Unstructured → Typed: marshals the Unstructured.Data to JSON and then unmarshals it into the Typed object (if Typed.GetType is registered).
 //   - Typed → Typed: performs a deep copy using Typed.DeepCopyTyped, with reflection-based assignment.
 //
 // Errors are returned if:
 //   - Either argument is nil.
-//   - A type is not registered in the Scheme (for Raw conversions).
+//   - A source type is not registered in the Scheme for Raw/Unstructured conversions.
 //   - A reflection-based assignment fails due to type mismatch.
@@
-// It returns a boolean indicating whether the conversion was handled, and an error if the conversion failed.
+// It returns an error if the conversion fails.

Also applies to: 448-452

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/registry.go` around lines 361 - 367, Update the public
Convert doc comment and related docs to reflect the refactor: explicitly
document the Unstructured → Raw conversion path (marshals Unstructured.Data to
JSON and stores it in Raw.Raw) in addition to Unstructured→Typed, Typed→Typed,
and Raw→Typed; remove the incorrect statement that registration failures are
limited to Raw conversions and instead state when Scheme registration is
required (e.g., any conversion that needs to instantiate or marshal/unmarshal a
Typed type); and update the convertFromTyped comment to remove the obsolete
"returns a boolean" note and state it now returns only error. Reference the
Convert function and convertFromTyped helper in registry.go when applying these
wording changes.

bindings/go/runtime/registry.go (1)

408-422: Consider the merge behavior of json.Unmarshal for reused destinations.

As discussed in past reviews, json.Unmarshal merges into existing values rather than replacing them. If a caller reuses a destination struct that already has fields set, fields not present in the source JSON will retain their old values. This is a minor concern if callers always pass freshly allocated destinations, but could cause subtle bugs if destinations are reused.

If full replacement semantics are desired, consider zeroing the destination before unmarshaling:

intoVal := reflect.ValueOf(into)
intoVal.Elem().Set(reflect.Zero(intoVal.Elem().Type()))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/registry.go` around lines 408 - 422, The convertFromRaw
method on Scheme currently calls json.Unmarshal(into) which will merge into a
reused destination rather than replace it; to ensure full replacement semantics,
zero the destination before unmarshaling (for the Raw → Typed branch) by
obtaining reflect.ValueOf(into), calling
Elem().Set(reflect.Zero(Elem().Type())), then proceed with json.Unmarshal;
update convertFromRaw (symbols: Scheme.convertFromRaw, Raw, Typed, fromType,
r.allowUnknown, json.Unmarshal, into) to perform this zeroing step so reused
destination structs do not retain stale fields.

bindings/go/runtime/unstructured_test.go (1)

200-204: Test case name is misleading.

The name states "falls back to unversioned" but the expected value is runtime.Type{} (a zero value), not an unversioned type. An unversioned type would have a non-empty Name. Consider renaming for clarity.

Suggested name fix

 {
-    name:     "invalid string with too many segments falls back to unversioned",
+    name:     "invalid string with too many segments returns zero Type",
     data:     map[string]any{"type": "some/invalid/type"},
     expected: runtime.Type{},
 },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/runtime/unstructured_test.go` around lines 200 - 204, The test
case name "invalid string with too many segments falls back to unversioned" is
misleading because the expected value is runtime.Type{} (zero value) not an
unversioned type with a Name; update the test case's name field (the map entry
with name: "invalid string with too many segments falls back to unversioned") to
reflect that it expects a zero/invalid Type (suggestions: "invalid string with
too many segments yields zero Type" or "invalid string with too many segments ->
zero value Type"), leaving the data and expected runtime.Type{} unchanged so the
test intent is clear.