internal/service/revisions.go

@@ -346,9 +346,13 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6
 	return cs, conflicts, nil
 }
 
-// countRevisions tallies revisions by (entity type, op), matching what the
+// countRevisions tallies revisions by (entity type, op).
-// history list read produces so both sides of the API describe a change set the
+//
-// same way.
+// This deliberately reproduces in Go what ListChangeSets does in SQL, because a
+// change set that has just been written has no rows to GROUP BY yet — the
+// alternative is a second round trip to count what we are already holding.
+// The parity is a real cross-layer contract, and TestRevertResultCarriesItsCounts
+// compares the two breakdowns row for row so it can't drift silently.
 func countRevisions(revs []domain.Revision) []domain.ChangeCount {
 	type key struct{ entityType, op string }
 	seen := map[key]int{}

internal/service/revisions_test.go

@@ -712,6 +712,11 @@ func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) {
 // from "the tally wasn't loaded", and the chat panel read that ambiguity the
 // wrong way: a successful undo reported "nothing left to undo", which is the
 // worst possible thing to say about an action that just worked.
+//
+// It also pins the cross-layer contract that fix created. countRevisions tallies
+// in Go what ListChangeSets tallies in SQL, and nothing but this test stops the
+// two drifting — so it compares the FULL per-(entity, op) breakdown, not just
+// the totals, which would agree even if the groupings had diverged.
 func TestRevertResultCarriesItsCounts(t *testing.T) {
 	s := newTestService(t, openConfig())
 	owner := seedUser(t, s, "[email protected]")
@@ -723,35 +728,77 @@ func TestRevertResultCarriesItsCounts(t *testing.T) {
 	if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil {
 		t.Fatalf("fill: %v", err)
 	}
-	fill := history(t, s, owner, g.ID)[0]
+	// A second, different kind of change, so the breakdown has more than one row
-	planted := totalOf(fill.Counts)
+	// to get wrong.
-	if planted == 0 {
+	if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("North Bed")}, bed.Version); err != nil {
-		t.Fatal("the fill recorded no counts")
+		t.Fatalf("rename: %v", err)
 	}
+	sets := history(t, s, owner, g.ID)
+	rename, fill := sets[0], sets[1]
 
-	undo, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI)
+	undoRename, conflicts, err := s.RevertChangeSet(ctx, owner, rename.ID, domain.SourceUI)
 	if err != nil || len(conflicts) != 0 {
-		t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts)
+		t.Fatalf("revert rename: err=%v conflicts=%+v", err, conflicts)
 	}
-	if undo == nil {
+	undoFill, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI)
-		t.Fatal("a revert that did work returned no change set")
+	if err != nil || len(conflicts) != 0 {
-	}
+		t.Fatalf("revert fill: err=%v conflicts=%+v", err, conflicts)
-	if got := totalOf(undo.Counts); got != planted {
-		t.Errorf("revert reported %d changes, want %d — an empty tally reads as 'nothing happened'", got, planted)
 	}
 
-	// And the list read agrees with what the revert returned, so both halves of
+	for _, undo := range []*domain.ChangeSet{undoRename, undoFill} {
-	// the API describe the same change set the same way.
+		if undo == nil {
-	listed := history(t, s, owner, g.ID)[0]
+			t.Fatal("a revert that did work returned no change set")
-	if listed.ID != undo.ID {
+		}
-		t.Fatalf("newest change set is %d, want the revert %d", listed.ID, undo.ID)
+		if len(undo.Counts) == 0 {
+			t.Fatalf("change set %d came back with no tally — an empty tally reads as 'nothing happened'", undo.ID)
+		}
 	}
-	if totalOf(listed.Counts) != totalOf(undo.Counts) {
+	// Undoing a fill deletes every plop it created, so the tallies must match.
-		t.Errorf("list says %d changes, revert said %d", totalOf(listed.Counts), totalOf(undo.Counts))
+	if got, want := countsTotal(undoFill.Counts), countsTotal(fill.Counts); got != want {
+		t.Errorf("undo of the fill reports %d changes, want %d", got, want)
+	}
+
+	// The SQL tally and the Go tally must agree, row for row.
+	listed := history(t, s, owner, g.ID)
+	byID := map[int64][]domain.ChangeCount{}
+	for _, cs := range listed {
+		byID[cs.ID] = cs.Counts
+	}
+	for _, undo := range []*domain.ChangeSet{undoRename, undoFill} {
+		fromSQL, ok := byID[undo.ID]
+		if !ok {
+			t.Fatalf("revert %d is missing from the history list", undo.ID)
+		}
+		if !sameCounts(undo.Counts, fromSQL) {
+			t.Errorf("change set %d: revert returned %+v, the list read says %+v — countRevisions and the SQL grouping have drifted",
+				undo.ID, undo.Counts, fromSQL)
+		}
 	}
 }
 
-func totalOf(counts []domain.ChangeCount) int {
+// sameCounts compares two tallies regardless of order.
+func sameCounts(a, b []domain.ChangeCount) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	index := func(cs []domain.ChangeCount) map[string]int {
+		m := map[string]int{}
+		for _, c := range cs {
+			m[c.EntityType+"/"+c.Op] = c.N
+		}
+		return m
+	}
+	ia, ib := index(a), index(b)
+	for k, v := range ia {
+		if ib[k] != v {
+			return false
+		}
+	}
+	return true
+}
+
+// countsTotal sums a tally — the server-side twin of the client's totalChanges.
+func countsTotal(counts []domain.ChangeCount) int {
 	n := 0
 	for _, c := range counts {
 		n += c.N

internal/store/revisions.go

@@ -119,7 +119,9 @@ func (d *DB) ListChangeSets(ctx context.Context, gardenID int64, limit, offset i
 		return sets, nil
 	}
 
-	// One grouped query for the whole page rather than N per-row counts.
+	// One grouped query for the whole page rather than N per-row counts. The
+	// service's countRevisions mirrors this grouping for change sets it has just
+	// written; TestRevertResultCarriesItsCounts holds the two in step.
 	countRows, err := d.sql.QueryContext(ctx,
 		`SELECT change_set_id, entity_type, op, COUNT(*)
 		   FROM revisions