From b45f7a0410e703552bd61da38f4924e0f1617a80 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 30 Sep 2026 00:00:09 +0000 Subject: [PATCH] Record sibling inheritance review findings --- docs/roadmap/remediation.md | 116 ++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..3d7ca98 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,116 @@ +# Sibling Inheritance Review Findings + +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. + +## 1. Preserve immutable enclosing snapshots during nested resolution + +Priority: P2 — incorrect historical explanation despite correct final values. + +Locations: [retained operation templates](../../internal/engine/inheritance.go) +(lines 230–235), [nested mutation](../../internal/engine/inheritance_walk.go) +(lines 107–109), and [explanation replay](../../internal/engine/inheritance_explain.go) +(lines 26–38). + +`inheritanceOperation.parent` and `.effective` refer to the same node trees that +the destination walker later modifies. Replacing nested `entry.Value` pointers +therefore changes already-recorded enclosing snapshots. Their origin indexes +still describe the earlier template, so historical values and origins can disagree. +Replaying an enclosing transfer prematurely installs already-resolved nested values +and loses the nested control input that should explain those values. + +Reproduction: load this configuration through the public loading path (a +`map[string]any` configuration suffices): + +```yaml +presets: + base: + settings: + primary: {width: 10} + secondary: {extends: primary} + custom: + extends: base + settings: + primary: {width: 20} +``` + +The final child width is correctly `20`. However: + +- `Explain(/presets/custom/settings/secondary/width)` reports its only + inheritance value transition as `20` to `20`, without showing its introduction. +- `Explain(/presets/custom/settings/secondary/extends)` returns two structural + events with both before and after absent. It never shows the inherited + `primary` reference before consumption. +- Explaining the enclosing child shows fully resolved nested values in its + enclosing transfer, before the nested operations that supposedly produce them. + +These observations were reproduced with temporary engine tests invoking `Load` +and `Inspection.Explain`; the temporary tests were removed after the review. +This violates the agreed inspectable derivation and enclosing-before-nested +history, not a request for additional historical guarantees. + +Required correction: + +- Keep retained parent/effective templates immutable while destination-local + traversal proceeds. Use detached working structure or copy-on-write ownership + as appropriate, preserving shared evidence without a snapshot per leaf. +- Keep each operation's values and origins at the same structural point. +- Replay inherited control arrival/consumption and nested value introduction + explicitly, with metadata and explanation agreeing. +- Preserve existing retained-history accounting for any additional owned nodes. + +Acceptance: add regressions using this nested example that assert meaningful +before/after values and consumed control history, not only the final result or +absence of `ChangeDecoding`. Cover a further enclosing chain and compare leaf +and container explanations. Existing ownership, redaction, and budget tests must +continue to pass. + +## 2. Remove whole-document budget rescans per sibling scope + +Priority: P2 — avoidable superlinear expansion work. + +Location: [per-scope global lower-bound check](../../internal/engine/inheritance_walk.go) +(line 95). + +Every resolved scope invokes `inheritanceLowerBoundWithin(*w.root, ...)`, traversing +the complete current document again. With N independent constant-sized sibling +scopes, input, output, and retained relationship evidence are all O(N), but this +check alone performs O(N²) visits. Its temporary traversal stacks also create +unnecessary allocation growth. This is independent of legitimate large inherited +output and of key-sorting costs. + +A temporary benchmark generated N independent root entries, each containing +`a: {x: v}` and `b: {extends: a}`, and invoked `expandInheritanceTree`. Allocation +measurements were approximately 5.8 MB/op at 256 scopes, 14.1 MB/op at 512, and +49.5 MB/op at 1,024. Timing was noisy and is not proposed as a gate; the repeated +global traversal establishes the complexity defect directly. Existing benchmarks +concentrate on shared parents/chains rather than this independent-scope shape. + +Required correction: + +- Maintain a document-level logical node total using old/new scope counts, or + equivalent incremental accounting, rather than rescanning unrelated branches. +- Keep early bounded expansion and the existing final node/depth verification; + do not weaken the existing budgets or add a new cap to work around the cost. +- Include an independent-scope benchmark alongside current shapes and inspect + growth relative to output/evidence size. Do not add timing thresholds. + +Acceptance: show that independent-scope accounting no longer revisits the whole +document per scope; retain at/beyond-budget tests and the existing value/origin +semantics. Benchmark evidence should demonstrate the removed allocation/work +growth without asserting an unconditional O(N) public guarantee. + +## Validation and contract review + +- `./scripts/check.sh` passed on Linux. +- `go test -race ./...` passed on Linux. +- Temporary diagnostic tests and the independent-scope benchmark reproduced the + findings; no probe files remain in the repository. +- No clear additional public prohibition, strengthened security guarantee, or + release gate was found. Reserved `extends`, sibling-only lookup, null cancellation, + invalid root/list-element references, inherited explicitness, and preservation + of version ownership reflect the agreed design. The performance correction + should preserve existing limits rather than introduce stricter ones.