docs: session handoff 48 — DSL→SQL echo Phases 1-3 done, polish only

docs/handoff/20260528-handoff-48.md

@@ -0,0 +1,256 @@
+# Session handoff — 2026-05-28 (48)
+
+Forty-eighth handover. **ADR-0038 is feature-complete except for the §4
+styled-runs rendering polish.** Buckets A + B and the §6 category-3 prose
+all ship, every catalogue row round-trips per line, docs are in sync,
+and a /runda Devil's-Advocate pass landed cleanup. The session built
+the renderer out from the handoff-46 walking skeleton through Phases
+1, 2, and 3 in one go; if you continue, the polish is the only open
+item. If you start a different feature, ADR-0038 won't surprise you.
+
+## §1. State at handoff
+
+**Branch:** `main`. **HEAD `5cb105b`.** **Tests: 2015 passing, 0 failing,
+0 skipped, 1 ignored.** **Clippy: clean** (`--all-targets -D warnings`,
+nursery).
+
+Commits since handoff-47's `c60026c`:
+
+```
+5cb105b docs(adr): /runda DA cleanup — reflect Phases 1-3 done, pin Bucket C
+e6ad1ae feat: DSL→SQL teaching echo — Phase 3 cat-3 caveat (ADR-0038)
+275c726 feat: DSL→SQL teaching echo — Bucket B renderer (ADR-0038 Phase 2)
+558cfae docs(adr): promote ADR-0037 + ADR-0038 to Accepted (Phase 1 shipped)
+```
+
+(Plus handoff-47 itself, `c60026c`, immediately prior.)
+
+## §2. ADR status
+
+| ADR | What | Build status |
+|---|---|---|
+| **ADR-0035 Am2** | standard-first dialect + `ALTER COLUMN` gap-fill | ✅ done (handoff-46 `338dc8a`) |
+| **ADR-0033 Am4** | `update … --all-rows` falls back to DSL | ✅ done (handoff-46 `9a23e28`) |
+| **ADR-0037** | `EffectiveMode` execution-time side-channel | ✅ **Accepted** — channel done (handoff-46 `04c8e42`), validated end-to-end by all four ADR-0038 commits |
+| **ADR-0038** | the echo + full catalogue | ✅ **Accepted** — Phases 1-3 done (`90479cb` / `275c726` / `e6ad1ae`); **§4 styled-runs polish remains** |
+| **ADR-0039** | EXPLAIN over advanced SQL | ⏸ deferred follow-up — separate feature |
+
+ADR-0037 and ADR-0038 were promoted to **Accepted** with implementation
+notes in `558cfae`; the /runda cleanup (`5cb105b`) refreshed both
+entries to reflect every shipping commit.
+
+## §3. What landed since handoff-47
+
+### §3a. Phase 2 — Bucket B renderers (`275c726`, +1049 / −53 across 4 files)
+
+Two new echo-build paths the handoff-46 ⚠️ gotcha foreshadowed:
+
+- **`collect_echo_lookups`** (pre-execution, runtime) — resolves names
+  the dropped thing or not-yet-created column would erase post-
+  execution: positional `drop index` (describe the table, find the
+  index by exact column-set match), `drop relationship` in both
+  selector forms (`Endpoints` via child describe + matching by
+  endpoints; `Named` via a small `list_tables` scan acceptable for
+  teaching-playground schemas), and the `--create-fk` pre-state
+  (whether the child column already existed + the parent's PK type to
+  derive the new column type via `Type::fk_target_type` — `serial →
+  int`, `shortid → text`).
+- **`build_schema_echo`** (post-execution, runtime) — subsumes the
+  Bucket A pure-`Command` schema cases (`create table`, `rename
+  column`, `add`/`drop constraint` — delegates to `command_to_sql`)
+  *and* renders Bucket B from the description (`add index` resolved
+  from `desc.indexes` by column set; `add relationship` from the
+  parent's `inbound_relationships` by endpoints) and the lookups.
+
+Plus a **payload refactor** — `Option<String>` → `Option<Vec<String>>`
+on all 7 success events + arms + render path, motivated by the
+category-2 multi-statement echoes (`drop column --cascade`, `add
+relationship --create-fk`). Each catalogue row's pure renderer (and
+its round-trip) lives in `src/echo.rs`; the runtime composes them.
+
+The `DropColumn` arm gained `build_drop_column_cascade_echo`, which
+reads `DropColumnResult::dropped_indexes` to emit the multi-line
+cascade echo; non-cascade falls through to the pre-execution Bucket A
+echo unchanged.
+
+### §3b. Phase 3 — category-3 prose (`e6ad1ae`, +130 / −8 across 5 files)
+
+Phase 3 turned out to be small: the existing `client_side.*` notes
+already covered **shortid generation** (`auto_fill_add_shortid`,
+`auto_fill_transition`) and **type-conversion transforms**
+(`transformed`, `transformed_lossy`), and they already render in the
+right position (between the echo and the structure). Only the **`change
+column … --dont-convert` caveat** was actually missing — the one
+Bucket A caveat (every other category-3 line is *illuminating*).
+
+Wiring: a new `dont_convert_caveat: bool` on `DslChangeColumnSucceeded`,
+set by the runtime when `submission_mode.is_advanced() && matches!(cmd,
+ChangeColumnType { mode: DontConvert, .. })`, rendered by
+`handle_dsl_change_column_success` between the client-side notes and
+the structure. Gated on advanced mode because the caveat references
+"the line above" — the echo, which only fires in advanced mode. New
+i18n key `client_side.dont_convert_caveat` (registered in
+`keys::KEYS_AND_PLACEHOLDERS`).
+
+### §3c. /runda DA cleanup (`5cb105b`, +128 / −32 across 4 files)
+
+Devil's-Advocate audit at the end of the session caught three doc-drift
+bugs the per-phase commits left behind (requirements.md M4 and both
+ADR-0038 Status block + README index entry still said "Phase 2 / Phase
+3 remain") and one test gap — Bucket C cases (`show table`, `explain`,
+`replay`, every `Command::App` variant) flowed through the catch-all
+`_ => None` without explicit no-echo coverage. The new
+`bucket_c_no_echo_commands_all_return_none` test pins all four; a
+future renderer arm at the wrong place can no longer silently leak.
+
+## §4. The architecture in shape now
+
+Two echo construction paths now run in `spawn_dsl_dispatch`, side by
+side:
+
+- **Pre-execution** (`echo::echo_for` → `echo::command_to_sql`) — the
+  outer `echo` var, used by the **non-Schema** arms (`Update`,
+  `Delete`, `AddColumn`, `DropColumn` non-cascade, `ChangeColumn`).
+  Pure function of the `Command`. Returns `Option<Vec<String>>`
+  (single-element for the Bucket A single-line cases).
+- **Post-execution** — three builders, each in the arm matching its
+  outcome:
+  - `build_schema_echo(command, mode, description, lookups)` for
+    `Schema` outcomes (subsumes Bucket A pure-Command schema +
+    Bucket B `add index` / `drop index` / `add`/`drop relationship`).
+  - `build_show_data_echo(database, command, mode)` for `Query`
+    outcomes (only `show data` echoes; the runtime describes the table
+    when the query is limited to source the `ORDER BY` PK).
+  - `build_drop_column_cascade_echo(command, mode, result)` for the
+    `DropColumn` arm when `cascade: true` (reads
+    `DropColumnResult::dropped_indexes`); non-cascade falls through to
+    the pre-execution echo.
+
+The pre-execution lookup helper `collect_echo_lookups` runs once at the
+top of the dispatch, captures whatever drops or `--create-fk` pre-state
+will need post-execution, and passes through to `build_schema_echo`.
+Replay still bypasses the whole spawn (`run_replay` calls
+`execute_command_typed` directly) so no echo fires there — per
+ADR-0038 §3.
+
+Rendering: `note_ok_summary` loops over `pending_echo: Vec<String>`,
+emitting one `Executing SQL: <line>` per entry via `note_system`.
+Plain `[system]` lines today — the polish replaces this with
+ADR-0028 styled runs.
+
+## §5. The one open item — §4 styled-runs polish
+
+ADR-0038 §4: "*The App renders it as one or more **de-emphasised**
+`OutputLine`s beneath the `[ok]` summary, using the ADR-0028 styled-
+runs mechanism (a dimmed `Executing SQL:` prefix; the SQL in a code-ish
+run). One statement per line (§6 category 2).*"
+
+Today the echo + the DontConvert caveat surface as plain `note_system`
+lines (a single style). The polish needs:
+
+- Read **ADR-0028** for the styled-runs `OutputLine` shape — there is
+  already a mechanism (used by the explain plan tree rendering — see
+  `output_render::render_explain_plan` for the established pattern of
+  pushing pre-built `OutputLine`s via `push_output`).
+- Replace `note_system(crate::t!("echo.executing_sql", sql = line))` in
+  `note_ok_summary` with a styled `OutputLine`: dimmed `Executing
+  SQL:` prefix run + a code-ish run for the SQL itself. Decide the
+  exact style (italic? dim? distinct fg colour?) consistent with
+  ADR-0028's existing styled-runs surface; check both `theme::light()`
+  and `theme::dark()` for legibility.
+- The DontConvert caveat is also a category-3 prose line — apply the
+  same de-emphasised treatment (or a complementary one — caveat could
+  warrant a `WARNING`/yellow accent, since it flags non-equivalence).
+- Multi-statement echoes: each line in the Vec becomes its own styled
+  `OutputLine`. Today the rendering already emits one line per entry;
+  the polish is per-line styling, not per-line splitting.
+
+Tests will need adjustment: the App-level tests (`bucket_a_success_*`,
+`bucket_b_multi_line_*`, `change_column_dont_convert_renders_*`)
+currently assert `texts[…].contains("Executing SQL: …")` from
+`note_system` output. After the polish, the same content will live in
+styled-runs — the tests should still find the substring within the
+output line text but may need to look at `OutputLine` styled-run
+content rather than the flat `text` field. Decide style-vs-text
+assertion shape early.
+
+The work is rendering-only — no Command-level changes, no event-shape
+changes, no worker changes. Estimated scope: small. The biggest care
+point is consistency with ADR-0028's existing styled-runs usage.
+
+## §6. Catalogue cheat-sheet (authoritative: ADR-0038 §7)
+
+Unchanged since handoff-47. **Every Bucket A and Bucket B row is now
+implemented and round-tripped** (`echo::tests::*` + the runtime
+end-to-end tests `bucket_b_resolved_name_echoes_against_real_worker` +
+`bucket_b_multi_statement_echoes_against_real_worker`). Bucket C
+(no-echo) is explicitly pinned by
+`bucket_c_no_echo_commands_all_return_none`. Category-3 lines flow
+through the existing `client_side.*` infrastructure plus the new
+`dont_convert_caveat`.
+
+## §7. Known follow-ups outside the ADR-0038 scope
+
+These are **not** the styled-runs polish — they are separate items
+the /runda surfaced or that emerged during the session, captured here
+so they aren't lost.
+
+- **Unquoted-identifier round-trip caveat.** The echo style (`render_*`
+  in `src/echo.rs`) emits bare identifiers — matching the existing
+  `render_create_table` aesthetic. A user-created column or table
+  named after a SQL keyword (e.g. `ORDER`, `SELECT`, `WHERE`) would
+  produce an echo whose round-trip is fragile. Pre-existing limitation
+  (the create-table echo had it from handoff-46); the §1 contract is
+  only formally violated for this edge case. If it ever bites a
+  learner, the fix is to add identifier quoting (probably backticks or
+  double-quotes per the advanced grammar's accepted spelling, plus a
+  conservative "needs quoting" predicate).
+- **No full-spawn integration test for the echo pipeline.** Coverage
+  today is layered: pure renderers + runtime-helper unit tests (with
+  real `Database`) + App-level rendering tests. The unwired glue is
+  small (~3 lines per arm in `spawn_dsl_dispatch`). A Tier-3 test
+  that drives the full submit → runtime → App pipeline would complete
+  the picture; not blocking.
+- **Pure-Command Schema commands double-compute echo.** `echo_for` is
+  called for every command, and the Schema arm then computes
+  `build_schema_echo` (whose catch-all redelegates to
+  `command_to_sql`). Minor inefficiency, no behaviour bug. Could be
+  optimized later; not worth the code complexity now.
+- **ADR-0039 (EXPLAIN over advanced SQL)** — separate deferred
+  follow-up, not part of ADR-0038. Recorded 2026-05-27 (handoff-45/46);
+  pick up when prioritised.
+
+## §8. Process pins (unchanged)
+
+- **Confirm every commit** (propose message, wait). **No AI attribution.**
+- **Test-first**; green + clippy-clean is the only acceptable end
+  state; current baseline **2015 / 0 / 1**.
+- **Keep docs lockstep**: ADR + `README.md` index + `requirements.md`.
+  This session's `5cb105b` was the corrective pass when the prior
+  per-phase commits left M4 / ADR-0038 status drifted.
+- **Round-trip every catalogue row** (the §1 copy-paste contract; for
+  multi-statement, *per line*).
+- **Pre-execution lookups** for any future Bucket-B-style additions
+  whose echo needs info the dropped/created thing erases. Mirror the
+  `collect_echo_lookups` shape (best-effort, fail-soft) and consume
+  in the post-execution arm.
+
+## §9. How to take over
+
+1. **Read, in order:** this file → **ADR-0038** (§4 styled-runs
+   reference is the relevant bit for the polish) → **ADR-0028**
+   (`OutputLine` styled-runs mechanism; `output_render::render_*` for
+   established patterns) → **`src/echo.rs`** (the renderer surface
+   itself is settled — but read to understand the catalogue shape if
+   you'll touch rendering) → **`src/app.rs`**'s `note_ok_summary` and
+   `handle_dsl_change_column_success` (the two consumers of the echo
+   + caveat strings — these are what the polish replaces).
+2. **Baseline:** `cargo test` (2015 / 0 / 1) + `cargo clippy
+   --all-targets -- -D warnings` (clean).
+3. **If continuing on ADR-0038**: the polish is the only open item.
+   See §5. Decide the App-test assertion shape early (style-vs-text).
+4. **If picking up something else**: ADR-0038 is done modulo the
+   polish; the §4 line of the ADR remains the formal outstanding
+   item but does not block other work. ADR-0039 (EXPLAIN over
+   advanced SQL) is a self-contained deferred follow-up if you want
+   the next feature.