Add Microsoft Entra ID authentication for Azure OpenAI#3250
Add Microsoft Entra ID authentication for Azure OpenAI#3250
Conversation
Add UnresolvedAzureAuthStrategy and ResolvedAzureAuthStrategy enums to support Entra ID authentication for azure-openai provider. Wire auth strategy detection into create_azure() based on presence of tenant_id/client_id properties vs api_key. No runtime behavior change yet - API key auth continues to work, Entra ID configs are parsed but will be wired to token acquisition in the next phase.
Add azure_identity/azure_core 0.27.0 as native-only dependencies. Create std_auth.rs with AzureAuth struct that caches credential objects globally and acquires bearer tokens at request time. Wire into build_request() behind cfg(not(wasm32)) so Entra ID clients send Authorization: Bearer instead of api-key header.
Extend JsCallbackProvider with AzureCredResult and azure channels. Add AzureCredProvider to js_callback_bridge.rs with loadAzureCreds callback. Create wasm_auth.rs as WASM counterpart to std_auth.rs. Unify build_request() to use azure_auth module alias (std_auth on native, wasm_auth on WASM). Wire loadAzureCreds into playground TS with a stub implementation for now.
Add AzureEntraId and AzureEntraIdSystemDefault client configs to integ-tests with corresponding BAML functions and TypeScript tests. Tests skip gracefully when Azure env vars are not set. Update Azure provider docs with Entra ID configuration example and param fields for tenant_id, client_id, and client_secret.
…creds Regenerate all language baml_client outputs to include new Entra ID test functions. Add Python integration test for Azure OpenAI Entra ID auth. Wire up Azure credential loading in vscode extension and update pnpm-lock.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
|
🌿 Preview your docs: https://boundary-preview-d891ae85-52d5-48d5-a551-858472b0997b.docs.buildwithfern.com |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Azure OpenAI authentication via Entra ID (service principal and system-default) and API-key paths, implements credential resolution and token acquisition (cached native, JS bridge for WASM), propagates azure_auth through OpenAI client config, and generates cross-language test clients and bindings. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Runtime as BAML Runtime / OpenAI client
participant AzureAuth as AzureAuth (std_auth / wasm_auth)
participant CredCache as Credential Cache
participant JSBridge as JS Callback Provider / Azure SDK
participant OpenAI as OpenAI API
Client->>Runtime: Invoke function with azure_auth config
Runtime->>AzureAuth: get_or_create(auth_strategy)
alt std target & cache hit
AzureAuth->>CredCache: lookup(key)
CredCache-->>AzureAuth: credential
else std target & cache miss
AzureAuth->>Azure SDK: build credential (ClientSecret / ManagedIdentity / AzureCli)
Azure SDK-->>AzureAuth: TokenCredential
AzureAuth->>CredCache: store(key, credential)
else wasm target
AzureAuth->>JSBridge: request token via loadAzureCreds
JSBridge-->>AzureAuth: access_token
end
AzureAuth->>Runtime: token() -> bearer token
Runtime->>OpenAI: HTTP request with Authorization: Bearer {token}
OpenAI-->>Runtime: Response
Runtime-->>Client: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf01657a-c2f0-4800-bc79-a1171f5d3423
⛔ Files ignored due to path filters (2)
engine/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
engine/.gitignoreengine/baml-lib/llm-client/src/clients/openai.rsengine/baml-runtime/Cargo.tomlengine/baml-runtime/src/internal/llm_client/primitive/openai/mod.rsengine/baml-runtime/src/internal/llm_client/primitive/openai/openai_client.rsengine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rsengine/baml-runtime/src/internal/llm_client/primitive/openai/wasm_auth.rsengine/baml-runtime/src/types/js_callback_provider.rsengine/baml-runtime/src/types/mod.rsengine/baml-schema-wasm/src/js_callback_bridge.rsfern/03-reference/baml/clients/providers/azure.mdxinteg-tests/baml_src/clients.bamlinteg-tests/baml_src/test-files/providers/azure.bamlinteg-tests/go/baml_client/baml_source_map.gointeg-tests/go/baml_client/functions.gointeg-tests/go/baml_client/functions_build_request.gointeg-tests/go/baml_client/functions_build_request_stream.gointeg-tests/go/baml_client/functions_parse.gointeg-tests/go/baml_client/functions_parse_stream.gointeg-tests/go/baml_client/functions_stream.gointeg-tests/openapi/baml_client/openapi.yamlinteg-tests/python-v1/baml_client/async_client.pyinteg-tests/python-v1/baml_client/inlinedbaml.pyinteg-tests/python-v1/baml_client/parser.pyinteg-tests/python-v1/baml_client/sync_client.pyinteg-tests/python/README.mdinteg-tests/python/baml_client/async_client.pyinteg-tests/python/baml_client/inlinedbaml.pyinteg-tests/python/baml_client/parser.pyinteg-tests/python/baml_client/sync_client.pyinteg-tests/python/test_azure_openai_entra.pyinteg-tests/react/baml_client/async_client.tsinteg-tests/react/baml_client/async_request.tsinteg-tests/react/baml_client/inlinedbaml.tsinteg-tests/react/baml_client/parser.tsinteg-tests/react/baml_client/react/hooks.tsxinteg-tests/react/baml_client/react/server.tsinteg-tests/react/baml_client/react/server_streaming.tsinteg-tests/react/baml_client/react/server_streaming_types.tsinteg-tests/react/baml_client/sync_client.tsinteg-tests/react/baml_client/sync_request.tsinteg-tests/ruby/baml_client/client.rbinteg-tests/rust/src/baml_client/baml_source_map.rsinteg-tests/rust/src/baml_client/functions/async_client.rsinteg-tests/rust/src/baml_client/functions/sync_client.rsinteg-tests/typescript-esm/baml_client/async_client.tsinteg-tests/typescript-esm/baml_client/async_request.tsinteg-tests/typescript-esm/baml_client/inlinedbaml.tsinteg-tests/typescript-esm/baml_client/parser.tsinteg-tests/typescript-esm/baml_client/sync_client.tsinteg-tests/typescript-esm/baml_client/sync_request.tsinteg-tests/typescript/baml_client/async_client.tsinteg-tests/typescript/baml_client/async_request.tsinteg-tests/typescript/baml_client/inlinedbaml.tsinteg-tests/typescript/baml_client/parser.tsinteg-tests/typescript/baml_client/sync_client.tsinteg-tests/typescript/baml_client/sync_request.tsinteg-tests/typescript/tests/providers/azure.test.tstypescript/apps/vscode-ext/package.jsontypescript/apps/vscode-ext/src/panels/WebviewPanelHost.tstypescript/apps/vscode-ext/src/webview-to-vscode-rpc.tstypescript/packages/playground-common/src/sdk/runtime/BamlRuntime.tstypescript/packages/playground-common/src/shared/baml-project-panel/vscode.tstypescript/packages/playground-common/src/shared/baml-project-panel/webview-to-vscode-rpc.ts
👮 Files not reviewed due to content moderation or server errors (5)
- integ-tests/typescript-esm/baml_client/inlinedbaml.ts
- integ-tests/typescript/baml_client/inlinedbaml.ts
- integ-tests/python-v1/baml_client/inlinedbaml.py
- integ-tests/rust/src/baml_client/baml_source_map.rs
- integ-tests/go/baml_client/baml_source_map.go
| let (azure_auth, api_key_for_header) = match (&api_key, &tenant_id, &client_id) { | ||
| // Both api_key and Entra ID fields provided — error | ||
| (Some(_), Some(_), _) | (Some(_), _, Some(_)) => { | ||
| properties.push_option_error( | ||
| "Cannot specify both api_key and tenant_id/client_id. Use either API key auth or Entra ID auth, not both.", | ||
| ); | ||
| (None, None) | ||
| } | ||
| // Entra ID: tenant_id or client_id present (without api_key) | ||
| (None, Some(_), _) | (None, _, Some(_)) => ( | ||
| Some(UnresolvedAzureAuthStrategy::EntraId { | ||
| tenant_id: tenant_id | ||
| .unwrap_or_else(|| StringOr::EnvVar("AZURE_TENANT_ID".into())), | ||
| client_id: client_id | ||
| .unwrap_or_else(|| StringOr::EnvVar("AZURE_CLIENT_ID".into())), | ||
| client_secret, | ||
| }), | ||
| None, | ||
| ), | ||
| // Explicit api_key provided, no Entra ID fields | ||
| (Some(_), None, None) => { | ||
| let key = api_key.clone().unwrap(); | ||
| ( | ||
| Some(UnresolvedAzureAuthStrategy::ApiKey(key.clone())), | ||
| Some(key), | ||
| ) | ||
| } | ||
| // Neither — default to API key auth (current behavior) | ||
| (None, None, None) => { | ||
| let key = StringOr::EnvVar("AZURE_OPENAI_API_KEY".to_string()); | ||
| ( | ||
| Some(UnresolvedAzureAuthStrategy::ApiKey(key.clone())), | ||
| Some(key), | ||
| ) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding unit tests for auth strategy detection logic.
The match logic determining authentication strategy has multiple branches and edge cases. Unit tests would help verify:
- Error raised when both
api_keyandtenant_id/client_idare provided EntraIdstrategy selected when only Entra ID fields presentApiKeystrategy selected when onlyapi_keypresent- Default to
ApiKeywithAZURE_OPENAI_API_KEYwhen nothing specified - Env var fallback for partial Entra ID configuration
As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".
| // Entra ID: tenant_id or client_id present (without api_key) | ||
| (None, Some(_), _) | (None, _, Some(_)) => ( | ||
| Some(UnresolvedAzureAuthStrategy::EntraId { | ||
| tenant_id: tenant_id | ||
| .unwrap_or_else(|| StringOr::EnvVar("AZURE_TENANT_ID".into())), | ||
| client_id: client_id | ||
| .unwrap_or_else(|| StringOr::EnvVar("AZURE_CLIENT_ID".into())), | ||
| client_secret, | ||
| }), | ||
| None, | ||
| ), |
There was a problem hiding this comment.
Partial Entra ID configuration silently falls back to environment variables.
When only tenant_id or only client_id is provided, the code silently defaults the missing field to an environment variable. For example, providing only tenant_id will cause client_id to default to AZURE_CLIENT_ID.
This could lead to confusing runtime errors if users don't realize partial configuration triggers env var lookup. Consider either:
- Requiring both fields when one is provided (explicit validation), or
- Documenting this fallback behavior clearly in user-facing docs.
| // Entra ID bearer token acquisition — uses azure_identity on native, JS callback bridge on WASM. | ||
| { | ||
| use internal_llm_client::openai::ResolvedAzureAuthStrategy; | ||
| match &self.properties.azure_auth { | ||
| Some( | ||
| ResolvedAzureAuthStrategy::EntraId { .. } | ||
| | ResolvedAzureAuthStrategy::SystemDefault, | ||
| ) => { | ||
| let azure_auth = super::azure_auth::AzureAuth::get_or_create( | ||
| self.properties.azure_auth.as_ref().unwrap(), | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| anyhow::anyhow!("Azure Entra ID authentication failed: {e}") | ||
| })?; | ||
| let token = azure_auth.token().await.map_err(|e| { | ||
| anyhow::anyhow!("Azure Entra ID token acquisition failed: {e}") | ||
| })?; | ||
| req = req.bearer_auth(&token); | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a unit test for this bearer-auth branch.
This is the feature’s critical path, but the local test updates in this file only add azure_auth: None; nothing here asserts that Entra ID/SystemDefault produce an Authorization header or that the Azure API-key path is skipped. A focused request-builder unit test would catch regressions without live Azure credentials.
As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible".
engine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rs
Outdated
Show resolved
Hide resolved
| const __options__ = { ...this.bamlOptions, ...(__baml_options__ || {}) } | ||
| const __signal__ = __options__.signal; | ||
|
|
||
| if (__signal__?.aborted) { | ||
| throw new BamlAbortError('Operation was aborted', __signal__.reason); | ||
| } | ||
|
|
||
| // Check if onTick is provided and reject for sync operations | ||
| if (__options__.onTick) { | ||
| throw new Error("onTick is not supported for synchronous functions. Please use the async client instead."); | ||
| } | ||
|
|
||
| const __collector__ = __options__.collector ? (Array.isArray(__options__.collector) ? __options__.collector : [__options__.collector]) : []; | ||
| const __rawEnv__ = __baml_options__?.env ? { ...process.env, ...__baml_options__.env } : { ...process.env }; | ||
| const __env__: Record<string, string> = Object.fromEntries( | ||
| Object.entries(__rawEnv__).filter(([_, value]) => value !== undefined) as [string, string][] | ||
| ); |
There was a problem hiding this comment.
Fix env propagation in these generated Entra ID sync wrappers.
Lines 7318-7321 and 7368-7371 rebuild __rawEnv__ from __baml_options__?.env only, so b.withOptions({ env: ... }) never reaches these calls. For the new Entra ID paths, that can silently drop Azure credential vars and fall back to bare process.env.
Suggested fix
- const __rawEnv__ = __baml_options__?.env ? { ...process.env, ...__baml_options__.env } : { ...process.env };
+ const __rawEnv__ = {
+ ...process.env,
+ ...(this.bamlOptions.env ?? {}),
+ ...(__baml_options__?.env ?? {}),
+ };Apply the same generator fix to both methods.
Also applies to: 7355-7371
Run cargo fmt and remove dead else-if branch in create_azure() that could never execute (api_key with Entra ID fields errors earlier, api_key alone sets api_key_for_header).
- Fix token scope from ai.azure.com to cognitiveservices.azure.com in vscode extension and Python test script - Remove unused azure_identity imports (AzureCliCredentialOptions, ClientSecretCredentialOptions) - Fix AzureCliCredential double-creation in SystemDefault chain - Replace JWT-like test fixture with plain string to avoid Gitleaks - Fix docs contradiction about api_key being "ignored" vs compile error - Remove unreachable else-if branch in create_azure() - Run cargo fmt
hellovai
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Error
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Error
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
|
🌿 Preview your docs: https://boundary-preview-c7800909-eba3-45b9-974e-f0ffca807e3e.docs.buildwithfern.com |
- Fix token scope from ai.azure.com to cognitiveservices.azure.com in vscode extension and Python test script - Remove unused azure_identity imports (AzureCliCredentialOptions, ClientSecretCredentialOptions) - Fix AzureCliCredential double-creation in SystemDefault chain - Replace JWT-like test fixture with plain string to avoid Gitleaks - Fix docs contradiction about api_key being "ignored" vs compile error - Remove unreachable else-if branch in create_azure() - Convert match single_pattern to if-let per clippy - Run cargo fmt
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
|
🌿 Preview your docs: https://boundary-preview-1eef61b6-516f-4d3a-9e49-c86e441c635f.docs.buildwithfern.com |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
typescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts (1)
729-730: 🧹 Nitpick | 🔵 TrivialCache
DefaultAzureCredentialinstead of constructing per request.Line 729 creates a new credential chain on every
LOAD_AZURE_CREDScall. Caching a class-level credential avoids repeated chain setup and matches the existing cached GCP pattern.#!/bin/bash set -euo pipefail # Verify whether DefaultAzureCredential is created per request and whether a cached member exists. rg -n --type=ts -C2 'new DefaultAzureCredential\(' typescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts rg -n --type=ts -C2 '_azureCredential' typescript/apps/vscode-ext/src/panels/WebviewPanelHost.tsinteg-tests/python/test_azure_openai_entra.py (1)
1-88:⚠️ Potential issue | 🔴 CriticalThis is not a pytest integration test and won’t run in the intended integ-test flow.
The file is currently a standalone script (
main()+if __name__ == "__main__"), so it contributes no pytest-discovered tests ininteg-tests/python. It should be rewritten as pytest test cases (and wired to the uv+maturin integration test path) so CI executes it consistently.As per coding guidelines,
integ-tests/python/**/*.py: "For Python integration tests in integ-tests/python, use uv + maturin to build and pytest to run tests".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 882e5cf5-6f04-4ccd-9a20-1e0f1b0b426a
📒 Files selected for processing (9)
engine/baml-lib/llm-client/src/clients/openai.rsengine/baml-runtime/src/internal/llm_client/primitive/openai/mod.rsengine/baml-runtime/src/internal/llm_client/primitive/openai/openai_client.rsengine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rsengine/baml-runtime/src/internal/llm_client/primitive/openai/wasm_auth.rsengine/baml-runtime/src/types/js_callback_provider.rsfern/03-reference/baml/clients/providers/azure.mdxinteg-tests/python/test_azure_openai_entra.pytypescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts
| If `client_secret` is omitted, BAML uses the system default credential chain (managed identity first, | ||
| then Azure CLI / azd CLI). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the credential behavior in EntraId mode without client_secret
rg -n -A 10 "No client_secret provided" engine/baml-runtime/Repository: BoundaryML/baml
Length of output: 1552
🏁 Script executed:
#!/bin/bash
# Check SystemDefault mode credential chain behavior
rg -n -B 5 -A 15 "ResolvedAzureAuthStrategy::SystemDefault" engine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rsRepository: BoundaryML/baml
Length of output: 2295
🏁 Script executed:
#!/bin/bash
# Check the actual documentation context around lines 81-82 and 103-104
sed -n '75,110p' fern/03-reference/baml/clients/providers/azure.mdxRepository: BoundaryML/baml
Length of output: 1279
Documentation does not match code behavior when client_secret is omitted.
The documentation claims that without client_secret, BAML uses "the system default credential chain (managed identity first, then Azure CLI / azd CLI)." However, examining the implementation in engine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rs lines 78-86, when client_secret is omitted in Entra ID mode (with tenant_id/client_id set), the code uses only ManagedIdentityCredential, not the full chain with Azure CLI fallback.
The full credential chain (managed identity → Azure CLI) is implemented only in SystemDefault mode (lines 88-103), which requires all three fields (tenant_id, client_id, client_secret) to be omitted.
Update lines 81-82 and 103-104 to clarify the two distinct authentication paths: service principal when all three fields are set, managed identity only when client_secret is omitted with Entra ID, and the full credential chain only when using system default authentication (all fields omitted).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
engine/baml-runtime/src/internal/llm_client/primitive/openai/openai_client.rs (1)
449-472: 🧹 Nitpick | 🔵 TrivialConsider guarding against double bearer auth (defensive coding).
If both
api_key(line 450) and Entra ID auth (line 470) are configured, bothbearer_authcalls execute and the second overwrites the first. While the PR summary states the parser prevents this combination, adding an explicit guard orelsebranch would make the mutual exclusivity clearer at runtime and prevent subtle bugs if parser validation is bypassed (e.g., dynamic clients).💡 Suggested defensive refactor
if let Some(key) = &self.properties.api_key { req = req.bearer_auth(key.render(expose_secrets)); - } - - // Entra ID bearer token acquisition — uses azure_identity on native, JS callback bridge on WASM. - { + } else if let Some( + ResolvedAzureAuthStrategy::EntraId { .. } + | ResolvedAzureAuthStrategy::SystemDefault, + ) = &self.properties.azure_auth + { + // Entra ID bearer token acquisition — uses azure_identity on native, JS callback bridge on WASM. use internal_llm_client::openai::ResolvedAzureAuthStrategy; - if let Some( - ResolvedAzureAuthStrategy::EntraId { .. } - | ResolvedAzureAuthStrategy::SystemDefault, - ) = &self.properties.azure_auth - { - let azure_auth = super::azure_auth::AzureAuth::get_or_create( - self.properties.azure_auth.as_ref().unwrap(), - ) - .await - .map_err(|e| anyhow::anyhow!("Azure Entra ID authentication failed: {e}"))?; - let token = azure_auth - .token() - .await - .map_err(|e| anyhow::anyhow!("Azure Entra ID token acquisition failed: {e}"))?; - req = req.bearer_auth(&token); - } + let azure_auth = super::azure_auth::AzureAuth::get_or_create( + self.properties.azure_auth.as_ref().unwrap(), + ) + .await + .map_err(|e| anyhow::anyhow!("Azure Entra ID authentication failed: {e}"))?; + let token = azure_auth + .token() + .await + .map_err(|e| anyhow::anyhow!("Azure Entra ID token acquisition failed: {e}"))?; + req = req.bearer_auth(&token); }
Add a unit test for the bearer-auth branch.
This is the feature's critical path, but the existing tests only set
azure_auth: None. A focused unit test asserting that Entra ID / SystemDefault produces anAuthorization: Bearerheader would catch regressions without requiring live Azure credentials.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cb62249-b089-4b4d-b303-8f7db6e7cb10
📒 Files selected for processing (1)
engine/baml-runtime/src/internal/llm_client/primitive/openai/openai_client.rs
- Rename test_azure_openai_entra.py to azure_openai_entra_example.py so pytest doesn't discover it (standalone script, not a test) - Fix docs: client_secret omitted uses ManagedIdentity only, not the full credential chain (chain is only for SystemDefault mode) - Normalize Azure error payload in WebviewPanelHost.ts to match the LoadAzureCredsResponse type shape - Clean up README duplicate line
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
hellovai
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
|
🌿 Preview your docs: https://boundary-preview-4e228d37-d541-4e65-bbcf-50f8c6e46075.docs.buildwithfern.com |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
integ-tests/python/azure_openai_entra_example.py (1)
9-19:⚠️ Potential issue | 🟠 MajorThis integration-test file bypasses the repo’s pytest + maturin workflow.
This is implemented as a standalone script (
main()), not a pytest test entrypoint, so it won’t participate in the expected integration-test execution path. Please either convert it to a pytest test module or move it outsideinteg-tests/pythonif it is intentionally just an example.As per coding guidelines, "For Python integration tests in integ-tests/python, use uv + maturin to build and pytest to run tests".
Also applies to: 34-88
typescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts (1)
726-730: 🧹 Nitpick | 🔵 TrivialReuse a cached
DefaultAzureCredentialinstance instead of recreating it per call.Line 729 creates a new credential each request; this repeats credential-chain initialization and can reduce token-cache effectiveness. This same concern was raised earlier and is still applicable.
#!/bin/bash # Verify whether DefaultAzureCredential is recreated in handlers # and whether a class-level cached credential exists. rg -n --type=ts -C3 '\bDefaultAzureCredential\b|_azureCredential|getToken\('
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3cce3f9-9538-4752-87ea-a004d18db5a6
📒 Files selected for processing (4)
fern/03-reference/baml/clients/providers/azure.mdxinteg-tests/python/README.mdinteg-tests/python/azure_openai_entra_example.pytypescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts
…re/identity, cache credential - Fix all stale filename references in azure_openai_entra_example.py - Add TestAzureEntraId/SystemDefault to TestAzureClients test suite - Use env.AZURE_OPENAI_API_VERSION instead of hard-coded api_version - Use it.skip pattern in azure.test.ts for proper skip reporting - Upgrade @azure/identity to ^4.2.1 (CVE-2024-35255 fix) - Cache DefaultAzureCredential as class member in WebviewPanelHost - Log warning on Azure credential cache lock poisoning - Clarify partial Entra ID config env var fallback in docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hellovai
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Failure
hellovai
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Failure
hellovai
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
integ-tests/python/azure_openai_entra_example.py (1)
1-19:⚠️ Potential issue | 🟠 MajorConvert this script into a pytest-discoverable integration test (or move it out of
integ-tests/python).This file is currently executable-example style (
main()+__main__) and won’t run as a standard pytest integration test in the required test workflow.As per coding guidelines "integ-tests/python/**/*.py: For Python integration tests in integ-tests/python, use uv + maturin to build and pytest to run tests".
Also applies to: 34-88
engine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rs (1)
27-37: 🧹 Nitpick | 🔵 TrivialAdd unit tests for
cache_keylogic.This is a pure function that maps auth strategies to cache keys and is ideal for unit testing without mocking dependencies. As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible."
🧪 Example test module
#[cfg(test)] mod tests { use super::*; #[test] fn test_cache_key_api_key() { let strategy = ResolvedAzureAuthStrategy::ApiKey; assert_eq!(AzureAuth::cache_key(&strategy), "api_key"); } #[test] fn test_cache_key_entra_id() { let strategy = ResolvedAzureAuthStrategy::EntraId { tenant_id: "tenant-123".to_string(), client_id: "client-456".to_string(), client_secret: Some("secret".to_string()), }; assert_eq!(AzureAuth::cache_key(&strategy), "tenant-123:client-456"); } #[test] fn test_cache_key_system_default() { let strategy = ResolvedAzureAuthStrategy::SystemDefault; assert_eq!(AzureAuth::cache_key(&strategy), "system_default"); } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f39a7f45-fe9b-4471-b7f7-e96f900bb360
📒 Files selected for processing (8)
engine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rsfern/03-reference/baml/clients/providers/azure.mdxinteg-tests/baml_src/clients.bamlinteg-tests/baml_src/test-files/providers/azure.bamlinteg-tests/python/azure_openai_entra_example.pyinteg-tests/typescript/tests/providers/azure.test.tstypescript/apps/vscode-ext/package.jsontypescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts
| Err(e) => { | ||
| errors.push(format!("ManagedIdentityCredential (init): {e}")); | ||
| log::debug!( | ||
| "Azure SystemDefault: ManagedIdentityCredential init failed, trying AzureCliCredential" | ||
| ); | ||
| AzureCliCredential::new(None) | ||
| .context("Failed to create AzureCliCredential")? | ||
| } |
There was a problem hiding this comment.
Missing token probe for AzureCliCredential in init-failure path.
When ManagedIdentityCredential::new(None) fails, the code falls back to AzureCliCredential but does not probe it with get_token before caching. This is inconsistent with the other fallback path (lines 112-128) where the CLI credential is tested.
This could cache a non-working credential if the user hasn't run az login, leading to confusing failures on subsequent requests. Additionally, if AzureCliCredential::new fails, the error message doesn't include the accumulated errors from the previous failures.
🐛 Proposed fix to add token probe and improve error reporting
Err(e) => {
errors.push(format!("ManagedIdentityCredential (init): {e}"));
log::debug!(
"Azure SystemDefault: ManagedIdentityCredential init failed, trying AzureCliCredential"
);
- AzureCliCredential::new(None)
- .context("Failed to create AzureCliCredential")?
+ let cli_cred = match AzureCliCredential::new(None) {
+ Ok(c) => c,
+ Err(e) => {
+ errors.push(format!("AzureCliCredential (init): {e}"));
+ anyhow::bail!(
+ "Azure SystemDefault credential chain exhausted. Tried:\n{}",
+ errors.join("\n")
+ );
+ }
+ };
+ match cli_cred.get_token(&[AZURE_OPENAI_TOKEN_SCOPE], None).await {
+ Ok(_) => {
+ log::debug!(
+ "Azure SystemDefault: AzureCliCredential succeeded"
+ );
+ cli_cred
+ }
+ Err(e) => {
+ errors.push(format!("AzureCliCredential: {e}"));
+ anyhow::bail!(
+ "Azure SystemDefault credential chain exhausted. Tried:\n{}",
+ errors.join("\n")
+ );
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Err(e) => { | |
| errors.push(format!("ManagedIdentityCredential (init): {e}")); | |
| log::debug!( | |
| "Azure SystemDefault: ManagedIdentityCredential init failed, trying AzureCliCredential" | |
| ); | |
| AzureCliCredential::new(None) | |
| .context("Failed to create AzureCliCredential")? | |
| } | |
| Err(e) => { | |
| errors.push(format!("ManagedIdentityCredential (init): {e}")); | |
| log::debug!( | |
| "Azure SystemDefault: ManagedIdentityCredential init failed, trying AzureCliCredential" | |
| ); | |
| let cli_cred = match AzureCliCredential::new(None) { | |
| Ok(c) => c, | |
| Err(e) => { | |
| errors.push(format!("AzureCliCredential (init): {e}")); | |
| anyhow::bail!( | |
| "Azure SystemDefault credential chain exhausted. Tried:\n{}", | |
| errors.join("\n") | |
| ); | |
| } | |
| }; | |
| match cli_cred.get_token(&[AZURE_OPENAI_TOKEN_SCOPE], None).await { | |
| Ok(_) => { | |
| log::debug!( | |
| "Azure SystemDefault: AzureCliCredential succeeded" | |
| ); | |
| cli_cred | |
| } | |
| Err(e) => { | |
| errors.push(format!("AzureCliCredential: {e}")); | |
| anyhow::bail!( | |
| "Azure SystemDefault credential chain exhausted. Tried:\n{}", | |
| errors.join("\n") | |
| ); | |
| } | |
| } | |
| } |
|
|
||
| Not used when `tenant_id` or `client_id` is set (Entra ID auth takes over). | ||
| </ParamField> |
There was a problem hiding this comment.
Inconsistent logic operator with Note at lines 42-44.
Line 68 uses "or" (Not used when tenant_id or client_id is set), but the Note at line 42 was corrected to use "and" (When tenant_id and client_id are present). This creates confusion about whether setting only one of these fields triggers Entra ID mode.
Given the env var fallback behavior described in lines 78-79 and 92-93, it appears either field can initiate Entra ID auth (with the other falling back to env). If so, "or" is correct here but the Note should be updated. Otherwise, align this with the Note.
📝 Suggested fix (if both are required)
- Not used when `tenant_id` or `client_id` is set (Entra ID auth takes over).
+ Not used when `tenant_id` and `client_id` are set (Entra ID auth takes over).| If `client_secret` is also provided, BAML uses service principal (client credentials) authentication. | ||
| If `client_secret` is omitted, BAML uses managed identity authentication only | ||
| (via `ManagedIdentityCredential` — suitable for Azure VMs, App Service, and AKS). |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor style: Three successive sentences begin with "If".
Consider varying the sentence structure for improved readability. This is a minor style concern flagged by static analysis.
✏️ Suggested revision
If `client_secret` is also provided, BAML uses service principal (client credentials) authentication.
- If `client_secret` is omitted, BAML uses managed identity authentication only
+ Without `client_secret`, BAML uses managed identity authentication only
(via `ManagedIdentityCredential` — suitable for Azure VMs, App Service, and AKS).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| If `client_secret` is also provided, BAML uses service principal (client credentials) authentication. | |
| If `client_secret` is omitted, BAML uses managed identity authentication only | |
| (via `ManagedIdentityCredential` — suitable for Azure VMs, App Service, and AKS). | |
| If `client_secret` is also provided, BAML uses service principal (client credentials) authentication. | |
| Without `client_secret`, BAML uses managed identity authentication only | |
| (via `ManagedIdentityCredential` — suitable for Azure VMs, App Service, and AKS). |
🧰 Tools
🪛 LanguageTool
[style] ~82-~82: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... (client credentials) authentication. If client_secret is omitted, BAML uses m...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
| // Azure client using Entra ID system default credential chain | ||
| // Tries ManagedIdentityCredential first, then DeveloperToolsCredential (Azure CLI / azd) | ||
| // No max_tokens so server default is used (works for both GPT-5 and o1-style deployments) | ||
| client<llm> AzureEntraIdSystemDefault { | ||
| provider azure-openai | ||
| options { | ||
| resource_name env.AZURE_OPENAI_RESOURCE_NAME | ||
| deployment_id env.AZURE_OPENAI_DEPLOYMENT_ID | ||
| api_version env.AZURE_OPENAI_API_VERSION | ||
| tenant_id env.AZURE_TENANT_ID | ||
| client_id env.AZURE_CLIENT_ID | ||
| max_tokens null | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of AzureEntraIdSystemDefault to assess rename impact
rg -n "AzureEntraIdSystemDefault" --type-add 'baml:*.baml' --type baml
rg -n "AzureEntraIdSystemDefault" --type ts --type pyRepository: BoundaryML/baml
Length of output: 50372
Update client name and comment for accuracy.
The client name AzureEntraIdSystemDefault is misleading. When tenant_id and client_id are set without client_secret, the client uses ManagedIdentityCredential only—not the full credential chain (managed identity → developer tools). The full chain is only used in true "SystemDefault" mode where all three Entra ID fields are omitted.
Rename the client to AzureEntraIdManagedIdentity and update the comment:
Suggested fix
-// Azure client using Entra ID system default credential chain
-// Tries ManagedIdentityCredential first, then DeveloperToolsCredential (Azure CLI / azd)
+// Azure client using Entra ID managed identity authentication
+// Uses ManagedIdentityCredential only (suitable for Azure VMs, App Service, AKS)
// No max_tokens so server default is used (works for both GPT-5 and o1-style deployments)
-client<llm> AzureEntraIdSystemDefault {
+client<llm> AzureEntraIdManagedIdentity {
provider azure-openai📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Azure client using Entra ID system default credential chain | |
| // Tries ManagedIdentityCredential first, then DeveloperToolsCredential (Azure CLI / azd) | |
| // No max_tokens so server default is used (works for both GPT-5 and o1-style deployments) | |
| client<llm> AzureEntraIdSystemDefault { | |
| provider azure-openai | |
| options { | |
| resource_name env.AZURE_OPENAI_RESOURCE_NAME | |
| deployment_id env.AZURE_OPENAI_DEPLOYMENT_ID | |
| api_version env.AZURE_OPENAI_API_VERSION | |
| tenant_id env.AZURE_TENANT_ID | |
| client_id env.AZURE_CLIENT_ID | |
| max_tokens null | |
| } | |
| } | |
| // Azure client using Entra ID managed identity authentication | |
| // Uses ManagedIdentityCredential only (suitable for Azure VMs, App Service, AKS) | |
| // No max_tokens so server default is used (works for both GPT-5 and o1-style deployments) | |
| client<llm> AzureEntraIdManagedIdentity { | |
| provider azure-openai | |
| options { | |
| resource_name env.AZURE_OPENAI_RESOURCE_NAME | |
| deployment_id env.AZURE_OPENAI_DEPLOYMENT_ID | |
| api_version env.AZURE_OPENAI_API_VERSION | |
| tenant_id env.AZURE_TENANT_ID | |
| client_id env.AZURE_CLIENT_ID | |
| max_tokens null | |
| } | |
| } |
| "update-test:syntaxes": "vscode-tmgrammar-snap --config package.json syntaxes/all.test.baml --updateSnapshot" | ||
| }, | ||
| "dependencies": { | ||
| "@azure/identity": "^4.2.1", |
There was a problem hiding this comment.
Update the lockfile with this dependency change to unblock CI.
Adding @azure/identity at Line 214 requires committing the updated pnpm-lock.yaml; CI is currently failing with frozen-lockfile specifier mismatch.
1 participant
Summary
azure-openaiprovider, enabling enterprise customers to authenticate via service principal credentials, managed identity, or Azure CLItenant_id/client_idproperty presence — errors if both API key and Entra ID fields are specifiedazure_identitycrate with global credential caching; WASM support via JS callback bridge for playgroundTest plan
AZURE_TENANT_ID,AZURE_CLIENT_ID,AZURE_CLIENT_SECRETset for a real Azure service principalcargo build -p baml-runtimeandcargo test -p baml-runtime --libpass (147+ tests)Authorization: Bearer <token>replacesapi-keyheader for Entra ID clients🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores