[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 7:43pm 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.

[github-actions]

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

---

Review of PR #871

I read the diffs for scanner.go, cbinding_recover.go, cbinding_testhelpers.go, tokscratch.go and dialect_extras.go. I did not run the tests, and I did not read the formatter changes or the 881-line JSON fixture. I found nothing blocking.

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

• scanStatementText appends ScanSQL findings and leaves the counts to Scan, so nothing is counted twice. Each new case takes only the text path, so AST and text checks can't report the same occurrence.
• I haven't checked this myself: statement types that wrap other statements, such as CREATE TRIGGER bodies, may reach the text path through more than one route. The test matrix suggests they don't, but a test where a statement appears inside an EXPLAIN or CREATE ... AS would settle it.
• Findings from ScanSQL(text) probably have locations relative to the statement text, not the original query. Please document that, or map the offsets as a follow-up.
• review_payloads_test.go needs a doc comment saying how to regenerate review_payload_findings.json. Otherwise future updates will be hand edits.

C binding (cbinding_recover.go)

• recoverExport is deferred directly, which is the only form where recover() works. Not exposing panic values is the right call.
• recover() != nil && result != nil: if the export's named result is a non-pointer, this path can't write it. I haven't checked every one of the 20 exports. The panic-injection test covers them, which is reassuring.
• gosqlx_free can recover a Go panic, but a native double free is a crash that recover can't catch. Say so in the doc comment so callers don't read it as "free is now safe to misuse".
• A recovered panic leaves no trace, which makes field debugging hard. Consider an opt-in hook that records only the panic's type, not its value.
• exportPanicHook and checkExportPanic ship in the production binary at the cost of one atomic load per call. A //go:build-tagged file or a test-only seam would keep them out of release builds, but the cost is negligible.
• versionIsNull in cbinding_testhelpers.go is fine.

AST accessors (dialect_extras.go)

• Returning values removes the shared-zero-pointer mutation hazard.
• The copy is shallow. Slices and maps inside SelectExtras, FunctionExtras and the others still alias the node, so element writes through the returned value still mutate the node. The doc comments say "a copy", which overpromises. Please change them to "shallow copy" and say that nested slices and maps are shared.
• SelectExtras is probably a large struct copied on every read. Your benchmark shows no regression, so this is only a note.

tokscratch

• requireImplementation turns a nil-function call into a clear panic message. It adds a nil check to each call, which is negligible.

Tests

• Panic injection across all 20 exports and the 91-payload matrix give solid coverage. Adding the nested-statement case above would be worthwhile.

Verdict: No blockers. I'd fix the "shallow copy" wording and the fixture regeneration note before merging. The other points are optional.