fix: X4 — advanced-mode SQL INSERT auto-fills omitted non-PK serial (MAX+1)

A Form-A advanced-mode INSERT that omitted a non-PK serial column left it
silently NULL (the column is INTEGER UNIQUE, not NOT NULL, so SQLite
permits it), while simple-mode do_insert auto-fills it with MAX+1. That
violated ADR-0018 §1's "auto-generated on every path" contract and was the
unprincipled serial-vs-shortid asymmetry the ADR set out to remove
(advanced mode already auto-fills shortid).

Fix (decision: advanced mode matches simple mode): the advanced-mode
auto-fill reconstruction — renamed plan_shortid_autofill →
plan_autogen_autofill — now also fills an omitted non-PK serial with
MAX(col)+1 … MAX+n per row (single- and multi-row), reading MAX once under
the worker's single-writer serialisation. PK serial stays on the rowid
alias; Form B (no column list) still supplies every column. Honours
ADR-0018 §1/§5; no ADR amendment needed (the contract already said "every
path"). requirements.md X4 marked resolved.

Tests: 1949 passing (+1), 0 failed, 0 skipped, 1 ignored; clippy clean.

docs/requirements.md

@@ -576,19 +576,23 @@ handoff-14 cleanup; 449 after B2/C2.)
 - [~] **X3** Accessibility: TUI screen-reader support is
   best-effort and not a v1 commitment; revisit if user need
   emerges.
