feat: gate passthrough methods by virtual key provider config#3734
feat: gate passthrough methods by virtual key provider config#3734TejasGhatte wants to merge 27 commits into
Conversation
Adds blocked model support for virtual key provider configurations. Provider keys already supported both allowed and blocked models, but virtual key provider configs only supported allowed models. This PR adds the missing blocked model flow for virtual keys, so specific models can be denied at the VK provider-config level. * Added `blacklisted_models` to virtual key provider configs. * Added a configstore migration for the new `blacklisted_models` column. * Updated virtual key create/update handlers to validate, persist, and return blocked models. * Added governance checks to reject virtual key requests when the requested model is blocked. * Made blocked models take priority over allowed models. * Updated virtual key model filtering to respect blocked models. * Added `Blocked Models` UI under provider configurations in the virtual key create/edit sheet. * Added blocked model display in the virtual key details sheet. * Updated frontend governance types for virtual key provider configs. This follows the existing provider-key blocked model behavior instead of introducing a separate flow. Behavior: * Empty `blacklisted_models` means no models are blocked. * `["*"]` blocks all models for that VK provider config. * If the same model exists in both allowed and blocked models, the blocked list wins. * [ ] Bug fix * [x] Feature * [ ] Refactor * [ ] Documentation * [ ] Chore/CI * [x] Core (Go) * [x] Transports (HTTP) * [ ] Providers/Integrations * [x] Plugins * [x] UI (React) * [ ] Docs Local UI flow: Start Bifrost locally with embedded UI on port `9090`, then open: `http://localhost:9090/workspace/governance/virtual-keys` Steps tested: 1. Open Virtual Keys. 2. Create or edit a virtual key. 3. Expand a provider config. 4. Confirm `Blocked Models` appears below `Allowed Models`. 5. Select a blocked model and save. 6. Reopen the virtual key and confirm the blocked model is still shown. 7. Refresh the page and confirm the value persists. Runtime validation: 1. Configure a VK provider config with `allowed_models: ["*"]` and `blacklisted_models: ["<model-to-block>"]`. 2. Send a request through that virtual key using the blocked model. Expected: request is rejected. 3. Send a request through the same virtual key using a model not present in `blacklisted_models`. Expected: request succeeds. 4. Configure the same model in both `allowed_models` and `blacklisted_models`. Expected: request is rejected because blocked models take priority. 5. Configure `blacklisted_models: []`. Expected: existing virtual key behavior remains unchanged. Sanity checks: `go test ./framework/configstore/... ./plugins/governance/... ./transports/bifrost-http/handlers/...` UI check: `cd ui && pnpm build` Added a recording showing the new `Blocked Models` field in the virtual key provider configuration flow. https://github.com/user-attachments/assets/64efca01-0366-491c-b9e7-95dfa08eb0bc * [ ] Yes * [x] No BF-896 This change improves virtual-key governance by allowing specific models to be denied for a VK provider config. No provider secrets, customer keys, auth tokens, or PII are exposed or stored by this change. Existing provider-key behavior is unchanged. * [x] I read `docs/contributing/README.md` and followed the guidelines * [ ] I added/updated tests where appropriate * [ ] I updated documentation where needed * [x] I verified builds succeed (Go and UI) * [x] I verified the CI pipeline passes locally if applicable.
## Summary Improves E2E test stability for virtual key editing by handling a budget reset dialog that can appear after saving, and marks a known flaky bulk-rotate test as `fixme` until the underlying UI bug is resolved. ## Changes - Added `preserveBudgetUsageIfPrompted()` helper that detects the `vk-budget-reset-dialog` and clicks the preserve button if it appears after saving a virtual key. This prevents test failures caused by an unexpected dialog interrupting the save flow. - Marked `should bulk rotate selected virtual keys only` as `test.fixme` due to a UI bug where the checkbox selection state resets when the search input filters out a previously selected row. When `bulkRotateVirtualKeys` searches by name to select each key, the first key becomes deselected as the search narrows to the second, resulting in only the last key being rotated. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the virtual keys E2E suite and confirm no failures occur on the save flow due to the budget reset dialog: ```sh cd tests/e2e npx playwright test features/virtual-keys/virtual-keys.spec.ts ``` The bulk rotate test will be skipped (`fixme`) and should not cause CI failures. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations None. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
## Summary Consolidates the Per User OAuth Postman coverage probes to align with the updated API surface, replacing the old register/authorize/token/consent/upstream endpoints with the new flow-based endpoints (`/api/oauth/per-user/flows/:flowId` and `/api/oauth/per-user/flows/:flowId/start`). ## Changes - Removed coverage probe requests for `Per User OAuth Register`, `Per User OAuth Authorize`, `Per User OAuth Token`, `Per User OAuth Upstream Authorize`, `Per User Consent VK`, `Per User Consent User ID`, `Per User Consent Skip`, and `Per User Consent Submit` - Added coverage probe requests for `Per User OAuth Flow Detail` (`GET /api/oauth/per-user/flows/coverage-probe-flow`) and `Per User OAuth Flow Start` (`GET /api/oauth/per-user/flows/coverage-probe-flow/start`) - Added `OAuth Callback (Coverage Probe)` to the raw text shapes validation map, replacing the three previously tracked OAuth probe entries ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the updated Postman collection against a running Bifrost instance and verify that the new flow-based coverage probe requests return expected responses and that the response structure validation passes. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. These are test coverage probes only. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
…`values.yaml` (#3612) ## Summary Adds inline documentation comments to the Bifrost Helm chart values for timeout configuration fields, clarifying the units and defaults for rule and provider execution timeouts. ## Changes - Updated the `timeout` comment for guardrail rules to specify seconds as the unit and note the default value of 60 seconds (previously showed `1000`, which was ambiguous in terms of units) - Added a `timeout` comment for guardrail providers to document the default value of 30 seconds ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Deploy or lint the Helm chart to confirm the values file renders correctly. ```sh helm lint helm-charts/bifrost ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
#3617) ## Summary Adds optional gRPC port support to the Bifrost Helm chart, allowing the cluster to expose a gRPC endpoint when configured. ## Changes - Added a conditional gRPC container port (`TCP`) to the deployment template, rendered only when `bifrost.cluster.grpc` is defined in values - Added a corresponding conditional gRPC service port to the service template, targeting the named `grpc` port ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Set the following in your Helm values and confirm the deployment and service include the gRPC port: ```yaml bifrost: cluster: grpc: port: 50051 ``` ```sh helm template ./helm-charts/bifrost | grep -A 4 grpc ``` Expected output should include the `grpc` port entry in both the deployment container ports and the service spec. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The gRPC port is only exposed when explicitly configured. No authentication or TLS is configured at the Helm chart level; ensure appropriate network policies or ingress controls are in place when enabling this port. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…lag in Redis health checks (#3618) ## Summary Simplifies Redis health check commands by leveraging the `REDISCLI_AUTH` environment variable for automatic authentication, removing the need for conditional `-a "$REDIS_PASSWORD"` flags in liveness and readiness probes. ## Changes - Added `REDISCLI_AUTH` environment variable to the Redis container, sourced from the same secret as `REDIS_PASSWORD`. When set, `redis-cli` automatically uses this variable for authentication without requiring the `-a` flag. - Replaced the conditional `redis-cli` probe commands (which branched on whether auth was enabled) with a single unconditional `redis-cli ping`. When auth is enabled, `REDISCLI_AUTH` handles it transparently; when auth is disabled, the variable is simply not set. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Deploy the Bifrost Helm chart with Redis auth enabled and verify that the liveness and readiness probes succeed without errors: ```sh helm upgrade --install bifrost ./helm-charts/bifrost \ --set vectorStore.redis.auth.enabled=true \ --set vectorStore.redis.auth.password=<your-password> kubectl describe pod <redis-pod> | grep -A5 "Liveness\|Readiness" ``` Expected: probes report successful `ping` responses and the pod reaches `Running` state. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `REDISCLI_AUTH` is sourced from the same Kubernetes secret as `REDIS_PASSWORD`, so there is no change in how the password is stored or exposed. This approach avoids passing the password as a visible command-line argument, which is a minor security improvement. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary The Weaviate PersistentVolumeClaim (PVC) in the Bifrost Helm chart was being created unconditionally whenever Weaviate was enabled and not using an external instance. This change gates PVC creation behind the `persistence.enabled` flag, preventing unnecessary PVC creation when persistence is not desired. ## Changes - Added `.Values.vectorStore.weaviate.persistence.enabled` as an additional condition to the PVC template, so the PVC is only created when persistence is explicitly enabled ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Deploy the Bifrost Helm chart with Weaviate enabled and `vectorStore.weaviate.persistence.enabled` set to `false`. Verify that no PVC is created. Then set it to `true` and confirm the PVC is created as expected. ```sh helm template bifrost ./helm-charts/bifrost \ --set vectorStore.enabled=true \ --set vectorStore.type=weaviate \ --set vectorStore.weaviate.enabled=true \ --set vectorStore.weaviate.external.enabled=false \ --set vectorStore.weaviate.persistence.enabled=false \ | grep -i PersistentVolumeClaim ``` Expected: no PVC output when `persistence.enabled=false`. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…chart NOTES.txt (#3620) ## Summary Adds runtime warnings to the Bifrost Helm chart's `NOTES.txt` when default passwords are detected for PostgreSQL or Redis, alerting operators before they deploy to production with insecure credentials. ## Changes - Added a warning message displayed post-install if PostgreSQL is still using the default password `"bifrost_password"`, prompting users to set `postgresql.auth.password` to a strong value. - Added a warning message displayed post-install if Redis is still using the default password `"redis_password"`, prompting users to set `vectorStore.redis.auth.password` to a strong value. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Deploy the Helm chart with default values and verify the warnings appear in the install notes: ```sh helm install bifrost ./helm-charts/bifrost # Expected: warnings about default PostgreSQL and Redis passwords appear in NOTES output helm install bifrost ./helm-charts/bifrost \ --set postgresql.auth.password=myStrongPass \ --set vectorStore.redis.auth.password=myStrongRedisPass # Expected: no warnings appear in NOTES output ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations This change surfaces a security risk to operators who leave default credentials in place when deploying to production. It does not change any authentication logic but ensures users are explicitly warned about insecure default passwords for PostgreSQL and Redis. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…eterministic Helm rendering (#3621) ## Summary Helm chart templates that iterate over maps (provider secrets and Weaviate env vars) were producing non-deterministic ordering, which causes unnecessary diff noise and potential reconciliation churn in GitOps workflows. This change enforces alphabetical ordering when ranging over these maps. ## Changes - Provider secrets in `deployment.yaml` and `stateful.yaml` now iterate over `providerSecrets` keys sorted alphabetically before looking up each value - Weaviate environment variables in `weaviate-deployment.yaml` now iterate over `env` keys sorted alphabetically before looking up each value ## Type of change - [ ] Bug fix - [x] Refactor - [ ] Feature - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Render the Helm templates and verify that environment variables appear in a consistent, alphabetically sorted order across multiple renders: ```sh helm template bifrost ./helm-charts/bifrost --values your-values.yaml ``` Run the render multiple times and confirm the output is identical each time, with env vars appearing in alphabetical order. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. This change only affects the ordering of environment variable declarations in rendered Kubernetes manifests. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…t Helm chart (#3689) ## Summary Adds support for using a pre-existing Kubernetes Secret to supply the PostgreSQL password when using the bundled (non-external) PostgreSQL deployment. Previously, only the `postgresql.auth.password` plaintext value or an external secret was supported. This allows users to manage the PostgreSQL credential through their own secret management tooling without exposing the password in `values.yaml`. ## Changes - Added `postgresql.auth.existingSecret` and `postgresql.auth.passwordKey` fields to `values.yaml`, allowing users to reference a pre-existing Kubernetes Secret for the bundled PostgreSQL password. - The bundled PostgreSQL pod (`postgresql-deployment.yaml`) now reads `POSTGRES_PASSWORD` from the specified existing secret and key, falling back to the auto-generated secret and `password` key when not set. - The `deployment.yaml` and `stateful.yaml` templates now inject `BIFROST_POSTGRES_PASSWORD` from the existing secret when `postgresql.auth.existingSecret` is set and external PostgreSQL is not enabled. - The `secrets.yaml` template skips creating the auto-generated PostgreSQL secret when `postgresql.auth.existingSecret` is provided, preventing conflicts. - The `_helpers.tpl` password helper returns the `env.BIFROST_POSTGRES_PASSWORD` sentinel (indicating the value should be read from the environment) when an existing secret is configured, rather than attempting to inline the password. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Set the following in your `values.yaml` or via `--set` flags and confirm the deployment reads the password from the referenced secret rather than generating one: ```sh # Create a secret manually kubectl create secret generic my-pg-secret --from-literal=password=mysecretpassword # Install/upgrade the chart referencing the existing secret helm upgrade --install bifrost ./helm-charts/bifrost \ --set postgresql.auth.existingSecret=my-pg-secret \ --set postgresql.auth.passwordKey=password # Verify the auto-generated secret is NOT created kubectl get secret bifrost-postgresql # should return NotFound # Verify the PostgreSQL pod and bifrost deployment reference my-pg-secret kubectl get deployment -o yaml | grep -A5 "POSTGRES_PASSWORD" kubectl get deployment -o yaml | grep -A5 "BIFROST_POSTGRES_PASSWORD" ``` New configuration values: | Key | Default | Description | |-----|---------|-------------| | `postgresql.auth.existingSecret` | `""` | Name of an existing Kubernetes Secret containing the PostgreSQL password. Takes precedence over `postgresql.auth.password`. | | `postgresql.auth.passwordKey` | `"password"` | Key within the existing secret that holds the password value. | ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations This change improves the security posture by allowing operators to avoid storing the PostgreSQL password as plaintext in `values.yaml`. When `existingSecret` is set, no Kubernetes Secret is generated by the chart, and both the PostgreSQL pod and the Bifrost application read the credential directly from the referenced secret at runtime. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…sql deployment (#3694) ## Summary Moves the hardcoded `fsGroup: 999` pod security context for the PostgreSQL deployment into a configurable `values.yaml` field, and adds support for a container-level security context on the PostgreSQL primary container. ## Changes - Replaced the hardcoded `fsGroup: 999` in the PostgreSQL deployment pod spec with a templated reference to `.Values.postgresql.primary.podSecurityContext`, defaulting to `fsGroup: 999` in `values.yaml` - Added an optional `containerSecurityContext` field under `postgresql.primary` (defaults to `{}`) that, when set, is applied to the PostgreSQL container's `securityContext` ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Deploy the Bifrost Helm chart and verify the PostgreSQL pod launches with the expected security context. Override `postgresql.primary.podSecurityContext` and `postgresql.primary.containerSecurityContext` in a custom `values.yaml` to confirm they are applied correctly. ```sh helm template bifrost ./helm-charts/bifrost -f custom-values.yaml | grep -A 10 securityContext ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations This change allows operators to customize the security context for the PostgreSQL pod and container, enabling stricter security policies (e.g., running as a non-root user, dropping capabilities) without requiring chart modifications. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…talog URL, and bug fixes (#3623) ## Summary Bumps the Bifrost Helm chart to version `2.1.18`, introducing new configuration options, fixing several rendering and runtime bugs, and updating the Helm index accordingly. ## Changes - Added `bifrost.featureFlags` map to `values.yaml` and `_helpers.tpl`, rendering into `feature_flags.flags` in the generated config JSON. Each entry accepts a literal boolean or `"env.NAME"` string. - Added `bifrost.modelCatalog.modelParametersUrl` to allow operators to override the URL Bifrost uses to fetch model parameter definitions. - Fixed the Deployment not exposing the cluster gRPC container port and `service.yaml` missing the gRPC service port, aligning both with StatefulSet/headless service behaviour. - Fixed Weaviate PVC rendering when `vectorStore.weaviate.persistence.enabled=false`; PVC is now gated on persistence being enabled. - Fixed Redis liveness/readiness probes passing the password via `-a` flag in process args; switched to the `REDISCLI_AUTH` environment variable to avoid credential exposure. - Fixed nondeterministic env var ordering for `providerSecrets` and `weaviate.env` map iterations by sorting keys with `sortAlpha`. - Corrected guardrail `timeout` example values in `values.yaml` (provider default `30s`, rule default `60s`). ## Type of change - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh # Lint and render the chart locally helm lint helm-charts/bifrost # Render templates and inspect output helm template bifrost helm-charts/bifrost \ --set bifrost.featureFlags.someFlag=true \ --set bifrost.modelCatalog.modelParametersUrl="https://example.com/params.json" # Verify gRPC port appears in both Deployment and Service helm template bifrost helm-charts/bifrost | grep -A5 "grpc" # Verify Weaviate PVC is absent when persistence is disabled helm template bifrost helm-charts/bifrost \ --set vectorStore.weaviate.persistence.enabled=false | grep -c "PersistentVolumeClaim" # Verify Redis probe uses REDISCLI_AUTH instead of -a flag helm template bifrost helm-charts/bifrost | grep "REDISCLI_AUTH" ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The Redis probe fix removes the password from process arguments, preventing credential leakage via `/proc` or process listings. No other security-sensitive changes. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary When constructing Bedrock messages for assistant turns that include reasoning details alongside tool calls, the reasoning content blocks were being appended after text and tool-use blocks. Bedrock requires reasoning blocks to appear first in the content array, so this ordering caused malformed requests during multi-turn conversations involving extended thinking and tool use. ## Changes - Reordered content block assembly in `convertMessage` so that reasoning blocks are always prepended before text/image content and tool-use blocks. - Added a test case `AssistantMessage_WithReasoningAndToolCalls_ReasoningComesFirst` that verifies the first content block is a reasoning block and that tool-use blocks follow it. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/bedrock/... -run TestMultiTurnReasoningContentPassthrough ``` The new subtest `AssistantMessage_WithReasoningAndToolCalls_ReasoningComesFirst` should pass, confirming that the reasoning block is the first element in the assembled content array and that a tool-use block appears after it. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
### TL;DR Fix incorrect `tool_calls` finish reason being returned when structured output (response format) is used with extended thinking enabled on Anthropic and Bedrock providers. ### What changed? - When extended thinking (reasoning) is active on Anthropic, forcing `tool_choice` to the structured output tool is now skipped, since Anthropic rejects that combination. The tool is still appended and the model may call it voluntarily. - The finish reason override from `tool_calls` → `stop` for structured output is now gated on whether the structured output tool block was **actually consumed into text content**, rather than just whether a structured output tool name was configured. This prevents incorrectly overriding the finish reason in cases where the tool was never invoked. - This fix is applied consistently across the chat completion (non-streaming and streaming), responses (non-streaming and streaming), and Bedrock chat completion paths. - A `UsedStructuredOutputTool` / `consumedStructuredOutput` flag is tracked per-response and per-stream to record when the SO tool block is folded back into text content, and the finish reason override only fires when that flag is set. - For the non-streaming responses path, the override logic inspects the response content blocks directly to confirm no real (non-SO) tool calls are present before remapping the stop reason. ### How to test? 1. Send a chat completion or responses request with a `response_format` (structured output) and extended thinking/reasoning enabled against an Anthropic model. Verify the finish reason is `stop` and no API rejection occurs due to conflicting `tool_choice`. 2. Send a request with `response_format` but **without** extended thinking. Verify the finish reason is still `stop` and the structured output JSON is returned as content. 3. Send a request that uses both real tool calls and structured output simultaneously. Verify the finish reason correctly reflects `tool_calls` for the real tools. 4. Repeat the above for streaming and non-streaming variants, and for the Bedrock provider. ### Why make this change? Anthropic rejects requests that combine extended thinking with a forced `tool_choice`, causing failures when structured output was requested alongside reasoning. Additionally, the previous finish reason override was too broad — it fired whenever a structured output tool name was present in context, even if the tool was never actually used, which could mask legitimate `tool_calls` finish reasons in mixed-tool scenarios.
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a deny-by-default per-provider "allow_passthrough" flag for virtual keys: DB migration and model field, governance checks (blocked with 403), HTTP request schemas and handlers, TypeScript API types, UI constants, form inputs, and details view updates. ChangesPassthrough API Permission Control
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant PreLLM as PreLLMHook
participant GovCheck as GovernancePlugin.checkPassthroughPermission
participant VKStore as Virtual Key Store
participant Eval as EvaluateGovernanceRequest
Client->>PreLLM: PassthroughRequest + VirtualKey
PreLLM->>GovCheck: checkPassthroughPermission(provider, virtualKey)
GovCheck->>VKStore: load virtual key provider configs
VKStore-->>GovCheck: provider config (allow_passthrough: false)
GovCheck-->>PreLLM: BifrostError 403 (passthrough not enabled)
PreLLM->>Eval: DecisionPassthroughBlocked
Eval-->>Client: 403 Passthrough not enabled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5The change is safe to merge. The core enforcement logic is correct — deny-by-default, proper nil handling on updates, and idempotent migration. The migration is guarded with HasColumn checks and has a rollback path. The plugins/governance/main.go and plugins/governance/resolver.go — the ordering of checkPassthroughPermission relative to EvaluateGovernanceRequest and the redundant store lookup are worth a follow-up but do not block merging. Important Files Changed
Reviews (3): Last reviewed commit: "feat: gate passthrough methods by virtua..." | Re-trigger Greptile |
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)
transports/bifrost-http/handlers/governance.go (1)
661-705: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConsider validating
allow_passthroughagainst supported providers.The handler accepts
allow_passthrough: truefor any configured provider, but onlyopenai,anthropic,gemini,vertex, andazuresupport passthrough (perPASSTHROUGH_ENABLED_PROVIDERSin the UI). Direct API calls could set this flag for unsupported providers, storing inconsistent data. While the governance plugin enforces passthrough permissions at runtime, adding validation here would prevent data inconsistency and provide immediate feedback.🛡️ Proposed validation
Define the supported provider set in the Go backend:
+var passthroughEnabledProviders = map[schemas.ModelProvider]struct{}{ + "openai": {}, + "anthropic": {}, + "gemini": {}, + "vertex": {}, + "azure": {}, +} + providerConfig := &configstoreTables.TableVirtualKeyProviderConfig{ VirtualKeyID: vk.ID, Provider: string(providerName), Weight: pc.Weight, AllowedModels: pc.AllowedModels, BlacklistedModels: pc.BlacklistedModels, AllowAllKeys: allowAllKeys, + AllowPassthrough: pc.AllowPassthrough, - AllowPassthrough: pc.AllowPassthrough, Keys: keys, } + + if pc.AllowPassthrough { + if _, ok := passthroughEnabledProviders[providerName]; !ok { + return &badRequestError{err: fmt.Errorf("provider %s does not support passthrough API", providerName)} + } + }Apply the same validation in the update path (lines 1113-1122 for new configs and lines 1189-1211 for existing configs).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/governance.go` around lines 661 - 705, The code allows pc.AllowPassthrough=true for any provider, which can store invalid passthrough flags; when building providerConfig in the provider loop (providerName, pc.AllowPassthrough, providerSet), validate pc.AllowPassthrough against a backend whitelist of passthrough-capable providers (same set used by the UI: openai, anthropic, gemini, vertex, azure) and return a badRequestError if true for an unsupported provider; apply the identical validation to the update-path provider handling blocks that construct new and existing provider configs so passthrough is rejected consistently server-side.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/governance/main.go`:
- Around line 1467-1475: The passthrough permission check (the block that checks
req.RequestType == schemas.PassthroughRequest || req.RequestType ==
schemas.PassthroughStreamRequest and calls p.checkPassthroughPermission with
virtualKeyValue and provider) runs too early and can leak provider-config state
for blocked virtual keys; move or guard that logic so it only executes after
EvaluateGovernanceRequest has validated the virtual key (i.e., call
p.checkPassthroughPermission only if EvaluateGovernanceRequest succeeds and the
VK is not blocked), or fold the passthrough check into the normal VK evaluation
flow so that virtual_key_blocked takes precedence over returning
passthrough_not_allowed.
In `@ui/lib/types/governance.ts`:
- Line 95: VirtualKeyProviderConfig's allow_passthrough is currently required
but the server may omit it; update the type so the property is optional (e.g.,
allow_passthrough?: boolean) in the VirtualKeyProviderConfig declaration and
adjust any places that assume it always exists (use safe checks or defaulting).
Ensure references to allow_passthrough in validation or usage handle undefined
rather than only boolean values.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 661-705: The code allows pc.AllowPassthrough=true for any
provider, which can store invalid passthrough flags; when building
providerConfig in the provider loop (providerName, pc.AllowPassthrough,
providerSet), validate pc.AllowPassthrough against a backend whitelist of
passthrough-capable providers (same set used by the UI: openai, anthropic,
gemini, vertex, azure) and return a badRequestError if true for an unsupported
provider; apply the identical validation to the update-path provider handling
blocks that construct new and existing provider configs so passthrough is
rejected consistently server-side.
🪄 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: bcddb9ae-1476-4757-8d52-956e9afdd1ce
📒 Files selected for processing (9)
framework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.goui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/constants/governance.tsui/lib/types/governance.ts
cb68a1a to
e6fa30e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/governance/main.go`:
- Around line 1474-1479: The passthrough denial returns early without marking
the request as governance-rejected; update the block that handles
schemas.PassthroughRequest and schemas.PassthroughStreamRequest so that when
p.checkPassthroughPermission(ctx, virtualKeyValue, provider) returns an error
you first set the request's governance-rejection marker (the same field/key used
elsewhere for blocked requests, e.g., req.GovernanceRejected or the repo's
equivalent metadata flag) and then return req,
&schemas.LLMPluginShortCircuit{Error: bifrostErr}, nil; modify the code in the
branch where virtualKeyValue != "" and bifrostErr != nil to set that marker
before the short-circuit return.
🪄 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: 915a806b-7235-4b2e-afb3-592a649d6e2f
📒 Files selected for processing (9)
framework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.goui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/constants/governance.tsui/lib/types/governance.ts
…3729) ## Summary Fixes a regression ([#3537](#3537)) where Bedrock-native passthrough requests containing `toolResult.content[].searchResult` or `toolResult.content[].video` blocks were silently dropped during the `ToBifrostResponsesRequest` → `ToBedrockResponsesRequest` round-trip. The outbound request would carry an empty `{"text": ""}` block instead of the original content. ## Changes - Added `BedrockSearchResultBlock`, `BedrockSearchResultContent`, `BedrockCitationsConfig`, `BedrockVideoBlock`, `BedrockVideoSource`, and `BedrockS3Location` types to `types.go` so these AWS Converse API structures are recognized during JSON unmarshal. - Added `SearchResult` and `Video` fields to `BedrockContentBlock` so the types are reachable from content blocks. - Introduced a sentinel-envelope mechanism (`encodeBedrockToolResultEnvelope` / `decodeBedrockToolResultEnvelope`) in `utils.go`. When a `toolResult.content` array contains blocks that Bifrost's intermediate format cannot model natively (e.g. `searchResult`, `video`), the full content array is serialized into a keyed JSON envelope and stored as the tool output string. On the return leg, the envelope is detected and decoded back into the original `BedrockContentBlock` slice before being forwarded to Bedrock. - Updated `ConvertBifrostMessagesToBedrockMessages` in `responses.go` to check for and decode the sentinel envelope before falling through to `tryParseJSONIntoContentBlock`. - Updated `convertSingleBedrockMessageToBifrostMessages` in `responses.go` to detect unrepresentable blocks and encode them into the sentinel envelope instead of attempting lossy text extraction. - Added regression tests `TestBedrockSearchResultToolResultRoundTrip` and `TestBedrockVideoToolResultRoundTrip` that exercise the full `ToBifrostResponsesRequest` → `ToBedrockResponsesRequest` round-trip and assert all fields survive intact. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/bedrock/... -run "TestBedrockSearchResultToolResultRoundTrip|TestBedrockVideoToolResultRoundTrip" -v ``` Both tests should pass. Prior to this fix, both would fail with `expected toolResult.content[].searchResult to round-trip; got nil` and `expected toolResult.content[].video to round-trip; got nil` respectively. To run the full Bedrock provider test suite: ```sh go test ./core/providers/bedrock/... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #3537 ## Security considerations The sentinel envelope is a self-contained JSON object keyed by a fixed internal string. It is only decoded when the key is present and the envelope contains exactly one key, preventing accidental misinterpretation of user-supplied JSON payloads. No secrets or PII are introduced by this mechanism. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
## Summary Extra headers forwarded to upstream providers (via `x-bf-eh-*` passthrough or direct allowlist forwarding) were not visible in trace spans, making it difficult to correlate what Bifrost actually sent to a provider with what observability backends recorded. This PR surfaces those headers as span attributes so the two views are consistent. ## Changes - In `executeRequestWithRetries`, after setting the retry count attribute, the extra headers stored in the request context under `BifrostContextKeyExtraHeaders` are now iterated and each header is recorded as a span attribute using the `gen_ai.request.extra_header` prefix. Single-value headers are stored as a plain string; multi-value headers are stored as a slice. - Added `AttrExtraHeaderPrefix = "gen_ai.request.extra_header"` to the trace schema constants to provide a stable, namespaced key for these attributes. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Send a request to Bifrost that includes extra headers (either via `x-bf-eh-*` or an allowlisted header). Inspect the resulting trace span and confirm attributes of the form `gen_ai.request.extra_header.<header-name>` are present with the correct values. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations Header values forwarded to upstream providers are recorded in trace spans. Ensure that any sensitive headers (e.g., authorization tokens, API keys) are not included in the extra headers allowlist or `x-bf-eh-*` passthrough, as they will become visible in observability backends. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds a new top-level OpenTelemetry feature page and documents that caller-supplied `x-bf-eh-*` headers (and direct-allowlist headers) are surfaced as `gen_ai.request.extra_header.<name>` span attributes on the `llm.call` span, enabling trace filtering and correlation by session, tenant, or correlation ID without additional instrumentation. ## Changes - Added `docs/features/otel.mdx` — a new top-level OTel overview page covering captured attributes, dynamic attribute injection via `x-bf-eh-*` headers, and links to the full integration guide and related references. - Registered `features/otel` in `docs/docs.json` so the new page appears in the navigation. - Updated `docs/features/observability/otel.mdx` to document the "Caller-Supplied Headers" behaviour under the captured data section, with an example showing `x-bf-eh-session-id` producing `gen_ai.request.extra_header.session-id` on the span. - Updated `docs/providers/request-options.mdx` to note that forwarded extra headers are also attached to the OTel span, and added a concrete `curl` example showing the dual effect on both the provider request and the `llm.call` span. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Navigate to the docs site and verify: 1. The new **OpenTelemetry** page appears in the sidebar under Features. 2. The `features/otel` page renders correctly with the attribute table, dynamic injection section, and the `curl` example. 3. The `features/observability/otel` page shows the new **Caller-Supplied Headers** subsection under the captured data section. 4. The `providers/request-options` page shows the updated extra-headers description and the session-ID example with the expected OTel outcome. ## Breaking changes - [ ] Yes - [x] No ## Security considerations The documentation explicitly notes that the same security denylist and header filter configuration that gates provider forwarding also gates which headers appear as span attributes — no additional headers are exposed beyond what is already forwarded to the upstream provider. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…nventions (#3732) ## Summary Aligns Bifrost's tracing attribute emissions with the OpenTelemetry GenAI semantic conventions spec. Bifrost-internal concepts (routing, governance, retry counters, etc.) were previously emitted under the `gen_ai.*` namespace, which pollutes the OTel spec namespace. This PR introduces a parallel `bifrost.*` namespace as the canonical home for those attributes and adds spec-compliant mappings for provider names, operation names, token usage keys, tool execution attributes, and more. Legacy `gen_ai.*` emissions are retained in parallel (tagged `// legacy:`) to avoid breaking existing dashboards, with a clear migration path to drop them once consumers have moved over. ## Changes - Added `core/schemas/otelconv.go` with `OTelOperationName` and `OTelProviderName` helpers that map Bifrost-internal types to OTel GenAI spec values (e.g. `Bedrock` → `aws.bedrock`, `ChatCompletionRequest` → `chat`). - Introduced a full set of `bifrost.*` attribute constants (`AttrBifrostProviderName`, `AttrBifrostVirtualKeyID`, `AttrBifrostRetries`, etc.) as the canonical namespace for Bifrost-internal span attributes. - Added new OTel spec-aligned attribute constants: `AttrOperationName`, `AttrChoiceCount`, `AttrEmbeddingsDimensionCount`, `AttrEncodingFormats`, `AttrUsageCacheReadInputTokens`, `AttrUsageCacheCreationInputTokens`, `AttrErrorTypeSpec`, and tool execution attributes (`AttrToolName`, `AttrToolCallID`, `AttrToolCallArguments`, `AttrToolCallResult`, `AttrToolType`). - Updated `executeRequestWithRetries` in `bifrost.go` to emit both legacy and canonical attributes, use `OTelProviderName`/`OTelOperationName`, and format the root span name as `"{operation} {model}"` per the GenAI semconv. - Updated `PopulateRequestAttributes`, `PopulateErrorAttributes`, `PopulateContextAttributes`, and all response/request attribute helpers in `llmspan.go` to emit both legacy and spec-aligned keys in parallel. - Fixed `AttrStopSequences` to emit a proper `[]string` slice instead of a comma-joined string; the joined form is preserved under `AttrBifrostStopSequencesJoined` for back-compat. - Added `gen_ai.usage.input_tokens` / `gen_ai.usage.output_tokens` alongside the deprecated `prompt_tokens` / `completion_tokens` keys across chat, text completion, and embedding response helpers. - Added OTel GenAI tool execution attributes to MCP execute-tool spans in `pluginpipeline.go`, including tool name, call ID, arguments, and result (captured via named returns). - Updated the OTel plugin converter to emit `bifrost.request.id` alongside the legacy `gen_ai.request_id`. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/... ./framework/tracing/... ./plugins/otel/... ``` Validate that spans emitted by `executeRequestWithRetries` contain both the legacy `gen_ai.*` attributes and the new `bifrost.*` / spec-aligned counterparts. Confirm MCP execute-tool spans include `gen_ai.tool.name`, `gen_ai.tool.call.id`, `gen_ai.tool.call.arguments`, and `gen_ai.tool.call.result`. Verify `gen_ai.provider.name` now emits the spec-canonical form (e.g. `aws.bedrock`) while `bifrost.provider.name` retains the short Bifrost name. ## Breaking changes - [ ] Yes - [x] No All previously emitted attributes are still present. New attributes are additive. The only behavioral difference is that `gen_ai.provider.name` now emits the OTel canonical form rather than the Bifrost short name; the short name is preserved under `bifrost.provider.name`. ## Security considerations No auth, secrets, PII, or sandboxing changes. Span attributes may now include tool call arguments and results on MCP tool spans — operators should ensure their observability backend's data retention policies are appropriate for that data. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…3733) ## Summary Fixes a race condition in `SetupStreamCancellation` where `BifrostContextKeyConnectionClosed` was set *after* calling `Close()` on the body stream. Because `Close()` immediately unblocks any in-progress `Read` (which may panic due to a force-closed connection), the recover block in `idleTimeoutReader.Read` could run before the flag was set — causing it to re-panic instead of returning `ErrStreamClosed`. Additionally, `idleTimeoutReader.Read` was returning `(0, nil)` when the connection was already marked closed, which is incorrect. It now returns the appropriate closed-stream error via `closedReadError()`. ## Changes - `SetupStreamCancellation` now sets `BifrostContextKeyConnectionClosed` **before** calling `Close()` or `CloseWithError()` in all four call sites, eliminating the race window where a panicking `Read` could recover before the flag was visible. - `idleTimeoutReader.Read` now returns `r.closedReadError()` instead of `(0, nil)` when the connection is already closed, ensuring callers receive a meaningful error rather than a silent empty read. - Added `syncedPanicBody`, a deterministic test helper that reproduces the exact race: `Close()` triggers a panic in `Read` and then blocks, holding `SetupStreamCancellation` inside `Close()` so the flag is guaranteed to be unset when the recover block runs under the unfixed code. - Added `TestSetupStreamCancellation_NoPanicOnCancelledContext` which fails against the unfixed code and passes after the fix. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) ## How to test ```sh go test ./core/providers/utils/... -race -count=1 -run TestSetupStreamCancellation_NoPanicOnCancelledContext go test ./core/providers/utils/... -race -count=1 ``` The new test should pass without any detected data races or re-panics. ## Breaking changes - [x] No ## Security considerations None. This is a stability fix for stream teardown on context cancellation. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary
Replaces `nil` logger arguments with a `noopLogger{}` instance in Bedrock transport tests to ensure tests use a valid logger implementation rather than a nil value.
## Changes
- Replaced `nil` with `noopLogger{}` when calling `NewBedrockProvider` across all Bedrock transport tests to provide a proper no-op logger, avoiding potential nil pointer dereferences during test execution.
## Type of change
- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./core/providers/bedrock/...
```
All Bedrock transport tests should pass without nil pointer panics or logger-related errors.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications. This is a test-only change that swaps a nil logger for a no-op implementation.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary Refactors VK blocked-model matching to use `slices.Contains`, as requested in review. This keeps the existing behavior unchanged while making the matching logic cleaner. Bare and provider-prefixed model names are still treated as equivalent, so entries like `mistral:latest` and `ollama/mistral:latest` continue to match correctly. Wildcard blocklists still block all models. ## Changes * Added `blockedModelCandidates()` to build normalized match candidates for a model string. * Includes the lowercased raw model name. * Includes the lowercased bare model name after provider-prefix parsing. * Updated `isModelBlockedByList()` to use `slices.Contains` for comparing normalized model forms. * Preserved existing blocklist behavior for: * bare model vs bare request * prefixed blocklist entry vs bare request * bare blocklist entry vs prefixed request * prefixed model vs prefixed request * wildcard `["*"]` Design decision: * This is only a small internal refactor of the VK blocklist helper. * No runtime behavior is intentionally changed. * Provider-key behavior is unchanged. ## Type of change * [ ] Bug fix * [ ] Feature * [x] Refactor * [ ] Documentation * [ ] Chore/CI ## Affected areas * [ ] Core (Go) * [ ] Transports (HTTP) * [ ] Providers/Integrations * [x] Plugins * [ ] UI (React) * [ ] Docs ## How to test Sanity checks: ```sh go test ./plugins/governance/... go build -o ./tmp/bifrost-http ./transports/bifrost-http ``` Verified local Ollama E2E behavior: * `["mistral:latest"]` + `mistral:latest` → `403 model_blocked` * `["ollama/mistral:latest"]` + `mistral:latest` → `403 model_blocked` * `["mistral:latest"]` + `ollama/mistral:latest` → `403 model_blocked` * `["ollama/mistral:latest"]` + `ollama/mistral:latest` → `403 model_blocked` * Different allowed model → `200 OK` * Empty blocklist → `200 OK` * Wildcard blocklist `["*"]` → all tested models blocked * Same model in allowlist and blocklist → `403 model_blocked` ## Screenshots/Recordings Not applicable. This PR only refactors backend governance matching logic. ## Breaking changes * [ ] Yes * [x] No ## Related issues Follow-up to #3718 ## Security considerations This keeps VK blocked-model enforcement intact for both bare and provider-prefixed model strings. No secrets, auth tokens, provider keys, or PII are exposed or stored by this change. Provider-key behavior is unchanged. ## Checklist * [x] I read `docs/contributing/README.md` and followed the guidelines * [x] I added/updated tests where appropriate * [ ] I updated documentation where needed * [x] I verified builds succeed (Go and UI) * [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds a new `mcp_enable_temp_token_auth` client config flag that gates whether Bifrost mints and accepts scoped short-lived temp tokens for MCP per-user OAuth authorization pages. Previously, temp-token auth was implicitly enabled whenever the `tempTokens` service was wired up. This change makes it an explicit opt-in, defaulting to `false`, so deployments must consciously enable it. ## Changes - Added `MCPEnableTempTokenAuth` bool field to `ClientConfig`, `TableClientConfig`, and all relevant migration/serialization paths, with a database migration to add the column. - `OAuth2Provider.InitiateUserOAuthFlow` now checks both that the `tempTokens` service is non-nil **and** that `MCPEnableTempTokenAuth` is `true` in client config before minting a temp token into the auth-page URL fragment. - `AuthMiddleware` gains a `tempTokensEnabled` atomic bool, initialized and updated via `ReloadClientConfigFromConfigStore`, so the `X-Bifrost-Temp-Token` fallback path in `tryTempTokenOrUnauthorized` is also gated by the same flag at runtime. - Added `UpdateTempTokenAuthEnabled` method on `AuthMiddleware` and wired it into `ReloadClientConfigFromConfigStore` so config changes take effect without a restart. - UI MCP settings view exposes a new "Allow Temp Token Auth Links" toggle, visible only when SSO/SCIM is enabled (`IS_ENTERPRISE && authType === "sso"`), with a descriptive label explaining the security trade-off. - Helm chart `_helpers.tpl`, `values.yaml`, and `values.schema.json` updated to expose `mcpEnableTempTokenAuth`. - `transports/config.schema.json` and the schema test updated to include `mcp_enable_temp_token_auth`. - Added `TestMCPTempTokenAuthEnabled` unit test covering the three states: no config, config with flag false, config with flag true. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./framework/configstore/... ./framework/oauth2/... ./transports/bifrost-http/... # UI cd ui pnpm i pnpm build ``` **Manual validation:** 1. Start Bifrost with `mcp_enable_temp_token_auth: false` (default). Initiate a per-user MCP OAuth flow and confirm the returned auth-page URL contains **no** `#` fragment. 2. Set `mcp_enable_temp_token_auth: true`. Repeat the flow and confirm the URL contains a `#mcp_auth=<token>` fragment. 3. Confirm that after the flow completes or expires, the temp token is cleaned up and the fragment link no longer works. 4. In the UI (enterprise SSO deployment), navigate to MCP settings and verify the "Allow Temp Token Auth Links" toggle appears and persists correctly. **New config field:** | Field | Type | Default | Description | |---|---|---|---| | `mcp_enable_temp_token_auth` | `boolean` | `false` | When true, Bifrost mints and accepts scoped temp tokens for MCP per-user OAuth auth pages | ## Breaking changes - [x] No The flag defaults to `false`, preserving existing behavior. Deployments that previously relied on temp-token auth being implicitly active (when the service was wired) must explicitly set `mcp_enable_temp_token_auth: true`. ## Security considerations Temp tokens embedded in URL fragments grant unauthenticated access to a specific MCP OAuth flow page for the lifetime of the flow. Keeping this opt-in (`false` by default) ensures operators consciously accept this trade-off. The tokens are scoped to a single `session_id` resource and are deleted on flow completion or expiry. The feature is surfaced in the UI only for SSO-enabled enterprise deployments where per-user OAuth flows are most relevant. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Extends the `/api/is-auth-enabled` endpoint contract to expose `has_valid_token` and `auth_type` fields alongside the existing `is_auth_enabled` field, and adds E2E test coverage for the new `mcp_enable_temp_token_auth` config flag. ## Changes - Updated the `Check If Auth Enabled` response field assertion to include `has_valid_token` and `auth_type` in addition to `is_auth_enabled`. - Added a test script to the `is-auth-enabled` request that validates: - `has_valid_token` is a boolean - `auth_type` is one of `'none'`, `'password'`, or `'sso'` - When auth is disabled, `auth_type` is `'none'` and `has_valid_token` is `false` - When auth is enabled, `auth_type` is not `'none'` - Added two new Postman requests under the config section: - **Update Config (Enable Temp Token Auth)** — PUTs `mcp_enable_temp_token_auth: true` to `/api/config` - **Get Config (Verify Temp Token Auth)** — GETs `/api/config` and asserts `mcp_enable_temp_token_auth` round-trips as `true` ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the updated Postman collection against a running Bifrost instance: ```sh newman run tests/e2e/api/collections/bifrost-api-management.postman_collection.json \ --env-var base_url=http://localhost:<port> ``` Expected outcomes: - `Check If Auth Enabled` response includes `is_auth_enabled`, `has_valid_token`, and `auth_type` - `auth_type` is one of `none`, `password`, or `sso` - `mcp_enable_temp_token_auth` can be set to `true` via PUT and is returned as `true` on subsequent GET ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The `has_valid_token` and `auth_type` fields are exposed on the `is-auth-enabled` endpoint. This endpoint is intended to be publicly accessible for client bootstrapping. Care should be taken to ensure no sensitive session or token details are leaked beyond the boolean and enum values validated here. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…al key quota query and response (#3721) ## Summary Provider-level budget and rate limit configurations were not being included when fetching virtual key quota data, meaning the `getVirtualKeyQuota` endpoint returned incomplete governance information. This PR extends the lean quota query to also load `ProviderConfigs` along with their associated `Budgets` and `RateLimit`, and exposes them in the API response. ## Changes - `GetVirtualKeyQuotaByValue` now preloads `ProviderConfigs`, `ProviderConfigs.Budgets`, and `ProviderConfigs.RateLimit` in addition to the existing top-level `Budgets` and `RateLimit` preloads. - The `getVirtualKeyQuota` HTTP handler now includes `provider_configs` in the JSON response payload. - Updated the comment on `GetVirtualKeyQuotaByValue` to accurately reflect what is and isn't loaded (provider `Keys` and `MCPConfigs` are still excluded; `ProviderConfigs` themselves are now included). ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Call the virtual key quota endpoint with a virtual key that has provider configs attached and verify the response includes `provider_configs` with their respective `budgets` and `rate_limit` fields populated. ```sh go test ./framework/configstore/... go test ./transports/bifrost-http/... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No new secrets or PII are introduced. `ProviderConfigs` data returned is scoped to the authenticated virtual key, consistent with existing quota data access controls. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary The auth middleware whitelist was being matched against the full request URI (including query parameters), which could cause whitelist entries to fail to match when query strings were present. This fix ensures the whitelist is matched against the path only. ## Changes - Replaced `ctx.Request.URI().RequestURI()` with `ctx.Path()` in the auth middleware so that URL whitelist checks are evaluated against the path component only, excluding any query parameters. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Add a whitelisted route to the auth config and make a request to that route with query parameters appended (e.g. `/login?redirect=/dashboard`). Verify that the request is correctly skipped for authorization rather than being blocked. ```sh go test ./... ``` ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations This change narrows the input used for whitelist matching to the path only. Ensure that no existing whitelist entries rely on query parameter values for access control decisions, as those will no longer be considered during the skip check. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
e6fa30e to
f156425
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/governance/main.go`:
- Around line 1778-1796: attachVKContext currently only sets governance
TeamID/CustomerID when the FK pointers vk.TeamID/vk.CustomerID are non-nil, but
callers can load relations (vk.Team or vk.Customer) with the ID present even
when the FK pointers are nil; update attachVKContext to also populate
schemas.BifrostContextKeyGovernanceTeamID and
schemas.BifrostContextKeyGovernanceCustomerID from vk.Team.ID and vk.Customer.ID
when vk.TeamID/vk.CustomerID are nil but vk.Team/vk.Customer are non-nil (ensure
you handle the actual field name/type of the nested ID and any pointer/value
differences), while continuing to set the Team/Customer names as before using
ctx.SetValue.
🪄 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: 44a7cad8-99cc-4dcc-acb4-0f679fef9058
📒 Files selected for processing (9)
framework/configstore/migrations.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.goui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/constants/governance.tsui/lib/types/governance.ts
| // attachVKContext sets team and customer governance context values from the virtual key, | ||
| // enabling correct attribution in logs and cost tracking. | ||
| func attachVKContext(ctx *schemas.BifrostContext, vk *configstoreTables.TableVirtualKey) { | ||
| if vk == nil { | ||
| return | ||
| } | ||
| if vk.TeamID != nil { | ||
| ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamID, *vk.TeamID) | ||
| } | ||
| if vk.Team != nil { | ||
| ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamName, vk.Team.Name) | ||
| } | ||
| if vk.CustomerID != nil { | ||
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, *vk.CustomerID) | ||
| } | ||
| if vk.Customer != nil { | ||
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerName, vk.Customer.Name) | ||
| } | ||
| } |
There was a problem hiding this comment.
Populate governance IDs from loaded relations too.
attachVKContext only writes TeamID/CustomerID when the FK pointer fields are non-nil. This plugin already has to handle VKs whose Team/Customer relations are populated while those FK pointers are nil, so that shape is real. In those cases the new shared helper drops governance IDs for passthrough, large-payload, and realtime paths, which can break downstream attribution that keys off the IDs.
🔧 Suggested fix
func attachVKContext(ctx *schemas.BifrostContext, vk *configstoreTables.TableVirtualKey) {
if vk == nil {
return
}
if vk.TeamID != nil {
ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamID, *vk.TeamID)
+ } else if vk.Team != nil {
+ ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamID, vk.Team.ID)
}
if vk.Team != nil {
ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamName, vk.Team.Name)
}
if vk.CustomerID != nil {
ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, *vk.CustomerID)
+ } else if vk.Customer != nil {
+ ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, vk.Customer.ID)
+ } else if vk.Team != nil && vk.Team.Customer != nil {
+ ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, vk.Team.Customer.ID)
}
if vk.Customer != nil {
ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerName, vk.Customer.Name)
+ } else if vk.Team != nil && vk.Team.Customer != nil {
+ ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerName, vk.Team.Customer.Name)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // attachVKContext sets team and customer governance context values from the virtual key, | |
| // enabling correct attribution in logs and cost tracking. | |
| func attachVKContext(ctx *schemas.BifrostContext, vk *configstoreTables.TableVirtualKey) { | |
| if vk == nil { | |
| return | |
| } | |
| if vk.TeamID != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamID, *vk.TeamID) | |
| } | |
| if vk.Team != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamName, vk.Team.Name) | |
| } | |
| if vk.CustomerID != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, *vk.CustomerID) | |
| } | |
| if vk.Customer != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerName, vk.Customer.Name) | |
| } | |
| } | |
| // attachVKContext sets team and customer governance context values from the virtual key, | |
| // enabling correct attribution in logs and cost tracking. | |
| func attachVKContext(ctx *schemas.BifrostContext, vk *configstoreTables.TableVirtualKey) { | |
| if vk == nil { | |
| return | |
| } | |
| if vk.TeamID != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamID, *vk.TeamID) | |
| } else if vk.Team != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamID, vk.Team.ID) | |
| } | |
| if vk.Team != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamName, vk.Team.Name) | |
| } | |
| if vk.CustomerID != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, *vk.CustomerID) | |
| } else if vk.Customer != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, vk.Customer.ID) | |
| } else if vk.Team != nil && vk.Team.Customer != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, vk.Team.Customer.ID) | |
| } | |
| if vk.Customer != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerName, vk.Customer.Name) | |
| } else if vk.Team != nil && vk.Team.Customer != nil { | |
| ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerName, vk.Team.Customer.Name) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/governance/main.go` around lines 1778 - 1796, attachVKContext
currently only sets governance TeamID/CustomerID when the FK pointers
vk.TeamID/vk.CustomerID are non-nil, but callers can load relations (vk.Team or
vk.Customer) with the ID present even when the FK pointers are nil; update
attachVKContext to also populate schemas.BifrostContextKeyGovernanceTeamID and
schemas.BifrostContextKeyGovernanceCustomerID from vk.Team.ID and vk.Customer.ID
when vk.TeamID/vk.CustomerID are nil but vk.Team/vk.Customer are non-nil (ensure
you handle the actual field name/type of the nested ID and any pointer/value
differences), while continuing to set the Team/Customer names as before using
ctx.SetValue.
6711ce3 to
a1beab5
Compare
e389df7 to
a65fce4
Compare

