fix: address all 19 post-UAT bugs — parser, CLI, security, AST (v1.8.0)

[ajitpratap0]

Summary

This PR resolves all 19 actionable bugs discovered by 8 parallel UAT agents after merging v1.8.0. Changes span the tokenizer, parser, AST, CLI, and security scanner and were implemented by 7 parallel fix agents with zero merge conflicts.

All 47 packages pass go test -race -timeout 120s ./... with zero race conditions.

---

Bug Fixes

Errors

ID	Fix
ERR-1	Add dedicated E1009 ErrCodeUnterminatedBlockComment error code and UnterminatedBlockCommentError builder; update tokenizer to emit E1009 instead of the generic string-error E1002 for unclosed /* ... */ comments

Parser — Core

ID	Fix
CORE-1	Reserved keywords (KEY, INDEX, VIEW, TABLE, COLUMN, DATABASE) now accepted as column names after a dot in qualified identifiers (e.g. a.key, schema.index) — fixes post-dot check in expressions_literal.go
CORE-2	NATURAL JOIN now stores join type "NATURAL" instead of "NATURAL INNER" — added explicitType flag in parseJoinType() so the implicit INNER default is not prepended
CORE-3	OVER <window_name> (bare named-window reference) now parsed correctly per SQL:2003 §7.11; parseWindowSpec() checks for an identifier before requiring (

Parser — Dialect

ID	Fix
DIALECT-1	MySQL backtick-quoted reserved words (`table`, `select`) now parse correctly as identifiers — isNonReservedKeyword() extended with DDL keyword types; regression test added
DIALECT-2	SQL Server bracket-quoted reserved words ([table], [select]) now parse correctly; regression test added
DIALECT-3	SQLite PRAGMA statement fully implemented — new PragmaStatement AST node and pragma.go parser supporting all three forms: PRAGMA name, PRAGMA name(arg), PRAGMA name = value
DIALECT-4	SQLite WITHOUT ROWID implemented in parseCreateTable(); parseColumnName() added to ddl_columns.go so reserved keywords like KEY work as DDL column names

CLI — Output

ID	Fix
CLI-1/2	token_count in parse output now reflects the actual token count from the tokenizer (was always 0 in JSON mode; was len(statements) in text mode)
CLI-3	analyze — Query Size now reports real character and line counts (was 0 characters, 0 lines)
CLI-4	format — first SELECT column is now correctly indented to match subsequent columns
CLI-5	parse — CTE/WITH clause now included in output ("has_with": true, "cte_count": N) for both text and JSON formats

CLI — UX

ID	Fix
CLI-6	validate — cmd.SilenceUsage = true added; cobra no longer prints the full usage/help block on domain errors (invalid SQL, empty input, etc.)
CLI-7	lint — now exits non-zero (exit 1) whenever any violation (error, warning, or info) is found, making it usable as a CI quality gate without --fail-on-warn; all affected tests updated
CLI-8	validate — standardized on ✅/❌ for success/failure across all output paths (was mixing ✓/✗ with ✅/❌)

Security

ID	Fix
SEC-1	Add tautology detection to ScanSQL() — 1=1, 'a'='a', col=col, and OR TRUE patterns all trigger PatternTautology / CRITICAL; uses two-step capture + strings.EqualFold comparison (RE2 does not support backreferences)
SEC-2	Split PatternUnionBased (was blanket CRITICAL) into PatternUnionInjection (CRITICAL — system tables + NULL-padding fingerprints) and PatternUnionGeneric (HIGH — any UNION SELECT); eliminates false-positive CRITICAL on legitimate application-level UNION queries

AST

ID	Fix
CORE-4	Add full Go deprecation doc to AlterTableStatement with migration guide pointing to AlterStatement (the type actually produced by all Parser.Parse* methods)

---

Files Changed

Area	Files
Errors	pkg/errors/errors.go, pkg/errors/builders.go
Tokenizer	pkg/sql/tokenizer/tokenizer.go
AST	pkg/sql/ast/ast.go
Keywords	pkg/sql/keywords/dialect.go
Parser	pkg/sql/parser/parser.go, select_clauses.go, window.go, expressions_literal.go, ddl.go, ddl_columns.go, pragma.go (new)
Parser tests	pkg/sql/parser/dialect_test.go, core_fixes_test.go (new)
Security	pkg/sql/security/scanner.go, scanner_test.go, dollar_quote_test.go, sec_verify_test.go (new)
CLI	cmd/gosqlx/cmd/parser_cmd.go, sql_analyzer.go, sql_formatter.go, validate.go, lint.go, lint_test.go, analyzer.go
CLI output	cmd/gosqlx/internal/output/json.go, json_test.go

---

Test Plan

• go test -race -timeout 120s ./... — all 47 packages pass, zero race conditions
• go test -run TestPerformanceRegression ./pkg/sql/parser/ — within tolerance
• go vet ./... — zero warnings
• gofmt — all files formatted
• Spot-checked CLI fixes with built binary:
  • validate "SELECT a.key FROM t1 a JOIN t2 b ON a.key = b.key" → ✅ Valid SQL
  • parse -f json "WITH cte AS (SELECT 1) SELECT * FROM cte" → "cte_count": 1, "token_count": 12
  • format "select id,name,email from users" → first column indented
  • analyze "SELECT * FROM t" → 15 characters, 1 lines
  • lint with violations → exit 1
  • validate "INVALID SQL" → no usage block printed

🤖 Generated with Claude Code

[claude]

Claude finished @ajitpratap0's task —— View job

---

Comprehensive PR Review — Post-UAT Bug Fixes v1.8.0 ✅

Review Summary

This is an exceptionally well-executed PR that addresses 19 post-UAT bugs with surgical precision. The implementation demonstrates strong software engineering practices with comprehensive test coverage, proper error handling, and thoughtful architecture decisions.

---

✅ Strengths

1. Error Handling (ERR-1)

• Excellent: New ErrCodeUnterminatedBlockComment (E1009) follows established patterns perfectly
• Clean API: UnterminatedBlockCommentError() builder provides consistent interface
• Proper Context: Meaningful error messages with 2-character highlight length for /*

2. Parser Core Fixes (CORE-1 to CORE-3)

CORE-1 - Reserved Keywords After Dot:

• Smart Implementation: isNonReservedKeyword() extension properly handles DDL keywords
• Comprehensive Coverage: Tests cover KEY, INDEX, VIEW, TABLE, COLUMN, DATABASE
• Location: expressions_literal.go:149-158 - clean logic

CORE-2 - NATURAL JOIN:

• Elegant Solution: explicitType flag in parseJoinType() prevents implicit INNER prepending
• Correct Behavior: NATURAL JOIN → "NATURAL" vs NATURAL INNER JOIN → "NATURAL INNER"

CORE-3 - Window Functions:

• Standards Compliant: Implements SQL:2003 §7.11 for bare window references
• Robust Parsing: parseWindowSpec() correctly handles both OVER (...) and OVER window_name

3. Dialect Support (DIALECT-1 to DIALECT-4)

SQLite PRAGMA:

• Complete Implementation: New pragma.go handles all three forms correctly
• Clean AST: PragmaStatement with Name, Arg, Value fields
• Good Error Handling: Proper token validation and consumption

MySQL/SQL Server Quoting:

• Regression Prevention: Dedicated tests ensure backticks and brackets work as identifiers
• Proper Integration: Leverages existing isNonReservedKeyword() infrastructure

4. Security Scanner Improvements (SEC-1, SEC-2)

Tautology Detection:

• ReDoS Mitigation: Smart 10k character limit with bounded regex patterns
• Two-Step Approach: RE2 limitation handled elegantly with capture + verification
• Comprehensive Patterns: Covers numeric, string, and identifier tautologies

UNION Classification:

• Risk-Based Severity:
  • PatternUnionInjection (CRITICAL): System tables + NULL padding
  • PatternUnionGeneric (HIGH): Standard UNION SELECT
• Reduces False Positives: Legitimate app-level UNIONs no longer flagged as CRITICAL

5. CLI Enhancements (CLI-1 to CLI-8)

User Experience:

• Silent Validation: cmd.SilenceUsage = true prevents help spam on SQL errors
• Consistent Exit Codes: lint exits 1 on any violation for CI integration
• Emoji Standardization: Unified ✅/❌ across all commands

Output Accuracy:

• Token Count Fix: Now reflects actual tokenizer output instead of hardcoded 0
• Query Analysis: Real character/line counts instead of placeholder zeros
• CTE Detection: Proper has_with and cte_count in JSON output

6. Testing Excellence

Comprehensive Coverage:

• New Test File: core_fixes_test.go with 230 lines of focused regression tests
• Security Verification: sec_verify_test.go validates both SEC-1 and SEC-2 fixes
• Type Safety: Proper two-value type assertions throughout

---

🔍 Code Quality Assessment

Architecture & Design

• Modular: Clean separation between tokenizer, parser, AST, and CLI layers
• Consistent Patterns: All new code follows existing conventions
• Backward Compatible: AST deprecation handled gracefully with migration guide

Performance Considerations

• Efficient Patterns: Pre-compiled regex with sync.Once initialization
• Memory Safety: Proper object pooling patterns maintained
• Bounded Operations: ReDoS protection via input length limits

Security

• Defense in Depth: Multiple layers of injection detection
• Safe Defaults: Unknown severities treated as high-priority (fail-safe)
• Input Validation: Proper bounds checking and sanitization

---

📋 Minor Observations

1. SQLite PRAGMA (pragma.go:47-50): The token consumption could benefit from a helper method for repeated patterns, but current implementation is clear and correct.

2. Tautology Patterns (scanner.go:341-348): The regex bounds (6 digits, 50 chars) are reasonable for ReDoS protection, though documentation of the rationale would help future maintainers.

3. Test Coverage: While comprehensive, consider adding edge case tests for malformed PRAGMA statements to ensure proper error handling.

---

🎯 Recommendations

Immediate (Optional)

• Consider documenting the tautology regex bounds in code comments
• Add a few malformed PRAGMA test cases for completeness

Future Considerations

• Monitor performance impact of new security patterns in production
• Consider consolidating CLI emoji constants into a shared package

---

✅ Final Verdict

APPROVED - This PR demonstrates exceptional software engineering with:

• Clean Architecture: Well-separated concerns with consistent patterns
• Comprehensive Testing: 230+ lines of new regression tests
• Security Focus: Thoughtful injection detection with proper severity classification
• User Experience: Polished CLI with consistent feedback
• Production Ready: Race-free with proper error handling

The implementation addresses all 19 identified bugs with surgical precision while maintaining the high code quality standards of GoSQLX. The test coverage and regression prevention measures ensure these fixes will remain stable.

Zero concerns with merging this PR.

---

[claude]

Claude finished @ajitpratap0's task —— View job

---

Comprehensive PR Review — Post-UAT Bug Fixes v1.8.0 ✅

I've completed a thorough review of this PR focusing on code quality, potential issues, performance, security, and test coverage.

Todo List:

• Analyze error handling changes (ERR-1)
• Review parser core fixes (CORE-1 to CORE-3)
• Examine dialect support improvements (DIALECT-1 to DIALECT-4)
• Evaluate CLI enhancements (CLI-1 to CLI-8)
• Review security scanner improvements (SEC-1, SEC-2)
• Assess AST changes (CORE-4)
• Check test coverage and quality
• Evaluate performance considerations
• Provide final recommendations

---

✅ Overall Assessment: EXCELLENT IMPLEMENTATION

This PR demonstrates exceptional software engineering practices with comprehensive bug fixes, thoughtful design decisions, and thorough testing.

---

✅ Code Quality & Best Practices

Error Handling (ERR-1):

• New ErrCodeUnterminatedBlockComment (E1009) follows established patterns perfectly
• Clean UnterminatedBlockCommentError() builder with proper 2-character context highlighting
• Consistent integration with existing error infrastructure

Parser Core Fixes (CORE-1 to CORE-3):

• CORE-1: Elegant handling of reserved keywords after dots in qualified identifiers
• CORE-2: Smart explicitType flag prevents incorrect "NATURAL INNER" concatenation
• CORE-3: SQL:2003 §7.11 compliant window function parsing with proper precedence

Dialect Support (DIALECT-1 to DIALECT-4):

• SQLite PRAGMA: Complete implementation handling all three forms with clean AST design
• Quoted Reserved Words: Comprehensive coverage for MySQL backticks and SQL Server brackets
• Keyword Integration: Seamless extension of existing isNonReservedKeyword() infrastructure

CLI Enhancements (CLI-1 to CLI-8):

• Token Count Fix: Now reflects actual tokenizer output instead of hardcoded 0
• UX Improvements: SilenceUsage = true, standardized emojis, proper exit codes
• Output Accuracy: Real character/line counts, CTE detection in JSON output

AST Changes (CORE-4):

• Proper deprecation documentation for AlterTableStatement with clear migration guide

---

🛡️ Security Analysis

SEC-1 - Tautology Detection:

• ReDoS Protection: Bounded regex patterns (6 digits, 50 chars) with 10k input limit
• Elegant Solution: Two-step capture + verification approach works around RE2 limitations
• Comprehensive Coverage: Detects numeric, string, and identifier tautologies

SEC-2 - UNION Classification:

• Risk-Based Severity: Properly distinguishes injection attempts (CRITICAL) from legitimate queries (HIGH)
• False Positive Reduction: System table detection prevents legitimate UNIONs from being flagged as CRITICAL
• Precise Matching: Both system table prefixes and exact names covered

---

⚡ Performance Considerations

Excellent Performance Design:

• sync.Once initialization for regex patterns prevents compilation overhead
• Bounded input processing prevents DoS vulnerabilities
• Zero-copy optimizations maintained throughout
• Object pooling patterns preserved

No Performance Regressions Identified:

• Changes are surgical and don't affect hot paths
• New security patterns are optimized for production use
• CLI improvements don't impact core parsing performance

---

🧪 Test Coverage

Comprehensive Testing:

• New Test File: /pkg/sql/parser/core_fixes_test.go (230 lines) with focused regression tests
• Security Verification: /pkg/sql/security/sec_verify_test.go validates both SEC-1 and SEC-2 fixes
• CLI Coverage: All CLI fixes have corresponding test updates
• Type Safety: Proper two-value type assertions throughout tests

Test Quality:

• Clear test naming with bug ID references
• Comprehensive edge case coverage
• Proper resource cleanup with defer ast.ReleaseAST()

---

🚨 Potential Issues

Minor Considerations (Non-blocking):

1. PRAGMA Parser (/pkg/sql/parser/pragma.go:47-50): Token consumption could be more defensive about malformed arguments, but current implementation is reasonable for production.

2. Tautology Regex Bounds: The 6-digit and 50-character limits are well-reasoned for ReDoS protection, though inline documentation of these bounds would help future maintainers.

3. CLI Test Updates: Some test expectations changed (exit codes, output format) - ensure downstream users are aware of these behavioral changes.

---

🏆 Final Recommendations

Immediate Action: APPROVE AND MERGE ✅

Why This PR is Exceptional:

• Addresses all 19 UAT bugs with surgical precision
• Maintains backward compatibility where possible
• Follows established patterns and conventions
• Comprehensive test coverage with regression prevention
• Security improvements are thoughtfully designed
• Performance impact is minimal and well-considered
• Documentation and error messages are clear and actionable

Future Considerations:

• Monitor security scanner performance in production
• Consider documenting regex bounds in security scanner
• Update user documentation for CLI behavioral changes

Zero concerns with merging this PR. The implementation quality is outstanding and demonstrates deep understanding of the codebase architecture.

---

[claude]

Claude finished @ajitpratap0's task —— View job

---

Comprehensive PR Review — Post-UAT Bug Fixes v1.8.0

I'm conducting a thorough review of this PR focusing on code quality, potential issues, performance, security, and test coverage.

Todo List:

• Analyze error handling changes (ERR-1)
• Review parser core fixes (CORE-1 to CORE-3)
• Examine dialect support improvements (DIALECT-1 to DIALECT-4)
• Evaluate CLI enhancements (CLI-1 to CLI-8)
• Review security scanner improvements (SEC-1, SEC-2)
• Assess AST changes (CORE-4)
• Check test coverage and quality
• Evaluate performance considerations
• Provide final recommendations

---

✅ Error Handling Changes (ERR-1)

Excellent implementation:

• New ErrCodeUnterminatedBlockComment (E1009) follows the established pattern perfectly
• Clean UnterminatedBlockCommentError() builder with proper 2-character context highlighting for /*
• Meaningful error message: "unterminated block comment (missing */)"
• Helpful hint: "Close the comment with / or check for unmatched /"

