merge backlog: worktree-agent-a0d4f6f75e1f8f7cd

internal/diff/diff.go

@@ -37,6 +37,44 @@ func (c Changeset) Actionable() []RecordDiff {
 	return out
 }
 
+// Updates returns managed (non-ReadOnly) diffs that add or bring a record in
+// line with the template (Kind == Add or Kind == Update). It never contains
+// Delete diffs, so it is safe for callers to apply automatically without
+// risking data loss from an incomplete or empty template.
+func (c Changeset) Updates() []RecordDiff {
+	var out []RecordDiff
+	for _, d := range c.Diffs {
+		if d.ReadOnly {
+			continue
+		}
+		if d.Kind == Add || d.Kind == Update {
+			out = append(out, d)
+		}
+	}
+	return out
+}
+
+// Prunes returns managed (non-ReadOnly) diffs where a record exists in the
+// zone but is absent from the template (Kind == Delete). These are
+// potentially destructive: an incomplete or empty template would surface
+// every managed zone record here. Callers should require explicit
+// confirmation before applying Prunes(), unlike Updates().
+//
+// Updates() and Prunes() partition Actionable(): every diff in Actionable()
+// appears in exactly one of the two.
+func (c Changeset) Prunes() []RecordDiff {
+	var out []RecordDiff
+	for _, d := range c.Diffs {
+		if d.ReadOnly {
+			continue
+		}
+		if d.Kind == Delete {
+			out = append(out, d)
+		}
+	}
+	return out
+}
+
 // Diff compares a template against the actual zone records.
 // Records present in the zone but absent from the template yield Delete.
 func Diff(template, actual []model.Record) Changeset {
@@ -70,6 +108,14 @@ func Diff(template, actual []model.Record) Changeset {
 	return Changeset{Diffs: diffs}
 }
 
+// index builds a lookup of records by Key(). If two records in recs share
+// the same Key() (e.g. a provider returning a duplicate RRset entry), this
+// is a deliberate, documented choice, not an oversight: the LAST record with
+// that Key() wins, since later map assignments overwrite earlier ones for
+// the same key during the loop below. Diff() only ever sees the winning
+// (last) record for a duplicated key; earlier duplicates are silently
+// collapsed. See TestIndexDedupLastWriteWins in diff_test.go, which pins
+// this behavior down.
 func index(recs []model.Record) map[string]model.Record {
 	m := make(map[string]model.Record, len(recs))
 	for _, r := range recs {

internal/diff/diff_test.go

@@ -113,3 +113,122 @@ func TestActionableExcludesInSyncAndReadOnly(t *testing.T) {
 		t.Fatalf("only b.example.com. is actionable, got %+v", act)
 	}
 }
+
+// Prune-guard: with an empty/nil template every managed zone record surfaces
+// as a Delete. Prunes() must isolate exactly those (potentially destructive)
+// entries, while Updates() must stay empty — there is nothing safe to apply.
+func TestPrunesEmptyTemplateAllManagedNoneInUpdates(t *testing.T) {
+	actual := []model.Record{
+		{Type: model.A, Name: "a.example.com.", TTL: 300, Values: []string{"1.1.1.1"}},
+		{Type: model.A, Name: "b.example.com.", TTL: 300, Values: []string{"2.2.2.2"}},
+		{Type: model.NS, Name: "example.com.", TTL: 3600, Values: []string{"ns1.example.com."}},
+	}
+	cs := Diff(nil, actual)
+
+	prunes := cs.Prunes()
+	if len(prunes) != 2 {
+		t.Fatalf("expected 2 managed prunes (A records only), got %d: %+v", len(prunes), prunes)
+	}
+	for _, d := range prunes {
+		if d.Kind != Delete {
+			t.Fatalf("Prunes() must only contain Delete diffs, got %+v", d)
+		}
+		if d.ReadOnly {
+			t.Fatalf("Prunes() must exclude ReadOnly diffs, got %+v", d)
+		}
+		if d.Type == model.NS {
+			t.Fatalf("NS must be excluded from Prunes(), got %+v", d)
+		}
+	}
+
+	updates := cs.Updates()
+	if len(updates) != 0 {
+		t.Fatalf("expected 0 updates for an empty template, got %d: %+v", len(updates), updates)
+	}
+}
+
+// Mixed case: Add/Update land in Updates(), Delete lands in Prunes(), and the
+// ReadOnly NS diff (here an Add) is excluded from both — mirroring Actionable().
+func TestUpdatesAndPrunesMixedSeparation(t *testing.T) {
+	tmpl := []model.Record{
+		{Type: model.A, Name: "a.example.com.", TTL: 300, Values: []string{"1.1.1.1"}}, // in sync
+		{Type: model.A, Name: "b.example.com.", TTL: 300, Values: []string{"9.9.9.9"}}, // update
+		{Type: model.A, Name: "c.example.com.", TTL: 300, Values: []string{"3.3.3.3"}}, // add
+		{Type: model.NS, Name: "example.com.", TTL: 3600, Values: []string{"ns1.example.com."}}, // read-only add
+	}
+	actual := []model.Record{
+		{Type: model.A, Name: "a.example.com.", TTL: 300, Values: []string{"1.1.1.1"}},
+		{Type: model.A, Name: "b.example.com.", TTL: 300, Values: []string{"2.2.2.2"}},
+		{Type: model.A, Name: "d.example.com.", TTL: 300, Values: []string{"4.4.4.4"}}, // delete (extra)
+	}
+	cs := Diff(tmpl, actual)
+
+	updates := cs.Updates()
+	if len(updates) != 2 {
+		t.Fatalf("expected 2 updates (b update + c add), got %d: %+v", len(updates), updates)
+	}
+	for _, d := range updates {
+		if d.Kind != Add && d.Kind != Update {
+			t.Fatalf("Updates() must only contain Add/Update diffs, got %+v", d)
+		}
+		if d.ReadOnly {
+			t.Fatalf("Updates() must exclude ReadOnly diffs, got %+v", d)
+		}
+	}
+
+	prunes := cs.Prunes()
+	if len(prunes) != 1 || prunes[0].Name != "d.example.com." {
+		t.Fatalf("expected exactly 1 prune (d.example.com.), got %+v", prunes)
+	}
+	if prunes[0].ReadOnly {
+		t.Fatalf("prune must not be ReadOnly, got %+v", prunes[0])
+	}
+
+	// Updates ∪ Prunes must equal Actionable, with no overlap.
+	act := cs.Actionable()
+	if len(act) != len(updates)+len(prunes) {
+		t.Fatalf("Updates()+Prunes() must partition Actionable(): actionable=%d updates=%d prunes=%d",
+			len(act), len(updates), len(prunes))
+	}
+}
+
+// Dedup semantics: index() is keyed by model.Record.Key(). If the same Key()
+// appears twice within a single slice (e.g. actual records fetched from a
+// provider with a duplicate RRset entry), the map assignment in the loop
+// means the LAST occurrence wins and earlier ones are discarded silently.
+// This test fixes that behavior in place so it is a documented, intentional
+// choice rather than an accidental artifact of map iteration order.
+func TestIndexDedupLastWriteWins(t *testing.T) {
+	actual := []model.Record{
+		{Type: model.A, Name: "dup.example.com.", TTL: 300, Values: []string{"1.1.1.1"}},
+		{Type: model.A, Name: "dup.example.com.", TTL: 300, Values: []string{"9.9.9.9"}}, // same Key(), later wins
+	}
+	tmpl := []model.Record{
+		{Type: model.A, Name: "dup.example.com.", TTL: 300, Values: []string{"9.9.9.9"}},
+	}
+	cs := Diff(tmpl, actual)
+
+	// Only one diff should exist for the duplicated key (dedup collapsed it),
+	// and it must reflect the LAST actual record ("9.9.9.9"), matching the
+	// template value, hence InSync rather than Update.
+	d := find(cs, "A dup.example.com.")
+	if d == nil {
+		t.Fatalf("expected a diff for dup.example.com.")
+	}
+	if d.Kind != InSync {
+		t.Fatalf("expected InSync (last actual record wins in index()), got %+v", d)
+	}
+	if d.Actual == nil || len(d.Actual.Values) != 1 || d.Actual.Values[0] != "9.9.9.9" {
+		t.Fatalf("expected Actual to be the last duplicate (9.9.9.9), got %+v", d.Actual)
+	}
+
+	count := 0
+	for _, dd := range cs.Diffs {
+		if dd.Type == model.A && dd.Name == "dup.example.com." {
+			count++
+		}
+	}
+	if count != 1 {
+		t.Fatalf("expected exactly 1 diff for the duplicated key, got %d", count)
+	}
+}