Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: Lint

on:
push:
branches: [master, main]
pull_request:
branches: [master, main]

jobs:
lint:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4

- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: '3.0'
bundler-cache: true

- name: Run RuboCop
run: bundle exec rubocop

- name: Check for files missing newlines
run: bundle exec rake check_newlines
76 changes: 76 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
plugins:
- rubocop-rake
- rubocop-rspec

AllCops:
TargetRubyVersion: 3.0
NewCops: enable
Exclude:
- 'vendor/**/*'
- 'spec/fixtures/**/*'
- 'tmp/**/*'
- 'pkg/**/*'
- 'node_modules/**/*'
- 'specs_e2e/**/*'
- 'e2e/**/*'

# Ensure all files end with a newline
Layout/TrailingEmptyLines:
Enabled: true
EnforcedStyle: final_newline

# Ensure no trailing whitespace
Layout/TrailingWhitespace:
Enabled: true

# Line length - be reasonable but not too strict
Layout/LineLength:
Max: 120
Exclude:
- 'spec/**/*'
- 'lib/generators/**/*'

# Allow longer blocks in specs and rake tasks
Metrics/BlockLength:
Exclude:
- 'spec/**/*'
- '**/*.rake'
- 'Rakefile'
- '*.gemspec'

# Allow longer methods in rake tasks
Metrics/MethodLength:
Exclude:
- '**/*.rake'
- 'Rakefile'

# String literals
Style/StringLiterals:
Enabled: true
EnforcedStyle: single_quotes
ConsistentQuotesInMultiline: true

# Frozen string literal pragma
Style/FrozenStringLiteralComment:
Enabled: true
EnforcedStyle: always
Exclude:
- 'spec/**/*'

# Documentation
Style/Documentation:
Enabled: false

# Allow compact module/class definitions
Style/ClassAndModuleChildren:
Enabled: false

# RSpec specific
RSpec/ExampleLength:
Max: 20

RSpec/MultipleExpectations:
Max: 5

RSpec/NestedGroups:
Max: 4
3 changes: 3 additions & 0 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,7 @@ RSpec::Core::RakeTask.new(:spec) do |t|
t.pattern = 'spec/cypress_on_rails/*_spec.rb'
end

desc 'Run all CI checks (specs, linting, newlines)'
task ci: %i[spec lint check_newlines]

task default: %w[spec build]
50 changes: 50 additions & 0 deletions bin/install-hooks
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#!/usr/bin/env ruby
# frozen_string_literal: true

require 'fileutils'

hooks_dir = File.expand_path('../.git/hooks', __dir__)
pre_commit_hook = File.join(hooks_dir, 'pre-commit')

# Create pre-commit hook content
hook_content = <<~HOOK
#!/bin/sh
# Pre-commit hook to run linters and check for newlines

# 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
Comment on lines +14 to +20
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
-  fi

Committable suggestion skipped: line range outside the PR's diff.


# 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
if [ $? -ne 0 ]; then
echo "❌ RuboCop failed. Fix issues or run 'bundle exec rake lint:fix'"
exit 1
fi
Comment on lines +24 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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 1

Wait, 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 ]; then

Or 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
+  done

Committable 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.

fi

echo "✅ All checks passed!"
HOOK

# Create hooks directory if it doesn't exist
FileUtils.mkdir_p(hooks_dir)

# Write the pre-commit hook
File.write(pre_commit_hook, hook_content)

# Make it executable
File.chmod(0o755, pre_commit_hook)

puts '✅ Pre-commit hook installed successfully!'
puts 'The hook will:'
puts ' - Check that all files end with a newline'
puts ' - Run RuboCop on staged Ruby files'
puts ''
puts 'To skip the hook temporarily, use: git commit --no-verify'
6 changes: 6 additions & 0 deletions cypress-on-rails.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ Gem::Specification.new do |s|
s.add_development_dependency 'factory_bot', '!= 6.4.5'
s.add_development_dependency 'vcr'
s.add_development_dependency 'gem-release'
s.add_development_dependency 'rubocop', '~> 1.81'
s.add_development_dependency 'rubocop-rake', '~> 0.7'
s.add_development_dependency 'rubocop-rspec', '~> 3.7'

s.required_ruby_version = '>= 3.0.0'

s.metadata = {
"bug_tracker_uri" => "https://github.com/shakacode/cypress-on-rails/issues",
"changelog_uri" => "https://github.com/shakacode/cypress-on-rails/blob/master/CHANGELOG.md",
Expand Down
63 changes: 63 additions & 0 deletions rakelib/lint.rake
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# frozen_string_literal: true

desc 'Run RuboCop'
task :rubocop do
sh 'bundle exec rubocop'
end

desc 'Run RuboCop with auto-correct'
task 'rubocop:auto_correct' do
sh 'bundle exec rubocop -A'
end

desc 'Run all linters'
task lint: :rubocop

desc 'Auto-fix all linting issues'
task 'lint:fix' => 'rubocop:auto_correct'

desc 'Ensure all files end with newline'
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)

content = File.read(file)
files_without_newline << file unless content.empty? || content.end_with?("\n")
end

if files_without_newline.any?
puts 'Files missing final newline:'
files_without_newline.each { |f| puts " #{f}" }
exit 1
else
puts '✓ All files end with newline'
end
end

desc 'Fix files missing final newline'
task :fix_newlines do
fixed_files = []

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
Comment on lines +45 to +55
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling and consider TOCTOU risks.

The current implementation has potential issues:

  1. Memory efficiency: Same concern as check_newlines - reading large files into memory
  2. TOCTOU race condition: File could be modified between read (line 50) and write (line 52)
  3. No error handling: File.write could fail partway through, potentially corrupting files
  4. 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
     end

Or 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.


if fixed_files.any?
puts "Fixed #{fixed_files.length} files:"
fixed_files.each { |f| puts " #{f}" }
else
puts '✓ All files already end with newline'
end
end