Skip to content

chore: add pre-commit diff-impact hook#271

Merged
carlos-alm merged 1 commit intomainfrom
chore/diff-impact-hook
Mar 3, 2026
Merged

chore: add pre-commit diff-impact hook#271
carlos-alm merged 1 commit intomainfrom
chore/diff-impact-hook

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Adds show-diff-impact.sh hook that automatically runs codegraph diff-impact --staged -T before git commit commands
  • Injects blast radius summary as additionalContext — informational only, never blocks commits
  • Registers hook in .claude/settings.json after guard-git.sh with 15s timeout

Details

The hook:

  • Triggers on PreToolUse for Bash when the command matches git commit
  • Guards: skips silently if no .codegraph/graph.db exists, no staged changes, or empty output
  • JSON-escapes the diff-impact output and injects it with [codegraph diff-impact] prefix
  • Every failure path exits 0 gracefully — never interferes with commits

Test plan

  • Stage a file and run git commit via Claude Code — hook output should appear as context
  • Verify hook doesn't block — commit proceeds normally
  • Verify graceful skip when no graph DB exists
  • Verify graceful skip when no staged changes

Add show-diff-impact.sh that automatically runs
`codegraph diff-impact --staged -T` before git commit commands.
The hook injects blast radius info as additionalContext —
informational only, never blocks commits.
@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @carlos-alm's task —— View job


PR Review: Pre-commit diff-impact hook

Todo List:

  • Read and analyze changed files
  • Apply Phase 1: Root Cause Analysis
  • Apply Phase 2: Configuration Change Detection
  • Apply Phase 3: Test Integrity Check
  • Apply Phase 4: Code Quality Review
  • Provide final assessment and rating

Root Cause Analysis

Problem Being Solved: The PR addresses a workflow improvement need mentioned in CLAUDE.md - automatically providing blast radius context before commits. The instruction "After modifying code: node src/cli.js diff-impact --staged -T — verify impact before committing" suggests manual verification was required but not automated.

Why This Approach: The hook-based approach is sound for this use case:

  • Non-blocking informational context (never fails commits)
  • Leverages existing diff-impact command
  • Integrates seamlessly with Claude Code's hook system
  • Follows the established pattern of other hooks in the repo

Risk Assessment: LOW - The implementation has extensive guards and graceful failure modes.

Critical Concerns

None identified. The implementation follows security best practices and defensive programming patterns.

Configuration Change Assessment

PASS - No high-risk configuration changes detected. The settings.json modification simply registers the new hook in the existing framework with appropriate timeout (15s).

Test Integrity Assessment

PASS - No test files modified, no assertions weakened, no coverage thresholds changed.

Code Quality Review

Strengths

  • Excellent defensive programming: Multiple guard clauses prevent execution when inappropriate
  • Proper JSON escaping: Uses Node.js JSON.stringify to safely escape output
  • Graceful error handling: All operations use || true and exit 0 on failure
  • Command detection: Robust regex pattern (^|\s|&&\s*)git\s+commit\b catches various git commit invocations
  • Respects existing patterns: Follows same structure as other hooks in the repo

Technical Implementation Details

  • Line 11-18: Safe JSON parsing with proper error handling
  • Line 25: Regex correctly handles command boundaries and chained commands
  • Line 30: Proper git root detection with fallback
  • Line 42: Uses the project's own CLI tool appropriately
  • Line 49-53: Clean JSON escaping without shell injection risks

Minor Observations

  • Line 42: Hardcoded path to src/cli.js - but this is appropriate for a repo-specific hook
  • Timeout set to 15s is reasonable for the diff-impact operation
  • Hook placement after guard-git.sh is logical (security checks first, then informational)

Security Assessment

  • No command injection vectors: All dynamic content properly escaped
  • No file system vulnerabilities: Proper path handling with git root detection
  • No privilege escalation risks: Script runs with user permissions only
  • Input validation: JSON parsing is protected with error handling

Final Assessment

## Root Cause Analysis
**Problem Being Solved**: Automates the manual step in CLAUDE.md workflow: "verify impact before committing"
**Why This Approach**: Hook-based automation provides seamless integration without blocking commits
**Risk Assessment**: LOW - Extensive defensive programming and graceful failure modes

## Critical Concerns
None - Implementation follows security best practices and defensive patterns

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: APPROVE
- Reasoning: Well-implemented workflow automation that solves a real problem. Excellent defensive programming, proper security practices, and seamless integration with existing hook system. Only minor consideration is that this adds some complexity to the commit workflow, but the non-blocking nature and clear benefits outweigh this concern.

Recommendation

This PR demonstrates excellent engineering practices and addresses a legitimate workflow improvement need. The implementation is secure, defensive, and well-integrated. APPROVED.


@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Greptile Summary

Adds an informational pre-commit hook that automatically runs codegraph diff-impact --staged -T and injects the blast radius summary as additional context before git commits - cleanly implemented with multiple safety guards to skip gracefully when the graph DB doesn't exist, no staged changes are present, or output is empty, ensuring it never interferes with the commit process.

Confidence Score: 5/5

  • Safe to merge - informational hook that never blocks commits
  • Well-designed implementation with proper guards, consistent with existing hook patterns, all failure paths exit gracefully, no security concerns
  • No files require special attention

Important Files Changed

Filename Overview
.claude/hooks/show-diff-impact.sh New pre-commit hook that injects codegraph diff-impact output as informational context - well-guarded with multiple safety checks, never blocks commits
.claude/settings.json Registers show-diff-impact.sh hook after guard-git.sh with appropriate 15s timeout

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PreToolUse: Bash command] --> B{Extract command<br/>from JSON}
    B -->|Success| C{Is git commit<br/>command?}
    B -->|Fail| Z[Exit 0: Allow]
    C -->|No| Z
    C -->|Yes| D{.codegraph/graph.db<br/>exists?}
    D -->|No| Z
    D -->|Yes| E{Has staged<br/>changes?}
    E -->|No| Z
    E -->|Yes| F[Run: codegraph diff-impact<br/>--staged -T]
    F --> G{Output<br/>empty?}
    G -->|Yes| Z
    G -->|No| H[JSON escape<br/>output]
    H --> I{Escape<br/>success?}
    I -->|No| Z
    I -->|Yes| J[Inject as additionalContext<br/>with allow decision]
    J --> Z
Loading

Last reviewed commit: bb86db7

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@carlos-alm carlos-alm merged commit 9b7c6e1 into main Mar 3, 2026
18 checks passed
@carlos-alm carlos-alm deleted the chore/diff-impact-hook branch March 3, 2026 03:06
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant