Retire obsolete collection identity before replacement agreement

docs/roadmap/remediation.md

@@ -435,7 +435,16 @@ Validation:
 
 ## 12. Complete Identity Retirement Across Replacement Shapes — P2
 
-Status: [ ] Open.
+Status: [x] Complete.
+
+Completion: source composition and inheritance now prepare eligible lower policy
+from actual replacement paths before agreement. Removed identity and record state
+cannot constrain a new pending key or reappear in later inheritance. Shared mode
+selection preserves explicit-mode persistence and authoritative registrations.
+Regressions cover null/scalar/mapping/list replacement, ancestor removal with a
+new key, reintroduction, and retained/application-constrained conflicts. Verified
+focused retirement/inheritance tests, engine ownership race coverage, and
+`./scripts/check.sh`.
 
 Owners: [`resolveStrictCollectionPolicies`](../../internal/engine/compose.go),
 especially its `sequencePresent` reset condition;

internal/engine/collection_policy.go

@@ -148,21 +148,29 @@ func resolveCollectionPolicy(
 		result.policy.explicitMode = declaredMode
 		result.policy.explicitModePresent = true
 		result.policy.modeLocation = cloneLocation(declaredModeLocation)
-		result.operation = declaredMode
 	} else if lower != nil && lower.explicitModePresent {
 		result.policy.explicitMode = lower.explicitMode
 		result.policy.explicitModePresent = true
 		result.policy.modeLocation = cloneLocation(lower.modeLocation)
-		result.operation = lower.explicitMode
-	} else if len(result.effectiveIdentityFields) != 0 {
-		result.operation = sequenceModeUpsert
-	} else {
-		result.operation = sequenceModeReplace
 	}
+	result.operation = collectionOperationMode(declaredMode, result.policy.explicitMode, len(result.effectiveIdentityFields) != 0)
 
 	return result, nil
 }
 
