Add smart pre-commit hooks to prevent CI failures #1792
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds Lefthook-based Git hooks and helper scripts, updates docs to describe automated linting, wires Lefthook installation into package postinstall and bootstrap, and adds the lefthook gem to development dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Git as Git (hooks)
participant LH as Lefthook
participant SH as bin/lefthook/*
Dev->>Git: git commit
Git->>LH: pre-commit
LH->>SH: get-changed-files (staged|all-changed)
par Parallel pre-commit steps
LH->>SH: ruby-autofix
LH->>SH: ruby-lint (rubocop)
LH->>SH: prettier-format
LH->>SH: check-trailing-newlines
end
alt Any step fails
LH-->>Git: non-zero exit
Git-->>Dev: Block commit with messages
else All pass
LH-->>Git: success
Git-->>Dev: Commit completes
end
sequenceDiagram
autonumber
actor Dev as Developer
participant Git as Git (hooks)
participant LH as Lefthook
participant SH as ruby-lint
Dev->>Git: git push
Git->>LH: pre-push
LH->>SH: ruby-lint (branch context)
alt Lint fails
LH-->>Git: non-zero exit
Git-->>Dev: Block push with guidance
else Lint passes
LH-->>Git: success
Git-->>Dev: Push proceeds
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
justin808
force-pushed
the
feature/precommit-hook-improvements
branch
from
c4bc979 to
c62359c
Compare
Code Review for PR #1792This PR adds valuable pre-commit hooks to prevent CI failures. The implementation is well-designed with smart branch-based checking. Strengths
Issues Found
Suggestions
SecurityNo issues found. Proper error handling and local-only operations. Recommendation: Approve with fixes - Essential the missing newline is added before merge. |
- Replace manual hook installation with modular Lefthook configuration - Check all changed files (staged + unstaged + untracked) for comprehensive coverage - Add separate scripts in bin/lefthook/ for testability and maintainability - Automatic installation via script/bootstrap and package.json postinstall - Provide helpful error messages with fix commands and skip options - Parallel execution of linting checks for performance Prevents CI failures by catching linting issues locally before commits. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
feature/precommit-hook-improvements
branch
from
c62359c to
3812b8f
Compare
Code Review: Smart Pre-commit Hooks ImplementationThank you for this excellent improvement to the development workflow! This PR successfully implements automatic Git hooks using Lefthook to prevent CI failures. Here's my comprehensive review: ✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
468-483: Fix Prettier command: use nps task (“yarn start format”), not undefined “yarn run format”.Current instruction likely fails as no “format” npm script exists.
-# Run Prettier for JavaScript/TypeScript formatting -yarn run format +# Run Prettier for JavaScript/TypeScript formatting +yarn start format
🧹 Nitpick comments (3)
CLAUDE.md (1)
15-20: Fix markdownlint MD036: use a heading instead of bold for section title.Converts emphasis-as-heading to a proper heading.
-**🚀 AUTOMATIC: Git hooks are installed automatically during setup** +### 🚀 AUTOMATIC: Git hooks are installed automatically during setupGemfile.development_dependencies (1)
41-41: Pin lefthook to a patch-stable constraint (~> 1.13.0).
Latest stable is 1.13.0 (released Sep 11, 2025). Patch-only pin allows safe fixes while preventing minor/major drift.File: Gemfile.development_dependencies Lines: 41-41
- gem "lefthook", require: false + gem "lefthook", "~> 1.13.0", require: falsebin/lefthook/get-changed-files (1)
10-10: Include renames (R) so moved files are processed.Add R to diff-filter to cover renamed files across contexts.
- git diff --cached --name-only --diff-filter=ACM | grep -E "$PATTERN" || true + git diff --cached --name-only --diff-filter=ACMR | grep -E "$PATTERN" || true @@ - (git diff --cached --name-only --diff-filter=ACM; git diff --name-only --diff-filter=ACM; git ls-files --others --exclude-standard) | sort -u | grep -E "$PATTERN" || true + (git diff --cached --name-only --diff-filter=ACMR; git diff --name-only --diff-filter=ACMR; git ls-files --others --exclude-standard) | sort -u | grep -E "$PATTERN" || true @@ - git diff --name-only --diff-filter=ACM "$base"...HEAD | grep -E "$PATTERN" || true + git diff --name-only --diff-filter=ACMR "$base"...HEAD | grep -E "$PATTERN" || trueAlso applies to: 14-14, 20-20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.lefthook.yml(1 hunks)CLAUDE.md(1 hunks)CONTRIBUTING.md(3 hunks)Gemfile.development_dependencies(1 hunks)bin/lefthook/check-trailing-newlines(1 hunks)bin/lefthook/get-changed-files(1 hunks)bin/lefthook/prettier-format(1 hunks)bin/lefthook/ruby-autofix(1 hunks)bin/lefthook/ruby-lint(1 hunks)package.json(1 hunks)script/bootstrap(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-20T03:28:45.556Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-20T03:28:45.556Z
Learning: Ensure every file ends with a newline character
Applied to files:
bin/lefthook/check-trailing-newlines
📚 Learning: 2025-09-20T03:28:45.556Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-20T03:28:45.556Z
Learning: Applies to **/*.{rb,rake} : Before every commit/push, run `bundle exec rubocop` and fix all RuboCop violations
Applied to files:
.lefthook.ymlbin/lefthook/ruby-lintscript/bootstrapbin/lefthook/ruby-autofixCLAUDE.mdCONTRIBUTING.md
📚 Learning: 2025-09-20T03:28:45.556Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-20T03:28:45.556Z
Learning: Applies to node_package/src/**/*.{ts,tsx,js,jsx} : Format JavaScript/TypeScript code with Prettier
Applied to files:
bin/lefthook/prettier-format
📚 Learning: 2025-09-20T03:28:45.556Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-20T03:28:45.556Z
Learning: Never push without running the full lint check first (`rake lint`)
Applied to files:
CLAUDE.mdCONTRIBUTING.md
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
15-15: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (10)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: build
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: claude-review
🔇 Additional comments (5)
CONTRIBUTING.md (2)
10-13: LGTM: clearer prerequisite about automatic Git hooks.
464-466: LGTM: concise automated-linting note..lefthook.yml (1)
20-24: LGTM: targeted pre-push branch linting.bin/lefthook/ruby-lint (1)
5-7: Pattern is quoted incorrectly; grep sees literal quotes → matches nothing.Remove embedded single quotes and escape the backslash.
-CONTEXT="${1:-staged}" -files="$(bin/lefthook/get-changed-files "$CONTEXT" '\.(rb|rake|ru)$')" +CONTEXT="${1:-staged}" +files="$(bin/lefthook/get-changed-files "$CONTEXT" \\.(rb|rake|ru)$)"Likely an incorrect or invalid review comment.
bin/lefthook/ruby-autofix (1)
5-7: Same quoting bug as ruby-lint: pattern never matches.-CONTEXT="${1:-staged}" -files="$(bin/lefthook/get-changed-files "$CONTEXT" '\.(rb|rake|ru)$')" +CONTEXT="${1:-staged}" +files="$(bin/lefthook/get-changed-files "$CONTEXT" \\.(rb|rake|ru)$)"Likely an incorrect or invalid review comment.
Comment on lines
+5
to
+19
| pre-commit: | ||
| parallel: true | ||
| commands: | ||
| autofix: | ||
| run: bin/lefthook/ruby-autofix all-changed | ||
|
|
||
| rubocop: | ||
| run: bin/lefthook/ruby-lint all-changed | ||
|
|
||
| prettier: | ||
| run: bin/lefthook/prettier-format all-changed | ||
|
|
||
| trailing-newlines: | ||
| run: bin/lefthook/check-trailing-newlines all-changed | ||
|
|
There was a problem hiding this comment.
Avoid running fixers and linters in parallel; enforce deterministic sequence.
parallel: true can race autofix/prettier with rubocop, causing flaky failures and missed restaging.
-pre-commit:
- parallel: true
+pre-commit:
+ parallel: false
commands:
autofix:
run: bin/lefthook/ruby-autofix all-changed
rubocop:
run: bin/lefthook/ruby-lint all-changed
prettier:
run: bin/lefthook/prettier-format all-changed
trailing-newlines:
run: bin/lefthook/check-trailing-newlines all-changed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pre-commit: | |
| parallel: true | |
| commands: | |
| autofix: | |
| run: bin/lefthook/ruby-autofix all-changed | |
| rubocop: | |
| run: bin/lefthook/ruby-lint all-changed | |
| prettier: | |
| run: bin/lefthook/prettier-format all-changed | |
| trailing-newlines: | |
| run: bin/lefthook/check-trailing-newlines all-changed | |
| pre-commit: | |
| parallel: false | |
| commands: | |
| autofix: | |
| run: bin/lefthook/ruby-autofix all-changed | |
| rubocop: | |
| run: bin/lefthook/ruby-lint all-changed | |
| prettier: | |
| run: bin/lefthook/prettier-format all-changed | |
| trailing-newlines: | |
| run: bin/lefthook/check-trailing-newlines all-changed |
🤖 Prompt for AI Agents
In .lefthook.yml around lines 5 to 19, the pre-commit hook is configured with
parallel: true which allows autofix/prettier to run concurrently with rubocop
causing race conditions and missed restaging; change the configuration to run
hooks sequentially (remove or set parallel to false) and reorder the commands so
auto-fixers (autofix) run first, then formatters (prettier), and finally linters
(rubocop, trailing-newlines) to ensure fixes are applied and files are restaged
before linting.
| set -euo pipefail | ||
|
|
||
| CONTEXT="${1:-staged}" | ||
| files="$(bin/lefthook/get-changed-files "$CONTEXT" '.*')" |
There was a problem hiding this comment.
Avoid false positives on binaries; fix quoting and user hint.
Current loop may flag images/binaries and breaks on spaces. Filter to text when possible, use arrays, and correct the quick-fix message.
-CONTEXT="${1:-staged}"
-files="$(bin/lefthook/get-changed-files "$CONTEXT" '.*')"
+CONTEXT="${1:-staged}"
+mapfile -t files < <(bin/lefthook/get-changed-files "$CONTEXT" '.*')
@@
-if [ -z "$files" ]; then
+if [ "${#files[@]}" -eq 0 ]; then
echo "✅ No files to check for trailing newlines"
exit 0
fi
@@
-failed_files=""
-for file in $files; do
+failed_files=()
+for file in "${files[@]}"; do
if [ -f "$file" ] && [ -s "$file" ]; then
+ # Skip non-text files if `file` is available
+ if command -v file >/dev/null 2>&1; then
+ if ! file -bi "$file" | grep -qi '^text/'; then
+ continue
+ fi
+ fi
if ! tail -c 1 "$file" | grep -q '^$'; then
echo "❌ Missing trailing newline: $file"
- failed_files="$failed_files $file"
+ failed_files+=("$file")
fi
fi
done
@@
-if [ -n "$failed_files" ]; then
+if [ "${#failed_files[@]}" -gt 0 ]; then
echo ""
echo "❌ Trailing newline check failed!"
- echo "💡 Add trailing newlines to:$failed_files"
- echo "🔧 Quick fix: for file in$failed_files; do echo >> \"\$file\"; done"
+ echo "💡 Add trailing newlines to: ${failed_files[*]}"
+ echo "🔧 Quick fix: for file in ${failed_files[*]}; do printf '\n' >> \"\$file\"; done"
echo "🚫 Skip hook: git commit --no-verify"
exit 1
fiAlso applies to: 19-27, 29-36
🤖 Prompt for AI Agents
In bin/lefthook/check-trailing-newlines around line 6 (and similarly for 19-27
and 29-36), the script uses an unquoted command substitution and a
word-splitting loop which breaks on filenames with spaces and can falsely flag
binaries; replace the string-splitting approach with a safe array read (e.g.,
use mapfile -t files < <(bin/lefthook/get-changed-files "$CONTEXT" '.*')) and
loop over "${files[@]}", and inside the loop detect text files before checking
trailing newlines (use file --mime-type or file --brief --mime-type and skip
entries whose mime type is not text or starts with "text/"); also update the
quick-fix hint message to reference the correct fixer script (e.g.,
bin/lefthook/fix-trailing-newlines) and ensure all filename usages are quoted to
preserve spaces.
Comment on lines
+17
to
+21
| # Find base branch (prefer main over master) | ||
| base="origin/main" | ||
| git rev-parse --verify --quiet "$base" >/dev/null || base="origin/master" | ||
| git diff --name-only --diff-filter=ACM "$base"...HEAD | grep -E "$PATTERN" || true | ||
| ;; |
There was a problem hiding this comment.
Harden base branch resolution for branch diffs.
Handles repos without origin/main or origin/master; prefers origin’s default HEAD, then sensible fallbacks.
- # Find base branch (prefer main over master)
- base="origin/main"
- git rev-parse --verify --quiet "$base" >/dev/null || base="origin/master"
- git diff --name-only --diff-filter=ACM "$base"...HEAD | grep -E "$PATTERN" || true
+ # Resolve base branch (prefer remote default, then origin/main, origin/master, main, master)
+ base=""
+ if git rev-parse --verify --quiet refs/remotes/origin/HEAD >/dev/null; then
+ base="$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD)"
+ fi
+ for candidate in "$base" origin/main origin/master main master; do
+ if [ -n "$candidate" ] && git rev-parse --verify --quiet "$candidate" >/dev/null; then
+ base="$candidate"
+ break
+ fi
+ done
+ if [ -z "$base" ]; then
+ echo "⚠️ Could not resolve base branch; defaulting to HEAD~1" >&2
+ base="HEAD~1"
+ fi
+ git diff --name-only --diff-filter=ACMR "$base"...HEAD | grep -E "$PATTERN" || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Find base branch (prefer main over master) | |
| base="origin/main" | |
| git rev-parse --verify --quiet "$base" >/dev/null || base="origin/master" | |
| git diff --name-only --diff-filter=ACM "$base"...HEAD | grep -E "$PATTERN" || true | |
| ;; | |
| # Resolve base branch (prefer remote default, then origin/main, origin/master, main, master) | |
| base="" | |
| if git rev-parse --verify --quiet refs/remotes/origin/HEAD >/dev/null; then | |
| base="$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD)" | |
| fi | |
| for candidate in "$base" origin/main origin/master main master; do | |
| if [ -n "$candidate" ] && git rev-parse --verify --quiet "$candidate" >/dev/null; then | |
| base="$candidate" | |
| break | |
| fi | |
| done | |
| if [ -z "$base" ]; then | |
| echo "⚠️ Could not resolve base branch; defaulting to HEAD~1" >&2 | |
| base="HEAD~1" | |
| fi | |
| git diff --name-only --diff-filter=ACMR "$base"...HEAD | grep -E "$PATTERN" || true | |
| ;; |
🤖 Prompt for AI Agents
In bin/lefthook/get-changed-files around lines 17 to 21, the current base branch
resolution only tries origin/main then origin/master and fails in repos without
those refs; update the logic to probe for origin/HEAD first, then origin/main,
then origin/master, then local main, then local master, using git rev-parse
--verify --quiet for each check and set the first valid ref as base; if none
exist, fall back to an empty result (no diff) or exit gracefully to avoid
errors. Ensure the script uses the chosen base in the git diff command exactly
as before.
Comment on lines
+5
to
+6
| CONTEXT="${1:-staged}" | ||
| files="$(bin/lefthook/get-changed-files "$CONTEXT" '\.(js|jsx|ts|tsx|json|md|yml|yaml)$')" |
There was a problem hiding this comment.
Quote-safe file handling: avoid word-splitting; use arrays and pass args safely.
Files with spaces/newlines will break current usage. Also restaging via echo|xargs is fragile. Refactor to arrays and quote expansions.
-# Format JS/TS/JSON/MD files with Prettier
+# Format JS/TS/JSON/MD/YAML files with Prettier
@@
-CONTEXT="${1:-staged}"
-files="$(bin/lefthook/get-changed-files "$CONTEXT" '\.(js|jsx|ts|tsx|json|md|yml|yaml)$')"
+CONTEXT="${1:-staged}"
+mapfile -t files < <(bin/lefthook/get-changed-files "$CONTEXT" '\.(js|jsx|ts|tsx|json|md|yml|yaml)$')
@@
-if [ -z "$files" ]; then
+if [ "${#files[@]}" -eq 0 ]; then
echo "✅ No files to format with Prettier"
exit 0
fi
@@
-printf " %s\n" $files
+printf " %s\n" "${files[@]}"
@@
-yarn run prettier --write $files
+yarn run prettier --write -- "${files[@]}"
@@
-# Re-stage files if running on staged or all-changed context
-if [ "$CONTEXT" = "staged" ] || [ "$CONTEXT" = "all-changed" ]; then
- echo $files | xargs -r git add
- echo "✅ Re-staged formatted files"
-fi
+# Re-stage files if running on staged or all-changed context
+if [ "$CONTEXT" = "staged" ] || [ "$CONTEXT" = "all-changed" ]; then
+ git add -- "${files[@]}"
+ echo "✅ Re-staged formatted files"
+fiAlso applies to: 8-11, 18-18, 20-20, 23-26
🤖 Prompt for AI Agents
In bin/lefthook/prettier-format around lines 5-6 (and also apply to 8-11, 18,
20, 23-26), the script word-splits filenames and is unsafe for names with
spaces/newlines; change to capture the output of bin/lefthook/get-changed-files
into a bash array (using mapfile/readarray with a null delimiter or read -r -d
'' loop), avoid unquoted expansions, and pass files as "${files[@]}" to prettier
and git commands; replace echo|xargs restaging with git add -- "${files[@]}" (or
use git -z plumbing) so all arguments are passed safely without word-splitting
or injection.
Comment on lines
+13
to
+26
| if [ "$CONTEXT" = "all-changed" ]; then | ||
| echo "🎨 Autofix on all changed Ruby files:" | ||
| else | ||
| echo "🎨 Autofix on $CONTEXT Ruby files:" | ||
| fi | ||
| printf " %s\n" $files | ||
|
|
||
| bundle exec rake autofix | ||
|
|
||
| # Re-stage files if running on staged or all-changed context | ||
| if [ "$CONTEXT" = "staged" ] || [ "$CONTEXT" = "all-changed" ]; then | ||
| echo $files | xargs -r git add | ||
| echo "✅ Re-staged formatted files" | ||
| fi |
There was a problem hiding this comment.
Limit autofix scope to changed files and restage safely.
Running rake autofix touches the whole repo. Prefer RuboCop autocorrect on the targeted set and robust restaging.
-if [ "$CONTEXT" = "all-changed" ]; then
- echo "🎨 Autofix on all changed Ruby files:"
-else
- echo "🎨 Autofix on $CONTEXT Ruby files:"
-fi
-printf " %s\n" $files
-
-bundle exec rake autofix
-
-# Re-stage files if running on staged or all-changed context
-if [ "$CONTEXT" = "staged" ] || [ "$CONTEXT" = "all-changed" ]; then
- echo $files | xargs -r git add
- echo "✅ Re-staged formatted files"
-fi
+mapfile -t arr < <(printf "%s\n" "$files")
+if [ "$CONTEXT" = "all-changed" ]; then
+ echo "🎨 Autofix on all changed Ruby files:"
+else
+ echo "🎨 Autofix on $CONTEXT Ruby files:"
+fi
+printf " %s\n" "${arr[@]}"
+
+# RuboCop autocorrect only on the targeted files
+bundle exec rubocop -A --force-exclusion -- "${arr[@]}"
+
+# Re-stage only the targeted files when applicable
+if [ "$CONTEXT" = "staged" ] || [ "$CONTEXT" = "all-changed" ]; then
+ git add -- "${arr[@]}"
+ echo "✅ Re-staged formatted files"
+fiCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bin/lefthook/ruby-autofix around lines 13 to 26, the script runs bundle exec
rake autofix which modifies the entire repo; change it to run RuboCop
autocorrect only on the targeted files and restage them safely: replace the rake
call with a NUL-safe invocation that feeds the selected file list to RuboCop
(e.g. printf '%s\0' "$files" | xargs -0 -r bundle exec rubocop -A --) and
similarly restage using printf '%s\0' "$files" | xargs -0 -r git add -- so
filenames with spaces are handled and only changed files are processed.
Comment on lines
+13
to
+21
| if [ "$CONTEXT" = "all-changed" ]; then | ||
| echo "🔍 RuboCop on all changed Ruby files:" | ||
| else | ||
| echo "🔍 RuboCop on $CONTEXT Ruby files:" | ||
| fi | ||
| printf " %s\n" $files | ||
|
|
||
| if ! bundle exec rubocop --force-exclusion --display-cop-names -- $files; then | ||
| echo "" |
There was a problem hiding this comment.
Handle filenames safely and pass args without word-splitting.
Use arrays and quote when invoking RuboCop.
-if [ "$CONTEXT" = "all-changed" ]; then
- echo "🔍 RuboCop on all changed Ruby files:"
-else
- echo "🔍 RuboCop on $CONTEXT Ruby files:"
-fi
-printf " %s\n" $files
-
-if ! bundle exec rubocop --force-exclusion --display-cop-names -- $files; then
+mapfile -t arr < <(printf "%s\n" "$files")
+if [ "$CONTEXT" = "all-changed" ]; then
+ echo "🔍 RuboCop on all changed Ruby files:"
+else
+ echo "🔍 RuboCop on $CONTEXT Ruby files:"
+fi
+printf " %s\n" "${arr[@]}"
+
+if ! bundle exec rubocop --force-exclusion --display-cop-names -- "${arr[@]}"; then
echo ""
echo "❌ RuboCop check failed!"
- echo "💡 Auto-fix: bundle exec rubocop --auto-correct --force-exclusion -- $files"
+ echo "💡 Auto-fix: bundle exec rubocop --auto-correct --force-exclusion -- ${arr[*]}"
echo "🚫 Skip hook: git commit --no-verify"
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ "$CONTEXT" = "all-changed" ]; then | |
| echo "🔍 RuboCop on all changed Ruby files:" | |
| else | |
| echo "🔍 RuboCop on $CONTEXT Ruby files:" | |
| fi | |
| printf " %s\n" $files | |
| if ! bundle exec rubocop --force-exclusion --display-cop-names -- $files; then | |
| echo "" | |
| mapfile -t arr < <(printf "%s\n" "$files") | |
| if [ "$CONTEXT" = "all-changed" ]; then | |
| echo "🔍 RuboCop on all changed Ruby files:" | |
| else | |
| echo "🔍 RuboCop on $CONTEXT Ruby files:" | |
| fi | |
| printf " %s\n" "${arr[@]}" | |
| if ! bundle exec rubocop --force-exclusion --display-cop-names -- "${arr[@]}"; then | |
| echo "" | |
| echo "❌ RuboCop check failed!" | |
| echo "💡 Auto-fix: bundle exec rubocop --auto-correct --force-exclusion -- ${arr[*]}" | |
| echo "🚫 Skip hook: git commit --no-verify" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In bin/lefthook/ruby-lint around lines 13 to 21, the script currently expands
$files unquoted which causes word-splitting and unsafe handling of filenames
with spaces/special chars; convert the files handling to a bash array (e.g.
populate files_array=(...)) and print them safely with printf '%s\n'
"${files_array[@]}", then invoke RuboCop using the array expansion with quotes
(bundle exec rubocop --force-exclusion --display-cop-names --
"${files_array[@]}") so each filename is passed as a single argument and no
word-splitting occurs.
Comment on lines
+485
to
493
| **Git hooks automatically run:** | ||
|
|
||
| - Format all JavaScript/TypeScript files with Prettier | ||
| - Format JavaScript/TypeScript files with Prettier (on changed files only) | ||
| - Check and fix linting issues with ESLint | ||
| - Check and fix Ruby style issues with RuboCop | ||
| - Ensure all tests pass before pushing | ||
| - Check and fix Ruby style issues with RuboCop (on all changed files) | ||
| - Ensure trailing newlines on all files | ||
|
|
||
| **Tip**: Set up your IDE to run these automatically on save to catch issues early. | ||
| **Setup**: Automatic during normal development setup | ||
|
|
There was a problem hiding this comment.
Docs/impl mismatch: ESLint is not part of the configured hooks.
Either add an ESLint hook or update docs to remove ESLint from “automatically run”.
-**Git hooks automatically run:**
-
-- Format JavaScript/TypeScript files with Prettier (on changed files only)
-- Check and fix linting issues with ESLint
-- Check and fix Ruby style issues with RuboCop (on all changed files)
-- Ensure trailing newlines on all files
+**Git hooks automatically run:**
+
+- Format JavaScript/TypeScript files with Prettier (on changed files only)
+- Check Ruby style with RuboCop (on changed files)
+- Ensure trailing newlines on all filesIf you do want ESLint here, I can add an eslint lefthook command and a small wrapper script.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Git hooks automatically run:** | |
| - Format all JavaScript/TypeScript files with Prettier | |
| - Format JavaScript/TypeScript files with Prettier (on changed files only) | |
| - Check and fix linting issues with ESLint | |
| - Check and fix Ruby style issues with RuboCop | |
| - Ensure all tests pass before pushing | |
| - Check and fix Ruby style issues with RuboCop (on all changed files) | |
| - Ensure trailing newlines on all files | |
| **Tip**: Set up your IDE to run these automatically on save to catch issues early. | |
| **Setup**: Automatic during normal development setup | |
| **Git hooks automatically run:** | |
| - Format JavaScript/TypeScript files with Prettier (on changed files only) | |
| - Check Ruby style with RuboCop (on changed files) | |
| - Ensure trailing newlines on all files | |
| **Setup**: Automatic during normal development setup | |
🤖 Prompt for AI Agents
In CONTRIBUTING.md around lines 485 to 493, the docs claim ESLint is
automatically run by git hooks but the repository hook configuration does not
include ESLint; update documentation or add the hook. Either remove the ESLint
bullet from the listed "Git hooks automatically run" section to match current
hook configuration, or add an ESLint lefthook entry and the accompanying wrapper
script to run eslint --fix (and update setup notes) so the docs and hooks are
consistent.
Comment on lines
+105
to
107
| "release:major": "node_package/scripts/release major", | ||
| "postinstall": "test -f .lefthook.yml && test -d .git && command -v bundle >/dev/null 2>&1 && bundle exec lefthook install || true" | ||
| }, |
There was a problem hiding this comment.
Harden postinstall repo detection; don’t assume .git is a directory.
Use Git to detect a repo; also guard on git availability.
- "postinstall": "test -f .lefthook.yml && test -d .git && command -v bundle >/dev/null 2>&1 && bundle exec lefthook install || true"
+ "postinstall": "test -f .lefthook.yml && command -v git >/dev/null 2>&1 && git rev-parse --is-inside-work-tree >/dev/null 2>&1 && command -v bundle >/dev/null 2>&1 && bundle exec lefthook install || true"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "release:major": "node_package/scripts/release major", | |
| "postinstall": "test -f .lefthook.yml && test -d .git && command -v bundle >/dev/null 2>&1 && bundle exec lefthook install || true" | |
| }, | |
| "release:major": "node_package/scripts/release major", | |
| "postinstall": "test -f .lefthook.yml && command -v git >/dev/null 2>&1 && git rev-parse --is-inside-work-tree >/dev/null 2>&1 && command -v bundle >/dev/null 2>&1 && bundle exec lefthook install || true" | |
| }, |
🤖 Prompt for AI Agents
In package.json around lines 105 to 107, the postinstall script assumes a
repository by testing for a .git directory; change it to first check that git is
available (command -v git) and then use git rev-parse --is-inside-work-tree to
detect a repo, keeping the existing .lefthook.yml and bundle checks and the
fallback || true; update the script so it only runs bundle exec lefthook install
when .lefthook.yml exists, git is present and reports inside a work tree, and
bundle is available.
Comment on lines
+34
to
+38
| # Install Git hooks for code quality (development only) | ||
| if [ -f ".lefthook.yml" ] && [ -d ".git" ]; then | ||
| echo "==> Installing Git hooks for code quality…" | ||
| bundle exec lefthook install | ||
| fi |
There was a problem hiding this comment.
Make Git repo detection work with worktrees/submodules (don’t rely on .git dir).
.git can be a file (worktrees, submodules). Use git rev-parse instead.
- if [ -f ".lefthook.yml" ] && [ -d ".git" ]; then
+ if [ -f ".lefthook.yml" ] && git rev-parse --git-dir >/dev/null 2>&1; then
echo "==> Installing Git hooks for code quality…"
bundle exec lefthook install
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Install Git hooks for code quality (development only) | |
| if [ -f ".lefthook.yml" ] && [ -d ".git" ]; then | |
| echo "==> Installing Git hooks for code quality…" | |
| bundle exec lefthook install | |
| fi | |
| # Install Git hooks for code quality (development only) | |
| if [ -f ".lefthook.yml" ] && git rev-parse --git-dir >/dev/null 2>&1; then | |
| echo "==> Installing Git hooks for code quality…" | |
| bundle exec lefthook install | |
| fi |
🤖 Prompt for AI Agents
In script/bootstrap around lines 34 to 38, the current check uses presence of a
.git directory which fails for worktrees/submodules where .git can be a file;
replace the filesystem check with a Git command check (e.g. run git rev-parse
--is-inside-work-tree or git rev-parse --git-dir and test its exit status) so
the condition reliably detects a Git repo across worktrees/submodules, and only
then echo the message and run bundle exec lefthook install; ensure the git
command’s stdout/stderr are redirected to suppress output and preserve the same
behavior when not in a repo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Implements automatic Git hooks using Lefthook to prevent CI failures by catching linting issues locally before commits reach the remote repository.
Key Improvements
bundle installoryarn install- no manual setup requiredbin/lefthook/with no code duplicationWhat It Checks
Developer Experience
Before: Manual setup required, only checked staged files
Now: Completely automatic, checks everything
Error Handling
When linting fails, provides clear fix commands:
Performance Testing
Hooks run in parallel and complete in ~1 second for typical changes, only processing files that have actually changed in the working directory.
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit