Correct diff masking for historical descendants
This commit is contained in:
@@ -248,8 +248,10 @@ Evidence: `go test ./internal/engine -run 'Diff|Effective|Snapshot'` and
|
|||||||
`./scripts/check.sh` passed. Coverage includes both mapping/scalar directions,
|
`./scripts/check.sh` passed. Coverage includes both mapping/scalar directions,
|
||||||
mapping/sequence replacements with numeric path tokens, null/container changes,
|
mapping/sequence replacements with numeric path tokens, null/container changes,
|
||||||
nested sequence mismatches, historical-only descendant classification,
|
nested sequence mismatches, historical-only descendant classification,
|
||||||
unrelated sensitive siblings, targeted masking through absence, and fallback
|
unrelated sensitive siblings, and targeted masking through absence. Post-review
|
||||||
preserving an absent reported side.
|
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
|
## 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
|
sequence-length changes, structural fallback, and equal values with differing
|
||||||
classification but no changed region.
|
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
|
## Stage 7 — Assemble the complete engine diff operation
|
||||||
|
|
||||||
Status: complete. Scope: internal-facing records, serialization, and real loads.
|
Status: complete. Scope: internal-facing records, serialization, and real loads.
|
||||||
|
|||||||
@@ -14,9 +14,16 @@ gates.
|
|||||||
|
|
||||||
## 1. Restrict complete-region fallback to incompatible present shapes (P2)
|
## 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
|
`preparePairedDiffNodes`, lines 63–66; and
|
||||||
[diff_snapshot_test.go](../../internal/engine/diff_snapshot_test.go),
|
[diff_snapshot_test.go](../../internal/engine/diff_snapshot_test.go),
|
||||||
`TestPreparePairedDiffSnapshotUsesHistoricalDescendantClassification`
|
`TestPreparePairedDiffSnapshotUsesHistoricalDescendantClassification`
|
||||||
|
|||||||
@@ -21,6 +21,101 @@ type facadeDiffSecretConfig struct {
|
|||||||
Secret configkit.Secret `yaml:"secret"`
|
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) {
|
func TestFacadeDiffReturnsDetachedCanonicalRecords(t *testing.T) {
|
||||||
left, err := configkit.Load[facadeDiffConfig](configkit.Bytes("left.yml", []byte("value: before\ncount: 0\nitems: [one, two]\ndynamic: {}\n")))
|
left, err := configkit.Load[facadeDiffConfig](configkit.Bytes("left.yml", []byte("value: before\ncount: 0\nitems: [one, two]\ndynamic: {}\n")))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@@ -61,9 +61,6 @@ func preparePairedDiffNodes(left, right *document.Node, path Path, sensitivity *
|
|||||||
case document.Sequence:
|
case document.Sequence:
|
||||||
return preparePairedDiffSequence(left, right, path, sensitivity)
|
return preparePairedDiffSequence(left, right, path, sensitivity)
|
||||||
default:
|
default:
|
||||||
if sensitivity.hasStrictDescendant(path.String()) {
|
|
||||||
return pairedDiffSnapshot{requiresFallback: true}, true
|
|
||||||
}
|
|
||||||
return pairedDiffSnapshot{left: left, right: right}, true
|
return pairedDiffSnapshot{left: left, right: right}, true
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -180,6 +180,13 @@ func TestPreparePairedDiffSnapshotMasksSensitiveStructuralReplacements(t *testin
|
|||||||
path: Keys("changed"),
|
path: Keys("changed"),
|
||||||
selector: "/changed/token",
|
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",
|
name: "scalar to mapping",
|
||||||
left: "changed: old-scalar\n",
|
left: "changed: old-scalar\n",
|
||||||
@@ -241,7 +248,7 @@ func TestPreparePairedDiffSnapshotMasksOriginalRegionForNestedSequenceMismatch(t
|
|||||||
assertWholeRegionFallback(t, paired, supported, left, right)
|
assertWholeRegionFallback(t, paired, supported, left, right)
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestPreparePairedDiffSnapshotUsesHistoricalDescendantClassification(t *testing.T) {
|
func TestPreparePairedDiffSnapshotKeepsPublicScalarsWithHistoricalDescendants(t *testing.T) {
|
||||||
leftRoot := parseCompositionDocument(t, "left", "changed: before\n")
|
leftRoot := parseCompositionDocument(t, "left", "changed: before\n")
|
||||||
rightRoot := parseCompositionDocument(t, "right", "changed: after\n")
|
rightRoot := parseCompositionDocument(t, "right", "changed: after\n")
|
||||||
left := mappingValue(t, leftRoot, "changed")
|
left := mappingValue(t, leftRoot, "changed")
|
||||||
@@ -249,18 +256,22 @@ func TestPreparePairedDiffSnapshotUsesHistoricalDescendantClassification(t *test
|
|||||||
paired, supported := preparePairedDiffSnapshot(
|
paired, supported := preparePairedDiffSnapshot(
|
||||||
left, right, Keys("changed"), newDiffSensitivity(map[string]bool{"/changed/removed/secret": true}, nil),
|
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")
|
right := parseCompositionDocument(t, "right", "branch: scalar\n")
|
||||||
paired, supported := preparePairedDiffSnapshot(
|
paired, supported := preparePairedDiffSnapshot(
|
||||||
nil, right, Keys("added"), newDiffSensitivity(nil, map[string]bool{"/added/branch/secret": true}),
|
nil, right, Keys("added"), newDiffSensitivity(nil, map[string]bool{"/added/branch/secret": true}),
|
||||||
)
|
)
|
||||||
if !supported || paired.left != nil || paired.leftRedacted || !paired.rightRedacted {
|
if !supported || paired.left != nil || paired.leftRedacted || paired.rightRedacted || paired.hiddenChanged {
|
||||||
t.Fatalf("fallback presence/redaction = %#v, supported=%v", paired, supported)
|
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) {
|
func TestPreparePairedDiffSnapshotPreservesPublicReplacementBesideSensitiveSibling(t *testing.T) {
|
||||||
|
|||||||
Reference in New Issue
Block a user