The integration with the tokenizer properly replaces generic string errors with structured E1009 errors.

---

[claude]

Claude finished @ajitpratap0's task —— View job

---

Comprehensive PR Review — Post-UAT Bug Fixes v1.8.0 ✅

I've completed a thorough review of this PR focusing on code quality, potential issues, performance, security, and test coverage.

Todo List:

• Analyze error handling changes (ERR-1)
• Review parser core fixes (CORE-1 to CORE-3)
• Examine dialect support improvements (DIALECT-1 to DIALECT-4)
• Evaluate CLI enhancements (CLI-1 to CLI-8)
• Review security scanner improvements (SEC-1, SEC-2)
• Assess AST changes (CORE-4)
• Check test coverage and quality
• Evaluate performance considerations
• Provide final recommendations

---

✅ Overall Assessment: EXCELLENT IMPLEMENTATION

This PR demonstrates exceptional software engineering practices with comprehensive bug fixes, thoughtful design decisions, and thorough testing.

---

✅ Code Quality & Best Practices

Error Handling (ERR-1):

• New ErrCodeUnterminatedBlockComment (E1009) follows established patterns perfectly
• Clean UnterminatedBlockCommentError() builder with proper 2-character context highlighting
• Consistent integration with existing error infrastructure

Parser Core Fixes (CORE-1 to CORE-3):

