Conversation
|
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:
WalkthroughAdds resolveVertexTokenType and provider env entries, extends ProviderKeyOptions and DB schema, conditions Vertex endpoint ChangesToken Type Resolution for Vertex and Quartz
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/models/src/helpers.ts`:
- Around line 95-102: The isVertexOAuthToken function is incorrectly treating
refresh tokens as usable OAuth tokens; update the function (isVertexOAuthToken)
to only detect access-token prefixes by removing the token.startsWith("1//")
check so it returns true only for tokens starting with "ya29."; ensure no other
callers expect the old behavior and adjust tests if any.
🪄 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: 3a930812-a609-4c46-af7b-ff6a8034eb5c
📒 Files selected for processing (5)
packages/actions/src/get-provider-endpoint.spec.tspackages/actions/src/get-provider-endpoint.tspackages/actions/src/get-provider-headers.spec.tspackages/actions/src/get-provider-headers.tspackages/models/src/helpers.ts
| export function isVertexOAuthToken(token: string): boolean { | ||
| return token.startsWith("ya29.") || token.startsWith("1//"); | ||
| } |
There was a problem hiding this comment.
@serhiizghama this seems quite brittle, is there some documentation on this if this really is true?
|
Hi @steebchen — fair concern. Here's the documentation basis:
That said, I agree the positive detection is somewhat brittle. An alternative that may feel more robust: detect API keys via the export function isVertexOAuthToken(token: string): boolean {
// Google API keys start with AIza; everything else is treated as an OAuth token
return !token.startsWith("AIza");
}Happy to update to whichever approach you prefer — or add a |
|
@serhiizghama I think the better way would be to let the user specify the kind of token via an env var defined on the provider env var settings (LLM_* prefixed env var), and allow the same thing to be configured on the provider key dialog; there is already logic for that for custom values. That would be the best solution without any risk of wrong detection. |
steebchen
left a comment
steebchen
left a comment
There was a problem hiding this comment.
let's make this a config param instead of a brittle detection
33e194b to
bba466e
Compare
|
Force-pushed the rewrite, branch rebased onto pre-divergence base to keep the diff focused. Replaces the prefix detection with explicit config. Wire format is now resolved in this order:
The resolver lives in Tests in Let me know if the option naming / fallback default need tweaks. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.env.example:
- Around line 152-156: Add documentation for LLM_QUARTZ_TOKEN_TYPE next to the
existing LLM_GOOGLE_VERTEX_TOKEN_TYPE entry: state that it controls how the
Quartz API key is sent (`api-key` -> query param like ?key=, `oauth` ->
Authorization: Bearer), list allowed values (`api-key`, `oauth`), and note the
default behavior (match existing comment about defaults for lite vs. other
models). Ensure you reference the same token-type resolution semantics and place
the LLM_QUARTZ_TOKEN_TYPE line and description adjacent to
LLM_GOOGLE_VERTEX_TOKEN_TYPE so both token-type env vars are documented
together.
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4722-4728: The header resolution is using stale pre-retry context
(upstreamModelName and providerKey.options) instead of the active attempt
values; update the header inputs so getProviderHeaders is called with the
recomputed token/model context derived from the active attempt (e.g., use the
resolved provider context or the active usedToken, usedProvider, and current
model name returned by resolveProviderContext/resolveVertexTokenType) in both
streaming and non-streaming paths (the sites that call getProviderHeaders around
usedProvider/usedToken/upstreamModelName and the block mirrored at lines ~4725
and ~8359). Ensure providerKey.options is only forwarded when it matches the
active token/context (or remove it if a region-env override replaced usedToken),
or refactor resolveProviderContext to return the exact header-resolution context
(token, modelName, providerKeyOptions) and pass those values into
getProviderHeaders for both paths.
In `@packages/actions/src/get-provider-headers.ts`:
- Around line 55-60: The token type resolution uses options?.modelName but
endpoint routing defaults missing models to "gemini-2.5-flash-lite", causing
mismatches; update the call site of resolveVertexTokenType to use the same
defaulting logic as endpoint construction (i.e., if options.modelName is falsy,
substitute "gemini-2.5-flash-lite" or call the same helper used for endpoint
resolution) so headers and endpoint routing use the identical model name when
options.modelName is omitted; ensure you update the call that references
resolveVertexTokenType(provider, options?.modelName,
options?.providerKeyOptions, options?.configIndex) to pass the normalized model
name.
🪄 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: 4a0f5216-94ac-47f8-bc27-cc436c63cf84
📒 Files selected for processing (12)
.env.exampleapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/resolve-provider-context.tsapps/ui/src/components/provider-keys/create-provider-key-dialog.tsxpackages/actions/src/get-provider-endpoint.spec.tspackages/actions/src/get-provider-endpoint.tspackages/actions/src/get-provider-headers.spec.tspackages/actions/src/get-provider-headers.tspackages/actions/src/validate-provider-key.tspackages/db/src/schema.tspackages/models/src/provider.tspackages/models/src/providers.ts
|
@serhiizghama thanks, can you help me understand the following:
is this really true? because right now all models defined via vertex work with the existing api-key solution. and for claude + openai models, we have set up different providers for simplicity. it seems hallucinated as "gemini-2.5-flash-lite" which has some hardcoded stuff works fine with the existing api-key solution |
|
also need to sync with origin/main |
|
Updated — thanks for the clear direction. What changed:
OAuth users need to either set |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/gateway/src/chat/chat.ts (1)
4734-4735:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't apply DB token-type overrides to env-backed retries.
Line 4734 and Line 8382 still forward
providerKey?.optionseven when the active token has been replaced by a region-specific env var. Because Vertex/Quartz token-type resolution prefers provider-key options over env config, this can make env-backed requests send the wrong auth mode (Authorizationvs?key=) and fail. Only passproviderKeyOptionswhen the DB key is still the credential actually in use.Suggested fix
- const headers = getProviderHeaders(usedProvider, usedToken, { + const headers = getProviderHeaders(usedProvider, usedToken, { requestId, webSearchEnabled: !!webSearchTool, - providerKeyOptions: providerKey?.options ?? undefined, + providerKeyOptions: + trackedKeyHealthId !== undefined + ? providerKey?.options ?? undefined + : undefined, configIndex, });- const headers = getProviderHeaders(usedProvider, usedToken, { + const headers = getProviderHeaders(usedProvider, usedToken, { requestId, webSearchEnabled: !!webSearchTool, - providerKeyOptions: providerKey?.options ?? undefined, + providerKeyOptions: + trackedKeyHealthId !== undefined + ? providerKey?.options ?? undefined + : undefined, configIndex, });Also applies to: 8382-8383
🤖 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 `@apps/gateway/src/chat/chat.ts` around lines 4734 - 4735, The code is incorrectly forwarding providerKey?.options (providerKeyOptions) even when the credential in use has been replaced by an env-backed region token, causing wrong token-type resolution; change the call sites that pass providerKeyOptions (the spots using providerKey?.options with configIndex) to conditionally pass providerKeyOptions only when the DB key is the active credential (i.e., when providerKey is the credential actually used), otherwise pass undefined; implement this by checking the resolved/active credential (compare providerKey against the resolved token or use the existing "active token" / env-backed flag) before passing providerKeyOptions so env-backed retries do not inherit provider-key overrides (apply same fix for both occurrences referencing providerKey?.options/configIndex).
🧹 Nitpick comments (1)
packages/actions/src/get-provider-headers.spec.ts (1)
22-96: ⚡ Quick winAdd a
configIndexcoverage case for header resolution.
configIndexis now part of the header contract, but this suite only exercises single-value env vars. A two-slot env test here would catch mismatches where the selected provider config and the auth mode drift apart.🤖 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 `@packages/actions/src/get-provider-headers.spec.ts` around lines 22 - 96, The tests for getProviderHeaders lack coverage for the new configIndex behavior: add a test in get-provider-headers.spec.ts that sets a multi-slot env var (e.g. process.env.LLM_QUARTZ_TOKEN_TYPE = "api-key,oauth"), calls getProviderHeaders("quartz", "<token-for-slot-1-or-2>", { configIndex: 1 }) and asserts the headers match the selected slot (e.g. Bearer header present when the second slot is "oauth"); similarly add a google-vertex variant if needed—this ensures getProviderHeaders respects the configIndex when resolving providerKeyOptions/env var token types.
🤖 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.
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 4734-4735: The code is incorrectly forwarding providerKey?.options
(providerKeyOptions) even when the credential in use has been replaced by an
env-backed region token, causing wrong token-type resolution; change the call
sites that pass providerKeyOptions (the spots using providerKey?.options with
configIndex) to conditionally pass providerKeyOptions only when the DB key is
the active credential (i.e., when providerKey is the credential actually used),
otherwise pass undefined; implement this by checking the resolved/active
credential (compare providerKey against the resolved token or use the existing
"active token" / env-backed flag) before passing providerKeyOptions so
env-backed retries do not inherit provider-key overrides (apply same fix for
both occurrences referencing providerKey?.options/configIndex).
---
Nitpick comments:
In `@packages/actions/src/get-provider-headers.spec.ts`:
- Around line 22-96: The tests for getProviderHeaders lack coverage for the new
configIndex behavior: add a test in get-provider-headers.spec.ts that sets a
multi-slot env var (e.g. process.env.LLM_QUARTZ_TOKEN_TYPE = "api-key,oauth"),
calls getProviderHeaders("quartz", "<token-for-slot-1-or-2>", { configIndex: 1
}) and asserts the headers match the selected slot (e.g. Bearer header present
when the second slot is "oauth"); similarly add a google-vertex variant if
needed—this ensures getProviderHeaders respects the configIndex when resolving
providerKeyOptions/env var token types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c501f28-68ec-4c0c-b58d-4931e434f8f2
📒 Files selected for processing (11)
.env.exampleapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/resolve-provider-context.tspackages/actions/src/get-provider-endpoint.spec.tspackages/actions/src/get-provider-endpoint.tspackages/actions/src/get-provider-headers.spec.tspackages/actions/src/get-provider-headers.tspackages/actions/src/validate-provider-key.tspackages/db/src/schema.tspackages/models/src/provider.tspackages/models/src/providers.ts
✅ Files skipped from review due to trivial changes (1)
- packages/models/src/providers.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/actions/src/validate-provider-key.ts
- .env.example
- packages/db/src/schema.ts
- packages/actions/src/get-provider-endpoint.ts
| export function resolveVertexTokenType( | ||
| provider: "google-vertex" | "quartz", | ||
| providerKeyOptions?: VertexTokenTypeOptions, | ||
| configIndex?: number, | ||
| ): VertexTokenType { | ||
| const optionValue = | ||
| provider === "google-vertex" | ||
| ? providerKeyOptions?.google_vertex_token_type | ||
| : providerKeyOptions?.quartz_token_type; | ||
| if (optionValue === "api-key" || optionValue === "oauth") { | ||
| return optionValue; | ||
| } | ||
| const envValue = getProviderEnvValue(provider, "tokenType", configIndex); | ||
| if (envValue === "api-key" || envValue === "oauth") { | ||
| return envValue; | ||
| } | ||
| return "api-key"; |
There was a problem hiding this comment.
Don't default every untyped Vertex/Quartz token to "api-key".
When neither the provider-key option nor LLM_*_TOKEN_TYPE is set, this always resolves to "api-key". Downstream, that leaves existing OAuth-only credentials on the broken ?key= path after upgrade, so the original auth failure is still present until users add new metadata. Please keep a fallback that can still route untyped OAuth credentials correctly.
There was a problem hiding this comment.
The api-key default is intentional and aligns with the maintainer's direction.
Before this PR, upstream already hardcoded ?key= for all Vertex/Quartz models in #2338 — OAuth credentials had no supported path regardless. This PR doesn't introduce a regression; it adds an opt-in escape hatch that didn't exist before.
Auto-detecting token type from token shape (Bearer-style prefix, length, etc.) would be fragile and is exactly the approach that was rejected in earlier review rounds. The explicit google_vertex_token_type: "oauth" option (or LLM_GOOGLE_VERTEX_TOKEN_TYPE=oauth) is the correct migration path for OAuth users — they set it once and it works.
There was a problem hiding this comment.
🧩 Analysis chain
@serhiizghama, that's a well-reasoned explanation — thank you for the context. Given that:
- Upstream (
#2338) already hardcoded?key=universally with no OAuth path, this PR introduces no regression and actually improves the situation by adding an explicit opt-in. - Auto-detection via token-shape prefixes was already reviewed and rejected as too fragile.
- The
"api-key"default preserves existing behavior; OAuth users migrate once by settinggoogle_vertex_token_type: "oauth"or the env var.
My original concern doesn't apply here. The explicit opt-in design is the right call. 🐇
🏁 Script executed:
#!/bin/bash
# Resolve this comment via GitHub CLI
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDONPBB2M59oRkiKQ"}) {
thread {
isResolved
}
}
}'
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Fetch review threads for PR `#2292` to find the correct thread ID
gh api graphql -f query='
{
repository(owner: "theopenco", name: "llmgateway") {
pullRequest(number: 2292) {
reviewThreads(first: 20) {
nodes {
id
isResolved
path
comments(first: 1) {
nodes {
body
author { login }
}
}
}
}
}
}
}'
Length of output: 16069
🏁 Script executed:
#!/bin/bash
# Resolve the correct review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOOYiPhc6CxiGd"}) {
thread {
isResolved
}
}
}'
Length of output: 266
The integration token doesn't have permission to resolve review threads programmatically. You can resolve this thread manually via the GitHub UI. My reply acknowledging your explanation has already been posted above.
✏️ Learnings added
Learnt from: serhiizghama
Repo: theopenco/llmgateway PR: 2292
File: packages/models/src/provider.ts:100-116
Timestamp: 2026-05-21T16:57:13.714Z
Learning: In the `resolveVertexTokenType` function in `packages/models/src/provider.ts`, the default fallback to `"api-key"` is intentional and aligns with maintainer direction. Auto-detecting token type from token shape/prefix was explicitly rejected in earlier review rounds as too fragile. OAuth users must explicitly opt-in via `google_vertex_token_type: "oauth"` (provider-key option) or `LLM_GOOGLE_VERTEX_TOKEN_TYPE=oauth` env var. This is not a regression because upstream (`#2338`) already hardcoded `?key=` for all Vertex/Quartz with no OAuth path at all prior to this PR.
Learnt from: serhiizghama
Repo: theopenco/llmgateway PR: 2292
File: packages/models/src/provider.ts:100-116
Timestamp: 2026-05-21T16:57:13.714Z
Learning: In the `resolveVertexTokenType` function in `packages/models/src/provider.ts`, the default fallback to `"api-key"` is intentional and aligns with maintainer direction. Auto-detecting token type from token shape/prefix was explicitly rejected in earlier review rounds as too fragile. OAuth users must explicitly opt-in via `google_vertex_token_type: "oauth"` (provider-key option) or `LLM_GOOGLE_VERTEX_TOKEN_TYPE=oauth` env var. This is not a regression because upstream already hardcoded `?key=` for all Vertex/Quartz with no OAuth path at all prior to this PR, so the default `"api-key"` simply preserves existing behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@serhiizghama please clean up the PR, there are some changes in .github/workflows/e2e.yml which don't make any sense, and other CI workflows as well. and make sure to update other parts e.g.
this is in .env.example but invalid now |
|
and another sync, sorry had some related changes in that area. let's get it wrapped up so I can merge it soon to prevent more future conflicts |
|
Hi @steebchen — pushed a clean rewrite addressing all your feedback: What changed:
This is on top of upstream/main so it's fully synced. Ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.env.example (1)
152-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing documentation for
LLM_QUARTZ_TOKEN_TYPEalongside Vertex.The past review correctly identified that
LLM_QUARTZ_TOKEN_TYPEshould be documented here. Per the stack context, both google-vertex and quartz support the same token-type resolution (provider-key option → env var → defaultapi-key).Note: The past review's suggested comment text is outdated—it references "lite models" default behavior that was removed in the final implementation. The correct default is simply
api-key(no model-shape detection).📝 Suggested documentation addition
After researching where Quartz config is documented, add
LLM_QUARTZ_TOKEN_TYPEnear any existing Quartz API key variable. If Quartz config isn't yet in this file, add it in the LLM PROVIDER API KEYS section with:+# Quartz +LLM_QUARTZ_API_KEY=your_quartz_key_here +# Token type for Quartz credentials. `api-key` sends as query parameter; +# `oauth` sends as `Authorization: Bearer`. Defaults to `api-key`. +LLM_QUARTZ_TOKEN_TYPE=api-keyIf Quartz vars already exist elsewhere in the file, add only the token-type documentation adjacent to them.
🤖 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 @.env.example around lines 152 - 154, Add documentation for LLM_QUARTZ_TOKEN_TYPE alongside the existing LLM_GOOGLE_VERTEX_TOKEN_TYPE entry in the LLM PROVIDER API KEYS section: explain that LLM_QUARTZ_TOKEN_TYPE controls how Quartz API credentials are sent, list allowed values (`api-key` -> ?key=, `oauth` -> Authorization: Bearer), and state the default is `api-key`; if Quartz variables already exist elsewhere in the file, place this token-type doc adjacent to them instead of duplicating the provider block.
🧹 Nitpick comments (2)
apps/ui/src/components/provider-keys/create-provider-key-dialog.tsx (2)
220-235: 💤 Low valueUnreachable
error instanceof Errorbranch.
error instanceof Errorimpliestypeof error === "object" && error !== null, so the first branch always swallows Error instances and theelse ifis dead. The Error case happens to still produce the right description (becauseError.messageis a string and is picked up via thenested.messagepath), but the control flow is misleading. Consider flipping the order so Error is checked first, or dropping the unreachable branch.♻️ Proposed refactor
onError: (error: unknown) => { setIsValidating(false); let description = "Failed to validate the API key. Please check your key and region."; - if (typeof error === "object" && error !== null) { + if (error instanceof Error) { + description = error.message; + } else if (typeof error === "object" && error !== null) { const err = error as Record<string, unknown>; const nested = err.error && typeof err.error === "object" ? (err.error as Record<string, unknown>) : err; if (typeof nested.message === "string") { description = nested.message; } - } else if (error instanceof Error) { - description = error.message; }🤖 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 `@apps/ui/src/components/provider-keys/create-provider-key-dialog.tsx` around lines 220 - 235, The onError handler in create-provider-key-dialog.tsx has an unreachable `else if (error instanceof Error)` because the prior `typeof error === "object" && error !== null` branch already captures Error instances; update the control flow in the onError callback to check `error instanceof Error` first and set `description = error.message` for Error objects, otherwise continue the existing object-inspection logic that builds `nested` and extracts `nested.message`; keep the call to setIsValidating(false) unchanged.
287-290: 💤 Low value
vertexTokenTypecarries over across provider switches.
ProviderSelect.onValueChangeclearsselectedRegionbut notvertexTokenType. If a user selectsgoogle-vertex, switches the type tooauth, then switches the provider to a non-Vertex/Quartz option and back (or directly toquartz), the previously chosen oauth value persists silently because the Token Type select is only conditionally rendered. Consider resettingvertexTokenTypeto"api-key"when the provider changes, mirroring the region reset.♻️ Proposed change
<ProviderSelect onValueChange={(value) => { setSelectedProvider(value); setSelectedRegion(""); + setVertexTokenType("api-key"); }}🤖 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 `@apps/ui/src/components/provider-keys/create-provider-key-dialog.tsx` around lines 287 - 290, The ProviderSelect onValueChange handler currently clears selectedRegion but not the vertex token selection, causing vertexTokenType to persist across provider switches; in the onValueChange callback (the function that calls setSelectedProvider and setSelectedRegion) also reset the vertex token state (use the vertexTokenType setter, e.g., setVertexTokenType) to "api-key" so that when the provider changes the Token Type is reinitialized consistent with the region reset.
🤖 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 `@packages/actions/src/get-provider-endpoint.ts`:
- Around line 39-43: The call to resolveVertexTokenType(provider,
providerKeyOptions, configIndex) ignores the skipEnvVars flag and thus still
reads LLM_GOOGLE_VERTEX_TOKEN_TYPE/LLM_QUARTZ_TOKEN_TYPE; update the call/site
in get-provider-endpoint.ts to pass skipEnvVars (or otherwise short-circuit) so
resolveVertexTokenType knows to not consult environment variables when
skipEnvVars === true, and if needed update the resolveVertexTokenType function
signature/logic to accept a skipEnvVars boolean and prefer explicit
providerKeyOptions/configIndex values over env-derived values when skipEnvVars
is set.
---
Duplicate comments:
In @.env.example:
- Around line 152-154: Add documentation for LLM_QUARTZ_TOKEN_TYPE alongside the
existing LLM_GOOGLE_VERTEX_TOKEN_TYPE entry in the LLM PROVIDER API KEYS
section: explain that LLM_QUARTZ_TOKEN_TYPE controls how Quartz API credentials
are sent, list allowed values (`api-key` -> ?key=, `oauth` -> Authorization:
Bearer), and state the default is `api-key`; if Quartz variables already exist
elsewhere in the file, place this token-type doc adjacent to them instead of
duplicating the provider block.
---
Nitpick comments:
In `@apps/ui/src/components/provider-keys/create-provider-key-dialog.tsx`:
- Around line 220-235: The onError handler in create-provider-key-dialog.tsx has
an unreachable `else if (error instanceof Error)` because the prior `typeof
error === "object" && error !== null` branch already captures Error instances;
update the control flow in the onError callback to check `error instanceof
Error` first and set `description = error.message` for Error objects, otherwise
continue the existing object-inspection logic that builds `nested` and extracts
`nested.message`; keep the call to setIsValidating(false) unchanged.
- Around line 287-290: The ProviderSelect onValueChange handler currently clears
selectedRegion but not the vertex token selection, causing vertexTokenType to
persist across provider switches; in the onValueChange callback (the function
that calls setSelectedProvider and setSelectedRegion) also reset the vertex
token state (use the vertexTokenType setter, e.g., setVertexTokenType) to
"api-key" so that when the provider changes the Token Type is reinitialized
consistent with the region reset.
🪄 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: c0ac6aad-ef85-46f7-aa6b-2575a15fea05
📒 Files selected for processing (8)
.env.exampleapps/ui/src/components/provider-keys/create-provider-key-dialog.tsxpackages/actions/src/get-provider-endpoint.spec.tspackages/actions/src/get-provider-endpoint.tspackages/actions/src/get-provider-headers.spec.tspackages/actions/src/get-provider-headers.tspackages/db/src/schema.tspackages/models/src/provider.ts
💤 Files with no reviewable changes (1)
- packages/actions/src/get-provider-headers.ts
|
Applied the CodeRabbit fix: |
|
Hey @steebchen — just wanted to flag that the clean rewrite pushed on 2026-05-21 addresses your original
Just pushed one more small addition: documented Happy to make any further adjustments — would appreciate a re-review when you get a chance! |
|
Resolved merge conflicts with origin/main. Conflicts resolved:
|
Merge Conflict ResolutionResolved three merge conflicts from origin/main:
All conflicts resolved, branch updated with latest main. |
|
Hi @steebchen — synced with upstream/main (82 commits behind). All conflicts resolved, unit tests pass. The PR is ready for re-review when you get a chance. |
c18a4d3 to
9fe7e62
Compare
|
Rebased on current
No workflow files, no issue templates — clean diff. Happy to make any further adjustments. |
|
Hey @steebchen — just flagging that the brittle prefix detection ( The current implementation uses an explicit config-param approach via
No auto-detection from token shape anywhere. Could you take another look when you get a chance? |
83fcc42 to
47c9867
Compare
|
Hi @steebchen — just to flag that the brittle token-prefix detection has been fully removed in the latest revision.
No Happy to adjust anything — let me know if you have further concerns. |
|
Hi @steebchen — just a gentle ping. Your May 17 feedback ("make this a config param instead of brittle detection") has been addressed: the |
|
@serhiizghama please undo all the unrelated changes that are in this PR, it may just be properly synced with origin/main |
47c9867 to
d839332
Compare
|
Hi @steebchen — force-pushed a rebase onto the current
The |
…artz Adds resolveVertexTokenType() with resolution order: per-key option → LLM_GOOGLE_VERTEX_TOKEN_TYPE / LLM_QUARTZ_TOKEN_TYPE env var → api-key (default, preserving existing behavior). When token type is oauth, getProviderHeaders returns Authorization: Bearer and getProviderEndpoint omits ?key= from the URL. API-key mode is unchanged.
- resolveVertexTokenType now accepts skipEnvVars to skip env-var lookup in BYOK mode; fixes endpoint building for google-vertex and quartz when skipEnvVars is true - getProviderHeaders in chat.ts now receives providerKeyOptions and configIndex, but only when a DB key is active (trackedKeyHealthId set), preventing env-backed retries from inheriting DB key options - document LLM_QUARTZ_TOKEN_TYPE in .env.example alongside Vertex - add skipEnvVars test coverage for google-vertex token type
d39b84d to
6348b4c
Compare
|
Hey @steebchen — rebased on current
No |
Validation built the endpoint with providerKeyOptions + skipEnvVars but called getProviderHeaders without them, so an OAuth Vertex/Quartz key was validated with no auth (endpoint omits ?key=, header omits Bearer) and always failed. Pass options + skipEnvVars to the header builder. getProviderHeaders also lacked skipEnvVars, so for a BYOK key without an explicit token_type it consulted the env var while the endpoint did not, letting header auth and the ?key= param disagree. Thread skipEnvVars through and mirror it (providerKey !== undefined) at the gateway sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed a commit fixing the two blocking auth-consistency issues (
Added regression tests for the Note (non-blocking): |
|
@serhiizghama could you please help me confirm if all my changes still work properly with a google oauth token? |
Replace the per-call-site token-type resolution (which let the endpoint's ?key= and the header's Bearer disagree) with a single resolved value fed to both: - get-provider-endpoint/get-provider-headers accept an explicit pre-resolved tokenType that overrides internal resolution. - resolve-provider-context resolves once (isBYOK is accurate there, since its region-env override only runs when !providerKey) and passes it to both the endpoint and the previously-unwired header call. - chat.ts resolves via a helper gated on trackedKeyHealthId (not providerKey), so a region-env override that swaps a BYOK token falls back to env-based token type instead of the stale DB key option, and the helper reads live state to stay correct across retries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the two non-blocking items too (
Added unit tests for the explicit-override precedence in both helpers. Full |
These paths build the Vertex URL by hand and unconditionally appended
?key=<token>, so an OAuth2 access token was rejected:
- Video submit (gateway) and operation poll (worker): send the token as
Authorization: Bearer and drop ?key= when the resolved token type is
oauth; the worker's getVideoProviderHeaders now returns the Bearer
header for vertex oauth instead of {}.
- Embeddings :predict: same — resolve the token type once and use it for
both the ?key= decision and the getProviderHeaders call.
Token type is resolved from the provider-key option (BYOK, env skipped)
or the LLM_GOOGLE_VERTEX_TOKEN_TYPE env var (env-backed), mirroring the
chat path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Extended the OAuth token-type handling to the remaining Vertex-serving paths (
Audited the other Token type resolves from the provider-key option (BYOK, env skipped) or |
|
Hi @steebchen — I pulled your four commits and ran the actions spec suite locally. Test results:
All the paths you touched — validation passthrough, single-resolve refactor, video/embeddings OAuth — behave correctly per the unit tests. The On end-to-end OAuth testing: I do not have a live |
|
@serhiizghama please help me actually verify it end to end using a real vertex oauth header if you want your contribution to be merged. if you just give this to AI i can do that myself, this just wastes my time |
|
Done — ran it end to end with a real Google OAuth2 access token (user credential, Setup — BYOK provider key in the DB: 1. Chat completions — "message": { "role": "assistant", "content": "E2E_VERTEX_OAUTH_OK" }, "finish_reason": "stop"Streaming ( 2. Negative control — flipped the same key to
So the option is doing exactly what it should: 3. Validation path (your 9771cc4) — 4. Embeddings (part of 11b4b29) — The one path I couldn't exercise with a real credential is video generation (no Veo access on this project). Everything else — completions, streaming, validation, embeddings — confirmed working with a real Vertex OAuth bearer. |
# Conflicts: # apps/gateway/src/chat/chat.ts # packages/actions/src/get-provider-headers.spec.ts # packages/actions/src/get-provider-headers.ts
|
Rebased on main and resolved the conflicts — green now. The only real overlap was with the new Vertex flex/priority service-tier headers in |
Quartz is not real Vertex/AI Studio and does not need the OAuth token-type behaviour. Restrict the api-key/oauth token type to google-vertex only: - resolveVertexTokenType now accepts only "google-vertex". - getProviderHeaders: quartz returns the plain header (no Bearer); the token-type + service-tier logic stays google-vertex only. - buildVertexCompatibleEndpoint forces api-key (?key=) for quartz. - Drop quartz_token_type from ProviderKeyOptions, the LLM_QUARTZ_TOKEN_TYPE env mapping/.env.example, the UI Token Type selector, and the chat/resolve-context/video token-type guards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Problem
Closes #1252.
When a user configures a Google Vertex AI provider key with an OAuth2 access token (
ya29.*) instead of an API key, the gateway sends the token as a?key=<token>query parameter. Vertex AI rejects this with:The reverse is also true — Vertex API keys must be sent as
?key=, notAuthorization: Bearer. The current code unconditionally treats the configured credential as an API key.Solution
Detect OAuth-shaped tokens by prefix and route them differently:
isVertexOAuthToken(token)helper in@llmgateway/modelsreturnstruefor tokens starting withya29.(access tokens) or1//(refresh-token-style credentials).get-provider-headers.tssetsAuthorization: Bearer <token>forgoogle-vertex/quartzwhen the token looks like an OAuth token; otherwise returns no auth header (so the existing?key=path still works for API keys).get-provider-endpoint.ts(buildVertexCompatibleEndpoint) skips thekey=<token>query param when the token is OAuth-shaped, for both the lite-model and project-scoped paths.API-key behaviour is unchanged.
Testing
get-provider-headers.spec.tscovering API key, access token, refresh token, and request-id passthrough for bothgoogle-vertexandquartz.get-provider-endpoint.spec.tscovering OAuth and API-key tokens across the lite-model and project-scoped branches.pnpm vitest run packages/actions— 133 tests pass.pnpm lint— no new errors.Summary by CodeRabbit
New Features
Database
Documentation
Tests