fix(analyzer): count WHERE/LIMIT only where it bounds the statement

.codeant/review.json

@@ -62,7 +62,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. 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.",
+      "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). HasWhere/HasLimit count a clause only where it bounds the statement's rows: the top level, and for a SELECT a derived table, CTE body or parenthesised set-operation operand, never IN (...), a scalar subquery or a function argument; the parsers OR their top-level AST answer with the fallback's (keepFallbackBounds) for every SELECT (#91). 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

@@ -166,6 +166,10 @@ reviews:
         with Exact=false, because blanking fields nothing was derived for drops
         findings silently (#81). HasLimit means a row count, not a limit node
         (#82).
+        HasWhere/HasLimit count only a clause that bounds the statement's rows:
+        top level, or for a SELECT a derived table, CTE body or parenthesised
+        set-operation operand — never IN (...) or a scalar subquery. Parsers OR their top-level AST
+        answer with the fallback's for every SELECT (#91).
 
     - path: "config/**"
       instructions: >-

.greptile/config.json

@@ -68,7 +68,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. 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.",
+      "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). HasWhere/HasLimit count a clause only where it bounds the statement's rows: the top level, and for a SELECT a derived table, CTE body or parenthesised set-operation operand, never IN (...), a scalar subquery or a function argument; the parsers OR their top-level AST answer with the fallback's (keepFallbackBounds) for every SELECT (#91). 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. 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).
+**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). `HasWhere`/`HasLimit` count a clause only where it bounds the statement's rows: the top level, and for a `SELECT` a derived table, CTE body or parenthesised set-operation operand (`scopedBounds`, which counts a keyword only where every enclosing parenthesis is a row source, so a scalar subquery nested inside a derived table does not count either), never `IN (...)`, a scalar subquery or a function argument. The parsers OR their top-level AST answer with the fallback's (`keepFallbackBounds`) for every `SELECT`, so the two agree by construction (#91).
 
 **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

@@ -15,6 +15,18 @@ the same version in lockstep.
 
 ### Fixed
 
+- **A `WHERE` or `LIMIT` inside a subquery counts only where it bounds the
+  statement** ([#91]). The default parser counted one anywhere and the
+  dialect parsers only at the top level, so the parsers reported findings the
+  default parser did not. Both now count the top level, and for a `SELECT` a
+  derived table, CTE body or parenthesised set-operation operand; one inside
+  `IN (...)`, a scalar subquery or a function argument does not. The default
+  parser therefore reports some findings it used to miss:
+  `UPDATE t SET a = (SELECT b FROM u WHERE …)` gets `update-without-where`
+  (it updates every row), and `… WHERE id IN (SELECT … LIMIT 1) ORDER BY a`
+  gets `orderby-without-limit`. The parsers take the derived-table, CTE and
+  set-operand part from the default parser, which replaces the set-operation
+  special case.
 - **A `sqlguard:ignore` inside a string literal no longer suppresses
   anything** ([#66]). The in-SQL directive only had to follow a comment
   marker somewhere earlier in the text, and the marker could itself be inside
@@ -70,6 +82,7 @@ the same version in lockstep.
 
 [#66]: https://github.com/KARTIKrocks/sqlguard/issues/66
 [#81]: https://github.com/KARTIKrocks/sqlguard/issues/81
+[#91]: https://github.com/KARTIKrocks/sqlguard/issues/91
 [#82]: https://github.com/KARTIKrocks/sqlguard/issues/82
 
 ## [0.5.0] - 2026-09-25