[ajitpratap0]

Summary

Follow-ups from the pre-merge security and AST reviews of #866–#868:

• Scanner.Scan(tree) now sees newly parseable statements. Standalone VALUES and every statement the parser keeps as raw text go through the ScanSQL rules: utility, SET, transaction, USE, unsupported, T-SQL IF/WHILE/BEGIN bodies, PREPARE/EXECUTE, CREATE POLICY/TRIGGER. VALUES (SLEEP(5)) and IF 1=1 WAITFOR DELAY '0:0:5' are now reported as TIME_BASED. Each statement takes exactly one path, so nothing is reported twice.
• C binding: every //export recovers from a panic and returns its normal error result with a fixed message. A panic can no longer crash the host process, and panic values (which may contain SQL or paths) are not exposed.
• Read-only stand-ins: DialectExtras() / DialectModifiers() return values instead of pointers to shared zero values, so a caller can no longer mutate every node through them. EnsureExtras() remains the mutable path. These accessors are unreleased, so the signature change is safe.
• internal/tokscratch panics with a clear message if the tokenizer isn't linked.

Test Plan

• gofmt, go vet, golangci-lint run (0 issues), task test:race, task test:cbinding
• Payload matrix from the security review (91 payloads): every finding ScanSQL produced is preserved, and Scan(tree) now matches ScanSQL on the newly parseable statements
• Panic injection for all 20 C exports
• BenchmarkRealWorldParseFormat vs main, 4 interleaved runs: median 9.68 vs 9.63 ms/op, same B/op and allocs/op

Breaking Changes

DialectExtras() / DialectModifiers() now return values (SelectExtras, FunctionExtras, BinaryExtras, InsertDialectFields, DMLTargetFields, TableModifiers) instead of pointers. These were added after v1.16.0 and are unreleased.

🤖 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 27, 2026 8:51am UTC

[github-actions]

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

---

Review of PR #871

I read the diff for the scanner, C binding recovery, tokscratch and the DialectExtras changes. I did not run the tests, and I did not read the formatter, test or fixture files in detail. The change looks sound overall, and I found nothing blocking.

Scanner (pkg/sql/security/scanner.go)

• Each text-backed statement takes only the ScanSQL path. The AST walker has no case for these types, so findings are not duplicated, and the comment says counts are updated once in Scan.
• The 91-payload fixture is a good regression guard. It is a large JSON file (881 lines), so say in the test's doc comment how to regenerate it. That keeps future updates from being blind edits.
• Raw-text scanning has no source position mapping. Check that findings from ScanSQL(text) get locations relative to the statement text, not the original query. If they do, consider documenting it; if not, it is a small follow-up.

C binding (cbinding_recover.go and exports)

• recoverExport is deferred directly, so recover() works as intended. Named results are assigned correctly.
• The fixed error messages avoid leaking SQL or paths, and gosqlx_version returning NULL on panic is documented.
• gosqlx_free recovers a Go panic, but a real free fault (a double free, for example) is a native crash that recover cannot catch. The doc comment could say so, so callers don't assume free is now safe against misuse.
• A recovered panic is silent, which makes field debugging hard. Consider an opt-in debug hook or env-gated log that records the panic type only, not the value.
• checkExportPanic is a test hook that ships in the production binary. It costs one atomic load per call. That is fine, but a build-tagged test file would keep it out of release builds.

AST accessors (dialect_extras.go)

• Returning values fixes the shared-zero-pointer mutation hazard.
• The copy is shallow. Slices and maps inside SelectExtras and the others still alias the node, so element writes through the copy still mutate the node. The "copy" wording in the docs slightly overpromises. Say "shallow copy" there.
• SelectExtras may be a large struct copied on every read. Your benchmark shows no regression, so this is only worth noting.

tokscratch

• The clear panic message is a good change. A nil-function call would otherwise fail with an opaque nil dereference.

Tests

• Panic injection across all 20 exports and the scanner matrix give solid coverage.

Verdict: Approve-worthy once you decide on the "shallow copy" doc wording. The other points are optional.