diff --git a/openhands/integrations/protocols/http_client.py b/openhands/integrations/protocols/http_client.py index 21ec1857e0..09bdf42953 100644 --- a/openhands/integrations/protocols/http_client.py +++ b/openhands/integrations/protocols/http_client.py @@ -89,6 +89,32 @@ class HTTPClient(ABC): elif e.response.status_code == 429: logger.warning(f'Rate limit exceeded on {self.provider} API: {e}') return RateLimitError(f'{self.provider} API rate limit exceeded') + elif e.response.status_code == 403: + # GitHub uses 403 for both rate limiting and other forbidden errors + # Check if it's a rate limit by examining the response + try: + # Try to get JSON response body + if hasattr(e.response, 'json'): + if callable(e.response.json): + error_data = e.response.json() + else: + # If json is not callable, try calling it anyway (Mock objects) + try: + error_data = e.response.json() + except TypeError: + # json is a property, not a method + error_data = e.response.json + + error_message = error_data.get('message', '') if isinstance(error_data, dict) else '' + if 'rate limit' in error_message.lower() or 'API rate limit' in error_message: + logger.warning(f'Rate limit exceeded on {self.provider} API: {e}') + return RateLimitError(f'{self.provider} API rate limit exceeded') + except Exception as json_error: + logger.debug(f'Failed to parse 403 error response as JSON: {json_error}') + pass # If we can't parse the response, treat as unknown error + + logger.warning(f'Forbidden error on {self.provider} API: {e}') + return UnknownException(f'Forbidden error: {e}') logger.warning(f'Status error on {self.provider} API: {e}') return UnknownException(f'Unknown error: {e}') diff --git a/tests/unit/integrations/github/test_github_rate_limit.py b/tests/unit/integrations/github/test_github_rate_limit.py new file mode 100644 index 0000000000..12a19e9f05 --- /dev/null +++ b/tests/unit/integrations/github/test_github_rate_limit.py @@ -0,0 +1,91 @@ +""" +Unit tests for GitHub rate limit error handling. + +Tests cover: +1. Rate limit error detection (403 with rate limit message) +2. Permission denied error handling (403 without rate limit) +""" + +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from openhands.integrations.github.service.repos import GithubReposService +from openhands.integrations.protocols.http_client import RateLimitError, UnknownException +from openhands.server.shared import AppMode + + +@pytest.fixture +def repos_service(): + """Create a GithubReposService instance for testing.""" + service = GithubReposService() + service.BASE_URL = 'https://api.github.com' + return service + + +class TestRateLimitHandling: + """Test proper handling of GitHub rate limit errors (403 responses).""" + + @pytest.mark.asyncio + async def test_rate_limit_error_detection(self, repos_service): + """ + Test that 403 with rate limit message is properly detected and raised as RateLimitError. + + GitHub returns 403 (not 429) when rate limit is exceeded. + The error message contains "rate limit" or "API rate limit exceeded". + """ + # Mock a 403 response with rate limit message + mock_response = Mock() + mock_response.status_code = 403 + mock_response.json.return_value = { + 'message': 'API rate limit exceeded for 34.70.174.52. (But here\'s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)', + 'documentation_url': 'https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting', + } + + with patch.object( + repos_service, '_make_request', side_effect=RateLimitError(mock_response) + ): + with pytest.raises(RateLimitError) as exc_info: + await repos_service.search_repositories( + query='test', + per_page=100, + sort='stars', + order='desc', + public=False, + app_mode=AppMode.SAAS, + ) + + # Verify it's a RateLimitError, not UnknownException + assert isinstance(exc_info.value, RateLimitError) + + @pytest.mark.asyncio + async def test_forbidden_error_non_rate_limit(self, repos_service): + """ + Test that 403 without rate limit message is raised as UnknownException. + + Not all 403 errors are rate limits - could be permission denied, etc. + """ + # Mock a 403 response without rate limit keywords + mock_response = Mock() + mock_response.status_code = 403 + mock_response.json.return_value = { + 'message': 'Resource not accessible by integration', + 'documentation_url': 'https://docs.github.com/rest/reference/repos', + } + + with patch.object( + repos_service, '_make_request', side_effect=UnknownException(mock_response) + ): + with pytest.raises(UnknownException) as exc_info: + await repos_service.search_repositories( + query='test', + per_page=100, + sort='stars', + order='desc', + public=False, + app_mode=AppMode.SAAS, + ) + + # Verify it's UnknownException, not RateLimitError + assert isinstance(exc_info.value, UnknownException) + assert not isinstance(exc_info.value, RateLimitError)