feat: add Lefthook for Git hooks management

[justin808]

Summary

• Created bin/setup-git-hooks script that installs a pre-commit hook
• Pre-commit hook runs RuboCop on staged Ruby files only (for fast feedback)
• Pre-commit hook runs all RSpec tests before allowing commits
• Added documentation in README explaining how to set up and use the hooks
• Hook provides colored terminal output for better visibility
• Hook can be bypassed with --no-verify flag when needed

Inspiration

This implementation is inspired by shakacode/shakapacker's approach to preventing simple failures from being committed. The hook helps catch issues early in the development cycle before they reach CI.

Test plan

• Created the setup script and verified it's executable
• Ran the setup script to install the hook
• Verified all RSpec tests pass (276 examples, 0 failures, 100% coverage)
• Tested that the pre-commit hook runs successfully
• Confirmed hook provides clear colored output
• Verified hook can be bypassed with --no-verify

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Integrated Git hook workflow to run staged-file checks (trailing-newline validation and Ruby linting) on commit and full lint/test runs on push.
• Chores
  • Added a development-only dependency to support hook tooling.
  • Project bootstrap now installs the Git hooks automatically.
• Documentation
  • Added detailed Git Hooks guidance (installation, behavior, bypassing, and CI notes).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds Lefthook integration: adds lefthook gem, creates lefthook.yml, adds two executable hook scripts (bin/lefthook/rubocop-lint, bin/lefthook/check-trailing-newlines), updates bin/setup to install hooks, and adds duplicated "Git Hooks" documentation to README.md.

Changes

Cohort / File(s)	Summary
Dependency Gemfile	Added lefthook gem with require: false.
Configuration lefthook.yml	New Lefthook configuration: fast, parallel pre-commit (staged RuboCop + trailing-newlines) and pre-push (full rubocop + rspec) hooks.
Hook scripts bin/lefthook/rubocop-lint, bin/lefthook/check-trailing-newlines	New executable Bash scripts: rubocop-lint runs RuboCop on staged files with failure banners and hints; check-trailing-newlines validates staged files end with a newline and reports failures.
Setup bin/setup	Added bundle exec lefthook install step after bundle install to register Git hooks.
Documentation README.md	Added comprehensive "Git Hooks" section (appears in two locations) describing Lefthook integration, installation/update of hooks, hook behavior, bypass methods, and CI note.

Sequence Diagram(s)

sequenceDiagram
    actor Dev as Developer
    participant Git as Git
    participant LH as Lefthook
    participant Scripts as Hook Scripts
    participant Tools as RuboCop/Tests

    Dev->>Git: git commit
    Git->>LH: invoke pre-commit
    LH->>Scripts: run rubocop-lint (staged_files)
    Scripts->>Tools: bundle exec rubocop [staged_files]
    alt rubocop fails
        Tools-->>Scripts: non-zero
        Scripts-->>Git: reject commit (exit 1)
    else rubocop passes
        Scripts-->>LH: success
        LH->>Scripts: run check-trailing-newlines (staged_files)
        Scripts->>Scripts: validate trailing newlines
        alt newline check fails
            Scripts-->>Git: reject commit (exit 1)
        else
            Scripts-->>Git: allow commit
        end
    end

    Dev->>Git: git push
    Git->>LH: invoke pre-push
    LH->>Scripts: run full-rubocop & full-rspec (parallel)
    Scripts->>Tools: bundle exec rubocop / bundle exec rspec
    alt any fails
        Tools-->>Git: reject push
    else
        Git-->>Dev: push proceeds
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Review shell script safety (quoting, handling file lists, set -euo pipefail).
• Verify lefthook.yml placeholders ({staged_files}) and parallel/fast settings.
• Confirm bin/setup lefthook install is idempotent.
• Check README duplication for consistency.

Possibly related issues

• feat: add Lefthook for Git hooks management cypress-playwright-on-rails#209 — Adds lefthook gem, config, scripts and docs matching these changes.
• feat: add Lefthook for Git hooks management shakapacker#825 — Implements Lefthook integration and helper scripts similar to this PR.
• feat: add Lefthook for Git hooks management react_on_rails#1960 — Introduces lefthook files and setup steps consistent with this PR.

Poem

🐇 I hopped through hooks and scripts today,

Trailing newlines found their way,
RuboCop whispered: "tidy please",
Lefthook guards commits with ease,
A neat repo — rabbit's hooray.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: add Lefthook for Git hooks management' clearly and concisely summarizes the main change: introducing Lefthook for managing Git hooks. It accurately reflects the core additions across multiple files including the Lefthook configuration, setup integration, and hook scripts.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/add-precommit-hook

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d036e07 and 30fc454.

📒 Files selected for processing (1)

• bin/lefthook/check-trailing-newlines (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• bin/lefthook/check-trailing-newlines

⏰ 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). (15)

• GitHub Check: test (windows-latest, 2.6)
• GitHub Check: test (windows-latest, 3.1)
• GitHub Check: test (windows-latest, 2.7)
• GitHub Check: test (windows-latest, 3.0)
• GitHub Check: test (macos-latest, head)
• GitHub Check: test (ubuntu-latest, head)
• GitHub Check: test (macos-latest, 3.0)
• GitHub Check: test (macos-latest, 2.6)
• GitHub Check: test (macos-latest, 2.7)
• GitHub Check: test (ubuntu-latest, 3.1)
• GitHub Check: test (macos-latest, 3.1)
• GitHub Check: test (ubuntu-latest, 2.6)
• GitHub Check: test (ubuntu-latest, 3.0)
• GitHub Check: test (ubuntu-latest, 2.7)
• GitHub Check: claude-review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #32: Add pre-commit hook

Summary

This PR adds a Git pre-commit hook system to prevent simple test and lint failures from being committed. Overall, this is a well-implemented feature that aligns with the project's quality standards. The implementation is clean, well-documented, and provides a good developer experience.

---

Strengths

Code Quality ✅

• Follows project conventions: Uses double quotes (per .rubocop.yml line 114-115), proper indentation, and Ruby idioms
• Clean structure: The setup script is well-organized with clear sections and appropriate use of FileUtils
• Good DX: Colored output and clear error messages in the hook provide excellent developer feedback
• Proper shebang: Uses #!/usr/bin/env ruby and #!/bin/bash appropriately

Documentation ✅

• Clear README section explaining setup and usage
• Inline comments in both scripts
• Mentions the --no-verify escape hatch (important for edge cases)

Security ✅

• No command injection risks: Uses proper Ruby file operations instead of shell commands
• Appropriate permissions: Sets 0o755 on the hook file (executable but not world-writable)
• Safe git command: Uses git rev-parse --git-common-dir to find the correct hooks directory

---

Issues & Recommendations

1. Critical: RuboCop Configuration Conflict ⚠️

The .rubocop.yml explicitly excludes bin/**/* (lines 20, 25), but the pre-commit hook runs RuboCop on staged Ruby files. This creates an inconsistency:

File: bin/setup-git-hooks:40

if ! bundle exec rubocop $STAGED_RUBY_FILES; then

Problem: If a developer stages files in bin/, RuboCop will lint them during pre-commit, but not during CI (which respects the exclusions).

Recommendation: The hook should respect RuboCop's configured exclusions. Consider one of these approaches:

Option A (Recommended): Let RuboCop handle exclusions automatically:

# RuboCop will automatically respect .rubocop.yml exclusions
if ! bundle exec rubocop $STAGED_RUBY_FILES; then

This already works correctly - RuboCop respects its config even when given specific files.

Option B: Document this behavior in the README to clarify that bin/ files won't be checked.

2. Performance: Running Full Test Suite on Every Commit 🐌

File: bin/setup-git-hooks:53