-- [~] **X4** Auto-fill semantics differ between simple and advanced
-  mode — **possible bug, to investigate** (raised 2026-05-26). The
-  simple-mode insert (`do_insert`, `db.rs`) auto-fills an omitted
-  **non-PK `serial`** column with `MAX(col)+1` *and* generates
-  `shortid`s; the advanced-mode SQL insert (`plan_shortid_autofill`)
-  auto-fills **only `shortid`** and does **not** do non-PK `serial`
-  MAX+1. The `shortid` distinction is intentional; the non-PK-`serial`
-  gap looks unintended (no obvious reason advanced mode should skip
-  filling a `serial`). Decide whether advanced mode should match
-  simple mode's serial auto-fill (likely yes) or whether the
-  difference is deliberate, and align ADR-0018 accordingly. Left
-  untouched by the ADR-0036 value-validation work (which is surgical
-  and does not change auto-fill).
+- [x] **X4** Auto-fill semantics differ between simple and advanced
+  mode — **resolved 2026-05-27** (raised 2026-05-26). Was: simple-mode
+  `do_insert` auto-fills an omitted **non-PK `serial`** column with
+  `MAX(col)+1`, but the advanced-mode SQL insert auto-filled **only
+  `shortid`**, leaving an omitted non-PK serial **silently NULL** —
+  violating ADR-0018 §1's "auto-generated on every path" contract (the
+  column is `INTEGER UNIQUE`, not `NOT NULL`, so SQLite permits the
+  NULL). Confirmed by characterization, escalated, and fixed (decision:
+  advanced mode matches simple mode): the advanced-mode auto-fill
+  reconstruction (`db.rs`, renamed `plan_shortid_autofill` →
+  **`plan_autogen_autofill`**) now also fills an omitted non-PK serial
+  with `MAX+1` per row (single- and multi-row), mirroring `do_insert`
+  and the existing shortid fill. PK serial is excluded (rowid alias);
+  Form B (no column list) still supplies every column. Covered by
+  `tests/sql_insert.rs::sql_insert_autofills_omitted_nonpk_serial`.
+  Honours ADR-0018 §1/§5; no ADR amendment needed (the contract already
+  said "every path"). ADR-0036 was correct that it did not touch this.
 - [~] **X5** Framework cohesion / restructuring — **strategic,
   revisit later** (raised 2026-05-26). The grammar/execution
   framework (lexer → walker `Node`s → unified grammar tree → typed

src/db.rs

@@ -8306,7 +8306,22 @@ fn existing_shortids(
 ///
 /// `serial` columns are not handled here — an omitted `serial`
 /// primary key is filled by the engine's rowid (ADR-0033 §6).
-fn plan_shortid_autofill(
+/// Auto-fill the omitted **auto-generated** columns of an advanced-mode
+/// Form-A `INSERT` (a `(col, …)` list is present) by reconstructing a
+/// parameterised statement, honouring ADR-0018 §1's "auto-generated on
+/// every path" contract — the same contract simple-mode `do_insert`
+/// honours:
+///   - **`shortid`** columns omitted from the list get a fresh distinct
+///     id per row (ADR-0018 §3).
+///   - **non-PK `serial`** columns omitted from the list get `MAX(col)+1`,
+///     `MAX+2`, … per row (ADR-0018 §5; the X4 fix — advanced mode used
+///     to leave them NULL). PK serial relies on the engine's rowid alias
+///     and is never in this set.
+///
+/// Returns the original `sql` + an empty param vec when there is nothing
+/// to fill (Form B / no omitted auto-gen column / zero rows), so the
+/// caller executes the verbatim statement unchanged.
+fn plan_autogen_autofill(
     conn: &Connection,
     target_table: &str,
     sql: &str,
@@ -8319,18 +8334,29 @@ fn plan_shortid_autofill(
     }
     let schema = read_schema(conn, target_table)?;
     // Identifiers are case-preserving but matched case-insensitively
-    // (ADR-0009): a shortid column counts as omitted unless the user
-    // listed a name equal to it ignoring ASCII case.
+    // (ADR-0009): a column counts as omitted unless the user listed a
+    // name equal to it ignoring ASCII case.
     let listed_ci: Vec<String> =
         listed_columns.iter().map(|c| c.to_ascii_lowercase()).collect();
+    let is_omitted = |c: &&ReadColumn| !listed_ci.contains(&c.name.to_ascii_lowercase());
     let omitted_shortids: Vec<String> = schema
         .columns
         .iter()
         .filter(|c| c.user_type == Some(Type::ShortId))
-        .filter(|c| !listed_ci.contains(&c.name.to_ascii_lowercase()))
+        .filter(is_omitted)
         .map(|c| c.name.clone())
         .collect();
-    if omitted_shortids.is_empty() {
+    // ADR-0018 §5 + X4: a non-PK serial omitted from the list is
+    // auto-filled MAX+1 per row (PK serial uses the rowid alias and is
+    // excluded). Mirrors simple-mode `do_insert`.
+    let omitted_serials: Vec<String> = schema
+        .columns
+        .iter()
+        .filter(|c| c.user_type == Some(Type::Serial) && !c.primary_key)
+        .filter(is_omitted)
+        .map(|c| c.name.clone())
+        .collect();
+    if omitted_shortids.is_empty() && omitted_serials.is_empty() {
         return Ok((sql.to_string(), Vec::new()));
     }
 
@@ -8377,10 +8403,39 @@ fn plan_shortid_autofill(
         id_batches.push(generate_shortid_batch(n, &existing)?);
     }
 
-    // Reconstruct: listed columns followed by the omitted shortid
-    // columns; one parameterised tuple per materialised row.
-    let all_cols: Vec<&String> =
-        listed_columns.iter().chain(omitted_shortids.iter()).collect();
+    // A serial sequence per omitted non-PK serial column: MAX(col)+1 …
+    // MAX(col)+n. MAX is read once (current table state); the worker-
+    // thread serialisation (ADR-0010) makes the read-then-insert safe.
+    // All values exceed the current MAX, so they collide with neither
+    // existing rows nor each other (the column's UNIQUE holds). MAX+1
+    // (gaps jumped, not back-filled) matches `do_insert` and the rowid
+    // PK case (ADR-0018 §5 / Resolution 2).
+    let mut serial_batches: Vec<Vec<rusqlite::types::Value>> =
+        Vec::with_capacity(omitted_serials.len());
+    for col in &omitted_serials {
+        let max: i64 = conn
+            .query_row(
+                &format!(
+                    "SELECT COALESCE(MAX({col}), 0) FROM {tbl};",
+                    col = quote_ident(col),
+                    tbl = quote_ident(target_table),
+                ),
+                [],
+                |row| row.get(0),
+            )
+            .map_err(DbError::from_rusqlite)?;
+        serial_batches
+            .push((1..=n as i64).map(|i| rusqlite::types::Value::Integer(max + i)).collect());
+    }
+
+    // Reconstruct: listed columns, then the omitted shortid columns,
+    // then the omitted non-PK serial columns; one parameterised tuple
+    // per materialised row. The param push order below matches this.
+    let all_cols: Vec<&String> = listed_columns
+        .iter()
+        .chain(omitted_shortids.iter())
+        .chain(omitted_serials.iter())
+        .collect();
     let cols_csv = all_cols
         .iter()
         .map(|c| quote_ident(c))
@@ -8397,6 +8452,9 @@ fn plan_shortid_autofill(
         for batch in &id_batches {
             params.push(batch[row_idx].clone());
         }
+        for batch in &serial_batches {
+            params.push(batch[row_idx].clone());
+        }
         let placeholders = (0..per_tuple)
             .map(|_| {
                 let s = format!("?{ph}");
@@ -8433,10 +8491,10 @@ fn plan_shortid_autofill(
 /// persistence failure rolls the insert back), then commit.
 ///
 /// Grammar-as-text (ADR-0030 §4): normally the values are literals
-/// in `sql` and no parameters are bound. The sub-phase 3d shortid
-/// auto-fill path is the exception — it reconstructs a
-/// parameterised `INSERT` (see `plan_shortid_autofill`); either
-/// way `history.log` records the original `source`, never the
+/// in `sql` and no parameters are bound. The auto-fill path (an
+/// omitted `shortid` or non-PK `serial` column) is the exception — it
+/// reconstructs a parameterised `INSERT` (see `plan_autogen_autofill`);
+/// either way `history.log` records the original `source`, never the
 /// rewritten statement (ADR-0030 §11). FK / UNIQUE / NOT NULL
 /// engine errors surface enriched via `execute_with_fk_enrichment`
 /// + the friendly-error layer.
@@ -8512,7 +8570,7 @@ fn do_sql_insert(
     // params vec with the original `sql` means "no auto-fill —
     // execute verbatim" (the 3b path).
     let (exec_sql, params) =
-        plan_shortid_autofill(conn, target_table, sql, listed_columns, row_source, &trailing_tail)?;
+        plan_autogen_autofill(conn, target_table, sql, listed_columns, row_source, &trailing_tail)?;
     let tx = conn
         .unchecked_transaction()
         .map_err(DbError::from_rusqlite)?;

src/dsl/grammar/sql_insert.rs

@@ -105,7 +105,7 @@ fn fallback_value_list() -> Node {
 ///     `VALUES` position, so it is correctly absent here.
 ///   - **Form B** (no column list): ALL columns in declaration order,
 ///     including `serial` / `shortid` — advanced-mode Form B auto-fills
-///     *nothing* (`plan_shortid_autofill` returns early on an empty
+///     *nothing* (`plan_autogen_autofill` returns early on an empty
 ///     column list), so the user supplies a value for every column.
 ///
 /// Empty when schemaless, the table is unknown, or a Form A list

tests/sql_insert.rs

@@ -707,6 +707,37 @@ fn autofill_does_not_mask_arity_mismatch() {
     assert!(rows.is_empty(), "no row should land on a rejected insert: {rows:?}");
 }
 
+#[test]
+fn sql_insert_autofills_omitted_nonpk_serial() {
+    // ADR-0018 §1/§5 + X4: advanced-mode SQL INSERT auto-fills an omitted
+    // non-PK `serial` column with MAX+1 (per row), exactly as simple-mode
+    // `do_insert` does — honouring the "auto-generated on every path"
+    // contract. (Was: silently inserting NULL.) Mirrors the existing
+    // shortid auto-fill, which already runs on this path.
+    let (project, db, _dir) = open_project_db();
+    let rt = rt();
+    create_cols(&db, &rt, "t", &[("id", Type::Int), ("seq", Type::Serial)], &["id"]);
+
+    // Single row, omitting the non-PK serial `seq`.
+    run_sqlinsert(&db, &rt, "insert into t (id) values (10)").expect("single-row insert runs");
+    // Multi-row, omitting `seq` — each row gets a distinct, increasing
+    // serial continuing from the current MAX.
+    run_sqlinsert(&db, &rt, "insert into t (id) values (20), (30)")
+        .expect("multi-row insert runs");
+
+    let rows = csv_rows(&project, "t");
+    // No NULL serials, and the sequence is 1, 2, 3 across the three rows.
+    assert_eq!(
+        rows,
+        vec![
+            vec!["10".to_string(), "1".to_string()],
+            vec!["20".to_string(), "2".to_string()],
+            vec!["30".to_string(), "3".to_string()],
+        ],
+        "omitted non-PK serial auto-filled MAX+1 per row (no NULLs): {rows:?}",
+    );
+}
+
 #[test]
 fn autofill_insert_select_wider_projection_is_rejected() {
     // SELECT projects more columns than the list: the guard defers