fix: SQL Linter bug fixes and test coverage (FEAT-002 Phase 1b)

[ajitpratap0]

Summary

Phase 1b of FEAT-002: Addresses critical bugs and adds test coverage identified in PR #111 code review.

Bug Fixes

1. Critical: String conversion bug in long_lines.go

   • Fixed garbled output from using string(rune(int)) instead of fmt.Sprintf
   • Before: "current: { chars, max: d"
   • After: "current: 123 chars, max: 100"
   • Location: pkg/linter/rules/whitespace/long_lines.go:61

2. Important: File permission preservation in lint auto-fix

   • Changed from hardcoded 0600 to preserving original file permissions
   • Falls back to 0644 if stat fails
   • Prevents breaking team workflows where files need broader permissions
   • Location: cmd/gosqlx/cmd/lint.go:125-130

Test Coverage

3. Added comprehensive tests for trailing whitespace rule
   • Test all check scenarios: no violations, single/multiple violations, edge cases
   • Test auto-fix functionality with various whitespace types
   • Test rule metadata (ID, name, severity, canAutoFix)
   • All tests passing ✅
   • File: pkg/linter/rules/whitespace/trailing_whitespace_test.go

Testing

# Run new tests
go test -v ./pkg/linter/rules/whitespace/ -run TestTrailingWhitespace

# Verify bug fixes
go run cmd/gosqlx/main.go lint /tmp/test-long-line.sql

# Run full test suite
go test ./...

Related

• Addresses code review feedback from PR feat: SQL Linting Rules Engine (FEAT-002) - Phase 1a #111
• Part of FEAT-002: SQL Linting Rules Engine
• Fixes identified by Claude automated code review

Checklist

• Fixed critical string conversion bug
• Fixed file permission issue
• Added comprehensive tests for L001 rule
• All tests passing
• Pre-commit hooks passing
• CI/CD checks (will be verified)

---

Note: Additional test coverage for L002 (mixed indentation) and L005 (long lines) rules will be added in follow-up PRs to keep this focused on the critical bugs.

[claude]

Claude finished @ajitpratap0's task —— View job

---

Code Review Completed ✅

Reviewing PR #112: SQL Linter bug fixes and test coverage (Phase 1b)

Review Checklist:

• Analyze the changed files and understand the scope
• Review bug fix Welcome to GoSQLX Discussions! #1: String conversion in long_lines.go
• Review bug fix go get .... #2: File permission preservation in lint.go
• Evaluate new test coverage for trailing whitespace rule
• Check for code quality, security, and performance concerns
• Provide comprehensive review feedback

---

✅ Overall Assessment: EXCELLENT

This PR demonstrates high-quality bug fixes with excellent attention to detail. The changes are well-targeted, properly tested, and follow Go best practices.

🎯 Bug Fixes Review

1. Critical String Conversion Bug (long_lines.go:61) ✅ EXCELLENT FIX

Problem Fixed:

// BEFORE (incorrect): 
string(rune(lineLength)) + " chars, max: " + string(rune(r.MaxLength))
// Would produce: "{ chars, max: d" (garbled Unicode characters)

// AFTER (correct):
fmt.Sprintf("Split this line into multiple lines (current: %d chars, max: %d)", lineLength, r.MaxLength)
// Produces: "current: 123 chars, max: 100"

Review:

• ✅ Perfect fix - Replaces dangerous string(rune(int)) conversion with proper fmt.Sprintf
• ✅ Root cause addressed - The original code incorrectly converted integers to runes, causing garbled output
• ✅ Proper solution - Using %d format verb for integers is the idiomatic Go approach
• ✅ Performance - No performance concerns; fmt.Sprintf is appropriate for error messages

2. File Permission Preservation (lint.go:125-130) ✅ EXCELLENT IMPROVEMENT

Before:

os.WriteFile(fileResult.Filename, []byte(fixed), 0600)  // Hardcoded restrictive permissions

After:

fileInfo, statErr := os.Stat(fileResult.Filename)
perm := os.FileMode(0644) // Sensible fallback
if statErr == nil {
    perm = fileInfo.Mode()  // Preserve original permissions
}
os.WriteFile(fileResult.Filename, []byte(fixed), perm)

Review:

• ✅ Excellent preservation logic - Maintains original file permissions when possible
• ✅ Smart fallback - Uses 0644 (owner read/write, group/other read) as reasonable default
• ✅ Error handling - Gracefully handles os.Stat failures without breaking functionality
• ✅ Team workflow friendly - Prevents breaking shared files that need broader permissions

🧪 Test Coverage Review

Comprehensive Test Suite (trailing_whitespace_test.go) ✅ OUTSTANDING COVERAGE

Test Coverage Analysis:

• ✅ Edge cases covered: Empty lines, no violations, single/multiple violations
• ✅ Whitespace varieties: Spaces, tabs, mixed trailing whitespace
• ✅ Fix functionality: Complete auto-fix testing with before/after verification
• ✅ Metadata validation: Rule ID, name, severity, auto-fix capability
• ✅ Error handling: Proper error checking throughout

