From 83c59ea7d34f979bb6e57e0b4d1829159e935b94 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 30 Sep 2026 00:04:02 +0000 Subject: [PATCH] Fix nested inheritance history and budget accounting --- docs/roadmap/remediation.md | 27 +++++- internal/engine/inheritance_bench_test.go | 22 +++++ internal/engine/inheritance_history_test.go | 92 +++++++++++++++++++++ internal/engine/inheritance_walk.go | 32 +++++-- internal/engine/inheritance_walk_test.go | 23 ++++++ 5 files changed, 187 insertions(+), 9 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 3d7ca98..12dd5e2 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -3,8 +3,31 @@ Reviewed implementation commits `78d4b59` through `2ca1f80` against the [feature roadmap](named-inheritance.md), [implementation plan](implementation.md), and the user's agreed sibling lookup, enclosing-first expansion, presence, and -provenance decisions. Both findings below require remediation. No implementation -fixes are included in this review. +provenance decisions. Both findings below are now resolved. The original review +evidence is retained below; source line references identify the reviewed baseline. + +## Resolution — 2026-09-30 + +- The destination walker now detaches each mapping/sequence container before + replacing its child pointers. Retained enclosing templates remain immutable; + untouched subtrees remain shared, with existing retained-history accounting. + Regressions check value introduction, inherited control arrival/consumption, + container explanations, a three-generation chain, and sequence-contained scopes. + The nested-history regression failed before the fix and passes afterward. +- Budget accounting now maintains one logical document total and updates it from + old/new scope counts. Each scope receives the budget remaining after other + branches are reserved. Early expansion checks and final node/depth validation + remain in place. Boundary tests include independent scopes inside sequences, + unchanged sibling data, and consumed null directives. +- The maintained `BenchmarkInheritanceIndependentScopes` covers 256, 512, and + 1,024 independent scopes. Allocations measured approximately 8.95, 17.87, and + 36.12 MB/op respectively: roughly proportional to scope count. This benchmark + has a larger per-scope payload than the original review probe, so absolute + values are not a before/after comparison. No timing threshold was introduced. +- Validation after the fixes: `./scripts/check.sh`, `go test -race ./...`, + the new independent-scope benchmark, and the existing expansion/inspected-load + benchmarks passed on Linux. Existing redaction, ownership, and history-budget + tests remain passing; no public contract or limit was changed. ## 1. Preserve immutable enclosing snapshots during nested resolution diff --git a/internal/engine/inheritance_bench_test.go b/internal/engine/inheritance_bench_test.go index 09a5d4c..25dec5a 100644 --- a/internal/engine/inheritance_bench_test.go +++ b/internal/engine/inheritance_bench_test.go @@ -58,6 +58,28 @@ func BenchmarkInheritanceInspectedLoad(b *testing.B) { } } +// Each scope has constant-sized output and evidence. This distinguishes budget +// accounting overhead from legitimate growth in expanded parent payloads. +func BenchmarkInheritanceIndependentScopes(b *testing.B) { + for _, size := range []int{256, 512, 1024} { + b.Run(strconv.Itoa(size), func(b *testing.B) { + input := &document.Node{Kind: document.Mapping} + for index := 0; index < size; index++ { + input.Mapping = append(input.Mapping, document.Entry{ + Key: strconv.Itoa(index), Value: inheritanceBenchmarkInput("wide", 1), + }) + } + b.ReportAllocs() + b.ResetTimer() + for range b.N { + if _, diagnostics := expandInheritanceTree(input, defaultProjectionLimits()); len(diagnostics) != 0 { + b.Fatal(diagnostics) + } + } + }) + } +} + func inheritanceBenchmarkInput(shape string, size int) *document.Node { value := func(text string) *document.Node { return &document.Node{Kind: document.String, Text: text} } mapping := func(entries ...document.Entry) *document.Node { diff --git a/internal/engine/inheritance_history_test.go b/internal/engine/inheritance_history_test.go index 5727d09..cadf98b 100644 --- a/internal/engine/inheritance_history_test.go +++ b/internal/engine/inheritance_history_test.go @@ -8,6 +8,98 @@ import ( "gitea.maximumdirect.net/eric/ConfigKit/internal/document" ) +func TestNestedInheritanceHistoryRetainsUnresolvedTemplates(t *testing.T) { + input := `presets: + base: + settings: + primary: {width: 10} + secondary: {extends: primary} + custom: + extends: base + settings: + primary: {width: 20} + grandchild: + extends: custom + settings: + primary: {width: 30} +` + loaded, err := Load[map[string]any](Bytes("nested", []byte(input)), testSecretBinding()) + if err != nil { + t.Fatal(err) + } + for name, width := range map[string]string{"custom": "20", "grandchild": "30"} { + t.Run(name, func(t *testing.T) { + root := Keys("presets", name) + value, _ := loaded.Inspection.Explain(root.Key("settings").Key("secondary").Key("width")) + introduced := false + for _, change := range value.History { + if change.Origin.Operation == ChangeInheritance && !change.Before.Present && change.After.YAML == width+"\n" { + introduced = true + } + } + if !introduced || value.Effective.YAML != width+"\n" { + t.Fatalf("missing nested introduction: %#v", value) + } + controlPath := root.Key("settings").Key("secondary").Key("extends") + control, _ := loaded.Inspection.Explain(controlPath) + arrived, consumed := false, false + for _, change := range control.History { + arrived = arrived || change.After.YAML == "primary\n" + consumed = consumed || (change.Before.YAML == "primary\n" && !change.After.Present) + } + if !arrived || !consumed || control.Effective.Present { + t.Fatalf("missing control history: %#v", control) + } + container, _ := loaded.Inspection.Explain(root) + unresolved := false + for _, change := range container.History { + if change.Origin.Operation == ChangeInheritance && strings.Contains(change.After.YAML, "extends: primary") { + unresolved = true + } + } + if !unresolved || strings.Contains(container.Effective.YAML, "extends:") { + t.Fatalf("invalid enclosing snapshot: %#v", container) + } + if _, present := loaded.Inspection.Metadata().Lookup(controlPath); present { + t.Fatal("control is effective") + } + if len(loaded.Inspection.Metadata().History(controlPath)) != len(control.History) { + t.Fatal("metadata and explanation disagree") + } + }) + } +} + +func TestNestedInheritanceHistoryInsideSequence(t *testing.T) { + input := `base: + items: + - first: {value: inherited} + second: {extends: first} +child: {extends: base} +` + loaded, err := Load[map[string]any](Bytes("sequence", []byte(input)), testSecretBinding()) + if err != nil { + t.Fatal(err) + } + path := Keys("child", "items").Index(0).Key("second") + control, _ := loaded.Inspection.Explain(path.Key("extends")) + consumed := false + for _, change := range control.History { + consumed = consumed || (change.Before.YAML == "first\n" && !change.After.Present) + } + if !consumed || control.Effective.Present { + t.Fatalf("sequence control history = %#v", control) + } + value, _ := loaded.Inspection.Explain(path.Key("value")) + introduced := false + for _, change := range value.History { + introduced = introduced || (!change.Before.Present && change.After.YAML == "inherited\n") + } + if !introduced || value.Effective.YAML != "inherited\n" { + t.Fatalf("sequence value history = %#v", value) + } +} + func inheritanceInspectionForTest(t *testing.T, sources ...*document.Node) (*Inspection, *inheritanceScope) { t.Helper() composed, diagnostics := composeDocuments(sources...) diff --git a/internal/engine/inheritance_walk.go b/internal/engine/inheritance_walk.go index b3b209f..f4d51eb 100644 --- a/internal/engine/inheritance_walk.go +++ b/internal/engine/inheritance_walk.go @@ -8,7 +8,7 @@ import ( ) type inheritanceWalker struct { - root **document.Node + nodes int // logical non-directive nodes in the entire working tree origins map[string]Origin limits projectionLimits retained *retainedNodeCounter @@ -38,8 +38,12 @@ func expandInheritanceWithBudgets(input *document.Node, inputOrigins map[string] if inputOrigins == nil { origins = directInheritanceOrigins(input, Path{}, nil) } + nodes, within := inheritanceLowerBoundCount(root, limits.nodes) + if !within { + return nil, []Diagnostic{inheritanceBudgetDiagnostic(Path{}, root.Location)} + } walker := inheritanceWalker{ - root: &root, limits: limits, origins: origins, + nodes: nodes, limits: limits, origins: origins, retained: newRetainedNodeCounter(historyLimit), } if problem := walker.visit(&root, Path{}, true); problem != nil { @@ -79,7 +83,14 @@ func (w *inheritanceWalker) visit(node **document.Node, path Path, unscoped bool } } if scopeHasInheritanceDefinitions(*node) { - resolved, diagnostics := expandSiblingDefinitionsWithOrigins(*node, path, w.limits, w.origins, w.retained) + // Only this scope can change the logical total. Reserve the nodes + // outside it so expansion checks the remaining budget before cloning. + before, _ := inheritanceLowerBoundCount(*node, -1) + limits := w.limits + if limits.nodes >= 0 { + limits.nodes -= w.nodes - before + } + resolved, diagnostics := expandSiblingDefinitionsWithOrigins(*node, path, limits, w.origins, w.retained) if len(diagnostics) != 0 { return &diagnostics[0] } @@ -92,11 +103,15 @@ func (w *inheritanceWalker) visit(node **document.Node, path Path, unscoped bool w.controls = append(w.controls, resolved.controls...) w.replacements = append(w.replacements, resolved.replacements...) w.operations = append(w.operations, resolved.operations...) - if !inheritanceLowerBoundWithin(*w.root, w.limits.nodes) { - problem := inheritanceBudgetDiagnostic(path, (*node).Location) - return &problem - } + after, _ := inheritanceLowerBoundCount(*node, -1) + w.nodes += after - before } + // Operations retain immutable enclosing templates. Detach just this + // container before replacing descendants during destination resolution; + // untouched subtrees remain shared with the retained evidence. + working := **node + working.Mapping = slices.Clone(working.Mapping) + *node = &working indices := make([]int, len((*node).Mapping)) for index := range indices { indices[index] = index @@ -111,6 +126,9 @@ func (w *inheritanceWalker) visit(node **document.Node, path Path, unscoped bool } } case document.Sequence: + working := **node + working.Sequence = slices.Clone(working.Sequence) + *node = &working for index := range (*node).Sequence { child := &(*node).Sequence[index] if problem := w.visit(child, path.Index(index), (*child).Kind == document.Mapping); problem != nil { diff --git a/internal/engine/inheritance_walk_test.go b/internal/engine/inheritance_walk_test.go index cd0d7cc..1cb056b 100644 --- a/internal/engine/inheritance_walk_test.go +++ b/internal/engine/inheritance_walk_test.go @@ -194,3 +194,26 @@ func TestInheritanceExpansionHonorsOutputNodeAndDepthBudgets(t *testing.T) { }) } } + +func TestInheritanceBudgetIncludesOtherScopesAndSequenceItems(t *testing.T) { + input := parseCompositionDocument(t, "source-1", `extends: null +groups: + - a: {x: one} + b: {extends: a} + - a: {x: two} + b: {extends: a} +unchanged: {x: three} +`) + // Root (1), groups key/list (2), two expanded scopes (18), and + // unchanged key/mapping/key/value (4). Consumed controls cost nothing. + for _, limit := range []int{24, 25, 26} { + result, diagnostics := expandInheritanceTree(input, projectionLimits{nodes: limit, depth: 5}) + if limit < 25 { + if result != nil || len(diagnostics) != 1 || diagnostics[0].Code != CodeSchemaProjectionLimit { + t.Fatalf("limit %d accepted or wrong error: %#v", limit, diagnostics) + } + } else if result == nil || len(diagnostics) != 0 { + t.Fatalf("limit %d rejected: %#v", limit, diagnostics) + } + } +}