Undo reported "nothing left to undo" after a successful undo
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) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
This commit is contained in:
@@ -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, "[email protected]")
|
||||
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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user