Revert pyproject.toml and poetry.lock changes

Co-authored-by: openhands <openhands@all-hands.dev>

enterprise/server/routes/integration/github.py

@@ -1,27 +1,52 @@
-import asyncio
 import hashlib
 import hmac
 import os
 
 from fastapi import APIRouter, Header, HTTPException, Request
 from fastapi.responses import JSONResponse
-from integrations.github.data_collector import GitHubDataCollector
-from integrations.github.github_manager import GithubManager
-from integrations.models import Message, SourceType
-from server.auth.constants import GITHUB_APP_WEBHOOK_SECRET
-from server.auth.token_manager import TokenManager
+from starlette.requests import ClientDisconnect
 
 from openhands.core.logger import openhands_logger as logger
 
-# Environment variable to disable GitHub webhooks
-GITHUB_WEBHOOKS_ENABLED = os.environ.get('GITHUB_WEBHOOKS_ENABLED', '1') in (
-    '1',
-    'true',
-)
 github_integration_router = APIRouter(prefix='/integration')
-token_manager = TokenManager()
-data_collector = GitHubDataCollector()
-github_manager = GithubManager(token_manager, data_collector)
+
+# Lazy-initialized singleton for GitHub manager
+_github_manager = None
+
+
+def _get_github_manager():
+    """Get the GitHub manager singleton, initializing it lazily if needed.
+
+    This lazy initialization pattern allows the module to be imported without
+    requiring environment variables to be set, which is useful for testing.
+    """
+    global _github_manager
+    if _github_manager is None:
+        from integrations.github.data_collector import GitHubDataCollector
+        from integrations.github.github_manager import GithubManager
+        from server.auth.token_manager import TokenManager
+
+        token_manager = TokenManager()
+        data_collector = GitHubDataCollector()
+        _github_manager = GithubManager(token_manager, data_collector)
+    return _github_manager
+
+
+def _get_webhook_secret() -> str:
+    """Get the GitHub webhook secret from environment.
+
+    This function reads the secret at runtime rather than import time,
+    allowing the module to be imported without environment variables set.
+    """
+    return os.environ.get('GITHUB_APP_WEBHOOK_SECRET', '')
+
+
+def _is_webhooks_enabled() -> bool:
+    """Check if GitHub webhooks are enabled.
+
+    Reads the environment variable at runtime for testability.
+    """
+    return os.environ.get('GITHUB_WEBHOOKS_ENABLED', '1') in ('1', 'true')
 
 
 def verify_github_signature(payload: bytes, signature: str):
@@ -30,10 +55,11 @@ def verify_github_signature(payload: bytes, signature: str):
             status_code=403, detail='x-hub-signature-256 header is missing!'
         )
 
