[ajitpratap0]

Summary

• watch: new --dialect flag; --mode values other than validate/format are rejected; changed files are re-validated, and format mode writes in place through the same verified path as format -i.
• MCP: parse_sql takes an optional dialect (enum of supported dialects); format_sql description now says comments are preserved; parse counts exclude GO separators.
• transpile: --help lists all eight dialects instead of three pairs. SELECT TOP n → LIMIT n (or FETCH FIRST for generic); LIMIT n → TOP n for SQL Server targets. TOP PERCENT, WITH TIES, non-literal counts, TOP mixed with LIMIT/OFFSET/FETCH, DML TOP and LIMIT WITH TIES→SQL Server return errors.
• lint: syntax errors are reported once (report only), not again by main.

Test Plan

• task check (fmt, vet, golangci-lint, race tests)
• task test:cbinding
• New tests: watch (dialect, both modes, unknown mode), lint output, transpile help, MCP parse dialect, row-limit rewrites
• CLI smoke: sqlserver→postgres TOP→LIMIT, postgres→sqlserver LIMIT→TOP, TOP PERCENT→mysql errors

Breaking Changes

• gosqlx watch --mode <unknown> now errors instead of silently validating.
• Transpiling SQL Server TOP to another dialect now rewrites or errors; previously TOP passed through unchanged.

🤖 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 5:01am UTC

[github-actions]

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

---

Review of PR #876

I reviewed the diff statically. I did not run the test suite or build, so the task check result in the PR description is unverified by me.

Overall: The change is well scoped and the safety choices are good. Unsafe TOP forms return errors instead of being silently reinterpreted. watch now reuses the format -i path, so it gets the same verification and write behaviour.

Suggestions and possible issues

1. Oracle target for TOP n → LIMIT n (pkg/transpiler/row_limits.go, the s.Limit = &limit block). Only generic (and "") gets FETCH FIRST. Oracle has no LIMIT, and older Oracle versions have no FETCH FIRST either. Please check what the formatter emits for sqlserver→oracle. If it prints LIMIT, either use FETCH FIRST for Oracle too or return an error. A test for the Oracle target would catch this.
2. Literal type matching (row_limits.go, the count.Type check). "INTEGER", "int" and "NUMBER" are matched as strings, which is brittle. If the parser has a constant for the integer literal type, use it. NUMBER also admits values like 1.5, which only fail later at Atoi. That is correct but a roundabout path, and the error message is the same, so it works.
3. Pooled literal released while the AST is still live (row_limits.go, ast.PutLiteralValue(count)). top.Count still points at the released literal, and s.Top is nil'd right after, so this looks safe today. It would be a use-after-release if a later rule or the caller held on to top. A one-line comment would help, or skip the release, since it is a one-off micro-optimisation.
4. RulesFor now never returns nil for cross-dialect pairs, because it always appends rowLimitRule. The doc comment is updated. Check that existing tests and callers that assert len(RulesFor(...)) == 0 for "unregistered pairs" still hold. task test:race presumably covers this.
5. watch format mode (watch.go). Writing in place fires another fsnotify event for the same file. The second pass reports "No changes needed", so it terminates. If format -i ever stops being idempotent for some input, this becomes a loop. It would be worth confirming that the debounce or an ignore-self-write guard covers this, or adding a test where the formatter output changes on every run.
6. silentError in lint is a reasonable fix for the duplicate-report problem. The if syntaxErrorCount > 0 path wraps it even when totalViolations > 0, and that summary line is now hidden from main's output. Check the report itself still shows the violation counts.
7. MCP parse_sql (pkg/mcp/tools.go). The enum limits the tool-schema input, but direct callers of parseSQLInternal still go through ParseWithDialect validation, which is good. The "GO separators excluded from count" change from the PR description is not in this part of the diff. I only saw the test change, so please make sure that behaviour is tested.
8. Minor: the SetOperation handling mutates LeftParenthesized, RightParenthesized and LeftWithHoisted inside the Inspect callback. That is fine, but the SQLite-only error path means TOP inside a UNION operand errors for SQLite and silently parenthesises for others. Mention this in the CHANGELOG so the SQLite exception is not a surprise.

