fix(parsers): stop dialect parsers dropping findings the fallback reports

.codeant/review.json

@@ -56,7 +56,7 @@
     },
     {
       "id": "parser-never-breaks-query-path",
-      "description": "An analyzer.Parser must never let a parse failure break the caller's query path: analyzer.Analyze degrades to the FallbackParser, and a dialect parser returns a best-effort Statement (Exact=false) rather than an error. A dialect parser may only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind, the zero-dependency 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 in TestParser_NeverAddsFindingTheFallbackDoesNot. The dialect parsers intentionally keep MaxInListLen, ImplicitCommaJoin and CartesianJoin as fallback heuristics; that carve-out is documented, not a bug.",
+      "description": "An analyzer.Parser must never let a parse failure break the caller's query path: analyzer.Analyze degrades to the FallbackParser, and a dialect parser returns a best-effort Statement (Exact=false) rather than an error. A dialect parser may only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind, the zero-dependency 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 in TestParser_NeverAddsFindingTheFallbackDoesNot. A dialect parser resets the structural fields only for an AST node it models; any other node - CREATE VIEW ... AS SELECT, EXPLAIN, DDL - must return the fallback's Statement untouched with Exact=false, since blanking fields nothing was derived for silently drops findings (#81), pinned by TestParser_KeepsFallbackFactsForUnmodelledStatements. A field refilled from the AST must mean what its name says: HasLimit is a row count, not the presence of a limit node (#82). The dialect parsers intentionally keep MaxInListLen, ImplicitCommaJoin and CartesianJoin as fallback heuristics; that carve-out is documented, not a bug.",
       "files": ["parsers/**/*.go", "analyzer/analyzer.go", "analyzer/fallback.go", "analyzer/parser.go", "analyzer/statement.go"],
       "scope": ["pr", "ide"]
     },

.coderabbit.yaml

@@ -150,6 +150,11 @@ reviews:
         derive structural facts from the AST but intentionally keep MaxInListLen,
         ImplicitCommaJoin and CartesianJoin as fallback heuristics — that carve-
         out is documented, not a bug.
+        Structural fields are reset only inside the case for an AST node the
+        parser models; any other node returns the fallback's Statement untouched
+        with Exact=false, because blanking fields nothing was derived for drops
+        findings silently (#81). HasLimit means a row count, not a limit node
+        (#82).
 
     - path: "config/**"
       instructions: >-

.greptile/config.json

@@ -62,7 +62,7 @@
     },
     {
       "id": "parser-never-breaks-query-path",
-      "rule": "A pluggable SQL parser (parsers/pgparser, parsers/mysqlparser) or analyzer.Parser implementation must never let a parse failure propagate as an error that breaks the caller's query path. On a parse failure, degrade to the FallbackParser's best-effort Statement (Exact=false) instead — that degradation lives in analyzer.Analyze. A dialect parser may also only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind the zero-dependency 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 in TestParser_NeverAddsFindingTheFallbackDoesNot. The dialect parsers deliberately keep MaxInListLen/ImplicitCommaJoin/CartesianJoin as fallback heuristics rather than deriving them from the AST — that is a documented carve-out, not a bug to fix.",
+      "rule": "A pluggable SQL parser (parsers/pgparser, parsers/mysqlparser) or analyzer.Parser implementation must never let a parse failure propagate as an error that breaks the caller's query path. On a parse failure, degrade to the FallbackParser's best-effort Statement (Exact=false) instead — that degradation lives in analyzer.Analyze. A dialect parser may also only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind the zero-dependency 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 in TestParser_NeverAddsFindingTheFallbackDoesNot. A dialect parser resets the structural fields only for an AST node it models; any other node (CREATE VIEW ... AS SELECT, EXPLAIN, DDL) must return the fallback's Statement untouched with Exact=false, since blanking fields nothing was derived for silently drops findings (#81), pinned by TestParser_KeepsFallbackFactsForUnmodelledStatements. A field refilled from the AST must mean what its name says: HasLimit is a row count, not the presence of a limit node (#82). The dialect parsers deliberately keep MaxInListLen/ImplicitCommaJoin/CartesianJoin as fallback heuristics rather than deriving them from the AST — that is a documented carve-out, not a bug to fix.",
       "scope": ["parsers/**/*.go", "analyzer/**/*.go"],
       "severity": "high"
     },

AGENTS.md

@@ -47,7 +47,7 @@ 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.
+**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. The converse holds too: **a dialect parser resets the structural fields only inside a `case` for an AST node it models, and any other node returns the fallback's `Statement` untouched with `Exact = false`.** Blanking them up front dropped every finding on `CREATE VIEW v AS SELECT * FROM t`, `EXPLAIN SELECT *` and the like (#81), because nothing had been derived to replace them — a false negative the "never adds" test cannot see, so `TestParser_KeepsFallbackFactsForUnmodelledStatements` pins it. A field refilled from the AST must mean what its name says: `HasLimit` is a row count, not the presence of a limit node, which pgparser also builds for a bare `OFFSET` (#82).
 
 **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.
 

CHANGELOG.md

@@ -9,6 +9,32 @@ the same version in lockstep.
 
 ## [Unreleased]
 
+### Fixed
+
+- **The dialect parsers no longer drop findings on statements they do not
+  model** ([#81]). `pgparser` and `mysqlparser` cleared every structural
+  field before looking at the AST and refilled them only for
+  `SELECT`/`INSERT`/`UPDATE`/`DELETE`, so anything else the grammar accepted
+  lost what the fallback had found and was still marked `Exact`:
+  `CREATE VIEW v AS SELECT * FROM t`, `CREATE TABLE … AS SELECT *` and
+  `EXPLAIN SELECT *` all lost `select-star`. Those statements now keep the
+  fallback's `Statement` with `Exact == false`. Both parsers also read the
+  row source of `INSERT … SELECT *` now, and read every operand of a
+  `UNION`/`INTERSECT`/`EXCEPT` instead of none of them, so
+  `SELECT * FROM t UNION SELECT * FROM u` reports `select-star` and
+  `select-without-limit` as the fallback does. `mysqlparser` had treated a
+  `UNION` as `StmtOther`.
+- **`pgparser` no longer treats a bare `OFFSET` as a `LIMIT`** ([#82]). The
+  grammar builds a limit node for `OFFSET n` alone, and `HasLimit` was set
+  from the node rather than from a row count, so `select-without-limit` and
+  `orderby-without-limit` never fired on `SELECT a FROM t ORDER BY a OFFSET
+  5000`. `LIMIT ALL` still counts as a limit.
+- The fallback parser reads the `ORDER BY` of a statement wrapped in
+  parentheses, `(SELECT … ORDER BY a)`, as the statement's own, as both
+  grammars do. It read it as a subquery's and missed
+  `orderby-without-limit`, which made `pgparser` report a finding the
+  fallback did not.
+
 ### Security
 
 - **`parsers/pgparser` no longer pulls a six-year-old gRPC stack.**
@@ -27,6 +53,9 @@ the same version in lockstep.
   `colord` and `serialize-javascript`. Build-time only — nothing here ships to
   consumers of the Go modules or to readers of the published site.
 
+[#81]: https://github.com/KARTIKrocks/sqlguard/issues/81
+[#82]: https://github.com/KARTIKrocks/sqlguard/issues/82
+
 ## [0.5.0] - 2026-09-25
 
 ### Fixed

analyzer/analyzer_test.go

@@ -195,6 +195,12 @@ func TestCheckOrderByWithoutLimit(t *testing.T) {
 		{"window order by", "SELECT row_number() OVER (ORDER BY id) FROM users", false},
 		{"ordered aggregate", "SELECT GROUP_CONCAT(x ORDER BY y) FROM t", false},
 		{"window order by with top-level order by", "SELECT rank() OVER (ORDER BY a) FROM t ORDER BY b", true},
+		// Parentheses around the whole statement are not a subquery.
+		{"parenthesised statement", "(SELECT id FROM users ORDER BY name)", true},
+		{"doubly parenthesised statement", "((SELECT id FROM users ORDER BY name));", true},
+		{"parenthesised statement with limit", "(SELECT id FROM users ORDER BY name LIMIT 10)", false},
+		{"parenthesised union arm", "(SELECT a FROM t ORDER BY a) UNION (SELECT b FROM u)", false},
+		{"subquery order by", "SELECT id FROM (SELECT id FROM users ORDER BY name) s", false},
 	}
 
 	for _, tt := range tests {

analyzer/fallback.go

@@ -370,7 +370,11 @@ func hasTopLevelJoin(region string) bool {
 // zero — a result-set sort, not a window-function (OVER (ORDER BY ...)),
 // ordered-aggregate (GROUP_CONCAT(... ORDER BY ...), WITHIN GROUP (ORDER BY
 // ...)), or subquery ordering, none of which sort the statement's result set.
+// Parentheses around the whole statement are not a subquery, so they are
+// unwrapped first: "(SELECT ... ORDER BY a)" sorts its result like the bare
+// form, and the dialect grammars read it that way.
 func hasTopLevelOrderBy(sanitized string) bool {
+	sanitized = unwrapStatementParens(sanitized)
 	for _, loc := range fbOrderByRe.FindAllStringIndex(sanitized, -1) {
 		if parenDepthBefore(sanitized, loc[0]) == 0 {
 			return true
@@ -439,6 +443,25 @@ func parenContent(s string, open int) (string, bool) {
 	return "", false
 }
 
+// unwrapStatementParens strips parentheses that enclose the entire statement
+// (an optional trailing ";" aside), repeatedly, so "((SELECT 1))" yields
+// "SELECT 1". A statement that merely starts with a parenthesised operand, like
+// "(SELECT a FROM t) UNION (SELECT b FROM u)", is returned unchanged.
+func unwrapStatementParens(sanitized string) string {
+	for {
+		s := strings.TrimSpace(sanitized)
+		s = strings.TrimSpace(strings.TrimSuffix(s, ";"))
+		if !strings.HasPrefix(s, "(") {
+			return sanitized
+		}
+		inner, ok := parenContent(s, 0)
+		if !ok || len(inner)+2 != len(s) {
+			return sanitized
+		}
+		sanitized = inner
+	}
+}
+
 // fromRegion returns the slice of sanitized SQL between the first top-level
 // FROM keyword and the next top-level clause keyword (WHERE, GROUP BY, a set
 // operator, ...), or "" when there is no top-level FROM. "Top-level" means at

analyzer/statement.go

@@ -132,9 +132,12 @@ type Statement struct {
 	// treat zero as unknown and not as "short", to avoid false negatives.
 	LeadingWildcardTermLen int
 
-	// Exact is true when the Statement was produced by a real SQL parser
-	// (structural analysis), false when produced by the regex fallback
+	// Exact is true when a real SQL parser derived the Statement's structural
+	// facts from an AST, false when they came from the regex fallback
 	// (best-effort). Rules may use this to suppress lower-confidence findings.
+	// A dialect parser that accepts a statement it does not model (DDL,
+	// EXPLAIN, CREATE VIEW ... AS SELECT) returns the fallback's Statement
+	// unchanged, so Exact is false there too.
 	//
 	// "Exact" covers the structural facts the dialect parsers derive from the
 	// AST: Kind, HasWhere/HasLimit/HasOrderBy/HasFrom, SelectStar,
@@ -143,6 +146,9 @@ type Statement struct {
 	// ImplicitCommaJoin, CartesianJoin, and the literal/text-level fields
 	// (LeadingWildcard*, NonSargablePredicate, AddNotNullNoDefault) — because
 	// they read literal values the AST discards or are intentionally text-level.
-	// Each such field documents this.
+	// Each such field documents this. For a set operation (UNION / INTERSECT /
+	// EXCEPT) HasWhere and HasLimit are taken from the fallback too, which
+	// counts a WHERE or LIMIT anywhere in the text, so the grammar can never
+	// report select-without-limit where the default parser does not.
 	Exact bool
 }