Summary
Introduces an
allow_passthroughflag on virtual key provider configs that gates access to passthrough API endpoints on a per-provider basis. Previously, any virtual key could use passthrough endpoints for any provider without restriction. This change enforces a deny-by-default policy: passthrough must be explicitly enabled per provider on each virtual key.Changes
AllowPassthrough boolfield toTableVirtualKeyProviderConfigwith adefault:falseGORM tag, enforcing deny-by-default for all existing and new provider configs.migrationAddAllowPassthroughToProviderConfig) that adds theallow_passthroughcolumn to the virtual key provider config table, with rollback support.DecisionPassthroughBlockeddecision type and a corresponding 403 error response inEvaluateGovernanceRequest.checkPassthroughPermissionin the governance plugin'sPreLLMHook, which intercepts passthrough and passthrough-stream requests and short-circuits with a 403 if the VK's provider config does not haveAllowPassthroughset to true.allow_passthroughin the HTTP handler for both create and update virtual key endpoints, mapping it through to the provider config table.allow_passthroughto the UI type definitions (VirtualKeyProviderConfig,VirtualKeyProviderConfigRequest,VirtualKeyProviderConfigUpdateRequest) and form schema.PASSTHROUGH_ENABLED_PROVIDERSconstant (openai,anthropic,gemini,vertex,azure) used to conditionally render the passthrough toggle in the UI.PASSTHROUGH_ENABLED_PROVIDERS.PASSTHROUGH_ENABLED_PROVIDERS.Design decision: The flag is deny-by-default. Existing provider configs that relied on passthrough will need to be explicitly opted in after this migration runs.
Type of change
Affected areas
How to test
openaiandallow_passthrough: false(or omit it). Attempt a passthrough request using that virtual key — expect a 403 with decisionpassthrough_not_allowed.allow_passthrough: trueforopenai. Retry the passthrough request — expect it to succeed.PASSTHROUGH_ENABLED_PROVIDERSand that the details sheet reflects the correct enabled/disabled badge.allow_passthroughcolumn is added with a default offalse.Breaking changes
Any virtual key that was previously using passthrough endpoints will have passthrough blocked after this migration runs, since
allow_passthroughdefaults tofalse. Operators must explicitly enableallow_passthroughon the relevant provider configs for each virtual key that requires passthrough access.Security considerations
This change tightens the authorization surface for passthrough endpoints. Previously, possession of a valid virtual key was sufficient to use passthrough for any provider. Now, passthrough access must be explicitly granted per provider on each virtual key, reducing the risk of unintended direct provider access through governance-controlled keys.
Checklist
docs/contributing/README.mdand followed the guidelines