if ! bundle exec rspec; then

Issue: Running the entire test suite on every commit can be slow, especially as the project grows. The current test suite has 276 examples, which may take significant time.

Current state: Per the CI workflow (.github/workflows/checks.yml:53), tests run across multiple Ruby versions and OSes, but locally running all tests every time may slow down commits.

Recommendations:

1. Consider making full test runs optional: Add an environment variable to allow developers to skip tests when iterating rapidly:

   # Allow SKIP_TESTS=1 for rapid iteration
   if [ "$SKIP_TESTS" != "1" ]; then
     echo -e "${YELLOW}Running RSpec tests...${NC}"
     if ! bundle exec rspec; then
       echo -e "${RED}Tests failed! Please fix the tests before committing.${NC}"
       exit 1
     fi
   fi

2. Alternative: Run only tests related to changed files (though this requires more complex implementation)

3. Alternative: Make full test runs opt-in via git config:

   # Check if user wants full test runs (default: yes)
   RUN_FULL_TESTS=$(git config --bool hooks.runFullTests || echo "true")

3. Minor: Frozen String Literal Comment 📝

File: bin/setup-git-hooks:2

# frozen_string_literal: true

Observation: While the script includes this comment, .rubocop.yml line 122-123 explicitly disables the FrozenStringLiteralComment cop. The existing bin/console has it, but bin/setup (bash script) doesn't need it.

Recommendation: This is not an issue - it's actually good practice. The project disabled the cop to avoid forcing it everywhere, but using it where it makes sense is fine. Keep as-is.

4. Enhancement: Error Handling for Missing Dependencies 🛠️

File: bin/setup-git-hooks:40, 53

Issue: If bundle is not installed or dependencies aren't installed, the hook will fail with unclear errors.

Recommendation: Add a check in the generated hook:

# Pre-commit hook to run linting and tests on staged files

set -e

# Check if bundle is available
if ! command -v bundle &> /dev/null; then
  echo -e "${RED}Error: bundle is not installed. Run 'gem install bundler' first.${NC}"
  exit 1
fi

# Colors for output...

5. Enhancement: Hook Update Mechanism 🔄

Current behavior: If the hook logic needs to be updated in the future, developers must manually re-run bin/setup-git-hooks.

Recommendation: Consider adding a version check or hash to detect stale hooks:

HOOK_VERSION = "1.0.0"

# Add to hook content:
# Hook version: 1.0.0
# To update, run: bin/setup-git-hooks

Or mention in the README that developers should re-run the setup script after pulling updates.

6. Documentation: Missing from bin/setup 📚

File: bin/setup

Observation: The bin/setup script doesn't call bin/setup-git-hooks automatically. Developers might miss this step.

Recommendation: Either:

1. Add a note in bin/setup's output suggesting developers run it
2. Call it automatically from bin/setup
3. Document in README that it's a separate opt-in step (current approach is fine if intentional)

---

Test Coverage 📊

Issue: No tests for the setup script itself.

While the pre-commit hook is a development tool, consider adding a spec that:

• Verifies the setup script creates the hook file
• Checks that the hook has correct permissions
• Validates the hook content is generated correctly

Example:

# spec/bin/setup_git_hooks_spec.rb
RSpec.describe "bin/setup-git-hooks" do
  it "creates executable pre-commit hook" do
    # Test hook creation
  end
end

This may be overkill for a simple script, but would ensure the hook setup doesn't break in future refactorings.

---

Performance Considerations ⚡

1. RuboCop on staged files only: ✅ Good! This is the right approach
2. Full RSpec suite: ⚠️ See issue Prepare Changelog like Shakapacker #2 above
3. Bash string operations: ✅ Efficient use of git diff --cached --name-only

---

Security Considerations 🔒

1. File permissions: ✅ Correctly set to 0o755
2. No shell injection: ✅ Uses Ruby file operations
3. No credential risks: ✅ No sensitive data handling
4. Git directory detection: ✅ Uses proper git commands

Overall security: ✅ No concerns

---

Verdict

Approval Status: ✅ Approved with minor suggestions

This is a solid implementation that adds value to the development workflow. The suggestions above are mostly enhancements rather than blocking issues. The one item to consider is the performance impact of running the full test suite on every commit, but this can be addressed in a follow-up if it becomes a problem in practice.

Action Items (optional, non-blocking):

1. Consider adding an environment variable to skip tests for rapid iteration
2. Add error handling for missing bundle
3. Consider testing the setup script itself
4. Document hook update process

Great work on following the project conventions and providing clear documentation!

---

Review generated with Claude Code

[claude]

Code Review for PR #32

Overview

This PR adds a pre-commit hook setup script to catch lint and test failures before commits reach CI. The implementation is well-intentioned and follows patterns from shakacode/shakapacker, but there are several significant issues that need to be addressed.

---

Critical Issues

1. Performance Concern: Running Full Test Suite on Every Commit

The pre-commit hook runs bundle exec rspec for the entire test suite on every commit. According to the PR description, this includes 276 examples. This will significantly slow down the commit workflow.

Impact:

• Developers will experience delays on every commit
• This may encourage using --no-verify to bypass the hook, defeating its purpose
• For quick fixes or documentation changes, this creates unnecessary friction

Recommendation:
Consider one of these alternatives:

• Only run tests related to changed files (if feasible with RSpec)
• Add a flag/environment variable to allow quick commits and rely on CI for full test runs
• Document that this hook is optional and can be customized per developer preference
• Consider moving the full test suite run to a pre-push hook instead

---

2. Unused Variable: project_root

In bin/setup-git-hooks:12, the project_root variable is defined but never used:

project_root = File.expand_path("..", __dir__)

Recommendation: Remove this line to clean up the code.

---

3. Shell Variable Quoting Issue

In bin/setup-git-hooks:41, the bash variable $STAGED_RUBY_FILES is unquoted. If file paths contain spaces, the command will fail.

Recommendation: Quote the variable properly to handle filenames with spaces.

---

4. Missing Newline at End of File

According to CLAUDE.md, ALL files must end with a newline character. The file bin/setup-git-hooks ends at line 72 with no trailing newline.

Impact: This will cause CI to fail, violating one of the repository's critical requirements.

Recommendation: Ensure the file ends with a newline character.

---

5. Incomplete RuboCop Check

The hook only runs RuboCop on staged files, but according to CLAUDE.md:10, the requirement is to run bundle exec rubocop and fix ALL violations. The current implementation doesn't check if changes to one file might introduce violations elsewhere.

Recommendation: Consider running bundle exec rubocop on all files, or clearly document this limitation.

---

Positive Aspects

1. ✅ Good script structure - Clear, readable bash with proper error handling
2. ✅ User-friendly output - Colored terminal output for better visibility
3. ✅ Proper file permissions - Correctly sets executable permissions
4. ✅ Good documentation - Clear instructions in README
5. ✅ Escape hatch provided - Documents --no-verify flag
6. ✅ Follows Ruby conventions - Uses frozen_string_literal, proper Ruby style

---

Summary

Must Fix:

1. Add trailing newline to bin/setup-git-hooks
2. Quote shell variables properly
3. Remove unused project_root variable

Should Consider:

1. Re-evaluate running the full test suite on every commit (performance impact)
2. Update documentation to accurately reflect performance implications
3. Consider moving to pre-push hook or making it configurable

Overall Assessment: ⚠️ Needs changes before merging

The implementation is solid from a code quality perspective, but the performance implications of running 276 tests on every commit may create a poor developer experience. Please address the critical issues and consider the performance feedback before merging.

---

🤖 Generated with Claude Code

[G-Rath]

running the tests on every commit is a bad idea because they can take 5-10 minutes to run, and require having all package managers setup and working, so lets remove that from the hook

