94 lines
5.4 KiB
Markdown
94 lines
5.4 KiB
Markdown
# Effective configuration diff review findings
|
||
|
||
Reviewed `3697a51..2ebdfb5` against the agreed [diff roadmap](diff.md),
|
||
[implementation plan](implementation.md), and
|
||
[ADR 0010](../adr/0010-effective-configuration-diff.md).
|
||
|
||
The root-owned facade, frozen canonical comparison, atomic sequence records,
|
||
operation-local combined sensitivity, detached snapshots, reference-only
|
||
credential comparison, and consumer-owned presentation broadly match the
|
||
selected architecture. The finding below is an unintended strengthening of
|
||
masking behavior and a corresponding hidden-change reporting error. It does
|
||
not call for changes to loading, credential handling, dependencies, or release
|
||
gates.
|
||
|
||
## 1. Restrict complete-region fallback to incompatible present shapes (P2)
|
||
|
||
Status: complete. The scalar/default descendant fallback is removed. The two
|
||
stronger-policy tests now require visible public content, and a separate
|
||
historical-only descendant case verifies that actual incompatible present kinds
|
||
still trigger complete-region masking. Public-load regressions cover the
|
||
unchanged scalar inside an atomic sequence and both addition/removal directions
|
||
across historical absence; they failed before the fix and pass afterward.
|
||
Historical sensitivity remains available to existing explanation rendering.
|
||
Focused diff tests, `./scripts/check.sh`, and `go test -race ./...` passed.
|
||
|
||
Affected at the reviewed commit: [diff_snapshot.go](../../internal/engine/diff_snapshot.go),
|
||
`preparePairedDiffNodes`, lines 63–66; and
|
||
[diff_snapshot_test.go](../../internal/engine/diff_snapshot_test.go),
|
||
`TestPreparePairedDiffSnapshotUsesHistoricalDescendantClassification`
|
||
(lines 244–252) and
|
||
`TestPreparePairedDiffSnapshotFallbackPreservesAbsentReportedSide`
|
||
(lines 255–263).
|
||
|
||
The agreed implementation decisions 7–9 distinguish absence from a mismatch
|
||
between two present kinds. Complete-region masking is selected when an actual
|
||
structural replacement makes a classified descendant unsafe to correspond.
|
||
Public-only changes alongside unchanged masked content must retain
|
||
`HiddenChanged=false`.
|
||
|
||
The implementation already detects incompatible present kinds at lines 45–49,
|
||
but its scalar/default branch independently requests fallback whenever a strict
|
||
sensitive descendant exists. It does this even when both scalar values are
|
||
equal, or when one side is absent. Historical classification beneath a scalar
|
||
therefore masks otherwise public changes, potentially including the entire
|
||
containing atomic sequence, and sets `HiddenChanged=true` without a changed
|
||
classified value or an incompatible present-shape replacement.
|
||
|
||
Two public-API reproductions failed at the reviewed commit:
|
||
|
||
1. Both loads start with `items: [{secret: disposable}, N]`. A recorded resolver
|
||
replaces the first element with the same public scalar in both loads. The
|
||
path predicate retains historical sensitivity at `/items/0/secret`, while
|
||
only the public second element changes from `1` to `2`. Ordinary effective
|
||
rendering shows the public sequence. Diff instead returns wholly redacted
|
||
left/right sequence snapshots and `HiddenChanged=true`.
|
||
2. The left load removes `dynamic.entry` through recorded resolution after
|
||
observing a classified historical `dynamic.entry.token`. The right load
|
||
adds `dynamic.entry: public`. Diff correctly keeps the left side absent but
|
||
masks the public right scalar and reports a hidden change solely because of
|
||
the old descendant selector.
|
||
|
||
Required remediation:
|
||
|
||
- Remove the additional scalar/default descendant fallback. Preserve masking
|
||
for classified ancestors/equal paths and the existing incompatible-present-
|
||
kind fallback, including nested and historical-only descendant cases.
|
||
- Preserve targeted masking across absence and return public scalar snapshots
|
||
when no corresponding current value is classified. Do not solve this by
|
||
weakening either-side classification or discarding historical selectors.
|
||
- Revise the two named tests, which currently encode the stronger policy:
|
||
same-kind scalar replacements and absent-to-scalar additions should not
|
||
trigger fallback solely from a historical descendant selector. Keep separate
|
||
coverage that actual incompatible present kinds still trigger full masking.
|
||
- Add bounded public-load regressions for the two cases above, checking presence,
|
||
visible public snapshots, and `HiddenChanged=false`; include reverse-direction
|
||
comparison where it supplies distinct protection. Reuse existing masking and
|
||
ownership tests rather than introduce a second implementation.
|
||
- Run focused diff tests, `./scripts/check.sh`, and the applicable ownership/race
|
||
checks. Reconcile completion evidence with the corrected behavior; do not
|
||
strengthen the roadmap to accommodate the unintended restriction.
|
||
|
||
## Review validation
|
||
|
||
- `./scripts/check.sh` passed at `2ebdfb5`: formatting, whitespace, all-package
|
||
tests, vet, and build.
|
||
- `GOTOOLCHAIN=local GOPROXY=off go test -race ./...` passed.
|
||
- `GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...` passed.
|
||
- Two additional public-load probes in a disposable checkout outside this
|
||
repository failed with the outputs described above. Removing only the
|
||
scalar/default fallback made both probes pass. The two named existing tests
|
||
then failed, confirming that their expectations preserve the contract drift.
|
||
- No production fixes or regression tests were added to this repository during
|
||
the review. No benchmarks were needed or performance guarantees inferred.
|