ui: re-enable advanced-mode ambient assistance (ADR-0022 Amendment 1)

Advanced-mode hinting + completion-preview were dead: render_hint_panel returned None for advanced mode (stale ADR-0022 §12 gate, predating the SQL grammar) and the hint resolver/ambient_hint never threaded Mode, so a SQL statement was gated as "this is SQL". The unified walker (ADR-0030/ 0031/0032) speaks SQL, so this lifts the gate. - ambient_hint_in_mode + hint_resolution_at_input_in_mode + expected_for_hint_snapshot(mode); candidate/diagnostic/parse sub-calls run in the active mode. - render_hint_panel calls ambient for all modes; one-shot `:` sigil stripped (strip_one_shot_prefix) so `: sel` hints `select`. - ADR-0022 Amendment 1 + README index. Found by manual advanced-mode testing; Phase 2 marked SQL hint/completion green at the engine layer but never exercised the UI. App-level render test (advanced_mode_hint_panel_surfaces_sql_candidates) + ambient-layer regression locks. 1466 baseline green.
2026-05-21 19:18:27 +00:00
parent c87363168f
commit ed40445828
6 changed files with 253 additions and 28 deletions
@@ -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
@@ -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`)
@@ -97,10 +97,25 @@ pub struct HintResolution {
 pub fn hint_resolution_at_input(
    source: &str,
    schema: Option<&crate::completion::SchemaCache>,
+) -> Option<HintResolution> {
+    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<HintResolution> {
    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();
@@ -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<AmbientHint> {
+    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<AmbientHint> {
    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(
@@ -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
@@ -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,
+    // 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,
-        ),
-        EffectiveMode::AdvancedPersistent | EffectiveMode::AdvancedOneShot => None,
-    };
+        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);