fix: resolve CI failures and address CodeRabbit review feedback

- Fix pre-commit issues: formatting, import ordering, trailing whitespace
- Convert webhook handlers to class-based pattern for consistency
- Fix fail-open bug: TestCoverageCondition now returns violation on invalid regex
- Fix inconsistent extension filtering between evaluate() and validate()
- Fix max_hours=0 edge case in CommentResponseTimeCondition (falsy check)
- Refactor DiffPattern/SecurityPattern into shared _PatchPatternCondition base
- Remove redundant validate() overrides that duplicate BaseCondition logic

Author: dkargatzis

src/main.py

@@ -25,8 +25,8 @@
 from src.webhooks.handlers.deployment_status import DeploymentStatusEventHandler
 from src.webhooks.handlers.issue_comment import IssueCommentEventHandler
 from src.webhooks.handlers.pull_request import PullRequestEventHandler
-from src.webhooks.handlers.pull_request_review import handle_pull_request_review
-from src.webhooks.handlers.pull_request_review_thread import handle_pull_request_review_thread
+from src.webhooks.handlers.pull_request_review import PullRequestReviewEventHandler
+from src.webhooks.handlers.pull_request_review_thread import PullRequestReviewThreadEventHandler
 from src.webhooks.handlers.push import PushEventHandler
 from src.webhooks.router import router as webhook_router
 
@@ -75,9 +75,12 @@ async def lifespan(_app: FastAPI) -> Any:
     deployment_review_handler = DeploymentReviewEventHandler()
     deployment_protection_rule_handler = DeploymentProtectionRuleEventHandler()
 
+    pull_request_review_handler = PullRequestReviewEventHandler()
+    pull_request_review_thread_handler = PullRequestReviewThreadEventHandler()
+
     dispatcher.register_handler(EventType.PULL_REQUEST, pull_request_handler.handle)
-    dispatcher.register_handler(EventType.PULL_REQUEST_REVIEW, handle_pull_request_review)
-    dispatcher.register_handler(EventType.PULL_REQUEST_REVIEW_THREAD, handle_pull_request_review_thread)
+    dispatcher.register_handler(EventType.PULL_REQUEST_REVIEW, pull_request_review_handler.handle)
+    dispatcher.register_handler(EventType.PULL_REQUEST_REVIEW_THREAD, pull_request_review_thread_handler.handle)
     dispatcher.register_handler(EventType.PUSH, push_handler.handle)
     dispatcher.register_handler(EventType.CHECK_RUN, check_run_handler.handle)
     dispatcher.register_handler(EventType.ISSUE_COMMENT, issue_comment_handler.handle)

src/rules/conditions/access_control_advanced.py

@@ -8,6 +8,7 @@
 
 logger = logging.getLogger(__name__)
 
+
 class NoSelfApprovalCondition(BaseCondition):
     """Validates that a PR author cannot approve their own PR."""
 
@@ -34,7 +35,7 @@ async def evaluate(self, context: Any) -> list[Violation]:
         for review in reviews:
             review_state = review.get("state") if isinstance(review, dict) else getattr(review, "state", None)
             reviewer = review.get("author") if isinstance(review, dict) else getattr(review, "author", None)
-            
+
             if review_state == "APPROVED" and reviewer == author:
                 self_approved = True
                 break
@@ -51,10 +52,6 @@ async def evaluate(self, context: Any) -> list[Violation]:
 
         return []
 
-    async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool:
-        violations = await self.evaluate({"parameters": parameters, "event": event})
-        return len(violations) == 0
-
 
 class CrossTeamApprovalCondition(BaseCondition):
     """Validates that a PR has approvals from specific teams."""
@@ -74,19 +71,19 @@ async def evaluate(self, context: Any) -> list[Violation]:
             return []
 
         reviews = event.get("reviews", [])
-        
+
         # In a real implementation, we would map reviewers to their GitHub Teams
         # For now, we simulate this by checking if the required teams are in the requested_teams list
         # and if we have enough total approvals. A robust implementation would need a GraphQL call
         # to fetch user team memberships.
-        
+
         pr_details = event.get("pull_request_details", {})
         requested_teams = pr_details.get("requested_teams", [])
         requested_team_slugs = [t.get("slug") for t in requested_teams if t.get("slug")]
-        
+
         missing_teams = []
         for req_team in required_teams:
-            clean_team = req_team.replace("@", "").split("/")[-1] # Clean org/team to just team
+            clean_team = req_team.replace("@", "").split("/")[-1]  # Clean org/team to just team
             if clean_team in requested_team_slugs:
                 # Team was requested, now check if anyone approved (simplified check)
                 has_approval = any(
@@ -110,7 +107,3 @@ async def evaluate(self, context: Any) -> list[Violation]:
             ]
 
         return []
-
-    async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool:
-        violations = await self.evaluate({"parameters": parameters, "event": event})
-        return len(violations) == 0

src/rules/conditions/compliance.py

@@ -8,6 +8,7 @@
 
 logger = logging.getLogger(__name__)
 
+
 class SignedCommitsCondition(BaseCondition):
     """Validates that all commits in a PR are cryptographically signed."""
 
