feat: add provider keys HTTP handlers and refactor optional keys

[danpiths]

Summary

Adds HTTP handlers for the dedicated provider keys CRUD endpoints and removes
keys from provider API responses and payloads. Keys are now exclusively
managed via /api/providers/{provider}/keys/*. Also fixes a context timeout bug
in ReloadProvider where model discovery could exhaust the shared context
budget, causing subsequent DB calls to fail.

Changes

Provider keys handlers (provider_keys.go)

• New file with five handlers: listProviderKeys, getProviderKey,
  createProviderKey, updateProviderKey, deleteProviderKey
• Includes mergeUpdatedKey (redacted value preservation logic used by
  updateProviderKey)
• Key handlers enforce keyless provider validation and trigger model discovery
  after mutations

Provider handlers cleanup (providers.go)

• Registered new key routes: GET/POST /api/providers/{provider}/keys,
  GET/PUT/DELETE /api/providers/{provider}/keys/{key_id}
• Extracted inline anonymous structs into named providerCreatePayload and
  providerUpdatePayload types (without Keys field)
• Removed Keys field from ProviderResponse
• Switched addProvider from json.Unmarshal to sonic.Unmarshal
• Removed oldConfigRedacted fetch and the entire key merge block
  (mergeKeys, hasKeys, slices usage) from updateProvider
• Removed Keys from getProviderResponseFromConfig response builder
• Removed unused encoding/json import

Context timeout fix (server.go)

• Split shared bfCtx in ReloadProvider into separate contexts:
  filteredBfCtx (15s) for filtered ListModelsRequest and unfilteredBfCtx
  (fresh 15s) for unfiltered ListModelsRequest, each cancelled after use
• Changed GetKeysByProvider to use context.Background() since it's a local
  DB call that shouldn't be gated by model discovery timeouts
• Added hasNoKeys check to emit warn-level logs instead of errors when model
  discovery fails because no keys are configured
• Read in-memory key count via GetProviderKeysRaw for the hasNoKeys check

Tests (providers_test.go)

• Cleared file (contained only tests for removed inline struct decoding)

Type of change

• Feature
• Bug fix
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (Next.js)
• Docs

How to test

# Build
go build ./transports/bifrost-http/...

# Manual: start Bifrost, then test key CRUD
curl -X POST localhost:8080/api/providers/openai/keys -d '{"name":"test-key","value":"sk-test"}'
curl localhost:8080/api/providers/openai/keys
curl -X PUT localhost:8080/api/providers/openai/keys/{key_id} -d '{"name":"updated","value":"sk-new"}'
curl -X DELETE localhost:8080/api/providers/openai/keys/{key_id}

# Verify provider endpoints no longer return keys
curl localhost:8080/api/providers/openai | jq 'has("keys")'  # should be false

Screenshots/Recordings

N/A

Breaking changes

• Yes
• No

Provider API responses no longer include keys field. Provider create/update
payloads no longer accept keys. Clients must use the dedicated
/api/providers/{provider}/keys/* endpoints for key management.

Related issues

N/A

Security considerations

• Key handlers use existing redaction infrastructure (GetProviderKeyRedacted)
  before returning responses
• Keyless provider validation prevents key creation on providers that don't
  support keys

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[danpiths]

• feat: migrate frontend to dedicated provider keys API #2161
• feat: add provider keys HTTP handlers and refactor optional keys #2160 👈 (View in Graphite)
• feat: add provider keys CRUD to configstore and in-memory store #2159
• v1.5.0

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 06d9b625-9d72-4521-b6ec-d6404b863796

📥 Commits

Reviewing files that changed from the base of the PR and between 9b84e1e and 7897425.

📒 Files selected for processing (4)

• transports/bifrost-http/handlers/provider_keys.go
• transports/bifrost-http/handlers/providers.go
• transports/bifrost-http/handlers/providers_test.go
• transports/bifrost-http/server/server.go

---

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Added provider-scoped REST endpoints to list, retrieve, create, update, and delete provider keys.

• Refactor

  • Removed provider-level key handling from provider create/update payloads; keys are managed via dedicated endpoints.
  • Improved preservation of secret values during key updates so redacted fields retain original secrets when appropriate.
  • Model discovery now skips and logs when no keys are configured.

Walkthrough

Adds provider-scoped CRUD HTTP handlers for provider keys with redaction-aware merging; removes provider-level key handling from provider create/update flows; registers provider key routes under /api/providers/{provider}/keys; and updates server ReloadProvider to detect keyless/no-key providers and adjust/skip model discovery logging.

Changes

Cohort / File(s)	Summary
Provider Keys API transports/bifrost-http/handlers/provider_keys.go	New file implementing provider-scoped key handlers (list/get/create/update/delete), ListProviderKeysResponse, getKeyIDFromCtx helper, and mergeUpdatedKey logic; maps store errors to appropriate HTTP responses and triggers best-effort model discovery.
Provider handler updates transports/bifrost-http/handlers/providers.go	Removed provider-level keys from provider payloads/responses and deleted mergeKeys; added providerCreatePayload/providerUpdatePayload (no keys); switched provider create/update JSON parsing to sonic.Unmarshal; registered new provider key routes; minor control-flow cleanup.
Handler tests (placeholder) transports/bifrost-http/handlers/providers_test.go	Added an empty test source file declaring package handlers (no tests implemented).
Server: model discovery / reload transports/bifrost-http/server/server.go	ReloadProvider now computes hasNoKeys using raw provider keys plus keyless flags, delays creation of discovery context until after that check, and skips/changes model-discovery logging when no keys exist.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HTTP_Handler as Bifrost HTTP Handler
    participant ConfigStore
    participant ModelDisc as ModelDiscovery

    Client->>HTTP_Handler: POST /api/providers/{provider}/keys (key payload)
    HTTP_Handler->>ConfigStore: GetProviderConfigRaw(provider)
    alt provider is keyless
        HTTP_Handler-->>Client: 400 Bad Request
    else provider accepts keys
        HTTP_Handler->>ConfigStore: AddProviderKey(ctx, provider, key)
        alt AddProviderKey success
            ConfigStore->>HTTP_Handler: success
            HTTP_Handler->>ConfigStore: GetProviderKeyRedacted(provider, key_id)
            ConfigStore-->>HTTP_Handler: redacted key
            HTTP_Handler-->>Client: 201 Created (redacted key)
            HTTP_Handler->>ModelDisc: Trigger model discovery (best-effort, logged)
        else error (404/409/500)
            ConfigStore-->>HTTP_Handler: error
            HTTP_Handler-->>Client: 404/409/500
        end
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I nibble keys where routes now grow,
preserving secrets where redactions show,
I hop through reloads and skip the din,
if keys are none, model discovery stays thin,
a tiny hop, a satisfied nose 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: adding provider keys HTTP handlers and refactoring optional keys support.
Description check	✅ Passed	The description is comprehensive, covering all major sections of the template with detailed information about changes, testing, breaking changes, and security considerations.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/03-18-feat_add_provider_keys_http_handlers_and_refactor_optional_keys

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

transports/bifrost-http/server/server.go (1)

505-566: ⚠️ Potential issue | 🟠 Major

Both discovery calls execute unconditionally and hasNoKeys only changes logging.

ListModelsRequest is called at line 517 (filtered) and line 573 (unfiltered) regardless of hasNoKeys. The variable only determines which warning message is logged (lines 546-548 and 585-587), not whether discovery runs. For provider create/update and last-key delete, both requests still execute and block on potentially wasted work.

Additionally, the nested timeout structure in handlers/providers.go (line 809: context.WithTimeout(ctx, 15*time.Second)) combined with the inner contexts created with absolute deadlines (time.Now().Add(15*time.Second) at lines 517 and 570) can still cause cascading failures. If the filtered discovery consumes significant time, the parent context deadline is reached before the unfiltered discovery's inner deadline, causing it to fail with "context deadline exceeded." Short-circuit both discovery calls when hasNoKeys is true to avoid this unnecessary work.

🤖 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 505 - 566, The
discovery calls (s.Client.ListModelsRequest) should be skipped when hasNoKeys is
true to avoid wasted/blocking work: guard the filtered and unfiltered discovery
blocks (the code constructing filteredBfCtx, calling s.Client.ListModelsRequest,
calling filteredBfCtx.Cancel, and the later
unfilteredBfCtx/ListModelsRequest/cancel) with if !hasNoKeys { ... } so they are
not executed for keyless providers or providers with no keys; additionally,
create the per-call contexts from the incoming ctx deadline (use
context.WithTimeout(ctx, 15*time.Second) or derive deadline from ctx) instead of
time.Now().Add(15*time.Second) when calling schemas.NewBifrostContext to avoid
nested absolute deadlines that can cause cascading "context deadline exceeded"
errors; ensure s.updateKeyStatus, s.Config.ConfigStore.GetKeysByProvider and
s.Config.ModelCatalog.UpsertModelDataForProvider remain unchanged and still run
when discovery is skipped.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 389-400: The getKeyIDFromCtx function currently reads the "key_id"
user value raw; update getKeyIDFromCtx to URL-decode the extracted string (use
url.PathUnescape) before validation and return so client-supplied IDs with
escaped characters round-trip correctly; handle and return any decode error (and
still validate non-empty) while keeping the same error messages/flow for
missing/invalid parameters.
- Around line 90-92: The current guard only checks
providerConfig.CustomProviderConfig.IsKeyLess and misses built-in keyless
providers; change the condition in the key-add/delete handlers to block when the
provider is keyless regardless of source by checking the provider's canonical
keyless flag (e.g. call providerConfig.IsKeyLess() or test both
providerConfig.CustomProviderConfig.IsKeyLess and
providerConfig.BuiltInKeyless/Provider.IsKeyLess) before calling SendError, and
apply the same fix to the other occurrence around lines 158-160 so built-in
keyless providers are also prevented from having keys attached.

In `@transports/bifrost-http/handlers/providers.go`:
- Around line 88-107: The payload structs providerCreatePayload and
providerUpdatePayload currently ignore any incoming "keys" field, so modify
addProvider and updateProvider handlers to explicitly reject requests containing
a top-level "keys" key: parse the raw request body into a generic map[string]
before decoding into the typed struct, check if "keys" exists and if so return
HTTP 400 with a clear error message; only if "keys" is absent proceed to
unmarshal into providerCreatePayload/providerUpdatePayload and continue normal
processing. Ensure the validation is applied in the addProvider and
updateProvider request handling paths so legacy clients receive an explicit
error instead of silent data loss.

---

Outside diff comments:
In `@transports/bifrost-http/server/server.go`:
- Around line 505-566: The discovery calls (s.Client.ListModelsRequest) should
be skipped when hasNoKeys is true to avoid wasted/blocking work: guard the
filtered and unfiltered discovery blocks (the code constructing filteredBfCtx,
calling s.Client.ListModelsRequest, calling filteredBfCtx.Cancel, and the later
unfilteredBfCtx/ListModelsRequest/cancel) with if !hasNoKeys { ... } so they are
not executed for keyless providers or providers with no keys; additionally,
create the per-call contexts from the incoming ctx deadline (use
context.WithTimeout(ctx, 15*time.Second) or derive deadline from ctx) instead of
time.Now().Add(15*time.Second) when calling schemas.NewBifrostContext to avoid
nested absolute deadlines that can cause cascading "context deadline exceeded"
errors; ensure s.updateKeyStatus, s.Config.ConfigStore.GetKeysByProvider and
s.Config.ModelCatalog.UpsertModelDataForProvider remain unchanged and still run
when discovery is skipped.

🪄 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: dec8a738-6c62-4451-b854-71418beac328

📥 Commits

Reviewing files that changed from the base of the PR and between b3aefcd and 86de542.

📒 Files selected for processing (4)

• transports/bifrost-http/handlers/provider_keys.go
• transports/bifrost-http/handlers/providers.go
• transports/bifrost-http/handlers/providers_test.go
• transports/bifrost-http/server/server.go

✅ Files skipped from review due to trivial changes (1)

• transports/bifrost-http/handlers/providers_test.go

[coderabbitai]

♻️ Duplicate comments (3)

transports/bifrost-http/handlers/provider_keys.go (3)

90-93: ⚠️ Potential issue | 🟠 Major

Incomplete keyless provider validation.

This check only validates custom keyless providers but misses built-in keyless providers (Ollama, SGL). Based on core/utils.go, bifrost.IsKeylessProvider(provider) should also be checked to prevent attaching keys to built-in keyless providers.

🔧 Proposed fix

-	if providerConfig.CustomProviderConfig != nil && providerConfig.CustomProviderConfig.IsKeyLess {
+	isKeylessProvider := bifrost.IsKeylessProvider(provider) || (providerConfig.CustomProviderConfig != nil && providerConfig.CustomProviderConfig.IsKeyLess)
+	if isKeylessProvider {
 		SendError(ctx, fasthttp.StatusBadRequest, "Cannot add keys to a keyless provider")
 		return
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/provider_keys.go` around lines 90 - 93, The
current check only prevents adding keys for custom keyless providers via
providerConfig.CustomProviderConfig.IsKeyLess; update the validation to also
reject built-in keyless providers by calling
bifrost.IsKeylessProvider(providerConfig.Provider) (or the appropriate provider
identifier on providerConfig) before proceeding—if either
providerConfig.CustomProviderConfig.IsKeyLess is true OR
bifrost.IsKeylessProvider(...) returns true, call SendError(ctx,
fasthttp.StatusBadRequest, "Cannot add keys to a keyless provider") and return.
Ensure you reference providerConfig and bifrost.IsKeylessProvider in the same
block where SendError is currently invoked.

---

389-401: ⚠️ Potential issue | 🟡 Minor

URL-decode key_id before lookup.

Path parameters may contain URL-encoded characters. Without decoding, IDs with special characters won't round-trip correctly through GET/PUT/DELETE endpoints.

🔧 Proposed fix

+import "net/url"
+
 func getKeyIDFromCtx(ctx *fasthttp.RequestCtx) (string, error) {
 	keyValue := ctx.UserValue("key_id")
 	if keyValue == nil {
 		return "", fmt.Errorf("missing key_id parameter")
 	}

 	keyID, ok := keyValue.(string)
 	if !ok || keyID == "" {
 		return "", fmt.Errorf("invalid key_id parameter")
 	}

+	decodedKeyID, err := url.PathUnescape(keyID)
+	if err != nil {
+		return "", fmt.Errorf("invalid key_id encoding: %v", err)
+	}
+
-	return keyID, nil
+	return decodedKeyID, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/provider_keys.go` around lines 389 - 401,
The getKeyIDFromCtx function reads ctx.UserValue("key_id") but does not
URL-decode it, causing failures for path params with encoded characters; update
getKeyIDFromCtx to cast the value from ctx.UserValue("key_id") to string, call
url.PathUnescape (or equivalent) on that string and return an error if decoding
fails, and then return the decoded keyID so all GET/PUT/DELETE lookups use the
decoded ID.

---

158-161: ⚠️ Potential issue | 🟠 Major

Same incomplete keyless provider validation.

Apply the same fix as in createProviderKey to also check bifrost.IsKeylessProvider(provider).

🔧 Proposed fix

-	if providerConfig.CustomProviderConfig != nil && providerConfig.CustomProviderConfig.IsKeyLess {
+	isKeylessProvider := bifrost.IsKeylessProvider(provider) || (providerConfig.CustomProviderConfig != nil && providerConfig.CustomProviderConfig.IsKeyLess)
+	if isKeylessProvider {
 		SendError(ctx, fasthttp.StatusBadRequest, "Cannot update keys on a keyless provider")
 		return
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/provider_keys.go` around lines 158 - 161,
The current keyless provider validation only checks
providerConfig.CustomProviderConfig.IsKeyLess and misses providers that are
globally keyless; update the handler (the code block that currently returns
"Cannot update keys on a keyless provider") to also call
bifrost.IsKeylessProvider(provider) and return the same BadRequest via SendError
when that check is true—mirror the exact logic used in createProviderKey so both
CustomProviderConfig.IsKeyLess and bifrost.IsKeylessProvider(provider) are
evaluated before proceeding.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 90-93: The current check only prevents adding keys for custom
keyless providers via providerConfig.CustomProviderConfig.IsKeyLess; update the
validation to also reject built-in keyless providers by calling
bifrost.IsKeylessProvider(providerConfig.Provider) (or the appropriate provider
identifier on providerConfig) before proceeding—if either
providerConfig.CustomProviderConfig.IsKeyLess is true OR
bifrost.IsKeylessProvider(...) returns true, call SendError(ctx,
fasthttp.StatusBadRequest, "Cannot add keys to a keyless provider") and return.
Ensure you reference providerConfig and bifrost.IsKeylessProvider in the same
block where SendError is currently invoked.
- Around line 389-401: The getKeyIDFromCtx function reads
ctx.UserValue("key_id") but does not URL-decode it, causing failures for path
params with encoded characters; update getKeyIDFromCtx to cast the value from
ctx.UserValue("key_id") to string, call url.PathUnescape (or equivalent) on that
string and return an error if decoding fails, and then return the decoded keyID
so all GET/PUT/DELETE lookups use the decoded ID.
- Around line 158-161: The current keyless provider validation only checks
providerConfig.CustomProviderConfig.IsKeyLess and misses providers that are
globally keyless; update the handler (the code block that currently returns
"Cannot update keys on a keyless provider") to also call
bifrost.IsKeylessProvider(provider) and return the same BadRequest via SendError
when that check is true—mirror the exact logic used in createProviderKey so both
CustomProviderConfig.IsKeyLess and bifrost.IsKeylessProvider(provider) are
evaluated before proceeding.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 370f52c3-3115-4cc5-aa4a-7594e1fe0c86

📥 Commits

Reviewing files that changed from the base of the PR and between 86de542 and 9ff1b4f.

📒 Files selected for processing (4)

• transports/bifrost-http/handlers/provider_keys.go
• transports/bifrost-http/handlers/providers.go
• transports/bifrost-http/handlers/providers_test.go
• transports/bifrost-http/server/server.go

✅ Files skipped from review due to trivial changes (1)

• transports/bifrost-http/handlers/providers_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• transports/bifrost-http/handlers/providers.go

[coderabbitai]

🧹 Nitpick comments (1)

transports/bifrost-http/server/server.go (1)

557-566: Consider deferring Cancel() to handle panics.

Both filteredBfCtx.Cancel() and unfilteredBfCtx.Cancel() are called inline after use, but if a panic occurs between context creation and the Cancel() call, resources may leak. While unlikely in this code path, using defer is more defensive.

♻️ Suggested pattern for safer context cleanup

 	filteredBfCtx := schemas.NewBifrostContext(ctx, time.Now().Add(15*time.Second))
+	defer filteredBfCtx.Cancel()
 	filteredBfCtx.SetValue(schemas.BifrostContextKeySkipPluginPipeline, true)
 	filteredBfCtx.SetValue(schemas.BifrostContextKeyValidateKeys, true)

 	allModels, bifrostErr := s.Client.ListModelsRequest(filteredBfCtx, &schemas.BifrostListModelsRequest{
 		Provider: provider,
 	})
-	filteredBfCtx.Cancel()

Apply similarly to unfilteredBfCtx.

🤖 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 557 - 566, The context
cleanup for model discovery should use defer to ensure Cancel() runs even on
panics: after creating each context with schemas.NewBifrostContext (both
filteredBfCtx and unfilteredBfCtx) immediately defer the corresponding Cancel()
before any calls to s.Client.ListModelsRequest; keep the existing SetValue and
ListModelsRequest calls but remove the inline Cancel() calls so the deferred
Cancel() handles cleanup safely.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@transports/bifrost-http/server/server.go`:
- Around line 557-566: The context cleanup for model discovery should use defer
to ensure Cancel() runs even on panics: after creating each context with
schemas.NewBifrostContext (both filteredBfCtx and unfilteredBfCtx) immediately
defer the corresponding Cancel() before any calls to s.Client.ListModelsRequest;
keep the existing SetValue and ListModelsRequest calls but remove the inline
Cancel() calls so the deferred Cancel() handles cleanup safely.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: acbb2ab6-ea52-4353-ad90-c7d38a018f83

📥 Commits

Reviewing files that changed from the base of the PR and between 9ff1b4f and c00677a.

📒 Files selected for processing (4)

• transports/bifrost-http/handlers/provider_keys.go
• transports/bifrost-http/handlers/providers.go
• transports/bifrost-http/handlers/providers_test.go
• transports/bifrost-http/server/server.go

✅ Files skipped from review due to trivial changes (1)

• transports/bifrost-http/handlers/providers_test.go

[greptile-apps]

Confidence Score: 3/5

• PR is largely correct but has one logic regression in mergeUpdatedKey and is a breaking API change requiring coordinated client updates.
• The Description field is dropped from the key merge while Status (its semantic companion) is preserved, creating a potential inconsistent state. The context changes in server.go are functionally adequate. No new tests were added for the key handlers. The breaking removal of keys from provider responses needs careful client coordination.
• transports/bifrost-http/handlers/provider_keys.go — specifically the mergeUpdatedKey function around lines 393-394 where Description preservation was dropped.

Important Files Changed

Filename	Overview
transports/bifrost-http/handlers/provider_keys.go	New file implementing CRUD handlers for provider keys. Logic is mostly sound but mergeUpdatedKey drops Description preservation that was explicitly present in the old mergeKeys, creating a Status/Description inconsistency when model discovery fails post-update.
transports/bifrost-http/handlers/providers.go	Cleanly removes Keys from provider payloads/responses and extracts named types. The updateProvider flow correctly seeds config from oldConfigRaw.Keys so existing keys aren't lost. Unused encoding/json import removed.
transports/bifrost-http/server/server.go	Context fix moves GetKeysByProvider before bfCtx creation so it isn't subject to model-discovery timeouts. hasNoKeys check reduces log noise when no keys are configured. PR description overstates changes (context not split into separate instances, GetKeysByProvider still uses ctx not context.Background()), but runtime behaviour is correct.
transports/bifrost-http/handlers/providers_test.go	Cleared to a package declaration only — tests for the removed inline struct decoding were deleted with the refactor. No new tests cover the key handlers.

Comments Outside Diff (1)

1. transports/bifrost-http/server/server.go, line 533-545 (link)

   PR description overstates context split

   The PR description says:

   Split shared bfCtx into separate contexts: filteredBfCtx (15s) for filtered ListModelsRequest and unfilteredBfCtx (fresh 15s) for unfiltered ListModelsRequest, each cancelled after use

   In the actual code, both goroutines still share the same single bfCtx:

   go func() {
       allModels, bifrostErr = s.Client.ListModelsRequest(bfCtx, ...)  // filtered
   }()
   go func() {
       unfilteredModels, listModelsErr = s.Client.ListModelsRequest(bfCtx, ...) // unfiltered — same context
   }()

   Because the two goroutines run concurrently and start at the same time, both effectively have the full 15-second window, so the shared context does not cause the timeout exhaustion the PR intended to fix. However, the PR description also claims GetKeysByProvider was switched to context.Background(), but it still uses the passed-in ctx. These discrepancies between the description and implementation may confuse future reviewers trying to understand the intent of the fix.

_{Reviews (6): Last reviewed commit: "feat: add provider keys HTTP handlers an..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

transports/bifrost-http/handlers/provider_keys.go (1)

393-395: Description field cannot be updated by clients.

Description is unconditionally overwritten from oldRawKey, which means any user-provided description in the update request is discarded. While preserving ConfigHash and Status (system-managed fields) makes sense, Description appears to be user-editable based on its definition in the Key struct.

If this is intentional, consider adding a comment. Otherwise:

♻️ Proposed fix to allow Description updates

-	mergedKey.ConfigHash = oldRawKey.ConfigHash
-	mergedKey.Status = oldRawKey.Status
-	mergedKey.Description = oldRawKey.Description
+	// Preserve system-managed fields; allow user to update Description
+	mergedKey.ConfigHash = oldRawKey.ConfigHash
+	mergedKey.Status = oldRawKey.Status

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/provider_keys.go` around lines 393 - 395,
The code unconditionally copies Description from oldRawKey into mergedKey,
discarding any user-provided description from the update payload; change the
merge logic in the handler that builds mergedKey (references mergedKey,
oldRawKey, newRawKey) so Description is preserved from the incoming update
(newRawKey.Description) unless the client omitted it—i.e., do not overwrite
mergedKey.Description with oldRawKey.Description, instead set
mergedKey.Description = newRawKey.Description if non-empty, otherwise retain
oldRawKey.Description (keep ConfigHash and Status assignment as-is).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 270-398: mergeUpdatedKey currently unconditionally copies
Description from oldRawKey, which prevents client-sent updates; change it so
ConfigHash and Status remain copied from oldRawKey but Description is preserved
from updateKey when provided (fall back to oldRawKey.Description only if
updateKey.Description is empty/nil). Update the logic in mergeUpdatedKey (look
for mergedKey.Description = oldRawKey.Description) to use updateKey.Description
when non-empty, while keeping mergedKey.ConfigHash = oldRawKey.ConfigHash and
mergedKey.Status = oldRawKey.Status unchanged.

---

Nitpick comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 393-395: The code unconditionally copies Description from
oldRawKey into mergedKey, discarding any user-provided description from the
update payload; change the merge logic in the handler that builds mergedKey
(references mergedKey, oldRawKey, newRawKey) so Description is preserved from
the incoming update (newRawKey.Description) unless the client omitted it—i.e.,
do not overwrite mergedKey.Description with oldRawKey.Description, instead set
mergedKey.Description = newRawKey.Description if non-empty, otherwise retain
oldRawKey.Description (keep ConfigHash and Status assignment as-is).

🪄 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: ded48468-fa2a-4652-9847-ce6ff94edbcb

📥 Commits

Reviewing files that changed from the base of the PR and between 78f84df and cdd753f.

📒 Files selected for processing (4)

• transports/bifrost-http/handlers/provider_keys.go
• transports/bifrost-http/handlers/providers.go
• transports/bifrost-http/handlers/providers_test.go
• transports/bifrost-http/server/server.go

✅ Files skipped from review due to trivial changes (1)

• transports/bifrost-http/handlers/providers_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• transports/bifrost-http/server/server.go

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

transports/bifrost-http/handlers/providers.go (1)

371-381: ⚠️ Potential issue | 🔴 Critical

Don't write a stale Keys snapshot back during provider updates.

Keys is now managed by the new /api/providers/{provider}/keys handlers, but this code still copies oldConfigRaw.Keys into config and persists it again at Line 462. Any key create/update/delete that lands between those two points gets silently lost or resurrected. Preserve the current keys inside UpdateProviderConfig under the store lock, or stop writing Keys from this handler entirely.

Also applies to: 462-462

🤖 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 371 - 381, The
provider update currently copying oldConfigRaw.Keys into config causes races
with the new /api/providers/{provider}/keys endpoints; in UpdateProviderConfig
replace that behavior by not overwriting Keys from the handler: either
(preferred) read and preserve the live keys from the store while holding the
same store lock you use for UpdateProviderConfig and set config.Keys to that
value, or simply omit setting config.Keys here so UpdateProviderConfig does not
persist a stale snapshot; update the code paths referencing
configstore.ProviderConfig, UpdateProviderConfig and oldConfigRaw.Keys
accordingly to ensure keys are not clobbered by this handler.

♻️ Duplicate comments (1)

transports/bifrost-http/handlers/provider_keys.go (1)

393-395: ⚠️ Potential issue | 🟠 Major

Description updates are still being discarded.

This makes every PUT keep the old description, so clients still can't edit or clear it. ConfigHash and Status look system-managed here; Description should come from the request.

💡 Local fix

 	mergedKey.ConfigHash = oldRawKey.ConfigHash
 	mergedKey.Status = oldRawKey.Status
-	mergedKey.Description = oldRawKey.Description
 
 	return mergedKey

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@transports/bifrost-http/handlers/provider_keys.go` around lines 393 - 395,
The code is incorrectly copying Description from oldRawKey which prevents
clients from updating/clearing it; change the assignment so
mergedKey.Description is taken from the incoming request/new value (e.g.,
newRawKey.Description or the request payload) while keeping system-managed
fields like mergedKey.ConfigHash and mergedKey.Status sourced from oldRawKey as
before; update the assignment in the block that sets mergedKey fields (look for
mergedKey, oldRawKey, newRawKey) to use newRawKey.Description instead of
oldRawKey.Description.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 101-118: The handler currently appends caller-supplied key.ID
without checking uniqueness so the lib.ErrAlreadyExists branch never triggers;
before calling h.inMemoryStore.AddProviderKey(ctx, provider, key) ensure the ID
is unique by querying existing keys (e.g. call
h.inMemoryStore.GetProviderKey(ctx, provider, key.ID) or ListProviderKeys and
search for key.ID) and return a 409 (using lib.ErrAlreadyExists semantics) if a
match is found, or alternatively fix AddProviderKey itself to atomically check
for existing key.ID and return lib.ErrAlreadyExists; reference AddProviderKey,
GetProviderKey/ListProviderKeys, lib.ErrAlreadyExists, provider and key.ID when
implementing the check.

---

Outside diff comments:
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 371-381: The provider update currently copying oldConfigRaw.Keys
into config causes races with the new /api/providers/{provider}/keys endpoints;
in UpdateProviderConfig replace that behavior by not overwriting Keys from the
handler: either (preferred) read and preserve the live keys from the store while
holding the same store lock you use for UpdateProviderConfig and set config.Keys
to that value, or simply omit setting config.Keys here so UpdateProviderConfig
does not persist a stale snapshot; update the code paths referencing
configstore.ProviderConfig, UpdateProviderConfig and oldConfigRaw.Keys
accordingly to ensure keys are not clobbered by this handler.

---

Duplicate comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 393-395: The code is incorrectly copying Description from
oldRawKey which prevents clients from updating/clearing it; change the
assignment so mergedKey.Description is taken from the incoming request/new value
(e.g., newRawKey.Description or the request payload) while keeping
system-managed fields like mergedKey.ConfigHash and mergedKey.Status sourced
from oldRawKey as before; update the assignment in the block that sets mergedKey
fields (look for mergedKey, oldRawKey, newRawKey) to use newRawKey.Description
instead of oldRawKey.Description.

🪄 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: 6b9f3ad8-8c9a-4858-aeab-f83e58ca6af6

📥 Commits

Reviewing files that changed from the base of the PR and between cdd753f and 43e0a19.

📒 Files selected for processing (4)

• transports/bifrost-http/handlers/provider_keys.go
• transports/bifrost-http/handlers/providers.go
• transports/bifrost-http/handlers/providers_test.go
• transports/bifrost-http/server/server.go

✅ Files skipped from review due to trivial changes (2)

• transports/bifrost-http/handlers/providers_test.go
• transports/bifrost-http/server/server.go

[Pratham-Mishra04]

Merge activity

• Mar 25, 12:57 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Mar 25, 1:00 PM UTC: Graphite rebased this pull request as part of a merge.
• Mar 25, 1:01 PM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

[greptile-apps]

Description not preserved alongside Status

The old mergeKeys function preserved both Status AND Description from the raw key, with an explicit comment: "UI doesn't send them back, they're updated by model discovery". The new mergeUpdatedKey preserves Status but silently drops Description.

Status and Description are a pair — Description explains the current Status (e.g. Status = "error", Description = "API key invalid: 401"). Preserving one but resetting the other creates an inconsistent state after a PUT:

• If model discovery runs successfully after the update, updateKeyStatus will refresh both fields — no issue.
• If model discovery fails (or the provider has no keys at call time), Status is the old value (preserved) while Description is now "". The key surfaces an error status with no explanation, making it harder to diagnose.

Consider preserving Description for the same reason Status is preserved:

Suggested change

	mergedKey.ConfigHash = oldRawKey.ConfigHash
	mergedKey.Status = oldRawKey.Status
	mergedKey.ConfigHash = oldRawKey.ConfigHash
	mergedKey.Status = oldRawKey.Status
	mergedKey.Description = oldRawKey.Description