fix: H1a G3 advanced usage shows all valid forms; complete near-miss matrix (ADR-0042)
The /runda DA pass found G3 over-corrected: advanced-mode `create`/`drop` showed SQL forms only, hiding the DSL fallback forms that are valid input in advanced mode (verified: `create table Foo with pk`, `drop column …` parse and dispatch). Per the user decision, the advanced usage block now shows every form valid in the mode, SQL-primary first, then the DSL fallback forms — a usage hint must never hide working input. Simple mode unchanged (DSL forms only). Matrix completion (closing the residual coverage tail): - arg-less app commands (help/rebuild/new/load/undo/redo/export/import) audited + locked — all reject trailing junk with "expected end of input" + usage. - committed multi-forms (add index/constraint/1:n relationship, drop index/constraint/relationship, show table, change column, create index, alter table add/drop) audited + locked in near_miss_matrix_committed_multiforms — each renders its own form-specific missing-keyword message + usage. Also from the DA pass: - G2 distinct+all detector empirically verified unique to projection start (no misfire at count( / union / union all / select distinct). - stale `chumsky` comment removed (app.rs import handler). - ADR-0042 Implementation-outcome section records G1–G4, the user-confirmed G3 decision, and the now-complete matrix coverage. Full suite green (lib 1578 / it 387 / typing_surface_matrix 192); clippy clean.
This commit is contained in:
@@ -223,6 +223,55 @@ on any §2 wording change (expected; reviewed via `cargo insta`),
|
|||||||
but the two substring suites above must stay green without edits to
|
but the two substring suites above must stay green without edits to
|
||||||
their assertions.
|
their assertions.
|
||||||
|
|
||||||
|
## Implementation outcome (2026-06-05)
|
||||||
|
|
||||||
|
The baseline capture (§Implementation notes step 1) triaged four
|
||||||
|
gaps; all four are fixed test-first, locked by the near-miss matrix
|
||||||
|
in `tests/it/parse_error_pedagogy.rs`:
|
||||||
|
|
||||||
|
- **G1** — the bare `1` cardinality literal opening `add 1:n
|
||||||
|
relationship …` rendered cryptically. Render it as
|
||||||
|
`` `1:n relationship` `` in `format_expectation` (error wording
|
||||||
|
only; completion still offers the literal `1`).
|
||||||
|
- **G2** — bare `select` dumped the 14-item expression first-set.
|
||||||
|
Collapse it to "a projection: `*`, a column, or an expression"
|
||||||
|
in `format_walker_error`, detected by the `distinct`+`all`
|
||||||
|
quantifier pair being *jointly* expectable — a signature unique
|
||||||
|
to a projection start (empirically verified not to misfire at
|
||||||
|
`count(`, `union`, `union all`, `select distinct`, or mid-list).
|
||||||
|
Render-only; the completion/hint layer still expands the full
|
||||||
|
set.
|
||||||
|
- **G3** — the usage block was mode-blind (`render_usage_block`
|
||||||
|
resolved shared entry words to the first-registered Simple node).
|
||||||
|
`usage_key(s)_for_input` gain mode-aware `_in_mode` variants.
|
||||||
|
|
||||||
|
**Decision (user-confirmed, after the DA pass).** In advanced
|
||||||
|
mode the DSL forms remain *valid input* via fallback — verified:
|
||||||
|
`create table Foo with pk`, `drop column from table T: c`,
|
||||||
|
`drop relationship r`, `add column …` all parse and dispatch in
|
||||||
|
advanced mode. So the advanced usage block shows **every form
|
||||||
|
valid in the mode, mode-primary (SQL) first, then the DSL
|
||||||
|
fallback forms** — a usage hint must never hide input that works.
|
||||||
|
(An initial implementation that showed SQL-only was flagged by
|
||||||
|
the DA pass as hiding `create table … with pk` / `drop column …`
|
||||||
|
and corrected.) Simple mode shows DSL forms only — the SQL-only
|
||||||
|
forms hit the "this is SQL" rail and are unreachable.
|
||||||
|
|
||||||
|
- **G4** — `with` borrowed `select`'s usage; it gains its own
|
||||||
|
`parse.usage.with` CTE template.
|
||||||
|
|
||||||
|
Coverage: the matrix covers, in both modes, every entry word's bare
|
||||||
|
/ missing-clause / wrong-token near-misses, the app-lifecycle
|
||||||
|
trailing-junk cases, **and** the committed *multi-form* variants
|
||||||
|
(`add index` / `add constraint` / `add 1:n relationship`, `drop
|
||||||
|
index` / `drop constraint` / `drop relationship`, `show table`,
|
||||||
|
`change column …`, `create index`, `alter table … add` / `… drop`).
|
||||||
|
The committed forms were audited 2026-06-05 and each renders its own
|
||||||
|
form-specific missing-keyword message + usage (e.g. `add index` →
|
||||||
|
"expected `on` or `as`"; `drop constraint` → "expected `not`,
|
||||||
|
`unique`, `default`, or `check`"), regression-locked in
|
||||||
|
`near_miss_matrix_committed_multiforms`.
|
||||||
|
|
||||||
## Out of scope
|
## Out of scope
|
||||||
|
|
||||||
1. **Advanced-SQL engine-error sanitisation** — ADR-0019 §OOS-2.
|
1. **Advanced-SQL engine-error sanitisation** — ADR-0019 §OOS-2.
|
||||||
|
|||||||
+5
-5
@@ -1338,11 +1338,11 @@ impl App {
|
|||||||
},
|
},
|
||||||
),
|
),
|
||||||
AppCommand::Import { path, target } => {
|
AppCommand::Import { path, target } => {
|
||||||
// The path-bearing import goes through the
|
// A path-bearing import carries a non-empty path
|
||||||
// pre-chumsky source-slice (parser.rs), which
|
// from the walker. Bare `import` parses with an
|
||||||
// already validated non-empty path. Bare
|
// empty path string — surface the usage hint here
|
||||||
// `import` returns from chumsky with an empty
|
// at dispatch (not a parse error; ADR-0024 replaced
|
||||||
// path string — surface the usage error.
|
// the old chumsky source-slice path).
|
||||||
if path.is_empty() {
|
if path.is_empty() {
|
||||||
self.note_error(crate::t!("project.import_usage"));
|
self.note_error(crate::t!("project.import_usage"));
|
||||||
return Vec::new();
|
return Vec::new();
|
||||||
|
|||||||
+35
-19
@@ -566,11 +566,6 @@ pub fn usage_keys_for_input_in_mode(
|
|||||||
if candidates.is_empty() {
|
if candidates.is_empty() {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
let want = if mode == crate::mode::Mode::Advanced {
|
|
||||||
CommandCategory::Advanced
|
|
||||||
} else {
|
|
||||||
CommandCategory::Simple
|
|
||||||
};
|
|
||||||
let union = |nodes: &[(usize, &'static CommandNode, CommandCategory)]| -> Vec<&'static str> {
|
let union = |nodes: &[(usize, &'static CommandNode, CommandCategory)]| -> Vec<&'static str> {
|
||||||
let mut keys: Vec<&'static str> = Vec::new();
|
let mut keys: Vec<&'static str> = Vec::new();
|
||||||
for (_, node, _) in nodes {
|
for (_, node, _) in nodes {
|
||||||
@@ -582,23 +577,44 @@ pub fn usage_keys_for_input_in_mode(
|
|||||||
}
|
}
|
||||||
keys
|
keys
|
||||||
};
|
};
|
||||||
let matched: Vec<(usize, &'static CommandNode, CommandCategory)> =
|
// Advanced mode: every candidate form is reachable — the SQL
|
||||||
candidates.iter().copied().filter(|(_, _, cat)| *cat == want).collect();
|
// nodes are primary, and the DSL nodes remain valid via fallback
|
||||||
// Prefer the mode-matching nodes' usage. But a shared SQL node
|
// (verified: `create table … with pk` and `drop column …` both
|
||||||
// (`SQL_INSERT` / `SQL_UPDATE` / `SQL_DELETE`) declares no
|
// run in advanced mode). Show them all, mode-primary (Advanced)
|
||||||
// `usage_ids` of its own — it reuses the DSL template. When the
|
// first, so the usage hint never hides input that works. Simple
|
||||||
// mode-preferred set yields no usage keys, fall back to every
|
// mode: only the DSL forms — the SQL-only forms hit the "this is
|
||||||
// candidate so the entry word still shows a usage block rather
|
// SQL" rail and are not reachable. (ADR-0042 G3.)
|
||||||
// than the available-commands fallback (regression-locked by
|
let selected: Vec<(usize, &'static CommandNode, CommandCategory)> =
|
||||||
// the advanced near-miss matrix).
|
if mode == crate::mode::Mode::Advanced {
|
||||||
let mut keys = union(&matched);
|
let mut v: Vec<_> = candidates
|
||||||
if keys.is_empty() {
|
.iter()
|
||||||
keys = union(&candidates);
|
.copied()
|
||||||
}
|
.filter(|(_, _, c)| *c == CommandCategory::Advanced)
|
||||||
|
.collect();
|
||||||
|
v.extend(
|
||||||
|
candidates
|
||||||
|
.iter()
|
||||||
|
.copied()
|
||||||
|
.filter(|(_, _, c)| *c != CommandCategory::Advanced),
|
||||||
|
);
|
||||||
|
v
|
||||||
|
} else {
|
||||||
|
candidates
|
||||||
|
.iter()
|
||||||
|
.copied()
|
||||||
|
.filter(|(_, _, c)| *c == CommandCategory::Simple)
|
||||||
|
.collect()
|
||||||
|
};
|
||||||
|
// Degenerate guard: an advanced-only word in simple mode (not
|
||||||
|
// normally reachable — it hits the SQL rail first) leaves
|
||||||
|
// `selected` empty; fall back to all candidates so a usage block
|
||||||
|
// still renders rather than the available-commands fallback.
|
||||||
|
let pick = if selected.is_empty() { candidates } else { selected };
|
||||||
|
let keys = union(&pick);
|
||||||
if keys.is_empty() {
|
if keys.is_empty() {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
let entry = candidates[0].1.entry.primary;
|
let entry = pick[0].1.entry.primary;
|
||||||
Some((entry, keys))
|
Some((entry, keys))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -64,17 +64,22 @@ fn dump(input: &str, lines: &[String]) -> String {
|
|||||||
/// near-correct input plus substrings that MUST appear across its
|
/// near-correct input plus substrings that MUST appear across its
|
||||||
/// rendered error lines — the structural "name the missing
|
/// rendered error lines — the structural "name the missing
|
||||||
/// keyword/clause" message and the per-command usage template.
|
/// keyword/clause" message and the per-command usage template.
|
||||||
/// These are the cases triaged as already-good; the baseline dump
|
|
||||||
/// (`baseline_dump`, #[ignore]) shows the full rendering. Cases
|
|
||||||
/// flagged for wording fixes (bare `add` `1`, bare `select`
|
|
||||||
/// first-set, mode-blind usage) are deliberately NOT locked here
|
|
||||||
/// until their fixes land.
|
|
||||||
#[test]
|
#[test]
|
||||||
fn near_miss_matrix_simple_mode() {
|
fn near_miss_matrix_simple_mode() {
|
||||||
// (input, required-substrings-across-error-lines)
|
// (input, required-substrings-across-error-lines)
|
||||||
let matrix: &[(&str, &[&str])] = &[
|
let matrix: &[(&str, &[&str])] = &[
|
||||||
// app-lifecycle arg errors
|
// app-lifecycle arg errors. The arg-less commands all reject
|
||||||
|
// trailing junk with "expected end of input" + their usage
|
||||||
|
// (audited 2026-06-05); locked here as regression insurance.
|
||||||
("quit now", &["after `quit`, expected end of input", " quit"]),
|
("quit now", &["after `quit`, expected end of input", " quit"]),
|
||||||
|
("help foo", &["after `help`, expected end of input", " help"]),
|
||||||
|
("rebuild now", &["after `rebuild`, expected end of input", " rebuild"]),
|
||||||
|
("new foo", &["after `new`, expected end of input", " new"]),
|
||||||
|
("load foo", &["after `load`, expected end of input", " load"]),
|
||||||
|
("undo foo", &["after `undo`, expected end of input", " undo"]),
|
||||||
|
("redo foo", &["after `redo`, expected end of input", " redo"]),
|
||||||
|
("export foo bar", &["after `export foo`, expected end of input", "export [<path>]"]),
|
||||||
|
("import a b c", &["after `import a`, expected end of input", "import <zip-path>"]),
|
||||||
("save sideways", &["after `save`, expected end of input", "save | save as"]),
|
("save sideways", &["after `save`, expected end of input", "save | save as"]),
|
||||||
("mode sideways", &["unknown mode 'sideways'", "mode simple | mode advanced"]),
|
("mode sideways", &["unknown mode 'sideways'", "mode simple | mode advanced"]),
|
||||||
("messages louder", &["unknown messages mode 'louder'", "messages short"]),
|
("messages louder", &["unknown messages mode 'louder'", "messages short"]),
|
||||||
@@ -137,6 +142,61 @@ fn advanced_error_lines_for(input: &str) -> Vec<String> {
|
|||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Committed multi-form near-misses (ADR-0042 §1). A user who has
|
||||||
|
/// already chosen a form (`add index …`, `drop constraint …`,
|
||||||
|
/// `alter table T add …`) must get that form's specific
|
||||||
|
/// missing-keyword/clause message and usage — not the whole family
|
||||||
|
/// generically. Audited 2026-06-05; these render well today and are
|
||||||
|
/// locked here as the residual systematic-pass tail.
|
||||||
|
#[test]
|
||||||
|
fn near_miss_matrix_committed_multiforms() {
|
||||||
|
// (input, advanced?, required-substrings)
|
||||||
|
let matrix: &[(&str, bool, &[&str])] = &[
|
||||||
|
// add / drop multi-forms (simple)
|
||||||
|
("add index", false, &["after `add index`, expected `on` or `as`", "add index [as <Name>] on"]),
|
||||||
|
("add index on T", false, &["after `add index on T`, expected `(`", "add index [as <Name>] on"]),
|
||||||
|
("add constraint", false, &["after `add constraint`, expected `not`, `unique`, `default`, or `check`", "add constraint not null to"]),
|
||||||
|
("add constraint not null", false, &["after `add constraint not null`, expected `to`", "add constraint not null to"]),
|
||||||
|
("add 1:n relationship", false, &["after `add 1:n relationship`, expected `from` or `as`", "add 1:n relationship"]),
|
||||||
|
("add 1:n relationship from", false, &["after `add 1:n relationship from`, expected table name", "from <Parent>.<col>"]),
|
||||||
|
("drop constraint", false, &["after `drop constraint`, expected `not`, `unique`, `default`, or `check`", "drop constraint (not null"]),
|
||||||
|
("drop constraint not null", false, &["after `drop constraint not null`, expected `from`", "drop constraint (not null"]),
|
||||||
|
("drop index", false, &["after `drop index`, expected `on` or index name", "drop index <Name>", "drop index on <Table>"]),
|
||||||
|
("drop index on T", false, &["after `drop index on T`, expected `(`", "drop index on <Table>"]),
|
||||||
|
("drop relationship", false, &["after `drop relationship`, expected `from` or relationship name", "drop relationship <Name>"]),
|
||||||
|
("show table", false, &["after `show table`, expected table name", "show table <Table>"]),
|
||||||
|
("change column in table T: c", false, &["after `change column in table T: c`, expected `(`", "change column [in] [table]"]),
|
||||||
|
// advanced committed multi-forms
|
||||||
|
("create index on", true, &["after `create index on`, expected table name", "create [unique] index"]),
|
||||||
|
("create unique index", true, &["after `create unique index`, expected `on`, identifier, or `if`", "create [unique] index"]),
|
||||||
|
("alter table T add", true, &["after `alter table T add`, expected `column`, `constraint`, `check`, `unique`, `foreign`, or `primary`", "alter table <Table> add column"]),
|
||||||
|
("alter table T drop", true, &["after `alter table T drop`, expected `column` or `constraint`", "alter table <Table> drop column"]),
|
||||||
|
];
|
||||||
|
for (input, advanced, needles) in matrix {
|
||||||
|
let lines = if *advanced {
|
||||||
|
advanced_error_lines_for(input)
|
||||||
|
} else {
|
||||||
|
error_lines_for(input)
|
||||||
|
};
|
||||||
|
let dump_msg = dump(input, &lines);
|
||||||
|
assert!(
|
||||||
|
lines.iter().any(|l| l.starts_with("parse error")),
|
||||||
|
"missing `parse error` line for {input:?}\n{dump_msg}",
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
!lines.iter().any(|l| l.starts_with("available commands:")),
|
||||||
|
"committed form {input:?} fell back to available-commands\n{dump_msg}",
|
||||||
|
);
|
||||||
|
let joined = lines.join("\n");
|
||||||
|
for needle in *needles {
|
||||||
|
assert!(
|
||||||
|
joined.contains(needle),
|
||||||
|
"committed form {input:?} missing expected substring {needle:?}\n{dump_msg}",
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn advanced_bare_select_collapses_projection_first_set() {
|
fn advanced_bare_select_collapses_projection_first_set() {
|
||||||
// ADR-0042 G2: bare `select` dumped the full 14-item
|
// ADR-0042 G2: bare `select` dumped the full 14-item
|
||||||
@@ -163,12 +223,16 @@ fn advanced_bare_select_collapses_projection_first_set() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn advanced_mode_usage_block_shows_sql_templates_not_dsl() {
|
fn advanced_mode_usage_block_shows_sql_and_dsl_forms() {
|
||||||
// ADR-0042 G3: `render_usage_block` was mode-blind — it
|
// ADR-0042 G3: `render_usage_block` was mode-blind — it
|
||||||
// resolved shared entry words to the first-registered (Simple)
|
// resolved shared entry words to the first-registered (Simple)
|
||||||
// node, so advanced-mode `create` showed the DSL `create table
|
// node, so advanced-mode `create` showed ONLY the DSL `create
|
||||||
// … with pk …` template, which is not valid SQL. Mode-aware
|
// table … with pk …` template and none of the SQL forms.
|
||||||
// selection now shows the SQL forms.
|
// Mode-aware selection shows every form valid in the mode,
|
||||||
|
// SQL-primary first. In advanced mode the DSL forms remain
|
||||||
|
// valid input (verified: `create table Foo with pk` parses and
|
||||||
|
// runs in advanced mode), so they MUST still appear — a usage
|
||||||
|
// hint never hides input that works.
|
||||||
let lines = advanced_error_lines_for("create");
|
let lines = advanced_error_lines_for("create");
|
||||||
let joined = lines.join("\n");
|
let joined = lines.join("\n");
|
||||||
let dump_msg = dump("create", &lines);
|
let dump_msg = dump("create", &lines);
|
||||||
@@ -181,9 +245,14 @@ fn advanced_mode_usage_block_shows_sql_templates_not_dsl() {
|
|||||||
"advanced `create` should show the SQL create-index usage\n{dump_msg}",
|
"advanced `create` should show the SQL create-index usage\n{dump_msg}",
|
||||||
);
|
);
|
||||||
assert!(
|
assert!(
|
||||||
!joined.contains("with pk"),
|
joined.contains("create table <Name> with pk"),
|
||||||
"advanced `create` must NOT show the DSL `with pk` template\n{dump_msg}",
|
"advanced `create` should ALSO show the DSL `with pk` form (valid in advanced mode)\n{dump_msg}",
|
||||||
);
|
);
|
||||||
|
// Ordering: the SQL form is listed before the DSL form
|
||||||
|
// (mode-primary first).
|
||||||
|
let sql_at = joined.find("create table [if not exists]").unwrap();
|
||||||
|
let dsl_at = joined.find("create table <Name> with pk").unwrap();
|
||||||
|
assert!(sql_at < dsl_at, "SQL form should precede the DSL form\n{dump_msg}");
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The advanced-mode near-miss matrix (ADR-0042 §1/§3). Mirrors
|
/// The advanced-mode near-miss matrix (ADR-0042 §1/§3). Mirrors
|
||||||
@@ -197,11 +266,12 @@ fn near_miss_matrix_advanced_mode() {
|
|||||||
("select", &["expected a projection: `*`, a column, or an expression", "select (* |"]),
|
("select", &["expected a projection: `*`, a column, or an expression", "select (* |"]),
|
||||||
("select * from", &["after `select * from`, expected table name", "select (* |"]),
|
("select * from", &["after `select * from`, expected table name", "select (* |"]),
|
||||||
("with", &["after `with`, expected identifier or `recursive`", "with [recursive]", "as ("]),
|
("with", &["after `with`, expected identifier or `recursive`", "with [recursive]", "as ("]),
|
||||||
// create / drop / alter — SQL templates (G3)
|
// create / drop / alter — SQL forms AND the still-valid DSL
|
||||||
("create", &["after `create`, expected `table`", "create table [if not exists]", "create [unique] index"]),
|
// fallback forms, SQL-primary first (G3).
|
||||||
|
("create", &["after `create`, expected `table`", "create table [if not exists]", "create [unique] index", "create table <Name> with pk"]),
|
||||||
("create table", &["after `create table`, expected identifier or `if`", "create table [if not exists]"]),
|
("create table", &["after `create table`, expected identifier or `if`", "create table [if not exists]"]),
|
||||||
("create index", &["after `create index`, expected `on`", "create [unique] index"]),
|
("create index", &["after `create index`, expected `on`", "create [unique] index"]),
|
||||||
("drop", &["after `drop`, expected `table`", "drop table [if exists]"]),
|
("drop", &["after `drop`, expected `table`", "drop table [if exists]", "drop column [from]", "drop relationship"]),
|
||||||
("alter", &["after `alter`, expected `table`", "alter table <Table> add column"]),
|
("alter", &["after `alter`, expected `table`", "alter table <Table> add column"]),
|
||||||
("alter table T", &["expected `add`, `drop`, `rename`, or `alter`", "alter table <Table>"]),
|
("alter table T", &["expected `add`, `drop`, `rename`, or `alter`", "alter table <Table>"]),
|
||||||
// shared insert/update/delete — must show usage, not the
|
// shared insert/update/delete — must show usage, not the
|
||||||
@@ -429,3 +499,7 @@ fn caret_aligns_under_offending_token() {
|
|||||||
"caret should sit at column 9 (under `f` of `frobulate` after the `running: ` prefix); got {leading_spaces} spaces in {caret:?}",
|
"caret should sit at column 9 (under `f` of `frobulate` after the `running: ` prefix); got {leading_spaces} spaces in {caret:?}",
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user