response is initialized to None at line 2804 before the async with block. If httpx.AsyncClient() construction fails (line 2805), the exception propagates immediately without entering the block body, leaving response = None. However, that exception would be caught by the outer try/except at line 788 in get_generic_sso_response, preventing execution from ever reaching the guard at line 2820.

The guard at lines 2820–2827 cannot be reached in practice because:

1. If AsyncClient() construction succeeds, the async with body is entered
2. Inside the body, either response is assigned (line 2807) or a ProxyException is raised (line 2818)
3. If AsyncClient() construction fails, the exception propagates before the guard is reached

The comment misleadingly suggests this guard handles the construction-failure case, but that scenario would cause the exception to propagate to the outer handler, not continue to line 2820. The guard is dead code and the accompanying comment creates a false expectation for future maintainers.

Suggested fix: Remove the unreachable guard, or restructure exception handling to actually reach this point (e.g., catch construction failures at a higher level before delegating to _pkce_token_exchange).

Stale empty-valued cache entry is never cleaned up in non-strict mode.

When a cached dict contains an empty/null code_verifier value, the code logs a warning indicating a potential storage bug, but the stale cache entry is never deleted. The issue: when code_verifier is falsy, the cache key is not returned to the caller, so _delete_pkce_verifier is never called.

The warning message already documents this as "may indicate a storage bug," but provides no cleanup action. Unlike the strict-mode path which raises an error, the non-strict path allows the request to continue while leaving the corrupted cache entry in memory until its 600-second TTL expires.

Consider implementing best-effort cleanup of the stale entry in the non-strict path after logging the warning, so corrupted or partially-stored verifiers are actively removed rather than left to natural TTL expiry.

Missing async mock setup causes silent cleanup failure in test

This test creates a MagicMock for the cache without setting up async_delete_cache as an AsyncMock. When the test runs with unexpected format data (integer 12345), the production code calls _delete_pkce_verifier() which attempts to await the delete method. Since the mock doesn't provide an awaitable, a TypeError is raised and silently caught by the exception handler — meaning the cleanup path never actually executes.

A regression that breaks the cleanup code would go completely undetected by this test.

Recommendation: Configure the mock to properly handle the async cleanup call:

1. Add mock_cache.async_delete_cache = AsyncMock() after line 3919
2. Add an assertion after the test to verify cleanup was called with the correct cache key
3. This matches the pattern already correctly used in test_prepare_token_exchange_parameters around line 3142

PKCE warning fires on every config hot-reload, not just startup

load_config() is called on startup and on every config-file hot-reload. This means the warning at lines 3033-3040 will emit repeatedly in long-running deployments that use dynamic config reloads, cluttering logs with repeated "startup" advisories.

Consider guarding with a module-level sentinel so the warning fires only once per process:

_pkce_redis_warning_emitted = False

def load_config(...):
    ...
    global _pkce_redis_warning_emitted
    use_pkce = os.getenv("GENERIC_CLIENT_USE_PKCE", "false").lower() == "true"
    if use_pkce and redis_usage_cache is None and not _pkce_redis_warning_emitted:
        _pkce_redis_warning_emitted = True
        verbose_proxy_logger.warning(...)

The comment at line 785 states code_verifier is initialized "so it's visible in the except block for error-hint logic," but the except block (lines 879-933) never references this variable. The error-hint logic uses pkce_configured = os.getenv(...) and error_message = str(e) instead. The initialization is harmless but the comment creates a false expectation for future maintainers.

The comment at line 2908 states "bearer credentials (access_token, id_token, refresh_token) must always come from the token endpoint, not from userinfo," but Case 3 in the merge loop (lines 2916-2926) explicitly contradicts this by accepting any of these fields from userinfo when absent from token_response.