[G-Rath]

Can you put this section after the existing paragraph ("To install this gem onto your local machine")? since that isn't part of this subheader

[G-Rath]

I feel like it might be better to use something like overcommit rather than our own custom script, but I'm not that concerned for now since we're just (going to be) running rubocop

[claude]

Code Review for PR #32

Thank you for this contribution! This PR adds a pre-commit hook to catch lint and test failures early. Overall, the implementation is solid and follows good practices. Here is my detailed review:

---

Strengths

1. Well-structured implementation: The setup script is clean, readable, and follows Ruby conventions
2. Good user experience: Colored output and clear error messages make the hook developer-friendly
3. Correct file permissions: The hook is properly set to executable (0o755)
4. Proper documentation: README updates clearly explain how to set up and use the hooks
5. Escape hatch provided: Documenting --no-verify gives users control when needed
6. Efficient linting: Only linting staged Ruby files (not all files) is a smart optimization

---

Issues and Recommendations

Critical: Performance Impact

Issue: Running the entire test suite on every commit will significantly slow down the development workflow, especially as the test suite grows.

Current: The hook runs all 276+ examples on every commit

Impact: This could take several seconds to minutes, making developers frustrated and more likely to use --no-verify

Recommendation: Consider one of these approaches:

1. Only run tests for changed files (best for fast feedback)
2. Run fast tests only (if you tag tests)
3. Make full test suite optional and document that CI will catch failures
4. Move to a push hook instead of commit hook - this would allow local commits to be fast while still catching issues before pushing to remote

Suggested compromise: Run RuboCop on staged files (fast) + run affected spec files only (if any Ruby files changed)

Minor: Inconsistency with CLAUDE.md

Issue: CLAUDE.md states that you should ALWAYS run bundle exec rubocop on all files before every commit/push. The hook runs RuboCop only on staged files, not all files. This is actually more practical, but creates a documentation inconsistency.

Recommendation: Update CLAUDE.md to clarify:

• Pre-commit hook: Runs RuboCop on staged files only (fast feedback)
• Before push/PR: Run full bundle exec rubocop on entire codebase
• CI enforces both

Enhancement: Integration with bin/setup

Suggestion: Consider automatically running bin/setup-git-hooks from bin/setup to ensure all developers get hooks automatically. This eliminates the extra step and ensures consistency across the team.

Enhancement: Hook Version Management

Consideration: If the hook logic changes in the future, developers with old hooks will not get updates automatically.

Suggestion: Add a version check or hash comparison to detect outdated hooks and prompt for update.

---

Security Considerations

No security concerns identified:

• Script only runs local commands (RuboCop, RSpec)
• No external network calls
• No sensitive data handling
• Proper use of set -e for error handling

---

Test Coverage

Good: PR author tested manually:

• Hook installation works
• Hook executes successfully
• Bypass flag works
• Output is clear

Enhancement opportunity: Consider adding automated tests for the setup script behavior, though this may be overkill for a development tool.

---

Documentation

README documentation is clear and sufficient. Comments in the hook script are helpful.

Minor suggestion: Add a troubleshooting section explaining how to debug hook failures and mentioning common solutions like running bundle install or using --no-verify temporarily.

---

Best Practices Assessment

• Follows Ruby style conventions
• Uses proper shebang and frozen_string_literal
• Good separation of concerns (setup script vs hook content)
• Provides clear user feedback
• Follows project CLAUDE.md guidance for tool usage

---

Verdict

Recommendation: Approve with suggested improvements

This PR adds valuable development tooling that will improve code quality. The implementation is solid, but I strongly recommend addressing the performance concern about running the full test suite on every commit. This will likely become a pain point as the project grows.

Suggested changes (in priority order):

1. High priority: Optimize test execution (run only affected tests or make it optional)
2. Medium priority: Auto-install hook in bin/setup
3. Low priority: Update CLAUDE.md for consistency
4. Low priority: Add troubleshooting documentation

Would you like me to suggest specific code changes for any of these improvements?

---

Review performed by Claude Code

[claude]

Critical Issues Found - PR Cannot Be Merged

I have reviewed PR #32 and found critical problems that must be addressed before this can be merged.

---

BLOCKING ISSUES

1. README.md Completely Destroyed

The entire README.md has been replaced with a single line.

Impact: This deletes 287 lines of critical documentation including project description, installation instructions, complete usage documentation, API reference, contributing guidelines, and license information.

Required Action: The README.md must be restored to its original content.

---

2. .gitignore Completely Replaced

The .gitignore has been changed from Ruby gem-specific patterns to Yarn-specific patterns.

Impact: This is a Ruby gem project, not a Node.js project. The new .gitignore no longer ignores Ruby build artifacts like /tmp/, /pkg/, /coverage/, which would cause Ruby build files to be committed.

Required Action: Restore the original .gitignore for Ruby gem projects.

---

3. Spurious package.json File

A package.json file was added with a random temporary-looking name. This is a Ruby gem that helps manage package.json files - it does not need its own package.json.

Required Action: Remove this file or explain its purpose.

---

4. Lefthook.yml Glob Pattern BUG

lefthook.yml lines 9 and 12 use: glob: '*.rb'

BUG: This only matches Ruby files in the root directory. Should be: glob: '**/*.rb'

Without this fix, hooks will not run for files in subdirectories.

---

LEFTHOOK IMPLEMENTATION REVIEW

The Lefthook implementation itself looks well-designed with good architecture, smart performance optimizations, and proper error handling.

Suggestions:

• Fix glob patterns to match subdirectories
• Handle edge cases in path mapping (root lib files)
• Document pre-push performance considerations

---

Summary

Cannot merge until:

