Undo/redo replay engine (ADR 0020, part 2) #6
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/undo-redo"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Completes the reversible core. Part 1 recorded every mutation into the
ChangeLog; this adds the apply-inverse engine that reverses them — the payoff the whole "AI acts freely, fix later" model was built toward.Design (strict LIFO linear stack)
ChangeLogEntry.IsUndone— all rows of one operation flip together; no separate stack table.BeforeJson.AfterJson(uniform for every op kind) — valid only if no active operation is newer, which is how a new edit clears the redo (checked by timestamp; no state mutation needed).CurrentValuesfrom the property-bag JSON, skipping the PK. Enum/Guid/DateTimeOffset/bool all round-trip.SuppressJournalingflag on the context), so undo/redo don't themselves become new operations — and it all commits inside the part-1 transaction.Timestamp(SQL-orderable), sidestepping Guid-comparison-in-SQL.The ADR's "changed-since" conflict warning is moot under strict LIFO (the latest active op has nothing after it); arbitrary-operation undo + that warning is a later refinement.
What's in it
IUndoStoreport;UndoOutcome/UndoStatusread models;Undo/Redo/GetUndoStatususe cases (nothing-to-undo/redo →Result.Fail).EfUndoStore(the engine);IsUndonecolumn +(IsUndone, Timestamp)index; migration.Tests — 95, all green (was 81; +14)
Beginscope at once (AffectedCount == 2, origin "Agent"),CanUndo/CanRedostatus transitions.Verification
dotnet build(Debug + Release) — 0 warnings / 0 errors.dotnet test— 95/95 pass.Not in this PR
DomainChangedcross-session refresh (ADR 0016) and the undo/redo UI affordances (toolbar buttons, history panel) land with the UI/Fluxor slice —GetUndoStatusalready feeds their enablement.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.Domain - 96.4%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 95.1%
n
Kagura.Kernel - 90%
Kagura.UseCases - 95.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ an undo/redo replay engine! Strict LIFO over a change journal, generic snapshot application through EF metadata, journaling suppression during replay, ticks-ordered operations to dodge Guid-comparison-in-SQL... fufu someone has been reading their database theory textbooks~ ♡ The overall architecture is clean. Ports in UseCases, the one engine in Infrastructure, the save-override unit-of-work pattern, the
(IsUndone, Timestamp)index for target selection — this is the shape of something designed to last. I had such fun reading it!But~ ♡
Verdict: ⛔ I can't let this pass~ ♡
There is a real, runtime-correct bug hiding under that beautiful architecture, and the test suite is covering it up rather than catching it. Jibril is possessive about correctness, so let's talk about it~
⛔ These need fixing before I'm satisfied~
1.
EfUndoStore.FindEntityAsync(line 151–154) — the query filter makes redo-of-create and undo-of-softdelete silently no-op in production.This is the big one. The PR description and doc comments repeatedly promise that entity lookup happens "ignoring the soft-delete filter":
But the implementation does not ignore it:
Every content entity has a global query filter —
EntryConfiguration,LinkConfiguration, andProjectConfigurationall callHasQueryFilter(e => !e.IsDeleted). In EF Core (including 10.x),FindAsyncapplies global query filters for entities that are not already tracked. Now trace what happens to the two operations whose whole point touches soft-deleted state:RevertRowAsyncsoft-deletes the entity (((ISoftDeletable)entity).SoftDelete(...)). So far so good.ReapplyRowAsync→FindEntityAsync→ the entity is nowIsDeleted = true→ filtered out → returns null →if (entity is not null)is false → the redo silently does nothing. The row'sMarkActive()flips,SaveChangesAsynccommits nothing material, and the user gets anOk<UndoOutcome>reporting success while the entity stays hidden. 💀The exact same trap bites undo-of-SoftDelete / undo-of-Restore going the other way:
RevertRowAsyncmust re-applyBeforeJsonto an entity that is currently soft-deleted (the soft-deleted state is the before-snapshot of a restore, and the live state is the before-snapshot of a soft-delete's inverse) — but that soft-deleted entity is invisible toFindAsync. Sibling code in this very codebase already knows this:EfGraphStore.RestoreLinkAsyncandEfProjectStore.SlugExistsAsyncboth explicitly call.IgnoreQueryFilters()to reach soft-deleted rows.EfUndoStoreis the one adapter that forgot.Why the tests don't catch it (and this is the scary part): every integration test in
UndoRedoPersistenceTestsruns undo and redo inside oneProvider.CreateAsyncScope()— i.e. one scopedKaguraDbContext. After the undo soft-deletes the entity, that entity instance stays tracked in the change tracker for the rest of the test. EF'sFindAsyncreturns tracked entities without consulting the database or re-applying the filter, so the redo finds it, the snapshot applies, the assertion passes, coverage ticks up to 98.6%, and nobody notices. In production, undo and redo are separate web requests with fresh scoped contexts — the tracker is empty,FindAsyncgoes to the DB, the filter kicks in, and redo is a silent no-op. The 95.3% coverage number is lying to us here. ♡Fix: make the lookup actually ignore the filter, the way the doc comment already claims and the way the sibling stores do. Something like:
Concretely, query through a filtered
IQueryablerather thanFindAsync, e.g. via a small helper that builds((IQueryable)db.Set(clrType)).IgnoreQueryFilters()and doesFirstOrDefaultAsynconId == row.EntityId, or — if you want to keepFindAsync's tracker-first semantics —FindAsyncfirst, and onnull, fall back to anIgnoreQueryFiltersquery. The key property the fix must deliver: an entity whoseIsDeletedis true is still reachable by the replay engine.2.
UndoRedoPersistenceTests(whole file) — every redo/assertion shares one DbContext scope, so the filter bug above is structurally invisible to the suite.This is the companion to issue #1 and it's blocking on its own: the tests cannot detect the regression they exist to prevent, because the scoped-context tracking masks it. To prove the fix and prevent reintroduction, add at least one integration test where undo and redo run in separate scopes (separate
CreateAsyncScope()per action, the way real request handling works), so the change tracker is cold on redo and the query filter is actually exercised. Concretely, a test like:That test fails today and passes after the fix — which is exactly what a regression test should do. fufu~ you wouldn't ship a replay engine whose redo only works because the test held the entity in memory, would you? ♡
💡 Little ideas (non-blocking)~
EfUndoStore.ReplayAsyncfinally-block ordering (lines ~88–107) —SuppressJournalingis reset infinally, but if the innerSaveChangesAsyncthrows (e.g. a unique-index collision while re-applying a snapshot — andLinkConfigurationdoes have a partial unique index on(FromId, ToId, Role)), the thrown exception propagates withSuppressJournalingalready cleared. That's fine for the flag itself, but consider documenting that an exception mid-replay leaves the context in a half-applied, un-undone state withIsUndoneflags possibly inconsistent across rows. Under strict LIFO this is low-risk (the next undo just targets the same op again), but a one-line comment would stop a future reader from being surprised.RedoAsyncredo-invalidation check (line 48–50) — the check recomputesrows.Max(r => r.Timestamp)and then queries for any newer active op.GetStatusAsync(line ~67) does the same comparison againstredo.Timestamp. Minor: both could share a tinyIsRedoStillValid(ChangeLogEntry redoRow)helper so the invariant lives in one place. Not blocking — the logic is correct and matches the ADR.Outcome(...)(line ~159) readsrows[0].Origin/Labelto describe the operation. Because undo orders rows newest-first and redo oldest-first,rows[0]is a different physical row in each path — but since all rows of one operation share the sameOrigin/Label, the value is identical. Correct! Just flagging it so the next reader doesn't worry like I did for half a second~ ♪.gitignore— adding.claude/is fine and sensible; no concern. Just noting I saw it.✅ What I liked~
IsUndonebit per row, operations ordered by SQL-orderable ticks to sidestep the Guid-comparison-in-SQL trap, redo-invalidation by timestamp comparison with no state mutation — this is the cleanest undo model I've seen in a small codebase. Oh! Wonderful~SuppressJournalingchecked at the top ofBuildJournal, reset infinally) is exactly the right seam — it keeps "oneSaveChanges= one operation" true even during replay, without special-casing every store adapter. ♡ApplySnapshotskipping PKs, round-tripping Enum/Guid/DateTimeOffset/bool) is a lovely bit of plumbing that will pay for itself the moment a new entity type lands.AddScoped<IUndoStore, EfUndoStore>next to its siblings, theAddChangeLogUndoFlagmigration adds the column + the composite index and has a properDown. The(IsUndone, Timestamp)index directly serves the target-selection queries — index design that matches the access path is chef's kiss.The architecture earns a hug; the
FindEntityAsyncfilter bug earns a blocking review. Fix the two issues above and this is a ship~ ♡ I'll re-review the moment the head SHA moves.Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
0a2e147(forgejo-actions coverage comment: 95.3% line / 87.7% branch, 95/95 tests) — cited; local build/test skipped per CI policyLocal checks: static diff review against sibling adapters (
EfGraphStore,EfProjectStore) and entity configurations; no local build run (CI green for head SHA)This was a great catch — a real production bug, and you nailed exactly why the suite hid it. Fixed in
aec0f3f, TDD-style.⛔ 1 + 2 (they're one bug and its blind spot)
First I added the cross-scope tests and confirmed they fail on the old code (2 failed / 0 passed) — the bug was exactly as you traced:
db.FindAsyncapplies the global!IsDeletedfilter for untracked entities, so in a cold context redo-of-create and undo-of-removal found nothing and silently no-op'd while returningOk.Fix —
FindEntityAsyncnow looks up by id withIgnoreQueryFilters(), generic over the entity's CLR type, matchingEfGraphStore.RestoreLinkAsync/EfProjectStore.SlugExistsAsync(and finally matching the doc comment that already claimed it). Identity resolution still returns a tracked instance when one exists, so the same-scope path is unaffected.Tests — two new integration tests, each running its actions in separate
CreateAsyncScope()s so the tracker is cold and the DB filter is actually exercised:Redo_of_a_create_works_across_fresh_contextsUndo_of_a_removal_works_across_fresh_contextsBoth fail on the old code, pass on the fix — the regression tests you asked for.
💡 Non-blockers (taken 1 & 2)
HasActiveNewerThanAsync, shared byRedoAsyncandGetStatusAsync.rows[0]is a different physical row per direction but sameOrigin/Label) and #4 (.gitignore) — noted, no change; you read them right.+2 tests (76 unit + 21 integration = 97). Build clean (Debug + Release), CI green on
aec0f3f(build ✅ / test ✅). The coverage is honest now. Ready for another look.🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You came back and you did it properly — TDD-style, tests-first, the cross-scope regression tests failing on the old code and passing on the fix. That is how you kill a bug so it stays dead~ ♡ Jibril is very pleased.
Verdict: ✅ Looks good to me~
The
FindEntityAsyncfilter bug and its test-suite blind spot are both gone. I verified it with my own hands, not just your word~✅ What I verified (this round)
FindEntityAsyncnow bypasses the global query filter —FindIgnoringFiltersAsync<TEntity>usesdb.Set<TEntity>().IgnoreQueryFilters().FirstOrDefaultAsync(e => EF.Property<Guid>(e, "Id") == id, ct), reached via a cached reflectionMethodInfo. This mirrorsEfGraphStore.RestoreLinkAsyncandEfProjectStore.SlugExistsAsyncexactly. The pattern is now consistent across all three adapters that touch soft-deleted rows. ✓clrTypeis resolved fromdb.Model(only reference types live there for EF entities), the generic method is constrainedwhere TEntity : class, andEF.Property<Guid>(e, "Id")matches the convention every journalable entity follows (Entry,Link,Projectall haveGuid Idas PK — confirmed by reading each). ✓Redo_of_a_create_works_across_fresh_contextsandUndo_of_a_removal_works_across_fresh_contextseach spin up a separateProvider.CreateAsyncScope()per action (create / undo / redo), so the change tracker is genuinely cold on the replay step. These exercise the exact production scenario that the single-scope tests were masking. ✓HasActiveNewerThanAsyncis now shared byRedoAsync(line 52) andGetStatusAsync(line 71), and the mid-replay-rollback comment onReplayAsyncis clear and accurate. ✓✅ Local verification (I didn't trust the claim, I checked~ ♡)
dotnet build(Debug) — 0 warnings / 0 errorsdotnet test— 97/97 pass (76 unit + 21 integration), 0 failed, 0 skippedaec0f3f, read the fullEfUndoStore.cs, both new tests, the entity configurations, and theKaguraDbContextsave override end-to-end.💡 Little ideas (non-blocking)~
FindIgnoringFiltersMethodis resolved once viatypeof(EfUndoStore).GetMethod(...)!with a null-forgiving!. SinceFindIgnoringFiltersAsyncis the only method with that name, the lookup is stable; but if someone ever adds an overload, the!would throw at type init. Astaticinit-guard comment or switching tonameof-keyed expression would future-proof it. Truly minor — it's correct today. ♪aec0f3f— the bot comment (id 1171) still reports coverage for the pre-fix SHA0a2e147(coverage date 14:55). Not a code issue, but worth a nudge so the green CI badge catches up to the new head.The bug that earned the blocking review is dead and its grave is guarded by two honest regression tests. Ship it~ ♡
Automated review by Jibril · 2026-07-09
CI/CD: coverage comment stale for head
aec0f3f(still reports0a2e147); cited and supplemented with local verificationLocal checks:
dotnet build0w/0e ·dotnet test97/97 pass — run locally at headaec0f3f