For example, many providers omit refresh_token from the token response (it's optional). If a non-standard provider includes refresh_token in userinfo, Case 3 would silently accept it into the merged response. A refresh_token field in userinfo is not a standard OIDC field and could allow userinfo providers to inject long-lived credentials.

Update the comment to either:

1. Explicitly document Case 3 as an intentional fallback for non-standard providers that embed credentials in userinfo (and note the security implications), or
2. Modify the loop to guard refresh_token by accepting it only from token_response, never from userinfo, regardless of key absence

Corrupt verifier not cleaned up in strict mode

In non-strict mode (line 2682), when cached_data is not None but unusable, _delete_pkce_verifier is called to evict the corrupt entry so it doesn't linger until TTL expiry. The strict-mode branches at lines 2620–2648 raise ProxyException for the same conditions (_empty_value_in_dict and unrecognized format) but skip that cleanup call.

The corrupt entry stays in Redis for up to 600 s. In strict mode operators are already aware something is wrong and may re-examine the cache for diagnosis — a dangling entry makes that harder. The fix is symmetric with the non-strict cleanup:

if _empty_value_in_dict:
    await SSOAuthenticationHandler._delete_pkce_verifier(cache_key)  # best-effort
    raise ProxyException(
        message=...,
        ...
    )
elif cached_data is not None:
    await SSOAuthenticationHandler._delete_pkce_verifier(cache_key)  # best-effort
    verbose_proxy_logger.error(...)
    raise ProxyException(...)

Since _delete_pkce_verifier already swallows its own exceptions, this does not change the raise semantics.

response consumed outside async with block — comment contradicts actual scope

response is assigned inside the async with httpx.AsyncClient() as http_client: block (line ~2807) but consumed (.status_code, .text, .json()) after the block closes. The inline comment correctly notes that httpx buffers the full response body before __aexit__, so reading these attributes after the client is closed is safe today.

The risk is forward-maintenance: a future refactor to streaming (stream=True) would silently break these post-block accesses. Consider moving the non-200 check, JSON parse, and access_token validation inside the async with block to make the lifetime dependency explicit, or at minimum add a short explanatory comment near the consumption site reinforcing the buffering guarantee:

# response.status_code / .text / .json() are safe here: httpx buffers the full
# body before __aexit__, so these attributes remain valid after the client closes.
if response.status_code != 200:

async_delete_cache not set up as AsyncMock — cleanup call silently swallowed

mock_cache is a plain MagicMock(). When the non-strict path calls await SSOAuthenticationHandler._delete_pkce_verifier(cache_key), it internally awaits active_cache.async_delete_cache(...). Because mock_cache.async_delete_cache is a regular MagicMock (not an AsyncMock), the await raises a TypeError that is silently swallowed by _delete_pkce_verifier's own except Exception.

The cleanup branch is therefore never actually exercised. A regression that breaks the cleanup dispatch in this path would pass the test silently.

Fix: configure async_delete_cache as an AsyncMock and assert it is called:

mock_cache.async_delete_cache = AsyncMock()
# … run the test …
mock_cache.async_delete_cache.assert_called_once()

This matches the pattern already used correctly in test_prepare_token_exchange_parameters (around line 3142).

redirect_uri not asserted in POST body

fake_post thoroughly validates credential placement (auth, code_verifier, absent client_id/client_secret) but does not assert that redirect_uri was forwarded when redirect_url is non-None. Since the production code includes redirect_uri in token_data only via the if redirect_url: guard at line 2780, accidentally removing that guard would silently omit redirect_uri from the POST body — and many OAuth providers reject requests with a redirect_uri_mismatch error in that case.

Add the assertion inside fake_post:

The same gap exists in test_pkce_token_exchange_credentials_in_body at line 3481 and in test_pkce_token_exchange_public_client_no_secret.

Over-permissive message assertion

The message check "unavailable" in ... or "no userinfo" in ... or "userinfo" in exc_info.value.message.lower() will pass for literally any ProxyException whose message contains the word "userinfo", including unrelated errors. The third clause is so broad that it adds no signal.

The actual error raised by _get_pkce_userinfo when userinfo_endpoint is set but the endpoint returns empty/null and no id_token is present is:

"SSO user info unavailable: userinfo endpoint failed and no id_token was present in the token response."

Tighten the assertion to match the actual message that should be raised:

Non-200 userinfo response body omitted from diagnostic log

The warning logs only the HTTP status code when the userinfo endpoint returns a non-200 response. Without the response body it is impossible to distinguish "401 - token expired", "403 - insufficient scope", "429 - rate limited", or "503 - maintenance page" from a log entry alone.

Module-level sentinel persists state across tests in the same process

_pkce_no_redis_warning_emitted is a module-level boolean. Once set to True during any test that exercises load_config() with PKCE enabled and no Redis, it stays True for the remainder of the pytest session. Any later test that asserts the advisory warning fires (e.g. a future regression test for the startup warning) will silently fail to observe the log entry because the sentinel prevents it from being emitted.

Consider resetting the sentinel at the top of test setup, or scope it to the load_config call via a local parameter so tests are fully isolated:

_pkce_no_redis_warning_emitted: bool = False

The simplest fix for test authors is to reset the sentinel in test teardown:

import litellm.proxy.proxy_server as ps
ps._pkce_no_redis_warning_emitted = False

If a test for this warning is added, it should include that teardown step.