[ajitpratap0]

Summary

Fixes the issues the 2026-10-28 re-verification found after #878–#885:

• CLI security scan (MEDIUM): gosqlx analyze --security and gosqlx lint's security pass used only Scan(AST), so SELECT 1; DROP TABLE users reported "No security issues detected". Both now also run ScanSQL on the raw text and merge the findings, deduplicated by pattern, severity and description. Inline SQL containing / (UNION/**/SELECT, 4 / 2) is no longer mistaken for a missing file path; real paths still read files.
• SQL Server row limits: LIMIT n OFFSET m and bare OFFSET without ORDER BY now get ORDER BY (SELECT NULL), and FETCH FIRST gets the required OFFSET 0 ROWS. Set operations without ORDER BY still return an error, because SQL Server requires UNION ORDER BY items to appear in the select list.
• Derived tables: row limits inside derived tables, JOIN operands and scalar subqueries are rewritten for the target dialect. Oracle kept LIMIT there.
• WITH TIES: ast.QueryTailFrom dropped the flag. It now survives set-operation tails, and targets that cannot express it return an error.
• LSP: unknown request methods return -32601 (MethodNotFound) instead of -32603.

Test Plan

• New tests:

File	Covers
analyze_security_regression_test.go	text and JSON output, clean SQL, /**/ input, existing files
cmdutil/input_test.go	input-vs-file detection
row_limits_test.go	SQL Server row limits; outputs reparse in the target dialect
pkg/transpiler/wpq_nested_test.go, pkg/formatter/wpq_nested_test.go	nested row limits
query_tail_test.go	WITH TIES round trips
method_not_found_test.go	LSP error code

• task check (55 packages, golangci-lint 0 issues) and task test:cbinding pass locally.
• Performance: a cheap text pre-check keeps select lists without a subquery on the old path, so allocations match main (16 for Format, 26 for realworld dialect formatting). Realworld dialect formatting is about 4% slower from the new derived-table and set-operation rewrites. Nested scalar subqueries in the select list are copied per level; at the parser's maximum depth that costs low milliseconds.

Breaking Changes

• analyze --security and lint may report more findings, namely those ScanSQL catches.
• Transpile to a target that cannot express WITH TIES now returns an error instead of silently dropping it.

🤖 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 9:02pm UTC

[github-actions]

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

---

Review of PR #887

I read the non-test diff only. I did not build the code or run the tests.

Overall: The changes are focused and the tests cover each fix. The notes below are mostly edge cases and maintainability.

Findings

1. pkg/formatter/render_select_ext.go (selectColumnsForDialect)

   • Reflection copy: It deep-copies expressions with reflect (copyRenderValue) and then replaces each subquery with a UtilityStatement{Text: ...}. This works, but it is fragile. If ast gains new node types or unexported or pooled fields, the copy can silently go wrong.
   • Local named copy: The variable copy shadows the builtin. go vet accepts it, but renaming it avoids confusion.
   • select pre-check: containsFoldASCII(plain, "select") is a heuristic. A column or string literal that merely contains "select" takes the slow path. That only costs time, because the result is still correct. The other direction is the risk: a subquery that renders without the select keyword, such as a VALUES or TABLE subquery, would skip the rewrite. This is probably rare, but it is worth a comment or a test.
   • Missing test: Add one for a quoted identifier or string containing "select".

2. pkg/transpiler/row_limits.go

   • ORDER BY (SELECT NULL) injection: It is only valid on SQL Server, and the code correctly gates it on to == DialectSQLServer. Inside a derived table or CTE, SQL Server also forbids ORDER BY unless TOP, OFFSET or FOR XML is present. OFFSET is present here, so this is fine.
   • QueryTail handling: *tail = *ast.QueryTailFrom(view) rebuilds the tail from a scratch view. Any QueryTail field that Select() doesn't carry would be lost. It is correct today, but a test that round-trips every field would guard against future drift.
   • normalizeTies: LIMIT … WITH TIES is converted to FETCH FIRST … WITH TIES for Postgres, Oracle and generic. Other targets get an error, and ClickHouse passes through. MySQL and SQLite error out, which matches the stated breaking change. Two cases deserve tests:
     • FETCH … WITH TIES with no ORDER BY.
     • A LimitExpr that is not a literal.

