The agent is going to act freely — "empty the garlic bed and plant cucumbers" runs without a confirmation prompt. That's only a defensible default if the result is easy to roll back, so undo lands before the agent write path, not after it.
The model
The unit of undo is the operation, not the row. A change set groups the row-level revisions it produced; revert replays their inverses. Emptying a bed and replanting it touches one object and many plantings — undoing half of that is worse than useless.
Two properties shape everything else:
Revert is itself a change set (reverts_id names its target). History is append-only, so an undo can be undone. git revert, not git reset.
Revert is version-guarded per entity. Every snapshot carries the row's version; if something edited that row after the change set being reverted, restoring the old snapshot would silently discard the newer edit. That entity is reported as a conflict and left alone — the rest of the change set still reverts.
Auto-scope is what keeps this shallow
Service mutations call record(...), which joins the change set on the context if one is open and otherwise opens a single-op one on the spot. REST handlers needed no edits at all and every UI mutation lands in history for free; only the agent wraps a whole turn with WithChangeSet. Multi-row operations pass all their changes in one record call, so a 12-plop FillRegion is one change set with 12 revisions and one undo.
Revisions are buffered and written with their change set in a single transaction, so an operation that fails partway leaves no half-recorded history. Recording itself is best-effort after the write: the row already landed, so failing the caller there would report a failure that didn't happen and invite a duplicate retry. A gap is logged loudly instead.
Two sharp edges, handled explicitly
Object deletion cascades its plantings away at the FK level, where the service never sees them. deleteObjectRecording snapshots them first — otherwise the delete would be listed in history without actually being undoable. There's a test for exactly that.
Restoring deleted rows reuses their ids, and a restored plop needs its object back first. planRevert runs three explicit passes (restore parents → children, then updates, then delete created children → their parents) rather than trusting reverse-seq to happen to order things correctly. Pinned by a unit test on the plan itself.
Garden deletion stays out of scope, as designed — the cascade is invisible to the service, and ON DELETE CASCADE on change_sets.garden_id would take the history with it. #49 will say so in the panel rather than pretend otherwise.
API
GET /gardens/:id/history → { changeSets: [...], hasMore } (viewer+)
POST /change-sets/:id/revert → 201 { changeSet, conflicts: [] } (editor+)
409 { changeSet, conflicts: [...] } on a partial revert
The 409 carries both fields deliberately: a partial revert really did change things, so #49 can say "2 of 3 changes undone; the north bed was edited since and was left alone" instead of a generic failure.
Verification
16 new tests. Each acceptance criterion has one:
auto-scope records a plain REST-shaped mutation with no explicit change set
FillRegion of N plops → one change set, N revisions; revert removes all N
revert of a revert restores the change, and the original is marked reverted
an entity edited afterwards conflicts by name while the rest still reverts
deleting an object and undoing it brings back the bed and its plops
clear-bed undoes to a fully planted bed
viewer → ErrForbidden, stranger → ErrNotFound
a failed turn leaves no change set; a turn that changed nothing writes none
Closes #48. Phase 0 of #58.
The agent is going to act freely — *"empty the garlic bed and plant cucumbers"* runs without a confirmation prompt. That's only a defensible default if the result is easy to roll back, so undo lands **before** the agent write path, not after it.
### The model
The unit of undo is the **operation, not the row**. A change set groups the row-level revisions it produced; revert replays their inverses. Emptying a bed and replanting it touches one object and many plantings — undoing half of that is worse than useless.
Two properties shape everything else:
- **Revert is itself a change set** (`reverts_id` names its target). History is append-only, so an undo can be undone. `git revert`, not `git reset`.
- **Revert is version-guarded per entity.** Every snapshot carries the row's `version`; if something edited that row after the change set being reverted, restoring the old snapshot would silently discard the newer edit. That entity is reported as a conflict and left alone — **the rest of the change set still reverts.**
### Auto-scope is what keeps this shallow
Service mutations call `record(...)`, which joins the change set on the context if one is open and otherwise opens a single-op one on the spot. **REST handlers needed no edits at all** and every UI mutation lands in history for free; only the agent wraps a whole turn with `WithChangeSet`. Multi-row operations pass all their changes in one `record` call, so a 12-plop `FillRegion` is one change set with 12 revisions and one undo.
Revisions are buffered and written with their change set in a single transaction, so an operation that fails partway leaves no half-recorded history. Recording itself is best-effort *after* the write: the row already landed, so failing the caller there would report a failure that didn't happen and invite a duplicate retry. A gap is logged loudly instead.
### Two sharp edges, handled explicitly
**Object deletion cascades its plantings** away at the FK level, where the service never sees them. `deleteObjectRecording` snapshots them first — otherwise the delete would be *listed* in history without actually being undoable. There's a test for exactly that.
**Restoring deleted rows reuses their ids**, and a restored plop needs its object back first. `planRevert` runs three explicit passes (restore parents → children, then updates, then delete created children → their parents) rather than trusting reverse-`seq` to happen to order things correctly. Pinned by a unit test on the plan itself.
**Garden deletion stays out of scope**, as designed — the cascade is invisible to the service, and `ON DELETE CASCADE` on `change_sets.garden_id` would take the history with it. #49 will say so in the panel rather than pretend otherwise.
### API
```
GET /gardens/:id/history → { changeSets: [...], hasMore } (viewer+)
POST /change-sets/:id/revert → 201 { changeSet, conflicts: [] } (editor+)
409 { changeSet, conflicts: [...] } on a partial revert
```
The 409 carries **both** fields deliberately: a partial revert really did change things, so #49 can say *"2 of 3 changes undone; the north bed was edited since and was left alone"* instead of a generic failure.
### Verification
16 new tests. Each acceptance criterion has one:
- auto-scope records a plain REST-shaped mutation with no explicit change set
- `FillRegion` of N plops → one change set, N revisions; revert removes all N
- revert of a revert restores the change, and the original is marked reverted
- an entity edited afterwards conflicts by name while the rest still reverts
- deleting an object and undoing it brings back the bed **and its plops**
- clear-bed undoes to a fully planted bed
- viewer → `ErrForbidden`, stranger → `ErrNotFound`
- a failed turn leaves no change set; a turn that changed nothing writes none
`go test ./...`, `go vet ./...`, `gofmt` all green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The undo substrate. The agent is going to act freely — "empty the garlic bed and
plant cucumbers" runs without a confirmation prompt — and that is only a
defensible default if the result is easy to roll back. So undo lands before the
agent write path, not after it.
The unit of undo is the OPERATION, not the row. A change set groups the
row-level revisions it produced; revert replays their inverses. Emptying a bed
and replanting it touches one object and many plantings, and undoing half of
that is worse than useless.
Two properties shape the rest:
Revert is itself a change set (reverts_id names its target), so history is
append-only and an undo can be undone. git revert, not git reset.
Revert is version-guarded per entity. Every snapshot carries the row's version;
if something edited that row after the change set being reverted, restoring the
old snapshot would silently discard the newer edit, so it is reported as a
conflict and left alone while the rest of the change set still reverts.
The auto-scope is what keeps this invasive but shallow. Service mutations call
record(), which joins the change set on the context if one is open and otherwise
opens a single-op one on the spot. REST handlers needed no edits at all and
every UI mutation lands in history for free; only the agent wraps a whole turn.
Multi-row operations (FillRegion, ClearObject) pass all their changes in one
record call, so a 12-plop fill is one change set with 12 revisions and one undo.
Revisions are buffered and written with their change set in a single
transaction, so an operation that fails partway leaves no half-recorded history.
Recording is best-effort after the fact: the row is already written, so failing
the caller there would report a failure that didn't happen and invite a
duplicate retry. A gap is logged loudly instead.
Two sharp edges handled explicitly rather than by luck:
Deleting an object cascades its plantings away at the FK level, where the
service never sees them. deleteObjectRecording snapshots them first, or the
delete would be listed in history and not actually be undoable.
Restoring deleted rows reuses their ids, and a restored plop needs its object
back first. planRevert runs three explicit passes — restore parents then
children, then updates, then delete created children before their parents —
instead of trusting reverse-seq ordering to happen to be right.
Garden deletion stays out of scope, as designed: the cascade is invisible to the
service and ON DELETE CASCADE would take the history with it. The history UI
will say so rather than pretend otherwise.
Closes#48
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
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
Good review — six real bugs, three of which would have broken this feature's central promise. All fixed, six new tests, one per bug.
The worst one, and I want to be clear it was mine
WithChangeSetdiscarded the history for changes that had already committed. I wrote the doc comment saying "an operation that fails partway leaves no half-recorded change set behind" and reasoned it was correct — but it isn't, because this scope buffers history, not data. The store writes inside fn commit as they go. Dropping the buffer left real mutations with no change set and no way to undo them.
And that is exactly the situation undo exists for: an agent turn that did half a thing and then failed. The one case where you most want to roll back was the one case with nothing to roll back to.
Now the buffer is written, the summary says the operation failed partway, and the error is still returned. RevertChangeSet does the same when a revert fails mid-loop. The partial state is still partial — but it's visible and undoable instead of orphaned. That reframes the atomicity findings (flagged under correctness, performance and error-handling by 3 models): a per-row store API means a partial apply is possible, so the answer is to make the partial recoverable rather than to pretend it can't happen.
versionGuard conflicted with the revert's own work
If one change set held two 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 snapshot's version no longer matched the live row. The revert flagged its own work as somebody else's edit and half-failed. Silent, and it would only ever show up on exactly the multi-step agent turns this was built for. RevertChangeSet now tracks what it has written and the guard compares against that.
The rest
LEFT JOIN duplicated rows once a change set had been reverted more than once (undo, redo, undo again) — one duplicate row per revert, silently corrupting the page. Now a scalar subquery. Tested at the store level, because getting a target legitimately reverted twice through the service is hard: the second attempt conflicts, which is itself correct.
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 but 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 — telling the caller a clear had failed after it had already applied, inviting a retry of an applied operation. That path now logs the gap and reports success, matching how record() treats its own write failures.
RestoreObject/RestorePlanting race between the existence check and the insert surfaced a raw constraint error. The revert re-checks and reports ConflictExists, which is what that condition actually means.
RevertChangeSet hardcoded source=ui, so an agent undoing its own work was indistinguishable from a person clicking undo — defeating the badge. It takes a source now.
Refactors
The three revert bodies repeated a load/guard/unsnapshot/update skeleton (3/5 models). Now one generic revertEntity over a small per-type op table, so the ordering, guards and conflict reporting can't drift apart. The API 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 answers 200 rather than 201 with a null change set.
Declined, with reasoning
GetChangeSet has no LIMIT on revisions. Deliberate: a revert has to load all of them or it isn't a revert. A cap would silently produce partial undos, which is worse than a slow one.
SourceAPI is never produced. True, but it's in the migration's CHECK constraint and reserved for the future API-key surface; removing it needs a migration. REST is the UI today, so auto-scope recording ui is accurate rather than a placeholder.
go test ./..., go vet ./..., gofmt green.
Good review — six real bugs, three of which would have broken this feature's central promise. All fixed, six new tests, one per bug.
### The worst one, and I want to be clear it was mine
`WithChangeSet` **discarded the history for changes that had already committed.** I wrote the doc comment saying "an operation that fails partway leaves no half-recorded change set behind" and reasoned it was correct — but it isn't, because this scope buffers *history*, not *data*. The store writes inside `fn` commit as they go. Dropping the buffer left real mutations with no change set and no way to undo them.
And that is *exactly* the situation undo exists for: an agent turn that did half a thing and then failed. The one case where you most want to roll back was the one case with nothing to roll back to.
Now the buffer is written, the summary says the operation failed partway, and the error is still returned. `RevertChangeSet` does the same when a revert fails mid-loop. The partial state is still partial — but it's visible and undoable instead of orphaned. That reframes the atomicity findings (flagged under correctness, performance and error-handling by 3 models): a per-row store API means a partial apply is possible, so the answer is to make the partial recoverable rather than to pretend it can't happen.
### `versionGuard` conflicted with the revert's own work
If one change set held **two 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 snapshot's version no longer matched the live row. The revert flagged its own work as somebody else's edit and half-failed. Silent, and it would only ever show up on exactly the multi-step agent turns this was built for. `RevertChangeSet` now tracks what it has written and the guard compares against that.
### The rest
- **LEFT JOIN duplicated rows** once a change set had been reverted more than once (undo, redo, undo again) — one duplicate row per revert, silently corrupting the page. Now a scalar subquery. Tested at the store level, because getting a target legitimately reverted twice *through the service* is hard: the second attempt conflicts, which is itself correct.
- **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 but 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 — telling the caller a clear had failed after it had already applied, inviting a retry of an applied operation. That path now logs the gap and reports success, matching how `record()` treats its own write failures.
- **`RestoreObject`/`RestorePlanting` race** between the existence check and the insert surfaced a raw constraint error. The revert re-checks and reports `ConflictExists`, which is what that condition actually means.
- **`RevertChangeSet` hardcoded `source=ui`**, so an agent undoing its own work was indistinguishable from a person clicking undo — defeating the badge. It takes a source now.
### Refactors
The three revert bodies repeated a load/guard/unsnapshot/update skeleton (3/5 models). Now one generic `revertEntity` over a small per-type op table, so the ordering, guards and conflict reporting can't drift apart. The API 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 answers 200 rather than 201 with a null change set.
### Declined, with reasoning
- **`GetChangeSet` has no LIMIT on revisions.** Deliberate: a revert has to load all of them or it isn't a revert. A cap would silently produce partial undos, which is worse than a slow one.
- **`SourceAPI` is never produced.** True, but it's in the migration's CHECK constraint and reserved for the future API-key surface; removing it needs a migration. REST *is* the UI today, so auto-scope recording `ui` is accurate rather than a placeholder.
`go test ./...`, `go vet ./...`, `gofmt` green.
steve
merged commit f208da94d6 into main2026-07-21 05:07:31 +00:00
steve
deleted branch feat/revision-history2026-07-21 05:07:31 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #48. Phase 0 of #58.
The agent is going to act freely — "empty the garlic bed and plant cucumbers" runs without a confirmation prompt. That's only a defensible default if the result is easy to roll back, so undo lands before the agent write path, not after it.
The model
The unit of undo is the operation, not the row. A change set groups the row-level revisions it produced; revert replays their inverses. Emptying a bed and replanting it touches one object and many plantings — undoing half of that is worse than useless.
Two properties shape everything else:
reverts_idnames its target). History is append-only, so an undo can be undone.git revert, notgit reset.version; if something edited that row after the change set being reverted, restoring the old snapshot would silently discard the newer edit. That entity is reported as a conflict and left alone — the rest of the change set still reverts.Auto-scope is what keeps this shallow
Service mutations call
record(...), which joins the change set on the context if one is open and otherwise opens a single-op one on the spot. REST handlers needed no edits at all and every UI mutation lands in history for free; only the agent wraps a whole turn withWithChangeSet. Multi-row operations pass all their changes in onerecordcall, so a 12-plopFillRegionis one change set with 12 revisions and one undo.Revisions are buffered and written with their change set in a single transaction, so an operation that fails partway leaves no half-recorded history. Recording itself is best-effort after the write: the row already landed, so failing the caller there would report a failure that didn't happen and invite a duplicate retry. A gap is logged loudly instead.
Two sharp edges, handled explicitly
Object deletion cascades its plantings away at the FK level, where the service never sees them.
deleteObjectRecordingsnapshots them first — otherwise the delete would be listed in history without actually being undoable. There's a test for exactly that.Restoring deleted rows reuses their ids, and a restored plop needs its object back first.
planRevertruns three explicit passes (restore parents → children, then updates, then delete created children → their parents) rather than trusting reverse-seqto happen to order things correctly. Pinned by a unit test on the plan itself.Garden deletion stays out of scope, as designed — the cascade is invisible to the service, and
ON DELETE CASCADEonchange_sets.garden_idwould take the history with it. #49 will say so in the panel rather than pretend otherwise.API
The 409 carries both fields deliberately: a partial revert really did change things, so #49 can say "2 of 3 changes undone; the north bed was edited since and was left alone" instead of a generic failure.
Verification
16 new tests. Each acceptance criterion has one:
FillRegionof N plops → one change set, N revisions; revert removes all NErrForbidden, stranger →ErrNotFoundgo test ./...,go vet ./...,gofmtall green.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
3/5 reviewers finished · updated 2026-07-21 05:07:05Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ⏳ 4/5 lensesopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ⏳ 4/5 lensesLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
Good review — six real bugs, three of which would have broken this feature's central promise. All fixed, six new tests, one per bug.
The worst one, and I want to be clear it was mine
WithChangeSetdiscarded the history for changes that had already committed. I wrote the doc comment saying "an operation that fails partway leaves no half-recorded change set behind" and reasoned it was correct — but it isn't, because this scope buffers history, not data. The store writes insidefncommit as they go. Dropping the buffer left real mutations with no change set and no way to undo them.And that is exactly the situation undo exists for: an agent turn that did half a thing and then failed. The one case where you most want to roll back was the one case with nothing to roll back to.
Now the buffer is written, the summary says the operation failed partway, and the error is still returned.
RevertChangeSetdoes the same when a revert fails mid-loop. The partial state is still partial — but it's visible and undoable instead of orphaned. That reframes the atomicity findings (flagged under correctness, performance and error-handling by 3 models): a per-row store API means a partial apply is possible, so the answer is to make the partial recoverable rather than to pretend it can't happen.versionGuardconflicted with the revert's own workIf one change set held two 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 snapshot's version no longer matched the live row. The revert flagged its own work as somebody else's edit and half-failed. Silent, and it would only ever show up on exactly the multi-step agent turns this was built for.
RevertChangeSetnow tracks what it has written and the guard compares against that.The rest
ClearObjectcleared by predicate but 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 — telling the caller a clear had failed after it had already applied, inviting a retry of an applied operation. That path now logs the gap and reports success, matching howrecord()treats its own write failures.RestoreObject/RestorePlantingrace between the existence check and the insert surfaced a raw constraint error. The revert re-checks and reportsConflictExists, which is what that condition actually means.RevertChangeSethardcodedsource=ui, so an agent undoing its own work was indistinguishable from a person clicking undo — defeating the badge. It takes a source now.Refactors
The three revert bodies repeated a load/guard/unsnapshot/update skeleton (3/5 models). Now one generic
revertEntityover a small per-type op table, so the ordering, guards and conflict reporting can't drift apart. The API view structs duplicated domain types that already carried every field, against the package's direct-JSON convention — dropped.intQuerymoved 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 answers 200 rather than 201 with a null change set.Declined, with reasoning
GetChangeSethas no LIMIT on revisions. Deliberate: a revert has to load all of them or it isn't a revert. A cap would silently produce partial undos, which is worse than a slow one.SourceAPIis never produced. True, but it's in the migration's CHECK constraint and reserved for the future API-key surface; removing it needs a migration. REST is the UI today, so auto-scope recordinguiis accurate rather than a placeholder.go test ./...,go vet ./...,gofmtgreen.