diff --git a/docs/handoff/20260522-handoff-31.md b/docs/handoff/20260522-handoff-31.md new file mode 100644 index 0000000..55512d8 --- /dev/null +++ b/docs/handoff/20260522-handoff-31.md @@ -0,0 +1,264 @@ +# Session handoff — 2026-05-22 (31) + +Thirty-first handover. This session shipped, in order: + +1. **Finished the advanced-mode completion interlude** left open by + handoff 30 — the **F5** core `walk_seq` fix, plus a follow-up + `walk_repeated` fix and a candidate-ordering reversal that grew + out of manual testing. +2. **Advanced ADR-0033 Phase 3 through sub-phases 3c, 3d, 3e** — + SQL `INSERT … SELECT`, `shortid` auto-fill, and SQL `UPDATE`. + +The next session should **start sub-phase 3f** (`DELETE` + +cascade summary) — the largest remaining DML sub-phase — which is +why we're handing off first. See §4 for a key 3f heads-up. + +## §1. State at handoff + +**Branch:** `main`. **Tests: 1524 passing, 0 failing, 1 ignored.** +**Clippy:** clean (`cargo clippy --all-targets -- -D warnings`). + +**Commits this session** (newest first). `origin/main` is at +`6ff9144` (3c), so the **last three are local-only**: + +``` +53808ed grammar+db: 3e — SQL UPDATE grammar + execution (ADR-0033 §2) (local) +18d34d0 db: 3d fix — don't let shortid auto-fill mask INSERT arity mismatch (local) +78ad476 db+grammar: 3d — shortid auto-fill for SQL INSERT (ADR-0033 §6) (local) +6ff9144 grammar: 3c — INSERT … SELECT row source (ADR-0033 §4) (origin/main) +7f68a53 walker+completion: surface list trailing-optionals + identifiers-first ordering (ADR-0022 Amendment 2) +43c49f4 walker: F5 — drop preceding-clause keywords from committed-child Incomplete sets +``` + +(Unpushed commits are a normal working state; pushing is the +user's step — do not prompt about it.) + +## §2. THE process lesson (read this first) + +This session's clearest signal is about **Devil's Advocate +discipline**, not code. Three times the lead produced a DA +"verdict" that merely restated the exit gate — and three times the +*user* had to prompt before a genuine adversarial pass happened. +Each real pass then found something concrete: + +- "What about more than one shortid in a row?" → an **untested** + multi-shortid path (it worked, but had zero coverage). +- "Did you check with the DA?" → the **arity-masking bug** (3d + auto-fill silently dropped extra columns / could read out of + range). Real correctness bug; fixed in `18d34d0`. +- "Sounds like the DA had nothing to add." → the **untested render + guard** in 3e (`handle_dsl_update_success`). + +**Takeaway for the next agent:** do the adversarial work +*proactively* and *in writing*, and make it produce a failing test +or a concrete change — not a paragraph that clears everything. +"The exit-gate tests pass" is not a DA review. Attack the code: +untested branches, edge cases the happy-path tests skip, behaviour +that diverges from the non-SQL path, things that only break when an +input is malformed. A clean PASS with no findings on a non-trivial +sub-phase should itself be suspicious. + +## §3. Phase 3 — what shipped (3c / 3d / 3e) + +Controlling docs (read both): +- `docs/adr/0033-sql-dml-grammar.md` — the decision (read + **Amendment 1** for the dispatch mechanism). +- `docs/plans/20260520-adr-0033-phase-3.md` — build order + (3a–3k), per-sub-phase exit + DA gates. + +### 3c — `INSERT … SELECT` (`6ff9144`) +Grammar-only: the INSERT row source became a +`Choice(VALUES_CLAUSE, Subgrammar(&sql_select::SQL_SELECT_COMPOUND))`. +`SQL_SELECT_COMPOUND` is itself a `Choice` that admits a leading +`WITH`, so the R4 (WITH-prefixed) row source parses for free. No +worker change — `do_sql_insert` already executes the validated SQL +and the engine handles insert-from-query. + +### 3d — `shortid` auto-fill (`78ad476`) + arity fix (`18d34d0`) +`Command::SqlInsert` gained `listed_columns` and `row_source`, +extracted in `build_sql_insert` from the matched path (the row +source is found by the **first `values`/`select`/`with` Word +token**, which is path-based so a string literal like `'select'` +can't be mistaken for the keyword). `plan_shortid_autofill` +(`db.rs`) implements the **user-chosen Option B**: when the column +list omits a `shortid` column, materialise the row source by +running it as a query, generate a distinct id per row via the +existing `generate_shortid_batch` (deduped against stored values), +and reconstruct a parameterised multi-row INSERT. Uniform for +VALUES and SELECT; handles multiple omitted shortids (one batch per +column). `serial` stays engine-filled via rowid. `history.log` +keeps the original line, never the rewrite (§11). + +**Arity guard (`18d34d0`):** the auto-fill path read exactly +`listed_columns.len()` cells per row — a column/value arity +mismatch silently dropped extra columns or read out of range. Now: +if the materialised statement's `column_count() != listed_count`, +skip auto-fill and run verbatim so the engine reports the mismatch +(a friendly pre-flight is 3i). + +Minor deviation from the plan: the plan's 3d step said "turn on +`writes_user_listed_column`". We did **not** flip that flag — the +worker only needs the column *names*, collected by role in +`build_sql_insert`. The flag drives completion-narrowing of VALUES +against the listed columns, which nothing needs yet. It remains +`false`; flipping it is a separate completion enhancement, not a +blocker. + +### 3e — SQL `UPDATE` (`53808ed`) +New `src/dsl/grammar/sql_update.rs`: `SQL_UPDATE_SHAPE = + SET col = sql_expr (',' …)* [WHERE sql_expr] [';']`. No +`--all-rows` rail (ADR-0030 §12). `Command::SqlUpdate { sql, +target_table }`, `Request::RunSqlUpdate`, `do_sql_update` (execute +verbatim, re-persist target, history). 3e surfaces the +**affected-row count only**; precise rows are RETURNING (3g). + +**Two findings that matter for later sub-phases:** + +1. **Cross-cut diagnostics are NOT automatically free.** The + schema-existence + predicate-warning passes (`mod.rs`) build + their scope from Tables idents whose **role is `"table_name"`** + (the pre-pass at `schema_existence_diagnostics`). A bespoke role + (`update_target_table`) left the SET/WHERE columns unchecked + (`diag_keys` returned `[]`). Fix: the UPDATE target uses the + shared `"table_name"` role. **3f's DELETE target must do the + same** to get the predicate diagnostics on its WHERE for free. +2. **Render guard.** A column-less `UpdateResult` would render a + misleading `(no rows)` band. `handle_dsl_update_success` now + skips `render_data_table` when `result.data.columns.is_empty()`. + The DSL UPDATE always has columns, so it's unaffected. Covered + by app-level tests both ways. + +`sql_select::WHERE_CLAUSE` is now `pub(crate)` so the DML +statements reuse the exact predicate clause. + +## §4. Sub-phase 3f — the next job (DELETE + cascade) + +Per the plan (`docs/plans/20260520-adr-0033-phase-3.md`, "Sub-phase +3f"): new `src/dsl/grammar/sql_delete.rs` (`DELETE FROM table +[WHERE] [';']`), `Command::SqlDelete` / `Request::RunSqlDelete` / +`do_sql_delete`, cascade-summary pre-count (ADR-0033 §7), the +`format_cascade_summary` formatter **shared** with the DSL +`do_delete`, and **multi-table persistence** (target + every +cascade-affected child). + +**Heads-up — the WHERE-byte-extraction problem is tractable for +DELETE.** In 3e I worried that a flat token path can't distinguish +a statement-level WHERE from a subquery's WHERE (a `where` token +can appear before *or* after the statement one). That ambiguity +comes from UPDATE's `SET` clause possibly holding a subquery-with- +WHERE *before* the statement WHERE. **DELETE has no SET** — nothing +before the statement WHERE can contain a subquery — so the +statement WHERE is simply the **first `where` Word token after the +target table**, and the predicate text is `source[where_start..]` +(trim trailing `;`). The R2 invariant +(`DELETE … WHERE x IN (SELECT … WHERE …)`) is fine: the nested +subquery WHERE is *inside* the predicate, which is exactly what the +cascade pre-count wants to inject. So mirror the +`build_sql_insert` row_source / `build_sql_update` extraction: find +the first `where` token, capture the clause text into +`Command::SqlDelete`, and inject it into +`SELECT FROM ` for each child +pre-count. When there is no WHERE, the pre-count is unbounded +(all target rows). + +Other 3f gotchas from the plan's DA gate: the cascade pre-count +must run **before** the DELETE (the rows being counted are the ones +about to vanish); cascade-affected child CSVs must all be +re-persisted; and the SQL path's per-relationship summary must +match the DSL path's on the same schema/data (shared formatter). + +## §5. Established patterns (reuse these in 3f–3i) + +- **Dev entry word per sub-phase.** SQL DML is isolated behind + `sqlinsert` / `sql_update` (and `sql_delete` next) entry words, + `CommandCategory::Advanced` in `REGISTRY` + (`src/dsl/grammar/mod.rs`). The `build_sql_*` ast-builder + reconstructs the real keyword (`insert`/`update`/`delete`) + + matched tail. **3j removes all dev words** and makes + `insert`/`update`/`delete` shared DSL/SQL entry words; 3j must + also de-dup the completion entry-word lists once a word appears + twice in `REGISTRY` (flagged in ADR-0033 Amendment 1). +- **`table_name` role for any target whose WHERE/SET columns need + schema diagnostics** (see §3e finding 1). +- **Static-vs-const in grammar files.** A `Node` referenced *by + value* in a `static [...]` array must be `const` (so it inlines); + a `Node` referenced via `&NODE` can be `static`. Getting this + wrong gives "cannot move out of a shared reference" (hit twice + in 3e). +- **Worker result + render.** SQL DML reuses the DSL + `CommandOutcome::{Insert,Update,Delete}` and the + `handle_dsl_*_success` renderers. Any new `Command` variant must + be added to: `command.rs` `verb()` + `target_table()`, the + `runtime.rs` dispatch, `app.rs` `build_translate_context`, and + the `tests/typing_surface/mod.rs` `command_kind_label` match + (all are non-exhaustive checks that will fail to compile until + covered — a useful forcing function). + +## §6. Escalations settled this session (do not re-litigate) + +- **Identifiers-first candidate ordering** (ADR-0022 Amendment 2): + schema identifiers sort *before* keywords, globally — the user + explicitly chose this over the handoff-14 keywords-first + invariant, after seeing that long SQL keyword runs pushed column + names off the single-row, window-scrolled candidate line. A + **two-line hint box** is recorded as a deferred follow-up. +- **3d SELECT shortid strategy = Option B** (materialise + dedup + + reinsert), user-confirmed. +- **`auto_column_overridden` WARNING stays INSERT-only** (the plan + default). 3e did not extend it to UPDATE; if 3f/3i wants to, the + plan says escalate. + +## §7. Still deferred (tracked, not lost) + +- **RETURNING** (3g) — precise DML row output; 3e/3f surface counts + (+ cascade summary) only until then. +- **UPSERT `ON CONFLICT`** (3h), **diagnostics** `insert_arity_mismatch` + / `auto_column_overridden` / `not_null_missing` (3i), + **dispatch wiring / dev-word removal** (3j), **verification + sweep** (3k). +- **Old-project migration** (from handoff 30 §4) — the pre- + `check_expr` 3-column `__rdbms_playground_columns` schema; the + user deferred a migration story to "the end". Targeted fix when + the migration framework (ADR-0015 Iter 6) lands. +- **Two-line hint box** (ADR-0022 Amendment 2). +- **`writes_user_listed_column`** flag (see §3d) — VALUES-against- + listed-columns completion narrowing. + +## §8. Process pins (unchanged, still binding) + +- **Confirm every commit.** Propose the 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.** Reproduce a bug with a FAILING test before + fixing; for features, exit-gate tests before "done". This is + also how the real DA findings surfaced this session — write the + cross-cut/edge test and watch it fail. +- **Core walker changes** (`walk_seq` / `walk_choice` / + `walk_repeated`) need explicit user OK before coding. (F5 and the + 3d-era `walk_repeated` change both went through this.) +- **Escalate ambiguity; never classify work out of scope without + user confirmation.** + +## §9. How to take over + +1. **Read, in order:** this file → + `docs/adr/0033-sql-dml-grammar.md` (esp. §7 cascade + §2 + UPDATE, already shipped) → + `docs/plans/20260520-adr-0033-phase-3.md` "Sub-phase 3f" → + `CLAUDE.md`. +2. **Baseline:** + ``` + cargo test # expect 1524 passing / 0 failing / 1 ignored + cargo clippy --all-targets -- -D warnings # clean + ``` +3. **Start 3f** per §4. Mirror `sql_update.rs` for the grammar and + `do_sql_update` for the worker shell; the new work is the + cascade pre-count + the shared `format_cascade_summary` refactor + + multi-table persistence. Use the `table_name` role for the + DELETE target. Capture the WHERE clause text via the first + `where` token (tractable for DELETE — §4). +4. **Do the DA pass for real** (§2). On a sub-phase this size, + expect to find at least one untested branch or edge case; + if you don't, look harder. +5. **Escalate** anything not settled in ADR-0033 / the plan.