bindings/go/transform/graph/builder/builder_test.go (1)

160-177: Consider asserting the resulting object shape, not just NoError

For this case, it would be valuable to also verify that version is actually absent in the processed output, so the test checks the semantic goal (drop optional nil field), not only execution success.

🤖 Prompt for AI Agents

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

In `@bindings/go/transform/graph/builder/builder_test.go` around lines 160 - 177,
The test currently only asserts staticAnalysisErr and runtimeProcessingErr are
NoError; add an assertion that the processed output for the second
transformation does not contain a version field (i.e. the optional cel reference
${add1.spec.object.?version} produced no value). After the processing step that
yields the result, locate the resolved object for "add2" (or the structure
populated from transformationSpec) and assert that add2.spec.object.version is
absent/nil (use the existing test helpers like require.Nil/require.NotContains
or equivalent) so the test verifies the semantic goal of dropping the optional
nil field.

bindings/go/transform/graph/runtime/resolver/resolver_test.go (1)

634-844: Please add one indexed-path optional deletion test

The new optional-field coverage is solid, but it would help to add a case like items[0].optionalField resolving to nil to lock behavior for array-nested optional fields.

🤖 Prompt for AI Agents

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

In `@bindings/go/transform/graph/runtime/resolver/resolver_test.go` around lines
634 - 844, Add a new t.Run inside TestResolveField_NilOptionalPointer_SpecSchema
that exercises an indexed-path optional deletion: create a resource with an
array property (e.g., "items": []interface{}{
map[string]interface{}{"optionalField": "${optExpr}", "other": "keep"} }), set
data map with "optExpr": nil, call r := NewResolver(resource, data, specSchema)
and result := r.resolveField(variable.FieldDescriptor{Path:
fieldpath.MustParse("items[0].optionalField"), Expressions:
[]variable.Expression{{Value: "optExpr"}}, StandaloneExpression: true}), then
require.NoError(result.Error), require.True(result.Resolved),
require.True(result.Deleted) and assert the first item no longer contains
"optionalField" but still contains "other"; use the same specSchema and
jsonschema refs as the surrounding tests and mirror assertions style used in
other t.Run cases.

bindings/go/transform/graph/runtime/resolver/resolver_test.go (1)

891-915: Add an array-item $ref regression case to harden isOptionalField.

The array test currently covers Items2020 with inline object schema, but not Items2020 pointing to a $ref schema. Adding that case will lock behavior for common generated-schema patterns.

Proposed test addition

 func TestIsOptionalField_NestedRefs(t *testing.T) {
@@
 	rArr := NewResolver(nil, nil, arrayTopSchema)

 	assert.False(t, rArr.isOptionalField(fieldpath.MustParse("servers[0].host")),
 		"host is required inside array item (Items2020)")
 	assert.True(t, rArr.isOptionalField(fieldpath.MustParse("servers[0].timeout")),
 		"timeout is optional inside array item (Items2020)")
 	assert.True(t, rArr.isOptionalField(fieldpath.MustParse("servers[2].timeout")),
 		"timeout is optional regardless of array index")
+
+	// array items via $ref
+	arrayTopSchemaWithRefItems := &jsonschema.Schema{
+		Properties: map[string]*jsonschema.Schema{
+			"servers": {Items2020: &jsonschema.Schema{Ref: itemSchema}},
+		},
+		Required: []string{"servers"},
+	}
+	rArrRef := NewResolver(nil, nil, arrayTopSchemaWithRefItems)
+	assert.False(t, rArrRef.isOptionalField(fieldpath.MustParse("servers[0].host")),
+		"host is required inside array item ($ref)")
+	assert.True(t, rArrRef.isOptionalField(fieldpath.MustParse("servers[0].timeout")),
+		"timeout is optional inside array item ($ref)")
 }

🤖 Prompt for AI Agents

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

In `@bindings/go/transform/graph/runtime/resolver/resolver_test.go` around lines
891 - 915, Add a regression test that mirrors the existing array Items2020 case
but where the array item schema is a $ref instead of an inline object: create a
referenced schema (e.g., "Server" schema with Required ["host"]) and have
arrayTopSchema.Properties["servers"].Items2020 point to a schema with Ref set to
that reference; then construct the resolver with NewResolver(nil, nil,
arrayTopSchema) and assert via
rArr.isOptionalField(fieldpath.MustParse("servers[0].host")) and
rArr.isOptionalField(fieldpath.MustParse("servers[0].timeout")) (and an index
like "servers[2].timeout") to ensure required/optional behavior matches the
inline Items2020 case.

bindings/go/transform/graph/runtime/resolver/resolver_test.go (1)

891-915: Consider adding a test case for array items with Ref.

The current array tests use Items2020 pointing directly to a schema with properties. Consider adding a test case where Items2020 itself contains a Ref (e.g., {Items2020: &jsonschema.Schema{Ref: itemSchema}}) to ensure consistent traversal behavior with the property-ref pattern tested elsewhere.

