diff --git a/docs/handoff/20260523-handoff-32.md b/docs/handoff/20260523-handoff-32.md new file mode 100644 index 0000000..ceb5eaf --- /dev/null +++ b/docs/handoff/20260523-handoff-32.md @@ -0,0 +1,347 @@ +# Session handoff — 2026-05-23 (32) + +Thirty-second handover. A long, productive session that pushed +**ADR-0033 Phase 3 from 3f through 3i** (DELETE, RETURNING, UPSERT, +the three DML diagnostics), wrote **ADR-0034** (history journal), +fixed a pre-existing self-referential-cascade bug, and closed with a +`/runda` adversarial round that found **six** latent +INSERT-target-scope false-positives. + +The next session should **start sub-phase 3j** (dispatch wiring — +make `insert`/`update`/`delete` shared DSL/SQL entry words and remove +the dev scaffold words), then **3k** (verification sweep). See §4. + +## §1. State at handoff + +**Branch:** `main`. **Tests: 1613 passing, 0 failing, 1 ignored.** +**Clippy:** clean (`cargo clippy --all-targets -- -D warnings`). + +**Commits this session** (newest first). `origin/main` is at +`6b8888f` (3h), so the **six 3i commits are local-only**: + +``` +8d17583 walker: 3i /runda DA round — fix INSERT-target scope confusion (6 cases) (local) +4fa0aa0 db+walker: 3i DA pass — not_null PK false-positive fix + arity hardening (local) +cfd925c grammar+db: 3i — DML column-existence + cross-cut verification (local) +2d1112d grammar+db: 3i — not_null_missing diagnostic + TableColumn constraints (local) +6db8253 grammar+db: 3i — insert_arity_mismatch diagnostic (ADR-0033 §8.1) (local) +be63315 grammar+db: 3i — auto_column_overridden diagnostic (ADR-0033 §8.2) (local) +6b8888f grammar+db: 3h — UPSERT ON CONFLICT DO NOTHING / DO UPDATE (origin/main) +fd8b74b grammar+db: 3g — RETURNING on INSERT/UPDATE/DELETE (ADR-0033 §5) +b935090 docs: ADR-0034 — history.log as a complete journal; replay reads ok-only +62f09be db: fix self-referential cascade over-count + SQL-delete render test +2c86a13 grammar+db: 3f — SQL DELETE + cascade summary (ADR-0033 §1/§7) +``` + +(Unpushed commits are a normal working state; pushing is the user's +step — do not prompt about it.) + +## §2. Process lessons (read first) + +1. **`/runda` (a forced extra DA round) earned its keep.** After I + reported 3i "done with a thorough DA pass," the user invoked + `/runda`. Probing the **interaction** of the new 3i diagnostic + passes with the *existing* `schema_existence` pass under + `INSERT … SELECT` surfaced **six** false-positive bugs I had + missed (§3i below). The lesson: my first DA pass tested each new + pass in isolation; the bugs lived in the **cross-cut** between new + and old passes on a compound input. **Always exercise new + diagnostics against the awkward compound forms** (`INSERT … SELECT + … ON CONFLICT … DO UPDATE … RETURNING`), not just the simple ones. + +2. **Probe-then-assert works.** Each `/runda` bug was confirmed with + a throwaway test asserting the *correct* behaviour, watching it + FAIL, then fixed, then the test kept. Don't fix on suspicion. + +3. **Escalate ADR-mechanism mismatches; don't silently "improve".** + Three times this session the implementation revealed the ADR's + prescribed mechanism was wrong or insufficient (3f cascade, + 3h `excluded` scoping, history logging). Each was escalated to the + user and resolved by ADR amendment / a new ADR / a recorded + decision — never silent drift. The user values this; keep doing it. + +## §3. What shipped this session (detail) + +### 3f — SQL `DELETE` + cascade (`2c86a13`) + self-ref fix (`62f09be`) +- `src/dsl/grammar/sql_delete.rs` (`FROM [WHERE] [;]`), + `Command::SqlDelete`, `Request::RunSqlDelete`, `do_sql_delete`. +- **ADR-0033 Amendment 2** (written this session): §7's WHERE-injected + pre-count rested on a *false premise* (it claimed the DSL + `do_delete` builds pre-counts from the typed `Expr` — it does not; + it uses **before/after child row-count diffing**). The pre-count + would also have broken §2's parity promise (reporting `SET NULL` + the DSL path doesn't). Replaced by mirroring `do_delete`'s + count-diff exactly → exact parity, no WHERE extraction, **risk R2 + withdrawn**. `ON DELETE CASCADE` row removals only; SET NULL + deferred for both paths. Render shared via `CommandOutcome::Delete`. +- **Self-ref cascade bug** (pre-existing in `do_delete`, surfaced by + 3f's DA): a self-referential `ON DELETE CASCADE` FK conflated the + directly-deleted rows with cascade rows. Fixed in **both** + `do_delete` and `do_sql_delete`: when `rel.other_table == target`, + `rows_changed -= rows_affected`. + +### 3g — `RETURNING` (`fd8b74b`) +- Shared `RETURNING_CLAUSE` (reuses Phase-2 `PROJECTION_LIST`, made + `pub(crate)`); optional tail on all three SQL DML shapes. +- `returning: bool` on the three `Command::Sql*` variants (per ADR + §5), set by the ast-builders, threaded Request→client→worker. +- `run_returning` (db.rs) collects the returned rows as a + `DataResult` (a RETURNING DML mutates *and* yields in one + `query_map`); reuses `resolve_select_column_types`. `DeleteResult` + gained a `data` field, rendered alongside the cascade summary. +- **Follow-set fix:** `returning` added to the table-source and + projection bare-alias follow-sets (`sql_select.rs`), so an + `INSERT … SELECT … FROM t RETURNING …` stops before RETURNING + instead of reading it as `t`'s alias. +- **Auto-fill × RETURNING:** `build_sql_insert`'s `row_source` now + stops before the first trailing clause (ON CONFLICT or RETURNING), + and the shortid-auto-fill rewrite re-appends the whole trailing + tail so generated ids surface in `RETURNING *`. + +### 3h — UPSERT `ON CONFLICT` (`6b8888f`) +- `on_conflict_clause` on `SQL_INSERT_SHAPE`: optional `(col,…)` + target (distinct `conflict_target_column` role so it never enters + `listed_columns`), `DO NOTHING` / `DO UPDATE SET … [WHERE …]`. +- **`do` is factored OUT of the action Choice** (`DO ( NOTHING | + UPDATE … )`) because a Choice with branches sharing a `do` prefix + trips the `walk_seq`/`walk_choice` interaction (ADR-0033 + Amendment 1) — a branch matching `do` then failing its 2nd token + returns a hard `Failed`, blocking fall-through. **Re-learn:** any + `Choice` whose branches share a leading token must factor it out. +- Worker runs the UPSERT verbatim (SQLite-native); no new exec path. +- **`excluded` pseudo-table:** resolves to the target's columns + inside the DO UPDATE action and completes at `excluded.|`, but stays + flagged outside it. **Mechanism deviation (recorded):** the plan + prescribed a `from_scope` `TableBinding` push; I used **byte-range + scoping** in the diagnostic pass (`upsert_excluded: (target, + do_update_start, returning_or_end)`) + `current_table_columns` in + completion. Same behaviour, no walker scope-frame change. + +### 3i — three DML diagnostics + cross-cut (`be63315`→`8d17583`) +All under ADR-0033 §8. Each diagnostic is a separate post-walk pass in +`src/dsl/walker/mod.rs`, wired into `walk` next to +`schema_existence_diagnostics`. Catalog keys in +`src/friendly/keys.rs` + `src/friendly/strings/en-US.yaml`. + +- **`auto_column_overridden`** (WARN, §8.2) — `dml_auto_column_diagnostics`: + an explicit value for a `serial`/`shortid` listed column. +- **`insert_arity_mismatch`** (ERROR, §8.1) — `dml_insert_arity_diagnostics`: + per-tuple VALUES arity (each offending row emits its own diagnostic; + the walk stops at the first depth-0 keyword so `ON CONFLICT`/`RETURNING` + parens aren't mis-counted) + INSERT…SELECT projection arity (plain + SELECT; WITH-prefixed deferred to avoid false positives). +- **`not_null_missing`** (WARN, §8.3) — `dml_not_null_missing_diagnostics`: + omitted NOT-NULL-no-default column; **excludes serial/shortid + (auto-filled) and defaulted columns**. Needed a **data-model + extension**: `TableColumn` (in `SchemaCache`) gained `not_null` + + `has_default` (with a `TableColumn::new` constructor), populated in + `build_schema_cache` from `ColumnDescription`. **DA fix:** the cache + builder must use `c.notnull` ALONE, *not* `|| c.primary_key` — an + `int` PK is an `INTEGER PRIMARY KEY` rowid alias that auto-fills, so + treating any PK as required false-flags it. +- **`dml_target_column_diagnostics`** (ERROR) — validates + `insert_column` / `update_set_column` / `conflict_target_column` + against the INSERT target (the cross-cut "schema-existence on INSERT + VALUES" gate item; also closed DA finding #12, the UPSERT DO UPDATE + SET divergence). + +#### The `/runda` round — INSERT-target scope confusion (commit `8d17583`) +**One root cause, six manifestations, all pre-existing +false-positives.** The INSERT target is recorded under role +`insert_target_table`, NOT as a diagnostic `bindings` entry. So refs +that should resolve to the *target* row were checked against the +statement's `bindings` — which for an `INSERT … SELECT` are the +SELECT's *source* tables. New helper **`bare_ref_insert_target`** +re-scopes a ref onto the target when it sits in a target-referencing +region (the DO UPDATE byte range, or an INSERT's RETURNING list). +Applied to: (1) insert column list, (2) ON CONFLICT target, +(3) DO UPDATE SET-RHS / WHERE bare refs (also closed #12's residual), +(4) RETURNING bare refs, (5) target-qualified `t.col`, (6) +target-qualified `t.*`. Each has positive + negative tests. + +## §4. Sub-phase 3j — the NEXT job (dispatch wiring) + +Read `docs/plans/20260520-adr-0033-phase-3.md` "Sub-phase 3j" and +ADR-0033 **Amendment 1** (the category-grouped dispatch mechanism — +this is the machinery 3j uses). + +**Goal:** make `insert` / `update` / `delete` **shared DSL/SQL entry +words** and **remove the dev scaffold words** (`sqlinsert`, +`sql_update`, `sql_delete`). + +**Current state of the scaffold** (what 3j removes/rewires): +- `src/dsl/grammar/data.rs`: `SQL_INSERT` (entry word `sqlinsert`), + `SQL_UPDATE` (`sql_update`), `SQL_DELETE` (`sql_delete`) + CommandNodes, registered `Advanced` in `REGISTRY` + (`src/dsl/grammar/mod.rs`). Their ast-builders `build_sql_insert` / + `build_sql_update` / `build_sql_delete` reconstruct the real keyword + (`format!("insert {tail}")` etc.) — **once the real entry word is + shared, these collapse to `source.trim()`** (the dev-word prefix + reconstruction goes away). +- The DSL CommandNodes `data::INSERT` / `UPDATE` / `DELETE` (entry + words `insert`/`update`/`delete`) are `Simple`. + +**The mechanism (ADR-0033 Amendment 1):** dispatch is +**category-grouped** in `walker::walk`. Each `REGISTRY` entry carries +a `CommandCategory::{Simple, Advanced}`. A **shared entry word has a +node in BOTH groups** — e.g. a `Simple` DSL `insert` node and an +`Advanced` SQL `insert` node, both entry word `insert`. Selection: +- **Simple mode** → only `Simple` candidates; if none but an + `Advanced` exists, emit the `advanced_mode.sql_in_simple` hint. +- **Advanced mode** → try `Advanced` candidate(s) first, then the + `Simple` candidate as fallback (`delete from t where id=1` → SQL; + `delete from t --all-rows` → falls through to DSL). + +**3j scope (from the plan):** +1. Register the SQL shapes under the real entry words `insert` / + `update` / `delete` with category `Advanced` (alongside the + existing `Simple` DSL nodes). The dispatcher already supports two + candidates per entry word (Amendment 1 / how `select`+`with` work). +2. Remove the `sqlinsert` / `sql_update` / `sql_delete` dev words + + their CommandNodes; simplify the three `build_sql_*` ast-builders + to `source.trim()` (no keyword reconstruction). +3. **De-duplicate the completion entry-word lists** — once a word + appears twice in `REGISTRY` (Simple + Advanced), the entry-word + candidate lists must not show it twice (flagged in ADR-0033 + Amendment 1 consequences; the `command_for_entry_word` / + completion filters in `mod.rs` are the sites). +4. Verify: existing DSL insert/update/delete tests still green + (unchanged inputs/outputs); SQL inputs route via the shared word in + Advanced; a structurally-ambiguous input (`delete from t where id = + 1`) routes SQL-first; a DSL-only input (`delete from t --all-rows`) + falls through to DSL. + +**3j heads-up — the diagnostics keyed on roles will keep working:** +the 3i passes match on roles (`insert_target_table`, `insert_column`, +etc.) that come from the SQL shapes, so they fire only when the SQL +shape matched. A DSL insert (different roles) won't trip them. But +**re-run the full diagnostic test set after 3j** — the dev-word → +real-word switch changes how the path is built (the entry-word offset +/ no keyword-prefix reconstruction), and the ast-builders' `source` +handling changes. + +**Tests that hard-code the dev words** must be migrated in 3j: +`tests/sql_insert.rs`, `tests/sql_update.rs`, `tests/sql_delete.rs` +use `sqlinsert …` / `sql_update …` / `sql_delete …` inputs and parse +helpers; the walker tests in `mod.rs` use `sqlinsert …` inputs. After +3j they become `insert …` / `update …` / `delete …` (Advanced mode). +This is a sizeable but mechanical migration — consider an Explore/ +general-purpose sub-agent for the input-string swaps, compile-verified. + +## §5. Sub-phase 3k — verification sweep (after 3j) + +Per the plan: fill the cross-cut verification matrix, produce the +phase-exit verification report at `docs/handoff/-phase-3- +verification.md`, final DA review (genuine, not rubber-stamp), confirm +zero failures / zero skips / no regression from the Phase-2 baseline +(1446/0/1 — note this session's count is 1613 and climbing as tests +were added per sub-phase). Clippy clean. + +## §6. Decisions settled this session (do not re-litigate) + +- **3f cascade = count-diff parity** (ADR-0033 **Amendment 2**), + user-confirmed. SET NULL reporting **deferred for + both DSL and SQL paths** (preserves parity); a future enhancement + touches both `do_delete` and `do_sql_delete` together. +- **`returning: bool`** on the Command variants (ADR §5), not a + text field — the auto-fill rewrite preserves the trailing tail + instead. +- **`excluded` scoping = byte-range** (not the plan's TableBinding + push) — recorded as a mechanism deviation; behaviour matches §9. +- **History journal = ADR-0034** (written, **Accepted**): see §7. +- **not_null = `c.notnull` only** (not `|| primary_key`) — int-PK + false-positive avoidance. + +## §7. Tracked deferred items (nothing lost) + +These are **parked, not dropped** — pick up after Phase 3 (3k) per the +user's "we'll take care of the extra tasks when phase 3 is complete". + +- **TASK #8 — Implement ADR-0034 (history journal).** `history.log` + is success-only, but the in-memory Up/Down recall ring records every + submission, and the ring is re-seeded from `history.log` on open — + so failed commands are recallable in-session but **lost across + sessions**. ADR-0034 (`docs/adr/0034-history-journal-and-replay- + filter.md`, **Accepted**): make `history.log` a complete journal + tagged `ok`/`err`; hydration reads all, replay reads `ok` only. + Code = two sub-tasks (journal-failures+filtering; replay-format). + Successful commands stay journalled transactionally by the worker; + failures journalled `err` best-effort from the runtime/app error + path (a parse failure never reaches the worker). Existing all-`ok` + logs need no migration. +- **TASK #9 — Fix broken replay + add the missing test.** `run_replay` + (`src/runtime.rs`) parses each whole trimmed line through the DSL + parser with NO understanding of the `||` format, + so `replay history.log` fails on line 1 — despite ADR-0006 / the + `resolve_replay_path` comment promising it works. **No test feeds + the pipe format to replay** (the `replay_history_log_records_ + subcommands_only` test only checks what replay *writes*). Fix: + `run_replay` accepts BOTH a journal record (extract+unescape source, + skip unless `status==ok`) and a bare command line; detection by the + leading timestamp+status prefix. Test-first with a real + `history.log`. + +### Residuals / observations (lower priority, all discussed) + +- **#12 residual is CLOSED** — DO UPDATE SET-RHS / WHERE bare refs are + now validated against the target (the `/runda` round). Task #12 is + marked completed. +- **`excluded` completion soft-leak:** completion offers the target's + columns for `excluded.|` even outside DO UPDATE (the diagnostic + pass strictly scopes it; completion is the softer surface). Untested, + advisory, acceptable. +- **Old-project migration** (from handoff 30 §4): the pre-`check_expr` + 3-column `__rdbms_playground_columns` schema; deferred to the + migration framework (ADR-0015 Iter 6). +- **Two-line hint box** (ADR-0022 Amendment 2) — deferred. +- **`writes_user_listed_column` flag** stays `false` (handoff 31 §7) — + VALUES-against-listed-columns completion narrowing, nothing needs it. + +## §8. Process pins (unchanged, still binding) + +- **Confirm every commit.** Propose the message; wait for the + go-ahead. **No auto-commit at sub-phase gates** (this overrides the + plan's standing commit pre-authorization — the user confirmed each + commit this whole session). +- **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". The `/runda` round is + the model: probe → confirm fail → fix → keep the test. +- **Core walker changes** (`walk_seq` / `walk_choice` / + `walk_repeated`) need explicit user OK before coding. (New *node + variants* and new *post-walk passes* do not — those were added + freely this session.) +- **Escalate ambiguity / ADR-mechanism mismatches; never classify + work out of scope without user confirmation.** Resolve mechanism + changes by ADR amendment BEFORE coding (3f, 3h, history all did). +- **`/runda`** (the extra-DA-round skill) is a user-invoked tool — do + not invoke it yourself, but internalise its lesson: re-check + cross-cut interactions. + +## §9. How to take over + +1. **Read, in order:** this file → + `docs/adr/0033-sql-dml-grammar.md` (esp. **Amendment 1** — + dispatch — and **Amendment 2** — cascade) → + `docs/plans/20260520-adr-0033-phase-3.md` "Sub-phase 3j" + "3k" → + `docs/adr/0034-history-journal-and-replay-filter.md` (parked) → + `CLAUDE.md`. +2. **Baseline:** + ``` + cargo test # expect 1613 passing / 0 failing / 1 ignored + cargo clippy --all-targets -- -D warnings # clean + ``` +3. **Start 3j** per §4. The dispatch machinery (Amendment 1, + category-grouped) already exists and is how `select`/`with` work — + 3j adds the SQL `insert`/`update`/`delete` nodes as `Advanced` + alongside the `Simple` DSL nodes, removes the dev words, simplifies + the three `build_sql_*` builders, de-dups completion entry-word + lists, and migrates the dev-word test inputs. Expect a sizeable but + mechanical test migration (the three `tests/sql_*.rs` + walker + tests hard-code `sqlinsert`/`sql_update`/`sql_delete`). +4. **Re-run the full diagnostic suite after 3j** (§4 heads-up) — the + path-shape change is the risk. +5. Then **3k** (verification sweep + phase-exit report). +6. **Escalate** anything not settled in ADR-0033 / the plan; the user + wants mechanism mismatches surfaced, not silently fixed.