feat: add pre-commit hooks for cycles, dead exports, signature warnings#381
feat: add pre-commit hooks for cycles, dead exports, signature warnings#381carlos-alm merged 7 commits intomainfrom
Conversation
…gs, and PR body guard Add four Claude Code hooks that run on git commit: - check-commit.sh: combined cycle detection (blocking) + signature change warning (informational). Runs checkData() once with both predicates in a single Node.js invocation, skipping boundaries for speed. Cycles only block if they involve session-edited files. Signature warnings are enriched with role-based risk levels and transitive caller counts via hoisted db.prepare() statements. - check-dead-exports.sh: blocks commits when staged src/ files (edited in this session) contain exports with zero consumers. Batches all files in one Node.js process with a single DB open. - guard-pr-body.sh: blocks PR creation if the body contains "generated with" (case-insensitive). Performance: previous design ran checkData() twice (once for cycles, once for signatures) and spawned per-file CLI processes for dead exports. This version cuts commit hook overhead from ~60s to ~20s by combining predicates and batching DB access.
|
Claude finished @carlos-alm's task —— View job PR Review: Pre-commit Hooks ImplementationTodo List:
Configuration and Documentation Review✅ Hook registration: All three hooks properly added to
✅ Documentation updated: ✅ Timeout ordering: Hooks are ordered by execution time, with the most expensive ( Root Cause AnalysisProblem Being Solved: Adding pre-commit validation to catch circular dependencies, dead exports, and improper PR descriptions before they enter the repository. Why This Approach: Leverages codegraph's existing validation predicates with performance optimizations. The session-scoped validation ensures hooks only check files the AI agent actually modified. Risk Assessment: LOW - Defensive implementation with proper error handling and guard clauses. Final Recommendation
Minor suggestions: Consider adding examples to the hook documentation showing what the error messages look like, but this doesn't block approval. |
Greptile SummaryThis PR introduces three new Claude Code PreToolUse hooks ( Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CC as Claude Code
participant GPB as guard-pr-body.sh
participant CHK as check-commit.sh
participant CDE as check-dead-exports.sh
participant DB as graph.db
participant GL as session-edits.log
Note over CC,GL: git commit triggered
CC->>CHK: PreToolUse (Bash: git commit)
CHK->>GL: read edited files
CHK->>DB: checkData(staged, cycles+signatures)
DB-->>CHK: predicates result
alt cycles involving edited files
CHK-->>CC: deny (BLOCKED: cycles)
else signature violations
CHK->>DB: openReadonlyOrFail() + BFS callers
DB-->>CHK: role, transitive callerCount
CHK-->>CC: allow + additionalContext (sig warning)
else clean
CHK-->>CC: exit 0
end
CC->>CDE: PreToolUse (Bash: git commit)
CDE->>GL: read edited files
CDE->>DB: exportsData(file, unused:true) per file
DB-->>CDE: zero-consumer exports
alt dead exports found
CDE-->>CC: deny (BLOCKED: dead exports)
else clean
CDE-->>CC: exit 0
end
Note over CC,GL: gh pr create triggered
CC->>GPB: PreToolUse (Bash: gh pr create)
GPB->>GPB: grep inline body for "generated with"
GPB->>GPB: read --body-file if present
alt "generated with" found
GPB-->>CC: deny (BLOCKED: remove generated-with line)
else clean
GPB-->>CC: exit 0
end
Last reviewed commit: 9c1cb2a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
.claude/hooks/check-dead-exports.sh
Outdated
| case "$file" in | ||
| src/*.js|src/*.ts|src/*.tsx) ;; | ||
| *) continue ;; | ||
| esac |
There was a problem hiding this comment.
Glob pattern silently skips subdirectories
The case pattern src/*.js|src/*.ts|src/*.tsx only matches files directly under src/ — the shell * glob does not cross directory boundaries. Files in subdirectories (e.g., src/extractors/csharp.js, src/extractors/javascript.js, and the 9 other files under src/extractors/) will silently pass through the filter and never be checked for dead exports.
This means the dead-export check provides incomplete coverage for a significant portion of the codebase today (the entire src/extractors/ subtree), and will silently miss new subdirectories added in the future.
To match nested paths you need a double-glob via extglob or a different approach:
# Option A — use a regex match instead of case/glob
case "$file" in
src/*.js|src/**/*.js|src/*.ts|src/**/*.ts|src/*.tsx|src/**/*.tsx) ;;
*) continue ;;
esac
Note: src/**/*.js requires shopt -s globstar to be enabled. A safer, portable alternative is a grep filter:
# Option B — portable substring match
if ! echo "$file" | grep -qE '^src/.*\.(js|ts|tsx)$'; then
continue
fi
There was a problem hiding this comment.
Fixed — replaced the \ glob with a \ filter so files in subdirectories (e.g. ) are now matched correctly.
.claude/hooks/check-commit.sh
Outdated
| } | ||
| } | ||
|
|
||
| const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'low'; |
There was a problem hiding this comment.
Inconsistent risk-level capitalisation
HIGH and MEDIUM are uppercased while low is lowercase. If any downstream code or display logic ever parses or compares these risk strings (e.g. to colour-code output, filter by severity, or sort), the mixed casing will cause silent mismatches.
| const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'low'; | |
| const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'LOW'; |
There was a problem hiding this comment.
Fixed — changed \ to \ for consistent risk-level capitalisation.
.claude/hooks/guard-pr-body.sh
Outdated
| if echo "$cmd" | grep -qi 'generated with'; then | ||
| echo "BLOCK: Remove any 'Generated with ...' line from the PR body." >&2 | ||
| exit 2 |
There was a problem hiding this comment.
Hook uses a different blocking mechanism than the other two new hooks
check-commit.sh and check-dead-exports.sh both output a structured JSON payload with permissionDecision: 'deny' and exit 0. This hook instead writes to stderr and exits with code 2.
The two mechanisms are valid, but the inconsistency means error messages will surface differently in the Claude Code UI. More importantly, if set -euo pipefail is in effect in a parent shell and the exit code is not explicitly handled, unexpected propagation could occur.
Consider also adding the missing exit 0 at the end of the "allowed" path for symmetry with the other hooks, and/or adopting the structured JSON format used by check-commit.sh for a consistent user experience.
There was a problem hiding this comment.
Fixed — replaced stderr + exit 2 with the structured JSON \ + exit 0 pattern used by the other hooks, and added an explicit \ at the end of the allowed path.
.claude/hooks/check-commit.sh
Outdated
| const db = openReadonlyOrFail(); | ||
| const stmtNode = db.prepare( | ||
| 'SELECT id, role FROM nodes WHERE name = ? AND file = ? AND line = ?' | ||
| ); | ||
| const stmtCallers = db.prepare( | ||
| 'SELECT DISTINCT n.id FROM edges e JOIN nodes n ON e.source_id = n.id WHERE e.target_id = ? AND e.kind = \\'calls\\'' | ||
| ); | ||
|
|
||
| const lines = []; | ||
| for (const v of sigPred.violations) { | ||
| const node = stmtNode.get(v.name, v.file, v.line); | ||
| const role = node?.role || 'unknown'; | ||
|
|
||
| let callerCount = 0; | ||
| if (node) { | ||
| const visited = new Set([node.id]); | ||
| let frontier = [node.id]; | ||
| for (let d = 0; d < 3; d++) { | ||
| const next = []; | ||
| for (const fid of frontier) { | ||
| for (const c of stmtCallers.all(fid)) { | ||
| if (!visited.has(c.id)) { | ||
| visited.add(c.id); | ||
| next.push(c.id); | ||
| callerCount++; | ||
| } | ||
| } | ||
| } | ||
| frontier = next; | ||
| if (!frontier.length) break; | ||
| } | ||
| } | ||
|
|
||
| const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'low'; | ||
| lines.push(risk + ': ' + v.name + ' (' + v.kind + ') [' + role + '] at ' + v.file + ':' + v.line + ' — ' + callerCount + ' transitive callers'); | ||
| } | ||
| db.close(); |
There was a problem hiding this comment.
DB left open if an exception is thrown during signature enrichment
openReadonlyOrFail() is called at line 90 and db.close() is at line 126, but there is no try/finally (or equivalent) around the violation-processing loop. If any call — e.g., stmtCallers.all(fid) — throws unexpectedly, db.close() is never reached.
For a short-lived hook process this is largely harmless (the OS will clean up), but SQLite better-sqlite3 connections hold a file lock; leaving one open during tests or in parallel hook scenarios could cause SQLITE_BUSY errors.
Wrap the enrichment block:
const db = openReadonlyOrFail();
try {
// ... prepare statements, loop over violations ...
} finally {
db.close();
}There was a problem hiding this comment.
Fixed — wrapped the DB enrichment block in try/finally so db.close() is always reached even if an exception is thrown during the BFS loop.
| "type": "command", | ||
| "command": "bash \"$CLAUDE_PROJECT_DIR/.claude/hooks/check-commit.sh\"", | ||
| "timeout": 20 | ||
| }, |
There was a problem hiding this comment.
check-commit.sh timeout (20 s) may be too tight for large graphs
The hook runs checkData() (cycle detection over all staged files) plus an optional BFS up to depth-3 over transitive callers for every signature violation, all in a single Node.js process. On a graph with tens of thousands of nodes/edges the enrichment loop alone can exceed 20 seconds.
When a PreToolUse hook times out, Claude Code silently allows the operation, so a timeout doesn't cause false blocks — but it does silently skip the cycle/signature checks. Consider bumping the timeout to align with the 30 s already used by check-dead-exports.sh, or at least document the intentional trade-off.
There was a problem hiding this comment.
Fixed — bumped check-commit.sh timeout from 20s to 30s to match check-dead-exports.sh and avoid silent skips on larger graphs.
- check-dead-exports.sh: use grep filter instead of case glob to match files in subdirectories (e.g. src/extractors/*.js) - check-commit.sh: uppercase 'LOW' for consistent risk-level casing - check-commit.sh: wrap DB enrichment block in try/finally to prevent leaked SQLite connections on unexpected errors - guard-pr-body.sh: use structured JSON deny + exit 0 consistent with the other hooks instead of stderr + exit 2 - settings.json: bump check-commit.sh timeout from 20s to 30s to match check-dead-exports and avoid silent skips on larger graphs
|
Claude finished @carlos-alm's task —— View job PR Review: Pre-commit Hooks ImplementationTodo List:
Root Cause AnalysisProblem Being Solved: Adding pre-commit validation to catch circular dependencies, dead exports, and improper PR descriptions before they enter the repository. Why This Approach: Leverages codegraph's existing validation predicates with performance optimizations. The session-scoped validation ensures hooks only check files the AI agent actually modified. Risk Assessment: LOW - Defensive implementation with proper error handling and guard clauses. Hook Implementation ReviewFixed Issues from Previous Review: Remaining Issues: Security Analysis: Performance Claims ValidationSingle checkData() call: ✅ Confirmed - Line 58-62 in check-commit.sh runs both cycles and signatures predicates in one call instead of two separate calls. Batched dead export checks: ✅ Confirmed - Lines 65-89 in check-dead-exports.sh processes all files in a single Node.js invocation instead of spawning separate processes. Performance improvement estimates appear valid based on the architectural changes. Configuration and Documentation Review✅ Hook registration: All three hooks properly added to
✅ Documentation updated: ✅ CLI improvement: Minor enhancement in ✅ Timeout adjustments: The timeout for Final Recommendation
Condition: Fix the risk level casing inconsistency in check-commit.sh line 124 ( Minor suggestions: Consider adding examples to the hook documentation showing what the error messages look like, but this doesn't block approval. |
| done <<< "$STAGED" | ||
|
|
||
| if [ -z "$FILES_TO_CHECK" ]; then | ||
| exit 0 |
There was a problem hiding this comment.
"One DB connection" claim is inaccurate.
The comment states "check all files with one DB connection", but exportsData() in src/queries.js opens and closes its own better-sqlite3 connection on every invocation:
export function exportsData(file, customDbPath, opts = {}) {
const db = openReadonlyOrFail(customDbPath); // opens
// ...
db.close(); // closes
}Because the hook loops over files and calls exportsData once per file, there will be N separate DB open/close cycles, not one. The batching into a single Node.js process is a real win (avoiding N child-process spawns), but the DB-connection count claim is misleading.
Consider updating the comment to say "single Node.js process" (accurate) instead of "one DB connection":
| exit 0 | |
| # Single Node.js invocation: check all files (multiple DB opens per file) |
There was a problem hiding this comment.
Fixed in b3f6f41 — changed comment to "in one process" (accurate) instead of "one DB connection" (misleading). The real benefit is batching into a single Node.js process, not sharing a DB connection.
docs/guides/recommended-practices.md
Outdated
|
|
||
| > **Windows note:** If your hooks use bash scripts, normalize backslashes inside `node -e` rather than bash (`${VAR//\\//}` fails on Git Bash). See this repo's `.claude/hooks/enrich-context.sh` for the pattern. | ||
|
|
||
| **Commit check hook** (PreToolUse on Bash): when Claude runs `git commit`, the hook runs `checkData()` once with cycles + signatures predicates enabled (boundaries skipped for speed). If circular dependencies involve files edited in this session, blocks the commit. If function signatures were modified, injects a risk-rated warning via `additionalContext` — `HIGH` for core symbols, `MEDIUM` for utility, `low` for others — with transitive caller counts. Non-blocking for signatures, blocking for cycles. |
There was a problem hiding this comment.
Risk-level capitalisation inconsistency.
The documentation here says low (lowercase), but the code in check-commit.sh now emits 'LOW' (uppercase):
const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'LOW';To avoid confusing readers who might copy-paste these strings for filtering or display logic, update the docs to match the code:
| **Commit check hook** (PreToolUse on Bash): when Claude runs `git commit`, the hook runs `checkData()` once with cycles + signatures predicates enabled (boundaries skipped for speed). If circular dependencies involve files edited in this session, blocks the commit. If function signatures were modified, injects a risk-rated warning via `additionalContext` — `HIGH` for core symbols, `MEDIUM` for utility, `low` for others — with transitive caller counts. Non-blocking for signatures, blocking for cycles. | |
| **Commit check hook** (PreToolUse on Bash): when Claude runs `git commit`, the hook runs `checkData()` once with cycles + signatures predicates enabled (boundaries skipped for speed). If circular dependencies involve files edited in this session, blocks the commit. If function signatures were modified, injects a risk-rated warning via `additionalContext` — `HIGH` for core symbols, `MEDIUM` for utility, `LOW` for others — with transitive caller counts. Non-blocking for signatures, blocking for cycles. |
Also update the Dead export check hook documentation at line 344 to fix the misleading "one DB connection" claim:
| **Commit check hook** (PreToolUse on Bash): when Claude runs `git commit`, the hook runs `checkData()` once with cycles + signatures predicates enabled (boundaries skipped for speed). If circular dependencies involve files edited in this session, blocks the commit. If function signatures were modified, injects a risk-rated warning via `additionalContext` — `HIGH` for core symbols, `MEDIUM` for utility, `low` for others — with transitive caller counts. Non-blocking for signatures, blocking for cycles. | |
| **Dead export check hook** (PreToolUse on Bash): when Claude runs `git commit`, the hook batch-checks all staged `src/` files edited in this session for exports with zero consumers. Batches all files in a single Node.js process for efficiency (not per-file CLI calls). If any export has zero consumers, blocks the commit. |
There was a problem hiding this comment.
Fixed in b3f6f41 — updated docs to use LOW (uppercase) matching the code, and corrected the "one DB connection" claim to "single Node.js process".
|
Claude finished @carlos-alm's task —— View job PR Review: Pre-commit Hooks ImplementationTodo List:
Root Cause AnalysisProblem Being Solved: Adding pre-commit validation to catch circular dependencies, dead exports, and improper PR descriptions before they enter the repository. Why This Approach: Leverages codegraph's existing validation predicates with performance optimizations. The session-scoped validation ensures hooks only check files the AI agent actually modified. Risk Assessment: LOW - Defensive implementation with proper error handling and guard clauses. Hook Implementation ReviewFixed Issues from Previous Review: Remaining Documentation Issues (Non-Critical): Security Analysis: Performance Claims ValidationSingle checkData() call: ✅ Confirmed - Line 58-62 in check-commit.sh runs both cycles and signatures predicates in one call instead of two separate calls. Batched dead export checks: ✅ Confirmed - Lines 65-89 in check-dead-exports.sh processes all files in a single Node.js invocation instead of spawning separate processes. Performance improvement estimates appear valid based on the architectural changes. Configuration and Documentation Review✅ Hook registration: All three hooks properly added to
✅ Documentation updated: ✅ CLI improvement: Minor enhancement in ✅ Timeout adjustments: The timeout for Final Recommendation
Condition: Fix the two minor documentation inconsistencies in Minor suggestions: Consider adding examples to the hook documentation showing what the error messages look like, but this doesn't block approval. |
|
Claude finished @carlos-alm's task —— View job PR Review: Pre-commit Hooks ImplementationTodo List:
Root Cause AnalysisProblem Being Solved: Adding pre-commit validation to catch circular dependencies, dead exports, and improper PR descriptions before they enter the repository. Why This Approach: Leverages codegraph's existing validation predicates with performance optimizations. The session-scoped validation ensures hooks only check files the AI agent actually modified. Risk Assessment: LOW - Defensive implementation with proper error handling and guard clauses. Hook Implementation ReviewFixed Issues from Previous Review: Security Analysis: Remaining Documentation Issues (Minor): Performance Claims ValidationSingle checkData() call: ✅ Confirmed - Line 58-62 in check-commit.sh runs both cycles and signatures predicates in one call instead of two separate calls. Batched dead export checks: ✅ Confirmed - Lines 65-89 in check-dead-exports.sh processes all files in a single Node.js invocation instead of spawning separate processes. Performance improvement estimates appear valid based on the architectural changes. Configuration and Documentation Review✅ Hook registration: All three hooks properly added to
✅ Documentation updated: ✅ CLI improvement: Minor enhancement in ✅ Timeout adjustments: The timeout for Final Recommendation
Conditions: Fix two minor documentation inaccuracies:
Minor suggestions: Consider adding examples to the hook documentation showing what the error messages look like, but this doesn't block approval. |
| echo "$cmd" | grep -qi 'gh pr create' || exit 0 | ||
|
|
||
| # Block if body contains "generated with" | ||
| if echo "$cmd" | grep -qi 'generated with'; then |
There was a problem hiding this comment.
--body-file bypass renders the guard ineffective
The check inspects $cmd (the raw command string) for the literal text generated with. However, when gh pr create is invoked with --body-file /path/to/body.md, the PR body lives in a file — the command string only contains the path, not the body content. If that file contains "generated with", this hook silently allows the PR creation.
Claude Code regularly uses --body-file for longer PR bodies (the body is written to a temp file first). This means the guard is easily bypassed in normal usage.
To close the gap, extract the --body-file path from $cmd and read the file content as well:
# Block if body contains "generated with" — inline body
if echo "$cmd" | grep -qi 'generated with'; then
# ... deny ...
fi
# Also check --body-file path
BODY_FILE=$(echo "$cmd" | grep -oP '(?<=--body-file\s)\S+' || true)
if [ -n "$BODY_FILE" ] && [ -f "$BODY_FILE" ]; then
if grep -qi 'generated with' "$BODY_FILE"; then
# ... deny ...
fi
fiThere was a problem hiding this comment.
Fixed in 9880b4d — the hook now extracts the \ path from the command and inspects that file's content for "generated with", closing the bypass.
.claude/hooks/check-dead-exports.sh
Outdated
| const data = exportsData(file, undefined, { noTests: true, unused: true }); | ||
| if (data && data.results) { | ||
| for (const r of data.results) { | ||
| if (r.consumerCount === 0) { |
There was a problem hiding this comment.
Redundant consumerCount === 0 check — unused: true already filters
exportsData is called with unused: true on line 75. Inside exportsFileImpl in src/queries.js (lines 2388–2391), that flag already filters data.results to only zero-consumer exports:
if (unused) {
filteredResults = results.filter((r) => r.consumerCount === 0);
}As a result, every entry in data.results is already guaranteed to have consumerCount === 0 when unused: true is passed. The guard here is always true and the inner loop body executes unconditionally.
| if (r.consumerCount === 0) { | |
| if (data && data.results) { | |
| for (const r of data.results) { | |
| dead.push(r.name + ' (' + data.file + ':' + r.line + ')'); |
There was a problem hiding this comment.
Fixed in 9880b4d — removed the redundant .filter(r => r.consumerCount === 0) since unused: true already guarantees all results have zero consumers.
.claude/hooks/check-commit.sh
Outdated
| #!/usr/bin/env bash | ||
| # check-commit.sh — PreToolUse hook for Bash (git commit) | ||
| # Combined cycle-detection (blocking) + signature-change warning (informational). | ||
| # Runs checkData() ONCE with both predicates, single DB connection. |
There was a problem hiding this comment.
Header comment "single DB connection" is inaccurate
checkData() opens and closes its own better-sqlite3 connection internally, then the enrichment block (line 90) opens a second connection via openReadonlyOrFail(). The total is two DB opens per hook run — not one.
The real performance win here is running everything in one Node.js process (not spawning a child process per check), and the enrichment block uses one connection shared across all violations.
| # Runs checkData() ONCE with both predicates, single DB connection. | |
| # Runs checkData() ONCE with both predicates, single Node.js process. |
There was a problem hiding this comment.
Fixed in 9880b4d — changed header to "single Node.js process" which accurately reflects the behavior (two DB opens, but one process).
|
Claude finished @carlos-alm's task —— View job PR Review: Pre-commit Hooks ImplementationTodo List:
Root Cause AnalysisProblem Being Solved: Adding pre-commit validation to catch circular dependencies, dead exports, and improper PR descriptions before they enter the repository. Why This Approach: Leverages codegraph's existing validation predicates with performance optimizations. The session-scoped validation ensures hooks only check files the AI agent actually modified. Risk Assessment: LOW - Defensive implementation with proper error handling and guard clauses. Hook Implementation ReviewSecurity Analysis: Fixed Issues from Previous Reviews: Performance Claims ValidationSingle checkData() call: ✅ Confirmed - Line 58-62 in check-commit.sh runs both cycles and signatures predicates in one call instead of two separate calls. Batched dead export checks: ✅ Confirmed - Lines 65-89 in check-dead-exports.sh processes all files in a single Node.js invocation instead of spawning separate processes. Performance improvement estimates appear valid based on the architectural changes. Configuration and Documentation Review✅ Hook registration: All three hooks properly added to
✅ Documentation updated: ✅ CLI improvement: Minor enhancement in Greptile's Remaining Concerns Assessment🟡 Minor Issues (Non-blocking):
✅ Previously Critical Issues Now Fixed:
Final Recommendation
Minor suggestions: The redundant check and comment inaccuracies are style issues that don't affect functionality and can be addressed in future iterations if desired. |
|
Claude finished @carlos-alm's task —— View job PR Review: Pre-commit Hooks ImplementationTodo List:
Root Cause AnalysisProblem Being Solved: Adding pre-commit validation to catch circular dependencies, dead exports, and improper PR descriptions before they enter the repository. Why This Approach: Leverages codegraph's existing validation predicates with performance optimizations. The session-scoped validation ensures hooks only check files the AI agent actually modified. Risk Assessment: LOW - Defensive implementation with proper error handling and guard clauses. Hook Implementation ReviewSecurity Analysis: Fixed Issues from Previous Reviews: Performance Claims ValidationSingle checkData() call: ✅ Confirmed - Line 58-62 in check-commit.sh runs both cycles and signatures predicates in one call instead of two separate calls. Batched dead export checks: ✅ Confirmed - Lines 65-89 in check-dead-exports.sh processes all files in a single Node.js invocation instead of spawning separate processes. Performance improvement estimates appear valid based on the architectural changes. Configuration and Documentation Review✅ Hook registration: All three hooks properly added to
✅ Documentation updated: ✅ CLI improvement: Minor enhancement in Greptile's Remaining Concerns Assessment🟡 Minor Issues (Non-blocking):
✅ Previously Critical Issues Now Fixed:
Final Recommendation
Minor suggestions: The redundant check and comment inaccuracies are style issues that don't affect functionality and can be addressed in future iterations if desired. |
| const sigPred = data.predicates.find(p => p.name === 'signatures'); | ||
| if (sigPred && !sigPred.passed && sigPred.violations?.length) { | ||
| // Enrich with role + transitive caller count using a single DB connection | ||
| const db = openReadonlyOrFail(); | ||
| const lines = []; | ||
| try { | ||
| const stmtNode = db.prepare( | ||
| 'SELECT id, role FROM nodes WHERE name = ? AND file = ? AND line = ?' | ||
| ); | ||
| const stmtCallers = db.prepare( | ||
| 'SELECT DISTINCT n.id FROM edges e JOIN nodes n ON e.source_id = n.id WHERE e.target_id = ? AND e.kind = \\'calls\\'' | ||
| ); | ||
|
|
||
| for (const v of sigPred.violations) { | ||
| const node = stmtNode.get(v.name, v.file, v.line); | ||
| const role = node?.role || 'unknown'; | ||
|
|
||
| let callerCount = 0; | ||
| if (node) { | ||
| const visited = new Set([node.id]); | ||
| let frontier = [node.id]; | ||
| for (let d = 0; d < 3; d++) { | ||
| const next = []; | ||
| for (const fid of frontier) { | ||
| for (const c of stmtCallers.all(fid)) { | ||
| if (!visited.has(c.id)) { | ||
| visited.add(c.id); | ||
| next.push(c.id); | ||
| callerCount++; | ||
| } | ||
| } | ||
| } | ||
| frontier = next; | ||
| if (!frontier.length) break; | ||
| } | ||
| } | ||
|
|
||
| const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'LOW'; | ||
| lines.push(risk + ': ' + v.name + ' (' + v.kind + ') [' + role + '] at ' + v.file + ':' + v.line + ' — ' + callerCount + ' transitive callers'); | ||
| } | ||
| } finally { | ||
| db.close(); | ||
| } | ||
|
|
||
| if (lines.length > 0) { | ||
| output.sigWarning = lines.join('\n'); | ||
| } |
There was a problem hiding this comment.
Signature warning not scoped to session-edited files
The cycle check (lines 68–84) explicitly filters to cycles that involve files from the session edit log. The signature warning block here has no equivalent filter — it processes sigPred.violations unconditionally for all staged files, regardless of whether you authored those changes.
In practice this means that after a git merge or git cherry-pick, any signature changes introduced by a teammate will also fire warnings, even though the current session author has no context about them. The cycle check avoids this noise by scoping to edited; the sig check should too:
// ── Signature warning (informational, never blocks) ──
const sigPred = data.predicates.find(p => p.name === 'signatures');
if (sigPred && !sigPred.passed && sigPred.violations?.length) {
const edited = new Set(editedRaw.split('\n').filter(Boolean));
// Scope to violations in session-edited files only
const scopedViolations = edited.size > 0
? sigPred.violations.filter(v => edited.has(v.file))
: sigPred.violations;
if (scopedViolations.length === 0) {
// nothing to do
} else {
const db = openReadonlyOrFail();
...
}
}| if [ "$ACTION" = "deny" ]; then | ||
| REASON=$(echo "$RESULT" | node -e "let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{process.stdout.write(JSON.parse(d).reason||'')}catch{}})" 2>/dev/null) || true | ||
| node -e " | ||
| console.log(JSON.stringify({ | ||
| hookSpecificOutput: { | ||
| hookEventName: 'PreToolUse', | ||
| permissionDecision: 'deny', | ||
| permissionDecisionReason: process.argv[1] | ||
| } | ||
| })); | ||
| " "$REASON" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Signature warnings silently dropped when a cycle block also fires
When ACTION === 'deny', the script emits the deny JSON and exits without ever reaching the SIG_WARNING extraction block (lines 159–174). If the same commit has both cycle violations and signature changes, the developer sees only the cycle error and loses all visibility into the signature risk information that would otherwise have been surfaced.
Consider including any sigWarning payload in the permissionDecisionReason of the deny response so it isn't silently discarded:
output.action = 'deny';
output.reason = 'BLOCKED: Circular dependencies detected ...\n'
+ (output.sigWarning ? '\n--- Signature warnings ---\n' + output.sigWarning : '');| fi | ||
|
|
||
| # Also check --body-file path | ||
| BODY_FILE=$(echo "$cmd" | grep -oP '(?<=--body-file\s)\S+' || true) |
There was a problem hiding this comment.
\S+ silently truncates --body-file paths that contain spaces
The lookbehind regex (?<=--body-file\s)\S+ matches only non-whitespace characters after --body-file, so a quoted path like --body-file "/home/user name/tmp/body.md" would yield /home/user — a path that almost certainly doesn't exist. When the resolved path doesn't exist, the [ -f "$BODY_FILE" ] guard silently skips the file check, leaving the guard ineffective for that invocation.
A more robust extraction handles optional quotes:
BODY_FILE=$(echo "$cmd" | grep -oP '(?<=--body-file\s)["\x27]?\S+["\x27]?' | tr -d "'\"" || true)Or, using a POSIX-safe sed approach:
BODY_FILE=$(echo "$cmd" | sed -nE "s/.*--body-file[[:space:]]+'?\"?([^'\" ]+).*/\1/p" || true)
|
Claude finished @carlos-alm's task —— View job PR Review: Pre-commit Hooks ImplementationTodo List:
Root Cause AnalysisProblem Being Solved: Adding pre-commit validation to catch circular dependencies, dead exports, and improper PR descriptions before they enter the repository. Why This Approach: Leverages codegraph's existing validation predicates with performance optimizations. The session-scoped validation ensures hooks only check files the AI agent actually modified. Risk Assessment: LOW - Defensive implementation with proper error handling and guard clauses. Hook Implementation ReviewSecurity Analysis: Fixed Issues from Previous Reviews: Performance Claims ValidationSingle checkData() call: ✅ Confirmed - Line 58-62 in check-commit.sh runs both cycles and signatures predicates in one call instead of two separate calls. Batched dead export checks: ✅ Confirmed - Lines 65-89 in check-dead-exports.sh processes all files in a single Node.js invocation instead of spawning separate processes. Performance improvement estimates appear valid based on the architectural changes. Configuration and Documentation Review✅ Hook registration: All three hooks properly added to
✅ Documentation updated: ✅ CLI improvement: Minor enhancement in Greptile's Remaining Concerns Assessment🟡 Minor Issues (Non-blocking):
✅ Previously Critical Issues Now Fixed:
Final Recommendation
Minor suggestions: The redundant check and comment inaccuracies are style issues that don't affect functionality and can be addressed in future iterations if desired. |
1 participant
Summary
check-commit.sh: combined cycle detection (blocking) + signature change warning (informational) in a singlecheckData()call — no duplicate predicate runscheck-dead-exports.sh: blocks commits when session-edited src/ files have zero-consumer exports — batches all files in one Node.js processguard-pr-body.sh: blocks PR creation with "generated with" in body.claude/settings.jsonwith appropriate timeoutsdocs/guides/recommended-practices.mdPerformance (vs PR #376)
checkData()calls per commitBoundaries predicate is skipped during commit hooks (rarely needed, expensive).
Splits from #376
Extracted non-hook changes to focused PRs:
CORE_SYMBOL_KINDSin flow matchingTest plan
check-commit.shblocks on cycles involving session-edited filescheck-commit.shwarns (non-blocking) on signature changes with risk levelscheck-dead-exports.shblocks on zero-consumer exportsguard-pr-body.shblocks PRs with "generated with"