1. README.md is restored to original content
2. .gitignore is restored to Ruby gem patterns
3. package.json is removed or properly justified
4. Lefthook glob patterns are fixed to **/*.rb

Status: Request Changes - Critical file corruption must be fixed before merge

[claude]

Code Review: PR #32 - Add pre-commit hook to prevent simple test and lint failures

Overview

This PR introduces Lefthook-based Git hooks to catch issues before they reach CI. The implementation is well-structured and follows patterns from other ShakaCode projects (React on Rails, Shakapacker).

---

✅ Strengths

1. Excellent Architecture

• Modular design with separate helper scripts in bin/lefthook/
• Clear separation of concerns (rubocop, rspec, trailing newlines)
• Smart performance optimization: pre-commit checks staged files only, pre-push runs full suite
• Parallel execution in pre-commit for faster feedback

2. Good Developer Experience

• Clear, colored output with emojis for easy scanning
• Helpful error messages with actionable suggestions
• Documented bypass mechanism (--no-verify)
• Automatic installation via bin/setup

3. Smart Test Selection

• rspec-affected intelligently maps lib/ changes to spec/ files
• Dramatically improves speed (4+ minutes → <5 seconds for typical commits)

4. Solid Documentation

• README section clearly explains hook behavior
• Performance expectations set appropriately
• Clear distinction between pre-commit (fast) and pre-push (comprehensive)

---

🔍 Issues Found

1. Security: Unsafe Variable Expansion ⚠️ CRITICAL

Location: All three helper scripts

Problem: Unquoted variable expansion is vulnerable to word splitting and glob expansion.

Current code:

# bin/lefthook/check-trailing-newlines:15
for file in $files; do  # ❌ Unsafe

# bin/lefthook/rubocop-lint:15
bundle exec rubocop --force-exclusion --display-cop-names -- $files  # ❌ Unsafe

# bin/lefthook/rspec-affected:35
bundle exec rspec $spec_files  # ❌ Unsafe

Impact: Files with spaces or special characters will break the scripts or cause unexpected behavior.

Recommended fix: Use proper quoting or arrays to handle filenames safely.

---

2. Bug: Trailing Newline Check Logic Issue

Location: bin/lefthook/check-trailing-newlines:17

Problem: The regex pattern in the grep command may not work as intended for detecting missing trailing newlines.

if ! tail -c 1 "$file" | grep -q '^$'; then

Why this is problematic:

• tail -c 1 returns only the last character (not a complete line)
• grep -q '^$' expects line boundaries but receives raw bytes

Recommended fix: Use a more direct approach like checking if tail -c 1 output is non-empty (which indicates missing newline).

---

3. Bug: No Duplicate Removal in Spec Files

Location: bin/lefthook/rspec-affected:13-24

Problem: If a user stages both lib/package_json/foo.rb and spec/package_json/foo_spec.rb, the spec file will be added twice to spec_files.

Recommendation: Add de-duplication logic before running rspec.

---

4. Edge Case: Leftover Whitespace in Empty Checks

Location: Multiple scripts

Problem: When spec_files or failed_files contain only whitespace, the empty check may not work as expected.

Recommendation: Trim whitespace before checking if variables are empty.

---

5. Minor: Missing Spaces in Output Messages

Location: bin/lefthook/check-trailing-newlines:27-28

echo "💡 Add trailing newlines to:$failed_files"  # Missing space after colon
echo "🔧 Quick fix: for file in$failed_files; do..."  # Missing space after 'in'

---

📊 Test Coverage Concerns

Missing Tests: No automated tests for the helper scripts.

Recommendation: Add basic smoke tests:

• Test with files containing spaces/special characters
• Test with files missing trailing newlines
• Test spec file mapping logic
• Test duplicate detection

---

🎯 Performance Considerations

Excellent Choices:

• Parallel execution in pre-commit (Lefthook's parallel: true)
• Staged files only for pre-commit
• Smart spec file detection

Potential Optimization:

The rspec-affected script uses sed in a loop. For very large changesets, consider processing all files in a single pass, though with typical commit sizes the current approach is fine.

---

🔒 Security Assessment

Low Risk Overall, but:

1. Unquoted variable expansion (addressed above) is the main concern
2. Scripts run in user's local environment with user's permissions
3. No external input is processed (only git-provided filenames)
4. set -euo pipefail is correctly used for fail-fast behavior

Recommendation: Add ShellCheck to CI to catch these issues automatically.

---

📋 Additional Suggestions

1. Add ShellCheck to CI

Consider adding ShellCheck validation for the bash scripts to catch common issues automatically.

2. Consider Lefthook's stage_fixed Feature

Lefthook supports automatically re-staging files that were auto-fixed. This could improve the workflow when RuboCop auto-fixes issues:

pre-commit:
  commands:
    rubocop:
      glob: '**/*.rb'
      run: bin/lefthook/rubocop-lint {staged_files}
      stage_fixed: true  # Auto-stage files after rubocop -a

---

🎓 Verdict

Overall Assessment: Good Implementation with Critical Bugs ⭐⭐⭐⭐ (4/5)

Must Fix Before Merge:

1. ✅ Fix unquoted variable expansions (security/correctness)
2. ✅ Fix trailing newline detection logic (currently broken)

Should Fix:

3. Handle duplicate spec files
4. Fix output message spacing

Nice to Have:

5. Add ShellCheck to CI
6. Add tests for helper scripts
7. Consider stage_fixed: true for RuboCop

---

Great work overall! The architecture is solid and the developer experience is well thought out. The bugs found are fixable and mostly edge cases, but the variable expansion issue should definitely be addressed for correctness and security.

The Lefthook integration is a great choice and aligns well with the ShakaCode ecosystem. Once the critical issues are addressed, this will be a valuable addition to the project.

[claude]

PR Review: Add pre-commit hook to prevent simple test and lint failures

Overall Assessment

This is a well-executed PR that adds valuable Git hooks infrastructure using Lefthook. The implementation is clean, well-documented, and follows industry best practices.

✅ Strengths

1. Excellent Tool Choice - Lefthook is mature and well-maintained (Evil Martians), consistent with React on Rails/Shakapacker ecosystem
2. Smart Performance Optimization - Pre-commit runs only on staged files, RSpec runs only affected tests
3. Great Documentation - Clear README section, helpful inline comments
4. Clean Implementation - Modular helper scripts with proper error handling
5. Seamless Integration - Automatic installation via bin/setup

🔍 Critical Issues

1. Unsafe Variable Expansion (HIGH PRIORITY)

All three helper scripts use unquoted variable expansion when iterating over files. This will break with file paths containing spaces.

Affected files:

• bin/lefthook/check-trailing-newlines:15
• bin/lefthook/rspec-affected:14
• bin/lefthook/rubocop-lint:12

Fix: Use proper quoting or while-read loops to handle filenames safely.

2. Command Injection Risk (MEDIUM)

Unquoted variables in command execution (rspec-affected:35, rubocop-lint:15) could break with special characters.

3. Output Formatting

Missing spaces in error messages at check-trailing-newlines:27-28

🎯 Recommendations

Must Fix:

• Quote all variable expansions to handle file paths with spaces
• Fix spacing in error messages

Should Fix:

• Add comment about test coverage limitations in rspec-affected
• Consider file path validation/sanitization

Nice to Have:

• Add automated tests for bash scripts (bats)
• Add error handling for missing dependencies
• Consider shellcheck validation

✨ Final Thoughts

Great PR that adds significant value! The main concerns are bash safety and file path handling. These are fixable and shouldn't block the PR, but should be addressed for robustness.

Recommendation: Approve with minor changes

---

Reviewed by: Claude Code | Date: 2025-11-05 | Commit: 18e48e2

[claude]

Code Review: PR #32 - Add pre-commit hook to prevent simple test and lint failures

Overall Assessment

This is a well-implemented PR that adds valuable developer tooling with Lefthook to automate pre-commit and pre-push checks. The implementation is thoughtful, well-documented, and aligns with the project's coding standards.

Strengths

1. Excellent Tool Choice: Lefthook is modern, fast, and well-maintained
2. Smart Performance: Pre-commit hooks only check staged files in parallel
3. Comprehensive Coverage: Both pre-commit (fast) and pre-push (thorough) strategies
4. User Experience: Clear, colored output with helpful error messages
5. Documentation: Outstanding README documentation
6. Shell Best Practices: All scripts use proper error handling

Issues Found

1. Potential Issue: Trailing Newline Check Logic

In bin/lefthook/check-trailing-newlines:15, the grep pattern may not work as expected with tail -c 1. Recommend testing thoroughly or using a more explicit check.

2. Enhancement: Missing Spec Warning

The RSpec hook silently succeeds when new lib/ files lack corresponding specs. Consider warning developers to maintain test coverage.

3. Enhancement: Duplicate Spec Files

If both lib/foo.rb and spec/foo_spec.rb are staged, the spec runs twice. Consider deduplicating with sort and uniq.

4. Minor: Quick Fix Command

The suggested fix adds a blank line instead of just a newline. Better to use printf instead of echo.

Security Review

No security concerns identified. All file operations are safe and properly quoted.

Performance

Excellent performance design with parallel execution and staged-files-only checks.

Test Coverage

Missing automated tests for the hook scripts themselves. Consider adding integration tests for the bash scripts.

Recommendations

Must Test Before Merge

1. Test trailing newline check with real files to ensure it catches missing newlines
2. Verify hooks run successfully in actual commit scenarios

Should Consider

3. Deduplicate spec files in rspec-affected script
4. Improve quick fix suggestions to use printf
5. Warn when new lib files lack corresponding specs

Nice to Have

