From c12ed1da9acc59af6497bce1fa7c28b9c07ce034 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Thu, 28 May 2026 16:38:33 +0000 Subject: [PATCH] fix: INSERT Form B value-count UX (ADR-0033 Amendment 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three layered fixes for advanced/simple-mode positional INSERT value-count mismatches (e.g. `insert into T values (...)` with the wrong number of values for T's column count), plus ADR-0033 Amendment 5 recording the gate refinement. Walker diagnostic (dml_insert_arity_diagnostics): the function's own doc-comment recorded the no-column-list (Form B) case as deferred. This commit closes that gap. Form B mismatches now emit a new diagnostic.insert_arity_mismatch_form_b ERROR per offending tuple, keyed off the target table's column count from the schema cache. The [ERR] validity indicator (ADR-0027) lights up at typing time for the reported scenario, no longer needing a submit. Cross-mode pointer gate (advanced_alternative_note): refactored from a hand-rolled Form B count check to a single input_verdict_in_mode( input, schema, Mode::Advanced) call. The pointer fires only when the verdict is None — the ADR-0027 sense of "valid". Any future static check added to the verdict pipeline participates automatically; no per-feature maintenance. Teaching notes for the value-count cases users can hit before the indicator turns red: - simple-mode submit: insert.form_b_extra_values_note covers under-, in-window, and over-supply against the Form B contract; suppressed when the cross-mode pointer fires (to avoid parallel advice). - advanced-mode dispatch pre-flight: insert.form_b_positional_count_mismatch_note catches a submitted mismatch with a teaching message before the engine produces its raw NOT-NULL / type error. The advanced_mode.also_valid_sql pointer wording was reworked to "trying to write SQL? switch with `mode advanced`, or prefix `:` to run once". One insta snapshot regenerated. ADR-0033 Amendment 5 records the gate change: Amendment 3's "would parse in advanced mode" now reads as "valid in advanced mode" in the explicit verdict-is-None sense, with the precise definition spelled out so future readers can't drift back to the syntactic-only reading. ADR-0000 index entry updated; docs/requirements.md H1a citation added listing the three new pedagogical strings. Tests added (8): four walker arity tests (under-supply, over-supply, match, unknown-table); two app-level teaching-note tests for the sibling cases (under-supply, over-supply beyond total); one pointer- gate unit test pinning the bug-case suppression; one gate-precedence test ensuring only one advice line per error. Existing simple_mode_submit_of_sql_construct_appends_advanced_pointer updated to use a known schema (the new validity gate requires it). Full suite: 2031 passed, 0 failed, 0 unexpected skips. Clippy clean. --- docs/adr/0033-sql-dml-grammar.md | 94 +++++ docs/adr/README.md | 2 +- docs/requirements.md | 11 +- src/app.rs | 333 +++++++++++++++++- src/dsl/walker/mod.rs | 182 ++++++++-- src/friendly/keys.rs | 20 ++ src/friendly/strings/en-US.yaml | 20 +- src/input_render.rs | 281 ++++++++++++++- ..._is_invalid@form_b_extra_serial_value.snap | 3 +- 9 files changed, 898 insertions(+), 48 deletions(-) diff --git a/docs/adr/0033-sql-dml-grammar.md b/docs/adr/0033-sql-dml-grammar.md index c48398b..73c973b 100644 --- a/docs/adr/0033-sql-dml-grammar.md +++ b/docs/adr/0033-sql-dml-grammar.md @@ -1553,6 +1553,100 @@ The existing `delete … --all-rows` fall-back is unaffected. Folded into the ADR-0038 echo effort (the fix that makes `update … --all-rows` echoable). +## Amendment 5 — `also_valid_sql` fires on *validity*, not just *parse* (2026-05-28) + +Amendment 3 introduced the `advanced_mode.also_valid_sql` pointer and +described its gate as *"the same line would parse in advanced mode."* +Issue #1 surfaced that this is too weak: an advanced-mode positional +`INSERT INTO T VALUES (…)` with a value count that doesn't match the +target table's column count **parses** (the SQL grammar accepts any +positional value count) but then fails at the engine. The pointer +fired, telling the user to switch modes, and the same line failed +again — the suggestion was misleading. Recorded with explicit user +approval (2026-05-28). + +### Why "parse" was the wrong bar + +"Would parse" is a purely syntactic check: the structural grammar +accepts the input. It says nothing about whether subsequent static +checks (type slots, INSERT arity, predicate warnings, schema-existence +diagnostics, …) would also pass. The cross-mode pointer's job is to +direct the user to the mode that **actually** runs the line; a +syntactic-only gate over-promises in every case where a non-grammar +static check would still reject. The Form B positional arity case is +just the first one observed. + +### "Valid" — the explicit definition used by this amendment + +The pointer fires only when the line is **valid** in advanced mode, +where **valid** is the ADR-0027 sense — operationally, the value +returned by `crate::dsl::walker::input_verdict_in_mode(input, schema, +Mode::Advanced)` is `None`: + +- The structural parse succeeds (`WalkOutcome::Match`), **and** +- No diagnostic of `Severity::Warning` or `Severity::Error` is + produced by any pass that contributes to `input_verdict_in_mode` + (schema-existence, SQL-predicate warnings, compound-arity errors, + INSERT-arity errors — Form A *and* Form B — and any further + diagnostic added to the pipeline in the future). + +This is the same condition the in-app `[ERR]` / `[WRN]` validity +indicator uses (ADR-0027 §2). Tying the pointer to it gives one +canonical "is this line OK in this mode?" answer that every surface +shares — by construction, the indicator and the pointer can never +disagree about a given line. + +### Decision + +- `advanced_alternative_note` (`src/input_render.rs`) reads + `input_verdict_in_mode(…, Mode::Advanced)`. The pointer fires only + for verdict `None`. +- Any static diagnostic that participates in the verdict automatically + participates in the gate. No bespoke pre-flight check lives in the + pointer helper. +- The Amendment 3 description (*"would parse in advanced mode"*) + should be read as a synonym for **valid** in this amendment's + sense, not as a literal syntactic-only test. + +### Diagnostic added to close the issue #1 gap + +ADR-0033 §8.1's existing `dml_insert_arity_diagnostics` checked only +Form A (column list present). Its own doc-comment recorded the Form B +case as deferred. This amendment closes that gap: the same function +now also checks the Form B case (no column list) by comparing each +VALUES tuple's arity against the target table's full column count from +the schema cache, emitting a new diagnostic key +`diagnostic.insert_arity_mismatch_form_b`. The `[ERR]` validity +indicator now lights up at typing time for the user-reported scenario, +and the cross-mode pointer gate sees the verdict and stays silent. + +### Tests + +The amendment's behavioural promise is locked down by: + +- `dsl::walker::tests::insert_form_b_arity_mismatch_under_supply_fires` — + the user's reported 4-col / 3-value case produces an Error diagnostic. +- `dsl::walker::tests::insert_form_b_arity_mismatch_over_supply_fires` — + symmetric case, 5 values for a 4-col table. +- `dsl::walker::tests::insert_form_b_arity_match_is_silent` — happy path + (every column supplied, autos included) stays silent. +- `dsl::walker::tests::insert_form_b_arity_unknown_table_is_silent` — + defensive: an unknown table defers to the schema-existence pass. +- `input_render::tests::ambient_hint_omits_advanced_pointer_when_form_b_value_count_wouldnt_match` — + the pointer is suppressed for the bug case via the validity verdict. +- `app::tests::simple_mode_submit_of_sql_construct_appends_advanced_pointer` — + multi-row VALUES (a true SQL-only construct) still fires the pointer + when the table exists in the schema. + +### Behaviour preserved + +The pointer still fires for every input that is genuinely SQL-only +syntax in simple mode and is valid as advanced SQL — `delete … +returning *`, multi-row VALUES against a known table, `INSERT … ON +CONFLICT …` over a known target, and so on. The narrowing is +**exactly** the set of inputs where switching modes wouldn't help, no +more. + ## See also - ADR-0005 — the ten-type vocabulary INSERT works with. diff --git a/docs/adr/README.md b/docs/adr/README.md index c0ea708..a66d5bd 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -38,7 +38,7 @@ This directory contains the project's ADRs, recorded per - [ADR-0030 — Advanced mode: the standard-SQL surface](0030-advanced-mode-sql-surface.md) — **Accepted**, SQL added as grammar *within the unified grammar tree* (ADR-0024), not a separate batch parser — so SQL gets the same completion / highlighting / hints / parse-errors as the DSL; mode gates the SQL forms; DDL routes through the typed `Command` executor (metadata + type vocabulary preserved), DML and `SELECT` execute as validated SQL; engine-neutral posture, the DSL→SQL teaching echo; supersedes ADR-0001's `sqlparser-rs` reservation; phased plan (`Q1` / `Q2` / `Q4`); **§13 OOS-2 (EXPLAIN of advanced SQL) superseded by ADR-0039** - [ADR-0031 — The SQL expression grammar](0031-sql-expression-grammar.md) — **Accepted**, the stratified SQL expression grammar fragment commissioned by ADR-0030 §3: a single precedence ladder (`OR`/`AND`/`NOT`, the comparison/`LIKE`/`IN`/`BETWEEN`/`IS NULL` predicate set, arithmetic incl. `||`, function calls, `CASE`) — the superset of ADR-0026's DSL `WHERE` grammar, authored as a parallel fragment so simple mode is untouched; pure validation, builds **no** AST (consumers run/store SQL as text per ADR-0030 §4/§6); reuses ADR-0026's `Subgrammar` recursion + depth cap unchanged; subquery expressions and qualified column refs deferred to ADR-0030 Phase 2 - [ADR-0032 — The full SQL `SELECT` grammar](0032-sql-select-grammar.md) — **Accepted**, the Phase-2 grammar commissioned by ADR-0030 §3: full `SELECT` with `INNER`/`LEFT`/`RIGHT`/`FULL OUTER`/`CROSS` joins, `GROUP BY`/`HAVING`, all four set ops (`UNION`/`UNION ALL`/`INTERSECT`/`EXCEPT`), `WITH` and `WITH RECURSIVE` CTEs, `LIMIT … OFFSET`, `DISTINCT`, `t.*`, and bare-alias projection (lifting Phase-1 §4.2); additive extensions to ADR-0031's `sql_expr` for scalar subqueries, `IN (SELECT …)`, `[NOT] EXISTS`, and qualified column refs (redeeming ADR-0031 §7 OOS-1/OOS-2); grammar-recursion via `Subgrammar(&SQL_SELECT_COMPOUND)` reuses ADR-0026's `MAX_SUBGRAMMAR_DEPTH = 64` cap unchanged; **softens ADR-0030 §8's "ambient assistance comes for free" claim**: completion scope needs new `WalkContext` accumulators (a `from_scope_stack` of `ScopeFrame`s holding `from_scope` / `cte_bindings` / `projection_aliases`), a **new walker node variant `Node::ScopedSubgrammar(&Node)`** as the push/pop trigger (existing `Node::Subgrammar` unchanged so DSL `Expr` and `sql_expr` recursion are unaffected), qualified-prefix completion narrowing, body-projection-derived CTE column resolution (so `SELECT *` and explicit-projection CTE bodies both yield real column completion past `cte_alias.|`), and a **post-walk fixup pass** that re-resolves projection-list identifier highlighting/validity once `FROM` is parsed (the projection-before-FROM problem); classifies every Phase-2 validation case against ADR-0027's ERROR/WARNING guideline (§11): five new `diagnostic.*` keys for parse-time-detectable cases (unknown qualifier, ambiguous column, projection-alias misplaced, CTE/compound arity mismatch) plus eight `engine.*` translation keys; a MatchedPath-walking predicate-warnings variant that closes the Phase-1 gap where SQL `WHERE` expressions emitted no `LIKE`-on-numeric / `= NULL` / type-mismatch warnings (ADR-0027 Amendment 1 finally extends to the SQL surface); adds a worker-side post-prepare type-resolution pass via engine column-origin metadata so bare column refs recover their playground type (partially lifting Phase-1 §4.5, the bool→0/1 case) — `Cargo.toml` gains `column_metadata` to rusqlite features (verified against pinned 0.39.0); `__rdbms_*` rejection extended to every new table-source slot; Amendment 1 narrows §12's resolution rule from a grammar-side structural classification to "trust the engine's column-origin metadata verbatim" after an empirical probe showed origin metadata follows through non-recursive CTEs, scalar subqueries, derived tables, set ops, and joins — the one structural exception is recursive CTE result columns, which return None and stay typeless; Amendment 2 records that §10.6's "rewrite the highlight class" prescription is realised via the two-pass schema-existence diagnostic + the renderer's diagnostic-overlay path (no separate per-byte rewrite step needed; no new HighlightClass variant), and that the projection-before-FROM completion narrowing has been improved by an `src/completion.rs` look-ahead probe when the leading walk's `from_scope` is empty but the full input parses -- [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; **Amendment 4, 2026-05-27** (design agreed, pending impl): **reverses Amendment 3's `update … --all-rows` counter-example as a bug** — surfaced by the ADR-0038 echo design. The walker has no `--` comment support (it lexes two minus operators) while the engine treats `--` as a comment, so `update T set x=42 --all-rows` was silently parsed as the expression `42 - -all - rows` over **non-existent columns** `all`/`rows` (an ADR-0027 "flag-if-it-will-fail" case) and matched `SqlUpdate`. Decision: the `--all-rows` sequence makes the SQL `UPDATE` shape **fail**, so dispatch **falls back to the DSL `Update { AllRows }`** — symmetry with `delete … --all-rows`; no `--` comment feature introduced (trailing comments stay unsupported). Inverts `sql_dml_e2e.rs::e2e_update_all_rows_in_advanced_does_not_fall_back_to_dsl`; mechanism settled test-first in the build; folded into the ADR-0038 effort (makes `update … --all-rows` echoable) +- [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; **Amendment 4, 2026-05-27** (design agreed, pending impl): **reverses Amendment 3's `update … --all-rows` counter-example as a bug** — surfaced by the ADR-0038 echo design. The walker has no `--` comment support (it lexes two minus operators) while the engine treats `--` as a comment, so `update T set x=42 --all-rows` was silently parsed as the expression `42 - -all - rows` over **non-existent columns** `all`/`rows` (an ADR-0027 "flag-if-it-will-fail" case) and matched `SqlUpdate`. Decision: the `--all-rows` sequence makes the SQL `UPDATE` shape **fail**, so dispatch **falls back to the DSL `Update { AllRows }`** — symmetry with `delete … --all-rows`; no `--` comment feature introduced (trailing comments stay unsupported). Inverts `sql_dml_e2e.rs::e2e_update_all_rows_in_advanced_does_not_fall_back_to_dsl`; mechanism settled test-first in the build; folded into the ADR-0038 effort (makes `update … --all-rows` echoable); **Amendment 5, 2026-05-28** (implemented + verified, user-confirmed): `advanced_mode.also_valid_sql` (the cross-mode pointer from Amendment 3) fires on **validity**, not just **parse** — *"valid"* meaning `input_verdict_in_mode(input, schema, Mode::Advanced) == None` in the ADR-0027 sense (parse succeeds *and* no Warning/Error diagnostic from any pass). Surfaced by **issue #1**: a positional `INSERT INTO T VALUES (…)` (no column list) with a value count that didn't match the target's column count parsed in advanced but failed at the engine, so the syntactic-only Amendment-3 gate promised a mode switch that wouldn't help. Closes the gap by (a) extending `dml_insert_arity_diagnostics` (§8.1, previously Form A only — its own doc-comment deferred Form B) to also check the no-column-list form against the schema's column count, emitting a new `diagnostic.insert_arity_mismatch_form_b` ERROR per offending tuple, and (b) refactoring `advanced_alternative_note` to read the validity verdict instead of running its own bespoke check — any static diagnostic added to the pipeline in the future automatically participates in the pointer gate. Side benefit: the `[ERR]` validity indicator now lights up at typing time for the reported scenario, no longer needing a submit to learn the line is wrong. Tests pinned: `insert_form_b_arity_mismatch_under_supply_fires` / `_over_supply_fires` / `_match_is_silent` / `_unknown_table_is_silent` (walker); `ambient_hint_omits_advanced_pointer_when_form_b_value_count_wouldnt_match` (gate); `simple_mode_submit_of_sql_construct_appends_advanced_pointer` (pointer still fires for genuine SQL-only constructs against a known schema). Amendment 3's "would parse in advanced mode" should henceforth be read as a synonym for "valid in advanced mode" in this stricter sense; the user-confirmed behavioural change is exactly the issue #1 bug case (no other input flips its pointer state) - [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; **Amendment 2, 2026-05-27** (design agreed, pending impl): a **standard-first dialect stance** (refines ADR-0030's "standard SQL" posture — ISO spelling is canonical + echoed where one exists; a vendor shorthand may be *accepted* but isn't canonical; where ISO offers none, *one* documented vendor spelling is a deliberate extension) + an `ALTER COLUMN` **constraint gap-fill** surfaced by the ADR-0038 echo design: makes ISO `ALTER COLUMN … SET DATA TYPE` the canonical type-change verb with `TYPE` retained as a synonym (**reverses §4f's "no `SET DATA TYPE`"**), and adds `SET/DROP DEFAULT` (ISO) + `SET/DROP NOT NULL` (the one documented extension — ISO has no in-place NOT-NULL verb; PostgreSQL's chosen for being type-independent), all **rebuild-backed via the existing ADR-0029 `do_add_constraint`/`do_drop_constraint` executors** (dry-run + internal-table guards free, no new worker layer), reaching simple↔advanced constraint-mod **parity for NOT NULL + DEFAULT**; the **rebuild stays hidden** (Category-1 engine detail, ADR-0038). Residual gap left open + flagged: dropping a **column-level (anonymous) UNIQUE/CHECK** (no portable name — same class as Am1's parallel gap), which ADR-0038's catalogue marks "no headline echo" - [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; **Phases 1–2 implemented** 2026-05-26 — `INSERT … VALUES` and `UPDATE … SET` literal validation + offending-value retention, capture-at-parse with no grammar change; **Phase 3a implemented** 2026-05-26 — live typed-slot hints + numeric-shape highlighting for `UPDATE`/UPSERT `SET col = ` via a boundary-aware lookahead (Amendment 1 corrects this ADR's naive-`Choice` sketch); **Phase 3b implemented** 2026-05-27 — per-position typed slots for `INSERT … VALUES` (single/multi-row, Form A/B) via a new zero-width `Node::SetColumn` primitive + an arity-gating tuple lookahead that preserves the §8.1 arity diagnostic; **fully implemented**). **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 — a typed-literal slot vs `sql_expr` reusing the DSL `TypedValueSlot`s at `data.rs:141`/`189`/`269`, discriminated by a **boundary-aware lookahead** not a naive `Choice` per **Amendment 1**; split into **3a** `SET` (done) and **3b** `VALUES` (pending); supersedes only Phase 1/2'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 a00546b..18c26c4 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -526,7 +526,16 @@ handoff-14 cleanup; 449 after B2/C2.) INSERT removes one such case; ADR-0024's typed value slots give per-column-type rejection wording; `insert into T (col)` with no `values` clause now flags "looks like Form A — add - `values (...)`"). A systematic pass is still pending. + `values (...)`"; **issue #1 / ADR-0033 Amendment 5, 2026-05-28** + added two pedagogical lines that teach the INSERT Form B + positional-`VALUES` contract — a simple-mode submit-time + teaching note covering under-supply, over-supply, and extra + values (`insert.form_b_extra_values_note`) and an + advanced-mode dispatch-time pre-flight note for the same + value-count class (`insert.form_b_positional_count_mismatch_note`), + plus a walker-level `diagnostic.insert_arity_mismatch_form_b` + ERROR that lights the `[ERR]` validity indicator at typing + time). A systematic pass is still pending. - [ ] **H2** `hint` provides contextual help for the current input or the most recent error. - [ ] **H3** `help` provides general reference and per-command diff --git a/src/app.rs b/src/app.rs index d503771..4919966 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1340,6 +1340,27 @@ impl App { vec![Action::Replay { path }] } Ok(cmd) => { + // Issue #1 sub-task 3: advanced-mode positional + // `INSERT INTO T VALUES (…)` (no column list) with a + // value count that doesn't match the column count gets + // a teaching note here, *before* dispatch. The engine + // would otherwise surface a raw NOT-NULL / type error + // that doesn't mention the column-list override. + if let Some(note) = crate::input_render::form_b_positional_count_mismatch_note( + &cmd, + &self.schema_cache, + ) { + self.push_output(OutputLine { + text: crate::t!("dsl.running", input = input), + kind: OutputKind::Echo, + mode_at_submission: mode, + styled_runs: None, + }); + self.note_error(note); + return vec![Action::JournalFailure { + source: input.to_string(), + }]; + } self.push_output(OutputLine { text: crate::t!("dsl.running", input = input), kind: OutputKind::Echo, @@ -1405,6 +1426,19 @@ impl App { { self.note_error(note); } + // Issue #1 sub-task 2: append a teaching note when the + // Form B `insert into values (…)` line failed + // because the user supplied more values than the + // non-auto-generated columns expect. The parse error + // shows the literal "expected `)`"; the note explains + // *why* fewer values are expected and shows the + // column-list override path. + if mode == Mode::Simple + && let Some(note) = + crate::input_render::form_b_extra_values_note(input, &self.schema_cache) + { + self.note_error(note); + } // ADR-0021 §2: append the usage block (if a // known command-entry keyword was consumed) or // the available-commands fallback (§5). @@ -2842,15 +2876,293 @@ mod tests { ); } + /// Helper: install a `Customers(id serial, Name text, Age int, + /// SerNo serial)` schema cache on `app` for the Form-B education + /// tests (issue #1). + fn install_customers_schema_two_serials(app: &mut App) { + use crate::completion::TableColumn; + use crate::dsl::types::Type; + let cols = [ + ("id", Type::Serial), + ("Name", Type::Text), + ("Age", Type::Int), + ("SerNo", Type::Serial), + ]; + app.schema_cache.tables.push("Customers".to_string()); + let tc: Vec = cols + .iter() + .map(|(n, t)| TableColumn { + name: (*n).to_string(), + user_type: *t, + not_null: false, + has_default: false, + }) + .collect(); + for c in &tc { + app.schema_cache.columns.push(c.name.clone()); + } + app.schema_cache + .table_columns + .insert("Customers".to_string(), tc); + } + + #[test] + fn simple_mode_submit_form_b_extra_value_shows_only_one_advice_line() { + // Issue #1 sub-task 2 + sub-task 1 interaction: in the + // 3-column Form B case (`Customers(id serial, Name, Email)` + + // 3 values, e.g. `insert into Customers values (1, 'Alice', + // 'a@b.c')`) the line IS valid in advanced mode, so the + // cross-mode pointer fires. The sub-task 2 teaching note + // ("list every column") would be parallel advice — both are + // valid escape hatches — and showing both clutters the error. + // The teaching note must defer to the cross-mode pointer. + use crate::completion::TableColumn; + use crate::dsl::types::Type; + let mut app = App::new(); + let cols = [ + ("id", Type::Serial), + ("Name", Type::Text), + ("Email", Type::Text), + ]; + app.schema_cache.tables.push("Customers".to_string()); + let tc: Vec = cols + .iter() + .map(|(n, t)| TableColumn { + name: (*n).to_string(), + user_type: *t, + not_null: false, + has_default: false, + }) + .collect(); + for c in &tc { + app.schema_cache.columns.push(c.name.clone()); + } + app.schema_cache + .table_columns + .insert("Customers".to_string(), tc); + type_str( + &mut app, + "insert into Customers values (1, 'Alice', 'a@b.c')", + ); + let _ = submit(&mut app); + let out = error_lines(&app); + // The cross-mode pointer (sub-task 1) fires — the line works + // in advanced. Substring "mode advanced" is the durable + // actionable fragment. + assert!( + out.contains("mode advanced"), + "cross-mode pointer must fire for the 3-col line: {out}", + ); + // The sub-task 2 teaching note is suppressed: no "list every + // column" / "auto-generated and filled" prose alongside it. + assert!( + !out.contains("auto-generated and filled"), + "sub-task 2 note must defer to the cross-mode pointer: {out}", + ); + } + + #[test] + fn advanced_mode_submit_of_form_b_value_count_mismatch_shows_preflight_note() { + // Issue #1 sub-task 3: advanced-mode positional `INSERT INTO T + // VALUES (…)` (no column list) requires every column. When the + // value count doesn't match, today's flow lets the engine + // produce a raw constraint or type error; we'd rather catch it + // at dispatch time and surface a teaching note that names the + // table's columns and shows the column-list override. + let mut app = App::new(); + install_customers_schema_two_serials(&mut app); + app.mode = Mode::Advanced; + type_str(&mut app, "insert into Customers values ('Oli', 52, 3)"); + let actions = submit(&mut app); + // The pre-flight rejected the line — no ExecuteDsl dispatch. + assert!( + !actions + .iter() + .any(|a| matches!(a, Action::ExecuteDsl { .. })), + "advanced-mode pre-flight must suppress dispatch on Form-B count mismatch; got: {actions:?}", + ); + let out = error_lines(&app); + // The teaching note names the rule … + assert!( + out.contains("every column"), + "missing the positional-VALUES rule in: {out}", + ); + // … names the table's columns so the user can see what's needed … + assert!( + out.contains("Name") && out.contains("Age") && out.contains("id") && out.contains("SerNo"), + "missing the column-name list in: {out}", + ); + // … and shows the column-list override targeting the non-auto columns. + assert!( + out.contains("insert into Customers (Name, Age)"), + "missing the column-list override example in: {out}", + ); + } + + #[test] + fn simple_mode_submit_of_form_b_extra_value_teaches_serial_skip() { + // Issue #1 sub-task 2: when the user supplies too many positional + // values in simple-mode Form B, the bare "expected `)`" parse + // error doesn't explain why fewer values are expected. The + // submit error gets a teaching line that names the columns + // Form B fills automatically, names the columns it expects + // values for, and points at the column-list (Form A) override. + let mut app = App::new(); + install_customers_schema_two_serials(&mut app); + type_str(&mut app, "insert into Customers values ('Oli', 52, 3)"); + let _ = submit(&mut app); + let out = error_lines(&app); + // The teaching line names the user-supplied columns … + assert!(out.contains("Name") && out.contains("Age"), "missing non-auto column names in: {out}"); + // … the auto-generated columns … + assert!(out.contains("id") && out.contains("SerNo"), "missing auto column names in: {out}"); + // … signals the contract … + assert!( + out.contains("auto-generated"), + "missing the contract word in: {out}", + ); + // … and shows the Form-A override path with every column listed. + assert!( + out.contains("insert into Customers (id, Name, Age, SerNo)"), + "missing the Form-A override example in: {out}", + ); + // Issue #1 sub-task 1 gate: the cross-mode pointer must be + // suppressed for the user's reported case (count mismatches in + // advanced too — switching modes wouldn't help). Without this + // assertion, a regression of the gate would silently restore + // the misleading pointer alongside the teaching note. + assert!( + !out.contains("mode advanced"), + "cross-mode pointer must NOT fire for the 4-col mismatch case: {out}", + ); + } + + #[test] + fn simple_mode_submit_of_form_b_undersupply_teaches() { + // Issue #1 siblings task: simple-mode under-supply + // (`insert into Customers values ('Oli')` for a 4-col table + // whose Form B expects 2 values for `Name`, `Age`). Today the + // user sees a bare parse error; with the extended teaching + // note they get the same forward-looking explanation as the + // over-supply case — what's expected, what's auto-filled, how + // to use the column-list form to override. + let mut app = App::new(); + install_customers_schema_two_serials(&mut app); + type_str(&mut app, "insert into Customers values ('Oli')"); + let _ = submit(&mut app); + let out = error_lines(&app); + assert!( + out.contains("Name") && out.contains("Age"), + "missing non-auto column names in: {out}", + ); + assert!( + out.contains("id") && out.contains("SerNo"), + "missing auto column names in: {out}", + ); + assert!( + out.contains("auto-generated"), + "missing contract word in: {out}", + ); + assert!( + out.contains("insert into Customers (id, Name, Age, SerNo)"), + "missing column-list override in: {out}", + ); + } + + #[test] + fn simple_mode_submit_of_form_b_oversupply_beyond_total_teaches() { + // Issue #1 siblings task: simple-mode over-supply *beyond* the + // total column count (5 values for a 4-col table). The + // teaching note still fires — the explanation of Form B's + // contract applies regardless of how far past Form B's count + // the user went. + let mut app = App::new(); + install_customers_schema_two_serials(&mut app); + type_str( + &mut app, + "insert into Customers values ('Oli', 52, 3, 13, 99)", + ); + let _ = submit(&mut app); + let out = error_lines(&app); + assert!( + out.contains("Name") && out.contains("Age"), + "missing non-auto column names in: {out}", + ); + assert!( + out.contains("auto-generated"), + "missing contract word in: {out}", + ); + assert!( + out.contains("insert into Customers (id, Name, Age, SerNo)"), + "missing column-list override in: {out}", + ); + } + + #[test] + fn advanced_mode_submit_with_all_values_for_serials_succeeds_no_preflight() { + // Issue #1 — the user explicitly confirmed that supplying every + // column including the serials works in advanced mode + // (`insert into Customers values (13, 'Oli', 42, 13)`). The + // pre-flight must not interfere with that happy path: it fires + // only on count mismatch, and the dispatch reaches the worker + // normally. This locks down the bottom edge of the pre-flight + // gate. + let mut app = App::new(); + install_customers_schema_two_serials(&mut app); + app.mode = Mode::Advanced; + type_str( + &mut app, + "insert into Customers values (13, 'Oli', 42, 13)", + ); + let actions = submit(&mut app); + assert!( + actions + .iter() + .any(|a| matches!(a, Action::ExecuteDsl { .. })), + "the line must dispatch normally; pre-flight must not fire when value count matches column count; got: {actions:?}", + ); + // And no teaching note prose appears in the output … + let out = error_lines(&app); + assert!( + !out.contains("requires a value for every column"), + "pre-flight must stay silent when counts match: {out}", + ); + } + #[test] fn simple_mode_submit_of_sql_construct_appends_advanced_pointer() { - // ADR-0033 Amendment 3: submitting a line in simple mode that - // fails as DSL but would run as SQL in advanced mode appends - // the `advanced_mode.also_valid_sql` pointer to the parse - // error — keeping the DSL detail and pointing at advanced - // mode. Multi-row VALUES is a definite DSL error and valid SQL - // (no schema needed). + // ADR-0033 Amendment 3 (+ Amendment 5): submitting a line in + // simple mode that fails as DSL but would be valid in advanced + // mode appends the `advanced_mode.also_valid_sql` pointer to + // the parse error — keeping the DSL detail and pointing at + // advanced mode. Multi-row VALUES is a definite DSL error and + // valid SQL with a real schema (the validity verdict needs the + // table to exist; an unknown-table diagnostic would correctly + // suppress the pointer). + use crate::completion::TableColumn; + use crate::dsl::types::Type; let mut app = App::new(); + app.schema_cache.tables.push("T".to_string()); + let tc = vec![ + TableColumn { + name: "a".to_string(), + user_type: Type::Int, + not_null: false, + has_default: false, + }, + TableColumn { + name: "b".to_string(), + user_type: Type::Int, + not_null: false, + has_default: false, + }, + ]; + for c in &tc { + app.schema_cache.columns.push(c.name.clone()); + } + app.schema_cache + .table_columns + .insert("T".to_string(), tc); type_str(&mut app, "insert into T values (1, 2), (3, 4)"); let actions = submit(&mut app); assert!( @@ -2860,7 +3172,7 @@ mod tests { let has_pointer = app .output .iter() - .any(|l| l.kind == OutputKind::Error && l.text.contains("advanced mode")); + .any(|l| l.kind == OutputKind::Error && l.text.contains("mode advanced")); assert!( has_pointer, "expected the advanced-mode pointer on submit; output:\n{}", @@ -2876,10 +3188,15 @@ mod tests { let mut app = App::new(); type_str(&mut app, "frobulate widgets"); let _ = submit(&mut app); + // The pointer's current phrasing (`also_valid_sql`) is + // "trying to write SQL? …"; an unknown command produces no + // advanced-mode hint at all, so we look for any line carrying + // the "mode advanced" actionable fragment that the pointer + // always emits. let has_pointer = app .output .iter() - .any(|l| l.text.contains("valid as SQL in advanced mode")); + .any(|l| l.text.contains("mode advanced")); assert!(!has_pointer, "unknown command must not point at advanced mode"); } diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs index ad789f2..4ee5135 100644 --- a/src/dsl/walker/mod.rs +++ b/src/dsl/walker/mod.rs @@ -1397,11 +1397,12 @@ fn dml_auto_column_diagnostics( diagnostics } -/// `insert_arity_mismatch` ERROR (ADR-0033 §8.1, sub-phase 3i). +/// `insert_arity_mismatch` ERROR (ADR-0033 §8.1, sub-phase 3i; Form B +/// branch added 2026-05-28 per issue #1 / ADR-0033 Amendment 5). /// -/// Walker pre-flight when the explicit `(column_name_list)` arity -/// disagrees with a row's arity — pre-empting the engine's -/// less-helpful "N values for M columns". Two row-source shapes: +/// Walker pre-flight when the value-tuple arity disagrees with the +/// expected arity — pre-empting the engine's less-helpful +/// "N values for M columns". Two row-source shapes: /// /// - **VALUES**: each `value_tuple` is checked independently; /// **each** offending row emits its own diagnostic on that tuple's @@ -1412,10 +1413,17 @@ fn dml_auto_column_diagnostics( /// projection isn't the first `select` token) — the engine still /// reports it; a false positive would be worse than deferring. /// -/// Only fires when an explicit column list is present; the -/// no-column-list form (arity vs the table's full column count) -/// is deferred (needs the schema and is outside the 3i exit gate). -fn dml_insert_arity_diagnostics(path: &MatchedPath) -> Vec { +/// Expected arity comes from one of two sources: +/// - **Form A** (explicit `(col, …)` list): the list's length. +/// - **Form B** (no column list): the target table's column count — +/// requires the schema and a known table; otherwise this pass is +/// silent and the engine reports the mismatch as before. This +/// covers the user-reported issue #1 case (advanced positional +/// INSERT with wrong value count). +fn dml_insert_arity_diagnostics( + path: &MatchedPath, + schema: Option<&crate::completion::SchemaCache>, +) -> Vec { use crate::dsl::grammar::IdentSource; use outcome::{Diagnostic, MatchedKind, Severity}; @@ -1432,9 +1440,36 @@ fn dml_insert_arity_diagnostics(path: &MatchedPath) -> Vec ) }) .count(); - if col_arity == 0 { - return Vec::new(); - } + + // Resolve the expected arity + which message template to use. + // Form A: explicit column list → its own length. + // Form B: no list → the target table's column count, *if* we know + // it. Without a schema or a recognised target the pass goes + // silent (the unknown-table case is flagged by the schema- + // existence pass instead). + let (expected, message_key): (usize, &'static str) = if col_arity > 0 { + (col_arity, "diagnostic.insert_arity_mismatch") + } else { + let Some(schema) = schema else { + return Vec::new(); + }; + let Some(target) = path.items.iter().find_map(|it| match it.kind { + MatchedKind::Ident { + source: IdentSource::Tables, + role: "insert_target_table", + } => Some(it.text.as_str()), + _ => None, + }) else { + return Vec::new(); + }; + let Some(cols) = schema.table_columns.get(target) else { + return Vec::new(); + }; + if cols.is_empty() { + return Vec::new(); + } + (cols.len(), "diagnostic.insert_arity_mismatch_form_b") + }; // Index of the row-source keyword (first VALUES / SELECT / WITH). let Some(kw_idx) = path @@ -1456,9 +1491,9 @@ fn dml_insert_arity_diagnostics(path: &MatchedPath) -> Vec severity: Severity::Error, span, message: crate::friendly::translate( - "diagnostic.insert_arity_mismatch", + message_key, &[ - ("expected", &col_arity as &dyn std::fmt::Display), + ("expected", &expected as &dyn std::fmt::Display), ("actual", &actual as &dyn std::fmt::Display), ], ), @@ -1483,7 +1518,7 @@ fn dml_insert_arity_diagnostics(path: &MatchedPath) -> Vec } MatchedKind::Punct(')') => { depth -= 1; - if depth == 0 && tuple_arity != col_arity { + if depth == 0 && tuple_arity != expected { emit((tuple_start, it.span.1), tuple_arity, &mut diagnostics); } } @@ -1522,7 +1557,7 @@ fn dml_insert_arity_diagnostics(path: &MatchedPath) -> Vec } } } - if proj_arity != col_arity { + if proj_arity != expected { emit(anchor.unwrap_or(path.items[kw_idx].span), proj_arity, &mut diagnostics); } } @@ -2483,10 +2518,13 @@ fn decide( // (ADR-0033 Amendment 3). "This is SQL" is reserved for // entry words with no DSL form (`select` / `with`): there // the DSL surface has nothing to offer. For a DSL line - // that fails here but *would* run in advanced mode, a - // "(valid as SQL in advanced mode)" pointer is appended at - // the rendering layer (see `advanced_alternative_note`), - // combining the DSL fix with the mode hint. + // that fails here but would be *valid* in advanced mode (the + // ADR-0027 sense, per ADR-0033 Amendment 5: verdict `None`, + // i.e. parse succeeds + no Warning/Error from any + // diagnostic pass), a "Trying to write SQL?" pointer is + // appended at the rendering layer (see + // `advanced_alternative_note`), combining the DSL fix with + // the mode hint. match simple.first() { Some(&(sidx, snode)) => Decision::Commit { idx: sidx, node: snode }, None => { @@ -2768,10 +2806,12 @@ fn walk_one_command<'a>( // ADR-0033 §8.2 — WARNING when a SQL INSERT lists a // serial/shortid (auto-generated) column explicitly. d.extend(dml_auto_column_diagnostics(&path, ctx.schema)); - // ADR-0033 §8.1 — ERROR when a column list's arity disagrees - // with a VALUES tuple (per row) or the INSERT…SELECT - // projection. - d.extend(dml_insert_arity_diagnostics(&path)); + // ADR-0033 §8.1 (+ Amendment 5, issue #1) — ERROR when the + // arity disagrees with a VALUES tuple (per row) or the + // INSERT…SELECT projection. Form A uses the column list's + // length; Form B uses the schema's column count for the + // target table. + d.extend(dml_insert_arity_diagnostics(&path, ctx.schema)); // ADR-0033 §8.3 — WARNING when an INSERT's column list omits // a NOT-NULL-no-default (non-auto-gen) column. d.extend(dml_not_null_missing_diagnostics(&path, ctx.schema)); @@ -5019,6 +5059,96 @@ mod tests { ); } + #[test] + fn insert_form_b_arity_mismatch_under_supply_fires() { + // Issue #1: advanced-mode positional INSERT without a column + // list requires a value for every column. Three values for a + // 4-column table (the user's reported bug case) is a + // mismatch — the diagnostic powers the validity verdict that + // drives the [ERR] indicator and ADR-0033 Amendment 5's + // `also_valid_sql` gate. + let schema = schema_with( + "Customers", + &[ + ("id", Type::Serial), + ("Name", Type::Text), + ("Age", Type::Int), + ("SerNo", Type::Serial), + ], + ); + let diags = diag_keys( + "insert into Customers values ('Oli', 52, 3)", + &schema, + ); + assert!( + diags.iter().any(|d| d.contains("value(s) are given")), + "Form B under-supply must fire arity diagnostic; got {diags:?}", + ); + } + + #[test] + fn insert_form_b_arity_mismatch_over_supply_fires() { + // Symmetric case: more values than columns — engine would + // reject; the diagnostic catches it pre-flight. + let schema = schema_with( + "Customers", + &[ + ("id", Type::Serial), + ("Name", Type::Text), + ("Age", Type::Int), + ("SerNo", Type::Serial), + ], + ); + let diags = diag_keys( + "insert into Customers values ('Oli', 52, 3, 13, 99)", + &schema, + ); + assert!( + diags.iter().any(|d| d.contains("value(s) are given")), + "Form B over-supply must fire arity diagnostic; got {diags:?}", + ); + } + + #[test] + fn insert_form_b_arity_match_is_silent() { + // The user's confirmed happy path: every column supplied + // positionally (autos included) — no arity diagnostic. + let schema = schema_with( + "Customers", + &[ + ("id", Type::Serial), + ("Name", Type::Text), + ("Age", Type::Int), + ("SerNo", Type::Serial), + ], + ); + let diags = diag_keys( + "insert into Customers values (13, 'Oli', 42, 13)", + &schema, + ); + assert!( + !diags.iter().any(|d| d.contains("value(s) are given")), + "matched Form B arity must not fire; got {diags:?}", + ); + } + + #[test] + fn insert_form_b_arity_unknown_table_is_silent() { + // Defensive: when the target table isn't in the schema cache, + // we can't know the expected count — defer to the engine + // (the schema-existence pass flags the unknown table, not the + // arity pass). + let schema = schema_with("Customers", &[("Name", Type::Text)]); + let diags = diag_keys( + "insert into Strangers values ('Oli', 52, 3)", + &schema, + ); + assert!( + !diags.iter().any(|d| d.contains("value(s) are given")), + "unknown table must not trigger an arity diagnostic; got {diags:?}", + ); + } + #[test] fn insert_arity_match_is_silent() { let schema = schema_with("t", &[("a", Type::Int), ("b", Type::Int)]); @@ -6211,8 +6341,10 @@ mod dispatch_3a_tests { // DSL candidate, even when the input matches only the SQL // tail. The DSL grammar then rejects the SQL-only tail with a // normal parse error (a `Mismatch`), surfacing the actionable - // DSL detail; the "(valid as SQL in advanced mode)" pointer is - // added at the rendering layer, not here. "This is SQL" as a + // DSL detail; the "Trying to write SQL?" pointer (ADR-0033 + // Amendment 3, gated per Amendment 5 on the line being *valid* + // in advanced mode — verdict `None`) is added at the rendering + // layer, not here. "This is SQL" as a // dispatch outcome is reserved for entry words with no DSL // form (see `simple_mode_sql_only_entry_word_is_this_is_sql`). let cands = shared(); diff --git a/src/friendly/keys.rs b/src/friendly/keys.rs index de22dab..d54e3f3 100644 --- a/src/friendly/keys.rs +++ b/src/friendly/keys.rs @@ -45,6 +45,12 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ ("diagnostic.duplicate_cte", &["name"]), ("diagnostic.eq_null", &[]), ("diagnostic.insert_arity_mismatch", &["expected", "actual"]), + // ADR-0033 §8.1 / Amendment 5: Form B (no column list) variant. + // Cited from issue #1 — every column must be supplied positionally. + ( + "diagnostic.insert_arity_mismatch_form_b", + &["expected", "actual"], + ), ("diagnostic.not_null_missing", &["column"]), ("diagnostic.like_numeric", &["column", "type"]), ("diagnostic.projection_alias_misplaced", &["alias", "clause"]), @@ -339,6 +345,20 @@ pub const KEYS_AND_PLACEHOLDERS: &[(&str, &[&str])] = &[ // ---- Advanced-mode SQL surface (ADR-0030) ---- ("advanced_mode.sql_in_simple", &["command"]), ("advanced_mode.also_valid_sql", &[]), + // Education note appended to a simple-mode INSERT Form B + // parse error when the user supplied more values than the + // non-auto-generated columns expect (issue #1 sub-task 2). + ( + "insert.form_b_extra_values_note", + &["table", "expected_phrase", "auto_phrase", "all_cols"], + ), + // Pre-flight teaching note for advanced-mode positional + // `INSERT INTO T VALUES (…)` (no column list) when the value + // count doesn't match the column count (issue #1 sub-task 3). + ( + "insert.form_b_positional_count_mismatch_note", + &["table", "col_count", "col_list", "supplied", "non_auto_csv"], + ), ("select.internal_table", &["table"]), ( "cli.invalid_value", diff --git a/src/friendly/strings/en-US.yaml b/src/friendly/strings/en-US.yaml index c5cdd72..cfd3710 100644 --- a/src/friendly/strings/en-US.yaml +++ b/src/friendly/strings/en-US.yaml @@ -573,6 +573,8 @@ diagnostic: # ADR-0033 §8 — Phase-3 DML diagnostic keys. auto_column_overridden: "column `{column}` is auto-generated (`{type}`); providing an explicit value bypasses the auto-counter and may collide with later auto-generated values" insert_arity_mismatch: "the column list names {expected} column(s) but {actual} value(s) are given" + # ADR-0033 §8.1 / Amendment 5: Form B (no column list) variant. + insert_arity_mismatch_form_b: "with no column list, all {expected} column(s) need a value but {actual} value(s) are given" not_null_missing: "column `{column}` is required (`NOT NULL`, no default); the statement will fail when run" # Engine-error translations: an engine-rejected SQL statement @@ -705,8 +707,22 @@ advanced_mode: sql_in_simple: "`{command}` is SQL — available in advanced mode. Switch with `mode advanced`, or prefix the line with `:` to run it once." # Appended to a simple-mode DSL error when the same line would run # in advanced mode — keeps the actionable DSL fix and adds the mode - # pointer (ADR-0033 Amendment 3). - also_valid_sql: "(valid as SQL in advanced mode — `mode advanced` or prefix `:`)" + # pointer (ADR-0033 Amendment 3). Suppressed by `advanced_alternative_note` + # for cases the advanced engine would also reject (issue #1). + also_valid_sql: "(trying to write SQL? switch with `mode advanced`, or prefix `:` to run once)" + +# ---- Insert education hints (issue #1 sub-task 2) ------------------- +# Appended to a simple-mode INSERT Form B parse error when the user +# supplied more values than Form B's non-auto-generated columns expect +# (but not more than every column). Teaches the contract that excludes +# `serial` / `shortid` columns and points at the column-list form. +insert: + form_b_extra_values_note: "`insert into {table} values (…)` expects {expected_phrase} — {auto_phrase}. To set them explicitly, list every column: `insert into {table} ({all_cols}) values (…)`." + # Pre-flight teaching note for advanced-mode positional + # `INSERT INTO T VALUES (…)` (no column list) when the value count + # doesn't match the column count (issue #1 sub-task 3). The override + # uses the non-auto columns so the user can omit serial/shortid. + form_b_positional_count_mismatch_note: "Positional `insert into {table} values (…)` requires a value for every column. `{table}` has {col_count} columns ({col_list}); you supplied {supplied}. To omit auto-generated columns, list the columns you want: `insert into {table} ({non_auto_csv}) values (…)`." # ---- SQL SELECT (advanced mode; ADR-0030 / ADR-0031) ---------------- select: diff --git a/src/input_render.rs b/src/input_render.rs index 2ac1147..b05bb3e 100644 --- a/src/input_render.rs +++ b/src/input_render.rs @@ -300,11 +300,24 @@ pub fn ambient_hint_in_mode( /// The `advanced_mode.also_valid_sql` pointer string, or `None`. /// /// Returns the pointer when a simple-mode line is a *definite* DSL -/// error (not merely incomplete) yet parses in advanced mode. Used to -/// combine the DSL fix with the mode hint — both while typing -/// (`ambient_hint_in_mode`) and on submit (`App::dispatch_dsl`) — so -/// the pointer reaches SQL constructs that surface only on submit, -/// e.g. `delete … returning` (ADR-0033 Amendment 3). +/// error (not merely incomplete) yet would be **valid** in advanced +/// mode (ADR-0033 Amendment 5). +/// +/// "Valid" is the ADR-0027 sense: the advanced-mode input-validity +/// verdict comes back as `None` (no Warning, no Error) after every +/// static check — parse, schema existence, type slots, INSERT arity, +/// predicate warnings, and any future addition. The pointer fires only +/// when switching modes is **actually** the next step the user can +/// take to make the line valid: a `Severity::Warning` or +/// `Severity::Error` from the advanced-mode pipeline means switching +/// wouldn't resolve the line, so the pointer would mislead. +/// +/// Issue #1 origin: previously the gate was a syntactic "would parse" +/// check, which fired for the user's reported case (4-col table, +/// 3 positional values) where the line parses but fails at the engine. +/// Now the gate reads `input_verdict_in_mode` so any static +/// rejection — including the Form B arity diagnostic added in this +/// same change (issue #1) — suppresses the pointer automatically. #[must_use] pub fn advanced_alternative_note( input: &str, @@ -317,10 +330,228 @@ pub fn advanced_alternative_note( if !definite_dsl_error { return None; } - if parse_command_with_schema_in_mode(input, cache, Mode::Advanced).is_ok() { - Some(crate::t!("advanced_mode.also_valid_sql")) + // The validity-verdict-driven gate (ADR-0033 Amendment 5): the + // line must be fully valid (verdict `None`) in advanced mode. + if crate::dsl::walker::input_verdict_in_mode(input, Some(cache), Mode::Advanced).is_some() + { + return None; + } + Some(crate::t!("advanced_mode.also_valid_sql")) +} + +/// Education note for the simple-mode INSERT Form B count-mismatch +/// case (issue #1 sub-task 2). +/// +/// When a simple-mode `insert into values (…)` (Form B) supplies +/// more values than the non-auto-generated columns of `` — but no +/// more than every column — the bare "expected `)`" parse error +/// doesn't explain the contract that excluded the serial / shortid +/// columns. This helper returns a note that names the columns Form B +/// expects values for, names the auto-generated ones it skips, and +/// shows the column-list (Form A) override. +/// +/// Returns `None` when the input doesn't match the pattern (e.g. the +/// advanced parse fails, the schema isn't known, the table has no +/// auto-generated columns, the value count is at or below the non-auto +/// count, or the value count exceeds the total column count — which is +/// a different error class the engine surfaces directly). +/// +/// Also `None` when the cross-mode `advanced_alternative_note` pointer +/// would fire for the same input: those two pieces of advice overlap +/// (both are escape hatches from the Form-B mismatch — one to advanced +/// mode, one to Form A) and showing both would clutter the error +/// without adding pedagogy. The cross-mode pointer wins because it +/// only fires when switching modes actually works (issue #1 sub-task +/// 1's gate); when it doesn't fire, this note steps in. +#[must_use] +pub fn form_b_extra_values_note( + input: &str, + cache: &crate::completion::SchemaCache, +) -> Option { + use crate::dsl::command::Command; + use crate::dsl::types::Type; + if advanced_alternative_note(input, cache).is_some() { + return None; + } + let parsed = parse_command_with_schema_in_mode(input, cache, Mode::Advanced).ok()?; + let Command::SqlInsert { + target_table, + listed_columns, + literal_rows, + .. + } = parsed + else { + return None; + }; + if !listed_columns.is_empty() || literal_rows.is_empty() { + return None; + } + let table_cols = cache.table_columns.get(&target_table)?; + let is_auto = |t: Type| matches!(t, Type::Serial | Type::ShortId); + let auto: Vec<&str> = table_cols + .iter() + .filter(|c| is_auto(c.user_type)) + .map(|c| c.name.as_str()) + .collect(); + if auto.is_empty() { + return None; + } + let non_auto: Vec<&str> = table_cols + .iter() + .filter(|c| !is_auto(c.user_type)) + .map(|c| c.name.as_str()) + .collect(); + // Defence in depth: a table that is all-auto has no non-auto + // column to talk about. The cross-mode-pointer gate above would + // normally shield us (the line works in advanced — the pointer + // fires and we return early), but the helper is `pub` and should + // not produce "expects 0 values for " on its own. + if non_auto.is_empty() { + return None; + } + // Fire whenever the supplied count differs from Form B's expected + // count — the teaching message is forward-looking (it states what + // Form B expects + the override path) so it works for under-supply, + // over-supply within the column range, and over-supply past the + // total column count alike (issue #1 siblings task — over- / + // under-supply previously fell through to the bare parse error). + let value_count = literal_rows[0].len(); + if value_count == non_auto.len() { + return None; + } + let expected_phrase = if non_auto.len() == 1 { + format!("1 value for {}", quote_join_and(&non_auto)) } else { - None + format!( + "{count} values for {names}", + count = non_auto.len(), + names = quote_join_and(&non_auto), + ) + }; + let auto_phrase = if auto.len() == 1 { + format!( + "{name} is auto-generated and filled automatically", + name = quote_join_and(&auto), + ) + } else { + format!( + "{names} are auto-generated and filled automatically", + names = quote_join_and(&auto), + ) + }; + let all_cols = table_cols + .iter() + .map(|c| c.name.as_str()) + .collect::>() + .join(", "); + Some(crate::t!( + "insert.form_b_extra_values_note", + table = target_table, + expected_phrase = expected_phrase, + auto_phrase = auto_phrase, + all_cols = all_cols, + )) +} + +/// Pre-flight note for advanced-mode `insert into values (…)` +/// (positional, no column list) when the value count doesn't match +/// the table's column count (issue #1 sub-task 3). +/// +/// The advanced-mode SQL grammar accepts any positional value count; +/// the engine then rejects the line with a NOT-NULL / type-mismatch +/// error that doesn't tell the user about the column-list override. +/// This helper detects the case at dispatch time and returns a note +/// that names the rule, lists the target table's columns, and shows +/// the column-list (Form A) override. +/// +/// Returns `None` when the input doesn't match the pattern (the parse +/// isn't a Form B `SqlInsert`, the schema isn't known, the table has +/// no auto-generated columns or no non-auto columns, the literal-row +/// shape is empty, or every row's value count already matches the +/// column count). +/// +/// Conservative on multi-row VALUES: fires only when **every** row's +/// value count is the same wrong number. Mixed-length rows fall +/// through to the engine error — they're a different shape of mistake. +#[must_use] +pub fn form_b_positional_count_mismatch_note( + parsed: &crate::dsl::command::Command, + cache: &crate::completion::SchemaCache, +) -> Option { + use crate::dsl::command::Command; + use crate::dsl::types::Type; + let Command::SqlInsert { + target_table, + listed_columns, + literal_rows, + .. + } = parsed + else { + return None; + }; + if !listed_columns.is_empty() || literal_rows.is_empty() { + return None; + } + let table_cols = cache.table_columns.get(target_table)?; + let col_count = table_cols.len(); + let row_lens: Vec = literal_rows.iter().map(Vec::len).collect(); + // Only fire when every row is the same (wrong) length. + let first_len = row_lens[0]; + if !row_lens.iter().all(|n| *n == first_len) { + return None; + } + if first_len == col_count { + return None; + } + let is_auto = |t: Type| matches!(t, Type::Serial | Type::ShortId); + // The override only makes sense when (a) there's something to + // skip and (b) something to list. A table that is all-auto can't + // omit anything via the column list; a table with no autos + // doesn't benefit from the column-list form for this purpose. + let has_auto = table_cols.iter().any(|c| is_auto(c.user_type)); + if !has_auto { + return None; + } + let non_auto: Vec<&str> = table_cols + .iter() + .filter(|c| !is_auto(c.user_type)) + .map(|c| c.name.as_str()) + .collect(); + if non_auto.is_empty() { + return None; + } + let all_cols_list = quote_join_and( + &table_cols + .iter() + .map(|c| c.name.as_str()) + .collect::>(), + ); + let non_auto_csv = non_auto.join(", "); + Some(crate::t!( + "insert.form_b_positional_count_mismatch_note", + table = target_table, + col_count = col_count, + col_list = all_cols_list, + supplied = first_len, + non_auto_csv = non_auto_csv, + )) +} + +/// Format `items` as an English-prose list with backtick-quoted +/// identifiers: `["a"]` → `` `a` ``, `["a","b"]` → `` `a` and `b` ``, +/// `["a","b","c"]` → `` `a`, `b`, and `c` `` (Oxford comma). +fn quote_join_and(items: &[&str]) -> String { + match items { + [] => String::new(), + [only] => format!("`{only}`"), + [a, b] => format!("`{a}` and `{b}`"), + rest => { + let head: Vec = rest[..rest.len() - 1] + .iter() + .map(|s| format!("`{s}`")) + .collect(); + format!("{}, and `{}`", head.join(", "), rest[rest.len() - 1]) + } } } @@ -1124,8 +1355,10 @@ mod tests { // The DSL detail survives … assert!(p.contains("Name"), "expected DSL slot detail, got: {p:?}"); // … and the advanced-mode pointer is appended. + // Substring "mode advanced" is the actionable fragment + // (the switch command) that survives wording revisions. assert!( - p.contains("advanced mode"), + p.contains("mode advanced"), "expected the advanced-mode pointer, got: {p:?}", ); } @@ -1145,12 +1378,40 @@ mod tests { let input = "insert into Customers values ('Alice')"; if let Some(AmbientHint::Prose(p)) = ambient_hint(input, input.len(), None, &cache) { assert!( - !p.contains("advanced mode"), + !p.contains("mode advanced"), "a valid DSL command must not carry the advanced pointer, got: {p:?}", ); } } + #[test] + fn ambient_hint_omits_advanced_pointer_when_form_b_value_count_wouldnt_match() { + // Issue #1: simple-mode rejects `insert into Customers values + // ('Oli', 52, 3)` because Form B skips both serials and expects + // 2 values for `Name`, `Age`. The same line ALSO fails in + // advanced mode: positional VALUES requires every column (4 + // total) but only 3 were supplied. The cross-mode pointer would + // be misleading — switching modes wouldn't help — so it must + // not be appended. + use crate::dsl::types::Type; + let cache = schema_with_columns( + "Customers", + &[ + ("id", Type::Serial), + ("Name", Type::Text), + ("Age", Type::Int), + ("SerNo", Type::Serial), + ], + ); + let input = "insert into Customers values ('Oli', 52, 3)"; + let note = advanced_alternative_note(input, &cache); + assert!( + note.is_none(), + "Form B mismatch where advanced mode would also reject — \ + the cross-mode pointer must be suppressed; got: {note:?}", + ); + } + #[test] fn ambient_hint_at_insert_second_value_shows_text_prose() { use crate::dsl::types::Type; diff --git a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_extra_value_for_serial_column_is_invalid@form_b_extra_serial_value.snap b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_extra_value_for_serial_column_is_invalid@form_b_extra_serial_value.snap index f60a8c5..a70709f 100644 --- a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_extra_value_for_serial_column_is_invalid@form_b_extra_serial_value.snap +++ b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_extra_value_for_serial_column_is_invalid@form_b_extra_serial_value.snap @@ -1,5 +1,6 @@ --- source: tests/typing_surface/insert_form_b.rs +assertion_line: 186 description: "input=\"insert into Customers values (1, 'Alice', 'a@b.c')\" cursor=50" expression: "& a" --- @@ -11,7 +12,7 @@ Assessment { ), hint: Some( Prose( - "for `Name`: Type a quoted string (e.g. 'Alice') or null (`id` auto-generated — skipped here; list columns explicitly, e.g. `insert into T (...) values (...)`, to set it.) (valid as SQL in advanced mode — `mode advanced` or prefix `:`)", + "for `Name`: Type a quoted string (e.g. 'Alice') or null (`id` auto-generated — skipped here; list columns explicitly, e.g. `insert into T (...) values (...)`, to set it.) (trying to write SQL? switch with `mode advanced`, or prefix `:` to run once)", ), ), completion: Some(