🤖 Prompt for AI Agents

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

In `@bindings/go/transform/graph/runtime/resolver/resolver_test.go` around lines
891 - 915, Add a new test variant that mirrors the existing array item checks
but sets Items2020 to a schema containing a Ref instead of inline properties:
create itemSchema (same as existing), then create arrayTopSchema where "servers"
has Items2020: &jsonschema.Schema{Ref: itemSchema} and instantiate resolver via
NewResolver(nil, nil, arrayTopSchema); assert using
rArr.isOptionalField(fieldpath.MustParse("servers[0].host")) and
"servers[0].timeout" (and a different index like "servers[2].timeout") to verify
required/optional behavior matches the inline-Items2020 case. Ensure you
reference the same symbols (itemSchema, arrayTopSchema, Items2020, Ref,
NewResolver, isOptionalField, fieldpath.MustParse) so traversal with a Ref is
covered.

bindings/go/transform/graph/runtime/resolver/resolver.go (1)

396-403: Final segment handling assumes map key without validation.

The code at line 402 uses segment.Name unconditionally, but doesn't verify that the final segment is indeed a map key (not an array index). While current usage via resolveField ensures this is always true (since isOptionalField returns false for array indices), the function signature doesn't enforce this constraint.

Consider adding a guard for robustness:

Defensive check

 if i == len(path)-1 {
-    // final segment: always a map key deletion
+    // final segment: only map key deletion is supported
+    if segment.Index != nil {
+        return fmt.Errorf("cannot delete array element at path segment: %v", segment)
+    }
     currentMap, ok := current.(map[string]interface{})
     if !ok {
         return fmt.Errorf("expected map at path segment: %v", segment)
     }
     delete(currentMap, segment.Name)
     return nil
 }

🤖 Prompt for AI Agents

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

In `@bindings/go/transform/graph/runtime/resolver/resolver.go` around lines 396 -
403, The final-segment deletion assumes the segment is a map key and uses
segment.Name unconditionally; add a defensive check before treating it as a map
key (e.g., verify the segment represents a key rather than an array index —
using whatever discriminator the Segment type exposes, such as a boolean like
IsIndex or by checking an empty Name) and return a clear error if it's not
valid; update the block in the resolver function where currentMap is asserted
(the same code path referenced by resolveField and isOptionalField) to validate
the segment kind before calling delete(currentMap, segment.Name).

bindings/go/transform/graph/runtime/resolver/resolver.go (1)

323-360: Add defensive dereferencing for root-level schema refs in isOptionalField.

The current implementation doesn't handle cases where the passed schema is itself a $ref wrapper (particularly in Ref chains). While specSubSchema dereferences one level, if the resulting target itself has a Ref, traversal begins without dereferencing it, causing schemaProperty to fail to find properties. Add the derefSchema helper function and apply it at the root and all intermediate levels as proposed to handle Ref chains defensively.

♻️ Proposed hardening

+func derefSchema(s *jsonschema.Schema) *jsonschema.Schema {
+	for s != nil && s.Ref != nil {
+		s = s.Ref
+	}
+	return s
+}
+
 func (r *Resolver) isOptionalField(path fieldpath.Path) bool {
 	if r.schema == nil || len(path) == 0 {
 		return false
 	}
-	current := r.schema
+	current := derefSchema(r.schema)
 	// Walk the segments and either check a property or the ref schema until we get to the last segment, where we check if it's required or not.
 	for i, segment := range path {
+		current = derefSchema(current)
 		if current == nil {
 			return false
 		}
 		if segment.Index != nil {
 			itemSchema := current.Items2020
 			if itemSchema == nil {
 				return false
 			}
-			if itemSchema.Ref != nil {
-				current = itemSchema.Ref
-			} else {
-				current = itemSchema
-			}
+			current = derefSchema(itemSchema)
 			continue
 		}
 		propSchema := schemaProperty(current, segment.Name)
 		if propSchema == nil {
 			return false
 		}
@@ 
-		if propSchema.Ref != nil {
-			// if we have a ref, we need to resolve it before continuing to the next segment
-			current = propSchema.Ref
-		} else {
-			current = propSchema
-		}
+		current = derefSchema(propSchema)
 	}
 	return false
 }

🤖 Prompt for AI Agents

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

In `@bindings/go/transform/graph/runtime/resolver/resolver.go` around lines 323 -
360, The isOptionalField implementation must defensively dereference $ref
wrapper schemas to avoid failing property lookups when the root or intermediate
target is a Ref; add and use a helper derefSchema (or reuse specSubSchema
behavior) to replace current = propSchema.Ref / itemSchema.Ref / r.schema with
derefSchema(current) at the start and whenever you follow a Ref (including the
root r.schema and after resolving itemSchema.Ref or propSchema.Ref) so
schemaProperty always receives the actual target schema rather than a Ref
wrapper; update isOptionalField to call derefSchema on r.schema initially and
after each Ref resolution before continuing traversal.