Add RuboCop linting and newline enforcement #193
Conversation
Set up automated linting and code quality tools: 1. Added RuboCop with extensions: - rubocop-rake for rake task linting - rubocop-rspec for RSpec linting - Created .rubocop.yml with sensible defaults 2. Created rake tasks for linting (rakelib/lint.rake): - rake lint - Run all linters - rake lint:fix - Auto-fix issues - rake check_newlines - Verify files end with newlines - rake fix_newlines - Fix missing newlines 3. Added pre-commit hook installer (bin/install-hooks): - Checks for missing newlines - Runs RuboCop on staged Ruby files - Prevents commits with linting issues 4. Added GitHub Actions workflow (.github/workflows/lint.yml): - Runs on all PRs and pushes to master - Enforces linting in CI - Checks for missing newlines 5. Updated CI workflow: - Added :ci task to run specs, lint, and newline checks - Ensures all checks pass before merge To install pre-commit hook: ./bin/install-hooks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds linting infrastructure: a GitHub Actions workflow for RuboCop and newline checks, RuboCop configuration, Rake tasks for linting and newline hygiene (plus a CI aggregate task), a git pre-commit hook installer to run checks locally, and development dependencies for RuboCop. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant GA as Actions: Lint Job
participant Ruby as Ruby/Rubygems
participant Rake as Rake Tasks
Dev->>GH: Push / Open PR (main/master)
GH-->>GA: Trigger workflow
GA->>GA: Checkout repo
GA->>Ruby: Setup Ruby 3.2 + Bundler cache
GA->>Rake: bundle exec rubocop
Rake-->>GA: RuboCop status
GA->>Rake: bundle exec rake check_newlines
Rake-->>GA: Newline check status
GA-->>GH: Report job status (pass/fail)
note over GA: New/changed: automated lint + newline checks
sequenceDiagram
autonumber
actor Dev as Developer
participant Hook as pre-commit hook
participant Rake as Rake
participant RC as RuboCop
Dev->>Hook: git commit
Hook->>Rake: rake check_newlines
alt Missing final newline
Rake-->>Hook: Non-zero exit
Hook-->>Dev: Abort commit with message
else All files ok
Hook->>Hook: Detect staged *.rb/*.rake
alt Ruby files staged
Hook->>RC: bundle exec rubocop --only staged files
alt Lint failures
RC-->>Hook: Non-zero exit
Hook-->>Dev: Abort commit with message
else Clean
RC-->>Hook: Zero exit
Hook-->>Dev: Commit proceeds
end
else No Ruby files
Hook-->>Dev: Commit proceeds
end
end
note over Hook: New local guardrails before commit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review - PR #193: Add RuboCop linting and newline enforcementSummaryThis PR adds comprehensive linting infrastructure with RuboCop, automated newline enforcement, and CI/CD integration. Overall, this is a well-structured and valuable addition to the project that will improve code quality and consistency. ✅ Strengths1. Comprehensive Linting Setup
2. Developer Experience
3. CI/CD Integration
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cypress-on-rails.gemspec (1)
25-27: Pin RuboCop versions to prevent breaking changes.Development dependencies without version constraints can introduce breaking changes or security vulnerabilities when newer versions are released. Consider pinning to major versions to ensure consistent behavior across environments.
Apply this diff to add version constraints:
- s.add_development_dependency 'rubocop' - s.add_development_dependency 'rubocop-rake' - s.add_development_dependency 'rubocop-rspec' + s.add_development_dependency 'rubocop', '~> 1.0' + s.add_development_dependency 'rubocop-rake', '~> 0.6' + s.add_development_dependency 'rubocop-rspec', '~> 3.0'rakelib/lint.rake (2)
23-29: Consider memory efficiency for large files.Reading entire files into memory (line 28) could cause issues with large files (e.g., lockfiles, generated assets). Consider checking file size before reading or using a buffered approach that only reads the last bytes.
Apply this diff to add a size check:
next unless File.file?(file) + # Skip files larger than 10MB to avoid memory issues + next if File.size(file) > 10 * 1024 * 1024 + content = File.read(file) files_without_newline << file unless content.empty? || content.end_with?("\n")Alternatively, for a more robust solution that handles large files:
# Check only the last byte(s) for newline without loading entire file File.open(file, 'rb') do |f| f.seek(-1, IO::SEEK_END) rescue next files_without_newline << file unless f.read(1) == "\n" end
23-25: Extract duplicate file filtering logic.The file exclusion logic is duplicated between
check_newlinesandfix_newlines. Extract this into a shared helper method to follow DRY principles and ease maintenance.Add this helper at the top of the file (after line 1):
# frozen_string_literal: true def files_to_check Dir.glob('**/*.{rb,rake,yml,yaml,md,gemspec,ru,erb,js,json}').each do |file| next if file.include?('vendor/') || file.include?('node_modules/') || file.include?('.git/') next if file.include?('pkg/') || file.include?('tmp/') || file.include?('coverage/') next unless File.file?(file) yield file end endThen update both tasks:
task :check_newlines do files_without_newline = [] - Dir.glob('**/*.{rb,rake,yml,yaml,md,gemspec,ru,erb,js,json}').each do |file| - next if file.include?('vendor/') || file.include?('node_modules/') || file.include?('.git/') - next if file.include?('pkg/') || file.include?('tmp/') || file.include?('coverage/') - next unless File.file?(file) - + files_to_check do |file| content = File.read(file) files_without_newline << file unless content.empty? || content.end_with?("\n") endAlso applies to: 45-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/lint.yml(1 hunks).rubocop.yml(1 hunks)Rakefile(1 hunks)bin/install-hooks(1 hunks)cypress-on-rails.gemspec(1 hunks)rakelib/lint.rake(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: claude-review
- GitHub Check: rails_6_1
- GitHub Check: rails_7_2
- GitHub Check: rails_8
🔇 Additional comments (3)
rakelib/lint.rake (1)
3-11: LGTM!The RuboCop task definitions are correct and use safe, hardcoded commands.
Rakefile (1)
8-9: LGTM!The CI task provides a convenient entry point for running all checks. The task dependencies are correctly defined and the tasks exist in the appropriate files.
.rubocop.yml (1)
5-7: Verify Ruby 2.6 support or update to a supported version
Ruby 2.6 reached EOL in March 2022 and no longer receives security updates. Confirm whether the project must still support 2.6; if not, bumpTargetRubyVersionin.rubocop.ymlto 2.7 or 3.x.
| # Check for files missing newlines | ||
| echo "Checking for files missing newlines..." | ||
| bundle exec rake check_newlines | ||
| if [ $? -ne 0 ]; then | ||
| echo "❌ Some files are missing final newlines. Run 'bundle exec rake fix_newlines' to fix." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Newline check runs on all files, not just staged.
The check_newlines task (line 16) checks all files in the repository, not just staged files. This is inconsistent with the RuboCop check which only lints staged files. This could:
- Fail commits due to existing issues in unstaged files
- Prevent developers from making incremental fixes
- Slow down pre-commit hooks unnecessarily
Consider checking only staged files or documenting this intentional behavior.
If you want to check only staged files, apply this diff:
# Check for files missing newlines
echo "Checking for files missing newlines..."
- bundle exec rake check_newlines
+ staged_files=$(git diff --cached --name-only --diff-filter=ACM)
+ if [ -n "$staged_files" ]; then
+ echo "$staged_files" | while IFS= read -r file; do
+ if [ -f "$file" ] && ! [ -s "$file" ]; then
+ continue
+ fi
+ if [ -f "$file" ] && [ -n "$(tail -c 1 "$file")" ]; then
+ echo "❌ File missing final newline: $file"
+ exit 1
+ fi
+ done
+ fi
- if [ $? -ne 0 ]; then
- echo "❌ Some files are missing final newlines. Run 'bundle exec rake fix_newlines' to fix."
- exit 1
- fiCommittable suggestion skipped: line range outside the PR's diff.
| files=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\.(rb|rake)$') | ||
| if [ -n "$files" ]; then | ||
| bundle exec rubocop $files | ||
| if [ $? -ne 0 ]; then | ||
| echo "❌ RuboCop failed. Fix issues or run 'bundle exec rake lint:fix'" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Critical: Fix command injection vulnerability in git hook.
Line 26 has an unquoted variable $files which creates a command injection vulnerability. If a malicious actor creates a file with special characters in the name (spaces, semicolons, backticks, etc.), they could execute arbitrary commands during commit.
Apply this diff to properly quote the variable:
# Run RuboCop on staged Ruby files
echo "Running RuboCop on staged files..."
files=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\.(rb|rake)$')
if [ -n "$files" ]; then
- bundle exec rubocop $files
+ bundle exec rubocop $files
if [ $? -ne 0 ]; then
echo "❌ RuboCop failed. Fix issues or run 'bundle exec rake lint:fix'"
exit 1Wait, I need to check the actual quoting. Let me re-examine line 26 in the provided code.
Actually, looking at line 26: bundle exec rubocop $files, the variable IS unquoted. This needs to be fixed:
- bundle exec rubocop $files
+ bundle exec rubocop "$files"However, this still has an issue - if there are multiple files, they'll be treated as a single argument. The proper fix is:
files=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\.(rb|rake)$')
if [ -n "$files" ]; then
- bundle exec rubocop $files
+ echo "$files" | xargs bundle exec rubocop
if [ $? -ne 0 ]; thenOr use a while loop for better handling:
- files=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\.(rb|rake)$')
- if [ -n "$files" ]; then
- bundle exec rubocop $files
- if [ $? -ne 0 ]; then
- echo "❌ RuboCop failed. Fix issues or run 'bundle exec rake lint:fix'"
- exit 1
- fi
- fi
+ git diff --cached --name-only --diff-filter=ACM | grep -E '\\.(rb|rake)$' | \
+ while IFS= read -r file; do
+ bundle exec rubocop "$file"
+ if [ $? -ne 0 ]; then
+ echo "❌ RuboCop failed. Fix issues or run 'bundle exec rake lint:fix'"
+ exit 1
+ fi
+ doneCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bin/install-hooks around lines 24 to 30, the script passes an unquoted $files
variable to rubocop which creates a command-injection and argument-splitting
issue; update the hook to safely handle filenames by iterating over the list of
staged files (or by using an array/quoted expansion) and invoking rubocop with
each filename as a separate, properly quoted argument, ensuring the variable is
quoted and/or looped so filenames with spaces or special characters cannot be
interpreted as shell metacharacters.
| Dir.glob('**/*.{rb,rake,yml,yaml,md,gemspec,ru,erb,js,json}').each do |file| | ||
| next if file.include?('vendor/') || file.include?('node_modules/') || file.include?('.git/') | ||
| next if file.include?('pkg/') || file.include?('tmp/') || file.include?('coverage/') | ||
| next unless File.file?(file) | ||
|
|
||
| content = File.read(file) | ||
| unless content.empty? || content.end_with?("\n") | ||
| File.write(file, content + "\n") | ||
| fixed_files << file | ||
| end | ||
| end |
There was a problem hiding this comment.
Add error handling and consider TOCTOU risks.
The current implementation has potential issues:
- Memory efficiency: Same concern as
check_newlines- reading large files into memory - TOCTOU race condition: File could be modified between read (line 50) and write (line 52)
- No error handling:
File.writecould fail partway through, potentially corrupting files - Atomicity: Should write to temp file and rename for atomic updates
Apply this diff to improve safety:
content = File.read(file)
unless content.empty? || content.end_with?("\n")
- File.write(file, content + "\n")
- fixed_files << file
+ begin
+ # Write to temp file first for atomicity
+ temp_file = "#{file}.tmp"
+ File.write(temp_file, content + "\n")
+ File.rename(temp_file, file)
+ fixed_files << file
+ rescue => e
+ warn "Failed to fix #{file}: #{e.message}"
+ File.delete(temp_file) if File.exist?(temp_file)
+ end
endOr consider using File.open with append mode which is safer for just adding a newline:
unless content.empty? || content.end_with?("\n")
begin
File.open(file, 'a') { |f| f.write("\n") }
fixed_files << file
rescue => e
warn "Failed to fix #{file}: #{e.message}"
end
end🤖 Prompt for AI Agents
In rakelib/lint.rake around lines 45–55, avoid reading entire file into memory
and fix TOCTOU/atomicity and error handling: instead of File.read + File.write,
open the file and check only the final byte (seek to the end or use File.size?)
to determine if a trailing newline is missing, then either append a newline with
File.open(file, 'a') inside a begin/rescue to handle and log errors, or write to
a temporary file and perform an atomic rename (write to tmp in same directory,
fsync, then File.rename) to avoid corruption; ensure you rescue exceptions from
IO operations, warn or log failures including the filename and error message,
and only add the file to fixed_files on successful write/rename.
- Set minimum Ruby version to 3.0 across all configurations - Updated .rubocop.yml TargetRubyVersion from 2.6 to 3.0 - Updated GitHub Actions workflow to use Ruby 3.0 - Added required_ruby_version >= 3.0.0 to gemspec - Pinned RuboCop versions for consistency: - rubocop ~> 1.81 - rubocop-rake ~> 0.7 - rubocop-rspec ~> 3.7 - Updated to use new plugin syntax instead of require This ensures consistency between CI, local development, and RuboCop configuration. Ruby 3.0 is a reasonable minimum for modern projects. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
LGTM |
2 participants
Summary
Adds comprehensive linting and code quality enforcement using RuboCop.
Changes
1. RuboCop Configuration
.rubocop.ymlwith sensible defaults2. Rake Tasks (
rakelib/lint.rake)rake lint- Run all lintersrake lint:fix- Auto-fix issuesrake check_newlines- Verify files end with newlinesrake fix_newlines- Fix missing newlinesrake ci- Run all checks (specs, lint, newlines)3. Pre-commit Hook (
bin/install-hooks)4. GitHub Actions (
.github/workflows/lint.yml)Benefits
rake lint:fixandrake fix_newlinesInstallation
To install pre-commit hook locally:
This is a separate PR from the release script changes for independent review.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores