From e4f2f5fa1514e2c6b9fba3b266516a99ace53bf2 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Sun, 24 May 2026 18:59:06 +0000 Subject: [PATCH] =?UTF-8?q?feat:=20ADR-0034=20=E2=80=94=20history=20journa?= =?UTF-8?q?l=20records=20err=20+=20replay=20parses/filters=20the=20journal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replay (§3): run_replay parses || journal records — runs ok, skips non-ok — while still accepting bare .commands scripts (prefix-detected so a | inside a bare command isn't misread). Fixes replay history.log, which died on line 1. Journal failures (§1/§2): failed commands are recorded err via a new Action::JournalFailure, emitted by the pure-sync App for both parse failures and worker-execution failures (runtime appends best-effort, never fatal). Hydration reads all records so typo'd/rejected commands are recallable across sessions. Amendment 1 — replay filters app-lifecycle commands: a working replay history.log exposed that the journal also records save as/load/new/export/import/rebuild/mode (which would panic the worker dispatch or abort replay). Replay now re-applies only schema/data writes and skips every app-lifecycle command + nested replay, classified by entry word so modal/incomplete forms (save as, bare mode) and quit skip uniformly rather than aborting. All skips continue (reversing the nested-replay refusal); import and nested replay warn. replay.error_nested removed; replay.skipped_import/_replay added; ReplayCompleted carries warnings. requirements.md U3/U4 updated; app-command runtime-failure journalling tracked as a follow-up. 1659 passing / 0 failing / 0 skipped / 1 ignored. Clippy clean. --- .../0034-history-journal-and-replay-filter.md | 87 +++++++++++ docs/adr/README.md | 2 +- .../20260524-adr-0034-history-journal.md | 16 ++ docs/requirements.md | 23 ++- src/action.rs | 11 ++ src/app.rs | 124 ++++++++++++++- src/event.rs | 11 ++ src/friendly/keys.rs | 3 +- src/friendly/strings/en-US.yaml | 8 +- src/persistence/history.rs | 138 +++++++++++++++- src/persistence/mod.rs | 44 +++++- src/runtime.rs | 131 +++++++++++----- tests/iteration5_export_import.rs | 9 +- tests/iteration6_resume_history.rs | 25 +++ tests/parse_error_pedagogy.rs | 9 +- tests/replay_command.rs | 147 ++++++++++++++++-- tests/sql_select.rs | 10 +- tests/walking_skeleton.rs | 8 +- 18 files changed, 730 insertions(+), 76 deletions(-) diff --git a/docs/adr/0034-history-journal-and-replay-filter.md b/docs/adr/0034-history-journal-and-replay-filter.md index bebfb7f..744c97a 100644 --- a/docs/adr/0034-history-journal-and-replay-filter.md +++ b/docs/adr/0034-history-journal-and-replay-filter.md @@ -219,6 +219,93 @@ code lands as two tracked sub-tasks (test-first): the `err` ones; a plain `.commands` script still replays unchanged; a bare command containing `|` is not misparsed. +Both shipped 2026-05-24 (test-first), along with Amendment 1. + +**Deferred follow-up (user-confirmed, 2026-05-24).** `err` +journalling covers parse failures of *any* submitted line and +DSL/SQL *worker* execution failures (which surface as +`DslFailed`). An app-lifecycle command that *parses* and then +fails at the *runtime* stage (e.g. a `save as` / `import` that +fails on I/O) is **not** yet journalled `err` — those failures +surface as their own runtime events, not `DslFailed`. Recording +them is a tracked follow-up; the recall-typos motivation of §2 is +already met by parse-failure journalling. + +## Amendment 1 — Replay filters out app-lifecycle commands (2026-05-24) + +This amendment **extends Decision §2's "each consumer filters"** to a +second filter dimension on replay, and **supersedes the runtime's +prior nested-`replay` refusal**. It was written during implementation, +after the §3 change (replay learning the journal format) made a latent +problem reachable, and is recorded with explicit user approval. + +### The finding — a working `replay history.log` exposes app commands + +§3 makes `replay history.log` actually work. But the journal is a +*complete* record (§1), and the runtime journals successful +app-lifecycle commands too — `save as` / `load` / `new` (project +switches), `export` / `import`, `rebuild`, `mode`. Before §3 these were +unreachable (replay died on line 1); now replay reaches them, and: + +- the ones that parse to `Command::App` (`export`, `mode`, `rebuild`, + …) hit the worker dispatch's `unreachable!()` arm — a **panic**; +- the ones whose target comes from a modal (`save as ` / + `load ` / `new `) **fail to parse** on the command line + and **abort** the whole replay. + +Either way, a normal journal breaks replay. Executing these would also +be *wrong*: they are session/project orchestration, not schema/data +reconstruction — `load` / `new` would switch projects mid-replay. + +### The decision — replay re-applies only state mutations + +Replay re-applies **only the schema/data write commands** (create/drop/ +alter table, add/drop/change column, add/drop relationship, add/drop +index, add/drop constraint, insert/update/delete — DSL and SQL). It +**skips** every `Command::App(_)` and a nested `Command::Replay`. Reads +(`show` / `select` / `explain`) still run (harmless; they never appear +in a journal anyway). + +- **All skips continue** — replay never aborts on a skippable command. + This reverses the prior nested-`replay` *refusal*: a journal that + happens to contain a `replay` the user once ran must not force them + to hand-edit the log. Skipping a nested `replay` also removes the + infinite-loop footgun by construction (the nested file is never + re-entered). +- **Two skips warn; the rest are silent.** Skipping `import` or a + nested `replay` can leave the replayed state *incomplete* (the + imported data / the nested file's commands are not reconstructed), + so each emits a `[skip]` warning (`replay.skipped_import` / + `replay.skipped_replay`) surfaced in the replay summary. `save` / + `save as` / `load` / `new` / `export` / `mode` / `messages` / + `help` / `quit` / `rebuild` skip silently — omitting them changes + nothing about the reconstructed schema/data. + +### Detection + +A line is classified after the §3 status filter: parse it; a +`Command::App(_)` or `Command::Replay` is skipped (warned for `import` +/ `replay`); a write/read is dispatched. The modal forms (`save as` / +`load` / `new`) do not parse, so a parse failure whose entry word is +one of those is skipped rather than aborting — any *other* parse +failure is a genuine malformed command and still stops replay with a +line-numbered error (so a typo in a hand-built script fails loudly). + +### Consequences + +- `AppEvent::ReplayCompleted` carries `warnings: Vec`; the App + renders them after the `[ok] replay …` summary. +- The `replay.error_nested` catalog key is removed (nested replay is + no longer an error); `replay.skipped_import` / `replay.skipped_replay` + are added. +- A journal whose later write command depended on `import` / + `rebuild`-created state may not fully replay (that command can fail + on the missing dependency). This is accepted: replay reconstructs the + *command sequence*, and externally-sourced data is not a reproducible + command. The `import` warning flags exactly this risk. +- ADR-0006's "`history.log` can be replayed" is now true in practice + for the state-building subset. + ## See also - ADR-0004 — the project format `history.log` lives in. diff --git a/docs/adr/README.md b/docs/adr/README.md index 0e4b580..0fa228d 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -39,4 +39,4 @@ This directory contains the project's ADRs, recorded per - [ADR-0031 — The SQL expression grammar](0031-sql-expression-grammar.md) — **Accepted**, the stratified SQL expression grammar fragment commissioned by ADR-0030 §3: a single precedence ladder (`OR`/`AND`/`NOT`, the comparison/`LIKE`/`IN`/`BETWEEN`/`IS NULL` predicate set, arithmetic incl. `||`, function calls, `CASE`) — the superset of ADR-0026's DSL `WHERE` grammar, authored as a parallel fragment so simple mode is untouched; pure validation, builds **no** AST (consumers run/store SQL as text per ADR-0030 §4/§6); reuses ADR-0026's `Subgrammar` recursion + depth cap unchanged; subquery expressions and qualified column refs deferred to ADR-0030 Phase 2 - [ADR-0032 — The full SQL `SELECT` grammar](0032-sql-select-grammar.md) — **Accepted**, the Phase-2 grammar commissioned by ADR-0030 §3: full `SELECT` with `INNER`/`LEFT`/`RIGHT`/`FULL OUTER`/`CROSS` joins, `GROUP BY`/`HAVING`, all four set ops (`UNION`/`UNION ALL`/`INTERSECT`/`EXCEPT`), `WITH` and `WITH RECURSIVE` CTEs, `LIMIT … OFFSET`, `DISTINCT`, `t.*`, and bare-alias projection (lifting Phase-1 §4.2); additive extensions to ADR-0031's `sql_expr` for scalar subqueries, `IN (SELECT …)`, `[NOT] EXISTS`, and qualified column refs (redeeming ADR-0031 §7 OOS-1/OOS-2); grammar-recursion via `Subgrammar(&SQL_SELECT_COMPOUND)` reuses ADR-0026's `MAX_SUBGRAMMAR_DEPTH = 64` cap unchanged; **softens ADR-0030 §8's "ambient assistance comes for free" claim**: completion scope needs new `WalkContext` accumulators (a `from_scope_stack` of `ScopeFrame`s holding `from_scope` / `cte_bindings` / `projection_aliases`), a **new walker node variant `Node::ScopedSubgrammar(&Node)`** as the push/pop trigger (existing `Node::Subgrammar` unchanged so DSL `Expr` and `sql_expr` recursion are unaffected), qualified-prefix completion narrowing, body-projection-derived CTE column resolution (so `SELECT *` and explicit-projection CTE bodies both yield real column completion past `cte_alias.|`), and a **post-walk fixup pass** that re-resolves projection-list identifier highlighting/validity once `FROM` is parsed (the projection-before-FROM problem); classifies every Phase-2 validation case against ADR-0027's ERROR/WARNING guideline (§11): five new `diagnostic.*` keys for parse-time-detectable cases (unknown qualifier, ambiguous column, projection-alias misplaced, CTE/compound arity mismatch) plus eight `engine.*` translation keys; a MatchedPath-walking predicate-warnings variant that closes the Phase-1 gap where SQL `WHERE` expressions emitted no `LIKE`-on-numeric / `= NULL` / type-mismatch warnings (ADR-0027 Amendment 1 finally extends to the SQL surface); adds a worker-side post-prepare type-resolution pass via engine column-origin metadata so bare column refs recover their playground type (partially lifting Phase-1 §4.5, the bool→0/1 case) — `Cargo.toml` gains `column_metadata` to rusqlite features (verified against pinned 0.39.0); `__rdbms_*` rejection extended to every new table-source slot; Amendment 1 narrows §12's resolution rule from a grammar-side structural classification to "trust the engine's column-origin metadata verbatim" after an empirical probe showed origin metadata follows through non-recursive CTEs, scalar subqueries, derived tables, set ops, and joins — the one structural exception is recursive CTE result columns, which return None and stay typeless; Amendment 2 records that §10.6's "rewrite the highlight class" prescription is realised via the two-pass schema-existence diagnostic + the renderer's diagnostic-overlay path (no separate per-byte rewrite step needed; no new HighlightClass variant), and that the projection-before-FROM completion narrowing has been improved by an `src/completion.rs` look-ahead probe when the leading walk's `from_scope` is empty but the full input parses - [ADR-0033 — The full SQL DML grammar (`INSERT` / `UPDATE` / `DELETE`)](0033-sql-dml-grammar.md) — **Accepted** (implemented + verified through sub-phase 3k, 2026-05-23; phase-exit report `docs/handoff/20260523-phase-3-verification.md`), the Phase-3 grammar commissioned by ADR-0030 §3: single- and multi-row `INSERT` (incl. `INSERT … SELECT` recursing through ADR-0032's `SQL_SELECT_COMPOUND`), `UPDATE` with `SET` assignment list, `DELETE`, all three optionally followed by `RETURNING projection_list`, plus full `ON CONFLICT … DO NOTHING / DO UPDATE` UPSERT on INSERT; **fixes the DSL-vs-SQL dispatch architecture for shared entry words (`insert`/`update`/`delete`)**: SQL-first / DSL-fallback in Advanced mode via a `Choice(SQL_shape, DSL_shape)` per shape, gated by a new walker capability `Node::Guard(fn)` — a zero-byte-consumption gating node that fails the enclosing Seq with a `ValidationError`; carries `Command::SqlInsert` / `SqlUpdate` / `SqlDelete` variants and `do_sql_*` worker handlers each of which knows the target table (for re-persistence) and the `returning: bool` flag (for DataResult routing); `shortid` auto-fill mirrors the DSL `do_insert` mechanism via worker post-fill; SQL DELETE produces the same per-relationship cascade summary the DSL DELETE does (ADR-0014 parity); three new walker diagnostics (`insert_arity_mismatch` ERROR, `auto_column_overridden` WARNING, `not_null_missing` WARNING) with positive + negative tests each; OOS list explicitly carves out `DEFAULT VALUES` (the project's planned seed feature), SQLite-specific `OR REPLACE` / `OR IGNORE` / `OR ABORT` / `OR FAIL` / `OR ROLLBACK` prefixes, `UPDATE FROM` multi-table updates, and WITH-prefixed DML; the `excluded` keyword inside `ON CONFLICT DO UPDATE` is a deliberate carve-out from ADR-0030 §7's engine-neutral posture (no standard-SQL UPSERT spelling exists that SQLite and PostgreSQL share); eleven phased sub-phases each with explicit exit gates + written DA gate, opening with the dispatch mechanism before any DML grammar lands; initial DA review recorded seven critiques that were resolved before status moved to Proposed; **Amendment 1 supersedes §2's dispatch mechanism**: the originally-chosen `Node::Guard(fn)` + `Choice(SQL_shape, DSL_shape)` was found during 3a to be unworkable as framed (any guard-in-`Choice` mechanism forces a `walk_choice` change — `walk_choice` only falls through on `NoMatch`, so Simple-mode valid-DSL would wrongly surface "this is SQL", and `walk_seq` treats a `NoMatch` past `idx 0` as a hard `Failed`, breaking Advanced-mode DSL fall-through); replaced by **category-grouped, mode-aware dispatch** in `walker::walk` (each `REGISTRY` entry tagged `CommandCategory::{Simple, Advanced}`, generalising the existing whole-command `is_advanced_only` gate), shared entry words carrying a node in both groups, no `Node::Guard` and no `walk_choice`/`walk_seq` change, advanced-mode completion SQL-first with DSL as a full-line fallback; **Amendment 2 (sub-phase 3f) supersedes §7's cascade mechanism**: the WHERE-injected per-child pre-count rested on a premise that was factually wrong about the DSL handler (which detects cascades by before/after row-count diffing inside a transaction, not by `Expr`-derived pre-count subqueries) and would have broken the §2 parity promise by reporting `SET NULL` the DSL path doesn't; replaced by mirroring `do_delete`'s count-diff exactly (verbatim DELETE executes, child-count diff observes the cascade — `ON DELETE CASCADE` row removals only, SET NULL deferred for both paths to preserve parity), which shares the render-layer formatter for free via `CommandOutcome::Delete` and **withdraws risk R2** (no WHERE-byte extraction, no N+1 subquery); **Amendment 3 (sub-phase 3j) records the command-identity model and defers the execution-mode side-channel**: a command is the typed outcome of a *mode-rooted* grammar path and its identity is intrinsic (Advanced mode tries SQL first, falls back to the *Simple* DSL command when no SQL branch matches a token, e.g. `delete … --all-rows`; note `update … --all-rows` does *not* fall back — the SQL `SET` expression eats `--all-rows`, harmless since the engine treats it as a comment); **Simple mode commits the DSL candidate for shared words** so the *real* DSL error surfaces, and when that line would also run in advanced mode the rendering layer **combines** them — DSL error **plus** an `advanced_mode.also_valid_sql` pointer ("… (valid as SQL in advanced mode)") — keeping the actionable DSL fix while pointing at advanced mode; bare "this is SQL" is reserved for entry words with no DSL form (`select`/`with`); a fully-overlapping input (`insert … values …`) legitimately yields *two distinct commands* (`Command::Insert` typed-AST vs `Command::SqlInsert` validated-text) that do the same thing but execute differently (ADR-0030 §4), so each is tested in the mode that produces it; **corrects the plan's 3j exit-gate premise** that the DSL DML tests run in Simple mode (they call `parse_command`, which defaults to Advanced) — the real invariant is "Simple-mode behaviour unchanged, Advanced mode SQL-first, DSL grammar tested in Simple mode, both variants tested in their producing mode", with §6/§7 parity keeping the paths observably equivalent; and **defers to its own future ADR** the execution-time mode side-channel (three-way `Mode`: simple/advanced/advanced-one-shot threaded through `Action`→worker, for mode-dependent *output* like echoing generated SQL) — today only the *rendering* side-channel `OutputLine.mode_at_submission` exists, and the three-way distinction is not required for Phase 3 dispatch correctness -- [ADR-0034 — `history.log` as a complete command journal; replay reads success-only](0034-history-journal-and-replay-filter.md) — **Accepted**, resolves a three-way tension in `history.log`'s roles found while implementing ADR-0033 3f: (1) the persistent log is success-only while the in-memory Up/Down recall ring records *every* submission (success or failure, "so users can recall and edit typo'd commands"), and the ring is re-seeded from the log on project open — so **failed commands are recallable within a session but silently lost across sessions**; (2) replay wants the state-building (successful) commands while recall wants everything typed, which one success-only file cannot serve; (3) `replay history.log` never actually worked — `run_replay` parses each whole line through the DSL parser with no understanding of the `||` record shape, so a real log fails on line 1, and **no test ever fed the pipe format to replay** (the `replay_history_log_records_subcommands_only` test only checks what replay *writes*, never replays the log as input). Decision: `history.log` becomes a **complete journal** — every submission recorded, tagged `ok`/`err` via the status field the format already reserved (ADR-0015 §5) — and **each consumer filters**: hydration reads all records (cross-session recall matches in-session), replay reads `ok` only (and learns the journal format, while still accepting bare-command `.commands` scripts; detection by the leading timestamp+status prefix so a `|` inside a bare command isn't misread). Successful commands stay journalled transactionally by the worker; failed commands are journalled `err` best-effort from the runtime/app error path (a parse failure never reaches the worker). Amends ADR-0006's "successfully executed" wording and ADR-0015 §5 ("status always `ok`") / §12 (hydration). Code deferred to two tracked test-first sub-tasks (journal-failures+filtering; replay-parses-journal-format); existing all-`ok` logs need no migration +- [ADR-0034 — `history.log` as a complete command journal; replay reads success-only](0034-history-journal-and-replay-filter.md) — **Accepted**, resolves a three-way tension in `history.log`'s roles found while implementing ADR-0033 3f: (1) the persistent log is success-only while the in-memory Up/Down recall ring records *every* submission (success or failure, "so users can recall and edit typo'd commands"), and the ring is re-seeded from the log on project open — so **failed commands are recallable within a session but silently lost across sessions**; (2) replay wants the state-building (successful) commands while recall wants everything typed, which one success-only file cannot serve; (3) `replay history.log` never actually worked — `run_replay` parses each whole line through the DSL parser with no understanding of the `||` record shape, so a real log fails on line 1, and **no test ever fed the pipe format to replay** (the `replay_history_log_records_subcommands_only` test only checks what replay *writes*, never replays the log as input). Decision: `history.log` becomes a **complete journal** — every submission recorded, tagged `ok`/`err` via the status field the format already reserved (ADR-0015 §5) — and **each consumer filters**: hydration reads all records (cross-session recall matches in-session), replay reads `ok` only (and learns the journal format, while still accepting bare-command `.commands` scripts; detection by the leading timestamp+status prefix so a `|` inside a bare command isn't misread). Successful commands stay journalled transactionally by the worker; failed commands are journalled `err` best-effort from the runtime/app error path (a parse failure never reaches the worker). Amends ADR-0006's "successfully executed" wording and ADR-0015 §5 ("status always `ok`") / §12 (hydration). Code deferred to two tracked test-first sub-tasks (journal-failures+filtering; replay-parses-journal-format); existing all-`ok` logs need no migration; **implemented 2026-05-24** (plan `docs/plans/20260524-adr-0034-history-journal.md`); **Amendment 1 (2026-05-24): replay filters out app-lifecycle commands** — a working `replay history.log` (the §3 fix) exposed that the journal also records `save as`/`load`/`new`/`export`/`import`/`rebuild`/`mode` (which would panic the worker dispatch or abort the replay), so replay now re-applies **only** schema/data write commands and **skips** every `Command::App` + nested `Command::Replay`; **all skips continue** (never abort — reversing the prior nested-`replay` refusal, so a journal containing a once-run `replay` needs no hand-editing, and the infinite-loop footgun is closed by construction), with a `[skip]` **warning** on `import` and nested-`replay` skips (their omission can leave replayed state incomplete) and silent skips for the rest; `replay.error_nested` removed, `replay.skipped_import`/`replay.skipped_replay` added, `ReplayCompleted` carries `warnings` diff --git a/docs/plans/20260524-adr-0034-history-journal.md b/docs/plans/20260524-adr-0034-history-journal.md index 38f8e63..f6497b7 100644 --- a/docs/plans/20260524-adr-0034-history-journal.md +++ b/docs/plans/20260524-adr-0034-history-journal.md @@ -246,6 +246,22 @@ Push is the user's step. available at the err-append site (it is for recall today; verify for the journal). +## Implementation outcome (2026-05-24) + +Both sub-tasks landed test-first as planned. A **third concern surfaced +during implementation** and was resolved with the user, becoming +**ADR-0034 Amendment 1**: making `replay history.log` work (Sub-task 2) +exposed that the journal also records app-lifecycle commands +(`save as` / `load` / `new` / `export` / `import` / `rebuild` / `mode`), +which would panic the worker dispatch or abort replay. Replay now +**skips** every app-lifecycle command + nested `replay` (re-applying +only schema/data writes); all skips continue (the prior nested-`replay` +refusal is reversed), with a `[skip]` warning on `import` / nested +`replay`. Classification is by entry word so the modal / incomplete +forms skip uniformly rather than aborting. See the amendment in +`docs/adr/0034-…` and `tests/replay_command.rs`. `requirements.md` +U3 / U4 updated to match. + ## What this plan does NOT contain - Time estimates (milestones, not hours). diff --git a/docs/requirements.md b/docs/requirements.md index 5ab2161..1db575d 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -370,15 +370,26 @@ handoff-14 cleanup; 449 after B2/C2.) - [ ] **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. -- [x] **U3** `history.log` records every successfully executed - command in append-only form (Iteration 2). Format: - `|ok|` per ADR-0015 §5. +- [x] **U3** `history.log` records every submitted command in + append-only form, tagged with its outcome (Iteration 2; + broadened by ADR-0034). Format: `||` + per ADR-0015 §5 / ADR-0034 §1 — `status` is `ok` for a + successful command and `err` for one that failed to parse or + execute. Hydration (cross-session recall) reads all records; + replay reads `ok` only. - [x] **U4** `replay` runs commands from a `history.log` or `.commands` file. *(Implemented via ADR-0024 Phase E: `runtime::run_replay` parses each non-blank, non-`#`-comment - line with the schema-aware parser and dispatches it through - the normal pipeline; stops at the first error, no rollback; - nested replay refused. Covered by `tests/replay_command.rs`.)* + line in advanced mode and dispatches it through the normal + pipeline; stops at the first genuine error, no rollback. + ADR-0034 §3: replay reads journal records (`|| + `), running `ok` records and skipping non-`ok`, while + still accepting bare-command scripts. ADR-0034 Amendment 1: + replay re-applies only schema/data write commands and **skips** + every app-lifecycle command + nested `replay` — all skips + continue (a nested `replay` is now skipped, not refused), with a + `[skip]` warning on `import` / nested-`replay`. Covered by + `tests/replay_command.rs`.)* ## Sharing and export (per ADR-0007) diff --git a/src/action.rs b/src/action.rs index 6004000..5772880 100644 --- a/src/action.rs +++ b/src/action.rs @@ -24,6 +24,17 @@ pub enum Action { command: Command, source: String, }, + /// Record a *failed* submission to `history.log` as an `err` + /// record (ADR-0034 §1/§2). Emitted by the pure-sync `App` + /// for both failure kinds — a line that failed to parse (at + /// submit) and a command the worker rejected (on + /// `AppEvent::DslFailed`) — because the App does no I/O. The + /// runtime appends best-effort: a failure to record a failure + /// must never escalate a user error into a fatal (ADR-0034 + /// §4). `source` is the original user-typed text. + JournalFailure { + source: String, + }, /// User issued the `rebuild` app-level command (ADR-0015 /// §7, §11). Runtime computes a summary from /// `project.yaml` + `data/` and posts back as diff --git a/src/app.rs b/src/app.rs index 8829ff0..d66a884 100644 --- a/src/app.rs +++ b/src/app.rs @@ -455,9 +455,14 @@ impl App { command, error, facts, + source, } => { self.handle_dsl_failure(&command, error, facts); - Vec::new() + // 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 }] } AppEvent::TablesRefreshed(tables) => { trace!(count = tables.len(), "tables refreshed"); @@ -556,12 +561,22 @@ impl App { self.note_error(crate::t!("project.export_failed", error = error)); Vec::new() } - AppEvent::ReplayCompleted { path, count } => { + AppEvent::ReplayCompleted { + path, + count, + warnings, + } => { self.note_system(crate::t!( "replay.completed", path = path, count = count )); + // ADR-0034: surface `[skip]` warnings for app-lifecycle + // commands whose omission can leave the replayed state + // incomplete (`import`, nested `replay`). + for warning in warnings { + self.note_system(warning); + } Vec::new() } AppEvent::ReplayFailed { @@ -1207,7 +1222,14 @@ impl App { if let ParseError::Invalid { .. } = &err { self.note_error(render_usage_block(input)); } - Vec::new() + // ADR-0034 §1/§2: a submitted line that failed to + // parse is journalled `err` so it is recallable + // across sessions (the same `source` an `ok` + // command would record). The runtime does the + // append; the App only emits the intent. + vec![Action::JournalFailure { + source: input.to_string(), + }] } } } @@ -2308,7 +2330,12 @@ mod tests { let mut app = App::new(); type_str(&mut app, "create table Customers"); let actions = submit(&mut app); - assert!(actions.is_empty()); + // A definite parse error journals `err` (ADR-0034) and does + // not dispatch a command to the worker. + assert!( + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "expected only a JournalFailure, no dispatch; got {actions:?}", + ); // Parse-error rendering is now multi-line (ADR-0021): // caret + "parse error: …" + "usage: …" — the test // checks that some error line mentions `with pk`. @@ -2328,7 +2355,10 @@ mod tests { let mut app = App::new(); type_str(&mut app, "frobulate widgets"); let actions = submit(&mut app); - assert!(actions.is_empty()); + assert!( + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "a definite parse error journals err without dispatching; got {actions:?}", + ); let has_parse_error = app .output .iter() @@ -2351,7 +2381,10 @@ mod tests { let mut app = App::new(); type_str(&mut app, "insert into T values (1, 2), (3, 4)"); let actions = submit(&mut app); - assert!(actions.is_empty(), "the bad line must not dispatch"); + assert!( + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "the bad line journals err but must not dispatch; got {actions:?}", + ); let has_pointer = app .output .iter() @@ -2663,6 +2696,7 @@ mod tests { app.update(AppEvent::ReplayCompleted { path: "seed.commands".to_string(), count: 4, + warnings: Vec::new(), }); let last = app.output.back().unwrap(); assert_eq!(last.kind, OutputKind::System); @@ -2671,6 +2705,30 @@ mod tests { assert!(last.text.contains("seed.commands"), "{}", last.text); } + #[test] + fn replay_completed_event_renders_skip_warnings() { + // ADR-0034 Amendment 1: `[skip]` warnings (import / nested + // replay) surface in the output after the summary line. + let mut app = App::new(); + app.update(AppEvent::ReplayCompleted { + path: "history.log".to_string(), + count: 2, + warnings: vec![ + "[skip] line 3: `import a.zip` — replay does not re-import".to_string(), + "[skip] line 7: nested `replay x` — its commands were not replayed".to_string(), + ], + }); + let text: String = app + .output + .iter() + .map(|l| l.text.as_str()) + .collect::>() + .join("\n"); + assert!(text.contains("[ok] replay"), "summary present:\n{text}"); + assert!(text.contains("import a.zip"), "import skip warning rendered:\n{text}"); + assert!(text.contains("nested `replay x`"), "nested-replay skip warning rendered:\n{text}"); + } + #[test] fn replay_failed_event_renders_line_number_and_command_echo() { let mut app = App::new(); @@ -2731,6 +2789,7 @@ mod tests { kind: crate::db::SqliteErrorKind::NoSuchTable, }, facts: crate::friendly::FailureContext::default(), + source: String::new(), }); let last = app.output.back().unwrap(); assert_eq!(last.kind, OutputKind::Error); @@ -2788,6 +2847,7 @@ mod tests { command: cmd, error: err, facts, + source: String::new(), }); let body = app .output @@ -2836,6 +2896,7 @@ mod tests { command: cmd, error: err, facts, + source: String::new(), }); let body = app .output @@ -2867,6 +2928,7 @@ mod tests { command: cmd.clone(), error: err(), facts: crate::friendly::FailureContext::default(), + source: String::new(), }); let verbose_text = app .output @@ -2886,6 +2948,7 @@ mod tests { command: cmd, error: err(), facts: crate::friendly::FailureContext::default(), + source: String::new(), }); let short_text = app .output @@ -2920,7 +2983,10 @@ mod tests { let mut app = App::new(); type_str(&mut app, "add column to table T: c (varchar)"); let actions = submit(&mut app); - assert!(actions.is_empty()); + assert!( + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "expected only a JournalFailure, no dispatch; got {actions:?}", + ); let mentions_varchar = app .output .iter() @@ -3193,6 +3259,50 @@ mod tests { ); } + #[test] + fn submitting_an_unparseable_line_emits_journal_failure() { + // ADR-0034 §1/§2: a submitted line that fails to parse is + // journalled `err` (recallable across sessions). The + // pure-sync App emits the intent; the runtime does the I/O. + let mut app = App::new(); + type_str(&mut app, "florp glorp"); + let actions = submit(&mut app); + assert!( + matches!( + actions.as_slice(), + [Action::JournalFailure { source }] if source == "florp glorp" + ), + "expected JournalFailure for the typo'd line; got {actions:?}", + ); + } + + #[test] + fn dsl_failure_event_emits_journal_failure_carrying_the_source() { + // ADR-0034 §1/§2: an execution failure (the worker rejected + // a parsed command) is journalled `err` too. The runtime + // forwards the source on `DslFailed`; the App turns it into + // a `JournalFailure` action. + let mut app = App::new(); + let actions = app.update(AppEvent::DslFailed { + command: Command::DropTable { + name: "Ghost".to_string(), + }, + error: crate::db::DbError::Sqlite { + message: "no such table: Ghost".to_string(), + kind: crate::db::SqliteErrorKind::NoSuchTable, + }, + facts: crate::friendly::FailureContext::default(), + source: "drop table Ghost".to_string(), + }); + assert!( + matches!( + actions.as_slice(), + [Action::JournalFailure { source }] if source == "drop table Ghost" + ), + "expected JournalFailure carrying the source; got {actions:?}", + ); + } + #[test] fn history_skips_consecutive_duplicates() { let mut app = App::new(); diff --git a/src/event.rs b/src/event.rs index ae6ac28..79481f3 100644 --- a/src/event.rs +++ b/src/event.rs @@ -76,6 +76,12 @@ pub enum AppEvent { command: Command, error: DbError, facts: crate::friendly::FailureContext, + /// The original user-typed source line, retained so the + /// App can journal the failed command as an `err` record + /// (ADR-0034 §1/§2). The worker only journals successful + /// commands, so an execution failure would otherwise be + /// lost across sessions. + source: String, }, /// Refreshed list of tables in the database. TablesRefreshed(Vec), @@ -146,6 +152,11 @@ pub enum AppEvent { ReplayCompleted { path: String, count: usize, + /// Pre-rendered `[skip]` warnings for app-lifecycle commands + /// whose omission can leave the replayed state incomplete — + /// `import` and a nested `replay` (ADR-0034). Other skipped + /// app commands are silent and do not appear here. + warnings: Vec, }, /// A `replay ` aborted at line `line_number`. `command` /// is the line text as it appeared in the file (for the diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index cd892a8..2ba2863 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -468,10 +468,11 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("replay.command_echo", &["command"]), ("replay.completed", &["path", "count"]), ("replay.error_could_not_open", &["path", "detail"]), - ("replay.error_nested", &[]), ("replay.error_parse", &["detail"]), ("replay.failed_at_line", &["path", "line_number", "error"]), ("replay.failed_open", &["path", "error"]), + ("replay.skipped_import", &["line", "command"]), + ("replay.skipped_replay", &["line", "command"]), // ---- UNIQUE violations (anchor: "already has the value") ---- ( "error.unique.insert.headline", diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index 55256f5..ff8d166 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -842,4 +842,10 @@ replay: # compose with `failed_at_line`'s `{error}` placeholder. error_could_not_open: "could not open `{path}`: {detail}" error_parse: "parse error: {detail}" - error_nested: "nested `replay` is not allowed inside a replay file" + # Skipped during replay (ADR-0034): app-lifecycle commands are + # not re-applied. Most skip silently; `import` and a nested + # `replay` warn because skipping them can leave the replayed + # state incomplete (imported data / the nested file's commands + # are not reconstructed). + skipped_import: "[skip] line {line}: `{command}` — replay does not re-import; the imported data is not reconstructed" + skipped_replay: "[skip] line {line}: nested `{command}` — its commands were not replayed" diff --git a/src/persistence/history.rs b/src/persistence/history.rs index b91ea74..cd0bf13 100644 --- a/src/persistence/history.rs +++ b/src/persistence/history.rs @@ -22,10 +22,26 @@ use std::time::{SystemTime, UNIX_EPOCH}; use super::PersistenceError; -/// Format a single log record. Pure; no I/O. +/// Journal-record status tokens (ADR-0034 §1). Kept as named +/// constants so the writer and the readers (hydration + replay) +/// cannot drift on the spelling. +pub(super) const STATUS_OK: &str = "ok"; +pub(super) const STATUS_ERR: &str = "err"; + +/// Format a successful-command record. Pure; no I/O. pub(super) fn format_record(command_text: &str, timestamp_iso: String) -> String { + format_record_with_status(command_text, timestamp_iso, STATUS_OK) +} + +/// Format a record with an explicit status token (ADR-0034 §1). +/// Pure; no I/O. +pub(super) fn format_record_with_status( + command_text: &str, + timestamp_iso: String, + status: &str, +) -> String { let escaped = escape_command(command_text); - format!("{timestamp_iso}|ok|{escaped}\n") + format!("{timestamp_iso}|{status}|{escaped}\n") } /// Read the most-recent `max_n` user-issued command sources @@ -89,6 +105,54 @@ fn parse_record_source(line: &str) -> Option { Some(unescape_command(source)) } +/// A parsed journal record (ADR-0034 §3). `source` is already +/// unescaped. +pub(super) struct JournalRecord { + pub status_is_ok: bool, + pub source: String, +} + +/// Classify `line` as a journal record or a bare command +/// (ADR-0034 §3). Returns `Some(JournalRecord)` only when the +/// line begins with a valid `||` +/// prefix — so a bare command containing `|` (e.g. +/// `select 'a|b' from t`) is `None` (treated as bare by the +/// caller) because it does not start with a timestamp. A valid +/// timestamp prefix with a non-`ok` (or unrecognised) status is +/// still a journal record, reported with `status_is_ok = false` +/// so replay skips it rather than mis-running it as a command. +pub(super) fn parse_journal_record(line: &str) -> Option { + let mut parts = line.splitn(3, '|'); + let ts = parts.next()?; + let status = parts.next()?; + let source = parts.next()?; + if !looks_like_iso8601(ts) { + return None; + } + Some(JournalRecord { + status_is_ok: status == STATUS_OK, + source: unescape_command(source), + }) +} + +/// True when `s` is exactly an `YYYY-MM-DDTHH:MM:SSZ` timestamp +/// — the shape `utc_iso8601_now` emits. Used to distinguish a +/// journal record's leading field from a bare command that +/// merely contains `|`. +fn looks_like_iso8601(s: &str) -> bool { + let b = s.as_bytes(); + if b.len() != 20 { + return false; + } + let digit = |i: usize| b[i].is_ascii_digit(); + digit(0) && digit(1) && digit(2) && digit(3) && b[4] == b'-' + && digit(5) && digit(6) && b[7] == b'-' + && digit(8) && digit(9) && b[10] == b'T' + && digit(11) && digit(12) && b[13] == b':' + && digit(14) && digit(15) && b[16] == b':' + && digit(17) && digit(18) && b[19] == b'Z' +} + fn unescape_command(s: &str) -> String { let mut out = String::with_capacity(s.len()); let mut chars = s.chars(); @@ -211,6 +275,76 @@ mod tests { assert_eq!(line, "T|ok|foo\\nbar\n"); } + // ---- ADR-0034 §3 — journal-record detection for replay ---- + + #[test] + fn parse_journal_record_ok_extracts_unescaped_source() { + let rec = parse_journal_record( + "2026-05-24T10:00:00Z|ok|create table T with pk id(int)", + ) + .expect("valid ok journal record"); + assert!(rec.status_is_ok); + assert_eq!(rec.source, "create table T with pk id(int)"); + } + + #[test] + fn parse_journal_record_err_is_record_but_not_ok() { + // A valid timestamp + `err` status is a journal record (so + // replay treats it as skippable), reported `status_is_ok = + // false`. + let rec = parse_journal_record("2026-05-24T10:00:02Z|err|insert into T values (1)") + .expect("valid err journal record"); + assert!(!rec.status_is_ok); + } + + #[test] + fn parse_journal_record_unknown_status_is_record_but_not_ok() { + // A valid timestamp + an unrecognised status is still a + // journal record (ADR-0034 §1 "readers ignore values they + // do not recognise"); replay skips it rather than running it. + let rec = parse_journal_record("2026-05-24T10:00:02Z|frobnicate|whatever") + .expect("valid-ts record with unknown status"); + assert!(!rec.status_is_ok); + } + + #[test] + fn parse_journal_record_rejects_bare_command_with_pipe() { + // A bare command that merely contains `|` must NOT be read + // as a journal record — its first field is not a timestamp. + assert!(parse_journal_record("select 'a|b' from t").is_none()); + assert!(parse_journal_record("show data Orders").is_none()); + } + + #[test] + fn parse_journal_record_rejects_timestamp_ish_but_invalid_prefix() { + // Boundary: looks vaguely date-y but isn't the exact + // `YYYY-MM-DDTHH:MM:SSZ` shape → bare command. + assert!(parse_journal_record("2026-5-24|ok|x").is_none()); + assert!(parse_journal_record("2026-05-24 10:00:00|ok|x").is_none()); + assert!(parse_journal_record("notatimestamp|ok|x").is_none()); + } + + #[test] + fn parse_journal_record_preserves_pipe_in_source() { + // `|` is not escaped by the writer (it's a valid SQL char); + // `splitn(3, '|')` keeps everything after the second `|`. + let rec = parse_journal_record("2026-05-24T10:00:00Z|ok|select 'a|b' from t") + .expect("ok record"); + assert_eq!(rec.source, "select 'a|b' from t"); + } + + #[test] + fn parse_journal_record_round_trips_a_written_record() { + // What `format_record` writes, `parse_journal_record` reads + // back to the original command (escape→unescape lossless for + // the awkward cases). + let cmd = "update T set v = 'x\\y' where id = 1"; + let line = format_record(cmd, "2026-05-24T10:00:00Z".to_string()); + let rec = parse_journal_record(line.trim_end()).expect("round-trip record"); + assert!(rec.status_is_ok); + assert_eq!(rec.source, cmd); + } + #[test] fn backslash_is_escaped() { let line = format_record("a\\b", "T".to_string()); diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index fda6bbe..f0035b3 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -272,13 +272,30 @@ impl Persistence { } } - /// Append one record to `history.log`. + /// Append one successful-command record to `history.log`. pub fn append_history(&self, command_text: &str) -> Result<(), PersistenceError> { let path = self.project_path.join(HISTORY_LOG); let line = history::format_record(command_text, history::utc_iso8601_now()); history::append(&path, &line) } + /// Append a failed-command record to `history.log`, tagged + /// `err` (ADR-0034 §1). Used by the runtime's error path so a + /// command that failed to parse or to execute is still + /// recallable across sessions (it never reaches the worker's + /// 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> { + let path = self.project_path.join(HISTORY_LOG); + let line = history::format_record_with_status( + command_text, + history::utc_iso8601_now(), + history::STATUS_ERR, + ); + history::append(&path, &line) + } + /// Read the most-recent `max_n` sources out of /// `history.log` for input-history hydration on project /// open (ADR-0015 §12). Returned in chronological order @@ -289,6 +306,31 @@ impl Persistence { } } +/// How `run_replay` should treat one already-trimmed, +/// non-blank, non-`#` line (ADR-0034 §3). +pub(crate) enum ReplayLine { + /// Run this command text — either a journal `ok` record's + /// extracted source, or a bare command verbatim. + Run(String), + /// A journal record whose status is not `ok` — skip it + /// silently (a skipped failure is not a replay failure). + Skip, +} + +/// Classify one replay input line (ADR-0034 §3). A journal +/// record (`||`) runs its source only when +/// `ok` and is skipped otherwise; any other line is a bare +/// command run verbatim. Detection is by the leading +/// timestamp+status prefix, so a bare command that merely +/// contains `|` (e.g. `select 'a|b' from t`) is run as-is. +pub(crate) fn classify_replay_line(line: &str) -> ReplayLine { + match history::parse_journal_record(line) { + Some(rec) if rec.status_is_ok => ReplayLine::Run(rec.source), + Some(_) => ReplayLine::Skip, + None => ReplayLine::Run(line.to_string()), + } +} + /// Write `body` to `path` atomically via temp file + fsync + /// rename. The temp file is named `.tmp` in the same /// directory so the rename stays on the same filesystem. diff --git a/src/runtime.rs b/src/runtime.rs index 53c22b9..122b836 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -384,6 +384,19 @@ async fn run_loop( source, ); } + Action::JournalFailure { source } => { + // 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. + if let Err(e) = crate::persistence::Persistence::new( + session.project().path().to_path_buf(), + ) + .append_history_failure(&source) + { + tracing::warn!(error = %e, "failed to journal err record (ignored)"); + } + } Action::PrepareRebuild => { spawn_prepare_rebuild( session.project().path().to_path_buf(), @@ -1143,6 +1156,9 @@ fn spawn_dsl_dispatch( source: String, ) { tokio::spawn(async move { + // Retain the source for `DslFailed` so the App can journal a + // rejected command as `err` (ADR-0034 §1/§2). + let source_for_journal = source.clone(); let outcome = execute_command_typed(&database, command.clone(), source).await; let event = match outcome { Ok(CommandOutcome::Schema(description)) => AppEvent::DslSucceeded { @@ -1199,6 +1215,7 @@ fn spawn_dsl_dispatch( command: command.clone(), error, facts, + source: source_for_journal, } } }; @@ -1627,65 +1644,88 @@ pub async fn run_replay( }; let mut count: usize = 0; + let mut warnings: Vec = Vec::new(); for (idx, raw) in body.lines().enumerate() { let line_number = idx + 1; let trimmed = raw.trim(); if trimmed.is_empty() || trimmed.starts_with('#') { continue; } - // Parse the line through the same DSL parser the - // interactive path uses. The schema is re-snapshotted - // every line because earlier replayed commands - // (`create table`, `add column`, …) mutate it — so - // Phase D typed-slot rejections (wrong-count value - // lists, wrong-type column values) fire at replay - // parse time, matching the interactive path, rather - // than only at bind time. A failure here is structural - // (bad syntax / typed-slot reject) — report and stop - // without dispatching. + // ADR-0034 §3: a journal record (`||`) + // contributes its extracted source when `ok` and is skipped + // otherwise; any other line is a bare command run verbatim. + // This is what makes `replay history.log` work without + // breaking hand-written `.commands` scripts. + let command_text = match crate::persistence::classify_replay_line(trimmed) { + crate::persistence::ReplayLine::Skip => continue, + crate::persistence::ReplayLine::Run(text) => text, + }; + // ADR-0034 Amendment 1: classify by entry word BEFORE parsing. + // App-lifecycle commands and a nested `replay` are skipped + // during replay — they are session navigation, not schema/data + // reconstruction (and the worker dispatch cannot run them). + // Classifying by the leading word handles the modal forms + // (`save as` / `load` / `new`) and any incomplete app form + // uniformly: ALL such skips continue, never aborting the + // replay. `import` and a nested `replay` warn, because skipping + // them can leave the replayed state incomplete (imported data / + // the nested file's commands are not reconstructed); the rest + // skip silently. Skipping a nested `replay` also closes the + // infinite-loop footgun by construction. + let entry = command_text + .split_whitespace() + .next() + .unwrap_or("") + .to_ascii_lowercase(); + if entry == "import" { + warnings.push(crate::t!( + "replay.skipped_import", + line = line_number, + command = command_text + )); + continue; + } + if entry == "replay" { + warnings.push(crate::t!( + "replay.skipped_replay", + line = line_number, + command = command_text + )); + continue; + } + if is_app_lifecycle_entry_word(&entry) { + continue; + } + // A schema/data write (or read, or a genuine typo). Parse with + // the live schema — re-snapshotted every line because earlier + // replayed commands mutate it (so Phase D typed-slot rejections + // fire at parse time, matching the interactive path). A parse + // failure here is a genuine malformed command (not an app + // command, which was skipped above) — report it with the line + // number and stop. let schema = build_schema_cache(database).await; - // Replay parses each line like an interactive submission and - // executes the resulting command — in advanced mode (the full - // surface). A bad value in a shared-entry-word DML line is - // caught either at parse time (a DSL form's typed slot) or at - // execute time (the engine's column-type enforcement); either - // way the offending line fails and replay stops without - // applying it. let command = match crate::dsl::parser::parse_command_with_schema( - trimmed, &schema, + &command_text, &schema, ) { Ok(c) => c, Err(e) => { events.push(AppEvent::ReplayFailed { path: path.to_string(), line_number, - command: trimmed.to_string(), + command: command_text.clone(), error: crate::t!("replay.error_parse", detail = e), }); return events; } }; - // Nested replay is intentionally refused. Allowing it - // would invite easy infinite-loop footguns (a script - // replaying itself, two scripts replaying each other); - // the DSL is small enough that we'd rather close that - // door than design fences around it. A real - // composition story lands later if the need is proven. - if matches!(command, Command::Replay { .. }) { - events.push(AppEvent::ReplayFailed { - path: path.to_string(), - line_number, - command: trimmed.to_string(), - error: crate::t!("replay.error_nested"), - }); - return events; - } // Dispatch through the same path as interactive input so // per-command persistence (history.log, project.yaml, - // CSVs) fires as if the user had typed each line. + // CSVs) fires as if the user had typed each line. The + // source re-journalled is the *extracted* command, not the + // raw `|ok|…` record (ADR-0034 §3). let outcome = - execute_command_typed(database, command, trimmed.to_string()).await; + execute_command_typed(database, command, command_text.clone()).await; match outcome { Ok(_) => { count += 1; @@ -1709,7 +1749,7 @@ pub async fn run_replay( events.push(AppEvent::ReplayFailed { path: path.to_string(), line_number, - command: trimmed.to_string(), + command: command_text.clone(), error: e.friendly_message(), }); return events; @@ -1720,10 +1760,27 @@ pub async fn run_replay( events.push(AppEvent::ReplayCompleted { path: path.to_string(), count, + warnings, }); events } +/// True when `entry` (a lowercased leading command word) is an +/// app-lifecycle command that replay skips (ADR-0034 Amendment 1). +/// `import` and `replay` are handled separately by the caller +/// (they warn); this is the silent-skip set. Mirrors the +/// `AppCommand` entry words (see `src/completion.rs`'s +/// `empty_input_offers_app_command_entry_keywords`); both must list +/// the same words. A new `AppCommand` must be added here so replay +/// skips it rather than aborting. `q` is intentionally absent: it is +/// not a recognised command, so a `q` line is a genuine error. +fn is_app_lifecycle_entry_word(entry: &str) -> bool { + matches!( + entry, + "save" | "load" | "new" | "export" | "mode" | "messages" | "rebuild" | "help" | "quit" + ) +} + /// Resolve a `replay ` argument: absolute paths pass /// through unchanged; relative paths are joined under the active /// project's root so `replay history.log` works without ceremony diff --git a/tests/iteration5_export_import.rs b/tests/iteration5_export_import.rs index 43676f6..09a79fb 100644 --- a/tests/iteration5_export_import.rs +++ b/tests/iteration5_export_import.rs @@ -182,8 +182,13 @@ fn import_with_empty_target_after_as_errors() { let actions = submit(&mut app); // "as " trailing whitespace is trimmed by .split_once + .trim, // making the as-target empty. We surface this as a usage - // error rather than silently importing without a target. - assert!(actions.is_empty()); + // error rather than silently importing without a target. The + // failed line is journalled `err` (ADR-0034) but no import + // dispatches. + assert!( + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "expected only a JournalFailure, no import dispatch; got {actions:?}", + ); // The friendly parse-error rendering produces multiple // output lines (caret, message, usage). Scan for the anchor // phrase rather than asserting on the final line. The diff --git a/tests/iteration6_resume_history.rs b/tests/iteration6_resume_history.rs index 89a8030..148e479 100644 --- a/tests/iteration6_resume_history.rs +++ b/tests/iteration6_resume_history.rs @@ -155,6 +155,31 @@ fn read_recent_history_returns_appended_entries_in_order() { ); } +#[test] +fn hydration_reads_both_ok_and_err_records() { + // ADR-0034 §1/§2: failed commands are journalled `err`, and + // input-history hydration reads ALL records (ok + err) so a + // typo'd / rejected command from a previous session is + // recallable after restart — matching the in-session ring's + // "record everything" behaviour. + 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(); + let entries = p.read_recent_history(10).unwrap(); + assert_eq!( + entries, + vec![ + "create table A with pk".to_string(), + "insert into A (1, 2, 3)".to_string(), // the err record is recalled + "show data A".to_string(), + ], + "hydration includes the err record", + ); +} + #[test] fn seed_history_replaces_in_memory_history() { let mut app = App::new(); diff --git a/tests/parse_error_pedagogy.rs b/tests/parse_error_pedagogy.rs index ad20496..d93f01e 100644 --- a/tests/parse_error_pedagogy.rs +++ b/tests/parse_error_pedagogy.rs @@ -34,15 +34,16 @@ fn submit(app: &mut App) -> Vec { } /// Run `input` through the app and return every error-kind -/// output line. Asserts the submission produced no actions -/// (i.e. the parse failed). +/// output line. Asserts the submission parse-failed — which now +/// emits exactly a `JournalFailure` (ADR-0034: the failed line is +/// journalled `err`) and dispatches no command to the worker. fn error_lines_for(input: &str) -> Vec { let mut app = App::new(); type_str(&mut app, input); let actions = submit(&mut app); assert!( - actions.is_empty(), - "expected parse failure (no actions) for {input:?}, got {actions:?}", + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "expected parse failure (only a JournalFailure) for {input:?}, got {actions:?}", ); app.output .iter() diff --git a/tests/replay_command.rs b/tests/replay_command.rs index f495c47..bb9ea1e 100644 --- a/tests/replay_command.rs +++ b/tests/replay_command.rs @@ -113,6 +113,123 @@ fn replay_three_lines_dispatches_three_commands() { assert_eq!(data_result.rows[0][1].as_deref(), Some("Alice")); } +#[test] +fn replay_of_actual_history_log_runs_ok_commands_and_skips_err() { + // ADR-0034 §3 + Problem 3 (handoff-34 §4): `replay history.log` + // must work. The journal is the pipe format + // `||`; replay extracts ``, runs + // `ok` records, and skips `err` ones (like blank / `#` lines — a + // skipped failure is not a replay failure). + // + // This is the ADR-0034 headline reproduction. It is RED before the + // fix: today `run_replay` feeds the whole `2026-…|ok|…` line to the + // parser, which dies on line 1 (the timestamp is not a command). + let data = tempdir(); + let (project, db) = open_project_db(data.path()); + write_script( + project.path(), + "history.log", + "2026-05-24T10:00:00Z|ok|create table T with pk id(int)\n\ + 2026-05-24T10:00:01Z|ok|add column T: v (text)\n\ + 2026-05-24T10:00:02Z|err|insert into T values (1, 2, 3, 4)\n\ + 2026-05-24T10:00:03Z|ok|insert into T (id, v) values (1, 'alpha')\n", + ); + + let events = + rt().block_on(async { run_replay(&db, project.path(), "history.log").await }); + // Three `ok` records replayed; the `err` record is skipped (not + // counted, not a failure). + assert_completed(&events, 3); + + let data_result = rt() + .block_on(async { db.query_data("T".to_string(), None, None, None).await }) + .expect("query_data"); + assert_eq!(data_result.rows.len(), 1, "only the ok INSERT applied"); + assert_eq!(data_result.rows[0][1].as_deref(), Some("alpha")); +} + +#[test] +fn replay_skips_app_lifecycle_commands_silently() { + // ADR-0034: a real `history.log` contains app-lifecycle commands + // (`save as` / `load` / `new` / `export` / `mode` / `rebuild` …). + // Replay skips them — they are session navigation, not schema/data + // reconstruction, and the worker dispatch cannot run them (it would + // panic on a parsed app command, or abort on the modal forms that + // don't parse). These skip SILENTLY (no warning). + let data = tempdir(); + let (project, db) = open_project_db(data.path()); + // Every silent-skip app-lifecycle form, including the modal forms + // that don't parse on the command line (`save as` / `load` / `new`), + // the bare incomplete form (`mode`), and the safety-critical `quit` + // (a journalled quit must NOT quit during replay). None may abort; + // none warns. + write_script( + project.path(), + "history.log", + "2026-05-24T10:00:00Z|ok|create table T with pk id(int)\n\ + 2026-05-24T10:00:01Z|ok|save as backup\n\ + 2026-05-24T10:00:02Z|ok|load other\n\ + 2026-05-24T10:00:03Z|ok|new scratch\n\ + 2026-05-24T10:00:04Z|ok|mode advanced\n\ + 2026-05-24T10:00:05Z|ok|mode\n\ + 2026-05-24T10:00:06Z|ok|messages verbose\n\ + 2026-05-24T10:00:07Z|ok|export out.zip\n\ + 2026-05-24T10:00:08Z|ok|rebuild\n\ + 2026-05-24T10:00:09Z|ok|help\n\ + 2026-05-24T10:00:10Z|ok|quit\n\ + 2026-05-24T10:00:11Z|ok|add column T: v (text)\n\ + 2026-05-24T10:00:12Z|ok|insert into T (id, v) values (1, 'alpha')\n", + ); + let events = + rt().block_on(async { run_replay(&db, project.path(), "history.log").await }); + // Three data/schema commands ran; every app-lifecycle line was + // skipped silently (no panic, no abort, no warnings, no quit). + match events.last().expect("event") { + AppEvent::ReplayCompleted { count, warnings, .. } => { + assert_eq!(*count, 3, "only the 3 write commands ran; events: {events:?}"); + assert!(warnings.is_empty(), "these skips are silent; got {warnings:?}"); + } + other => panic!("expected ReplayCompleted, got {other:?}"), + } + let data_result = rt() + .block_on(async { db.query_data("T".to_string(), None, None, None).await }) + .expect("query_data"); + assert!( + data_result.columns.iter().any(|c| c == "v"), + "the add-column line applied; columns: {:?}", + data_result.columns, + ); + assert_eq!(data_result.rows.len(), 1, "the insert applied"); + assert_eq!(data_result.rows[0][1].as_deref(), Some("alpha")); +} + +#[test] +fn replay_skips_import_with_a_warning() { + // ADR-0034: `import` is skipped like other app commands, but warns + // — skipping it can leave the replayed state incomplete (the + // imported data is not reconstructed). + let data = tempdir(); + let (project, db) = open_project_db(data.path()); + write_script( + project.path(), + "history.log", + "2026-05-24T10:00:00Z|ok|create table T with pk id(int)\n\ + 2026-05-24T10:00:01Z|ok|import shared.zip as Imported\n", + ); + let events = + rt().block_on(async { run_replay(&db, project.path(), "history.log").await }); + match events.last().expect("event") { + AppEvent::ReplayCompleted { count, warnings, .. } => { + assert_eq!(*count, 1, "only the create ran; events: {events:?}"); + assert!( + warnings.iter().any(|w| w.contains("import shared.zip")), + "expected an import skip warning; got {warnings:?}", + ); + } + other => panic!("expected ReplayCompleted, got {other:?}"), + } +} + #[test] fn replay_skips_blank_lines_and_comments() { let data = tempdir(); @@ -307,7 +424,12 @@ fn replay_aborts_on_first_runtime_failure_and_reports_line() { } #[test] -fn replay_refuses_nested_replay() { +fn replay_skips_nested_replay_with_a_warning() { + // ADR-0034: a nested `replay` is no longer refused (which would + // force a user to hand-edit a journal that happens to contain a + // `replay` they once ran). It is skipped — sidestepping the + // infinite-loop footgun by construction — and warned about, + // because the nested file's commands are not reconstructed. let data = tempdir(); let (project, db) = open_project_db(data.path()); write_script(project.path(), "inner.commands", "create table T with pk id(int)\n"); @@ -320,14 +442,21 @@ fn replay_refuses_nested_replay() { let events = rt().block_on(async { run_replay(&db, project.path(), "outer.commands").await }); - let failed = assert_failed_at(&events, 2); - let AppEvent::ReplayFailed { error, .. } = failed else { - unreachable!() - }; - assert!( - error.contains("nested `replay`"), - "expected nested-replay refusal: {error}" - ); + // The outer `create table U` ran; the nested `replay` was + // skipped (count 1), with a warning. + match events.last().expect("event") { + AppEvent::ReplayCompleted { count, warnings, .. } => { + assert_eq!(*count, 1, "only the outer create ran; events: {events:?}"); + assert!( + warnings.iter().any(|w| w.contains("nested") && w.contains("replay inner.commands")), + "expected a nested-replay skip warning; got {warnings:?}", + ); + } + other => panic!("expected ReplayCompleted (nested replay skipped), got {other:?}"), + } + // The nested file's table was NOT created (the replay was skipped). + let cols = rt().block_on(async { db.query_data("T".to_string(), None, None, None).await }); + assert!(cols.is_err(), "inner.commands' table T must not exist (nested replay skipped)"); } #[test] diff --git a/tests/sql_select.rs b/tests/sql_select.rs index bea9d77..eb778c3 100644 --- a/tests/sql_select.rs +++ b/tests/sql_select.rs @@ -81,9 +81,11 @@ fn simple_mode_select_yields_sql_hint_and_does_not_dispatch() { assert_eq!(app.mode, Mode::Simple); type_str(&mut app, "select * from anywhere"); let actions = submit(&mut app); + // The failed simple-mode submission is journalled `err` + // (ADR-0034) but dispatches no command. assert!( - actions.is_empty(), - "simple-mode `select` must not produce a dispatch action; got {actions:?}", + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "simple-mode `select` must not dispatch (only journal err); got {actions:?}", ); // The error output spans multiple lines (the message and a // caret pointer). The hint catalog key @@ -135,8 +137,8 @@ fn advanced_mode_select_from_internal_table_is_rejected() { type_str(&mut app, "select * from __rdbms_playground_columns"); let actions = submit(&mut app); assert!( - actions.is_empty(), - "internal-table reference must not dispatch; got {actions:?}", + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "internal-table reference must not dispatch (only journal err); got {actions:?}", ); let error_text: String = app .output diff --git a/tests/walking_skeleton.rs b/tests/walking_skeleton.rs index 2111708..6c8a040 100644 --- a/tests/walking_skeleton.rs +++ b/tests/walking_skeleton.rs @@ -108,7 +108,12 @@ fn typing_invalid_simple_input_shows_a_parse_error_not_an_echo() { let theme = Theme::dark(); type_str(&mut app, "hello world"); let actions = submit(&mut app); - assert!(actions.is_empty()); + // The failed line journals `err` (ADR-0034) but does not echo + // or dispatch a command. + assert!( + matches!(actions.as_slice(), [Action::JournalFailure { .. }]), + "expected only a JournalFailure; got {actions:?}", + ); let rendered = rendered_text(&mut app, &theme, 80, 24); assert!( rendered.contains("parse error"), @@ -605,6 +610,7 @@ fn dsl_failure_shows_friendly_error_in_output() { kind: rdbms_playground::db::SqliteErrorKind::NoSuchTable, }, facts: rdbms_playground::friendly::FailureContext::default(), + source: String::new(), }); let rendered = rendered_text(&mut app, &Theme::dark(), 80, 24); assert!(