140 lines
7.3 KiB
Markdown
140 lines
7.3 KiB
Markdown
# 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 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
|
||
|
||
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.
|