diff --git a/docs/handoff/20260529-handoff-51.md b/docs/handoff/20260529-handoff-51.md new file mode 100644 index 0000000..a63bcfd --- /dev/null +++ b/docs/handoff/20260529-handoff-51.md @@ -0,0 +1,289 @@ +# Session handoff — 2026-05-29 (51) + +Fifty-first handover. **Continuation of handoff-50's bug-fix loop.** +The user steered this session twice with strategic judgement that +proved correct both times (see §2). Three issues resolved — two bug +fixes and one substantial cross-mode unification — plus two follow-up +trackers spawned from #6's earlier /runda pass were tackled (#18 +closed benign-but-hardened, #17 fixed). **ADR-0022 gained Amendment 3; +ADR-0036 gained Amendment 2.** + +## §1. State at handoff + +**Branch:** `main`. **HEAD `10e5197`.** **Tests: 2051 passing, 0 +failing, 0 unexpected skips, 1 pre-existing ignored** (the same +ignored doctest as prior baselines). **Clippy: clean.** Nothing +pushed (per the user's rules — push is the user's step). + +Commits since handoff-50's `abc9bb6`: + +``` +10e5197 feat: bring simple-mode insert arity diagnostics to parity with advanced +7cccf4e refactor: make completion-drop gate schema-aware for consistency +fa5d0dc fix: insert VALUES between-values hint points at comma not close-paren +``` + +Test count moved 2040 → 2051 (+11 net across the three commits). + +## §2. The two user steers that shaped the session + +This session is worth reading for **how the user's judgement +redirected the work** — both calls were right and both uncovered real +problems a naive pass would have missed. + +1. **"Most of the open issues were reported at the same time — the + fixes we already applied may have changed the outcome of other + reports."** This re-validation instinct was correct. Acting on it, + every issue picked up got a *confirm-it-still-reproduces* first + step. Result: **#2's reported root cause was wrong** (the report + blamed string literals; it was not literal-type-specific at all), + and **#18 turned out benign** (the divergence it described cannot + reach the code path it gates). Neither would have surfaced without + re-probing against current HEAD first. **This is now the standing + agreement: the first step of every bug pickup is to reproduce it on + current code before designing a fix.** + +2. **On #17, the user rejected the "easy way out."** When implementation + of the chosen Approach A revealed a larger blast radius, the lead + floated reverting to the lighter Approach B. The user pushed back: + *"this feels like mistakes were made in the past which resulted in + this discrepancy, and approach B is the easy way out … I'd rather + stick to A and get this sorted really cleanly — we're still in the + original dev phase, it'd be good to create a strong basis for future + maintenance, even if it means more work now."* This was **exactly + right**: there genuinely was a past discrepancy (issue #1's + simple-mode submit teaching note was a *workaround* for the very gap + #17 names), and Approach A — done properly — unified both modes onto + one model rather than piling a third bespoke surface on top. + Sticking with A also surfaced (and closed) a genuine + **execution-safety regression** that the lighter path would have left + buried. + +## §3. Resolved issues + +### #2 — VALUES between-values hint (commit `fa5d0dc`) + +**Re-characterised on investigation.** The report's hypothesis (per- +column inference abandoned *when the first literal doesn't match the +first column's type role*) was **wrong** — probing `values ('Oli'`, +`values (42`, `values (3.5` all reproduced identically. Not literal- +type-specific. + +**Real root cause:** the ambient-hint ladder's bottom rung +(`ambient_hint_core_in_mode`, `src/input_render.rs`) parsed +**schemalessly** (`parse_command_in_mode`) while every higher rung was +already schema-aware. Type-blind, the grammar closes an `insert … +values (…)` tuple after one value, so the "Next:" hint pointed at `)` +when the schema-aware walk (knowing the remaining columns) expects `,`. +The same schemaless parse also reported wrong-arity *closed* tuples as +complete → the hint read "submit with Enter" for an input the real +parse rejects. + +**Fix:** the fallback rung now calls +`parse_command_with_schema_in_mode(input, cache, mode)`. One-line +change (the cache was already in scope). Verified the friendly arity +diagnostic is **not masked** — in advanced mode it fires at a higher +rung and still wins (guard test +`advanced_mode_wrong_arity_insert_keeps_friendly_diagnostic_over_fallback`). + +Three `typing_surface` snapshots re-baselined (form_a one-value +`Next:` `)`→`,`; form_b too-few and form_c wrong-count: the misleading +"Submit with Enter" → the accurate parse error). **ADR-0022 +Amendment 3** records the schema-aware fallback. + +### #18 — completion-drop schemaless gate (commit `7cccf4e`, closed benign-but-hardened) + +The ticket flagged `completion.rs:336`'s `input_parses_complete` gate +using the schemaless parse (sibling of #2). **Investigated as the +ticket asked: it's benign with the current grammar.** The gate drives +exactly one thing — dropping a candidate that *exactly equals* the +trailing partial (don't re-suggest `pk` at the end of `create table T +with pk`). The schemaless/schema-aware parse *does* diverge (e.g. +`insert into T values (1, 1, 'x')` — int into a bool column), but the +divergence is confined to **value type/arity**, while the drop only +ever removes a re-offered **keyword** == partial — and those positions +never coincide. Probed every plausible co-occurrence; candidate output +was identical with and without the fix. + +**Disposition (user chose "keep as hardening"):** applied the one-line +schema-aware fix anyway (matches #2, strictly safe — can only ever +*retain* a candidate the schemaless gate would have dropped). No +behavioural test possible (no observable bug); the suite staying green +is the regression cover. Rationale recorded in a code comment. + +### #17 — simple-mode arity diagnostic parity (commit `10e5197`) + +**The substantial one.** Simple-mode wrong-count inserts showed a bare +`expected `,`/`)`` while advanced mode showed a friendly *"N column(s) +… M value(s)"* message. Root cause: simple `insert` and advanced +`insert` are different grammar nodes (ADR-0033 Am1). Advanced routes a +wrong-count tuple to a type-blind fallback (`tuple_value_list`) so it +**structurally matches** and `dml_insert_arity_diagnostics` fires; the +simple DSL value list (`column_value_list`) built a fixed-length typed +`Seq`, so a wrong-count tuple **Mismatched** and the diagnostic never +ran. + +**Fix — unified onto the one ADR-0027 model (structural parse + ERROR +diagnostic), advanced left byte-for-byte unchanged:** + +1. **Grammar:** new `dsl_insert_value_list` gate (`data.rs`) routes a + wrong-count DSL insert tuple to the shared `FALLBACK_VALUE_LIST` so + it matches and the diagnostic fires. **Gated to `Mode::Simple`** — + in advanced the DSL node stays strict (otherwise a non-SQL shape + like Form C `insert into T (1,2)` would spuriously match and be + accepted in advanced; `sql_insert.rs` owns advanced inserts). + `count_tuple_values` (moved to `shared.rs`) and a new + `insert_target_columns` (extracted from `column_value_list`) are now + **shared** by both grammars so the gate and the typed slots never + disagree. +2. **Diagnostic is mode-aware:** `dml_insert_arity_diagnostics` gained a + `mode` param. Advanced Form B expects **all** columns (auto-fills + nothing); simple Form B/C expects the **user-fillable** columns + (`serial`/`shortid` auto-fill, ADR-0018 §3). It now also counts the + DSL Form A role (`insert_first_item`, distinct from SQL + `insert_column`) and scans the keyword-less Form C tuple. New + catalog keys `diagnostic.insert_arity_mismatch_form_b_simple` (names + the fillable *and* the auto-generated columns) and + `diagnostic.insert_arity_mismatch_all_auto`. +3. **Submit safety (the regression that A surfaced):** a wrong-count + DSL insert now parses `Ok` + carries the ERROR diagnostic, so it + would otherwise have **dispatched and executed**. A unified Ok-arm + pre-flight (`dsl_insert_count_mismatch_notes`, `input_render.rs`) + now **blocks `ExecuteDsl`** and shows the teaching note — mirroring + the advanced Ok-arm pre-flight. The issue #1 Err-arm note retires. + `advanced_alternative_note`'s gate now reads the validity verdict + (not just `DefiniteErrorAt`) so the cross-mode pointer still fires + for the new parse-Ok-with-ERROR shape, but **only** when the line is + genuinely valid in advanced (Form B 3-val → pointer; Form C / too-few + → no pointer). + +**Scope guard (important for future readers):** this is *arity- +diagnostic UX parity only*. It does **not** consolidate value-handling, +execution, or `serial`/`shortid` auto-fill across modes — ADR-0036's +deliberate mode-distinctness stands. The per-mode count difference is a +*consequence* of the auto-fill difference, not a violation of it. + +**Documented as ADR-0036 Amendment 2** (cross-ref ADR-0033 §8.1) + +README index + requirements.md H1a. + +## §4. ADR / docs work + +- **ADR-0022 Amendment 3** (in `fa5d0dc`): the ambient-hint fallback + rung is now schema-aware. Records that Amendment 1's listing of + `parse_command_in_mode` as the fallback was an oversight (every other + rung was schema-aware), and that the friendly arity diagnostic still + wins at its higher rung. README index updated same-edit. +- **ADR-0036 Amendment 2** (in `10e5197`): simple-mode arity-diagnostic + parity (full detail in §3 #17). README index updated same-edit. +- **requirements.md H1a** gained citations for both #2 and #17 + contributions. +- **#18** is a code-comment-only decision (benign hardening); no ADR. + +## §5. What's open + +### Bug reports still open (filed handoff-50) + +| # | Title | Label | +|---|---|---| +| [7](https://github.com/oliversturm/rdbms-playground/issues/7) | Advanced mode: `explain` not yet supported | enhancement | +| [8](https://github.com/oliversturm/rdbms-playground/issues/8) | Advanced-mode syntax highlighting: identifiers and type keywords share the teal colour | bug | +| [9](https://github.com/oliversturm/rdbms-playground/issues/9) | `[ok] explain