feat: add expires_at field to virtual keys - #3229
Closed
crust3780 wants to merge 1 commit 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:
📝 WalkthroughWalkthroughThis PR implements virtual key expiry by adding an optional ChangesVirtual Key Expiry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
crust3780
marked this pull request as ready for review
May 5, 2026 11:39
![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: 2
🧹 Nitpick comments (2)
plugins/governance/resolver.go (1)
262-273: ⚡ Quick winPrefer reusing
isVirtualKeyUsable()to avoid policy drift.This block duplicates active/expiry gating already defined in
plugins/governance/utils.go. Reusing the helper here keeps virtual-key usability checks centralized and consistent across governance paths.♻️ Suggested refactor
- if !vk.IsActive { + if !vk.IsActive { return &EvaluationResult{ Decision: DecisionVirtualKeyBlocked, Reason: "Virtual key is inactive", } } - if vk.ExpiresAt != nil && time.Now().UTC().After(vk.ExpiresAt.UTC()) { + if !isVirtualKeyUsable(vk) { return &EvaluationResult{ Decision: DecisionVirtualKeyBlocked, Reason: "Virtual key has expired", } }🤖 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/resolver.go` around lines 262 - 273, This code duplicates active/expiry checks for virtual keys; replace the manual gate with a call to the central helper isVirtualKeyUsable(vk) from plugins/governance/utils.go, remove the duplicated if-blocks, and if isVirtualKeyUsable indicates the key is not usable return an &EvaluationResult{Decision: DecisionVirtualKeyBlocked, Reason: <use reason returned by isVirtualKeyUsable or a consistent message>}; reference the local variable vk and the types EvaluationResult and DecisionVirtualKeyBlocked when making the replacement so behavior stays consistent across governance paths.ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx (1)
375-375: ⚡ Quick winUse a stable key for budget rows instead of the array index.
key={bIdx}is avoidable here and can cause row reconciliation issues when budgets change order or entries are inserted/removed.As per coding guidelines: "Always use stable, unique keys in lists; never use array index as key unless unavoidable".♻️ Proposed fix
- {displayBudgets.map((b, bIdx) => ( - <div key={bIdx} className="space-y-2 rounded-lg border p-4"> + {displayBudgets.map((b) => ( + <div key={b.id ?? b.reset_duration} className="space-y-2 rounded-lg border p-4">🤖 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/virtualKeyDetailsSheet.tsx` at line 375, Replace the unstable array index key (key={bIdx}) used for budget rows with a stable unique identifier from each budget item (e.g., budget.id or another persistent unique field on the budget object) in the VirtualKeyDetailsSheet component's budget list render; update the JSX where bIdx is used (the div with className "space-y-2 rounded-lg border p-4") to use that stable id (or a fallback like budget.uuid or a concatenation of intrinsic unique fields) so React can reconcile rows correctly when budgets are reordered or inserted/removed.
🤖 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/migrations.go`:
- Around line 7301-7316: The migration adds/drops the expires_at column for
tables.TableVirtualKey but doesn't create the GORM index, causing schema drift;
update the migration (the Up closure where mg.AddColumn is called and the
Rollback closure where mg.DropColumn is called) to explicitly create the index
after adding the column (use mg.CreateIndex or db.Migrator().CreateIndex for
tables.TableVirtualKey, "ExpiresAt"/"expires_at" as appropriate) and explicitly
drop the index in Rollback (mg.DropIndex or Migrator().DropIndex) so upgraded
DBs get the same index that fresh installs get.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 232-233: The prefill uses UTC components (new
Date(virtualKey.expires_at).toISOString().slice(0,16)) which makes the
datetime-local input show UTC as if it were local; change the expiresAt
assignment so you convert the stored UTC instant to the user's local wall-clock
string (YYYY-MM-DDTHH:MM) before slicing—i.e., if virtualKey?.expires_at exists,
build the local datetime string from new Date(virtualKey.expires_at) using
getFullYear(), getMonth()+1, getDate(), getHours(), getMinutes() with
zero-padding and produce "YYYY-MM-DDTHH:MM"; keep null when no expires_at. This
ensures the input value (expiresAt) matches the browser's local interpretation
and aligns with the parsing done later where the field is re-parsed into an ISO
instant.
---
Nitpick comments:
In `@plugins/governance/resolver.go`:
- Around line 262-273: This code duplicates active/expiry checks for virtual
keys; replace the manual gate with a call to the central helper
isVirtualKeyUsable(vk) from plugins/governance/utils.go, remove the duplicated
if-blocks, and if isVirtualKeyUsable indicates the key is not usable return an
&EvaluationResult{Decision: DecisionVirtualKeyBlocked, Reason: <use reason
returned by isVirtualKeyUsable or a consistent message>}; reference the local
variable vk and the types EvaluationResult and DecisionVirtualKeyBlocked when
making the replacement so behavior stays consistent across governance paths.
In `@ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx`:
- Line 375: Replace the unstable array index key (key={bIdx}) used for budget
rows with a stable unique identifier from each budget item (e.g., budget.id or
another persistent unique field on the budget object) in the
VirtualKeyDetailsSheet component's budget list render; update the JSX where bIdx
is used (the div with className "space-y-2 rounded-lg border p-4") to use that
stable id (or a fallback like budget.uuid or a concatenation of intrinsic unique
fields) so React can reconcile rows correctly when budgets are reordered or
inserted/removed.
🪄 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: 5db224e7-461a-487f-897d-577145b0a081
📒 Files selected for processing (11)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.goui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/lib/types/governance.ts
Contributor
|
❤️ for the PR @crust3780 - can you look at the comment (specifically timezone) - keep everything in UTC on the server side |
Contributor
Confidence Score: 5/5Safe to merge — expiry is enforced fail-closed at all governance enforcement points and the DB migration handles both fresh installs and upgrades correctly. The change is narrowly scoped: it adds a single nullable timestamp column, wires it through existing governance paths without altering their error-handling model, and the OptionalJSON approach correctly handles null vs absent in the update path. No pre-existing enforcement paths are weakened. No files require special attention. Important Files Changed
Reviews (24): Last reviewed commit: "feat: add expires_at field to virtual ke..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Author
|
@akshaydeo should be fixed now |
Contributor
Author
|
I think a schema update may be missing? Currently, configuration is only possible via UI and database. |
Contributor
|
will need that too |
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 `@helm-charts/bifrost/values-examples/providers-and-virtual-keys.yaml`:
- Around line 554-559: The example hardcodes a near-term expiry for the virtual
key (id "vk-testing-limited") which will soon be expired and confuse users;
update the expires_at field in that block (the "expires_at" value for id
"vk-testing-limited") to be either removed, set to a far-future date (e.g.,
9999-12-31T23:59:59Z) or replaced with a clear placeholder like
"<ENTER_EXPIRY_ISO8601>" so consumers must intentionally supply an expiration
instead of copying a soon-expiring timestamp.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 1236-1239: The expires_at property currently allows any string;
update its JSON Schema entry (the "expires_at" property in the chart/schema) to
enforce RFC3339/ISO-8601 by adding "format": "date-time", and ensure the
authoritative transports/config.schema.json is updated first (then propagate
changes to handlers and docs) so invalid timestamps are rejected at validation
time.
🪄 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: 6728f98a-f4ef-4633-bbaa-b37b47ced50d
📒 Files selected for processing (5)
helm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values-examples/providers-and-virtual-keys.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamltransports/config.schema.json
✅ Files skipped from review due to trivial changes (1)
- helm-charts/bifrost/values.yaml
Contributor
Author
|
implemented and fixed, should be complete now. |
|
|
Contributor
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 `@transports/bifrost-http/handlers/governance.go`:
- Around line 40-51: UnmarshalJSON currently preserves the incoming time zone;
update NullableTime.UnmarshalJSON to normalize the parsed time to UTC before
assigning (i.e., call t = t.UTC() after successful json.Unmarshal and then set
n.Time = &t). Also ensure the CreateVirtualKey flow normalizes the decoded
pointer field by either changing CreateVirtualKeyRequest.ExpiresAt to
*NullableTime so UnmarshalJSON handles UTC normalization, or after JSON decoding
in createVirtualKey set req.ExpiresAt = &[]time.Time{req.ExpiresAt.UTC()}[0] (or
use a local var) so the stored ExpiresAt is always in UTC.
🪄 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: 734eabc7-864c-4b6d-bbee-6c5a77963f6c
📒 Files selected for processing (1)
transports/bifrost-http/handlers/governance.go
crust3780
marked this pull request as draft
May 6, 2026 11:20
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 6, 2026 11:25
98bbf29 to
56d2fab
Compare
crust3780
marked this pull request as ready for review
May 6, 2026 11:33
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 6, 2026 11:41
82b2be4 to
275bf60
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/governance/main.go (1)
1458-1466:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn the correct decision for inactive/expired keys in MCP recheck.
At Line 1458,
!ok || !isVirtualKeyUsable(vk)currently returnsDecisionVirtualKeyNotFound.
For keys that exist but are inactive/expired, this should beDecisionVirtualKeyBlocked(or a branched message), otherwise rejection reasons and metrics get misclassified.Suggested fix
- if !ok || !isVirtualKeyUsable(vk) { + if !ok { // VK became invalid or expired after initial check - fail closed for security ctx.SetValue(governanceRejectedContextKey, true) return req, &schemas.MCPPluginShortCircuit{Error: &schemas.BifrostError{ Type: bifrost.Ptr(string(DecisionVirtualKeyNotFound)), StatusCode: bifrost.Ptr(403), Error: &schemas.ErrorField{ Message: "Virtual key not found", }, }}, nil } + if !isVirtualKeyUsable(vk) { + ctx.SetValue(governanceRejectedContextKey, true) + return req, &schemas.MCPPluginShortCircuit{Error: &schemas.BifrostError{ + Type: bifrost.Ptr(string(DecisionVirtualKeyBlocked)), + StatusCode: bifrost.Ptr(403), + Error: &schemas.ErrorField{ + Message: "Virtual key is inactive or expired", + }, + }}, nil + }🤖 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 1458 - 1466, The current recheck branch treats both missing and expired/inactive virtual keys the same; update the conditional handling in the MCP recheck so that when vk is absent (!ok) you keep returning DecisionVirtualKeyNotFound, but when vk exists and !isVirtualKeyUsable(vk) you return DecisionVirtualKeyBlocked (or a distinct blocked message) instead of DecisionVirtualKeyNotFound; keep the ctx.SetValue(governanceRejectedContextKey, true) and MCPPluginShortCircuit/error structure but change the bifrost.Type to DecisionVirtualKeyBlocked and adjust the Error.Message to indicate the key is inactive/expired so metrics and rejection reasons are classified correctly (refer to isVirtualKeyUsable, DecisionVirtualKeyNotFound, DecisionVirtualKeyBlocked, governanceRejectedContextKey, MCPPluginShortCircuit).
🤖 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 `@plugins/governance/main.go`:
- Around line 1458-1466: The current recheck branch treats both missing and
expired/inactive virtual keys the same; update the conditional handling in the
MCP recheck so that when vk is absent (!ok) you keep returning
DecisionVirtualKeyNotFound, but when vk exists and !isVirtualKeyUsable(vk) you
return DecisionVirtualKeyBlocked (or a distinct blocked message) instead of
DecisionVirtualKeyNotFound; keep the ctx.SetValue(governanceRejectedContextKey,
true) and MCPPluginShortCircuit/error structure but change the bifrost.Type to
DecisionVirtualKeyBlocked and adjust the Error.Message to indicate the key is
inactive/expired so metrics and rejection reasons are classified correctly
(refer to isVirtualKeyUsable, DecisionVirtualKeyNotFound,
DecisionVirtualKeyBlocked, governanceRejectedContextKey, MCPPluginShortCircuit).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb558857-b816-4fbf-9d6a-cfa8c727f73c
📒 Files selected for processing (16)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/virtualkey.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values-examples/providers-and-virtual-keys.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/governance/main.goplugins/governance/resolver.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/config.schema.jsonui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/lib/types/governance.ts
Contributor
Author
|
@akshaydeo I think I have addressed every issue. However, I am a bit unsure about the various locations at which the expiry is tested. its tested in preHttp but also in prellm / premcp but i think if prehttp short circuits it will never reach the late code paths. could you clarify where the best location to test it is and if those later code paths can be deleted? |
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 27, 2026 07:19
e88e0c7 to
bb26974
Compare
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 27, 2026 07:31
bb26974 to
a6e43b0
Compare
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 27, 2026 09:06
a6e43b0 to
bae56c2
Compare
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 27, 2026 09:07
bae56c2 to
9eafc0a
Compare
Contributor
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 `@transports/bifrost-http/handlers/governance_test.go`:
- Around line 85-107: The test TestUpdateVirtualKeyRequestExpiresAtJSON fails to
compile because it uses require.NoError and assert.* helpers but the file lacks
imports for github.com/stretchr/testify/require and
github.com/stretchr/testify/assert; fix by adding those two imports to the test
file’s import block so the symbols require.NoError, assert.False, assert.True,
assert.Nil, require.NotNil, and assert.Equal resolve correctly (locate the
import block in transports/bifrost-http/handlers/governance_test.go).
🪄 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: 48e02f4e-4fa9-497c-a07b-374dfa52bdc1
📒 Files selected for processing (18)
docs/openapi/schemas/management/governance.yamlframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/virtualkey.gohelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values-examples/providers-and-virtual-keys.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/governance/main.goplugins/governance/resolver.goplugins/governance/utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/config.schema.jsonui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/lib/types/governance.ts
✅ Files skipped from review due to trivial changes (1)
- helm-charts/bifrost/values-examples/providers-and-virtual-keys.yaml
🚧 Files skipped from review as they are similar to previous changes (14)
- transports/config.schema.json
- helm-charts/bifrost/templates/_helpers.tpl
- framework/configstore/tables/virtualkey.go
- ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
- framework/configstore/rdb.go
- docs/openapi/schemas/management/governance.yaml
- helm-charts/bifrost/values.yaml
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- framework/configstore/migrations.go
- plugins/governance/utils.go
- ui/lib/types/governance.ts
- transports/bifrost-http/handlers/governance.go
- plugins/governance/main.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 27, 2026 10:16
9eafc0a to
976b43c
Compare
Contributor
Author
|
@akshaydeo did rebase and cleanup and verified this works in a local test instance |
crust3780
force-pushed
the
virtual-key-expiry
branch
3 times, most recently
from
May 29, 2026 07:40
883f61b to
ff295cc
Compare
Contributor
Author
|
rebased and used optionaljson instead of custom type |
crust3780
force-pushed
the
virtual-key-expiry
branch
from
May 29, 2026 17:06
ff295cc to
2a192bc
Compare
akshaydeo
force-pushed
the
dev
branch
3 times, most recently
from
June 4, 2026 10:02
6711ce3 to
a1beab5
Compare
akshaydeo
force-pushed
the
dev
branch
2 times, most recently
from
June 8, 2026 11:25
e389df7 to
a65fce4
Compare
18 tasks
Contributor
Author
|
closed in favor of #3765 |
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
4 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
This PR adds a new Virtual Key field "Expires At" which defines an optional timestamp that defines whether a key is active. If the timestamp is older than the current time, the key becomes expired and requests don't work anymore. The date can be changed or removed and the key is active again.
Use case is to add Virtual Keys that are only valid for a limited time, e.g. for testing purposes.
Changes
Expiry is enforced at request time without mutating the DB record. Re-enable by setting is_active=true and/or updating expires_at.
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Close #3207
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Release Notes