diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index c363452..b0bc9f3 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -818,7 +818,14 @@ collection-size limit. ## 20. Remove Full Metadata Scans Per CLI Replacement Leaf — P2 -Status: [ ] Open. +Status: [x] Complete. + +Completion: presence and origin removal now visits only the known replaced +pre-CLI subtree. Partial mappings still retain omitted fields and their metadata. +Added broad-mapping coverage with escaped keys, explicit zero/false values, +empty-list replacement, null pointers, retained siblings, and deleted descendant +metadata. Existing nested CLI and typed-ownership regressions and +`./scripts/check.sh` pass. Owners: `updateOverridePresence`, `deletePresencePathSubtree`, and `updateOverrideOrigins` in diff --git a/internal/engine/override_decode.go b/internal/engine/override_decode.go index 43874d3..6b85850 100644 --- a/internal/engine/override_decode.go +++ b/internal/engine/override_decode.go @@ -2,7 +2,6 @@ package engine import ( "reflect" - "strings" "gitea.maximumdirect.net/eric/ConfigKit/internal/document" ) @@ -171,17 +170,16 @@ func updateOverridePresence(presence map[string]inputPresence, existing, incomin return } - deletePresencePathSubtree(presence, path) + deletePresencePathSubtree(presence, existing, path) collectPresence(incoming, path, presence) } -func deletePresencePathSubtree(presence map[string]inputPresence, path Path) { - pointer := path.String() - for candidate := range presence { - if candidate == pointer || strings.HasPrefix(candidate, pointer+"/") { - delete(presence, candidate) - } - } +func deletePresencePathSubtree(presence map[string]inputPresence, existing *document.Node, path Path) { + // The typed pre-CLI tree contains every effective descendant. Visit only + // the replaced subtree rather than searching unrelated metadata entries. + walkDocument(existing, path, func(path Path, _ *document.Node) { + delete(presence, path.String()) + }) } func updateOverrideOrigins(origins map[string]Origin, existing, incoming *document.Node, path Path, location document.Location) { @@ -200,12 +198,9 @@ func updateOverrideOrigins(origins map[string]Origin, existing, incoming *docume return } - pointer := path.String() - for candidate := range origins { - if candidate == pointer || strings.HasPrefix(candidate, pointer+"/") { - delete(origins, candidate) - } - } + walkDocument(existing, path, func(path Path, _ *document.Node) { + delete(origins, path.String()) + }) collectOverlayOrigins(origins, incoming, path, LayerCLI) } diff --git a/internal/engine/override_metadata_test.go b/internal/engine/override_metadata_test.go new file mode 100644 index 0000000..51d4902 --- /dev/null +++ b/internal/engine/override_metadata_test.go @@ -0,0 +1,82 @@ +package engine + +import ( + "fmt" + "reflect" + "strings" + "testing" +) + +func TestBroadPartialCLIOverrideRetainsUnchangedMetadata(t *testing.T) { + type child struct { + Name string `yaml:"name"` + } + type record struct { + Count int `yaml:"count"` + Zero int `yaml:"zero"` + Enabled bool `yaml:"enabled"` + Items []child `yaml:"items"` + Maybe *child `yaml:"maybe"` + } + type config struct { + Records map[string]record `yaml:"records"` + } + var body strings.Builder + body.WriteString("records:\n") + updates := make(map[string]any) + for index := range 96 { + name := fmt.Sprintf("key/%d~", index) + fmt.Fprintf(&body, " %s: {count: 9, zero: 0, enabled: false, items: [{name: old}], maybe: {name: old}}\n", name) + if index%2 == 0 { + updates[name] = map[string]any{"count": 0, "items": []any{}, "maybe": nil} + } + } + input := parseDocumentForDecode(t, body.String()) + schema := mustSchemaFor[config](t) + decoded, issues := decodeConfiguration(schema, reflect.ValueOf(config{}), input) + if len(issues) != 0 { + t.Fatal(issues) + } + batch, diagnostics := preparePostStructuralOverrides(overridePreparationInput{root: input}, nil, + []Override{{Path: Keys("records"), Value: updates, Flag: "--records"}}, nil, defaultImportLimits()) + if len(diagnostics) != 0 { + t.Fatal(diagnostics) + } + result, issues := applyDecodedOverrides(schema, decoded, batch, decodeContext{}) + if len(issues) != 0 { + t.Fatal(issues) + } + for index := range 96 { + name := fmt.Sprintf("key/%d~", index) + path := Keys("records", name) + value := result.value.Interface().(config).Records[name] + for _, key := range []string{"zero", "enabled"} { + pointer := path.Key(key).String() + if _, explicit := result.presence[pointer]; !explicit || result.origins[pointer].Layer != LayerBase { + t.Fatalf("retained explicit field lost metadata at %s", pointer) + } + } + if index%2 != 0 { + if value.Count != 9 || len(value.Items) != 1 || value.Maybe == nil || result.origins[path.Key("count").String()].Layer != LayerBase { + t.Fatalf("omitted record changed at %s", path) + } + continue + } + if value.Count != 0 || len(value.Items) != 0 || value.Maybe != nil { + t.Fatalf("CLI replacement failed at %s: %#v", path, value) + } + for _, removed := range []Path{path.Key("items").Index(0), path.Key("items").Index(0).Key("name"), path.Key("maybe").Key("name")} { + _, present := result.presence[removed.String()] + _, origin := result.origins[removed.String()] + if present || origin { + t.Fatalf("removed descendant kept metadata at %s", removed) + } + } + for _, key := range []string{"count", "items", "maybe"} { + pointer := path.Key(key).String() + if presence, ok := result.presence[pointer]; !ok || presence.location.SourceID != batch.overrides[0].source.ID || result.origins[pointer].Layer != LayerCLI { + t.Fatalf("CLI field lost metadata at %s", pointer) + } + } + } +}