Address Gadfly review on revision history

Six real bugs, three of which would have broken the feature's central promise.

The worst was that a failed operation threw away the history for changes that
had already committed. WithChangeSet buffers HISTORY, not data — the store
writes inside fn commit as they go — so discarding the buffer on error left real
mutations with no change set and no way to undo them. That is exactly the
situation undo exists for: an agent turn that did half a thing and then failed.
Now the buffer is written, the summary says the operation failed partway, and
the error is still returned. RevertChangeSet does the same for a revert that
fails mid-loop. The partial state is still partial, but it is visible and
undoable instead of orphaned.

versionGuard falsely conflicted whenever one change set held more than one
revision for the same entity — an agent turn that moves a bed and then renames
it. Each inverse bumps the row's version, so from the second one on the
snapshot's version no longer matched the live row and the revert flagged its own
work as somebody else's edit. RevertChangeSet now tracks what it has written and
the guard compares against that.

ListChangeSets found "was this reverted?" with a LEFT JOIN, which emits one
duplicate row per revert once a change set has been reverted more than once
(undo, redo, undo again). Now a scalar subquery.

Reverting an object creation cascade-deleted anything planted in that bed since,
quietly. The plops were snapshotted so it was recoverable, but deleting
someone's plants as a side effect of an unrelated undo should be reported. It
now conflicts and leaves the bed alone.

ClearObject cleared by predicate and snapshotted by a separate read, so a plop
created between the two was removed with no revision to undo it by. It now
clears exactly the ids it read. It also returned an error when only the
post-clear re-read failed, which told the caller a clear had failed after it had
already applied — inviting a retry of an applied operation. That path now logs
the history gap and reports success, matching how record() treats its own write
failures.

RestoreObject/RestorePlanting could lose the race between the existence check
and the insert and surface a raw constraint error. The revert now re-checks and
reports ConflictExists, which is what that condition means.

RevertChangeSet takes a source, so an agent undoing its own work is
distinguishable from a person clicking undo — the whole point of the badge.

Refactoring: the three revert bodies repeated a load/guard/unsnapshot/update
skeleton (flagged by 3 of 5 models). They are now one generic revertEntity over
a small per-type op table, so the ordering, guards and conflict reporting cannot
drift apart. The API's view structs duplicated domain types that already carried
every field, against the package's direct-JSON convention — dropped. intQuery
moved next to the other request helpers. planRevert's comment said three passes
where the code runs five. A revert where every revision is already a no-op now
answers 200 rather than 201 with a null change set.

Six new tests, one per bug.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

internal/service/revisions.go

@@ -122,11 +122,29 @@ func (s *Service) WithChangeSet(ctx context.Context, actorID, gardenID int64, op
 		source: opts.Source, summary: opts.Summary, agentRunID: opts.AgentRunID,
 	}
 	if err := fn(context.WithValue(ctx, changeSetKey{}, sc)); err != nil {
+		// fn failed, but the mutations it completed before failing are already
+		// committed — this scope buffers history, not data. Dropping the buffer
+		// would leave those changes with no history and no way to undo them,
+		// which is exactly the situation undo exists for. Record what happened,
+		// mark the summary, and still report the failure.
+		sc.summary = partialSummary(sc.summary)
+		if _, cerr := s.commitScope(ctx, sc, nil); cerr != nil {
+			slog.Error("service: partial turn could not be recorded", "error", cerr, "garden", gardenID)
+		}
 		return nil, err
 	}
 	return s.commitScope(ctx, sc, nil)
 }
 
+// partialSummary marks a change set whose operation failed partway, so the
+// history list can say so rather than presenting it as a completed action.
+func partialSummary(summary string) string {
+	if summary == "" {
+		return "Partly applied (the operation failed partway)"
+	}
+	return summary + " (failed partway)"
+}
+
 // commitScope writes a scope's buffered revisions as one change set. revertsID is
 // set only by RevertChangeSet. A scope with no revisions writes nothing — an
 // operation that changed nothing doesn't belong in history.