@@ -34,9 +35,15 @@ async def evaluate(self, context: Any) -> list[Violation]:
         unsigned_shas = []
         for commit in commits:
             # We will need to update the GraphQL query to fetch verificationStatus
-            is_verified = commit.get("is_verified", False) if isinstance(commit, dict) else getattr(commit, "is_verified", False)
+            is_verified = (
+                commit.get("is_verified", False) if isinstance(commit, dict) else getattr(commit, "is_verified", False)
+            )
             if not is_verified:
-                sha = str(commit.get("oid", "unknown")) if isinstance(commit, dict) else str(getattr(commit, "oid", "unknown"))
+                sha = (
+                    str(commit.get("oid", "unknown"))
+                    if isinstance(commit, dict)
+                    else str(getattr(commit, "oid", "unknown"))
+                )
                 unsigned_shas.append(sha[:7])
 
         if unsigned_shas:
@@ -51,10 +58,6 @@ async def evaluate(self, context: Any) -> list[Violation]:
 
         return []
 
-    async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool:
-        violations = await self.evaluate({"parameters": parameters, "event": event})
-        return len(violations) == 0
-
 
 class ChangelogRequiredCondition(BaseCondition):
     """Validates that a CHANGELOG update is included if source files are modified."""
@@ -83,7 +86,7 @@ async def evaluate(self, context: Any) -> list[Violation]:
             filename = f.get("filename", "")
             if not filename:
                 continue
-                
+
             # Check if it's a changelog file
             if "CHANGELOG" in filename.upper() or filename.startswith(".changeset/"):
                 changelog_changed = True
@@ -101,7 +104,3 @@ async def evaluate(self, context: Any) -> list[Violation]:
             ]
 
         return []
-
-    async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool:
-        violations = await self.evaluate({"parameters": parameters, "event": event})
-        return len(violations) == 0

src/rules/conditions/filesystem.py

@@ -310,7 +310,14 @@ async def evaluate(self, context: Any) -> list[Violation]:
             compiled_pattern = re.compile(test_pattern)
         except re.error:
             logger.error(f"Invalid test_file_pattern regex: {test_pattern}")
-            return []
+            return [
+                Violation(
+                    rule_description=self.description,
+                    severity=Severity.MEDIUM,
+                    message=f"Invalid test_file_pattern regex: '{test_pattern}'",
+                    how_to_fix="Fix the regular expression in the 'test_file_pattern' parameter.",
+                )
+            ]
 
         for f in changed_files:
             filename = f.get("filename", "")
@@ -355,14 +362,14 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b
         try:
             compiled_pattern = re.compile(test_pattern)
         except re.error:
-            return True
+            return False
 
         source_modified = False
         test_modified = False
 
         for f in changed_files:
             filename = f.get("filename", "")
-            if not filename or filename.endswith(".md") or filename.endswith(".yaml"):
+            if not filename or filename.endswith((".md", ".txt", ".yaml", ".json")):
                 continue
 
             if compiled_pattern.search(filename):

src/rules/conditions/pull_request.py

@@ -367,22 +367,30 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b
         return bool(_ISSUE_REF_PATTERN.search(combined))
 
 
-class DiffPatternCondition(BaseCondition):
-    """Validates that a PR diff does not contain specified restricted patterns."""
+class _PatchPatternCondition(BaseCondition):
+    """Base class for conditions that match regex patterns against PR diff patches.
 
-    name = "diff_pattern"
-    description = "Checks if code changes contain restricted patterns or fail to contain required patterns."
-    parameter_patterns = ["diff_restricted_patterns"]
-    event_types = ["pull_request"]
-    examples = [{"diff_restricted_patterns": ["console\\.log", "TODO:"]}]
+    Subclasses configure the parameter key, violation severity, and message format.
+    """
+
+    _pattern_param_key: str = ""
+    _violation_severity: Severity = Severity.MEDIUM
+
+    def _make_message(self, matched: list[str], filename: str) -> str:
+        """Return the violation message. Override for custom wording."""
+        return f"Patterns {matched} found in added lines of {filename}"
+
+    def _make_how_to_fix(self) -> str:
+        """Return the how_to_fix text. Override for custom wording."""
+        return "Remove the matched patterns from your code changes."
 
     async def evaluate(self, context: Any) -> list[Violation]:
-        """Evaluate diff-pattern condition."""
+        """Evaluate patch-pattern condition."""
         parameters = context.get("parameters", {})
         event = context.get("event", {})
 
-        restricted_patterns = parameters.get("diff_restricted_patterns")
-        if not restricted_patterns or not isinstance(restricted_patterns, list):
+        patterns = parameters.get(self._pattern_param_key)
+        if not patterns or not isinstance(patterns, list):
             return []
 
         changed_files = event.get("changed_files", [])
@@ -397,98 +405,73 @@ async def evaluate(self, context: Any) -> list[Violation]:
             if not patch:
                 continue
 
