feat(hooks): add pre-commit quality check hook#926
feat(hooks): add pre-commit quality check hook#926xingzihai wants to merge 3 commits intoaffaan-m:mainfrom
Conversation
Add comprehensive Git workflow skill covering: - Branching strategies (GitHub Flow, Trunk-Based, GitFlow) - Conventional commits format and best practices - Merge vs rebase with clear guidance - Pull request workflow and templates - Conflict resolution strategies - Branch management and naming conventions - Release management with semantic versioning - Git configuration and useful aliases - Common workflows and anti-patterns This skill helps developers and teams establish consistent Git practices for collaborative development.
- Add pre-bash-commit-quality.js hook script - Runs quality checks before git commit commands: - Lints staged files (ESLint, Pylint, golint) - Validates commit message format (conventional commits) - Detects console.log/debugger statements - Warns about TODO/FIXME without issue references - Detects potential hardcoded secrets - Updates hooks.json with new hook configuration - Updates README.md with hook documentation Cross-platform (Windows, macOS, Linux)
|
Analysis Failed
Troubleshooting
Retry: |
📝 WalkthroughWalkthroughThe changes establish a pre-commit quality gate system by adding a Node.js hook script that validates staged files and commit messages for issues like console.log statements and commit message format violations. Accompanying documentation covers Git workflow best practices and performance optimization patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR adds a The hook architecture and
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Claude Code
participant RWF as run-with-flags.js
participant H as pre-bash-commit-quality.js
participant G as git (staging area)
participant FS as File System (working tree)
participant L as Linters (eslint/pylint/golint)
C->>RWF: git commit -m "..." (Bash tool, strict profile)
RWF->>H: stdin JSON { tool_input: { command } }
H->>G: git diff --cached --name-only (getStagedFiles)
G-->>H: list of staged file paths
H->>FS: fs.readFileSync(filePath) ⚠️ reads working tree, not index
FS-->>H: file content
H->>H: findFileIssues (console.log / debugger / secrets / TODO)
H->>H: validateCommitMessage (conventional commit format)
H->>L: spawnSync eslint / pylint / golint
L-->>H: lint output (exit code)
note over H: ⚠️ lint failures printed but errorCount not incremented
alt errorCount > 0
H->>C: process.exit(2) — commit blocked ⚠️ stdout never written
else warnings only
H-->>C: exit 0 + rawInput on stdout — commit allowed with warnings
end
Reviews (1): Last reviewed commit: "feat(hooks): add pre-commit quality chec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (lintResults.eslint && !lintResults.eslint.success) { | ||
| console.error('\n🔍 ESLint Issues:'); | ||
| console.error(lintResults.eslint.output); | ||
| } | ||
|
|
||
| if (lintResults.pylint && !lintResults.pylint.success) { | ||
| console.error('\n🔍 Pylint Issues:'); | ||
| console.error(lintResults.pylint.output); | ||
| } | ||
|
|
||
| if (lintResults.golint && !lintResults.golint.success) { | ||
| console.error('\n🔍 golint Issues:'); | ||
| console.error(lintResults.golint.output); | ||
| } |
There was a problem hiding this comment.
eslint, pylint, and golint failures are printed to stderr, but neither errorCount nor totalIssues is updated. Because the blocking decision at line 336 only checks errorCount > 0, a failed linter run will always allow the commit through — defeating a core purpose of the hook.
| if (lintResults.eslint && !lintResults.eslint.success) { | |
| console.error('\n🔍 ESLint Issues:'); | |
| console.error(lintResults.eslint.output); | |
| } | |
| if (lintResults.pylint && !lintResults.pylint.success) { | |
| console.error('\n🔍 Pylint Issues:'); | |
| console.error(lintResults.pylint.output); | |
| } | |
| if (lintResults.golint && !lintResults.golint.success) { | |
| console.error('\n🔍 golint Issues:'); | |
| console.error(lintResults.golint.output); | |
| } | |
| if (lintResults.eslint && !lintResults.eslint.success) { | |
| console.error('\n🔍 ESLint Issues:'); | |
| console.error(lintResults.eslint.output); | |
| errorCount++; | |
| totalIssues++; | |
| } | |
| if (lintResults.pylint && !lintResults.pylint.success) { | |
| console.error('\n🔍 Pylint Issues:'); | |
| console.error(lintResults.pylint.output); | |
| errorCount++; | |
| totalIssues++; | |
| } | |
| if (lintResults.golint && !lintResults.golint.success) { | |
| console.error('\n🔍 golint Issues:'); | |
| console.error(lintResults.golint.output); | |
| errorCount++; | |
| totalIssues++; | |
| } |
| // Check for lowercase first letter (conventional) | ||
| if (message.charAt(0) === message.charAt(0).toUpperCase() && conventionalCommit.test(message)) { | ||
| const afterColon = message.split(':')[1]; | ||
| if (afterColon && afterColon.trim().charAt(0) === afterColon.trim().charAt(0).toUpperCase()) { | ||
| issues.push({ | ||
| type: 'capitalization', | ||
| message: 'Subject should start with lowercase after type', | ||
| suggestion: 'Use lowercase for the first letter of the subject' | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The outer guard message.charAt(0) === message.charAt(0).toUpperCase() evaluates to false for every well-formed conventional commit (because the type prefix — feat, fix, etc. — always starts with a lowercase letter). The inner block that checks whether the subject after the colon is capitalised therefore never executes.
The outer condition should be removed; the only relevant check is whether the subject that follows the colon starts with an uppercase letter.
| // Check for lowercase first letter (conventional) | |
| if (message.charAt(0) === message.charAt(0).toUpperCase() && conventionalCommit.test(message)) { | |
| const afterColon = message.split(':')[1]; | |
| if (afterColon && afterColon.trim().charAt(0) === afterColon.trim().charAt(0).toUpperCase()) { | |
| issues.push({ | |
| type: 'capitalization', | |
| message: 'Subject should start with lowercase after type', | |
| suggestion: 'Use lowercase for the first letter of the subject' | |
| }); | |
| } | |
| } | |
| // Check for lowercase first letter (conventional) | |
| if (conventionalCommit.test(message)) { | |
| const afterColon = message.split(':')[1]; | |
| if (afterColon && /^[A-Z]/.test(afterColon.trim())) { | |
| issues.push({ | |
| type: 'capitalization', | |
| message: 'Subject should start with lowercase after type', | |
| suggestion: 'Use lowercase for the first letter of the subject' | |
| }); | |
| } | |
| } |
| try { | ||
| const content = fs.readFileSync(filePath, 'utf8'); | ||
| const lines = content.split('\n'); |
There was a problem hiding this comment.
fs.readFileSync(filePath, 'utf8') reads from the working directory. If a developer stages part of a file with git add -p and then continues editing, the hook will analyse the newer, unstaged version — potentially both missing real staged issues and flagging unstaged changes that will not be committed.
The staged snapshot for a file can be read from git's object store without a temp file:
const { execSync } = require('child_process');
// inside the try block, replace the readFileSync call:
const content = execSync(`git show :${filePath}`, { encoding: 'utf8' });This ensures the hook always checks exactly what will be included in the commit.
| if (totalIssues > 0) { | ||
| console.error(`\n📊 Summary: ${totalIssues} issue(s) found (${errorCount} error(s), ${totalIssues - errorCount} warning(s))`); |
There was a problem hiding this comment.
info-severity items as warnings
totalIssues - errorCount bundles both warning and info (TODO/FIXME) severities under the label warning(s). This overstates the warning count and hides the distinction between actionable warnings and informational notices.
| if (totalIssues > 0) { | |
| console.error(`\n📊 Summary: ${totalIssues} issue(s) found (${errorCount} error(s), ${totalIssues - errorCount} warning(s))`); | |
| if (totalIssues > 0) { | |
| const warnCount = filesToCheck.reduce((acc, file) => { | |
| return acc + findFileIssues(file).filter(i => i.severity === 'warning').length; | |
| }, 0); | |
| const infoCount = totalIssues - errorCount - warnCount; | |
| console.error(`\n📊 Summary: ${totalIssues} issue(s) found (${errorCount} error(s), ${warnCount} warning(s), ${infoCount} info)`); |
|
|
||
| if (errorCount > 0) { | ||
| console.error('\n[Hook] ❌ Commit blocked due to critical issues. Fix them before committing.'); | ||
| process.exit(2); | ||
| } else { |
There was a problem hiding this comment.
process.exit(2) inside run() prevents stdout from being written and breaks testability
When errors are found, process.exit(2) is called from inside run(), which terminates the process before the caller in the stdin entry-point can execute process.stdout.write(data) (line 369). The hook's own README advises "Always output the original data to stdout." More importantly, because run is exported via module.exports, any test that calls run() directly with a blocking input will crash the test runner process rather than returning a value.
A cleaner pattern is to have run() return a structured result and let the entry point handle both the write and the exit:
// Inside run(), replace process.exit(2) with:
return { output: rawInput, exitCode: 2 };
// …and at the end of the normal path:
return { output: rawInput, exitCode: 0 };
// In the stdin entry point:
process.stdin.on('end', () => {
const { output, exitCode } = run(data);
process.stdout.write(output);
process.exit(exitCode);
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
scripts/hooks/pre-bash-commit-quality.js (4)
336-342: Exit code handling is correct but could be clearer.The
process.exit(2)insiderun()terminates the process immediately when critical issues are found, while the stdin entry point'sprocess.exit(0)only executes when no blocking issues were detected. This works correctly but the control flow could be more explicit.Consider returning an exit code from
run()instead of callingprocess.exit()directly, for better testability:♻️ Suggested refactor for testability
if (errorCount > 0) { console.error('\n[Hook] ❌ Commit blocked due to critical issues. Fix them before committing.'); - process.exit(2); + return { output: rawInput, exitCode: 2 }; } else { console.error('\n[Hook] ⚠️ Warnings found. Consider fixing them, but commit is allowed.'); console.error('[Hook] To bypass these checks, use: git commit --no-verify'); } } else { console.error('\n[Hook] ✅ All checks passed!'); } - return rawInput; + return { output: rawInput, exitCode: 0 }; }Then update the stdin entry point to handle the returned object.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-bash-commit-quality.js` around lines 336 - 342, The run() function currently calls process.exit(...) directly which makes testing and control flow harder; refactor run() (the function named run) to return a numeric exit code (e.g., 0 for success, 2 for blocking errors, 0 or 1 for warnings as desired) instead of invoking process.exit, and update the stdin entrypoint that calls run() to receive that return value and call process.exit(returnedCode) once run() resolves; ensure the existing logic around errorCount, the warning branch, and messages printed to stderr remain unchanged except for replacing direct process.exit calls with returned codes.
45-48: Extract checkable extensions to a module-level constant.The extensions array is hardcoded inline. Per coding guidelines, prefer constants for configuration values to improve maintainability.
♻️ Suggested refactor
+const CHECKABLE_EXTENSIONS = ['.js', '.jsx', '.ts', '.tsx', '.py', '.go', '.rs']; + function shouldCheckFile(filePath) { - const checkableExtensions = ['.js', '.jsx', '.ts', '.tsx', '.py', '.go', '.rs']; - return checkableExtensions.some(ext => filePath.endsWith(ext)); + return CHECKABLE_EXTENSIONS.some(ext => filePath.endsWith(ext)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-bash-commit-quality.js` around lines 45 - 48, Extract the hardcoded extensions array from inside shouldCheckFile into a module-level constant (e.g., CHECKABLE_EXTENSIONS) and update shouldCheckFile to reference that constant; specifically, move const checkableExtensions = ['.js', '.jsx', '.ts', '.tsx', '.py', '.go', '.rs'] to the top of the module as CHECKABLE_EXTENSIONS and change the body of shouldCheckFile to return CHECKABLE_EXTENSIONS.some(ext => filePath.endsWith(ext)). Ensure the constant name is exported if needed elsewhere.
62-73:console.logdetection misses cases inside multi-line comments.The check
!line.trim().startsWith('//')and!line.trim().startsWith('*')only catches single-line comments. Aconsole.logmentioned in a multi-line comment body (not starting with*) would still trigger a false warning.This is a minor edge case—most multi-line comment lines start with
*by convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-bash-commit-quality.js` around lines 62 - 73, The console.log detection within the lines.forEach loop incorrectly flags console.log instances that appear inside multi-line /* ... */ comments; modify the logic to track multi-line comment state (e.g., a boolean like inBlockComment) by setting it true when a line contains the start token '/*' and false when it contains the end token '*/' (handle cases where both are on the same line), and skip pushing issues for console.log when inBlockComment is true (leaving the existing single-line comment checks and '*' convention intact); update the block inside lines.forEach (and any surrounding scope) to consult this inBlockComment flag before adding to issues so console.log inside /* ... */ is ignored.
231-246: Replacegolintwithstaticcheckfor Go linting in pre-commit checks.The
golinttool is deprecated and frozen as of Go 1.16+. Usestaticcheckinstead, which is actively maintained and recommended by the Go team. Update lines 231–246 to callstaticcheckrather thangolint, and align with the repo's guidance (from.cursor/rules/golang-hooks.md) to usestaticcheckfor extended static checks on modified packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-bash-commit-quality.js` around lines 231 - 246, Replace the deprecated 'golint' invocation with 'staticcheck' in the block that runs linters for Go files: change the spawnSync command to call 'staticcheck' using the same goFiles array, update the results object key from results.golint to results.staticcheck, and determine success based on the tool's output (e.g., treat empty stdout/stderr as success). Keep the same spawnSync options (encoding, stdio, timeout) and update the catch comment to reflect that staticcheck may be unavailable. Reference the existing spawnSync call, goFiles variable, and results.golint symbol when applying this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/performance-optimizer.md`:
- Around line 299-305: The effect registers an anonymous listener with useEffect
but calls eventEmitter.off('update') without the same handler, so the listener
won't be removed; fix by defining the handler as a named/const function (e.g.,
const updateHandler = () => console.log(largeDataRef.current)) inside the
effect, use eventEmitter.on('update', updateHandler) and return a cleanup that
calls eventEmitter.off('update', updateHandler) so the exact handler reference
is removed; keep the rest of useEffect and largeDataRef usage intact.
- Around line 108-115: The useMemo example incorrectly uses items.sort() which
mutates the original array; update the GOOD example so the memoized computation
creates a new array first (e.g., with [...items] or items.slice()) and then
sorts it to avoid mutating items, keeping the variable name sortedItems and the
useMemo wrapper intact (i.e., change the computation to something like using a
non-mutating clone before .sort within useMemo and keep [items] as the
dependency).
In `@scripts/hooks/pre-bash-commit-quality.js`:
- Around line 127-132: The current regex in validateCommitMessage is too fragile
for quoted strings with escapes; update validateCommitMessage to robustly parse
-m/--message by either using a proper shell-argument parser (e.g., shell-quote)
or replacing the regex with one that handles double-quoted, single-quoted, and
unquoted forms while allowing escaped quotes (capture quoted content via groups
and unescape it), then set message from the first non-empty capture (the
existing messageMatch → message extraction). Ensure the new logic still supports
both -m and --message and handles = or whitespace separators.
- Around line 256-270: The hook cannot see the "strict" profile because
run(rawInput) is called without profile info; update the script to read
process.env.ECC_HOOK_PROFILE inside run (or helper functions) and branch
behavior accordingly instead of relying on an argument passed by run-with-flags;
specifically, adjust any conditional logic currently assuming a profile (e.g.,
around stricter checks in run or functions invoked by run) to check
process.env.ECC_HOOK_PROFILE === 'strict' and behave differently, and keep
isHookEnabled() as-is (it still gates execution).
---
Nitpick comments:
In `@scripts/hooks/pre-bash-commit-quality.js`:
- Around line 336-342: The run() function currently calls process.exit(...)
directly which makes testing and control flow harder; refactor run() (the
function named run) to return a numeric exit code (e.g., 0 for success, 2 for
blocking errors, 0 or 1 for warnings as desired) instead of invoking
process.exit, and update the stdin entrypoint that calls run() to receive that
return value and call process.exit(returnedCode) once run() resolves; ensure the
existing logic around errorCount, the warning branch, and messages printed to
stderr remain unchanged except for replacing direct process.exit calls with
returned codes.
- Around line 45-48: Extract the hardcoded extensions array from inside
shouldCheckFile into a module-level constant (e.g., CHECKABLE_EXTENSIONS) and
update shouldCheckFile to reference that constant; specifically, move const
checkableExtensions = ['.js', '.jsx', '.ts', '.tsx', '.py', '.go', '.rs'] to the
top of the module as CHECKABLE_EXTENSIONS and change the body of shouldCheckFile
to return CHECKABLE_EXTENSIONS.some(ext => filePath.endsWith(ext)). Ensure the
constant name is exported if needed elsewhere.
- Around line 62-73: The console.log detection within the lines.forEach loop
incorrectly flags console.log instances that appear inside multi-line /* ... */
comments; modify the logic to track multi-line comment state (e.g., a boolean
like inBlockComment) by setting it true when a line contains the start token
'/*' and false when it contains the end token '*/' (handle cases where both are
on the same line), and skip pushing issues for console.log when inBlockComment
is true (leaving the existing single-line comment checks and '*' convention
intact); update the block inside lines.forEach (and any surrounding scope) to
consult this inBlockComment flag before adding to issues so console.log inside
/* ... */ is ignored.
- Around line 231-246: Replace the deprecated 'golint' invocation with
'staticcheck' in the block that runs linters for Go files: change the spawnSync
command to call 'staticcheck' using the same goFiles array, update the results
object key from results.golint to results.staticcheck, and determine success
based on the tool's output (e.g., treat empty stdout/stderr as success). Keep
the same spawnSync options (encoding, stdio, timeout) and update the catch
comment to reflect that staticcheck may be unavailable. Reference the existing
spawnSync call, goFiles variable, and results.golint symbol when applying this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ff7a221-ec83-4d87-aaf5-b3ec3b20ced9
📒 Files selected for processing (5)
agents/performance-optimizer.mdhooks/README.mdhooks/hooks.jsonscripts/hooks/pre-bash-commit-quality.jsskills/git-workflow/SKILL.md
| // BAD: Expensive computation on every render | ||
| const sortedItems = items.sort((a, b) => a.name.localeCompare(b.name)); | ||
|
|
||
| // GOOD: Memoize expensive computations | ||
| const sortedItems = useMemo( | ||
| () => items.sort((a, b) => a.name.localeCompare(b.name)), | ||
| [items] | ||
| ); |
There was a problem hiding this comment.
Same mutation issue as in the git-workflow skill.
The useMemo example uses items.sort() which mutates the original array. Per coding guidelines on immutable patterns:
🔧 Fix the example
// GOOD: Memoize expensive computations
const sortedItems = useMemo(
- () => items.sort((a, b) => a.name.localeCompare(b.name)),
+ () => [...items].sort((a, b) => a.name.localeCompare(b.name)),
[items]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agents/performance-optimizer.md` around lines 108 - 115, The useMemo example
incorrectly uses items.sort() which mutates the original array; update the GOOD
example so the memoized computation creates a new array first (e.g., with
[...items] or items.slice()) and then sorts it to avoid mutating items, keeping
the variable name sortedItems and the useMemo wrapper intact (i.e., change the
computation to something like using a non-mutating clone before .sort within
useMemo and keep [items] as the dependency).
| useEffect(() => { | ||
| eventEmitter.on('update', () => { | ||
| console.log(largeDataRef.current); | ||
| }); | ||
| return () => eventEmitter.off('update'); | ||
| }, []); | ||
| ``` |
There was a problem hiding this comment.
Event listener cleanup may be incorrect.
The cleanup eventEmitter.off('update') doesn't pass the handler function reference. Most event emitter implementations require the exact handler to be passed to off() for proper removal:
🔧 Suggested fix
useEffect(() => {
+ const handler = () => {
+ console.log(largeDataRef.current);
+ };
- eventEmitter.on('update', () => {
- console.log(largeDataRef.current);
- });
- return () => eventEmitter.off('update');
+ eventEmitter.on('update', handler);
+ return () => eventEmitter.off('update', handler);
}, []);📝 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.
| useEffect(() => { | |
| eventEmitter.on('update', () => { | |
| console.log(largeDataRef.current); | |
| }); | |
| return () => eventEmitter.off('update'); | |
| }, []); | |
| ``` | |
| useEffect(() => { | |
| const handler = () => { | |
| console.log(largeDataRef.current); | |
| }; | |
| eventEmitter.on('update', handler); | |
| return () => eventEmitter.off('update', handler); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agents/performance-optimizer.md` around lines 299 - 305, The effect registers
an anonymous listener with useEffect but calls eventEmitter.off('update')
without the same handler, so the listener won't be removed; fix by defining the
handler as a named/const function (e.g., const updateHandler = () =>
console.log(largeDataRef.current)) inside the effect, use
eventEmitter.on('update', updateHandler) and return a cleanup that calls
eventEmitter.off('update', updateHandler) so the exact handler reference is
removed; keep the rest of useEffect and largeDataRef usage intact.
| function validateCommitMessage(command) { | ||
| // Extract commit message from command | ||
| const messageMatch = command.match(/(?:-m|--message)[=\s]+["']?([^"']+)["']?/); | ||
| if (!messageMatch) return null; | ||
|
|
||
| const message = messageMatch[1]; |
There was a problem hiding this comment.
Commit message extraction regex is fragile with quoted strings.
The regex [^"']+ stops at the first quote character, causing incorrect extraction for messages containing escaped quotes:
git commit -m "fix: handle \"quoted\" text" # Captures only: fix: handle \Consider a more robust approach or document this limitation.
🔧 Potential improvement
function validateCommitMessage(command) {
- const messageMatch = command.match(/(?:-m|--message)[=\s]+["']?([^"']+)["']?/);
+ // Match -m "message" or --message="message", handling escaped quotes
+ const messageMatch = command.match(/(?:-m|--message)[=\s]+["']?((?:[^"'\\]|\\.)*)["']?/);
if (!messageMatch) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/pre-bash-commit-quality.js` around lines 127 - 132, The current
regex in validateCommitMessage is too fragile for quoted strings with escapes;
update validateCommitMessage to robustly parse -m/--message by either using a
proper shell-argument parser (e.g., shell-quote) or replacing the regex with one
that handles double-quoted, single-quoted, and unquoted forms while allowing
escaped quotes (capture quoted content via groups and unescape it), then set
message from the first non-empty capture (the existing messageMatch → message
extraction). Ensure the new logic still supports both -m and --message and
handles = or whitespace separators.
| function run(rawInput) { | ||
| try { | ||
| const input = JSON.parse(rawInput); | ||
| const command = input.tool_input?.command || ''; | ||
|
|
||
| // Only run for git commit commands | ||
| if (!command.includes('git commit')) { | ||
| return rawInput; | ||
| } | ||
|
|
||
| // Check if this is an amend (skip checks for amends to avoid blocking) | ||
| if (command.includes('--amend')) { | ||
| return rawInput; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the run() signature and whether any profile/config is accessible
ast-grep --pattern 'function run($RAW) {
$$$
}'
# Check if any other hooks access ECC_HOOK_PROFILE for profile-aware behavior
rg -n 'ECC_HOOK_PROFILE' scripts/hooks/Repository: affaan-m/everything-claude-code
Length of output: 14640
🏁 Script executed:
cat -n scripts/hooks/run-with-flags.js | sed -n '110,160p'Repository: affaan-m/everything-claude-code
Length of output: 2164
🏁 Script executed:
# Check hooks.json for how this hook is registered
cat -n hooks.json | grep -A 5 "pre-bash-commit-quality"
# Check if ECC_HOOK_PROFILE is set anywhere in the codebase
rg -n 'ECC_HOOK_PROFILE' . --type js --type sh
# Check if isHookEnabled uses profilesCsv
cat -n scripts/hooks/run-with-flags.js | sed -n '1,110p' | tail -50Repository: affaan-m/everything-claude-code
Length of output: 6241
🏁 Script executed:
# Find hooks.json
find . -name "hooks.json" -type f
# Check the hook registration
fd hooks.json
# Look at how pre-bash-commit-quality is configured
rg -B 2 -A 8 'pre-bash-commit-quality' --type jsonRepository: affaan-m/everything-claude-code
Length of output: 788
🏁 Script executed:
# Verify that pre-bash-commit-quality.js doesn't read ECC_HOOK_PROFILE
rg 'ECC_HOOK_PROFILE' scripts/hooks/pre-bash-commit-quality.js
# Check if it reads process.env for any profile or configuration
rg 'process\.env' scripts/hooks/pre-bash-commit-quality.js | head -20Repository: affaan-m/everything-claude-code
Length of output: 57
🏁 Script executed:
# Check isHookEnabled function to understand what it does with profiles
rg -B 5 -A 20 'function isHookEnabled' scripts/lib/hook-flags.js
# Verify the full context of how hooks.json "strict" is supposed to work
cat -n scripts/lib/hook-flags.js | head -80Repository: affaan-m/everything-claude-code
Length of output: 2925
The script cannot access the "strict" profile information at runtime to conditionally adapt its behavior.
While hooks.json registers this hook with "strict" as the third argument, the run() function receives only (raw, { truncated, maxStdin }) (line 140 of run-with-flags.js). The "strict" profile is consumed by isHookEnabled() to determine whether the hook executes at all, but is never forwarded to the hook itself.
This means the script cannot differentiate between "strict" vs "standard" profiles within run(). If the PR description claims behavior is "gated by a strict profile," this is misleading—the script always runs the same checks when enabled. If profile-aware behavior (e.g., stricter or lenient checks based on profile) is intended, the script needs to read process.env.ECC_HOOK_PROFILE directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/pre-bash-commit-quality.js` around lines 256 - 270, The hook
cannot see the "strict" profile because run(rawInput) is called without profile
info; update the script to read process.env.ECC_HOOK_PROFILE inside run (or
helper functions) and branch behavior accordingly instead of relying on an
argument passed by run-with-flags; specifically, adjust any conditional logic
currently assuming a profile (e.g., around stricter checks in run or functions
invoked by run) to check process.env.ECC_HOOK_PROFILE === 'strict' and behave
differently, and keep isHookEnabled() as-is (it still gates execution).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="agents/performance-optimizer.md">
<violation number="1" location="agents/performance-optimizer.md:98">
P2: The useCallback example omits `handleClick` from the dependency array, which can cause stale closures if `handleClick` changes; the recommended example should include it.</violation>
<violation number="2" location="agents/performance-optimizer.md:113">
P2: The GOOD React example uses items.sort inside useMemo, but Array.prototype.sort mutates the input array. This can mutate props/state in place and cause subtle rendering bugs; the example should clone first (e.g., [...items].sort(...)) or use toSorted().</violation>
<violation number="3" location="agents/performance-optimizer.md:303">
P2: Cleanup example calls `eventEmitter.off('update')` without the listener reference, which doesn’t remove the registered handler for common EventEmitter APIs (e.g., Node.js expects `off(event, listener)`). This makes the “GOOD” cleanup example misleading and can leave the listener attached.</violation>
<violation number="4" location="agents/performance-optimizer.md:367">
P3: The Performance Report Template uses a triple-backtick outer fence while containing inner triple-backtick code blocks (```typescript), which will close the outer block early and break Markdown rendering. Use a longer outer fence (e.g., ````) to allow nested fences.</violation>
</file>
<file name="scripts/hooks/pre-bash-commit-quality.js">
<violation number="1" location="scripts/hooks/pre-bash-commit-quality.js:59">
P2: Pre-commit checks read working-tree file contents rather than the staged snapshot, so partially staged files can produce false positives/negatives in commit gating.</violation>
<violation number="2" location="scripts/hooks/pre-bash-commit-quality.js:155">
P3: Remove the outer uppercase-first-character guard here. Combined with the conventional-commit regex, this condition is unreachable, so subject capitalization after `:` is never validated.</violation>
<violation number="3" location="scripts/hooks/pre-bash-commit-quality.js:197">
P2: Hardcoded `node_modules/.bin/eslint` path is not reliable on Windows (shim is typically `eslint.cmd`/`eslint.ps1`), so ESLint can be skipped even when installed, undermining the cross-platform claim.</violation>
<violation number="4" location="scripts/hooks/pre-bash-commit-quality.js:217">
P2: Pylint availability detection is incorrect: spawnSync doesn’t throw when pylint is missing, so the hook records a failure and reports “Pylint Issues” instead of skipping when pylint isn’t installed.</violation>
<violation number="5" location="scripts/hooks/pre-bash-commit-quality.js:317">
P1: Count failed linter runs as errors before computing the summary/block decision. Right now eslint/pylint/golint failures are only logged, so commits can pass despite lint errors.</violation>
<violation number="6" location="scripts/hooks/pre-bash-commit-quality.js:334">
P3: Report `warning` and `info` counts separately in the summary. The current calculation labels all non-error findings as warnings, which misrepresents TODO/FIXME info-level notices.</violation>
<violation number="7" location="scripts/hooks/pre-bash-commit-quality.js:338">
P2: Avoid calling `process.exit()` inside `run()`; return control to the entry point so stdout is always written before exiting. Exiting here also makes direct calls to `run()` terminate the process instead of returning a result.</violation>
</file>
<file name="hooks/README.md">
<violation number="1" location="hooks/README.md:26">
P2: README overstates commit-message enforcement; hook validates message format only when `git commit` includes inline `-m/--message`.</violation>
</file>
<file name="skills/git-workflow/SKILL.md">
<violation number="1" location="skills/git-workflow/SKILL.md:716">
P3: File is missing a trailing newline, which can trip markdown/editorconfig linters and block CI checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Run linter | ||
| const lintResults = runLinter(filesToCheck); | ||
|
|
||
| if (lintResults.eslint && !lintResults.eslint.success) { |
There was a problem hiding this comment.
P1: Count failed linter runs as errors before computing the summary/block decision. Right now eslint/pylint/golint failures are only logged, so commits can pass despite lint errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/pre-bash-commit-quality.js, line 317:
<comment>Count failed linter runs as errors before computing the summary/block decision. Right now eslint/pylint/golint failures are only logged, so commits can pass despite lint errors.</comment>
<file context>
@@ -0,0 +1,374 @@
+ // Run linter
+ const lintResults = runLinter(filesToCheck);
+
+ if (lintResults.eslint && !lintResults.eslint.success) {
+ console.error('\n🔍 ESLint Issues:');
+ console.error(lintResults.eslint.output);
</file context>
|
|
||
| // GOOD: Memoize expensive computations | ||
| const sortedItems = useMemo( | ||
| () => items.sort((a, b) => a.name.localeCompare(b.name)), |
There was a problem hiding this comment.
P2: The GOOD React example uses items.sort inside useMemo, but Array.prototype.sort mutates the input array. This can mutate props/state in place and cause subtle rendering bugs; the example should clone first (e.g., [...items].sort(...)) or use toSorted().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At agents/performance-optimizer.md, line 113:
<comment>The GOOD React example uses items.sort inside useMemo, but Array.prototype.sort mutates the input array. This can mutate props/state in place and cause subtle rendering bugs; the example should clone first (e.g., [...items].sort(...)) or use toSorted().</comment>
<file context>
@@ -0,0 +1,445 @@
+
+// GOOD: Memoize expensive computations
+const sortedItems = useMemo(
+ () => items.sort((a, b) => a.name.localeCompare(b.name)),
+ [items]
+);
</file context>
| <Button onClick={() => handleClick(id)}>Submit</Button> | ||
|
|
||
| // GOOD: Stable callback with useCallback | ||
| const handleButtonClick = useCallback(() => handleClick(id), [id]); |
There was a problem hiding this comment.
P2: The useCallback example omits handleClick from the dependency array, which can cause stale closures if handleClick changes; the recommended example should include it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At agents/performance-optimizer.md, line 98:
<comment>The useCallback example omits `handleClick` from the dependency array, which can cause stale closures if `handleClick` changes; the recommended example should include it.</comment>
<file context>
@@ -0,0 +1,445 @@
+<Button onClick={() => handleClick(id)}>Submit</Button>
+
+// GOOD: Stable callback with useCallback
+const handleButtonClick = useCallback(() => handleClick(id), [id]);
+<Button onClick={handleButtonClick}>Submit</Button>
+
</file context>
| eventEmitter.on('update', () => { | ||
| console.log(largeDataRef.current); | ||
| }); | ||
| return () => eventEmitter.off('update'); |
There was a problem hiding this comment.
P2: Cleanup example calls eventEmitter.off('update') without the listener reference, which doesn’t remove the registered handler for common EventEmitter APIs (e.g., Node.js expects off(event, listener)). This makes the “GOOD” cleanup example misleading and can leave the listener attached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At agents/performance-optimizer.md, line 303:
<comment>Cleanup example calls `eventEmitter.off('update')` without the listener reference, which doesn’t remove the registered handler for common EventEmitter APIs (e.g., Node.js expects `off(event, listener)`). This makes the “GOOD” cleanup example misleading and can leave the listener attached.</comment>
<file context>
@@ -0,0 +1,445 @@
+ eventEmitter.on('update', () => {
+ console.log(largeDataRef.current);
+ });
+ return () => eventEmitter.off('update');
+}, []);
+```
</file context>
|
|
||
| if (errorCount > 0) { | ||
| console.error('\n[Hook] ❌ Commit blocked due to critical issues. Fix them before committing.'); | ||
| process.exit(2); |
There was a problem hiding this comment.
P2: Avoid calling process.exit() inside run(); return control to the entry point so stdout is always written before exiting. Exiting here also makes direct calls to run() terminate the process instead of returning a result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/pre-bash-commit-quality.js, line 338:
<comment>Avoid calling `process.exit()` inside `run()`; return control to the entry point so stdout is always written before exiting. Exiting here also makes direct calls to `run()` terminate the process instead of returning a result.</comment>
<file context>
@@ -0,0 +1,374 @@
+
+ if (errorCount > 0) {
+ console.error('\n[Hook] ❌ Commit blocked due to critical issues. Fix them before committing.');
+ process.exit(2);
+ } else {
+ console.error('\n[Hook] ⚠️ Warnings found. Consider fixing them, but commit is allowed.');
</file context>
| // Run Pylint if available | ||
| if (pyFiles.length > 0) { | ||
| try { | ||
| const result = spawnSync('pylint', ['--output-format=text', ...pyFiles], { |
There was a problem hiding this comment.
P2: Pylint availability detection is incorrect: spawnSync doesn’t throw when pylint is missing, so the hook records a failure and reports “Pylint Issues” instead of skipping when pylint isn’t installed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/pre-bash-commit-quality.js, line 217:
<comment>Pylint availability detection is incorrect: spawnSync doesn’t throw when pylint is missing, so the hook records a failure and reports “Pylint Issues” instead of skipping when pylint isn’t installed.</comment>
<file context>
@@ -0,0 +1,374 @@
+ // Run Pylint if available
+ if (pyFiles.length > 0) {
+ try {
+ const result = spawnSync('pylint', ['--output-format=text', ...pyFiles], {
+ encoding: 'utf8',
+ stdio: ['pipe', 'pipe', 'pipe'],
</file context>
|
|
||
| ## Performance Report Template | ||
|
|
||
| ```markdown |
There was a problem hiding this comment.
P3: The Performance Report Template uses a triple-backtick outer fence while containing inner triple-backtick code blocks (```typescript), which will close the outer block early and break Markdown rendering. Use a longer outer fence (e.g., ````) to allow nested fences.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At agents/performance-optimizer.md, line 367:
<comment>The Performance Report Template uses a triple-backtick outer fence while containing inner triple-backtick code blocks (```typescript), which will close the outer block early and break Markdown rendering. Use a longer outer fence (e.g., ````) to allow nested fences.</comment>
<file context>
@@ -0,0 +1,445 @@
+
+## Performance Report Template
+
+```markdown
+# Performance Audit Report
+
</file context>
| } | ||
|
|
||
| // Check for lowercase first letter (conventional) | ||
| if (message.charAt(0) === message.charAt(0).toUpperCase() && conventionalCommit.test(message)) { |
There was a problem hiding this comment.
P3: Remove the outer uppercase-first-character guard here. Combined with the conventional-commit regex, this condition is unreachable, so subject capitalization after : is never validated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/pre-bash-commit-quality.js, line 155:
<comment>Remove the outer uppercase-first-character guard here. Combined with the conventional-commit regex, this condition is unreachable, so subject capitalization after `:` is never validated.</comment>
<file context>
@@ -0,0 +1,374 @@
+ }
+
+ // Check for lowercase first letter (conventional)
+ if (message.charAt(0) === message.charAt(0).toUpperCase() && conventionalCommit.test(message)) {
+ const afterColon = message.split(':')[1];
+ if (afterColon && afterColon.trim().charAt(0) === afterColon.trim().charAt(0).toUpperCase()) {
</file context>
|
|
||
| // Summary | ||
| if (totalIssues > 0) { | ||
| console.error(`\n📊 Summary: ${totalIssues} issue(s) found (${errorCount} error(s), ${totalIssues - errorCount} warning(s))`); |
There was a problem hiding this comment.
P3: Report warning and info counts separately in the summary. The current calculation labels all non-error findings as warnings, which misrepresents TODO/FIXME info-level notices.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/pre-bash-commit-quality.js, line 334:
<comment>Report `warning` and `info` counts separately in the summary. The current calculation labels all non-error findings as warnings, which misrepresents TODO/FIXME info-level notices.</comment>
<file context>
@@ -0,0 +1,374 @@
+
+ // Summary
+ if (totalIssues > 0) {
+ console.error(`\n📊 Summary: ${totalIssues} issue(s) found (${errorCount} error(s), ${totalIssues - errorCount} warning(s))`);
+
+ if (errorCount > 0) {
</file context>
| | Pull | `git pull origin branch-name` | | ||
| | Stash | `git stash push -m "message"` | | ||
| | Undo last commit | `git reset --soft HEAD~1` | | ||
| | Revert commit | `git revert HEAD` | No newline at end of file |
There was a problem hiding this comment.
P3: File is missing a trailing newline, which can trip markdown/editorconfig linters and block CI checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/git-workflow/SKILL.md, line 716:
<comment>File is missing a trailing newline, which can trip markdown/editorconfig linters and block CI checks.</comment>
<file context>
@@ -0,0 +1,716 @@
+| Pull | `git pull origin branch-name` |
+| Stash | `git stash push -m "message"` |
+| Undo last commit | `git reset --soft HEAD~1` |
+| Revert commit | `git revert HEAD` |
\ No newline at end of file
</file context>
|
thanks, queued for review. |
2 participants
Summary
This PR adds a new pre-commit quality check hook that runs quality checks before
git commitcommands.Features
Pre-Commit Quality Check Hook
The new hook (
pre-bash-commit-quality.js) performs the following checks before commits:Staged File Analysis
git diff --cached.js,.jsx,.ts,.tsx,.py,.go,.rs)Code Quality Checks
console.logstatements (warning)debuggerstatements (error - blocks commit)Linting
Commit Message Validation
type(scope): description)Files Changed
scripts/hooks/pre-bash-commit-quality.js- New hook script (cross-platform Node.js)hooks/hooks.json- Added hook configurationhooks/README.md- Added documentationExit Codes
0- Success (allow commit)2- Block commit (critical issues found)Testing
The hook:
git commitcommands--amendcommits to avoid blockingstrictmodeExample Output
Summary by CodeRabbit
New Features
Documentation