diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 08210e2..2a7ecfc 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -248,8 +248,10 @@ 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, targeted masking through absence, and fallback -preserving an absent reported side. +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 @@ -279,6 +281,14 @@ 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. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 8774e3e..428e45d 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -14,9 +14,16 @@ gates. ## 1. Restrict complete-region fallback to incompatible present shapes (P2) -Status: open. +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: [diff_snapshot.go](../../internal/engine/diff_snapshot.go), +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` diff --git a/facade_diff_test.go b/facade_diff_test.go index 60bd86a..a755fb7 100644 --- a/facade_diff_test.go +++ b/facade_diff_test.go @@ -21,6 +21,101 @@ type facadeDiffSecretConfig struct { Secret configkit.Secret `yaml:"secret"` } +func TestFacadeDiffKeepsPublicSequenceChangesWithHistoricalDescendants(t *testing.T) { + type configuration struct { + Items []any `yaml:"items"` + } + load := func(public string) *configkit.Result[configuration] { + t.Helper() + result, err := configkit.Load(configkit.Bytes("historical-sequence.yml", []byte("items: [{secret: disposable-canary}, "+public+"]\n")), + configkit.Options[configuration]{Definition: configkit.Definition[configuration]{ + Secret: func(path configkit.Path) bool { return path.String() == "/items/0/secret" }, + Resolve: func(current *configuration, resolution *configkit.Resolution) error { + current.Items[0] = "public scalar" + resolution.Record(configkit.Keys("items"), configkit.ChangeNormalization, "normalize first item") + return nil + }, + }}) + if err != nil { + t.Fatal(err) + } + return result + } + left, right := load("1"), load("2") + history, ok := left.Explain(configkit.Keys("items").Index(0).Key("secret")) + if !ok || history.Effective.Present || len(history.History) == 0 { + t.Fatalf("removed historical secret = %#v, known=%v", history, ok) + } + maskedHistory := false + for _, change := range history.History { + maskedHistory = maskedHistory || change.Before.Redacted || change.After.Redacted + } + if !maskedHistory { + t.Fatal("historical sensitivity was not retained") + } + differences, err := configkit.Diff(left, right) + if err != nil || len(differences) != 1 || differences[0].Path.String() != "/items" { + t.Fatalf("sequence difference = %#v, %v", differences, err) + } + difference := differences[0] + if !difference.Left.Present || !difference.Right.Present || difference.Left.Redacted || difference.Right.Redacted || difference.HiddenChanged { + t.Fatalf("public sequence change was concealed: %#v", difference) + } + if difference.Left.YAML != "- public scalar\n- 1\n" || difference.Right.YAML != "- public scalar\n- 2\n" { + t.Fatalf("public sequence snapshots = %#v", difference) + } +} + +func TestFacadeDiffKeepsPublicScalarAcrossHistoricalAbsence(t *testing.T) { + type configuration struct { + Dynamic map[string]any `yaml:"dynamic"` + } + options := configkit.Options[configuration]{Definition: configkit.Definition[configuration]{ + Secret: func(path configkit.Path) bool { return path.String() == "/dynamic/entry/token" }, + Resolve: func(current *configuration, resolution *configkit.Resolution) error { + delete(current.Dynamic, "entry") + resolution.Record(configkit.Keys("dynamic", "entry"), configkit.ChangeNormalization, "remove entry") + return nil + }, + }} + absent, err := configkit.Load(configkit.Bytes("removed-entry.yml", []byte("dynamic: {entry: {token: disposable-canary}}\n")), options) + if err != nil { + t.Fatal(err) + } + options.Definition.Resolve = nil + present, err := configkit.Load(configkit.Bytes("public-entry.yml", []byte("dynamic: {entry: public}\n")), options) + if err != nil { + t.Fatal(err) + } + for _, test := range []struct { + name string + left, right *configkit.Result[configuration] + leftPresent bool + }{ + {name: "addition", left: absent, right: present}, + {name: "removal", left: present, right: absent, leftPresent: true}, + } { + t.Run(test.name, func(t *testing.T) { + differences, err := configkit.Diff(test.left, test.right) + if err != nil || len(differences) != 1 || differences[0].Path.String() != "/dynamic/entry" { + t.Fatalf("entry difference = %#v, %v", differences, err) + } + difference := differences[0] + if difference.Left.Present != test.leftPresent || difference.Right.Present == test.leftPresent || + difference.Left.Redacted || difference.Right.Redacted || difference.HiddenChanged { + t.Fatalf("public presence change was concealed: %#v", difference) + } + wantLeft, wantRight := "", "public\n" + if test.leftPresent { + wantLeft, wantRight = wantRight, wantLeft + } + if difference.Left.YAML != wantLeft || difference.Right.YAML != wantRight { + t.Fatalf("entry snapshots = %#v", difference) + } + }) + } +} + func TestFacadeDiffReturnsDetachedCanonicalRecords(t *testing.T) { left, err := configkit.Load[facadeDiffConfig](configkit.Bytes("left.yml", []byte("value: before\ncount: 0\nitems: [one, two]\ndynamic: {}\n"))) if err != nil { diff --git a/internal/engine/diff_snapshot.go b/internal/engine/diff_snapshot.go index 4b8f4c6..494fc61 100644 --- a/internal/engine/diff_snapshot.go +++ b/internal/engine/diff_snapshot.go @@ -61,9 +61,6 @@ func preparePairedDiffNodes(left, right *document.Node, path Path, sensitivity * case document.Sequence: return preparePairedDiffSequence(left, right, path, sensitivity) default: - if sensitivity.hasStrictDescendant(path.String()) { - return pairedDiffSnapshot{requiresFallback: true}, true - } return pairedDiffSnapshot{left: left, right: right}, true } } diff --git a/internal/engine/diff_snapshot_test.go b/internal/engine/diff_snapshot_test.go index 75924a9..e93ae24 100644 --- a/internal/engine/diff_snapshot_test.go +++ b/internal/engine/diff_snapshot_test.go @@ -180,6 +180,13 @@ func TestPreparePairedDiffSnapshotMasksSensitiveStructuralReplacements(t *testin path: Keys("changed"), selector: "/changed/token", }, + { + name: "mapping to scalar with historical-only descendant", + left: "changed: {public: old}\n", + right: "changed: new-scalar\n", + path: Keys("changed"), + selector: "/changed/removed/secret", + }, { name: "scalar to mapping", left: "changed: old-scalar\n", @@ -241,7 +248,7 @@ func TestPreparePairedDiffSnapshotMasksOriginalRegionForNestedSequenceMismatch(t assertWholeRegionFallback(t, paired, supported, left, right) } -func TestPreparePairedDiffSnapshotUsesHistoricalDescendantClassification(t *testing.T) { +func TestPreparePairedDiffSnapshotKeepsPublicScalarsWithHistoricalDescendants(t *testing.T) { leftRoot := parseCompositionDocument(t, "left", "changed: before\n") rightRoot := parseCompositionDocument(t, "right", "changed: after\n") left := mappingValue(t, leftRoot, "changed") @@ -249,18 +256,22 @@ func TestPreparePairedDiffSnapshotUsesHistoricalDescendantClassification(t *test paired, supported := preparePairedDiffSnapshot( left, right, Keys("changed"), newDiffSensitivity(map[string]bool{"/changed/removed/secret": true}, nil), ) - assertWholeRegionFallback(t, paired, supported, left, right) + if !supported || paired.leftRedacted || paired.rightRedacted || paired.hiddenChanged { + t.Fatalf("public scalar replacement = %#v, supported=%v", paired, supported) + } + assertDiffStringNode(t, paired.left, "before") + assertDiffStringNode(t, paired.right, "after") } -func TestPreparePairedDiffSnapshotFallbackPreservesAbsentReportedSide(t *testing.T) { +func TestPreparePairedDiffSnapshotKeepsPublicContentAcrossAbsence(t *testing.T) { right := parseCompositionDocument(t, "right", "branch: scalar\n") paired, supported := preparePairedDiffSnapshot( nil, right, Keys("added"), newDiffSensitivity(nil, map[string]bool{"/added/branch/secret": true}), ) - if !supported || paired.left != nil || paired.leftRedacted || !paired.rightRedacted { - t.Fatalf("fallback presence/redaction = %#v, supported=%v", paired, supported) + if !supported || paired.left != nil || paired.leftRedacted || paired.rightRedacted || paired.hiddenChanged { + t.Fatalf("public addition presence/redaction = %#v, supported=%v", paired, supported) } - assertDiffStringNode(t, paired.right, "[REDACTED]") + assertDiffStringNode(t, mappingValue(t, paired.right, "branch"), "scalar") } func TestPreparePairedDiffSnapshotPreservesPublicReplacementBesideSensitiveSibling(t *testing.T) {