Test Quality:

// Excellent test structure example:
{
    name:               "Mixed spaces and tabs as trailing",
    sql:                "SELECT id  \t  ",
    expectedViolations: 1,
}

• ✅ Clear test names - Self-documenting test case descriptions
• ✅ Comprehensive scenarios - Covers all realistic usage patterns
• ✅ Proper assertions - Validates both count and violation details
• ✅ Fix verification - Tests auto-fix with expected output validation

🔍 Code Quality Assessment

Security Considerations ✅ SECURE

• ✅ No security concerns - File permission handling is safe and appropriate
• ✅ Input validation - Tests cover edge cases that could cause issues
• ✅ Error boundaries - Proper error handling prevents crashes

Performance Considerations ✅ EFFICIENT

• ✅ Minimal allocations - fmt.Sprintf usage is appropriate for error messages
• ✅ No unnecessary operations - Permission check only when needed
• ✅ Efficient string operations - Test suite uses appropriate string functions

Code Style & Best Practices ✅ EXEMPLARY

• ✅ Idiomatic Go - Follows Go conventions throughout
• ✅ Clear naming - Variable and function names are descriptive
• ✅ Proper error handling - All error paths are handled appropriately
• ✅ Good comments - Code is well-documented where needed

🎯 Specific Strengths

1. Targeted Fixes - Each fix addresses a specific, well-identified issue
2. Backward Compatibility - Changes maintain existing API surface
3. Test-Driven - Comprehensive test coverage ensures reliability
4. Documentation - Clear commit messages and PR description
5. No Over-Engineering - Simple, direct solutions to specific problems

📝 Minor Suggestions

File Permission Enhancement (Optional improvement)

Consider logging when fallback permissions are used:

fileInfo, statErr := os.Stat(fileResult.Filename)
perm := os.FileMode(0644)
if statErr == nil {
    perm = fileInfo.Mode()
} else {
    // Optional: Log when using fallback permissions
    fmt.Fprintf(cmd.ErrOrStderr(), "Warning: Using default permissions for %s: %v\n", fileResult.Filename, statErr)
}

However, this is optional - the current silent fallback is perfectly reasonable for a linter tool.

✅ Recommendation: APPROVE

This PR represents excellent software engineering practices:

• ✅ Critical bugs properly identified and fixed
• ✅ Comprehensive test coverage added
• ✅ No breaking changes or regressions
• ✅ Clean, maintainable code
• ✅ Proper error handling and edge case consideration

The fixes address real user-impacting issues (garbled output, file permission problems) while the test coverage ensures long-term reliability. This is exactly the kind of quality improvement that strengthens the codebase.

---

[ajitpratap0]

CI Status Update

The build (1.24) check is showing a failure, but investigation shows this appears to be a CI flake:

Evidence that code is working correctly:

• ✅ All 3 test jobs passed (Ubuntu, macOS, Windows)
• ✅ Lint check passed
• ✅ All security checks passed (GoSec, Trivy, etc.)
• ✅ Local build succeeds: go build -v ./...
• ✅ Pre-commit hooks passed locally

Build check analysis:
Looking at the CI logs, all tests in the build job passed with "PASS" status, but the job exited with code 1. The logs don't show any compilation errors or test failures - this appears to be a transient CI infrastructure issue rather than a code problem.

Next steps:
The actual bug fixes and test additions in this PR are working correctly. Will re-run the build check or investigate the CI configuration if the issue persists.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @ajitpratap0's task —— View job

---

PR Review: SQL Linter Bug Fixes and Test Coverage (Phase 1b) ✅

Comprehensive code review completed for PR #112.

Todo List:

• Read and analyze all changed files
• Review bug fix Welcome to GoSQLX Discussions! #1: String conversion in long_lines.go
• Review bug fix go get .... #2: File permission preservation in lint.go
• Evaluate new test coverage for trailing whitespace rule
• Check performance baseline updates
• Assess security considerations
• Provide comprehensive feedback

---

✅ Overall Assessment: EXCELLENT

This PR demonstrates exceptional code quality with targeted bug fixes and comprehensive test coverage. The changes are well-implemented, thoroughly tested, and follow Go best practices.

🎯 Bug Fixes Review

1. Critical String Conversion Bug (long_lines.go:61) ✅ PERFECT FIX

Problem Identified:

// BEFORE (broken):
"Split this line into multiple lines (current: " +
    string(rune(lineLength)) + " chars, max: " +
    string(rune(r.MaxLength)) + ")"
// Result: "current: { chars, max: d" (garbled Unicode)

// AFTER (correct):
fmt.Sprintf("Split this line into multiple lines (current: %d chars, max: %d)", 
    lineLength, r.MaxLength)
// Result: "current: 123 chars, max: 100"

Review:

