fix(governance): support expiring virtual keys - #3765
Closed
Vaibhav701161 wants to merge 86 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional expiry and post-expiry deletion for virtual keys: DB migrations and model fields, store query/delete helpers and sweeper, governance enforcement, HTTP create/update validation and wiring, and frontend types/UI for configuring and displaying expiry. ChangesVirtual Key Expiry Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Vaibhav701161
force-pushed
the
fix-bf-1171-vk-expiry
branch
6 times, most recently
from
May 29, 2026 07:40
8a43321 to
ab276dc
Compare
Vaibhav701161
marked this pull request as ready for review
May 29, 2026 09:45
Vaibhav701161
marked this pull request as draft
May 29, 2026 09:49
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/components/ui/datePickerWithRange.tsx (1)
272-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the existing default trigger styling.
Defaulting
buttonVariantto"default"changes every existingDateTimePickerconsumer that relied on the previous outline trigger. Keep the default as"outline"and only switch variants when a caller opts in.Suggested fix
-interface DateTimePickerProps extends React.HTMLAttributes<HTMLDivElement> { +interface DateTimePickerProps extends React.HTMLAttributes<HTMLDivElement> { buttonClassName?: string; - buttonVariant?: "default" | "outline" | "ghost" | "secondary"; + buttonVariant?: "default" | "outline" | "ghost" | "secondary"; @@ export function DateTimePicker(props: DateTimePickerProps) { - const { className, buttonClassName, buttonVariant = "default", buttonLabel, triggerLabel, onTrigger, dateTime } = props; + const { className, buttonClassName, buttonVariant = "outline", buttonLabel, triggerLabel, onTrigger, dateTime } = props;Also applies to: 337-350
🤖 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 `@ui/components/ui/datePickerWithRange.tsx` around lines 272 - 285, The default for the trigger button variant was changed and breaks existing consumers: revert the default back to "outline" in the DateTimePickerProps and the DateTimePicker function signature so existing callers keep the previous outline styling; update the buttonVariant default value from "default" to "outline" wherever it appears (reference the DateTimePickerProps definition, the DateTimePicker function parameter destructuring, and the other occurrence around the 337-350 block) so the component only switches variants when a caller explicitly passes a different value.
🤖 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 1808-1832: sweepExpiredVirtualKeys can run concurrently across
instances causing duplicate sweeps; add a small random jitter to the periodic
trigger or wrap the sweep in the existing DistributedLockManager pattern used
elsewhere (e.g., the startup reset usage) so only one instance performs the
delete loop at a time; update the scheduler that calls sweepExpiredVirtualKeys
to add a random +/- jitter to the tick interval, or call
DistributedLockManager.TryWithLock/Acquire around the body of
sweepExpiredVirtualKeys to obtain a short-lived lock before querying and
deleting keys, and ensure the lock is released on error or completion.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 240-297: ExpiryPickerField is missing data-testid attributes on
interactive controls; add unique data-testid values to the Clear button, the
"Never" Button, each preset Button (use label or index to build the id), the
DateTimePicker trigger (e.g. custom-picker), and the delete-after-expiry toggle
control elsewhere in this component; update the Button elements (the Clear
Button, "Never" Button, the map over EXPIRY_PRESETS that calls
presetFromNow(ms)) and the DateTimePicker props to include data-testid props,
and ensure the delete-after-expiry toggle element (where it is defined around
lines 1248-1275) also gets a stable data-testid to match repository E2E
conventions.
In `@ui/components/ui/datePickerWithRange.tsx`:
- Around line 359-399: The layout currently uses a fixed horizontal container
("flex flex-row gap-2") inside PopoverContent which forces the two-month
Calendar and TimePicker onto one row; change that container to a responsive
layout (e.g., "flex flex-col md:flex-row gap-2" or equivalent) so it stacks
vertically on small viewports and switches to a row on medium+ screens; update
any styling on the Calendar/TimePicker wrapper divs if needed to preserve
spacing when stacked; verify the Calendar component usage (selected,
numberOfMonths) still behaves correctly when stacked.
---
Outside diff comments:
In `@ui/components/ui/datePickerWithRange.tsx`:
- Around line 272-285: The default for the trigger button variant was changed
and breaks existing consumers: revert the default back to "outline" in the
DateTimePickerProps and the DateTimePicker function signature so existing
callers keep the previous outline styling; update the buttonVariant default
value from "default" to "outline" wherever it appears (reference the
DateTimePickerProps definition, the DateTimePicker function parameter
destructuring, and the other occurrence around the 337-350 block) so the
component only switches variants when a caller explicitly passes a different
value.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3154c550-b8af-4772-947c-0f89dabc41f8
📒 Files selected for processing (13)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config_test.goui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/components/ui/datePickerWithRange.tsxui/lib/types/governance.ts
Vaibhav701161
force-pushed
the
fix-bf-1171-vk-expiry
branch
from
May 29, 2026 09:51
5e084e3 to
4a4d8c2
Compare
Vaibhav701161
marked this pull request as ready for review
May 29, 2026 09:51
Contributor
Confidence Score: 3/5The core expiry feature is correctly implemented in the resolver and MCP path, but PreRequestHook has two behavioral gaps affecting governance enforcement consistency. In PreRequestHook, the replacement of !virtualKey.IsActiveValue() with virtualKey.IsExpiredAt(...) inadvertently drops the inactive-key early-return, and expired keys are silently allowed through (returning nil instead of a fail-closed error), leaving routing side-effects from invalid keys applied before the downstream rejection. The MCP path handles both conditions explicitly with 403 short-circuits. plugins/governance/main.go — specifically the PreRequestHook guard at line 1112 Important Files Changed
Reviews (8): Last reviewed commit: "chore(ui): drop unrelated date picker ne..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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)
framework/configstore/migrations.go (1)
9002-9014:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an index for the expiry cleanup sweep.
The new cleanup path filters virtual keys by
delete_after_expiryandexpires_at <= now, so documenting “No index” here locks in a full scan ofgovernance_virtual_keyson every sweeper pass. Please add a follow-up migration that creates an index for that cleanup query, using the repo’s concurrent-index pattern on Postgres.As per coding guidelines, "When migrations are added or changed, verify they avoid deadlocks on large tables and create indexes concurrently."
🤖 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 `@framework/configstore/migrations.go` around lines 9002 - 9014, Add a follow-up migration that creates a concurrent Postgres index on governance_virtual_keys(delete_after_expiry, expires_at) so the sweeper query can use the index instead of doing a full table scan; implement it as a new migrator.Migration (e.g., ID "add_index_governance_virtual_keys_delete_after_expiry_expires_at") following the repo's concurrent-index pattern: check the dialect is postgres, ensure the CREATE INDEX CONCURRENTLY is executed outside a transaction (no TX wrapping), test for existence first (pg_catalog or pg_indexes) and run tx.Exec("CREATE INDEX CONCURRENTLY IF NOT EXISTS ...") or equivalent so it avoids locking and deadlocks on large tables; reference the existing migrationAddVirtualKeyExpiresAtColumn and tables.TableVirtualKey when locating where to add this migration.
🤖 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.
Outside diff comments:
In `@framework/configstore/migrations.go`:
- Around line 9002-9014: Add a follow-up migration that creates a concurrent
Postgres index on governance_virtual_keys(delete_after_expiry, expires_at) so
the sweeper query can use the index instead of doing a full table scan;
implement it as a new migrator.Migration (e.g., ID
"add_index_governance_virtual_keys_delete_after_expiry_expires_at") following
the repo's concurrent-index pattern: check the dialect is postgres, ensure the
CREATE INDEX CONCURRENTLY is executed outside a transaction (no TX wrapping),
test for existence first (pg_catalog or pg_indexes) and run tx.Exec("CREATE
INDEX CONCURRENTLY IF NOT EXISTS ...") or equivalent so it avoids locking and
deadlocks on large tables; reference the existing
migrationAddVirtualKeyExpiresAtColumn and tables.TableVirtualKey when locating
where to add this migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce509372-6f02-4eb7-8c62-d819880dcd72
📒 Files selected for processing (2)
framework/configstore/migrations.gotransports/bifrost-http/handlers/governance.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@framework/configstore/rdb.go`:
- Around line 2957-2961: The DeleteExpiredVirtualKey implementation currently
issues a direct Delete on tables.TableVirtualKey which bypasses the
cleanup/contract enforced by DeleteVirtualKey and can leave
governance/OAuth/MCP/budget/rate-limit state inconsistent; change this path to
locate expired virtual key rows (using the same predicate: id,
delete_after_expiry, expires_at <= time.Now().UTC()) and for each matching id
call RDBConfigStore.DeleteVirtualKey(ctx, id) (or extract the shared cleanup
logic into a helper used by both DeleteVirtualKey and DeleteExpiredVirtualKey)
so all dependent state is cleaned consistently for TableVirtualKey entries.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 812-814: The MockConfigStore.DeleteExpiredVirtualKey currently
always returns true, which masks failure paths; change the implementation of
MockConfigStore.DeleteExpiredVirtualKey to return the neutral zero value (false,
nil) instead of (true, nil) so unit tests don't assume deletes succeed, and rely
on SQLite-backed integration tests to exercise successful delete behavior;
locate the method named DeleteExpiredVirtualKey on the MockConfigStore and
update its return values accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e6dabab-49ed-4aa5-acd3-8a422989738a
📒 Files selected for processing (4)
framework/configstore/rdb.goframework/configstore/store.goplugins/governance/main.gotransports/bifrost-http/lib/config_test.go
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/virtual-keys/views/virtualKeySheet.tsx (1)
392-400: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using the existing
toDatetimeLocalhelper.The inline IIFE correctly converts UTC to local datetime-local format, but the
toDatetimeLocalhelper defined above does the same thing more simply and would be consistent with how dates are converted elsewhere in this file.expiresAt: virtualKey?.expires_at - ? (() => { - const d = new Date(virtualKey.expires_at); - return new Date(d.getTime() - d.getTimezoneOffset() * 60000) - .toISOString() - .slice(0, 16); - })() + ? toDatetimeLocal(new Date(virtualKey.expires_at)) : null,🤖 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 `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 392 - 400, Replace the inline IIFE used to convert virtualKey.expires_at into a datetime-local string with the existing toDatetimeLocal helper to keep conversion consistent; specifically, in the object construction that sets expiresAt (currently using (() => { const d = new Date(virtualKey.expires_at); ... })()), call toDatetimeLocal(virtualKey.expires_at) when virtualKey?.expires_at is truthy and leave null otherwise, leaving deleteAfterExpiry assignment unchanged.
🤖 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.
Outside diff comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 392-400: Replace the inline IIFE used to convert
virtualKey.expires_at into a datetime-local string with the existing
toDatetimeLocal helper to keep conversion consistent; specifically, in the
object construction that sets expiresAt (currently using (() => { const d = new
Date(virtualKey.expires_at); ... })()), call
toDatetimeLocal(virtualKey.expires_at) when virtualKey?.expires_at is truthy and
leave null otherwise, leaving deleteAfterExpiry assignment unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cec4ff9-eb6a-4815-b9b9-012b6e68f1b0
📒 Files selected for processing (2)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/datePickerWithRange.tsx
akshaydeo
force-pushed
the
dev
branch
2 times, most recently
from
May 31, 2026 08:03
8c3e42e to
b95e8e7
Compare
|
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. |
akshaydeo
force-pushed
the
dev
branch
3 times, most recently
from
June 8, 2026 11:25
e389df7 to
a65fce4
Compare
Contributor
|
@Vaibhav701161 will you finish this PR? if so, I would close mine #3229 which is a bit more barebones than this i think |
## Summary Adds an "Allow Private Network" toggle to the custom provider creation form, enabling users to configure whether a custom provider can connect to private network IP ranges (e.g., `192.168.x.x`, `10.x.x.x`). Link-local addresses remain blocked regardless of this setting. ## Changes - Added `allow_private_network` as an optional boolean field to the custom provider form schema, defaulting to `false` - Wired the field value into `network_config.allow_private_network` when saving the provider - Added a labeled toggle switch in the form UI with a description clarifying which address ranges are affected and which remain blocked ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test 1. Navigate to the custom provider creation sheet in the workspace providers UI. 2. Verify the "Allow Private Network" toggle is visible and defaults to off. 3. Enable the toggle and save the provider — confirm `allow_private_network: true` is included in the saved `network_config`. 4. Disable the toggle and save — confirm `allow_private_network: false` is sent. 5. Verify the toggle is disabled when the user lacks provider create access. ```sh cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` ## Screenshots/Recordings _Add before/after screenshots of the custom provider form showing the new toggle._ ## Breaking changes - [ ] Yes - [x] No ## Related issues Link related issues and discussions. Example: Closes #123 ## Security considerations This toggle explicitly opts a custom provider into connecting to private network ranges. It defaults to `false` (blocked), preserving the existing secure-by-default behavior. Link-local addresses remain blocked unconditionally to prevent SSRF via metadata endpoints. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an "Allow Private Network" toggle option in the custom provider creation form, enabling users to control private network access settings when setting up custom providers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Adds two new E2E routing scenarios to validate alias gate enforcement and alias collision behavior. These tests cover edge cases in key-level alias resolution that were previously untested. ## Changes - Added a `reverse-alias-gate` scenario that verifies a key's model gate is enforced on the alias name, not the resolved model ID. Routing via the alias succeeds and resolves to the underlying model, but routing directly to the resolved model ID (e.g., `gpt-4o-mini`) returns a 400 with "no keys found that support model" — alias targets are not implicitly added to a key's allowed models. - Added an `alias-collision-last-wins` scenario that verifies deterministic behavior when two keys define the same alias pointing to different models. The last-defined key's alias target wins, and routing confirms both the correct key and resolved model ID. - Both scenarios include setup, routing assertions with retry/polling logic, and cleanup steps in the Postman collection and the builder script. ## 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 E2E Postman collection against a live environment: ```sh # Regenerate the collection from the builder script node tests/e2e/api/runners/build-routing-wiring.mjs # Run the collection with Newman newman run tests/e2e/api/collections/bifrost-routing-wiring.postman_collection.json \ --env-var base_url=<your_base_url> \ --env-var run_id=<unique_run_id> ``` Expected outcomes: - `reverse-alias-gate`: Step 03 returns 200 with `resolved_key_alias.model_id = "gpt-4o-mini"`; Step 04 returns 400 with error message containing "no keys found that support model". - `alias-collision-last-wins`: Step 04 returns 200, routed via key `k2`, with `resolved_key_alias.model_id = "gpt-4o-mini"`. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. These are test-only changes that exercise existing routing and key gate logic. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded end-to-end test coverage for alias routing behavior: added scenarios that verify routing via an alias succeeds, that routing directly to the resolved model id is rejected when not permitted, and that when multiple keys declare the same alias the last-defined key wins. These tests validate alias resolution, gating, and collision handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…4252) ## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes #123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Enhanced model-catalog wiring tests with richer scenario generation and expanded assertions. * Added variable-based absence and non-empty expectations for model listings. * New test steps to capture live models, verify providers, assert model details, and assert base-model membership. * Broader scenario coverage: wildcard re-gating, multi-key aggregation, discovery degradation, keyless providers, and explicit allow-list behaviors. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ck e2e scenarios (#4253) ## Summary This PR adds two new E2E routing scenarios and fixes the field ordering of `weight` in virtual key provider config payloads across all existing test cases. It also updates the VK builder to support omitting `weight` entirely, enabling "weightless" (allow-list-only) virtual key configurations that skip load balancing. ## Changes - **Weightless VK allow-list scenario**: Adds a new E2E test (`weightless-vk-allowlist-via-logs`) that creates a VK with two providers and no weights. Provider A serves `gpt-4o-mini`, Provider B serves only `gpt-4o`. Routing `gpt-4o-mini` confirms governance filters out Provider B by capability and, finding no weighted configs, skips load balancing entirely and routes to Provider A. The `routing_engine_logs` are asserted to contain `"not in allowed models list"`, `"No weighted configs"`, and `"skipping load balancing"`. - **Case-insensitive alias fallback scenario**: Adds a new E2E test (`alias-case-insensitive-fallback`) that registers a mixed-case alias (`CatWiring-CI-{{run_id}}`) on a key and routes the lowercased form. The test asserts that the router resolves the alias via a case-insensitive fallback and that `routing_info.resolved_key_alias.model_id` equals `gpt-4o-mini`. - **`weight: null` support in VK builder**: The `createVK` step in `build-routing-wiring.mjs` now omits the `weight` field entirely when `pc.weight === null`, rather than defaulting to `1`. This is what enables the weightless VK scenario above. - **Field ordering normalization**: Moved `weight` to the end of the provider config object in all existing VK creation payloads throughout the Postman collection, making the ordering consistent with the new builder output. ## 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 Run the Postman collection or the Newman-based E2E runner against a live Bifrost instance: ```sh # Run the full routing-wiring collection via Newman newman run tests/e2e/api/collections/bifrost-routing-wiring.postman_collection.json \ --environment <your-env-file> \ --folder "A weightless VK is an allow-list (no LB), confirmed by the routing log trail" \ --folder "A request resolves to an alias whose name differs only in case" ``` To regenerate the Postman collection from the scenario definitions: ```sh node tests/e2e/api/runners/build-routing-wiring.mjs ``` Verify the generated collection matches the committed one. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. Changes are limited to E2E test definitions and the test collection builder. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added scenarios validating "weightless" virtual-key behavior that enforces allow-list filtering and skips load balancing, with assertions from routing logs. * Added scenario verifying case-insensitive alias resolution. * Adjusted test request payloads to reorder fields for consistency and clearer validation across routing/governance scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…tep (#4254) ## Summary Adds end-to-end test coverage for request-level fallback routing and VK-governed fallback behaviour. Previously, the routing wiring collection had no tests exercising the fallback path. This PR introduces four new scenarios covering the core fallback contract and adds a pre-run setup step that clears all providers before any scenario executes, eliminating cross-run collisions caused by leftover providers from prior or interrupted runs. ## Changes - Added a `clearProvidersFolder()` helper in `collection-builder.mjs` that emits a two-request loop (list → delete) at the top of every collection, draining all existing providers before scenarios begin. The `__purge_target` and `__purge_queue` collection variables that drive the loop are declared in `buildCollection`. - Added a `badKey` flag to the `key()` helper and `keyBody()` in `build-routing-wiring.mjs`. When set, the key receives a hardcoded invalid credential (`sk-deadbeef-invalid-000`) instead of a real env-backed value, forcing an upstream auth failure to exercise the fallback path without needing a separate provider type. - Added `expectIsFallback`, `expectPrimaryProviderRef`, and `expectPrimaryModel` assertion fields to `routeAssertLines()`, and a `fallbacks` field to route steps that serialises fallback targets as `"provider/model"` strings on the `/v1/chat/completions` request body. - Added four new routing scenarios: - **`fallback-cross-provider`**: primary has an invalid key; a single request-level fallback to a healthy second provider succeeds, and `routing_info` reports `is_fallback=true` with the original primary recorded. - **`fallback-chain-first-healthy-wins`**: primary and first fallback both have invalid keys; the second fallback in the chain succeeds. - **`fallback-pruned-by-vk-allowlist`**: a VK that permits only the primary provider causes the off-allowlist fallback to be pruned before the attempt loop, so the request fails with 401 rather than being rescued. - **`vk-auto-attached-fallback`**: a VK with two weighted providers auto-attaches the non-primary configs as fallbacks; with the dominant-weight provider's key broken, all six sampled requests land on the healthy low-weight provider. - Regenerated both `bifrost-routing-wiring.postman_collection.json` and `bifrost-model-catalog-wiring.postman_collection.json` to include the clear-providers setup folder and the new scenario items. ## 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 Run the routing wiring collection against a local Bifrost instance with `OPENAI_API_KEY` set in the environment: ```sh newman run tests/e2e/api/collections/bifrost-routing-wiring.postman_collection.json \ --env-var base_url=http://localhost:8080 \ --env-var run_id=$(date +%s) ``` The four new scenarios (`fallback-cross-provider`, `fallback-chain-first-healthy-wins`, `fallback-pruned-by-vk-allowlist`, `vk-auto-attached-fallback`) should all pass. The setup folder at the top of the run should complete without error regardless of whether providers already exist on the instance. To regenerate the collection from the builder after further changes: ```sh node tests/e2e/api/runners/build-routing-wiring.mjs ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The invalid credentials used in `badKey` scenarios (`sk-deadbeef-invalid-000`) are intentionally non-functional placeholder values and are never sourced from environment secrets. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added comprehensive E2E coverage for fallback routing: request-level provider failovers, ordered fallback chaining, allowlist pruning of fallbacks, and governance-driven auto-attached fallbacks when primaries fail. * Implemented automated provider cleanup as part of test setup to ensure a clean environment before running E2E suites. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…outing harness ledger docs (#4255) ## Summary This PR documents the latency trade-offs of semantic caching, introduces a local routing harness ledger convention for e2e test journaling, and adds the corresponding gitignore entry to keep those ledger files out of version control. ## Changes - Added a `<Warning>` block to the semantic caching docs explaining the latency overhead for direct lookups, semantic lookups, and cache writes — including the nuance that a semantic cache hit still costs an embedding round-trip, and a semantic miss pays that cost on top of the full LLM call. - Added a row to the direct vs. semantic comparison table covering added latency per mode. - Added a `Routing Harness Ledger` section to the e2e API README describing the daily journaling convention (`routing/ledger-YYYY-MM-DD.md`), its structure, and the rule against rewriting past days. - Added `tests/e2e/api/routing/ledger-*` to `.gitignore` so local run journals are never committed. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Verify the docs render correctly and that the gitignore pattern excludes ledger files as expected: ```sh # Confirm ledger files are ignored touch tests/e2e/api/routing/ledger-2025-01-01.md git status # should not appear as an untracked file ``` Review the updated semantic caching docs to confirm the warning block and table row render as intended. ## Breaking changes - [x] No ## Related issues N/A ## Security considerations None. No code changes; no secrets, auth, or PII involved. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a Latency overhead warning to semantic caching docs describing added costs for cache reads and asynchronous writes * Clarified direct vs. semantic caching comparison with an explicit “Added latency” row * New routing harness ledger guidance describing daily ledger entries and append-only practices * **Chores** * Updated ignore rules to exclude local test artifacts and routing ledger journal files <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ield directly (#4279) ## Summary Simplifies the load balancing skip logic in the governance plugin by leveraging the provider value already parsed from `GetRequestFields()`, and bumps several indirect dependencies in the model catalog resolver module. ## Changes - The `loadBalanceProvider` function previously discarded the provider returned by `GetRequestFields()` and re-parsed it manually from the model string when a `/` was detected, with additional branching to check the in-memory store for configured providers. This logic is replaced by a straightforward check: if `provider` is already non-empty (as returned directly by `GetRequestFields()`), skip load balancing immediately. This removes the dependency on `inMemoryStore` for this check and eliminates the `strings.Contains` slash-detection path. - Bumped `cloud.google.com/go/iam` from `v1.5.3` to `v1.7.0`. - Bumped `github.com/aws/aws-sdk-go-v2` from `v1.41.7` to `v1.41.12`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/configsources` from `v1.4.23` to `v1.4.28`. - Bumped `github.com/aws/aws-sdk-go-v2/internal/endpoints/v2` from `v2.7.23` to `v2.7.28`. - Bumped `github.com/aws/smithy-go` from `v1.25.1` to `v1.27.1`. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/governance/... ``` Verify that requests with a provider already set on the request are not load balanced, and that requests without a provider still proceed through load balancing as expected. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Optimized load balancing logic for more efficient provider selection * **Chores** * Updated dependencies including Google Cloud IAM and AWS SDK modules to latest stable versions <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…r of AES-only encryption (#4245) ## Summary Removes Vault-based secret storage from all sensitive-field GORM hooks and related database operations, leaving only the standard `encrypt` package path for at-rest encryption. This eliminates the dual-path complexity that existed for MCP client configs, OAuth tokens, sessions, temp tokens, and vector store configs. ## Changes - Removed all `VaultIsEnabled()` branches from `BeforeSave`, `AfterFind`, and `AfterDelete` hooks across `TableMCPClient`, `TableMCPPerUserHeaderCredential`, `TableOauthToken`, `TableOauthUserSession`, `TableOauthUserToken`, `SessionsTable`, `TempToken`, and `TableVectorStoreConfig`. - Removed `AfterDelete` vault cleanup hooks from all affected table types. - Removed `DeleteVaultSecrets` helper methods from `TableOauthUserToken`, `TableOauthUserSession`, and `TempToken`. - Removed pre/post-transaction vault compensation logic (`vaultStoredPaths`, `vaultRemovePaths`) from `UpdateMCPClientConfig` in `rdb.go`. - Removed the pre-transaction vault ID collection and post-transaction goroutine vault cleanup from `DeleteMCPClientConfig`, moving the record lookup inside the transaction instead. - Removed vault ID pre-collection and post-delete goroutine cleanup from `DeleteTempTokensByResourceID` and `DeleteExpiredTempTokens`. - Bumped `cloud.google.com/go/iam`, `aws-sdk-go-v2`, and `smithy-go` dependency versions in the `modelcatalogresolver` plugin. ## 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 ./framework/configstore/... go test ./framework/configstore/tables/... ``` Verify that MCP client configs, OAuth tokens, sessions, and temp tokens are correctly encrypted and decrypted using the `encrypt` package when `encrypt.IsEnabled()` is true, and that no vault-related paths are written or read. ## Breaking changes - [x] Yes - [ ] No Any deployments that previously relied on Vault-backed secret storage for these table types will no longer have secrets written to or read from Vault. Rows with `encryption_status = 'vault'` will not be decrypted correctly after this change. A migration to re-encrypt existing vault-backed rows using the standard encryption path is required before deploying. ## Security considerations Vault integration for field-level secret storage has been removed. All sensitive fields (OAuth tokens, MCP connection strings, headers, session tokens, temp tokens, vector store config) are now exclusively encrypted via the `encrypt` package. Ensure the `encrypt` package key material is properly secured in your deployment environment, as Vault is no longer available as an alternative secret backend. ## 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 Expands the Datadog connector's Helm chart and documentation to expose the full set of connector configuration fields, and adds environment variable substitution support for `agent_addr` and `dogstatsd_addr`. ## Changes - `agent_addr` and `dogstatsd_addr` now accept `EnvVar` values (e.g. `env.DD_AGENT_ADDR`, `env.DD_DOGSTATSD_ADDR`), enabling dynamic address resolution at runtime — useful for injecting a node-local Datadog agent's address via `status.hostIP` in Kubernetes. - Added `ml_app`, `dogstatsd_addr`, `enable_metrics`, `enable_llm_obs`, `disable_content_logging`, `agentless`, `api_key`, `site`, and `request_headers` to the Helm chart's `_helpers.tpl`, `values.schema.json`, and `values.yaml` so all connector options are configurable via Helm. - Added a `version` field to the connector schema at the top level. - Updated `values.yaml` with inline comments documenting agentless mode, env var substitution, and optional fields. - Updated documentation to reflect that `agent_addr` and `dogstatsd_addr` support `env.VAR_NAME` substitution, and added corresponding examples to the environment variable substitution section. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Deploy Bifrost via Helm with the Datadog connector configured using `env.VAR_NAME` references for `agent_addr` and `dogstatsd_addr`, and verify the connector resolves the addresses from the injected environment variables at runtime. ```sh helm upgrade --install bifrost ./helm-charts/bifrost \ --set bifrost.connectors.datadog.enabled=true \ --set bifrost.connectors.datadog.config.agent_addr="env.DD_AGENT_ADDR" \ --set bifrost.connectors.datadog.config.dogstatsd_addr="env.DD_DOGSTATSD_ADDR" ``` Confirm that previously unsupported fields (`ml_app`, `enable_metrics`, `enable_llm_obs`, `agentless`, `api_key`, `site`, `request_headers`) are correctly rendered into the generated config. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `api_key` supports `env.VAR_NAME` substitution, ensuring Datadog API keys are not hardcoded in Helm values and can be injected securely via Kubernetes secrets or environment variables. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified env.VAR_NAME substitution for Datadog config fields with updated examples; updated metrics reference for renamed metric and type change with migration guidance; added new automatic tag (bifrost_node). * **New Features** * Added expanded Datadog configuration options: ML/LLM observability toggles, metrics enablement, agentless/API settings, DogStatsD address, request header support, and app identification; Helm charts now validate required API key when agentless is enabled. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
) ## Summary When creating virtual keys via governance config, if a virtual key does not have an ID set, it would be stored without one, causing failures or inconsistent state. This PR ensures a UUID is automatically assigned to any virtual key that is missing an ID before it is persisted. ## Changes - Added a check during governance config creation to assign a new UUID to any virtual key whose `ID` field is empty, preventing virtual keys from being created without a valid identifier. ## 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 Configure a governance config with a virtual key that has no `ID` field set and trigger the config store creation. Verify that the virtual key is created successfully with an auto-generated UUID. ```sh go test ./transports/bifrost-http/... ``` ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No security implications. UUID generation uses a standard random UUID, which does not expose any sensitive information. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed an issue where virtual keys could be created without identifiers during system initialization and governance configuration operations. The system now ensures each virtual key automatically receives a unique identifier if one is not provided, preventing data inconsistencies and improving system reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…arge-payload path (#4282) ## Summary When a large-payload request arrives via the `PreRequestHook` path, the request body is not parsed, so `req.Model` is taken directly from metadata and may carry a provider-prefixed string like `openai/gpt-4o`. Previously, `runPreRequestRouting` passed this raw string directly as the model name without parsing the provider prefix, causing load balancing to ignore the caller's explicit routing intent. This change ensures the provider prefix is extracted and honored in the same way the transport layer handles body-having requests. ## Changes - `runPreRequestRouting` now calls `schemas.ParseModelString` on the incoming model string before constructing the synthetic `BifrostRequest`, populating both `Provider` and `Model` on the `ChatRequest` so that an explicit prefix like `openai/gpt-4o` correctly pins routing to that provider instead of being subject to VK load balancing. - Added `prerequestrouting_test.go` covering three cases: an explicit provider prefix bypasses load balancing and always resolves to the specified provider; a bare model string still goes through VK load balancing and returns provider-prefixed; and an unknown slash-containing prefix (e.g. a HuggingFace-style namespace like `meta-llama/llama-3.1-8b-instant`) is treated as part of the model name with load balancing still applied. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/governance/... ``` Expected: all three new tests pass, confirming that provider-prefixed models in the large-payload path are routed to the correct provider, bare models still load balance, and unknown slash-prefixes are preserved as model namespaces. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. This change only affects internal routing logic for constructing synthetic requests during pre-request hook processing. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Fixed request routing to properly preserve explicit provider specifications in model names (e.g., `provider/model` format), ensuring routing intent is maintained for large-payload requests. * **Tests** * Added comprehensive tests for pre-request routing behavior, including explicit provider prefix handling and load balancing scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
… failed provider config (#4258) ## Summary When processing an authoritative provider fails, the provider's file-based config should still be written to the store rather than silently falling back to the previously persisted config. The old behavior preserved the existing store entry on error, which could mask bad config and prevent updates from taking effect. ## Changes - On a failed `processAuthoritativeProvider` call, the provider entry in `authoritativeProviders` is now set to `providerCfgInFile` (the config as read from the file) instead of `existingCfg` (the previously persisted config). - This ensures that a malformed or invalid provider config in the file is surfaced and written through, rather than silently falling back to stale data that could hide the problem. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Deploy a configuration with a provider entry that triggers a processing error and verify that the provider's config in the store reflects the file-based config rather than the previously persisted value. ```sh go test ./... ``` ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No direct security implications. Ensuring the file-based config is authoritative prevents stale or unintended provider configs from persisting silently in the store. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Authoritative provider entries from config files are now always processed during reconciliation, streamlining validation and merge behavior. * Validation failures are logged as warnings but no longer block processing; missing key IDs are generated and provider/key data are normalized. * **Bug Fix** * When a provider already exists, file-specified keys are merged with stored keys while preserving the provider's existing status and description, preventing inadvertent pruning. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Extends the Vertex provider's Files API to work with customer-owned GCS buckets via the OpenAI-compatible drop-in (`/openai/v1/files`) and the native resumable upload path. Previously, file operations (upload, list, retrieve, delete, content download) were only wired for Bedrock (S3) and Gemini. This PR adds the same CRUD surface for Vertex using GCS as the backing store, fixes a `content_length` type-coercion bug in the resumable upload path, and introduces opaque base64 encoding for `gs://` file IDs so they round-trip safely through URL path segments without requiring callers to percent-encode slashes. ## Changes - **`gs://` file ID encoding**: `gs://` URIs returned by Vertex contain slashes that break single-segment path routing on retrieve/delete/content endpoints. Upload, list, retrieve, and delete responses now base64-encode `gs://` IDs via `encodeStorageFileID`; incoming path parameters are decoded via `decodeStorageFileID` (which also falls back to percent-decoding for raw or percent-encoded URIs passed directly). - **OpenAI integration layer**: Extended the `Bedrock`-only base64 encode/decode branches in `CreateOpenAIFileRouteConfigs` and `extractFileIDFromPath` to also cover `Vertex`. Added GCS bracket-notation query/form parsing (`storage_config[gcs][bucket]`, `storage_config[gcs][prefix]`) in `extractFileListQueryParams` and `parseOpenAIFileUploadMultipartRequest`. - **`content_length` type coercion fix**: The resumable GCS upload session minter previously only accepted `float64` for `content_length` in `ExtraParams`. It now handles `int`, `int64`, and `string` (via `gcsParseSize`) so the `X-Upload-Content-Length` header is set correctly regardless of how the value arrives. - **Provider harness test collection**: Added a new folder `11b. Vertex GCS Files` with seven `[PREVIEW]`-tagged requests covering upload, list, retrieve, content download, delete (OpenAI drop-in), mint resumable session, PUT bytes directly to GCS, and resumable cleanup (native). Content-shape validation is skipped for `/files/.../content` and direct GCS storage URLs to avoid false positives. - **Python integration tests**: File tests (41–45) are refactored from Bedrock-only to provider-agnostic via a new `get_file_storage_config` helper that returns the appropriate `s3` or `gcs` storage config and skips when the backing bucket is not configured. Vertex file scenarios are enabled in `config.yml`. - **Makefile**: `VERTEX_GCS_BUCKET`, `VERTEX_GCS_PREFIX`, and `VERTEX_API_KEY` environment variables are forwarded to Newman as `vertexGcsBucket`, `vertexGcsPrefix`, and `vertexKey` in all three harness runner branches. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh # Unit / build go test ./... # Provider harness (requires a configured Vertex key + GCS bucket) VERTEX_API_KEY=<key> \ VERTEX_GCS_BUCKET=<bucket> \ VERTEX_GCS_PREFIX=bifrost-e2e/ \ make run-provider-harness-test FOLDER="11b. Vertex GCS Files" # Python integration tests cd tests/integrations/python pytest tests/test_openai.py -k "test_41 or test_42 or test_43 or test_44 or test_45" ``` Set the following environment variables to enable Vertex GCS file tests: | Variable | Description | |---|---| | `VERTEX_API_KEY` | Vertex AI API key (forwarded as `vertexKey` to Newman) | | `VERTEX_GCS_BUCKET` | GCS bucket name used for file storage | | `VERTEX_GCS_PREFIX` | Object prefix within the bucket (e.g. `bifrost-e2e/`) | ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations GCS bucket names and prefixes are passed as user-supplied form fields and query parameters. They are forwarded directly to the Vertex provider and used only to construct GCS object paths; no credentials are derived from them. The base64 encoding of `gs://` IDs is for URL-safety only and provides no confidentiality guarantee — callers should treat file IDs as opaque handles. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Vertex-backed file management with GCS: upload (including resumable), list, retrieve, download, and delete via Files API. * **Bug Fixes** * Safer file ID handling for URLs by making Vertex/GCS IDs opaque and path-safe. * More robust handling of content-length for Vertex uploads. * **Tests** * Expanded e2e and integration tests covering Vertex GCS file flows and resumable uploads; shared test helpers for storage config. * **Chores** * Test harness help output documents Vertex GCS env vars and forwards them to test runs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary
Adds full OpenAI-compatible batch API support for Vertex AI (create, retrieve, list, cancel) via the HTTP transport, and fixes ID stability issues in both the Vertex and Gemini batch implementations.
## Changes
- **Vertex batch IDs are now base64 (RawURLEncoding) encoded/decoded** in the OpenAI HTTP integration, matching the existing Bedrock pattern. Vertex batch IDs are full resource names (`projects/.../batchPredictionJobs/{id}`) and `input_file_id` values are `gs://` URIs — both contain slashes that would break URL path routing without encoding.
- **Gemini batch list** now returns the full resource name (`batches/<id>`) as the ID instead of stripping the prefix, making IDs stable and consistent with create/retrieve responses.
- **Vertex cancel and delete** now echo back the caller's batch ID directly instead of re-extracting a bare job ID from the resource name, keeping IDs stable across the full lifecycle.
- **`extractBatchIDFromName` (Gemini) and `vertexBatchJobIDFromName` (Vertex)** helper functions removed since IDs are no longer stripped.
- **Vertex `BatchCreate` supports raw-passthrough mode**: validation and typed-field extraction are skipped when a raw request body is present, allowing callers to pass arbitrary Vertex `BatchPredictionJob` payloads directly.
- **Vertex `BatchCreate` no longer strips `output_uri`/`gcs_bucket`/`gcs_prefix` control params** from `extra_params` before forwarding to Vertex, since those keys are no longer injected by the typed path.
- **Vertex region validation** relaxed: the `"global"` region is no longer explicitly rejected; only an empty region is an error.
- **Integration test config** updated to enable Vertex batch scenarios (`batch_file_upload`, `batch_list`, `batch_retrieve`, `batch_cancel`) and disable virtual key testing temporarily.
- **Integration tests** added for Vertex batch create-with-file, retrieve, cancel, and full e2e (upload → create → poll → list → cleanup).
## Type of change
- [ ] Bug fix
- [x] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./core/providers/vertex/...
go test ./core/providers/gemini/...
go test ./transports/bifrost-http/...
```
For integration tests, set the following environment variables and run the Python test suite:
```sh
export VERTEX_PROJECT_ID=<your-project>
export VERTEX_REGION=<e.g. us-central1>
export VERTEX_GCS_BUCKET=<your-bucket>
# Then run:
pytest tests/integrations/python/tests/test_openai.py -k "batch"
```
Expected: Vertex batch create, retrieve, cancel, list, and e2e file API tests pass. Batch IDs returned by create round-trip correctly through retrieve and cancel without modification.
## Breaking changes
- [x] Yes
- [ ] No
Vertex batch IDs returned by the OpenAI HTTP transport are now base64 (RawURLEncoding) encoded. Any client storing a Vertex batch ID from a previous version will need to re-create the batch, as old bare IDs will not decode correctly. Gemini batch list IDs now include the `batches/` prefix where previously only the suffix was returned.
## Related issues
## Security considerations
Batch IDs and `input_file_id` values are base64-encoded for URL safety only; no secrets or PII are introduced. GCS URIs passed as `input_file_id` are opaque to the transport layer and are only decoded immediately before being forwarded to the Vertex API.
## 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
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* Vertex batch functionality: file upload, create, list, retrieve, and cancel operations now supported
* Raw passthrough mode for batch API requests enabling native request handling
* **Improvements**
* Consistent batch ID encoding across providers using URL-safe base64 format
* Streamlined Vertex endpoint configuration by removing region constraints
* **Tests**
* Expanded integration and end-to-end test coverage for Vertex batch workflows
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
…andleRequest`/`handleStreamRequest` (#4285) ## Summary Removes the routing allowlist enforcement (`enforceRoutingAllowlist`) that was previously applied inside `handleRequest` and `handleStreamRequest`. This eliminates the gate that blocked requests when the resolved provider was not present in the `BifrostContextKeyRoutingAllowedProviders` allowlist published by plugins. ## Changes - Removed the `enforceRoutingAllowlist` function entirely, which checked the resolved provider against a plugin-supplied allowlist and pruned fallbacks accordingly. - Removed the calls to `enforceRoutingAllowlist` in both `handleRequest` and `handleStreamRequest`. ## 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 version go test ./... ``` Verify that requests to providers previously blocked by a routing allowlist now proceed without a `400 Bad Request` error. Confirm that plugin-based governance restrictions relying on `BifrostContextKeyRoutingAllowedProviders` no longer block routing at this layer. ## Screenshots/Recordings N/A ## Breaking changes - [x] Yes - [ ] No Any plugin that relied on `BifrostContextKeyRoutingAllowedProviders` to restrict which providers could be used will no longer have that restriction enforced at the routing layer. Governance plugins using this mechanism will need an alternative enforcement strategy. ## Related issues N/A ## Security considerations Previously, the allowlist enforcement ensured that plugin-imposed provider restrictions (e.g., governance or virtual key policies) could not be bypassed by user-specified provider prefixes or downstream layers. With this removal, that enforcement layer no longer exists. Any equivalent access control must now be handled elsewhere. ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Removed routing allowlist enforcement and fallback pruning from the orchestration flow, so resolved primary providers and fallback lists are no longer validated or filtered at that stage. The existing fallback decision flow remains in place; overall routing orchestration logic is simplified. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com> (cherry picked from commit e87c7cc)
- Add comment at each nil,nil guard in HTTPTransportPreHook, governLargePayload, and governRealtimeQueryParam explaining that blocking is deferred to PreLLMHook (fail-closed behavior documented) - Add Expired badge to visible table Status column; expired VKs show destructive badge instead of the active toggle (same boundary as backend) - Use now := time.Now().UTC() in create/update handler validation to avoid repeated inline clock calls Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com> (cherry picked from commit 55f1f108b30ce2244f2cb69e75a71d72cc4587aa) Signed-off-by: Vaibhav mittal <vaibhavmittal929@gmail.com>
Update DateTimePicker's popover to use the same flex-row/2-month calendar layout and time-row structure as DateTimePickerWithRange (the Logs calendar). VK expiry custom picker now matches the Logs date picker style visually. DateTimePickerWithRange (Logs) is unchanged.
Vaibhav701161
force-pushed
the
fix-bf-1171-vk-expiry
branch
from
June 11, 2026 09:55
8ccffbf to
de51b89
Compare
Pratham-Mishra04
requested changes
Jun 11, 2026
| ID: "add_virtual_key_expires_at_column", | ||
| Migrate: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| if !tx.Migrator().HasColumn(&tables.TableVirtualKey{}, "expires_at") { |
Collaborator
There was a problem hiding this comment.
use gorm to create this column
|
|
||
| // IsExpiredAt reports whether the virtual key has passed its expiry. | ||
| // now == expires_at is treated as expired; nil ExpiresAt means never expires. | ||
| func (vk *TableVirtualKey) IsExpiredAt(now time.Time) bool { |
Collaborator
There was a problem hiding this comment.
dont take now, initialize here onlt
| var ok bool | ||
| virtualKey, ok = p.store.GetVirtualKey(ctx, virtualKeyValue) | ||
| if !ok || virtualKey == nil || !virtualKey.IsActiveValue() { | ||
| if !ok || virtualKey == nil || virtualKey.IsExpiredAt(time.Now().UTC()) { |
Collaborator
There was a problem hiding this comment.
any reason to remove is active value?
Comment on lines
+1112
to
1114
| if !ok || virtualKey == nil || virtualKey.IsExpiredAt(time.Now().UTC()) { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
PreRequestHook
When an expired key is detected here, the function returns nil (no error). That falls through to stampGovernanceCtxFromVK(ctx, nil), sets no governance context, and the request continues to EvaluateGovernanceRequest in PreLLMHook. Because virtualKey is nil, governance constraints tied to this VK (provider allow-listing, budgets, rate limits) are bypassed for the routing phase. Contrast with PreMCPHook in the same PR, which correctly returns a 403 short-circuit for expired keys. The request is ultimately rejected by EvaluateGovernanceRequest, but the silent return nil is inconsistent with the MCP path and causes the VK's routing side-effects to be skipped entirely rather than actively blocked.
Comment on lines
1109
to
1114
| if virtualKeyValue != "" { | ||
| var ok bool | ||
| virtualKey, ok = p.store.GetVirtualKey(ctx, virtualKeyValue) | ||
| if !ok || virtualKey == nil || !virtualKey.IsActiveValue() { | ||
| if !ok || virtualKey == nil || virtualKey.IsExpiredAt(time.Now().UTC()) { | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
IsActiveValue() check dropped from PreRequestHook guard
The original guard was !ok || virtualKey == nil || !virtualKey.IsActiveValue(). This PR replaces !virtualKey.IsActiveValue() with virtualKey.IsExpiredAt(...), removing the inactive-key early-return entirely. An inactive VK now proceeds past this guard: its provider allowlist is published and routing rules apply before EvaluateGovernanceRequest blocks the request downstream. PreMCPHook was correctly updated to handle both inactive and expired with explicit 403 short-circuits; the same treatment belongs here.
akshaydeo
force-pushed
the
dev
branch
4 times, most recently
from
June 19, 2026 07:23
7f86f4e to
2f96e3a
Compare
akshaydeo
force-pushed
the
dev
branch
2 times, most recently
from
July 1, 2026 12:24
ac30a53 to
7c66b20
Compare
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
12 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
Adds expiry support for virtual keys.
Virtual keys can now optionally have an
expires_attimestamp. Existing keys without expiry continue working as before. Once the expiry time passes, requests using that virtual key fail closed withVirtual key has expired.Changes
expires_atto virtual keys.governance_virtual_keys.expires_at.expires_atin virtual key updates so expiry can be set/cleared correctly.expires_atandclear_expires_atsupport in create/update virtual key APIs.Design notes:
expires_at = nullkeeps current behavior. The key never expires automatically.expires_at <= nowmeans the key is expired and requests are blocked.Type of change
Affected areas
How to test
Validated locally with Bifrost running on port
9090and Ollama as the local provider.Commands run:
go test ./framework/configstore/...
go test ./plugins/governance/...
go test ./transports/bifrost-http/handlers/...
go test ./...
cd ui
pnpm build
Manual validation covered:
400.clear_expires_at.expires_atandclear_expires_at. API returns400.403withVirtual key has expired.x-bf-vk,Authorization: Bearer,x-api-key, andx-goog-api-key.403with expired message.expires_atand no expiry index.Screenshots/Recordings
Breaking changes
Existing virtual keys continue working because
expires_atdefaults toNULL.Related issues
Linear: BF-1171
Security considerations
This changes virtual key authorization behavior. Expired virtual keys fail closed during request evaluation. No secrets are logged, no request-path DB writes were added, and no new token flow or credential type was introduced.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit