Retire completed diff plans for release preparation
This commit is contained in:
@@ -1,199 +0,0 @@
|
||||
# Effective Configuration Diff Roadmap
|
||||
|
||||
Status: implemented. This document preserves the selected contract and scope;
|
||||
the [configuration contract](../config.md#effective-configuration-diffs) owns
|
||||
current behavior, and the [implementation record](implementation.md) preserves
|
||||
the completed sequence and validation evidence.
|
||||
|
||||
## Targeted end state
|
||||
|
||||
ConfigKit compares the canonical effective inspection values retained by two
|
||||
successful loads and returns structured differences. Consumers decide how to
|
||||
render those differences: a changes table, a tree, or a changed-region diff are
|
||||
possible presentations. ConfigKit owns comparison and safe value disclosure,
|
||||
while applications own presentation and labels.
|
||||
|
||||
Both configurations have completed the normal loading lifecycle, including
|
||||
typed decoding, resolution, secret lookup, and validation. Invalid input on
|
||||
either side produces the existing load failure and diagnostics, with no usable
|
||||
diff. Comparing successful results does not rerun callbacks, reopen sources, or
|
||||
resolve credentials again. Later mutation of caller-owned `Result.Config` does
|
||||
not change the frozen values being compared.
|
||||
|
||||
The comparison is over retained inspection values, not every value accessible
|
||||
through the returned Go configuration. In particular, typed secret references
|
||||
contribute their configured environment names, not resolved credential bytes.
|
||||
|
||||
The motivating workflows are:
|
||||
|
||||
- Load `app.conf.bak` and `app.conf`, then inspect effective differences.
|
||||
- Load a base configuration and separately load it with a selected overlay,
|
||||
then inspect the overlay's effective impact, including subsequent resolution.
|
||||
|
||||
An overlay alone is not the right side of the second workflow: omitted overlay
|
||||
keys retain base values. Both workflows use successful, independent loads;
|
||||
applications control their definitions, options, and external inputs. Using the
|
||||
same definition and controlled inputs makes comparisons easier to interpret.
|
||||
Sharing a Go type does not prove equal defaults or callbacks, and comparison
|
||||
does not require callback-function identity checks.
|
||||
|
||||
## Selected public contract
|
||||
|
||||
### Entry point and records
|
||||
|
||||
The target entry point is:
|
||||
|
||||
```go
|
||||
Diff[T any](left, right *Result[T]) ([]Difference, error)
|
||||
```
|
||||
|
||||
It compares results of the same configuration type. Nil or uninitialized results
|
||||
produce an error rather than an empty successful diff. Valid results with equal
|
||||
effective values produce no difference records. Loading remains a separate
|
||||
operation; this API does not aggregate load failures or orchestrate callbacks.
|
||||
|
||||
The root-owned `Difference` record contains:
|
||||
|
||||
- `Path Path`: the semantic path of the changed region;
|
||||
- `Left ValueSnapshot`: the safe value before the change;
|
||||
- `Right ValueSnapshot`: the safe value after the change; and
|
||||
- `HiddenChanged bool`: whether at least one actual change in the region is
|
||||
withheld by diff redaction, as defined below.
|
||||
|
||||
Both sides reuse the existing `ValueSnapshot` representation: `Present`,
|
||||
`Redacted`, and deterministic `YAML`. Absence is `Present=false` with empty
|
||||
YAML; explicit null is present and renders as `null`. Presence identifies
|
||||
additions and removals. A record with both sides present describes a replacement
|
||||
at its reported path. Record membership establishes a change even when both
|
||||
safe snapshots render identically.
|
||||
|
||||
Paths and records are detached. Consumer mutation does not affect subsequent
|
||||
comparisons or either result's existing inspection state. The public facade
|
||||
exposes no internal document or parser types.
|
||||
|
||||
### Equality, granularity, and ordering
|
||||
|
||||
Compare the underlying canonical effective inspection values before redaction.
|
||||
Equal values produce no differences even if source locations, explicit-input
|
||||
status, or loading histories differ. An explicit zero and an omitted field
|
||||
defaulting to zero therefore compare equally. Presence and provenance remain
|
||||
separately inspectable through each result's existing APIs. Original comments,
|
||||
formatting, and mapping declaration order do not participate in equality.
|
||||
|
||||
Retain the existing comparator's granularity:
|
||||
|
||||
- Mappings compare recursively. Additions and removals report the affected key;
|
||||
changes beneath mappings report the changed descendant region.
|
||||
- Sequences compare atomically: any change reports the sequence path.
|
||||
- Scalar, cross-kind, and null/container replacements report the affected path.
|
||||
|
||||
Sequence comparison introduces no element alignment or public correspondence
|
||||
between old and new indices. Records are ordered by canonical pointer spelling,
|
||||
matching the existing comparator. Paths retain semantic key/index intent and
|
||||
proper escaping, including numeric mapping keys.
|
||||
|
||||
### Cross-configuration redaction
|
||||
|
||||
Apply the applicable sensitivity from both loads before returning either side's
|
||||
value. A classification on either side protects both snapshots. Include
|
||||
historical and dependency-derived classification, classified ancestors and
|
||||
descendants, and paths absent from one effective tree.
|
||||
|
||||
Where the two shapes permit corresponding sensitive regions to be identified,
|
||||
mask those regions while retaining public sibling values. Do not treat matching
|
||||
pointer text alone as proof of matching mapping-key versus sequence-index shape.
|
||||
|
||||
When a structural replacement prevents safe correspondence for a classified
|
||||
region, mask the complete reported replacement region on both present sides.
|
||||
For example, replacing a mapping containing a classified descendant with a
|
||||
scalar masks the whole mapping/scalar replacement, rather than trying to apply
|
||||
the old descendant path to the scalar. This fallback also covers incompatible
|
||||
mapping/sequence shapes within an atomic replacement. An absent side remains
|
||||
absent; redaction does not fabricate a value for it.
|
||||
|
||||
`ValueSnapshot.Redacted` describes masking, not change. `HiddenChanged` is true
|
||||
when underlying value or presence differences occur in a region withheld by the
|
||||
combined redaction policy. It is false when only public content changes and all
|
||||
masked regions retain equal values and presence. This includes atomic sequence
|
||||
records: an unchanged masked member does not alone set the flag. The complete
|
||||
replacement fallback sets the flag because the changed region itself is
|
||||
withheld. The flag does not identify individual changed children, counts, or
|
||||
sequence correspondence, and does not assert that every redacted child changed.
|
||||
|
||||
For a wholly masked changed value, record membership and `HiddenChanged` let a
|
||||
consumer show `[REDACTED]` on the left and `[CHANGED]` on the right. These
|
||||
strings are presentation choices, not configuration values or the encoding of
|
||||
change state. Literal marker-looking input remains ordinary input; applications
|
||||
use record metadata rather than interpreting YAML marker text.
|
||||
|
||||
Errors and returned records disclose neither classified values nor credential
|
||||
bytes.
|
||||
|
||||
### Credential boundary and renderer context
|
||||
|
||||
Compare classified values and configured secret reference names retained in the
|
||||
effective inspection snapshots. Rotation of a resolved credential behind an
|
||||
unchanged reference is outside this comparison. Diff does not read mutable
|
||||
`Result.Config`, reread the environment, or introduce credential retention or
|
||||
credential-derived comparison tokens.
|
||||
|
||||
Return changed regions only. The records do not include paired full-document
|
||||
context or a structured dynamic value tree, and do not alone supply unchanged
|
||||
context for unified hunks. Calling `WriteEffective` independently on each side
|
||||
is not a substitute for cross-protected context because each call applies only
|
||||
that result's sensitivity. Consumers can render the returned safe regions
|
||||
without reconstructing unredacted context.
|
||||
|
||||
## Scope and architectural fit
|
||||
|
||||
This extends ConfigKit's existing snapshot-based inspection responsibilities.
|
||||
Applications can compare completed configurations without duplicating structural
|
||||
comparison, presence handling, or safe value disclosure. Loading and composition
|
||||
rules remain unchanged.
|
||||
|
||||
The feature does not include input-text comparison, invalid-document inspection,
|
||||
patch application, generating a reconstructing overlay, fingerprints, automatic
|
||||
reload, resolved-credential comparison, cross-type comparison, or a built-in
|
||||
renderer. A diff may describe removals that existing overlay semantics cannot
|
||||
express. These boundaries define this feature's selected scope, not intrinsic
|
||||
correctness restrictions on every possible future extension.
|
||||
|
||||
## Implementation references
|
||||
|
||||
- [Deferred scope](deferred.md) continues to exclude fingerprints and unrelated
|
||||
items; effective configuration diffs are implemented.
|
||||
- The [configuration contract](../config.md#typed-projection-and-ownership)
|
||||
describes canonical typed projection and private structural comparison.
|
||||
[The existing comparator](../../internal/engine/document_compare.go) reports
|
||||
mapping additions, removals, and replacements, ignores mapping order, treats
|
||||
sequences atomically, and sorts changes by pointer spelling. It ignores
|
||||
provenance and sensitivity metadata.
|
||||
- Results retain private frozen inspection state. Existing
|
||||
[inspection types](../../inspection.go) include `ValueSnapshot`, with
|
||||
presence, redaction, and deterministic YAML. Explanation records describe
|
||||
one load's history rather than a comparison between loads.
|
||||
- [Secret classification](../config.md#secret-classification-and-effective-rendering)
|
||||
includes historical classification and dependency propagation. Existing
|
||||
snapshots apply one load's sensitivity; diff requires protection from both.
|
||||
- [Typed secret references](../config.md#typed-secret-references) retain reference
|
||||
names in inspection, not resolved credential bytes.
|
||||
- The [public facade](../internal/overview.md) is root-owned and delegates to
|
||||
the engine. The new operation and records follow that same boundary.
|
||||
|
||||
## Implementation outcome
|
||||
|
||||
The accepted inspection boundary is recorded in
|
||||
[ADR 0010](../adr/0010-effective-configuration-diff.md), and the deferred
|
||||
inventory retains fingerprints while recognizing implemented diffs. The engine
|
||||
owns canonical comparison and paired disclosure in
|
||||
[internal/engine/diff.go](../../internal/engine/diff.go); the root facade exposes
|
||||
detached records in [diff.go](../../diff.go). The configuration
|
||||
contract, consumer guidance, component overview, architecture policy, and
|
||||
development guide now link to their respective behavior and implementation
|
||||
owners.
|
||||
|
||||
Validation and coverage are recorded in the completed
|
||||
[implementation plan](implementation.md). The implementation compares frozen
|
||||
effective values, protects both sides with combined sensitivity, retains the
|
||||
selected credential boundary, and provides executable file and overlay
|
||||
workflows. Publication and release preparation remain separate.
|
||||
@@ -1,450 +0,0 @@
|
||||
# Implement effective configuration diffs
|
||||
|
||||
Status: complete. The [diff roadmap](diff.md) owns the agreed target contract and
|
||||
scope. This document preserves the implementation sequence and completion
|
||||
evidence; completed planning records retire during separately authorized release
|
||||
preparation.
|
||||
|
||||
Planning baseline: `e82b1c90543a6ab92138ee8c0c2a4cd4daea8af0`.
|
||||
No diff API exists at this baseline. This plan adds comparison of successful
|
||||
loads' frozen canonical inspection values; it does not change loading rules.
|
||||
|
||||
## Working instructions
|
||||
|
||||
- Begin each stage with [development.md](../development.md), applicable policies,
|
||||
working-tree inspection, and the owning implementation and tests. Follow
|
||||
`AGENTS.md`'s graph-first code discovery guidance. Preserve unrelated work.
|
||||
- Implement only the current stage and its relevant tests/documentation. Update
|
||||
its completion status and evidence here, then commit the validated nonempty
|
||||
unit with a short plain-English message. Keep subsequent stages planned.
|
||||
- Use the roadmap's same-type `Diff`, root-owned `Difference`, existing
|
||||
`ValueSnapshot`, atomic sequences, cross-protected redaction, and reference-only
|
||||
credential boundary. Do not replace these with an easier partial feature.
|
||||
- Keep loading, callback auditing, secret classification, existing rendering,
|
||||
and historical explanation behavior unchanged. No dependencies, toolchain
|
||||
changes, new resource caps, callback identity checks, release gates, renderer,
|
||||
sequence alignment, credential comparison tokens, or background work are
|
||||
needed. These are this implementation's scope boundaries, not new repository
|
||||
prohibitions on unrelated future designs.
|
||||
- New diff state is operation-local. Read retained trees/indexes without mutation;
|
||||
do not add load-time caches, retain paired documents after comparison, or share
|
||||
mutable state between calls. Build safe output before exposing records.
|
||||
- Run each stage's focused checks and the everyday `./scripts/check.sh` before
|
||||
committing production changes. Documentation-only stages need local-link and
|
||||
whitespace checks. Run engine race tests after the integrated engine stage
|
||||
and the full race suite during final verification. Reuse successful checks for
|
||||
unchanged content. These are development checks, not new release gates.
|
||||
- Tests remain offline and use disposable canaries, temporary files, and
|
||||
controlled environment variables. Environment-mutating tests are not parallel.
|
||||
Reuse existing loading tests rather than reproduce the loader. Benchmarks are
|
||||
optional; no machine-specific thresholds or public complexity promises.
|
||||
- Do not push, tag, publish, upgrade consumer dependencies, or retire unfinished
|
||||
roadmaps. Release preparation and inclusion in a particular version are
|
||||
separate decisions; do not assume this feature belongs in the prepared v0.6.0
|
||||
note.
|
||||
|
||||
## Implementation decisions
|
||||
|
||||
These decisions complete the roadmap's implementation details without extending
|
||||
its public contract.
|
||||
|
||||
1. Root `diff.go` defines `Difference` and
|
||||
`Diff[T any](left, right *Result[T]) ([]Difference, error)`. Engine `diff.go`
|
||||
owns `Diff(left, right *Inspection) ([]Difference, error)` and an internal-
|
||||
facing `Difference` record. Root conversion remains explicit, never an alias.
|
||||
2. A usable result has a non-nil retained inspection with a non-nil final tree.
|
||||
A valid defaults-only load is usable. Nil/zero results fail with a value-free
|
||||
error and no records. Do not validate `Result.Config`, options identity,
|
||||
source identity, or matching defaults. Do not add a public error hierarchy or
|
||||
diagnostic codes just for this feature; exact error wording is not a promise.
|
||||
3. Obtain regions with `compareConfigurationValues(left.final, right.final)`.
|
||||
Resolve their nodes with the existing shape-aware document lookup and each
|
||||
inspection's immutable lookup. Preserve the comparator's pointer ordering,
|
||||
canonical scalar equality, atomic sequences, and owned paths. Do not alter
|
||||
comparison used by resolution or validation.
|
||||
4. Combine true entries from both `Inspection.secrets` maps once per operation.
|
||||
Those maps already include historical and dependency-derived classification;
|
||||
do not rerun predicates or propagation. Canonical pointer classification
|
||||
retains its existing meaning. Actual document traversal determines whether
|
||||
a token is a mapping key or sequence index; matching text alone does not prove
|
||||
structural correspondence.
|
||||
5. Use a small private diff-local sensitivity lookup for ancestor/equal masking
|
||||
and strict-descendant classification. Reuse `pathPrefixIndex` with owner-held
|
||||
IDs where useful. Do not scan the entire classification map for each visited
|
||||
node or each difference, or build a second general path framework. Retain
|
||||
historical-only selectors even when absent from both current trees.
|
||||
6. Prepare safe paired subtrees by traversing the union of mapping keys and
|
||||
corresponding sequence positions. Position pairing is solely for disclosure
|
||||
and hidden-change detection; the public delta remains the whole sequence,
|
||||
with no element alignment. Absence is a nil node; explicit null is a present
|
||||
`document.Null` node.
|
||||
7. If a selector masks the current path by equality or ancestry, replace each
|
||||
present node there with the ordinary redaction marker. Otherwise recurse
|
||||
through compatible mappings/sequences. A missing key/index or entirely absent
|
||||
side is absence, not a kind mismatch: mask present sensitive descendants and
|
||||
preserve public siblings.
|
||||
8. A kind mismatch between two present nodes requires complete reported-region
|
||||
masking if there is a classified strict descendant of that mismatch path.
|
||||
A sensitive ancestor/equal selector already masks it under item 7. Check
|
||||
nested mismatches within atomic sequence/addition/replacement regions, not
|
||||
only the reported roots. On such a mismatch, mask both present roots of the
|
||||
original reported region; leave an absent side absent. An unrelated sensitive
|
||||
sibling does not trigger fallback for a public-only mismatch. Missing or
|
||||
incompatible historical descendant selectors still require this fallback.
|
||||
9. Compute `HiddenChanged` from raw value/presence differences at masking
|
||||
frontiers. At a masked frontier use `configurationValuesEqual` on the two
|
||||
raw nodes; otherwise OR the children's flags. A classified but unchanged
|
||||
subtree yields false even alongside a public change. Complete-region fallback
|
||||
yields true for an emitted changed region. Do not compare redacted YAML, use
|
||||
`Redacted` as a change flag, or expose child identities/counts through this flag.
|
||||
10. Render only already-safe documents through the private YAML adapter, using
|
||||
existing deterministic snapshot formatting. An absent side is the zero
|
||||
`ValueSnapshot`. Unexpected serialization failure returns a fixed value-free
|
||||
error with no usable partial diff, rather than silently substituting a marker
|
||||
and misreporting `HiddenChanged`. Existing explanation snapshot fallback
|
||||
behavior remains unchanged. No raw dumper/codec error text is exposed.
|
||||
11. No-difference results contain zero records; do not promise a distinction
|
||||
between nil and allocated empty slices. Return detached paths and immutable
|
||||
YAML strings. Sensitivity differences alone do not emit records for equal
|
||||
effective values. Frozen snapshots include secret reference names, never
|
||||
credentials, even when the caller changes the environment after loading.
|
||||
|
||||
## Stage 1 — Establish evidence and record the architectural decision
|
||||
|
||||
Status: complete. Scope: orientation, baseline evidence, and one ADR; no feature
|
||||
code or current-behavior documentation changes.
|
||||
|
||||
1. Confirm the baseline and inspect the roadmap and relevant code/test owners:
|
||||
`document_compare.go`, `snapshot.go`, `render.go`, `load.go`, `load_result.go`,
|
||||
`document_walk.go`, `document_lookup.go`, root `lifecycle.go`, `inspection.go`,
|
||||
`conversion.go`, and `path.go`.
|
||||
2. Run `./scripts/check.sh`; record revision, local Go version, results, and any
|
||||
environmental limitation here. Identify existing coverage for canonical
|
||||
comparison, snapshot presence/redaction, immutable inspection, and load failure.
|
||||
3. Write `docs/adr/0010-effective-configuration-diff.md` using the repository's
|
||||
ADR format and the agreed roadmap. Check the next unused ADR number before
|
||||
creating it. Record the accepted architectural choice and supporting reasons:
|
||||
completed-load inspection, canonical equality, shared composition model,
|
||||
cross-protected snapshots, application-owned presentation, and the credential
|
||||
boundary. Put rejected choices only in Alternatives Considered. Do not discuss
|
||||
downstream-specific conventions or use negative parallelism outside that
|
||||
section. The roadmap continues to own implementation status.
|
||||
|
||||
Done: baseline evidence and an accepted decision are recorded. Existing public
|
||||
behavior remains unchanged; the deferred inventory is reconciled when the API
|
||||
lands in Stage 10.
|
||||
|
||||
### Baseline evidence
|
||||
|
||||
- Implementation baseline: `3697a511242d60c860549f09f02b75d1eb2fe660`;
|
||||
planning baseline: `e82b1c90543a6ab92138ee8c0c2a4cd4daea8af0`. The worktree was
|
||||
clean before validation. The accepted architectural decision is recorded in
|
||||
[ADR 0010](../adr/0010-effective-configuration-diff.md).
|
||||
- `GOTOOLCHAIN=local go version`: `go1.26.8 linux/amd64`.
|
||||
`GOTOOLCHAIN=local ./scripts/check.sh`: passed formatting, whitespace,
|
||||
`go test ./...`, `go vet ./...`, and `go build ./...`. No environmental
|
||||
limitations were encountered.
|
||||
- Existing owners identified: canonical changed-region comparison and escaped
|
||||
ordering in `internal/engine/document_compare.go` and
|
||||
`TestCompareConfigurationValuesSortEscapedPointers`; snapshot presence and
|
||||
redaction in `internal/engine/snapshot.go`, `internal/engine/render.go`, and
|
||||
`TestWriteEffectiveRedactsTagsDynamicHistoryAndDependencyCycles`; frozen
|
||||
inspection state and read-only lookups in `internal/engine/load_result.go`,
|
||||
`internal/engine/document_lookup.go`, `TestFacadeInspectionUsesFrozenState`,
|
||||
and `TestConcurrentReadOnlyInspectionQueries`; load-failure behavior in
|
||||
`TestLoadReturnsNilForSourceSchemaAndDecodeFailures` and
|
||||
`TestFacadeLoadReturnsFacadeLoadError`.
|
||||
|
||||
## Stage 2 — Prepare canonical changed regions
|
||||
|
||||
Status: complete. Scope: a private helper and structural tests only.
|
||||
|
||||
1. Add `internal/engine/diff.go` with private preparation over two inspections.
|
||||
Reject unavailable inspection/final state with safe errors and no regions.
|
||||
2. Reuse `compareConfigurationValues`; package each delta's owned path with
|
||||
borrowed immutable left/right nodes found through existing lookup helpers.
|
||||
Do not clone metadata, inspect sources, replay history, project mutable Go
|
||||
values, or write to retained lookup maps.
|
||||
3. Add `internal/engine/diff_test.go` coverage for equal roots/mapping order,
|
||||
additions/removals, null versus absence, empty containers, scalar kinds,
|
||||
atomic sequences, root changes, lexical pointer order, escaped/empty/numeric
|
||||
keys, and unavailable inspections. Reuse comparator fixtures where suitable.
|
||||
Private node fixtures may exercise structural cases not reachable from one
|
||||
typed schema; full-load tests belong in later stages.
|
||||
|
||||
Validate: `go test ./internal/engine -run 'Diff|CompareConfiguration|Path'` and
|
||||
the everyday check. Done: raw regions match existing structural semantics, and
|
||||
no consumer-facing operation exposes them yet.
|
||||
|
||||
## Stage 3 — Combine sensitivity into a local lookup
|
||||
|
||||
Status: complete. Scope: classification lookup, independently of safe rendering.
|
||||
|
||||
1. Add `internal/engine/diff_sensitivity.go` and focused tests. Union true
|
||||
classifications from both inspections without mutating either map.
|
||||
2. Implement ancestor/equal masking and strict-descendant lookup with separator-
|
||||
aware canonical tokens. Preserve root versus the empty-key pointer, escaping,
|
||||
exact terminal boundaries, and `/a` versus `/ab`.
|
||||
3. Exercise either-side classification, false map entries, duplicates, root,
|
||||
ancestors/descendants, and historical-only paths. Verify both input maps are
|
||||
unchanged. Test queries by their results, not the chosen trie layout.
|
||||
|
||||
Validate: `go test ./internal/engine -run 'Diff|PathPrefix'` and the everyday
|
||||
check. Done: later disclosure can query combined sensitivity without repeated
|
||||
whole-map scans or new persistent inspection state.
|
||||
|
||||
## Stage 4 — Build paired safe snapshots for compatible shapes
|
||||
|
||||
Status: complete. Scope: compatible mapping/sequence/scalar traversal and absence.
|
||||
|
||||
1. Add private paired traversal in `internal/engine/diff_snapshot.go`, passing
|
||||
the reported region path and Stage 3's lookup. Build safe owned containers
|
||||
for each present side. Reuse the existing marker and safe snapshot conventions.
|
||||
2. Mask ancestor/equal-selected nodes before exposing bytes. Visit the union of
|
||||
mapping keys and sequence positions, preserving each side's own content and
|
||||
length. Do not fill missing keys or indices with explicit null.
|
||||
3. Preserve public siblings and deterministic YAML ordering. Borrow immutable
|
||||
scalar nodes only where safe; never modify retained node structs or slices.
|
||||
4. Test aligned sensitive/public mappings, sequences with unchanged masked
|
||||
members, absent subtrees, added/removed sensitive descendants, classified root
|
||||
and containers, one-sided classification, and literal marker-looking strings.
|
||||
Keep this helper private and unexposed; Stage 5 completes incompatible shapes
|
||||
before Stage 7 exposes the engine operation. Unsupported intermediate branches
|
||||
must not return unprotected values.
|
||||
|
||||
Validate: `go test ./internal/engine -run 'Diff|Effective|Snapshot'` and the
|
||||
everyday check. Done: compatible paired masking preserves presence and public
|
||||
content, while existing standalone rendering remains unchanged.
|
||||
|
||||
Evidence: the focused engine tests and `./scripts/check.sh` passed. The check
|
||||
completed formatting, whitespace, `go test ./...`, `go vet ./...`, and
|
||||
`go build ./...` on `go1.26.8 linux/amd64`. Tests confirm one-sided
|
||||
classification protects both sides, absent subtrees retain absence and public
|
||||
siblings, compatible sequences preserve each side's length, retained trees are
|
||||
unchanged, and unsupported intermediate shapes return no nodes.
|
||||
|
||||
## Stage 5 — Complete structural-replacement masking
|
||||
|
||||
Status: complete. Scope: the fallback rule and its shape-sensitive regressions.
|
||||
|
||||
1. Extend the paired traversal to detect incompatible present kinds at the root
|
||||
or nested positions. Use strict-descendant classification under the mismatch
|
||||
path to request complete masking of the original reported region.
|
||||
2. On fallback discard partial safe trees for that region, mask each present
|
||||
reported root, and preserve any absent side. Without applicable sensitivity,
|
||||
ordinary public kind replacements retain their values. Do not infer shape
|
||||
from pointer spelling or ignore a historical selector that is now absent.
|
||||
3. Test both directions of mapping/scalar, mapping/sequence with numeric keys,
|
||||
and null/container replacements; nested mismatches inside atomic sequences;
|
||||
and historical-only descendant classification. Contrast public mismatches
|
||||
with unrelated unchanged sensitive siblings, and ordinary missing keys/indices
|
||||
that permit targeted redaction rather than complete masking.
|
||||
|
||||
Validate: focused `Diff` tests, existing rendering tests, and the everyday
|
||||
check. Done: every shape branch has defined disclosure behavior without blanket
|
||||
masking of otherwise compatible public regions.
|
||||
|
||||
Evidence: `go test ./internal/engine -run 'Diff|Effective|Snapshot'` and
|
||||
`./scripts/check.sh` passed. Coverage includes both mapping/scalar directions,
|
||||
mapping/sequence replacements with numeric path tokens, null/container changes,
|
||||
nested sequence mismatches, historical-only descendant classification,
|
||||
unrelated sensitive siblings, and targeted masking through absence. Post-review
|
||||
remediation corrects the original stronger-policy tests: historical descendants
|
||||
alone do not mask same-kind scalars or public additions across absence. A
|
||||
historical-only selector still triggers fallback for incompatible present kinds.
|
||||
|
||||
## Stage 6 — Distinguish hidden changes from unchanged masked content
|
||||
|
||||
Status: complete. Scope: one flag threaded through the existing paired traversal.
|
||||
|
||||
1. Have paired preparation return `HiddenChanged` alongside the safe nodes and
|
||||
each side's redaction state. At directly masked frontiers compare raw nodes
|
||||
with `configurationValuesEqual`; include presence. OR flags through unmasked
|
||||
containers. Full-region fallback on a delta sets true.
|
||||
2. Do not use safe YAML equality to infer raw equality. Do not set the flag simply
|
||||
because either snapshot is redacted. Do not align sequences or emit new
|
||||
descendant delta records; hidden detection uses the same positional disclosure
|
||||
traversal as Stage 4.
|
||||
3. Test wholly masked changes, identical rendered snapshots with differing raw
|
||||
values, mixed public/hidden changes, public-only changes with unchanged masked
|
||||
content (false), masked additions/removals, changed sequence lengths, and
|
||||
fallback (true). Include raw equality under different classifications to show
|
||||
that classification alone does not create a delta.
|
||||
|
||||
Validate: `go test ./internal/engine -run 'Diff|Secret|Sensitivity'` and the
|
||||
everyday check. Done: the flag communicates withheld change without claiming
|
||||
that every masked child changed or revealing additional change details.
|
||||
|
||||
Evidence: the focused engine tests and `./scripts/check.sh` passed. Tests cover
|
||||
changed raw values behind identical markers, mixed and public-only changes,
|
||||
unchanged masked mapping and sequence members, masked additions/removals,
|
||||
sequence-length changes, structural fallback, and equal values with differing
|
||||
classification but no changed region.
|
||||
|
||||
Post-review regressions also verify public-only sequence changes alongside an
|
||||
unchanged scalar with historical sensitive descendants, plus public scalar
|
||||
addition/removal across historical absence. These retain visible snapshots and
|
||||
`HiddenChanged=false` while historical explanation sensitivity remains intact.
|
||||
The corrected tests failed before remediation and passed after removing the
|
||||
extra scalar/default fallback. Focused diff checks, `./scripts/check.sh`, and
|
||||
`go test -race ./...` passed.
|
||||
|
||||
## Stage 7 — Assemble the complete engine diff operation
|
||||
|
||||
Status: complete. Scope: internal-facing records, serialization, and real loads.
|
||||
|
||||
1. Define engine `Difference` with `Path`, `Left`, `Right`, and `HiddenChanged`,
|
||||
and the operation over two `*Inspection` values. Wire Stages 2–6 together:
|
||||
prepare raw regions; build the local classification lookup once; make safe
|
||||
pairs; serialize safe nodes; append records in comparator order.
|
||||
2. Match existing `ValueSnapshot` YAML/presence/redaction behavior on ordinary
|
||||
successful serialization. Return safe errors with no records for unavailable
|
||||
inputs or unexpected serialization failure. Do not reuse explanation's
|
||||
silent rendering-failure fallback for the diff flag. Do not expose internal
|
||||
node pointers in the result.
|
||||
3. Add full-load tests using typed/dynamic fields, defaults, recorded resolution,
|
||||
dependency-derived sensitivity, and historical removal/replacement. Confirm
|
||||
actual `Inspection.final` values and `Inspection.secrets` drive comparison.
|
||||
4. Load typed secrets with disposable environment values: rotation behind the
|
||||
same reference produces no diff; changing reference names does. Exercise
|
||||
optional secret absence and classified reference names. Change the environment
|
||||
after loading to prove diff does not reread it.
|
||||
5. Exercise unchanged standalone effective rendering, explanations, metadata,
|
||||
and independent loads after comparison. Repeated diff calls remain detached.
|
||||
|
||||
Validate: `go test ./internal/engine -run 'Diff|Explain|History|Secret|Independent'`,
|
||||
`go test -race ./internal/engine`, and the everyday check. Done: complete safe
|
||||
comparison works through the engine with no loading or existing inspection
|
||||
regression. Do not expose a partial public API earlier.
|
||||
|
||||
Evidence: focused engine tests, `go test -race ./internal/engine`, and
|
||||
`./scripts/check.sh` passed. Real-load coverage verifies typed secret references,
|
||||
dynamic values, defaults, recorded resolution and dependency-derived sensitivity,
|
||||
historical secret removal/replacement, credential rotation without reference
|
||||
change, reference-name changes, optional nil references, and frozen state after
|
||||
environment mutation. Effective rendering, explanation, metadata lookup,
|
||||
independent loads, detached repeated results, unavailable-inspection errors, and
|
||||
value-free serialization errors remain covered.
|
||||
|
||||
## Stage 8 — Add the root-owned public facade
|
||||
|
||||
Status: complete. Scope: public declarations, conversion, and external tests.
|
||||
|
||||
1. Add root `diff.go` with the exact roadmap record and generic entry point.
|
||||
Validate nil/zero `Result` state without inspecting caller-owned `Config`;
|
||||
delegate through the private inspection pointer. Convert engine records with
|
||||
existing path and `fromEngineValueSnapshot` conventions. Ensure engine paths
|
||||
are owned before wrapping them; root signatures expose only root types.
|
||||
2. Add Go documentation for direction, presence, granularity, ordering, frozen
|
||||
state, combined sensitivity, `HiddenChanged`, equal-result behavior, errors,
|
||||
and credential limitation. No extra options, method-only alternative,
|
||||
exported bridge, generic value tree, or second supported import package.
|
||||
3. Add `facade_diff_test.go` in the external test package for representative
|
||||
results, nil/zero inputs, defaults-only loads, equal values from different
|
||||
sources/defaults/resolvers, explicit zero versus omitted/defaulted zero,
|
||||
and null/absence. Mutate `Result.Config` and returned records, then confirm
|
||||
repeated diffs and existing inspection still use frozen values.
|
||||
4. Confirm callbacks run during loads only. A temporary source file may be
|
||||
removed after both loads and comparison still works. Test invalid left and
|
||||
right loads retain the normal nil-result/LoadError behavior; callers must
|
||||
handle those errors before diffing. Do not add a new load-failure aggregator.
|
||||
|
||||
Validate: `go test . -run 'Diff|FacadeInspection|PublicFacadeBoundary|DependencyImportBoundary'`
|
||||
and the everyday check. Done: the selected API is available through the stable
|
||||
root facade without weakened ownership or changed load contracts.
|
||||
|
||||
Evidence: the focused facade and boundary tests and `./scripts/check.sh` passed.
|
||||
External-package coverage verifies canonical record order, recursive mapping and
|
||||
atomic sequence snapshots, absence versus null, nil/zero result errors,
|
||||
defaults-only and equal-value comparisons across sources and resolvers, frozen
|
||||
state after caller and record mutation, callbacks only during load, comparison
|
||||
after temporary source removal, combined secret redaction, credential rotation,
|
||||
and the existing nil-result/`LoadError` load contract.
|
||||
|
||||
## Stage 9 — Demonstrate both consumer workflows
|
||||
|
||||
Status: complete. Scope: maintained examples and their behavior tests.
|
||||
|
||||
1. Add a small complete example under `examples/consumers` that loads two caller-
|
||||
selected files with one definition, handles each load error, calls `Diff`, and
|
||||
presents safe changes using consumer-owned formatting. Tests use temporary
|
||||
files; documentation contains no real application configuration or credentials.
|
||||
2. Add a base-versus-base-plus-overlay example using normal loading options and
|
||||
the selected overlay. Include recorded resolution and a classified nested
|
||||
value, showing the difference after resolution rather than diffing overlay
|
||||
source text. Recreate one-shot sources where necessary under existing reuse
|
||||
rules. Demonstrate retained lower-priority siblings.
|
||||
3. Use `Present` for absence, record membership for change, and `HiddenChanged`
|
||||
for withheld change indicators. Do not parse marker text. Do not supplement
|
||||
records with separately rendered full contexts that lack combined sensitivity.
|
||||
4. Add an executable root `ExampleDiff` using minimal mapping inputs if this
|
||||
provides useful API discovery without duplicating the complete workflows.
|
||||
Verify example output is deterministic and secret-free.
|
||||
|
||||
Validate: `go test ./examples/consumers`, `go test . -run '^ExampleDiff$'` when
|
||||
added, and the everyday check. Done: applications can implement the motivating
|
||||
workflows without internal imports or their own structural comparison.
|
||||
|
||||
Evidence: `go test ./examples/consumers`, `go test . -run '^ExampleDiff$'`,
|
||||
and `./scripts/check.sh` passed. Temporary-file coverage exercises both
|
||||
caller-selected loads and base-versus-selected-overlay comparison. The overlay
|
||||
case verifies a recorded derived URL after resolution, a retained lower-priority
|
||||
port, bilateral redaction and `HiddenChanged` for a nested secret-reference
|
||||
change, deterministic consumer formatting, and no credential disclosure. The
|
||||
presenter uses snapshot presence and record membership without interpreting the
|
||||
redaction marker; no full-context rendering supplements the diff records.
|
||||
|
||||
## Stage 10 — Reconcile documentation and audit the complete feature
|
||||
|
||||
Status: complete. Scope: current-behavior documentation, integration audit, and
|
||||
completion evidence; no release publication.
|
||||
|
||||
1. Add an effective-diff section to `docs/config.md` for shared equality,
|
||||
granularity, cross-redaction, and credential semantics. Symbol details remain
|
||||
beside declarations. Add task guidance/example links in
|
||||
`docs/consumers/README.md` and concise navigation in the root README.
|
||||
2. Update `docs/internal/overview.md` for implemented ownership; add the new ADR
|
||||
and focused diff test commands to `docs/development.md`. Update architecture
|
||||
policy only to recognize the approved inspection boundary and link to owners;
|
||||
preserve existing loading, composition, confidentiality, and ownership rules.
|
||||
3. In `docs/roadmap/deferred.md`, remove configuration diffs from the excluded
|
||||
list, keep fingerprints and unrelated exclusions, and link implemented diff
|
||||
behavior to its owner. No unrelated policy provenance, licensing, dependencies,
|
||||
prepared release notes, or release machinery changes are required.
|
||||
4. Review the complete diff against the planning baseline and every roadmap
|
||||
contract. Check all structural/redaction branches, error disclosure, the
|
||||
root/private boundary, detached records, and unaffected standalone inspection.
|
||||
Remove abandoned temporary branches/helpers from earlier stages; do not keep
|
||||
alternate production comparators or unsafe public entry points.
|
||||
5. Run `./scripts/check.sh` and `GOTOOLCHAIN=local GOPROXY=off go test -race ./...`.
|
||||
Confirm tests cover both workflows, aligned and incompatible secret shapes,
|
||||
hidden-change false/true cases, reference rotation, invalid results, ownership,
|
||||
and the existing public/import boundaries. Document actual results, revisions,
|
||||
and limitations here without claiming unmeasured speedups.
|
||||
6. Mark stages and roadmap complete only once the selected outcomes and owning
|
||||
documentation agree. Keep planned/deferred work accurate. Retire completed
|
||||
planning records later during separately authorized release preparation.
|
||||
|
||||
Done: the agreed feature is implemented, validated, and documented without
|
||||
stronger unrelated guarantees or changes to the publication contract.
|
||||
|
||||
Evidence: the planned feature baseline was `e82b1c90543a6ab92138ee8c0c2a4cd4daea8af0`;
|
||||
the reviewed implementation through consumer examples is commit `a37e689`,
|
||||
following the accepted decision at `3697a51` and public facade at `8ae542b`.
|
||||
The complete baseline-to-implementation diff was reviewed against this plan and
|
||||
the selected contract. `go test ./internal/engine -run 'Diff|Explain|History|Secret|Independent'`,
|
||||
`go test . -run 'Diff|FacadeInspection|PublicFacadeBoundary|DependencyImportBoundary'`,
|
||||
`go test ./examples/consumers`, `go test . -run '^ExampleDiff$'`, the local
|
||||
documentation-link test, and `./scripts/check.sh` passed on
|
||||
`go1.26.8 linux/amd64`. `GOTOOLCHAIN=local GOPROXY=off go test -race ./...` also passed.
|
||||
The feature does not compare resolved credential bytes or supply paired full-
|
||||
document rendering context; credential rotation behind the same reference is
|
||||
not reported. Fingerprints remain deferred, no performance claim is made, and
|
||||
release publication remains separate.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None required for this scope. The roadmap's four choices are approved; the
|
||||
shape fallback, `HiddenChanged` calculation, error handling, and sequencing are
|
||||
resolved above. Any new requirement found during implementation should be
|
||||
reported with its impact rather than inferred as an additional contract.
|
||||
@@ -1,93 +0,0 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user