• CORE-1: Elegant handling of reserved keywords after dots in qualified identifiers
• CORE-2: Smart explicitType flag prevents incorrect "NATURAL INNER" concatenation
• CORE-3: SQL:2003 §7.11 compliant window function parsing with proper precedence

Dialect Support (DIALECT-1 to DIALECT-4):

• SQLite PRAGMA: Complete implementation handling all three forms with clean AST design
• Quoted Reserved Words: Comprehensive coverage for MySQL backticks and SQL Server brackets
• Keyword Integration: Seamless extension of existing isNonReservedKeyword() infrastructure

CLI Enhancements (CLI-1 to CLI-8):

• Token Count Fix: Now reflects actual tokenizer output instead of hardcoded 0
• UX Improvements: SilenceUsage = true, standardized emojis, proper exit codes
• Output Accuracy: Real character/line counts, CTE detection in JSON output

AST Changes (CORE-4):

• Proper deprecation documentation for AlterTableStatement with clear migration guide

---

🛡️ Security Analysis

SEC-1 - Tautology Detection:

• ReDoS Protection: Bounded regex patterns (6 digits, 50 chars) with 10k input limit
• Elegant Solution: Two-step capture + verification approach works around RE2 limitations
• Comprehensive Coverage: Detects numeric, string, and identifier tautologies

