add Codex subscription provider with device auth#2715
Open
yaronshanisima wants to merge 4 commits intomaximhq:mainfrom
Open
add Codex subscription provider with device auth#2715yaronshanisima wants to merge 4 commits intomaximhq:mainfrom
yaronshanisima wants to merge 4 commits intomaximhq:mainfrom
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
👮 Files not reviewed due to content moderation or server errors (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new "codex" provider with OAuth device-code and refresh flows, provider wiring, credential persistence (DB + migrations), HTTP auth endpoints, frontend device-auth UI and RTK endpoints, request/response normalization for Chat/Responses, and tests. Changes
Sequence DiagramsequenceDiagram
participant User as User/Client
participant UI as Frontend UI
participant API as CodexAuthHandler
participant Store as Config Store
participant Codex as Codex OAuth Service
participant Provider as CodexProvider
User->>UI: Click "Connect with OpenAI"
UI->>API: POST /codex/auth/start (keyId)
API->>Codex: StartDeviceAuthorization()
Codex-->>API: device_auth_id, user_code, interval
API->>Store: CreateCodexAuthSession(pending)
API-->>UI: session_id, user_code, verification_uri
UI->>User: Display user code & open verification URL
loop User authorizes device
UI->>API: GET /codex/auth/session/{sessionId} (poll)
API->>Store: GetCodexAuthSessionByID()
API->>Codex: PollDeviceAuthorization()
alt authorized
Codex-->>API: authorization_code, code_verifier
API->>Codex: ExchangeDeviceAuthorizationCode()
Codex-->>API: access_token, refresh_token, id_token
API->>Store: UpdateCodexAuthSession(authorized) & PersistCodexKeyConfig
API-->>UI: status: authorized
else pending / retry
API-->>UI: status: pending
end
end
UI->>Provider: Make request with Codex key
Provider->>Store: Read Codex credentials
Provider->>Codex: Chat/Responses request (with access token)
Codex-->>Provider: Response
Provider-->>UI: Response result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
|
@yaronshanisima thanks for the PR. But this is an extremely big PR which will definitely take time from our side to review and test properly. Could you please attach a demo video of the entire flow if possible? |
yaronshanisima
force-pushed
the
yaron/codex-subscription-provider
branch
from
f32567a to
2111eb9
Compare
Contributor
Confidence Score: 4/5
Important Files Changed
Reviews (5): Last reviewed commit: "clean up codex docs and schema diffs" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
yaronshanisima
force-pushed
the
yaron/codex-subscription-provider
branch
from
2111eb9 to
ec6efc1
Compare
![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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/configstore/rdb.go (2)
423-433:⚠️ Potential issue | 🟠 Major
UpdateProviderstill drops provider-level Codex settings.
Save(&dbProvider)never seesconfigCopy.CodexConfigbecause this method doesn't assign it before the save. Any pricing-mode or other provider-level Codex edits made through the update path are silently lost.Suggested fix
dbProvider.CustomProviderConfig = configCopy.CustomProviderConfig dbProvider.OpenAIConfig = configCopy.OpenAIConfig + dbProvider.CodexConfig = configCopy.CodexConfig dbProvider.PricingOverrides = configCopy.PricingOverrides dbProvider.ConfigHash = configCopy.ConfigHashAlso applies to: 474-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 423 - 433, The UpdateProvider path is not copying provider-level Codex settings so Save(&dbProvider) never persists configCopy.CodexConfig; update the assignment block that currently sets NetworkConfig, ConcurrencyAndBufferSize, ProxyConfig, SendBackRawRequest, SendBackRawResponse, StoreRawRequestResponse, CustomProviderConfig, OpenAIConfig, PricingOverrides, ConfigHash to also assign dbProvider.CodexConfig = configCopy.CodexConfig before calling Save(&dbProvider), and make the same addition at the other update location referenced around the 474-475 area so provider-level Codex edits are not lost.
563-573:⚠️ Potential issue | 🟠 Major
AddProviderdoesn't persistCodexKeyConfigon new keys.The provider row now stores
CodexConfig, but theTableKeyliteral still omitskey.CodexKeyConfig. Creating a Codex key with preloaded/manual credentials will insert the key and drop those credentials immediately.Suggested fix
dbKey := tables.TableKey{ Provider: dbProvider.Name, ProviderID: dbProvider.ID, KeyID: key.ID, Name: key.Name, Value: key.Value, Models: key.Models, BlacklistedModels: key.BlacklistedModels, Weight: &key.Weight, Enabled: key.Enabled, UseForBatchAPI: key.UseForBatchAPI, AzureKeyConfig: key.AzureKeyConfig, VertexKeyConfig: key.VertexKeyConfig, BedrockKeyConfig: key.BedrockKeyConfig, ReplicateKeyConfig: key.ReplicateKeyConfig, VLLMKeyConfig: key.VLLMKeyConfig, + CodexKeyConfig: key.CodexKeyConfig, ConfigHash: key.ConfigHash, Status: string(key.Status), Description: key.Description, }Also applies to: 583-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 563 - 573, The AddProvider path is building tables.TableKey entries but omits key.CodexKeyConfig, so Codex credentials are dropped; update the TableKey literal(s) created alongside dbProvider (look for the TableKey construction near dbProvider := tables.TableProvider and the similar block at the later occurrence) to include CodexKeyConfig: key.CodexKeyConfig, ensuring both places persist the Codex key config when inserting keys; no other structural changes required.
🧹 Nitpick comments (8)
core/providers/codex/auth_test.go (1)
5-19: Basic test coverage for auth helpers.Tests cover the happy path for
ExtractAccountIDandExpiresAtFromNow. Consider adding edge case tests for:
ExtractAccountID(nil)- Malformed JWT tokens
- Tokens without account ID claims
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/codex/auth_test.go` around lines 5 - 19, Add unit tests covering edge cases for ExtractAccountID and ExpiresAtFromNow: add a test that calls ExtractAccountID(nil) and asserts it returns an empty string (or the expected zero value), tests that pass malformed JWT strings to ExtractAccountID and assert it safely returns empty/error without panicking, and a test with a well-formed JWT that lacks the account ID claim to assert empty result; also add a test for ExpiresAtFromNow with zero/negative input to assert expected behavior (empty or RFC3339 handling). Target the existing test file and the functions ExtractAccountID and ExpiresAtFromNow when adding these negative-case tests.ui/app/workspace/providers/fragments/index.ts (1)
3-4: Prefer@/...alias imports for new UI exports.These new exports continue relative paths; please switch them to the UI alias pattern for consistency with current repo conventions.
♻️ Suggested update
-export { CodexAuthControls } from "./codexAuthControls"; -export { CodexConfigFormFragment } from "./codexConfigFormFragment"; +export { CodexAuthControls } from "@/app/workspace/providers/fragments/codexAuthControls"; +export { CodexConfigFormFragment } from "@/app/workspace/providers/fragments/codexConfigFormFragment";Based on learnings: In the UI codebase, prefer alias imports using
@/... over relative imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/index.ts` around lines 3 - 4, Replace the relative export paths with the UI alias pattern: change the two exports for CodexAuthControls and CodexConfigFormFragment to re-export from the corresponding "@/..." alias modules (use the module paths that map to the same files under the UI alias) so the file exports CodexAuthControls and CodexConfigFormFragment via the alias imports instead of "./codexAuthControls" and "./codexConfigFormFragment".ui/lib/store/apis/providersApi.ts (1)
124-147: SeedgetCodexAuthSessionfrom these mutation results.
startCodex*andcancelCodexAuthSessionreturn the same session payload thatgetCodexAuthSessionreads, but nothing patches the cached session entry. The session view will stay stale until the next poll/refetch, which is especially visible right after cancel.Based on learnings "In ui/lib/store/apis/, optimistically patch the cache with onQueryStarted + routingRulesApi.util.updateQueryData for mutations ... instead of using invalidatesTags."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/store/apis/providersApi.ts` around lines 124 - 147, The getCodexAuthSession query cache is never updated when startCodexBrowserAuth, startCodexDeviceAuth, or cancelCodexAuthSession mutate the session; add onQueryStarted handlers to those three mutations that await the mutation response and then call providersApi.util.updateQueryData for getCodexAuthSession (using the sessionId from the response) to replace/seed the cached CodexAuthSessionResponse so the UI reflects the new session state immediately; ensure you handle errors by reverting the update if the mutation fails.ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
281-288: Subscribe tokey.idbefore passing it toCodexAuthControls.
getValues()is just a snapshot. If the create flow persists the key and writeskey.idback into the form, this prop can stayundefineduntil some unrelated rerender, soCodexAuthControlswon't reliably see the new key ID.♻️ Proposed refactor
-import { Control, UseFormReturn } from "react-hook-form"; +import { Control, UseFormReturn, useWatch } from "react-hook-form"; ... + const keyId = useWatch({ control, name: "key.id" }); ... <CodexAuthControls providerName={providerName} - keyId={form.getValues("key.id")} + keyId={keyId} form={form} isEditing={isEditing} isConfigManaged={isConfigManaged}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 281 - 288, Replace the snapshot call form.getValues("key.id") with a reactive subscription so CodexAuthControls always receives updates when the form writes back the persisted key id; e.g. use form.watch("key.id") or React Hook Form's useWatch tied to the same form instance and pass that watchedKeyId to CodexAuthControls (referencing CodexAuthControls, form, and the "key.id" field).transports/bifrost-http/handlers/codexauth.go (1)
243-257: Silently ignored errors in terminal state updates.The
completeSessionandfailSessionmethods ignore errors fromUpdateCodexAuthSession. While this is acceptable for terminal states (the session won't be polled again), consider logging these failures for observability.💡 Suggested improvement for observability
func (h *CodexAuthHandler) completeSession(session *configstoreTables.TableCodexAuthSession) { now := time.Now() session.Status = codexAuthSessionSucceeded session.CompletedAt = &now session.LastError = nil - _ = h.store.ConfigStore.UpdateCodexAuthSession(context.Background(), session) + if err := h.store.ConfigStore.UpdateCodexAuthSession(context.Background(), session); err != nil { + // Log but don't fail - session is already in terminal state + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/codexauth.go` around lines 243 - 257, The terminal-state helpers completeSession and failSession currently discard errors from h.store.ConfigStore.UpdateCodexAuthSession; change both to capture the returned error and log it (e.g., using an existing logger on the handler such as h.logger or processLogger) including context like the session ID and whether it was a success or failure so failed updates are observable; still treat the state change as terminal but do not ignore the UpdateCodexAuthSession error—log it with a clear message and the error value after calling UpdateCodexAuthSession.ui/app/workspace/providers/fragments/codexConfigFormFragment.tsx (1)
37-41: Consider removingformfrom the dependency array.Including
formin the dependency array may cause unnecessary form resets since theformobject reference can change on re-renders. The reset should only trigger whenprovider.codex_config?.pricing_modeorprovider.namechanges.💡 Suggested refinement
useEffect(() => { form.reset({ pricing_mode: provider.codex_config?.pricing_mode ?? "included_zero", }); - }, [form, provider.codex_config?.pricing_mode, provider.name]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [provider.codex_config?.pricing_mode, provider.name]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/codexConfigFormFragment.tsx` around lines 37 - 41, The useEffect currently resets the form via form.reset when any of its dependencies change; remove form from the dependency array so the effect only runs when provider.codex_config?.pricing_mode or provider.name change. Locate the useEffect that calls form.reset (reference symbols: useEffect, form.reset, provider.codex_config?.pricing_mode, provider.name) and update the dependency array to exclude form to avoid unnecessary resets while keeping the two provider-based dependencies.core/providers/codex/codex.go (1)
83-143: Consider reusing the HTTP client for token refresh.The
authHeadersmethod creates a newhttp.Clienton line 95 for each token refresh. While functional, this could be optimized by reusingh.httpClient(which appears to be intended for auth operations but isn't stored on the provider).💡 Suggested improvement
Consider adding an
authHTTPClient *http.Clientfield toCodexProviderinitialized in the constructor, similar toCodexAuthHandler, to avoid repeated client creation during token refresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/codex/codex.go` around lines 83 - 143, The authHeaders method on CodexProvider creates a new http.Client per refresh; add a reusable http client field (e.g., authHTTPClient *http.Client) to CodexProvider, initialize it in the provider constructor, and use that client when calling RefreshAccessToken instead of creating &http.Client{Timeout: 20*time.Second}; update authHeaders to call RefreshAccessToken(requestCtx, provider.authHTTPClient, refreshToken) and ensure any related methods like persistRefreshedCredentials and RefreshAccessToken still work with the reused client.ui/app/workspace/providers/fragments/codexAuthControls.tsx (1)
147-160: Consider cleaning up the popup window on component unmount.If the user navigates away while the auth dialog is open, the popup window will remain open. Consider adding cleanup in a useEffect.
💡 Suggested cleanup on unmount
+ useEffect(() => { + return () => { + if (popupRef.current && !popupRef.current.closed) { + popupRef.current.close(); + } + }; + }, []); + const beginDeviceFlow = async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx` around lines 147 - 160, The popup opened in beginDeviceFlow (popupRef.current = window.open(...)) is not closed if the component unmounts; add a useEffect cleanup that on unmount checks popupRef.current, calls .close() if it's open, and clears popupRef.current to avoid leaking the window; also consider closing the popup when setSession(null) or when the dialog is dismissed. Locate popupRef usage and implement the cleanup in a useEffect with a return cleanup function that closes and nulls popupRef.current.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/bifrost.go`:
- Around line 3640-3641: The Codex provider branch returns
codex.NewCodexProvider(config, bifrost.logger) but must return the
Provider,error tuple like the other providers; change the return in the
schemas.Codex case to return codex.NewCodexProvider(config, bifrost.logger), nil
(or adjust to match NewCodexProvider's signature) so it returns (Provider,
error). Also add CODEX_API_KEY into the workflow environment blocks in
pr-tests.yml and release-pipeline.yml — in each job env section where
OPENAI_API_KEY and ANTHROPIC_API_KEY are set (around 6–7 job env blocks), add
CODEX_API_KEY: ${{ secrets.CODEX_API_KEY }} so Codex is available to CI jobs.
In `@core/providers/codex/auth.go`:
- Around line 185-196: Remove the dead/unreferenced helper function
generateRandomString from the file (the function declaration and its body) since
it isn't used anywhere; delete the entire generateRandomString function
definition to eliminate dead code and keep the codebase clean, and run
tests/linters to ensure no references remain to generateRandomString.
In `@core/providers/codex/codex_test.go`:
- Around line 82-88: The test currently only checks block types and non-nil
output text; update it to assert the actual preserved string contents so
normalization hasn't altered or blanked them: after locating the blocks via
request.Input[0].Content.ContentBlocks[0] and
request.Input[1].Content.ContentBlocks[0] assert that the original user message
text equals the input block's text field and that the original assistant/output
message text equals the ResponsesOutputMessageContentText (or its text field)
value; modify the assertions in codex_test.go to compare the expected literal
strings to these fields rather than only checking type/non-nil.
In `@core/providers/openai/openai.go`:
- Line 1119: The branch that checks for schemas.Codex using providerName is
incorrect for aliased providers; update the call sites (where
providerUtils.DrainNonSSEStreamResponse is invoked) to use the canonical/base
provider type or an explicit capability flag instead of providerName so aliased
Codex providers are treated as Codex for streaming behavior; propagate the base
discriminator from the provider (the same value used by
providerUtils.CheckOperationAllowed) or add a boolean like
supportsCodexStreaming to the shared helper signatures, and ensure
provider.GetProviderKey() continues to be used only for alias/error attribution
while the base type/capability is used for branching in
DrainNonSSEStreamResponse and related logic.
In `@framework/configstore/migrations.go`:
- Around line 3992-3999: The two new fields CodexAccessTokenExpiresAt and
CodexAuthMethod on TableKey are not being encrypted/decrypted like the other
Codex fields; update their types to the same pointer EnvVar type used by
CodexRefreshToken/CodexAccessToken/CodexAccountID (or otherwise wrap them
consistently) and modify the TableKey.BeforeSave() logic to call
encryptEnvVarPtr() for CodexAccessTokenExpiresAt and CodexAuthMethod and the
TableKey.AfterFind() logic to call decryptEnvVarPtr() for those same fields so
they go through the same encryption/decryption hooks as the other Codex
credential fields.
- Around line 280-287: Don't modify historical migrations; instead add a new
forward-only migration (e.g., migrationRecomputeProviderConfigHashWithCodex) and
call it immediately after migrationAddCodexConfigColumns in the migration
sequence. The new migration should scan providers/users that need backfilling,
recompute their config_hash including the new codex_config_json field, and
update only rows that are missing or outdated (idempotent and forward-only),
leaving migrationAddCodexConfigColumns, migrationAddCodexAuthSessionsTable, and
migrationCleanupCodexBrowserAuthColumns unchanged.
In `@framework/configstore/tables/codexauth.go`:
- Around line 36-46: The current encryption block only sets s.EncryptionStatus =
EncryptionStatusEncrypted when at least one field was actually encrypted,
leaving rows with empty DeviceAuthID at plain_text; change the logic in the
encrypt.IsEnabled() branch (around s.DeviceAuthID and encryptString usage) to
always set s.EncryptionStatus = EncryptionStatusEncrypted whenever
encrypt.IsEnabled() returns true, even if no individual field was encrypted, so
the repo invariant for encrypted-row status is preserved.
In `@transports/bifrost-http/handlers/codexauth.go`:
- Line 218: Update the error messages created with fmt.Errorf to follow Go
conventions: start with a lowercase letter and no trailing punctuation. Replace
messages like "Codex key %s not found" with "codex key %s not found" in the
return fmt.Errorf(...) that references keyID, and make the same change for the
other occurrence noted (the similar fmt.Errorf at the second location). Ensure
both fmt.Errorf calls are updated so error strings are lowercase and have no
trailing punctuation.
In `@transports/bifrost-http/lib/config.go`:
- Around line 3123-3174: The current flow snapshots c.Providers[schemas.Codex]
under c.Mu.RLock, releases the lock, persists the full updated provider, then
overwrites c.Providers with a stale snapshot; instead serialize the
read-modify-write by doing the modify-and-persist while holding a proper
write/compare step: inside ConfigStore.ExecuteTransaction (or just before it)
read the latest provider state (use UpdateProvider/GetProvider API) into a local
variable, apply only the per-key updates to that latest provider (modify the
matching Keys entry by ID and replace only CodexKeyConfig fields), persist via
c.ConfigStore.UpdateProvider, and only after a successful transaction acquire
c.Mu.Lock() and merge/replace the provider in c.Providers (or check that the
in-memory provider hasn’t diverged) before unlocking; this avoids clobbering
concurrent UpdateProviderConfig/RemoveProvider operations and ensures the update
is atomic with respect to other goroutines.
In `@transports/bifrost-http/server/server.go`:
- Around line 1161-1166: The persistCodexCredentialRefresh function uses
context.Background()/schemas.NoDeadline which can block indefinitely; modify
persistCodexCredentialRefresh (or add an overload) to accept a caller
context.Context (or derive a bounded context from an existing server
shutdown/context field) and create a cancellable/timeout context before calling
s.Config.PersistCodexKeyCredentials so DB/config store calls can't hang forever;
update callers to pass through their request context (or use a configured
timeout on BifrostHTTPServer) and ensure the call uses that derived context
instead of context.Background().
- Around line 1142-1149: The credential persister middleware is being appended
into commonMiddlewares after TransportInterceptorMiddleware runs, so the
interceptor's getBifrostContextFromFastHTTP materializes a BifrostContext
without the persister; move the persister setup (the func that calls
ctx.SetUserValue(schemas.BifrostContextKeyCodexCredentialPersister,
schemas.CodexCredentialPersister(s.persistCodexCredentialRefresh))) to execute
before TransportInterceptorMiddleware (either insert it before you prepend
TransportInterceptorMiddleware at the prepend point around where
TracingMiddleware is handled, or prepend it separately ahead of
TransportInterceptorMiddleware at line ~1373), and ensure AuthMiddleware is also
arranged so it runs after the persister but not before the interceptor;
reference TransportInterceptorMiddleware, getBifrostContextFromFastHTTP,
TracingMiddleware, AuthMiddleware,
schemas.BifrostContextKeyCodexCredentialPersister and
s.persistCodexCredentialRefresh when locating where to move the code.
In `@transports/config.schema.json`:
- Around line 2698-2704: Add a new schema definition "#/$defs/codex_key" that
extends "base_key" and requires the provider-specific "codex_key_config"
property (mirroring how "azure_key" and "vllm_key" are defined), then update the
"provider_with_codex_config.keys" items reference to point to
"#/$defs/codex_key" instead of "#/$defs/base_key" so keys must include the
Codex-specific config; ensure "codex_key" appears alongside other key defs in
"$defs" and that "codex_key_config" is referenced as required within that
definition.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 209-274: Add stable data-testid attributes to the new Codex
manual-auth UI elements so e2e tests can target them: add
data-testid="apikey-codex-manual-credentials-trigger" to the CollapsibleTrigger,
data-testid="apikey-codex-refresh-token-input" to the EnvVarInput rendering
`key.codex_key_config.refresh_token`,
data-testid="apikey-codex-access-token-input" to the EnvVarInput rendering
`key.codex_key_config.access_token`, data-testid="apikey-codex-account-id-input"
to the EnvVarInput rendering `key.codex_key_config.account_id`, and
data-testid="apikey-codex-access-token-expiry-input" to the Input rendering
`key.codex_key_config.access_token_expires_at`; ensure attributes are placed on
the interactive component instances (CollapsibleTrigger, EnvVarInput, Input)
rather than wrappers to match existing `apikey-*` selectors convention.
---
Outside diff comments:
In `@framework/configstore/rdb.go`:
- Around line 423-433: The UpdateProvider path is not copying provider-level
Codex settings so Save(&dbProvider) never persists configCopy.CodexConfig;
update the assignment block that currently sets NetworkConfig,
ConcurrencyAndBufferSize, ProxyConfig, SendBackRawRequest, SendBackRawResponse,
StoreRawRequestResponse, CustomProviderConfig, OpenAIConfig, PricingOverrides,
ConfigHash to also assign dbProvider.CodexConfig = configCopy.CodexConfig before
calling Save(&dbProvider), and make the same addition at the other update
location referenced around the 474-475 area so provider-level Codex edits are
not lost.
- Around line 563-573: The AddProvider path is building tables.TableKey entries
but omits key.CodexKeyConfig, so Codex credentials are dropped; update the
TableKey literal(s) created alongside dbProvider (look for the TableKey
construction near dbProvider := tables.TableProvider and the similar block at
the later occurrence) to include CodexKeyConfig: key.CodexKeyConfig, ensuring
both places persist the Codex key config when inserting keys; no other
structural changes required.
---
Nitpick comments:
In `@core/providers/codex/auth_test.go`:
- Around line 5-19: Add unit tests covering edge cases for ExtractAccountID and
ExpiresAtFromNow: add a test that calls ExtractAccountID(nil) and asserts it
returns an empty string (or the expected zero value), tests that pass malformed
JWT strings to ExtractAccountID and assert it safely returns empty/error without
panicking, and a test with a well-formed JWT that lacks the account ID claim to
assert empty result; also add a test for ExpiresAtFromNow with zero/negative
input to assert expected behavior (empty or RFC3339 handling). Target the
existing test file and the functions ExtractAccountID and ExpiresAtFromNow when
adding these negative-case tests.
In `@core/providers/codex/codex.go`:
- Around line 83-143: The authHeaders method on CodexProvider creates a new
http.Client per refresh; add a reusable http client field (e.g., authHTTPClient
*http.Client) to CodexProvider, initialize it in the provider constructor, and
use that client when calling RefreshAccessToken instead of creating
&http.Client{Timeout: 20*time.Second}; update authHeaders to call
RefreshAccessToken(requestCtx, provider.authHTTPClient, refreshToken) and ensure
any related methods like persistRefreshedCredentials and RefreshAccessToken
still work with the reused client.
In `@transports/bifrost-http/handlers/codexauth.go`:
- Around line 243-257: The terminal-state helpers completeSession and
failSession currently discard errors from
h.store.ConfigStore.UpdateCodexAuthSession; change both to capture the returned
error and log it (e.g., using an existing logger on the handler such as h.logger
or processLogger) including context like the session ID and whether it was a
success or failure so failed updates are observable; still treat the state
change as terminal but do not ignore the UpdateCodexAuthSession error—log it
with a clear message and the error value after calling UpdateCodexAuthSession.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 281-288: Replace the snapshot call form.getValues("key.id") with a
reactive subscription so CodexAuthControls always receives updates when the form
writes back the persisted key id; e.g. use form.watch("key.id") or React Hook
Form's useWatch tied to the same form instance and pass that watchedKeyId to
CodexAuthControls (referencing CodexAuthControls, form, and the "key.id" field).
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx`:
- Around line 147-160: The popup opened in beginDeviceFlow (popupRef.current =
window.open(...)) is not closed if the component unmounts; add a useEffect
cleanup that on unmount checks popupRef.current, calls .close() if it's open,
and clears popupRef.current to avoid leaking the window; also consider closing
the popup when setSession(null) or when the dialog is dismissed. Locate popupRef
usage and implement the cleanup in a useEffect with a return cleanup function
that closes and nulls popupRef.current.
In `@ui/app/workspace/providers/fragments/codexConfigFormFragment.tsx`:
- Around line 37-41: The useEffect currently resets the form via form.reset when
any of its dependencies change; remove form from the dependency array so the
effect only runs when provider.codex_config?.pricing_mode or provider.name
change. Locate the useEffect that calls form.reset (reference symbols:
useEffect, form.reset, provider.codex_config?.pricing_mode, provider.name) and
update the dependency array to exclude form to avoid unnecessary resets while
keeping the two provider-based dependencies.
In `@ui/app/workspace/providers/fragments/index.ts`:
- Around line 3-4: Replace the relative export paths with the UI alias pattern:
change the two exports for CodexAuthControls and CodexConfigFormFragment to
re-export from the corresponding "@/..." alias modules (use the module paths
that map to the same files under the UI alias) so the file exports
CodexAuthControls and CodexConfigFormFragment via the alias imports instead of
"./codexAuthControls" and "./codexConfigFormFragment".
In `@ui/lib/store/apis/providersApi.ts`:
- Around line 124-147: The getCodexAuthSession query cache is never updated when
startCodexBrowserAuth, startCodexDeviceAuth, or cancelCodexAuthSession mutate
the session; add onQueryStarted handlers to those three mutations that await the
mutation response and then call providersApi.util.updateQueryData for
getCodexAuthSession (using the sessionId from the response) to replace/seed the
cached CodexAuthSessionResponse so the UI reflects the new session state
immediately; ensure you handle errors by reverting the update if the mutation
fails.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d06c0331-a1db-4728-b565-731397115f2d
📒 Files selected for processing (44)
core/bifrost.gocore/providers/codex/auth.gocore/providers/codex/auth_test.gocore/providers/codex/codex.gocore/providers/codex/codex_test.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/schemas/account.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/provider.gocore/utils.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/codexauth.goframework/configstore/tables/key.goframework/configstore/tables/provider.goframework/modelcatalog/codex.goframework/modelcatalog/codex_test.goframework/modelcatalog/main.goframework/modelcatalog/main_test.goframework/modelcatalog/pricing.goplugins/maxim/plugin_test.gotransports/bifrost-http/handlers/codexauth.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/providerConfigSheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/codexAuthControls.tsxui/app/workspace/providers/fragments/codexConfigFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/providerKeyForm.tsxui/lib/constants/config.tsui/lib/constants/icons.tsxui/lib/constants/logs.tsui/lib/store/apis/providersApi.tsui/lib/types/config.tsui/lib/types/schemas.ts
👮 Files not reviewed due to content moderation or server errors (1)
- ui/lib/constants/icons.tsx
Comment on lines
+3640
to
+3641
| case schemas.Codex: | ||
| return codex.NewCodexProvider(config, bifrost.logger) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Provider registration check =="
rg -n --type=go -C2 'case\s+schemas\.Codex|NewCodexProvider' core/bifrost.go
echo
echo "== Workflow Codex env/config check =="
for f in .github/workflows/pr-tests.yml .github/workflows/release-pipeline.yml; do
if [[ -f "$f" ]]; then
echo "---- $f ----"
rg -n -C3 'codex|CODEX' "$f" || true
else
echo "Missing expected workflow file: $f"
fi
doneRepository: maximhq/bifrost
Length of output: 507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All workflow files in .github/workflows =="
fd -e yml -e yaml . .github/workflows/ 2>/dev/null | sort
echo
echo "== Check what env vars/configs are in pr-tests.yml (full file) =="
head -100 .github/workflows/pr-tests.yml
echo
echo "== Check what env vars/configs are in release-pipeline.yml (full file) =="
head -100 .github/workflows/release-pipeline.yml
echo
echo "== Search for any existing provider env patterns to understand the structure =="
rg -n 'OPENAI|ANTHROPIC' .github/workflows/ -C 2Repository: maximhq/bifrost
Length of output: 19520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check return statements for all providers in bifrost.go =="
rg -n 'return.*Provider.*config.*bifrost.logger' core/bifrost.go | head -20Repository: maximhq/bifrost
Length of output: 1447
Add CODEX_API_KEY to workflow environments and fix return statement.
The Codex provider registration exists but has two issues:
- Missing
, nilin return statement (line 3641) — inconsistent with providers like OpenAI, Anthropic, Mistral, Gemini that return(Provider, error). - CODEX_API_KEY not wired into workflows — add
CODEX_API_KEY: ${{ secrets.CODEX_API_KEY }}to env sections inpr-tests.ymlandrelease-pipeline.yml(approximately 6–7 job env blocks where OPENAI_API_KEY and ANTHROPIC_API_KEY appear).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/bifrost.go` around lines 3640 - 3641, The Codex provider branch returns
codex.NewCodexProvider(config, bifrost.logger) but must return the
Provider,error tuple like the other providers; change the return in the
schemas.Codex case to return codex.NewCodexProvider(config, bifrost.logger), nil
(or adjust to match NewCodexProvider's signature) so it returns (Provider,
error). Also add CODEX_API_KEY into the workflow environment blocks in
pr-tests.yml and release-pipeline.yml — in each job env section where
OPENAI_API_KEY and ANTHROPIC_API_KEY are set (around 6–7 job env blocks), add
CODEX_API_KEY: ${{ secrets.CODEX_API_KEY }} so Codex is available to CI jobs.
Comment on lines
+185
to
+196
| func generateRandomString(length int) (string, error) { | ||
| const chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~" | ||
| bytes := make([]byte, length) | ||
| if _, err := rand.Read(bytes); err != nil { | ||
| return "", err | ||
| } | ||
| result := make([]byte, length) | ||
| for i, value := range bytes { | ||
| result[i] = chars[int(value)%len(chars)] | ||
| } | ||
| return string(result), nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of generateRandomString across the codebase
rg -n 'generateRandomString' --type=goRepository: maximhq/bifrost
Length of output: 147
Remove unused generateRandomString function.
This function is not called anywhere in the codebase and should be removed as dead code. If it is intended for future use, it can be added back later when needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/providers/codex/auth.go` around lines 185 - 196, Remove the
dead/unreferenced helper function generateRandomString from the file (the
function declaration and its body) since it isn't used anywhere; delete the
entire generateRandomString function definition to eliminate dead code and keep
the codebase clean, and run tests/linters to ensure no references remain to
generateRandomString.
Comment on lines
+82
to
+88
| require.Len(t, request.Input, 2) | ||
| require.Len(t, request.Input[0].Content.ContentBlocks, 1) | ||
| require.Equal(t, schemas.ResponsesInputMessageContentBlockTypeText, request.Input[0].Content.ContentBlocks[0].Type) | ||
| require.Len(t, request.Input[1].Content.ContentBlocks, 1) | ||
| require.Equal(t, schemas.ResponsesOutputMessageContentTypeText, request.Input[1].Content.ContentBlocks[0].Type) | ||
| require.NotNil(t, request.Input[1].Content.ContentBlocks[0].ResponsesOutputMessageContentText) | ||
| } |
Contributor
There was a problem hiding this comment.
Assert the preserved text value, not just that a typed block exists.
This test still passes if normalization rewrites or blanks the user/assistant payload, because it only checks the block type and a non-nil output-text field. Since the contract here is preservation, assert the actual text contents too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/providers/codex/codex_test.go` around lines 82 - 88, The test currently
only checks block types and non-nil output text; update it to assert the actual
preserved string contents so normalization hasn't altered or blanked them: after
locating the blocks via request.Input[0].Content.ContentBlocks[0] and
request.Input[1].Content.ContentBlocks[0] assert that the original user message
text equals the input block's text field and that the original assistant/output
message text equals the ResponsesOutputMessageContentText (or its text field)
value; modify the assertions in codex_test.go to compare the expected literal
strings to these fields rather than only checking type/non-nil.
| // Skip scanner for non-SSE responses — avoids bufio.Scanner buffer bloat | ||
| // on non-line-delimited data (e.g. provider returned JSON instead of SSE). | ||
| if providerUtils.DrainNonSSEStreamResponse(resp) { | ||
| if providerName != schemas.Codex && providerUtils.DrainNonSSEStreamResponse(resp) { |
Contributor
There was a problem hiding this comment.
Branch this Codex exception on the base provider, not providerName.
Line 1119 and Line 1721 compare the resolved providerName to schemas.Codex, but providerName is also the alias-facing key written into response/error metadata. A custom-named Codex provider won't hit this bypass and will still take the old DrainNonSSEStreamResponse path, so the new Codex streaming behavior won't apply for aliased Codex configs. Please plumb the canonical provider type, or an explicit capability flag, into these shared helpers and branch on that instead.
Based on learnings: providerUtils.CheckOperationAllowed uses the base provider type discriminator, while provider.GetProviderKey() is reserved for alias/error attribution.
Also applies to: 1721-1721
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/providers/openai/openai.go` at line 1119, The branch that checks for
schemas.Codex using providerName is incorrect for aliased providers; update the
call sites (where providerUtils.DrainNonSSEStreamResponse is invoked) to use the
canonical/base provider type or an explicit capability flag instead of
providerName so aliased Codex providers are treated as Codex for streaming
behavior; propagate the base discriminator from the provider (the same value
used by providerUtils.CheckOperationAllowed) or add a boolean like
supportsCodexStreaming to the shared helper signatures, and ensure
provider.GetProviderKey() continues to be used only for alias/error attribution
while the base type/capability is used for branching in
DrainNonSSEStreamResponse and related logic.
Comment on lines
+280
to
+287
| if err := migrationAddCodexConfigColumns(ctx, db); err != nil { | ||
| return err | ||
| } | ||
| if err := migrationAddCodexAuthSessionsTable(ctx, db); err != nil { | ||
| return err | ||
| } | ||
| if err := migrationCleanupCodexBrowserAuthColumns(ctx, db); err != nil { | ||
| return err |
Contributor
There was a problem hiding this comment.
Add a new forward-only Codex hash migration here instead of editing old ones.
Threading Codex into add_config_hash_column / add_store_raw_request_response_column is unsafe in both directions: databases that already applied those IDs will never rerun the new backfill, and databases upgrading from a release that predates add_config_hash_column will hit that historical migration before codex_config_json exists. Keep the old migrations pre-Codex and add a new migration immediately after migrationAddCodexConfigColumns that recomputes provider config_hash with the new field.
Suggested follow-up
if err := migrationAddCodexConfigColumns(ctx, db); err != nil {
return err
}
+if err := migrationBackfillCodexProviderConfigHash(ctx, db); err != nil {
+ return err
+}
if err := migrationAddCodexAuthSessionsTable(ctx, db); err != nil {
return err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/configstore/migrations.go` around lines 280 - 287, Don't modify
historical migrations; instead add a new forward-only migration (e.g.,
migrationRecomputeProviderConfigHashWithCodex) and call it immediately after
migrationAddCodexConfigColumns in the migration sequence. The new migration
should scan providers/users that need backfilling, recompute their config_hash
including the new codex_config_json field, and update only rows that are missing
or outdated (idempotent and forward-only), leaving
migrationAddCodexConfigColumns, migrationAddCodexAuthSessionsTable, and
migrationCleanupCodexBrowserAuthColumns unchanged.
Comment on lines
+1161
to
+1166
| func (s *BifrostHTTPServer) persistCodexCredentialRefresh(keyID string, refreshed *schemas.CodexKeyConfig) error { | ||
| if s.Config == nil || refreshed == nil { | ||
| return nil | ||
| } | ||
| ctx := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) | ||
| return s.Config.PersistCodexKeyCredentials(ctx, keyID, refreshed) |
Contributor
There was a problem hiding this comment.
Don't write refreshed credentials with an unbounded background context.
PersistCodexKeyCredentials can hit the DB/config store from the inference path. Using context.Background() plus schemas.NoDeadline means a slow store can stall token refresh and request completion indefinitely.
Suggested fix
func (s *BifrostHTTPServer) persistCodexCredentialRefresh(keyID string, refreshed *schemas.CodexKeyConfig) error {
if s.Config == nil || refreshed == nil {
return nil
}
- ctx := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline)
+ ctx := schemas.NewBifrostContext(context.Background(), time.Now().Add(5*time.Second))
+ defer ctx.Cancel()
return s.Config.PersistCodexKeyCredentials(ctx, keyID, refreshed)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/server/server.go` around lines 1161 - 1166, The
persistCodexCredentialRefresh function uses
context.Background()/schemas.NoDeadline which can block indefinitely; modify
persistCodexCredentialRefresh (or add an overload) to accept a caller
context.Context (or derive a bounded context from an existing server
shutdown/context field) and create a cancellable/timeout context before calling
s.Config.PersistCodexKeyCredentials so DB/config store calls can't hang forever;
update callers to pass through their request context (or use a configured
timeout on BifrostHTTPServer) and ensure the call uses that derived context
instead of context.Background().
Comment on lines
+2698
to
+2704
| "keys": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/$defs/base_key" | ||
| }, | ||
| "minItems": 1, | ||
| "description": "Codex subscription credentials for this provider" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the structure and find the codex.keys section
fd config.schema.json transports/Repository: maximhq/bifrost
Length of output: 88
🏁 Script executed:
# Now let's examine the relevant section and surrounding context
cat -n transports/config.schema.json | sed -n '2690,2720p'Repository: maximhq/bifrost
Length of output: 1156
🏁 Script executed:
# Let's search for base_key, azure_key, vllm_key, and codex_key definitions
rg "\"(base_key|azure_key|vllm_key|codex_key)\"" transports/config.schema.json -A 10Repository: maximhq/bifrost
Length of output: 855
🏁 Script executed:
# Check if there's any codex_key_config mentioned in the schema
rg "codex_key_config" transports/config.schema.jsonRepository: maximhq/bifrost
Length of output: 484
🏁 Script executed:
# Let's also check the git history to see if there are related PRs or changes
git log --oneline -20 transports/config.schema.jsonRepository: maximhq/bifrost
Length of output: 120
🏁 Script executed:
# Get full azure_key and vllm_key definitions
rg "\"azure_key\"|\"vllm_key\"" transports/config.schema.json -A 30 | head -100Repository: maximhq/bifrost
Length of output: 1870
🏁 Script executed:
# Search for any existing codex_key definition
rg "\"codex_key\"" transports/config.schema.jsonRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Get the full context of codex_key_config usage
rg "codex_key_config" transports/config.schema.json -B 5 -A 10Repository: maximhq/bifrost
Length of output: 3586
🏁 Script executed:
# Check the recent commit that added codex
git show ec6efc1d --statRepository: maximhq/bifrost
Length of output: 50372
🏁 Script executed:
# Look for codex_key definition (not codex_key_config)
rg "\"codex_key\"" transports/config.schema.json -B 2 -A 40Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Get the full base_key definition to see if it validates codex_key_config properly
rg "\"base_key\"" transports/config.schema.json -A 60 | head -80Repository: maximhq/bifrost
Length of output: 1806
🏁 Script executed:
# Search for where azure_key and vllm_key are actually used in provider schemas
rg "provider_with_azure_config|provider_with_vllm_config" transports/config.schema.json -A 15Repository: maximhq/bifrost
Length of output: 1871
🏁 Script executed:
# Check the full context of how keys are validated across all providers
rg "provider_with_.*_config.*items" transports/config.schema.json -B 2 -A 8Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Search more broadly for codex_key in the entire config file
grep -n "codex_key" transports/config.schema.json | head -20Repository: maximhq/bifrost
Length of output: 524
Create a dedicated codex_key schema definition for the Codex provider.
The provider_with_codex_config.keys currently references only base_key, which does not validate Codex-specific credentials. This allows invalid configurations like { "name": "...", "weight": 1 } to pass validation. Other specialized providers (azure_key, vllm_key) define their own key schemas extending base_key with required provider-specific config fields. Codex should follow the same pattern: create a codex_key definition that enforces codex_key_config as required, then update line 2701 to reference it.
Proposed fix
"keys": {
"type": "array",
"items": {
- "$ref": "#/$defs/base_key"
+ "$ref": "#/$defs/codex_key"
},
"minItems": 1,
"description": "Codex subscription credentials for this provider"
}Add #/$defs/codex_key alongside azure_key and vllm_key that extends base_key and requires codex_key_config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/config.schema.json` around lines 2698 - 2704, Add a new schema
definition "#/$defs/codex_key" that extends "base_key" and requires the
provider-specific "codex_key_config" property (mirroring how "azure_key" and
"vllm_key" are defined), then update the "provider_with_codex_config.keys" items
reference to point to "#/$defs/codex_key" instead of "#/$defs/base_key" so keys
must include the Codex-specific config; ensure "codex_key" appears alongside
other key defs in "$defs" and that "codex_key_config" is referenced as required
within that definition.
Comment on lines
+209
to
+274
| <CollapsibleTrigger className="flex w-full items-center justify-between text-left"> | ||
| <div className="space-y-1"> | ||
| <div className="text-sm font-medium">Manual credentials (advanced)</div> | ||
| <p className="text-muted-foreground text-xs"> | ||
| Most users should use the connect flow above. Only open this if you already have Codex tokens and want to manage them | ||
| manually. | ||
| </p> | ||
| </div> | ||
| <ChevronDown className={`h-4 w-4 shrink-0 transition-transform ${showManualCodexCredentials ? "rotate-180" : ""}`} /> | ||
| </CollapsibleTrigger> | ||
| <CollapsibleContent className="data-[state=closed]:animate-collapse-up data-[state=open]:animate-collapse-down overflow-hidden"> | ||
| <div className="mt-4 space-y-4"> | ||
| <FormField | ||
| control={control} | ||
| name={`key.codex_key_config.refresh_token`} | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Refresh Token</FormLabel> | ||
| <FormControl> | ||
| <EnvVarInput placeholder="Refresh token or env.CODEX_REFRESH_TOKEN" type="text" {...field} /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={control} | ||
| name={`key.codex_key_config.access_token`} | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Cached Access Token</FormLabel> | ||
| <FormControl> | ||
| <EnvVarInput placeholder="Optional access token or env.CODEX_ACCESS_TOKEN" type="text" {...field} /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <div className="grid gap-4 md:grid-cols-2"> | ||
| <FormField | ||
| control={control} | ||
| name={`key.codex_key_config.account_id`} | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>ChatGPT Account ID</FormLabel> | ||
| <FormControl> | ||
| <EnvVarInput placeholder="Optional account id or env.CODEX_ACCOUNT_ID" type="text" {...field} /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={control} | ||
| name={`key.codex_key_config.access_token_expires_at`} | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Access Token Expiry</FormLabel> | ||
| <FormControl> | ||
| <Input placeholder="2026-04-14T12:00:00Z" type="text" {...field} value={field.value || ""} /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Add stable selectors for the new Codex manual-auth controls.
The new trigger and credential inputs are all interactive, but none expose stable selectors like the sibling apikey-* controls already in this file. That will make the Codex setup flow harder to cover in e2e/regression tests.
Examples here would be enough: apikey-codex-manual-credentials-trigger, apikey-codex-refresh-token-input, apikey-codex-access-token-input, apikey-codex-account-id-input, and apikey-codex-access-token-expiry-input.
As per coding guidelines, ui/**/*.{tsx,ts} should add new interactive UI elements with data-testid attributes following data-testid="<entity>-<element>-<qualifier>".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines
209 - 274, Add stable data-testid attributes to the new Codex manual-auth UI
elements so e2e tests can target them: add
data-testid="apikey-codex-manual-credentials-trigger" to the CollapsibleTrigger,
data-testid="apikey-codex-refresh-token-input" to the EnvVarInput rendering
`key.codex_key_config.refresh_token`,
data-testid="apikey-codex-access-token-input" to the EnvVarInput rendering
`key.codex_key_config.access_token`, data-testid="apikey-codex-account-id-input"
to the EnvVarInput rendering `key.codex_key_config.account_id`, and
data-testid="apikey-codex-access-token-expiry-input" to the Input rendering
`key.codex_key_config.access_token_expires_at`; ensure attributes are placed on
the interactive component instances (CollapsibleTrigger, EnvVarInput, Input)
rather than wrappers to match existing `apikey-*` selectors convention.
Author
|
@akshaydeo regrading a video Yaron.Shani_s.Video.-.Apr.14.2026.mp4 |
Contributor
|
I actually skimmed through the PR - there are too many redflags at the moment including a static OAuth client ID etc. Ill review it by end of the week once Greptile and CR are done |
Author
@akshaydeo thanks, fully understand. The app client id was taken from opencode. It also being used in other repos, but its indeed red flag. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
framework/configstore/migrations.go (1)
280-287:⚠️ Potential issue | 🔴 CriticalAdd a forward-only Codex config-hash backfill here.
Threading Codex into the old
add_config_hash_column/add_store_raw_request_response_columnmigrations is still unsafe. Databases that already applied those IDs will never recomputeconfig_hash, and databases upgrading from a pre-Codex release can hit the historical hash migration beforecodex_config_jsonexists. Keep the old migrations pre-Codex and add a new idempotent backfill immediately aftermigrationAddCodexConfigColumns.Suggested follow-up
if err := migrationAddCodexConfigColumns(ctx, db); err != nil { return err } +if err := migrationBackfillCodexProviderConfigHash(ctx, db); err != nil { + return err +} if err := migrationAddCodexAuthSessionsTable(ctx, db); err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 280 - 287, Add an idempotent, forward-only backfill immediately after calling migrationAddCodexConfigColumns: implement a new function (e.g., migrationBackfillCodexConfigHash) that finds records where codex_config_json IS NOT NULL and config_hash IS NULL, computes and sets the config_hash only for those rows (skip rows that already have config_hash), and do not implement a down/rollback path; then invoke migrationBackfillCodexConfigHash(ctx, db) right after migrationAddCodexConfigColumns and before migrationAddCodexAuthSessionsTable/migrationCleanupCodexBrowserAuthColumns to ensure safe incremental upgrades.ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
198-278:⚠️ Potential issue | 🟡 MinorAdd stable selectors to the new Codex manual-auth controls.
The new
CollapsibleTriggerand credential inputs are interactive, but they still don't expose the repo's requireddata-testidselectors. That will make the Codex setup flow harder to cover in e2e/regression tests. Examples here would be enough:apikey-codex-manual-credentials-trigger,apikey-codex-refresh-token-input,apikey-codex-access-token-input,apikey-codex-account-id-input, andapikey-codex-access-token-expiry-input.As per coding guidelines,
ui/**/*.{tsx,ts}should add new interactive UI elements withdata-testidattributes followingdata-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 198 - 278, The CollapsibleTrigger and the manual Codex input controls lack stable test selectors; add data-testid attributes to CollapsibleTrigger (e.g., data-testid="apikey-codex-manual-credentials-trigger"), the refresh token EnvVarInput (apikey-codex-refresh-token-input), the cached access token EnvVarInput (apikey-codex-access-token-input), the account id EnvVarInput (apikey-codex-account-id-input), and the access token expiry Input (apikey-codex-access-token-expiry-input); update the JSX for the CollapsibleTrigger, the EnvVarInput instances used in the FormField names key.codex_key_config.refresh_token, key.codex_key_config.access_token, key.codex_key_config.account_id, and the Input used for key.codex_key_config.access_token_expires_at to include these data-testid attributes following the project pattern.framework/configstore/tables/codexauth.go (1)
36-46:⚠️ Potential issue | 🟠 MajorAlways mark the row as encrypted when encryption is enabled.
Rows without
DeviceAuthIDcurrently keep the default"plain_text"status, so the startup encryption pass will keep treating them as unprocessed. Sets.EncryptionStatus = EncryptionStatusEncryptedwheneverencrypt.IsEnabled()is true, regardless of whether a specific field was encrypted.🛠️ Proposed fix
if encrypt.IsEnabled() { - encrypted := false + s.EncryptionStatus = EncryptionStatusEncrypted if s.DeviceAuthID != nil && *s.DeviceAuthID != "" { if err := encryptString(s.DeviceAuthID); err != nil { return fmt.Errorf("failed to encrypt codex device auth id: %w", err) } - encrypted = true - } - if encrypted { - s.EncryptionStatus = EncryptionStatusEncrypted } }Based on learnings, ensure
EncryptionStatusis set to"encrypted"whenever encryption is enabled, even if no fields are actually encrypted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/codexauth.go` around lines 36 - 46, The encryption status is only set to EncryptionStatusEncrypted when a field was actually encrypted, leaving rows with no DeviceAuthID marked as plain_text; modify the logic in the codexauth save/prepare code (around encrypt.IsEnabled(), s.DeviceAuthID, encryptString) so that whenever encrypt.IsEnabled() returns true you unconditionally set s.EncryptionStatus = EncryptionStatusEncrypted (move or add the assignment outside the conditional that checks the encrypted boolean) so rows are marked "encrypted" even if no specific field was changed.transports/bifrost-http/server/server.go (2)
1161-1167:⚠️ Potential issue | 🔴 CriticalDon't persist refreshed credentials on an unbounded background context.
This runs on the request path after token refresh. Using
context.Background()withschemas.NoDeadlinemeans a slow config store/DB can stall request completion indefinitely. Thread the caller context through, or derive a short timeout and cancel it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/server/server.go` around lines 1161 - 1167, The persistCodexCredentialRefresh function uses schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) which creates an unbounded background context and can hang request handling; change it to accept and use the caller context (e.g., add ctx context.Context parameter to persistCodexCredentialRefresh and pass that into schemas.NewBifrostContext) or, if signature change is not possible, derive a bounded context via context.WithTimeout and cancel it, then call s.Config.PersistCodexKeyCredentials with that bounded context; update all callers of persistCodexCredentialRefresh accordingly and keep references to schemas.NewBifrostContext and s.Config.PersistCodexKeyCredentials intact.
1142-1149:⚠️ Potential issue | 🔴 CriticalInject the credential persister before
TransportInterceptorMiddleware.This middleware is appended into
commonMiddlewares, but the inference chain later prependshandlers.TransportInterceptorMiddleware(s.Config)ahead of that slice.TransportInterceptorMiddlewarematerializes the*schemas.BifrostContextfirst, so refreshed Codex tokens never seeschemas.BifrostContextKeyCodexCredentialPersister. Move this setup ahead of the interceptor in the inference middleware order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/server/server.go` around lines 1142 - 1149, The credential persister middleware (setting schemas.BifrostContextKeyCodexCredentialPersister with schemas.CodexCredentialPersister(s.persistCodexCredentialRefresh)) is appended to commonMiddlewares after which handlers.TransportInterceptorMiddleware(s.Config) is later prepended, so the interceptor runs before the persister; change the order so the persister middleware is inserted/Prepended before TransportInterceptorMiddleware (i.e., ensure the middleware that sets the CodexCredentialPersister on the request context runs earlier than handlers.TransportInterceptorMiddleware by moving its addition ahead of where TransportInterceptorMiddleware is placed or by prepending it to the middleware slice).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/provider.go`:
- Around line 504-514: The CodexConfig currently allows any non-empty string for
PricingMode which leads to silent zero-rating; add explicit validation to reject
unknown values by ensuring CodexPricingMode only accepts the defined constants
(CodexPricingModeIncludedZero and CodexPricingModeOpenAIEquivalent). Implement
an IsValid() method on CodexPricingMode or a custom UnmarshalJSON/UnmarshalText
for CodexPricingMode that returns an error for unknown values, and call/trigger
that validation when decoding/constructing CodexConfig (or add a Validate() on
CodexConfig) so invalid pricing_mode values are rejected rather than treated as
zero-priced; update any config-loading paths to surface the error.
In `@transports/bifrost-http/handlers/codexauth.go`:
- Around line 243-257: The helper functions completeSession and failSession
currently swallow UpdateCodexAuthSession errors and use context.Background();
change their signatures to accept a context.Context (e.g., completeSession(ctx
context.Context, session *configstoreTables.TableCodexAuthSession) error and
failSession(ctx context.Context, session
*configstoreTables.TableCodexAuthSession, message string) error), update the
session fields the same way but call
h.store.ConfigStore.UpdateCodexAuthSession(ctx, session) and return any error
from that call instead of discarding it, and then update the caller
refreshSessionState (and any other callers) to pass down the incoming context
and handle/return the error so persistence failures bubble up instead of being
best-effort.
- Around line 90-126: The getAuthSessionStatus and cancelAuthSession handlers
currently dereference h.store.ConfigStore and can panic when the server runs
without DB-backed config storage; add the same guard used by
getEditableCodexKey/startDeviceAuth: check that h.store != nil and
h.store.ConfigStore != nil at the start of both getAuthSessionStatus and
cancelAuthSession and if missing return SendError(ctx,
fasthttp.StatusServiceUnavailable, "Config store unavailable") (or equivalent
503 message) before any calls to h.store.ConfigStore or refreshSessionState;
ensure both functions return immediately after sending the 503.
- Around line 43-46: RegisterRoutes is missing the browser-auth route used by
the frontend; add a POST route for
"/api/providers/codex/keys/{keyId}/auth/browser/start" in
CodexAuthHandler.RegisterRoutes and wire it to the appropriate handler (e.g.,
h.startBrowserAuth) using lib.ChainMiddlewares like the other routes; if
startBrowserAuth does not exist, implement a handler function with the same
signature/behavior pattern as startDeviceAuth (validate keyId, start the browser
auth flow, return session info and errors).
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 370-381: mergeKeys() currently preserves redacted secrets for
Azure/Vertex/Bedrock/VLLM but not for Codex, which causes device-auth fields to
be overwritten with redacted/empty UI values; update mergeKeys() to detect
redacted placeholders or empty values on incoming schemas.Key Codex fields and,
when detected, keep the existing stored values for CodexRefreshToken,
CodexAccessToken, CodexAccessTokenExpiresAt, CodexAccountID and CodexAuthMethod
instead of replacing them; ensure this logic mirrors the existing preservation
checks used for Azure/Vertex/Bedrock/VLLM and operates on the same key merge
path for provider keys (schemas.Key).
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx`:
- Around line 147-160: beginDeviceFlow currently can reject from
ensureKeyID("device") or startDeviceAuth(...).unwrap() and the caller ignores
the returned promise, causing unhandled rejections and no user feedback; wrap
the body of beginDeviceFlow in a try/catch, catch any errors from ensureKeyID or
startDeviceAuth, set a visible error via existing UI paths (e.g. call
setStatusMessage with a user-friendly message and error details or trigger the
toast/notification utility used in this component), and return/exit after
handling; keep the success path that calls setSession, setIsOpen and opens
popupRef unchanged so valid flows are unaffected.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 280-287: Add an idempotent, forward-only backfill immediately
after calling migrationAddCodexConfigColumns: implement a new function (e.g.,
migrationBackfillCodexConfigHash) that finds records where codex_config_json IS
NOT NULL and config_hash IS NULL, computes and sets the config_hash only for
those rows (skip rows that already have config_hash), and do not implement a
down/rollback path; then invoke migrationBackfillCodexConfigHash(ctx, db) right
after migrationAddCodexConfigColumns and before
migrationAddCodexAuthSessionsTable/migrationCleanupCodexBrowserAuthColumns to
ensure safe incremental upgrades.
In `@framework/configstore/tables/codexauth.go`:
- Around line 36-46: The encryption status is only set to
EncryptionStatusEncrypted when a field was actually encrypted, leaving rows with
no DeviceAuthID marked as plain_text; modify the logic in the codexauth
save/prepare code (around encrypt.IsEnabled(), s.DeviceAuthID, encryptString) so
that whenever encrypt.IsEnabled() returns true you unconditionally set
s.EncryptionStatus = EncryptionStatusEncrypted (move or add the assignment
outside the conditional that checks the encrypted boolean) so rows are marked
"encrypted" even if no specific field was changed.
In `@transports/bifrost-http/server/server.go`:
- Around line 1161-1167: The persistCodexCredentialRefresh function uses
schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) which
creates an unbounded background context and can hang request handling; change it
to accept and use the caller context (e.g., add ctx context.Context parameter to
persistCodexCredentialRefresh and pass that into schemas.NewBifrostContext) or,
if signature change is not possible, derive a bounded context via
context.WithTimeout and cancel it, then call s.Config.PersistCodexKeyCredentials
with that bounded context; update all callers of persistCodexCredentialRefresh
accordingly and keep references to schemas.NewBifrostContext and
s.Config.PersistCodexKeyCredentials intact.
- Around line 1142-1149: The credential persister middleware (setting
schemas.BifrostContextKeyCodexCredentialPersister with
schemas.CodexCredentialPersister(s.persistCodexCredentialRefresh)) is appended
to commonMiddlewares after which
handlers.TransportInterceptorMiddleware(s.Config) is later prepended, so the
interceptor runs before the persister; change the order so the persister
middleware is inserted/Prepended before TransportInterceptorMiddleware (i.e.,
ensure the middleware that sets the CodexCredentialPersister on the request
context runs earlier than handlers.TransportInterceptorMiddleware by moving its
addition ahead of where TransportInterceptorMiddleware is placed or by
prepending it to the middleware slice).
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 198-278: The CollapsibleTrigger and the manual Codex input
controls lack stable test selectors; add data-testid attributes to
CollapsibleTrigger (e.g.,
data-testid="apikey-codex-manual-credentials-trigger"), the refresh token
EnvVarInput (apikey-codex-refresh-token-input), the cached access token
EnvVarInput (apikey-codex-access-token-input), the account id EnvVarInput
(apikey-codex-account-id-input), and the access token expiry Input
(apikey-codex-access-token-expiry-input); update the JSX for the
CollapsibleTrigger, the EnvVarInput instances used in the FormField names
key.codex_key_config.refresh_token, key.codex_key_config.access_token,
key.codex_key_config.account_id, and the Input used for
key.codex_key_config.access_token_expires_at to include these data-testid
attributes following the project pattern.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c084bbef-1bbd-4920-abd2-9c090813f74a
⛔ Files ignored due to path filters (3)
docs/media/ui-codex-screenshot-1.pngis excluded by!**/*.pngdocs/media/ui-codex-screenshot-2.pngis excluded by!**/*.pngdocs/media/ui-codex-screenshot-3.pngis excluded by!**/*.png
📒 Files selected for processing (44)
core/bifrost.gocore/providers/codex/auth.gocore/providers/codex/auth_test.gocore/providers/codex/codex.gocore/providers/codex/codex_test.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/schemas/account.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/provider.gocore/utils.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/codexauth.goframework/configstore/tables/key.goframework/configstore/tables/provider.goframework/modelcatalog/codex.goframework/modelcatalog/codex_test.goframework/modelcatalog/main.goframework/modelcatalog/main_test.goframework/modelcatalog/pricing.goplugins/maxim/plugin_test.gotransports/bifrost-http/handlers/codexauth.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/providerConfigSheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/codexAuthControls.tsxui/app/workspace/providers/fragments/codexConfigFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/providerKeyForm.tsxui/lib/constants/config.tsui/lib/constants/icons.tsxui/lib/constants/logs.tsui/lib/store/apis/providersApi.tsui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (10)
- ui/lib/constants/config.ts
- transports/bifrost-http/lib/ctx.go
- core/providers/codex/auth_test.go
- plugins/maxim/plugin_test.go
- ui/app/workspace/providers/fragments/index.ts
- framework/modelcatalog/main.go
- core/providers/openai/openai.go
- framework/modelcatalog/codex_test.go
- core/providers/codex/codex_test.go
- framework/modelcatalog/main_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- ui/app/workspace/providers/dialogs/providerConfigSheet.tsx
- ui/lib/constants/logs.ts
- core/schemas/context.go
- framework/configstore/store.go
- ui/app/workspace/providers/fragments/codexConfigFormFragment.tsx
- core/bifrost.go
- framework/configstore/clientconfig.go
- framework/configstore/rdb.go
- transports/config.schema.json
- framework/configstore/tables/key.go
- transports/bifrost-http/lib/config_test.go
- ui/lib/types/schemas.ts
- core/providers/mistral/mistral.go
- core/providers/codex/auth.go
- core/schemas/account.go
- core/providers/codex/codex.go
👮 Files not reviewed due to content moderation or server errors (1)
- ui/lib/constants/icons.tsx
Comment on lines
+504
to
+514
| type CodexPricingMode string | ||
|
|
||
| const ( | ||
| CodexPricingModeIncludedZero CodexPricingMode = "included_zero" | ||
| CodexPricingModeOpenAIEquivalent CodexPricingMode = "openai_equivalent" | ||
| ) | ||
|
|
||
| // CodexConfig holds provider-level settings for the ChatGPT Plus/Pro-backed Codex provider. | ||
| type CodexConfig struct { | ||
| PricingMode CodexPricingMode `json:"pricing_mode,omitempty"` | ||
| } |
Contributor
There was a problem hiding this comment.
Reject unknown pricing_mode values instead of silently zero-rating Codex.
Any non-empty string survives normalization here. Downstream, framework/modelcatalog/pricing.go only treats openai_equivalent specially; every other value falls back to zeroPricing(...). That means a typo like "openai-equivalant" will underbill all Codex traffic instead of failing fast. Please validate against the known enum values before this config is accepted.
Also applies to: 551-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/schemas/provider.go` around lines 504 - 514, The CodexConfig currently
allows any non-empty string for PricingMode which leads to silent zero-rating;
add explicit validation to reject unknown values by ensuring CodexPricingMode
only accepts the defined constants (CodexPricingModeIncludedZero and
CodexPricingModeOpenAIEquivalent). Implement an IsValid() method on
CodexPricingMode or a custom UnmarshalJSON/UnmarshalText for CodexPricingMode
that returns an error for unknown values, and call/trigger that validation when
decoding/constructing CodexConfig (or add a Validate() on CodexConfig) so
invalid pricing_mode values are rejected rather than treated as zero-priced;
update any config-loading paths to surface the error.
Comment on lines
+43
to
+46
| func (h *CodexAuthHandler) RegisterRoutes(r *router.Router, middlewares ...schemas.BifrostHTTPMiddleware) { | ||
| r.POST("/api/providers/codex/keys/{keyId}/auth/device/start", lib.ChainMiddlewares(h.startDeviceAuth, middlewares...)) | ||
| r.GET("/api/providers/codex/auth/sessions/{id}", lib.ChainMiddlewares(h.getAuthSessionStatus, middlewares...)) | ||
| r.DELETE("/api/providers/codex/auth/sessions/{id}", lib.ChainMiddlewares(h.cancelAuthSession, middlewares...)) |
Contributor
There was a problem hiding this comment.
The browser-auth endpoint is missing from the handler.
ui/lib/store/apis/providersApi.ts now exposes startCodexBrowserAuth against /api/providers/codex/keys/{keyId}/auth/browser/start, but this handler only registers auth/device/start, session status, and cancel. Any browser-auth call will 404 until the matching route is added or the hook is removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/handlers/codexauth.go` around lines 43 - 46,
RegisterRoutes is missing the browser-auth route used by the frontend; add a
POST route for "/api/providers/codex/keys/{keyId}/auth/browser/start" in
CodexAuthHandler.RegisterRoutes and wire it to the appropriate handler (e.g.,
h.startBrowserAuth) using lib.ChainMiddlewares like the other routes; if
startBrowserAuth does not exist, implement a handler function with the same
signature/behavior pattern as startDeviceAuth (validate keyId, start the browser
auth flow, return session info and errors).
Comment on lines
+90
to
+126
| func (h *CodexAuthHandler) getAuthSessionStatus(ctx *fasthttp.RequestCtx) { | ||
| sessionID := ctx.UserValue("id").(string) | ||
| session, err := h.store.ConfigStore.GetCodexAuthSessionByID(ctx, sessionID) | ||
| if err != nil { | ||
| SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to get auth session: %v", err)) | ||
| return | ||
| } | ||
| if session == nil { | ||
| SendError(ctx, fasthttp.StatusNotFound, "Auth session not found") | ||
| return | ||
| } | ||
| if err := h.refreshSessionState(ctx, session); err != nil { | ||
| SendError(ctx, fasthttp.StatusBadGateway, fmt.Sprintf("Failed to refresh auth session: %v", err)) | ||
| return | ||
| } | ||
| SendJSON(ctx, h.sessionResponse(session)) | ||
| } | ||
|
|
||
| func (h *CodexAuthHandler) cancelAuthSession(ctx *fasthttp.RequestCtx) { | ||
| sessionID := ctx.UserValue("id").(string) | ||
| session, err := h.store.ConfigStore.GetCodexAuthSessionByID(ctx, sessionID) | ||
| if err != nil { | ||
| SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to get auth session: %v", err)) | ||
| return | ||
| } | ||
| if session == nil { | ||
| SendError(ctx, fasthttp.StatusNotFound, "Auth session not found") | ||
| return | ||
| } | ||
| session.Status = codexAuthSessionCancelled | ||
| now := time.Now() | ||
| session.CompletedAt = &now | ||
| if err := h.store.ConfigStore.UpdateCodexAuthSession(ctx, session); err != nil { | ||
| SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to cancel auth session: %v", err)) | ||
| return | ||
| } | ||
| SendJSON(ctx, h.sessionResponse(session)) |
Contributor
There was a problem hiding this comment.
Guard session GET/DELETE when DB-backed config storage is unavailable.
startDeviceAuth() routes through getEditableCodexKey() and returns a clean error when h.store/h.store.ConfigStore is missing, but getAuthSessionStatus() and cancelAuthSession() dereference h.store.ConfigStore directly. Since server.RegisterAPIRoutes() always registers this handler, these endpoints can panic in non-DB deployments instead of returning 503.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/handlers/codexauth.go` around lines 90 - 126, The
getAuthSessionStatus and cancelAuthSession handlers currently dereference
h.store.ConfigStore and can panic when the server runs without DB-backed config
storage; add the same guard used by getEditableCodexKey/startDeviceAuth: check
that h.store != nil and h.store.ConfigStore != nil at the start of both
getAuthSessionStatus and cancelAuthSession and if missing return SendError(ctx,
fasthttp.StatusServiceUnavailable, "Config store unavailable") (or equivalent
503 message) before any calls to h.store.ConfigStore or refreshSessionState;
ensure both functions return immediately after sending the 503.
Comment on lines
+243
to
+257
| func (h *CodexAuthHandler) completeSession(session *configstoreTables.TableCodexAuthSession) { | ||
| now := time.Now() | ||
| session.Status = codexAuthSessionSucceeded | ||
| session.CompletedAt = &now | ||
| session.LastError = nil | ||
| _ = h.store.ConfigStore.UpdateCodexAuthSession(context.Background(), session) | ||
| } | ||
|
|
||
| func (h *CodexAuthHandler) failSession(session *configstoreTables.TableCodexAuthSession, message string) { | ||
| now := time.Now() | ||
| session.Status = codexAuthSessionFailed | ||
| session.CompletedAt = &now | ||
| session.LastError = &message | ||
| _ = h.store.ConfigStore.UpdateCodexAuthSession(context.Background(), session) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't make terminal session persistence best-effort.
After tokens are written, completeSession() updates the DB with context.Background() and discards the error. If that write fails, the current poll returns authorized from memory while the stored session stays pending, so later polls can retry the exchange or show stale state. Persist the terminal state with the caller context and bubble failures back to refreshSessionState().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/handlers/codexauth.go` around lines 243 - 257, The
helper functions completeSession and failSession currently swallow
UpdateCodexAuthSession errors and use context.Background(); change their
signatures to accept a context.Context (e.g., completeSession(ctx
context.Context, session *configstoreTables.TableCodexAuthSession) error and
failSession(ctx context.Context, session
*configstoreTables.TableCodexAuthSession, message string) error), update the
session fields the same way but call
h.store.ConfigStore.UpdateCodexAuthSession(ctx, session) and return any error
from that call instead of discarding it, and then update the caller
refreshSessionState (and any other callers) to pass down the incoming context
and handle/return the error so persistence failures bubble up instead of being
best-effort.
Comment on lines
370
to
+381
| var payload = struct { | ||
| Keys []schemas.Key `json:"keys"` // API keys for the provider | ||
| NetworkConfig schemas.NetworkConfig `json:"network_config"` // Network-related settings | ||
| ConcurrencyAndBufferSize schemas.ConcurrencyAndBufferSize `json:"concurrency_and_buffer_size"` // Concurrency settings | ||
| ProxyConfig *schemas.ProxyConfig `json:"proxy_config,omitempty"` // Proxy configuration | ||
| SendBackRawRequest *bool `json:"send_back_raw_request,omitempty"` // Include raw request in BifrostResponse | ||
| SendBackRawResponse *bool `json:"send_back_raw_response,omitempty"` // Include raw response in BifrostResponse | ||
| Keys []schemas.Key `json:"keys"` // API keys for the provider | ||
| NetworkConfig schemas.NetworkConfig `json:"network_config"` // Network-related settings | ||
| ConcurrencyAndBufferSize schemas.ConcurrencyAndBufferSize `json:"concurrency_and_buffer_size"` // Concurrency settings | ||
| ProxyConfig *schemas.ProxyConfig `json:"proxy_config,omitempty"` // Proxy configuration | ||
| SendBackRawRequest *bool `json:"send_back_raw_request,omitempty"` // Include raw request in BifrostResponse | ||
| SendBackRawResponse *bool `json:"send_back_raw_response,omitempty"` // Include raw response in BifrostResponse | ||
| StoreRawRequestResponse *bool `json:"store_raw_request_response,omitempty"` // Capture raw request/response for internal logging only | ||
| CustomProviderConfig *schemas.CustomProviderConfig `json:"custom_provider_config,omitempty"` // Custom provider configuration | ||
| OpenAIConfig *schemas.OpenAIConfig `json:"openai_config,omitempty"` // OpenAI-specific configuration | ||
| PricingOverrides []schemas.ProviderPricingOverride `json:"pricing_overrides,omitempty"` // Provider-level pricing overrides | ||
| CustomProviderConfig *schemas.CustomProviderConfig `json:"custom_provider_config,omitempty"` // Custom provider configuration | ||
| OpenAIConfig *schemas.OpenAIConfig `json:"openai_config,omitempty"` // OpenAI-specific configuration | ||
| CodexConfig *schemas.CodexConfig `json:"codex_config,omitempty"` // Codex-specific configuration | ||
| PricingOverrides []schemas.ProviderPricingOverride `json:"pricing_overrides,omitempty"` // Provider-level pricing overrides |
Contributor
There was a problem hiding this comment.
Preserve redacted Codex key credentials during updates.
This path now round-trips Codex-managed keys through mergeKeys(), but mergeKeys() still only restores redacted secrets for Azure/Vertex/Bedrock/VLLM. A normal edit/save of a connected Codex key will overwrite the stored device-auth fields (CodexRefreshToken, CodexAccessToken, CodexAccessTokenExpiresAt, CodexAccountID, CodexAuthMethod) with redacted placeholders or empties from the UI, breaking subsequent requests. Please add the same redaction-preservation logic for the Codex fields before shipping.
Also applies to: 517-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/bifrost-http/handlers/providers.go` around lines 370 - 381,
mergeKeys() currently preserves redacted secrets for Azure/Vertex/Bedrock/VLLM
but not for Codex, which causes device-auth fields to be overwritten with
redacted/empty UI values; update mergeKeys() to detect redacted placeholders or
empty values on incoming schemas.Key Codex fields and, when detected, keep the
existing stored values for CodexRefreshToken, CodexAccessToken,
CodexAccessTokenExpiresAt, CodexAccountID and CodexAuthMethod instead of
replacing them; ensure this logic mirrors the existing preservation checks used
for Azure/Vertex/Bedrock/VLLM and operates on the same key merge path for
provider keys (schemas.Key).
Comment on lines
+147
to
+160
| const beginDeviceFlow = async () => { | ||
| setStatusMessage(null); | ||
| const resolvedKeyID = await ensureKeyID("device"); | ||
| const nextSession = await startDeviceAuth(resolvedKeyID).unwrap(); | ||
| setSession(nextSession); | ||
| setIsOpen(true); | ||
| if (nextSession.verification_uri) { | ||
| popupRef.current = window.open( | ||
| nextSession.verification_uri, | ||
| "codex_device_auth", | ||
| "width=640,height=760,resizable=yes,scrollbars=yes", | ||
| ); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle device-flow startup failures instead of dropping the promise.
beginDeviceFlow() can reject when draft persistence fails (for example, missing key name) or when startDeviceAuth(...).unwrap() errors. The caller does void beginDeviceFlow(), so those failures turn into unhandled promises and the user gets no feedback. Catch the error here and surface it via toast or an inline alert before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx` around lines 147
- 160, beginDeviceFlow currently can reject from ensureKeyID("device") or
startDeviceAuth(...).unwrap() and the caller ignores the returned promise,
causing unhandled rejections and no user feedback; wrap the body of
beginDeviceFlow in a try/catch, catch any errors from ensureKeyID or
startDeviceAuth, set a visible error via existing UI paths (e.g. call
setStatusMessage with a user-friendly message and error details or trigger the
toast/notification utility used in this component), and return/exit after
handling; keep the success path that calls setSession, setIsOpen and opens
popupRef unchanged so valid flows are unaffected.
Author
|
I will go over the comments and try to fix those best I can. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
transports/bifrost-http/lib/config_test.go (1)
447-461: KeepMockConfigStore.PersistCodexKeyConfigminimal in this test double.This new in-memory mutation logic makes the mock behaviorful and can diverge from real store semantics. Prefer keeping this method as a simple stub here and validating Codex key persistence via SQLite-backed integration tests (
createTestSQLiteConfigStorepath).Based on learnings: "In tests under transports/bifrost-http/lib/config_test.go, keep MockConfigStore methods (e.g., GetVirtualKeysPaginated) as simple, returning zero/nil values. Do not embed business logic (filtering/pagination) in the mock. Cover behavior with SQLite-backed integration tests using createTestSQLiteConfigStore instead to validate end-to-end behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config_test.go` around lines 447 - 461, Remove the in-memory mutation/filtering logic inside MockConfigStore.PersistCodexKeyConfig and make it a minimal stub: do not read or write m.providers, do not attempt to find keys, and simply return a zero/nil result (e.g., return nil) so the mock stays behaviorless; rely on the SQLite-backed integration tests via createTestSQLiteConfigStore to validate actual persistence semantics.framework/configstore/rdb.go (1)
871-889: Prefer column-scoped update for Codex credential persistenceCurrent flow does
SELECTthenSaveof the full row. A targeted update (Model(...).Where(...).Update(...)) is safer and avoids unintentionally writing unrelated stale fields.♻️ Proposed refactor
func (s *RDBConfigStore) PersistCodexKeyConfig(ctx context.Context, keyID string, keyConfig *schemas.CodexKeyConfig) error { if keyConfig == nil { return nil } - var dbKey tables.TableKey - if err := s.db.WithContext(ctx).Where("provider = ? AND key_id = ?", string(schemas.Codex), keyID).First(&dbKey).Error; err != nil { - if errors.Is(err, gorm.ErrRecordNotFound) { - return ErrNotFound - } - return fmt.Errorf("failed to load codex key for persistence: %w", err) - } - - dbKey.CodexKeyConfig = keyConfig - if err := s.db.WithContext(ctx).Save(&dbKey).Error; err != nil { - return fmt.Errorf("failed to persist codex key config: %w", err) - } + result := s.db.WithContext(ctx). + Model(&tables.TableKey{}). + Where("provider = ? AND key_id = ?", string(schemas.Codex), keyID). + Update("codex_key_config", keyConfig) + if result.Error != nil { + return fmt.Errorf("failed to persist codex key config: %w", result.Error) + } + if result.RowsAffected == 0 { + return ErrNotFound + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 871 - 889, The PersistCodexKeyConfig method in RDBConfigStore currently selects the full tables.TableKey row then calls Save, which can overwrite unrelated fields; change it to perform a column-scoped update instead: locate PersistCodexKeyConfig and replace the SELECT+Save flow with a targeted update using s.db.WithContext(ctx).Model(&tables.TableKey{}).Where("provider = ? AND key_id = ?", string(schemas.Codex), keyID).Update("codex_key_config", keyConfig) (or Updates if necessary) and return ErrNotFound if the initial lookup should detect missing rows, ensuring only the CodexKeyConfig column is modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/rdb.go`:
- Around line 302-303: The code copies key.CodexKeyConfig directly into the
persisted record causing existing Codex credentials to be wiped when an update
payload omits them; update the save/update paths (where Save is called for
existing keys) to first load the existing record (using the same
store/repository methods already present), and if key.CodexKeyConfig is nil,
preserve the existingRecord.CodexKeyConfig instead of overwriting it; apply this
pattern for all update branches that set CodexKeyConfig (references:
CodexKeyConfig field assignment and the Save calls used for existing-key
updates).
---
Nitpick comments:
In `@framework/configstore/rdb.go`:
- Around line 871-889: The PersistCodexKeyConfig method in RDBConfigStore
currently selects the full tables.TableKey row then calls Save, which can
overwrite unrelated fields; change it to perform a column-scoped update instead:
locate PersistCodexKeyConfig and replace the SELECT+Save flow with a targeted
update using s.db.WithContext(ctx).Model(&tables.TableKey{}).Where("provider = ?
AND key_id = ?", string(schemas.Codex), keyID).Update("codex_key_config",
keyConfig) (or Updates if necessary) and return ErrNotFound if the initial
lookup should detect missing rows, ensuring only the CodexKeyConfig column is
modified.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 447-461: Remove the in-memory mutation/filtering logic inside
MockConfigStore.PersistCodexKeyConfig and make it a minimal stub: do not read or
write m.providers, do not attempt to find keys, and simply return a zero/nil
result (e.g., return nil) so the mock stays behaviorless; rely on the
SQLite-backed integration tests via createTestSQLiteConfigStore to validate
actual persistence semantics.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b293b5c5-bdc0-40a7-a6de-7629aa4e576b
📒 Files selected for processing (18)
core/providers/codex/codex.gocore/schemas/provider.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/modelcatalog/main.goframework/modelcatalog/main_test.goframework/modelcatalog/pricing.gotransports/bifrost-http/handlers/codexauth.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/providerConfigSheet.tsxui/app/workspace/providers/fragments/index.tsui/lib/types/config.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (4)
- framework/modelcatalog/main.go
- ui/app/workspace/providers/fragments/index.ts
- ui/app/workspace/providers/dialogs/providerConfigSheet.tsx
- core/schemas/provider.go
🚧 Files skipped from review as they are similar to previous changes (3)
- transports/bifrost-http/server/server.go
- transports/bifrost-http/handlers/codexauth.go
- core/providers/codex/codex.go
👮 Files not reviewed due to content moderation or server errors (4)
- framework/configstore/clientconfig.go
- transports/bifrost-http/lib/config.go
- ui/lib/types/config.ts
- framework/configstore/store.go
Comment on lines
+302
to
303
| CodexKeyConfig: key.CodexKeyConfig, | ||
| ConfigHash: keyHash, |
Contributor
There was a problem hiding this comment.
Prevent accidental Codex credential wipe during key updates
CodexKeyConfig is copied directly from incoming config, but existing-key paths later call Save (Line 361, Line 376, Line 522). If incoming config omits Codex credentials (nil), persisted DB credentials get overwritten and lost.
💡 Proposed fix
@@
if result.Error == nil {
// Update existing key with new data
dbKey.ID = existingKey.ID // Keep the same database ID
dbKey.ProviderID = existingKey.ProviderID // Preserve the existing ProviderID
dbKey.Enabled = existingKey.Enabled // Preserve the existing Enabled status
dbKey.Status = existingKey.Status // Preserve status (UI-managed)
dbKey.Description = existingKey.Description // Preserve description (UI-managed)
dbKey.EncryptionStatus = existingKey.EncryptionStatus // Preserve encryption status
+ if dbKey.CodexKeyConfig == nil {
+ dbKey.CodexKeyConfig = existingKey.CodexKeyConfig
+ }
if err := txDB.WithContext(ctx).Save(&dbKey).Error; err != nil {
return s.parseGormError(err)
}
@@
if result.Error == nil {
// Found by name - update existing key, preserve original KeyID
dbKey.ID = existingKey.ID // Keep the same database ID
dbKey.KeyID = existingKey.KeyID // Preserve original KeyID
dbKey.ProviderID = existingKey.ProviderID // Preserve the existing ProviderID
dbKey.Enabled = existingKey.Enabled // Preserve the existing Enabled status
dbKey.Status = existingKey.Status // Preserve status (UI-managed)
dbKey.Description = existingKey.Description // Preserve description (UI-managed)
dbKey.EncryptionStatus = existingKey.EncryptionStatus // Preserve encryption status
+ if dbKey.CodexKeyConfig == nil {
+ dbKey.CodexKeyConfig = existingKey.CodexKeyConfig
+ }
if err := txDB.WithContext(ctx).Save(&dbKey).Error; err != nil {
return s.parseGormError(err)
}
@@
if existingKey, exists := existingKeysMap[key.ID]; exists {
dbKey.ID = existingKey.ID // Keep the same database ID
dbKey.ConfigHash = existingKey.ConfigHash // Preserve config hash
dbKey.Status = existingKey.Status // Preserve status (UI-managed)
dbKey.Description = existingKey.Description // Preserve description (UI-managed)
dbKey.EncryptionStatus = existingKey.EncryptionStatus // Preserve encryption status
+ if dbKey.CodexKeyConfig == nil {
+ dbKey.CodexKeyConfig = existingKey.CodexKeyConfig
+ }
if err := txDB.WithContext(ctx).Save(&dbKey).Error; err != nil {
return s.parseGormError(err)
}Also applies to: 473-474
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/configstore/rdb.go` around lines 302 - 303, The code copies
key.CodexKeyConfig directly into the persisted record causing existing Codex
credentials to be wiped when an update payload omits them; update the
save/update paths (where Save is called for existing keys) to first load the
existing record (using the same store/repository methods already present), and
if key.CodexKeyConfig is nil, preserve the existingRecord.CodexKeyConfig instead
of overwriting it; apply this pattern for all update branches that set
CodexKeyConfig (references: CodexKeyConfig field assignment and the Save calls
used for existing-key updates).
Comment on lines
+529
to
+536
| func accessTokenExpired(expiresAt *string) bool { | ||
| if expiresAt == nil || strings.TrimSpace(*expiresAt) == "" { | ||
| return true | ||
| } | ||
| parsed, err := time.Parse(time.RFC3339, *expiresAt) | ||
| if err != nil { | ||
| return true | ||
| } |
Contributor
There was a problem hiding this comment.
accessTokenExpired returns true when AccessTokenExpiresAt is nil or empty. In the "manual" auth path a user may supply only an access_token with no expiry; the code treats the token as expired, falls through to the refresh-token branch, and if no refresh token is present it returns HTTP 401 — the manually-supplied access token is never used. The fix should skip the expiry guard when the auth method is manual or, at minimum, fall back to the access token when no refresh token is available:
func accessTokenExpired(expiresAt *string) bool {
if expiresAt == nil || strings.TrimSpace(*expiresAt) == "" {
return false // treat unknown expiry as still valid; caller should refresh on 401
}
parsed, err := time.Parse(time.RFC3339, *expiresAt)
if err != nil {
return true
}
return time.Now().After(parsed.Add(-30 * time.Second))
}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/app/workspace/providers/fragments/codexAuthControls.tsx (1)
147-160:⚠️ Potential issue | 🟠 MajorHandle device-flow startup failures instead of dropping the promise.
beginDeviceFlow()can reject whenensureKeyID()fails or whenstartDeviceAuth(...).unwrap()errors. The caller usesvoid beginDeviceFlow(), so failures become unhandled promise rejections with no user feedback.🛡️ Proposed fix to handle errors
const beginDeviceFlow = async () => { + try { setStatusMessage(null); const resolvedKeyID = await ensureKeyID("device"); const nextSession = await startDeviceAuth(resolvedKeyID).unwrap(); setSession(nextSession); setIsOpen(true); if (nextSession.verification_uri) { popupRef.current = window.open( nextSession.verification_uri, "codex_device_auth", "width=640,height=760,resizable=yes,scrollbars=yes", ); } + } catch (error) { + setStatusMessage(getErrorMessage(error)); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx` around lines 147 - 160, beginDeviceFlow currently lets rejections from ensureKeyID or startDeviceAuth(...).unwrap() bubble into unhandled promise rejections; wrap the function body in a try/catch, call ensureKeyID("device") and startDeviceAuth(resolvedKeyID).unwrap() inside the try, and on success keep the existing setSession, setIsOpen and popupRef.open logic; in the catch call setStatusMessage(error?.message || String(error)) and clean up state (close popupRef.current if opened, setSession(null) or setIsOpen(false) as appropriate) and optionally log the error; reference beginDeviceFlow, ensureKeyID, startDeviceAuth, setStatusMessage, setSession, setIsOpen, and popupRef when making these changes.
🧹 Nitpick comments (3)
docs/providers/supported-providers/overview.mdx (1)
11-11: LinkCoherehere for consistency with the other providers in the same sentence.It is underlined but not clickable, unlike Anthropic/Gemini/Bedrock.
Suggested doc tweak
-Bifrost can also act as a provider-compatible gateway (for example, <u>[Anthropic](../../integrations/anthropic-sdk/overview)</u>, <u>[Google Gemini](../../integrations/genai-sdk/overview)</u>, <u>Cohere</u>, <u>[Bedrock](../../integrations/bedrock-sdk/overview)</u>, and others), exposing provider-specific endpoints so you can use existing provider SDKs or integrations with no code changes, see [What is an integration?](../../integrations/what-is-an-integration) for details. +Bifrost can also act as a provider-compatible gateway (for example, <u>[Anthropic](../../integrations/anthropic-sdk/overview)</u>, <u>[Google Gemini](../../integrations/genai-sdk/overview)</u>, <u>[Cohere](./cohere)</u>, <u>[Bedrock](../../integrations/bedrock-sdk/overview)</u>, and others), exposing provider-specific endpoints so you can use existing provider SDKs or integrations with no code changes, see [What is an integration?](../../integrations/what-is-an-integration) for details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/providers/supported-providers/overview.mdx` at line 11, The sentence in docs/providers/supported-providers/overview.mdx lists providers but "Cohere" is underlined plain text instead of a clickable link; change the "Cohere" token in that sentence to use the same markdown link pattern as the others (e.g., replace Cohere with [Cohere](../../integrations/cohere-sdk/overview) or the correct Cohere integration path) so it becomes a clickable link consistent with "Anthropic", "Google Gemini", and "Bedrock".ui/app/workspace/providers/fragments/codexAuthControls.tsx (2)
49-49: Clean up popup window on component unmount.If the component unmounts while the auth dialog is open, the popup window remains open with no way to close it programmatically.
♻️ Suggested cleanup effect
const popupRef = useRef<Window | null>(null); + +useEffect(() => { + return () => { + if (popupRef.current && !popupRef.current.closed) { + popupRef.current.close(); + } + }; +}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx` at line 49, Add a cleanup effect to CodexAuthControls that closes the auth popup when the component unmounts: implement a useEffect in the codexAuthControls.tsx component that returns a cleanup function which checks popupRef.current, calls popupRef.current.close() if it's still open, and then nulls popupRef.current; also remove any related event listeners or polling tied to that popup inside the same cleanup so no orphaned handlers remain.
110-130: Consider using server-provided polling interval.The polling interval is hardcoded to 2000ms, but the backend returns
interval_secondsin the session response (see context snippet 1). Using the server-recommended interval respects rate limits and adapts to backend configuration changes.♻️ Suggested improvement
useEffect(() => { if (!session || session.status !== "pending") { return; } + const intervalMs = (session.interval_seconds ?? 2) * 1000; - const timer = window.setInterval(async () => { + const timer = window.setInterval(async () => { try { const nextSession = await getCodexAuthSession(session.id).unwrap(); setSession(nextSession); // ... } catch (error) { setStatusMessage(getErrorMessage(error)); } - }, 2000); + }, intervalMs); return () => window.clearInterval(timer); }, [getCodexAuthSession, session, syncFormFromProvider]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx` around lines 110 - 130, The effect currently hardcodes a 2000ms poll interval; update the polling in the useEffect that uses getCodexAuthSession/session to respect the server-provided interval_seconds from the session response: when fetching nextSession (and also on initial session) read nextSession.interval_seconds (or session.interval_seconds) and use that value (converted to ms) to set the window.setInterval delay, restarting/clearing the timer whenever the interval value or session.id changes; keep the same logic for setting setSession, setStatusMessage, syncFormFromProvider, and error handling (getErrorMessage) but replace the fixed 2000 with the server-provided value and fall back to a sensible default if interval_seconds is missing or invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx`:
- Around line 102-108: syncFormFromProvider currently uses the original keyId
prop which can be stale after ensureKeyID/onEnsurePersisted persists a new key;
instead, store the resolved key id returned by ensureKeyID (e.g., resolvedKeyID)
in component state and update it after persistence, then update
syncFormFromProvider to look up the key using that resolvedKeyID (referencing
functions getProvider and syncFormFromProvider) so it always finds the persisted
key; ensure the state is included in the syncFormFromProvider dependency array
and set when ensureKeyID/onEnsurePersisted completes.
- Around line 267-276: Add data-testid attributes to the interactive Buttons
inside DialogFooter: for the cancel button rendered when session?.status ===
"pending" (the Button that calls handleCancel) add a data-testid like
"codexauth-cancel-button" (or use entity-element-qualifier matching repo
conventions), and for the close button (the Button that calls handleCloseDialog)
add a data-testid like "codexauth-close-button"; update the JSX for those Button
elements in codexAuthControls.tsx (the Button calling handleCancel and the
Button calling handleCloseDialog) to include the corresponding data-testid
attributes following the <entity>-<element>-<qualifier> pattern.
---
Duplicate comments:
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx`:
- Around line 147-160: beginDeviceFlow currently lets rejections from
ensureKeyID or startDeviceAuth(...).unwrap() bubble into unhandled promise
rejections; wrap the function body in a try/catch, call ensureKeyID("device")
and startDeviceAuth(resolvedKeyID).unwrap() inside the try, and on success keep
the existing setSession, setIsOpen and popupRef.open logic; in the catch call
setStatusMessage(error?.message || String(error)) and clean up state (close
popupRef.current if opened, setSession(null) or setIsOpen(false) as appropriate)
and optionally log the error; reference beginDeviceFlow, ensureKeyID,
startDeviceAuth, setStatusMessage, setSession, setIsOpen, and popupRef when
making these changes.
---
Nitpick comments:
In `@docs/providers/supported-providers/overview.mdx`:
- Line 11: The sentence in docs/providers/supported-providers/overview.mdx lists
providers but "Cohere" is underlined plain text instead of a clickable link;
change the "Cohere" token in that sentence to use the same markdown link pattern
as the others (e.g., replace Cohere with
[Cohere](../../integrations/cohere-sdk/overview) or the correct Cohere
integration path) so it becomes a clickable link consistent with "Anthropic",
"Google Gemini", and "Bedrock".
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx`:
- Line 49: Add a cleanup effect to CodexAuthControls that closes the auth popup
when the component unmounts: implement a useEffect in the codexAuthControls.tsx
component that returns a cleanup function which checks popupRef.current, calls
popupRef.current.close() if it's still open, and then nulls popupRef.current;
also remove any related event listeners or polling tied to that popup inside the
same cleanup so no orphaned handlers remain.
- Around line 110-130: The effect currently hardcodes a 2000ms poll interval;
update the polling in the useEffect that uses getCodexAuthSession/session to
respect the server-provided interval_seconds from the session response: when
fetching nextSession (and also on initial session) read
nextSession.interval_seconds (or session.interval_seconds) and use that value
(converted to ms) to set the window.setInterval delay, restarting/clearing the
timer whenever the interval value or session.id changes; keep the same logic for
setting setSession, setStatusMessage, syncFormFromProvider, and error handling
(getErrorMessage) but replace the fixed 2000 with the server-provided value and
fall back to a sensible default if interval_seconds is missing or invalid.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf511e36-d2d2-4859-aa79-1f0feb216dec
📒 Files selected for processing (12)
core/schemas/account.godocs/docs.jsondocs/providers/supported-providers/codex.mdxdocs/providers/supported-providers/overview.mdxframework/configstore/migrations.goframework/modelcatalog/pricing.gotransports/config.schema.jsonui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/codexAuthControls.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- framework/modelcatalog/pricing.go
✅ Files skipped from review due to trivial changes (1)
- docs/providers/supported-providers/codex.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- core/schemas/account.go
- framework/configstore/migrations.go
- ui/app/workspace/providers/views/providerKeyForm.tsx
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
Comment on lines
+102
to
+108
| const syncFormFromProvider = useCallback(async () => { | ||
| const updatedProvider = await getProvider(providerName).unwrap(); | ||
| const updatedKey = updatedProvider.keys.find((key) => key.id === keyId); | ||
| if (updatedKey?.codex_key_config) { | ||
| form.setValue("key.codex_key_config", updatedKey.codex_key_config, { shouldDirty: false }); | ||
| } | ||
| }, [form, getProvider, keyId, providerName]); |
Contributor
There was a problem hiding this comment.
Potential stale keyId after persistence.
When syncFormFromProvider runs after device auth completes, it searches for the key using the original keyId prop. However, if onEnsurePersisted was called (for new keys), the actual persisted key may have a different ID. The resolvedKeyID from ensureKeyID should be used instead.
Consider storing the resolved key ID in state after ensureKeyID returns, then using that in syncFormFromProvider.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx` around lines 102
- 108, syncFormFromProvider currently uses the original keyId prop which can be
stale after ensureKeyID/onEnsurePersisted persists a new key; instead, store the
resolved key id returned by ensureKeyID (e.g., resolvedKeyID) in component state
and update it after persistence, then update syncFormFromProvider to look up the
key using that resolvedKeyID (referencing functions getProvider and
syncFormFromProvider) so it always finds the persisted key; ensure the state is
included in the syncFormFromProvider dependency array and set when
ensureKeyID/onEnsurePersisted completes.
Comment on lines
+267
to
+276
| <DialogFooter> | ||
| {session?.status === "pending" ? ( | ||
| <Button type="button" variant="outline" onClick={() => void handleCancel()}> | ||
| Cancel authorization | ||
| </Button> | ||
| ) : null} | ||
| <Button type="button" variant="outline" onClick={handleCloseDialog}> | ||
| Close | ||
| </Button> | ||
| </DialogFooter> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add missing data-testid attributes for interactive buttons.
Per coding guidelines, interactive UI elements should have data-testid attributes following the <entity>-<element>-<qualifier> pattern.
♻️ Proposed fix
{session?.status === "pending" ? (
- <Button type="button" variant="outline" onClick={() => void handleCancel()}>
+ <Button type="button" variant="outline" onClick={() => void handleCancel()} data-testid="codex-cancel-auth-btn">
Cancel authorization
</Button>
) : null}
- <Button type="button" variant="outline" onClick={handleCloseDialog}>
+ <Button type="button" variant="outline" onClick={handleCloseDialog} data-testid="codex-close-dialog-btn">
Close
</Button>As per coding guidelines: ui/**/*.{tsx,ts}: Add new interactive UI elements with data-testid attributes following the pattern: data-testid="<entity>-<element>-<qualifier>".
📝 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
| <DialogFooter> | |
| {session?.status === "pending" ? ( | |
| <Button type="button" variant="outline" onClick={() => void handleCancel()}> | |
| Cancel authorization | |
| </Button> | |
| ) : null} | |
| <Button type="button" variant="outline" onClick={handleCloseDialog}> | |
| Close | |
| </Button> | |
| </DialogFooter> | |
| <DialogFooter> | |
| {session?.status === "pending" ? ( | |
| <Button type="button" variant="outline" onClick={() => void handleCancel()} data-testid="codex-cancel-auth-btn"> | |
| Cancel authorization | |
| </Button> | |
| ) : null} | |
| <Button type="button" variant="outline" onClick={handleCloseDialog} data-testid="codex-close-dialog-btn"> | |
| Close | |
| </Button> | |
| </DialogFooter> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/workspace/providers/fragments/codexAuthControls.tsx` around lines 267
- 276, Add data-testid attributes to the interactive Buttons inside
DialogFooter: for the cancel button rendered when session?.status === "pending"
(the Button that calls handleCancel) add a data-testid like
"codexauth-cancel-button" (or use entity-element-qualifier matching repo
conventions), and for the close button (the Button that calls handleCloseDialog)
add a data-testid like "codexauth-close-button"; update the JSX for those Button
elements in codexAuthControls.tsx (the Button calling handleCancel and the
Button calling handleCloseDialog) to include the corresponding data-testid
attributes following the <entity>-<element>-<qualifier> pattern.
Comment on lines
+185
to
+215
| func (h *CodexAuthHandler) persistTokensToKey(ctx context.Context, keyID string, tokens *providerCodex.TokenResponse, authMethod schemas.CodexAuthMethod) error { | ||
| providerConfig, _, err := h.getEditableCodexKey(keyID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| updatedKeys := append([]schemas.Key(nil), providerConfig.Keys...) | ||
| for idx := range updatedKeys { | ||
| if updatedKeys[idx].ID != keyID { | ||
| continue | ||
| } | ||
| var existingAccountID *schemas.EnvVar | ||
| if updatedKeys[idx].CodexKeyConfig != nil { | ||
| existingAccountID = updatedKeys[idx].CodexKeyConfig.AccountID | ||
| } | ||
| refreshToken := schemas.NewEnvVar(tokens.RefreshToken) | ||
| accessToken := schemas.NewEnvVar(tokens.AccessToken) | ||
| accountIDValue := providerCodex.ExtractAccountID(tokens) | ||
| accessTokenExpiresAt := providerCodex.ExpiresAtFromNow(tokens.ExpiresIn) | ||
| updatedKeys[idx].CodexKeyConfig = &schemas.CodexKeyConfig{ | ||
| RefreshToken: *refreshToken, | ||
| AccessToken: accessToken, | ||
| AccessTokenExpiresAt: &accessTokenExpiresAt, | ||
| AuthMethod: authMethod, | ||
| } | ||
| if accountIDValue != "" { | ||
| updatedKeys[idx].CodexKeyConfig.AccountID = schemas.NewEnvVar(accountIDValue) | ||
| } else if existingAccountID != nil { | ||
| updatedKeys[idx].CodexKeyConfig.AccountID = existingAccountID | ||
| } | ||
| providerConfig.Keys = updatedKeys | ||
| return h.store.UpdateProviderConfig(ctx, schemas.Codex, *providerConfig) |
Contributor
There was a problem hiding this comment.
PersistCodexKeyCredentials
persistTokensToKey reads the full Codex provider config via getEditableCodexKey, modifies one key in the snapshot, and then calls UpdateProviderConfig. UpdateProvider (the DB write underneath) deletes all keys not present in the snapshot and re-inserts them. Any key added between the read on line 186 and the write on line 215 is permanently deleted from the database — the same race flagged in PersistCodexKeyCredentials.
The fix used by PersistCodexKeyCredentials applies here too: build the CodexKeyConfig, call h.store.ConfigStore.PersistCodexKeyConfig(ctx, keyID, codexKeyConfig) for the DB update, then update the in-memory cache with a targeted single-key write instead of a full provider rebuild.
Author
|
Its also worth mentioning that if you guys dont want this thing in your upstream, using projects like https://github.com/EvanZhouDev/openai-oauth as custom provider also works. I tested this project with bifrost custom provider, and it works. Maybe mentioning this as part of the docs might be an alternative is upstream support if not possible. |
22 tasks
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
3 participants
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
codexprovider with ChatGPT Plus/Pro device auth, DB-backed credential persistence, and a provider-specific UI flowRelated Issue
Validation
go test ./core/...go test ./framework/configstorego test ./framework/modelcataloggo test ./transports/bifrost-http/libgo test ./transports/bifrost-http/handlersgo test ./transports/bifrost-http/servergo test ./plugins/maximnpx tsc --noEmitScreenshots