more google adc stuff#3964
Open
sxlijin wants to merge 12 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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:
📝 WalkthroughWalkthroughVertex AI credential handling now distinguishes inline JSON, credential file paths, and ADC. The forked Google Cloud auth library adds deterministic discovery, token caching, project resolution, multiple credential exchanges, and expanded tests. ChangesVertex authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant VertexClient
participant auth_vertex
participant forked_google_cloud_auth
participant GoogleCloudEndpoints
VertexClient->>auth_vertex: provide Vertex options
auth_vertex->>auth_vertex: resolve inline JSON, file path, or ADC
auth_vertex->>forked_google_cloud_auth: request token and project data
forked_google_cloud_auth->>GoogleCloudEndpoints: exchange credentials or query metadata
GoogleCloudEndpoints-->>forked_google_cloud_auth: return token and project data
forked_google_cloud_auth-->>auth_vertex: return authentication values
auth_vertex-->>VertexClient: send authenticated Vertex request
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![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: 86fa89723d
ℹ️ 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".
Comment on lines
+828
to
+832
| if let Some(home) = io.env("HOME").await.filter(|s| !s.is_empty()) { | ||
| return Some(format!("{home}/.config/gcloud")); | ||
| } | ||
| if let Some(appdata) = io.env("APPDATA").await.filter(|s| !s.is_empty()) { | ||
| return Some(format!("{appdata}/gcloud")); |
There was a problem hiding this comment.
For Windows processes where both HOME and APPDATA are set, this returns $HOME/.config/gcloud before ever checking %APPDATA%\gcloud, so token_from_adc and project_id miss the real well-known ADC file and fall through to metadata/no credentials. Use a platform-specific order, or check APPDATA before HOME on Windows, to make the documented Windows ADC path reachable.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/sys_llm/src/auth_request/vertex.rs (1)
74-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
VertexAiOptions.project_idbefore credential discovery.
project_idis exposed inVertexAiOptionsand named in the error message, but this path only resolves from credentials/environment. A configuredoptions.project_idtherefore cannot fill the Vertex URL placeholder.Proposed fix
- let project_id = project_id_from_credentials(&creds, &adapter) - .await - .ok_or_else(|| { + let project_id = match vertex_opts + .as_ref() + .and_then(|opts| opts.project_id.as_deref()) + .map(str::trim) + .filter(|id| !id.is_empty()) + { + Some(id) => id.to_owned(), + None => project_id_from_credentials(&creds, &adapter).await.ok_or_else(|| { BuildRequestError::Other( "Could not resolve project_id for Vertex AI. Set options.project_id, \ the GOOGLE_CLOUD_PROJECT env var, or provide credentials containing \ a project_id." .to_string(), ) - })?; + })?, + };Add a regression test with
VertexAiOptions.project_idset and a credential document containing no project ID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/sys_llm/src/auth_request/vertex.rs` around lines 74 - 89, Update the project ID resolution in the Vertex request-building path to prefer the configured VertexAiOptions.project_id before falling back to project_id_from_credentials and environment discovery, while preserving the existing error when neither source resolves a value. Add a regression test covering VertexAiOptions.project_id with credentials that omit a project ID, and verify the placeholder in the request URL is replaced with the configured value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_builtins2/baml_std/baml/ns_llm/llm_types.baml`:
- Around line 561-563: Update the credential-path documentation near the LLM
credential option to explicitly include external_account_authorized_user
alongside the currently listed supported Google credential formats, keeping the
statement consistent with the resolver’s supported credential types.
In `@baml_language/forks/google-cloud-auth/src/lib.rs`:
- Around line 261-268: Remove the global cache lookup and storage for the GCE
metadata path in the metadata-token flow surrounding mint_metadata, so each
auth_vertex/BamlTokenIo runtime contacts its own metadata server; alternatively,
derive the cache key from a runtime-specific or credential-source-specific
namespace rather than the fixed "\0gce-metadata" value.
---
Outside diff comments:
In `@baml_language/crates/sys_llm/src/auth_request/vertex.rs`:
- Around line 74-89: Update the project ID resolution in the Vertex
request-building path to prefer the configured VertexAiOptions.project_id before
falling back to project_id_from_credentials and environment discovery, while
preserving the existing error when neither source resolves a value. Add a
regression test covering VertexAiOptions.project_id with credentials that omit a
project ID, and verify the placeholder in the request URL is replaced with the
configured value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98cc1fe4-17f1-41ba-8603-586756e10f85
⛔ Files ignored due to path filters (2)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/forks/google-cloud-auth-systest/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
baml_language/crates/baml_builtins2/baml_std/baml/ns_llm/llm_types.bamlbaml_language/crates/sys_llm/src/auth_request/vertex.rsbaml_language/crates/sys_llm/src/build_request/google.rsbaml_language/forks/google-cloud-auth-systest/README.mdbaml_language/forks/google-cloud-auth/Cargo.tomlbaml_language/forks/google-cloud-auth/src/lib.rsbaml_language/forks/google-cloud-auth/tests/adc_precedence.rsbaml_language/forks/google-cloud-auth/tests/auth_flows.rsbaml_language/forks/google-cloud-auth/tests/project_id.rsbaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.pybaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlthoughts/sam-projects/google-adc-credentials/background.md
💤 Files with no reviewable changes (1)
- baml_language/crates/sys_llm/src/build_request/google.rs
Comment on lines
+561
to
+563
| /// Path to a Google credential JSON file (service account, authorized user, | ||
| /// workload identity federation, or impersonated service account). Inline | ||
| /// JSON is not supported; when unset, Application Default Credentials apply. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document workforce refresh credentials too.
external_account_authorized_user is supported by the resolver but omitted from this public option contract.
- /// workload identity federation, or impersonated service account). Inline
+ /// workload identity federation, workforce refresh, or impersonated service
+ /// account). Inline📝 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.
Suggested change
| /// Path to a Google credential JSON file (service account, authorized user, | |
| /// workload identity federation, or impersonated service account). Inline | |
| /// JSON is not supported; when unset, Application Default Credentials apply. | |
| /// Path to a Google credential JSON file (service account, authorized user, | |
| /// workload identity federation, workforce refresh, or impersonated service | |
| /// account). Inline JSON is not supported; when unset, Application Default Credentials apply. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/crates/baml_builtins2/baml_std/baml/ns_llm/llm_types.baml`
around lines 561 - 563, Update the credential-path documentation near the LLM
credential option to explicitly include external_account_authorized_user
alongside the currently listed supported Google credential formats, keeping the
statement consistent with the resolver’s supported credential types.
Comment on lines
261
to
+268
| // 3. GCE metadata server. | ||
| token_from_metadata(io, scope).await | ||
| let key = cache_key("\u{0}gce-metadata", scope); | ||
| if let Some(token) = cached_token(&key) { | ||
| return Ok(token); | ||
| } | ||
| let token = mint_metadata(io, scope).await?; | ||
| store_token(key, &token); | ||
| Ok(token.access_token) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not share metadata tokens across runtime instances.
The fixed "\0gce-metadata" cache key is scoped only by OAuth scope. Since auth_vertex creates a new BamlTokenIo around its supplied RuntimeIo per call, a later isolated runtime can receive the first runtime’s cached bearer token without contacting its metadata server. Namespace this cache per runtime/credential source, or do not globally cache metadata tokens.
Safe immediate fix
- let key = cache_key("\u{0}gce-metadata", scope);
- if let Some(token) = cached_token(&key) {
- return Ok(token);
- }
let token = mint_metadata(io, scope).await?;
- store_token(key, &token);
Ok(token.access_token)📝 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.
Suggested change
| // 3. GCE metadata server. | |
| token_from_metadata(io, scope).await | |
| let key = cache_key("\u{0}gce-metadata", scope); | |
| if let Some(token) = cached_token(&key) { | |
| return Ok(token); | |
| } | |
| let token = mint_metadata(io, scope).await?; | |
| store_token(key, &token); | |
| Ok(token.access_token) | |
| // 3. GCE metadata server. | |
| let token = mint_metadata(io, scope).await?; | |
| Ok(token.access_token) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/forks/google-cloud-auth/src/lib.rs` around lines 261 - 268,
Remove the global cache lookup and storage for the GCE metadata path in the
metadata-token flow surrounding mint_metadata, so each auth_vertex/BamlTokenIo
runtime contacts its own metadata server; alternatively, derive the cache key
from a runtime-specific or credential-source-specific namespace rather than the
fixed "\0gce-metadata" value.
Binary size checks failed❌ 1 violations · ✅ 6 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 14750419
gzip_bytes = 4188447Generated by |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/sys_llm/src/auth_request/vertex.rs`:
- Around line 235-247: Update project_id_from_credentials to propagate errors
when a ResolvedCredentials::CredentialsFile path cannot be read, instead of
silently falling back to google_cloud_auth::project_id(adapter). Reuse
read_credentials_file or change the function’s return type as needed so the read
failure reaches the caller, while preserving JSON project-id extraction for
successfully read credential files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eb02c41b-989c-4ecb-b3f7-f28853e4ab1b
⛔ Files ignored due to path filters (2)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/forks/google-cloud-auth-systest/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
baml_language/crates/baml_builtins2/baml_std/baml/ns_llm/llm_types.bamlbaml_language/crates/sys_llm/src/auth_request/vertex.rsbaml_language/crates/sys_llm/src/build_request/google.rsbaml_language/forks/google-cloud-auth-systest/README.mdbaml_language/forks/google-cloud-auth/Cargo.tomlbaml_language/forks/google-cloud-auth/src/lib.rsbaml_language/forks/google-cloud-auth/tests/adc_precedence.rsbaml_language/forks/google-cloud-auth/tests/auth_flows.rsbaml_language/forks/google-cloud-auth/tests/project_id.rs
💤 Files with no reviewable changes (1)
- baml_language/crates/sys_llm/src/build_request/google.rs
✅ Files skipped from review due to trivial changes (1)
- baml_language/forks/google-cloud-auth-systest/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_builtins2/baml_std/baml/ns_llm/llm_types.baml
- baml_language/forks/google-cloud-auth/Cargo.toml
- baml_language/forks/google-cloud-auth/tests/project_id.rs
- baml_language/forks/google-cloud-auth/tests/adc_precedence.rs
Contributor
|
Found 18 test failures on Blacksmith runners: Failures
|
…allbacks Remove the legacy credential paths google-auth does not have: inline JSON in GOOGLE_APPLICATION_CREDENTIALS, the BAML-specific GOOGLE_APPLICATION_CREDENTIALS_CONTENT env var, the Vercel quoted-JSON unwrap, and the `gcloud auth print-access-token` / `gcloud config get-value project` shell-outs. The `credentials` option is now strictly a file path, exactly like GOOGLE_APPLICATION_CREDENTIALS. Inline credentials keep exactly one, explicit home: `credentials_content`, retyped from string to `json | null` so it takes a real JSON object (a pre-serialized JSON string still works). Resolution order is now: options.credentials_content (inline JSON) -> options.credentials (file path) -> Application Default Credentials.
Three parity fixes with google-auth (Python/Node): - A set GOOGLE_APPLICATION_CREDENTIALS whose file cannot be read is now a hard error instead of silently falling through to the well-known file or metadata server. - The well-known ADC config dir falls back to %APPDATA%\gcloud on Windows (after CLOUDSDK_CONFIG and HOME). - The no-credentials error now says what was checked and points at `gcloud auth application-default login`, like DefaultCredentialsError.
Every call previously re-minted a token (fresh JWT sign + token-endpoint
round trip per request). Tokens are now cached process-wide, keyed by
SHA-256(credential material, scope) so the map never holds raw credentials
and distinct identities cannot collide, and reused until google-auth's
3m45s REFRESH_THRESHOLD before expiry. `expires_in` is parsed from token
responses; a response without one never expires (google-auth parity).
Internals refactor to support this: flows return a `Token {access_token,
expires_at}` and the public entry points are thin cache wrappers. In-file
MockIo tests that duplicated tests/auth_flows.rs are dropped in favor of the
integration suite; integration tests now use unique credential material per
test so the shared cache cannot leak tokens across tests.
…d credentials Extend the credential-type dispatch beyond service_account/authorized_user to the remaining google-auth types BAML can support pure-Rust and wasm-safe: - external_account (workload identity federation): file- and url-sourced subject tokens (text or JSON-field format), STS token exchange, optional service-account impersonation, Basic client auth, and the workforce-pool userProject option. - impersonated_service_account: nested source credentials (service account or authorized user) exchanged at the IAM Credentials generateAccessToken endpoint, with delegates; expireTime (RFC 3339) feeds the token cache. - external_account_authorized_user: workforce refresh grant against STS. AWS-sourced federation, executable credential sources, and GDCH are rejected with a clear AuthError::Unsupported. The dispatch is now public as token_from_credentials_json, and the vertex `credentials` file option routes through it, so a WIF or impersonation config file works anywhere GOOGLE_APPLICATION_CREDENTIALS would.
Move project resolution into the fork and complete google-auth parity: project_id() follows GOOGLE_CLOUD_PROJECT / legacy GCLOUD_PROJECT, the GOOGLE_APPLICATION_CREDENTIALS file, the active gcloud configuration's core.project (read from the config file on disk — the value google-auth reports, without shelling out), the well-known ADC file's quota/project id, then the GCE metadata server. quota_project_id() resolves GOOGLE_CLOUD_QUOTA_PROJECT and credential-file quota_project_id. The vertex consumer now delegates its project-id fallback chain to the fork (dropping its local ADC-file/metadata reimplementation) and stamps x-goog-user-project on requests when a quota project is configured, matching google-auth Credentials.apply. Fork version bumped to 0.2.0.
Document the offline coverage added across the previous changes: WIF (file/url, impersonation, workforce user-project), impersonated service accounts, workforce refresh grants, APPDATA discovery, the GAC hard error, token caching, and the project-id/quota-project chain. Offline tier is now 40 tests.
An env var set to the empty string was silently treated as unset (`.filter(|s| !s.is_empty())` / `if !val.is_empty()`), hiding the misconfiguration behind lower-priority sources. Set-but-empty values are now honored verbatim so they fail visibly: GOOGLE_APPLICATION_CREDENTIALS="" is a hard no-credentials error, GCLOUD_PROJECT="" yields an empty project id (and a visibly broken request) instead of falling through, and CLOUDSDK_CONFIG="" / HOME="" / APPDATA="" / GOOGLE_CLOUD_QUOTA_PROJECT="" / CLOUDSDK_ACTIVE_CONFIG_NAME="" all take effect as written. Same for the GOOGLE_CLOUD_QUOTA_PROJECT read in the vertex consumer and the systest harness's NativeIo. (Deliberate divergence from google-auth, which treats some empty vars as unset; documented in the crate docs.)
…tex-ai pointer google-ai silently sent an unauthenticated request when no api_key was configured (the server 401 was the only signal). Request construction now fails fast in the vertex error style, offering both ways out: set options.api_key (env.GOOGLE_API_KEY / env.GEMINI_API_KEY), or switch to the vertex-ai provider with project_id + location to use Google Cloud credentials — mirroring google-genai's "Missing key inputs argument!" error, which likewise points at the Vertex backend.
…ION / GOOGLE_CLOUD_PROJECT Match google-genai: a vertex-ai client no longer needs location in BAML options. When unset, the URL is built with a location placeholder (like the existing project_id one) that auth resolves from GOOGLE_CLOUD_LOCATION, handling the region-less "global" endpoint host. project_id already resolved through GOOGLE_CLOUD_PROJECT via the credential chain. The compile-time "requires either base_url or location" diagnostic is removed — the compiler cannot see the env var — and a client with neither errors at $build_request with an actionable message naming both fixes.
…rtex backend google-genai parity for the backend switch: when GOOGLE_GENAI_USE_VERTEXAI is "true"/"1", a google-ai client builds a Vertex request instead — Vertex URL (location/project from GOOGLE_CLOUD_LOCATION / GOOGLE_CLOUD_PROJECT or the credential chain), Google Cloud credentials via the ADC chain, and bearer auth — exactly like `vertexai=True` in the Python SDK. The google-ai default base_url is recognized and ignored on the Vertex path so the flip does not send bearer tokens to generativelanguage. The missing-api_key error now also mentions this env-only route. With this, an ADK-style env setup (GOOGLE_GENAI_USE_VERTEXAI=true + GOOGLE_CLOUD_PROJECT + GOOGLE_CLOUD_LOCATION + gcloud ADC) works with provider google-ai and no BAML option changes.
Four hermetic scenarios through real BAML programs calling $build_request (one test fn: the scenarios mutate real Google env vars, and this file is its own test binary, so nothing races them; all scenarios are network-free): - google-ai with `api_key env.GOOGLE_API_KEY` unset: the strict env read fails naming the var; with no api_key at all, the fail-fast error points at vertex-ai and GOOGLE_GENAI_USE_VERTEXAI. - google-ai with GOOGLE_GENAI_USE_VERTEXAI=true: builds a Vertex URL with location/project from GOOGLE_CLOUD_LOCATION / GOOGLE_CLOUD_PROJECT (express-mode query key keeps it token-free). This caught a real bug, fixed in the location-placeholder change: url::Url lowercases the host when query params are appended, so the placeholder must be lowercase. - vertex-ai, GOOGLE_CLOUD_PROJECT set, GOOGLE_CLOUD_LOCATION unset: actionable location error naming options.location and the env var. - vertex-ai, both unset: same location-first error, before any credential IO.
… resolution project_id_from_credentials swallowed an unreadable options.credentials path and fell through to the env/ADC project chain. On paths that only need the project-id placeholder — express-mode API-key auth mints no token — the broken file was completely masked and the request went out with a project from the environment. The file read now reuses read_credentials_file, so a bad path surfaces the same "failed to open credentials file" error everywhere it is consulted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
credentialsfile path →credentials_contentinline JSON → ADC), plus consistent use of the same resolved credentials for tokens and project handling.credentials(now treated strictly as a file path).x-goog-user-projectis added only when missing.