Skip to content

Conversation

Helmi
Copy link
Owner

@Helmi Helmi commented Sep 20, 2025

Summary

Implements intelligent notification filtering to solve notification fatigue from excessive bash command alerts. After several days of testing, the filtering significantly reduces noise while preserving important notifications.

Closes #14

Changes Made

Core Filtering Logic

  • Minimal dangerous command whitelist: Only rm, sudo, chmod, chown, mv trigger notifications
  • Expanded safe commands: Added cp, curl, wget (moved from dangerous), plus git commands and development tools
  • Path-based filtering: Commands targeting files outside project directory trigger notifications with special messaging

Enhanced Messaging

  • System files: "Accessing system file outside project" for /etc/, /usr/, /var/
  • External projects: "Accessing file outside project" for other code directories
  • Improved TTS: Better command replacement using regex word boundaries

Technical Details

  • Added is_command_targeting_outside_project() function for external file detection
  • Improved apply_command_replacement() with regex word boundary matching
  • Maintains all existing notification types (task completion, errors, input requests)
  • Only filters routine bash command noise

Testing Results

Significant noise reduction - Routine commands (ls, git status, npm run) now silent
Safety preserved - Dangerous operations (rm, sudo) still notify
External file detection - Commands targeting system files properly flagged
No false negatives - All important events still captured

Files Modified

  • src/ccnotify/notify.py - Core filtering and messaging logic

Breaking Changes

None - purely additive filtering that reduces notifications without removing important ones.

- Add minimal whitelist for dangerous commands (rm, sudo, chmod, chown, mv)
- Move cp, curl, wget from dangerous to safe commands list
- Add git commands and development tools to safe commands
- Implement path-based filtering for external file operations
- Add special messaging for system files and external projects
- Improve apply_command_replacement with regex word boundaries

Closes #14
Copy link

claude bot commented Sep 20, 2025

Claude finished @Helmi's task —— View job


Code Review Complete ✅

I've thoroughly reviewed the smart notification filtering implementation. Here's my detailed feedback:

Todo List:

  • Read and analyze the modified file (src/ccnotify/notify.py)
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Examine test coverage
  • Provide comprehensive feedback

🎯 Overall Assessment

This PR implements a thoughtful solution to notification fatigue by intelligently filtering bash command notifications. The implementation is well-designed with good separation of concerns.

✅ Strengths

Code Quality & Architecture:

  • Clean separation of filtering logic with dedicated helper functions
  • Good use of regex word boundaries in apply_command_replacement() (src/ccnotify/notify.py:656)
  • Sensible default configuration with minimal dangerous command whitelist
  • Well-structured external file detection logic

Security & Safety:

  • Maintains notifications for genuinely dangerous operations (rm, sudo, chmod, chown, mv)
  • Proper external file detection for system directories (/etc/, /usr/, /var/)
  • Path resolution with proper exception handling

⚠️ Issues Found

1. Path Traversal Vulnerability Risk src/ccnotify/notify.py:331

target_path = Path(part).resolve()

The use of Path.resolve() without validation could potentially be exploited with malicious path inputs containing .. sequences. Consider adding path sanitization.

2. Command Injection Potential src/ccnotify/notify.py:314

cmd_parts = command.split()

Simple string splitting may not handle complex shell syntax (quotes, escapes) correctly. Consider using shlex.split() for proper shell command parsing.

3. Regex Performance Concern src/ccnotify/notify.py:644

match = re.search(pattern, command, re.IGNORECASE)

Pattern matching in a loop without compilation could impact performance with many patterns. Consider pre-compiling regex patterns.

4. Exception Handling Too Broad src/ccnotify/notify.py:355

except Exception:
    return False, None

Catches all exceptions including system errors that should be handled differently. Be more specific about expected exception types.

5. Missing Input Validation src/ccnotify/notify.py:302
The is_command_targeting_outside_project() function doesn't validate the command parameter for None/empty values before processing.

🚀 Performance Considerations

Positive:

  • Efficient early returns for safe commands
  • Good caching strategy for project name resolution
  • Minimal overhead for filtered notifications

Suggestions:

  • Consider caching compiled regex patterns
  • The path resolution in a loop could be optimized for commands with many arguments

🔒 Security Assessment

Good practices:

  • Conservative dangerous command whitelist
  • System file detection
  • Proper path resolution

Recommendations:

  • Add input sanitization for path operations
  • Consider rate limiting for notification generation
  • Validate command strings before processing

🧪 Test Coverage

Missing Areas:

  • No visible test coverage for the new filtering logic
  • Edge cases like malformed commands, path traversal attempts
  • Integration tests for external file detection
  • Command replacement regex boundary matching

Suggested Test Cases:

def test_dangerous_command_detection():
    # Test rm, sudo, chmod detection
    
def test_external_file_filtering():
    # Test /etc/, /usr/ path detection
    
def test_command_injection_prevention():
    # Test malformed/malicious commands
    
def test_regex_word_boundaries():
    # Test improved command replacement

📝 Minor Improvements

  1. Magic Numbers: Consider defining constants for cache expiry (line 266) and path validation
  2. Documentation: Add docstring examples for the new filtering functions
  3. Type Hints: The return type tuple[bool, Optional[str]] should be Tuple[bool, Optional[str]] for older Python versions
  4. Logging: Add more debug logging for filtered commands to aid troubleshooting

🎯 Recommendations

  1. High Priority: Address the path traversal and command injection concerns
  2. Medium Priority: Add comprehensive test coverage for new filtering logic
  3. Low Priority: Performance optimizations for regex compilation

The core concept and implementation are solid - this will significantly reduce notification noise while maintaining safety. The security concerns are addressable and don't affect the core functionality.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement smart notification filtering to reduce notification fatigue
1 participant