refactor(models): consolidate group-role topology equality

pkg/stereopair's sameRoles and pkg/service/datastore's
sameGroupGenerationTopology independently reimplemented the same
Role-keyed topology comparison, but normalized IP addresses
differently (net.ParseIP-only vs. plain string equality) -- exactly
the class of disagreement models.SameGroup was already created to fix
for order-sensitivity. Both now delegate their per-role comparison to
a new models.SameGroupRoles, which treats equal-but-differently-
formatted IPs as a match without regressing the common
both-addresses-unset case either implementation relied on.
models.SameGroup and datastore's sameStereoPair stay distinct
(commented why): both are intentionally ID/IP-agnostic for reasons
unrelated to this consolidation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

pkg/models/group.go

@@ -2,6 +2,7 @@ package models
 
 import (
 	"encoding/xml"
+	"net"
 	"strings"
 )
 
@@ -36,6 +37,61 @@ type GroupRole struct {
 	IPAddress string `xml:"ipAddress,omitempty"`
 }
 
+// SameGroupRoles reports whether two role slices describe the same stereo
+// pair topology: matching length, no duplicate Role value on either side, and
+// every role paired by Role with equal DeviceID and IPAddress. It tolerates
+// any role count rather than assuming exactly LEFT/RIGHT.
+//
+// This is the shared core behind both pkg/stereopair's and
+// pkg/service/datastore's topology-equality checks -- they used to compare
+// IPAddress independently (one via net.ParseIP, one via plain string
+// equality), which could disagree about whether two differently-formatted
+// but equal addresses matched. See i655 code-review finding #10.
+func SameGroupRoles(a, b []GroupRole) bool {
+	if len(a) != len(b) {
+		return false
+	}
+
+	byRole := make(map[string]GroupRole, len(a))
+	for _, role := range a {
+		if _, duplicate := byRole[role.Role]; duplicate {
+			return false
+		}
+
+		byRole[role.Role] = role
+	}
+
+	seen := make(map[string]struct{}, len(b))
+	for _, role := range b {
+		if _, duplicate := seen[role.Role]; duplicate {
+			return false
+		}
+
+		seen[role.Role] = struct{}{}
+
+		other, ok := byRole[role.Role]
+		if !ok || other.DeviceID != role.DeviceID || !sameRoleIPAddress(other.IPAddress, role.IPAddress) {
+			return false
+		}
+	}
+
+	return true
+}
+
+// sameRoleIPAddress treats identical strings (including two empty/unset
+// addresses) as equal, and otherwise falls back to parsed-IP equality so two
+// differently-formatted representations of the same address still match. It
+// never treats one populated and one empty/unparsable address as a match.
+func sameRoleIPAddress(a, b string) bool {
+	if a == b {
+		return true
+	}
+
+	parsedA, parsedB := net.ParseIP(a), net.ParseIP(b)
+
+	return parsedA != nil && parsedB != nil && parsedA.Equal(parsedB)
+}
+
 // SameGroup reports whether left and right describe the same stereo-pair
 // configuration, comparing role assignments by device ID rather than by
 // slice order. The device's own /getGroup response and its groupUpdated
