From 4fa0aa06e9febed5a82a3eee209c12b3a4b3af69 Mon Sep 17 00:00:00 2001
From: "claude@clouddev1" <claude-clouddev1@oliversturm.ch>
Date: Fri, 22 May 2026 22:06:04 +0000
Subject: [PATCH] =?UTF-8?q?db+walker:=203i=20DA=20pass=20=E2=80=94=20not?=
 =?UTF-8?q?=5Fnull=20PK=20false-positive=20fix=20+=20arity=20hardening?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

DA pass on 3i. Fix: build_schema_cache set not_null = c.notnull ||
c.primary_key, which would false-flag an omitted `int` PK as a
not_null_missing WARNING — but an int PK is an INTEGER PRIMARY KEY
rowid alias that auto-fills (and SQLite's PK-NULL quirk means a PK
isn't implicitly NOT NULL anyway). Use c.notnull alone (ADR-0033
§8.3 "declared NOT NULL"): faithful and false-positive-free.

Arity-walk hardening (same class as the ON CONFLICT regression the
existing tests caught mid-3i): RETURNING after VALUES is a depth-0
keyword that ends the tuple list (only the real tuple is flagged),
and a comma nested in a function-call value (depth ≥ 2) does not
inflate the tuple's value count.

Tests (+2). 1598 pass / 0 fail / 1 ignored. Clippy clean.
---
 src/dsl/walker/mod.rs | 23 +++++++++++++++++++++++
 src/runtime.rs        | 15 +++++++++------
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/src/dsl/walker/mod.rs b/src/dsl/walker/mod.rs
index 5484a55..d09ab3f 100644
--- a/src/dsl/walker/mod.rs
+++ b/src/dsl/walker/mod.rs
@@ -4554,6 +4554,29 @@ mod tests {
         );
     }
 
+    #[test]
+    fn insert_arity_with_returning_flags_only_tuple() {
+        // RETURNING is a depth-0 keyword that ends the VALUES clause;
+        // the walk must stop there (same guard as ON CONFLICT) so the
+        // RETURNING projection isn't mis-counted as a value tuple.
+        let schema = schema_with("t", &[("a", Type::Int), ("b", Type::Int)]);
+        let diags = diag_keys("sqlinsert into t (a, b) values (1, 2, 3) returning *", &schema);
+        let n = diags.iter().filter(|d| d.contains("value(s) are given")).count();
+        assert_eq!(n, 1, "only the 3-value tuple is flagged; got {diags:?}");
+    }
+
+    #[test]
+    fn insert_arity_ignores_commas_nested_in_a_value() {
+        // A comma inside a function call (depth ≥ 2) is not a tuple
+        // separator and must not inflate the value count.
+        let schema = schema_with("t", &[("a", Type::Int), ("b", Type::Int)]);
+        let diags = diag_keys("sqlinsert into t (a, b) values (1, coalesce(1, 2))", &schema);
+        assert!(
+            !diags.iter().any(|d| d.contains("value(s) are given")),
+            "two values (one a 2-arg call) match the 2-col list; got {diags:?}",
+        );
+    }
+
     #[test]
     fn insert_select_arity_mismatch_fires() {
         // INSERT … SELECT: 1 listed column vs a 2-item projection.
diff --git a/src/runtime.rs b/src/runtime.rs
index 4669992..b2246b1 100644
--- a/src/runtime.rs
+++ b/src/runtime.rs
@@ -1002,12 +1002,15 @@ async fn build_schema_cache(database: &Database) -> crate::completion::SchemaCac
                     c.user_type.map(|ty| TableColumn {
                         name: c.name,
                         user_type: ty,
-                        // A PK column is effectively NOT NULL even when
-                        // PRAGMA's notnull flag isn't set on it
-                        // (ADR-0033 §8.3); the not_null_missing pass
-                        // excludes auto-gen types (serial/shortid)
-                        // separately.
-                        not_null: c.notnull || c.primary_key,
+                        // Only a column the engine reports as NOT NULL
+                        // (ADR-0033 §8.3 "declared NOT NULL"). We do
+                        // NOT treat a PK as implicitly not-null: an
+                        // `int` PK is an `INTEGER PRIMARY KEY` rowid
+                        // alias that auto-fills, so flagging it omitted
+                        // would be a false positive (SQLite reports
+                        // notnull=0 for it anyway). serial/shortid are
+                        // excluded by type in the not_null_missing pass.
+                        not_null: c.notnull,
                         has_default: c.default.is_some(),
                     })
                 })