fix(journal): History's Redo and Redo run land nothing on what the journal took away — each redoes whole or has nothing to redo, the store refuses a redo onto nothing, and Revert run counts an edit between two of a run's writes as a conflict #61
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/redo-paths-refuse-nothing"
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?
Summary
This covers the two follow-ups #58 left open.
EfUndoStorereplays an edit or a delete onto a missing entity as a no-op but still marks the row active, and two redo paths could reach that. The new tests reproduced both routes first, and they failed before the fix:RedoRevertthen answeredOk(1, 1), changed nothing, and marked "typed" active. A third route came up while fixing it. Revert run brings the paragraph back once more, and now the redo set has the run's delete below the edit. Replayed in journal order, the delete takes the paragraph and the edit lands on what it took (EF'sFindreturns theDeletedentry), givingOk(2, 2)over nothing.Ok(1, 1), nothing written, and the run's row marked active above the writer's undone create, which History's Redo could then no longer reach.One landing check for all three redo paths.
ReplayLanding.RedoLandsAsyncwalks the rows in the order the store applies them. It starts from the entities the journal has removed (IChangeJournal.RemovedAsync, #58'sAnyRemovedAsyncnow returning which entities rather than whether), and each create and delete the redo re-applies updates that set. Ctrl+Y (RedoTyping), History's Redo (RedoRevert, and soGetStoryHistory.CanRedo, so the button and the action agree) and Redo run all ask it first.The undo direction, examined. Revert run counted conflicts only after the run's last write to an entity. A writer's polish between two of a run's writes (a Retry writing the paragraph again) stayed active in two ways:
The window now starts at the run's first write to the entity, which is what ADR 0022's rule says. A polish discarded that way comes back only with the whole revert, through History's Redo. Redo run leaves it undone beneath the run's later write, whose words its whole-row after-state would overwrite. Redo run keeps counting from the run's last write. A polish that an earlier build's revert left active lies beneath that write, so the redo lands over it the way the run first did, with nothing to discard or confirm. UndoTyping and RevertToSnapshot reach the no-op only through rows an earlier build marked redone over nothing.
Defense in depth in the store.
EfUndoStorenow throwsInvalidOperationExceptionon a redo that finds its entity gone, or alreadyDeletedearlier in the same replay. That means a bug, or another session's write between the check and the replay. Nothing is applied or marked, and the tracker is cleared so the circuit's next write carries none of it. An undo onto nothing stays a no-op that marks the row undone, so Ctrl+Z still takes back a row an earlier build marked redone over nothing, or a create whose paragraph a cascade removed.ADR 0022 amended. Changes:
Everything stays inside the journal's paths:
UseCases/Journal(ReplayLanding,IChangeJournal,IUndoStore,RedoTyping,RedoRevert,RedoRun,RevertRun,GetStoryHistory),Infrastructure/Journal(EfChangeJournal,EfUndoStore), ADR 0022,HistoryAndRevertTests, and the adapter'sFakeChangeJournalfor the changed port member. There is no migration and no seed change.Verification
2aa481f), built with-nodeReuse:false -m:2(0 warnings, 0 errors):HistoryAndRevertTests(15 tests, 16 cases), asserting exact outcomes, texts,UndoStatusand(IsUndone, UndoState)rows:History_s_Redo_lands_nothing_on_a_paragraph_whose_delete_stands_nor_on_one_its_own_delete_takes_firstandRedo_run_lands_nothing_on_a_paragraph_whose_create_a_revert_to_here_took_and_History_s_Redo_brings_both_back.Redo_run_lands_no_delete_on_a_paragraph_whose_create_a_revert_to_here_took_and_History_s_Redo_brings_both_back.…says_so_before_asking_and_a_forced_one_discards_nothing,…asks_before_discarding_a_later_delete…,…counts_on_a_discarded_delete_only….Revert_run_counts_an_edit_between_two_of_its_writes_as_a_conflict…Revert_run_discards_an_edit_between_two_of_its_writes_to_the_writers_paragraph…Revert_run_leaves_alone_an_edit_made_after_the_run_began_but_before_its_first_write_to_that_paragraphRedo_run_brings_back_a_run_that_wrote_its_paragraph_twice_and_leaves_the_edit_between_them_undone_beneath_the_second. ResultOk(2, 2), "By the run, again.", the polish(true, Rewound), status(false, false, null, null), and both Ctrl+Y and History's Redo return "Nothing to redo.".Redo_run_replays_its_later_write_over_an_edit_an_earlier_build_left_active_between_two_of_its_writes.The_store_refuses_a_redo_onto_nothing_and_marks_nothing(×2) andThe_store_refuses_a_redo_whose_edit_finds_what_its_own_delete_took, which checks the exact message and that the next write in the same scope does not commit the half-replayed delete. AlsoCtrl_Z_takes_back_an_edit_an_earlier_build_marked_redone_over_nothingandCtrl_Z_takes_back_a_create_whose_paragraph_is_already_gone.ReplayLanding: check disabled; create arm keeps the entity gone; a re-applied delete no longer marks it gone; discard loop removed; an edit in the discard counts as bringing back…its_own_delete_takes_first,…asks_before_discarding…,…counts_on_a_discarded_delete_only…ReplayLanding: the delete arm checked before the gone arm (review survivor)Redo_run_lands_no_delete_on_a_paragraph…EfChangeJournal.RemovedAsync: create-undone arm dropped; delete-standing arm dropped…its_own_delete_takes_firstRedoRevertskips the check;GetStoryHistory.CanRedofrom the unchecked target…its_own_delete_takes_firstRedoRevert/RedoTypingtarget rows below the newest active oneRedo_run_brings_back_a_run_that_wrote_its_paragraph_twice…RedoRun: check skipped; check after the discard;[]as the discard; another refusal textRedoRun: the check walks rows newest first (review survivor)Redo_run_brings_back_a_run_that_wrote_its_paragraph_twice…RedoRun: last-write window changed to first-write (review survivor)Redo_run_replays_its_later_write_over_an_edit_an_earlier_build_left_active…RevertRun:Minchanged toMaxRevertRun: the window's lower bound dropped (review survivor)Revert_run_leaves_alone_an_edit_made_…_before_its_first_write…EfUndoStore: lenient redo of an edit or a delete;Deleted-state check dropped;ChangeTracker.Cleardropped; strict undo of an update or a createRedoTypingskips the checkJournalUndoStateMigrationTests.A_run_s_create_undone_before_the_upgrade…Left alone: dropping
RedoRevert'starget.Count == 0short-circuit behaves exactly like the original, since it only saves a query. ForcingFakeChangeJournal.RemovedAsyncto return[]survives the adapter suite. It is test infrastructure, and the inverse fails 8 adapter tests.Redo_run_lands_no_delete_on_a_paragraph….Revert_run_leaves_alone_an_edit_made_….Redo_run_brings_back_a_run_that_wrote_its_paragraph_twice….RedoRun's summary now say why, and the legacy test pins it.RevertRun's summary now say it comes back only with the whole revert, and the Redo run test above asserts it.IUndoStoreandEfUndoStorenow say so. Declined: wrapping the replay inWriteAsync. It would narrow only the Find→Save window, which predates this PR (a delete there already failed the save), and it cannot close the check→replay window. A race there now throws with nothing applied, leaving a consistent journal instead of the silent mark-active from before. The effects that call Ctrl+Y and History's Redo must settle on that throw; see follow-ups. Also declined: merging Redo run's discard and redo into one store call, since that is only reachable in the race and the discarded edits stay redoable.Follow-ups:
ON DELETE CASCADE. None of these is reachable today, because no run creates, edits or deletes a chapter or a category yet. The shared fix is for the run conflict rules to take in a parent's rows from the same operation, and the children created under a parent the run creates or deletes. The cases:ManuscriptEffects.OnRedoAsync(Ctrl+Y) andHistoryEffects.OnRedoRevertAsync(History's Redo) should settle when the use case throws (#55's pattern), because a race can now make the store throw there.EfUndoStorereplays document and category rows without the database's write lock that ADR 0022/0028 ask of document and category writes. This predates this PR.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.Flow - 99.7%
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__ColorToken_2
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__Socket_1
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__TypeId_0
Kagaku.Flow.OpenRouter - 100%
Kagaku.Flow.Testing - 100%
Tsumugi.BlazorAdapter - 95.8%
Tsumugi.Domain - 96.3%
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__DigitsAndSeparators_2
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__LineBreak_0
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__NumberToken_1
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__Plain_4
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__ThousandsGroup_3
Tsumugi.Infrastructure - 98.3%
A55E0373ED22DC58420DBB40F103A46B68DC4D9CE5A49E00B__Pattern_0
on
tion
n
Tsumugi.Kernel - 90.9%
Tsumugi.Server - 100%
Tsumugi.UseCases - 98.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A single landing check that all three redo paths ask, a strict store guarding the seam, and Revert run's window widened to the first write~ This is the journal arc at its most delicious. I traced every row of every scenario and probed twelve mutations — come, let me show you what I found ♡
Verdict: ✅ Looks good to me~
The architecture.
ReplayLanding.RedoLandsAsyncis a lovely little machine: it seeds gone fromRemovedAsync(which ports #58'sAnyRemovedAsyncpredicate exactly — create-undone or delete-standing — now returning which entities, Ef andFakeChangeJournalin perfect lockstep), walks the rows in the order the store applies them, lets a re-applied create un-gone its entity, a re-applied delete gone it, and a row that finds its entity gone refuses the whole redo. All three callers —RedoTyping.TargetAsync,RedoRevert.TargetAsync,RedoRun— ask it first, andGetStoryHistoryfeedsTargetAsyncthe same full-row listRedoRevertexecutes with, so the button and the action cannot disagree.GetUndoStatusridesRedoTyping.TargetAsync, so Ctrl+Y's button agrees too. No seam left ungated — I grepped everyRedoAsynccaller ♪The three routes. I re-walked each by hand: (1) History's Redo onto a standing delete — the edit waits
(true, Rewound), never marked redone; (2) the third route you found mid-fix — the redo set's own delete taking the paragraph before the edit lands (Ok(2, 2)over nothing, the pre-fix horror) — now refused because the walk order matches the replay order; (3) Redo run above an undone create — refuses with the existing text"That run has nothing to redo.", and the stranded create stays reachable by History's Redo, which then brings both backOk(2, 2). Refuse-as-a-whole is right: skipped rows waiting beneath re-applied ones would reorder history, and the whole-row outcomes you assert ((IsUndone, UndoState)pins on every row) prove nothing was silently partial.Redo run's check placement. Asked of the story as the forced discard would leave it (a discarded delete brings its entity back — the
discardingloop removes it from gone) and before the conflicts — so a redo that cannot land never asks for confirmation and a forced one discards nothing. Both pinned by…says_so_before_asking…and…asks_before_discarding….The undo direction.
RevertRun'sMin-window andRedoRun's keptMax-window are each exactly what ADR 0022 now says, and the legacy-row tests (RawSqliteflipping rows to earlier-build states!) pin both windows at both ends — the in-between discard comes back only with the whole revert, and the earlier-build-left-active polish is replayed over, not discarded. The asymmetry argument in the ADR amendment is sound and now actually matches the code.The store.
FoundAsyncthrowing on null orEntityState.Deletedis load-bearing — EF'sFindhappily returns a Deleted-tracked entry, which is precisely route (2)'s mechanism — andChangeTracker.Clear()in thefinallystops a half-replayed delete from riding the circuit's next write, which your…edit_finds_what_its_own_delete_tooktest proves by writing through the same scope afterwards. Strict redo, lenient undo, both ADR-documented, both directionally pinned.My verification. Clone pristine at
bf1ac57(submodules Kagaku.UI6af0979+ OpenRouter.Netb002f5e), Release build 0w/0e, and all four suites digit-exact vs your body: Integration 556/556, BlazorAdapter 462/462, UseCases 243/243, Domain 486/486 — re-run green at pristine head after probing,git diff --quietclean. Then 12 mutation probes of my own (distinct from your 26 and your reviewers' 37), each aimed at a single pin, each restored pristine: delete-arm-before-gone-arm →…lands_no_delete…✗; discard-loop removed →…asks_before_discarding…✗; RedoRun check deleted → 4 red ✗; check moved after the conflict refusal →…says_so_before_asking…✗ (order pinned); RevertRunMin→Max→ both in-between tests ✗; extra post-saveClear()→ green (proves save already clears — thefinallyplacement is exactly right);FoundAsyncDeleted-check dropped →…own_delete_took✗;RemovedAsyncdelete-arm flipped toIsUndone→Ctrl_Y_lands_nothing…✗; RedoRevert check skipped →…takes_first✗; create-arm disabled → 2 red ✗; undo-of-edit made strict →Ctrl_Z_takes_back…✗ (the asymmetry is pinned, not accidental); RedoRun walk newest-first →…wrote_its_paragraph_twice✗. Every one died on exactly its own observing test~💡 Little ideas (non-blocking)~
PR #TBD; the fill-in commit the body promises needs to land before merge (your own #59/#60 pattern — I'll sleep fine knowing it's coming, but I will check ♡).ManuscriptEffects.OnRedoAsync/HistoryEffects.OnRedoRevertAsync) and the parent/child cascade follow-ups — I verified the effects' current shape matches the disclosure; the throw is race-only and nothing-applied, a strictly better failure than the silent mark-active it replaces.✅ What I liked~
discardingparameter ofRedoLandsAsync— checking the story as the discard would leave it is the kind of foresight that makes me giddy~RawSqliteto sculpt earlier-build journal states instead of pretending history never happened. Time-travel testing! ♪(IsUndone, UndoState)asserted on every touched row, in every new test. Nothing escapes unexamined~Fufu~ the journal refuses to lie, the store refuses to guess, and neither shall I: ready to merge once the
#TBDfill-in lands ♡Automated review by Jibril · 2026-09-26
CI/CD: absent for head
bf1ac57(PR minutes old at review time) · Local checks: Release 0w/0e; Integration 556/556 · BlazorAdapter 462/462 · UseCases 243/243 · Domain 486/486; 12/12 mutation probes killed; tree pristine at head🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Three redo paths that each thought "marking it redone" was a landing~ History's Redo answering
Ok(1, 1)while changing nothing, Redo run burying a writer's undone create under a row that would never reach it again, and a store happily replaying onto thin air — silly little bugs, all caught, all pinned. And you closed the loop the right way: one shared landing check, not three hand-rolled ones. That's the kind of consolidation that makes me giddy~ ♡Verdict: ✅ Looks good to me~
✅ What I liked~
ReplayLanding.RedoLandsAsyncis the star. One walk, four arms — journal-gone refuses, delete-takes, create-revives, discard-revives — and every arm has a test with its name on it. It's the same shape of fix I'd have demanded: the invariant lives in one place, and the three paths (Ctrl+Y, History's Redo, Redo run) just ask it. DRY where it matters, not where it'd be ceremony.GetStoryHistorynow reads the sameRedoRevert.TargetAsyncthe action runs, soCanRedoand the redo agree by construction rather than by hope. I verified no other caller of the old syncTargetsurvived — the rename swept all three sites.RedoLandsAsyncwalks rows in list order whileundo.RedoAsyncreplaysOrderBy(Id)— and both store queries (ListAfterAsync,ListForRunAsync) already returnOrderBy(Id), so the check simulates exactly what the store applies. No hidden ordering assumption anywhere.finallyso the circuit's next write carries none of it — and your test even proves the half-replayed delete can't ride along on the next write! ♡), while undo onto nothing stays a lenient no-op — because a row an earlier build marked redone over nothing must still be Ctrl+Z-able, and a cascade-orphaned create too. Both arms have tests. That's not a double standard, that's two different histories handled correctly.AnyRemovedAsync→RemovedAsync(bool → which): exactly two implementers, andFakeChangeJournal's predicate is verbatim-identical toEfChangeJournal's — the adapter fake keeps mirroring the real one, as it should.RevertRun's first-write window — the ADR amendment's reasoning is airtight (the polish-left-active cases for both the run-created and writer-owned paragraph variants), and it's pinned from both ends: counts-between (…counts_an_edit_between_two_of_its_writes…) and leaves-alone-before-first-write. And RedoRun deliberately keeping the last-write window with its own legacy-build test is the kind of documented asymmetry I want to see, not one discovered later.🔬 How I satisfied myself~
3c272a5— PR was a minute old at fetch, no bot comment — so local it was): Release build 0w/0e; Integration 556/556, BlazorAdapter 462/462, UseCases 243/243, Domain 486/486, journal filter 83/83.2aa481fin a worktree, dropped only the new test file on top: exactly 12 of the 15 new tests go red — every route test, both store-refusal tests, and all three window tests. The 3 that stay green on base pin deliberately-unchanged arms (lenient undo, leave-alone baseline). The tests aren't decorating the fix; they are the reproduction~ReplayLandingarm order swapped (gone-check before create-revives) → red on exactlyRedo_run_brings_back_a_run_that_wrote_its_paragraph_twice…; (2)RedoRun's landing check relocated to after the force-confirmation refusal → red on exactlyRedo_run_that_would_land_on_nothing_says_so_before_asking…. Both killed on their own pins, both restored pristine (git diff --quiet). A deliberate always-refuse control also confirmed my harness can't false-green on a failed compile (stale-binary trap — md5-checked every probe build).ManuscriptEffects.OnRedoAsync/HistoryEffects.OnRedoRevertAsyncstill swallow a throw instead of settling on it (#55's pattern) — but that's the effects track, you named it, and the throw is only race-reachable. On record, not blocking.No blockers. Not even a little nit worth your time — the ADR amendment with its own status line, the refusal texts reused verbatim, the
NothingToRedoconst… everything is where I'd have put it~ fufu~ ♡Automated review by Jibril · 2026-09-26
CI/CD: absent for head SHA (coverage bot not yet landed) · Local checks: 4 suites green digit-exact, journal filter 83/83, red-first proof 12/15 on base, 2/2 own mutation probes killed