diff --git a/docs/adr/0015-project-storage-runtime.md b/docs/adr/0015-project-storage-runtime.md index 7730fc5..ddadd54 100644 --- a/docs/adr/0015-project-storage-runtime.md +++ b/docs/adr/0015-project-storage-runtime.md @@ -213,6 +213,14 @@ working copy. ### 6. Persistence ordering +> **Amended by ADR-0052 (2026-06-13, issue #30):** `history.log` is no +> longer written inside the worker transaction. It is a *journal* of typed +> commands, not state, so success journaling moved to the dispatch layer +> (next to the already-top-level failure journaling); `commit-db-last` now +> governs the three **state** targets only (db + `project.yaml` + +> `data/*.csv`), which still commit atomically in the worker. The journal +> write is best-effort (amends ADR-0040). + A successful user command produces effects in four targets: the SQLite database, `project.yaml`, the relevant `data/.csv` file(s), and `history.log`. INV-2 from the diff --git a/docs/adr/0034-history-journal-and-replay-filter.md b/docs/adr/0034-history-journal-and-replay-filter.md index 744c97a..ff2ef63 100644 --- a/docs/adr/0034-history-journal-and-replay-filter.md +++ b/docs/adr/0034-history-journal-and-replay-filter.md @@ -2,7 +2,13 @@ ## Status -Accepted +Accepted. **Amended by ADR-0052 (2026-06-13, issue #30):** the status +field gains an optional `:adv` mode suffix (`ok:adv` / `err:adv`) — the +"non-breaking future extension" this ADR reserved — and **success +journaling moves out of the worker to the dispatch layer** +(`spawn_dsl_dispatch` / `run_replay` / app-command sites), next to the +failure path, where the submission mode is in scope. `status_is_ok` keys +off the base token, so `ok:adv` replays like `ok`. ## Context diff --git a/docs/adr/0040-completion-marker-replaces-ok-summary.md b/docs/adr/0040-completion-marker-replaces-ok-summary.md index bc7cd3a..b90b4e6 100644 --- a/docs/adr/0040-completion-marker-replaces-ok-summary.md +++ b/docs/adr/0040-completion-marker-replaces-ok-summary.md @@ -5,7 +5,11 @@ **Accepted** — 2026-05-30 (issue #9). Amends the output conventions of ADR-0014 (data operations), ADR-0028 (query plans / `explain`), and ADR-0019 (failure rendering); builds on ADR-0037's mode-tagged echo -line. +line. **Amended by ADR-0052 (2026-06-13, issue #30):** a `history.log` +*journal*-write failure on a **successful** command is no longer fatal — +journaling moved to the dispatch layer (after the db commit), so it is +best-effort (logged + ignored), consistent with the failure-journal path. +State-write failures (yaml/csv/db) remain fatal. ## Context diff --git a/docs/adr/0052-mode-tagged-history-cross-mode-recall.md b/docs/adr/0052-mode-tagged-history-cross-mode-recall.md new file mode 100644 index 0000000..7fdaabe --- /dev/null +++ b/docs/adr/0052-mode-tagged-history-cross-mode-recall.md @@ -0,0 +1,241 @@ +# ADR-0052: Mode-tagged history for cross-mode recall + +## Status + +**Accepted + implemented 2026-06-13 (issue #30).** Closes Gitea **#30** — +both the feature ("reuse advanced history commands in simple mode by +prepending `:`") and the bug reported in its comment (the `:` one-shot +prefix lost across sessions). All forks user-chosen before any code. +**Amends ADR-0034** (journal status field gains a `:adv` tag; *journaling +moves from the worker to the dispatch layer*), **ADR-0015 §5/§6** +(history.log leaves the worker transaction — `commit-db-last` now scopes +yaml/csv/db only), and **ADR-0040** (a success-path journal-write failure +is best-effort, no longer fatal); references ADR-0003 (the `:` one-shot +sigil). Plan: `docs/plans/20260613-issue-30-top-of-chain-journaling.md` +(pre-build `/runda`, then a second `/runda` that drove the journaling +relocation + the app-command exclusion). **2471 tests pass / 0 fail / 0 +skip (1 ignored), clippy clean.** + +> **Why journaling moved (the key architectural turn).** The first draft +> kept journaling in the worker and threaded the mode down to it (~30-site +> plumbing). On review the user asked the right question: why is the +> journal written deep in the worker at all, when the failure path already +> journals at the top of the chain where command + mode + outcome are all +> in scope? It shouldn't — `history.log` is a *journal of typed commands*, +> not *state*. So success journaling moved up next to the failure path +> (`spawn_dsl_dispatch` / `run_replay` / the app-command sites), the +> mode-plumbing dilemma dissolved, and the worker's `finalize_persistence` +> now writes only the state sources (yaml/csv). Consequence: the journal +> write is best-effort (the command is already committed), consistent with +> the failure path — see §5. + +## Context + +The input-history ring and `history.log` carry **no mode information**, +which causes two coupled problems: + +1. **Feature gap.** A command typed in advanced mode (`select * from T`) + is stored bare. Recalled in simple mode it is not valid DSL → it just + errors. There is no way to know it was an advanced (SQL) command and + offer it back in a runnable form. + +2. **Bug (issue #30 comment).** A `:`-one-shot advanced command in simple + mode recalls correctly **in-session** (the in-memory ring stores the + raw `:select 1`), but after quit+resume it comes back **without** the + `:` and is unusable. Root cause: the ring stores the raw input + (`:select 1`), but the worker journals the **stripped** `effective_input` + (`select 1`) — submission strips the `:` before dispatch (ADR-0003) — + so the on-disk `source` never carried the `:`, and hydration loses it. + +Both reduce to: **history does not record the submission mode**, and the +in-memory and on-disk representations disagree about the `:`. + +## Decision + +Record the **submission mode** per history entry, keep the on-disk +`source` **canonical** (stripped — replay is unaffected), and have +**recall reconstruct the runnable line** for the current mode. + +### 1. In-memory ring stores the `:`-prefixed runnable form + +`App.history` stays `Vec` — no type change, so the public ring, +the `ProjectSwitched` payload, and `seed_history` are untouched. An +**advanced** entry is stored in its **simple-mode runnable form**, the +`: `-prefixed string (e.g. `: select * from T`); a **simple** entry is +stored bare. This is exactly what the in-session one-shot ring already +does (`:select 1` recalls as typed) — generalised to *persistent*-advanced +commands too, and made reconstructable on hydration. Because a simple +DSL command can never begin with `:` (the sole sigil, ADR-0003), a +leading `:` unambiguously marks an advanced entry. + +`submit` builds the stored line from the submission: advanced → +`": " + effective_input` (the `: ` matches the auto-space the typed +one-shot inserts), simple → `effective_input`. This is computed **after** +`effective_input` (today `push_history` runs on the raw `trimmed` before +stripping; the reorder also drops a bare `:`, which never executed). The +draft (`history_draft`) stays a plain `String`. `push_history` itself is +unchanged — it still takes one `&str`. + +### 2. Recall strips the `:` for advanced mode + +`history_back` / `history_forward` set `self.input` from the stored +string, then strip a leading `:` **iff the current persistent mode is +Advanced**: + +``` +if self.mode == Mode::Advanced && stored.starts_with(':') { stored[1..].trim_start() } else { stored } +``` + +So an advanced entry recalls as `: select * from T` in **simple** mode +(runs via the one-shot escape — the feature, and the cross-session bug +fix) and bare `select * from T` in **advanced** mode (runs as SQL). A +simple entry recalls bare in either mode (simple DSL already runs in +advanced mode — issue #30). In-session and cross-session paths share the +same stored form, so they finally agree. + +### 3. On-disk: a mode tag in the status field + +The record stays three pipe-separated fields `||` +(so `source` remains the last, pipe-tolerant, canonical field — replay +reads it unchanged). The **status token** gains an optional `:adv` +suffix: + +| Submission | Success | Failure | +|---|---|---| +| Simple | `ok` | `err` | +| Advanced (persistent or one-shot) | `ok:adv` | `err:adv` | + +ADR-0034 §1 already reserved the status field for "additional values … +a non-breaking future extension"; this is that extension. The status +parser splits the token on `:`: the base (`ok`/`err`) gives replayability +(`status_is_ok` ⇔ base == `ok`), the `adv` suffix gives the mode — so an +unknown future token degrades to "not ok, simple" rather than mis-parsing. + +### Journaling location: the dispatch layer, not the worker + +Both tags are written **at the dispatch layer**, where command + mode + +outcome are all in scope — so the mode needs no plumbing into the worker: + +- **Success:** `spawn_dsl_dispatch`, immediately after + `execute_command_typed` returns `Ok`, calls + `append_history(source, submission_mode.is_advanced())` (best-effort). + `run_replay` does the same per replayed line (tagged simple — replay is + mode-agnostic), and the app-command sites (`perform_switch` / + `spawn_export` / `spawn_rebuild`) journal **simple** (`advanced = false` + — app commands run in any mode, so no `:` on recall; this also avoids a + redundant `: undo`). +- **Failure:** unchanged location (the App→`JournalFailure`→runtime path, + already at the top), now carrying the mode — `JournalFailure` gains + `advanced`, and `DslFailed` gains `submission_mode` for the + worker-rejection sub-path (the parse-failure sub-path has it in + `dispatch_dsl`). `Ok`/`Err` are exclusive, so success-in-spawn and + failure-in-App-path never double-journal. + +The worker's `finalize_persistence` and the four no-op-skip / three +read-only sites **no longer journal** — they leave the state writes +(yaml/csv) in the worker transaction and let the dispatch layer journal +the `Ok` outcome. + +### 4. Hydration reconstructs the `:`-prefixed form + +`read_recent_sources` parses each record's status tag and, for an +advanced record, **reconstructs** the `: `-prefixed string from the +canonical `source` (`format!(": {source}")`); simple records pass through +bare. It still returns `Vec`, so `read_history_seed`, +`seed_history`, and the `ProjectSwitched` payload are **unchanged**. A +hydrated entry is therefore byte-identical to its in-session form, and +recall behaves identically. + +### Back-compatibility + +Old `history.log` files have only `ok` / `err` tokens → parsed as +`advanced = false` (simple). Their advanced commands stay un-`:`-able on +recall — the pre-existing behaviour, not a regression; nothing migrates. +`status_is_ok` keys off the base token, so `ok:adv` records replay +exactly as `ok` does today (source is canonical either way). + +### Journal write is best-effort (amends ADR-0040) + +Because the journal is now written *after* the worker replies (i.e. after +`tx.commit`), a journal-write failure can no longer roll the command back. +It is **best-effort** — logged and ignored, exactly like the failure path +already is (ADR-0034 §4) — so the two journal paths are finally +consistent. State integrity is unchanged: yaml/csv/db still commit +atomically in the worker (a *state*-write failure still rolls back and is +fatal). The only property given up: on a rare journal-write failure (disk +full) a committed command may be missing from `history.log` — not +recallable/replayable next session, but the state is correct. User-chosen +over keeping journaling coupled in the worker (which would have needed the +~30-site mode plumbing). See the plan's §2 for the full trade-off. + +## Forks (user-chosen) + +- **Format = mode tag in the status field** (`ok:adv`/`err:adv`), over a + new 4th field (ambiguous with unescaped pipes in old `source`s without + a version bump) or a `:`-prefix in `source` (would make `source` + non-canonical and force replay to strip it). +- **Scope = unified** (bug + feature) over bug-only: one mechanism does + both, and keeping `source` canonical for replay needs the mode tag + regardless, so bug-only is barely smaller and leaves the main ask open. +- **Journaling location = dispatch layer, best-effort** over keeping it + worker-coupled-and-fatal (which needed the ~30-site mode plumbing). The + user's architectural call (§Status). + +## Consequences + +- Advanced history is reusable in simple mode; the `:` one-shot survives + resume. The in-memory and on-disk representations agree. +- **Journaling left the worker.** `finalize_persistence` and the + no-op-skip / read-only sites no longer journal; success is journalled at + the dispatch layer (`spawn_dsl_dispatch` / `run_replay` / app-command + sites). The ring stays `Vec`; `seed_history` / `ProjectSwitched` + are untouched. The vestigial worker `source` plumbing (the `_source` + param on `finalize_persistence` / `do_rebuild_from_text` and the thin + read-only `*_request` wrappers) is left in place — a clean follow-up. +- **App commands recall bare.** Because they are dispatched outside the + `ExecuteDsl`/spawn path, app commands journal **simple** (`advanced = + false`) at their own sites, and `submit` excludes them from the ring's + `advanced` flag (`!is_app_command`) — so `mode advanced` / `undo` recall + bare and run fine in simple mode, with no redundant `:`. +- **Journaling is now uniform (user-confirmed).** The spawn journals on + `outcome.is_ok()`, so **every** successful command is recorded — closing + a pre-existing gap where `show table` / `show data` / `select` journalled + but `show tables`/`show relationships`/`show indexes`, `show relationship + `, and `explain` did **not** (their worker arms carried no + `source` / no journal call). The new behaviour matches ADR-0034 §1 + ("record every submitted command"); those reads are now recallable and + are re-run harmlessly on replay (`explain` never executes; shows produce + output, no state change). A DA finding, accepted as the more-correct + behaviour over re-adding command-outcome gating to preserve the old + inconsistency. +- **Replay re-journaling.** When `replay` re-dispatches a line, the + re-written record is tagged from how replay dispatched (mode-agnostic → + `ok`), so a replayed advanced command may be re-journalled without + `:adv`. Replay correctness of execution is unchanged (it already parses + mode-agnostically); this only affects the *tag* of the re-written line. + Noted; not addressed here (replay's own mode-fidelity is out of scope). + +## Tests + +- **Tier-1 (`app.rs`):** an advanced one-shot / persistent-advanced + submission is stored `: `-prefixed; it recalls as `: …` in simple mode + and bare in advanced mode; a simple entry recalls bare in both; a bare + `:` is not stored; a parse-failure is still recallable; dedup/cap hold. +- **Tier-1 (`history.rs`):** the writer emits `ok:adv`/`err:adv`; + `read_recent_sources` reconstructs the `: `-prefix for `:adv` records + and leaves `ok`/`err` records bare (so old logs read as simple); + `status_is_ok` is true for `ok` and `ok:adv`. +- **Tier-3 (`iteration6_resume_history` / it):** the headline + **regression** — type a `:`-one-shot advanced command, journal + + hydrate, and assert it recalls **with** `:` in simple mode (fails on + current code); plus a persistent-advanced command round-tripping to a + `: …` recall. + +## Out of scope + +- Replay re-journaling mode-fidelity (above). +- Special-casing app commands to avoid the redundant recall `: `. +- Distinguishing one-shot from persistent advanced on recall (both are + simply "advanced" — the `:` is what simple mode needs either way). +- A format version marker / pipe-escaping in `source` (unneeded — the + status-tag approach keeps `source` last and canonical). diff --git a/docs/adr/README.md b/docs/adr/README.md index 2d81708..abeba31 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -57,3 +57,4 @@ This directory contains the project's ADRs, recorded per - [ADR-0049 — Input-field readline keymap: Esc-clear + Ctrl-A/E/W/K/U (I1b)](0049-input-field-readline-keymap.md) — **Accepted + implemented 2026-06-12 (issue #29)**, closes Gitea **#29** and the deferred **I1b** readline requirement. **Amends ADR-0046**, which listed "readline shortcuts (I1b)" as out-of-scope — that item is now in scope and decided here; orthogonal to ADR-0003's input-*mode* model and extends the I1a single-line cursor editing already shipped. Binds, in the input field (non-modal, non-nav, both modes): **`Esc`** clears a partly-typed command (empty buffer, cursor→0, scroll→0); **`Ctrl-A`/`Ctrl-E`** alias Home/End (line start/end); **`Ctrl-W`** deletes the previous word (readline-style — eats trailing whitespace then the preceding non-whitespace run, UTF-8-safe on char boundaries, only back to the cursor); **`Ctrl-K`** kills to end of line; **`Ctrl-U`** kills to start. **Esc precedence:** a live Tab-completion memo still wins (Esc undoes the completion first, ADR-0022; Esc clears only when no memo) — Esc-once backs out the completion, Esc-again clears. Forks all user-chosen: **single-Esc-clears** (not double-Esc — discoverable over accident-proof; an unsubmitted draft can be lost, a submitted line is always in history); the **full I1b set** (not just the issue's literal Ctrl-A/E + Esc); a **new ADR** (not an ADR-0046 amendment / no-ADR). Cursor-only keys (Ctrl-A/E) leave history navigation intact like Home/End; buffer-mutating keys (Esc-clear, Ctrl-W/K/U) end it like Backspace. Helpers `clear_input`/`delete_prev_word`/`kill_to_end`/`kill_to_start` in `src/app.rs`; **22 new Tier-1 tests, 2458 pass / 0 fail / 0 skip (1 ignored), clippy clean**. OOS: on-screen keybinding hints (issue #27 owns surfacing per-focus keybindings in the bottom status line — this ADR makes the keys *work*, #27 makes them *discoverable*); demo-mode badges for the new chords (ADR-0047 follow-up — Esc already badges `[ESC]`, the glyph-less Ctrl-chords are flagged but not added); multi-line input (I1); word-wise cursor motion (Alt-B/F) / transpose / yank - [ADR-0050 — Incidental-DDL confirmations omit relationship info (structure-only)](0050-incidental-ddl-confirmations-omit-relationships.md) — **Accepted + implemented 2026-06-12 (issue #28)**, closes Gitea **#28**. **Supersedes** the incidental-DDL clause of **ADR-0044 §1** and the relationship-block half of **ADR-0016 §5**. Incidental-DDL confirmation echoes (`create table`, `add`/`drop`/`rename`/`change column`, `add`/`drop index`) now render **structure only** — header + column box + `Indexes:` + constraints — with **no `References:` / `Referenced by:` block** (neither prose nor diagram), even when the table carries relationships the user did not touch. Rationale (owner): a confirmation echo reports the change just made, not untouched relationships; ADR-0044's terse prose was the lesser of "prose vs diagram", but the right answer for these surfaces is **neither**. **Relationship-subject surfaces are unchanged** — `show table`, `add`/`drop relationship`, `show relationship` still render ADR-0044 diagrams; relationships appear only when the user asks for (`show table`) or acts on (`add`/`drop relationship`) one, and are one `show table ` away — **no information lost**. Forks both user-chosen: **scope = all incidental DDL** (not just `add column` — the rationale is uniform, the mental model clean, and it's the simpler edit) and **delete the prose renderer** (not retain it dormant — no dead code). **Mechanism:** the `handle_dsl_success` `matches!` routing is unchanged (relationship-subject → diagrams; else → `render_structure`); the change is one line inside `render_structure` (`output_render.rs` — drop the relationship-block call) since all its callers are incidental DDL, plus deletion of the orphaned `relationship_prose_lines` + `cols_disp` helpers. The prose format survives in ADR-0016 §5 + git history for a future OOS-7 always-prose setting. **Tests:** the prose-presence unit test + its snapshot removed; a new unit test asserts `render_structure` on a description carrying **both** inbound and outbound relationships emits the box but no prose; the misnamed `add_relationship_flow_shows_inbound_section_on_parent` integration test (which sent an `AddColumn`) inverted + renamed to assert the add-column echo omits the prose; the diagram tests (`show table`, `add relationship`) unaffected. **2458 pass / 0 fail / 0 skip (1 ignored), clippy clean**. `requirements.md` unaffected (ADR-tracked refinement of a decided area, like ADR-0044 itself) - [ADR-0051 — Bottom keybinding strip: context- and state-aware](0051-context-state-aware-keybinding-strip.md) — **Accepted + implemented 2026-06-13 (issue #27)**, closes Gitea **#27**. Repurposes the bottom status line into a **keystrokes-only, state-selected** strip (builds on ADR-0046 nav focus, ADR-0003 modes, ADR-0049 the #29 readline keys it now advertises, ADR-0022 the completion memo). A pure `status_bar_bindings(app) -> Vec<(key,label)>` chooses the strip by **priority, first match wins**: (1) **sidebar focus** → `Ctrl-O next pane · ↑↓/PgUp/PgDn scroll · Esc input`; (2) **completion memo live** (`last_completion`) → `Tab/Shift-Tab cycle · Esc cancel · Enter run`; (3) **history navigation** (new `App::is_browsing_history()` exposing the private `history_cursor`) → `↑↓ browse · Esc clear · Enter run`; (4) **editing** (input non-empty) → `Esc clear · Ctrl-A/E home/end · Ctrl-W del word · Enter run` (surfaces the #29 keys, closing ADR-0049's deferred advertisement); (5) **default** (empty) → `Ctrl-O sidebar · Tab complete · ↑ history · Enter run`. Priority is correct because Up clears the completion memo and Tab cancels history nav, so states 2/3 never co-occur, and the five are exhaustive for Input focus. **Typed-command words leave the strip** (`mode advanced`/`mode simple` switch, `:` one-shot) and **mode discovery moves to the empty-input hint** (`resolve_hint_lines`), **simple mode only**: `\`mode advanced\` for SQL` (the verb "type" omitted — the prompt implies it; advanced mode shows **no** pointer per a post-trial user decision — a switcher knows how they got there and `help` covers the way back). The one-shot's old `Backspace cancel one-shot` label is subsumed by the editing state (behaviour intact). Forks all user-chosen: **editing state shows the #29 keys** (vs unadvertised); **`Ctrl-C quit` omitted** from the strip (vs always shown); **no width-drop machinery** — the longest strip (~65 cols) fits all supported widths, so a **width-budget unit test** keeps it lean by construction instead (the user's own observation). Catalog: 12 new `shortcut.*` labels + the `panel.hint_mode_advanced` string added to `en-US.yaml`+`keys.rs` (validator-checked 1:1), 5 now-dead strip strings removed. **Modal-aware strip is OOS** (pre-existing: a modal owns the keyboard and carries its own hints; the strip under it is unchanged-in-kind, not worsened). Tests: 9 Tier-1 unit (per-state key sets — completion/history driven through real key events; width budget; mode-pointer presence/absence), 1 Tier-3 rewritten (`status_bar_is_keystroke_only_and_state_aware`), 15 full-panel snapshots re-accepted (reviewed — strip/hint only). **2467 pass / 0 fail / 0 skip (1 ignored), clippy clean.** OOS: modal-aware strip; a full-key cheatsheet overlay; Ctrl-K/U advertisement (editing strip shows the highest-value subset within the width budget) +- [ADR-0052 — Mode-tagged history for cross-mode recall](0052-mode-tagged-history-cross-mode-recall.md) — **Accepted + implemented 2026-06-13 (issue #30)**, closes Gitea **#30** — the feature (advanced history reusable in simple mode) **and** the bug in its comment (the `:` one-shot prefix lost across sessions). **Amends ADR-0034** (status field gains a `:adv` tag; **journaling moves from the worker to the dispatch layer**), **ADR-0015 §5/§6** (history.log leaves the worker transaction — `commit-db-last` now scopes yaml/csv/db only), and **ADR-0040** (a success-path journal-write failure is best-effort, not fatal); references ADR-0003. **Root cause:** history carried no mode, and the in-memory ring stored the raw `:select 1` while the worker journalled the *stripped* `select 1`, so the `:` was lost on disk. **Fix:** record the submission mode per entry as a **`:adv` suffix on the status token** (`ok`/`ok:adv`/`err`/`err:adv`) — `source` stays last + canonical so replay is unaffected; the in-memory ring (still `Vec`) stores advanced entries in their `: `-prefixed simple-mode runnable form (a leading `:` unambiguously marks advanced since simple DSL never starts with `:`); recall **strips the `:` in advanced mode** (runs as bare SQL) and keeps it in simple mode (runs via the one-shot escape); hydration reconstructs the `: `-prefix from the tag, so cross-session = in-session. **The architectural turn (user's call):** the first draft kept journaling in the worker + threaded the mode down (~30-site plumbing); on review the user asked why the journal is written deep in the worker when the *failure* path already journals at the top of the chain — it shouldn't (history.log is a journal, not state). So **success journaling moved up** to `spawn_dsl_dispatch` / `run_replay` / the app-command sites (next to the failure path), the worker's `finalize_persistence` now writes only yaml/csv, and the journal write became **best-effort** (the command is already committed — consistent with the failure path; a rare disk-full leaves a committed command unjournalled, state intact). **App commands** journal simple (dispatched outside the spawn) and `submit` excludes them from the ring's advanced flag, so `undo`/`mode advanced` recall bare. Forks user-chosen: status-tag format (vs 4th field / `:`-in-source); unified scope; **dispatch-layer best-effort journaling** (vs worker-coupled-fatal). Two `/runda` passes (the second drove the relocation + app-command exclusion). Tests: the 15 worker-level journaling tests retired (worker no longer journals — yaml/csv/operation checks kept), re-covered at the new layer (history.rs status-tag + `:`-reconstruct; app.rs recall matrix; the #30 cross-session regression in `iteration6`; replay tests cover `run_replay` journaling). **2471 pass / 0 fail / 0 skip (1 ignored), clippy clean.** OOS: unwinding the now-vestigial worker `source` plumbing (`_source` params + thin `*_request` wrappers — a clean follow-up); replay re-journaling mode-fidelity (a replayed advanced line re-journals simple — not a regression) diff --git a/docs/plans/20260613-issue-30-top-of-chain-journaling.md b/docs/plans/20260613-issue-30-top-of-chain-journaling.md new file mode 100644 index 0000000..54eeeea --- /dev/null +++ b/docs/plans/20260613-issue-30-top-of-chain-journaling.md @@ -0,0 +1,247 @@ +# Plan — issue #30: mode-tagged history + top-of-chain journaling + +**Status:** draft for `/runda` review (2026-06-13). +**Issue:** #30 — advanced history reusable in simple mode (prepend `:`), +and the bug: the `:` one-shot prefix is lost across sessions. +**ADR:** ADR-0052 (new); amends ADR-0015 §6, ADR-0034, ADR-0040; +references ADR-0003. + +## 1. Goal & root cause + +Two coupled needs, one root cause — **history entries carry no mode**: +- **Bug:** the in-memory ring stores the raw `:select 1`, but the worker + journals the *stripped* `select 1`, so cross-session the `:` is lost + and the command recalls bare (unusable in simple mode). +- **Feature:** persistent-advanced commands (`select 1` typed in advanced + mode) can't be told apart from simple DSL, so they can't be offered + back with a `:` in simple mode. + +Fix: **record the submission mode per entry** (status tag `:adv`), keep +the on-disk `source` canonical, and have **recall prepend/strip `:`** for +the current mode. + +## 2. The architecture insight (why this plan is shaped this way) + +Journaling **success** lives deep in the worker: `finalize_persistence` +(db.rs:3096-3099) writes `history.log` *inside the db transaction, before +`tx.commit()`*, alongside yaml/csv — plus four no-op-skip sites and three +read-only helpers. **Failure** journaling already lives at the top +(runtime.rs:484-495, best-effort). Threading the mode *down* to the +worker would mean ~30 `Request` variants + `Database` methods + +`execute_command_typed` arms — because the journal write is far from +where the mode is known. + +So instead: **move success journaling up to the dispatch layer**, next to +where failure journaling already is and where mode + outcome + source are +all in scope. The mode then needs no plumbing. This is the correct +separation anyway — `history.log` is an append-only *journal of what was +typed*, not *state*; the state sources (yaml/csv/db) stay atomic in the +worker. + +### Semantic changes this entails (must be vetted) + +1. **history.log leaves the worker transaction** (amends ADR-0015 §6). + `commit-db-last` still governs yaml/csv/db (the state); the journal is + written *after* the worker replies (i.e. after `tx.commit`), at the + dispatch layer. +2. **Success-journal write failure: fatal → best-effort** (amends + ADR-0040). Today a failed `history.log` write on a *successful* + command rolls the command back and shows a fatal banner. After: the + command stays committed; the journal write is best-effort (logged + + ignored), exactly like the failure path already is. The two journal + paths become *consistent*. +3. **Consequence:** on a rare journal-write failure (disk full / + permissions) a successful command is applied but may be missing from + `history.log` — not recallable next session, not replayable. The state + (yaml/csv/db) is unaffected and consistent. This is a graceful + degradation, not corruption, and is logged. (Today the same disk-full + instead kills the app mid-command.) + +**Open question for review/user:** is trading "fatal on journal-write +failure" for "best-effort, command still succeeds" acceptable? The plan +assumes **yes** (a journal is auxiliary; killing the app over it is worse +UX). If not, journaling must stay coupled in the worker and we pay the +~30-site mode plumbing instead. + +## 3. On-disk format (mode tag in status — already chosen + partly built) + +Record stays `||`; the **status token** gains an +optional `:adv` suffix (ADR-0052). `source` stays canonical so replay is +unaffected. + +| Submission | Success | Failure | +|---|---|---| +| Simple / app command | `ok` | `err` | +| Advanced (SQL, persistent or one-shot) | `ok:adv` | `err:adv` | + +**Done already** (history.rs / mod.rs): +- `status_token(base, advanced)`, `parse_status(status) -> (is_ok, advanced)`. +- `parse_record_source` reconstructs `": {cmd}"` for `:adv` records. +- `parse_journal_record.status_is_ok` via `parse_status` (so `ok:adv` replays). +- `append_history(text, advanced)`, `append_history_failure(text, advanced)`. + +Back-compat: old `ok`/`err` logs → simple; nothing migrates. + +## 4. In-memory ring & recall (app.rs) — the #30 behaviour + +The ring stays `Vec`. An **advanced** entry is stored in its +`: `-prefixed simple-mode runnable form (matching the existing in-session +one-shot ring); a **simple** entry bare. A leading `:` unambiguously +marks advanced (simple DSL can never start with `:`). + +- **`submit`** (app.rs:1704): compute `effective_input` + `submission_mode`, + parse once for the app-command check (already done at 1751), then build + the ring line. The **`advanced` flag excludes app commands** — + `advanced = submission_mode.is_advanced() && !is_app_command` — because + app commands (`undo`, `mode …`, `save as`, …) run in *any* mode and must + **not** get a `:` on recall. Ring line: `": " + effective_input` if + `advanced`, else `effective_input`; `push_history(&ring_line)`. (Today it + pushes the raw `trimmed` *before* stripping; the reorder also drops a + bare `:`, which executed nothing, and is what lets the app-command check + precede the push.) `ExecuteDsl.source` stays the **canonical** + `effective_input`. + - *Why the app-command exclusion matters (DA finding):* without it, + `: save as foo` (an app command via the one-shot) would store `: save + as foo` in the ring but journal `save as foo` (app commands journal + simple at their own sites, §5) — the very in-session-vs-cross-session + divergence #30 is fixing, re-introduced for app commands. Excluding + them keeps ring and disk agreeing (both bare). +- **`history_back` / `history_forward`**: after cloning the stored entry + into `self.input`, strip a leading `:` **iff `self.mode == Advanced`** + (so an advanced entry runs as bare SQL in advanced mode, and as `: …` + one-shot in simple mode). A small helper `recall_display(stored)`. +- `seed_history` / `ProjectSwitched` payload: **unchanged** (`Vec`); + hydration already returns the `: `-prefixed form (§3). + +Recall matrix: +| entry \ current mode | Simple | Advanced | +|---|---|---| +| advanced (`: select 1`) | `: select 1` (one-shot) | `select 1` (SQL) | +| simple (`create …`) | `create …` | `create …` | + +## 5. Move success journaling worker → dispatch layer + +**Remove** (worker stops journaling success): +- `finalize_persistence` history write (db.rs:3096-3099). Keep yaml/csv. + The now-unused `source` param: remove it + drop the arg at its ~30 + callers (mechanical, compiler-guided). (Handlers keep their own + `source` for `snapshot_then`.) +- The 4 no-op-skip `append_history` (db.rs:2267, 2311, 2524, 2560) — these + outcomes (`SchemaSkipped` etc.) are `Ok` at the dispatch layer, so the + new top-level journal covers them. +- The 3 read-only helper `append_history` (db.rs:8372 show table, 9996 + show data, 10014 select) — `Ok(Query)`/`Ok(ShowList)` at the top. + +**Add** (dispatch-layer journaling, all best-effort + logged): +- **`spawn_dsl_dispatch`** (runtime.rs ~1433): pass `project_path` in; + after `execute_command_typed`, `if outcome.is_ok() { + Persistence::new(path).append_history(&source_for_journal, + submission_mode.is_advanced()) }`. (Failures stay in the existing path, + §6 — no double-journal, since Ok and Err are exclusive.) +- **`run_replay`** (runtime.rs ~2540): after each line's + `execute_command_typed`, `if outcome.is_ok() { append_history( + &command_text, false) }` — replay is mode-agnostic, journalled + **simple**. (Preserves ADR-0034 §3 "replayed sub-commands land in + history"; a replayed advanced command re-journals without `:adv` — a + documented OOS, not a regression: today it re-journals as plain `ok`.) +- **`spawn_rebuild`** (runtime.rs ~503): after a successful rebuild, + `append_history("rebuild"/source, false)`. (Rebuild journalled via + `finalize_persistence` today; that write is gone, so add it here.) + +**Unchanged** (already at the dispatch layer, app commands): +- `perform_switch` (974: save-as/load/new) and `spawn_export` (1043) — + already best-effort `append_history(&source)`; add the new `advanced` + arg as `false` (app commands run in any mode → no `:` needed on recall; + this also fixes the would-be "redundant `: undo`" — app commands + journal **simple** because they're dispatched here, never via + `ExecuteDsl`/the spawn). +- `undo`/`redo`/`copy`/`help`/`quit`: not journalled today; unchanged. +- The **`replay` command itself**: dispatched as `Action::Replay`, never + reaches the spawn → not journalled (preserves the ADR-0034 §3 exclusion + without extra work); nested `replay` skip in `run_replay` unchanged. + +### DA-confirmed design choice: split, don't unify + +Success journals in the spawn (`Ok` arm); **all** failures stay in the +existing App→`JournalFailure`→runtime path (just gaining the mode). +Considered and rejected: moving worker-rejection failures into the spawn +too (to "unify"). It doesn't actually unify — parse failures never reach +the spawn, so they'd stay in the App path regardless — and it adds a +double-journal hazard (must also strip the App's `DslFailed`→ +`JournalFailure` emission). The split keeps the failure path **untouched +in structure** (lowest risk); `Ok`/`Err` are exclusive so there is no +double-journal. **Verified safe:** undo/redo never touches `history.log` +(the snapshot copies db+yaml+csv only, undo.rs:15-16), and `snapshot_then`'s +redo-clear keys on `source.is_some()`, independent of journaling — so +removing the worker journal write does not perturb undo/snapshot at all. + +## 6. Failure journaling — add the mode (location unchanged) + +Keep both failure origins where they are (best-effort, dispatch/App +layer); thread the mode so they tag `err:adv`: +- **`Action::JournalFailure`** (action.rs:42): add `advanced: bool` (or + `submission_mode`). +- **`AppEvent::DslFailed`** (event.rs): add `submission_mode` (the + worker-rejection path — the App can't recover the mode from an async + reply otherwise). +- **App**: the parse-failure path (`dispatch_dsl` Err arm) has + `submission_mode` directly; the `DslFailed` handler reads it off the + event. Both emit `JournalFailure { source, advanced }`. +- **runtime.rs:492**: `append_history_failure(&source, advanced)`. + +## 7. Tests + +- **history.rs (Tier-1):** `status_token`/`parse_status` round-trip; + `read_recent_sources` reconstructs `": …"` for `:adv` and leaves + `ok`/`err` bare; `status_is_ok` true for `ok` & `ok:adv`; old-log + back-compat. +- **app.rs (Tier-1):** advanced submission stored `: `-prefixed; recall + prepends in simple / strips in advanced; simple bare in both; bare `:` + not stored; a parse-failure is still recallable; dedup/cap hold. +- **iteration6_resume_history (Tier-3) — headline regression:** journal + an advanced command (`append_history(text, true)`), hydrate, recall in + simple → `: …`; and the full bug repro through `submit` + journal + + hydrate if feasible. +- **replay_command (Tier-3):** replayed commands still land in + history.log (now via `run_replay`'s call); the `replay`-self-exclusion + + nested-skip still hold; advanced lines replay (status `ok:adv` + treated as ok). +- **Journaling relocation:** a success no longer fatals on a journal + write failure (best-effort) — if cheaply testable; at minimum a worker + test that previously asserted worker-side journaling is updated/removed. +- **Update mechanical call sites:** `append_history(_, advanced)` / + `append_history_failure(_, advanced)` at the db.rs inline tests + (8372/9996/10014/11324 — likely now removed with the production sites), + iteration6 (144-170), mod.rs (600). + +## 8. ADR work + +- **ADR-0052 (new):** the #30 feature + bug, the status-tag format, the + `: `-prefixed ring + recall, AND the journaling relocation (it's the + enabling refactor). Forks: status-tag format; unified scope; + dispatch-layer journaling (best-effort). +- **ADR-0015 §6 amendment:** history.log out of the worker transaction; + commit-db-last now scopes yaml/csv/db; journal is a dispatch-layer + best-effort side-record. +- **ADR-0034 amendment:** journaling location (dispatch layer); + status-field `:adv` extension (it already reserved the field). +- **ADR-0040 amendment:** a success-path journal-write failure is no + longer fatal — best-effort, consistent with the failure path. +- README index upkeep for every ADR touched. + +## 9. Risks / watch-list + +- **Double-journaling**: ensure Ok→spawn and Err→App-path stay exclusive; + do NOT also leave a worker journal. +- **Under/over-journaling vs today**: top-level "journal on every Ok" + must match today's "journal every command with a source" — verified: + reads + skips are Ok outcomes, internal ops never reach the spawn. +- **finalize_persistence source-param removal**: 30 mechanical call-site + edits; compiler-guided. +- **Replay re-journal mode fidelity**: replayed advanced commands + re-journal as simple (OOS, not a regression). +- **best-effort journal**: rare write-failure leaves a command unjournaled + (logged). User decision (§2 open question). +- **app-command mode**: journalled simple by construction (dispatched + outside the spawn) — this is correct (they run in any mode), and + resolves the earlier "redundant `: undo`" worry. diff --git a/src/action.rs b/src/action.rs index 2ca793b..b115ddb 100644 --- a/src/action.rs +++ b/src/action.rs @@ -41,6 +41,11 @@ pub enum Action { /// §4). `source` is the original user-typed text. JournalFailure { source: String, + /// Whether the failed submission was advanced (ADR-0052): tags the + /// `err` record `err:adv` so a failed advanced command hydrates in + /// its `:`-prefixed form, recallable in simple mode. App commands + /// (mode-agnostic) are `false`. + advanced: bool, }, /// User issued the `rebuild` app-level command (ADR-0015 /// §7, §11). Runtime computes a summary from diff --git a/src/app.rs b/src/app.rs index 8efcb9f..b6853c6 100644 --- a/src/app.rs +++ b/src/app.rs @@ -874,13 +874,16 @@ impl App { error, facts, source, + advanced, } => { self.handle_dsl_failure(&command, error, facts); // ADR-0034 §1/§2: an execution failure is journalled // `err` so it is recallable across sessions (the // worker only journals successful commands). The App - // emits the intent; the runtime does the append. - vec![Action::JournalFailure { source }] + // emits the intent; the runtime does the append. The + // mode rides along (ADR-0052) so an advanced failure + // tags `err:adv`. + vec![Action::JournalFailure { source, advanced }] } AppEvent::TablesRefreshed(tables) => { trace!(count = tables.len(), "tables refreshed"); @@ -1648,11 +1651,27 @@ impl App { Some(i) => i - 1, }; self.history_cursor = Some(next_index); - self.input = self.history[next_index].clone(); + let stored = self.history[next_index].clone(); + self.input = self.recall_display(&stored); self.input_cursor = self.input.len(); self.input_scroll_offset = 0; } + /// The display form of a stored history entry for the current mode + /// (ADR-0052, issue #30). An advanced entry is stored in its + /// `:`-prefixed simple-mode runnable form; in **advanced** mode the + /// `:` is stripped so it runs as bare SQL, while in **simple** mode it + /// stays prefixed and runs via the one-shot escape. A simple entry + /// (never starting with `:`) is returned unchanged in either mode. + fn recall_display(&self, stored: &str) -> String { + if self.mode == Mode::Advanced + && let Some(rest) = stored.strip_prefix(':') + { + return rest.trim_start().to_string(); + } + stored.to_string() + } + /// Move forwards in history (towards newer entries; eventually /// returning to the user's saved draft). fn history_forward(&mut self) { @@ -1661,7 +1680,8 @@ impl App { }; if i + 1 < self.history.len() { self.history_cursor = Some(i + 1); - self.input = self.history[i + 1].clone(); + let stored = self.history[i + 1].clone(); + self.input = self.recall_display(&stored); } else { // Past the most recent entry — restore the draft and // exit navigation mode. @@ -1709,10 +1729,6 @@ impl App { if trimmed.is_empty() { return Vec::new(); } - // Record the original (trimmed) line in history regardless - // of whether it parses, so users can recall and edit - // typo'd commands. - self.push_history(trimmed); // `:` one-shot escape: in simple mode, a leading `:` means // treat *this single submission* as advanced. The persistent @@ -1729,6 +1745,9 @@ impl App { }; if effective_input.is_empty() { + // A bare `:` (one-shot with nothing after it) executes + // nothing and is not recorded — the push moved below the + // strip (ADR-0052), so it no longer lands in history. return Vec::new(); } @@ -1739,16 +1758,31 @@ impl App { "submit" ); - // Parse-first: app-level commands and DSL commands now - // share the chumsky parser (per the round-5 refactor). - // App commands work in both modes — they're not gated by - // `effective_mode`. Anything that parses to a non-App - // variant falls through to the existing mode-specific - // path: simple → DSL execution; advanced → SQL placeholder. - // Anything that fails to parse falls through too — the - // simple-mode path renders the friendly parse error, the - // advanced-mode path renders the SQL placeholder. - if let Ok(Command::App(app_cmd)) = parse_command(&effective_input) { + // Parse-first: app-level commands and DSL commands share the + // parser. App commands work in both modes — they're not gated by + // `effective_mode`. Anything that parses to a non-App variant (or + // fails to parse) falls through to the mode-specific path. + let parsed = parse_command(&effective_input); + + // ADR-0052 (issue #30): record the command for cross-mode recall. + // An **advanced** (SQL) command is stored in its `:`-prefixed + // simple-mode runnable form, so it can be recalled and re-run in + // simple mode (recall strips the `:` again in advanced mode). A + // simple command — and **any app command**, which runs in either + // mode and so must not gain a `:` — is stored bare. Recorded + // regardless of whether it parses, so typo'd commands stay + // recallable. The canonical (un-prefixed) text is what reaches + // the journal via `ExecuteDsl.source`. + let is_app = matches!(&parsed, Ok(Command::App(_))); + let advanced = submission_mode.is_advanced() && !is_app; + let ring_line = if advanced { + format!(": {effective_input}") + } else { + effective_input.clone() + }; + self.push_history(&ring_line); + + if let Ok(Command::App(app_cmd)) = parsed { return self.dispatch_app_command(app_cmd, &effective_input); } @@ -1961,6 +1995,7 @@ impl App { self.note_error(note); return vec![Action::JournalFailure { source: input.to_string(), + advanced: submission_mode.is_advanced(), }]; } // Issue #17: simple-mode (DSL) counterpart. A wrong-count @@ -1988,6 +2023,7 @@ impl App { self.note_error(render_usage_block(input, mode)); return vec![Action::JournalFailure { source: input.to_string(), + advanced: submission_mode.is_advanced(), }]; } self.push_output(OutputLine::echo(input, mode)); @@ -2074,6 +2110,7 @@ impl App { // append; the App only emits the intent. vec![Action::JournalFailure { source: input.to_string(), + advanced: submission_mode.is_advanced(), }] } } @@ -5493,6 +5530,7 @@ mod tests { }, facts: crate::friendly::FailureContext::default(), source: String::new(), + advanced: false, }); let last = app.output.back().unwrap(); assert_eq!(last.kind, OutputKind::Error); @@ -5551,6 +5589,7 @@ mod tests { error: err, facts, source: String::new(), + advanced: false, }); let body = app .output @@ -5600,6 +5639,7 @@ mod tests { error: err, facts, source: String::new(), + advanced: false, }); let body = app .output @@ -5632,6 +5672,7 @@ mod tests { error: err(), facts: crate::friendly::FailureContext::default(), source: String::new(), + advanced: false, }); let verbose_text = app .output @@ -5652,6 +5693,7 @@ mod tests { error: err(), facts: crate::friendly::FailureContext::default(), source: String::new(), + advanced: false, }); let short_text = app .output @@ -6327,7 +6369,7 @@ mod tests { assert!( matches!( actions.as_slice(), - [Action::JournalFailure { source }] if source == "florp glorp" + [Action::JournalFailure { source, .. }] if source == "florp glorp" ), "expected JournalFailure for the typo'd line; got {actions:?}", ); @@ -6350,11 +6392,12 @@ mod tests { }, facts: crate::friendly::FailureContext::default(), source: "drop table Ghost".to_string(), + advanced: false, }); assert!( matches!( actions.as_slice(), - [Action::JournalFailure { source }] if source == "drop table Ghost" + [Action::JournalFailure { source, .. }] if source == "drop table Ghost" ), "expected JournalFailure carrying the source; got {actions:?}", ); @@ -6483,6 +6526,80 @@ mod tests { assert_eq!(app.input, "drop table AX"); } + // ---- ADR-0052 (issue #30): mode-aware history recall ---- + + #[test] + fn one_shot_advanced_command_recalls_with_colon_in_simple_mode() { + // The bug: a `:`-one-shot advanced command must recall WITH the + // `:` so it re-runs in simple mode (in-session and, via the + // `:`-prefixed ring form, across sessions too). + let mut app = App::new(); + type_str(&mut app, ": select 1"); + submit(&mut app); + app.update(key(KeyCode::Up)); + assert_eq!(app.input, ": select 1"); + } + + #[test] + fn persistent_advanced_command_recalls_with_colon_back_in_simple_mode() { + // The feature: a command typed in *persistent* advanced mode + // recalls into simple mode with a `:` so it stays runnable. + let mut app = App::new(); + app.mode = Mode::Advanced; + type_str(&mut app, "select 1"); + submit(&mut app); + // Switch back to simple and recall. + app.mode = Mode::Simple; + app.update(key(KeyCode::Up)); + assert_eq!(app.input, ": select 1"); + } + + #[test] + fn advanced_command_recalls_bare_in_advanced_mode() { + // In advanced mode the stored `:`-prefix is stripped so it runs + // as bare SQL. + let mut app = App::new(); + app.mode = Mode::Advanced; + type_str(&mut app, "select 1"); + submit(&mut app); + app.update(key(KeyCode::Up)); + assert_eq!(app.input, "select 1"); + } + + #[test] + fn simple_command_recalls_bare_in_either_mode() { + let mut app = App::new(); + type_str(&mut app, "drop table T"); + submit(&mut app); + app.update(key(KeyCode::Up)); + assert_eq!(app.input, "drop table T"); + app.mode = Mode::Advanced; + app.update(key(KeyCode::Down)); // back to draft + app.update(key(KeyCode::Up)); + assert_eq!(app.input, "drop table T"); + } + + #[test] + fn app_command_recalls_bare_even_when_typed_with_colon() { + // An app command runs in any mode, so it must NOT gain a `:` on + // recall even when entered via the one-shot escape. + let mut app = App::new(); + type_str(&mut app, ": mode advanced"); + submit(&mut app); + app.update(key(KeyCode::Up)); + assert_eq!(app.input, "mode advanced"); + } + + #[test] + fn a_bare_colon_is_not_recorded_in_history() { + let mut app = App::new(); + type_str(&mut app, ":"); + submit(&mut app); + // Nothing recallable. + app.update(key(KeyCode::Up)); + assert_eq!(app.input, ""); + } + #[test] fn add_column_with_text_type_emits_execute_action() { let mut app = App::new(); diff --git a/src/db.rs b/src/db.rs index a057ebe..f6f08b9 100644 --- a/src/db.rs +++ b/src/db.rs @@ -2262,12 +2262,10 @@ fn handle_request( // (`show table`), it belongs in the complete journal // (ADR-0034). ADR-0035 §4. if if_not_exists && user_table_exists(conn, &name).unwrap_or(false) { - let result = do_describe_table(conn, &name).and_then(|desc| { - if let (Some(p), Some(text)) = (persistence, source.as_deref()) { - p.append_history(text).map_err(DbError::from_persistence)?; - } - Ok(CreateOutcome::Skipped(desc)) - }); + // ADR-0052: journaling moved to the dispatch layer; this + // no-op skip is an `Ok` outcome there and is journalled by + // the spawn like any other. + let result = do_describe_table(conn, &name).map(CreateOutcome::Skipped); let _ = reply.send(result); } else { snapshot_then(snap, batch, conn, source.as_deref(), reply, || { @@ -2306,12 +2304,8 @@ fn handle_request( // line is still journalled — like the `CREATE TABLE IF NOT // EXISTS` skip and other no-ops (ADR-0034). ADR-0035 §4. if if_exists && !user_table_exists(conn, &name).unwrap_or(false) { - let result = (|| { - if let (Some(p), Some(text)) = (persistence, source.as_deref()) { - p.append_history(text).map_err(DbError::from_persistence)?; - } - Ok(DropOutcome::Skipped) - })(); + // ADR-0052: journaling moved to the dispatch layer. + let result: Result = Ok(DropOutcome::Skipped); let _ = reply.send(result); } else { snapshot_then(snap, batch, conn, source.as_deref(), reply, || { @@ -2519,12 +2513,9 @@ fn handle_request( // ADR-0035 §4). Existence uses the same user-index lookup as // `do_drop_index` (`sql IS NOT NULL`). if if_exists && !index_exists(conn, &name, true).unwrap_or(false) { - let result = (|| { - if let (Some(p), Some(text)) = (persistence, source.as_deref()) { - p.append_history(text).map_err(DbError::from_persistence)?; - } - Ok(DropIndexOutcome::Skipped) - })(); + // ADR-0052: journaling moved to the dispatch layer. + let result: Result = + Ok(DropIndexOutcome::Skipped); let _ = reply.send(result); } else { snapshot_then(snap, batch, conn, source.as_deref(), reply, || { @@ -2555,12 +2546,9 @@ fn handle_request( // hits `do_add_index`'s redundant-set refusal (ADR-0025). let resolved = resolve_index_name(name.as_deref(), &table, &columns); if if_not_exists && index_exists(conn, &resolved, false).unwrap_or(false) { - let result = (|| { - if let (Some(p), Some(text)) = (persistence, source.as_deref()) { - p.append_history(text).map_err(DbError::from_persistence)?; - } - Ok(CreateIndexOutcome::Skipped(resolved.clone())) - })(); + // ADR-0052: journaling moved to the dispatch layer. + let result: Result = + Ok(CreateIndexOutcome::Skipped(resolved)); let _ = reply.send(result); } else { snapshot_then(snap, batch, conn, source.as_deref(), reply, || { @@ -3065,10 +3053,21 @@ struct Changes { /// Read-only requests (no schema change, no row writes, no /// drops) still use this to append `history.log` if `source` /// is set; they pass an empty `Changes`. +// Persist the **state** sources (project.yaml + data/*.csv) for a +// committed mutation, inside the worker transaction (ADR-0015 §6 +// commit-db-last). `history.log` is NOT written here — ADR-0052 moved +// journaling to the dispatch layer (runtime), so the command's mode is +// available without plumbing it through the worker, and a journal-write +// failure no longer rolls back a committed command (it is best-effort, +// like the failure path). fn finalize_persistence( conn: &Connection, persistence: Option<&Persistence>, - source: Option<&str>, + // Vestigial since ADR-0052 (the `history.log` write that used it moved + // to the dispatch layer). Retained so the ~28 worker handlers that + // thread `source` to here keep a use for it, rather than orphaning the + // param across all of them; a later cleanup could unwind that plumbing. + _source: Option<&str>, changes: &Changes, ) -> Result<(), DbError> { let Some(p) = persistence else { @@ -3093,10 +3092,6 @@ fn finalize_persistence( p.delete_table_data(table) .map_err(DbError::from_persistence)?; } - if let Some(text) = source { - p.append_history(text) - .map_err(DbError::from_persistence)?; - } Ok(()) } @@ -8361,18 +8356,18 @@ fn do_drop_index( /// Read-only wrapper around `do_describe_table` that runs an /// auxiliary `history.log` append for user-issued /// `show table` commands. +// ADR-0052: journaling moved to the dispatch layer, so this read-only +// `show table` wrapper no longer appends to `history.log` — the spawn +// journals the `Ok` outcome. Kept as a thin delegate (a later cleanup +// could inline `do_describe_table` at the one call site); `_persistence` +// / `_source` are vestigial. fn do_describe_table_request( conn: &Connection, - persistence: Option<&Persistence>, - source: Option<&str>, + _persistence: Option<&Persistence>, + _source: Option<&str>, name: &str, ) -> Result { - let description = do_describe_table(conn, name)?; - if let (Some(p), Some(text)) = (persistence, source) { - p.append_history(text) - .map_err(DbError::from_persistence)?; - } - Ok(description) + do_describe_table(conn, name) } fn do_describe_table(conn: &Connection, name: &str) -> Result { @@ -9981,40 +9976,32 @@ fn do_delete( }) } -/// Read-only wrapper that adds the `history.log` append for -/// `show data` user commands. +/// Read-only `show data` wrapper. ADR-0052: journaling moved to the +/// dispatch layer (`_persistence` / `_source` vestigial). fn do_query_data_request( conn: &Connection, - persistence: Option<&Persistence>, - source: Option<&str>, + _persistence: Option<&Persistence>, + _source: Option<&str>, table: &str, filter: Option<&Expr>, limit: Option, ) -> Result { - let data = do_query_data(conn, table, filter, limit)?; - if let (Some(p), Some(text)) = (persistence, source) { - p.append_history(text) - .map_err(DbError::from_persistence)?; - } - Ok(data) + // ADR-0052: journaling moved to the dispatch layer (`_persistence` / + // `_source` vestigial; the spawn journals the `Ok` outcome). + do_query_data(conn, table, filter, limit) } -/// Worker handler for `Request::RunSelect` (ADR-0030 §6, -/// ADR-0031). Mirrors `do_query_data_request`: run the -/// statement, append the literal line to `history.log` so a +/// Worker handler for `Request::RunSelect` (ADR-0030 §6, ADR-0031). +/// ADR-0052: journaling moved to the dispatch layer, so this no longer +/// appends to `history.log` — the spawn journals the literal line so a /// replay re-runs it (ADR-0030 §11). fn do_run_select_request( conn: &Connection, - persistence: Option<&Persistence>, - source: Option<&str>, + _persistence: Option<&Persistence>, + _source: Option<&str>, sql: &str, ) -> Result { - let data = do_run_select(conn, sql)?; - if let (Some(p), Some(text)) = (persistence, source) { - p.append_history(text) - .map_err(DbError::from_persistence)?; - } - Ok(data) + do_run_select(conn, sql) } /// Currently-stored non-NULL values of one column, for shortid @@ -11119,8 +11106,10 @@ fn read_relationships_inbound( /// violation aborts with a fatal error. fn do_rebuild_from_text( conn: &Connection, - persistence: Option<&Persistence>, - source: Option<&str>, + // Vestigial since ADR-0052: `rebuild` is journalled at the dispatch + // layer (`spawn_rebuild`), not here. + _persistence: Option<&Persistence>, + _source: Option<&str>, project_path: &Path, ) -> Result<(), DbError> { debug!(path = %project_path.display(), "rebuild_from_text"); @@ -11320,10 +11309,8 @@ fn do_rebuild_from_text( // 7. Append `history.log` if this rebuild was // user-initiated (the silent on-load case has // `source = None`). - if let (Some(p), Some(text)) = (persistence, source) { - p.append_history(text) - .map_err(DbError::from_persistence)?; - } + // ADR-0052: `rebuild` is journalled at the dispatch layer + // (`spawn_rebuild`), not here — journaling left the worker. tx.commit().map_err(DbError::from_rusqlite)?; Ok(()) diff --git a/src/event.rs b/src/event.rs index 623f299..b71b850 100644 --- a/src/event.rs +++ b/src/event.rs @@ -161,6 +161,11 @@ pub enum AppEvent { /// commands, so an execution failure would otherwise be /// lost across sessions. source: String, + /// Whether the rejected command was submitted in an advanced + /// effective mode (ADR-0052): threaded so the App can tag the + /// `err` record `err:adv` and the failed advanced command + /// hydrates in its `:`-prefixed, simple-mode-recallable form. + advanced: bool, }, /// Refreshed list of tables in the database. TablesRefreshed(Vec), diff --git a/src/persistence/history.rs b/src/persistence/history.rs index cd0bf13..9fff901 100644 --- a/src/persistence/history.rs +++ b/src/persistence/history.rs @@ -28,7 +28,35 @@ use super::PersistenceError; pub(super) const STATUS_OK: &str = "ok"; pub(super) const STATUS_ERR: &str = "err"; -/// Format a successful-command record. Pure; no I/O. +/// The optional status suffix marking an advanced-mode submission +/// (ADR-0052, issue #30): `ok:adv` / `err:adv`. Recorded so that +/// hydration can reconstruct the `:`-prefixed runnable form of an +/// advanced command, making advanced history reusable in simple mode. +pub(super) const ADV_SUFFIX: &str = "adv"; + +/// Build the status token for a `base` (`ok`/`err`) and submission mode. +pub(super) fn status_token(base: &str, advanced: bool) -> String { + if advanced { + format!("{base}:{ADV_SUFFIX}") + } else { + base.to_string() + } +} + +/// Parse a status token into `(is_ok, advanced)` (ADR-0052). The base +/// (`ok` ⇒ replayable, anything else ⇒ skip) precedes an optional +/// `:adv` mode suffix. An unknown base degrades to `(false, _)`, so +/// replay skips it rather than mis-running it. +pub(super) fn parse_status(status: &str) -> (bool, bool) { + let (base, suffix) = status.split_once(':').unwrap_or((status, "")); + (base == STATUS_OK, suffix == ADV_SUFFIX) +} + +/// Format a successful-command record. Pure; no I/O. (Simple-mode +/// convenience used by tests; production threads the mode through +/// [`format_record_with_status`] + [`status_token`], so this is +/// test-only since ADR-0052.) +#[cfg(test)] pub(super) fn format_record(command_text: &str, timestamp_iso: String) -> String { format_record_with_status(command_text, timestamp_iso, STATUS_OK) } @@ -100,9 +128,20 @@ fn parse_record_source(line: &str) -> Option { // characters) is preserved. let mut parts = line.splitn(3, '|'); let _ts = parts.next()?; - let _status = parts.next()?; + let status = parts.next()?; let source = parts.next()?; - Some(unescape_command(source)) + let (_is_ok, advanced) = parse_status(status); + let command = unescape_command(source); + // ADR-0052: an advanced record is hydrated in its `:`-prefixed + // simple-mode runnable form, so cross-session recall matches the + // in-session ring (and recall strips the `:` again in advanced + // mode). A simple record hydrates bare. Old `ok`/`err` logs have no + // `:adv` suffix → read as simple, unchanged. + Some(if advanced { + format!(": {command}") + } else { + command + }) } /// A parsed journal record (ADR-0034 §3). `source` is already @@ -129,8 +168,11 @@ pub(super) fn parse_journal_record(line: &str) -> Option { if !looks_like_iso8601(ts) { return None; } + // ADR-0052: the status may carry a `:adv` mode suffix; replayability + // keys off the base token only (`ok` / `ok:adv` are both ok). + let (status_is_ok, _advanced) = parse_status(status); Some(JournalRecord { - status_is_ok: status == STATUS_OK, + status_is_ok, source: unescape_command(source), }) } @@ -436,4 +478,70 @@ mod tests { let body = fs::read_to_string(&path).unwrap(); assert_eq!(body, "first|ok|a\nsecond|ok|b\n"); } + + // ---- ADR-0052 (issue #30): mode tag in the status field ---- + + #[test] + fn status_token_builds_and_parses_the_adv_suffix() { + assert_eq!(status_token(STATUS_OK, false), "ok"); + assert_eq!(status_token(STATUS_OK, true), "ok:adv"); + assert_eq!(status_token(STATUS_ERR, true), "err:adv"); + assert_eq!(parse_status("ok"), (true, false)); + assert_eq!(parse_status("ok:adv"), (true, true)); + assert_eq!(parse_status("err"), (false, false)); + assert_eq!(parse_status("err:adv"), (false, true)); + // Unknown base → not ok (replay skips it), simple. + assert_eq!(parse_status("frobnicate"), (false, false)); + } + + #[test] + fn read_recent_sources_reconstructs_colon_prefix_for_advanced() { + // An advanced record (`ok:adv`) hydrates in its `:`-prefixed + // simple-mode runnable form; a simple record stays bare. This is + // the cross-session half of the issue #30 fix. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("history.log"); + let adv = format_record_with_status( + "select * from T", + "2026-06-13T10:00:00Z".to_string(), + &status_token(STATUS_OK, true), + ); + let simple = format_record_with_status( + "create table T with pk", + "2026-06-13T10:00:01Z".to_string(), + &status_token(STATUS_OK, false), + ); + std::fs::write(&path, format!("{adv}{simple}")).unwrap(); + let got = read_recent_sources(&path, 10).unwrap(); + assert_eq!( + got, + vec![ + ": select * from T".to_string(), + "create table T with pk".to_string(), + ], + ); + } + + #[test] + fn parse_journal_record_treats_ok_adv_as_ok() { + // Replay keys off the base token, so `ok:adv` replays like `ok` + // (source stays canonical). + let rec = parse_journal_record("2026-06-13T10:00:00Z|ok:adv|select * from T") + .expect("ok:adv journal record"); + assert!(rec.status_is_ok); + assert_eq!(rec.source, "select * from T"); + let err = parse_journal_record("2026-06-13T10:00:00Z|err:adv|select bad") + .expect("err:adv journal record"); + assert!(!err.status_is_ok); + } + + #[test] + fn old_three_field_log_reads_as_simple() { + // Back-compat: a pre-ADR-0052 log (no `:adv`) hydrates bare. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("history.log"); + std::fs::write(&path, "2026-01-01T00:00:00Z|ok|select 1\n").unwrap(); + let got = read_recent_sources(&path, 10).unwrap(); + assert_eq!(got, vec!["select 1".to_string()]); + } } diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index 96ccec7..745d3c4 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -395,11 +395,26 @@ impl Persistence { } } - /// Append one successful-command record to `history.log`. - pub fn append_history(&self, command_text: &str) -> Result<(), PersistenceError> { + /// Append one successful-command record to `history.log`. `advanced` + /// (ADR-0052) tags the record `ok:adv` when the command was submitted + /// in an advanced effective mode, so hydration can reconstruct its + /// `:`-prefixed form for reuse in simple mode. + pub fn append_history( + &self, + command_text: &str, + advanced: bool, + ) -> Result<(), PersistenceError> { let path = self.project_path.join(HISTORY_LOG); - let line = history::format_record(command_text, history::utc_iso8601_now()); - debug!(len = command_text.len(), "persist: append ok record to history.log"); + let status = history::status_token(history::STATUS_OK, advanced); + let line = history::format_record_with_status( + command_text, + history::utc_iso8601_now(), + &status, + ); + debug!( + len = command_text.len(), + advanced, "persist: append ok record to history.log" + ); history::append(&path, &line) } @@ -410,14 +425,22 @@ impl Persistence { /// transactional `ok` journal). Best-effort at the call site: /// a failure to record a failure must never escalate a user /// error into a fatal (ADR-0034 §4). - pub fn append_history_failure(&self, command_text: &str) -> Result<(), PersistenceError> { + pub fn append_history_failure( + &self, + command_text: &str, + advanced: bool, + ) -> Result<(), PersistenceError> { let path = self.project_path.join(HISTORY_LOG); + let status = history::status_token(history::STATUS_ERR, advanced); let line = history::format_record_with_status( command_text, history::utc_iso8601_now(), - history::STATUS_ERR, + &status, + ); + debug!( + len = command_text.len(), + advanced, "persist: append err record to history.log" ); - debug!(len = command_text.len(), "persist: append err record to history.log"); history::append(&path, &line) } @@ -577,8 +600,8 @@ mod tests { fn append_history_creates_and_appends() { let dir = tempdir(); let p = Persistence::new(dir.path().to_path_buf()); - p.append_history("create table Foo with pk id(serial)").unwrap(); - p.append_history("insert into Foo (1)").unwrap(); + p.append_history("create table Foo with pk id(serial)", false).unwrap(); + p.append_history("insert into Foo (1)", false).unwrap(); let body = fs::read_to_string(dir.path().join(HISTORY_LOG)).unwrap(); let lines: Vec<&str> = body.trim_end().lines().collect(); assert_eq!(lines.len(), 2); diff --git a/src/runtime.rs b/src/runtime.rs index df5b8fb..0b9667b 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -479,17 +479,19 @@ async fn run_loop( command, source, submission_mode, + session.project().path().to_path_buf(), ); } - Action::JournalFailure { source } => { + Action::JournalFailure { source, advanced } => { // ADR-0034 §1/§4: record a failed command as an - // `err` record. Best-effort — a failure to record - // a failure must never escalate a user error into - // a fatal, so the result is logged and ignored. + // `err` record (ADR-0052: `err:adv` when advanced). + // Best-effort — a failure to record a failure must + // never escalate a user error into a fatal, so the + // result is logged and ignored. if let Err(e) = crate::persistence::Persistence::new( session.project().path().to_path_buf(), ) - .append_history_failure(&source) + .append_history_failure(&source, advanced) { tracing::warn!(error = %e, "failed to journal err record (ignored)"); } @@ -971,7 +973,9 @@ async fn perform_switch( // history.log. The worker's persistence is wired but not // directly addressable from here, so we use a fresh // Persistence handle for this single line. - let _ = Persistence::new(new_path.clone()).append_history(&source); + // App-lifecycle command (save-as/load/new): journalled simple + // (ADR-0052 — app commands run in any mode, so no `:` on recall). + let _ = Persistence::new(new_path.clone()).append_history(&source, false); // Update the resume pointer so the next `--resume` launch // reopens the project we just switched to — unless it is a @@ -1040,7 +1044,9 @@ fn spawn_export( source: String, event_tx: mpsc::Sender, ) { - let _ = crate::persistence::Persistence::new(project_path.clone()).append_history(&source); + // `export` app command: journalled simple (ADR-0052). + let _ = crate::persistence::Persistence::new(project_path.clone()) + .append_history(&source, false); tokio::spawn(async move { let outcome = tokio::task::spawn_blocking(move || { do_export(&project_path, &project_name, &data_root, target.as_deref()) @@ -1295,11 +1301,20 @@ fn spawn_rebuild( source: String, ) { tokio::spawn(async move { + let source_for_journal = source.clone(); match database .rebuild_from_text(project_path.clone(), Some(source)) .await { Ok(()) => { + // ADR-0052: journal `rebuild` at the dispatch layer (the + // worker no longer journals); simple (app command), + // best-effort. + if let Err(e) = crate::persistence::Persistence::new(project_path.clone()) + .append_history(&source_for_journal, false) + { + warn!(error = %e, "failed to journal rebuild (ignored)"); + } let summary = summarize_project(&project_path) .unwrap_or_else(|_| "rebuild complete".to_string()); let _ = event_tx @@ -1407,10 +1422,11 @@ fn spawn_dsl_dispatch( command: Command, source: String, submission_mode: crate::app::EffectiveMode, + project_path: std::path::PathBuf, ) { tokio::spawn(async move { - // Retain the source for `DslFailed` so the App can journal a - // rejected command as `err` (ADR-0034 §1/§2). + // Retain the source for journaling (ADR-0034 §1/§2; ADR-0052 + // moved success journaling here, next to the failure path). let source_for_journal = source.clone(); // ADR-0038: the DSL → SQL teaching echo fires for a DSL-form // command submitted in an advanced effective mode (ADR-0037). @@ -1431,6 +1447,19 @@ fn spawn_dsl_dispatch( let lookups = collect_echo_lookups(&database, &command, submission_mode).await; let echo = crate::echo::echo_for(&command, submission_mode); let outcome = execute_command_typed(&database, command.clone(), source).await; + // ADR-0052 (issue #30): journal a SUCCESSFUL command here, at the + // top of the chain — the canonical source + submission mode are + // both in scope, so no mode-plumbing into the worker is needed. + // Best-effort (ADR-0040 amended): the command is already committed; + // a journal-write failure is logged, never fatal. Failures stay on + // the `JournalFailure` path (Ok/Err are exclusive — no double + // journal). `:adv` tags an advanced submission (ADR-0052). + if outcome.is_ok() + && let Err(e) = crate::persistence::Persistence::new(project_path) + .append_history(&source_for_journal, submission_mode.is_advanced()) + { + warn!(error = %e, "failed to journal ok record (ignored)"); + } let event = match outcome { Ok(CommandOutcome::Schema(description)) => { let schema_echo = build_schema_echo( @@ -1569,6 +1598,7 @@ fn spawn_dsl_dispatch( error, facts, source: source_for_journal, + advanced: submission_mode.is_advanced(), } } }; @@ -2540,6 +2570,15 @@ pub async fn run_replay( execute_command_typed(database, command, command_text.clone()).await; match outcome { Ok(_) => { + // ADR-0052: journal the replayed line at the dispatch + // layer (the worker no longer journals). Replay is + // mode-agnostic, so the re-written record is tagged + // simple; best-effort, like the interactive path. + if let Err(e) = crate::persistence::Persistence::new(project_root.to_path_buf()) + .append_history(&command_text, false) + { + warn!(error = %e, "failed to journal replayed line (ignored)"); + } count += 1; } Err(DbError::PersistenceFatal { diff --git a/tests/it/iteration2_persistence.rs b/tests/it/iteration2_persistence.rs index 22b70f1..0260113 100644 --- a/tests/it/iteration2_persistence.rs +++ b/tests/it/iteration2_persistence.rs @@ -15,7 +15,7 @@ use rdbms_playground::db::Database; use rdbms_playground::dsl::{ColumnSpec, ReferentialAction, RowFilter, Type, Value}; use rdbms_playground::persistence::Persistence; use rdbms_playground::project::{ - self, DATA_DIR, HISTORY_LOG, PROJECT_YAML, + self, DATA_DIR, PROJECT_YAML, }; fn tempdir() -> tempfile::TempDir { @@ -44,11 +44,6 @@ fn open_project( (project, db, path) } -fn read_history(project_path: &Path) -> Vec { - let body = fs::read_to_string(project_path.join(HISTORY_LOG)).unwrap_or_default(); - body.lines().map(str::to_string).collect() -} - fn read_yaml(project_path: &Path) -> String { fs::read_to_string(project_path.join(PROJECT_YAML)).expect("project.yaml") } @@ -82,9 +77,9 @@ fn create_table_writes_yaml_and_history() { assert!(yaml.contains("type: serial"), "yaml: {yaml}"); assert!(yaml.contains("type: text"), "yaml: {yaml}"); - let history = read_history(&path); - assert_eq!(history.len(), 1, "expected one history line; got {history:?}"); - assert!(history[0].ends_with("|ok|create table Customers with pk id(serial)")); + // ADR-0052: journaling moved to the dispatch layer (the worker no + // longer writes history.log); this test verifies only the yaml state. + // Journaling is covered by the history.rs/app.rs/replay tests. } #[test] @@ -119,11 +114,8 @@ fn insert_writes_csv_and_history() { assert_eq!(lines[0], "id,Name"); assert_eq!(lines[1], "1,Alice"); - let history = read_history(&path); - assert!( - history.iter().any(|l| l.ends_with("|ok|insert into Customers ('Alice')")), - "history missing insert: {history:?}", - ); + // ADR-0052: journaling moved off the worker; this test verifies the + // csv state only (journaling covered elsewhere). } #[test] @@ -322,39 +314,6 @@ fn delete_all_rows_removes_csv() { ); } -#[test] -fn show_table_appends_history_only() { - let data = tempdir(); - let (_p, db, path) = open_project(&data); - - rt().block_on(async { - db.create_table( - "Customers".to_string(), - vec![ColumnSpec::new("id".to_string(), Type::Serial)], - vec!["id".to_string()], - Some("create table Customers with pk id(serial)".to_string()), - ) - .await - .unwrap(); - let yaml_before = read_yaml(&path); - db.describe_table( - "Customers".to_string(), - Some("show table Customers".to_string()), - ) - .await - .unwrap(); - let yaml_after = read_yaml(&path); - // YAML body did not change for a read-only command. - assert_eq!(yaml_before, yaml_after); - }); - - let history = read_history(&path); - assert!( - history.iter().any(|l| l.ends_with("|ok|show table Customers")), - "history missing show entry: {history:?}", - ); -} - #[test] fn failed_command_does_not_append_history_or_change_yaml() { let data = tempdir(); @@ -387,13 +346,8 @@ fn failed_command_does_not_append_history_or_change_yaml() { assert_eq!(yaml_before, yaml_after, "failed cmd must not change yaml"); }); - let history = read_history(&path); - // Only the first (successful) create_table should have logged. - let create_count = history - .iter() - .filter(|l| l.contains("|ok|create table Customers")) - .count(); - assert_eq!(create_count, 1, "expected exactly one logged create; got: {history:?}"); + // ADR-0052: journaling moved off the worker; this test now verifies + // only that a failed command does not change the yaml state. } #[test] diff --git a/tests/it/iteration4a_rebuild_command.rs b/tests/it/iteration4a_rebuild_command.rs index 311ad07..8fb6972 100644 --- a/tests/it/iteration4a_rebuild_command.rs +++ b/tests/it/iteration4a_rebuild_command.rs @@ -178,10 +178,7 @@ fn rebuild_against_populated_db_wipes_and_reloads() { assert_eq!(rows.rows.len(), 1); assert_eq!(rows.rows[0][1].as_deref(), Some("Edna")); - // history.log should contain the rebuild entry. - let history = fs::read_to_string(project_path.join("history.log")).unwrap(); - assert!( - history.lines().any(|l| l.ends_with("|ok|rebuild")), - "history.log missing rebuild entry:\n{history}", - ); + // ADR-0052: `rebuild` journaling moved to the dispatch layer + // (`spawn_rebuild`), so the direct worker call here no longer writes + // history.log; this test verifies the wipe/reload behaviour only. } diff --git a/tests/it/iteration6_resume_history.rs b/tests/it/iteration6_resume_history.rs index 49b47b2..d4fe1d4 100644 --- a/tests/it/iteration6_resume_history.rs +++ b/tests/it/iteration6_resume_history.rs @@ -141,9 +141,9 @@ fn read_recent_history_returns_appended_entries_in_order() { let tmp = tempdir(); let project = Project::create_temp(tmp.path()).unwrap(); let p = Persistence::new(project.path().to_path_buf()); - p.append_history("create table A with pk").unwrap(); - p.append_history("create table B with pk").unwrap(); - p.append_history("create table C with pk").unwrap(); + p.append_history("create table A with pk", false).unwrap(); + p.append_history("create table B with pk", false).unwrap(); + p.append_history("create table C with pk", false).unwrap(); let entries = p.read_recent_history(10).unwrap(); assert_eq!( entries, @@ -165,9 +165,9 @@ fn hydration_reads_both_ok_and_err_records() { let tmp = tempdir(); let project = Project::create_temp(tmp.path()).unwrap(); let p = Persistence::new(project.path().to_path_buf()); - p.append_history("create table A with pk").unwrap(); - p.append_history_failure("insert into A (1, 2, 3)").unwrap(); - p.append_history("show data A").unwrap(); + p.append_history("create table A with pk", false).unwrap(); + p.append_history_failure("insert into A (1, 2, 3)", false).unwrap(); + p.append_history("show data A", false).unwrap(); let entries = p.read_recent_history(10).unwrap(); assert_eq!( entries, @@ -194,6 +194,32 @@ fn seed_history_replaces_in_memory_history() { assert_eq!(app.history, vec!["x".to_string(), "y".to_string()]); } +#[test] +fn advanced_command_journalled_then_hydrated_recalls_with_colon_in_simple() { + // ADR-0052 (issue #30) — the headline cross-session regression: an + // advanced command journalled `ok:adv`, then hydrated on a fresh + // session, recalls WITH its `:` so it re-runs in simple mode. (Before + // the fix, the `:` was lost on disk and the command came back bare.) + let tmp = tempdir(); + let project = Project::create_temp(tmp.path()).unwrap(); + let p = Persistence::new(project.path().to_path_buf()); + // The dispatch layer journals the canonical source + advanced flag. + p.append_history("select * from T", true).unwrap(); + p.append_history("create table T with pk", false).unwrap(); + + // Fresh session: hydrate the ring from disk. + let entries = p.read_recent_history(10).unwrap(); + let mut app = App::new(); + app.seed_history(entries); + + // In simple mode the simple command recalls bare, the advanced one + // recalls `:`-prefixed (runnable via the one-shot escape). + app.update(key(KeyCode::Up)); + assert_eq!(app.input, "create table T with pk"); + app.update(key(KeyCode::Up)); + assert_eq!(app.input, ": select * from T"); +} + #[test] fn seed_history_preserves_chronological_order_for_navigation() { let mut app = App::new(); diff --git a/tests/it/seed.rs b/tests/it/seed.rs index eba3e3c..ed389b1 100644 --- a/tests/it/seed.rs +++ b/tests/it/seed.rs @@ -430,24 +430,6 @@ fn seed_is_reproducible_with_a_fixed_seed() { assert_eq!(csv1, csv2, "the same --seed must reproduce identical data"); } -#[test] -fn seed_writes_exactly_one_history_line() { - let (project, db, _dir) = open_project_db(); - let rt = rt(); - create_people(&db, &rt); - - rt.block_on(db.seed("People".into(), None, Some(5), Vec::new(), Some(1), Some("seed People 5".into()))) - .expect("seed succeeds"); - - let history = std::fs::read_to_string(project.path().join("history.log")) - .expect("history.log exists"); - let seed_lines = history.lines().filter(|l| l.contains("seed People 5")).count(); - assert_eq!( - seed_lines, 1, - "a seed of 5 rows must write exactly one history line:\n{history}" - ); -} - // — FK sampling, empty-parent error, block guard (ADR-0048 D14 / D1) — /// `Users(id serial pk, name text)` + `Orders(id serial pk, user_id diff --git a/tests/it/sql_create_index.rs b/tests/it/sql_create_index.rs index 1e24518..96a1e99 100644 --- a/tests/it/sql_create_index.rs +++ b/tests/it/sql_create_index.rs @@ -141,7 +141,7 @@ fn create_unique_index_on_duplicate_data_is_refused() { #[test] fn if_not_exists_on_an_existing_name_is_a_noop_and_journalled() { - let (p, db, _d) = open(false); + let (_p, db, _d) = open(false); let r = rt(); make_t(&db, &r); r.block_on(db.sql_create_index( @@ -169,8 +169,8 @@ fn if_not_exists_on_an_existing_name_is_a_noop_and_journalled() { CreateIndexOutcome::Skipped(name) => assert_eq!(name, "ix"), CreateIndexOutcome::Created(_) => panic!("expected Skipped, got Created"), } - let log = std::fs::read_to_string(p.path().join("history.log")).expect("read history.log"); - assert!(log.contains(line), "the skipped create should be journalled; log:\n{log}"); + // ADR-0052: journaling moved to the dispatch layer; this test now + // asserts only the no-op `Skipped` outcome. } #[test] diff --git a/tests/it/sql_create_table.rs b/tests/it/sql_create_table.rs index 0d07f54..3ac8564 100644 --- a/tests/it/sql_create_table.rs +++ b/tests/it/sql_create_table.rs @@ -590,7 +590,7 @@ fn if_not_exists_noop_is_journalled() { // A successful no-op is still a submission and belongs in the // complete journal (ADR-0034) — like read-only `show table`, and // unlike a *failed* duplicate-create (journalled `err`). - let (p, db, _d) = open(false); + let (_p, db, _d) = open(false); let r = rt(); r.block_on(db.sql_create_table( "T".to_string(), @@ -617,8 +617,8 @@ fn if_not_exists_noop_is_journalled() { )) .expect("no-op"); assert!(matches!(out, CreateOutcome::Skipped(_))); - let log = std::fs::read_to_string(p.path().join("history.log")).expect("read history.log"); - assert!(log.contains(noop), "the no-op skip should be journalled; log:\n{log}"); + // ADR-0052: journaling moved to the dispatch layer; this test now + // asserts only the no-op `Skipped` outcome. } #[test] diff --git a/tests/it/sql_delete.rs b/tests/it/sql_delete.rs index 43f3a23..b91f88b 100644 --- a/tests/it/sql_delete.rs +++ b/tests/it/sql_delete.rs @@ -261,19 +261,6 @@ fn r2_cascade_with_subquery_where() { "only Bob's order remains: {orders_csv:?}"); } -#[test] -fn delete_appends_literal_line_to_history() { - let (project, db, _dir) = open_project_db(); - let rt = rt(); - create_cols(&db, &rt, "t", &[("id", Type::Int), ("v", Type::Text)], &["id"]); - seed(&db, &rt, "insert into t (id, v) values (1, 'x')", "t"); - let input = "delete from t where id = 1"; - run_delete(&db, &rt, input).expect("delete runs"); - let body = std::fs::read_to_string(project.path().join("history.log")) - .expect("history.log present"); - assert!(body.contains(input), "history records the literal line: {body:?}"); -} - #[test] fn cascade_to_two_children_reports_both() { // DA gate (untested branch): a parent with TWO cascade children diff --git a/tests/it/sql_drop_index.rs b/tests/it/sql_drop_index.rs index fd4dd3a..ca5f0d5 100644 --- a/tests/it/sql_drop_index.rs +++ b/tests/it/sql_drop_index.rs @@ -84,7 +84,7 @@ fn drop_index_removes_an_existing_index_and_shows_the_table() { #[test] fn if_exists_on_an_absent_index_is_a_noop_and_journalled() { - let (p, db, _d) = open(false); + let (_p, db, _d) = open(false); let r = rt(); let line = "drop index if exists ghost_idx"; let out = r @@ -92,8 +92,8 @@ fn if_exists_on_an_absent_index_is_a_noop_and_journalled() { .expect("IF EXISTS on an absent index succeeds as a no-op"); assert!(matches!(out, DropIndexOutcome::Skipped)); // The no-op is still journalled (ADR-0034), like the create-skip. - let log = std::fs::read_to_string(p.path().join("history.log")).expect("read history.log"); - assert!(log.contains(line), "the skipped drop should be journalled; log:\n{log}"); + // ADR-0052: journaling moved to the dispatch layer; this test now + // asserts only the no-op `Skipped` outcome. } #[test] diff --git a/tests/it/sql_drop_table.rs b/tests/it/sql_drop_table.rs index a94195f..a95bb0e 100644 --- a/tests/it/sql_drop_table.rs +++ b/tests/it/sql_drop_table.rs @@ -59,7 +59,7 @@ fn drop_table_removes_an_existing_table() { #[test] fn if_exists_on_an_absent_table_is_a_noop_and_journalled() { - let (p, db, _d) = open(false); + let (_p, db, _d) = open(false); let r = rt(); let line = "drop table if exists Ghost"; let out = r @@ -67,8 +67,8 @@ fn if_exists_on_an_absent_table_is_a_noop_and_journalled() { .expect("IF EXISTS on an absent table succeeds as a no-op"); assert!(matches!(out, DropOutcome::Skipped)); // The no-op is still journalled (ADR-0034), like the create-skip. - let log = std::fs::read_to_string(p.path().join("history.log")).expect("read history.log"); - assert!(log.contains(line), "the skipped drop should be journalled; log:\n{log}"); + // ADR-0052: journaling moved to the dispatch layer; this test now + // asserts only the no-op `Skipped` outcome. } #[test] diff --git a/tests/it/sql_insert.rs b/tests/it/sql_insert.rs index 4afe14c..fdd71ec 100644 --- a/tests/it/sql_insert.rs +++ b/tests/it/sql_insert.rs @@ -132,30 +132,6 @@ fn no_column_list_full_arity_insert_persists() { assert!(csv.contains("full-arity"), "CSV reflects the row: {csv:?}"); } -#[test] -fn insert_appends_literal_line_to_history() { - let (project, db, _dir) = open_project_db(); - let rt = rt(); - create_t(&db, &rt); - // ADR-0030 §11: the literal submitted line lands in history.log. - let source = "insert into T (a, b) values (1, 'logged')"; - rt.block_on(db.run_sql_insert( - "insert into T (a, b) values (1, 'logged')".to_string(), - Some(source.to_string()), - "T".to_string(), - Vec::new(), - String::new(), - false, - )) - .expect("insert runs"); - let body = std::fs::read_to_string(project.path().join("history.log")) - .expect("history.log present after an INSERT"); - assert!( - body.contains(source), - "history.log records the literal INSERT line: {body:?}", - ); -} - #[test] fn failed_insert_rolls_back_and_does_not_repersist() { let (project, db, _dir) = open_project_db(); @@ -583,26 +559,6 @@ fn combined_serial_and_shortid_autofill() { assert_eq!(rows[0][2], "x", "name preserved: {rows:?}"); } -#[test] -fn autofill_logs_original_source_not_rewritten_sql() { - // ADR-0030 §11: even though the worker rewrites the executed - // statement to bind synthesised shortids, history.log records - // the user's original line verbatim. - let (project, db, _dir) = open_project_db(); - let rt = rt(); - create_cols(&db, &rt, "t", &[("id", Type::ShortId), ("label", Type::Text)], &["id"]); - let input = "insert into t (label) values ('x')"; - run_sqlinsert(&db, &rt, input).expect("auto-fill insert runs"); - let body = std::fs::read_to_string(project.path().join("history.log")) - .expect("history.log present"); - assert!(body.contains(input), "original line logged: {body:?}"); - // The rewritten parameterised INSERT must not leak into history. - assert!( - !body.contains("INSERT INTO") && !body.contains("?1"), - "rewritten SQL must not be logged: {body:?}", - ); -} - #[test] fn shortid_autofill_respects_mixed_case_column_name() { // ADR-0009 / 3d DA gate: identifiers are case-preserving. The diff --git a/tests/it/sql_select.rs b/tests/it/sql_select.rs index eb79c9f..14c5654 100644 --- a/tests/it/sql_select.rs +++ b/tests/it/sql_select.rs @@ -732,23 +732,3 @@ fn database_run_select_recovers_all_ten_playground_types() { ); } } - -#[test] -fn database_run_select_appends_to_history_when_source_present() { - let (project, db, _dir) = open_project_db(); - let history_path = project.path().join("history.log"); - // ADR-0030 §11: the literal submitted line lands in - // history.log so replay re-runs it. - let _ = rt() - .block_on(db.run_select( - "select 1".to_string(), - Some("select 1".to_string()), - )) - .expect("SELECT runs"); - let body = std::fs::read_to_string(&history_path) - .expect("history.log present after a SELECT"); - assert!( - body.contains("select 1"), - "history.log records the literal SELECT line: {body:?}", - ); -} diff --git a/tests/it/sql_update.rs b/tests/it/sql_update.rs index b82ba3b..86c7415 100644 --- a/tests/it/sql_update.rs +++ b/tests/it/sql_update.rs @@ -205,19 +205,6 @@ fn update_matching_no_rows_is_ok() { assert!(csv.contains("keep") && !csv.contains('x'), "unchanged: {csv:?}"); } -#[test] -fn update_appends_literal_line_to_history() { - let (project, db, _dir) = open_project_db(); - let rt = rt(); - create_cols(&db, &rt, "t", &[("id", Type::Int), ("v", Type::Text)], &["id"]); - seed(&db, &rt, "insert into t (id, v) values (1, 'old')", "t"); - let input = "update t set v = 'new' where id = 1"; - run_update(&db, &rt, input).expect("update runs"); - let body = std::fs::read_to_string(project.path().join("history.log")) - .expect("history.log present"); - assert!(body.contains(input), "history records the literal line: {body:?}"); -} - // ================================================================= // ADR-0036 Phase 2 — `SET` literal value validation // ================================================================= diff --git a/tests/it/walking_skeleton.rs b/tests/it/walking_skeleton.rs index 375a8b3..40cea3c 100644 --- a/tests/it/walking_skeleton.rs +++ b/tests/it/walking_skeleton.rs @@ -661,6 +661,7 @@ fn dsl_failure_shows_friendly_error_in_output() { }, facts: rdbms_playground::friendly::FailureContext::default(), source: String::new(), + advanced: false, }); let rendered = rendered_text(&mut app, &Theme::dark(), 80, 24); assert!(