diff --git a/docs/adr/0022-ambient-typing-assistance.md b/docs/adr/0022-ambient-typing-assistance.md index 03fba23..eaf70da 100644 --- a/docs/adr/0022-ambient-typing-assistance.md +++ b/docs/adr/0022-ambient-typing-assistance.md @@ -368,6 +368,87 @@ If profiling later shows render-loop pressure, options are (b) lex+parse only on input change, not every render. Both are local optimisations; not part of this ADR. +## Amendment 1 — Advanced-mode ambient assistance re-enabled (2026-05-21) + +This amendment **supersedes §12's carve-out** that ambient +typing assistance is a simple-mode-only feature. + +### The obsolete premise + +§12 disabled ambient assistance in advanced mode because, at the +time, "the DSL lexer + parser don't speak SQL; using them for SQL +input would mark almost everything as Identifier/Error and +mislead." That premise no longer holds. ADR-0030 / ADR-0031 / +ADR-0032 moved the SQL surface into the **same unified, mode-aware +walker grammar** (ADR-0023 / ADR-0024). The walker now speaks SQL: +in `Mode::Advanced` it parses `SELECT` (and, from ADR-0033, DML), +highlights SQL keywords, resolves slot hints, and produces +completion candidates — exactly the inputs ambient assistance +needs. + +### The bug this fixes + +Despite the walker being mode-aware, the **UI never surfaced** +advanced-mode ambient assistance: + +- `render_hint_panel` hard-returned `None` for advanced mode (the + stale §12 gate), so the hint panel showed only `panel.hint_empty` + — no prose hints and no candidate preview for SQL. +- The hint resolver (`hint_resolution_at_input` → + `expected_for_hint_snapshot`) and `ambient_hint` never threaded + the mode, so even the engine-level calls defaulted to + `Mode::Simple` and gated a SQL statement as "this is SQL". + +The result: in advanced mode, hinting and completion-preview for +SQL were completely dead, even for a bare `SELECT`. + +This gap survived Phase 2 because ADR-0032's cross-cut matrix rows +for "Tab completion works for SQL keywords" / "Hint-panel prose +appears at every SQL slot" were validated by **engine-level** tests +(`completion_probe_in_mode(…, Advanced)`, `hint_mode_*` called +directly) — which prove the walker *can* produce SQL +hints/candidates but never exercise the UI that suppressed them. +This is the "free-for-free claim shipping without a real-app test" +failure mode the project's process pins call out. + +### What changed + +- **`ambient_hint_in_mode(input, cursor, memo, cache, mode)`** — + the mode-aware ambient entry point. `ambient_hint` is now a thin + wrapper that forwards `Mode::Simple`. Its sub-calls + (`input_diagnostics_in_mode`, `hint_resolution_at_input_in_mode`, + `candidates_at_cursor_in_mode`, the fallback `parse_command_in_mode`) + all run in the supplied `mode`. +- **`hint_resolution_at_input_in_mode` + `expected_for_hint_snapshot`** + now set `ctx.mode`, so the hint walk respects the active mode. +- **`render_hint_panel`** calls `ambient_hint_in_mode` with the + effective mode for *all* modes (no more advanced-mode `None`). +- **One-shot `:` handling.** In one-shot advanced mode the raw + input carries the `:` sigil, which is not part of the grammar. + The panel strips it (mirroring `App::submit`) before the ambient + walk, so `: sel` hints `select` rather than the sigil. + +### What still holds + +- **Simple mode is unchanged.** The simple-mode entry point keeps + gating SQL as "this is SQL"; advanced assistance is opt-in via + mode, never leaked into simple mode (regression-locked). +- **Syntax highlighting** already ran with `Mode::Advanced` and is + unaffected. +- **The validity indicator** was already mode-aware (ADR-0032 + §10.6); this amendment aligns the ambient hint panel with it. +- **§13 performance posture** is unchanged — one walk per render, + now in the active mode. + +### Coverage + +App-level regression at the layer Phase 2 missed: +`src/ui.rs::advanced_mode_hint_panel_surfaces_sql_candidates` +(renders the panel in advanced mode and asserts the FROM-slot +table candidate appears). Ambient-layer locks: +`src/input_render.rs::advanced_mode_ambient_offers_sql_from_slot_candidate` +and `simple_mode_ambient_does_not_surface_sql_candidates`. + ### 14. Catalog additions ```yaml diff --git a/docs/adr/README.md b/docs/adr/README.md index 4f1f1e0..e0d2b8b 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) +- [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 - [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**, `add index` / `drop index`, persistence, rebuild-table preservation, and items-list display (`C3` index portion + `S2`) diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs index f323752..85eb9c2 100644 --- a/src/dsl/walker/mod.rs +++ b/src/dsl/walker/mod.rs @@ -97,10 +97,25 @@ pub struct HintResolution { pub fn hint_resolution_at_input( source: &str, schema: Option<&crate::completion::SchemaCache>, +) -> Option { + hint_resolution_at_input_in_mode(source, schema, crate::mode::Mode::Simple) +} + +/// Mode-aware hint resolver (ADR-0022 Amendment 1). +/// +/// Walks `source` in `mode` so advanced-mode SQL resolves slot +/// hints instead of being gated by the simple-mode "this is SQL" +/// path. The no-mode [`hint_resolution_at_input`] defaults to +/// `Mode::Simple`. +#[must_use] +pub fn hint_resolution_at_input_in_mode( + source: &str, + schema: Option<&crate::completion::SchemaCache>, + mode: crate::mode::Mode, ) -> Option { use crate::dsl::grammar::HintMode; - let snap = expected_for_hint_snapshot(source, schema); + let snap = expected_for_hint_snapshot(source, schema, mode); // Empty expected set means the command is already complete // (`WalkOutcome::Match`) — no slot to hint at. if snap.expected.is_empty() { @@ -1608,6 +1623,7 @@ struct HintWalkSnapshot { fn expected_for_hint_snapshot( source: &str, schema: Option<&crate::completion::SchemaCache>, + mode: crate::mode::Mode, ) -> HintWalkSnapshot { use crate::dsl::grammar::REGISTRY; @@ -1633,6 +1649,7 @@ fn expected_for_hint_snapshot( let mut ctx = schema.map_or_else(context::WalkContext::new, |s| { context::WalkContext::with_schema(s) }); + ctx.mode = mode; let (result, _cmd) = walk(source, outcome::WalkBound::EndOfInput, &mut ctx); let Some(result) = result else { return empty_snapshot(); diff --git a/src/input_render.rs b/src/input_render.rs index bf3808c..a490751 100644 --- a/src/input_render.rs +++ b/src/input_render.rs @@ -215,8 +215,9 @@ pub enum AmbientHint { }, } -/// Compute the ambient hint for the input panel -/// (ADR-0022 §6). +/// Compute the simple-mode ambient hint for the input panel +/// (ADR-0022 §6). Thin wrapper over [`ambient_hint_in_mode`]; +/// advanced-mode callers pass the active mode instead. /// /// Returns `None` for empty input — caller falls back to /// `panel.hint_empty`. @@ -226,6 +227,27 @@ pub fn ambient_hint( cursor: usize, memo: Option<&crate::completion::LastCompletion>, cache: &crate::completion::SchemaCache, +) -> Option { + ambient_hint_in_mode(input, cursor, memo, cache, Mode::Simple) +} + +/// Mode-aware ambient hint for the input panel (ADR-0022 +/// Amendment 1). +/// +/// Walks the input in `mode` so advanced-mode SQL surfaces slot +/// hints + completion candidates instead of the simple-mode +/// "this is SQL" gate. The simple-mode entry point [`ambient_hint`] +/// forwards here with `Mode::Simple`. +/// +/// Returns `None` for empty input — caller falls back to +/// `panel.hint_empty`. +#[must_use] +pub fn ambient_hint_in_mode( + input: &str, + cursor: usize, + memo: Option<&crate::completion::LastCompletion>, + cache: &crate::completion::SchemaCache, + mode: Mode, ) -> Option { if input.trim().is_empty() { return None; @@ -254,7 +276,7 @@ pub fn ambient_hint( // (the panel explains where the user is looking), else the // most severe one. The error overlay still marks every // flagged token; this panel carries the *why*. - let diagnostics = crate::dsl::walker::input_diagnostics(input, Some(cache)); + let diagnostics = crate::dsl::walker::input_diagnostics_in_mode(input, Some(cache), mode); if let Some(diag) = pick_hint_diagnostic(&diagnostics, cursor.min(input.len())) { return Some(AmbientHint::Prose(diag.message.clone())); } @@ -275,7 +297,7 @@ pub fn ambient_hint( // (Date → "Type a date as 'YYYY-MM-DD'", etc.) and surface // the column name when the walker has it bound. let resolution = - crate::dsl::walker::hint_resolution_at_input(leading, Some(cache)); + crate::dsl::walker::hint_resolution_at_input_in_mode(leading, Some(cache), mode); match resolution.as_ref().map(|r| r.mode) { Some(crate::dsl::grammar::HintMode::ProseOnly(key)) => { // The cursor sits at a slot where Tab candidates @@ -340,12 +362,12 @@ pub fn ambient_hint( // Candidates win when any exist — the panel surfaces them // directly because they're more actionable than prose // framings. - // `ambient_hint` is only called for `EffectiveMode::Simple` - // (ui.rs gates it), so completion runs through the - // simple-mode walker view — `select` does not surface here - // (ADR-0030 §2). + // Candidate completion runs through the `mode`-aware walker + // view (ADR-0022 Amendment 1): in advanced mode SQL keywords + // and schema candidates surface; in simple mode `select` is + // gated as "this is SQL" (ADR-0030 §2). if let Some(comp) = - crate::completion::candidates_at_cursor_in_mode(input, cursor, cache, Mode::Simple) + crate::completion::candidates_at_cursor_in_mode(input, cursor, cache, mode) { return Some(AmbientHint::Candidates { items: comp.candidates, @@ -373,10 +395,11 @@ pub fn ambient_hint( found = inv.found, ))); } - // Otherwise fall back to the prose framings from stage 5. - // ADR-0030 §2: simple-mode hint uses the simple-mode walker - // view so a SQL form surfaces with the "this is SQL" hint. - match parse_command_in_mode(input, Mode::Simple) { + // Otherwise fall back to the prose framings from stage 5, + // 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) { Ok(_) => Some(AmbientHint::Prose(crate::t!("hint.ambient_complete"))), Err(ParseError::Empty) => None, Err(ParseError::Invalid { @@ -750,6 +773,54 @@ mod tests { assert!(ambient_hint(" ", 3, None, &empty_cache()).is_none()); } + #[test] + fn advanced_mode_ambient_offers_sql_from_slot_candidate() { + // ADR-0022 Amendment 1: advanced-mode ambient assistance + // surfaces SQL completion candidates (here the FROM-slot + // table) instead of the simple-mode "this is SQL" gate. + let cache = + schema_with_columns("Customers", &[("id", crate::dsl::types::Type::Int)]); + let input = "select * from "; + match ambient_hint_in_mode( + input, + input.len(), + None, + &cache, + crate::mode::Mode::Advanced, + ) { + Some(AmbientHint::Candidates { items, .. }) => assert!( + items.iter().any(|c| c.text == "Customers"), + "FROM slot should offer table `Customers`; got {items:?}", + ), + other => panic!("expected candidates in advanced mode, got {other:?}"), + } + } + + #[test] + fn simple_mode_ambient_does_not_surface_sql_candidates() { + // The simple-mode entry point keeps gating SQL — advanced + // assistance is opt-in via mode, never leaked into simple. + let cache = + schema_with_columns("Customers", &[("id", crate::dsl::types::Type::Int)]); + let input = "select * from "; + let hint = ambient_hint_in_mode( + input, + input.len(), + None, + &cache, + crate::mode::Mode::Simple, + ); + let offers_table = matches!( + &hint, + Some(AmbientHint::Candidates { items, .. }) + if items.iter().any(|c| c.text == "Customers"), + ); + assert!( + !offers_table, + "simple mode must not surface SQL FROM candidates: {hint:?}", + ); + } + // ---- Phase D typed-slot hints (end-to-end) ------- fn schema_with_columns( diff --git a/src/snapshots/rdbms_playground__ui__tests__one_shot_advanced_dark.snap b/src/snapshots/rdbms_playground__ui__tests__one_shot_advanced_dark.snap index 0f94f63..cb62313 100644 --- a/src/snapshots/rdbms_playground__ui__tests__one_shot_advanced_dark.snap +++ b/src/snapshots/rdbms_playground__ui__tests__one_shot_advanced_dark.snap @@ -22,7 +22,7 @@ expression: snapshot │ ││: sel │ │ │╰──────────────────────────────────────────────────╯ │ │╭ Hint ────────────────────────────────────────────╮ -│ ││Type a command — press Tab for options, `help` for│ +│ ││select │ ╰──────────────────────────╯╰──────────────────────────────────────────────────╯ Project: Term Planner Enter submit · Backspace cancel one-shot · Ctrl-C quit diff --git a/src/ui.rs b/src/ui.rs index ee377c6..8bd85c1 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -725,6 +725,23 @@ fn runs_to_spans<'a>( .collect() } +/// Strip a leading one-shot `:` sigil (and the whitespace after +/// it) from `input`, returning the advanced command slice and the +/// cursor remapped into it. Mirrors `App::submit`'s `:` handling +/// so the hint panel hints at the command, not the sigil +/// (ADR-0022 Amendment 1). Used only when the effective mode is +/// `AdvancedOneShot`, where `input` is guaranteed to start (after +/// any leading whitespace) with `:`. +fn strip_one_shot_prefix(input: &str, cursor: usize) -> (&str, usize) { + let lead_ws = input.len() - input.trim_start().len(); + let after_colon = lead_ws + 1; // skip the `:` + let ws_after = input[after_colon..].len() - input[after_colon..].trim_start().len(); + let prefix_len = (after_colon + ws_after).min(input.len()); + let effective = &input[prefix_len..]; + let effective_cursor = cursor.saturating_sub(prefix_len).min(effective.len()); + (effective, effective_cursor) +} + fn render_hint_panel(app: &App, theme: &Theme, frame: &mut Frame<'_>, area: Rect) { let block = Block::default() .borders(Borders::ALL) @@ -740,21 +757,33 @@ fn render_hint_panel(app: &App, theme: &Theme, frame: &mut Frame<'_>, area: Rect // Resolution order for the hint panel body: // 1. An explicit app-set hint (e.g. modal contexts) wins. - // 2. Otherwise, in simple mode with non-empty input, - // the ambient typing-assistance hint (ADR-0022 §6). + // 2. Otherwise, with non-empty input, the ambient + // typing-assistance hint (ADR-0022 §6) computed in the + // effective mode. // 3. Otherwise, the existing empty-state placeholder. - // Advanced mode skips ambient hinting (ADR-0022 §12) — - // the DSL lexer/parser don't speak SQL. + // ADR-0022 Amendment 1: advanced mode no longer skips ambient + // hinting. The original §12 carve-out predated the unified + // mode-aware walker (ADR-0030/0031/0032); the walker now + // speaks SQL, so `ambient_hint_in_mode` surfaces SQL slot + // hints + completion candidates in advanced mode too. let empty_hint = crate::t!("panel.hint_empty"); - let ambient = match app.effective_mode() { - EffectiveMode::Simple => crate::input_render::ambient_hint( - &app.input, - app.input_cursor, - app.last_completion.as_ref(), - &app.schema_cache, - ), - EffectiveMode::AdvancedPersistent | EffectiveMode::AdvancedOneShot => None, + // In one-shot advanced mode (`:` prefix in simple mode) the + // raw input carries the `:` sigil, which is not part of the + // grammar. Strip it for the ambient computation so the hint + // reflects the advanced command — mirroring `App::submit`. + let (hint_input, hint_cursor) = match app.effective_mode() { + EffectiveMode::AdvancedOneShot => { + strip_one_shot_prefix(&app.input, app.input_cursor) + } + _ => (app.input.as_str(), app.input_cursor), }; + let ambient = crate::input_render::ambient_hint_in_mode( + hint_input, + hint_cursor, + app.last_completion.as_ref(), + &app.schema_cache, + app.effective_mode().as_mode(), + ); let muted = Style::default().fg(theme.muted); let line = match (app.hint.as_deref(), ambient) { (Some(set), _) => Line::from(Span::styled(set.to_string(), muted)), @@ -1025,6 +1054,29 @@ mod tests { insta::assert_snapshot!("default_advanced_dark", snapshot); } + #[test] + fn advanced_mode_hint_panel_surfaces_sql_candidates() { + // Regression reproduction (ADR-0022 Amendment 1): in + // advanced mode the hint panel must surface ambient + // assistance for SQL — here the FROM-slot table candidate + // `Customers` — not the empty placeholder. Before the fix + // `render_hint_panel` returned `None` for advanced mode and + // the hint resolver/completion ran in simple mode, so a SQL + // statement got the "this is SQL" gate and no candidates. + let mut app = App::new(); + app.mode = Mode::Advanced; + app.schema_cache.tables.push("Customers".to_string()); + app.input.push_str("select * from "); + app.input_cursor = app.input.len(); + let theme = Theme::dark(); + let rendered = render_to_string(&mut app, &theme, 80, 24); + assert!( + rendered.contains("Customers"), + "advanced-mode hint panel should surface the FROM-slot \ + candidate `Customers`; got:\n{rendered}", + ); + } + #[test] fn highlighted_input_all_token_classes_snapshot() { // ADR-0022 stage 2: representative input that exercises @@ -1050,8 +1102,12 @@ mod tests { // Typing `:sel` in simple mode should flip the input panel // label to `Advanced:` while the persistent mode stays simple. // The visible input includes the auto-inserted space after `:`. + // With the cursor after `sel` (ADR-0022 Amendment 1), the hint + // panel now offers the advanced `select` completion — the `:` + // sigil is stripped before the ambient walk. let mut app = App::new(); app.input.push_str(": sel"); + app.input_cursor = app.input.len(); let theme = Theme::dark(); let snapshot = render_to_string(&mut app, &theme, 80, 24); insta::assert_snapshot!("one_shot_advanced_dark", snapshot);