From a4b5a6840a4f299ab5f52a21ab7c02d15fc12959 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 9 Oct 2026 12:57:40 +0000 Subject: [PATCH] Record effective diff review findings --- docs/roadmap/remediation.md | 86 +++++++++++++++++++++++++++++++++++++ 1 file changed, 86 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..8774e3e --- /dev/null +++ b/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.