From c7c221243c7786974a051afa97c4f87934360ff4 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 18 Nov 2025 14:35:25 +0200 Subject: [PATCH 1/4] feat(mcp): add ENABLE_MCP_SERVER environment variable check Make MCP server endpoints conditional based on ENABLE_MCP_SERVER=true environment variable, matching the pattern used for log server endpoints. Changes: - Add MCP_SERVER_ENABLED constant for environment variable check - Wrap entire MCP initialization block in conditional - Add security warning about network exposure and authentication - Follow DRY principle by using constant instead of duplicate env check Pattern consistency with log server implementation ensures maintainable and secure deployment options. --- webhook_server/app.py | 79 ++++++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/webhook_server/app.py b/webhook_server/app.py index beac2a2c..341d8fff 100644 --- a/webhook_server/app.py +++ b/webhook_server/app.py @@ -45,6 +45,7 @@ # Constants APP_URL_ROOT_PATH: str = "/webhook_server" LOG_SERVER_ENABLED: bool = os.environ.get("ENABLE_LOG_SERVER") == "true" +MCP_SERVER_ENABLED: bool = os.environ.get("ENABLE_MCP_SERVER") == "true" # Global variables ALLOWED_IPS: tuple[ipaddress._BaseNetwork, ...] = () @@ -389,7 +390,7 @@ def get_log_viewer_controller() -> LogViewerController: # Log Viewer Endpoints - Only register if ENABLE_LOG_SERVER=true -if os.environ.get("ENABLE_LOG_SERVER") == "true": +if LOG_SERVER_ENABLED: @FASTAPI_APP.get("/logs", operation_id="get_log_viewer_page", response_class=HTMLResponse) def get_log_viewer_page(controller: LogViewerController = controller_dependency) -> HTMLResponse: @@ -1051,44 +1052,46 @@ async def websocket_log_stream( ) -# Create MCP instance with the main app -# NOTE: No authentication configured - MCP server runs without auth -mcp = FastApiMCP(FASTAPI_APP, exclude_tags=["mcp_exclude"]) - -# Create stateless HTTP transport to avoid session management issues -# Override with stateless session manager -http_transport = FastApiHttpSessionManager( - mcp_server=mcp.server, - event_store=None, # No event store needed for stateless mode - json_response=True, -) -# Manually patch to use stateless mode -http_transport._session_manager = None # Force recreation with stateless=True - - -# Register the HTTP endpoint manually -@FASTAPI_APP.api_route("/mcp", methods=["GET", "POST", "DELETE"], include_in_schema=False, operation_id="mcp_http") -async def handle_mcp_streamable_http(request: Request) -> Response: - # Ensure session manager is created with stateless=True - if http_transport._session_manager is None: - http_transport._session_manager = StreamableHTTPSessionManager( - app=mcp.server, - event_store=http_transport.event_store, - json_response=True, - stateless=True, # Enable stateless mode - no session management required - ) - # Start the session manager - - async def run_manager() -> None: - async with http_transport._session_manager.run(): - await asyncio.Event().wait() +# MCP Integration - Only register if ENABLE_MCP_SERVER=true +if MCP_SERVER_ENABLED: + # Create MCP instance with the main app + # NOTE: No authentication configured - MCP server runs without auth + # โš ๏ธ SECURITY WARNING: Deploy only on trusted networks (VPN, internal) + # Never expose to public internet - use reverse proxy with auth for external access + mcp = FastApiMCP(FASTAPI_APP, exclude_tags=["mcp_exclude"]) + + # Create stateless HTTP transport to avoid session management issues + # Override with stateless session manager + http_transport = FastApiHttpSessionManager( + mcp_server=mcp.server, + event_store=None, # No event store needed for stateless mode + json_response=True, + ) + # Manually patch to use stateless mode + http_transport._session_manager = None # Force recreation with stateless=True + + # Register the HTTP endpoint manually + @FASTAPI_APP.api_route("/mcp", methods=["GET", "POST", "DELETE"], include_in_schema=False, operation_id="mcp_http") + async def handle_mcp_streamable_http(request: Request) -> Response: + # Ensure session manager is created with stateless=True + if http_transport._session_manager is None: + http_transport._session_manager = StreamableHTTPSessionManager( + app=mcp.server, + event_store=http_transport.event_store, + json_response=True, + stateless=True, # Enable stateless mode - no session management required + ) + # Start the session manager - http_transport._manager_task = asyncio.create_task(run_manager()) - http_transport._manager_started = True - await asyncio.sleep(0.1) # Give it time to initialize + async def run_manager() -> None: + async with http_transport._session_manager.run(): + await asyncio.Event().wait() - return await http_transport.handle_fastapi_request(request) + http_transport._manager_task = asyncio.create_task(run_manager()) + http_transport._manager_started = True + await asyncio.sleep(0.1) # Give it time to initialize + return await http_transport.handle_fastapi_request(request) -LOGGER.info("MCP integration initialized successfully (no authentication configured)") -LOGGER.debug("MCP HTTP endpoint mounted at: /mcp") + LOGGER.info("MCP integration initialized successfully (no authentication configured)") + LOGGER.debug("MCP HTTP endpoint mounted at: /mcp") From 33a14c257a025c60565b8c4c977679d2c6ae70de Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 18 Nov 2025 16:11:55 +0200 Subject: [PATCH 2/4] docs: remove PODMAN_TROUBLESHOOTING.md Remove outdated Podman troubleshooting documentation. --- docs/PODMAN_TROUBLESHOOTING.md | 266 --------------------------------- 1 file changed, 266 deletions(-) delete mode 100644 docs/PODMAN_TROUBLESHOOTING.md diff --git a/docs/PODMAN_TROUBLESHOOTING.md b/docs/PODMAN_TROUBLESHOOTING.md deleted file mode 100644 index fa56d53e..00000000 --- a/docs/PODMAN_TROUBLESHOOTING.md +++ /dev/null @@ -1,266 +0,0 @@ -# Podman Runtime Directory Troubleshooting - -This document provides solutions for common Podman runtime directory issues, particularly the "boot ID mismatch" error that occurs after system reboots. - -## ๐Ÿšจ Problem Description - -**Error Symptoms:** - -- Registry authentication failing after system reboot -- "Boot ID mismatch" errors in Podman logs -- Container startup failures related to runtime directories -- Need to manually delete `/tmp/storage-run-${UID}/containers` and `/tmp/storage-run-${UID}/libpod/tmp` - -**Root Cause:** -Podman creates runtime directories in `/tmp/storage-run-*` that reference the system's boot session. After a reboot, these directories become stale because they still reference the old boot ID, causing authentication and runtime failures. - -## โœ… Automated Solutions (Recommended) - -### 1. Built-in Cleanup (Enabled by Default) - -The webhook server now includes automatic Podman runtime cleanup that runs on every container start: - -```bash -# This happens automatically in entrypoint.py -./scripts/podman-cleanup.sh -``` - -**What it does:** - -- โœ… Detects stale runtime directories -- โœ… Safely removes `/tmp/storage-run-${UID}/*` paths -- โœ… Reinitializes Podman storage -- โœ… Provides detailed logging of cleanup actions - -### 2. Docker Compose Integration - -The updated `docker-compose.yaml` includes automatic cleanup: - -```yaml -services: - github-webhook-server: - # ... other config ... - volumes: - - "/tmp/podman-storage-${USER:-1000}:/tmp/storage-run-${USER:-1000}" - command: sh -c 'rm -rf /tmp/storage-run-${USER:-1000}/* 2>/dev/null || true && exec uv run entrypoint.py' -``` - -## ๐Ÿ› ๏ธ Manual Solutions - -### Quick Fix (Immediate) - -If you encounter the issue right now: - -```bash -# Stop the container -docker-compose down - -# Clean up stale directories (replace 1000 with your actual UID) -export PODMAN_UID=${UID:-1000} -sudo rm -rf /tmp/storage-run-${PODMAN_UID}/containers -sudo rm -rf /tmp/storage-run-${PODMAN_UID}/libpod/tmp -sudo rm -rf /tmp/storage-run-${PODMAN_UID}/libpod -sudo rm -rf /tmp/storage-run-${PODMAN_UID} - -# Restart the container -docker-compose up -d -``` - -### Host-Level Prevention (Optional) - -For system-wide cleanup of all Podman directories: - -```bash -# Add to crontab (crontab -e) for daily cleanup -0 2 * * * find /tmp -name "storage-run-*" -type d -mtime +1 -exec rm -rf {} + 2>/dev/null -``` - -## ๐Ÿ” Diagnosis Commands - -### Check for Stale Directories - -```bash -# List all Podman runtime directories -find /tmp -name "storage-run-*" -type d 2>/dev/null - -# Check directory ages -find /tmp -name "storage-run-*" -type d -exec ls -la {} \; 2>/dev/null - -# Check current boot ID -cat /proc/sys/kernel/random/boot_id -``` - -### Monitor Podman Storage - -```bash -# Check Podman storage info -podman system info --format='{{.Store.GraphRoot}}' - -# Reset Podman storage (if needed) -podman system reset --force -``` - -### Container Logs - -```bash -# Check container startup logs -docker-compose logs github-webhook-server | grep -i "cleanup\|podman" - -# Monitor real-time logs -docker-compose logs -f github-webhook-server -``` - -## โš™๏ธ Configuration Options - -### Environment Variables - -Control cleanup behavior via environment variables: - -```yaml -environment: - - PODMAN_CLEANUP_ENABLED=true # Enable/disable automatic cleanup - - PODMAN_CLEANUP_TIMEOUT=30 # Cleanup script timeout (seconds) - - PODMAN_CLEANUP_VERBOSE=true # Enable verbose cleanup logging -``` - -### Custom Cleanup Script - -You can customize the cleanup script at `scripts/podman-cleanup.sh`: - -```bash -#!/bin/bash -# Add your custom cleanup logic here - -# Example: Clean additional directories -rm -rf /tmp/custom-podman-temp/* 2>/dev/null || true - -# Example: Send notifications -curl -X POST "https://your-monitoring-endpoint.com/podman-cleanup" \ - -d '{"status": "completed", "timestamp": "$(date -Iseconds)"}' || true -``` - -## ๐Ÿ”„ Alternative Approaches - -### 1. Persistent Storage Mount - -Mount Podman storage to a persistent location: - -```yaml -volumes: - - "./podman-storage:/home/podman/.local/share/containers" - - "./podman-tmp:/tmp/storage-run-${USER:-1000}" -``` - -### 2. Init Container Pattern - -Use an init container for cleanup: - -```yaml -services: - podman-init: - image: alpine:latest - command: sh -c 'rm -rf /tmp/cleanup/* && echo "Cleanup completed"' - volumes: - - "/tmp/storage-run-${USER:-1000}:/tmp/cleanup" - - github-webhook-server: - depends_on: - - podman-init - # ... rest of config ... -``` - -### 3. Host-Level Cron Job - -Set up a cron job on the host system: - -```bash -# Add to crontab (crontab -e) -# Clean up stale Podman directories daily at 2 AM -0 2 * * * find /tmp -name "storage-run-*" -type d -mtime +1 -exec rm -rf {} + 2>/dev/null -``` - -## ๐Ÿ“Š Monitoring and Alerts - -### Health Check Integration - -The cleanup status is logged and can be monitored: - -```bash -# Check recent cleanup logs -docker-compose logs github-webhook-server | grep "๐Ÿงน\|โœ…\|โš ๏ธ" - -# Set up log monitoring (example with journald) -journalctl -u docker-compose@github-webhook-server -f | grep -i cleanup -``` - -### Prometheus Metrics (Future Enhancement) - -Consider adding cleanup metrics: - -```python -# Example metrics that could be added -podman_cleanup_total = Counter('podman_cleanup_total', 'Total cleanup operations') -podman_cleanup_duration = Histogram('podman_cleanup_duration_seconds', 'Cleanup duration') -podman_stale_directories = Gauge('podman_stale_directories', 'Number of stale directories found') -``` - -## ๐Ÿ› Troubleshooting - -### Cleanup Script Fails - -```bash -# Check script permissions -ls -la scripts/podman-cleanup.sh - -# Make executable if needed -chmod +x scripts/podman-cleanup.sh - -# Test manually -./scripts/podman-cleanup.sh -``` - -### Permission Issues - -```bash -# Fix ownership if needed (replace 1000 with your actual UID) -export PODMAN_UID=${UID:-1000} -sudo chown -R ${PODMAN_UID}:${PODMAN_UID} /tmp/storage-run-${PODMAN_UID} - -# Check SELinux context (if applicable) -ls -Z /tmp/storage-run-${PODMAN_UID} -``` - -### Container Won't Start - -```bash -# Reset everything -docker-compose down -sudo rm -rf /tmp/storage-run-* -docker-compose up -d - -# Check system resources -df -h /tmp -free -h -``` - -## ๐Ÿ“ Best Practices - -1. **Always enable automatic cleanup** in production environments -2. **Monitor cleanup logs** to catch potential issues early -3. **Test recovery procedures** during maintenance windows -4. **Keep host system `/tmp` clean** with regular maintenance -5. **Document any custom modifications** to cleanup procedures - -## ๐Ÿ”— Related Issues - -- [Podman Issue #12345](https://github.com/containers/podman/issues/12345) - Boot ID mismatch -- [Containers/Storage Issue #789](https://github.com/containers/storage/issues/789) - Runtime directory cleanup - -## ๐Ÿ“ž Support - -If you continue to experience issues: - -1. Check the [main troubleshooting guide](../README.md#troubleshooting) -2. Review container logs with `docker-compose logs -f` -3. Create an issue with logs and system information -4. Include output of `podman system info` and `df -h /tmp` From 28b43d4a93c1dfdbb5e65e98ae033f17ae53257a Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 19 Nov 2025 18:21:03 +0200 Subject: [PATCH 3/4] test: fix failing tests and improve test coverage - Fixed 39 failing tests in test_pull_request_handler.py by adding proper mocks and assertions - Added comprehensive tests in test_app.py for websocket, lifespan, and log viewer endpoints - Fixed test_check_run_handler.py output truncation test to verify correct behavior - Added cleanup method tests in test_github_api.py - Achieved 91.46% test coverage (exceeds 90% requirement) - Fixed prek violations: secret detection and import organization - All 929 tests now passing successfully --- webhook_server/app.py | 50 ++- webhook_server/libs/github_api.py | 18 +- .../libs/handlers/check_run_handler.py | 37 +- .../libs/handlers/pull_request_handler.py | 4 +- webhook_server/tests/test_app.py | 357 ++++++++++++++- .../tests/test_check_run_handler.py | 4 +- webhook_server/tests/test_github_api.py | 60 +++ .../tests/test_pull_request_handler.py | 407 +++++++++++++++++- 8 files changed, 888 insertions(+), 49 deletions(-) diff --git a/webhook_server/app.py b/webhook_server/app.py index 341d8fff..b8954392 100644 --- a/webhook_server/app.py +++ b/webhook_server/app.py @@ -54,6 +54,10 @@ _lifespan_http_client: httpx.AsyncClient | None = None _background_tasks: set[asyncio.Task] = set() +# MCP Globals +http_transport: Any | None = None +mcp: Any | None = None + # Helper function to wrap the imported gate_by_allowlist_ips with ALLOWED_IPS async def gate_by_allowlist_ips_dependency(request: Request) -> None: @@ -148,6 +152,26 @@ async def lifespan(_app: FastAPI) -> AsyncGenerator[None, None]: "Check network connectivity to GitHub/Cloudflare API endpoints." ) + # Initialize MCP session manager if enabled and configured + global http_transport, mcp + if MCP_SERVER_ENABLED and http_transport is not None and mcp is not None: + if http_transport._session_manager is None: + http_transport._session_manager = StreamableHTTPSessionManager( + app=mcp.server, + event_store=http_transport.event_store, + json_response=True, + stateless=True, # Enable stateless mode - no session management required + ) + + async def run_manager() -> None: + if http_transport and http_transport._session_manager: + async with http_transport._session_manager.run(): + await asyncio.Event().wait() + + http_transport._manager_task = asyncio.create_task(run_manager()) + http_transport._manager_started = True + LOGGER.info("MCP session manager initialized in lifespan") + yield except Exception as ex: @@ -329,7 +353,10 @@ async def process_with_error_handling( try: # Initialize GithubWebhook inside background task to avoid blocking webhook response _api: GithubWebhook = GithubWebhook(hook_data=_hook_data, headers=_headers, logger=_logger) - await _api.process() + try: + await _api.process() + finally: + await _api.cleanup() except RepositoryNotFoundInConfigError: # Repository-specific error - not exceptional, log as error not exception _logger.error(f"{_log_context} Repository not found in configuration") @@ -1073,23 +1100,10 @@ async def websocket_log_stream( # Register the HTTP endpoint manually @FASTAPI_APP.api_route("/mcp", methods=["GET", "POST", "DELETE"], include_in_schema=False, operation_id="mcp_http") async def handle_mcp_streamable_http(request: Request) -> Response: - # Ensure session manager is created with stateless=True - if http_transport._session_manager is None: - http_transport._session_manager = StreamableHTTPSessionManager( - app=mcp.server, - event_store=http_transport.event_store, - json_response=True, - stateless=True, # Enable stateless mode - no session management required - ) - # Start the session manager - - async def run_manager() -> None: - async with http_transport._session_manager.run(): - await asyncio.Event().wait() - - http_transport._manager_task = asyncio.create_task(run_manager()) - http_transport._manager_started = True - await asyncio.sleep(0.1) # Give it time to initialize + # Session manager is initialized in lifespan + if http_transport is None or http_transport._session_manager is None: + LOGGER.error("MCP session manager not initialized") + raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="MCP server not initialized") return await http_transport.handle_fastapi_request(request) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 1f379b9a..cad23a00 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -712,6 +712,22 @@ def _current_pull_request_supported_retest(self) -> list[str]: current_pull_request_supported_retest.append(CONVENTIONAL_TITLE_STR) return current_pull_request_supported_retest + async def cleanup(self) -> None: + """Clean up temporary resources. + + Explicitly removes the temporary clone directory. This should be called + when the GithubWebhook instance is no longer needed. + """ + if hasattr(self, "clone_repo_dir") and os.path.exists(self.clone_repo_dir): + try: + # Use to_thread for blocking I/O operation + await asyncio.to_thread(shutil.rmtree, self.clone_repo_dir, ignore_errors=True) + if hasattr(self, "logger"): + self.logger.debug(f"{self.log_prefix} Cleaned up temp directory: {self.clone_repo_dir}") + except Exception as ex: + if hasattr(self, "logger"): + self.logger.warning(f"{self.log_prefix} Failed to cleanup temp directory: {ex}") + def __del__(self) -> None: """Remove the shared clone directory when the webhook object is destroyed. @@ -724,6 +740,6 @@ def __del__(self) -> None: try: shutil.rmtree(self.clone_repo_dir, ignore_errors=True) if hasattr(self, "logger"): - self.logger.debug(f"Cleaned up temp directory: {self.clone_repo_dir}") + self.logger.debug(f"Cleaned up temp directory (in __del__): {self.clone_repo_dir}") except Exception: pass # Ignore errors during cleanup diff --git a/webhook_server/libs/handlers/check_run_handler.py b/webhook_server/libs/handlers/check_run_handler.py index aae3a517..d3f0c52a 100644 --- a/webhook_server/libs/handlers/check_run_handler.py +++ b/webhook_server/libs/handlers/check_run_handler.py @@ -301,12 +301,39 @@ def get_check_run_text(self, err: str, out: str) -> str: err_clean = strip_ansi_codes(err) out_clean = strip_ansi_codes(out) - total_len: int = len(err_clean) + len(out_clean) + # GitHub limit is 65535 characters, but we use 65534 to be safe + # We reserve space for the markdown wrapper: ```\n{err}\n\n{out}\n``` + # Wrapper overhead = len("```\n\n\n```") = 10 characters + MAX_LEN = 65534 + WRAPPER_OVERHEAD = 10 + + # Prepare error part first - we want to preserve it as much as possible + # If error itself is huge, we might need to truncate it too, but usually it's small + # If error + wrapper > MAX_LEN, we truncate error + if len(err_clean) + WRAPPER_OVERHEAD > MAX_LEN: + err_clean = err_clean[: MAX_LEN - WRAPPER_OVERHEAD - 3] + "..." + out_clean = "" # No space for output + + # Calculate remaining space for output + current_len = len(err_clean) + WRAPPER_OVERHEAD + remaining_space = MAX_LEN - current_len + + if len(out_clean) > remaining_space: + # Truncate output: keep start and end + truncation_msg = "\n...[TRUNCATED]...\n" + msg_len = len(truncation_msg) + + if remaining_space <= msg_len: + # Very little space, just take head + out_clean = out_clean[:remaining_space] + else: + # Keep head and tail + available_content = remaining_space - msg_len + head_len = available_content // 2 + tail_len = available_content - head_len + out_clean = out_clean[:head_len] + truncation_msg + out_clean[-tail_len:] - if total_len > 65534: # GitHub limit is 65535 characters - _output = f"```\n{err_clean}\n\n{out_clean}\n```"[:65534] - else: - _output = f"```\n{err_clean}\n\n{out_clean}\n```" + _output = f"```\n{err_clean}\n\n{out_clean}\n```" _hased_str = "*****" diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 6d08302f..fd249a83 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -545,6 +545,7 @@ async def _delete_ghcr_tag_via_github_api( f"{self.log_prefix} Package {package_name} not found for owner {owner_name} on GHCR" ) return + # Find version with matching tag version_to_delete_id: int | None = None for version in versions: @@ -1057,7 +1058,8 @@ async def _check_if_pr_approved(self, labels: list[str]) -> str: reviewer = _label.split(LABELS_SEPARATOR)[-1] if LGTM_BY_LABEL_PREFIX.lower() in _label.lower() and reviewer in all_reviewers_without_pr_owner: lgtm_count += 1 - all_reviewers_without_pr_owner_and_lgtmed.remove(reviewer) + if reviewer in all_reviewers_without_pr_owner_and_lgtmed: + all_reviewers_without_pr_owner_and_lgtmed.remove(reviewer) self.logger.debug(f"{self.log_prefix} lgtm_count: {lgtm_count}") for _label in labels: diff --git a/webhook_server/tests/test_app.py b/webhook_server/tests/test_app.py index bb8f7f9f..4852675d 100644 --- a/webhook_server/tests/test_app.py +++ b/webhook_server/tests/test_app.py @@ -1,17 +1,26 @@ +import asyncio import hashlib import hmac import ipaddress import json import os from typing import Any -from unittest.mock import AsyncMock, Mock, patch +from unittest.mock import AsyncMock, MagicMock, Mock, patch import httpx import pytest +from fastapi.responses import StreamingResponse from fastapi.testclient import TestClient from webhook_server import app as app_module -from webhook_server.app import FASTAPI_APP +from webhook_server.app import ( + FASTAPI_APP, + HTTPException, + get_log_viewer_controller, + require_log_server_enabled, + status, + websocket_log_stream, +) from webhook_server.libs.exceptions import RepositoryNotFoundInConfigError from webhook_server.utils.app_utils import ( gate_by_allowlist_ips, @@ -28,6 +37,12 @@ def client(self) -> TestClient: """FastAPI test client.""" return TestClient(FASTAPI_APP) + @pytest.fixture(autouse=True) + def reset_allowed_ips(self): + """Ensure ALLOWED_IPS is empty to avoid IP gating issues during tests.""" + with patch("webhook_server.app.ALLOWED_IPS", ()): + yield + @pytest.fixture def valid_webhook_payload(self) -> dict[str, Any]: """Valid webhook payload for testing.""" @@ -637,3 +652,341 @@ def test_static_files_validation_logic(self, mock_isdir: Mock, mock_exists: Mock error_msg = str(exc_info.value) assert "exists but is not a directory" in error_msg assert "css/ and js/ subdirectories" in error_msg + + def test_require_log_server_enabled_raises(self) -> None: + """Test that require_log_server_enabled raises 404 when disabled.""" + with patch("webhook_server.app.LOG_SERVER_ENABLED", False): + with pytest.raises(HTTPException) as exc: + require_log_server_enabled() + assert exc.value.status_code == 404 + assert "Log server is disabled" in exc.value.detail + + @pytest.mark.asyncio + async def test_websocket_log_stream_disabled(self) -> None: + """Test websocket connection when log server is disabled.""" + mock_ws = AsyncMock() + with patch("webhook_server.app.LOG_SERVER_ENABLED", False): + await websocket_log_stream(mock_ws) + mock_ws.close.assert_called_once_with(code=status.WS_1008_POLICY_VIOLATION, reason="Log server is disabled") + + @pytest.mark.asyncio + async def test_websocket_log_stream_enabled(self) -> None: + """Test websocket connection when log server is enabled.""" + mock_ws = AsyncMock() + mock_controller = AsyncMock() + + with patch("webhook_server.app.LOG_SERVER_ENABLED", True): + with patch("webhook_server.app.get_log_viewer_controller", return_value=mock_controller): + await websocket_log_stream(mock_ws, hook_id="123") + mock_controller.handle_websocket.assert_called_once() + # Verify arguments + call_args = mock_controller.handle_websocket.call_args + assert call_args.kwargs["websocket"] == mock_ws + assert call_args.kwargs["hook_id"] == "123" + + @patch("webhook_server.app.os.path.exists") + async def test_lifespan_static_files_not_found(self, mock_exists: Mock) -> None: + """Test lifespan raises FileNotFoundError when static files missing.""" + mock_exists.return_value = False + # Mock AsyncClient to avoid connection errors if it tries to create one + with patch("httpx.AsyncClient", return_value=AsyncMock()): + with pytest.raises(FileNotFoundError): + async with app_module.lifespan(FASTAPI_APP): + pass + + @patch("webhook_server.app.os.path.exists") + @patch("webhook_server.app.os.path.isdir") + async def test_lifespan_static_files_not_dir(self, mock_isdir: Mock, mock_exists: Mock) -> None: + """Test lifespan raises NotADirectoryError when static files path is not a dir.""" + mock_exists.return_value = True + mock_isdir.return_value = False + with patch("httpx.AsyncClient", return_value=AsyncMock()): + with pytest.raises(NotADirectoryError): + async with app_module.lifespan(FASTAPI_APP): + pass + + def test_get_log_viewer_controller_singleton(self) -> None: + """Test singleton behavior.""" + # Reset singleton for test by patching it to None initially + with patch("webhook_server.app._log_viewer_controller_singleton", None): + # Need to patch LogViewerController to avoid actual instantiation issues + with patch("webhook_server.app.LogViewerController") as MockController: + c1 = get_log_viewer_controller() + c2 = get_log_viewer_controller() + assert c1 is c2 + assert c1 is not None + MockController.assert_called_once() + + @patch("webhook_server.app.Config") + async def test_lifespan_background_tasks(self, mock_config: Mock) -> None: + """Test waiting for background tasks.""" + + # We need to populate _background_tasks with a dummy task + async def dummy_coro(): + await asyncio.sleep(0.01) + + dummy_task = asyncio.create_task(dummy_coro()) + + # Mock config/etc to pass startup + mock_config.return_value.root_data = {"verify-github-ips": False, "verify-cloudflare-ips": False} + + # Use a set containing our task + tasks_set = {dummy_task} + + with patch("webhook_server.app._background_tasks", tasks_set): + with patch("httpx.AsyncClient", return_value=AsyncMock()): + async with app_module.lifespan(FASTAPI_APP): + pass + + # Task should be done + assert dummy_task.done() + + @patch("webhook_server.app.MCP_SERVER_ENABLED", True) + @patch("webhook_server.app.StreamableHTTPSessionManager") + @patch("webhook_server.app.Config") + async def test_lifespan_mcp_init(self, mock_config: Mock, mock_stream_manager: Mock) -> None: + """Test MCP initialization in lifespan.""" + # Mock config + mock_config.return_value.root_data = {"verify-github-ips": False, "verify-cloudflare-ips": False} + + # Mock transport and mcp globals + mock_transport_instance = Mock() + mock_transport_instance._session_manager = None + # Mock the event store which is accessed + mock_transport_instance.event_store = Mock() + + mock_mcp_instance = Mock() + mock_mcp_instance.server = Mock() + + # We need to inject these mocks into the module globals `http_transport` and `mcp` + with patch("webhook_server.app.http_transport", mock_transport_instance): + with patch("webhook_server.app.mcp", mock_mcp_instance): + with patch("httpx.AsyncClient", return_value=AsyncMock()): + async with app_module.lifespan(FASTAPI_APP): + # Give the background task a moment to run + await asyncio.sleep(0.01) + + # Verify session manager was initialized + # The code sets: http_transport._session_manager = StreamableHTTPSessionManager(...) + assert mock_transport_instance._manager_started is True + + @patch("webhook_server.app.get_cloudflare_allowlist") + @patch("webhook_server.app.Config") + async def test_lifespan_cloudflare_fail_only(self, mock_config: Mock, mock_cf_allowlist: Mock) -> None: + """Test lifespan fails when only Cloudflare enabled and fails.""" + mock_config.return_value.root_data = { + "verify-github-ips": False, + "verify-cloudflare-ips": True, + } + mock_cf_allowlist.side_effect = Exception("Cloudflare error") + + with patch("httpx.AsyncClient", return_value=AsyncMock()): + with pytest.raises(Exception, match="Cloudflare error"): + async with app_module.lifespan(FASTAPI_APP): + pass + + @patch("webhook_server.app.get_github_allowlist") + @patch("webhook_server.app.Config") + async def test_lifespan_github_fail_only(self, mock_config: Mock, mock_gh_allowlist: Mock) -> None: + """Test lifespan fails when only GitHub enabled and fails.""" + mock_config.return_value.root_data = { + "verify-github-ips": True, + "verify-cloudflare-ips": False, + } + mock_gh_allowlist.side_effect = Exception("GitHub error") + + with patch("httpx.AsyncClient", return_value=AsyncMock()): + with pytest.raises(Exception, match="GitHub error"): + async with app_module.lifespan(FASTAPI_APP): + pass + + @patch("webhook_server.app.Config") + async def test_lifespan_background_tasks_timeout(self, mock_config: Mock) -> None: + """Test cancelling background tasks on timeout.""" + # Use a mock task instead of a real one to verify cancel call + mock_task = Mock() + + mock_config.return_value.root_data = {"verify-github-ips": False, "verify-cloudflare-ips": False} + + tasks_set = {mock_task} + + with patch("webhook_server.app._background_tasks", tasks_set): + with patch("httpx.AsyncClient", return_value=AsyncMock()): + # Mock asyncio.wait to return (empty, pending) to simulate timeout + # asyncio.wait is used twice: first for completion, second for cancellation + with patch("asyncio.wait", new_callable=AsyncMock) as mock_wait: + # First call returns ([], [task]), second call returns ([], []) + mock_wait.side_effect = [ + (set(), {mock_task}), # First wait times out + (set(), set()), # Second wait after cancel + ] + + async with app_module.lifespan(FASTAPI_APP): + pass + + assert mock_wait.call_count == 2 + # Task should have been cancelled + mock_task.cancel.assert_called_once() + + def test_process_webhook_missing_event_header(self, client: TestClient) -> None: + """Test missing X-GitHub-Event header.""" + response = client.post("/webhook_server", content="{}", headers={}) + assert response.status_code == 400 + assert "Missing X-GitHub-Event header" in response.json()["detail"] + + @patch("webhook_server.app.Config") + def test_process_webhook_missing_repo_name(self, mock_config: Mock, client: TestClient) -> None: + """Test payload missing repository.name.""" + # Ensure no secret so verify_signature is skipped + mock_config.return_value.root_data = {"webhook-secret": None} + + headers = {"X-GitHub-Event": "push", "Content-Type": "application/json"} + payload = {"repository": {"full_name": "org/repo"}} + response = client.post("/webhook_server", content=json.dumps(payload), headers=headers) + assert response.status_code == 400 + assert "Missing repository.name in payload" in response.json()["detail"] + + @patch("webhook_server.app.Config") + def test_process_webhook_missing_repo_full_name(self, mock_config: Mock, client: TestClient) -> None: + """Test payload missing repository.full_name.""" + mock_config.return_value.root_data = {"webhook-secret": None} + + headers = {"X-GitHub-Event": "push", "Content-Type": "application/json"} + payload = {"repository": {"name": "repo"}} + response = client.post("/webhook_server", content=json.dumps(payload), headers=headers) + assert response.status_code == 400 + assert "Missing repository.full_name in payload" in response.json()["detail"] + + @patch("webhook_server.app.LOG_SERVER_ENABLED", True) + def test_get_log_viewer_page(self, client: TestClient) -> None: + """Test get_log_viewer_page endpoint.""" + mock_instance = MagicMock() + mock_instance.get_log_page.return_value = "" + + # Patch the singleton directly as controller_dependency captures reference to get_log_viewer_controller + with patch("webhook_server.app._log_viewer_controller_singleton", mock_instance): + response = client.get("/logs") + assert response.status_code == 200 + + @patch("webhook_server.app.LOG_SERVER_ENABLED", True) + def test_get_log_entries(self, client: TestClient) -> None: + """Test get_log_entries endpoint.""" + mock_instance = MagicMock() + # The controller returns a dict + mock_instance.get_log_entries.return_value = {"entries": []} + + with patch("webhook_server.app._log_viewer_controller_singleton", mock_instance): + response = client.get("/logs/api/entries") + assert response.status_code == 200 + assert response.json() == {"entries": []} + + @patch("webhook_server.app.LOG_SERVER_ENABLED", True) + def test_export_logs(self, client: TestClient) -> None: + """Test export_logs endpoint.""" + mock_instance = MagicMock() + + def iter_content(): + yield b"data" + + mock_instance.export_logs.return_value = StreamingResponse(iter_content()) + + with patch("webhook_server.app._log_viewer_controller_singleton", mock_instance): + response = client.get("/logs/api/export?format_type=json") + assert response.status_code == 200 + assert response.content == b"data" + + @patch("webhook_server.app.get_github_allowlist") + @patch("webhook_server.app.get_cloudflare_allowlist") + @patch("webhook_server.app.Config") + async def test_lifespan_log_viewer_shutdown( + self, mock_config: Mock, mock_cf_allowlist: Mock, mock_gh_allowlist: Mock + ) -> None: + """Test LogViewerController shutdown in lifespan.""" + mock_config.return_value.root_data = {"verify-github-ips": False, "verify-cloudflare-ips": False} + + mock_controller = AsyncMock() + + # Inject mock controller into singleton + with patch("webhook_server.app._log_viewer_controller_singleton", mock_controller): + with patch("httpx.AsyncClient", return_value=AsyncMock()): + async with app_module.lifespan(FASTAPI_APP): + pass + + mock_controller.shutdown.assert_called_once() + + @patch("webhook_server.app.GithubWebhook") + @patch("webhook_server.app.Config") + def test_process_webhook_connect_error(self, mock_config: Mock, mock_webhook_cls: Mock, client: TestClient) -> None: + """Test process_webhook with connection error.""" + mock_config.return_value.root_data = {"webhook-secret": None} + + mock_instance = mock_webhook_cls.return_value + mock_instance.process = AsyncMock(side_effect=httpx.ConnectError("Connection failed")) + mock_instance.cleanup = AsyncMock() + + with patch("webhook_server.app.get_logger_with_params") as mock_get_logger: + mock_logger = mock_get_logger.return_value + + headers = {"X-GitHub-Event": "push", "Content-Type": "application/json", "X-GitHub-Delivery": "123"} + payload = {"repository": {"name": "repo", "full_name": "org/repo"}} + + captured_coro = None + + # Mock create_task to return a dummy task but capture coro + mock_task = MagicMock() + + def side_effect(coro): + nonlocal captured_coro + captured_coro = coro + return mock_task + + with patch("webhook_server.app.asyncio.create_task", side_effect=side_effect): + response = client.post("/webhook_server", content=json.dumps(payload), headers=headers) + assert response.status_code == 200 + + # Run captured coro + if captured_coro: + asyncio.run(captured_coro) + + # Verify exception logging + mock_logger.exception.assert_called() + call_args = mock_logger.exception.call_args + assert "API connection error - check network connectivity" in call_args[0][0] + + @patch("webhook_server.app.GithubWebhook") + @patch("webhook_server.app.Config") + def test_process_webhook_repo_not_found( + self, mock_config: Mock, mock_webhook_cls: Mock, client: TestClient + ) -> None: + """Test process_webhook with repository not found error.""" + mock_config.return_value.root_data = {"webhook-secret": None} + + mock_instance = mock_webhook_cls.return_value + mock_instance.process = AsyncMock(side_effect=RepositoryNotFoundInConfigError("Repo not found")) + mock_instance.cleanup = AsyncMock() + + with patch("webhook_server.app.get_logger_with_params") as mock_get_logger: + mock_logger = mock_get_logger.return_value + + headers = {"X-GitHub-Event": "push", "Content-Type": "application/json", "X-GitHub-Delivery": "123"} + payload = {"repository": {"name": "repo", "full_name": "org/repo"}} + + captured_coro = None + mock_task = MagicMock() + + def side_effect(coro): + nonlocal captured_coro + captured_coro = coro + return mock_task + + with patch("webhook_server.app.asyncio.create_task", side_effect=side_effect): + response = client.post("/webhook_server", content=json.dumps(payload), headers=headers) + assert response.status_code == 200 + + if captured_coro: + asyncio.run(captured_coro) + + # Verify error logging + mock_logger.error.assert_called() + call_args = mock_logger.error.call_args + assert "Repository not found in configuration" in call_args[0][0] diff --git a/webhook_server/tests/test_check_run_handler.py b/webhook_server/tests/test_check_run_handler.py index 2401f56a..879202c5 100644 --- a/webhook_server/tests/test_check_run_handler.py +++ b/webhook_server/tests/test_check_run_handler.py @@ -478,9 +478,11 @@ def test_get_check_run_text_long_length(self, check_run_handler: CheckRunHandler result = check_run_handler.get_check_run_text(long_err, long_out) - # Should be truncated to 65534 characters + # Should be truncated to 65534 characters (GitHub limit safe margin) assert len(result) == 65534 assert result.startswith("```\n") + # Verify the fix: it should end with the code block closer + assert result.endswith("\n```") def test_get_check_run_text_token_replacement(self, check_run_handler: CheckRunHandler) -> None: """Test that sensitive tokens are replaced in check run text.""" diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 696b5e8b..b4fe7322 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1562,3 +1562,63 @@ async def test_process_push_event_normal_push_not_deletion( # Verify PushHandler.process_push_webhook_data was called mock_process_push.assert_called_once() + + @pytest.mark.asyncio + async def test_cleanup( + self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock, to_thread_sync: Any + ) -> None: + """Test cleanup method removes temporary directory.""" + mock_logger = Mock() + with patch("webhook_server.libs.github_api.Config") as mock_config: + mock_config.return_value.repository = True + mock_config.return_value.repository_local_data.return_value = {} + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), "token", "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api"): + with patch("webhook_server.libs.github_api.get_repository_github_app_api"): + with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix"): + gh = GithubWebhook(minimal_hook_data, minimal_headers, mock_logger) + + # Set a fake clone dir + gh.clone_repo_dir = "/tmp/fake-clone-dir" + + with patch("os.path.exists", return_value=True): + with patch("shutil.rmtree") as mock_rmtree: + with patch("asyncio.to_thread", side_effect=to_thread_sync): + await gh.cleanup() + + mock_rmtree.assert_called_once_with("/tmp/fake-clone-dir", ignore_errors=True) + mock_logger.debug.assert_called() + + @pytest.mark.asyncio + async def test_cleanup_exception( + self, minimal_hook_data: dict, minimal_headers: dict, logger: Mock, to_thread_sync: Any + ) -> None: + """Test cleanup method handles exceptions.""" + mock_logger = Mock() + with patch("webhook_server.libs.github_api.Config") as mock_config: + mock_config.return_value.repository = True + mock_config.return_value.repository_local_data.return_value = {} + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), "token", "apiuser") + + with patch("webhook_server.libs.github_api.get_github_repo_api"): + with patch("webhook_server.libs.github_api.get_repository_github_app_api"): + with patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix"): + gh = GithubWebhook(minimal_hook_data, minimal_headers, mock_logger) + + gh.clone_repo_dir = "/tmp/fake-clone-dir" + + with patch("os.path.exists", return_value=True): + + def rmtree_fail(*args, **kwargs): + raise PermissionError("Access denied") + + with patch("shutil.rmtree", side_effect=rmtree_fail): + with patch("asyncio.to_thread", side_effect=to_thread_sync): + await gh.cleanup() + + mock_logger.warning.assert_called() diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 7872e8a2..02287505 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -1,10 +1,12 @@ import asyncio -from unittest.mock import AsyncMock, Mock, patch +from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest from github import GithubException from github.PullRequest import PullRequest +from webhook_server.libs.github_api import GithubWebhook +from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler from webhook_server.utils.constants import ( APPROVED_BY_LABEL_PREFIX, @@ -43,13 +45,14 @@ class TestPullRequestHandler: @pytest.fixture def mock_github_webhook(self) -> Mock: """Create a mock GithubWebhook instance.""" - mock_webhook = Mock() + mock_webhook = Mock(spec=GithubWebhook) mock_webhook.hook_data = { "action": "opened", - "pull_request": {"number": 123, "merged": False}, + "pull_request": {"number": 123, "merged": False, "title": "Test PR"}, "sender": {"login": "test-user"}, + "label": {"name": "bug"}, } - mock_webhook.logger = Mock() + mock_webhook.logger = MagicMock() mock_webhook.log_prefix = "[TEST]" mock_webhook.repository = Mock() mock_webhook.issue_url_for_welcome_msg = "welcome-message-url" @@ -63,27 +66,75 @@ def mock_github_webhook(self) -> Mock: mock_webhook.set_auto_merge_prs = [] mock_webhook.auto_merge_enabled = True mock_webhook.container_repository = "docker.io/org/repo" + # New attributes for coverage + mock_webhook.conventional_title = False + mock_webhook.minimum_lgtm = 1 + mock_webhook.container_repository_username = "test-user" + mock_webhook.container_repository_password = "test-password" # pragma: allowlist secret + mock_webhook.github_api = Mock() + mock_webhook.tox = True + mock_webhook.pre_commit = True + mock_webhook.python_module_install = False + mock_webhook.pypi = False + mock_webhook.token = "test-token" # pragma: allowlist secret + mock_webhook.auto_verify_cherry_picked_prs = True + mock_webhook.last_commit = Mock() return mock_webhook @pytest.fixture def mock_owners_file_handler(self) -> Mock: """Create a mock OwnersFileHandler instance.""" - mock_handler = Mock() + mock_handler = Mock(spec=OwnersFileHandler) mock_handler.all_pull_request_approvers = ["approver1", "approver2"] mock_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"] mock_handler.root_approvers = ["root-approver"] mock_handler.root_reviewers = ["root-reviewer"] + mock_handler.assign_reviewers = AsyncMock() return mock_handler @pytest.fixture def pull_request_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> PullRequestHandler: """Create a PullRequestHandler instance with mocked dependencies.""" - return PullRequestHandler(mock_github_webhook, mock_owners_file_handler) + # Create handler instance first + handler = PullRequestHandler(mock_github_webhook, mock_owners_file_handler) + + # Replace handler instances with mocks that have async methods + handler.labels_handler = Mock() + handler.labels_handler._add_label = AsyncMock() + handler.labels_handler._remove_label = AsyncMock() + handler.labels_handler.add_size_label = AsyncMock() + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) + handler.labels_handler.wip_or_hold_labels_exists = Mock(return_value=False) + + handler.check_run_handler = Mock() + handler.check_run_handler.set_verify_check_queued = AsyncMock() + handler.check_run_handler.set_verify_check_success = AsyncMock() + handler.check_run_handler.set_merge_check_in_progress = AsyncMock() + handler.check_run_handler.set_merge_check_success = AsyncMock() + handler.check_run_handler.set_merge_check_failure = AsyncMock() + handler.check_run_handler.set_merge_check_queued = AsyncMock() + handler.check_run_handler.set_run_tox_check_queued = AsyncMock() + handler.check_run_handler.set_run_pre_commit_check_queued = AsyncMock() + handler.check_run_handler.set_python_module_install_queued = AsyncMock() + handler.check_run_handler.set_container_build_queued = AsyncMock() + handler.check_run_handler.set_conventional_title_queued = AsyncMock() + + handler.runner_handler = Mock() + handler.runner_handler.run_container_build = AsyncMock() + handler.runner_handler.run_tox = AsyncMock() + handler.runner_handler.run_pre_commit = AsyncMock() + handler.runner_handler.run_conventional_title_check = AsyncMock() + handler.runner_handler.run_build_container = AsyncMock() + handler.runner_handler.run_install_python_module = AsyncMock() + handler.runner_handler.run_podman_command = AsyncMock(return_value=(0, "", "")) + handler.runner_handler.cherry_pick = AsyncMock() + + return handler @pytest.fixture def mock_pull_request(self) -> Mock: """Create a mock PullRequest instance.""" - mock_pr = Mock() + mock_pr = MagicMock() mock_pr.number = 123 mock_pr.title = "Test PR" mock_pr.body = "Test PR body" @@ -91,7 +142,7 @@ def mock_pull_request(self) -> Mock: mock_pr.labels = [] mock_pr.create_issue_comment = Mock() mock_pr.edit = Mock() - mock_pr.is_merged = False + mock_pr.is_merged = Mock(return_value=False) mock_pr.base = Mock() mock_pr.base.ref = "main" mock_pr.user = Mock() @@ -100,6 +151,7 @@ def mock_pull_request(self) -> Mock: mock_pr.mergeable_state = "clean" mock_pr.enable_automerge = Mock() mock_pr.add_to_assignees = Mock() + mock_pr.get_issue_comments = Mock(return_value=[]) return mock_pr @pytest.mark.asyncio @@ -121,12 +173,12 @@ async def test_process_pull_request_webhook_data_edited_action_title_changed( """Test processing pull request webhook data when action is edited and title is changed.""" pull_request_handler.hook_data["action"] = "edited" pull_request_handler.hook_data["changes"] = {"title": {"from": "old title"}} + pull_request_handler.github_webhook.conventional_title = True - with patch.object( - pull_request_handler.runner_handler, "run_conventional_title_check" - ) as mock_run_conventional_title_check: - await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) - mock_run_conventional_title_check.assert_called_once_with(pull_request=mock_pull_request) + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + pull_request_handler.runner_handler.run_conventional_title_check.assert_called_once_with( + pull_request=mock_pull_request + ) @pytest.mark.asyncio async def test_process_pull_request_webhook_data_opened_action( @@ -884,7 +936,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_success( ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/org/repo"), patch.object(pull_request_handler.github_webhook, "github_api", Mock(requester=mock_requester)), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret patch.object(mock_pull_request, "create_issue_comment", new=Mock()), ): await pull_request_handler.delete_remote_tag_for_merged_or_closed_pr(pull_request=mock_pull_request) @@ -919,7 +971,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_users_scope_fallba ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/org/repo"), patch.object(pull_request_handler.github_webhook, "github_api", Mock(requester=mock_requester)), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret patch.object(mock_pull_request, "create_issue_comment", new=Mock()), ): await pull_request_handler.delete_remote_tag_for_merged_or_closed_pr(pull_request=mock_pull_request) @@ -948,7 +1000,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_package_not_found( ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/org/repo"), patch.object(pull_request_handler.github_webhook, "github_api", Mock(requester=mock_requester)), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret ): await pull_request_handler.delete_remote_tag_for_merged_or_closed_pr(pull_request=mock_pull_request) assert pull_request_handler.logger.step.called @@ -974,7 +1026,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_tag_not_found( ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/org/repo"), patch.object(pull_request_handler.github_webhook, "github_api", Mock(requester=mock_requester)), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret ): await pull_request_handler.delete_remote_tag_for_merged_or_closed_pr(pull_request=mock_pull_request) assert pull_request_handler.logger.step.called @@ -999,7 +1051,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_api_failure( ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/org/repo"), patch.object(pull_request_handler.github_webhook, "github_api", Mock(requester=mock_requester)), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret ): await pull_request_handler.delete_remote_tag_for_merged_or_closed_pr(pull_request=mock_pull_request) assert pull_request_handler.logger.step.called @@ -1021,7 +1073,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_no_api( ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/org/repo"), patch.object(pull_request_handler.github_webhook, "github_api", None), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret ): await pull_request_handler.delete_remote_tag_for_merged_or_closed_pr(pull_request=mock_pull_request) assert pull_request_handler.logger.step.called @@ -1044,7 +1096,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_invalid_format( ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/invalid"), patch.object(pull_request_handler.github_webhook, "github_api", Mock(requester=mock_requester)), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret ): # Directly call _delete_ghcr_tag_via_github_api to test invalid format check await pull_request_handler._delete_ghcr_tag_via_github_api( @@ -1079,7 +1131,7 @@ async def test_delete_remote_tag_for_merged_or_closed_pr_ghcr_delete_404( ), patch.object(pull_request_handler.github_webhook, "container_repository", "ghcr.io/org/repo"), patch.object(pull_request_handler.github_webhook, "github_api", Mock(requester=mock_requester)), - patch.object(pull_request_handler.github_webhook, "token", "test-token"), + patch.object(pull_request_handler.github_webhook, "token", "test-token"), # pragma: allowlist secret patch.object(mock_pull_request, "create_issue_comment", new=Mock()), ): await pull_request_handler.delete_remote_tag_for_merged_or_closed_pr(pull_request=mock_pull_request) @@ -1415,3 +1467,316 @@ async def set_automerge_stub(*args, **kwargs): # type: ignore[unused-argument] # Verify automerge and other tasks were called despite error in one task assert calls["set_automerge"] == 1 + + @pytest.mark.asyncio + async def test_process_opened_async_exception( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test exception handling in async tasks for opened event.""" + mock_github_webhook.hook_data["action"] = "opened" + + # Mock methods to raise exception + with ( + patch.object( + pull_request_handler, + "create_issue_for_new_pull_request", + new=AsyncMock(side_effect=Exception("Task failed")), + ), + patch.object(pull_request_handler, "set_wip_label_based_on_title", new=AsyncMock()), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request", new=AsyncMock()), + patch.object(pull_request_handler, "set_pull_request_automerge", new=AsyncMock()), + patch.object(pull_request_handler, "_prepare_welcome_comment", return_value="welcome"), + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + + # Verify error logging + pull_request_handler.logger.error.assert_called_with("[TEST] Async task failed: Task failed") + + @pytest.mark.asyncio + async def test_process_synchronize_async_exception( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test exception handling in async tasks for synchronize event.""" + mock_github_webhook.hook_data["action"] = "synchronize" + + with ( + patch.object( + pull_request_handler, + "process_opened_or_synchronize_pull_request", + new=AsyncMock(side_effect=Exception("Sync failed")), + ), + patch.object(pull_request_handler, "remove_labels_when_pull_request_sync", new=AsyncMock()), + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + + pull_request_handler.logger.error.assert_called_with("[TEST] Async task failed: Sync failed") + + @pytest.mark.asyncio + async def test_process_labeled_can_be_merged( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test labeled event with can-be-merged label (should skip).""" + mock_github_webhook.hook_data["action"] = "labeled" + mock_github_webhook.hook_data["label"] = {"name": CAN_BE_MERGED_STR} + mock_github_webhook.verified_job = False + + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + + # Verify step call with substring + found = False + for call in pull_request_handler.logger.step.call_args_list: + if "skipped - can-be-merged label" in str(call): + found = True + break + assert found, "Log step for can-be-merged label skip not found" + + @pytest.mark.asyncio + async def test_process_labeled_wip( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test labeled event with WIP label.""" + mock_github_webhook.hook_data["action"] = "labeled" + mock_github_webhook.hook_data["label"] = {"name": WIP_STR} + mock_github_webhook.verified_job = False + + # Mock labels + mock_label = MagicMock() + mock_label.name = WIP_STR + mock_pull_request.labels = [mock_label] + + with patch.object(pull_request_handler, "check_if_can_be_merged", new=AsyncMock()) as mock_check_merge: + with patch("asyncio.to_thread", side_effect=lambda f, *args: f(*args) if callable(f) else None): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + mock_check_merge.assert_awaited_once() + + @pytest.mark.asyncio + async def test_process_unhandled_action( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test unhandled action.""" + mock_github_webhook.hook_data["action"] = "unknown_action" + + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + + found = False + for call in pull_request_handler.logger.step.call_args_list: + if "no action handler - completed" in str(call): + found = True + break + assert found + + @pytest.mark.asyncio + async def test_delete_ghcr_tag_exceptions( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test exceptions in _delete_ghcr_tag_via_github_api.""" + mock_github_webhook.build_and_push_container = True + mock_github_webhook.container_repository = "ghcr.io/org/pkg" + mock_github_webhook.container_repository_and_tag = MagicMock(return_value="ghcr.io/org/pkg:123") + mock_github_webhook.github_api = MagicMock() + mock_github_webhook.token = "token" # pragma: allowlist secret + + mock_pull_request.number = 123 + + # 1. Invalid repository format - call directly to bypass parent check + mock_github_webhook.container_repository = "ghcr.io/invalid" + await pull_request_handler._delete_ghcr_tag_via_github_api(mock_pull_request, "ghcr.io/invalid", "123") + pull_request_handler.logger.error.assert_called_with( + "[TEST] Invalid container repository format: ghcr.io/invalid" + ) + + # 2. Package not found (GithubException 404) + mock_github_webhook.container_repository = "ghcr.io/org/pkg" + mock_github_webhook.github_api.requester.requestJsonAndCheck = MagicMock( + side_effect=GithubException(404, "Not Found") + ) + + with patch( + "asyncio.to_thread", side_effect=lambda f, *args, **kwargs: f(*args, **kwargs) if callable(f) else None + ): + await pull_request_handler._delete_ghcr_tag_via_github_api(mock_pull_request, "ghcr.io/org/pkg:123", "123") + + pull_request_handler.logger.warning.assert_called_with("[TEST] Package pkg not found for owner org on GHCR") + + @pytest.mark.asyncio + async def test_add_assignee_exception( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test exception in add_pull_request_owner_as_assingee.""" + mock_pull_request.user.login = "user" + + # Set side_effect to raise first time, then succeed (or return None) second time + mock_pull_request.add_to_assignees.side_effect = [Exception("Failed"), None] + + pull_request_handler.owners_file_handler.root_approvers = ["approver1"] + + with patch( + "asyncio.to_thread", side_effect=lambda f, *args, **kwargs: f(*args, **kwargs) if callable(f) else None + ): + await pull_request_handler.add_pull_request_owner_as_assingee(mock_pull_request) + + pull_request_handler.logger.debug.assert_any_call("[TEST] Exception while adding PR owner as assignee: Failed") + pull_request_handler.logger.debug.assert_any_call("[TEST] Falling back to first approver as assignee") + # Should verify add_to_assignees called twice + assert mock_pull_request.add_to_assignees.call_count == 2 + + @pytest.mark.asyncio + async def test_process_opened_setup_task_failure( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test setup task failure in process_opened_or_synchronize_pull_request.""" + mock_github_webhook.conventional_title = True + + pull_request_handler.owners_file_handler.assign_reviewers = AsyncMock(side_effect=Exception("Setup failed")) + # Mock other methods to return coroutines + with ( + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()), + patch.object(pull_request_handler, "label_pull_request_by_merge_state", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_merge_check_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_run_tox_check_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_run_pre_commit_check_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_python_module_install_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_container_build_queued", new=AsyncMock()), + patch.object(pull_request_handler, "_process_verified_for_update_or_new_pull_request", new=AsyncMock()), + patch.object(pull_request_handler.labels_handler, "add_size_label", new=AsyncMock()), + patch.object(pull_request_handler, "add_pull_request_owner_as_assingee", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_conventional_title_queued", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_tox", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_pre_commit", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_install_python_module", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_build_container", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_conventional_title_check", new=AsyncMock()), + ): + await pull_request_handler.process_opened_or_synchronize_pull_request(mock_pull_request) + + pull_request_handler.logger.error.assert_any_call("[TEST] Setup task failed: Setup failed") + + @pytest.mark.asyncio + async def test_process_opened_ci_task_failure( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test CI task failure in process_opened_or_synchronize_pull_request.""" + mock_github_webhook.conventional_title = False + + # Mock setup tasks to succeed + pull_request_handler.owners_file_handler.assign_reviewers = AsyncMock() + + with ( + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()), + patch.object(pull_request_handler, "label_pull_request_by_merge_state", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_merge_check_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_run_tox_check_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_run_pre_commit_check_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_python_module_install_queued", new=AsyncMock()), + patch.object(pull_request_handler.check_run_handler, "set_container_build_queued", new=AsyncMock()), + patch.object(pull_request_handler, "_process_verified_for_update_or_new_pull_request", new=AsyncMock()), + patch.object(pull_request_handler.labels_handler, "add_size_label", new=AsyncMock()), + patch.object(pull_request_handler, "add_pull_request_owner_as_assingee", new=AsyncMock()), + patch.object( + pull_request_handler.runner_handler, "run_tox", new=AsyncMock(side_effect=Exception("CI failed")) + ), + patch.object(pull_request_handler.runner_handler, "run_pre_commit", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_install_python_module", new=AsyncMock()), + patch.object(pull_request_handler.runner_handler, "run_build_container", new=AsyncMock()), + ): + await pull_request_handler.process_opened_or_synchronize_pull_request(mock_pull_request) + + pull_request_handler.logger.error.assert_any_call("[TEST] CI/CD task failed: CI failed") + + @pytest.mark.asyncio + async def test_create_issue_for_new_pr_disabled( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test create_issue_for_new_pull_request when disabled.""" + mock_github_webhook.create_issue_for_new_pr = False + + await pull_request_handler.create_issue_for_new_pull_request(mock_pull_request) + + pull_request_handler.logger.info.assert_called_with( + "[TEST] Issue creation for new PRs is disabled for this repository" + ) + + @pytest.mark.asyncio + async def test_create_issue_for_new_pr_auto_verified( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test create_issue_for_new_pull_request for auto-verified user.""" + mock_github_webhook.create_issue_for_new_pr = True + mock_github_webhook.parent_committer = "user" + mock_github_webhook.auto_verified_and_merged_users = ["user"] + + await pull_request_handler.create_issue_for_new_pull_request(mock_pull_request) + + pull_request_handler.logger.info.assert_called_with( + "[TEST] Committer user is part of ['user'], will not create issue." + ) + + @pytest.mark.asyncio + async def test_set_pull_request_automerge_exception( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test exception in set_pull_request_automerge.""" + # Make enable_automerge raise exception + mock_pull_request.enable_automerge.side_effect = Exception("Automerge failed") + mock_pull_request.raw_data = {} + + pull_request_handler.github_webhook.set_auto_merge_prs = ["main"] + mock_pull_request.base.ref = "main" + + with patch( + "asyncio.to_thread", side_effect=lambda f, *args, **kwargs: f(*args, **kwargs) if callable(f) else None + ): + await pull_request_handler.set_pull_request_automerge(mock_pull_request) + + pull_request_handler.logger.error.assert_called_with( + "[TEST] Exception while setting auto merge: Automerge failed" + ) + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_unknown( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test label_pull_request_by_merge_state when unknown.""" + mock_pull_request.mergeable_state = "unknown" + + with patch( + "asyncio.to_thread", side_effect=lambda f, *args, **kwargs: f(*args, **kwargs) if callable(f) else None + ): + await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + + # Should return early + pull_request_handler.labels_handler._add_label.assert_not_called() + + @pytest.mark.asyncio + async def test_delete_registry_tag_via_regctl_failure( + self, pull_request_handler: PullRequestHandler, mock_github_webhook: Mock, mock_pull_request: Mock + ) -> None: + """Test failures in _delete_registry_tag_via_regctl.""" + mock_github_webhook.container_repository_username = "user" + mock_github_webhook.container_repository_password = "pass" # pragma: allowlist secret + mock_github_webhook.container_repository = "registry.io/repo" + + # 1. Login failure + pull_request_handler.runner_handler.run_podman_command = AsyncMock( + return_value=(False, "Login failed", "Error") + ) + + await pull_request_handler._delete_registry_tag_via_regctl(mock_pull_request, "tag", "pr-123", "registry.io") + pull_request_handler.logger.error.assert_called_with( + "[TEST] Failed to delete tag: tag. OUT:Login failed. ERR:Error" + ) + + # 2. Tag delete failure + pull_request_handler.runner_handler.run_podman_command = AsyncMock( + side_effect=[ + (True, "Login success", ""), # login + (True, "pr-123", ""), # tag ls + (False, "Delete failed", "Error"), # tag delete + (True, "", ""), # logout + ] + ) + + await pull_request_handler._delete_registry_tag_via_regctl(mock_pull_request, "tag", "pr-123", "registry.io") + pull_request_handler.logger.error.assert_called_with( + "[TEST] Failed to delete tag: tag. OUT:Delete failed. ERR:Error" + ) From 305a9940ea5aaea24d43a9b675444d961b5a4ca8 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 19 Nov 2025 20:22:06 +0200 Subject: [PATCH 4/4] fix: cleaup() we always have logger and clone_repo_dir --- webhook_server/libs/github_api.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index cad23a00..0e9b5c5f 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -718,15 +718,13 @@ async def cleanup(self) -> None: Explicitly removes the temporary clone directory. This should be called when the GithubWebhook instance is no longer needed. """ - if hasattr(self, "clone_repo_dir") and os.path.exists(self.clone_repo_dir): + if self.clone_repo_dir and os.path.exists(self.clone_repo_dir): try: # Use to_thread for blocking I/O operation await asyncio.to_thread(shutil.rmtree, self.clone_repo_dir, ignore_errors=True) - if hasattr(self, "logger"): - self.logger.debug(f"{self.log_prefix} Cleaned up temp directory: {self.clone_repo_dir}") + self.logger.debug(f"{self.log_prefix} Cleaned up temp directory: {self.clone_repo_dir}") except Exception as ex: - if hasattr(self, "logger"): - self.logger.warning(f"{self.log_prefix} Failed to cleanup temp directory: {ex}") + self.logger.warning(f"{self.log_prefix} Failed to cleanup temp directory: {ex}") def __del__(self) -> None: """Remove the shared clone directory when the webhook object is destroyed. @@ -736,10 +734,9 @@ def __del__(self) -> None: clean up their own directories. Only the base clone directory must be removed here to prevent accumulating stale repositories on disk. """ - if hasattr(self, "clone_repo_dir") and os.path.exists(self.clone_repo_dir): + if self.clone_repo_dir and os.path.exists(self.clone_repo_dir): try: shutil.rmtree(self.clone_repo_dir, ignore_errors=True) - if hasattr(self, "logger"): - self.logger.debug(f"Cleaned up temp directory (in __del__): {self.clone_repo_dir}") + self.logger.debug(f"Cleaned up temp directory (in __del__): {self.clone_repo_dir}") except Exception: pass # Ignore errors during cleanup