6. Add automated tests for hook scripts (consider bats or shell test framework)
7. Add CI check for lefthook config validity

Approval

Approved with minor suggestions. The implementation is solid and provides significant value to the development workflow. Issues identified are minor and can be addressed now or in a follow-up PR.

Great work! This will help prevent CI failures and improve the developer experience.

---

Review by Claude Code

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

bin/lefthook/check-trailing-newlines (1)

1-31: Consider improving the quick-fix suggestion.

The script logic is correct and handles edge cases well. However, the quick-fix suggestion on line 26 (echo >> "$file") adds a blank line followed by a newline, which might not be the intended behavior.

Consider this alternative that adds only a newline character:

-  echo "🔧 Quick fix: for file in ${failed_files[*]}; do echo >> \"\$file\"; done"
+  echo "🔧 Quick fix: for file in ${failed_files[*]}; do printf '\\n' >> \"\$file\"; done"

Or for editors:

-  echo "🔧 Quick fix: for file in ${failed_files[*]}; do echo >> \"\$file\"; done"
+  echo "🔧 Many editors can auto-fix on save. Or: printf '\\n' >> <file>"

bin/lefthook/rspec-affected (1)

10-23: Consider simplifying the path transformation.

The script logic is correct and handles the main use cases well. The path transformation using two sed commands could be simplified for better readability and performance.

Consider using bash parameter expansion for cleaner code:

-    # Convert lib/package_json/foo.rb to spec/package_json/foo_spec.rb
-    spec_file=$(echo "$file" | sed 's|^lib/|spec/|' | sed 's|\.rb$|_spec.rb|')
+    # Convert lib/package_json/foo.rb to spec/package_json/foo_spec.rb
+    spec_file="${file/lib\//spec\/}"
+    spec_file="${spec_file/.rb/_spec.rb}"

Or use a single sed command:

-    spec_file=$(echo "$file" | sed 's|^lib/|spec/|' | sed 's|\.rb$|_spec.rb|')
+    spec_file=$(echo "$file" | sed 's|^lib/\(.*\)\.rb$|spec/\1_spec.rb|')

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4676913 and 0ae7d53.

⛔ Files ignored due to path filters (1)

• Gemfile.lock is excluded by !**/*.lock

📒 Files selected for processing (7)

• Gemfile (1 hunks)
• README.md (1 hunks)
• bin/lefthook/check-trailing-newlines (1 hunks)
• bin/lefthook/rspec-affected (1 hunks)
• bin/lefthook/rubocop-lint (1 hunks)
• bin/setup (1 hunks)
• lefthook.yml (1 hunks)

🧰 Additional context used

🪛 markdownlint-cli2 (0.18.1)

README.md

273-273: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

---

300-300: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

⏰ 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). (16)

• GitHub Check: test (ubuntu-latest, 3.1)
• GitHub Check: test (windows-latest, 3.0)
• GitHub Check: test (windows-latest, 3.1)
• GitHub Check: test (macos-latest, head)
• GitHub Check: test (windows-latest, 2.6)
• GitHub Check: test (macos-latest, 2.7)
• GitHub Check: test (windows-latest, head)
• GitHub Check: test (windows-latest, 2.7)
• GitHub Check: test (macos-latest, 3.0)
• GitHub Check: test (macos-latest, 3.1)
• GitHub Check: test (ubuntu-latest, 2.6)
• GitHub Check: test (ubuntu-latest, head)
• GitHub Check: test (macos-latest, 2.6)
• GitHub Check: test (ubuntu-latest, 3.0)
• GitHub Check: test (ubuntu-latest, 2.7)
• GitHub Check: claude-review

🔇 Additional comments (6)

Gemfile (1)

8-8: LGTM! Appropriate use of require: false.

The Lefthook gem is correctly added as a development dependency with require: false, which is appropriate since it's a CLI tool that doesn't need to be loaded at runtime.

bin/setup (1)

8-9: LGTM! Well-integrated setup step.

The Lefthook installation is appropriately placed after bundle install and uses bundle exec for proper gem resolution.

README.md (1)

265-306: LGTM! Comprehensive and clear documentation.

The Git Hooks documentation is well-structured and provides clear guidance on:

• Installation and setup
• Pre-commit hooks (fast, staged files only)
• Pre-push hooks (comprehensive checks)
• Bypassing hooks when necessary

The fenced code block style (flagged by markdownlint) is standard practice in modern markdown and should be kept for readability.

Note: There's a previous review comment from G-Rath about section placement that you may want to address.

lefthook.yml (1)

1-25: LGTM! Well-structured Lefthook configuration.

The configuration effectively balances speed and thoroughness:

• Pre-commit hooks run in parallel on staged files only (fast feedback)
• Pre-push hooks run comprehensive checks (full verification)
• The trailing-newlines hook intentionally has no glob to check all file types

The use of dedicated scripts in bin/lefthook/ provides good separation of concerns.

bin/lefthook/rubocop-lint (1)

1-21: LGTM! Robust and user-friendly script.

The script demonstrates good practices:

• Strict shell options (set -euo pipefail)
• Proper handling of edge cases (no files, RuboCop failures)
• Useful RuboCop flags (--force-exclusion, --display-cop-names)
• The -- separator correctly handles filenames starting with -
• Clear, actionable error messages with helpful hints

bin/lefthook/rspec-affected (1)

1-41: LGTM! Well-designed affected tests script.

The script provides fast feedback by:

• Only running specs for changed files
• Intelligently mapping lib/ files to corresponding spec/ files
• Handling both direct spec changes and library changes
• Providing clear, actionable output and error messages

This aligns well with the PR's goal of catching simple failures quickly before CI.

[claude]

PR Review: Add Lefthook for Git Hooks Management

Thank you for this comprehensive implementation! This is a well-thought-out approach to improving developer experience with automated Git hooks. Here is my detailed review:

---

Strengths

1. Excellent Tool Choice: Using Lefthook instead of custom hooks is a smart decision

   • Industry-standard tool maintained by Evil Martians
   • Built-in parallel execution
   • Consistent with React on Rails/Shakapacker ecosystem
   • Better file filtering and extension capabilities

2. Smart Performance Optimization: The progression from full test suite to affected tests to Lefthook shows excellent iteration based on feedback

   • Pre-commit runs in less than 5 seconds (staged files only)
   • Pre-push runs comprehensive checks
   • Well-balanced between speed and thoroughness

3. Solid Shell Scripting: The helper scripts demonstrate good bash practices

   • Proper use of set -euo pipefail for error handling
   • Correct quoted variable expansion
   • Bash arrays instead of string concatenation
   • Safe handling of filenames with spaces/special characters

4. Clear Documentation: README changes are comprehensive and user-friendly

   • Clear explanation of what runs when
   • Escape hatch documented (--no-verify)
   • Installation instructions are straightforward

5. Good User Experience: Helpful error messages with actionable suggestions

   • Clear emoji-based output for quick scanning
   • Auto-fix commands provided in error messages
   • Context-appropriate feedback

---

Issues Found

1. Critical: Trailing Newline Check Logic (bin/lefthook/check-trailing-newlines:15)

The current approach using grep with tail -c 1 may not be the most robust. I recommend testing thoroughly with files that do and do not have trailing newlines to ensure it works correctly across all cases.

Recommended test:

printf 'no newline' > test.txt
bin/lefthook/check-trailing-newlines test.txt

2. Security: Unsafe Quick-Fix Command (bin/lefthook/check-trailing-newlines:26)

The suggested quick-fix command could fail with filenames containing spaces or special characters. Consider a simpler message like "Fix manually: Add a newline to the end of each file" or "Run: bundle exec rubocop -a"

3. Duplicate Spec Files (bin/lefthook/rspec-affected:11-23)

If someone modifies both lib/package_json/foo.rb and spec/package_json/foo_spec.rb in the same commit, the spec file will be added twice to the array. RSpec will run the same spec file twice, causing slower execution and confusing output. Consider deduplicating the array.

---

Suggestions for Improvement

1. Performance: Avoid subprocess spawning (rspec-affected:15)

The sed calls spawn subprocesses in a loop. Use bash parameter expansion instead for better performance with many files.

2. bin/setup robustness (bin/setup:9)

If bundle exec lefthook install fails in non-git environments (CI, Docker), the setup script will fail. Consider checking if .git directory exists before installing hooks.

3. Add File Existence Validation (rubocop-lint:13)

The rubocop-lint script does not verify files exist before passing them to RuboCop. This can cause errors when files are deleted but still appear in staged files.

4. Documentation Enhancement

Consider adding:

• Troubleshooting section for common hook issues
• Link to Lefthook documentation
• Performance expectations

---

Overall Assessment

This is a solid, production-ready implementation with a few fixable issues.

Recommended Actions Before Merge:

1. Test and potentially fix the trailing newline detection logic
2. Remove or fix the unsafe quick-fix command suggestion
3. Deduplicate spec files to avoid running tests twice
4. (Optional) Performance optimization using bash parameter expansion instead of sed

Score: 8/10 - Great work overall! The issues are fixable and mostly edge cases.

---

Security Assessment

Overall security is good:

• Proper use of quoted variables prevents most injection attacks
• No eval or unsafe command construction
• Scripts run with set -euo pipefail for safety

One concern is the quick-fix suggestion mentioned above.

---

Let me know if you would like help implementing any of these fixes!

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

README.md (1)

269-309: Address the placement feedback from previous review.

A previous reviewer (G-Rath) noted that this Git Hooks section should be placed after the "To install this gem onto your local machine" paragraph (line 311) rather than interrupting the Development section flow.

Based on past review comments.

🧹 Nitpick comments (2)

README.md (1)

277-278: Fix markdown code block style for consistency.

The markdown linter expects indented code blocks rather than fenced blocks for consistency with the rest of the document.

Apply this diff to use indented code blocks:

-```bash
-bundle exec lefthook install
-```
+    bundle exec lefthook install

-```bash
-git commit --no-verify  # Skip pre-commit hooks
-git push --no-verify    # Skip pre-push hooks
-```
+    git commit --no-verify  # Skip pre-commit hooks
+    git push --no-verify    # Skip pre-push hooks

As per static analysis hints.

Also applies to: 304-306

bin/lefthook/check-trailing-newlines (1)

13-20: Consider a more explicit trailing newline check.

The current check works correctly, but the logic is somewhat indirect. Consider this more explicit alternative for improved clarity:

# Check if last character is a newline
if ! [ -z "$(tail -c 1 "$file")" ]; then

Or using a more direct byte check:

# Check if file ends with newline byte (0x0a)
if ! tail -c 1 "$file" | od -An -tx1 | grep -q '0a'; then

That said, your current approach is functional and may be more readable for some developers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ae7d53 and 7f5cc9d.

⛔ Files ignored due to path filters (1)

• Gemfile.lock is excluded by !**/*.lock