-            matched = match_patterns_in_patch(patch, restricted_patterns)
+            matched = match_patterns_in_patch(patch, patterns)
             if matched:
                 filename = file_info.get("filename", "unknown")
                 violations.append(
                     Violation(
                         rule_description=self.description,
-                        severity=Severity.MEDIUM,
-                        message=f"Restricted patterns {matched} found in added lines of {filename}",
-                        how_to_fix="Remove the restricted patterns from your code changes.",
+                        severity=self._violation_severity,
+                        message=self._make_message(matched, filename),
+                        how_to_fix=self._make_how_to_fix(),
                     )
                 )
 
         return violations
 
     async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool:
         """Legacy validation interface."""
-        restricted_patterns = parameters.get("diff_restricted_patterns")
-        if not restricted_patterns or not isinstance(restricted_patterns, list):
+        patterns = parameters.get(self._pattern_param_key)
+        if not patterns or not isinstance(patterns, list):
             return True
 
         changed_files = event.get("changed_files", [])
         from src.rules.utils.diff import match_patterns_in_patch
 
         for file_info in changed_files:
             patch = file_info.get("patch")
-            if patch and match_patterns_in_patch(patch, restricted_patterns):
+            if patch and match_patterns_in_patch(patch, patterns):
                 return False
 
         return True
 
 
-class SecurityPatternCondition(BaseCondition):
-    """Detects security-sensitive patterns (like API keys) in code changes."""
+class DiffPatternCondition(_PatchPatternCondition):
+    """Validates that a PR diff does not contain specified restricted patterns."""
 
-    name = "security_pattern"
-    description = "Detects hardcoded secrets, API keys, or sensitive data in PR diffs."
-    parameter_patterns = ["security_patterns"]
+    name = "diff_pattern"
+    description = "Checks if code changes contain restricted patterns or fail to contain required patterns."
+    parameter_patterns = ["diff_restricted_patterns"]
     event_types = ["pull_request"]
-    examples = [{"security_patterns": ["api_key", "secret", "password", "token"]}]
-
-    async def evaluate(self, context: Any) -> list[Violation]:
-        """Evaluate security-pattern condition."""
-        parameters = context.get("parameters", {})
-        event = context.get("event", {})
-
-        security_patterns = parameters.get("security_patterns")
-        if not security_patterns or not isinstance(security_patterns, list):
-            return []
+    examples = [{"diff_restricted_patterns": ["console\\.log", "TODO:"]}]
 
-        changed_files = event.get("changed_files", [])
-        if not changed_files:
-            return []
+    _pattern_param_key = "diff_restricted_patterns"
+    _violation_severity = Severity.MEDIUM
 
-        from src.rules.utils.diff import match_patterns_in_patch
+    def _make_message(self, matched: list[str], filename: str) -> str:
+        return f"Restricted patterns {matched} found in added lines of {filename}"
 
-        violations = []
-        for file_info in changed_files:
-            patch = file_info.get("patch")
-            if not patch:
-                continue
+    def _make_how_to_fix(self) -> str:
+        return "Remove the restricted patterns from your code changes."
 
-            # In a real scenario, this would use a more robust secrets scanner.
-            # Here we just use the diff matcher with the provided regex/string patterns.
-            matched = match_patterns_in_patch(patch, security_patterns)
-            if matched:
-                filename = file_info.get("filename", "unknown")
-                violations.append(
-                    Violation(
-                        rule_description=self.description,
-                        severity=Severity.CRITICAL,
-                        message=f"Security-sensitive patterns {matched} detected in {filename}",
-                        how_to_fix="Remove hardcoded secrets or sensitive patterns from the code.",
-                    )
-                )
 
-        return violations
+class SecurityPatternCondition(_PatchPatternCondition):
+    """Detects security-sensitive patterns (like API keys) in code changes."""
 
-    async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> bool:
-        """Legacy validation interface."""
-        security_patterns = parameters.get("security_patterns")
-        if not security_patterns or not isinstance(security_patterns, list):
-            return True
+    name = "security_pattern"
+    description = "Detects hardcoded secrets, API keys, or sensitive data in PR diffs."
+    parameter_patterns = ["security_patterns"]
+    event_types = ["pull_request"]
+    examples = [{"security_patterns": ["api_key", "secret", "password", "token"]}]
 
-        changed_files = event.get("changed_files", [])
-        from src.rules.utils.diff import match_patterns_in_patch
+    _pattern_param_key = "security_patterns"
+    _violation_severity = Severity.CRITICAL
 
-        for file_info in changed_files:
-            patch = file_info.get("patch")
-            if patch and match_patterns_in_patch(patch, security_patterns):
-                return False
+    def _make_message(self, matched: list[str], filename: str) -> str:
+        return f"Security-sensitive patterns {matched} detected in {filename}"
 
-        return True
+    def _make_how_to_fix(self) -> str:
+        return "Remove hardcoded secrets or sensitive patterns from the code."
 
 
 class UnresolvedCommentsCondition(BaseCondition):

src/rules/conditions/temporal.py

@@ -261,7 +261,7 @@ async def evaluate(self, context: Any) -> list[Violation]:
         event = context.get("event", {})
 
         max_hours = parameters.get("max_comment_response_time_hours")
-        if not max_hours:
+        if max_hours is None:
             return []
 
         review_threads = event.get("review_threads", [])