From 81acf0c9287478d70d7f2225939f21bd703bacb9 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Sun, 29 Mar 2026 00:07:18 -0400 Subject: [PATCH] fix(hooks): make pre-commit quality checks enforce staged state --- agents/performance-optimizer.md | 17 +-- hooks/README.md | 2 +- scripts/hooks/pre-bash-commit-quality.js | 129 ++++++++++++-------- skills/git-workflow/SKILL.md | 2 +- tests/hooks/pre-bash-commit-quality.test.js | 81 ++++++++++++ 5 files changed, 172 insertions(+), 59 deletions(-) create mode 100644 tests/hooks/pre-bash-commit-quality.test.js diff --git a/agents/performance-optimizer.md b/agents/performance-optimizer.md index 663a1891..914c69a9 100644 --- a/agents/performance-optimizer.md +++ b/agents/performance-optimizer.md @@ -95,7 +95,7 @@ for (const post of allPosts) { // GOOD: Stable callback with useCallback -const handleButtonClick = useCallback(() => handleClick(id), [id]); +const handleButtonClick = useCallback(() => handleClick(id), [handleClick, id]); // BAD: Object creation in render @@ -110,7 +110,7 @@ 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].sort((a, b) => a.name.localeCompare(b.name)), [items] ); @@ -297,10 +297,11 @@ useEffect(() => { }, [largeData]); useEffect(() => { - eventEmitter.on('update', () => { + const handleUpdate = () => { console.log(largeDataRef.current); - }); - return () => eventEmitter.off('update'); + }; + eventEmitter.on('update', handleUpdate); + return () => eventEmitter.off('update', handleUpdate); }, []); ``` @@ -364,7 +365,7 @@ getTTFB(console.log); // Time to First Byte ## Performance Report Template -```markdown +````markdown # Performance Audit Report ## Executive Summary @@ -413,7 +414,7 @@ const fastCode = ...; - Bundle size reduction: XX KB (XX%) - LCP improvement: XXms - Time to Interactive improvement: XXms -``` +```` ## When to Run @@ -442,4 +443,4 @@ const fastCode = ...; --- -**Remember**: Performance is a feature. Users notice speed. Every 100ms of improvement matters. Optimize for the 90th percentile, not the average. \ No newline at end of file +**Remember**: Performance is a feature. Users notice speed. Every 100ms of improvement matters. Optimize for the 90th percentile, not the average. diff --git a/hooks/README.md b/hooks/README.md index 0355b4d7..27fae0a5 100644 --- a/hooks/README.md +++ b/hooks/README.md @@ -23,7 +23,7 @@ User request → Claude picks a tool → PreToolUse hook runs → Tool executes | **Dev server blocker** | `Bash` | Blocks `npm run dev` etc. outside tmux — ensures log access | 2 (blocks) | | **Tmux reminder** | `Bash` | Suggests tmux for long-running commands (npm test, cargo build, docker) | 0 (warns) | | **Git push reminder** | `Bash` | Reminds to review changes before `git push` | 0 (warns) | -| **Pre-commit quality check** | `Bash` | Runs quality checks before `git commit`: lints staged files, validates commit message format, detects console.log/debugger/secrets | 2 (blocks critical) / 0 (warns) | +| **Pre-commit quality check** | `Bash` | Runs quality checks before `git commit`: lints staged files, validates commit message format when provided via `-m/--message`, detects console.log/debugger/secrets | 2 (blocks critical) / 0 (warns) | | **Doc file warning** | `Write` | Warns about non-standard `.md`/`.txt` files (allows README, CLAUDE, CONTRIBUTING, CHANGELOG, LICENSE, SKILL, docs/, skills/); cross-platform path handling | 0 (warns) | | **Strategic compact** | `Edit\|Write` | Suggests manual `/compact` at logical intervals (every ~50 tool calls) | 0 (warns) | | **InsAIts security monitor (opt-in)** | `Bash\|Write\|Edit\|MultiEdit` | Optional security scan for high-signal tool inputs. Disabled unless `ECC_ENABLE_INSAITS=1`. Blocks on critical findings, warns on non-critical, and writes audit log to `.insaits_audit_session.jsonl`. Requires `pip install insa-its`. [Details](../scripts/hooks/insaits-security-monitor.py) | 2 (blocks critical) / 0 (warns) | diff --git a/scripts/hooks/pre-bash-commit-quality.js b/scripts/hooks/pre-bash-commit-quality.js index 4d48e510..10e2d589 100644 --- a/scripts/hooks/pre-bash-commit-quality.js +++ b/scripts/hooks/pre-bash-commit-quality.js @@ -15,7 +15,7 @@ * 2 - Block commit (quality issues found) */ -const { execSync, spawnSync } = require('child_process'); +const { spawnSync } = require('child_process'); const path = require('path'); const fs = require('fs'); @@ -26,15 +26,25 @@ const MAX_STDIN = 1024 * 1024; // 1MB limit * @returns {string[]} Array of staged file paths */ function getStagedFiles() { - try { - const output = execSync('git diff --cached --name-only --diff-filter=ACMR', { - encoding: 'utf8', - stdio: ['pipe', 'pipe', 'pipe'] - }); - return output.trim().split('\n').filter(f => f.length > 0); - } catch { + const result = spawnSync('git', ['diff', '--cached', '--name-only', '--diff-filter=ACMR'], { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'] + }); + if (result.status !== 0) { return []; } + return result.stdout.trim().split('\n').filter(f => f.length > 0); +} + +function getStagedFileContent(filePath) { + const result = spawnSync('git', ['show', `:${filePath}`], { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'] + }); + if (result.status !== 0) { + return null; + } + return result.stdout; } /** @@ -56,7 +66,10 @@ function findFileIssues(filePath) { const issues = []; try { - const content = fs.readFileSync(filePath, 'utf8'); + const content = getStagedFileContent(filePath); + if (content == null) { + return issues; + } const lines = content.split('\n'); lines.forEach((line, index) => { @@ -152,9 +165,9 @@ function validateCommitMessage(command) { } // Check for lowercase first letter (conventional) - if (message.charAt(0) === message.charAt(0).toUpperCase() && conventionalCommit.test(message)) { + if (conventionalCommit.test(message)) { const afterColon = message.split(':')[1]; - if (afterColon && afterColon.trim().charAt(0) === afterColon.trim().charAt(0).toUpperCase()) { + if (afterColon && /^[A-Z]/.test(afterColon.trim())) { issues.push({ type: 'capitalization', message: 'Subject should start with lowercase after type', @@ -193,21 +206,18 @@ function runLinter(files) { // Run ESLint if available if (jsFiles.length > 0) { - try { - const eslintPath = path.join(process.cwd(), 'node_modules', '.bin', 'eslint'); - if (fs.existsSync(eslintPath)) { - const result = spawnSync(eslintPath, ['--format', 'compact', ...jsFiles], { - encoding: 'utf8', - stdio: ['pipe', 'pipe', 'pipe'], - timeout: 30000 - }); - results.eslint = { - success: result.status === 0, - output: result.stdout || result.stderr - }; - } - } catch { - // ESLint not available + const eslintBin = process.platform === 'win32' ? 'eslint.cmd' : 'eslint'; + const eslintPath = path.join(process.cwd(), 'node_modules', '.bin', eslintBin); + if (fs.existsSync(eslintPath)) { + const result = spawnSync(eslintPath, ['--format', 'compact', ...jsFiles], { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 30000 + }); + results.eslint = { + success: result.status === 0, + output: result.stdout || result.stderr + }; } } @@ -219,10 +229,14 @@ function runLinter(files) { stdio: ['pipe', 'pipe', 'pipe'], timeout: 30000 }); - results.pylint = { - success: result.status === 0, - output: result.stdout || result.stderr - }; + if (result.error && result.error.code === 'ENOENT') { + results.pylint = null; + } else { + results.pylint = { + success: result.status === 0, + output: result.stdout || result.stderr + }; + } } catch { // Pylint not available } @@ -236,10 +250,14 @@ function runLinter(files) { stdio: ['pipe', 'pipe', 'pipe'], timeout: 30000 }); - results.golint = { - success: !result.stdout || result.stdout.trim() === '', - output: result.stdout - }; + if (result.error && result.error.code === 'ENOENT') { + results.golint = null; + } else { + results.golint = { + success: !result.stdout || result.stdout.trim() === '', + output: result.stdout + }; + } } catch { // golint not available } @@ -251,32 +269,29 @@ function runLinter(files) { /** * Core logic — exported for direct invocation * @param {string} rawInput - Raw JSON string from stdin - * @returns {string} The original input (pass-through) + * @returns {{output:string, exitCode:number}} Pass-through output and exit code */ -function run(rawInput) { +function evaluate(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; + return { output: rawInput, exitCode: 0 }; } // Check if this is an amend (skip checks for amends to avoid blocking) if (command.includes('--amend')) { - return rawInput; + return { output: rawInput, exitCode: 0 }; } - const issues = []; - const warnings = []; - // Get staged files const stagedFiles = getStagedFiles(); if (stagedFiles.length === 0) { console.error('[Hook] No staged files found. Use "git add" to stage files first.'); - return rawInput; + return { output: rawInput, exitCode: 0 }; } console.error(`[Hook] Checking ${stagedFiles.length} staged file(s)...`); @@ -285,6 +300,8 @@ function run(rawInput) { const filesToCheck = stagedFiles.filter(shouldCheckFile); let totalIssues = 0; let errorCount = 0; + let warningCount = 0; + let infoCount = 0; for (const file of filesToCheck) { const fileIssues = findFileIssues(file); @@ -295,6 +312,8 @@ function run(rawInput) { console.error(` ${icon} Line ${issue.line}: ${issue.message}`); totalIssues++; if (issue.severity === 'error') errorCount++; + if (issue.severity === 'warning') warningCount++; + if (issue.severity === 'info') infoCount++; } } } @@ -308,6 +327,8 @@ function run(rawInput) { if (issue.suggestion) { console.error(` 💡 ${issue.suggestion}`); } + totalIssues++; + warningCount++; } } @@ -317,25 +338,31 @@ function run(rawInput) { if (lintResults.eslint && !lintResults.eslint.success) { console.error('\n🔍 ESLint Issues:'); console.error(lintResults.eslint.output); + totalIssues++; + errorCount++; } if (lintResults.pylint && !lintResults.pylint.success) { console.error('\n🔍 Pylint Issues:'); console.error(lintResults.pylint.output); + totalIssues++; + errorCount++; } if (lintResults.golint && !lintResults.golint.success) { console.error('\n🔍 golint Issues:'); console.error(lintResults.golint.output); + totalIssues++; + errorCount++; } // Summary if (totalIssues > 0) { - console.error(`\n📊 Summary: ${totalIssues} issue(s) found (${errorCount} error(s), ${totalIssues - errorCount} warning(s))`); + console.error(`\n📊 Summary: ${totalIssues} issue(s) found (${errorCount} error(s), ${warningCount} warning(s), ${infoCount} info)`); 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'); @@ -349,7 +376,11 @@ function run(rawInput) { // Non-blocking on error } - return rawInput; + return { output: rawInput, exitCode: 0 }; +} + +function run(rawInput) { + return evaluate(rawInput).output; } // ── stdin entry point ──────────────────────────────────────────── @@ -365,10 +396,10 @@ if (require.main === module) { }); process.stdin.on('end', () => { - data = run(data); - process.stdout.write(data); - process.exit(0); + const result = evaluate(data); + process.stdout.write(result.output); + process.exit(result.exitCode); }); } -module.exports = { run }; \ No newline at end of file +module.exports = { run, evaluate }; diff --git a/skills/git-workflow/SKILL.md b/skills/git-workflow/SKILL.md index d57f51d3..8d1e2523 100644 --- a/skills/git-workflow/SKILL.md +++ b/skills/git-workflow/SKILL.md @@ -713,4 +713,4 @@ git add node_modules/ | 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 +| Revert commit | `git revert HEAD` | diff --git a/tests/hooks/pre-bash-commit-quality.test.js b/tests/hooks/pre-bash-commit-quality.test.js new file mode 100644 index 00000000..478d34d2 --- /dev/null +++ b/tests/hooks/pre-bash-commit-quality.test.js @@ -0,0 +1,81 @@ +/** + * Tests for scripts/hooks/pre-bash-commit-quality.js + * + * Run with: node tests/hooks/pre-bash-commit-quality.test.js + */ + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const hook = require('../../scripts/hooks/pre-bash-commit-quality'); + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + return true; + } catch (err) { + console.log(` ✗ ${name}`); + console.log(` Error: ${err.message}`); + return false; + } +} + +function inTempRepo(fn) { + const prevCwd = process.cwd(); + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pre-bash-commit-quality-')); + + try { + spawnSync('git', ['init'], { cwd: repoDir, stdio: 'pipe', encoding: 'utf8' }); + spawnSync('git', ['config', 'user.name', 'ECC Test'], { cwd: repoDir, stdio: 'pipe', encoding: 'utf8' }); + spawnSync('git', ['config', 'user.email', 'ecc@example.com'], { cwd: repoDir, stdio: 'pipe', encoding: 'utf8' }); + process.chdir(repoDir); + return fn(repoDir); + } finally { + process.chdir(prevCwd); + fs.rmSync(repoDir, { recursive: true, force: true }); + } +} + +let passed = 0; +let failed = 0; + +console.log('\nPre-Bash Commit Quality Hook Tests'); +console.log('==================================\n'); + +if (test('evaluate blocks commits when staged snapshot contains debugger', () => { + inTempRepo(repoDir => { + const filePath = path.join(repoDir, 'index.js'); + fs.writeFileSync(filePath, 'function main() {\n debugger;\n}\n', 'utf8'); + spawnSync('git', ['add', 'index.js'], { cwd: repoDir, stdio: 'pipe', encoding: 'utf8' }); + + const input = JSON.stringify({ tool_input: { command: 'git commit -m "fix: test debugger hook"' } }); + const result = hook.evaluate(input); + + assert.strictEqual(result.output, input, 'should preserve stdin payload'); + assert.strictEqual(result.exitCode, 2, 'should block commit when staged snapshot has debugger'); + }); +})) passed++; else failed++; + +if (test('evaluate inspects staged snapshot instead of newer working tree content', () => { + inTempRepo(repoDir => { + const filePath = path.join(repoDir, 'index.js'); + fs.writeFileSync(filePath, 'function main() {\n return 1;\n}\n', 'utf8'); + spawnSync('git', ['add', 'index.js'], { cwd: repoDir, stdio: 'pipe', encoding: 'utf8' }); + + // Working tree diverges after staging; hook should still inspect staged content. + fs.writeFileSync(filePath, 'function main() {\n debugger;\n return 1;\n}\n', 'utf8'); + + const input = JSON.stringify({ tool_input: { command: 'git commit -m "fix: staged snapshot only"' } }); + const result = hook.evaluate(input); + + assert.strictEqual(result.output, input, 'should preserve stdin payload'); + assert.strictEqual(result.exitCode, 0, 'should ignore unstaged debugger in working tree'); + }); +})) passed++; else failed++; + +console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); +process.exit(failed > 0 ? 1 : 0);