fix: prevent false error notifications for NotebookEdit and spurious input alerts

[Helmi]

Summary

• Fixed NotebookEdit operations always triggering false error notifications
• Improved input_needed notification logic to reduce false positives
• Added debug logging for better troubleshooting

Closes #7

Changes

NotebookEdit False Failures Fix

• Modified error detection logic in src/ccnotify/notify.py (lines 922-925)
• Changed from checking if "error" key exists to checking if error value is non-empty
• Empty error fields ("error": "") no longer trigger error notifications

Input Needed False Positives Fix

• Added keyword filtering to only trigger input_needed for actual permission requests
• Added debug logging to track what triggers notifications
• Non-permission notifications are now logged but not shown to user

Technical Details

The main issue was in the error detection logic:

# Before: triggers on empty strings
"error" in tool_response

# After: only triggers on non-empty errors  
tool_response.get("error") and tool_response.get("error") \!= ""

For input_needed notifications, added filtering for permission-related keywords:

• "permission to use"
• "permission needed"
• "needs your permission"
• "confirm", "approve", "allow"

Testing

• Built package successfully with uvx hatch build
• Code formatted with uvx black
• Changes preserve existing behavior for actual errors/permissions
• Debug logging added to help identify any remaining edge cases

Edge Cases Considered

• Multiple tool types tested for regression
• Empty vs null error fields handled
• Permission bypass scenarios considered

[claude]

Claude finished @Helmi's task —— View job

---

Code Review for PR #9

Excellent work on fixing the false notification issues! This PR addresses real problems with clear, well-targeted solutions.

Todo List

• Read and analyze the changed file (src/ccnotify/notify.py)
• Review NotebookEdit error detection changes
• Review input_needed notification filtering changes
• Check for code quality and best practices
• Identify potential bugs or edge cases
• Assess performance considerations
• Check for security concerns
• Evaluate test coverage needs
• Provide comprehensive feedback

---

✅ Overall Assessment: APPROVE

This PR implements targeted, well-reasoned fixes that address the specific issues described in #7 without introducing breaking changes.

Key Improvements

1. NotebookEdit Error Detection Fix (lines 922-925)

# Before: triggers on empty strings
"error" in tool_response

# After: only triggers on non-empty errors  
tool_response.get("error") and tool_response.get("error") != ""

✅ Excellent Fix: This prevents false positives from empty error fields while maintaining proper error detection. The logic is sound and handles both missing and empty error fields correctly.

2. Input Needed Filtering (lines 960-996)

The new keyword-based filtering is a smart approach:

permission_keywords = [
    "permission to use", "permission needed", "needs your permission",
    "confirm", "approve", "allow"
]

✅ Well-designed: This dramatically reduces false positives while catching legitimate permission requests. The fallback behavior (log but don't notify) is appropriate.

3. Enhanced Debug Logging

Added strategic logging at lines 952-955 and 993-996 to help diagnose future issues.

✅ Good practice: This will make troubleshooting much easier without being too verbose.

---

Code Quality Assessment

✅ Strengths

• Backward compatible: Changes preserve existing behavior for real errors/permissions
• Defensive coding: Uses .get() method with defaults throughout
• Clear intent: Code changes directly address the described problems
• Good logging: Strategic debug messages for troubleshooting
• Maintainable: Changes are localized and easy to understand

✅ Best Practices Followed

• Proper null/empty checking with tool_response.get("error") and tool_response.get("error") != ""
• Early returns to avoid nested conditionals
• Meaningful variable names (permission_keywords, raw_message)
• Consistent error handling patterns

---

Technical Analysis

Performance: ✅ No Concerns

• Keyword matching uses simple in operator on small list (6 items)
• Early return prevents unnecessary processing
• No performance regression expected

Security: ✅ No Issues

• No external input validation concerns
• Debug logging truncates messages ([:200], [:100]) to prevent log flooding
• No sensitive data exposure risks

Edge Cases: ✅ Well Handled

• Empty/null error fields: ✅ Handled with .get() and explicit empty string check
• Missing message fields: ✅ Protected with elif raw_message: check
• Case sensitivity: ✅ Uses .lower() for keyword matching
• Multiple keywords: ✅ Uses any() for proper OR logic

---

Minor Suggestions (Non-blocking)

1. Consider adding a constant for the keyword list:

   PERMISSION_KEYWORDS = [
       "permission to use", "permission needed", "needs your permission",
       "confirm", "approve", "allow"
   ]

   This would make it easier to maintain and test.

2. The error condition could be simplified:

   # Current
   tool_response.get("error") and tool_response.get("error") != ""
   
   # Alternative (slightly more concise)
   bool(tool_response.get("error", "").strip())

   Though the current version is more explicit about intent.

---

Test Coverage Recommendations

While the manual testing described (build + format) is good, consider adding unit tests for:

• Empty error field scenarios
• Various permission keyword combinations
• Non-permission notifications that should be filtered out
• Edge cases like None vs empty string vs missing fields

---

Verification Questions (Answered ✅)

• ✅ Does it fix the NotebookEdit false failures? Yes - empty error fields won't trigger notifications
• ✅ Does it reduce input_needed false positives? Yes - only permission-related messages trigger notifications
• ✅ Does it maintain compatibility? Yes - real errors and permissions still work
• ✅ Is the debugging improved? Yes - strategic logging added

This is a solid, well-implemented fix that directly addresses the reported issues. Great work! 🚀

---