Found by running the real thing on the live instance, immediately after the assistant came up.
The agent replaced a bed of garlic with cucumbers. Undo restored the garlic correctly — six plops back on the canvas, change set 21 reverting 20 with eight revisions — and then reported:
Nothing left to undo — this was already reversed.
Why
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 — the obvious next move is to do it again, and a second undo would have re-applied the cucumbers.
Fixed on both sides
Client: a null change set is 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.
Server:RevertChangeSet 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 test I already had was testing my assumption, not the API
describeUndo's existing unit test passed because the fixture I wrote populated counts. It was asserting what I believed the response looked like, not what it is. The new tests pin the real shape:
Service: a revert's counts match the fill it undid, and match what the list read reports for the same change set.
Client: an empty tally still reads as Undone.; only a null change set reads as a no-op.
87 tests, go test ./..., go vet, gofmt, tsc --noEmit, npm run build green.
Found by running the real thing on the live instance, immediately after the assistant came up.
The agent replaced a bed of garlic with cucumbers. **Undo restored the garlic correctly** — six plops back on the canvas, change set 21 reverting 20 with eight revisions — and then reported:
> Nothing left to undo — this was already reversed.
### Why
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 — the obvious next move is to **do it again**, and a second undo would have re-applied the cucumbers.
### Fixed on both sides
- **Client:** a `null` change set is 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.
- **Server:** `RevertChangeSet` 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 test I already had was testing my assumption, not the API
`describeUndo`'s existing unit test passed because the fixture I wrote populated `counts`. It was asserting what I believed the response looked like, not what it is. The new tests pin the real shape:
- Service: a revert's counts match the fill it undid, **and** match what the list read reports for the same change set.
- Client: an empty tally still reads as `Undone.`; only a `null` change set reads as a no-op.
87 tests, `go test ./...`, `go vet`, `gofmt`, `tsc --noEmit`, `npm run build` green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
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
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
4/5 reviewers finished · updated 2026-07-21 12:25:07Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ⏳ 4/5 lenses
- ⚠️ **security** — could not complete
- 🔄 **correctness** — running
- ⚠️ **maintainability** — could not complete
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Fixed in 9a3f1d7. Two of the three findings are the same problem seen from opposite ends, which is a good catch.
countRevisions reproduces in Go what ListChangeSets does in SQL, and nothing held the two together — while my new test compared only totals, which would agree even if the groupings had diverged completely. So the finding about the duplicated grouping and the finding about the weak assertion are one issue: I introduced a cross-layer contract and then wrote a test that couldn't see it break.
The test now compares the full per-(entityType, op) breakdown row for row, across two reverts of different shapes so there's more than one row to get wrong. Both ends name the contract and point at the test. That turns "someone will remember to keep these in step" into something CI notices.
The duplication itself stays, deliberately: 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're already holding.
totalOf became countsTotal with a comment naming it as the server-side twin of the client's totalChanges, rather than looking like an accidental third copy.
go test ./..., go vet, gofmt green.
Verified live after the first fix: the agent swapped a garlic bed to cucumbers, Undo restored the garlic, and the message now reads Undone. A follow-up question produced no change set, and the model referenced the earlier turn ("the cucumber swap from earlier didn't stick") — so conversation persistence works too.
Fixed in `9a3f1d7`. Two of the three findings are the same problem seen from opposite ends, which is a good catch.
`countRevisions` reproduces in Go what `ListChangeSets` does in SQL, and nothing held the two together — while my new test compared only **totals**, which would agree even if the groupings had diverged completely. So the finding about the duplicated grouping and the finding about the weak assertion are one issue: I introduced a cross-layer contract and then wrote a test that couldn't see it break.
The test now compares the full per-`(entityType, op)` breakdown **row for row**, across two reverts of different shapes so there's more than one row to get wrong. Both ends name the contract and point at the test. That turns "someone will remember to keep these in step" into something CI notices.
**The duplication itself stays**, deliberately: 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're already holding.
`totalOf` became `countsTotal` with a comment naming it as the server-side twin of the client's `totalChanges`, rather than looking like an accidental third copy.
`go test ./...`, `go vet`, `gofmt` green.
---
**Verified live after the first fix:** the agent swapped a garlic bed to cucumbers, Undo restored the garlic, and the message now reads `Undone.` A follow-up question produced no change set, and the model referenced the earlier turn ("the cucumber swap from earlier didn't stick") — so conversation persistence works too.
steve
merged commit 4ea0d0b262 into main2026-07-21 12:25:35 +00:00
steve
deleted branch fix/undo-reports-noop2026-07-21 12:25:35 +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.
Found by running the real thing on the live instance, immediately after the assistant came up.
The agent replaced a bed of garlic with cucumbers. Undo restored the garlic correctly — six plops back on the canvas, change set 21 reverting 20 with eight revisions — and then reported:
Why
The revert response carries the change set
commitScopejust inserted, and that row has no per-op tally:Countsis populated by the history list query, not by the insert.describeUndotreated 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 — the obvious next move is to do it again, and a second undo would have re-applied the cucumbers.
Fixed on both sides
nullchange set is 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.RevertChangeSetfills 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 test I already had was testing my assumption, not the API
describeUndo's existing unit test passed because the fixture I wrote populatedcounts. It was asserting what I believed the response looked like, not what it is. The new tests pin the real shape:Undone.; only anullchange set reads as a no-op.87 tests,
go test ./...,go vet,gofmt,tsc --noEmit,npm run buildgreen.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
4/5 reviewers finished · updated 2026-07-21 12:25:07Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/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.
Fixed in
9a3f1d7. Two of the three findings are the same problem seen from opposite ends, which is a good catch.countRevisionsreproduces in Go whatListChangeSetsdoes in SQL, and nothing held the two together — while my new test compared only totals, which would agree even if the groupings had diverged completely. So the finding about the duplicated grouping and the finding about the weak assertion are one issue: I introduced a cross-layer contract and then wrote a test that couldn't see it break.The test now compares the full per-
(entityType, op)breakdown row for row, across two reverts of different shapes so there's more than one row to get wrong. Both ends name the contract and point at the test. That turns "someone will remember to keep these in step" into something CI notices.The duplication itself stays, deliberately: a change set that has just been written has no rows to
GROUP BYyet, so the alternative to counting in Go is a second round trip to count what we're already holding.totalOfbecamecountsTotalwith a comment naming it as the server-side twin of the client'stotalChanges, rather than looking like an accidental third copy.go test ./...,go vet,gofmtgreen.Verified live after the first fix: the agent swapped a garlic bed to cucumbers, Undo restored the garlic, and the message now reads
Undone.A follow-up question produced no change set, and the model referenced the earlier turn ("the cucumber swap from earlier didn't stick") — so conversation persistence works too.