fix(exceptions): Address Greptile review feedback

- Fix Timeout (408) categorization: now correctly mapped to SERVER (retryable) instead of CLIENT
- Add type guard for status_code: handle string status codes without TypeError
- Add missing Google gRPC statuses: PERMISSION_DENIED (AUTH) and DEADLINE_EXCEEDED (SERVER)
- Refactor categorize_exception: extract helper functions to eliminate nested conditionals
- Add comprehensive tests for all edge cases (408, string status_code, new gRPC statuses)

Addresses feedback from greptile-apps bot review.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Santazuki 2026-06-10 01:43:28 +08:00
parent d110ad31f7
commit da00342b24
No known key found for this signature in database
GPG key ID: 6EF151543E7A25D2
2 changed files with 85 additions and 14 deletions

View file

@ -86,14 +86,18 @@ def google_parse_error(data: dict, status: int) -> ParsedError:
"""Google Generative AI / Vertex AI error parser.
Google returns error status strings in the response body (e.g.
'UNAUTHENTICATED', 'RESOURCE_EXHAUSTED', 'UNAVAILABLE') that
override HTTP status for categorization.
'UNAUTHENTICATED', 'PERMISSION_DENIED', 'RESOURCE_EXHAUSTED',
'UNAVAILABLE', 'DEADLINE_EXCEEDED') that override HTTP status
for categorization.
"""
err = _extract_error_body(data)
message = err.get("message")
google_status = err.get("status", "").upper()
if status in (401, 403) or google_status == "UNAUTHENTICATED":
if status in (401, 403) or google_status in (
"UNAUTHENTICATED",
"PERMISSION_DENIED",
):
return ParsedError(
category=ErrorCategory.AUTH, message=message, status_code=status
)
@ -101,7 +105,11 @@ def google_parse_error(data: dict, status: int) -> ParsedError:
return ParsedError(
category=ErrorCategory.RATE_LIMIT, message=message, status_code=status
)
if status >= 500 or google_status in ("UNAVAILABLE", "INTERNAL"):
if status >= 500 or google_status in (
"UNAVAILABLE",
"INTERNAL",
"DEADLINE_EXCEEDED",
):
return ParsedError(
category=ErrorCategory.SERVER,
message=message or "Server error",
@ -128,24 +136,58 @@ def categorize_exception(exc: Exception) -> Optional[ErrorCategory]:
if isinstance(category, ErrorCategory):
return category
# Fall back to status-code-based inference for existing exception types.
# Try status-code-based inference
status = getattr(exc, "status_code", None)
if status is not None:
if status in (401, 403):
return ErrorCategory.AUTH
if status == 429:
return ErrorCategory.RATE_LIMIT
if status >= 500:
return ErrorCategory.SERVER
if 400 <= status < 500:
return ErrorCategory.CLIENT
category_from_status = _categorize_by_status_code(status)
if category_from_status is not None:
return category_from_status
# Type-name heuristics for exceptions that lack a status_code attribute.
# Fall back to type-name heuristics
return _categorize_by_exception_name(exc)
def _categorize_by_status_code(status: any) -> Optional[ErrorCategory]:
"""Categorize error by HTTP status code.
Handles both integer and string status codes.
"""
# Normalize to integer
if not isinstance(status, int):
try:
status = int(status)
except (ValueError, TypeError):
return None
# Auth errors
if status in (401, 403):
return ErrorCategory.AUTH
# Rate limiting
if status == 429:
return ErrorCategory.RATE_LIMIT
# Server errors (including 408 Request Timeout which should be retryable)
if status == 408 or status >= 500:
return ErrorCategory.SERVER
# Client errors (4xx except auth and rate limit)
if 400 <= status < 500:
return ErrorCategory.CLIENT
return None
def _categorize_by_exception_name(exc: Exception) -> Optional[ErrorCategory]:
"""Categorize error by exception class name patterns."""
name = type(exc).__name__.lower()
if "auth" in name:
return ErrorCategory.AUTH
if "rate" in name or "throttl" in name:
return ErrorCategory.RATE_LIMIT
if "server" in name or "service" in name or "timeout" in name:
return ErrorCategory.SERVER

View file

@ -93,6 +93,16 @@ class TestGoogleParseError:
assert result.category == ErrorCategory.CLIENT
assert result.message == "Invalid argument"
def test_body_permission_denied(self):
"""PERMISSION_DENIED should map to AUTH."""
result = google_parse_error({"error": {"status": "PERMISSION_DENIED"}}, 200)
assert result.category == ErrorCategory.AUTH
def test_body_deadline_exceeded(self):
"""DEADLINE_EXCEEDED should map to SERVER (retryable)."""
result = google_parse_error({"error": {"status": "DEADLINE_EXCEEDED"}}, 200)
assert result.category == ErrorCategory.SERVER
class TestCategorizeException:
"""Integration: extract ErrorCategory from existing LiteLLM exceptions."""
@ -167,6 +177,25 @@ class TestCategorizeException:
exc.status_code = 404 # type: ignore[attr-defined]
assert categorize_exception(exc) == ErrorCategory.CLIENT
def test_exception_with_status_code_408_timeout(self):
"""408 Request Timeout should be SERVER (retryable), not CLIENT."""
exc = Exception()
exc.status_code = 408 # type: ignore[attr-defined]
assert categorize_exception(exc) == ErrorCategory.SERVER
def test_exception_with_string_status_code(self):
"""String status_code should be converted to int."""
exc = Exception()
exc.status_code = "503" # type: ignore[attr-defined]
assert categorize_exception(exc) == ErrorCategory.SERVER
def test_exception_with_invalid_status_code(self):
"""Invalid status_code should fall through to name heuristics."""
exc = Exception()
exc.status_code = "invalid" # type: ignore[attr-defined]
# Falls through to None since no name match
assert categorize_exception(exc) is None
def test_unknown_returns_none(self):
assert categorize_exception(ValueError("unexpected")) is None