@@ -247,12 +265,20 @@ func (s *Service) GardenHistory(ctx context.Context, actorID, gardenID int64, li
 
 // RevertChangeSet undoes a change set by applying the inverse of each of its
 // revisions, inside one NEW change set that points back at the target — so the
-// undo is itself undoable. Requires editor role on the garden.
+// undo is itself undoable. Requires editor role on the garden. source says who
+// asked (the agent can undo its own work, and the history badge should show that).
 //
 // Entities that changed after the target change set are reported as conflicts and
 // left untouched; everything else still reverts. Returns the new change set (nil
-// when nothing could be reverted) alongside those conflicts.
-func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int64) (*domain.ChangeSet, []domain.RevertConflict, error) {
+// when nothing needed reverting) alongside those conflicts.
+//
+// The inverses are applied one row at a time rather than in a single
+// transaction, because the store's version-guarded update path is per-row. That
+// means a failure partway leaves the garden partly reverted — so whatever DID
+// apply is recorded before the error is returned, and the partial revert is
+// itself a change set you can undo. Losing the history for changes that really
+// happened would be strictly worse than the partial state.
+func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int64, source string) (*domain.ChangeSet, []domain.RevertConflict, error) {
 	target, err := s.store.GetChangeSet(ctx, changeSetID)
 	if err != nil {
 		return nil, nil, err
@@ -260,15 +286,30 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6
 	if _, err := s.requireGardenRole(ctx, actorID, target.GardenID, roleEditor); err != nil {
 		return nil, nil, err
 	}
+	if !validChangeSource(source) {
+		return nil, nil, domain.ErrInvalidInput
+	}
 
 	conflicts := []domain.RevertConflict{}
 	sc := &changeScope{
-		gardenID: target.GardenID, actorID: actorID, source: domain.SourceUI,
+		gardenID: target.GardenID, actorID: actorID, source: source,
 		summary: revertSummary(target),
 	}
+	// A change set may hold several revisions for the SAME entity (an agent turn
+	// that moves a bed and then renames it). Each inverse bumps that row's
+	// version, so from the second one on, the version in the snapshot no longer
+	// matches the live row — and a naive guard would call our own work a conflict.
+	// This tracks what we just wrote so the guard compares against reality.
+	applied := map[entityKey]int64{}
+
 	for _, r := range planRevert(target.Revisions) {
-		changes, conflict, err := s.applyInverse(ctx, r)
+		changes, conflict, err := s.applyInverse(ctx, r, applied)
 		if err != nil {
+			// Record what actually landed before surfacing the failure, so the
+			// partial revert is visible and undoable rather than orphaned.
+			if _, cerr := s.commitScope(ctx, sc, &target.ID); cerr != nil {
+				slog.Error("service: partial revert could not be recorded", "error", cerr, "changeSet", changeSetID)
+			}
 			return nil, nil, err
 		}
 		if conflict != nil {
@@ -289,15 +330,27 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6
 	return cs, conflicts, nil
 }
 
-// planRevert orders a change set's revisions for inverse application. Three
-// passes, because the inverses carry foreign-key dependencies the original
-// operations did not:
+// entityKey identifies a row across the entity types a revision can name.
+type entityKey struct {
+	entityType string
+	id         int64
+}
+
+func keyOf(r domain.Revision) entityKey {
+	return entityKey{entityType: r.EntityType, id: r.EntityID}
+}
+
+// planRevert orders a change set's revisions for inverse application, because the
+// inverses carry foreign-key dependencies the original operations did not. Five
+// passes, in this order:
 //
-//  1. restore deleted rows — parents (objects) BEFORE children (plantings), or a
-//     restored plop has no object to hang off and its FK rejects it;
+//  0. restore deleted OBJECTS — parents first, or a restored plop has no object
+//     to hang off and its FK rejects it;
+//  1. restore everything else deleted;
 //  2. undo updates;
-//  3. delete created rows — children before parents, so a delete never leans on
-//     the cascade to clean up rows this revert is responsible for.
+//  3. delete created non-objects (plantings) — children before parents;
+//  4. delete created OBJECTS last, so a delete never leans on the cascade to
+//     clean up rows this revert is itself responsible for.
 //
 // Reverse seq order within each pass: the last change made is the first undone.
 // The passes are explicit rather than relying on reverse seq happening to order
@@ -332,172 +385,192 @@ func planRevert(revs []domain.Revision) []domain.Revision {
 // in the revert's own change set), or a conflict describing why it declined. A
 // returned error is an infrastructure failure and aborts the revert; a conflict
 // is a normal outcome that skips just this entity.
-func (s *Service) applyInverse(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) {
+//
+// applied carries the versions this revert has already written, so a change set
+// holding several revisions for one entity doesn't mistake its own earlier work
+// for someone else's edit.
+func (s *Service) applyInverse(ctx context.Context, r domain.Revision, applied map[entityKey]int64) ([]change, *domain.RevertConflict, error) {
 	switch r.EntityType {
 	case domain.EntityObject:
-		return s.revertObject(ctx, r)
+		return revertEntity(ctx, s, r, applied, objectRevertOps(s))
 	case domain.EntityPlanting:
-		return s.revertPlanting(ctx, r)
+		return revertEntity(ctx, s, r, applied, plantingRevertOps(s))
 	case domain.EntityGarden:
-		return s.revertGarden(ctx, r)
+		return revertEntity(ctx, s, r, applied, gardenRevertOps(s))
 	default:
 		return nil, conflict(r, domain.ConflictUnsupported, ""), nil
 	}
 }
 
-func (s *Service) revertObject(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) {
-	switch r.Op {
-	case domain.OpCreate:
-		// Inverse of a create is a delete.
-		cur, err := s.store.GetObject(ctx, r.EntityID)
-		if errors.Is(err, domain.ErrNotFound) {
+// revertOps is everything reverting one entity type needs. The three
+// implementations differ only in which store methods they call and how a row
+// names itself; the ordering, guards and conflict reporting are shared below so
+// they cannot drift apart.
+type revertOps[T any] struct {
+	entityType string
+	get        func(ctx context.Context, id int64) (*T, error)
+	update     func(ctx context.Context, row *T) (*T, error)
+	// restore re-inserts a deleted row under its original id. nil means this
+	// entity type is never deleted (gardens), and a delete revision for it is
+	// reported as unsupported rather than silently skipped.
+	restore func(ctx context.Context, row *T) (*T, error)
+	// remove undoes a creation. It returns the changes it made, because deleting
+	// an object also deletes the plantings hanging off it.
+	remove func(ctx context.Context, row *T) ([]change, error)
+	// blockRemoval optionally refuses a removal, e.g. an object that has gained
+	// plantings since. Returns a conflict reason, or "" to allow it.
+	blockRemoval func(ctx context.Context, row *T) (string, error)
+	version      func(row *T) int64
+	setVersion   func(row *T, v int64)
+	label        func(row *T) string
+}
+
+// revertEntity applies the inverse of one revision for one entity type.
+func revertEntity[T any](
+	ctx context.Context, s *Service, r domain.Revision, applied map[entityKey]int64, ops revertOps[T],
+) ([]change, *domain.RevertConflict, error) {
+	key := keyOf(r)
+
+	// Inverse of a delete is a restore, under the original id.
+	if r.Op == domain.OpDelete {
+		if ops.restore == nil {
+			return nil, conflict(r, domain.ConflictUnsupported, ""), nil
+		}
+		if existing, err := ops.get(ctx, r.EntityID); err == nil {
+			return nil, conflict(r, domain.ConflictExists, ops.label(existing)), nil
+		} else if !errors.Is(err, domain.ErrNotFound) {
+			return nil, nil, err
+		}
+		var target T
+		if err := unsnapshot(r.Before, &target); err != nil {
+			return nil, nil, err
+		}
+		restored, err := ops.restore(ctx, &target)
+		if err != nil {
+			// The id may have been taken between the check above and this insert.
+			// Re-check rather than surfacing a driver-specific constraint error:
+			// "someone else has that id now" is a conflict, not a failure.
+			if existing, gerr := ops.get(ctx, r.EntityID); gerr == nil {
+				return nil, conflict(r, domain.ConflictExists, ops.label(existing)), nil
+			}
+			return nil, nil, err
+		}
+		applied[key] = ops.version(restored)
+		return []change{changeCreate(ops.entityType, r.EntityID, restored)}, nil, nil
+	}
+
+	cur, err := ops.get(ctx, r.EntityID)
+	if errors.Is(err, domain.ErrNotFound) {
+		if r.Op == domain.OpCreate {
 			return nil, nil, nil // already gone: the inverse is a no-op, not a conflict
 		}
-		if err != nil {
-			return nil, nil, err
-		}
-		if c := versionGuard(r, cur.Version, cur.Name); c != nil {
-			return nil, c, nil
-		}
-		changes, err := s.deleteObjectRecording(ctx, cur)
-		return changes, nil, err
-
-	case domain.OpUpdate:
-		cur, err := s.store.GetObject(ctx, r.EntityID)
-		if errors.Is(err, domain.ErrNotFound) {
-			return nil, conflict(r, domain.ConflictMissing, ""), nil
-		}
-		if err != nil {
-			return nil, nil, err
-		}
-		if c := versionGuard(r, cur.Version, cur.Name); c != nil {
-			return nil, c, nil
-		}
-		var target domain.GardenObject
-		if err := unsnapshot(r.Before, &target); err != nil {
-			return nil, nil, err
-		}
-		target.Version = cur.Version
-		updated, err := s.store.UpdateObject(ctx, &target)
-		if errors.Is(err, domain.ErrVersionConflict) {
-			return nil, conflict(r, domain.ConflictChanged, cur.Name), nil
-		}
-		if err != nil {
-			return nil, nil, err
-		}
-		return []change{changeUpdate(domain.EntityObject, updated.ID, cur, updated)}, nil, nil
-
-	case domain.OpDelete:
-		// Inverse of a delete is a restore, under the original id.
-		if existing, err := s.store.GetObject(ctx, r.EntityID); err == nil {
-			return nil, conflict(r, domain.ConflictExists, existing.Name), nil
-		} else if !errors.Is(err, domain.ErrNotFound) {
-			return nil, nil, err
-		}
-		var target domain.GardenObject
-		if err := unsnapshot(r.Before, &target); err != nil {
-			return nil, nil, err
-		}
-		restored, err := s.store.RestoreObject(ctx, &target)
-		if err != nil {
-			return nil, nil, err
-		}
-		return []change{changeCreate(domain.EntityObject, restored.ID, restored)}, nil, nil
-	}
-	return nil, conflict(r, domain.ConflictUnsupported, ""), nil
-}
-
-func (s *Service) revertPlanting(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) {
-	switch r.Op {
-	case domain.OpCreate:
-		cur, err := s.store.GetPlanting(ctx, r.EntityID)
-		if errors.Is(err, domain.ErrNotFound) {
-			return nil, nil, nil // already gone
-		}
-		if err != nil {
-			return nil, nil, err
-		}
-		if c := versionGuard(r, cur.Version, ""); c != nil {
-			return nil, c, nil
-		}
-		if err := s.store.DeletePlanting(ctx, cur.ID); err != nil {
-			return nil, nil, err
-		}
-		return []change{changeDelete(domain.EntityPlanting, cur.ID, cur)}, nil, nil
-
-	case domain.OpUpdate:
-		cur, err := s.store.GetPlanting(ctx, r.EntityID)
-		if errors.Is(err, domain.ErrNotFound) {
-			return nil, conflict(r, domain.ConflictMissing, ""), nil
-		}
-		if err != nil {
-			return nil, nil, err
-		}
-		if c := versionGuard(r, cur.Version, ""); c != nil {
-			return nil, c, nil
-		}
-		var target domain.Planting
-		if err := unsnapshot(r.Before, &target); err != nil {
-			return nil, nil, err
-		}
-		target.Version = cur.Version
-		updated, err := s.store.UpdatePlanting(ctx, &target)
-		if errors.Is(err, domain.ErrVersionConflict) {
-			return nil, conflict(r, domain.ConflictChanged, ""), nil
-		}
-		if err != nil {
-			return nil, nil, err
-		}
-		return []change{changeUpdate(domain.EntityPlanting, updated.ID, cur, updated)}, nil, nil
-
-	case domain.OpDelete:
-		if _, err := s.store.GetPlanting(ctx, r.EntityID); err == nil {
-			return nil, conflict(r, domain.ConflictExists, ""), nil
-		} else if !errors.Is(err, domain.ErrNotFound) {
-			return nil, nil, err
-		}
-		var target domain.Planting
-		if err := unsnapshot(r.Before, &target); err != nil {
-			return nil, nil, err
-		}
-		restored, err := s.store.RestorePlanting(ctx, &target)
-		if err != nil {
-			return nil, nil, err
-		}
-		return []change{changeCreate(domain.EntityPlanting, restored.ID, restored)}, nil, nil
-	}
-	return nil, conflict(r, domain.ConflictUnsupported, ""), nil
-}
-
-// revertGarden undoes a garden metadata edit. Garden creation and deletion are
-// never recorded (see the migration), so update is the only op reachable here.
-func (s *Service) revertGarden(ctx context.Context, r domain.Revision) ([]change, *domain.RevertConflict, error) {
-	if r.Op != domain.OpUpdate {
-		return nil, conflict(r, domain.ConflictUnsupported, ""), nil
-	}
-	cur, err := s.store.GetGarden(ctx, r.EntityID)
-	if errors.Is(err, domain.ErrNotFound) {
 		return nil, conflict(r, domain.ConflictMissing, ""), nil
 	}
 	if err != nil {
 		return nil, nil, err
 	}
-	if c := versionGuard(r, cur.Version, cur.Name); c != nil {
+	if c := versionGuard(r, ops.version(cur), ops.label(cur), applied[key]); c != nil {
 		return nil, c, nil
 	}
-	var target domain.Garden
+
+	// Inverse of a create is a removal.
+	if r.Op == domain.OpCreate {
+		if ops.remove == nil {
+			return nil, conflict(r, domain.ConflictUnsupported, ""), nil
+		}
+		if ops.blockRemoval != nil {
+			reason, err := ops.blockRemoval(ctx, cur)
+			if err != nil {
+				return nil, nil, err
+			}
+			if reason != "" {
+				return nil, conflict(r, reason, ops.label(cur)), nil
+			}
+		}
+		changes, err := ops.remove(ctx, cur)
+		if err != nil {
+			return nil, nil, err
+		}
+		delete(applied, key)
+		return changes, nil, nil
+	}
+
+	if r.Op != domain.OpUpdate {
+		return nil, conflict(r, domain.ConflictUnsupported, ""), nil
+	}
+	var target T
 	if err := unsnapshot(r.Before, &target); err != nil {
 		return nil, nil, err
 	}
-	target.Version = cur.Version
-	updated, err := s.store.UpdateGarden(ctx, &target)
+	ops.setVersion(&target, ops.version(cur))
+	updated, err := ops.update(ctx, &target)
 	if errors.Is(err, domain.ErrVersionConflict) {
-		return nil, conflict(r, domain.ConflictChanged, cur.Name), nil
+		return nil, conflict(r, domain.ConflictChanged, ops.label(cur)), nil
 	}
 	if err != nil {
 		return nil, nil, err
 	}
-	return []change{changeUpdate(domain.EntityGarden, updated.ID, cur, updated)}, nil, nil
+	applied[key] = ops.version(updated)
+	return []change{changeUpdate(ops.entityType, r.EntityID, cur, updated)}, nil, nil
+}
+
+func objectRevertOps(s *Service) revertOps[domain.GardenObject] {
+	return revertOps[domain.GardenObject]{
+		entityType: domain.EntityObject,
+		get:        s.store.GetObject,
+		update:     s.store.UpdateObject,
+		restore:    s.store.RestoreObject,
+		remove:     s.deleteObjectRecording,
+		// Undoing "added a bed" would cascade away anything planted in it since.
+		// Those plops are snapshotted, so it would be recoverable — but deleting
+		// someone's plants as a side effect of an unrelated undo should be
+		// reported, not performed quietly.
+		blockRemoval: func(ctx context.Context, o *domain.GardenObject) (string, error) {
+			plops, err := s.store.ListPlantingsForObject(ctx, o.ID)
+			if err != nil {
+				return "", err
+			}
+			if len(plops) > 0 {
+				return domain.ConflictChanged, nil
+			}
+			return "", nil
+		},
+		version:    func(o *domain.GardenObject) int64 { return o.Version },
+		setVersion: func(o *domain.GardenObject, v int64) { o.Version = v },
+		label:      func(o *domain.GardenObject) string { return o.Name },
+	}
+}
+
+func plantingRevertOps(s *Service) revertOps[domain.Planting] {
+	return revertOps[domain.Planting]{
+		entityType: domain.EntityPlanting,
+		get:        s.store.GetPlanting,
+		update:     s.store.UpdatePlanting,
+		restore:    s.store.RestorePlanting,
+		remove: func(ctx context.Context, p *domain.Planting) ([]change, error) {
+			if err := s.store.DeletePlanting(ctx, p.ID); err != nil {
+				return nil, err
+			}
+			return []change{changeDelete(domain.EntityPlanting, p.ID, p)}, nil
+		},
+		version:    func(p *domain.Planting) int64 { return p.Version },
+		setVersion: func(p *domain.Planting, v int64) { p.Version = v },
+		label:      func(p *domain.Planting) string { return "" },
+	}
+}
+
+// gardenRevertOps has no restore or remove: garden creation and deletion are
+// never recorded (see the migration), so metadata updates are all that can reach
+// here, and anything else is honestly reported as unsupported.
+func gardenRevertOps(s *Service) revertOps[domain.Garden] {
+	return revertOps[domain.Garden]{
+		entityType: domain.EntityGarden,
+		get:        s.store.GetGarden,
+		update:     s.store.UpdateGarden,
+		version:    func(g *domain.Garden) int64 { return g.Version },
+		setVersion: func(g *domain.Garden, v int64) { g.Version = v },
+		label:      func(g *domain.Garden) string { return g.Name },
+	}
 }
 
 // deleteObjectRecording deletes an object and returns every change to record.
@@ -526,17 +599,26 @@ func (s *Service) deleteObjectRecording(ctx context.Context, o *domain.GardenObj
 // one the change set left behind — i.e. something edited it since, and reverting
 // would silently discard that edit. A revision with no after-snapshot (a delete)
 // has nothing to compare and passes.
-func versionGuard(r domain.Revision, currentVersion int64, name string) *domain.RevertConflict {
-	var after struct {
-		Version int64 `json:"version"`
-	}
+//
+// appliedVersion is non-zero when THIS revert already wrote to this row, which
+// happens whenever a change set holds more than one revision for the same entity.
+// In that case the live version is our own doing and is the correct thing to
+// expect — comparing against the snapshot would flag our own work as a conflict.
+func versionGuard(r domain.Revision, currentVersion int64, name string, appliedVersion int64) *domain.RevertConflict {
 	if r.After == nil {
 		return nil
 	}
-	if err := json.Unmarshal([]byte(*r.After), &after); err != nil {
-		return conflict(r, domain.ConflictUnsupported, name)
+	expected := appliedVersion
+	if expected == 0 {
+		var after struct {
+			Version int64 `json:"version"`
+		}
+		if err := json.Unmarshal([]byte(*r.After), &after); err != nil {
+			return conflict(r, domain.ConflictUnsupported, name)
+		}
+		expected = after.Version
 	}
-	if after.Version != currentVersion {
+	if expected != currentVersion {
 		return conflict(r, domain.ConflictChanged, name)
 	}
 	return nil