diff --git a/docs/handoff/20260528-handoff-48.md b/docs/handoff/20260528-handoff-48.md new file mode 100644 index 0000000..45d27f8 --- /dev/null +++ b/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` → `Option>` +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>` + (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`, +emitting one `Executing SQL: ` 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.