Skip to content

bug: fix unreachable None guard in apikey_from_env#434

Open
NishchayMahor wants to merge 1 commit into
Portkey-AI:mainfrom
NishchayMahor:fix/apikey-from-env-none-guard
Open

bug: fix unreachable None guard in apikey_from_env#434
NishchayMahor wants to merge 1 commit into
Portkey-AI:mainfrom
NishchayMahor:fix/apikey-from-env-none-guard

Conversation

@NishchayMahor

Copy link
Copy Markdown

What

apikey_from_env has a if provider is None: return "" guard, but it sits after the line that dereferences provider, so it's unreachable — a None provider crashes instead:

def apikey_from_env(provider):
    env_key = f"{provider.upper().replace('-', '_')}_API_KEY"   # AttributeError if provider is None
    if provider is None:                                        # dead code — never reached for None
        return ""
    ...
apikey_from_env(None)   # AttributeError: 'NoneType' object has no attribute 'upper'

This is reachable through LLMOptions.parse_api_key, which calls apikey_from_env(values.get("provider", "")); constructing LLMOptions with provider=None (and no api_key/virtual_key) crashes with an opaque AttributeError instead of the intended empty-string behavior.

Fix

Move the None check above the dereference, so the guard actually runs:

def apikey_from_env(provider):
    if provider is None:
        return ""
    env_key = f"{provider.upper().replace('-', '_')}_API_KEY"
    ...

Testing

Added tests/test_utils.py (offline, no network/API key): asserts apikey_from_env(None) == "", plus positive cases (env lookup and dash normalization via monkeypatch.setenv). pytest tests/test_utils.py → 3 passed; the None case raises AttributeError on main without this change. black/ruff clean.

apikey_from_env computed env_key = provider.upper()... before the
'if provider is None: return ""' guard, so apikey_from_env(None) raised
AttributeError instead of returning "" (the guard was dead code). This is
reachable via LLMOptions.parse_api_key when provider is None. Move the None
check above the dereference, and add offline unit tests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a crash in apikey_from_env when provider=None by moving the None guard before dereferencing provider, and adds unit tests to cover the regression and env var normalization behavior.

Changes:

  • Reorders the provider is None guard in apikey_from_env to prevent an AttributeError for None providers.
  • Adds tests/test_utils.py with coverage for None, env lookup, and dash-to-underscore normalization.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
portkey_ai/api_resources/utils.py Moves the None check ahead of the provider.upper() dereference to avoid a runtime crash.
tests/test_utils.py Adds regression/unit tests for apikey_from_env behavior (None handling + env key normalization).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 387 to +390
def apikey_from_env(provider: Union[ProviderTypes, ProviderTypesLiteral, str]) -> str:
env_key = f"{provider.upper().replace('-', '_')}_API_KEY"
if provider is None:
return ""
env_key = f"{provider.upper().replace('-', '_')}_API_KEY"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants