[ajitpratap0]

Summary

• Scanner.Scan(tree) only inspected top-level SELECT WHERE/HAVING/columns/FROM, INSERT VALUES rows, UPDATE/DELETE WHERE and set-operation leaves. Time-based, out-of-band and tautology patterns nested anywhere else were missed, while ScanSQL caught them. On origin/main, 37 of 59 probe queries showed this gap. Missed locations included CTEs, derived tables, subqueries, CASE/CAST/IN/BETWEEN/ANY, ORDER/GROUP BY, JOIN ON, window specs, FILTER, LIMIT, LATERAL/APPLY, PREWHERE, QUALIFY, CONNECT BY, RETURNING, ON CONFLICT, INSERT ... SELECT, MERGE, CREATE TABLE AS, CREATE [MATERIALIZED] VIEW and EXPLAIN.
• Scan now uses ast.Inspect. It checks every FunctionCall and table function once and checks tautologies in every condition position (WHERE, HAVING, QUALIFY, PREWHERE, START WITH/CONNECT BY, JOIN ON/MATCH_CONDITION, MERGE ON/WHEN, CASE WHEN, FILTER, ON CONFLICT WHERE). UNION checks now also run on outermost set operations nested in CTEs and subqueries. Text-bodied nodes (Values, Utility, Set, Transaction, Use, Unsupported) still use the text path and are pruned from the AST walk, so no occurrence is reported twice. JoinClause.Left is skipped because it repeats an earlier reference.
• Qualified identifier comparisons such as a.id = b.id are no longer reported as tautologies. Before this change only the column name was compared.
• T-SQL IF ... WAITFOR DELAY and standalone VALUES were already detected on main (fix(security,cbinding): scan raw-text statements, recover from panics in C exports #871). They now have regression tests.

Test Plan

• New pkg/sql/security/ast_coverage_test.go:
  • 50 positive cases across PostgreSQL, Oracle, ClickHouse, SQL Server, MySQL and Snowflake, with exact-count assertions where duplicates are a risk.
  • A parity check that, for the TIME_BASED, OUT_OF_BAND and TAUTOLOGY patterns, Scan reports everything ScanSQL reports.
  • Clean negative cases, nested UNION system-table cases, and a concurrent-scan test.
• task check (fmt, vet, golangci-lint 0 issues, test:race): pass, no FAIL lines.
• task test:cbinding: pass.
• Scanner benchmarks, -count=5 -benchmem, before (origin/main) vs after. Typical Scan cost rose from about 25–200 ns to about 160–1250 ns, with 7–16 allocations per call. Most of that comes from allocating the Children() slices during the walk. The old cost was low because the scanner skipped most of the tree. The scanner is not a tokenizer/parser/formatter hot path, and parsing the same query costs several microseconds, so this is accepted as the cost of complete coverage. The two text-based benchmarks did not change significantly (DangerousFunctions −14%, TimeBasedInjection no change).

Breaking Changes

None to the API. Scan now returns more findings for queries with nested dangerous patterns. It returns fewer for qualified column equalities (a.id = b.id), which are no longer reported as tautologies.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 28, 2026 7:30pm UTC

[github-actions]

Claude finished @ajitpratap0's task in 23s —— View job

---

Review of PR #879: walk the full AST in Scanner.Scan

I read the diff and checked ast.Walk and ast.Inspect in pkg/sql/ast/visitor.go. I did not run the test suite, because the shell sandbox blocked go test here. I'm relying on the PR's reported task check and test:cbinding results.

Overall: The change looks sound. It replaces about 200 lines of hand-written traversal, which silently skipped many node types, with one ast.Inspect pass. That removes a whole class of "forgot to descend into X" bugs.

What I verified

• Ancestor stack stays balanced. Walk calls Visit(nil) only after it has descended into a node's children. When the callback returns false, as in the pruned text-bodied nodes and the JoinClause.Left skip, it never calls Visit(nil). Those paths also return before the append, so each push has exactly one pop.
• Concurrency. ancestors is local to each Scan call, so scans share no state. This matches the concurrent-scan test.
• No double reporting. FunctionCall is checked once by the walk. scanTableReference now only checks the table-function name, and the walk visits the TableFunc call itself separately. Text-bodied nodes are pruned, so the text and AST paths don't overlap.
• Tautology fix. Requiring leftIdent.Table == rightIdent.Table correctly stops a.id = b.id from being flagged.

Suggestions (non-blocking)

1. Tautology check on Filter. s.scanCondition(n.Filter, result) runs inside the FunctionCall case. That is correct, but it is the only condition scan done outside a statement-level case. A short comment saying so would help. Also confirm that Children() for FunctionCall includes Filter, so functions inside it get visited (the PR's FILTER test likely covers this).
2. Stray blank line. In scanFunctionCall, removing the argument recursion leaves an empty line before the closing } at about line 1153 of scanner.go. gofmt accepts it, but it should be removed.
3. Parent check on SetOperation. The parent.(*ast.SetOperation) check relies on Left and Right being direct children. If a parenthesized wrapper node is ever introduced between a set operation and its operand, the nested operation would get a second UNION check and report duplicates. A test with a parenthesized operand (a UNION (b UNION c)) would pin this down. The PR mentions nested UNION cases, so this may already be covered.
4. Allocation cost. The benchmark growth (about 25–200 ns up to about 160–1250 ns, 7–16 allocs) is acknowledged and reasonable. If it ever matters, the main cost is Children() slice allocation inside Walk, which is outside this PR's scope. A Scan benchmark on a large query (many CTEs or joins) would help catch regressions.
5. Behavior change. Scan now returns more findings for nested patterns and fewer tautology findings for qualified equalities. Both are documented in the PR description and CHANGELOG, which is good. Downstream consumers (cbinding, LSP, CLI) that count findings may see different numbers, so mention this in the release notes.

Tests

ast_coverage_test.go is thorough: 50 positive cases across six dialects, a Scan vs ScanSQL parity check, negatives, and a concurrency test. One gap: add a negative case that a.id = a.id still IS flagged, to lock in the qualified-identifier edge of the tautology fix.

No blocking issues found.

[github-actions]

Claude finished @ajitpratap0's task in 19s —— View job

---

Review of PR #879: walk the full AST in Scanner.Scan

I read the scanner.go diff against origin/main. I did not run the tests. I'm relying on the PR's reported task check and test:cbinding results.

Overall: The change looks good. Replacing the hand-written traversal with a single ast.Inspect pass removes the "forgot to descend into X" class of gaps. It also makes Scan and ScanSQL agree.

Checked

• Ancestor stack. Every path that returns false does so before the append, so no Visit(nil) pop is owed for it. Every pushed node gets exactly one pop. This holds for the pruned text-bodied nodes and for the JoinClause.Left skip.
• Concurrency. ancestors is local to each call, so scans share no state.
• No double reporting. scanFunctionCall now checks only the call's own name, and the walk visits each FunctionCall once. scanTableReference checks only the table-function name. Text-bodied nodes are pruned, so the text and AST paths don't overlap.
• Qualified tautologies. Adding leftIdent.Table == rightIdent.Table correctly stops a.id = b.id from being flagged.

Suggestions (non-blocking)

1. Stray blank line. scanFunctionCall (pkg/sql/security/scanner.go, near the end of the function) now has an empty line before the closing }. This is left over from removing the argument recursion. Please delete it.
2. Comment on the FunctionCall case. s.scanCondition(n.Filter, result) is the only condition scan that doesn't sit in a statement-level case. A one-line comment would help. Please also confirm that FunctionCall.Children() includes Filter, so functions inside the FILTER clause get visited. The FILTER test probably covers this.
3. Parent-based SetOperation skip. The parent.(*ast.SetOperation) check relies on operands being direct children. Please confirm that the test file includes a UNION (b UNION c) and (a UNION b) UNION c. It should also include a set operation inside a CTE or subquery that sits under another set operation, to pin down the no-duplicate behaviour. The removed code's comment shows these parenthesized forms were a known case.
4. Test gap. Add a case that a.id = a.id is still flagged as a tautology. Add another that a.id = b.id is not. The second locks in the qualifier fix. The PR describes it but I didn't see it listed.
5. Performance. The cost rise (about 25–200 ns to about 160–1250 ns, 7–16 allocs) is mostly Children() slice allocation inside Walk. That is reasonable for complete coverage and outside this PR's scope. A Scan benchmark on a large query, such as many CTEs or joins, would help catch regressions later.
6. Release notes. Downstream consumers that count findings (CLI, LSP, cbinding) will see different numbers. More findings appear for nested patterns, and fewer appear for qualified equalities. The CHANGELOG entry covers this. Please also mention it in the release notes.

No blocking issues found.