From fa5d0dc6da7c424a3f633ecb87f34908e9daf623 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Fri, 29 May 2026 10:22:57 +0000 Subject: [PATCH] fix: insert VALUES between-values hint points at comma not close-paren MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ambient-hint fallback in ambient_hint_core_in_mode parsed schemalessly, so the type-blind grammar closed an `insert … values (…)` tuple after the first value and the "Next:" hint pointed at `)`. With a schema available the walk knows the remaining columns and the correct next token is `,`. Parse the fallback with the schema cache so the expected-token prose matches the rest of the (already schema-aware) hint ladder. Also corrects wrong-arity closed tuples where the schemaless parse accepted the input and the hint said "submit with Enter" for a command the schema-aware parse rejects — the hint now surfaces the accurate error. Three typing-surface snapshots updated to match. Docs: ADR-0022 Amendment 3 (+ README index) records the schema-aware fallback; requirements.md H1a cites the hint-accuracy improvement. --- docs/adr/0022-ambient-typing-assistance.md | 38 ++++++ docs/adr/README.md | 2 +- docs/requirements.md | 10 +- src/input_render.rs | 109 +++++++++++++++++- ...complete@form_a_in_progress_one_value.snap | 3 +- ..._at_close_paren@form_b_too_few_values.snap | 3 +- ...e_count_is_invalid@form_c_wrong_count.snap | 3 +- 7 files changed, 161 insertions(+), 7 deletions(-) diff --git a/docs/adr/0022-ambient-typing-assistance.md b/docs/adr/0022-ambient-typing-assistance.md index 170c4e4..aa15796 100644 --- a/docs/adr/0022-ambient-typing-assistance.md +++ b/docs/adr/0022-ambient-typing-assistance.md @@ -549,6 +549,44 @@ ordering; `order_by_after_sort_item_offers_direction`, `create_table_after_column_spec_offers_constraints` lock the trailing-optional fix. ~20 typing-surface snapshots re-baselined. +## Amendment 3 — Ambient-hint fallback parses with the schema (2026-05-29) + +Amendment 1's "What changed" listed *the fallback +`parse_command_in_mode`* among the sub-calls threaded through the +active `mode`. That fallback (the bottom rung of `ambient_hint`'s +candidate-or-prose ladder, which produces the `ambient_complete` / +`ambient_expected` / `ambient_error_with_usage` prose) was +**schemaless** — it took the input and mode but not the `SchemaCache`, +even though every earlier rung (`input_diagnostics_in_mode`, +`hint_resolution_at_input_in_mode`, `candidates_at_cursor_in_mode`) +already ran schema-aware. That was an oversight, not a decision: the +ADR's whole intent is a schema- and mode-consistent hint surface. + +The gap surfaced as a user-reported bug (issue #2). Between two values +of an `insert … values (…)` tuple, the type-blind (schemaless) grammar +closes the tuple after one value, so the expected-token prose pointed +at `)` when the schema-aware grammar — knowing the remaining columns — +expects `,`. The same schemaless parse also accepted wrong-arity closed +tuples as complete, so the prose read *"submit with Enter"* for an +input the schema-aware parse (and the on-submit path) rejects. + +**Change:** the fallback rung now calls +`parse_command_with_schema_in_mode(input, cache, mode)`. The whole +ladder is now schema-consistent. No new walk — the call site already +held the `cache`; this swaps which parse function consumes it. + +**What still holds:** the friendly arity diagnostic (ADR-0033 §8.1) is +checked at a higher rung, so where it fires (advanced-mode wrong-arity +tuples) it still wins over this fallback — locked by +`advanced_mode_wrong_arity_insert_keeps_friendly_diagnostic_over_fallback`. +The §13 one-walk-per-render posture is unchanged. + +**Coverage:** `ambient_hint_between_values_points_to_comma_not_close_paren`, +`ambient_hint_after_last_value_points_to_close_paren`, the no-masking +guard above, and three re-baselined `typing_surface` snapshots +(`form_a_in_progress_one_value`, `form_b_too_few_values`, +`form_c_wrong_count`). + ## Out of scope Deliberately deferred to keep this ADR shippable as a single diff --git a/docs/adr/README.md b/docs/adr/README.md index a66d5bd..88eebd2 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -27,7 +27,7 @@ This directory contains the project's ADRs, recorded per - [ADR-0019 — Friendly error layer (H1) and i18n message catalog](0019-friendly-error-layer-and-i18n.md) - [ADR-0020 — Tokenization layer for the DSL parser](0020-tokenization-layer-for-the-dsl-parser.md) - [ADR-0021 — Parser-as-source-of-truth for H1a (per-command usage in parse errors)](0021-parser-as-source-of-truth-for-h1a.md) -- [ADR-0022 — Ambient typing assistance: colour, hint panel, completion (I3 + I4)](0022-ambient-typing-assistance.md) — **Amendment 1 supersedes §12's simple-mode-only carve-out**: the unified mode-aware walker (ADR-0030/0031/0032) now speaks SQL, so advanced-mode ambient assistance is re-enabled. `ambient_hint_in_mode` + `hint_resolution_at_input_in_mode` + `expected_for_hint_snapshot` thread `Mode`; `render_hint_panel` calls ambient for all modes (no more advanced-mode `None`); the one-shot `:` sigil is stripped before the ambient walk. Fixes a live bug where advanced-mode SQL hinting/completion-preview were dead despite Phase 2 marking them green (validated at the engine layer, not the UI). Simple-mode gating, highlighting, and the §13 performance posture are unchanged; covered by an app-level render test plus ambient-layer regression locks; **Amendment 2 reverses the handoff-14 keywords-first candidate ordering** — schema identifiers (table/column/relationship names) now sort *before* keywords so a name the user would have to look up stays visible in the single-row, window-scrolled candidate line (keywords are learned over time; the `tok_identifier`/`tok_keyword` colour split marks the boundary); shipped with a `walk_repeated` fix that surfaces a list item's trailing optionals at a clean boundary (`order by Name ` → `asc`/`desc`, `select Name ` → `as`, `create table … Code(text) ` → `not`/`unique`/`default`/`check`; the `,` separator deliberately not surfaced); records a deferred two-line hint box for growing lists +- [ADR-0022 — Ambient typing assistance: colour, hint panel, completion (I3 + I4)](0022-ambient-typing-assistance.md) — **Amendment 1 supersedes §12's simple-mode-only carve-out**: the unified mode-aware walker (ADR-0030/0031/0032) now speaks SQL, so advanced-mode ambient assistance is re-enabled. `ambient_hint_in_mode` + `hint_resolution_at_input_in_mode` + `expected_for_hint_snapshot` thread `Mode`; `render_hint_panel` calls ambient for all modes (no more advanced-mode `None`); the one-shot `:` sigil is stripped before the ambient walk. Fixes a live bug where advanced-mode SQL hinting/completion-preview were dead despite Phase 2 marking them green (validated at the engine layer, not the UI). Simple-mode gating, highlighting, and the §13 performance posture are unchanged; covered by an app-level render test plus ambient-layer regression locks; **Amendment 2 reverses the handoff-14 keywords-first candidate ordering** — schema identifiers (table/column/relationship names) now sort *before* keywords so a name the user would have to look up stays visible in the single-row, window-scrolled candidate line (keywords are learned over time; the `tok_identifier`/`tok_keyword` colour split marks the boundary); shipped with a `walk_repeated` fix that surfaces a list item's trailing optionals at a clean boundary (`order by Name ` → `asc`/`desc`, `select Name ` → `as`, `create table … Code(text) ` → `not`/`unique`/`default`/`check`; the `,` separator deliberately not surfaced); records a deferred two-line hint box for growing lists; **Amendment 3 makes the ambient-hint fallback rung schema-aware** — Amendment 1's bottom-rung `parse_command_in_mode` was schemaless while every earlier rung was not, so between-values insert hints pointed at `)` (type-blind close) instead of `,` and wrong-arity closed tuples read "submit with Enter" for an input the schema-aware parse rejects (issue #2); now uses `parse_command_with_schema_in_mode`, no extra walk, with the friendly arity diagnostic still winning at its higher rung - [ADR-0023 — Unified declarative grammar tree](0023-unified-grammar-tree.md) — direction (superseded for execution detail by ADR-0024) - [ADR-0024 — Unified grammar tree: execution plan](0024-unified-grammar-tree-execution-plan.md) — **Accepted**, the executable spec — implemented (Phases A–F; Phase F shipped "minimal", `parser.rs` retained as the router — see the ADR's Phase F implementation note) - [ADR-0025 — Indexes](0025-indexes.md) — **Accepted** (**Amendment 1, 2026-05-25**: UNIQUE indexes admitted on the **advanced-mode** surface via `CREATE UNIQUE INDEX` — ADR-0035 §4d; the `IndexSchema.unique` flag round-trips through `project.yaml` with no new metadata table since the engine reports uniqueness natively; simple-mode `add unique index` stays deferred), `add index` / `drop index`, persistence, rebuild-table preservation, and items-list display (`C3` index portion + `S2`) diff --git a/docs/requirements.md b/docs/requirements.md index 18c26c4..24fcc84 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -535,7 +535,15 @@ handoff-14 cleanup; 449 after B2/C2.) 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. + time; **issue #2 / ADR-0022 Amendment 3, 2026-05-29** made the + ambient-hint fallback rung schema-aware so the "Next:" prose + names the schema-correct next token (`,` between values, `)` + after the last) instead of the type-blind close-paren, and so a + wrong-arity closed tuple surfaces the real parse error rather + than a misleading "submit with Enter"). A systematic pass is + still pending; simple-mode wrong-count tuples still get a generic + expected-token message rather than the friendly arity diagnostic + advanced mode shows (issue #17). - [ ] **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/input_render.rs b/src/input_render.rs index dfc2b7f..89fc860 100644 --- a/src/input_render.rs +++ b/src/input_render.rs @@ -24,7 +24,7 @@ use ratatui::style::{Color, Modifier, Style}; -use crate::dsl::parser::{parse_command_in_mode, parse_command_with_schema, parse_command_with_schema_in_mode}; +use crate::dsl::parser::{parse_command_with_schema, parse_command_with_schema_in_mode}; use crate::mode::Mode; use crate::dsl::walker; use crate::dsl::{ParseError, parse_command}; @@ -748,7 +748,15 @@ fn ambient_hint_core_in_mode( // parsed in the active `mode` (ADR-0022 Amendment 1). In // simple mode a SQL form still surfaces the "this is SQL" // hint (ADR-0030 §2); in advanced mode it parses as SQL. - match parse_command_in_mode(input, mode) { + // + // Issue #2: parse *with the schema* so the expected-token prose + // reflects the schema-aware grammar. Between two values of an + // `insert … values (…)` tuple, the type-blind (schemaless) grammar + // closes the tuple after one value and points at `)`; the + // schema-aware walk knows the remaining columns and correctly + // points at `,`. The other hint paths above already use the cache, + // so this keeps the whole ladder schema-consistent. + match parse_command_with_schema_in_mode(input, cache, mode) { Ok(_) => Some(AmbientHint::Prose(crate::t!("hint.ambient_complete"))), Err(ParseError::Empty) => None, Err(ParseError::Invalid { @@ -1785,6 +1793,103 @@ mod tests { } } + #[test] + fn ambient_hint_between_values_points_to_comma_not_close_paren() { + // Issue #2: at the cursor just after a completed first value + // (no comma yet) the between-values fallback hint must reflect + // the SCHEMA-AWARE grammar — there is a second column still to + // fill, so the meaningful next token is `,`, not `)`. The bug + // was that this fallback parsed schemalessly, so the type-blind + // grammar closed the tuple after one value and pointed at `)`. + // Not literal-kind-specific: each case below uses a value that + // is type-correct for the first column, so the only difference + // from the report's string case is the literal shape. + use crate::dsl::types::Type; + let cases: &[(&[(&str, Type)], &str)] = &[ + // string first value (the report's case): first col text. + (&[("Name", Type::Text), ("Age", Type::Int)], + "insert into Customers values ('Oli'"), + // integer first value: first col int. + (&[("Age", Type::Int), ("Name", Type::Text)], + "insert into Customers values (42"), + // real first value: first col real. + (&[("Score", Type::Real), ("Name", Type::Text)], + "insert into Customers values (3.5"), + ]; + for (cols, input) in cases { + let cache = schema_with_columns("Customers", cols); + match ambient_hint(input, input.len(), None, &cache) { + Some(AmbientHint::Prose(p)) => { + assert!( + p.contains(','), + "expected a comma next-token hint for {input:?}, got: {p:?}", + ); + assert!( + !p.contains(')'), + "must not point at the closing paren mid-tuple for \ + {input:?}, got: {p:?}", + ); + } + other => panic!("expected Prose for {input:?}, got {other:?}"), + } + } + } + + #[test] + fn advanced_mode_wrong_arity_insert_keeps_friendly_diagnostic_over_fallback() { + // Issue #2 no-masking guard. In advanced mode a wrong-arity + // insert tuple structurally matches via the type-blind path and + // the walker emits the friendly `insert_arity_mismatch_form_b` + // diagnostic (ADR-0033 §8.1 / Amendment 5). That diagnostic is + // checked EARLY in the hint ladder, so the issue-#2 schema-aware + // fallback (which would otherwise say "expected `)`") must NOT + // shadow it. Locks ladder ordering so the fix can't regress the + // richer message. + 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)"; + match ambient_hint_in_mode(input, input.len(), None, &cache, Mode::Advanced) { + Some(AmbientHint::Prose(p)) => { + assert!( + p.contains("value(s) are given"), + "expected the friendly arity diagnostic, got: {p:?}", + ); + } + other => panic!("expected Prose, got {other:?}"), + } + } + + #[test] + fn ambient_hint_after_last_value_points_to_close_paren() { + // Counterpart to the issue #2 fix: once every column has a + // value, the schema-aware fallback SHOULD point at `)` — there + // is nothing left to fill. Guards against over-correcting the + // fix into never suggesting the close paren. + use crate::dsl::types::Type; + let cache = schema_with_columns( + "Customers", + &[("Name", Type::Text), ("Age", Type::Int)], + ); + let input = "insert into Customers values ('Oli', 52"; + match ambient_hint(input, input.len(), None, &cache) { + Some(AmbientHint::Prose(p)) => { + assert!( + p.contains(')'), + "expected a close-paren next-token hint, got: {p:?}", + ); + } + other => panic!("expected Prose, got {other:?}"), + } + } + #[test] fn ambient_hint_at_value_slot_falls_back_to_generic_without_schema() { // Empty cache: the walker can't resolve the column type diff --git a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_a__form_a_in_progress_with_one_value_is_incomplete@form_a_in_progress_one_value.snap b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_a__form_a_in_progress_with_one_value_is_incomplete@form_a_in_progress_one_value.snap index 37a7a22..8f817a3 100644 --- a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_a__form_a_in_progress_with_one_value_is_incomplete@form_a_in_progress_one_value.snap +++ b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_a__form_a_in_progress_with_one_value_is_incomplete@form_a_in_progress_one_value.snap @@ -1,5 +1,6 @@ --- source: tests/typing_surface/insert_form_a.rs +assertion_line: 243 description: "input=\"insert into Orders (OrderId, CustId, Total) values (42\" cursor=54" expression: "& a" --- @@ -9,7 +10,7 @@ Assessment { state: IncompleteAtEof, hint: Some( Prose( - "Next: `)`", + "Next: `,`", ), ), completion: None, diff --git a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_too_few_values_is_invalid_at_close_paren@form_b_too_few_values.snap b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_too_few_values_is_invalid_at_close_paren@form_b_too_few_values.snap index d6936e0..f302076 100644 --- a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_too_few_values_is_invalid_at_close_paren@form_b_too_few_values.snap +++ b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_b__form_b_with_too_few_values_is_invalid_at_close_paren@form_b_too_few_values.snap @@ -1,5 +1,6 @@ --- source: tests/typing_surface/insert_form_b.rs +assertion_line: 168 description: "input=\"insert into Customers values ('Alice')\" cursor=38" expression: "& a" --- @@ -11,7 +12,7 @@ Assessment { ), hint: Some( Prose( - "Submit with Enter", + "after `insert into Customers values ('Alice'`, expected `,` — usage: insert into [([, ...])] [values] ([, ...])", ), ), completion: None, diff --git a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_c__form_c_wrong_value_count_is_invalid@form_c_wrong_count.snap b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_c__form_c_wrong_value_count_is_invalid@form_c_wrong_count.snap index 60ffa80..c8f1c22 100644 --- a/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_c__form_c_wrong_value_count_is_invalid@form_c_wrong_count.snap +++ b/tests/typing_surface/snapshots/typing_surface_matrix__typing_surface__insert_form_c__form_c_wrong_value_count_is_invalid@form_c_wrong_count.snap @@ -1,5 +1,6 @@ --- source: tests/typing_surface/insert_form_c.rs +assertion_line: 101 description: "input=\"insert into Customers ('Alice', 'a@b.c', 'extra')\" cursor=49" expression: "& a" --- @@ -11,7 +12,7 @@ Assessment { ), hint: Some( Prose( - "Submit with Enter", + "after `insert into Customers ('Alice', 'a@b.c'`, expected `)` — usage: insert into
[([, ...])] [values] ([, ...])", ), ), completion: None,