Record effective diff review findings

docs/roadmap/remediation.md

@@ -0,0 +1,86 @@
+# 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: open.
+
+Affected: [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.