Tests

Coverage is good: watch (both modes, dialect, unknown mode), lint output, transpile help, MCP dialect and row-limit rewrites. I would add:

• an Oracle target case (point 1);
• TOP in a CTE or subquery, going to SQL Server and away from it;
• LIMIT 0;
• OFFSET with LIMIT going to SQL Server (documented as left to the formatter).

Breaking changes

The two listed in the PR description are reasonable and documented in the CHANGELOG.

No security concerns found. Performance is fine, with one Inspect walk per transpile.

--- · branch fix/wp-g

[github-actions]

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

---

Review of PR #876 (updated for the latest push)

I reviewed this statically. I read pkg/transpiler/row_limits.go in full. I did not build the code or run the tests, so the task check claim in the description is unverified by me. I did not re-read the other changed files in this pass, so my earlier notes on watch, lint and MCP still stand but are unconfirmed.

Overall: The change is well scoped. Unsafe TOP forms return errors instead of being silently reinterpreted, and watch format mode reuses the format -i write path.

Issues and suggestions

1. Oracle target (row_limits.go:93-100). Only generic and "" get FETCH FIRST. Every other target, including Oracle, gets s.Limit. Oracle has no LIMIT, so confirm what the formatter prints for sqlserver→oracle. If it prints LIMIT, use FETCH FIRST for Oracle or return an error. Add a test for the Oracle target either way.
2. Literal type matching (row_limits.go:84). The code compares against the strings "INTEGER", "int" and "NUMBER". That is brittle, and the rewrite at line 105 creates a literal with the lowercase "int". Use a shared constant if the AST has one. Values like 1.5 pass the type check and are only rejected at Atoi. The result is correct, but it is a roundabout path.
3. Pooled literal released while still referenced (row_limits.go:95). top.Count still points at the literal that was just returned to the pool, even though s.Top is nil'd. This is safe today. It becomes a use-after-release if anything else holds top or the literal. Either skip the release (the saving is small) or add a comment explaining why it is safe.
4. TOP with LIMIT 0. limit < 0 is rejected, so TOP 0 becomes LIMIT 0, which is fine. The reverse, LIMIT 0 → TOP 0, should be covered by a test.
5. Set-operation operands (row_limits.go:41-64):
   • TOP inside a set-operation operand errors for SQLite but gets parenthesised for other targets. Mention the SQLite exception in the CHANGELOG.
   • Only direct SelectStatement operands are handled. A nested SetOperation operand is still reached by Inspect, so it is probably fine, but a test would confirm it.
6. Mutation inside Inspect. The rule mutates the AST in place. Please confirm Transpile always owns a freshly parsed AST. If an AST is ever cached or shared, this would corrupt it.
7. RulesFor now always includes rowLimitRule for cross-dialect pairs. Check that no existing test asserts an empty rule list for unregistered pairs. task test:race should catch this.
8. watch format mode. Writing in place triggers another fsnotify event for the same file. This terminates only because format -i is idempotent. Confirm the debounce or an ignore-self-write guard covers a non-idempotent formatter case.
9. lint silentError. Check that the violation counts still appear in the report when syntaxErrorCount > 0, now that main no longer prints the summary line.
10. MCP. The description says parse counts exclude GO separators. I only saw a test change for this, so confirm there is a test that covers it directly.

Suggested extra tests

• an Oracle target case;
• TOP in a CTE or subquery, both to SQL Server and away from it;
• LIMIT 0 → SQL Server;
• LIMIT with OFFSET → SQL Server, which is currently left to the formatter.

Breaking changes

The two listed in the description are reasonable and are in the CHANGELOG.

No security concerns found. Performance is fine, with one Inspect walk per transpile.
· branch fix/wp-g