4aeea55984
Record the submission mode per history entry so advanced commands are reusable in simple mode, and fix the bug where a ':'-one-shot command lost its ':' across sessions (ADR-0052, closing #30). Format: the history.log status token gains an optional ':adv' suffix (ok / ok:adv / err / err:adv); 'source' stays last and canonical, so replay is unaffected. The in-memory ring (still Vec<String>) stores advanced entries ': '-prefixed; recall strips the ':' in advanced mode and keeps it in simple; hydration reconstructs the prefix from the tag. Journaling moved from the worker to the dispatch layer (spawn_dsl_- dispatch / run_replay / app-command sites), where the mode is in scope with no worker plumbing; finalize_persistence writes only yaml/csv (commit-db-last still atomic for state). The journal write is now best-effort (command already committed), consistent with the failure path. App commands journal simple, so they recall bare. Journaling is now uniform (every successful command, per ADR-0034) — closing a gap where show tables/relationships/explain didn't journal. Amends ADR-0034 (status tag + journaling location), ADR-0015 §6 (history.log out of the worker tx), ADR-0040 (journal-write best-effort). 15 worker-level journaling tests retired, re-covered at the new layer (history.rs format, app.rs recall matrix, iteration6 cross-session regression, replay). 2471 pass / 0 fail / 0 skip, clippy clean.
248 lines
13 KiB
Markdown
248 lines
13 KiB
Markdown
# 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 `<ts>|<status>|<source>`; 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<String>`. 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<String>`);
|
|
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.
|