diff --git a/docs/adr/0010-effective-configuration-diff.md b/docs/adr/0010-effective-configuration-diff.md index 876f550..54a2755 100644 --- a/docs/adr/0010-effective-configuration-diff.md +++ b/docs/adr/0010-effective-configuration-diff.md @@ -2,9 +2,10 @@ ## Status -Accepted. The [feature roadmap](../roadmap/diff.md) owns the target -contract and implementation status; the -[implementation plan](../roadmap/implementation.md) owns execution order. +Accepted and implemented. The +[configuration contract](../config.md#effective-configuration-diffs) owns current +behavior. Completed planning and remediation records are preserved in Git +history at `1c2610c` under `docs/roadmap/`. ## Date diff --git a/docs/development.md b/docs/development.md index 5c1445b..928cbca 100644 --- a/docs/development.md +++ b/docs/development.md @@ -106,6 +106,13 @@ git show 8e91a2b:docs/roadmap/startup-benchmark.md Remaining optimization candidates belong to [deferred scope](roadmap/deferred.md#deferred-performance-experiments). +The completed effective-configuration diff roadmap, implementation plan, and +resolved review findings are preserved in Git history at `1c2610c` under +`docs/roadmap/`. The +[configuration contract](config.md#effective-configuration-diffs) owns current +comparison and disclosure behavior, and +[ADR 0010](adr/0010-effective-configuration-diff.md) records the decision. + Install the Go version required by `go.mod` or a compatible newer toolchain. Acquire declared dependencies explicitly before entering an offline development or validation environment: diff --git a/docs/roadmap/diff.md b/docs/roadmap/diff.md deleted file mode 100644 index c0a3224..0000000 --- a/docs/roadmap/diff.md +++ /dev/null @@ -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. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index 2a7ecfc..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -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. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md deleted file mode 100644 index 428e45d..0000000 --- a/docs/roadmap/remediation.md +++ /dev/null @@ -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.