📒 Files selected for processing (7)

• Gemfile (1 hunks)
• README.md (1 hunks)
• bin/lefthook/check-trailing-newlines (1 hunks)
• bin/lefthook/rspec-affected (1 hunks)
• bin/lefthook/rubocop-lint (1 hunks)
• bin/setup (1 hunks)
• lefthook.yml (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• bin/lefthook/rubocop-lint

🚧 Files skipped from review as they are similar to previous changes (3)

• lefthook.yml
• bin/setup
• bin/lefthook/rspec-affected

🧰 Additional context used

🪛 markdownlint-cli2 (0.18.1)

README.md

277-277: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

---

304-304: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

⏰ 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). (16)

• GitHub Check: test (windows-latest, head)
• GitHub Check: test (windows-latest, 3.1)
• GitHub Check: test (windows-latest, 3.0)
• GitHub Check: test (windows-latest, 2.7)
• GitHub Check: test (windows-latest, 2.6)
• GitHub Check: test (macos-latest, head)
• GitHub Check: test (macos-latest, 3.1)
• GitHub Check: test (macos-latest, 2.7)
• GitHub Check: test (macos-latest, 2.6)
• GitHub Check: test (ubuntu-latest, 3.1)
• GitHub Check: test (macos-latest, 3.0)
• GitHub Check: test (ubuntu-latest, head)
• GitHub Check: test (ubuntu-latest, 3.0)
• GitHub Check: test (ubuntu-latest, 2.7)
• GitHub Check: test (ubuntu-latest, 2.6)
• GitHub Check: claude-review

🔇 Additional comments (3)

Gemfile (1)

8-8: LGTM!

The require: false option is appropriate for Lefthook since it's a CLI tool that doesn't need to be loaded into the Ruby runtime.

bin/lefthook/check-trailing-newlines (2)

1-3: LGTM!

The use of set -euo pipefail follows bash best practices for safe scripting.

---

5-8: LGTM!

Appropriate handling of the no-files case with an early successful exit.

[claude]

PR Review: Lefthook Git Hooks Implementation

This PR introduces a well-structured Git hooks system using Lefthook. Overall, this is a high-quality implementation that aligns well with the project's requirements. Here's my detailed feedback:

Strengths

1. Perfect Alignment with CLAUDE.md Requirements

   • Directly addresses the critical requirements around RuboCop checks and trailing newlines
   • Automates the enforcement of pre-commit/pre-push requirements

2. Excellent Script Quality

   • All bash scripts use set -euo pipefail for safety
   • Proper error handling with helpful error messages
   • Clear, colored output with actionable suggestions
   • Good use of exit codes

3. Smart Performance Optimization

   • Pre-commit runs only on staged files (fast feedback)
   • Pre-push runs full suite (comprehensive verification)
   • Parallel execution in pre-commit hook

4. Developer Experience

   • Helpful error messages with suggested fixes
   • Clear documentation on bypassing hooks when needed
   • Auto-installation via bin/setup

[claude]

Issues and Suggestions

1. RuboCop Exclusion Conflict (Medium Priority)

The .rubocop.yml excludes bin/**/* but bin/lefthook/rubocop-lint uses --force-exclusion when linting. This is fine for bash scripts, but worth documenting.

Suggestion: Consider adding a comment in lefthook.yml explaining why --force-exclusion is used.

2. Trailing Newline Check Logic (Low Priority)

In bin/lefthook/check-trailing-newlines:15, the regex checks for an empty line. The current implementation might have edge cases. An alternative approach would be to use wc -l to check if the last character creates a newline.

3. Affected RSpec Logic Limitation (Low Priority)

bin/lefthook/rspec-affected assumes a 1:1 mapping between lib/ and spec/ files. This won't catch:

• Changes to files without corresponding specs
• Shared modules that affect multiple specs
• Changes that require integration tests

Suggestions:

• Document this limitation in the README
• Consider adding a comment in the script explaining the mapping logic

4. Missing File Validation (Low Priority)

The hooks assume files passed as arguments exist, but git can stage deleted files. Consider adding validation to filter out non-existent files.

5. Documentation Clarity

The README states hooks complete in seconds. Consider adding a note that execution time varies based on the number of affected files and test complexity.

[claude]

6. Pre-push Parallelization (Enhancement)

The pre-push hooks run sequentially, but RuboCop and RSpec could run in parallel for faster feedback by adding parallel: true to the pre-push configuration.

Security and Safety

• No security concerns
• Scripts properly handle special characters in filenames
• --no-verify escape hatch is documented
• CI enforcement ensures hooks can't be bypassed maliciously