@@ -45,6 +101,11 @@ type GroupRole struct {
 // reflect.DeepEqual (order-sensitive) would then report a spurious change
 // even though nothing about the pair actually changed. Two nil Groups are
 // equal; exactly one nil is not.
+//
+// SameGroup stays a distinct, IP-agnostic implementation from
+// SameGroupRoles: its callers (event/status projection) need
+// order-independence without caring about IP, and adding an IP check here
+// would change that contract.
 func SameGroup(left, right *Group) bool {
 	if left == nil && right == nil {
 		return true

pkg/models/group_test.go

@@ -0,0 +1,90 @@
+package models
+
+import "testing"
+
+func TestSameGroupRoles(t *testing.T) {
+	base := []GroupRole{
+		{DeviceID: "LEFT-ID", Role: "LEFT", IPAddress: "192.0.2.10"},
+		{DeviceID: "RIGHT-ID", Role: "RIGHT", IPAddress: "192.0.2.11"},
+	}
+
+	t.Run("identical roles match", func(t *testing.T) {
+		if !SameGroupRoles(base, append([]GroupRole(nil), base...)) {
+			t.Fatal("identical roles reported as different")
+		}
+	})
+
+	t.Run("role order does not matter", func(t *testing.T) {
+		reordered := []GroupRole{base[1], base[0]}
+		if !SameGroupRoles(base, reordered) {
+			t.Fatal("reordered roles reported as different")
+		}
+	})
+
+	t.Run("differently formatted equal IP matches", func(t *testing.T) {
+		other := append([]GroupRole(nil), base...)
+		other[0].IPAddress = "::ffff:192.0.2.10" // IPv4-mapped IPv6 form of the same address
+		if !SameGroupRoles(base, other) {
+			t.Fatal("differently-formatted equal IP addresses reported as different")
+		}
+	})
+
+	t.Run("both empty IP addresses match", func(t *testing.T) {
+		noIP := []GroupRole{
+			{DeviceID: "LEFT-ID", Role: "LEFT"},
+			{DeviceID: "RIGHT-ID", Role: "RIGHT"},
+		}
+		if !SameGroupRoles(noIP, append([]GroupRole(nil), noIP...)) {
+			t.Fatal("two roles with unset IP addresses reported as different")
+		}
+	})
+
+	t.Run("different IP does not match", func(t *testing.T) {
+		other := append([]GroupRole(nil), base...)
+		other[0].IPAddress = "198.51.100.10"
+		if SameGroupRoles(base, other) {
+			t.Fatal("different IP addresses reported as same")
+		}
+	})
+
+	t.Run("populated vs empty IP does not match", func(t *testing.T) {
+		other := append([]GroupRole(nil), base...)
+		other[0].IPAddress = ""
+		if SameGroupRoles(base, other) {
+			t.Fatal("populated vs empty IP address reported as same")
+		}
+	})
+
+	t.Run("different DeviceID does not match", func(t *testing.T) {
+		other := append([]GroupRole(nil), base...)
+		other[0].DeviceID = "OTHER-ID"
+		if SameGroupRoles(base, other) {
+			t.Fatal("different DeviceID reported as same")
+		}
+	})
+
+	t.Run("different Role does not match", func(t *testing.T) {
+		other := append([]GroupRole(nil), base...)
+		other[0].Role = "RIGHT"
+		if SameGroupRoles(base, other) {
+			t.Fatal("mismatched Role reported as same")
+		}
+	})
+
+	t.Run("different length does not match", func(t *testing.T) {
+		if SameGroupRoles(base, base[:1]) {
+			t.Fatal("different-length role slices reported as same")
+		}
+	})
+
+	t.Run("duplicate role on either side does not match", func(t *testing.T) {
+		duplicateA := []GroupRole{base[0], base[0]}
+		duplicateB := []GroupRole{base[1], base[1]}
+		if SameGroupRoles(duplicateA, base) {
+			t.Fatal("duplicate role in first argument reported as same")
+		}
+		if SameGroupRoles(base, duplicateB) {
+			t.Fatal("duplicate role in second argument reported as same")
+		}
+	})
+}

pkg/service/datastore/datastore.go

@@ -3422,6 +3422,13 @@ func stereoRoleDevices(group *models.Group) (left, right string, ok bool) {
 	return left, right, left != "" && right != ""
 }
 
+// sameStereoPair stays a distinct, ID-agnostic and IP-agnostic
+// implementation from sameGroupGenerationTopology: AddGroup's idempotency
+// reuse check needs to recognize a retried create (which supplies no
+// pre-existing group ID and may carry a since-changed IP) as "the same
+// pair," not just an exact topology match. See i655 code-review finding #10
+// (and the separately-tracked finding #4 about this reuse potentially
+// returning a stale IP).
 func sameStereoPair(a, b *models.Group) bool {
 	if len(a.Roles.Roles) != 2 || len(b.Roles.Roles) != 2 || a.MasterDeviceID != b.MasterDeviceID {
 		return false
@@ -3433,38 +3440,16 @@ func sameStereoPair(a, b *models.Group) bool {
 	return aOK && bOK && aLeft == bLeft && aRight == bRight
 }
 
+// sameGroupGenerationTopology delegates its per-role comparison to
+// models.SameGroupRoles, the shared topology-equality core also used by
+// pkg/stereopair -- see i655 code-review finding #10 (three independent,
+// subtly different implementations used to coexist).
 func sameGroupGenerationTopology(a, b *models.Group) bool {
-	if a == nil || b == nil || a.ID != b.ID ||
-		a.MasterDeviceID != b.MasterDeviceID || len(a.Roles.Roles) != len(b.Roles.Roles) {
+	if a == nil || b == nil || a.ID != b.ID || a.MasterDeviceID != b.MasterDeviceID {
 		return false
 	}
 
-	roles := make(map[string]models.GroupRole, len(a.Roles.Roles))
-	for i := range a.Roles.Roles {
-		role := a.Roles.Roles[i]
-		if _, duplicate := roles[role.Role]; duplicate {
-			return false
-		}
-
-		roles[role.Role] = role
-	}
-
-	seen := make(map[string]struct{}, len(b.Roles.Roles))
-	for i := range b.Roles.Roles {
-		role := b.Roles.Roles[i]
-		if _, duplicate := seen[role.Role]; duplicate {
-			return false
-		}
-
-		seen[role.Role] = struct{}{}
-
-		other, found := roles[role.Role]
-		if !found || other.DeviceID != role.DeviceID || other.IPAddress != role.IPAddress {
-			return false
-		}
-	}
-
-	return true
+	return models.SameGroupRoles(a.Roles.Roles, b.Roles.Roles)
 }
 
 func groupDeviceIDs(group *models.Group) map[string]struct{} {

pkg/stereopair/coordinator.go

@@ -1502,24 +1502,11 @@ func sameGroupTopology(a, b *models.Group) bool {
 		sameRoles(a.Roles.Roles, b.Roles.Roles)
 }
 
+// sameRoles delegates to models.SameGroupRoles, the shared topology-equality
+// core also used by pkg/service/datastore -- see i655 code-review finding
+// #10 (three independent, subtly different implementations used to coexist).
 func sameRoles(a, b []models.GroupRole) bool {
-	if len(a) != len(b) {
-		return false
-	}
-
-	byRole := make(map[string]models.GroupRole, len(a))
-	for i := range a {
-		byRole[a[i].Role] = a[i]
-	}
-
-	for i := range b {
-		other, ok := byRole[b[i].Role]
-		if !ok || other.DeviceID != b[i].DeviceID || !sameIP(other.IPAddress, b[i].IPAddress) {
-			return false
-		}
-	}
-
-	return true
+	return models.SameGroupRoles(a, b)
 }
 
 func groupContainsDevice(group *models.Group, deviceID string) bool {