fix(azure): resolve one authentication mode - #3689
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f89264f3ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| api_key is not None and api_key != API_KEY_SENTINEL, | ||
| azure_ad_token is not None, | ||
| azure_ad_token_provider is not None, |
There was a problem hiding this comment.
When the module-level client is forced to Azure, _load_client() materializes AZURE_OPENAI_AD_TOKEN into azure_ad_token before passing it to _AzureModuleClient; therefore an explicit openai.api_key or openai.azure_ad_token_provider is counted here alongside that ambient token and raises MutuallyExclusiveAuthError. For example, openai.api_type = "azure" with an explicit API key and an existing token environment variable now fails instead of honoring the explicit credential as the direct client does. Preserve whether the token came from the environment in this path and add a focused module-client authentication regression.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
| "import os\n" | ||
| "from unittest.mock import patch\n" | ||
| "with patch.dict(os.environ, {\n" | ||
| " 'AZURE_OPENAI_AD_TOKEN': 'fake-ambient-token',\n" | ||
| " 'OPENAI_API_TYPE': 'azure',\n" | ||
| " 'OPENAI_API_VERSION': '2024-02-01',\n" | ||
| " 'AZURE_OPENAI_ENDPOINT': 'https://azure.test',\n" | ||
| "}, clear=True):\n" | ||
| " import openai\n" | ||
| " assert openai.azure_ad_token is None\n" | ||
| " openai.api_key = 'fake-explicit-key'\n" | ||
| " client = openai.models._client\n" | ||
| " assert client._azure_ad_token is None\n" | ||
| " assert client.api_key == 'fake-explicit-key'\n" | ||
| " client.close()\n", |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass review confirms Azure authentication-mode precedence, sync/async providers, module-client behavior, copies, and CI are sound. One small required cleanup remains: the newly introduced B018 suppressions are unnecessary and conflict with the repository instruction to prefer straightforward lint-compliant tests.
| openai.api_key = "fake-explicit-key" | ||
| openai.azure_ad_token_provider = lambda: "fake-explicit-token" | ||
| with pytest.raises(MutuallyExclusiveAuthError): | ||
| openai.models._client # noqa: B018 |
There was a problem hiding this comment.
[Low] Remove the two avoidable B018 suppressions. This new line and line 274 both use # noqa: B018, although _ = openai.models._client evaluates the same lazy property, preserves the surrounding exception assertion, and passes Ruff without suppression. Please replace both bare accesses with that compliant form and remove both directives.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass re-review confirms both requested B018 suppressions were removed with the straightforward _ = openai.models._client form, and the follow-up changes only those two test lines. Azure synchronous/asynchronous credential precedence, environment fallback, provider refresh, module configuration, copies, and Realtime authentication remain sound. CI and CodeQL are green; prior concerns are resolved.
2 participants
Summary
MutuallyExclusiveAuthError.No dependency or exported client API changes. This is independent of the Azure redirect-transport changes in #3684.