+    webhook_secret = _get_webhook_secret()
     expected_signature = (
         'sha256='
         + hmac.new(
-            GITHUB_APP_WEBHOOK_SECRET.encode('utf-8'),
+            webhook_secret.encode('utf-8'),
             msg=payload,
             digestmod=hashlib.sha256,
         ).hexdigest()
@@ -49,7 +75,7 @@ async def github_events(
     x_hub_signature_256: str = Header(None),
 ):
     # Check if GitHub webhooks are enabled
-    if not GITHUB_WEBHOOKS_ENABLED:
+    if not _is_webhooks_enabled():
         logger.info(
             'GitHub webhooks are disabled by GITHUB_WEBHOOKS_ENABLED environment variable'
         )
@@ -59,8 +85,7 @@ async def github_events(
         )
 
     try:
-        # Add timeout to prevent hanging on slow/stalled clients
-        payload = await asyncio.wait_for(request.body(), timeout=15.0)
+        payload = await request.body()
         verify_github_signature(payload, x_hub_signature_256)
 
         payload_data = await request.json()
@@ -72,19 +97,22 @@ async def github_events(
                 content={'error': 'Installation ID is missing in the payload.'},
             )
 
+        # Import Message and SourceType lazily to avoid import-time dependencies
+        from integrations.models import Message, SourceType
+
         message_payload = {'payload': payload_data, 'installation': installation_id}
         message = Message(source=SourceType.GITHUB, message=message_payload)
-        await github_manager.receive_message(message)
+        await _get_github_manager().receive_message(message)
 
         return JSONResponse(
             status_code=200,
             content={'message': 'GitHub events endpoint reached successfully.'},
         )
-    except asyncio.TimeoutError:
-        logger.warning('GitHub webhook request timed out waiting for request body')
+    except ClientDisconnect:
+        logger.debug('GitHub webhook client disconnected before completing request')
         return JSONResponse(
-            status_code=408,
-            content={'error': 'Request timeout - client took too long to send data.'},
+            status_code=499,
+            content={'error': 'Client disconnected.'},
         )
     except Exception as e:
         logger.exception(f'Error processing GitHub event: {e}')

enterprise/tests/unit/server/routes/conftest.py

@@ -0,0 +1,8 @@
+"""Pytest configuration for server.routes tests.
+
+This module sets up the test environment for server routes.
+
+Note: The server.routes.integration.github module uses lazy initialization
+for external dependencies (TokenManager, GithubManager, etc.), so it can be
+imported directly without requiring environment variables to be set.
+"""

enterprise/tests/unit/server/routes/test_github_integration_routes.py

@@ -0,0 +1,210 @@
+"""Unit tests for GitHub integration routes - ClientDisconnect handling.
+
+These tests verify that ClientDisconnect exceptions are properly handled
+when the FastAPI endpoint times out before the request body can be fully
+received from the client.
+
+These tests import and test the actual github_events endpoint from
+server.routes.integration.github, mocking only external dependencies.
+
+Note: The github module uses lazy initialization for external dependencies,
+so it can be imported directly without requiring environment variables.
+"""
+
+import hashlib
+import hmac
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+from fastapi import Request
+from server.routes.integration.github import github_events
+from starlette.requests import ClientDisconnect
+
+
+@pytest.fixture
+def mock_request():
+    """Create a mock FastAPI Request object."""
+    req = MagicMock(spec=Request)
+    req.headers = {}
+    return req
+
+
+def create_valid_signature(payload: bytes, secret: str = 'test-secret') -> str:
+    """Create a valid HMAC signature for the given payload."""
+    signature = hmac.new(
+        secret.encode('utf-8'),
+        msg=payload,
+        digestmod=hashlib.sha256,
+    ).hexdigest()
+    return f'sha256={signature}'
+
+
+class TestClientDisconnect:
+    """Test cases for ClientDisconnect handling in github_events endpoint."""
+
+    @pytest.mark.asyncio
+    @patch('server.routes.integration.github.logger')
+    @patch('server.routes.integration.github._is_webhooks_enabled', return_value=True)
+    async def test_client_disconnect_returns_499(
+        self, mock_webhooks_enabled, mock_logger, mock_request
+    ):
+        """Test that ClientDisconnect is caught and returns 499 status code.
+
+        This tests the scenario where the FastAPI endpoint times out before
+        the request body can be fully received, causing starlette to raise
+        ClientDisconnect.
+        """
+        # Create a mock request that raises ClientDisconnect when body() is called
+        # This simulates what happens when the client disconnects or times out
+        mock_request.body = AsyncMock(side_effect=ClientDisconnect())
+
+        # Call the endpoint
+        response = await github_events(
+            request=mock_request,
+            x_hub_signature_256='sha256=test',
+        )
+
+        assert response.status_code == 499
+        assert response.body == b'{"error":"Client disconnected."}'
+
+    @pytest.mark.asyncio
+    @patch('server.routes.integration.github.logger')
+    @patch('server.routes.integration.github.verify_github_signature')
+    @patch('server.routes.integration.github._is_webhooks_enabled', return_value=True)
+    async def test_client_disconnect_during_json_parsing(
+        self, mock_webhooks_enabled, mock_verify_sig, mock_logger, mock_request
+    ):
+        """Test ClientDisconnect during request.json() call returns 499."""
+        payload = b'{"test": "data"}'
+        mock_request.body = AsyncMock(return_value=payload)
+        # ClientDisconnect can also happen during json parsing
+        mock_request.json = AsyncMock(side_effect=ClientDisconnect())
+        mock_verify_sig.return_value = None  # Skip signature verification
+
+        response = await github_events(
+            request=mock_request,
+            x_hub_signature_256='sha256=test',
+        )
+
+        assert response.status_code == 499
+        assert response.body == b'{"error":"Client disconnected."}'
+
+    @pytest.mark.asyncio
+    @patch('server.routes.integration.github.logger')
+    @patch('server.routes.integration.github._is_webhooks_enabled', return_value=True)
+    async def test_client_disconnect_does_not_propagate_as_unhandled_exception(
+        self, mock_webhooks_enabled, mock_logger, mock_request
+    ):
+        """Test that ClientDisconnect doesn't cause unhandled exception logging."""
+        mock_request.body = AsyncMock(side_effect=ClientDisconnect())
+
+        # The function should return normally without raising
+        response = await github_events(
+            request=mock_request,
+            x_hub_signature_256='sha256=test',
+        )
+
+        # The generic exception handler should NOT be triggered
+        # (it uses logger.exception which includes 'Error processing GitHub event')
+        mock_logger.exception.assert_not_called()
+
+        assert response.status_code == 499
+
+    @pytest.mark.asyncio
+    @patch('server.routes.integration.github.logger')
+    @patch('server.routes.integration.github._is_webhooks_enabled', return_value=True)
+    async def test_client_disconnect_is_not_caught_by_generic_exception_handler(
+        self, mock_webhooks_enabled, mock_logger, mock_request
+    ):
+        """Test that ClientDisconnect is caught by its specific handler, not the generic one.
+
+        The generic exception handler returns 400 and logs with exception().
+        ClientDisconnect should return 499 and log with debug().
+        """
+        mock_request.body = AsyncMock(side_effect=ClientDisconnect())
+
+        response = await github_events(
+            request=mock_request,
+            x_hub_signature_256='sha256=test',
+        )
+
+        # Should be 499 (ClientDisconnect), not 400 (generic exception)
+        assert response.status_code == 499
+
+        # Should use debug(), not exception()
+        mock_logger.debug.assert_called_once()
+        mock_logger.exception.assert_not_called()
+
+
+class TestWebhooksDisabled:
+    """Test cases for when webhooks are disabled."""
+
+    @pytest.mark.asyncio
+    @patch('server.routes.integration.github.logger')
+    @patch('server.routes.integration.github._is_webhooks_enabled', return_value=False)
+    async def test_webhooks_disabled_returns_200(
+        self, mock_webhooks_enabled, mock_logger, mock_request
+    ):
+        """Test that disabled webhooks return 200 with appropriate message."""
+        response = await github_events(
+            request=mock_request,
+            x_hub_signature_256='sha256=test',
+        )
+
+        assert response.status_code == 200
+        assert b'GitHub webhooks are currently disabled' in response.body
+
+
+class TestSuccessfulRequest:
+    """Test cases for successful webhook processing."""
+
+    @pytest.mark.asyncio
+    @patch('server.routes.integration.github._get_github_manager')
+    @patch('server.routes.integration.github.verify_github_signature')
+    @patch('server.routes.integration.github.logger')
+    @patch('server.routes.integration.github._is_webhooks_enabled', return_value=True)
+    async def test_successful_request_returns_200(
+        self,
+        mock_webhooks_enabled,
+        mock_logger,
+        mock_verify_sig,
+        mock_get_github_manager,
+        mock_request,
+    ):
+        """Test that a successful request returns 200."""
+        payload = b'{"installation": {"id": 123}}'
+        mock_request.body = AsyncMock(return_value=payload)
+        mock_request.json = AsyncMock(return_value={'installation': {'id': 123}})
+        mock_verify_sig.return_value = None
+        mock_github_manager = MagicMock()
+        mock_github_manager.receive_message = AsyncMock()
+        mock_get_github_manager.return_value = mock_github_manager
+
+        response = await github_events(
+            request=mock_request,
+            x_hub_signature_256='sha256=test',
+        )
+
+        assert response.status_code == 200
+        assert b'GitHub events endpoint reached successfully' in response.body
+
+    @pytest.mark.asyncio
+    @patch('server.routes.integration.github.verify_github_signature')
+    @patch('server.routes.integration.github.logger')
+    @patch('server.routes.integration.github._is_webhooks_enabled', return_value=True)
+    async def test_missing_installation_id_returns_400(
+        self, mock_webhooks_enabled, mock_logger, mock_verify_sig, mock_request
+    ):
+        """Test that missing installation ID returns 400."""
+        payload = b'{"action": "opened"}'
+        mock_request.body = AsyncMock(return_value=payload)
+        mock_request.json = AsyncMock(return_value={'action': 'opened'})
+        mock_verify_sig.return_value = None
+
+        response = await github_events(
+            request=mock_request,
+            x_hub_signature_256='sha256=test',
+        )
+
+        assert response.status_code == 400
+        assert b'Installation ID is missing' in response.body