Apply nested collection operations when copying mappings

docs/roadmap/remediation.md

@@ -219,7 +219,16 @@ without a replacement value and during append/upsert.
 
 ## 6. Enforce Collection Operations Beneath Newly Added Ancestors — P2
 
-Status: [ ] Open.
+Status: [x] Complete.
+
+Completion: copying a mapping now visits nested collection operations with absent
+lower values, preserving ordinary ancestor removal and whole-record semantics.
+Incoming record-owned policy transfers through the same operation. Regressions
+cover absent/empty/null ancestors, identity and kind failures, empty and valid
+initialization, and later inheritance using an imported item's nested identity.
+Corrected an older test that deferred the required error until a later tier.
+Verified focused merge/collection/inheritance tests, focused ownership race
+coverage, and `./scripts/check.sh`.
 
 Owners: [`mergeRecursiveMappings`](../../internal/engine/recursive_merge.go) and
 [`mergeOverlayMappingsWithCollectionPolicy`](../../internal/engine/overlay.go).

internal/engine/collection_ancestor_test.go

@@ -0,0 +1,106 @@
+package engine
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+	"testing/fstest"
+)
+
+func TestNestedCollectionInitializationIsIndependentOfAncestorPresence(t *testing.T) {
+	t.Parallel()
+	for _, ancestor := range []string{"{}", "group: {}", "group: null"} {
+		for _, test := range []struct {
+			mode, key, value string
+			valid            bool
+		}{
+			{mode: "append", value: "[{id: a}]"},
+			{mode: "upsert", value: "[{id: a}]"},
+			{mode: "append", value: "[]"},
+			{mode: "upsert", value: "[]"},
+			{mode: "append", key: ", key: id", value: "{}"},
+			{mode: "upsert", key: ", key: id", value: "null"},
+			{mode: "append", key: ", key: id", value: "[{id: a}]", valid: true},
+			{mode: "upsert", key: ", key: id", value: "[]", valid: true},
+		} {
+			t.Run(fmt.Sprintf("%s/%s/%s/%s", ancestor, test.mode, test.key, test.value), func(t *testing.T) {
+				main := "configkit:\n  sequences: [{destination: [group, items], mode: " + test.mode + test.key + "}]\ngroup: {items: " + test.value + "}\n"
+				loaded, err := testGroupedLoad(loadRequest[map[string]any]{
+					main:      loadInput{source: Bytes("main", []byte(main))},
+					underlays: []loadGroup{{inputs: []loadInput{{source: Bytes("lower", []byte(ancestor))}}}},
+				})
+				if test.valid {
+					if err != nil || loaded == nil {
+						t.Fatalf("valid initialization failed: %v", err)
+					}
+					return
+				}
+				var loadErr *LoadError
+				if !errors.As(err, &loadErr) || loaded != nil {
+					t.Fatalf("invalid collection operation succeeded: %#v, %v", loaded, err)
+				}
+				diagnostics := loadErr.Diagnostics()
+				if len(diagnostics) != 1 || diagnostics[0].Code != CodeSequenceInvalid || diagnostics[0].Path.String() != "/group/items" {
+					t.Fatalf("operation diagnostics = %#v", diagnostics)
+				}
+			})
+		}
+	}
+}
+
+func TestAddedAncestorTransfersNestedRecordPolicies(t *testing.T) {
+	t.Parallel()
+	type record struct {
+		ID    string           `yaml:"id"`
+		Steps []map[string]any `yaml:"steps"`
+	}
+	type config struct {
+		Group struct {
+			Items []record `yaml:"items"`
+		} `yaml:"group"`
+	}
+	files := fstest.MapFS{
+		"main.yml": {Data: []byte(`configkit:
+  sequences: [{destination: [group, items], key: id}]
+  imports: [{path: base.yml, destination: [group, items], placement: sequence-item}]
+group:
+  items: [{id: child, extends: {id: base}, steps: [{name: child}]}]
+`)},
+		"base.yml": {Data: []byte("configkit:\n  sequences: [{destination: [steps], key: name}]\nid: base\nsteps: [{name: base}]\n")},
+	}
+	loaded, err := testGroupedLoad(loadRequest[config]{
+		main:      loadInput{source: NewFilesystem("catalog", files).File("main.yml"), imports: true},
+		underlays: []loadGroup{{inputs: []loadInput{{source: Bytes("lower", []byte("{}"))}}}},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, item := range loaded.Config.Group.Items {
+		if item.ID == "child" {
+			if len(item.Steps) != 2 || item.Steps[0]["name"] != "base" || item.Steps[1]["name"] != "child" {
+				t.Fatalf("record-local identity was lost when adding its ancestor: %#v", item.Steps)
+			}
+			return
+		}
+	}
+	t.Fatal("missing child record")
+}
+
+func TestInheritanceChecksCollectionOperationUnderAddedAncestor(t *testing.T) {
+	t.Parallel()
+	_, err := testLoad[map[string]any](Bytes("inheritance", []byte(`configkit:
+  sequences: [{destination: [child, group, items], mode: append}]
+base: {}
+child:
+  extends: base
+  group: {items: [{id: a}]}
+`)))
+	var loadErr *LoadError
+	if !errors.As(err, &loadErr) {
+		t.Fatalf("missing nested operation error: %v", err)
+	}
+	diagnostics := loadErr.Diagnostics()
+	if len(diagnostics) != 1 || diagnostics[0].Code != CodeSequenceInvalid || diagnostics[0].Path.String() != "/child/group/items" {
+		t.Fatalf("nested inheritance operation diagnostics = %#v", diagnostics)
+	}
+}

internal/engine/inheritance_schedule.go

@@ -267,11 +267,7 @@ func applyInheritanceScopeTemplates(adapter inheritanceScopeAdapter, order []int
 					if !active {
 						return nil, false
 					}
-					lowerPath, within := relativeHistoryPath(node.path, childPath)
+					lowerValue := existing
-					if !within {
-						return nil, false
-					}
-					lowerValue, _ := documentAtPath(parent, lowerPath)
 					combined, problems := combineSequence(lowerValue, incoming, resolution, sequenceCombinationContext{destination: childPath})
 					if len(problems) != 0 {
 						combinationDiagnostics = append(combinationDiagnostics, problems...)

internal/engine/item_policy_test.go

@@ -298,7 +298,7 @@ func TestAncestorReplacementRequiresIdentityWhenRetainedModeReturns(t *testing.T
 			collectionPolicies: scope, origins: make(map[string]Origin),
 		})
 	}
-	reintroduceAncestor := func(t *testing.T, assembly *assemblyAccumulator) {
+	removeAncestor := func(t *testing.T, assembly *assemblyAccumulator) {
 		t.Helper()
 		if diagnostics := assembly.applyOverlayWithCollectionPolicy(
 			parseItemPolicyDocument(t, "remove", "catalog: null\n"), nil, collectionPolicyScope{}, nil,
@@ -306,23 +306,17 @@ func TestAncestorReplacementRequiresIdentityWhenRetainedModeReturns(t *testing.T
 		); len(diagnostics) != 0 {
 			t.Fatalf("ancestor removal diagnostics = %#v", diagnostics)
 		}
-		if diagnostics := assembly.applyOverlayWithCollectionPolicy(
-			parseItemPolicyDocument(t, "reintroduction", "catalog:\n  prompts: [{id: restored, name: restored}]\n"), nil,
-			collectionPolicyScope{}, nil, LayerBase, sequencePolicy{}, nil,
-		); len(diagnostics) != 0 {
-			t.Fatalf("ancestor reintroduction diagnostics = %#v", diagnostics)
-		}
 	}
 
 	t.Run("retained append mode requires a renewed identity", func(t *testing.T) {
 		assembly := newAssembly(t)
-		reintroduceAncestor(t, assembly)
+		removeAncestor(t, assembly)
 		policy, ok := assembly.current.collectionPolicies.at(path)
 		if !ok || policy.identityPresent || !policy.explicitModePresent || policy.explicitMode != sequenceModeAppend {
-			t.Fatalf("reintroduced policy = %#v, %v", policy, ok)
+			t.Fatalf("retired collection policy = %#v, %v", policy, ok)
 		}
 		diagnostics := assembly.applyOverlayWithCollectionPolicy(
-			parseItemPolicyDocument(t, "next", "catalog:\n  prompts: [{value: next}]\n"), nil,
+			parseItemPolicyDocument(t, "reintroduction", "catalog:\n  prompts: [{id: restored, name: restored}]\n"), nil,
 			collectionPolicyScope{}, nil, LayerBase, sequencePolicy{}, nil,
 		)
 		if len(diagnostics) != 1 || diagnostics[0].Code != CodeSequenceInvalid {
@@ -332,7 +326,7 @@ func TestAncestorReplacementRequiresIdentityWhenRetainedModeReturns(t *testing.T
 
 	t.Run("a renewed key restores the retained append mode", func(t *testing.T) {
 		assembly := newAssembly(t)
-		reintroduceAncestor(t, assembly)
+		removeAncestor(t, assembly)
 		declaration := sequenceDeclarationSpec{
 			destination: path, identityFields: []string{"name"}, identityPresent: true,
 		}

internal/engine/recursive_merge.go

@@ -22,21 +22,28 @@ func mergeRecursiveMappings(target, incoming *document.Node, current Path, index
 			if hooks.merge != nil {
 				if merged, handled := hooks.merge(path, nil, entry.Value); handled {
 					cloned := entry
-					cloned.Value = document.Clone(merged)
+					cloned.Value = cloneMergeSubtree(merged, path, hooks.merge)
 					index.append(target, cloned)
 					continue
 				}
 			}
-			cloned := entry
-			cloned.Value = document.Clone(entry.Value)
-			index.append(target, cloned)
 			if hooks.added != nil {
 				hooks.added(path, entry.Value)
 			}
+			cloned := entry
+			cloned.Value = cloneMergeSubtree(entry.Value, path, hooks.merge)
+			index.append(target, cloned)
 			continue
 		}
 
 		existing := target.Mapping[position].Value
+		if hooks.merge != nil {
+			if merged, handled := hooks.merge(path, existing, entry.Value); handled {
+				target.Mapping[position] = entry
+				target.Mapping[position].Value = cloneMergeSubtree(merged, path, hooks.merge)
+				continue
+			}
+		}
 		if existing.Kind == document.Mapping && entry.Value.Kind == document.Mapping {
 			if hooks.combined != nil {
 				hooks.combined(path, entry.Value)
@@ -44,18 +51,33 @@ func mergeRecursiveMappings(target, incoming *document.Node, current Path, index
 			mergeRecursiveMappings(existing, entry.Value, path, index, hooks)
 			continue
 		}
-		if hooks.merge != nil {
-			if merged, handled := hooks.merge(path, existing, entry.Value); handled {
-				target.Mapping[position] = entry
-				target.Mapping[position].Value = document.Clone(merged)
-				continue
-			}
-		}
 
 		if hooks.replaced != nil {
 			hooks.replaced(path, existing, entry.Value)
 		}
 		target.Mapping[position] = entry
-		target.Mapping[position].Value = document.Clone(entry.Value)
+		target.Mapping[position].Value = cloneMergeSubtree(entry.Value, path, hooks.merge)
 	}
 }
+
+// A copied mapping can contain collection operations even though its ancestor
+// has no retained mapping. Visit those operations with absent lower values.
+// Ordinary origin/replacement hooks already accounted for the whole subtree;
+// do not replay them for every descendant. Records inside sequences remain whole.
+func cloneMergeSubtree(incoming *document.Node, path Path, merge func(Path, *document.Node, *document.Node) (*document.Node, bool)) *document.Node {
+	if incoming == nil || incoming.Kind != document.Mapping || merge == nil {
+		return document.Clone(incoming)
+	}
+	cloned := *incoming
+	cloned.Mapping = make([]document.Entry, len(incoming.Mapping))
+	for index, entry := range incoming.Mapping {
+		childPath := path.Key(entry.Key)
+		value := entry.Value
+		if merged, handled := merge(childPath, nil, value); handled {
+			value = merged
+		}
+		entry.Value = cloneMergeSubtree(value, childPath, merge)
+		cloned.Mapping[index] = entry
+	}
+	return &cloned
+}