fix: insert-without-columns is wrong under a dialect parser (#68)

AGENTS.md

@@ -45,6 +45,8 @@ When changing the public API or Go version, update all nine `go.mod` files and `
 
 **The analyzer is parser-pluggable.** `analyzer.Analyzer` runs `Rule`s against a normalized, dialect-agnostic `Statement` produced by an `analyzer.Parser` (`analyzer/parser.go`, `statement.go`). The default `FallbackParser` (`fallback.go`) is zero-dependency, strips comments/string literals, and never errors. `analyzer.Analyze` degrades to the FallbackParser if a configured parser errors, so analysis never breaks the caller's query path. Real grammars are supplied via `middleware.WithParser(...)` / `analyzer.Default().WithParser(...)` using the `parsers/*` modules. Rules read the `Statement`, never raw SQL.
 
+**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how every instance in #68 arose (`INSERT ... DEFAULT VALUES` and `UPSERT`, plain and CTE-prefixed, under `pgparser`; `REPLACE` and the `INTO`-less `INSERT t VALUES (...)` under `mysqlparser`). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `detectKind` recognizes the insert-like keywords in two places, leading and after a `WITH` clause, and both require a table name after the keyword so a `REPLACE(str, from, to)` call is never read as the statement. `insertColumnsListed` anchors on the **statement head** whenever it starts the statement and falls back to the `INTO` clause only for the CTE-prefixed forms, where the keyword sits mid-statement and can also occur inside the CTE body. That order matters: searching for `INTO` first matches an identifier named `into` in the column list of a form that omits the keyword, reads the list as the target table, and reports a statement that does name its columns. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags.
+
 **Rules self-register; config is resolved once, never per query.** Built-in rules call `analyzer.Register(RuleSpec{...})` from `init()` in `analyzer/rules.go` — a stable name, default severity, and a settings-aware `Factory`. To add a rule, write it and add one `Register` call; **do not** hand-maintain a rule list in `Default()`. Being addressable by name is what makes enable/disable, severity overrides, per-rule `Settings`, and suppressions work uniformly. `analyzer.Profile` (disabled set, `only` whitelist, severity map, per-rule settings) is applied in `DefaultWithProfile` at construction; the per-query `Analyze` path does no config work (it runs on every query through the driver — keep it allocation-light). `analyzer` must stay free of `config`/YAML imports.
 
 **Not every registered rule is evaluated by the Analyzer.** A `RuleSpec` with a nil `Factory` registers a name without a rule body — `RuleSpec.Evaluated()` reports the difference, and `DefaultWithProfile` skips those when binding. Seven use this: `slow-query` and `n-plus-one`, which `middleware` derives from latency and fingerprint counts, and `seq-scan` / `high-cost` / `full-table-scan` / `no-index-used` / `filesort`, which `explain` derives from the database's own plan. None can be run against a parsed `Statement`, but all seven are documented as rules, so they register to be **addressable**: `disable`, `only`, `severity` and `settings` must mean the same thing on every surface. Their owners ask `Analyzer.RuleEnabled` / `RuleSeverity` / `RuleSettings` for the resolved decision. **`RuleEnabled` answers `disable:` and `severity: off` but deliberately ignores the `only:` whitelist**, because `only:` selects which rules are evaluated _against a statement_ and none of these seven are: a list written to focus `sqlguard scan` would otherwise switch off latency and N+1 reporting in a running application and blank out `sqlguard explain`, none of which it names, and none of which warns. Switching one off takes naming it, which is why `disable:` reaches every surface and `only:` reaches one (pinned by `TestRuleEnabledIgnoresOnly`, `TestRuleEnabledHonoursDisableInsideAnOnlyList` and `TestApplyProfile_IgnoresOnly`). `Guard` resolves its two once in `NewGuard` (the per-query path stays config-free), and `explain` filters centrally in `applyProfile` rather than at each of the five sites that build a finding, so a plan rule added later cannot forget the check. All seven are registered in `analyzer/rules.go`, **not** in the packages that own them: `config` validates names against `analyzer.RuleNames()` while importing only `analyzer`, so registering `seq-scan` from `explain`'s `init()` would make a config naming it fail for anyone who doesn't link that package. Per-rule tunables live in `rules.settings` for these too (`slow-query.threshold`, `n-plus-one.threshold`/`window`, the latter pair also being what turns N+1 on from a file); an explicit Go option (`WithSlowQueryThreshold`, `WithN1Detection`) outranks the file. `config.checkSettings` validates the whole settings block, because every read path — `Settings.Duration`, `Settings.Int` — falls back to the caller's default rather than failing, so anything it cannot use becomes a silently wrong threshold (or, for `n-plus-one`, detection that never switches on). It checks the key against `ruleSettings`, which is the complete tunable set by design: a key absent from a listed rule is a misspelling, and any key on an unlisted rule is a mistake, since that rule reads no settings. It also checks the value's type, rejects a non-positive `n-plus-one.threshold`, and reports half a paired block. `config.checkScanHasRules` covers the shapes that only became expressible once the seven were registered, and asks `Profile.Skip` — the same question `DefaultWithProfile` asks — rather than testing the list against `EvaluatedRuleNames()`, which was blind to a name `only:` selects and `disable:` or `severity: off` then takes away again. It is gated on `only:` being configured, because disabling every rule without one is a legitimate runtime-only setup. Validation reads values through `Settings.LookupDuration`/`LookupInt`, the accessors `Duration`/`Int` themselves delegate to, so a value the loader accepts can never be one the reader silently replaces with a default.

CHANGELOG.md

@@ -9,6 +9,60 @@ the same version in lockstep.
 
 ## [Unreleased]
 
+### Fixed
+
+- **`pgparser` no longer reports `insert-without-columns` on
+  `INSERT INTO t DEFAULT VALUES`** ([#68]). The grammar encodes that form as
+  an absent row source, and the parser refilled `InsertColumnsListed` from
+  `len(Columns)` alone after blanking it — so the AST path dropped the
+  fallback's explicit handling ("DEFAULT VALUES inserts no data") and then
+  marked the result `Exact`. Opting into the exact parser made this rule
+  strictly worse than the zero-dependency default, inverting the trade-off
+  documented in [SQL Parsers](https://kartikrocks.github.io/sqlguard/docs/parsers).
+- **`insert-without-columns` now covers every keyword that inserts rows
+  positionally**, not just `INSERT INTO`: MySQL/SQLite's `REPLACE`, the
+  `UPSERT` the CockroachDB-derived grammar behind `pgparser` accepts, and the
+  forms that omit MySQL's optional `INTO` (`INSERT t VALUES (…)`,
+  `REPLACE t VALUES (…)`), including a `LOW_PRIORITY`/`DELAYED`/`IGNORE`
+  modifier run. All bind by column order and carry the same schema-change
+  risk, and all are the same AST node to a real grammar — so the dialect
+  parsers already reported them while the fallback read them as an
+  unrecognized statement kind and said nothing. `REPLACE(str, from, to)` is
+  never mistaken for a statement, and a `REPLACE()` call inside a CTE does not
+  displace the real statement head. A CTE prefix puts the keyword
+  mid-statement, past the leading-keyword check, so
+  `WITH c AS (…) UPSERT INTO t SELECT …` is covered too.
+- **A column named `into` no longer defeats `insert-without-columns`.** The
+  target table was found by scanning for `INTO` anywhere in the statement, so
+  ``INSERT t (`into`) VALUES (1)`` — a form that omits the optional keyword —
+  had its column list read as the target table and was reported as having no
+  columns. The statement head is now the anchor whenever it starts the
+  statement; `INTO` remains the anchor for the CTE-prefixed forms, where the
+  keyword sits mid-statement.
+
+### Added
+
+- **Parser parity is pinned by a test.** `pgparser` and `mysqlparser` each run
+  a corpus through both the dialect grammar and the fallback and assert the
+  grammar never reports a rule the fallback does not
+  (`TestParser_NeverAddsFindingTheFallbackDoesNot`). Opting into a real parser
+  should only remove findings, which is the direction the docs promise. It is
+  an invariant over a corpus rather than a proof — all four bugs above are the
+  same shape, and only one of them had been noticed.
+
+### Changed
+
+- **`StmtInsert` now covers `REPLACE` and `UPSERT`**, which reaches
+  `sqlguard explain`: both were previously refused as unrecognized statements
+  and are now admitted under `--allow-dml`, planned and rolled back like any
+  other DML. On a server where the keyword is not valid, the server's syntax
+  error replaces sqlguard's refusal.
+- **`insert-without-columns` message reworded** from "INSERT without explicit
+  column list" to "Row-inserting statement without an explicit column list",
+  since it no longer fires only on `INSERT`.
+
+[#68]: https://github.com/KARTIKrocks/sqlguard/issues/68
+
 ## [0.4.0] - 2026-09-25
 
 ### Changed

analyzer/analyzer_test.go

@@ -119,6 +119,34 @@ func TestCheckInsertWithoutColumns(t *testing.T) {
 		{"mysql set form", "INSERT INTO users SET name = 'alice', email = 'a@test.com'", false},
 		{"default values", "INSERT INTO users DEFAULT VALUES", false},
 		{"cte insert no columns", "WITH s AS (SELECT 1) INSERT INTO users SELECT * FROM s", true},
+		// REPLACE is MySQL/SQLite's insert-or-overwrite; positionally it is an
+		// INSERT and carries the same column-order risk. A real MySQL grammar
+		// parses it into the same AST node, so reading it as StmtOther here
+		// made the rule fire only for callers who opted into mysqlparser.
+		{"replace no columns", "REPLACE INTO users VALUES ('alice')", true},
+		{"replace with columns", "REPLACE INTO users (name) VALUES ('alice')", false},
+		// INTO is optional in MySQL for both keywords, and a modifier run may
+		// sit between the keyword and the table.
+		{"replace without into", "REPLACE users VALUES ('alice')", true},
+		{"replace without into with columns", "REPLACE users (name) VALUES ('alice')", false},
+		{"insert without into", "INSERT users VALUES ('alice')", true},
+		{"insert without into with columns", "INSERT users (name) VALUES ('alice')", false},
+		{"insert with modifier", "INSERT LOW_PRIORITY INTO users VALUES ('alice')", true},
+		{"replace delayed without into", "REPLACE DELAYED users VALUES ('alice')", true},
+		// UPSERT reaches the INSERT node in the grammar behind pgparser.
+		{"upsert no columns", "UPSERT INTO users VALUES ('alice')", true},
+		{"upsert with columns", "UPSERT INTO users (name) VALUES ('alice')", false},
+		// REPLACE(str, from, to) is a string function, not a statement.
+		{"replace function is not a statement", "SELECT REPLACE(name, 'a', 'b') FROM users", false},
+		{"replace function inside an insert", "INSERT INTO users (name) VALUES (REPLACE(x, 'a', 'b'))", false},
+		{"cte containing a replace call", "WITH s AS (SELECT REPLACE(a, 'x', 'y') AS n FROM u) INSERT INTO users SELECT n FROM s", true},
+		{"cte upsert no columns", "WITH s AS (SELECT 1 AS n) UPSERT INTO users SELECT n FROM s", true},
+		{"cte upsert with columns", "WITH s AS (SELECT 1 AS n) UPSERT INTO users (name) SELECT n FROM s", false},
+		// A column named "into" must not be read as the INTO clause, which
+		// would make the column list look like the target table.
+		{"column named into without the keyword", "INSERT users (`into`) VALUES ('alice')", false},
+		{"replace with a column named into", "REPLACE users (`into`) VALUES ('alice')", false},
+		{"column named into with the keyword", "INSERT INTO users (`into`) VALUES ('alice')", false},
 	}
 
 	for _, tt := range tests {

analyzer/fallback.go

@@ -37,7 +37,30 @@ var (
 	// singular VALUE; SELECT/WITH/TABLE cover INSERT ... SELECT and friends.
 	fbInsertDataRe = regexp.MustCompile(`(?i)\b(VALUES?|SELECT|WITH|TABLE|SET|DEFAULT)\b`)
 	fbLeadKindRe   = regexp.MustCompile(`(?i)^\s*\(*\s*(SELECT|INSERT|UPDATE|DELETE|WITH)\b`)
-	fbDMLWordRe    = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`)
+	// fbInsertModRe is the optional modifier run MySQL allows between the
+	// statement keyword and the target table.
+	fbInsertModRe = `(?:\s+(?:LOW_PRIORITY|DELAYED|HIGH_PRIORITY|IGNORE))*`
+	// fbLeadInsertLikeRe recognizes the statement keywords that are an INSERT
+	// positionally without being spelled INSERT: MySQL/SQLite's REPLACE and the
+	// UPSERT accepted by the CockroachDB-derived grammar behind pgparser. Both
+	// bind by column order and carry the same schema-change risk, and both are
+	// the same AST node to the real grammars — so leaving them as StmtOther
+	// here means the rule fires only for callers who opted into a parser.
+	// INTO is optional (MySQL's is), but a table name must follow, which is
+	// what separates the statement from the REPLACE(str, from, to) function.
+	fbLeadInsertLikeRe = regexp.MustCompile(`(?i)^\s*\(*\s*(?:REPLACE|UPSERT)` + fbInsertModRe + `\s+(?:INTO\s+)?[^\s(]`)
+	// fbInsertHeadRe spans the statement keyword and its modifiers, used to
+	// find the target table when the optional INTO is absent. Anchored at the
+	// start, unlike fbIntoRe: without that anchor the keyword could be matched
+	// inside a CTE body (a REPLACE() call in a WITH clause), and the INTO-less
+	// forms are MySQL-only, where the CTE-prefixed shape needs INTO anyway.
+	fbInsertHeadRe = regexp.MustCompile(`(?i)^\s*\(*\s*(?:INSERT|REPLACE|UPSERT)` + fbInsertModRe + `\b`)
+	// fbInsertLikeWordRe is fbLeadInsertLikeRe without the start anchor, for
+	// the CTE-prefixed forms where the keyword follows the WITH clause. The
+	// table-name requirement is what keeps a REPLACE(str, from, to) call inside
+	// a CTE body from being read as the statement.
+	fbInsertLikeWordRe = regexp.MustCompile(`(?i)\b(?:REPLACE|UPSERT)` + fbInsertModRe + `\s+(?:INTO\s+)?[^\s(]`)
+	fbDMLWordRe        = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE)\b`)
 
 	// fbWhereRegionEndRe marks the first clause keyword that ends the WHERE
 	// region, so a function in ORDER BY / GROUP BY / HAVING isn't read as a
@@ -142,11 +165,24 @@ func (p *FallbackParser) Parse(sql string) (*Statement, error) {
 // both count as listed (no positional column-order risk to warn about).
 // Comment-free, literal-blanked input expected; heuristic by contract.
 func insertColumnsListed(sanitized string) bool {
-	loc := fbIntoRe.FindStringIndex(sanitized)
-	if loc == nil {
-		return true // no INTO found — can't tell, don't flag
+	// The statement head is the anchor whenever it starts the statement. Trying
+	// INTO first instead reads an identifier named "into" — a column name in a
+	// form that omits the keyword — as the clause, which makes the column list
+	// look like the target table and reports the columns as unlisted. Whether
+	// the real INTO stays in the span does not matter: it holds no "(".
+	var rest string
+	switch head := fbInsertHeadRe.FindStringIndex(sanitized); {
+	case head != nil:
+		rest = sanitized[head[1]:]
+	default:
+		// A CTE prefix puts the keyword mid-statement, where it can also occur
+		// inside the CTE body, so INTO is the reliable anchor there.
+		loc := fbIntoRe.FindStringIndex(sanitized)
+		if loc == nil {
+			return true // no recognizable anchor — can't tell, don't flag
+		}
+		rest = sanitized[loc[1]:]
 	}
-	rest := sanitized[loc[1]:]
 	data := fbInsertDataRe.FindStringIndex(rest)
 	if data == nil {
 		return true // no recognizable data clause — don't flag
@@ -448,6 +484,9 @@ func parenDepthBefore(s string, idx int) int {
 func detectKind(sanitized string) StmtKind {
 	m := fbLeadKindRe.FindStringSubmatch(sanitized)
 	if m == nil {
+		if fbLeadInsertLikeRe.MatchString(sanitized) {
+			return StmtInsert
+		}
 		return StmtOther
 	}
 	switch strings.ToUpper(m[1]) {
@@ -472,6 +511,9 @@ func detectKind(sanitized string) StmtKind {
 				return StmtDelete
 			}
 		}
+		if fbInsertLikeWordRe.MatchString(sanitized) {
+			return StmtInsert
+		}
 		return StmtSelect
 	}
 	return StmtOther

analyzer/rules.go

@@ -110,7 +110,7 @@ func CheckInsertWithoutColumns(s *Statement) (Result, bool) {
 		return Result{
 			RuleName:   "insert-without-columns",
 			Query:      s.Raw,
-			Message:    "INSERT without explicit column list. This breaks if table schema changes.",
+			Message:    "Row-inserting statement without an explicit column list. This breaks if table schema changes.",
 			Suggestion: "Specify columns explicitly: INSERT INTO table (col1, col2) VALUES (...).",
 		}, true
 	}

analyzer/statement.go

@@ -8,7 +8,12 @@ const (
 	StmtUnknown StmtKind = iota
 	// StmtSelect is a SELECT (or WITH ... SELECT) query.
 	StmtSelect
-	// StmtInsert is an INSERT statement.
+	// StmtInsert is an INSERT statement, or one of the dialect keywords that
+	// inserts rows the same way: MySQL/SQLite's REPLACE and the UPSERT the
+	// CockroachDB-derived grammar behind pgparser accepts. They bind values by
+	// column order exactly as INSERT does, so every rule that targets INSERT
+	// targets them. Note this also makes them DML to explain.validate, which
+	// accepts them under WithAllowDML rather than refusing them as unrecognized.
 	StmtInsert
 	// StmtUpdate is an UPDATE statement.
 	StmtUpdate
@@ -60,8 +65,13 @@ type Statement struct {
 	SelectDistinct bool
 
 	// InsertColumnsListed reports whether an INSERT names its target columns
-	// explicitly: INSERT INTO t (a, b) VALUES (...). Only meaningful when
-	// Kind == StmtInsert.
+	// explicitly, or has no columns to name. The second case is what keeps
+	// PostgreSQL's INSERT INTO t DEFAULT VALUES and MySQL's
+	// INSERT INTO t SET col = ... out of insert-without-columns: neither binds
+	// caller-supplied values by position, so no schema change can shift them.
+	// A parser filling this from an AST must account for both — reading it as
+	// "the column list is non-empty" alone reports DEFAULT VALUES.
+	// Only meaningful when Kind == StmtInsert.
 	InsertColumnsListed bool
 
 	// LeadingWildcardLike reports a LIKE pattern beginning with a wildcard