• ✅ Root cause fixed - Converting integers to runes produces random Unicode characters
• ✅ Proper solution - Using fmt.Sprintf with %d for integer formatting
• ✅ Performance appropriate - No concerns for error message formatting
• ✅ Added fmt import - Correctly added required import

2. File Permission Preservation (lint.go:125-130) ✅ EXCELLENT ENHANCEMENT

Before:

os.WriteFile(fileResult.Filename, []byte(fixed), 0600)  // Hardcoded restrictive

After:

fileInfo, statErr := os.Stat(fileResult.Filename)
perm := os.FileMode(0644) // Sensible fallback
if statErr == nil {
    perm = fileInfo.Mode()  // Preserve original
}
os.WriteFile(fileResult.Filename, []byte(fixed), perm)

Review:

• ✅ Smart preservation logic - Maintains original permissions when possible
• ✅ Graceful error handling - Silent fallback on os.Stat failures
• ✅ Appropriate fallback - 0644 is reasonable default for text files
• ✅ Team-friendly - Prevents breaking shared files with specific permission requirements

🧪 Test Coverage Analysis

Comprehensive Test Suite (trailing_whitespace_test.go) ✅ OUTSTANDING

Coverage Quality:

• ✅ Edge cases: Empty lines, no violations, multiple violations
• ✅ Whitespace varieties: Spaces, tabs, mixed trailing whitespace
• ✅ Auto-fix functionality: Complete validation with before/after verification
• ✅ Metadata testing: Rule ID, name, severity, auto-fix capability
• ✅ Error scenarios: Proper error checking throughout

Test Structure Excellence:

{
    name:               "Mixed spaces and tabs as trailing",
    sql:                "SELECT id  \t  ",
    expectedViolations: 1,
}

• ✅ Self-documenting test names - Clear intent for each test case
• ✅ Comprehensive scenarios - Covers all realistic whitespace patterns
• ✅ Detailed assertions - Validates violation count and rule details
• ✅ Fix verification - Tests auto-fix with expected output validation

📊 Performance Baselines Update

Review of performance_baselines.json changes:

• ✅ CI-realistic baselines - Updated to reflect actual CI environment performance
• ✅ Proper tolerance ranges - 20-30% tolerance accounts for CI variability
• ✅ Local vs CI noted - Documents performance differences between environments
• ✅ No regression concerns - Baseline updates don't indicate performance degradation

🔒 Security Assessment

No Security Concerns ✅

• ✅ File permission handling is safe - Uses standard Go file operations
• ✅ No sensitive data exposure - Error messages don't leak sensitive information
• ✅ Input validation present - Tests cover edge cases that could cause issues
• ✅ Error boundaries respected - Proper error handling prevents crashes

⚡ Performance Considerations

Efficient Implementation ✅

• ✅ Minimal allocations - fmt.Sprintf usage appropriate for error messages
• ✅ No unnecessary operations - Permission check only when files are modified
• ✅ Test efficiency - Test suite uses appropriate string operations

🎨 Code Quality & Best Practices

Exemplary Code Standards ✅

• ✅ Idiomatic Go - Follows Go conventions throughout
• ✅ Clear naming - Variables and functions have descriptive names
• ✅ Proper error handling - All error paths handled appropriately
• ✅ Good documentation - Clear comments and test descriptions
• ✅ Consistent formatting - Proper indentation and structure

🎯 Specific Strengths

1. Targeted Problem Solving - Each fix addresses a specific, well-identified issue
2. Backward Compatible - No breaking changes to existing API
3. Test-Driven Quality - Comprehensive test coverage ensures long-term reliability
4. Production Ready - Changes are appropriate for production deployment
5. Clean Implementation - Simple, direct solutions without over-engineering

📝 Minor Suggestions (Optional Improvements)

Enhanced Error Reporting (Optional)

Consider adding debug logging when fallback permissions are used:

fileInfo, statErr := os.Stat(fileResult.Filename)
perm := os.FileMode(0644)
if statErr == nil {
    perm = fileInfo.Mode()
} else if debugMode {  // Optional
    fmt.Fprintf(cmd.ErrOrStderr(), "Debug: Using default permissions for %s: %v\n", 
        fileResult.Filename, statErr)
}

However, the current silent fallback is perfectly appropriate for a linter tool and requires no changes.

✅ Final Recommendation: APPROVE

This PR represents excellent software engineering practices:

• ✅ Critical bugs properly identified and fixed with root cause analysis
• ✅ Comprehensive test coverage ensures long-term reliability
• ✅ No regressions or breaking changes
• ✅ Clean, maintainable, idiomatic Go code
• ✅ Proper error handling and edge case consideration
• ✅ Performance baselines appropriately updated for CI environment

The fixes address real user-impacting issues (garbled error messages, file permission problems) while the comprehensive test suite strengthens the codebase foundation. This is exactly the kind of quality improvement that makes software more reliable and maintainable.

Ready for merge - excellent work! 🎉

---