fix: improve error handling in get_blocklisted_functions

[KRRT7]

Summary

• Fix improper error handling in get_blocklisted_functions() that was causing excessive Sentry alerts
• Only report actual server errors (500+) to Sentry, not expected cases like 401/404

Context

Issue PYTHON-CLI-2T5 has 10,769 occurrences since April 2025. The endpoint works correctly, but the CLI was treating all non-2xx responses as exceptions worth reporting.

[aseembits93]

repeat of previous condition

[aseembits93]

@claude review this

[claude]

Claude finished @aseembits93's task in 1m 7s —— View job

---

Code Review Complete

• Read the changed file
• View the PR diff
• Analyze error handling logic
• Provide comprehensive review feedback

---

Summary

Good improvement to reduce Sentry noise! The changes properly distinguish between expected client errors (401/404) and actual server errors (500+). However, there are several issues that need to be addressed:

Issues Found

1. Critical: Type Error on Line 364 🔴

Location: codeflash/api/cfapi.py:364

if "error" in content.lower():

Problem: content is typed as dict[str, list[str]] but you're calling .lower() on it, which will raise an AttributeError at runtime. You can't call .lower() on a dictionary.

Impact: This will cause the function to crash and fall through to the exception handler, defeating the purpose of the improved error handling.

Fix: You likely meant to check if "error" is a key in the dictionary:

if "error" in content:

Or if you want to check the string representation:

if "error" in str(content).lower():

Fix this →

2. Logic Issue: Duplicate Condition on Line 359 ⚠️

Location: codeflash/api/cfapi.py:356-360

elif not req.ok:
    logger.warning(f"Unexpected response {req.status_code} from verify-existing-optimizations")

if not req.ok:
    return {}

Problem: As @aseembits93 correctly noted in the review comments, the if not req.ok: on line 359 is redundant. This condition was already checked in the elif above. This means:

• Lines 349-357 will never reach line 359 with req.ok == True
• The second check is unnecessary

Impact: Code redundancy and potential confusion. While not a bug, it makes the logic harder to follow.

Suggestion: Remove the redundant check and simplify:

        if req.status_code >= 500:
            logger.error(f"Server error getting blocklisted functions: {req.status_code}")
            sentry_sdk.capture_message(f"Server error in verify-existing-optimizations: {req.status_code}")
            return {}
        elif req.status_code == 401:
            logger.debug(f"Not authorized to check blocklisted functions for {owner}/{repo} PR #{pr_number}")
            return {}
        elif req.status_code == 404:
            logger.debug(f"PR #{pr_number} not found for {owner}/{repo}")
            return {}
        elif not req.ok:
            logger.warning(f"Unexpected response {req.status_code} from verify-existing-optimizations")
            return {}

Fix this →

3. Edge Case: Response Body Might Not Be JSON ⚠️

Location: codeflash/api/cfapi.py:362

content: dict[str, list[str]] = req.json()

Problem: If the server returns a non-200 status that isn't caught above (e.g., 3xx redirects, or if the logic changes), calling .json() might fail if the response body isn't valid JSON.

Impact: Could raise a ValueError or JSONDecodeError and fall through to the exception handler.

Suggestion: Add a try-except around the JSON parsing or verify the response is actually JSON:

try:
    content: dict[str, list[str]] = req.json()
except (ValueError, requests.exceptions.JSONDecodeError):
    logger.warning(f"Invalid JSON response from verify-existing-optimizations")
    return {}

Fix this →

Positive Aspects ✅

1. Correct Sentry filtering: Only reporting 500+ errors to Sentry is the right approach - this will dramatically reduce the noise
2. Appropriate logging levels: Using debug for expected cases (401/404) and error for server issues is good practice
3. Using suppress_errors=True: Correctly prevents double-logging in make_cfapi_request
4. Graceful degradation: Always returning {} on errors allows the CLI to continue without blocklist information

Recommendations

1. Fix the critical .lower() bug (issue setup github actions #1) - this will break at runtime
2. Remove the duplicate condition (issue Refactor optimizer into FunctionOptimizer class #2) for cleaner code
3. Add JSON parsing protection (issue compare stdout #3) for robustness
4. Consider adding a test: This error handling path would benefit from a unit test to verify the 401/404/500 cases don't trigger Sentry

Testing Suggestion

Consider adding test cases like:

def test_get_blocklisted_functions_401_no_sentry():
    # Verify 401 doesn't call sentry_sdk.capture_message
    
def test_get_blocklisted_functions_500_reports_sentry():
    # Verify 500 does call sentry_sdk.capture_message

---