fix: close undo ledger holes for axis deletes and glyph renames#121
Merged
Conversation
ledger_steps had no arm for AxisDeleted or GlyphIdentityChanged, so undoing a deleteAxis or updateGlyph popped the entry, replayed nothing, and echoed success — the axis definition / old glyph identity was unrecoverable and the next undo target was consumed. - capture the full Axis (and its per-source location values, which Font::remove_axis strips) before a DeleteAxis applies - derive the GlyphIdentity pre/post pair from the change record itself - replace the ledger_steps wildcard with an explicit variant list so a new FontChange kind forces an undo decision at compile time - add Font::source_mut so replay can restore stripped source locations
undo/redo popped the entry before replaying and dropped it on error, so a transient store failure silently erased that history step. On a failed replay the entry is now re-pushed onto the stack it came from and the error propagates, matching the hand-back semantics the Ledger doc comment promised.
Per-intent matrix: apply, then assert undo restores the exact pre state and redo the exact post state (layer snapshots compare by equality). Also covers failed-replay hand-back for undo and redo via a wiped store, and MAX_ENTRIES trimming of the oldest ledger entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the highest-severity in-session data-loss bugs from the 2026-07 review:
DeleteAxisandUpdateGlyphhad no ledger steps — undo popped the entry, replayed nothing, and reported success, leaving the axis/old identity unrecoverable and consuming the next undo target. Failed replays also permanently dropped ledger entries.AxisDeletedgains a ledger step carrying the full axis and per-source location values —Font::remove_axisstrips the axis's value from every source location (with SQLite cascading), so the axis alone was insufficient pre-state; undo restores locations FK-safely in the same transaction.GlyphIdentityChangedgains a step (pre/post name + unicodes derived from the change record)._ => {}wildcard inledger_stepsis gone — the ten deliberately step-less variants are listed explicitly and documented, so any futureFontChangevariant is a compile error until its undo story is decided.Ledger::restore_undo/restore_redo), matching the doc comment's promised semantics; the font is untouched on failure, so retry is safe. Both paths have real injection tests (wipe store rows → replay fails → entry restored → repair → retry succeeds).Font::source_mutaccessor in shift-font (mirrorslayer_mut) — needed to restore stripped locations; flagged for review as an IR API addition.Tests: full apply→undo→assert-pre→redo→assert-post matrix over all 20
FontIntentkinds against real SQLite (shift-workspace 24 → 44 tests), plus previously-untestedMAX_ENTRIEStrimming.cargo test --workspace: 319 passed, 0 failed. clippy zero warnings.🤖 Generated with Claude Code