Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughPer-key Ollama and SGL server URLs were added to key schemas and persisted; provider-level BaseURL checks were relaxed. Providers now resolve per-key BaseURLs for requests and ListModels. Keyless detection and UI/validation were updated throughout configstore, migrations, transports, providers, and UI schemas. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User/UI
participant HTTP as Bifrost HTTP Handler
participant Core as Bifrost Core (requestWorker)
participant Keys as Config Store / Keys
participant Provider as Provider Impl (Ollama / SGL)
participant Remote as External Model Server
Client->>HTTP: submit request (ListModels / completion)
HTTP->>Core: forward request + provider info + keys
Core->>Provider: decide key path (providerRequiresKey(customConfig))
Provider->>Keys: resolve per-key BaseURL (baseURLOrError / getBaseURL)
alt base URL present
Keys-->>Provider: return base URL
Provider->>Remote: call per-key endpoint (/v1/...)
Remote-->>Provider: response
else missing base URL
Keys-->>Provider: return BifrostError
Provider-->>Core: propagate error
end
Provider-->>Core: aggregated/streamed responses
Core-->>HTTP: return result
HTTP-->>Client: deliver response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Confidence Score: 4/5Safe to merge with awareness of the open env-var URL persistence issue; all other previously-raised P1 concerns have been resolved Most prior P1 issues are resolved: shared-pointer mutation in GetAllKeys is fixed, Azure/Bedrock redaction bugs are fixed, SGL streaming path override is fixed, server-side URL validation is added, and the JSON schema is valid. One known issue remains unfixed from a prior review round — BeforeSave uses GetValue() instead of IsDefined(), which silently drops env-var-backed URLs when the env var is not present at save time. framework/configstore/tables/key.go — BeforeSave OllamaUrl/SGLUrl persistence uses GetValue() instead of IsDefined() Important Files Changed
Reviews (13): Last reviewed commit: "feat: adds support for multiple deployme..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 9
🧹 Nitpick comments (1)
core/utils.go (1)
86-92: Clarify whatproviderRequiresKeymeans now.After this change, the helper is really about whether requests still need a configured key entry, not whether the provider needs a non-empty API key value. Tightening the docstring will make it much harder to misuse next to
CanProviderKeyValueBeEmpty().📝 Suggested comment update
-// providerRequiresKey returns true if the given provider requires an API key for authentication. +// providerRequiresKey returns true if requests for this provider must still resolve through a configured key entry. +// The key value itself may be empty when routing/auth data lives in provider-specific key config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/utils.go` around lines 86 - 92, Update the providerRequiresKey function comment to clearly state that it determines whether a configured key entry is required for making requests (i.e., whether a key record must exist), not whether the provider’s API key string may be empty; reference the function name providerRequiresKey and contrast it with CanProviderKeyValueBeEmpty to avoid misuse, and mention the current behavior: customConfig with IsKeyLess true and BaseProviderType != schemas.Bedrock returns false (no key entry required), otherwise true.
🤖 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`:
- Line 4962: The current conditional if
providerRequiresKey(config.CustomProviderConfig) prevents key resolution for
custom providers (so Ollama/SGL lose their selected server URL via schemas.Key);
change the call-site so key selection runs for custom providers regardless of
whether providerRequiresKey returns true — e.g., unconditionally perform the
key-resolution/selection when config.CustomProviderConfig is present (or use a
new predicate like providerNeedsKeyObject(config.CustomProviderConfig) that
returns true for providers that need a Key object even if the API key value can
be empty) and keep providerRequiresKey for “requires non-empty API key” checks;
update the logic around providerRequiresKey, the key-resolution invocation, and
any downstream assumptions that expect a zero-value schemas.Key.
In `@core/utils.go`:
- Around line 95-99: CanProviderKeyValueBeEmpty currently returns true for
Ollama and SGL which causes GetAvailableProviders to consider those providers
available even when their per-key server URLs are missing; update the logic by
either (A) removing Ollama and SGL from CanProviderKeyValueBeEmpty and adding a
new helper (e.g., IsProviderConfigComplete(providerKey)) that verifies required
fields, or (B) keep CanProviderKeyValueBeEmpty as-is but modify
GetAvailableProviders in transports/bifrost-http/lib/config.go to explicitly
check ollama_key_config.url and sgl_key_config.url (and any other
provider-specific required fields) before advertising availability so that
Ollama/SGL keys are only shown as available when their server URL config is
present.
In `@framework/configstore/migrations.go`:
- Around line 5316-5355: Add a backfill step after adding the ollama_url and
sgl_url columns in migrationAddOllamaSGLKeyConfigColumns: query existing
provider rows to find providers with type "ollama" or "sgl" (or however provider
types are identified), read their network_config.base_url, update matching rows
in tables.TableKey to set ollama_url/sgl_url accordingly, and then recompute and
persist config_hash for each updated TableKey using the GenerateKeyHash
function; perform this work using the same tx (tx.WithContext(ctx)) and return
any errors so the migration remains atomic. Ensure you only touch keys that are
empty for the new columns to avoid overwriting explicit per-key URLs.
In `@framework/configstore/rdb.go`:
- Around line 78-79: Several manual constructions of tables.TableKey are
omitting the new per-key fields OllamaKeyConfig and SGLKeyConfig, causing
per-key server URL/config to be dropped during provider add/update/sync flows;
update each tables.TableKey literal (the constructors used around the code paths
for provider add/update/sync) to include OllamaKeyConfig: key.OllamaKeyConfig
and SGLKeyConfig: key.SGLKeyConfig (or the appropriate source variable) and
ensure any helper conversions or read/write paths (where ToTableKey or similar
helpers are not used) copy those fields so persistence preserves per-key
configs.
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 96-98: The write path accepts Ollama/SGL provider keys without
validating a server URL, allowing unusable keys to be persisted; after the
existing empty-value check in provider_keys.go, add validation that when
provider == schemas.Ollama the key.OllamaKeyConfig is non-nil and
key.OllamaKeyConfig.URL.GetValue() is non-empty, and likewise when provider ==
schemas.SGL the key.SGLKeyConfig is non-nil and key.SGLKeyConfig.URL.GetValue()
is non-empty; if either check fails, call SendError(ctx,
fasthttp.StatusBadRequest, "Server URL required for this provider") and return.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 692-695: The FormDescription string incorrectly shows Ollama's
port for SGL and splits the env variable suffix into two text nodes causing
"env.SGL _URL"; update the FormDescription in apiKeysFormFragment.tsx to (1) use
the correct example port for each provider (e.g., Ollama example
http://localhost:11434, SGL should use its correct host/port) and (2) render the
env variable as one contiguous string by constructing a single text node using
the isOllama ternary to produce either `env.OLLAMA_URL` or `env.SGL_URL` instead
of concatenating `env.{isOllama ? "OLLAMA" : "SGL"} _URL`; keep the existing
isOllama condition and FormDescription component.
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 151-152: The submit-path and UI are out of sync: the UI uses
hideBaseURL (provider.name === "vllm" || "ollama" || "sgl") but the submit logic
still sets requiresBaseUrl for "ollama" and "sgl". Update the requiresBaseUrl
logic to match the UI's effective condition (i.e., treat base URL as required
only when isCustomProvider and not one of the hidden providers) — e.g., derive
baseURLRequired from isCustomProvider && !hideBaseURL (or invert hideBaseURL)
and use that same variable in both the form rendering
(baseURLRequired/hideBaseURL) and the submit validation so saving network-only
changes succeeds when base_url is hidden.
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 36-41: The checks in ModelProviderKeysTableView are using
provider.name (the configured alias) so custom providers backed by
Ollama/SGL/VLLM are misclassified; update the logic to derive providerName from
the provider's effective base provider (e.g.,
provider.effectiveBaseProvider.name or the equivalent property on the provider
object) instead of provider.name, and use that value for the
isVLLM/isOllamaOrSGL checks, entityLabel/entityLabelPlural computation, and the
value passed into AddNewKeySheet so the sheet receives the real base provider
(VLLM/Ollama/SGL) rather than the alias.
In `@ui/lib/types/schemas.ts`:
- Around line 167-179: The JSON schema file transports/config.schema.json is
missing definitions for the new ollama_key_config and sgl_key_config fields that
correspond to the Zod schemas ollamaKeyConfigSchema and sglKeyConfigSchema;
update transports/config.schema.json to add properties "ollama_key_config" and
"sgl_key_config" (matching the shape used by envVarSchema: an object with value
and env_var string fields, same structure and validation as the existing
key-config entries in the file), ensure they are added to the top-level
properties and included in any required/optional lists consistent with other
key-config fields so config.json validation aligns with the Zod types.
---
Nitpick comments:
In `@core/utils.go`:
- Around line 86-92: Update the providerRequiresKey function comment to clearly
state that it determines whether a configured key entry is required for making
requests (i.e., whether a key record must exist), not whether the provider’s API
key string may be empty; reference the function name providerRequiresKey and
contrast it with CanProviderKeyValueBeEmpty to avoid misuse, and mention the
current behavior: customConfig with IsKeyLess true and BaseProviderType !=
schemas.Bedrock returns false (no key entry required), otherwise true.
🪄 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: 9b1f6db8-67ec-47c2-b406-86790786ca62
📒 Files selected for processing (20)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/utils.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
8a08e3f to
84d05ff
Compare
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
84d05ff to
4dc129a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/providers/fragments/networkFormFragment.tsx (1)
131-149:⚠️ Potential issue | 🟠 MajorAdd
enforce_http2to the provider-change reset payload.The
form.reset()call in the useEffect (lines 131-149) is missingenforce_http2, even though this field is present indefaultValues, the submit handler, and the form schema. When switching providers, the toggle won't reset to the provider's actual value, causing the form to display or submit the wrong HTTP/2 setting.♻️ Suggested fix
form.reset({ network_config: { base_url: provider.network_config?.base_url || undefined, extra_headers: provider.network_config?.extra_headers, default_request_timeout_in_seconds: provider.network_config?.default_request_timeout_in_seconds ?? DefaultNetworkConfig.default_request_timeout_in_seconds, max_retries: provider.network_config?.max_retries ?? DefaultNetworkConfig.max_retries, retry_backoff_initial: provider.network_config?.retry_backoff_initial ?? DefaultNetworkConfig.retry_backoff_initial, retry_backoff_max: provider.network_config?.retry_backoff_max ?? DefaultNetworkConfig.retry_backoff_max, insecure_skip_verify: provider.network_config?.insecure_skip_verify ?? DefaultNetworkConfig.insecure_skip_verify, ca_cert_pem: provider.network_config?.ca_cert_pem ?? DefaultNetworkConfig.ca_cert_pem, stream_idle_timeout_in_seconds: provider.network_config?.stream_idle_timeout_in_seconds ?? DefaultNetworkConfig.stream_idle_timeout_in_seconds, max_conns_per_host: provider.network_config?.max_conns_per_host ?? DefaultNetworkConfig.max_conns_per_host, + enforce_http2: provider.network_config?.enforce_http2 ?? DefaultNetworkConfig.enforce_http2, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx` around lines 131 - 149, The useEffect that calls form.reset to apply provider.network_config on provider change is missing the enforce_http2 field; update the payload passed to form.reset (inside the useEffect) to include enforce_http2: set it to provider.network_config?.enforce_http2 ?? DefaultNetworkConfig.enforce_http2 so the form.toggle and submit behavior (form.reset, provider.network_config, DefaultNetworkConfig) reflect the provider's actual HTTP/2 setting.
♻️ Duplicate comments (2)
ui/app/workspace/providers/fragments/networkFormFragment.tsx (1)
90-92:⚠️ Potential issue | 🟠 MajorDerive Base URL rules from the effective base provider.
provider.nameonly catches the built-ins. A custom provider backed byvllm,ollama, orsglwill still render Base URL andonSubmitwill still reject an emptynetwork_config.base_url, even though those providers now route via per-key URLs.♻️ Suggested fix
@@ const [updateProvider, { isLoading: isUpdatingProvider }] = useUpdateProviderMutation(); const isCustomProvider = !isKnownProvider(provider.name as string); + const effectiveProviderName = provider.custom_provider_config?.base_provider_type ?? provider.name; + const isPerKeyURLProvider = + effectiveProviderName === "vllm" || effectiveProviderName === "ollama" || effectiveProviderName === "sgl"; @@ const onSubmit = (data: NetworkOnlyFormSchema) => { - const requiresBaseUrl = isCustomProvider; + const requiresBaseUrl = isCustomProvider && !isPerKeyURLProvider; @@ - const baseURLRequired = isCustomProvider; - const hideBaseURL = provider.name === "vllm" || provider.name === "ollama" || provider.name === "sgl"; + const baseURLRequired = isCustomProvider && !isPerKeyURLProvider; + const hideBaseURL = isPerKeyURLProvider;Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx` around lines 90 - 92, The submit validation in onSubmit currently uses isCustomProvider / provider.name to decide if network_config.base_url is required, which misses custom providers that are backed by vllm/ollama/sgl; update the logic that computes requiresBaseUrl (and any similar check around lines 151-152) to derive the rule from the provider's effective backend (e.g., an effectiveProvider/effective_backend/provider.type field) rather than provider.name so providers routed via per-key URLs (vllm, ollama, sgl) do not force a base_url; adjust the check that rejects an empty data.network_config.base_url and any UI conditional that shows the Base URL field to use this effective-backend-derived predicate instead of the current isCustomProvider.framework/configstore/migrations.go (1)
5349-5388:⚠️ Potential issue | 🟠 MajorBackfill existing Ollama/SGL server URLs into the new key columns.
This only adds
ollama_url/sgl_url. Existing Ollama/SGL deployments still keep their endpoint on the provider-levelnetwork_config.base_url, while the rest of this stack now resolves those providers from per-key URLs. Upgraded keys will stay blank and model discovery/inference will fail until every key is edited manually. If you backfill here, refreshconfig_hashfor the rows you touch becauseGenerateKeyHashnow includes these configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5349 - 5388, The migrationAddOllamaSGLKeyConfigColumns migration must not only add/drop ollama_url and sgl_url columns but also backfill them from existing provider network_config.base_url and refresh the key's config_hash using GenerateKeyHash so upgraded keys continue to work. After adding the columns in Migrate, SELECT TableKey rows whose provider indicates Ollama/SGL (or whose provider.NetworkConfig.BaseURL is non-empty) and where the new ollama_url/sgl_url is empty, set the new column to the provider's network_config.base_url, call GenerateKeyHash on that TableKey to recompute config_hash, and persist the updated row within the same tx (using tx.Save or tx.Model(...).Updates). Do the reverse on Rollback only if needed (you can leave rollback as dropping the columns). Ensure you reference tables.TableKey, migrationAddOllamaSGLKeyConfigColumns, and GenerateKeyHash when implementing the backfill and config_hash refresh.
🧹 Nitpick comments (1)
core/providers/ollama/ollama.go (1)
67-120: Keep the per-key URL plumbing in shared OpenAI utilities.This stack adds the same per-key URL/list-models behavior to both Ollama and SGL. Extracting these helpers into
openai/utils.gowould keep the OpenAI-compatible providers thin and reduce drift.As per coding guidelines, "OpenAI-compatible providers (Groq, Cerebras, Ollama, etc.) should only contain minimal constructor and delegate to
openai.HandleOpenAI*functions." and "Provider constants, base URLs, and unexported helpers belong inutils.go; error parsing belongs inerrors.go."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/ollama/ollama.go` around lines 67 - 120, This file contains per-key base URL and list-models plumbing (getBaseURL, baseURLOrError, listModelsByKey, ListModels) that should be moved into the shared openai utilities; extract getBaseURL/baseURLOrError and the per-key listModelsByKey logic into openai/utils.go (and errors into openai/errors.go if needed) and expose a helper like openai.HandleOpenAIListModels (or reuse existing openai.ListModelsByKey signature) that accepts ctx, keys, request, the provider HTTP client, ExtraHeaders, provider key id, and the sendBackRaw flags; then simplify Ollama.ListModels to delegate to that openai helper and remove the duplicated plumbing from core/providers/ollama/ollama.go (keep only minimal constructor/delegation and provider-specific constants).
🤖 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/providers/ollama/ollama.go`:
- Around line 147-156: The streaming TextCompletion path is hardcoded to
baseURL+"/v1/completions" in TextCompletionStream; change it to use the same
context-aware path helper used by the non-streaming TextCompletion so
request-path overrides are respected. In practice, inside TextCompletionStream
(and before calling openai.HandleOpenAITextCompletionStreaming) build the
request path using the same helper/function that TextCompletion uses (rather
than concatenating "/v1/completions" to baseURL from baseURLOrError) and pass
that computed path as the third argument to
openai.HandleOpenAITextCompletionStreaming.
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 101-104: The current URL validation uses validateProviderKeyURL
which only checks literal schemas.Ollama / schemas.SGL, allowing custom
providers that point at those backends to bypass per-key URL checks; update the
code to validate against the resolved base provider type (use the baseProvider
variable from AddProvider instead of effectiveProvider) by passing the resolved
base provider into validateProviderKeyURL (or adding a new parameter/overload)
so the check treats custom_provider_config.base_provider_type as the underlying
provider and enforces the same URL constraints; apply the same change where
validateProviderKeyURL is called around the other occurrences (the blocks
referenced near lines 207-210 and 452-464) to ensure consistent validation.
- Around line 207-210: After merging updates into the stored key
(mergeUpdatedKey), re-run the empty-value guard so providers that disallow empty
values (use CanProviderKeyValueBeEmpty or the existing
validateProviderKeyURL/validateProviderKeyValue helper) cannot persist an empty
mergedKey.Value; if the revalidation fails, call SendError(ctx,
fasthttp.StatusBadRequest, err.Error()) and return. Ensure you reference
mergedKey and run the same validation used on create before proceeding with
update persistence.
In `@transports/config.schema.json`:
- Around line 2221-2223: The JSON schema contains invalid trailing commas after
the "$ref" entries for the network_config objects named "network_config" (the
two occurrences referencing "#/$defs/network_config"); remove the trailing
commas immediately after those "$ref" lines so each "network_config": { "$ref":
"#/$defs/network_config" } entry ends without a comma, ensuring the file is
valid JSON and parsable as a schema.
---
Outside diff comments:
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 131-149: The useEffect that calls form.reset to apply
provider.network_config on provider change is missing the enforce_http2 field;
update the payload passed to form.reset (inside the useEffect) to include
enforce_http2: set it to provider.network_config?.enforce_http2 ??
DefaultNetworkConfig.enforce_http2 so the form.toggle and submit behavior
(form.reset, provider.network_config, DefaultNetworkConfig) reflect the
provider's actual HTTP/2 setting.
---
Duplicate comments:
In `@framework/configstore/migrations.go`:
- Around line 5349-5388: The migrationAddOllamaSGLKeyConfigColumns migration
must not only add/drop ollama_url and sgl_url columns but also backfill them
from existing provider network_config.base_url and refresh the key's config_hash
using GenerateKeyHash so upgraded keys continue to work. After adding the
columns in Migrate, SELECT TableKey rows whose provider indicates Ollama/SGL (or
whose provider.NetworkConfig.BaseURL is non-empty) and where the new
ollama_url/sgl_url is empty, set the new column to the provider's
network_config.base_url, call GenerateKeyHash on that TableKey to recompute
config_hash, and persist the updated row within the same tx (using tx.Save or
tx.Model(...).Updates). Do the reverse on Rollback only if needed (you can leave
rollback as dropping the columns). Ensure you reference tables.TableKey,
migrationAddOllamaSGLKeyConfigColumns, and GenerateKeyHash when implementing the
backfill and config_hash refresh.
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 90-92: The submit validation in onSubmit currently uses
isCustomProvider / provider.name to decide if network_config.base_url is
required, which misses custom providers that are backed by vllm/ollama/sgl;
update the logic that computes requiresBaseUrl (and any similar check around
lines 151-152) to derive the rule from the provider's effective backend (e.g.,
an effectiveProvider/effective_backend/provider.type field) rather than
provider.name so providers routed via per-key URLs (vllm, ollama, sgl) do not
force a base_url; adjust the check that rejects an empty
data.network_config.base_url and any UI conditional that shows the Base URL
field to use this effective-backend-derived predicate instead of the current
isCustomProvider.
---
Nitpick comments:
In `@core/providers/ollama/ollama.go`:
- Around line 67-120: This file contains per-key base URL and list-models
plumbing (getBaseURL, baseURLOrError, listModelsByKey, ListModels) that should
be moved into the shared openai utilities; extract getBaseURL/baseURLOrError and
the per-key listModelsByKey logic into openai/utils.go (and errors into
openai/errors.go if needed) and expose a helper like
openai.HandleOpenAIListModels (or reuse existing openai.ListModelsByKey
signature) that accepts ctx, keys, request, the provider HTTP client,
ExtraHeaders, provider key id, and the sendBackRaw flags; then simplify
Ollama.ListModels to delegate to that openai helper and remove the duplicated
plumbing from core/providers/ollama/ollama.go (keep only minimal
constructor/delegation and provider-specific constants).
🪄 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: 78b93e4b-559c-4410-8323-7f76fbbd742f
📒 Files selected for processing (21)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/utils.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- core/schemas/account.go
🚧 Files skipped from review as they are similar to previous changes (11)
- transports/bifrost-http/server/server.go
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- ui/lib/types/config.ts
- framework/configstore/clientconfig.go
- core/providers/sgl/sgl.go
- core/bifrost.go
- transports/bifrost-http/lib/config.go
- framework/configstore/rdb.go
- ui/lib/types/schemas.ts
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- framework/configstore/tables/key.go
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
4dc129a to
a454d5f
Compare
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
a454d5f to
e52e8f4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/rdb.go (1)
533-607:⚠️ Potential issue | 🟠 MajorRegenerate
ConfigHashwhen an existing key changes.This branch computes a fresh
GenerateKeyHash(key)and then immediately restoresexistingKey.ConfigHash. Since the hash now includesOllamaKeyConfig/SGLKeyConfig, changing a server URL throughUpdateProviderwill persist the new config under the old hash, so any cache or reconciliation keyed offConfigHashstays stale.💡 Proposed fix
// Check if this key already exists 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 dbKey.CreatedAt = existingKey.CreatedAt // Preserve original creation timestamp if err := txDB.WithContext(ctx).Save(&dbKey).Error; err != nil {Please make the same change in
UpdateProviderKey, which resets the hash the same way later in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 533 - 607, The code currently computes a new GenerateKeyHash(key) then unconditionally restores existingKey.ConfigHash, preventing updated configs from getting a new hash; change the existing-key branch so you only preserve existingKey.ConfigHash when it matches the freshly computed keyHash, otherwise assign the new keyHash to dbKey.ConfigHash; keep preserving other UI-managed fields (Status, Description, EncryptionStatus, CreatedAt) as before. Make the identical change in UpdateProviderKey where the hash is likewise reset.
♻️ Duplicate comments (1)
core/utils.go (1)
89-101:⚠️ Potential issue | 🟠 MajorKeep key selection mandatory for Ollama/SGL custom providers.
requestWorkernow usesproviderRequiresKey()to decide whetherselectKeyFromProviderForModelruns at all. With this PR, Ollama/SGL resolve their base URL from the selected key, so a custom provider configured withIsKeyLess=truewill skip key selection and hit the provider without the URL it needs. The auth token may be optional, but the key object is still required for routing.💡 Proposed fix
func providerRequiresKey(customConfig *schemas.CustomProviderConfig) bool { - // Keyless custom providers are not allowed for Bedrock. - if customConfig != nil && customConfig.IsKeyLess && customConfig.BaseProviderType != schemas.Bedrock { + // Some base providers allow an empty auth token but still require a selected key + // for per-key routing data such as server URLs. + if customConfig != nil && + customConfig.IsKeyLess && + customConfig.BaseProviderType != schemas.Bedrock && + customConfig.BaseProviderType != schemas.Ollama && + customConfig.BaseProviderType != schemas.SGL { return false } return true }Even if the HTTP layer already rejects that combination, this core helper is also used by direct Go callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/utils.go` around lines 89 - 101, providerRequiresKey currently allows keyless custom providers for Ollama/SGL which breaks routing because those providers need a key object for base URL resolution; update providerRequiresKey to still require a key for custom providers whose BaseProviderType is Ollama or SGL (i.e., only return false when customConfig != nil && customConfig.IsKeyLess && customConfig.BaseProviderType is not Bedrock, Ollama, or SGL). Also remove schemas.Ollama and schemas.SGL from the true-returning list in CanProviderKeyValueBeEmpty so Ollama/SGL are treated as requiring a key (keep Vertex, Bedrock, VLLM, Azure as before); reference functions: providerRequiresKey and CanProviderKeyValueBeEmpty and consumers like selectKeyFromProviderForModel/requestWorker when making changes.
🤖 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 418-430: The merge logic in mergeUpdatedKey currently restores the
raw URL only when the incoming redacted token exactly equals the previously
stored redacted value, but it allows any other redacted placeholder to overwrite
the real URL and later pass validateProviderKeyURL; update mergeUpdatedKey to
detect redacted placeholder URLs (URL.IsRedacted()) that do NOT equal the
corresponding oldRedactedKey URL and treat them as invalid/untrusted—either
reject the update with an error or ignore the incoming redacted value so
mergedKey retains oldRawKey.URL; apply the same check for both
OllamaKeyConfig.URL and SGLKeyConfig.URL (the two existing blocks referencing
updateKey.OllamaKeyConfig, oldRedactedKey.OllamaKeyConfig,
oldRawKey.OllamaKeyConfig and the analogous SGL* symbols), and ensure
validateProviderKeyURL is invoked only on final, non-redacted values.
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Line 91: The provider form schema still forces base_url for Ollama/SGL; update
the logic where baseURLRequired is defined (currently enforcing base_url at
ui/lib/schemas/providerForm.ts) to mirror the network form by setting
baseURLRequired = isCustomProvider (use the same isCustomProvider check as in
networkFormFragment where requiresBaseUrl is set), so the schema no longer
requires base_url for Ollama/SGL providers.
---
Outside diff comments:
In `@framework/configstore/rdb.go`:
- Around line 533-607: The code currently computes a new GenerateKeyHash(key)
then unconditionally restores existingKey.ConfigHash, preventing updated configs
from getting a new hash; change the existing-key branch so you only preserve
existingKey.ConfigHash when it matches the freshly computed keyHash, otherwise
assign the new keyHash to dbKey.ConfigHash; keep preserving other UI-managed
fields (Status, Description, EncryptionStatus, CreatedAt) as before. Make the
identical change in UpdateProviderKey where the hash is likewise reset.
---
Duplicate comments:
In `@core/utils.go`:
- Around line 89-101: providerRequiresKey currently allows keyless custom
providers for Ollama/SGL which breaks routing because those providers need a key
object for base URL resolution; update providerRequiresKey to still require a
key for custom providers whose BaseProviderType is Ollama or SGL (i.e., only
return false when customConfig != nil && customConfig.IsKeyLess &&
customConfig.BaseProviderType is not Bedrock, Ollama, or SGL). Also remove
schemas.Ollama and schemas.SGL from the true-returning list in
CanProviderKeyValueBeEmpty so Ollama/SGL are treated as requiring a key (keep
Vertex, Bedrock, VLLM, Azure as before); reference functions:
providerRequiresKey and CanProviderKeyValueBeEmpty and consumers like
selectKeyFromProviderForModel/requestWorker when making changes.
🪄 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: 6b7cbb05-32dd-47a3-a13d-9ff511ba6102
📒 Files selected for processing (22)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- transports/bifrost-http/server/server.go
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- framework/configstore/clientconfig.go
- core/bifrost.go
- ui/lib/types/config.ts
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- core/providers/sgl/sgl.go
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- ui/lib/types/schemas.ts
- core/providers/ollama/ollama.go
- transports/bifrost-http/lib/config.go
- framework/configstore/migrations.go
- transports/config.schema.json
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
e52e8f4 to
9204f13
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docs/openapi/openapi.json (1)
134607-134672: De-duplicate repeated key-config schema blocks via shared component refs.The same key-config object structures are repeated many times. This is high drift risk and already makes consistency checks harder. Prefer a shared component schema in source OpenAPI YAML and reference it where needed, then regenerate JSON.
Based on learnings: "the bundled OpenAPI spec (docs/openapi/openapi.json) is inlined and follows the existing generated-spec pattern" and should align with the ref-based structure.
Also applies to: 135379-135444, 136037-136102, 136982-137047, 137896-137961, 138554-138619, 139413-139478, 140315-140380, 172982-173047, 198587-198652, 199256-199321, 199928-199993, 200550-200615, 201166-201231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 134607 - 134672, Multiple identical key-config object schemas (e.g., vllm_key_config, ollama_key_config, sgl_key_config) are duplicated inline; replace them with a single shared component schema and reference it instead: define a reusable component (e.g., KeyConfigComponent) matching the env-var structure (properties: value, env_var, from_env) and any provider-specific fields (model_name), then update each inline definition (vllm_key_config, ollama_key_config, sgl_key_config and the other occurrences listed) to use a $ref to that component; finally regenerate the bundled OpenAPI JSON so the inlined blocks are replaced by refs and the spec remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/openapi.json`:
- Around line 143653-143657: The description for the boolean property
allow_on_all_virtual_keys currently overstates permissions; update its text to
state that when true the MCP client is made available to all virtual keys by
default but tool execution remains governed by tools_to_execute and
tools_to_auto_execute and any explicit per-virtual-key MCP config (VK config)
takes precedence; replace the existing phrasing in the allow_on_all_virtual_keys
entries (including the other occurrences noted) with a concise sentence that the
flag grants client accessibility to all VKs by default while not bypassing
tool-level allowlists and being overridden by explicit VK MCP config.
- Around line 134631-134671: The OpenAPI schemas "ollama_key_config" and
"sgl_key_config" currently declare a "url" property but do not mark it required,
allowing payloads the runtime will reject; update each occurrence of the
"ollama_key_config" and "sgl_key_config" schema blocks in openapi.json to
include a required array that contains "url" (i.e., add "required": ["url"] at
the same object level as "properties"), and do this consistently for every
repeated occurrence referenced (e.g., the blocks starting at the given ranges)
so the spec matches the runtime contract.
In `@framework/configstore/migrations.go`:
- Around line 5438-5452: The Rollback closure in the migration is missing
removal of the huggingface_deployments_json column; update the Rollback function
that uses tx.Migrator() (same block referencing tables.TableKey,
migrator.HasColumn and migrator.DropColumn) to check for
"huggingface_deployments_json" and call migrator.DropColumn(&tables.TableKey{},
"huggingface_deployments_json") if present, returning any error as done for
"ollama_url" and "sgl_url" so the rollback reverses all three added columns.
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Line 91: The form and schema disagree on when base_url is required; update the
network form so requiresBaseUrl matches the schema by including Ollama and SGL
providers: change the requiresBaseUrl logic in networkFormFragment.tsx (the
requiresBaseUrl variable that currently uses isCustomProvider) to also check
data.selectedProvider or selectedProvider for "ollama" and "sgl" (same providers
used in providerForm.ts where baseURLRequired is computed) so the UI validation
aligns with the schema.
---
Nitpick comments:
In `@docs/openapi/openapi.json`:
- Around line 134607-134672: Multiple identical key-config object schemas (e.g.,
vllm_key_config, ollama_key_config, sgl_key_config) are duplicated inline;
replace them with a single shared component schema and reference it instead:
define a reusable component (e.g., KeyConfigComponent) matching the env-var
structure (properties: value, env_var, from_env) and any provider-specific
fields (model_name), then update each inline definition (vllm_key_config,
ollama_key_config, sgl_key_config and the other occurrences listed) to use a
$ref to that component; finally regenerate the bundled OpenAPI JSON so the
inlined blocks are replaced by refs and the spec remains consistent.
🪄 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: b42b2b8b-e4e9-4657-ae90-98355562bd24
📒 Files selected for processing (24)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (1)
- transports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (14)
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- framework/configstore/clientconfig.go
- framework/configstore/rdb.go
- core/schemas/envvar.go
- transports/bifrost-http/server/server.go
- core/bifrost.go
- ui/lib/types/config.ts
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- core/providers/ollama/ollama.go
- ui/lib/types/schemas.ts
- framework/configstore/tables/key.go
- transports/bifrost-http/handlers/provider_keys.go
- transports/bifrost-http/lib/config.go
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
9204f13 to
c4028da
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/lib/schemas/providerForm.ts (1)
228-246:⚠️ Potential issue | 🟠 MajorDon't require provider-level
base_urlfor custom aliases of per-key URL providers.
isCustomProvideris also true for aliases backed byvllm,ollama, orsgl, so this block still rejects providers that now resolve their endpoint from*_key_config.url. Please derive this fromdata.baseProviderType ?? data.selectedProviderand skip provider-levelbase_urlfor those base providers; the same effective-provider check should drive the related keyless/API-key gates below as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/schemas/providerForm.ts` around lines 228 - 246, The current base URL validation uses isCustomProvider and wrongly requires provider-level networkConfig.base_url for aliases whose effective base provider is vllm, ollama, or sgl; compute the effective provider as const effective = data.baseProviderType ?? data.selectedProvider and set baseURLRequired to true only when isCustomProvider is true AND effective is NOT one of ['vllm','ollama','sgl']; update the conditional that adds issues for networkConfig.base_url and the regex check to use this new baseURLRequired, and apply the same effective-provider-based check to the related keyless/API-key gating logic elsewhere so those gates skip provider-level base_url when the effective provider resolves to vllm/ollama/sgl.
♻️ Duplicate comments (2)
ui/app/workspace/providers/fragments/networkFormFragment.tsx (1)
90-92:⚠️ Potential issue | 🟠 MajorUse the effective base provider for Base URL gating.
provider.nameis the configured alias for custom providers. A custom provider backed byvllm,ollama, orsglwill still show and validatenetwork_config.base_urlhere, even though this stack moved those providers to per-key URLs. Please derive a singleisPerKeyURLProviderfromprovider.custom_provider_config?.base_provider_type ?? provider.nameand reuse it for bothrequiresBaseUrlandhideBaseURL.♻️ Minimal fix
+ const effectiveProviderName = provider.custom_provider_config?.base_provider_type ?? provider.name; + const isPerKeyURLProvider = + effectiveProviderName === "vllm" || effectiveProviderName === "ollama" || effectiveProviderName === "sgl"; + const onSubmit = (data: NetworkOnlyFormSchema) => { - const requiresBaseUrl = isCustomProvider; + const requiresBaseUrl = isCustomProvider && !isPerKeyURLProvider; if (requiresBaseUrl && (data.network_config?.base_url ?? "").trim() === "") { ... } }; - const baseURLRequired = isCustomProvider; - const hideBaseURL = provider.name === "vllm" || provider.name === "ollama" || provider.name === "sgl"; + const baseURLRequired = isCustomProvider && !isPerKeyURLProvider; + const hideBaseURL = isPerKeyURLProvider;Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx` around lines 90 - 92, The gating logic uses provider.name instead of the effective base provider type, so derive a single boolean like isPerKeyURLProvider from (provider.custom_provider_config?.base_provider_type ?? provider.name) and use that value in place of the current checks: set requiresBaseUrl = !isPerKeyURLProvider (or equivalent) and reuse the same boolean for hideBaseURL; update the onSubmit validation (where requiresBaseUrl is computed) and the hideBaseURL calculation to reference this new derived identifier so providers backed by vllm/ollama/sgl correctly skip base_url validation.ui/app/workspace/providers/views/modelProviderKeysTableView.tsx (1)
36-41:⚠️ Potential issue | 🟠 MajorPass the effective base provider into the key sheet.
provider.nameis the configured alias. For custom providers backed byvllm,ollama, orsgl, this passes the alias down asproviderName, so provider-specific sheet logic sees a generic provider instead of the required server/model-backed one.♻️ Minimal fix
- const providerName = provider.name?.toLowerCase() ?? ""; + const providerName = (provider.custom_provider_config?.base_provider_type ?? provider.name ?? "").toLowerCase();Also applies to: 97-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx` around lines 36 - 41, The code is using provider.name (the configured alias) to decide vllm/ollama/sgl behavior, so compute an effective base provider name (e.g. const effectiveProviderName = (provider.baseProvider ?? provider.name)?.toLowerCase()) and use that for isVLLM, isOllamaOrSGL, entityLabel and entityLabelPlural in ModelProviderKeysTableView; also pass the effective base provider into the key sheet component/prop(s) where providerName (or provider) is forwarded (the same change needed in the other usage around the later block currently using providerName), so provider-specific sheet logic receives the actual backing provider (vllm/ollama/sgl) rather than the alias.
🧹 Nitpick comments (2)
transports/bifrost-http/server/server.go (1)
510-511: Unify keyless/key-required predicate to avoid cross-path driftThis branch now treats any
CustomProviderConfig.IsKeyLessprovider as keyless, butcore/utils.gostill applies a Bedrock exception in key-requirement logic. Please derivehasNoKeysfrom the same shared predicate (or a single helper) so reload/model-discovery behavior can’t diverge from request-time key enforcement.Suggested alignment
- isKeylessProvider := providerInfo.CustomProviderConfig != nil && providerInfo.CustomProviderConfig.IsKeyLess - hasNoKeys := len(inMemoryKeys) == 0 && !isKeylessProvider + // Keep semantics aligned with central key requirement rules. + requiresKey := true + if providerInfo.CustomProviderConfig != nil && + providerInfo.CustomProviderConfig.IsKeyLess && + providerInfo.CustomProviderConfig.BaseProviderType != schemas.Bedrock { + requiresKey = false + } + hasNoKeys := len(inMemoryKeys) == 0 && requiresKey🤖 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 510 - 511, The hasNoKeys calculation is using a local isKeylessProvider boolean while key-requirement logic elsewhere uses the shared predicate in core/utils.go, which can cause drift; update this code to derive keylessness from the same shared helper used at request-time (rather than directly reading providerInfo.CustomProviderConfig.IsKeyLess) so both reload/model-discovery and request-time checks match; replace the isKeylessProvider/local logic and set hasNoKeys by calling the shared helper (the same function used in core/utils.go) that encapsulates the Bedrock exception semantics for keyless providers (ensure you reference the shared helper name when making the change so providerInfo handling and hasNoKeys are unified).core/providers/ollama/ollama.go (1)
67-119: Move the per-key URL helpers into the shared OpenAI-compatible layer.
getBaseURL,baseURLOrError, and the per-key list-model fan-out are now Ollama-specific glue, but SGL needs the same behavior. Extracting this intoopenai/provider utils (or at leastutils.go) will keep these providers thin and reduce drift.As per coding guidelines,
core/providers/{groq,cerebras,ollama,perplexity,openrouter,parasail,nebius,xai,sgl}/*.go: OpenAI-compatible providers should only contain minimal constructor and delegate toopenai.HandleOpenAI*functions; provider constants, base URLs, and unexported helpers belong inutils.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/ollama/ollama.go` around lines 67 - 119, The per-key URL helpers getBaseURL, baseURLOrError and the per-key fan-out listModelsByKey are Ollama-specific glue that should be moved into the shared OpenAI-compatible layer (openai package or provider utils) so other OpenAI-compatible providers (like SGL) can reuse them; create exported/shared functions (e.g., openai.GetBaseURL(key), openai.BaseURLOrError(key), and openai.ListModelsByKey(ctx, client, baseURLPath, key, request, extraHeaders, providerKey, sendRawReq, sendRawResp)) that encapsulate the URL-resolving and per-key request logic, update OllamaProvider.ListModels to delegate to those new openai/utils functions and remove getBaseURL, baseURLOrError, and listModelsByKey from ollama.go, and ensure existing calls to providerUtils.HandleMultipleListModelsRequests or similar are replaced to call the shared implementations so SGL and other providers can call the same helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/openapi.json`:
- Line 134643: The sgl_key_config property currently uses an invalid string ref
"$ref:'#/SglKeyConfig'"; replace that string with a proper JSON Schema object
(like the adjacent ollama_key_config inline schema) so $ref is not embedded as a
string — update the sgl_key_config schema definition to be an object describing
its properties/type (mirroring ollama_key_config structure) and apply the same
fix to all 14 occurrences (e.g., sgl_key_config entries at the other listed
locations) so the OpenAPI spec uses valid schema objects instead of string refs.
- Around line 134595-134617: The vllm_key_config schema currently lacks a
required constraint and incorrectly suggests model_name is required; update the
OpenAPI schema for vllm_key_config to include required: ["url"] (not model_name)
so the URL field is enforced, mirroring the ollama_key_config pattern; this
aligns with the Go struct fields (ModelName optional, URL required) and the
runtime check in baseURLOrError in core/providers/vllm/vllm.go that errors when
URL is empty.
In `@framework/configstore/migrations.go`:
- Around line 5390-5397: The migration is using nc.BaseURL for
tables.TableKey.Name which must be globally unique; change it to a synthetic
unique name derived from the provider and the generated key id (use the same
uuid.NewString() value assigned to KeyID) — e.g. construct Name from p.Name (or
p.ID) plus the KeyID so Name and KeyID are tied and guaranteed unique when
creating the tables.TableKey in the migration.
- Around line 5417-5445: The migration currently clears nc.BaseURL (nc.BaseURL =
"") which irreversibly removes the legacy provider URL and leaves the provider's
config_hash stale; instead, preserve the original BaseURL value and move it into
the new key column(s) (e.g., create/update tables.TableKey rows or the
ollama_url/sgl_url columns) while also updating network_config_json and
recomputing/updating the provider's config_hash in the same transaction; ensure
the Rollback restores nc.BaseURL from the new column(s) and recomputes the
original config_hash so older binaries keep reading network_config.base_url
(refer to nc.BaseURL, network_config_json, config_hash, Rollback and the code
paths that create/update tables.TableKey or ollama_url/sgl_url).
---
Outside diff comments:
In `@ui/lib/schemas/providerForm.ts`:
- Around line 228-246: The current base URL validation uses isCustomProvider and
wrongly requires provider-level networkConfig.base_url for aliases whose
effective base provider is vllm, ollama, or sgl; compute the effective provider
as const effective = data.baseProviderType ?? data.selectedProvider and set
baseURLRequired to true only when isCustomProvider is true AND effective is NOT
one of ['vllm','ollama','sgl']; update the conditional that adds issues for
networkConfig.base_url and the regex check to use this new baseURLRequired, and
apply the same effective-provider-based check to the related keyless/API-key
gating logic elsewhere so those gates skip provider-level base_url when the
effective provider resolves to vllm/ollama/sgl.
---
Duplicate comments:
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 90-92: The gating logic uses provider.name instead of the
effective base provider type, so derive a single boolean like
isPerKeyURLProvider from (provider.custom_provider_config?.base_provider_type ??
provider.name) and use that value in place of the current checks: set
requiresBaseUrl = !isPerKeyURLProvider (or equivalent) and reuse the same
boolean for hideBaseURL; update the onSubmit validation (where requiresBaseUrl
is computed) and the hideBaseURL calculation to reference this new derived
identifier so providers backed by vllm/ollama/sgl correctly skip base_url
validation.
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 36-41: The code is using provider.name (the configured alias) to
decide vllm/ollama/sgl behavior, so compute an effective base provider name
(e.g. const effectiveProviderName = (provider.baseProvider ??
provider.name)?.toLowerCase()) and use that for isVLLM, isOllamaOrSGL,
entityLabel and entityLabelPlural in ModelProviderKeysTableView; also pass the
effective base provider into the key sheet component/prop(s) where providerName
(or provider) is forwarded (the same change needed in the other usage around the
later block currently using providerName), so provider-specific sheet logic
receives the actual backing provider (vllm/ollama/sgl) rather than the alias.
---
Nitpick comments:
In `@core/providers/ollama/ollama.go`:
- Around line 67-119: The per-key URL helpers getBaseURL, baseURLOrError and the
per-key fan-out listModelsByKey are Ollama-specific glue that should be moved
into the shared OpenAI-compatible layer (openai package or provider utils) so
other OpenAI-compatible providers (like SGL) can reuse them; create
exported/shared functions (e.g., openai.GetBaseURL(key),
openai.BaseURLOrError(key), and openai.ListModelsByKey(ctx, client, baseURLPath,
key, request, extraHeaders, providerKey, sendRawReq, sendRawResp)) that
encapsulate the URL-resolving and per-key request logic, update
OllamaProvider.ListModels to delegate to those new openai/utils functions and
remove getBaseURL, baseURLOrError, and listModelsByKey from ollama.go, and
ensure existing calls to providerUtils.HandleMultipleListModelsRequests or
similar are replaced to call the shared implementations so SGL and other
providers can call the same helpers.
In `@transports/bifrost-http/server/server.go`:
- Around line 510-511: The hasNoKeys calculation is using a local
isKeylessProvider boolean while key-requirement logic elsewhere uses the shared
predicate in core/utils.go, which can cause drift; update this code to derive
keylessness from the same shared helper used at request-time (rather than
directly reading providerInfo.CustomProviderConfig.IsKeyLess) so both
reload/model-discovery and request-time checks match; replace the
isKeylessProvider/local logic and set hasNoKeys by calling the shared helper
(the same function used in core/utils.go) that encapsulates the Bedrock
exception semantics for keyless providers (ensure you reference the shared
helper name when making the change so providerInfo handling and hasNoKeys are
unified).
🪄 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: 3f928e10-7b83-4482-bded-5516f1bd7ba9
📒 Files selected for processing (25)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (3)
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- core/providers/sgl/sgl.go
- transports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- core/utils.go
- core/bifrost.go
- ui/lib/types/config.ts
- framework/configstore/rdb.go
- docs/openapi/schemas/management/providers.yaml
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/provider_keys.go
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
c4028da to
b1dd342
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/openapi/openapi.json (2)
143637-143640:⚠️ Potential issue | 🟠 MajorKeep
allow_on_all_virtual_keysscoped to client availability, not tool permissions.These descriptions are still inconsistent, and some copies explicitly say “All tools are allowed by default.” That overstates the flag’s effect: it should only grant default MCP-client availability to virtual keys, while tool execution remains gated by
tools_to_execute/tools_to_auto_executeand any explicit per-virtual-key config.✏️ Suggested replacement text for each repeated block
"allow_on_all_virtual_keys": { "type": "boolean", "default": false, - "description": "When true, this MCP client's tools are accessible to all virtual keys without requiring\nexplicit per-key assignment. All tools are allowed by default. If a virtual key has an\nexplicit MCP config for this client, that config takes precedence and overrides this behaviour.\n" + "description": "When true, this MCP client is available to all virtual keys by default. Tool execution remains governed by tools_to_execute and tools_to_auto_execute, and any explicit virtual-key MCP config for this client takes precedence.\n" }Also applies to: 143909-143912, 144167-144170, 144540-144543, 201657-201660, 201818-201821
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 143637 - 143640, The description for the allow_on_all_virtual_keys OpenAPI field incorrectly implies it grants tool permissions; update every repeated block containing "allow_on_all_virtual_keys" (e.g., the occurrences around the provided ranges) to clearly state it only grants default MCP-client availability to virtual keys and does NOT change tool execution permissions — tool access remains governed by tools_to_execute / tools_to_auto_execute and any explicit per-virtual-key MCP config which still takes precedence; make the wording concise and consistent across all occurrences.
134595-134621:⚠️ Potential issue | 🟠 MajorVerify
model_nameis actually required forvllm_key_config.This stack is about Ollama/SGL per-deployment URLs, but every regenerated
vllm_key_configblock now hard-requiresmodel_nametoo. If the VLLM runtime still only requiresurl, the OpenAPI contract will reject valid payloads and generated clients will enforce an unnecessary breaking field. Please fix this in the source schema that generates these repeated blocks, not just in the bundled JSON.Run this to confirm the runtime/source-schema contract before changing the generated spec:
#!/bin/bash set -euo pipefail echo "=== Go struct for VLLM key config ===" rg -n -A12 -B2 'type VLLMKeyConfig struct' --type go echo echo "=== VLLM runtime validation sites ===" rg -n -A6 -B6 'baseURLOrError|ModelName|URL' core/providers/vllm --type go echo echo "=== Source OpenAPI schema that generates the bundled JSON ===" rg -n -A14 -B3 'VllmKeyConfig|vllm_key_config|model_name|url' docs/openapi/schemas/management/providers.yamlExpected result: if the Go/runtime contract only errors on missing
url, dropmodel_namefrom therequiredarray in the source schema and regeneratedocs/openapi/openapi.json.🛠️ If verification confirms `model_name` is optional, this is the schema change to make at the source
"required": [ - "url", - "model_name" + "url" ]Based on learnings,
docs/openapi/openapi.jsonis an inlined generated spec and should stay aligned with the ref-based source schema.Also applies to: 135365-135391, 136021-136047, 136964-136990, 137876-137902, 138532-138558, 139389-139415, 140289-140315, 172954-172980, 198557-198583, 199224-199250, 199894-199920, 200514-200540, 201128-201154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/openapi.json` around lines 134595 - 134621, The OpenAPI bundle marks vllm_key_config.model_name as required but the runtime Go type and validation likely only require url; locate the source schema docs/openapi/schemas/management/providers.yaml and the Go struct type VLLMKeyConfig (search for "type VLLMKeyConfig struct") and confirm runtime validation sites under core/providers/vllm reference URL vs ModelName; if model_name is optional, remove "model_name" from the required array in the source YAML (and update any schema refs) and then regenerate docs/openapi/openapi.json so the inlined vllm_key_config blocks no longer force model_name as required across the generated spec.
🧹 Nitpick comments (1)
docs/openapi/schemas/management/providers.yaml (1)
146-149: Consider adding field-level descriptions for new key-config properties.The new fields are clear structurally, but API docs will be more usable with property-level descriptions/examples (especially for env-backed
url).Suggested doc-only refinement
VllmKeyConfig: type: object description: VLLM-specific key configuration properties: url: $ref: '../../schemas/management/common.yaml#/EnvVar' + description: Base URL for this vLLM server (supports env:VAR_NAME format) model_name: type: string + description: Exact model name served by this vLLM deployment required: - url - model_name OllamaKeyConfig: type: object description: Ollama-specific key configuration properties: url: $ref: '../../schemas/management/common.yaml#/EnvVar' + description: Base URL for this Ollama server (supports env:VAR_NAME format) required: - url SglKeyConfig: type: object description: SGLang-specific key configuration properties: url: $ref: '../../schemas/management/common.yaml#/EnvVar' + description: Base URL for this SGLang server (supports env:VAR_NAME format) required: - urlAlso applies to: 158-159, 167-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/schemas/management/providers.yaml` around lines 146 - 149, Add field-level descriptions and examples for the new properties to improve API docs: update the url property (which currently $ref's ../../schemas/management/common.yaml#/EnvVar) to include a clear description that it is an environment-backed service endpoint and provide an example/env var name; update the model_name property to include a short description (e.g., "name of the provider model") and an example value. Apply the same description+example pattern to the other occurrences of url/model_name in this schema (the other blocks around the url/model_name pairs).
🤖 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/tables/key.go`:
- Around line 347-359: The code clears k.OllamaUrl/k.SGLUrl when URL.GetValue()
is empty (unresolved env), which drops the original env reference; instead check
the raw EnvVar.Value() (or presence of URL.Value()) before nil-ing so env-backed
references are preserved for GORM. Concretely, in the blocks that inspect
k.OllamaKeyConfig.URL and k.SGLKeyConfig.URL use URL.Value() (or test that
URL.Value() != "") to decide whether to set k.OllamaUrl/k.SGLUrl to &u or nil,
so unresolved GetValue() doesn't cause you to overwrite the stored env var
reference; keep the same variables (k.OllamaKeyConfig, k.OllamaKeyConfig.URL,
k.OllamaUrl, k.SGLKeyConfig, k.SGLKeyConfig.URL, k.SGLUrl) when locating the
change.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 192-302: The allow/block model fields for keys are being hidden by
the wrapper condition that checks both isVLLM and isKeylessProvider; since
Ollama and SGL still persist key.models and key.blacklisted_models, remove the
isVLLM check and only hide these controls for keyless providers. Locate the JSX
wrapper using the variables isVLLM and isKeylessProvider that encloses the
FormField components for names "key.models" and "key.blacklisted_models" and
change the condition from {!isVLLM && !isKeylessProvider} to
{!isKeylessProvider} (or an explicit allow-list of providers that persist these
fields) so the ModelMultiselect blocks remain visible for Ollama/SGL.
- Around line 684-707: The visible server URL field never gets field-level
validation because the parent key config (ollama_key_config / sgl_key_config)
can be undefined; seed that object in the form state when isKeylessProvider is
true so the field exists before submit validation. In this component, add a
useEffect that checks isKeylessProvider and calls the form's setValue for
`key.ollama_key_config` or `key.sgl_key_config` (depending on isOllama) to an
object (e.g., { url: "" }) if it's currently undefined, ensuring the FormField
name `key.${isOllama ? "ollama_key_config" : "sgl_key_config"}.url` is present
and will surface validation errors for EnvVarInput before submit.
---
Duplicate comments:
In `@docs/openapi/openapi.json`:
- Around line 143637-143640: The description for the allow_on_all_virtual_keys
OpenAPI field incorrectly implies it grants tool permissions; update every
repeated block containing "allow_on_all_virtual_keys" (e.g., the occurrences
around the provided ranges) to clearly state it only grants default MCP-client
availability to virtual keys and does NOT change tool execution permissions —
tool access remains governed by tools_to_execute / tools_to_auto_execute and any
explicit per-virtual-key MCP config which still takes precedence; make the
wording concise and consistent across all occurrences.
- Around line 134595-134621: The OpenAPI bundle marks vllm_key_config.model_name
as required but the runtime Go type and validation likely only require url;
locate the source schema docs/openapi/schemas/management/providers.yaml and the
Go struct type VLLMKeyConfig (search for "type VLLMKeyConfig struct") and
confirm runtime validation sites under core/providers/vllm reference URL vs
ModelName; if model_name is optional, remove "model_name" from the required
array in the source YAML (and update any schema refs) and then regenerate
docs/openapi/openapi.json so the inlined vllm_key_config blocks no longer force
model_name as required across the generated spec.
---
Nitpick comments:
In `@docs/openapi/schemas/management/providers.yaml`:
- Around line 146-149: Add field-level descriptions and examples for the new
properties to improve API docs: update the url property (which currently $ref's
../../schemas/management/common.yaml#/EnvVar) to include a clear description
that it is an environment-backed service endpoint and provide an example/env var
name; update the model_name property to include a short description (e.g., "name
of the provider model") and an example value. Apply the same description+example
pattern to the other occurrences of url/model_name in this schema (the other
blocks around the url/model_name pairs).
🪄 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: ffb4a2dc-4c71-467d-9656-c151ae15b4f5
📒 Files selected for processing (25)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (2)
- transports/config.schema.json
- transports/bifrost-http/lib/config.go
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- framework/configstore/clientconfig.go
- ui/lib/schemas/providerForm.ts
- core/bifrost.go
- core/utils.go
- framework/configstore/rdb.go
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- ui/lib/types/config.ts
- core/providers/sgl/sgl.go
- ui/lib/types/schemas.ts
- transports/bifrost-http/handlers/provider_keys.go
- ui/app/workspace/providers/fragments/networkFormFragment.tsx
- framework/configstore/migrations.go
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
b1dd342 to
04a791d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/provider_keys.go (1)
418-430:⚠️ Potential issue | 🟠 MajorA masked URL can still overwrite the real server URL.
mergeUpdatedKeyonly restores Ollama/SGL URLs on an exact redacted-value match, butvalidateProviderKeyURLaccepts any other non-empty masked string. Sending a placeholder like"****"will persist that mask and break routing on the next request. After merge, rejectIsRedacted()URLs unless they were converted back to the raw value.Also applies to: 457-470
🤖 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 418 - 430, The merge logic currently can leave a masked URL (e.g. "****") in mergedKey if the incoming masked value doesn't exactly match the redacted-old value; to fix this, after merging OllamaKeyConfig/SGLKeyConfig in mergeUpdatedKey, check mergedKey.OllamaKeyConfig.URL.IsRedacted() and mergedKey.SGLKeyConfig.URL.IsRedacted() and reject (return an error) any redacted URL that was not restored from oldRawKey (i.e., only allow redacted values if you already replaced them with oldRawKey.URL); alternatively enforce this in validateProviderKeyURL by returning an error for any IsRedacted() URL that does not equal the old raw value. Reference symbols: mergeUpdatedKey, validateProviderKeyURL, updateKey, oldRedactedKey, oldRawKey, mergedKey, OllamaKeyConfig, SGLKeyConfig, URL.IsRedacted().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/openapi.json`:
- Around line 134595-134621: The OpenAPI schema for vllm_key_config incorrectly
lists "model_name" as required; remove "model_name" from the "required" array so
only "url" remains required (i.e., required: ["url"]), and apply this change to
all 14 occurrences of the vllm_key_config object in openapi.json (each block
containing "vllm_key_config", "url", and "model_name"); this matches the Go
struct's optional ModelName routing hint and runtime enforcement via
baseURLOrError.
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 96-99: The empty-value check is using the route provider name
instead of the resolved base provider, causing Bedrock-backed custom providers
to incorrectly fail; before calling bifrost.CanProviderKeyValueBeEmpty(provider)
use providerConfig.CustomProviderConfig.BaseProviderType when present (fall back
to the original provider otherwise) so the check is against the actual base
provider type, then keep the SendError behavior; apply the same change in
updateProviderKey (and the analogous checks around the 202-205 area) so both
create and update flows resolve base provider via
providerConfig.CustomProviderConfig.BaseProviderType before calling
CanProviderKeyValueBeEmpty.
- Around line 457-470: The validator currently rejects Ollama/SGL when
URL.GetValue() is empty; change validateProviderKeyURL to accept either a
literal URL or an env-var reference by verifying the URL config object exists
and that it either has a non-empty resolved value OR is specified as an env-var
reference. Concretely, for OllamaKeyConfig.URL and SGLKeyConfig.URL ensure you
check (URL.GetValue() != "" || URL.GetFromEnv() == true || URL.GetEnvVar() !=
"") (or the equivalent fields/methods present on the URL proto/struct:
FromEnv/EnvVar) instead of only URL.GetValue(), so unresolved env-var references
pass validation.
In `@transports/bifrost-http/lib/config.go`:
- Around line 3367-3407: The code is mutating live provider configs by assigning
pointers (e.g., configStoreKey.AzureKeyConfig = key.AzureKeyConfig) then calling
Redacted(), which writes masked values back into c.Providers under an RLock; fix
by deep-copying each provider config before redacting and assigning the copy to
configStoreKey so the original pointer is untouched — for AzureKeyConfig,
BedrockKeyConfig, VertexKeyConfig, VLLMKeyConfig (and similarly
OllamaKeyConfig/SGLKeyConfig), create a local copy (e.g., cfg :=
*key.AzureKeyConfig), redact fields on cfg (cfg.Endpoint = ...Redacted(), etc.),
then set configStoreKey.AzureKeyConfig = &cfg; do not assign
key.<Provider>KeyConfig directly.
- Around line 798-799: The reconciliation bug is due to GenerateKeyHash omitting
per-key backend URL fields so keys with identical API key/models but different
Ollama/SGL backends collide; update the GenerateKeyHash implementation to
include the relevant URL fields from schemas.Key (e.g.,
OllamaKeyConfig.ServerURL and SGLKeyConfig.ServerURL or the equivalent fields
present on OllamaKeyConfig/SGLKeyConfig) when building the hash input, ensuring
any call sites that construct a temporary schemas.Key (like the places passing
OllamaKeyConfig and SGLKeyConfig shown) now produce unique hashes for distinct
backend URLs; update or add tests to assert two keys differing only by these
server URLs produce different hashes/IDs.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 418-430: The merge logic currently can leave a masked URL (e.g.
"****") in mergedKey if the incoming masked value doesn't exactly match the
redacted-old value; to fix this, after merging OllamaKeyConfig/SGLKeyConfig in
mergeUpdatedKey, check mergedKey.OllamaKeyConfig.URL.IsRedacted() and
mergedKey.SGLKeyConfig.URL.IsRedacted() and reject (return an error) any
redacted URL that was not restored from oldRawKey (i.e., only allow redacted
values if you already replaced them with oldRawKey.URL); alternatively enforce
this in validateProviderKeyURL by returning an error for any IsRedacted() URL
that does not equal the old raw value. Reference symbols: mergeUpdatedKey,
validateProviderKeyURL, updateKey, oldRedactedKey, oldRawKey, mergedKey,
OllamaKeyConfig, SGLKeyConfig, URL.IsRedacted().
🪄 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: 75216539-c868-4f69-a8a4-1e3c1dacf5ce
📒 Files selected for processing (25)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (3)
- ui/lib/types/config.ts
- core/providers/ollama/ollama.go
- framework/configstore/migrations.go
🚧 Files skipped from review as they are similar to previous changes (11)
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- ui/lib/schemas/providerForm.ts
- framework/configstore/clientconfig.go
- transports/bifrost-http/server/server.go
- core/utils.go
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- core/bifrost.go
- ui/app/workspace/providers/fragments/networkFormFragment.tsx
- framework/configstore/rdb.go
- core/providers/sgl/sgl.go
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
04a791d to
d7fcc70
Compare
Contributor
There was a problem hiding this comment.
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)
ui/app/workspace/providers/fragments/networkFormFragment.tsx (1)
133-147:⚠️ Potential issue | 🟠 MajorReset
enforce_http2when the provider changes.This reset payload omits
network_config.enforce_http2, so the toggle can keep the previous provider’s value/default and a later save will write the wrong setting for the newly selected provider.🛠️ Proposed fix
form.reset({ network_config: { base_url: provider.network_config?.base_url || undefined, extra_headers: provider.network_config?.extra_headers, default_request_timeout_in_seconds: provider.network_config?.default_request_timeout_in_seconds ?? DefaultNetworkConfig.default_request_timeout_in_seconds, max_retries: provider.network_config?.max_retries ?? DefaultNetworkConfig.max_retries, retry_backoff_initial: provider.network_config?.retry_backoff_initial ?? DefaultNetworkConfig.retry_backoff_initial, retry_backoff_max: provider.network_config?.retry_backoff_max ?? DefaultNetworkConfig.retry_backoff_max, insecure_skip_verify: provider.network_config?.insecure_skip_verify ?? DefaultNetworkConfig.insecure_skip_verify, ca_cert_pem: provider.network_config?.ca_cert_pem ?? DefaultNetworkConfig.ca_cert_pem, stream_idle_timeout_in_seconds: provider.network_config?.stream_idle_timeout_in_seconds ?? DefaultNetworkConfig.stream_idle_timeout_in_seconds, max_conns_per_host: provider.network_config?.max_conns_per_host ?? DefaultNetworkConfig.max_conns_per_host, + enforce_http2: provider.network_config?.enforce_http2 ?? DefaultNetworkConfig.enforce_http2, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx` around lines 133 - 147, The reset payload in form.reset is missing network_config.enforce_http2, which causes the toggle to retain the previous provider's value; update the reset call in networkFormFragment.tsx to include network_config.enforce_http2 set to provider.network_config?.enforce_http2 ?? DefaultNetworkConfig.enforce_http2 so the toggle is correctly initialized when the provider changes (refer to the form.reset invocation, the network_config object, and DefaultNetworkConfig).
♻️ Duplicate comments (2)
transports/bifrost-http/handlers/provider_keys.go (2)
96-98:⚠️ Potential issue | 🟠 MajorResolve the base provider and use
EnvVar.IsDefined()for these key-value guards.These checks still validate against the route provider and
GetValue(). That breaks two supported cases: Bedrock-backed custom providers still hit the wrong empty-value rule, and unresolved env-var references are rejected even though the key is configured.🛠️ Proposed fix
+ baseProvider := provider + if providerConfig.CustomProviderConfig != nil && providerConfig.CustomProviderConfig.BaseProviderType != "" { + baseProvider = providerConfig.CustomProviderConfig.BaseProviderType + } - if !bifrost.CanProviderKeyValueBeEmpty(provider) && key.Value.GetValue() == "" { + if !bifrost.CanProviderKeyValueBeEmpty(baseProvider) && !key.Value.IsDefined() { SendError(ctx, fasthttp.StatusBadRequest, "Key value must not be empty") return }+ baseProvider := provider + if providerConfig.CustomProviderConfig != nil && providerConfig.CustomProviderConfig.BaseProviderType != "" { + baseProvider = providerConfig.CustomProviderConfig.BaseProviderType + } - if !bifrost.CanProviderKeyValueBeEmpty(provider) && mergedKey.Value.GetValue() == "" { + if !bifrost.CanProviderKeyValueBeEmpty(baseProvider) && !mergedKey.Value.IsDefined() { SendError(ctx, fasthttp.StatusBadRequest, "Key value must not be empty") return }Based on learnings,
EnvVar.IsDefined()should be used for configured-value checks, andaws bedrockis a supportedcustom_provider_config.base_provider_type.Also applies to: 202-205
🤖 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 96 - 98, The guard is checking the route-level provider and raw key.Value.GetValue(), causing Bedrock-backed custom providers and env-var-configured keys to be misvalidated; change the logic to resolve the base provider (use the provider's base_provider_type when available, e.g., handle custom_provider_config.base_provider_type == "aws bedrock") and replace GetValue() emptiness checks with EnvVar.IsDefined() for keys that may be configured via env vars: update the branch around bifrost.CanProviderKeyValueBeEmpty(provider) to compute the effective base provider and call bifrost.CanProviderKeyValueBeEmpty(effectiveProvider) and for value presence use key.Value.EnvVar.IsDefined() (falling back to GetValue() only when EnvVar is not used); apply the same pattern to the other occurrences referenced (around lines 202-205).
418-430:⚠️ Potential issue | 🟠 MajorDon’t let mismatched redacted URL placeholders overwrite the stored server URL.
If the client posts any redacted-looking Ollama/SGL URL that does not exactly match the current masked value, this merge leaves it intact and the later
validateProviderKeyURLcall still accepts it viaIsDefined(). That can replace the real URL with masked text on update.🛠️ Minimal fix
- if updateKey.OllamaKeyConfig != nil && oldRedactedKey.OllamaKeyConfig != nil && oldRawKey.OllamaKeyConfig != nil { - if updateKey.OllamaKeyConfig.URL.IsRedacted() && - updateKey.OllamaKeyConfig.URL.Equals(&oldRedactedKey.OllamaKeyConfig.URL) { + if updateKey.OllamaKeyConfig != nil && oldRawKey.OllamaKeyConfig != nil { + if updateKey.OllamaKeyConfig.URL.IsRedacted() { mergedKey.OllamaKeyConfig.URL = oldRawKey.OllamaKeyConfig.URL } } - if updateKey.SGLKeyConfig != nil && oldRedactedKey.SGLKeyConfig != nil && oldRawKey.SGLKeyConfig != nil { - if updateKey.SGLKeyConfig.URL.IsRedacted() && - updateKey.SGLKeyConfig.URL.Equals(&oldRedactedKey.SGLKeyConfig.URL) { + if updateKey.SGLKeyConfig != nil && oldRawKey.SGLKeyConfig != nil { + if updateKey.SGLKeyConfig.URL.IsRedacted() { mergedKey.SGLKeyConfig.URL = oldRawKey.SGLKeyConfig.URL } }🤖 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 418 - 430, The merge logic currently only restores the raw stored URL when the incoming redacted URL exactly equals oldRedactedKey.*, which allows other redacted-looking values to slip through and later be accepted by validateProviderKeyURL; update the condition in both the OllamaKeyConfig and SGLKeyConfig blocks so that if updateKey.<Provider>KeyConfig.URL.IsRedacted() is true you always restore the stored raw URL (mergedKey.<Provider>KeyConfig.URL = oldRawKey.<Provider>KeyConfig.URL) instead of preserving any redacted-looking input; keep the Equals check unnecessary — treat any IsRedacted() as needing replacement with oldRawKey to prevent masked text from overwriting the real URL.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config.go (1)
3357-3412: Deep-copy the returned key configs, not just the outer struct.This fixes the top-level redaction mutation, but
ReplicateKeyConfigis still returned by reference on Line 3396 and thecfg := *...copies for the other providers are still shallow. Nested maps/slices like deployments, scopes, or batch S3 config can still aliasc.Providers, so mutating the returnedTableKeycan still leak back into live config afterRLockis released. Please deep-copy the provider config payloads before assigning them intoconfigStoreKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 3357 - 3412, The code currently makes only shallow copies of provider configs (using cfg := *key.XKeyConfig and assigning key.ReplicateKeyConfig by reference), which allows nested slices/maps (deployments, scopes, batch S3, etc.) to alias the live config and be mutated after RLock is released; update the construction of configStoreKey so each provider config is deep-copied before assignment (deep-copy nested maps, slices, and pointer fields for AzureKeyConfig, BedrockKeyConfig, VertexKeyConfig, VLLMKeyConfig, OllamaKeyConfig, SGLKeyConfig and ReplicateKeyConfig) rather than assigning the original or a shallow copy, ensuring TableKey entries contain independent copies that cannot mutate c.Providers after the RLock is released.
🤖 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/providers/ollama/ollama.go`:
- Around line 69-86: The current logic treats an unset URL and a
configured-but-unresolved env var the same; update getBaseURL and baseURLOrError
to use EnvVar.IsDefined() on key.OllamaKeyConfig.URL instead of checking
GetValue() != "" so you can distinguish the two cases: in getBaseURL return the
trimmed GetValue() only when URL.IsDefined() is true (otherwise return ""), and
in baseURLOrError first check if URL.IsDefined() is false and return the
existing "no base URL configured" BifrostError, but if IsDefined() is true and
the returned value is empty return a different BifrostError indicating the URL
is configured via env var but not resolved (e.g., "configured but unresolved:
referenced env var is not set"), referencing OllamaProvider, getBaseURL,
baseURLOrError and key.OllamaKeyConfig.URL.IsDefined().
In `@transports/config.schema.json`:
- Around line 2215-2296: Add a new $defs entry named
"network_config_without_base_url" that duplicates the shape of the existing
"$defs/network_config" but omits the "base_url" property, then update the two
wrappers "provider_with_ollama_config" and "provider_with_sgl_config" to
reference "#/$defs/network_config_without_base_url" instead of
"#/$defs/network_config"; this enforces forbidding the legacy provider-level
base_url for Ollama and SGL while keeping all other network fields the same.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 73-80: The auth-tab detection currently only checks .value and
misses env-var-backed credentials; update the conditional logic that reads
form.getValues("key.azure_key_config.client_id"/"client_secret"/"tenant_id") and
form.getValues("key.value") to treat a field as configured if either
value?.trim() or env_var?.trim() is present (i.e., use value?.trim() ||
env_var?.trim()). Replace similar checks in the other related block (the checks
around lines 88-94) so env_var-backed keys are recognized and
setAzureAuthType("entra_id"/"default_credential"/"iam_role") correctly.
---
Outside diff comments:
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 133-147: The reset payload in form.reset is missing
network_config.enforce_http2, which causes the toggle to retain the previous
provider's value; update the reset call in networkFormFragment.tsx to include
network_config.enforce_http2 set to provider.network_config?.enforce_http2 ??
DefaultNetworkConfig.enforce_http2 so the toggle is correctly initialized when
the provider changes (refer to the form.reset invocation, the network_config
object, and DefaultNetworkConfig).
---
Duplicate comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 96-98: The guard is checking the route-level provider and raw
key.Value.GetValue(), causing Bedrock-backed custom providers and
env-var-configured keys to be misvalidated; change the logic to resolve the base
provider (use the provider's base_provider_type when available, e.g., handle
custom_provider_config.base_provider_type == "aws bedrock") and replace
GetValue() emptiness checks with EnvVar.IsDefined() for keys that may be
configured via env vars: update the branch around
bifrost.CanProviderKeyValueBeEmpty(provider) to compute the effective base
provider and call bifrost.CanProviderKeyValueBeEmpty(effectiveProvider) and for
value presence use key.Value.EnvVar.IsDefined() (falling back to GetValue() only
when EnvVar is not used); apply the same pattern to the other occurrences
referenced (around lines 202-205).
- Around line 418-430: The merge logic currently only restores the raw stored
URL when the incoming redacted URL exactly equals oldRedactedKey.*, which allows
other redacted-looking values to slip through and later be accepted by
validateProviderKeyURL; update the condition in both the OllamaKeyConfig and
SGLKeyConfig blocks so that if updateKey.<Provider>KeyConfig.URL.IsRedacted() is
true you always restore the stored raw URL (mergedKey.<Provider>KeyConfig.URL =
oldRawKey.<Provider>KeyConfig.URL) instead of preserving any redacted-looking
input; keep the Equals check unnecessary — treat any IsRedacted() as needing
replacement with oldRawKey to prevent masked text from overwriting the real URL.
---
Nitpick comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 3357-3412: The code currently makes only shallow copies of
provider configs (using cfg := *key.XKeyConfig and assigning
key.ReplicateKeyConfig by reference), which allows nested slices/maps
(deployments, scopes, batch S3, etc.) to alias the live config and be mutated
after RLock is released; update the construction of configStoreKey so each
provider config is deep-copied before assignment (deep-copy nested maps, slices,
and pointer fields for AzureKeyConfig, BedrockKeyConfig, VertexKeyConfig,
VLLMKeyConfig, OllamaKeyConfig, SGLKeyConfig and ReplicateKeyConfig) rather than
assigning the original or a shallow copy, ensuring TableKey entries contain
independent copies that cannot mutate c.Providers after the RLock is released.
🪄 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: 6d93a17b-89bc-4bee-b01d-b73d76356a31
📒 Files selected for processing (25)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (1)
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- transports/bifrost-http/server/server.go
- ui/lib/schemas/providerForm.ts
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- core/schemas/envvar.go
- core/providers/sgl/sgl.go
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- ui/lib/types/config.ts
- core/utils.go
- framework/configstore/tables/key.go
- docs/openapi/schemas/management/providers.yaml
- ui/lib/types/schemas.ts
- framework/configstore/migrations.go
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
d7fcc70 to
260b94e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/provider_keys.go (1)
428-440:⚠️ Potential issue | 🟠 MajorReject redacted URL placeholders that do not resolve back to the stored value.
The new Ollama/SGL merge blocks only restore the raw URL on an exact redacted-value match. Any other masked string survives into
mergedKey, andvalidateProviderKeyURL()accepts it becauseIsDefined()only checks presence. That still lets<redacted>-style placeholders overwrite a working backend URL on update, and it also accepts those placeholders on create.🛠️ Suggested direction
func validateProviderKeyURL(provider schemas.ModelProvider, key schemas.Key) error { + validateURL := func(field string, value schemas.EnvVar) error { + if !value.IsDefined() { + return fmt.Errorf("%s is required", field) + } + if !value.IsFromEnv() && value.IsRedacted() { + return fmt.Errorf("%s must not be redacted", field) + } + return nil + } + switch provider { case schemas.Ollama: - if key.OllamaKeyConfig == nil || !key.OllamaKeyConfig.URL.IsDefined() { + if key.OllamaKeyConfig == nil { return fmt.Errorf("ollama_key_config.url is required for Ollama keys") } + return validateURL("ollama_key_config.url", key.OllamaKeyConfig.URL) case schemas.SGL: - if key.SGLKeyConfig == nil || !key.SGLKeyConfig.URL.IsDefined() { + if key.SGLKeyConfig == nil { return fmt.Errorf("sgl_key_config.url is required for SGL keys") } + return validateURL("sgl_key_config.url", key.SGLKeyConfig.URL) } return nil }Because update validation runs on
mergedKey, exact redacted echoes from the UI will still work aftermergeUpdatedKey()restores the stored raw URL.Also applies to: 467-480
🤖 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 428 - 440, mergeUpdatedKey currently allows redacted URL placeholders that don't match the stored redacted value to pass through into mergedKey; add explicit rejection logic: in mergeUpdatedKey() when handling OllamaKeyConfig.URL and SGLKeyConfig.URL, detect if updateKey.<Provider>KeyConfig.URL.IsRedacted() is true and updateKey.<Provider>KeyConfig.URL.Equals(&oldRedactedKey.<Provider>KeyConfig.URL) is false, and return a validation error (or bubble an error) instead of leaving the placeholder in mergedKey; apply the same check for both OllamaKeyConfig and SGLKeyConfig (the blocks around the existing restoration code) so validateProviderKeyURL() runs against a legitimately restored or explicitly rejected URL.
🤖 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-104: The validation currently checks key.Value with GetValue()
== "" which incorrectly rejects unresolved env-var references; change the
presence check to use EnvVar.IsDefined() so keys like {from_env:true,
env_var:"..."} pass. Update the block using
bifrost.CanProviderKeyValueBeEmpty(baseProvider) to call EnvVar.IsDefined() on
key.Value (and similarly for mergedKey.Value in the later block referenced
around lines 212-215) and keep the existing SendError(fasthttp.StatusBadRequest,
"Key value must not be empty") behavior when EnvVar.IsDefined() returns false.
In `@transports/bifrost-http/lib/config.go`:
- Around line 3367-3412: GetAllKeys() currently does shallow copies (cfg :=
*key.AzureKeyConfig, etc.) which leaves nested mutable fields shared; deep-copy
nested slices/maps so snapshots are immutable after RUnlock. For each provider
config (AzureKeyConfig, BedrockKeyConfig, VertexKeyConfig, ReplicateKeyConfig,
VLLMKeyConfig, OllamaKeyConfig, SGLKeyConfig) allocate and copy nested fields:
copy BedrockKeyConfig.Deployments slice/map, BedrockKeyConfig.BatchS3Config and
BatchS3Config.Buckets, VertexKeyConfig.Deployments, any Scopes or map fields,
and ensure ReplicateKeyConfig is deep-copied (not assigned directly) including
its Deployments; replace lines that set configStoreKey.* = &cfg or assign
ReplicateKeyConfig with versions that perform element-wise copies of slices/maps
so no backing memory is shared.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/provider_keys.go`:
- Around line 428-440: mergeUpdatedKey currently allows redacted URL
placeholders that don't match the stored redacted value to pass through into
mergedKey; add explicit rejection logic: in mergeUpdatedKey() when handling
OllamaKeyConfig.URL and SGLKeyConfig.URL, detect if
updateKey.<Provider>KeyConfig.URL.IsRedacted() is true and
updateKey.<Provider>KeyConfig.URL.Equals(&oldRedactedKey.<Provider>KeyConfig.URL)
is false, and return a validation error (or bubble an error) instead of leaving
the placeholder in mergedKey; apply the same check for both OllamaKeyConfig and
SGLKeyConfig (the blocks around the existing restoration code) so
validateProviderKeyURL() runs against a legitimately restored or explicitly
rejected URL.
🪄 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: b21f70df-6518-430e-954e-6870b3556f47
📒 Files selected for processing (25)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (3)
- framework/configstore/tables/key.go
- transports/config.schema.json
- core/providers/sgl/sgl.go
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- ui/lib/schemas/providerForm.ts
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- transports/bifrost-http/server/server.go
- core/schemas/envvar.go
- core/utils.go
- framework/configstore/rdb.go
- core/bifrost.go
- framework/configstore/clientconfig.go
- ui/lib/types/config.ts
- docs/openapi/openapi.json
- core/providers/ollama/ollama.go
- framework/configstore/migrations.go
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
260b94e to
a92bac0
Compare
Contributor
There was a problem hiding this comment.
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 (2)
ui/app/workspace/providers/fragments/networkFormFragment.tsx (1)
131-149:⚠️ Potential issue | 🟠 Major
form.reset()is still droppingenforce_http2.The initial defaults and submit path both carry this field, but the provider-change reset omits it. After switching providers, the toggle falls back to
false, and saving any unrelated network change will silently clear a provider that had HTTP/2 forced on.🧩 Minimal fix
form.reset({ network_config: { base_url: provider.network_config?.base_url || undefined, extra_headers: provider.network_config?.extra_headers, default_request_timeout_in_seconds: provider.network_config?.default_request_timeout_in_seconds ?? DefaultNetworkConfig.default_request_timeout_in_seconds, max_retries: provider.network_config?.max_retries ?? DefaultNetworkConfig.max_retries, retry_backoff_initial: provider.network_config?.retry_backoff_initial ?? DefaultNetworkConfig.retry_backoff_initial, retry_backoff_max: provider.network_config?.retry_backoff_max ?? DefaultNetworkConfig.retry_backoff_max, insecure_skip_verify: provider.network_config?.insecure_skip_verify ?? DefaultNetworkConfig.insecure_skip_verify, ca_cert_pem: provider.network_config?.ca_cert_pem ?? DefaultNetworkConfig.ca_cert_pem, stream_idle_timeout_in_seconds: provider.network_config?.stream_idle_timeout_in_seconds ?? DefaultNetworkConfig.stream_idle_timeout_in_seconds, max_conns_per_host: provider.network_config?.max_conns_per_host ?? DefaultNetworkConfig.max_conns_per_host, + enforce_http2: provider.network_config?.enforce_http2 ?? DefaultNetworkConfig.enforce_http2, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx` around lines 131 - 149, The provider-change reset in useEffect for form.reset is missing the enforce_http2 field, so when provider.name or provider.network_config changes the enforce_http2 toggle is lost; update the reset payload to include enforce_http2: provider.network_config?.enforce_http2 ?? DefaultNetworkConfig.enforce_http2 so the form preserves the provider's HTTP/2 setting (look for useEffect, form.reset, provider.network_config, and DefaultNetworkConfig in networkFormFragment.tsx).ui/lib/types/schemas.ts (1)
219-233:⚠️ Potential issue | 🟡 MinorAccept env-var-backed key values in the fallback branch.
The new Ollama/SGL path correctly treats
env_varas configured, but the generic fallback still only acceptsvalue.value. That rejects valid inputs like{ env_var: "OPENAI_API_KEY" }on the client side.🐛 Suggested fix
- return data.value?.value && data.value?.value?.length > 0; + return !!data.value?.value?.trim() || !!data.value?.env_var?.trim();Based on learnings: When validating whether a
schemas.EnvVarfield has been configured by the user in maximhq/bifrost, useEnvVar.IsDefined()instead of checkingGetValue() != "".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/schemas.ts` around lines 219 - 233, The refine validator currently only accepts explicit value strings (checks data.value?.value) and rejects env-var-backed keys; update the fallback branch inside the .refine(...) closure to treat an EnvVar as configured by calling the EnvVar.IsDefined(...) helper (e.g., use schemas.EnvVar.IsDefined(data.value) or similar) instead of checking GetValue()/data.value?.value so that inputs like { env_var: "OPENAI_API_KEY" } pass validation; change the return expression in the fallback branch to return true when EnvVar.IsDefined(data.value).
♻️ Duplicate comments (1)
ui/app/workspace/providers/fragments/networkFormFragment.tsx (1)
91-92:⚠️ Potential issue | 🟠 MajorUse the effective base provider for base-URL rules.
This regressed to name-based checks. A custom provider backed by
ollama,sgl, orvllmwill still show and requirenetwork_config.base_url, even though this stack now resolves those URLs per key. That makes valid configs impossible to save from this tab.🛠️ Minimal fix
+ const effectiveProviderName = provider.custom_provider_config?.base_provider_type ?? provider.name; + const isPerKeyURLProvider = + effectiveProviderName === "vllm" || effectiveProviderName === "ollama" || effectiveProviderName === "sgl"; + const onSubmit = (data: NetworkOnlyFormSchema) => { - const requiresBaseUrl = isCustomProvider; + const requiresBaseUrl = isCustomProvider && !isPerKeyURLProvider; if (requiresBaseUrl && (data.network_config?.base_url ?? "").trim() === "") { ... } }; - const baseURLRequired = isCustomProvider; - const hideBaseURL = provider.name === "vllm" || provider.name === "ollama" || provider.name === "sgl"; + const baseURLRequired = isCustomProvider && !isPerKeyURLProvider; + const hideBaseURL = isPerKeyURLProvider;Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx` around lines 91 - 92, The current validation uses isCustomProvider/name-based checks (requiresBaseUrl = isCustomProvider and checking data.network_config?.base_url) which wrongly requires base_url for providers that resolve URLs per key (e.g., ollama, sgl, vllm); update the logic to use the effective base provider resolution instead of the raw provider name—replace the isCustomProvider usage in the base-URL requirement (the requiresBaseUrl check and the subsequent base_url trim validation) with a check against the resolved/effective provider result (e.g., call the function or property that returns the effective provider for the given key) so only providers that actually need network_config.base_url are enforced; apply the same change for the duplicate instance at the later check (lines referenced 151-152).
🧹 Nitpick comments (4)
core/providers/sgl/sgl.go (1)
196-220: Minor: Inconsistent use ofschemas.SGLvsprovider.GetProviderKey().Line 211 uses the constant
schemas.SGLdirectly, while other methods (e.g.,ChatCompletionat line 185,Embeddingat line 261) useprovider.GetProviderKey(). Consider usingprovider.GetProviderKey()for consistency, which also future-proofs against provider aliasing.♻️ Suggested fix
return openai.HandleOpenAIChatCompletionStreaming( ctx, provider.client, baseURL+providerUtils.GetPathFromContext(ctx, "/v1/chat/completions"), request, nil, provider.networkConfig.ExtraHeaders, providerUtils.ShouldSendBackRawRequest(ctx, provider.sendBackRawRequest), providerUtils.ShouldSendBackRawResponse(ctx, provider.sendBackRawResponse), - schemas.SGL, + provider.GetProviderKey(), postHookRunner, nil, nil, nil, nil, nil, provider.logger, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/sgl/sgl.go` around lines 196 - 220, In ChatCompletionStream, replace the hard-coded schemas.SGL argument with provider.GetProviderKey() to match the pattern used in ChatCompletion and Embedding; locate the ChatCompletionStream function and pass provider.GetProviderKey() in place of schemas.SGL when calling openai.HandleOpenAIChatCompletionStreaming so the provider key is consistent and resilient to aliasing.framework/configstore/rdb.go (1)
366-387: PrefertableKeyFromSchemaKey()in the remaining key write paths.These three constructors still hand-roll
tables.TableKey, which is exactly how the new Ollama/SGL fields were missed earlier. Reusing the helper here would keep future provider-specific key fields, Bedrock JSON handling, and other mapping changes from drifting again.♻️ Refactor sketch
- 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, - OllamaKeyConfig: key.OllamaKeyConfig, - SGLKeyConfig: key.SGLKeyConfig, - ConfigHash: keyHash, - Status: string(key.Status), - Description: key.Description, - } + dbKey, err := tableKeyFromSchemaKey(dbProvider, key) + if err != nil { + return err + } + dbKey.ConfigHash = keyHashAlso applies to: 539-560, 664-685
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 366 - 387, Replace the hand-constructed tables.TableKey literal with the shared helper tableKeyFromSchemaKey so provider-specific fields (Ollama/SGL, Bedrock JSON handling, etc.) and future mappings stay consistent: call tableKeyFromSchemaKey with the schema key, dbProvider and keyHash instead of manually building the tables.TableKey (ensure the resulting TableKey preserves Weight pointer, Enabled, UseForBatchAPI and Status/Description semantics), and apply the same replacement to the other manual tables.TableKey constructions in this file so all key write paths reuse the helper.framework/configstore/migrations.go (2)
5084-5098: UseschemaKeyFromTableKeyfor this hash backfill.This hand-built
schemas.Keyalready needed two more fields in this PR. Reusing the shared helper here avoids the next key-config addition silently drifting this migration’s hash input.♻️ Suggested simplification
- schemaKey := schemas.Key{ - Name: key.Name, - Value: key.Value, - Models: key.Models, - Weight: getWeight(key.Weight), - AzureKeyConfig: key.AzureKeyConfig, - VertexKeyConfig: key.VertexKeyConfig, - BedrockKeyConfig: key.BedrockKeyConfig, - ReplicateKeyConfig: key.ReplicateKeyConfig, - VLLMKeyConfig: key.VLLMKeyConfig, - OllamaKeyConfig: key.OllamaKeyConfig, - SGLKeyConfig: key.SGLKeyConfig, - Enabled: key.Enabled, - UseForBatchAPI: key.UseForBatchAPI, - } + schemaKey := schemaKeyFromTableKey(key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5084 - 5098, The migration builds a schemas.Key manually which risks missing future fields; replace the manual construction with the shared helper schemaKeyFromTableKey (use schemaKeyFromTableKey(key) or the helper's actual signature) so the hash/backfill uses the canonical field set for schemas.Key; update any necessary imports and ensure getWeight usage is preserved by calling the helper instead of the inline struct in this migration function.
5438-5445: Preferbifrost.Ptr(...)for the new pointer fields.This repo uses
bifrost.Ptr()instead of taking the address of locals. Keeping the migration consistent avoids introducing another pointer-construction style here.🧹 Suggested cleanup
- enabled := true - weight := 1.0 newKey := tables.TableKey{ Provider: p.Name, ProviderID: p.ID, KeyID: uuid.NewString(), - Weight: &weight, - Enabled: &enabled, + Weight: bifrost.Ptr(1.0), + Enabled: bifrost.Ptr(true), Models: schemas.WhiteList{"*"}, }Based on learnings: In the maximhq/bifrost repository, prefer using
bifrost.Ptr()to create pointers instead of the address operator (&) even when&would be valid syntactically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 5438 - 5445, The migration creates pointer fields using the address-of operator for local vars (enabled := true, weight := 1.0) when constructing tables.TableKey; replace those with the repository convention bifrost.Ptr(...) (e.g., Weight: bifrost.Ptr(weight), Enabled: bifrost.Ptr(enabled)) so the migration follows existing pointer-construction style used elsewhere (update the construction in the block that builds newKey in migrations.go).
🤖 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/apiKeysFormFragment.tsx`:
- Around line 58-61: The keyless detection uses the raw providerName (variables
isOllama/isSGL/isKeylessProvider and supportsBatchAPI) which misses custom
providers that are backed by ollama/sgl; update the logic to resolve the
effective/base provider first (e.g., derive effectiveBaseProvider =
provider?.baseProvider || providerName or call a helper like
resolveEffectiveProvider(providerName/provider)) and then compute
isOllama/isSGL/isKeylessProvider and supportsBatchAPI from that
effectiveBaseProvider so the UI will render the correct key vs *.url fields
(apply same change where those symbols are used elsewhere in this fragment).
---
Outside diff comments:
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 131-149: The provider-change reset in useEffect for form.reset is
missing the enforce_http2 field, so when provider.name or
provider.network_config changes the enforce_http2 toggle is lost; update the
reset payload to include enforce_http2: provider.network_config?.enforce_http2
?? DefaultNetworkConfig.enforce_http2 so the form preserves the provider's
HTTP/2 setting (look for useEffect, form.reset, provider.network_config, and
DefaultNetworkConfig in networkFormFragment.tsx).
In `@ui/lib/types/schemas.ts`:
- Around line 219-233: The refine validator currently only accepts explicit
value strings (checks data.value?.value) and rejects env-var-backed keys; update
the fallback branch inside the .refine(...) closure to treat an EnvVar as
configured by calling the EnvVar.IsDefined(...) helper (e.g., use
schemas.EnvVar.IsDefined(data.value) or similar) instead of checking
GetValue()/data.value?.value so that inputs like { env_var: "OPENAI_API_KEY" }
pass validation; change the return expression in the fallback branch to return
true when EnvVar.IsDefined(data.value).
---
Duplicate comments:
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 91-92: The current validation uses isCustomProvider/name-based
checks (requiresBaseUrl = isCustomProvider and checking
data.network_config?.base_url) which wrongly requires base_url for providers
that resolve URLs per key (e.g., ollama, sgl, vllm); update the logic to use the
effective base provider resolution instead of the raw provider name—replace the
isCustomProvider usage in the base-URL requirement (the requiresBaseUrl check
and the subsequent base_url trim validation) with a check against the
resolved/effective provider result (e.g., call the function or property that
returns the effective provider for the given key) so only providers that
actually need network_config.base_url are enforced; apply the same change for
the duplicate instance at the later check (lines referenced 151-152).
---
Nitpick comments:
In `@core/providers/sgl/sgl.go`:
- Around line 196-220: In ChatCompletionStream, replace the hard-coded
schemas.SGL argument with provider.GetProviderKey() to match the pattern used in
ChatCompletion and Embedding; locate the ChatCompletionStream function and pass
provider.GetProviderKey() in place of schemas.SGL when calling
openai.HandleOpenAIChatCompletionStreaming so the provider key is consistent and
resilient to aliasing.
In `@framework/configstore/migrations.go`:
- Around line 5084-5098: The migration builds a schemas.Key manually which risks
missing future fields; replace the manual construction with the shared helper
schemaKeyFromTableKey (use schemaKeyFromTableKey(key) or the helper's actual
signature) so the hash/backfill uses the canonical field set for schemas.Key;
update any necessary imports and ensure getWeight usage is preserved by calling
the helper instead of the inline struct in this migration function.
- Around line 5438-5445: The migration creates pointer fields using the
address-of operator for local vars (enabled := true, weight := 1.0) when
constructing tables.TableKey; replace those with the repository convention
bifrost.Ptr(...) (e.g., Weight: bifrost.Ptr(weight), Enabled:
bifrost.Ptr(enabled)) so the migration follows existing pointer-construction
style used elsewhere (update the construction in the block that builds newKey in
migrations.go).
In `@framework/configstore/rdb.go`:
- Around line 366-387: Replace the hand-constructed tables.TableKey literal with
the shared helper tableKeyFromSchemaKey so provider-specific fields (Ollama/SGL,
Bedrock JSON handling, etc.) and future mappings stay consistent: call
tableKeyFromSchemaKey with the schema key, dbProvider and keyHash instead of
manually building the tables.TableKey (ensure the resulting TableKey preserves
Weight pointer, Enabled, UseForBatchAPI and Status/Description semantics), and
apply the same replacement to the other manual tables.TableKey constructions in
this file so all key write paths reuse the helper.
🪄 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: 9820bcb5-efc5-4aa6-aab7-0cfb3d6304f6
📒 Files selected for processing (25)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
✅ Files skipped from review due to trivial changes (2)
- ui/lib/schemas/providerForm.ts
- framework/configstore/tables/key.go
🚧 Files skipped from review as they are similar to previous changes (11)
- framework/configstore/clientconfig.go
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- transports/bifrost-http/server/server.go
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- core/schemas/account.go
- docs/openapi/schemas/management/providers.yaml
- core/utils.go
- transports/bifrost-http/lib/config.go
- transports/config.schema.json
- docs/openapi/openapi.json
Contributor
Merge activity
|
sammaji
force-pushed
the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
from
a92bac0 to
0028481
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/providers/fragments/networkFormFragment.tsx (1)
67-82:⚠️ Potential issue | 🟠 MajorStrip hidden
base_urlvalues from VLLM/Ollama/SGL submissions.For providers where
hideBaseURLis true, the form still seedsnetwork_config.base_urlfromprovider.network_configand writes it back into the update payload. After this PR,transports/config.schema.jsonrejectsnetwork_config.base_urlfor those providers, so editing headers/timeouts on a legacy config will resubmit the hidden field and fail validation.🛠️ Proposed fix
export function NetworkFormFragment({ provider }: NetworkFormFragmentProps) { const dispatch = useAppDispatch(); const hasUpdateProviderAccess = useRbac(RbacResource.ModelProvider, RbacOperation.Update); const [updateProvider, { isLoading: isUpdatingProvider }] = useUpdateProviderMutation(); const isCustomProvider = !isKnownProvider(provider.name as string); + const hideBaseURL = provider.name === "vllm" || provider.name === "ollama" || provider.name === "sgl"; const form = useForm<NetworkOnlyFormSchema, any, NetworkOnlyFormSchema>({ resolver: zodResolver(networkOnlyFormSchema) as Resolver<NetworkOnlyFormSchema, any, NetworkOnlyFormSchema>, mode: "onChange", reValidateMode: "onChange", defaultValues: { network_config: { - base_url: provider.network_config?.base_url || undefined, + base_url: hideBaseURL ? undefined : provider.network_config?.base_url || undefined, extra_headers: provider.network_config?.extra_headers, default_request_timeout_in_seconds: provider.network_config?.default_request_timeout_in_seconds ?? DefaultNetworkConfig.default_request_timeout_in_seconds, @@ const updatedProvider = buildProviderUpdatePayload(provider, { network_config: { ...provider.network_config, - base_url: data.network_config?.base_url || undefined, + base_url: hideBaseURL ? undefined : data.network_config?.base_url || undefined, extra_headers: data.network_config?.extra_headers || undefined, default_request_timeout_in_seconds: data.network_config?.default_request_timeout_in_seconds ?? 30, @@ useEffect(() => { form.reset({ network_config: { - base_url: provider.network_config?.base_url || undefined, + base_url: hideBaseURL ? undefined : provider.network_config?.base_url || undefined, extra_headers: provider.network_config?.extra_headers, default_request_timeout_in_seconds: provider.network_config?.default_request_timeout_in_seconds ?? DefaultNetworkConfig.default_request_timeout_in_seconds, @@ }, [form, provider.name, provider.network_config]); const baseURLRequired = isCustomProvider; - const hideBaseURL = provider.name === "vllm" || provider.name === "ollama" || provider.name === "sgl";Also applies to: 101-116, 131-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx` around lines 67 - 82, The form is seeding and resubmitting network_config.base_url even for providers with hideBaseURL=true which later fails schema validation; update the defaultValues block (the network_config object) to conditionally omit/clear base_url when provider.hideBaseURL is true (e.g. set base_url: provider.hideBaseURL ? undefined : provider.network_config?.base_url) and apply the same conditional clearing in the other similar blocks (the other defaultValues occurrences you noted at 101-116 and 131-152) so hidden base URLs are not carried through into the update payload.
🧹 Nitpick comments (2)
docs/openapi/schemas/management/providers.yaml (1)
142-152: Remove the unusedVLLMKeyConfigschema at line 180.The new
VllmKeyConfigschema (line 142) is now the canonical definition and is properly wired at line 226. The oldVLLMKeyConfigschema (line 180) is not referenced anywhere in the YAML file and does not appear in the generatedopenapi.json. Removing it eliminates dead code and prevents future schema drift.Suggested cleanup
VLLMKeyConfig: - type: object - description: vLLM-specific key configuration for per-key routing to different vLLM instances - properties: - url: - $ref: '../../schemas/management/common.yaml#/EnvVar' - description: vLLM server base URL (required) - model_name: - type: string - description: Exact model name served on this vLLM instance🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/schemas/management/providers.yaml` around lines 142 - 152, Remove the unused duplicate schema VLLMKeyConfig and keep the canonical VllmKeyConfig: locate the older schema block named "VLLMKeyConfig" and delete it entirely, then scan the file for any $ref or usages referencing "VLLMKeyConfig" and update them to "VllmKeyConfig" if any exist (the canonical "VllmKeyConfig" at the top of the file should remain unchanged); this eliminates the dead schema and prevents future drift.core/utils.go (1)
89-102: Rename or rewordproviderRequiresKey.After this split, the helper no longer means “auth secret required” — standard Ollama/SGL paths still evaluate
truehere whileCanProviderKeyValueBeEmptyseparately allows an empty value. A name/doc comment closer toproviderRequiresConfiguredKeywould make the contract much clearer for future call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/utils.go` around lines 89 - 102, Rename and reword providerRequiresKey to providerRequiresConfiguredKey and update its doc comment to reflect the new contract (it should indicate whether a provider requires a configured API key value, i.e., exclude keyless custom providers except for Bedrock), then update all call sites to use providerRequiresConfiguredKey and adjust any related documentation/tests; ensure the function signature and semantics remain the same but the name and comment clearly express that this is about requiring a configured key (not general auth secret).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/app/workspace/providers/fragments/networkFormFragment.tsx`:
- Around line 67-82: The form is seeding and resubmitting
network_config.base_url even for providers with hideBaseURL=true which later
fails schema validation; update the defaultValues block (the network_config
object) to conditionally omit/clear base_url when provider.hideBaseURL is true
(e.g. set base_url: provider.hideBaseURL ? undefined :
provider.network_config?.base_url) and apply the same conditional clearing in
the other similar blocks (the other defaultValues occurrences you noted at
101-116 and 131-152) so hidden base URLs are not carried through into the update
payload.
---
Nitpick comments:
In `@core/utils.go`:
- Around line 89-102: Rename and reword providerRequiresKey to
providerRequiresConfiguredKey and update its doc comment to reflect the new
contract (it should indicate whether a provider requires a configured API key
value, i.e., exclude keyless custom providers except for Bedrock), then update
all call sites to use providerRequiresConfiguredKey and adjust any related
documentation/tests; ensure the function signature and semantics remain the same
but the name and comment clearly express that this is about requiring a
configured key (not general auth secret).
In `@docs/openapi/schemas/management/providers.yaml`:
- Around line 142-152: Remove the unused duplicate schema VLLMKeyConfig and keep
the canonical VllmKeyConfig: locate the older schema block named "VLLMKeyConfig"
and delete it entirely, then scan the file for any $ref or usages referencing
"VLLMKeyConfig" and update them to "VllmKeyConfig" if any exist (the canonical
"VllmKeyConfig" at the top of the file should remain unchanged); this eliminates
the dead schema and prevents future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ab99bba-f750-4c05-8141-3cbb486a243e
📒 Files selected for processing (25)
core/bifrost.gocore/providers/ollama/ollama.gocore/providers/sgl/sgl.gocore/schemas/account.gocore/schemas/envvar.gocore/utils.godocs/openapi/openapi.jsondocs/openapi/schemas/management/providers.yamlframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewKeySheet.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/app/workspace/providers/views/utils.tsui/lib/schemas/providerForm.tsui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- ui/app/workspace/providers/views/utils.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- framework/configstore/clientconfig.go
- ui/app/workspace/providers/dialogs/addNewKeySheet.tsx
- transports/bifrost-http/server/server.go
- ui/lib/schemas/providerForm.ts
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- ui/lib/types/config.ts
- core/providers/sgl/sgl.go
- core/schemas/account.go
- ui/lib/types/schemas.ts
- framework/configstore/tables/key.go
- docs/openapi/openapi.json
- framework/configstore/migrations.go
akshaydeo
deleted the
03-30-feat_adds_support_for_multiple_deployments_in_ollama_and_sgl
branch
This was referenced Apr 14, 2026
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
2 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
Refactors Ollama and SGL providers to support per-key server URLs instead of requiring a single provider-level base URL. This enables load balancing across multiple Ollama/SGL instances by configuring different server URLs for each key.
Changes
ollama_key_config.urlandsgl_key_config.url) rather than provider-levelbase_urlOllamaKeyConfigandSGLKeyConfigschemas with required URL fields, including database migrations and UI supportHandleMultipleListModelsRequestspatternType of change
Affected areas
How to test
Test with multiple Ollama/SGL instances:
ollama_key_config.urlvaluesBreaking changes
Migration Required: Existing Ollama/SGL configurations using provider-level
base_urlmust migrate to per-key URL configuration. Each key now requires anollama_key_config.urlorsgl_key_config.urlfield. The provider-levelbase_urlis no longer used.Security considerations
Per-key URL configuration allows more granular control over which servers each key can access, improving security isolation between different Ollama/SGL deployments.
Checklist
docs/contributing/README.mdand followed the guidelines