3. cmd/gosqlx/cmd/sql_analyzer.go (mergeSecurityFindings)

   • Dedup key: The key omits line and column. Two genuine findings with the same pattern, severity and description at different positions collapse into one. That is intentional here (the AST and raw scans report different positions), but a query with several identical injections will now show only one finding.
   • Nil results: result.Findings panics if any result is nil. The callers pass non-nil values, but a nil guard is cheap.
   • Missing cleanup in lint.go: The defer ast.ReleaseAST(tree) added in lint.go is a good fix. Check whether Analyze in sql_analyzer.go has the same issue.

4. cmd/gosqlx/cmd/input_utils.go and cmdutil/input.go

   • Behavior change: !looksLikeSQL(input) && (contains separator || .sql) means a missing file whose name looks like SQL is now treated as SQL. For example, DELETE/old.sql or select/x.sql is treated as inline SQL and fails at parse time with a less helpful message than "invalid file path". This is a reasonable trade-off.
   • Duplication: The cmd and cmdutil copies of this logic are duplicated. Check that looksLikeSQL and LooksLikeSQL stay in sync.

5. pkg/lsp

   • Error detection: errors.Is(err, errMethodNotFound) is clean. The fmt.Errorf("%w: %s", ...) form keeps the message text.
   • Notifications: Check that unknown notifications (requests with no ID) are still ignored rather than answered with -32601. The LSP spec says servers must not reply to them.

6. pkg/sql/ast/query_tail.go

   • WITH TIES carried through: The fix is correct and symmetric across Select() and QueryTailFrom.
   • Leak fix: The added release of Fetch.FetchExpr is a good catch.

Performance

The pre-check keeps the old path for most queries, which matches the allocation numbers in the description. The nested scalar subquery copy is O(depth²), which is acceptable at the parser's depth limit.

Test coverage

Coverage is good for the listed scenarios. The gaps to consider are the "select" heuristic edge cases, the FETCH … WITH TIES and LimitExpr paths, and notification handling in LSP.

No blocking issues found.

[github-actions]

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

---

Review of PR #887

I read the diff for the CLI, LSP and input-detection changes and checked it against the earlier review notes. I did not build the code or run the tests.

Overall: The changes are focused and each fix has a test. I found no blocking issues.

Verified

• LSP: The -32601 mapping is correct. errors.Is(err, errMethodNotFound) works through the %w wrap, and the message text is kept. Unknown notifications are not affected, because only the branch that handles requests with an ID calls sendError. The earlier concern about replying to notifications does not apply.
• mergeSecurityFindings: The dedup and severity counting are correct. The dedup key leaves out position, so several identical injections in one query collapse into one finding. That is a reasonable trade-off, and I'd note it in the docs or CHANGELOG.

Suggestions (non-blocking)

1. Nil guard: mergeSecurityFindings panics if a result is nil. Both callers pass non-nil values today, but if result == nil { continue } costs nothing.
2. Input detection: !looksLikeSQL(input) && (separator || .sql) means a missing file with an SQL-like name, such as select/x.sql, is treated as inline SQL. It then fails at parse time instead of reporting "invalid file path". That seems fine, but keep looksLikeSQL and cmdutil.LooksLikeSQL in sync. A shared helper would be better than two copies.
3. selectColumnsForDialect:
   • Rename the local variable copy, which shadows the builtin.
   • The reflection copy (copyRenderValue) is fragile if AST nodes gain unexported or pooled fields.
   • The "select" text pre-check could skip subqueries that render without that keyword (VALUES, TABLE). Add a comment or a test.
   • Add a test for a quoted identifier or string literal that contains "select".
4. Transpiler tests: Add cases for FETCH … WITH TIES without ORDER BY and for a non-literal LimitExpr. A test that round-trips every QueryTail field would catch future drift in QueryTailFrom.
5. lint.go: The added defer ast.ReleaseAST(tree) is a good fix. Check whether the other call sites of Analyze release their AST the same way.

Performance and security

The cheap pre-check keeps the common path unchanged, which matches the allocation numbers in the PR description. The nested scalar-subquery copy is O(depth²) and acceptable at the parser's depth limit. Running ScanSQL on the raw text closes the SELECT 1; DROP TABLE users gap without adding any new input handling.