Record sibling inheritance review findings

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.