+func collectionOperationMode(current, retained sequenceMode, identified bool) sequenceMode {
+	if current != 0 {
+		return current
+	}
+	if retained != 0 {
+		return retained
+	}
+	if identified {
+		return sequenceModeUpsert
+	}
+	return sequenceModeReplace
+}
+
 func cloneSequenceDeclarations(declarations []sequenceDeclarationSpec) []sequenceDeclarationSpec {
 	if declarations == nil {
 		return nil

internal/engine/collection_policy_state.go

@@ -168,11 +168,7 @@ func (scope *collectionPolicyScope) retireCollectionPoliciesAtOrBelow(ancestor P
 		if !pathHasPrefix(entry.destination, ancestor) {
 			continue
 		}
-		entry.policy.identityFields = nil
+		clearCollectionIdentity(&entry.policy)
-		entry.policy.identityPresent = false
-		entry.policy.identityLocation = nil
-		entry.policy.recordOrder = nil
-		entry.policy.recordPolicies = nil
 		scope.collections[key] = entry
 	}
 }

internal/engine/collection_retirement.go

@@ -0,0 +1,81 @@
+package engine
+
+import "gitea.maximumdirect.net/eric/ConfigKit/internal/document"
+
+// A true entry replaces the collection itself as well as its descendants;
+// false records ordinary ancestor replacement without selecting a collection
+// operation at that exact path.
+type collectionReplacementPaths map[string]bool
+
+func (paths collectionReplacementPaths) contains(path Path, includeExact bool) bool {
+	for length := 0; length <= len(path.segments); length++ {
+		prefix := Path{segments: path.segments[:length]}
+		inclusive, found := paths[prefix.String()]
+		if found && (length < len(path.segments) || includeExact && inclusive) {
+			return true
+		}
+	}
+	return false
+}
+
+// lowerCollectionPoliciesForClaims makes replacement eligibility independent of
+// declaration order. It removes only old YAML identity/record state; new pending
+// declarations and application constraints are reconciled afterward.
+func lowerCollectionPoliciesForClaims(
+	lower collectionPolicyScope,
+	claims []compositionClaim,
+	declarations map[string][]sequenceDeclarationSpec,
+	application sequencePolicy,
+) (collectionPolicyScope, collectionReplacementPaths) {
+	byPath := make(map[string]compositionClaim, len(claims))
+	rootPath := ""
+	if len(claims) != 0 {
+		rootPath = claims[0].path.String()
+	}
+	for _, claim := range claims {
+		byPath[claim.path.String()] = claim
+	}
+	replaced := make(collectionReplacementPaths)
+	for key, claim := range byPath {
+		old, hasOld := lower.collections[key]
+		current := declarations[key]
+		if claim.kind == document.Mapping && (key == rootPath || !hasOld && len(current) == 0) {
+			continue // Ordinary mapping combination preserves descendants.
+		}
+		mode := sequenceMode(0)
+		identity := old.policy.identityPresent
+		for _, declaration := range current {
+			identity = identity || declaration.identityPresent
+			if mode == 0 && declaration.modePresent {
+				mode = declaration.mode
+			}
+		}
+		retainedMode := sequenceMode(0)
+		if old.policy.explicitModePresent {
+			retainedMode = old.policy.explicitMode
+		}
+		_, registered := application.identityFieldsAt(claim.path)
+		mode = collectionOperationMode(mode, retainedMode, identity || registered)
+		if mode == sequenceModeReplace {
+			replaced[key] = true
+		} else if claim.kind != document.Mapping && claim.kind != document.Sequence {
+			replaced[key] = false
+		}
+	}
+	result := cloneCollectionPolicyScope(lower)
+	for key, entry := range result.collections {
+		if replaced.contains(entry.destination, true) {
+			clearCollectionIdentity(&entry.policy)
+			result.collections[key] = entry
+		}
+	}
+	return result, replaced
+}
+
+func clearCollectionIdentity(policy *collectionPolicy) {
+	policy.identityFields = nil
+	policy.identityPresent = false
+	policy.identityLocation = nil
+	policy.recordOrder = nil
+	policy.recordPolicies = nil
+}

internal/engine/collection_retirement_test.go

@@ -0,0 +1,111 @@
+package engine
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestSourceReplacementRetiresIdentityForEveryValueKind(t *testing.T) {
+	t.Parallel()
+	for _, value := range []string{"null", "42", "{}", "[{name: new}]"} {
+		t.Run(value, func(t *testing.T) {
+			lower := "configkit:\n  sequences: [{destination: [items], key: id}]\nitems: [{id: old}]\n"
+			main := "configkit:\n  sequences: [{destination: [items], key: name, mode: replace}]\nitems: " + value + "\n"
+			loaded, err := testGroupedLoad(loadRequest[map[string]any]{
+				main:      loadInput{source: Bytes("main", []byte(main))},
+				underlays: []loadGroup{{inputs: []loadInput{{source: Bytes("lower", []byte(lower))}}}},
+			})
+			if err != nil || loaded == nil {
+				t.Fatalf("replacement retained obsolete identity: %v", err)
+			}
+		})
+	}
+}
+
+func TestAncestorRemovalCanEstablishPendingReplacementIdentity(t *testing.T) {
+	t.Parallel()
+	for _, inheritance := range []bool{false, true} {
+		name := "tiers"
+		if inheritance {
+			name = "inheritance"
+		}
+		t.Run(name, func(t *testing.T) {
+			var loaded *testResult[map[string]any]
+			var err error
+			if inheritance {
+				loaded, err = testLoad[map[string]any](Bytes("source", []byte(`configkit:
+  sequences:
+    - {destination: [base, group, items], key: id}
+    - {destination: [child, group, items], key: name}
+base: {group: {items: [{id: old}]}}
+child:
+  extends: base
+  group: null
+grandchild:
+  extends: child
+  group: {items: [{name: new}]}
+`)))
+			} else {
+				loaded, err = testGroupedLoad(loadRequest[map[string]any]{
+					main: loadInput{source: Bytes("main", []byte("group: {items: [{name: new}]}\n"))},
+					underlays: []loadGroup{
+						{inputs: []loadInput{{source: Bytes("base", []byte("configkit:\n  sequences: [{destination: [group, items], key: id}]\ngroup: {items: [{id: old}]}\n"))}}},
+						{inputs: []loadInput{{source: Bytes("remove", []byte("configkit:\n  sequences: [{destination: [group, items], key: name}]\ngroup: null\n"))}}},
+					},
+				})
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+			value := loaded.Config
+			if inheritance {
+				value = value["grandchild"].(map[string]any)
+			}
+			items := value["group"].(map[string]any)["items"].([]any)
+			if len(items) != 1 || items[0].(map[string]any)["name"] != "new" {
+				t.Fatalf("reintroduced collection = %#v", items)
+			}
+		})
+	}
+}
+
+func TestInheritanceAncestorRetirementDoesNotResurrectOldIdentity(t *testing.T) {
+	t.Parallel()
+	_, err := testLoad[map[string]any](Bytes("source", []byte(`configkit:
+  sequences: [{destination: [base, group, items], key: id}]
+base: {group: {items: [{id: old}]}}
+child:
+  extends: base
+  group: null
+grandchild:
+  extends: child
+  group: {items: [{name: new}]}
+`)))
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestReplacementRetirementPreservesApplicationConstraintsAndRetainedKeys(t *testing.T) {
+	t.Parallel()
+	for _, withValue := range []bool{false, true} {
+		main := "configkit:\n  sequences: [{destination: [items], key: name, mode: replace}]\n"
+		if withValue {
+			main += "items: null\n"
+		}
+		definition := Definition[map[string]any]{}
+		if withValue {
+			definition.sequenceIdentityRegistrations = []sequenceIdentityRegistration{{
+				selector: sequenceIdentitySelector{literalSequenceSelector("items")}, identityFields: []string{"id"},
+			}}
+		}
+		_, err := testGroupedLoad(loadRequest[map[string]any]{
+			main: loadInput{source: Bytes("main", []byte(main))}, definition: definition,
+			underlays: []loadGroup{{inputs: []loadInput{{source: Bytes("lower", []byte("configkit:\n  sequences: [{destination: [items], key: id}]\nitems: [{id: old}]\n"))}}}},
+		})
+		var loadErr *LoadError
+		if !errors.As(err, &loadErr) || len(loadErr.Diagnostics()) == 0 || loadErr.Diagnostics()[0].Code != CodeSequenceConflict {
+			t.Fatalf("retirement bypassed identity agreement: %v", err)
+		}
+	}
+}

internal/engine/compose.go

@@ -293,14 +293,16 @@ func resolveStrictCollectionPolicies(
 ) (map[string][]string, collectionPolicyScope, []Diagnostic) {
 	paths := make([]Path, 0, len(claims)+len(declarations))
 	seen := make(map[string]bool, len(claims)+len(declarations))
-	sequencePresent := make(map[string]bool)
+	valuePresent := make(map[string]bool)
 	declarationsByPath := make(map[string][]sequenceDeclarationSpec, len(declarations))
 	for _, claim := range claims {
+		if len(lower.collections) != 0 {
+			valuePresent[claim.path.String()] = true
+		}
 		if claim.kind != document.Sequence {
 			continue
 		}
 		key := claim.path.String()
-		sequencePresent[key] = true
 		if !seen[key] {
 			seen[key] = true
 			paths = append(paths, claim.path.clone())
@@ -316,7 +318,10 @@ func resolveStrictCollectionPolicies(
 	}
 
 	identityFieldsByPath := make(map[string][]string)
-	effectiveLower := cloneCollectionPolicyScope(lower)
+	effectiveLower := lower
+	if len(lower.collections) != 0 {
+		effectiveLower, _ = lowerCollectionPoliciesForClaims(lower, claims, declarationsByPath, applicationPolicy)
+	}
 	collectionPolicies := cloneCollectionPolicyScope(effectiveLower)
 	var diagnostics []Diagnostic
 	for _, path := range paths {
@@ -331,28 +336,6 @@ func resolveStrictCollectionPolicies(
 		}
 		applicationIdentity, _ := applicationPolicy.identityFieldsAt(path)
 
-		// A replacing value drops the old YAML identity before this tier's
-		// peers are checked. The explicit operation itself remains applicable.
-		selectedMode := sequenceMode(0)
-		for _, declaration := range currentDeclarations {
-			if declaration.modePresent {
-				selectedMode = declaration.mode
-				break
-			}
-		}
-		if selectedMode == 0 && lowerPolicy != nil && lowerPolicy.explicitModePresent {
-			selectedMode = lowerPolicy.explicitMode
-		}
-		if sequencePresent[key] && selectedMode == sequenceModeReplace && lowerPolicy != nil {
-			lowerPolicy.identityFields = nil
-			lowerPolicy.identityPresent = false
-			lowerPolicy.identityLocation = nil
-			lowerPolicy.recordOrder = nil
-			lowerPolicy.recordPolicies = nil
-			effectiveLower.retireCollectionPoliciesAtOrBelow(path)
-			collectionPolicies.retireCollectionPoliciesAtOrBelow(path)
-		}
-
 		resolution, found := resolveCollectionPolicy(path, currentDeclarations, applicationIdentity, lowerPolicy)
 		if len(found) != 0 {
 			diagnostics = append(diagnostics, found...)
@@ -363,7 +346,7 @@ func resolveStrictCollectionPolicies(
 		}
 
 		policyForPath := resolution.policy
-		if hasLower && !sequencePresent[key] {
+		if hasLower && !valuePresent[key] {
 			// A declaration without a new value changes policy metadata but
 			// leaves retained record ownership untouched.
 			policyForPath = cloneCollectionPolicy(lowerEntry.policy)

internal/engine/inheritance_collection.go

@@ -93,6 +93,7 @@ func mergeInheritanceCollectionPolicies(
 	parentRootPath Path,
 	parentValue, childValue *document.Node,
 	applicationPolicy sequencePolicy,
+	replaced collectionReplacementPaths,
 ) (collectionPolicyScope, []Diagnostic) {
 	result := cloneCollectionPolicyScopeForInheritance(parent)
 	paths := make(map[string]Path, len(parent.collections)+len(child.collections))
@@ -107,7 +108,9 @@ func mergeInheritanceCollectionPolicies(
 		relative, inside := relativeHistoryPath(parentRootPath, path)
 		var retained, incoming *document.Node
 		if inside {
-			retained, _ = documentAtPath(parentValue, relative)
+			if !replaced.contains(path, false) {
+				retained, _ = documentAtPath(parentValue, relative)
+			}
 			incoming, _ = documentAtPath(childValue, relative)
 		}
 		resolution, lower, _, active, issues := inheritanceCollectionResolution(path, parent, child, applicationPolicy, incoming)
@@ -190,13 +193,11 @@ func applyInheritanceSequencePolicy(
 	combined sequenceCombinationResult,
 	resolution collectionPolicyResolution,
 	lowerScope, higherScope collectionPolicyScope,
-	result *collectionPolicyScope,
 ) collectionPolicy {
 	policy := cloneCollectionPolicyForInheritance(resolution.policy)
 	policy.recordOrder = nil
 	policy.recordPolicies = nil
 	if combined.value == nil || combined.value.Kind != document.Sequence {
-		result.retireCollectionPoliciesAtOrBelow(path)
 		return policy
 	}
 

internal/engine/inheritance_schedule.go

@@ -220,9 +220,19 @@ func applyInheritanceScopeTemplates(adapter inheritanceScopeAdapter, order []int
 			effectivePolicies := append(cloneInheritancePolicyTemplates(parentPolicies), cloneCollectionPolicyScopeForInheritance(templatePolicies))
 			parentPolicyScope := projectInheritanceCollectionPolicies(resolvedPolicies[node.parent], node.path)
 			childPolicyScope := projectCurrentInheritanceCollectionPolicies(templatePolicies, node.path)
+			var replaced collectionReplacementPaths
+			if len(parentPolicyScope.collections) != 0 || len(childPolicyScope.collections) != 0 {
+				claims := compositionClaims{byPath: make(map[string][]compositionClaim)}
+				claims.append(payload, node.path, 0)
+				declarations := make(map[string][]sequenceDeclarationSpec, len(childPolicyScope.collections))
+				for key, entry := range childPolicyScope.collections {
+					declarations[key] = inheritanceDeclarationsAt(entry.destination, entry.policy)
+				}
+				parentPolicyScope, replaced = lowerCollectionPoliciesForClaims(parentPolicyScope, claims.ordered, declarations, adapter.applicationPolicy())
+			}
 			merged := inheritanceParentPayload(parent, node.identityFields)
 			mergedPolicyScope, policyDiagnostics := mergeInheritanceCollectionPolicies(
-				parentPolicyScope, childPolicyScope, node.path, parent, payload, adapter.applicationPolicy(),
+				parentPolicyScope, childPolicyScope, node.path, parent, payload, adapter.applicationPolicy(), replaced,
 			)
 			if len(policyDiagnostics) != 0 {
 				return nil, policyDiagnostics
@@ -274,7 +284,7 @@ func applyInheritanceScopeTemplates(adapter inheritanceScopeAdapter, order []int
 						return existing, false
 					}
 					policy := applyInheritanceSequencePolicy(
-						childPath, combined, resolution, parentPolicyScope, childPolicyScope, &mergedPolicyScope,
+						childPath, combined, resolution, parentPolicyScope, childPolicyScope,
 					)
 					mergedPolicyScope.set(childPath, policy)
 					applyInheritanceSequenceOrigins(