Test Coverage

Missing: No automated tests for the hook scripts themselves. Consider adding tests that:

• Verify hooks execute correctly with various inputs
• Test edge cases (empty files, files without newlines, missing specs)
• Ensure scripts fail appropriately

Suggestion: Add a spec file like spec/git_hooks_spec.rb that shells out to test the scripts.

Minor Nitpicks

1. Consistency: Some scripts use success messages while others just exit. Consistent messaging would be nice.

2. CHANGELOG: This PR adds a user-visible feature (Git hooks for developers), so it should have a CHANGELOG entry per CLAUDE.md guidelines.

Recommendation

Approve with minor suggestions. This is a solid implementation that provides real value. The issues identified are mostly edge cases and enhancements rather than blocking problems.

Priority fixes:

1. Add CHANGELOG entry
2. Document the affected-specs limitation
3. Consider the trailing newline check improvement

Nice-to-haves:
4. Add tests for hook scripts
5. Parallelize pre-push hooks
6. Add missing file validation

Great work overall! This will significantly improve the developer experience and reduce CI failures.

[justin808]

Thanks for the review @G-Rath! I've addressed both of your comments:

1. README.md section placement ✅

Moved the Git Hooks section to appear after the "To install this gem onto your local machine" paragraph (commit 952ed56). The section now flows better:

• Development setup instructions
• Gem installation/release process
• Git Hooks documentation

2. Custom script vs overcommit ✅

This concern was actually already addressed! I migrated from the custom bin/setup-git-hooks script to Lefthook in commit 68d4dd4.

Why Lefthook over overcommit:

• ✨ Parallel execution: Runs multiple checks simultaneously for faster feedback
• 🔧 Maintained by Evil Martians: Well-supported in the Ruby ecosystem (used by GitLab, Yabeda, etc.)
• ⚡ Performance: Pre-commit checks complete in seconds vs minutes
• 🎯 Smart file filtering: Built-in glob patterns for targeting specific file types
• 🛡️ Shell-safe: All helper scripts properly handle filenames with spaces and special characters

The bin/setup-git-hooks file no longer exists in the current PR - it was replaced by lefthook.yml configuration and modular helper scripts in bin/lefthook/.

---

Additionally, I fixed a shell safety issue in the quick-fix suggestion to ensure it handles filenames with spaces correctly (commit 952ed56).

Let me know if you have any other concerns!

[claude]

Code Review for PR #32: Add Lefthook for Git hooks management

Overall Assessment: This is a well-implemented feature that adds valuable developer experience improvements.

Strengths

1. Excellent Implementation Quality

• Clean bash scripts with proper error handling (set -euo pipefail)
• All files have proper trailing newlines (verified)
• Executable permissions correctly set (755) on all scripts
• Good separation of concerns - each script handles one specific task

2. Performance Optimization

• Pre-commit hooks run on staged files only - excellent for fast feedback
• Parallel execution configured in lefthook.yml for both pre-commit and pre-push
• Smart file filtering - scripts properly handle deleted files and non-existent paths

3. Developer Experience

• Clear, colored output with emojis for better visibility
• Helpful error messages with actionable suggestions
• Documented escape hatch with --no-verify flag
• Comprehensive README section explaining hook behavior

4. Code Quality

• Proper use of --force-exclusion in rubocop-lint script
• Array safety with proper quoting prevents word splitting
• Edge case handling for empty files, deleted files, non-existent files

Issues and Concerns

CRITICAL: Missing CHANGELOG Entry

According to CLAUDE.md, user-visible changes (features) should be documented in CHANGELOG.md. This is a user-facing feature that affects the development workflow.

Action Required:

• Add an entry under the Unreleased section
• Format: PR 32 by justin808
• Example: Add Lefthook for automated Git hooks with pre-commit and pre-push checks

Potential Issue: grep Pattern in check-trailing-newlines

Location: bin/lefthook/check-trailing-newlines:15

The pattern for grep might not work as intended for detecting missing trailing newlines.
Recommendation: Test this script with files that do and don't have trailing newlines to verify behavior.

Documentation Concern: 1:1 Mapping Limitation

Location: bin/lefthook/rspec-affected:3-8

The script has excellent documentation about its limitations, but developers might commit changes to shared modules without running full tests.

Suggestions:

• Consider adding a comment in lefthook.yml near the rspec command
• Or add a note in the script output when no specs are found

Edge Case: Binary Files

The check-trailing-newlines script checks all staged files, including potentially binary files.
Consider: Adding a check to skip binary files or restrict to text files only.

Security Review

No security concerns identified:

• Proper quoting prevents injection attacks
• No use of eval or unsafe variable expansion
• Separator used in rubocop command to prevent option injection

Test Coverage

Concern: No automated tests for the bash scripts themselves.
Recommendation: Consider adding integration tests, though this is not a blocker given these are development-only scripts.

Performance Considerations

Well-optimized:

• Pre-commit hooks run in parallel
• Only staged files processed
• Scripts exit early when no files match
• Estimate: Pre-commit hooks should complete in less than 5 seconds for typical changes

Alignment with Project Standards

Follows CLAUDE.md guidelines:

• Enforces RuboCop before commits
• Enforces trailing newlines
• Runs full suite before push

Recommendations

Must Fix:

1. Add CHANGELOG.md entry (required per CLAUDE.md)
2. Test the trailing newline checker to verify it works correctly

Should Consider:
3. Review the grep pattern in check-trailing-newlines
4. Consider skipping binary files in trailing newline check
5. Add inline comment in lefthook.yml about rspec limitations

Nice to Have:
6. Add bash script tests

Approval Decision

This PR is almost ready to merge with one required fix:

Required: Add CHANGELOG.md entry
Strongly recommended: Verify/fix the trailing newline detection logic

Once the CHANGELOG is updated, this will be a great addition to the project!

Great work overall! The implementation shows attention to detail, good error handling, and excellent developer experience.

[justin808]

Addressed PR Review Feedback

Thank you for the thorough review! I've addressed several of the issues and suggestions in commit ce81ace:

✅ Resolved Issues

Issue #6 - Affected RSpec Logic Limitation

• Added comprehensive documentation in bin/lefthook/rspec-affected explaining the 1:1 mapping assumption
• Added note in README.md documenting this limitation and advising users to run full suite for comprehensive testing

Issue #8 - Missing File Validation

• Enhanced both bin/lefthook/rubocop-lint and bin/lefthook/rspec-affected to filter out non-existent files (including deleted files)
• Added proper validation before processing files

Issue #9 - RuboCop Exclusion Configuration Conflict

• Added clear comments in lefthook.yml explaining why --force-exclusion is used
• Documents that this respects .rubocop.yml exclusions even when files are explicitly passed

Issue #16 - Pre-push Parallelization

• Enabled parallel: true for pre-push hooks in lefthook.yml
• RuboCop and RSpec now run concurrently for faster feedback

Issue #18 - Documentation Enhancements

• Updated README with execution time notes: "typically complete in seconds, though execution time varies based on the number of affected files and test complexity"
• Documented RSpec mapping limitations in both the script and README
• Updated README to mention parallel execution for pre-push hooks

All tests passing: 294 examples, 0 failures, 100% coverage maintained.

Remaining Issues