SEC-2 - UNION Classification:

• Risk-Based Severity: Properly distinguishes injection attempts (CRITICAL) from legitimate queries (HIGH)
• False Positive Reduction: System table detection prevents legitimate UNIONs from being flagged as CRITICAL
• Precise Matching: Both system table prefixes and exact names covered

---

⚡ Performance Considerations

Excellent Performance Design:

• sync.Once initialization for regex patterns prevents compilation overhead
• Bounded input processing prevents DoS vulnerabilities
• Zero-copy optimizations maintained throughout
• Object pooling patterns preserved

No Performance Regressions Identified:

• Changes are surgical and don't affect hot paths
• New security patterns are optimized for production use
• CLI improvements don't impact core parsing performance

---

🧪 Test Coverage

Comprehensive Testing:

• New Test File: /pkg/sql/parser/core_fixes_test.go (230 lines) with focused regression tests
• Security Verification: /pkg/sql/security/sec_verify_test.go validates both SEC-1 and SEC-2 fixes
• CLI Coverage: All CLI fixes have corresponding test updates
• Type Safety: Proper two-value type assertions throughout tests

Test Quality:

• Clear test naming with bug ID references
• Comprehensive edge case coverage
• Proper resource cleanup with defer ast.ReleaseAST()

---

🚨 Potential Issues

Minor Considerations (Non-blocking):

1. PRAGMA Parser (/pkg/sql/parser/pragma.go:47-50): Token consumption could be more defensive about malformed arguments, but current implementation is reasonable for production.

2. Tautology Regex Bounds: The 6-digit and 50-character limits are well-reasoned for ReDoS protection, though inline documentation of these bounds would help future maintainers.

3. CLI Test Updates: Some test expectations changed (exit codes, output format) - ensure downstream users are aware of these behavioral changes.

---

🏆 Final Recommendations

Immediate Action: APPROVE AND MERGE ✅

Why This PR is Exceptional:

• Addresses all 19 UAT bugs with surgical precision
• Maintains backward compatibility where possible
• Follows established patterns and conventions
• Comprehensive test coverage with regression prevention
• Security improvements are thoughtfully designed
• Performance impact is minimal and well-considered
• Documentation and error messages are clear and actionable

Future Considerations:

• Monitor security scanner performance in production
• Consider documenting regex bounds in security scanner
• Update user documentation for CLI behavioral changes

Zero concerns with merging this PR. The implementation quality is outstanding and demonstrates deep understanding of the codebase architecture.

---