diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 949225a..a299410 100644 --- a/docs/roadmap/remediation.md +++ b/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; diff --git a/internal/engine/collection_policy.go b/internal/engine/collection_policy.go index b65bc3c..9ceaacd 100644 --- a/internal/engine/collection_policy.go +++ b/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 diff --git a/internal/engine/collection_policy_state.go b/internal/engine/collection_policy_state.go index e8ff9e6..525044f 100644 --- a/internal/engine/collection_policy_state.go +++ b/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 - entry.policy.identityPresent = false - entry.policy.identityLocation = nil - entry.policy.recordOrder = nil - entry.policy.recordPolicies = nil + clearCollectionIdentity(&entry.policy) scope.collections[key] = entry } } diff --git a/internal/engine/collection_retirement.go b/internal/engine/collection_retirement.go new file mode 100644 index 0000000..a54cf40 --- /dev/null +++ b/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 +} diff --git a/internal/engine/collection_retirement_test.go b/internal/engine/collection_retirement_test.go new file mode 100644 index 0000000..745d202 --- /dev/null +++ b/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) + } + } +} diff --git a/internal/engine/compose.go b/internal/engine/compose.go index d759a12..3db2435 100644 --- a/internal/engine/compose.go +++ b/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) diff --git a/internal/engine/inheritance_collection.go b/internal/engine/inheritance_collection.go index 3b327e2..3c15da5 100644 --- a/internal/engine/inheritance_collection.go +++ b/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 } diff --git a/internal/engine/inheritance_schedule.go b/internal/engine/inheritance_schedule.go index 1f5d8c0..b0b4a4d 100644 --- a/internal/engine/inheritance_schedule.go +++ b/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(