diff --git a/CLAUDE.md b/CLAUDE.md index 0d0cb38..418972b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -58,9 +58,14 @@ Current decisions at a glance (each backed by an ADR): supported. No real UUIDs (ADR-0005). FK column type compatibility via `Type::fk_target_type()` — `serial → int`, `shortid → text`, others identity (ADR-0011). -- **Safety:** auto-snapshot before destructive ops; `:undo`; - append-only `history.log` for replay and scripting (ADR-0006). - *(Designed; not yet implemented.)* +- **Safety:** append-only `history.log` for replay and scripting + (ADR-0006 U3/U4) — *implemented* (ADR-0034). Undo/snapshot half + (U1/U2): `undo` / `redo` app commands (no sigil) with auto-snapshot + before **every** mutation into a persisted N=50 ring; hybrid + whole-project snapshot (db backup API + yaml/csv copy); `--no-undo` + to disable (ADR-0006 **Amendment 1**). *(Designed + amended + + planned in `docs/plans/20260524-adr-0006-undo-snapshots.md`; not + yet implemented.)* - **Sharing:** `export` command produces a zip without the `.db`; no hosted publishing (ADR-0007). - **Testing:** four-tier strategy from `cargo test` units up to diff --git a/docs/adr/0006-undo-snapshots-and-replay-log.md b/docs/adr/0006-undo-snapshots-and-replay-log.md index c2da619..fe6776b 100644 --- a/docs/adr/0006-undo-snapshots-and-replay-log.md +++ b/docs/adr/0006-undo-snapshots-and-replay-log.md @@ -87,3 +87,123 @@ authoritative state of the project (that lives in `project.yaml` with no additional storage commitment. - Tutorial authors gain a natural "starter script" format for exercises. + +## Amendment 1 — Single-step undo: every-mutation snapshots, hybrid storage, batch granularity (2026-05-24) + +The replay/journal half of this ADR (U3/U4) shipped via ADR-0034. This +amendment settles the **undo/snapshot half (U1/U2)** before +implementation, and **supersedes the original Decision's +"snapshots only before destructive operations" model** and its +confirmation rationale. Written with explicit user approval; the +implementation plan is `docs/plans/20260524-adr-0006-undo-snapshots.md`. +**Not yet implemented** at the time of writing — this records the +agreed design. + +### Snapshot scope — every mutation (single-step undo) + +The original Decision snapshots "before any destructive operation — +`DROP`, `DELETE`, `TRUNCATE`, schema-rebuild migrations, restore" and +explicitly treats inserts/updates/schema-additions as *non-destructive +work between snapshots*. That is **replaced**: a snapshot is taken +before **every** data/schema mutation — insert, update, delete, drop, +all DDL, and all SQL DML. Undo therefore behaves like a familiar +single-step "undo my last command" (Ctrl-Z), which is the right model +for this teaching environment (clarity over micro-optimisation, per +the project's "pedagogy wins ties" posture). + +A consequence is that the original confirmation clause — "counts of +rows added/modified/deleted per table and any schema changes since the +snapshot" — **collapses**: with a snapshot per command there is no +intervening un-snapshotted work, so undo rolls back exactly one +command and the confirmation simply **names that command**. No +db-diff machinery is needed. + +### Confirmation rationale + +The original justified the always-on prompt by "undo is rare and +consequential." Under single-step undo, undo is *more frequent*, but +the prompt is **kept** anyway, now justified by "the prompt names the +exact command being undone." There is still no flag to suppress it. +`undo` and `redo` each confirm (`Y` confirms; `N`/`Esc` cancels), +mirroring the existing `rebuild` modal. The redo prompt names the +command that will be re-applied. + +### Snapshot mechanism — hybrid db + text (reconciles ADR-0015) + +The original specifies SQLite's online backup API. Since then, +ADR-0015 made `playground.db` a *derived* artifact with +`project.yaml` + `data/*.csv` as the authoritative source, committed +last for crash recovery. A db-only restore is therefore no longer +sufficient on its own. The agreed mechanism is a **hybrid +whole-project snapshot**: + +- the database is copied via the **online backup API** (honouring this + ADR; it is also the only safe way to copy a live database), **and** +- `project.yaml` + `data/*.csv` are copied as inert files. + +Undo **restores all three directly** — no rebuild, no re-derivation — +re-establishing a consistent `(db, yaml, csv)` triple. This satisfies +both this ADR (the backup API *is* used) and ADR-0015 (text remains +authoritative). The snapshot is staged *before* the mutation's +transaction and finalised into the ring *after* the database commit, +preserving ADR-0015 §6's commit-db-last ordering; a rolled-back +operation leaves no snapshot. + +### Storage and lifetime — persisted ring, N = 50 + +Snapshots are **persisted on disk** under the project in a +`.snapshots/` directory and survive quit (undo works after reopening). +The ring keeps the most recent **N = 50** snapshots (the original's +N = 10 is raised, since single-step undo means N counts *commands*; +still a single tunable constant), evicting the oldest on overflow. +`.snapshots/` is added to the `.gitignore` template, **excluded from +`export`** (like `playground.db` and `history.log`), and on the +temp-project cleanup allowlist so an otherwise-empty temp carrying a +snapshots directory remains safely deletable. + +### Redo + +`redo` is supported (as the original states). New semantics are +pinned: **the redo stack is discarded on any new mutation** (standard +linear undo/redo). Each undo pushes the pre-undo state so redo can +restore it. + +### Batch operations — one undo step; `import` excluded + +A single user command that runs many sub-operations — `replay` today, +and any future in-project batch command — records **one** boundary +snapshot for the whole batch (not one per sub-command), via a +Begin/EndBatch worker primitive that suppresses per-command staging and +finalises a single ring entry only if ≥1 mutation actually ran. This is +a performance win (a long `history.log` replay is one database copy, +not N) and the consistent reading of "one undo step per user command." + +`import` is **outside** the undo model entirely: per ADR-0015 §11 it +creates a *new* project and switches to it, leaving the current project +untouched on disk, so there is nothing to snapshot and it takes no undo +step (the new project simply starts with an empty ring). Project-switch +navigation undo ("go back to the previous project") is a separate, +out-of-scope mechanism — the prior project is intact and reachable via +`load` / `--resume`. + +### Disable switch + +A `--no-undo` CLI flag turns snapshotting off entirely (zero +per-command overhead), as a hardware escape hatch should per-command +snapshots prove too heavy. When set, `undo` / `redo` report that undo +is turned off. CLI-only for v1 (no in-app toggle). + +### Consequences + +- Per-mutation snapshotting costs one database backup + a text copy per + command; a bulk paste of N inserts makes N snapshots. Bounded by the + N = 50 ring and the `--no-undo` escape hatch; the ADR-0015 "batch" + command remains the future remedy, and a hardlink/copy-on-write dedup + of unchanged files between consecutive snapshots is a possible future + optimisation (not v1). +- 50 × (database + text) of persisted snapshots can reach tens of MB + for larger projects — an accepted, bounded cost. +- `Cargo.toml` gains the `backup` feature on `rusqlite`. +- The Phase-3 N/A matrix row ("auto-snapshot fires for SQL DML the same + as DSL") becomes non-vacuous: the snapshot hook lives in the worker + dispatch and covers DSL and SQL mutations uniformly. diff --git a/docs/adr/README.md b/docs/adr/README.md index 0fa228d..89e9cfb 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -11,7 +11,7 @@ This directory contains the project's ADRs, recorded per - [ADR-0003 — Input modes and command dispatch](0003-input-modes-and-command-dispatch.md) - [ADR-0004 — Project file format](0004-project-file-format.md) - [ADR-0005 — Column type vocabulary](0005-column-type-vocabulary.md) -- [ADR-0006 — Undo snapshots and replay log](0006-undo-snapshots-and-replay-log.md) +- [ADR-0006 — Undo snapshots and replay log](0006-undo-snapshots-and-replay-log.md) — **Accepted**. The **replay/journal half** (U3/U4) shipped via ADR-0034; the **undo/snapshot half** (U1/U2) is settled by **Amendment 1 (2026-05-24)** but **not yet implemented** (plan: `docs/plans/20260524-adr-0006-undo-snapshots.md`). Amendment 1 **supersedes the original "snapshots only before destructive operations" model**: a snapshot is taken before **every** data/schema mutation (DSL + SQL) for familiar single-step (Ctrl-Z) undo — so the confirmation collapses to *naming the one command being undone* (no db-diff). Snapshot is a **hybrid whole-project copy** — database via the online backup API **plus** `project.yaml`/`data/*.csv` as files — reconciling this ADR with ADR-0015's "text is authoritative, db is derived"; undo restores all three directly. Staged before the mutation's transaction, finalised after the db commit (preserves ADR-0015 §6 commit-db-last); rolled-back ops leave no snapshot. **Persisted** ring under `.snapshots/`, **N = 50** (raised from 10), git-ignored + export-excluded + temp-cleanup-aware. `redo` supported, **redo stack discarded on new work**. **Batch ops record one undo step** (`replay` + future batch via a Begin/EndBatch worker primitive); **`import` is outside undo** (it switches projects per ADR-0015 §11, leaving the current project untouched). A **`--no-undo` CLI flag** disables snapshotting (hardware escape hatch). Adds the `backup` feature to `rusqlite` - [ADR-0007 — Sharing and export](0007-sharing-and-export.md) - [ADR-0008 — Testing approach](0008-testing-approach.md) - [ADR-0009 — DSL command syntax conventions](0009-dsl-command-syntax-conventions.md) diff --git a/docs/plans/20260524-adr-0006-undo-snapshots.md b/docs/plans/20260524-adr-0006-undo-snapshots.md new file mode 100644 index 0000000..045d218 --- /dev/null +++ b/docs/plans/20260524-adr-0006-undo-snapshots.md @@ -0,0 +1,482 @@ +# Plan: ADR-0006 undo / snapshot half (U1/U2) + +**Date:** 2026-05-24 · **Author:** session 36 · **ADR:** 0006 +(undo snapshots and replay log) — replay half (U3/U4) shipped in +ADR-0034; this plan covers the **undo / snapshot half (U1/U2)**. + +**Status of this doc:** draft for review. The load-bearing design +calls below were confirmed with the user before writing (see §2); +the second-order calls in §10 are recommendations awaiting sign-off. +No code is written until this plan is approved. + +--- + +## 1. Baseline + +- `cargo test`: green (exit 0). Handoff 35 records **1659 passing / + 0 failing / 0 skipped / 1 ignored** (the `friendly/mod.rs` + doctest). This is the Phase-1 baseline Phase-5 is measured against. +- `cargo clippy --all-targets -- -D warnings`: clean (per handoff). + +--- + +## 2. Decisions locked with the user (do not re-litigate) + +All confirmed this session via direct Q&A. + +1. **Snapshot mechanism — hybrid whole-project snapshot.** A + snapshot stores the database (copied via SQLite's **online backup + API**, the only safe way to copy a live db) **plus** the + authoritative text (`project.yaml` + `data/*.csv`, copied as inert + files). Undo **restores all three directly** — no rebuild, no + re-derivation. This satisfies **both** ADR-0006 (backup API *is* + used) **and** ADR-0015 (text remains the authoritative copy), so + the *mechanism* needs no ADR amendment. Rationale for storing the + db too (rather than text-only + rebuild): the user prioritised a + direct, robust restore for a safety feature over the smaller disk + footprint of a rebuild-on-undo approach. +2. **Lifetime — persisted on disk, ring capped at N = 50.** Snapshots + live under the project directory and survive quit (undo works + after reopening). The ring keeps the most recent **50** snapshots; + the oldest is evicted on overflow. `50` is a single tunable + constant. +3. **Redo — discarded on new work.** Standard linear semantics: any + new mutation after an undo clears the redo stack. +4. **Scope — every mutation (single-step, Ctrl-Z-style undo).** A + snapshot is taken before **every** data/schema mutation (insert, + update, delete, drop, all DDL, all SQL DML), not only before + destructive ops. The user judged this the right model for the + playground's teaching philosophy. **This supersedes two pillars of + ADR-0006** and requires an amendment (see §4). +5. **`undo` requires confirmation** that names the command being + undone. (Confirmation is kept per the user's explicit instruction, + even though single-step undo is more frequent than the ADR's + original "rare and consequential" framing.) +6. **Disable switch — in the picture from day one.** A way to turn + undo off entirely, so per-command snapshotting can be avoided if it + proves too heavy on small hardware. Shape: a `--no-undo` CLI flag + (CLI-only for v1, no in-app toggle — confirmed). +7. **Batch operations record ONE undo step.** A single user command + that executes many sub-operations — `replay` today, and any future + in-project batch command — takes **one boundary snapshot** for the + whole batch, not one per sub-command. Undo rolls the batch back + wholesale. This is both a perf win (replaying a long `history.log` + is one db backup, not N) and the consistent reading of "one undo + step per user command." **`import` is *not* a batch op** — it + creates a new project and switches to it (ADR-0015 §11), leaving the + current project untouched on disk, so it sits **outside** the + per-project undo ring entirely (the new project just starts with an + empty ring). Project-switch-level "undo back to the previous + project" is a separate, out-of-scope mechanism. + +--- + +## 3. Phase 1 — Requirements checklist + +From ADR-0006 (as amended), the user decisions in §2, and the +project's working standards. + +### Functional + +- [ ] **R1** Auto-snapshot before every mutation (DSL + SQL), gated by + the enable flag. +- [ ] **R1b** A **batch** command (`replay`, future batch ops) records + exactly one boundary snapshot for the whole batch (not one per + sub-command), via the Begin/EndBatch primitive; finalized only if + ≥1 mutation actually ran. `import` takes **no** undo step (project + switch, not a current-db mutation). +- [ ] **R2** A snapshot captures db (backup API) + `project.yaml` + + `data/*.csv`, atomically, representing the **pre-mutation committed** + state. +- [ ] **R3** Snapshots are committed into the ring **only after** the + triggering mutation commits successfully; a rolled-back op leaves no + snapshot. Preserves ADR-0015 §6 commit-db-last ordering. +- [ ] **R4** Persisted ring buffer under the project dir, cap 50, + oldest evicted on overflow. +- [ ] **R5** `undo` app command (both modes, no sigil): confirmation + modal naming the command to be undone + its relative timestamp → + on confirm, restore the top snapshot (db + text) → pop the ring. +- [ ] **R6** Undo pushes the pre-undo (current) state onto a redo + stack so `redo` is possible (ADR-0006). Redo restores it. +- [ ] **R7** `redo` app command restores the top redo entry. New + mutation clears the redo stack (R-decision 3). +- [ ] **R8** Undo/redo into an empty stack reports a friendly note + ("nothing to undo" / "nothing to redo"); never errors. +- [ ] **R9** Disable switch (`--no-undo`): when set, no snapshots are + taken (zero per-command overhead) and `undo`/`redo` report that undo + is disabled. +- [ ] **R10** Restore correctly re-establishes a consistent (db, yaml, + csv) triple, including the NULL-vs-empty CSV distinction and + internal metadata tables (`__rdbms_playground_*`). +- [ ] **R11** After undo/redo, the TUI refreshes its view (re-query) + so the user sees the restored state. + +### Cross-cutting / integration + +- [ ] **R12** `undo`/`redo` added to `runtime::is_app_lifecycle_entry_word` + so **replay skips them** (ADR-0034 Amendment 1) — and to the + completion entry-keyword set (`src/completion.rs`), which the + function's doc-comment says must stay in lockstep. +- [ ] **R13** Snapshots dir added to the `.gitignore` template + (`src/project/mod.rs`), the **export exclusion** (`src/archive.rs` — + like `playground.db` and `history.log`), and the **temp-project + cleanup allowlist** (`safely_delete_temp_project`) so an otherwise- + empty temp carrying a snapshots dir is still safely deletable. +- [ ] **R14** `Cargo.toml`: add `"backup"` to the `rusqlite` feature + list. +- [ ] **R15** Engine-neutral user-facing strings (ADR-0002): no + "SQLite" / "backup API" / "PRAGMA" in notes or modal text. Use + "snapshot" / "the database". + +### Documentation + +- [ ] **R16** Write an **ADR-0006 amendment** (see §4); update + `docs/adr/README.md` in the same edit (ADR-0000 index rule). +- [ ] **R17** Update `docs/requirements.md` U1/U2 from designed → + implemented; update `CLAUDE.md`'s ADR-0006 "Designed; not yet + implemented" line. + +### Testing (ADR-0008 four tiers) + +- [ ] **R18** Tier 1 (unit/pure): ring eviction, redo-clear-on-new- + work, mutation-vs-readonly classification, modal key handling, + command parsing, disable-flag gating, CLI parse. +- [ ] **R19** Tier 2 (insta snapshots): undo/redo confirmation modal + rendering; disabled-undo note. +- [ ] **R20** Tier 3 (integration, real db + temp project): snapshot + taken on mutation; undo restores db **and** text; redo; ring + eviction at the cap; redo cleared by new work; `--no-undo` takes no + snapshots; persistence-ordering invariant preserved. +- [ ] **R21** Full-stack flow (per global standards): bootstrap a real + project, run a sequence of DSL **and SQL** mutations, undo/redo + across both, verify read-model + on-disk yaml/csv/db all consistent, + reopen the project and confirm the ring persisted. +- [ ] **R21b** Batch: a `replay` of a multi-command file produces + exactly **one** ring entry; undo rolls the whole replay back; an + all-skips replay leaves no undo step; `import` produces no undo step + in the current project. +- [ ] **R22** Phase-3 N/A matrix row closed ("auto-snapshot fires for + SQL DML the same as DSL") — now non-vacuous. +- [ ] **R23** End state: all green, **zero skips**, no regression vs + the §1 baseline. + +--- + +## 4. ADR-0006 amendment scope + +The every-mutation decision (§2.4) contradicts ADR-0006 as written, +so an amendment is required (the ADR is *Accepted* — we amend in place +with a dated note, per the project's "no silent drift" rule). The +amendment records: + +- **Snapshot scope:** "before every destructive operation" → + "before every data/schema mutation" (single-step undo). State the + pedagogical rationale (intuitive Ctrl-Z; the playground favours + clarity over micro-optimisation). +- **Confirmation rationale:** the original "undo is rare and + consequential, so always confirm" is replaced by "undo is + single-step and confirmation names the exact command being undone." + Confirmation is still always shown; there is still no suppress flag. + The summary simplifies — with per-command snapshots, undo rolls back + exactly one command, so the "discarded changes" summary is just that + command's description (no expensive db-diff needed). This resolves + ADR-0006's "counts of rows added/modified/deleted per table since + the snapshot" clause: under single-step undo there is no intervening + un-snapshotted work, so the clause collapses to "the one command." +- **Snapshot mechanism:** clarify the hybrid (db via backup API + + text via copy) and how it reconciles with ADR-0015 (db is derived; + text authoritative) — restore re-establishes the whole triple. +- **Storage & lifetime:** persisted ring under the project, cap 50, + git-ignored, export-excluded, temp-cleanup-aware. +- **Redo:** discarded on new work. +- **Disable switch:** `--no-undo` as a hardware escape hatch. +- **Batch granularity:** a single user command that runs many + sub-operations (`replay`, future batch ops) is one undo step, taken + at the batch boundary. Consistent with "one undo step per user + command." `import` is a project switch and takes no undo step. + +--- + +## 5. Architecture & design + +### 5.1 On-disk layout + +``` +/ + project.yaml + data/.csv + playground.db + .snapshots/ + index.json # ordered ring + redo stack: ids, timestamps, + # command text, status; the source of truth + # for ordering/eviction + 0001/ + playground.db # backup-API copy of the pre-op db + project.yaml # pre-op copy + data/
.csv # pre-op copies + 0002/ ... + .staging/ # temp area for an in-flight snapshot +``` + +- **`.snapshots/` is git-ignored, export-excluded, and on the + temp-cleanup allowlist** (R13). +- The **ring** (undo) and the **redo stack** are both recorded in + `index.json`; snapshot payload dirs are shared storage referenced by + id. Eviction beyond 50 deletes the oldest payload dir. +- Ids are monotonic; never reused, to avoid stale references. + +### 5.2 Snapshot mechanism + +- **db copy:** `rusqlite::backup` — `Backup::new(&live, &snap)` copies + live → snapshot file; for restore, `Backup::new(&snap, &live)` copies + snapshot → live (works on the open worker connection; requires the + `backup` cargo feature, R14). No transaction may be open on the live + connection during a backup step. +- **text copy:** plain file copy of `project.yaml` + every + `data/*.csv` into the snapshot dir. (CSVs are produced by the + project's hand-rolled writer that preserves NULL-vs-empty; we copy + the already-written files verbatim, so the distinction is preserved + for free — R10.) + +### 5.3 Timing vs ADR-0015 §6 (the load-bearing invariant) + +A snapshot brackets the existing 4-step persistence sequence: + +``` +0. STAGE snapshot (NEW) — backup live db + copy current text into + .snapshots/.staging/. The current on-disk + state is the last committed state (db, yaml, + csv are committed together per ADR-0015), so + staging captures the true pre-op state. +1. open txn; validate; mutate db + metadata (ADR-0015 step 1) +2. write text temp files; fsync (step 2) +3. atomic-rename text into place (step 3) +4. commit db (step 4) +5. FINALIZE snapshot (NEW) — atomic-rename .staging/ → .snapshots//, + append to index.json ring, evict oldest if + >50, clear redo stack. + On any failure in 1–4 → txn rolls back; DISCARD .staging/. +``` + +This **preserves commit-db-last**: the snapshot is finalised *after* +the db commit, and a rolled-back op leaves no ring entry (R3). A crash +mid-sequence leaves a `.staging/` orphan, cleaned on next open exactly +like ADR-0015's orphan text temps. + +### 5.4 Worker chokepoint (single point, not per-handler) + +Intercept centrally in `db.rs::handle_request` (the dispatcher), not +in each `do_*` handler: + +- **Classify** each `Request` as *mutating* or *read-only* (a single + `is_mutating(&Request) -> bool`, exhaustively matched so a new + request variant forces a classification decision at compile time). +- For mutating requests, **wrap** the handler call: + `stage_snapshot()` → run handler → on `Ok` `finalize_snapshot()` + + clear redo; on `Err` `discard_staging()`. +- The command text for the snapshot meta comes from the request's + existing `source: String` field (added during the ADR-0034 journal + work). +- **Refactor note:** mutating handlers currently send their reply via + the `oneshot` inside the match arm. To let the wrapper observe + success/failure, mutating handlers return their `Result` to the + dispatcher, which sends the reply **and** drives snapshot + finalize/discard. This is the main structural change in `db.rs`; + read-only handlers are untouched. (`RebuildFromText` is mutating and + snapshots like the rest; it reconstructs from text, but the pre-op + db+text are still the correct undo target.) +- **Gate:** when undo is disabled (R9), `is_mutating` still holds but + the stage/finalize calls are no-ops — the worker is told at + construction whether snapshotting is enabled. + +### 5.5 Undo / redo flow (commands → modal → worker) + +Mirrors the existing two-phase `rebuild` modal flow, with the +**prepare** step reading snapshot metadata rather than the project: + +1. `undo` parses to `Command::App(AppCommand::Undo)` → + `dispatch_app_command` returns `Action::PrepareUndo`. +2. Runtime handles `PrepareUndo`: reads `.snapshots/index.json` top + entry (command text + timestamp) — a cheap file read, like + `summarize_project` does for rebuild — and posts + `AppEvent::UndoPrepared { command, when }` (or + `UndoUnavailable` if the ring is empty → friendly note, R8). +3. `App::update` opens `Modal::UndoConfirm` showing "Undo + `` (run )? [Y] yes [N] no". `Y` → + `Action::Undo`; `N`/`Esc` → close + "undo cancelled". +4. Runtime handles `Action::Undo` → worker `Request::Undo`: + - push a snapshot of **current** state onto the redo stack, + - restore the top ring snapshot (backup-restore db + rename text + back), pop the ring, + - reply with the undone command text. +5. Runtime posts `AppEvent::UndoSucceeded { command }` / + `UndoFailed { error }`; `App` closes the modal, notes the outcome, + and triggers a re-query so the view refreshes (R11). + +`redo` is symmetric (`PrepareRedo` / `Redo`, `Request::Redo`, restores +the top redo entry, pushes current onto the undo ring). Whether redo +also confirms is an open sub-decision (§10). + +### 5.6 Batch operations — one undo step (Begin/EndBatch) + +`run_replay` (`runtime.rs:1622`) dispatches **one worker request per +replayed command** via `execute_command_typed`. Left to §5.4, a long +replay would stage one snapshot per line. Instead, a reusable batch +primitive collapses a batch to a single ring entry: + +- `Request::BeginBatch { source }` — stages **one** snapshot (meta = + the batch command, e.g. `replay history.log`) and sets the worker's + `in_batch` flag. While set, per-request staging in §5.4 is **skipped** + (the boundary snapshot already captured pre-batch state). +- `Request::EndBatch` — finalizes the staged snapshot into the ring + **only if ≥1 mutation occurred** during the batch (else discards it, + so an all-skips replay leaves no no-op undo step); clears `in_batch` + and the redo stack. +- `run_replay` brackets its loop with begin/end. A mid-replay + `PersistenceFatal` tears the runtime down before `EndBatch`, leaving + a `.staging/` orphan that is cleaned on next open (ADR-0015 parity); + undo state is moot at that point anyway. +- The same primitive serves any future in-project batch command. +- `import` does **not** use this — it creates a new project and + switches (ADR-0015 §11); the current project is untouched, so there + is nothing to snapshot and no undo step. `rebuild` is already a + single request (`RebuildFromText`) → naturally one snapshot, no + batch handling needed. + +### 5.7 New / changed types (from the Explore survey) + +| Layer | Change | +|---|---| +| `Cargo.toml` | add `"backup"` to `rusqlite` features | +| `src/cli.rs` | `Args.no_undo: bool`; parse `--no-undo` | +| `src/dsl/command.rs` | `AppCommand::Undo`, `AppCommand::Redo` | +| `src/dsl/grammar/app.rs` | `UNDO` / `REDO` `CommandNode` (EMPTY_SEQ, like `REBUILD`) | +| `src/action.rs` | `PrepareUndo`, `Undo`, `PrepareRedo`, `Redo` | +| `src/event.rs` | `UndoPrepared`/`UndoUnavailable`/`UndoSucceeded`/`UndoFailed` (+ redo) | +| `src/app.rs` | `Modal::UndoConfirm` (+ redo) struct; event arms; `handle_undo_confirm_key`; dispatch arms | +| `src/ui.rs` | `render_undo_confirm` (mirror `render_rebuild_confirm`) | +| `src/db.rs` | `is_mutating`; `stage/finalize/discard_snapshot`; `Request::Undo`/`Redo`/`BeginBatch`/`EndBatch`; dispatcher wrap; `undo_enabled` + `in_batch` worker fields | +| `src/persistence/` (or new `src/snapshots/`) | ring + redo store, `index.json`, eviction, restore | +| `src/runtime.rs` | `is_app_lifecycle_entry_word += undo,redo`; `PrepareUndo/Undo` (+redo) handling; thread `no_undo` to worker; bracket `run_replay` with Begin/EndBatch | +| `src/project/mod.rs` | `.snapshots/` in `.gitignore` template | +| `src/archive.rs` | exclude `.snapshots/` from export | +| `safely_delete_temp_project` | `.snapshots/` on the contents allowlist | +| `src/completion.rs` | `undo`/`redo` in app-command entry keywords | +| `src/friendly/strings/en-US.yaml` | modal title/prompt, success/cancel/disabled notes, CLI help | + +--- + +## 6. Phase 2/3 — candidates considered (condensed) + +The mechanism choice was the crux; three candidates were evaluated and +the user selected the hybrid (§2.1). Recorded here so the rejected +options aren't silently dropped: + +| Candidate | Stores | Undo restore | Verdict | +|---|---|---|---| +| **C1** db-only via backup API | db | restore db + re-derive text via `finalize_persistence` | Honors ADR-0006 letter; needs a new "db→all-text" re-derive path. Rejected: less direct restore. | +| **C2 (chosen)** hybrid | db + yaml + csv | restore all three directly | Most robust/direct restore; satisfies ADR-0006 + ADR-0015 with no mechanism amendment. **Selected.** | +| **C3** text-only | yaml + csv | restore text + `rebuild_from_text` | Smallest disk; reuses tested rebuild. Rejected: rebuild dependency at undo time is less direct for a safety feature. | + +Scope candidates (destructive-only / +UPDATE / every-mutation) and +redo semantics were likewise put to the user; selections in §2. + +--- + +## 7. Devil's Advocate review of this plan + +- **Per-mutation snapshot cost.** Backing up the db + copying all CSVs + before *every* mutation is the real cost; a bulk paste of N inserts + makes N snapshots. *Mitigations:* teaching-scale dbs are small; + ring cap 50 bounds total storage; `--no-undo` is the escape hatch; + the ADR-0015 "batch command" remains the future remedy. A + hardlink/CoW optimisation for unchanged files between consecutive + snapshots is noted as a **future** improvement, not v1. +- **Disk growth.** 50 × (db + text) could reach tens of MB for larger + projects. Honest consequence; bounded by the cap and the disable + flag. Flagged in the amendment's Consequences. +- **Backup API + open transaction.** A backup step requires no open + txn on the live connection. The stage step runs *before* the + mutation txn opens, and restore runs outside any txn — safe by + construction, but tests must assert it (R20). +- **Restore must include internal metadata tables.** The db backup is + byte-faithful so `__rdbms_playground_*` tables come along + automatically; the text copy carries the yaml/csv. R10 test guards + this explicitly. +- **Temp-cleanup interaction.** A mutation in a fresh temp creates + `.snapshots/`; if the user then undoes back to empty, an empty temp + may carry a `.snapshots/` dir. The allowlist entry (R13) keeps + `safely_delete_temp_project` willing to delete it; a test covers it. +- **Replay safety.** Without R12, a `history.log` containing `undo` + would abort/panic replay (ADR-0034). Adding to both the runtime + filter and the completion set is mandatory, not optional. +- **`is_mutating` exhaustiveness.** Must be a non-`_` match so any + future `Request` variant fails to compile until classified — + prevents a new mutation silently escaping the snapshot hook. + +No DA finding is deferred; all are addressed above or tracked as +explicit future items (hardlink optimisation) for user awareness. + +--- + +## 8. Implementation sequence (test-first throughout) + +1. **Cargo + CLI:** add `backup` feature; `--no-undo` flag + parse + tests. (R14, R9-partial) +2. **Snapshot store module:** `index.json` model, ring + redo, stage/ + finalize/discard/restore, eviction — Tier-1 tests first against a + temp dir + in-memory/temp db. (R2, R3, R4, R6, R7) +3. **Worker integration:** `is_mutating` (exhaustive), dispatcher wrap, + `undo_enabled` gate, `Request::Undo`/`Redo`. Tier-3 tests: + mutation→snapshot, undo restores db+text, redo, eviction, + redo-clear, ordering preserved, `--no-undo` no-ops. (R1, R3, R10) +4. **Commands + grammar:** `AppCommand::Undo/Redo`, grammar nodes, + parse tests; replay-filter + completion entries + their lockstep + test. (R5, R7, R12) +5. **Action/event/modal/UI:** prepare→confirm→execute flow; modal key + handling (Tier 1); modal + disabled-note rendering (Tier 2 insta); + re-query on success. (R5, R8, R11, R19) +6. **Cross-cutting files:** `.gitignore` template, export exclusion, + temp-cleanup allowlist — each with a test. (R13) +7. **Full-stack flow** mixing DSL + SQL mutations, undo/redo across + both, reopen-and-verify-persistence. (R21, R22) +8. **Docs:** ADR-0006 amendment + README index; requirements U1/U2; + CLAUDE.md line. (R16, R17) +9. **Phase 5 verification:** full suite green, zero skips, no + regression vs §1; DA final pass. (R23) + +--- + +## 9. Engine-neutral string notes (ADR-0002) + +User-facing text uses "snapshot", "undo", "the database". Never +"SQLite", "backup API", "PRAGMA", "VACUUM". Modal: "Undo `` +(run )?". Disabled: "Undo is turned off for this +session." Empty: "Nothing to undo." These wordings are placeholders +pending the catalog edit. + +--- + +## 10. Sub-decisions — all confirmed (2026-05-24) + +1. **Disable mechanism shape:** `--no-undo` CLI flag only for v1 (no + in-app toggle). +2. **Redo also confirms,** naming the command to re-apply (symmetry + with undo). +3. **Redo is in scope** for this pass (discard-on-new-work semantics). +4. **Confirm key:** `Y` confirms / `N` / `Esc` cancel, matching the + rebuild modal. + +**Still open (one question to the user):** whether project-switch-level +"undo back to the previous project" should be tracked as a separate +future feature. Default if unanswered: leave `import`/project-switch +wholly outside undo (the prior project is intact on disk and reachable +via `load` / `--resume`). + +--- + +## 11. Next action after approval + +On sign-off: write the ADR-0006 amendment first (it's the contract), +update the README index, then implement per §8 — test-first, escalate +any further ADR-vs-implementation mismatch rather than deciding. diff --git a/docs/requirements.md b/docs/requirements.md index 1db575d..f4e75b6 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -365,11 +365,23 @@ handoff-14 cleanup; 449 after B2/C2.) ## Undo and replay (per ADR-0006) -- [ ] **U1** Auto-snapshot before destructive operations into a - ring buffer (initial size N=10, tunable). -- [ ] **U2** `undo` restores the most recent snapshot; `redo` - re-applies; both prompt for confirmation showing the snapshot - timestamp and a summary of the changes that will be discarded. +- [ ] **U1** Auto-snapshot before **every** data/schema mutation + (DSL + SQL) into a persisted ring buffer (size N=50, tunable), + per ADR-0006 Amendment 1 (single-step undo, superseding the + original destructive-only model). Snapshot is a hybrid + whole-project copy (database via online backup API + `project.yaml` + / `data/*.csv` as files); staged before the mutation's transaction, + finalised after the db commit (preserves ADR-0015 §6). A batch + command (`replay` / future batch ops) records one boundary + snapshot; `import` takes none. A `--no-undo` CLI flag disables + snapshotting. *(Designed + amended + planned in + `docs/plans/20260524-adr-0006-undo-snapshots.md`; not yet + implemented.)* +- [ ] **U2** `undo` restores the most recent snapshot (database + + text, directly); `redo` re-applies (redo stack discarded on new + work); both prompt for confirmation naming the command being + undone / re-applied (`Y` confirms). *(Designed + amended; not yet + implemented — see U1's plan.)* - [x] **U3** `history.log` records every submitted command in append-only form, tagged with its outcome (Iteration 2; broadened by ADR-0034). Format: `||`