diff --git a/docs/adr/0030-advanced-mode-sql-surface.md b/docs/adr/0030-advanced-mode-sql-surface.md index 71e6267..0df6535 100644 --- a/docs/adr/0030-advanced-mode-sql-surface.md +++ b/docs/adr/0030-advanced-mode-sql-surface.md @@ -157,17 +157,18 @@ that `docs/simple-mode-limitations.md` records as the inverse of the simple-mode subset. The DSL `Expr` is the *DSL's* representation; the SQL surface does not round-trip through it. -> **Forward note (2026-05-26).** The **Proposed ADR-0036** narrows the -> "DML → validated SQL text" half of this section: **literal data -> values** in `INSERT`/`UPDATE` should be parsed to typed `Value`s, -> validated, and bound as parameters (as the DSL already does), not -> handed to the engine as text — so advanced mode gets the same -> value-validation, hinting, highlighting, and error context as simple -> mode. The `SELECT` half and the full-expression-surface rationale above -> **stand**: expressions and queries remain verbatim text (ADR-0026's -> limited `Expr` is *not* imposed on the SQL surface). The dividing line -> is "bindable literal vs engine-evaluated expression-or-query." Pending -> acceptance. +> **Forward note (2026-05-26, ADR-0036 Accepted).** ADR-0036 **augments** +> the "DML → validated SQL text" half of this section — it does **not** +> change the execution model. Advanced-mode DML still executes verbatim; +> what's added is that "validated" now also means *value*-validated: the +> **literal data values** in `INSERT`/`UPDATE` are checked against the +> playground type system (and retained for error reporting) **before** the +> verbatim statement runs, sharing the DSL's per-type validators. No +> binding, no reconstruction, no command-identity change (ADR-0033 Am3 +> stands). The `SELECT` half and the full-expression-surface rationale +> above **stand**: expressions and queries remain verbatim text (ADR-0026's +> limited `Expr` is *not* imposed on SQL). Dividing line: a static literal +> value (validate it) vs an engine-evaluated expression-or-query. ### 5. Type vocabulary — the playground's, not the engine's diff --git a/docs/adr/0033-sql-dml-grammar.md b/docs/adr/0033-sql-dml-grammar.md index cb2c5cc..ce2759b 100644 --- a/docs/adr/0033-sql-dml-grammar.md +++ b/docs/adr/0033-sql-dml-grammar.md @@ -10,15 +10,19 @@ for the phase-exit report and the filled cross-cut matrix in `docs/plans/20260520-adr-0033-phase-3.md`. Amendments 1–3 below are part of this acceptance. -> **Forward note (2026-05-26).** The **Proposed ADR-0036** narrows the -> §10 verbatim-execution model for **literal data values**: rather than -> handing the user's literal text to the engine, `INSERT`/`UPDATE` should -> parse literal values to typed `Value`s, validate them, retain them on -> the command (for binding *and* error enrichment), and bind them as -> parameters — as the simple-mode DSL already does. Expressions, `WHERE` -> predicates, `INSERT … SELECT`, `RETURNING`, UPSERT, and `SELECT` stay -> verbatim text. The `Sql*` command variants and worker handlers remain; -> only the literal-value execution path changes. Pending acceptance. +> **Forward note (2026-05-26, ADR-0036 Accepted).** ADR-0036 **augments** +> the §10 model — it does **not** change verbatim execution and does +> **not** collapse the two-command identity of **Amendment 3** (which +> stands). It adds a *value-validation* step: `INSERT`/`UPDATE` literal +> values are parsed to typed `Value`s, **validated** against the column +> type (sharing the DSL's validators), and **retained** on the command +> (so a constraint error can show the offending value) — then the +> statement still executes **verbatim**. `Command::SqlInsert` gains a +> captured-literals payload; its execution and `plan_shortid_autofill` +> are unchanged. Expressions, `WHERE`, `INSERT … SELECT`, `RETURNING`, +> UPSERT, and `SELECT` are untouched. (The serial/shortid auto-fill +> difference vs simple mode is tracked separately as `requirements.md` +> X4.) ## Context diff --git a/docs/adr/0036-typed-dml-values-vs-verbatim.md b/docs/adr/0036-typed-dml-values-vs-verbatim.md index 36f482d..cbd59f7 100644 --- a/docs/adr/0036-typed-dml-values-vs-verbatim.md +++ b/docs/adr/0036-typed-dml-values-vs-verbatim.md @@ -1,31 +1,48 @@ -# ADR-0036: DML execution — bind literal values, reserve verbatim text for expressions and queries +# ADR-0036: Value validation for advanced-mode DML — validate literals, keep execution and identity mode-specific ## Status -**Proposed** (design agreed with the user in conversation, 2026-05-26; -`/runda` verification pass completed 2026-05-26 — three factual/precision -corrections applied to the evidence + matrix, and three design forks -confirmed with the user: per-position classification with -`RETURNING`/`ON CONFLICT` as clauses over bindable `VALUES`; the -bindable-literal set including signed numerics, `NULL`, and boolean/string -literals; and `WHERE` left as text deliberately. Pending the user's -go-ahead to implement). +**Accepted** (design agreed with the user in conversation, 2026-05-26; +`/runda` verification pass completed 2026-05-26; the mechanism was then +**deliberately narrowed** during the same conversation — see below — from +"bind literal values through the DSL's path" to the surgical +**"validate-and-retain, execute verbatim"** after the user pushed back on +consolidating the two modes and a concrete auto-fill difference confirmed +that even the single-row literal case is **not** identical across modes). +**Phase 1 implemented 2026-05-26** (`INSERT … VALUES` literal validation + +offending-value retention; capture-at-parse, no grammar change, execution +unchanged). Phases 2 (`UPDATE … SET` literals) and 3 (completion +hinting/highlighting) pending. -**Supersedes**, in part, **ADR-0030 §4** ("DML and `SELECT` → executed -as the validated SQL itself … modelling them as a typed `Command` buys -nothing") and the **execution model of ADR-0033 §10** (the three -`Sql{Insert,Update,Delete}` variants executing the user's text -verbatim). It **narrows** those decisions rather than reversing them: -the `SELECT` half of ADR-0030 §4 stands unchanged, and verbatim text -execution remains correct — and necessary — for everything that is an -**expression or a query**. What changes is that **literal data values** -stop being handed to the engine as text and instead flow through the -same typed, validated, parameter-bound path the simple-mode DSL has used -all along. +**Augments** **ADR-0030 §4** and **ADR-0033 §10** — it does **not** +supersede them and does **not** change the execution model. Advanced-mode +DML still executes the validated SQL **verbatim**; ADR-0033 Amendment 3's +two-command identity (`Command::Insert` vs `Command::SqlInsert`) **stands +unchanged**. What this ADR adds is a **value-validation step**: the word +"validated" in "executed as the validated SQL itself" (ADR-0030 §4) is +extended to mean *value*-validated, not merely *syntactically* validated — +the literal data values in an advanced-mode `INSERT`/`UPDATE` are checked +against the playground type system (and retained for error reporting) +before the statement runs. Builds on the ADR-0035 precedent (DDL executes *structurally*, not -verbatim) and completes the thought: structure was the first place -"grammar as text" broke; literal data values are the second. +verbatim): there, structure was the first place "grammar as text" was too +broad. This ADR makes a **narrower** correction for DML — not to *how* it +executes, but to *what gets checked* before it does. + +**Conversation note (the principle this records).** The first instinct was +to *consolidate* — bind literals via the DSL path, even emit +`Command::Insert` from the advanced surface. That was rejected, for a +reason worth preserving: simple- and advanced-mode commands are kept +distinct **because they can legitimately differ**, and they do — e.g. +auto-fill: simple-mode `do_insert` fills an omitted non-PK `serial` with +`MAX(col)+1`, advanced-mode does not (`requirements.md` **X4**, flagged as +a possible bug to investigate separately). Collapsing the commands would +silently drag in such differences. The durable principle (also +`requirements.md` **X5**): **keep a distinct command per distinct case; +share execution *mechanics* as library helpers, never by fusing command +identity.** This ADR shares exactly one mechanic — the per-type value +validators — and nothing else. ## Context @@ -129,198 +146,176 @@ type recovery — entirely from **walking the validated parse**, with the engine executing the text. Queries have no data values to validate against columns; owning them buys nothing and costs enormously. -So the dividing line is **not** "DDL vs DML." It is **bindable literal -vs engine-evaluated expression-or-query.** +So the dividing line is **not** "DDL vs DML." It is **a static literal +value (which we can validate) vs an engine-evaluated +expression-or-query (which we cannot).** ## Decision ### 1. The principle -> **Own structure and literal values; reserve verbatim text for -> expressions and queries.** -> -> Classification is **per value position**, not per statement. Wherever a -> value position holds a **bare literal**, parse it to a typed `Value`, -> validate it against the playground type system, surface the column type -> in completion, highlight a mismatch, and execute it as a **bound -> parameter** — exactly as the simple-mode DSL does. Wherever the -> position holds a **computed expression** (`qty*2`, a function call, a -> scalar subquery), or the value *source* is itself a query -> (`INSERT … SELECT`), hand the validated text to the engine — because -> that is where text-to-engine is necessary, not merely convenient. +> **In advanced-mode `INSERT`/`UPDATE`, validate each literal data value +> against its target column's type before executing, and retain the +> literal so a constraint error can name it. Execute the statement +> verbatim, exactly as today. Do not bind, do not reconstruct, do not +> touch auto-fill, do not collapse command identity.** -The test for which side a position falls on is operational: **can this -value be bound as a parameter?** A literal can; a computed expression or -a subquery cannot — the engine must evaluate it. +Only the **value validation** is shared between simple and advanced mode — +via the existing per-type validators (`Value::bind_for_column` / +`validate_date` / `shortid::validate`). Everything else stays +mode-specific: execution is still verbatim text-to-engine, +`plan_shortid_autofill` is untouched, and `Command::SqlInsert` / +`Command::SqlUpdate` remain distinct from their DSL counterparts. -**What counts as a literal** (the bindable set — matching the DSL lexer's -`consume_number_literal` / `consume_string_literal` and the -`null`/`true`/`false` words): `NULL`, a boolean literal, a string -literal, and a **signed** numeric literal (`-5`, `3.14`). A signed -numeric must count as a literal *even if* the grammar would otherwise -read `-5` as unary-minus-of-`5`, or negative numbers would silently skip -validation. Anything else in a value position — arithmetic, function -calls, `CASE`, subqueries, column references — is an expression and stays -text. +**What counts as a literal** (the set we validate — matching the +`null`/`true`/`false` words plus number/string literals as the walker +tokenises them): `NULL`, a boolean literal, a string literal, and a +**signed** numeric literal (`-5`, `3.14`). A signed numeric counts as a +literal even though `sql_expr` tokenises the sign separately (`Punct('-')` +then `NumberLit`) — a leading sign at the start of a value position is +part of the literal, not an operator. Anything else in a value position — +arithmetic, function calls, `CASE`, subqueries, column references — is an +**expression**: there is no static value to validate, so it is left to the +engine (unchanged). -**Clauses are not value sources.** `RETURNING` and `ON CONFLICT …` are -clauses *on* a statement, not value positions. An -`INSERT INTO T VALUES (1, 'x') RETURNING id` or `… ON CONFLICT DO UPDATE -…` still has **bindable literal `VALUES`**; the clause rides on the -generated statement (and its own expressions — e.g. `excluded.x` in a -`DO UPDATE SET` — stay text). Only when the value *source* is a query -(`INSERT … SELECT`) is there nothing to bind. +**Why not bind / converge.** Binding was the *first* instinct and is +**rejected**. The two proven gaps (a malformed literal slipping through; +the offending value missing from errors) are closed by **validation + +retention alone** — binding adds nothing to either. Meanwhile, executing +the user's *own* text verbatim is already safe (their quoting stands; no +re-quoting risk because we do not reconstruct), and binding/convergence +would risk dragging in genuinely mode-specific behaviour (auto-fill — X4; +natural-order column mapping) that must stay separate. So we share the +validators and nothing else. This keeps the modes cleanly apart +(`requirements.md` X5) while fixing the bug that they should *not* differ +on: whether a learner's malformed value is caught. ### 2. What this means per statement -- **`INSERT … VALUES`** — each value position is a literal *or* an - expression. Literal positions are typed/validated/bound; expression - positions stay text. A row that is all literals executes exactly like - the DSL `do_insert` (`VALUES (?1, ?2, …)`); a row mixing the two - generates `VALUES (?1, a+1, ?2)`. Multi-row `VALUES` repeats this per - row. A trailing **`RETURNING`** or **`ON CONFLICT …`** clause is - carried on the generated statement — the bound `VALUES` are unaffected; - the clause's own expressions stay text. -- **`UPDATE … SET`** — `SET col = ` is typed/validated/bound; - `SET col = ` stays text. -- **`WHERE` (UPDATE/DELETE)** — **stays text, deliberately**, even for a - simple `WHERE id = 5`. `WHERE` is an expression in general; keeping it - text preserves the full SQL surface (ADR-0026's `Expr` is the *DSL's* - representation and is not imposed on SQL), and the value-feedback - motivation is already met by `VALUES`/`SET` — a constraint error names - a *written* value, not a predicate operand. This is a scope choice, not - an oversight; per-position binding is **not** applied to `WHERE`. -- **`DELETE`** — no data values to bind; `WHERE` per the rule above. -- **`INSERT … SELECT`** — entirely text: the value *source* is a query, - so there are no literals to bind. -- **`SELECT`** — unchanged. Text-to-engine, feedback-from-walk - (ADR-0032). Explicitly **not** lowered to a typed AST. +Execution is **unchanged** for every statement below; the only addition is +a pre-execution validation of literal value positions. -### 3. What it fixes — one change, four faces +- **`INSERT … VALUES`** — every literal position (single- or multi-row) is + validated against its column type before the verbatim insert runs; a + malformed literal is refused with the same friendly wording the DSL uses + (shared `bind_for_column`). Expression positions are skipped (nothing to + validate). `RETURNING` / `ON CONFLICT` / `INSERT … SELECT` need no + special handling — validation simply applies to whatever literal + `VALUES` are present, and the statement still executes verbatim. +- **`UPDATE … SET`** — `SET col = ` is validated; `SET col = + ` is skipped. (Phase 2 — see §5.) +- **`WHERE` (UPDATE/DELETE)** — **not** validated. `WHERE` is an + expression in general; the value-feedback motivation is met by + `VALUES`/`SET` (a constraint error names a *written* value). Deliberate + scope choice, not an oversight. +- **`SELECT`** — entirely unchanged. No data values to validate. -Retaining the typed literal (a) lets validation run on it (the proven -gap), (b) lets completion hint the column type and highlighting flag a -mismatch, (c) lets the worker **bind** it (safe, type-correct), and (d) -leaves it **on the command** so `enrich_*` can show the real value in a -constraint error. The neutral "that value" safety net (ADR-0035 -Amendment 1) remains correct for the genuinely-computed cases — there is -no input literal to show, and that is the honest answer. +### 3. What it fixes -### 4. Explicit requirement — carry the typed literals +Validating the literal closes the **validation gap** (the malformed `date` +`2025/01/15` is now refused in advanced mode, as proven by the +characterization test). Retaining the literal on the command closes the +**error-value gap** (`enrich_*` reads it, so a constraint error shows the +real value instead of the neutral "that value"). Completion **hinting / +highlighting** is **not** delivered here — it needs a grammar-level change +(§5, Phase 2). The neutral "that value" safety net (ADR-0035 Amendment 1) +remains correct for genuinely-computed expression values — there is no +input literal to show. -`SqlInsert`/`SqlUpdate` (or their successors) **must retain the parsed -typed literal values** (per column position), not only the raw text, so -that both the executor (binding) and the error enricher (display) can -use them. This is the concrete hinge on which faces (a), (c), and (d) -turn; it must not be left implicit. +### 4. Explicit requirement — retain the literals, change nothing else -The command **also** keeps the user's **original source text**: that is -what `history.log` records and what `replay` re-runs (ADR-0034), so the -journal stays faithful to what the user typed even though execution now -binds the literals from a generated statement. Two representations, one -command: original text for the journal, parsed literals for execution + -feedback + errors. +`Command::SqlInsert` (and later `SqlUpdate`) **gains a captured-literals +payload** (per row, per position; `None` for an expression position) in +addition to the existing raw text. The executor validates from it and the +error enricher reads it. The original source text is **unchanged** and is +still what `history.log` records and `replay` re-runs (ADR-0034). The +command variant, its execution, and `plan_shortid_autofill` are **not** +modified. Validation reuses the existing value-binding helper +(`impl_value_for` / `Value::bind_for_column`) for wording parity with the +DSL — the resulting bound value is **discarded** (we do not bind for +execution), only its `Result` is used. -### 5. Mechanism (recommended, with the alternative recorded) +### 5. Mechanism + phasing -- **Recommended — typed-slot hybrid grammar.** At each DML value - position, the SQL grammar offers `Choice(typed-literal-slot, - sql_expr)`: the typed slot (reusing the DSL's live `column_value_list` - / per-type `TypedValueSlot`s — `data.rs:141`/`189`/`269`, `shared.rs`) - captures + validates + retains the literal and drives - completion/highlighting; `sql_expr` is the fallback for a genuine - expression. This is the only - option that delivers **live completion hints** for SQL DML values, - because the hint comes from the grammar walk. -- **Alternative — post-parse semantic pass.** Keep the grammar as - `sql_expr` and add a pass that, for positions that are bare literals, - validates + retains them. Simpler; delivers validation, retention, and - the error-value fix, but **not** live completion hinting. Recorded as - the fallback if the hybrid grammar proves too costly in the walker. +- **Phase 1 (this ADR's immediate work) — capture + validate + retain.** + At parse, `build_sql_insert` classifies each `VALUES` position from the + matched path (a single literal token, or a signed number → a typed + `Value`; anything else → an expression marker) and stores the per-row + result on the command — **no grammar change, no reparse**. The executor + validates the captured literals against the resolved column types before + the verbatim insert; the enricher reads them. Covers single- and + multi-row, with or without `RETURNING`/`ON CONFLICT`, because execution + is untouched. +- **Phase 2 — `UPDATE … SET` literal validation** (same technique on the + SET assignment list). +- **Phase 3 — completion hinting / highlighting.** This is the *only* + part that needs a grammar change: a `Choice(typed-literal-slot, + sql_expr)` at each value position (reusing the DSL's live + `column_value_list` / `TypedValueSlot`s — `data.rs:141`/`189`/`269`), + so the column type drives a live hint and a mismatch highlights while + typing. When Phase 3 lands, the typed slot supersedes Phase 1's + classification of literals (the validation/enrichment built on top is + unaffected — that is the only throwaway, by design). -### 6. Phased implementation +### 6. Non-goals -1. **Literal `INSERT … VALUES`** — including the `RETURNING` and - `ON CONFLICT …` variants (they exist today and the shortid-autofill - path already carries those trailing clauses, so excluding them would - be the artificial split). The common case; converges execution on the - DSL bound path; closes the proven validation gap **and** the - error-value display in one move. Highest value, smallest blast radius. -2. **`UPDATE … SET` literals** (bind literal assignments; expression - assignments stay text). -3. **Completion/highlighting** for the typed slots on the SQL surface - (if not already delivered by the mechanism chosen in §5). - -`SELECT` and `INSERT … SELECT` (and every *expression* value position) -remain on the text side throughout — **by design and documented**, not by -omission. `RETURNING` / `ON CONFLICT` are not on this list: they are -clauses over a statement whose literal `VALUES` still bind (§1/§2), and -Phase 1 carries them. - -### 7. Non-goals - -- **A structural `SELECT`.** Not now, not as a consequence of this ADR. - Queries stay text. (If a future need ever forces `SELECT` toward an - owned AST, that is its own ADR with its own justification — flagged as - a watch-item, not a plan.) -- **A full typed SQL-expression AST.** Expressions stay validated text; - ADR-0031's "no `Expr` AST" stands. We do not impose the DSL's limited - `Expr` on the SQL surface (that would regress ADR-0030 §4's full-surface - guarantee). -- **Removing the verbatim path.** It remains the executor for every - expression and query position; this ADR narrows *where* it applies, it - does not delete it. +- **Binding / statement reconstruction.** Explicitly out. Execution stays + verbatim. (This was the rejected first instinct.) +- **Collapsing command identity.** `Command::Insert` and + `Command::SqlInsert` stay distinct; **ADR-0033 Amendment 3 stands**. +- **Changing auto-fill.** The simple-vs-advanced `serial`/`shortid` + auto-fill difference (`requirements.md` X4) is **untouched** here and + tracked separately as a possible bug. +- **A structural `SELECT`** and **a full typed SQL-expression AST** — + both out (queries and expressions stay text; ADR-0031's "no `Expr` AST" + and ADR-0030 §4's full-surface guarantee stand). ## Consequences - **Advanced mode stops being a feedback-free zone for data values.** A learner typing a malformed `date`/`shortid`/`int` literal in a SQL - `INSERT` gets the same catch-and-explain they get in simple mode. -- **One model, fewer paths.** Literal DML converges on the DSL's proven - parameter-binding execution; the two surfaces stop diverging on the - case they share. -- **The full SQL expression + query surface is preserved**, because - expressions and `SELECT` are explicitly left on the text side. No - regression to the advanced surface ADR-0030 §4 protects. -- **Rework, not rewrite.** ADR-0033's `Sql*` command variants and the - worker handlers stay; they gain typed-literal payloads and the literal - execution path changes. Phased so each step is independently shippable - and testable, with the existing ADR-0033 tests as the regression net. -- **A new seam to keep honest:** the literal-vs-expression boundary in - the grammar. It must be explicit and tested (a position that is a bare - literal binds; a position that is any expression stays text; a signed - numeric is a literal; `NULL`/`true`/`false` are literals), or it will - drift. -- **Execution reconstruction is the load-bearing implementation - challenge.** A row mixing literals and expressions must be re-emitted - as `VALUES (?1, expr-text, ?2)` — bound placeholders spliced with the - original expression spans, with any `RETURNING`/`ON CONFLICT` tail - re-appended. This is the strongest argument for the **typed-slot - grammar** mechanism (§5): it yields per-position structure (slot → - bind; `sql_expr` → text span) directly, whereas the post-parse pass - must recover those spans itself. The existing shortid-autofill rewrite - (`plan_shortid_autofill`) already does statement reconstruction with - bound params + a preserved trailing clause, so the pattern is not - new — but it must be got right or a literal silently becomes text. -- **Cost if we are wrong about `SELECT`.** If a future requirement shows - `SELECT` genuinely needs an owned AST, this ADR does not help with it — - but nor does it obstruct it. The boundary chosen here (queries = text) - is the current best line, taken with eyes open. + `INSERT` gets the same catch-and-explain they get in simple mode — + via the *shared validator*, not a shared command. +- **The modes stay cleanly separate.** Execution, auto-fill, and command + identity are all unchanged; the only thing now shared is the value + validators. This is the `requirements.md` X5 principle in practice + (share a mechanic, not a command) and avoids the consolidation traps + (X4 auto-fill) that the bind/converge approach would have hit. +- **Small, low-risk, no execution reconstruction.** Because we do not + rebuild the statement, there is no "mixed `VALUES (?1, expr, ?2)`" + splicing problem, no multi-row execution change, and no `RETURNING`/ + `ON CONFLICT`/`INSERT … SELECT` special-casing — they keep working as + the existing ADR-0033 tests assert. +- **One new seam to keep honest:** the literal-vs-expression + classification at parse. It must be tested (single literal / signed + literal / `NULL`/`true`/`false` → validated; arithmetic / function / + subquery → skipped), or it will drift. +- **A normalization difference is *avoided*, not introduced.** We + validate the literal but do not rewrite it; the engine stores the + user's text as written. (Had we bound/normalized, advanced inserts + might store a canonicalised value — a behaviour change we sidestep.) +- **Phase 3 will revisit literal *detection*** (swapping the parse-time + classification for typed slots that also drive hints). The + validation/enrichment built on it is permanent; only the detection is + provisional — a deliberate, documented small throwaway. ## See also -- ADR-0030 §4 — the execute-path split this ADR narrows for DML (the - `SELECT` half stands). -- ADR-0033 — the SQL DML grammar + the verbatim execution model this ADR - amends for literal values. -- ADR-0035 — the DDL precedent: structural, not verbatim ("verbatim was - a DML convenience, not a rule"). +- ADR-0030 §4 / ADR-0033 §10 — the execute-path this ADR **augments** + (adds value validation); the verbatim execution model and the + `SELECT`/expression text path both stand. +- ADR-0033 Amendment 3 — the two-command identity, **preserved** (this ADR + does *not* collapse `Insert`/`SqlInsert`). +- ADR-0035 — the DDL precedent (structural, not verbatim); this ADR is the + narrower DML analogue (validate, don't restructure). - ADR-0026 — the DSL's deliberately-limited `Expr`; *not* imposed on the - SQL surface here. -- ADR-0031 — `sql_expr` is validate-only (no `Expr` AST); unchanged. -- ADR-0032 — `SELECT` feedback-from-walk; the proof that text-to-engine - is right for queries. -- ADR-0029 — the column type/constraint model the literal validators + SQL surface. ADR-0031 — `sql_expr` is validate-only; unchanged. +- ADR-0032 — `SELECT` feedback-from-walk; the proof that text-to-engine is + right for queries. +- ADR-0029 — the column type/constraint model the shared validators enforce. - ADR-0035 Amendment 1 (F2 follow-up) — the neutral "that value" safety - net that remains correct for computed values. + net, correct for computed values. +- `requirements.md` **X4** (auto-fill difference — possible bug, untouched + here) and **X5** (framework cohesion / share-mechanics-not-commands — + the principle this ADR follows). diff --git a/docs/adr/README.md b/docs/adr/README.md index 8490b64..a1fb487 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -41,4 +41,4 @@ This directory contains the project's ADRs, recorded per - [ADR-0033 — The full SQL DML grammar (`INSERT` / `UPDATE` / `DELETE`)](0033-sql-dml-grammar.md) — **Accepted** (implemented + verified through sub-phase 3k, 2026-05-23; phase-exit report `docs/handoff/20260523-phase-3-verification.md`), the Phase-3 grammar commissioned by ADR-0030 §3: single- and multi-row `INSERT` (incl. `INSERT … SELECT` recursing through ADR-0032's `SQL_SELECT_COMPOUND`), `UPDATE` with `SET` assignment list, `DELETE`, all three optionally followed by `RETURNING projection_list`, plus full `ON CONFLICT … DO NOTHING / DO UPDATE` UPSERT on INSERT; **fixes the DSL-vs-SQL dispatch architecture for shared entry words (`insert`/`update`/`delete`)**: SQL-first / DSL-fallback in Advanced mode via a `Choice(SQL_shape, DSL_shape)` per shape, gated by a new walker capability `Node::Guard(fn)` — a zero-byte-consumption gating node that fails the enclosing Seq with a `ValidationError`; carries `Command::SqlInsert` / `SqlUpdate` / `SqlDelete` variants and `do_sql_*` worker handlers each of which knows the target table (for re-persistence) and the `returning: bool` flag (for DataResult routing); `shortid` auto-fill mirrors the DSL `do_insert` mechanism via worker post-fill; SQL DELETE produces the same per-relationship cascade summary the DSL DELETE does (ADR-0014 parity); three new walker diagnostics (`insert_arity_mismatch` ERROR, `auto_column_overridden` WARNING, `not_null_missing` WARNING) with positive + negative tests each; OOS list explicitly carves out `DEFAULT VALUES` (the project's planned seed feature), SQLite-specific `OR REPLACE` / `OR IGNORE` / `OR ABORT` / `OR FAIL` / `OR ROLLBACK` prefixes, `UPDATE FROM` multi-table updates, and WITH-prefixed DML; the `excluded` keyword inside `ON CONFLICT DO UPDATE` is a deliberate carve-out from ADR-0030 §7's engine-neutral posture (no standard-SQL UPSERT spelling exists that SQLite and PostgreSQL share); eleven phased sub-phases each with explicit exit gates + written DA gate, opening with the dispatch mechanism before any DML grammar lands; initial DA review recorded seven critiques that were resolved before status moved to Proposed; **Amendment 1 supersedes §2's dispatch mechanism**: the originally-chosen `Node::Guard(fn)` + `Choice(SQL_shape, DSL_shape)` was found during 3a to be unworkable as framed (any guard-in-`Choice` mechanism forces a `walk_choice` change — `walk_choice` only falls through on `NoMatch`, so Simple-mode valid-DSL would wrongly surface "this is SQL", and `walk_seq` treats a `NoMatch` past `idx 0` as a hard `Failed`, breaking Advanced-mode DSL fall-through); replaced by **category-grouped, mode-aware dispatch** in `walker::walk` (each `REGISTRY` entry tagged `CommandCategory::{Simple, Advanced}`, generalising the existing whole-command `is_advanced_only` gate), shared entry words carrying a node in both groups, no `Node::Guard` and no `walk_choice`/`walk_seq` change, advanced-mode completion SQL-first with DSL as a full-line fallback; **Amendment 2 (sub-phase 3f) supersedes §7's cascade mechanism**: the WHERE-injected per-child pre-count rested on a premise that was factually wrong about the DSL handler (which detects cascades by before/after row-count diffing inside a transaction, not by `Expr`-derived pre-count subqueries) and would have broken the §2 parity promise by reporting `SET NULL` the DSL path doesn't; replaced by mirroring `do_delete`'s count-diff exactly (verbatim DELETE executes, child-count diff observes the cascade — `ON DELETE CASCADE` row removals only, SET NULL deferred for both paths to preserve parity), which shares the render-layer formatter for free via `CommandOutcome::Delete` and **withdraws risk R2** (no WHERE-byte extraction, no N+1 subquery); **Amendment 3 (sub-phase 3j) records the command-identity model and defers the execution-mode side-channel**: a command is the typed outcome of a *mode-rooted* grammar path and its identity is intrinsic (Advanced mode tries SQL first, falls back to the *Simple* DSL command when no SQL branch matches a token, e.g. `delete … --all-rows`; note `update … --all-rows` does *not* fall back — the SQL `SET` expression eats `--all-rows`, harmless since the engine treats it as a comment); **Simple mode commits the DSL candidate for shared words** so the *real* DSL error surfaces, and when that line would also run in advanced mode the rendering layer **combines** them — DSL error **plus** an `advanced_mode.also_valid_sql` pointer ("… (valid as SQL in advanced mode)") — keeping the actionable DSL fix while pointing at advanced mode; bare "this is SQL" is reserved for entry words with no DSL form (`select`/`with`); a fully-overlapping input (`insert … values …`) legitimately yields *two distinct commands* (`Command::Insert` typed-AST vs `Command::SqlInsert` validated-text) that do the same thing but execute differently (ADR-0030 §4), so each is tested in the mode that produces it; **corrects the plan's 3j exit-gate premise** that the DSL DML tests run in Simple mode (they call `parse_command`, which defaults to Advanced) — the real invariant is "Simple-mode behaviour unchanged, Advanced mode SQL-first, DSL grammar tested in Simple mode, both variants tested in their producing mode", with §6/§7 parity keeping the paths observably equivalent; and **defers to its own future ADR** the execution-time mode side-channel (three-way `Mode`: simple/advanced/advanced-one-shot threaded through `Action`→worker, for mode-dependent *output* like echoing generated SQL) — today only the *rendering* side-channel `OutputLine.mode_at_submission` exists, and the three-way distinction is not required for Phase 3 dispatch correctness - [ADR-0034 — `history.log` as a complete command journal; replay reads success-only](0034-history-journal-and-replay-filter.md) — **Accepted**, resolves a three-way tension in `history.log`'s roles found while implementing ADR-0033 3f: (1) the persistent log is success-only while the in-memory Up/Down recall ring records *every* submission (success or failure, "so users can recall and edit typo'd commands"), and the ring is re-seeded from the log on project open — so **failed commands are recallable within a session but silently lost across sessions**; (2) replay wants the state-building (successful) commands while recall wants everything typed, which one success-only file cannot serve; (3) `replay history.log` never actually worked — `run_replay` parses each whole line through the DSL parser with no understanding of the `||` record shape, so a real log fails on line 1, and **no test ever fed the pipe format to replay** (the `replay_history_log_records_subcommands_only` test only checks what replay *writes*, never replays the log as input). Decision: `history.log` becomes a **complete journal** — every submission recorded, tagged `ok`/`err` via the status field the format already reserved (ADR-0015 §5) — and **each consumer filters**: hydration reads all records (cross-session recall matches in-session), replay reads `ok` only (and learns the journal format, while still accepting bare-command `.commands` scripts; detection by the leading timestamp+status prefix so a `|` inside a bare command isn't misread). Successful commands stay journalled transactionally by the worker; failed commands are journalled `err` best-effort from the runtime/app error path (a parse failure never reaches the worker). Amends ADR-0006's "successfully executed" wording and ADR-0015 §5 ("status always `ok`") / §12 (hydration). Code deferred to two tracked test-first sub-tasks (journal-failures+filtering; replay-parses-journal-format); existing all-`ok` logs need no migration; **implemented 2026-05-24** (plan `docs/plans/20260524-adr-0034-history-journal.md`); **Amendment 1 (2026-05-24): replay filters out app-lifecycle commands** — a working `replay history.log` (the §3 fix) exposed that the journal also records `save as`/`load`/`new`/`export`/`import`/`rebuild`/`mode` (which would panic the worker dispatch or abort the replay), so replay now re-applies **only** schema/data write commands and **skips** every `Command::App` + nested `Command::Replay`; **all skips continue** (never abort — reversing the prior nested-`replay` refusal, so a journal containing a once-run `replay` needs no hand-editing, and the infinite-loop footgun is closed by construction), with a `[skip]` **warning** on `import` and nested-`replay` skips (their omission can leave replayed state incomplete) and silent skips for the rest; `replay.error_nested` removed, `replay.skipped_import`/`replay.skipped_replay` added, `ReplayCompleted` carries `warnings` - [ADR-0035 — Advanced-mode SQL DDL](0035-advanced-mode-sql-ddl.md) — **Accepted** (design agreed 2026-05-24; validated end-to-end by sub-phases 4a/4a.2/4a.3/4b `CREATE TABLE` (incl. foreign keys) + 4c `DROP TABLE [IF EXISTS]` + 4d `CREATE [UNIQUE] INDEX` / `DROP INDEX [IF EXISTS]` + 4e `ALTER TABLE` add/drop/rename column + 4f `ALTER TABLE … ALTER COLUMN TYPE` + 4g `ALTER TABLE` add/drop constraint + add FK + 4h `ALTER TABLE … RENAME TO` + 4i verification sweep (completion merge + simple/advanced completion colour + describe of table-level constraints + self-ref FK indicator + CREATE-TABLE help/usage), implemented 2026-05-25/26 — **Phase 4 complete**; **Amendment 1, 2026-05-26**: drop a composite UNIQUE via a derived, engine-neutral `unique_` name that reuses the existing `DROP CONSTRAINT ` grammar — no new syntax, no metadata, §4g anonymity intact; `describe` shows the name; dropping a UNIQUE-covered *column* now refuses with that name + the drop command), **Phase 4** of the ADR-0030 roadmap (peer of 0031/0032/0033) and **clarifies ADR-0030 §4**. Advanced-mode `CREATE`/`DROP`/`ALTER TABLE` + `CREATE`/`DROP INDEX` get their **own per-statement commands** (`SqlCreateTable`/`SqlAlterTable`/`SqlDropTable`/`SqlCreateIndex`/`SqlDropIndex`), like DML's `Sql*` set — but unlike DML they **execute *structurally*, not verbatim** (raw execution would lose the playground's types, named relationships, and `STRICT`; "verbatim" was a DML convenience, not a rule). Handlers **reuse the low-level schema/metadata helpers** where the operation matches simple mode and **stand alone where the SQL surface is richer** (clarity over forced refactoring); simple mode is untouched (additive). Dispatch: `create`/`drop` reuse ADR-0033 Amendment 1's category-grouped mode-aware dispatch (SQL-first, simple fallback); `alter` is a new advanced-only entry word. Full surface (no pre-emptive cuts, `Q4`): `CREATE TABLE` with column + table constraints, single/compound `PRIMARY KEY`, inline + table-level `FOREIGN KEY` → **named relationships** (one statement = one command = **one undo step**, ADR-0006); `ALTER TABLE` add/drop/rename column, `ALTER COLUMN TYPE`, add/drop constraint, add FK, **`RENAME TO`** (advanced-only table rename — new low-level op renaming the table + its CSV + the relationship and table-CHECK metadata, closing the rename half of `C1`); `CREATE [UNIQUE] INDEX` / `DROP INDEX`. Type slot accepts the ten playground keywords **and** standard-SQL aliases (`integer`→`int`, `varchar`→`text`, `timestamp`→`datetime`, …; length args accepted-and-ignored; no engine type names in/out — ADR-0030 §5). `CHECK`/`DEFAULT` reuse ADR-0031 `sql_expr`. **Pre-implementation `/runda` refinements (2026-05-24, user-confirmed):** `CREATE TABLE`/`DROP TABLE` **admit `IF [NOT] EXISTS`** (no-op-that-succeeds-with-a-note — a near-universal cross-vendor idiom, reclassified *into* scope, not engine-specific); `INTEGER PRIMARY KEY` maps to a **plain `int`** PK, *not* auto-increment (`serial` stays the sole auto-increment type). **Column-type-conversion is unified** (ADR-0017 engine, mode-appropriate policy): clean auto-converts and incompatible/own-type-static cases refuse in both modes, but a **lossy** change refuses-by-default in simple mode (`--force-conversion` opts in) while advanced mode **performs it with a loss note** and relies on **`undo` as the safety net** — no force flag, no dropping to simple mode (a payoff of shipping ADR-0006 first). OOS: views/triggers/txn-control/PRAGMA/etc. (ADR-0030 §3), the Postgres `USING` clause, and the DSL→SQL teaching echo (ADR-0030 Phase 5). Sub-phases 4a–4i, plus **4a.2** (per-column `CHECK`/`DEFAULT` via raw `sql_expr` text — `sql_expr` is validate-only, no `Expr` AST — + composite `UNIQUE(a,b)`; no new internal table) and **4a.3** (table-level/multi-column `CHECK`, landed via the new `__rdbms_playground_table_checks` metadata table because SQLite has no PRAGMA for CHECK; the builder tells a table-level CHECK from a column-level one by element position) and **4b** (foreign keys — inline `REFERENCES` + table-level `FOREIGN KEY` → ADR-0013 named relationships in the create transaction, one undo step; self-references + bare `REFERENCES ` supported, user-confirmed) and **4c** (`DROP TABLE [IF EXISTS]` → `SqlDropTable`, reusing `do_drop_table`; `IF EXISTS` is a no-op-with-note via `DropOutcome::Skipped`) and **4d** (`CREATE [UNIQUE] INDEX [IF NOT EXISTS] [] ON (cols)` → `SqlCreateIndex` and `DROP INDEX [IF EXISTS] ` → `SqlDropIndex`, reusing `do_add_index`/`do_drop_index`; **`CREATE UNIQUE INDEX` admitted** — ADR-0025 **Amendment 1** — via an additive `IndexSchema.unique` flag that round-trips through `project.yaml` and rebuild, with `[unique]` markers in the structure view + items panel, while simple-mode `add unique index` stays deferred; `IF [NOT] EXISTS` reuses the 4c skip path; `create`/`drop` each gain a *second* advanced node, exercising the all-candidates dispatch) and **4e** (`ALTER TABLE` add/drop/rename column → `SqlAlterTable`; `alter` is a new advanced-**only** entry word, runtime-decomposed to the existing `do_add_column`/`do_drop_column`/`do_rename_column` — no new worker layer; `do_add_column` extended to consume raw `default_sql`/`check_sql` so ADD COLUMN reaches CREATE-TABLE constraint parity; drop/rename refuse a column any CHECK references (table-level AND column-level, incl. a column's own self-check on rename) — the 4a.3 deferral, via a raw-CHECK-text tokenizer in the shared executors, so it guards both surfaces and fixes a latent rename-drift bug; SQL DROP COLUMN refuses an index-covered column with no `--cascade` spelling; the column executors + `do_add_index` gained an internal-`__rdbms_*`-table guard — all user-confirmed) and **4f** (`ALTER TABLE … ALTER COLUMN TYPE` → a fourth `AlterTableAction`, runtime-decomposed to the existing `change_column_type` with `ChangeColumnMode::ForceConversion` — which **is** the §7 advanced policy: lossy converts *with a note* (no force flag), incompatible + ADR-0017 static refusals (`↔ blob`, same-type, `date ↔ datetime`, non-`int → serial`) still refuse, while **`int → serial` is allowed** (auto-fills nulls + UNIQUE, ADR-0018 §8 — the §7 "→serial refused" summary is looser than the code); the builder discriminates the fourth branch by the **`type` keyword** (unique — ADD COLUMN's type is an ident), the type slot reuses `SQL_TYPE`; the internal-`__rdbms_*` guard was folded into `do_change_column_type`, closing the simple `change column` exposure too — user-confirmed) and **4g** (`ALTER TABLE … ADD [CONSTRAINT ] (CHECK | UNIQUE | FOREIGN KEY)` + `DROP CONSTRAINT `; ADD = CHECK + composite UNIQUE + FK, with `ADD PRIMARY KEY` and a *named* UNIQUE refused — composite UNIQUE is anonymous in our model; each ADD reuses a low-level path (table-CHECK/UNIQUE rebuild with a dry-run guard; FK → `add_relationship`, bare `REFERENCES

` → parent single-PK), DROP CONSTRAINT resolves the name to a table-CHECK then a child-side FK; **named table-CHECKs round-trip** via a nullable `name` column on `__rdbms_playground_table_checks` (**rebuild-only** arrival — pre-4g projects gain it on `rebuild`, a named add on an un-upgraded project is refused with a friendly "rebuild first" message) *and* a `project.yaml` `check_constraints` extension to an `{expr, name}` mapping (the bare-string form still reads); the internal-`__rdbms_*` guard was folded into `do_add_constraint`/`do_add_relationship`, completing that guard class — all user-confirmed) and **4h** (`ALTER TABLE … RENAME TO` — the one genuinely new low-level op, `do_rename_table`: a native engine rename plus one-transaction reconciliation of every metadata row naming the table (`__rdbms_playground_columns`, **both ends** of `__rdbms_playground_relationships`, `__rdbms_playground_table_checks`), the CSV file (the existing rewrite+delete path — no new persistence method), and **CHECK text that qualifies a column with the old table name** (`T.age`→`U.age`, a planning-`/runda` finding — the engine rewrites the live CHECK but the stored text would drift and break a fresh rebuild; `rewrite_check_table_qualifier` keeps them in step); grammar splits the `rename` verb into one branch with an inner Choice on a distinct second keyword (`column` vs `to`), the new-name slot mirroring the `CREATE TABLE` name slot; refuses same-name / existing-target / `__rdbms_*` / non-existent, with **case-insensitive** collision checks behind an engine-neutral pre-check (a finished-slice `/runda` finding — the engine matches names case-insensitively); auto-named indexes *and* relationships keep their stale names (only table-name columns update — §6 scope); one undo step; advanced-only, closing the rename half of `C1` — all user-confirmed) and **4i** (the verification sweep that completes Phase 4: the shared-entry-word completion merge + the simple-vs-advanced completion colour-when-mixed with Both→Advanced→Simple block ordering; `describe` of table-level composite UNIQUE + table CHECK; the self-ref FK pre-submit indicator fix; and the CREATE-TABLE help/usage skeleton refresh). **All of Phase 4 (4a–4i) is shipped.** Each sub-phase has exit + DA gates -- [ADR-0036 — DML execution: bind literal values, verbatim text for expressions and queries](0036-typed-dml-values-vs-verbatim.md) — **Proposed** (design agreed 2026-05-26, pending a `/runda` pass + go-ahead to implement). **Narrows ADR-0030 §4 / ADR-0033 §10**: keeps DDL structural (ADR-0035) and `SELECT`/expressions as verbatim text (full SQL surface preserved, ADR-0026's limited `Expr` *not* imposed), but stops handing **literal data values** to the engine as text. The simple-mode DSL never did — `do_insert` parses each value to a typed `Value`, validates it (`validate_date`/`shortid::validate`/…), and binds it as a parameter; advanced-mode SQL spliced literals into text and let `STRICT` (storage types only) be the sole check. Result (investigated 2026-05-26; characterization test `sql_insert.rs::sql_dml_skips_app_level_value_validation_that_the_dsl_enforces` proves it): advanced-mode SQL DML gets **none** of the DSL's value feedback — no column-type hint, no value-vs-column highlight, no parse-time or bind-time type validation; a malformed `date` like `2025/01/15` is silently written. Same root cause as the F2 error-value gap (the literal is discarded, so `enrich_*` can't show it). **Principle: own structure + literal values, reserve verbatim text for expressions and queries** — the operational test is *can this value be bound as a parameter?* (a literal can; a computed expression/subquery/`SELECT` cannot). Classification is **per value position**, not per statement (a literal = `NULL`/boolean/string/**signed**-numeric binds; arithmetic/functions/subqueries/column-refs stay text). `INSERT … VALUES` literal positions typed/validated/bound; `UPDATE … SET` literal assignments bound. `RETURNING`/`ON CONFLICT …` are **clauses** carried on the generated statement — an `INSERT … VALUES (…) RETURNING/ON CONFLICT` still binds its literal `VALUES`; only `INSERT … SELECT` (value source *is* a query) has nothing to bind. `WHERE` (UPDATE/DELETE) stays text **deliberately** (it's an expression in general; the value-feedback motivation is met by `VALUES`/`SET`). `SELECT` stays text-with-feedback-from-walk by design. One change, four faces: validation + completion hint + highlight + offending-value-in-errors all follow from **retaining the typed literal on the command** (explicit requirement). Mechanism: recommended typed-slot hybrid grammar (`Choice(typed-literal-slot, sql_expr)`, reusing the DSL's live `column_value_list` / `TypedValueSlot`s at `data.rs:141`/`189`/`269` — the only option giving live completion hints) or a post-parse semantic pass (simpler, no live hints). Note: the DSL slots validate numeric *shape* at parse (int/decimal/bool) but defer `date`/`shortid` *format* to bind time, so SQL would converge on the same parse-then-bind split. Phased: literal `INSERT` first (closes the proven validation gap + error-value display together, smallest blast radius), then `UPDATE SET` literals, then hinting/highlighting. Non-goals: a structural `SELECT`, a full typed SQL-expression AST, removing the verbatim path. Supersedes-in-part once accepted; the neutral "that value" safety net (ADR-0035 Amendment 1) stays correct for genuinely-computed values +- [ADR-0036 — Value validation for advanced-mode DML](0036-typed-dml-values-vs-verbatim.md) — **Accepted** (design agreed + `/runda`'d 2026-05-26; mechanism then **deliberately narrowed** from "bind literals via the DSL path" to surgical **"validate-and-retain, execute verbatim"** after the user resisted consolidating the modes and a concrete auto-fill difference confirmed even the single-row literal case isn't identical across modes; Phase 1 implementing). **Augments — does NOT supersede — ADR-0030 §4 / ADR-0033 §10**: execution stays verbatim, ADR-0033 Amendment 3's two-command identity (`Insert` vs `SqlInsert`) **stands**. The problem (investigated 2026-05-26; characterization test `sql_insert.rs::sql_dml_skips_app_level_value_validation_that_the_dsl_enforces` proves it): advanced-mode SQL DML gets **none** of the DSL's value feedback — a malformed `date` like `2025/01/15` is silently written, and the offending value is missing from constraint errors — because literal values are spliced into text and discarded (only `STRICT` storage types check them). **Fix (surgical): validate each literal value against its column type before the verbatim insert, and retain it for error reporting — sharing only the per-type validators (`Value::bind_for_column`/`validate_date`/`shortid::validate`), nothing else.** No binding, no statement reconstruction, no auto-fill change, no command-identity collapse — because the two gaps are closed by validation + retention alone, and executing the user's own text is already safe. The literal set = `NULL`/boolean/string/**signed**-numeric; arithmetic/functions/subqueries/column-refs are expressions (skipped — the engine evaluates them). `WHERE` not validated (it's an expression in general; motivation met by `VALUES`/`SET`); `SELECT`/`INSERT … SELECT`/`RETURNING`/`ON CONFLICT` need no special handling since execution is untouched. Phased: **Phase 1** capture-at-parse + validate + retain for `INSERT … VALUES` (no grammar change, no reparse — closes both proven gaps); **Phase 2** `UPDATE … SET` literals; **Phase 3** completion hinting/highlighting (the only part needing a grammar change — `Choice(typed-literal-slot, sql_expr)` reusing the DSL `TypedValueSlot`s at `data.rs:141`/`189`/`269`; supersedes only Phase 1's literal *detection*, not the validation/enrichment on top). Non-goals: binding/reconstruction, collapsing command identity (Am3 stands), changing `serial`/`shortid` auto-fill (`requirements.md` **X4**, a separate possible-bug), a structural `SELECT`, a full SQL-expression AST. Embodies `requirements.md` **X5** (share a *mechanic*, not a *command*); the neutral "that value" safety net (ADR-0035 Amendment 1) stays correct for genuinely-computed values diff --git a/docs/requirements.md b/docs/requirements.md index 17d658d..a70a758 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -576,6 +576,38 @@ handoff-14 cleanup; 449 after B2/C2.) - [~] **X3** Accessibility: TUI screen-reader support is best-effort and not a v1 commitment; revisit if user need emerges. +- [~] **X4** Auto-fill semantics differ between simple and advanced + mode — **possible bug, to investigate** (raised 2026-05-26). The + simple-mode insert (`do_insert`, `db.rs`) auto-fills an omitted + **non-PK `serial`** column with `MAX(col)+1` *and* generates + `shortid`s; the advanced-mode SQL insert (`plan_shortid_autofill`) + auto-fills **only `shortid`** and does **not** do non-PK `serial` + MAX+1. The `shortid` distinction is intentional; the non-PK-`serial` + gap looks unintended (no obvious reason advanced mode should skip + filling a `serial`). Decide whether advanced mode should match + simple mode's serial auto-fill (likely yes) or whether the + difference is deliberate, and align ADR-0018 accordingly. Left + untouched by the ADR-0036 value-validation work (which is surgical + and does not change auto-fill). +- [~] **X5** Framework cohesion / restructuring — **strategic, + revisit later** (raised 2026-05-26). The grammar/execution + framework (lexer → walker `Node`s → unified grammar tree → typed + `Command`s → executors; ADR-0023/0024 and everything layered on + since) grew organically from the original grammar outline and now + reuses + mixes elements across many levels without a *cohesive + written specification* of what the framework comprises, which + elements are meant to be reusable, and where the boundaries sit for + the recurring "reuse vs create new" decision. A concrete symptom: + commands are coupled tightly enough to their execution that reusing + an *execution behaviour* tempts reusing the whole *command* (see the + ADR-0036 discussion — the temptation to emit `Command::Insert` from + the advanced path just to reuse `do_insert`). Desired end state + (user-stated): **unique commands for every unique case, with a clean, + documented structure for reusable mechanics** (so execution helpers + are shared as library functions, not by collapsing command + identity). Consider a dedicated specification + restructuring run + (its own ADR) to map the framework and set the reuse-boundary rules, + easing future maintenance and extension. Not scheduled. ---