From 42246fd6b747c33f1322a55df6aa37a52e5c0311 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 08:19:15 -0400 Subject: [PATCH 1/2] Undo reported "nothing left to undo" after a successful undo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by running the real thing: the agent replaced a bed of garlic with cucumbers, Undo restored the garlic correctly — and then said "Nothing left to undo — this was already reversed." The revert response carries the change set commitScope just inserted, and that row has no per-op tally: Counts is populated by the history LIST query, not by the insert. describeUndo treated a zero tally as "nothing happened", so a real undo with eight revisions behind it reported itself as a no-op. Telling someone an action did nothing when it just worked is about the worst thing a safety feature can say, because the obvious next move is to do it again. Fixed on both sides. The client now treats a NULL change set as the no-op signal, which is what the server actually means by it — an empty tally is not the same thing and must not be read as one. And RevertChangeSet now fills in the counts, so the revert response and the list read describe the same change set the same way, and a partial revert can say "2 of 3" from either. The unit test missed it because the fixture I wrote populated counts, so it was testing my assumption about the response rather than its actual shape. The new tests pin the real thing: the service asserts a revert's counts match the fill it undid AND match what the list read reports, and the client asserts an empty tally still reads as "Undone." Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/service/revisions.go | 36 ++++++++++++++++++++ internal/service/revisions_test.go | 54 ++++++++++++++++++++++++++++++ web/src/lib/history.test.ts | 9 +++++ web/src/lib/history.ts | 15 +++++---- 4 files changed, 108 insertions(+), 6 deletions(-) diff --git a/internal/service/revisions.go b/internal/service/revisions.go index fc373c9..271b7f7 100644 --- a/internal/service/revisions.go +++ b/internal/service/revisions.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "sort" "sync" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" @@ -334,9 +335,44 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6 if err != nil { return nil, nil, err } + // Fill in the per-op counts. commitScope returns the freshly inserted row, + // which carries no tally — and a caller receiving a change set with an empty + // Counts cannot tell "nothing was reverted" from "the tally wasn't loaded". + // That ambiguity is not theoretical: it made the chat panel report a + // successful undo as "nothing left to undo". + if cs != nil { + cs.Counts = countRevisions(sc.taken()) + } return cs, conflicts, nil } +// countRevisions tallies revisions by (entity type, op), matching what the +// history list read produces so both sides of the API describe a change set the +// same way. +func countRevisions(revs []domain.Revision) []domain.ChangeCount { + type key struct{ entityType, op string } + seen := map[key]int{} + order := []key{} + for _, r := range revs { + k := key{r.EntityType, r.Op} + if _, ok := seen[k]; !ok { + order = append(order, k) + } + seen[k]++ + } + sort.Slice(order, func(i, j int) bool { + if order[i].entityType != order[j].entityType { + return order[i].entityType < order[j].entityType + } + return order[i].op < order[j].op + }) + counts := make([]domain.ChangeCount, 0, len(order)) + for _, k := range order { + counts = append(counts, domain.ChangeCount{EntityType: k.entityType, Op: k.op, N: seen[k]}) + } + return counts +} + // entityKey identifies a row across the entity types a revision can name. type entityKey struct { entityType string diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go index 2963b47..8fd8fe9 100644 --- a/internal/service/revisions_test.go +++ b/internal/service/revisions_test.go @@ -704,3 +704,57 @@ func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) { t.Errorf("cleared %d of %d with %d revisions — all three should match", n, len(before), len(revs)) } } + +// TestRevertResultCarriesItsCounts — found by using the thing. +// +// commitScope returns the freshly inserted row, which carries no tally. A caller +// receiving a change set with empty Counts cannot tell "nothing was reverted" +// 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. +func TestRevertResultCarriesItsCounts(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + plant := seedOwnPlant(t, s, owner, 15) + ctx := context.Background() + + 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] + planted := totalOf(fill.Counts) + if planted == 0 { + t.Fatal("the fill recorded no counts") + } + + undo, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI) + if err != nil || len(conflicts) != 0 { + t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) + } + if undo == nil { + t.Fatal("a revert that did work returned no change set") + } + 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 + // the API describe the same change set the same way. + listed := history(t, s, owner, g.ID)[0] + if listed.ID != undo.ID { + t.Fatalf("newest change set is %d, want the revert %d", listed.ID, undo.ID) + } + if totalOf(listed.Counts) != totalOf(undo.Counts) { + t.Errorf("list says %d changes, revert said %d", totalOf(listed.Counts), totalOf(undo.Counts)) + } +} + +func totalOf(counts []domain.ChangeCount) int { + n := 0 + for _, c := range counts { + n += c.N + } + return n +} diff --git a/web/src/lib/history.test.ts b/web/src/lib/history.test.ts index 83176f7..b32f1f1 100644 --- a/web/src/lib/history.test.ts +++ b/web/src/lib/history.test.ts @@ -49,6 +49,14 @@ describe('describeConflict', () => { describe('describeUndo', () => { const target = changeSet({ counts: [{ entityType: 'object', op: 'update', n: 3 }] }) + // The bug this pair exists for: a real revert response carries a change set + // whose counts the server didn't populate. Reading that as "nothing happened" + // told the user a successful undo had done nothing. + it('trusts the change set, not the tally, for whether anything happened', () => { + const out = describeUndo({ id: 5 }, { changeSet: changeSet({ id: 6, counts: [] }), conflicts: [] }) + expect(out).toEqual({ tone: 'ok', message: 'Undone.' }) + }) + it('is a plain success when nothing conflicted', () => { const out = describeUndo(target, { changeSet: changeSet({ id: 2, counts: [{ entityType: 'object', op: 'update', n: 3 }] }), @@ -61,6 +69,7 @@ describe('describeUndo', () => { // already a no-op — reachable by undoing a creation whose object is gone. // Claiming "Undone." there reports work that didn't happen. it("doesn't claim to have undone a no-op", () => { + // A NULL change set — not an empty tally — is the server's no-op signal. const out = describeUndo(target, { changeSet: null, conflicts: [] }) expect(out.tone).toBe('ok') expect(out.message).toBe('Nothing left to undo — this was already reversed.') diff --git a/web/src/lib/history.ts b/web/src/lib/history.ts index c86d9f4..87df177 100644 --- a/web/src/lib/history.ts +++ b/web/src/lib/history.ts @@ -224,20 +224,23 @@ export interface UndoOutcome { */ export function describeUndo(target: UndoTarget, result: RevertResult): UndoOutcome { const skipped = result.conflicts.map(describeConflict).join('; ') + // A NULL change set is the server's signal that nothing needed doing. An empty + // `counts` is not the same thing and must not be read as one — that conflation + // made a successful undo report "nothing left to undo", which is the worst + // possible thing to tell someone about an action that just worked. + const didSomething = result.changeSet != null const applied = result.changeSet ? totalChanges(result.changeSet) : 0 if (result.conflicts.length === 0) { - // The server answers 200 with a null change set when every revision was - // already a no-op — reachable by undoing a creation whose object is gone. - // Claiming "Undone." there would be reporting work that didn't happen. - if (applied === 0) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' } + // Reachable by undoing a creation whose object is already gone. + if (!didSomething) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' } return { tone: 'ok', message: 'Undone.' } } - if (applied === 0) { + if (!didSomething) { return { tone: 'error', message: `Nothing was undone — ${skipped}.` } } // Only claim a denominator when we have one. "2 of 3" from a caller that // never knew the total would be a number invented to fill a sentence. const total = totalChanges(target) - const scale = total > 0 ? `${applied} of ${total} changes undone` : 'Partly undone' + const scale = total > 0 && applied > 0 ? `${applied} of ${total} changes undone` : 'Partly undone' return { tone: 'partial', message: `${scale} — ${skipped}.` } } -- 2.54.0 From 8a0f3678040205319f203382731d4abfa3714567 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 08:25:00 -0400 Subject: [PATCH 2/2] Make the Go/SQL tally parity a tested contract, not a hand-maintained one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gadfly's point stands: countRevisions reproduces in Go what ListChangeSets does in SQL, and nothing was holding the two together. Its sibling finding is the same problem seen from the test side — the test compared only TOTALS, which would agree even if the two groupings had diverged completely. Both ends now name the contract, and the test compares the full per-(entity, op) breakdown row for row, across two reverts of different shapes so there is more than one row to get wrong. That turns "someone will remember to keep these in step" into something CI notices. The duplication itself stays. A change set that has just been written has no rows to GROUP BY yet, so the alternative to counting in Go is a second round trip to count what we are already holding. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/service/revisions.go | 10 ++-- internal/service/revisions_test.go | 85 +++++++++++++++++++++++------- internal/store/revisions.go | 4 +- 3 files changed, 76 insertions(+), 23 deletions(-) diff --git a/internal/service/revisions.go b/internal/service/revisions.go index 271b7f7..a0ae39b 100644 --- a/internal/service/revisions.go +++ b/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 -// history list read produces so both sides of the API describe a change set the -// same way. +// countRevisions tallies revisions by (entity type, op). +// +// 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{} diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go index 8fd8fe9..fecd22f 100644 --- a/internal/service/revisions_test.go +++ b/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, "a@example.com") @@ -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] - planted := totalOf(fill.Counts) - if planted == 0 { - t.Fatal("the fill recorded no counts") + // A second, different kind of change, so the breakdown has more than one row + // to get wrong. + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("North Bed")}, bed.Version); err != nil { + 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 { - t.Fatal("a revert that did work returned no change set") - } - if got := totalOf(undo.Counts); got != planted { - t.Errorf("revert reported %d changes, want %d — an empty tally reads as 'nothing happened'", got, planted) + undoFill, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI) + if err != nil || len(conflicts) != 0 { + t.Fatalf("revert fill: err=%v conflicts=%+v", err, conflicts) } - // And the list read agrees with what the revert returned, so both halves of - // the API describe the same change set the same way. - listed := history(t, s, owner, g.ID)[0] - if listed.ID != undo.ID { - t.Fatalf("newest change set is %d, want the revert %d", listed.ID, undo.ID) + for _, undo := range []*domain.ChangeSet{undoRename, undoFill} { + if undo == nil { + t.Fatal("a revert that did work returned no change set") + } + 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) { - t.Errorf("list says %d changes, revert said %d", totalOf(listed.Counts), totalOf(undo.Counts)) + // Undoing a fill deletes every plop it created, so the tallies must match. + 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 diff --git a/internal/store/revisions.go b/internal/store/revisions.go index 669ebab..d696710 100644 --- a/internal/store/revisions.go +++ b/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 -- 2.54.0