diff --git a/docs/handoff/20260521-handoff-30.md b/docs/handoff/20260521-handoff-30.md new file mode 100644 index 0000000..52a0a89 --- /dev/null +++ b/docs/handoff/20260521-handoff-30.md @@ -0,0 +1,274 @@ +# Session handoff — 2026-05-21 (30) + +Thirtieth handover. This session did two things: + +1. **Started ADR-0033 Phase 3 (SQL DML)** — wrote the Phase-3 + implementation plan, then shipped sub-phases **3a** (DSL/SQL + dispatch mechanism) and **3b** (SQL `INSERT` grammar + + minimal execution). +2. **Ran an advanced-mode completion/hint bug-fixing interlude** + triggered by manual testing — fixed the dead advanced-mode + ambient assistance plus two completion bugs (F1, F2), and + triaged several other reports. + +The next session should **finish the interlude** (one bug left, +**F5**, which needs the user's OK because it's a core walker +change) and then **resume Phase 3 at sub-phase 3c** +(`INSERT … SELECT`). + +## §1. State at handoff + +**Branch:** `main`. **Tests: 1473 passing, 0 failing, 1 ignored.** +**Clippy:** clean (`cargo clippy --all-targets -- -D warnings`). + +**Commits this session** (newest first; the last three are +local-only, `origin/main` is at `4e16d97`): + +``` +1c8cbc1 completion+hint: F1/F2 advanced-mode completion fixes (local) +ed40445 ui: re-enable advanced-mode ambient assistance (ADR-0022 Amendment 1) (local) +c873631 grammar+db: 3b — SQL INSERT grammar + minimal execution (ADR-0033 §1) (local) +4e16d97 walker: 3a — category-grouped mode-aware dispatch (ADR-0033 Amendment 1) +a37a0b7 docs: ADR-0033 Phase 3 — implementation plan + cross-cut matrix +``` + +(Unpushed commits are a normal working state; pushing is the +user's step — do not prompt about it.) + +## §2. Phase 3 — the main job (ADR-0033, SQL DML) + +**Controlling docs (read both):** +- `docs/adr/0033-sql-dml-grammar.md` — the decision. **Read + Amendment 1** especially (it supersedes §2's dispatch + mechanism — see below). +- `docs/plans/20260520-adr-0033-phase-3.md` — the build order + (sub-phases 3a–3k), per-sub-phase exit gates + DA gates, the + cross-cut verification matrix (filled in during 3k), and the + **standing authorizations** (commits pre-authorized within + the plan — but see §5 below: the user has since asked to + approve each commit and watch context, so confirm commits). + +### 2.1. Done + +**Sub-phase 3a — DSL/SQL dispatch mechanism (commit `4e16d97`).** +The ADR originally specified `Node::Guard` + `Choice(SQL, DSL)`. +**That was found unworkable during 3a** (any guard-in-`Choice` +forces a `walk_choice` change; `walk_choice` only falls through +on `NoMatch`, and `walk_seq` treats a `NoMatch` past `idx 0` as a +hard `Failed`). It was replaced — **with the user's explicit +approval** — by **category-grouped, mode-aware dispatch**, +recorded in **ADR-0033 Amendment 1**: +- Each `REGISTRY` entry is tagged `CommandCategory::{Simple, + Advanced}` (in `src/dsl/grammar/mod.rs`), generalising the old + whole-command `is_advanced_only` gate. +- `walker::walk` is now a thin dispatcher over `decide` + + `walk_one_command` + scratch helpers (`src/dsl/walker/mod.rs`). + Simple mode commits the DSL node (or emits "this is SQL"); + Advanced tries Advanced candidate(s) first, DSL as fallback. +- A *shared* entry word carries a node in both groups; the + dispatcher selects by mode. **No `Node::Guard`, no + `walk_choice`/`walk_seq` change.** + +**Sub-phase 3b — SQL `INSERT` grammar + minimal execution +(commit `c873631`).** +- `src/dsl/grammar/sql_insert.rs::SQL_INSERT_SHAPE` — `INTO + [ (cols) ] VALUES tuple(s)`, `__rdbms_*` target + rejection (reuses `sql_select::reject_internal_table`, now + `pub(crate)`). +- `Command::SqlInsert { sql, target_table }`; + `Request::RunSqlInsert` + `do_sql_insert` worker (tx-guarded: + execute → `finalize_persistence` for CSV + history → commit, so + failures roll back and don't re-persist). Auto-show is + best-effort via `last_insert_rowid` range. +- **Isolated behind a temporary dev entry word `sqlinsert`** + (Advanced category) so the SQL path is testable WITHOUT making + `insert` a shared word yet. Tests use `sqlinsert into …`. +- **Deviations from the plan's literal `Command::SqlInsert` field + list** (to avoid dead-code under `-D warnings`): + `listed_columns` is deferred to **3d** (shortid auto-fill) and + `returning` to **3g** — the sub-phases that actually read them. +- Tests: 6 grammar accept/reject (`sql_insert.rs`) + 8 + integration (`tests/sql_insert.rs`). + +### 2.2. Remaining sub-phases (resume here after the interlude) + +Per the plan, in order: **3c** `INSERT … SELECT` → **3d** +`shortid` auto-fill (adds `listed_columns`) → **3e** `UPDATE` → +**3f** `DELETE` + cascade summary → **3g** `RETURNING` (adds +`returning` flag) → **3h** UPSERT (`ON CONFLICT`) → **3i** +diagnostics (`insert_arity_mismatch`, `auto_column_overridden`, +`not_null_missing`) → **3j** dispatch wiring → **3k** +verification sweep. + +**Things to remember for later sub-phases:** +- **3j** is where the dev `sqlinsert` word is removed and + `insert` becomes a real shared word (register the SQL INSERT + node as `Advanced` alongside the DSL `insert` `Simple` node). + With the grouping mechanism this is mostly registry wiring. + **3j must also add de-duplication** to the completion + entry-word lists (`completion_probe_in_mode` / + `expected_at_input_in_mode` in `src/dsl/walker/mod.rs`): once + `insert` appears twice in `REGISTRY`, the entry-word candidate + lists would show it twice. (Flagged in ADR-0033 Amendment 1.) +- **3j** must also confirm advanced-mode DSL inserts don't + regress through the (now-shared) SQL path — which is why 3j + comes after 3d's shortid-auto-fill parity. + +## §3. The advanced-mode completion/hint interlude + +Triggered by the user manually testing advanced mode. **Paused +Phase 3 to harden it** because broken ambient assistance poisons +every advanced-mode test we'd write. + +### 3.1. Fixed and committed + +**Ambient assistance re-enabled in advanced mode (ADR-0022 +Amendment 1, commit `ed40445`).** `render_hint_panel` hard- +returned `None` for advanced mode (a stale ADR-0022 §12 gate +predating the SQL grammar), and the hint resolver defaulted to +`Mode::Simple`. Now: `ambient_hint_in_mode` + +`hint_resolution_at_input_in_mode` + `expected_for_hint_snapshot` +thread `Mode`; `render_hint_panel` calls ambient for all modes; +the one-shot `:` sigil is stripped (`strip_one_shot_prefix`) +before the ambient walk. App-level render test +(`ui::advanced_mode_hint_panel_surfaces_sql_candidates`) + +ambient-layer locks. (Phase 2 had marked SQL hint/completion +"green" but only at the *engine* layer, never the UI — the gap +this fix closes.) + +**F1 + F2 (commit `1c8cbc1`).** +- **F1** — a premature "no such table/column" ERROR shadowed the + completion for the token being typed (the hint panel *is* the + completion UI; there is no separate popup). Fix in + `input_render.rs::ambient_hint_in_mode`: suppress an + under-cursor ERROR diagnostic when a completion exists for the + (non-empty) partial it overlaps; fall through to candidates. + Genuinely-unknown names still error; WARNINGs unaffected; both + modes. +- **F2** — `select from T` (after deleting `*`) offered + the global column list instead of `T`'s columns, because the + §10.6 look-ahead's full-input walk can't reach `FROM` through + an empty projection. Fix in + `completion.rs::candidates_at_cursor_with_in_mode`: when the + look-ahead finds no scope, retry with a neutral `1 ` placeholder + inserted at the cursor so the trailing `FROM`/CTE scope is + recovered for narrowing (only the repaired walk's + `from_scope`/`cte_bindings` are consumed). + +### 3.2. Triaged as NOT bugs (no work needed) + +- **F3** ("`order by Name`" flagged red / no suggestions) — was a + **downstream symptom of the old-project migration gap** (see + §4), not an ORDER BY grammar bug. On a fresh db it does not + occur (`order by ` parses clean, narrows + correctly). Confirmed by reproduction. +- **F4** ("only one error span highlights") — **multi-span + highlighting works** (two simultaneous unknown columns both get + flagged). The "des red while typing" the user saw is as-you-type + harshness on a *partial keyword* (`desc` mid-typed reads as + trailing junk → parse error), not a single-span limit. +- **Case-sensitivity** (table-name case vs. column narrowing) — + **not a bug**. `columns_for_table` (`completion.rs:90`) uses + `eq_ignore_ascii_case`; verified `from orders` / `from ORDERS` + / `from Orders` all narrow correctly. An earlier DA-gate "flag" + about this was an unverified guess and was **retracted**. + +### 3.3. REMAINING — F5 (needs the user's OK before coding) + +**F5 — the `order by ` (and projection) candidate list is padded +with ~13 invalid clause keywords** (`as`, `left`, `right`, +`full`, `cross`, `inner`, `join`, `where`, `group`, `having`, +`union`, `intersect`, `except`). The columns *are* present and +correctly filtered, but the noise shoves them off-screen. The +user is fine with keywords-before-columns ordering **as long as +the keyword list is reasonably filtered** — i.e. fix the spurious +keywords. + +**Confirmed mechanism** (probed): these are stale +*skipped-optional* clause keywords from the clauses positioned +**between the last-consumed token and `order by`** (the FROM's +JOIN options, WHERE/GROUP BY/HAVING, set-ops). They accumulate in +`walk_seq`'s `pending_skipped` and get merged into ORDER BY's +`Incomplete` expected set — even though ORDER BY consumed bytes +and committed *past* those optionals. Control inputs proved it: +`… where id = 1 order by ` drops `where`+JOIN keywords (consumed), +leaving `group/having/set-ops`; `select 1 order by ` (no FROM) +*adds* `from`. + +**Proposed fix (CORE walker change — get user OK):** in +`src/dsl/walker/driver.rs` `walk_seq`, the **`Incomplete` arm** +should NOT merge the accumulated `pending_skipped` when the child +that produced the `Incomplete` consumed bytes (i.e. committed past +those optionals). The `NoMatch` arm (child consumed nothing → +optionals are genuine alternatives at the cursor) keeps merging. +**Blast radius: all parsing (1469+ tests).** This is exactly the +kind of core change the project is careful about and the user +asked to approve it first. Do it test-first: a failing test that +asserts the `order by ` expected set excludes +`where`/`group`/`join`/`union`/etc., then the fix, then full +suite + clippy. + +## §4. Deferred — old-project migration (do NOT fix now) + +The user's first repro used an **old project** +(`~/.local/share/rdbms-playground/projects/20260521-repro-1/`, +`created_at: 2026-05-09`) whose `__rdbms_playground_columns` is +the **pre-`check_expr` 3-column schema**. Current code's +`read_schema` (`db.rs:4197`) hard-references `m.check_expr`, so: +- `show table` crashes (`no such column: m.check_expr`); +- `describe_table` fails for every table → `build_schema_cache` + (`runtime.rs:992`) **silently swallows the error** → + `table_columns` ends up empty → degraded completion (this was + the real cause of the "F3" symptoms above). + +**The user explicitly deferred this** — "having a migration story +at the end is perfectly fine." So: **no migration work now.** When +the migration framework lands (ADR-0015 Iter 6, pending), the +targeted fix is to ensure-current-metadata-columns on open +(idempotent `ALTER TABLE … ADD COLUMN check_expr TEXT`), make +`read_schema` not hard-crash on a missing internal column, and +stop `build_schema_cache` from silently swallowing the describe +error (it masked this for a long time). ADR-0015 would get an +amendment. **Recorded here so it isn't lost.** + +## §5. Process pins (important for the next session) + +- **Confirm every commit.** The ADR-0033 plan pre-authorized + commits, but mid-session the user asked to **approve each commit + and watch context use** — so propose the commit message and wait + for the go-ahead. No auto-commit at sub-phase gates. +- **Push is the user's step.** Never push; never prompt about it. +- **No AI attribution** in commits (global rule). +- **Test-first discipline** held throughout this session and + should continue: reproduce a bug with a FAILING test first, then + fix. For new features, exit-gate tests before "done." +- **Don't trust unverified diagnoses** — this session twice + produced wrong guesses (the case-sensitivity "bug"; F3 as an + ORDER BY grammar bug) that empirical probes disproved. Probe + before fixing. +- **DA discipline:** specific critiques first, verdict last; + a clean PASS with no critiques is a process failure. +- **Core walker changes (`walk_seq`/`walk_choice`) need explicit + user OK** before coding (the F5 fix is the pending example). + +## §6. How to take over + +1. **Read, in order:** this file → `docs/adr/0033-sql-dml-grammar.md` + (esp. **Amendment 1**) → `docs/plans/20260520-adr-0033-phase-3.md` + → `docs/adr/0022-ambient-typing-assistance.md` Amendment 1 → + `CLAUDE.md`. +2. **Baseline:** + ``` + cargo test # expect 1473 passing / 0 failing / 1 ignored + cargo clippy --all-targets -- -D warnings # clean + ``` +3. **First: finish the interlude — F5.** Confirm with the user + that the core `walk_seq` change is OK (it's the only blocker), + then implement test-first per §3.3. +4. **Then: resume Phase 3 at sub-phase 3c** (`INSERT … SELECT`) + per the plan, continuing 3c → 3k. Remember the 3j notes in §2.2 + (remove `sqlinsert` dev word, make `insert` shared, add + completion-list de-dup) and the deferred `listed_columns`/ + `returning` Command fields (3d/3g). +5. **Escalate** anything not settled in ADR-0033 / the plan; + don't classify work "out of scope" without user confirmation.