fix: address CodeRabbit review - validate agent responses and fail closed

Fix 6 critical/major issues found by CodeRabbit:

1. check_run.py - Validate agent response success:
   - Check result.success before reading violations
   - Return failed ProcessingResult on agent failure
   - Handle dict-backed rules in _convert_rules_to_new_format

2. deployment_protection_rule.py - Fail closed on agent failures:
   - Strengthen URL validation: require https://api.github.com
   - Validate agent response before auto-approving deployments
   - Reject deployment (fail closed) when agent fails instead of auto-approving

3. deployment_review.py - Handle edge cases:
   - Validate agent response success before acting on data
   - Handle dict-backed rules in _convert_rules_to_new_format
   - Return failed ProcessingResult on agent failure

These fixes ensure:
- Agent failures don't silently pass (previously: empty result = pass)
- Invalid deployments are rejected, not auto-approved
- Dict-backed rules from loaders don't crash with AttributeError
- URL validation requires proper HTTPS GitHub API endpoints

Author: dkargatzis

src/event_processors/check_run.py

@@ -88,6 +88,25 @@ async def process(self, task: Task) -> ProcessingResult:
             rules=formatted_rules,
         )
 
+        # Validate agent response before acting on data
+        if not result.success or not result.data or "violations" not in result.data:
+            logger.error(
+                "check_run_agent_failure",
+                extra={
+                    "operation": "check_run",
+                    "agent_success": result.success,
+                    "has_data": bool(result.data),
+                    "has_violations": "violations" in result.data if result.data else False,
+                },
+            )
+            return ProcessingResult(
+                success=False,
+                violations=[],
+                api_calls_made=1,
+                processing_time_ms=int((time.time() - start_time) * 1000),
+                error="Agent execution failed or returned invalid response",
+            )
+
         violations = result.data.get("violations", [])
 
         logger.info("=" * 80)
@@ -103,23 +122,33 @@ async def process(self, task: Task) -> ProcessingResult:
         )
 
     def _convert_rules_to_new_format(self, rules: list[Any]) -> list[dict[str, Any]]:
-        """Convert Rule objects to the new flat schema format."""
+        """Convert Rule objects or dicts to the new flat schema format."""
         formatted_rules = []
 
         for rule in rules:
-            # Convert Rule object to dict format
-            rule_dict = {
-                "description": rule.description,
-                "enabled": rule.enabled,
-                "severity": rule.severity.value if hasattr(rule.severity, "value") else rule.severity,
-                "event_types": [et.value if hasattr(et, "value") else et for et in rule.event_types],
-                "parameters": rule.parameters if hasattr(rule, "parameters") else {},
-            }
-
-            # If no parameters field, try to extract from conditions (backward compatibility)
-            if not rule_dict["parameters"] and hasattr(rule, "conditions"):
-                for condition in rule.conditions:
-                    rule_dict["parameters"].update(condition.parameters)
+            # Handle both Rule objects and dict-backed rules
+            if isinstance(rule, dict):
+                rule_dict = {
+                    "description": rule.get("description", ""),
+                    "enabled": rule.get("enabled", True),
+                    "severity": rule.get("severity", "medium"),
+                    "event_types": rule.get("event_types", []),
+                    "parameters": rule.get("parameters", {}),
+                }
+            else:
+                # Convert Rule object to dict format
+                rule_dict = {
+                    "description": rule.description,
+                    "enabled": rule.enabled,
+                    "severity": rule.severity.value if hasattr(rule.severity, "value") else rule.severity,
+                    "event_types": [et.value if hasattr(et, "value") else et for et in rule.event_types],
+                    "parameters": rule.parameters if hasattr(rule, "parameters") else {},
+                }
+
+                # If no parameters field, try to extract from conditions (backward compatibility)
+                if not rule_dict["parameters"] and hasattr(rule, "conditions"):
+                    for condition in rule.conditions:
+                        rule_dict["parameters"].update(condition.parameters)
 
             formatted_rules.append(rule_dict)
 

src/event_processors/deployment_protection_rule.py

@@ -31,10 +31,16 @@ def get_event_type(self) -> str:
 
     @staticmethod
     def _is_valid_callback_url(url: str | None) -> bool:
-        return bool(url and isinstance(url, str) and url.strip().startswith("http"))
+        """Validate callback URL is a proper GitHub API HTTPS endpoint."""
+        if not url or not isinstance(url, str):
+            return False
+        url = url.strip()
+        # Must be HTTPS and target GitHub API
+        return url.startswith("https://api.github.com/")
 
     @staticmethod
     def _is_valid_environment(env: str | None) -> bool:
+        """Validate environment name is present and non-empty."""
         return bool(env and isinstance(env, str) and env.strip())
 
     async def process(self, task: Task) -> ProcessingResult:
@@ -198,6 +204,33 @@ async def process(self, task: Task) -> ProcessingResult:
                 timeout_message=f"Agent execution timed out after {AGENT_TIMEOUT_SECONDS}s",
             )
 
+            # Validate agent response before acting on data
+            if not analysis_result.success or not analysis_result.data or "evaluation_result" not in analysis_result.data:
+                logger.error(
+                    "deployment_agent_failure",
+                    extra={
+                        "operation": "deployment_protection_rule",
+                        "agent_success": analysis_result.success,
+                        "has_data": bool(analysis_result.data),
+                        "has_evaluation_result": "evaluation_result" in analysis_result.data if analysis_result.data else False,
+                    },
+                )
+                # Fail closed: don't approve deployments when agent fails
+                if can_call_callback:
+                    await self._reject_deployment(
+                        deployment_callback_url,
+                        environment,
+                        [{"message": "Agent execution failed - deployment blocked for safety", "severity": "high"}],
+                        installation_id,
+                    )
+                return ProcessingResult(
+                    success=False,
+                    violations=[],
+                    api_calls_made=1,
+                    processing_time_ms=int((time.time() - start_time) * 1000),
+                    error="Agent execution failed or returned invalid response",
+                )
+
             violations = []
             if analysis_result.data and "evaluation_result" in analysis_result.data:
                 eval_result = analysis_result.data["evaluation_result"]

src/event_processors/deployment_review.py

@@ -118,6 +118,25 @@ async def process(self, task: Task) -> ProcessingResult:
             rules=formatted_rules,
         )
 
+        # Validate agent response before acting on data
+        if not result.success or not result.data or "violations" not in result.data:
+            logger.error(
+                "deployment_review_agent_failure",
+                extra={
+                    "operation": "deployment_review",
+                    "agent_success": result.success,
+                    "has_data": bool(result.data),
+                    "has_violations": "violations" in result.data if result.data else False,
+                },
+            )
+            return ProcessingResult(
+                success=False,
+                violations=[],
+                api_calls_made=1,
+                processing_time_ms=int((time.time() - start_time) * 1000),
+                error="Agent execution failed or returned invalid response",
+            )
+
         violations = result.data.get("violations", [])
 
         logger.info("=" * 80)
@@ -142,23 +161,33 @@ async def prepare_api_data(self, task: Task) -> dict[str, Any]:
 
     @staticmethod
     def _convert_rules_to_new_format(rules: list[Any]) -> list[dict[str, Any]]:
-        """Convert Rule objects to the new flat schema format."""
+        """Convert Rule objects or dicts to the new flat schema format."""
         formatted_rules = []
 
         for rule in rules:
-            # Convert Rule object to dict format
-            rule_dict = {
-                "description": rule.description,
-                "enabled": rule.enabled,
-                "severity": rule.severity.value if hasattr(rule.severity, "value") else rule.severity,
-                "event_types": [et.value if hasattr(et, "value") else et for et in rule.event_types],
-                "parameters": rule.parameters if hasattr(rule, "parameters") else {},
-            }
-
-            # If no parameters field, try to extract from conditions (backward compatibility)
-            if not rule_dict["parameters"] and hasattr(rule, "conditions"):
-                for condition in rule.conditions:
-                    rule_dict["parameters"].update(condition.parameters)
+            # Handle both Rule objects and dict-backed rules
+            if isinstance(rule, dict):
+                rule_dict = {
+                    "description": rule.get("description", ""),
+                    "enabled": rule.get("enabled", True),
+                    "severity": rule.get("severity", "medium"),
+                    "event_types": rule.get("event_types", []),
+                    "parameters": rule.get("parameters", {}),
+                }
+            else:
+                # Convert Rule object to dict format
+                rule_dict = {
+                    "description": rule.description,
+                    "enabled": rule.enabled,
+                    "severity": rule.severity.value if hasattr(rule.severity, "value") else rule.severity,
+                    "event_types": [et.value if hasattr(et, "value") else et for et in rule.event_types],
+                    "parameters": rule.parameters if hasattr(rule, "parameters") else {},
+                }
+
+                # If no parameters field, try to extract from conditions (backward compatibility)
+                if not rule_dict["parameters"] and hasattr(rule, "conditions"):
+                    for condition in rule.conditions:
+                        rule_dict["parameters"].update(condition.parameters)
 
             formatted_rules.append(rule_dict)