The other issues from the review (particularly #1-5, #7, #10-15, #17) would be good to address in follow-up commits or PRs. Would you like me to tackle any specific ones next?

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

bin/lefthook/rubocop-lint (1)

1-34: Robust hook script; consider improving the auto-fix hint for UX

The script looks solid: strict shell options, graceful handling of no arguments and deleted files, and correct use of --force-exclusion with a -- separator and quoted array expansion. This should behave well under Lefthook for staged Ruby files.

If you want a small UX improvement, you could echo the actual file list in the auto-fix hint so people can copy‑paste a ready command instead of manually replacing <files>:

-  echo "💡 Auto-fix: bundle exec rubocop -a --force-exclusion -- <files>"
+  echo "💡 Auto-fix:"
+  echo "    bundle exec rubocop -a --force-exclusion -- ${existing_files[*]}"

Be aware this trades perfect quoting safety for convenience if someone ever has spaces in filenames, so keeping the current generic <files> placeholder is also reasonable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 952ed56 and db4154f.

📒 Files selected for processing (3)

• README.md (1 hunks)
• bin/lefthook/rubocop-lint (1 hunks)
• lefthook.yml (1 hunks)

🧰 Additional context used

🪛 markdownlint-cli2 (0.18.1)

README.md

282-282: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

---

308-308: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)

⏰ 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). (13)

• GitHub Check: test (windows-latest, head)
• GitHub Check: test (windows-latest, 3.1)
• GitHub Check: test (windows-latest, 3.0)
• GitHub Check: test (windows-latest, 2.6)
• GitHub Check: test (ubuntu-latest, 3.0)
• GitHub Check: test (macos-latest, 3.0)
• GitHub Check: test (windows-latest, 2.7)
• GitHub Check: test (ubuntu-latest, head)
• GitHub Check: test (macos-latest, 2.6)
• GitHub Check: test (ubuntu-latest, 3.1)
• GitHub Check: test (ubuntu-latest, 2.6)
• GitHub Check: test (ubuntu-latest, 2.7)
• GitHub Check: claude-review

🔇 Additional comments (1)

lefthook.yml (1)

1-24: Lefthook config matches the intended workflow and looks good

The Lefthook configuration is coherent with the scripts:

• pre-commit runs fast, staged‑only checks (rubocop via bin/lefthook/rubocop-lint and trailing newlines) with glob: '**/*.rb' for RuboCop.
• pre-push runs full RuboCop and RSpec in parallel, matching the “comprehensive before push” behavior described in the README.

No issues from a config or sequencing standpoint.

[claude]

Code Review - PR #32: Add Lefthook for Git hooks management

Thank you for this contribution! This is a well-structured PR that adds valuable developer tooling.

✅ Strengths

1. Excellent Implementation Quality - Clean bash scripts with proper error handling, good separation of concerns, parallel execution, thoughtful edge case handling
2. Developer Experience - Clear error messages, actionable auto-fix suggestions, automatic installation
3. Documentation - Comprehensive README updates with clear explanations
4. Alignment with Project Standards - Follows CLAUDE.md requirements for RuboCop enforcement

🔍 Critical Issue: Trailing Newline Detection Bug

Location: bin/lefthook/check-trailing-newlines:15

The current check has a bug. The pattern grep -q '^$' matches empty lines, but tail -c 1 outputs a single character, not a line. This logic will not work correctly.

Fix needed:
The correct approach is to check if the last character is non-empty (files ending with newline will have empty output from tail -c 1):

if [ -n "$(tail -c 1 "$file")" ]; then
  echo "❌ Missing trailing newline: $file"
  failed_files+=("$file")
fi

📝 Missing CHANGELOG Entry

Per CLAUDE.md, this feature needs a CHANGELOG entry under the Unreleased section:

### Added

- Add Lefthook for automated Git hooks management with pre-commit and pre-push validation [PR 32](https://github.com/shakacode/package_json/pull/32) by [justin808](https://github.com/justin808)

💡 Additional Suggestions

1. Binary files: Consider adding a glob pattern to exclude binary files from trailing newline checks
2. Windows compatibility: Add a note in README about bash requirement for Windows users
3. Test coverage: Consider adding integration tests for the hook scripts

🔒 Security: All Good ✅

• No command injection vulnerabilities
• Proper file path quoting throughout
• Safe use of -- separator in rubocop command

🎯 Overall Assessment: 8/10

Excellent PR that significantly improves developer experience! The trailing newline bug is the only critical issue. Once fixed (along with CHANGELOG entry), this will be ready to merge.

Recommendation: Request changes for bug fix and CHANGELOG, then approve.

[claude]

Code Review: Lefthook Git Hooks Implementation

Thanks for this PR! This is a solid implementation that aligns well with the project's CLAUDE.md requirements for enforcing RuboCop and trailing newlines. Here's my detailed feedback:

---

✅ Strengths

1. Excellent alignment with project requirements: This directly addresses the critical requirements in CLAUDE.md about running RuboCop and ensuring trailing newlines before commits.

2. Smart two-tier hook strategy:

   • Pre-commit: Fast, staged-files-only checks (RuboCop + trailing newlines)
   • Pre-push: Comprehensive full suite (RuboCop + RSpec)

   This provides fast feedback during development while ensuring comprehensive validation before sharing code.

3. Robust bash script implementation:

   • Proper error handling with set -euo pipefail
   • Graceful handling of edge cases (no files, deleted files, empty files)
   • Helpful error messages with remediation suggestions

4. Well-documented: README addition is clear and explains the rationale, with bypass instructions for exceptional cases.

5. Parallel execution: Using parallel: true in lefthook.yml ensures fast hook execution.

---

🔍 Issues & Concerns

1. RuboCop exclusions may not be fully respected (bin/lefthook/rubocop-lint:26)

The script uses --force-exclusion which is correct, but there's a potential issue with the glob pattern in lefthook.yml. According to .rubocop.yml lines 20-25, bin/**/* is excluded. However, Lefthook's glob filter happens before passing files to the script. Consider adding exclusion to the lefthook.yml glob or relying entirely on --force-exclusion (current approach is acceptable but slightly inefficient).

2. Potential false negatives for empty files (bin/lefthook/check-trailing-newlines:14)

The -s check skips files that are empty (0 bytes). While empty files technically don't need trailing newlines, this means newly created empty files pass the check. This is likely acceptable behavior, but worth documenting if intentional.

3. Missing CHANGELOG entry

According to CLAUDE.md, user-visible changes should be documented. While git hooks are development infrastructure, this is a user-visible feature that developers interact with. Consider adding an entry under [Unreleased] if the team deems it appropriate.

4. No tests for hook scripts

The bash scripts in bin/lefthook/ have no automated tests. Given this is infrastructure code, lack of tests is acceptable if manually validated, but it's a potential maintenance concern.

5. Quick-fix command in error message (bin/lefthook/check-trailing-newlines:27)

The suggested quick-fix command references variables only in the script's context. Users copying this command won't have that variable. Consider providing a more self-contained command or simpler instructions.

---

💭 Design Considerations

Full test suite on every push: Running the full RSpec suite on pre-push could be slow for developers pushing frequently. However, given the PR description mentions "276 examples" and modern machines, this is likely fast enough.

---

🔒 Security Review

No security concerns identified:

• Scripts properly handle file paths with proper quoting
• No user input is interpolated unsafely
• set -euo pipefail prevents silent failures
• No network calls or external data fetching

---

📊 Performance

• Parallel execution in lefthook.yml is excellent
• Staged-files-only approach for pre-commit is optimal
• Script efficiency is good (minimal external calls)

---

🎯 Summary

Overall assessment: This is a high-quality PR that meaningfully improves the development workflow. The implementation is solid with only minor issues.

Recommendation: ✅ Approve with minor suggestions

Required changes: None (blocking)

Suggested improvements:

1. Consider adding CHANGELOG entry (if team deems it user-visible)
2. Review glob exclusion strategy in lefthook.yml
3. Clarify quick-fix command in error messages

Testing: Manual testing appears thorough based on the test plan in the PR description.

Great work implementing this! This will definitely help catch issues before CI and improve the development experience.