rotate vk ui chagnes

[akshaydeo]

Summary

This PR adds virtual key rotation (single and bulk) and improves budget reconciliation when updating virtual keys. Previously, lowering a budget below its current usage or inheriting usage above a new budget's limit would fail with an error. Now these cases are allowed, and users are prompted to either preserve or reset usage when budget-relevant changes are detected.

Changes

• Added a reset_budget_usage field to UpdateVirtualKeyRequest so callers can explicitly reset all budget usage counters to zero on update
• Added an id field to CreateBudgetRequest so existing budgets can be matched by ID rather than only by reset duration, enabling stable reconciliation when durations change
• Replaced the inline budget-by-duration lookup with buildBudgetLookup, findExistingBudget, resetBudgetUsageIfRequested, and inheritUsageFromClosestShorterBudget helpers for cleaner, reusable reconciliation logic
• Budget sort order now uses parsed duration values (compareBudgetRequestDurations) rather than raw string comparison, so durations like "1M" and "1d" sort correctly
• Removed the validation that rejected budget updates where preserved usage exceeded the new limit — usage above the limit is now allowed and surfaced as a warning in the UI instead
• Added a rotateVirtualKey mutation (single) and bulkRotateVirtualKeys mutation to the governance API, with corresponding TypeScript types (BulkRotateVirtualKeysRequest, BulkRotateVirtualKeysResponse)
• Added a Rotate Key button to the virtual key edit sheet with a confirmation dialog
• Added per-row checkboxes and a select-all checkbox to the virtual keys table, with a Rotate selected (N) bulk action button that appears when keys are selected
• When saving a virtual key with budget-relevant changes (limit, duration, or calendar alignment), the UI now intercepts the submit and shows a dialog asking whether to preserve or reset usage; if the preserved usage would meet or exceed the new limit, a contextual warning is shown
• BudgetLineEntry and related form schemas now carry the budget id through the UI so it is sent back on update

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

# Transports
go test ./transports/bifrost-http/handlers/...

# UI
cd ui
pnpm i
pnpm build

Single key rotation:

1. Open the edit sheet for any virtual key.
2. Click Rotate Key in the footer.
3. Confirm in the dialog — the key value should change and the previous value should stop working.

Bulk rotation:

1. Select one or more virtual keys using the row checkboxes.
2. Click Rotate selected (N) in the table header.
3. Confirm — all selected keys should be rotated and deselected.

Budget reset prompt:

1. Edit a virtual key that has an existing budget with non-zero usage.
2. Change the budget limit or reset duration and click Update.
3. A dialog should appear asking to preserve or reset usage.
4. If the preserved usage would meet or exceed the new limit, the dialog should show a warning message before the choice.

Budget ID matching:

1. Update a virtual key's budget by sending the existing budget id with a different reset_duration — the existing budget record should be updated in place rather than deleted and recreated.

Screenshots/Recordings

Add before/after screenshots of the Rotate Key button, bulk rotate action, and budget reset dialog.

Breaking changes

• Yes
• No

Related issues

Security considerations

Rotation replaces the secret value of a virtual key immediately; the previous value stops working as soon as the rotation completes. Bulk rotation applies the same guarantee to all selected keys atomically per key. No secrets are logged or returned beyond the standard virtual key response payload.

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

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d447341a-c885-4ae2-bb56-680828113cf2

📥 Commits

Reviewing files that changed from the base of the PR and between 09d4567 and efadd05.

📒 Files selected for processing (10)

• tests/e2e/core/actions/api.ts
• tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts
• tests/e2e/features/virtual-keys/virtual-keys.spec.ts
• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/governance_test.go
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/components/ui/multibudgets.tsx
• ui/lib/store/apis/governanceApi.ts
• ui/lib/types/governance.ts

---

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Single and bulk virtual-key rotation from UI and API with confirmations, polling, toasts, and per-ID error reporting; bulk rotate trims/deduplicates IDs.
  • Two-step budget-usage prompt on create/update to preserve or reset usage (reset flag sent); budgets can include optional IDs to aid matching and inheritance-aware warnings.
  • Table multi-select with indeterminate header checkbox, bulk-rotate toolbar, and improved CSV export.

• Tests

  • Expanded unit and E2E coverage for rotation, bulk-rotate, budget reconciliation, inheritance scenarios, and edge cases.

Walkthrough

Adds single and bulk virtual-key rotation (API, UI, E2E) and refactors budget reconciliation to support optional per-request usage reset, optional budget IDs for ID-first matching, duration-aware ordering, and inheritance of CurrentUsage from shorter-duration budgets.

Changes

Virtual Key Rotation with Budget Reset Handling

Layer / File(s)	Summary
Backend: request shapes & reconciliation helpers transports/bifrost-http/handlers/governance.go	UpdateVirtualKeyRequest gains reset_budget_usage and CreateBudgetRequest gains optional id. New helpers implement conditional reset, duration-aware comparison, ID-or-duration lookup, and usage inheritance from shorter durations.
Backend: virtual-key reconciliation path transports/bifrost-http/handlers/governance.go	VK update/create uses duration-aware sorting, builds by-ID/by-duration maps, resolves requests id-first, updates matched budgets (with optional reset), and initializes usage for new durations via closest-shorter inheritance.
Backend: provider-config reconciliation path transports/bifrost-http/handlers/governance.go	Provider-config reconciliation deterministically sorts existing budgets by (reset_duration, ID), builds lookup maps, matches by id then by reset_duration, applies conditional resets, and inherits usage for created durations.
Backend tests: reconciliation & rotation handlers transports/bifrost-http/handlers/governance_test.go	Test helper builds lookup from existing+request budgets and applies inheritance before limits; tests renamed/added to expect preserved/inherited usage. Rotation mocks/tests extended for not-found, update/reload failures, invalid bulk requests, trimming/dedup, and all-fail scenarios.
Frontend types & API ui/lib/types/governance.ts, ui/lib/store/apis/governanceApi.ts	Add reset_budget_usage and optional id on budget requests; add BulkRotateVirtualKeysRequest/Response; add RTK Query mutations rotateVirtualKey and bulkRotateVirtualKeys with hooks.
Frontend: VirtualKeySheet ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx	Wires rotate mutation; preserves optional budget id in schemas and form state; computes deterministic budget signatures; detects budget-reset-relevant changes and prompts Preserve/Reset; passes reset_budget_usage on create/update; adds rotation UI and reassignment dialogs.
Frontend: VirtualKeysTable ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx	Adds row multi-selection and select-all-visible, bulk-rotate confirmation and handler calling bulkRotateVirtualKeys, prunes selections on visibility changes, integrates toasts, and updates CSV export helpers.
Frontend: multibudgets component ui/components/ui/multibudgets.tsx	BudgetLineEntry adds optional id; per-line amount input dataTestId updated to ${testId}-amount-${index}.
E2E: API helpers, page objects, specs tests/e2e/*	Adds API helpers for rotate/bulkRotate, page methods to reveal/poll key values and drive rotation flows, config-store-missing guard, and new rotation UI/API tests (single, cancel, bulk, partial errors).

Sequence Diagram

sequenceDiagram
  participant User
  participant VKSheet as VirtualKeySheet
  participant FrontendAPI as RTKQuery
  participant Backend
  participant DB as Storage

  User->>VKSheet: Submit create/edit or rotate action
  VKSheet->>VKSheet: Compute budget signatures & detect changes
  alt Budget-relevant changes
    VKSheet->>User: Show Preserve/Reset confirmation
    User->>VKSheet: Choose Preserve or Reset
    VKSheet->>FrontendAPI: call rotate/update with reset_budget_usage flag
  else No budget changes
    VKSheet->>FrontendAPI: call rotate/update without reset flag
  end
  FrontendAPI->>Backend: POST rotate or POST/PUT update
  Backend->>Backend: Reconcile budgets (build lookup, id-first match, duration-sort)
  Backend->>DB: update/create budgets, reset or inherit CurrentUsage
  Backend-->>FrontendAPI: response with updated virtual key(s)
  FrontendAPI-->>VKSheet: mutation result
  VKSheet->>User: Show success/toast

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• maximhq/bifrost#3460: Touches virtual-key budgeting UI and budget id handling similar to this change.
• maximhq/bifrost#3599: Adds/expands rotation tests and UI/API wiring that relate to these rotation handlers and reconciliation fixes.

Suggested reviewers

• danpiths

Poem

🐇 I spin keys and tend the budget vine,
Preserve or reset — the choice is thine.
Bulk hops through rows, single key takes flight,
Inherit the usage, dawn after night.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.82% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'rotate vk ui chagnes' contains a typo ('chagnes' should be 'changes') and is vague about the full scope of changes, which include backend reconciliation logic and budget handling, not just UI changes.	Revise the title to be more descriptive and spell-checked, such as 'Add virtual key rotation and improve budget reconciliation' or 'Implement VK rotation with budget usage reset control'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is comprehensive and well-structured, covering summary, changes, type, affected areas, testing steps, breaking changes, and security considerations. All major template sections are present and substantive.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 05-19-rotate_vk_ui_chagnes

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

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

[akshaydeo]

• rotate vk ui chagnes #3600 👈 (View in Graphite)
• rotate vk backend chagnes #3599
• dev

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

[CLAassistant]

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

[coderabbitai]

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 263-285: The lookup maps built by buildBudgetLookup (byID,
byDuration) are reused across the reconciliation pass so a single existing
budget can be matched twice; fix findExistingBudget (which takes
CreateBudgetRequest, byID, byDuration) so that when it returns a found existing
budget you also remove that budget from both maps (e.g., delete from byID and
byDuration or mark its ID consumed) to prevent a later request from matching the
same row, and add a regression test that exercises the duration-swap case
(existing 1d budget renamed by ID to 1w while the payload also adds a new 1d) to
cover both the VK and provider-config reconciliation paths.

🪄 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: 5c8aa362-56f9-4a27-8a46-9fb9c9643de3

📥 Commits

Reviewing files that changed from the base of the PR and between 684383d and 78443e9.

📒 Files selected for processing (7)

• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/governance_test.go
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/components/ui/multibudgets.tsx
• ui/lib/store/apis/governanceApi.ts
• ui/lib/types/governance.ts

[greptile-apps]

Confidence Score: 4/5

Safe to merge with one fix: the budget-warning helper in virtualKeySheet.tsx can produce a false over-limit warning that may prompt users to reset usage they intended to preserve.

The Go backend changes are well-tested and the ID-claim logic in buildBudgetLookup correctly addresses the stale-lookup issue from the previous review. The UI findBudgetUsageWarning does not mirror the same ID-exclusion step, so a rename-by-ID plus new same-duration budget in one save emits a spurious warning. If the user clicks Reset Usage in response, the renamed budget's usage is zeroed out unintentionally.

ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx — findBudgetUsageWarning needs a claimedIDs pre-pass to exclude ID-claimed budgets from the duration map, matching the logic in buildBudgetLookup.

Important Files Changed

Filename	Overview
transports/bifrost-http/handlers/governance.go	Adds rotation handlers and budget reconciliation helpers; stale-duration-after-ID-match issue from previous review is now fixed.
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx	Adds rotate-key dialog, budget-reset intercept dialog, and budget ID flow-through. findBudgetUsageWarning doesn't pre-compute claimedIDs, producing a false over-limit warning in ID-rename + new same-duration budget scenarios.
ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx	Adds per-row checkboxes and bulk-rotate dialog; selection is pruned to visible page on page change; partial-success handling is correct.
transports/bifrost-http/handlers/governance_test.go	Extends coverage with rotation error paths, bulk input validation, inheritance-from-original-budgets, and duration-swap scenarios.
tests/e2e/features/virtual-keys/virtual-keys.spec.ts	Adds E2E coverage for single/bulk key rotation, cancel rotation, partial-bulk-failure, and skipIfConfigStoreMissing guard.
tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts	Adds page-object methods for rotation and checkbox selection.
tests/e2e/core/actions/api.ts	Adds API-action helpers rotate (single) and bulkRotate for virtual keys.
ui/lib/store/apis/governanceApi.ts	Registers rotateVirtualKey and bulkRotateVirtualKeys RTK Query mutations with correct tag invalidation.
ui/lib/types/governance.ts	Adds BulkRotateVirtualKeysRequest, BulkRotateVirtualKeysResponse, and reset_budget_usage to UpdateVirtualKeyRequest.
ui/components/ui/multibudgets.tsx	Adds id field to BudgetLineEntry so budget IDs flow through the multi-budget form component.

Comments Outside Diff (1)

1. ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx, line 1302-1316 (link)

   False budget warning when renaming budget by ID + adding new budget at old duration

   findBudgetUsageWarning builds existingByDuration from every existing budget, including those whose id is explicitly claimed by an ID-keyed entry in currentBudgets. The Go backend's buildBudgetLookup solves this by pre-computing claimedIDs and excluding those rows from the duration map; the UI helper does not mirror that logic.

   Concrete failure path: user renames existing budget {id:"A", duration:"1d", usage:80, max_limit:50} to {id:"A", duration:"1M", max_limit:100} and simultaneously adds a fresh {duration:"1d", max_limit:60}. The new 1d entry has no ID, so findBudgetUsageWarning resolves it via existingByDuration["1d"] and finds budget A's usage of 80, which is ≥ 60. It returns "Virtual key 1d budget has $80 usage, which meets or exceeds the new $60 limit." — a false warning. The new 1d budget the backend would actually create starts at zero. If the user clicks Reset Usage in response, the backend also resets budget A's usage (matched by ID) to zero, discarding usage that was legitimately intended to be preserved.

_{Reviews (6): Last reviewed commit: "rotate vk ui chagnes" | Re-trigger Greptile}

[coderabbitai]

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 `@tests/e2e/features/virtual-keys/virtual-keys.spec.ts`:
- Around line 494-495: The test pushes API-created virtual key names into
createdVKs (e.g., the push after createResp.virtual_key.value) but the suite’s
afterEach only deletes managementVKs, so API-created keys can leak; update the
cleanup logic to also remove entries from createdVKs (or ensure API-created keys
are added to managementVKs) by modifying the afterEach cleanup to iterate and
delete createdVKs (or merge createdVKs into managementVKs) so all keys pushed by
createResp.virtual_key.value are cleaned up after each test.

🪄 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: 7b78074f-52d6-4e86-a4c1-a4be99c88977

📥 Commits

Reviewing files that changed from the base of the PR and between 78443e9 and 3a1967c.

📒 Files selected for processing (10)

• tests/e2e/core/actions/api.ts
• tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts
• tests/e2e/features/virtual-keys/virtual-keys.spec.ts
• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/governance_test.go
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/components/ui/multibudgets.tsx
• ui/lib/store/apis/governanceApi.ts
• ui/lib/types/governance.ts

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

• ui/components/ui/multibudgets.tsx

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

• ui/lib/types/governance.ts
• ui/lib/store/apis/governanceApi.ts
• transports/bifrost-http/handlers/governance.go
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx

[coderabbitai]

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (1)

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

1497-1514: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Serialize virtual-key rotation per row.

This helper does a read-generate-write sequence without a lock or version check. Two concurrent rotations of the same key can interleave so the earlier request returns a secret that has already been overwritten by the later one. Wrap the fetch/update in a single transaction with SELECT ... FOR UPDATE (or an optimistic compare-and-swap) so the returned value is guaranteed to be the active key.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@transports/bifrost-http/handlers/governance.go` around lines 1497 - 1514,
rotateVirtualKeyByID currently does a fetch-modify-write without any concurrency
protection (calls configStore.GetVirtualKey then UpdateVirtualKey) which allows
two concurrent rotations to interleave; change the implementation to perform the
read-and-update inside a single serialized operation (either open a DB
transaction and SELECT ... FOR UPDATE on the virtual-key row before generating
and updating the value, or use an optimistic compare-and-swap via a version/ETag
on the row) so the write only succeeds if the row is still the same and the
returned value is the active one; update calls around configStore.GetVirtualKey
and configStore.UpdateVirtualKey (and the subsequent
governanceManager.ReloadVirtualKey) to run inside that
transaction/compare-and-swap and return an error if the CAS fails, ensuring the
returned preloadedVk is guaranteed to be the currently active key.

🤖 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 981-982: The call to inheritUsageFromClosestShorterBudget is using
the partially built reconciledBudgets slice, causing new budgets without ids to
lose preserved usage; change those calls to pass the original pre-reconciliation
budget snapshot (the variable holding the incoming/original budgets) instead of
reconciledBudgets so usage is inherited from the true prior state, and apply the
same change for the other occurrence around the block referenced at the second
location (lines ~1277-1278); ensure you still pass resetBudgetUsage and then
call validateBudget(&budget) as before.

---

Outside diff comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1497-1514: rotateVirtualKeyByID currently does a
fetch-modify-write without any concurrency protection (calls
configStore.GetVirtualKey then UpdateVirtualKey) which allows two concurrent
rotations to interleave; change the implementation to perform the
read-and-update inside a single serialized operation (either open a DB
transaction and SELECT ... FOR UPDATE on the virtual-key row before generating
and updating the value, or use an optimistic compare-and-swap via a version/ETag
on the row) so the write only succeeds if the row is still the same and the
returned value is the active one; update calls around configStore.GetVirtualKey
and configStore.UpdateVirtualKey (and the subsequent
governanceManager.ReloadVirtualKey) to run inside that
transaction/compare-and-swap and return an error if the CAS fails, ensuring the
returned preloadedVk is guaranteed to be the currently active key.

🪄 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: 255396e0-6d3b-41f7-8cce-a786e27fa0ce

📥 Commits

Reviewing files that changed from the base of the PR and between 3a1967c and c2c461f.

📒 Files selected for processing (10)

• tests/e2e/core/actions/api.ts
• tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts
• tests/e2e/features/virtual-keys/virtual-keys.spec.ts
• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/governance_test.go
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/components/ui/multibudgets.tsx
• ui/lib/store/apis/governanceApi.ts
• ui/lib/types/governance.ts

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

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

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

• ui/components/ui/multibudgets.tsx
• tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts
• tests/e2e/core/actions/api.ts
• tests/e2e/features/virtual-keys/virtual-keys.spec.ts
• ui/lib/store/apis/governanceApi.ts
• ui/lib/types/governance.ts
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx

[coderabbitai]

♻️ Duplicate comments (2)

tests/e2e/features/virtual-keys/virtual-keys.spec.ts (2)

520-520: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Track API-created keys in the suite cleanup array.

Line 520 appends to createdVKs, but this suite's afterEach only cleans managementVKs; these API-created keys can leak across tests.

🔧 Proposed fix

-      createdVKs.push(firstName, secondName)
+      managementVKs.push(firstName, secondName)

As per coding guidelines: "E2E tests must track created resources in arrays and clean up in afterEach hook to prevent test pollution".

🤖 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 `@tests/e2e/features/virtual-keys/virtual-keys.spec.ts` at line 520, The test
pushes API-created key names into createdVKs (createdVKs.push(firstName,
secondName)) but the afterEach only cleans managementVKs, so these API-created
keys can leak; update the cleanup logic so API-created keys are tracked and
removed: either push the API-created names into the suite cleanup array that
afterEach already iterates (e.g., add them to managementVKs) or extend the
afterEach to also iterate and teardown createdVKs; modify the test where
createdVKs is populated and the afterEach hook (or its helper) to ensure
createdVKs items are deleted after each test.

---

494-494: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Track API-created keys in the suite cleanup array.

Line 494 appends to createdVKs, but this suite's afterEach only cleans managementVKs; these API-created keys can leak across tests.

🔧 Proposed fix

-      createdVKs.push(vkName)
+      managementVKs.push(vkName)

As per coding guidelines: "E2E tests must track created resources in arrays and clean up in afterEach hook to prevent test pollution".

🤖 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 `@tests/e2e/features/virtual-keys/virtual-keys.spec.ts` at line 494, The test
appends API-created virtual key names to createdVKs but the suite only cleans up
managementVKs in the afterEach, so API-created keys can leak; update the
afterEach cleanup hook (the afterEach that currently references managementVKs)
to also iterate and delete entries in createdVKs (or merge createdVKs into the
existing cleanup logic), ensuring any vkName pushed to createdVKs is deleted
after each test; locate references to createdVKs, managementVKs, vkName and the
afterEach hook in virtual-keys.spec.ts to apply the change.

🤖 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.

Duplicate comments:
In `@tests/e2e/features/virtual-keys/virtual-keys.spec.ts`:
- Line 520: The test pushes API-created key names into createdVKs
(createdVKs.push(firstName, secondName)) but the afterEach only cleans
managementVKs, so these API-created keys can leak; update the cleanup logic so
API-created keys are tracked and removed: either push the API-created names into
the suite cleanup array that afterEach already iterates (e.g., add them to
managementVKs) or extend the afterEach to also iterate and teardown createdVKs;
modify the test where createdVKs is populated and the afterEach hook (or its
helper) to ensure createdVKs items are deleted after each test.
- Line 494: The test appends API-created virtual key names to createdVKs but the
suite only cleans up managementVKs in the afterEach, so API-created keys can
leak; update the afterEach cleanup hook (the afterEach that currently references
managementVKs) to also iterate and delete entries in createdVKs (or merge
createdVKs into the existing cleanup logic), ensuring any vkName pushed to
createdVKs is deleted after each test; locate references to createdVKs,
managementVKs, vkName and the afterEach hook in virtual-keys.spec.ts to apply
the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5a0b7e0c-ca9a-41ce-812e-302a4c2815ec

📥 Commits

Reviewing files that changed from the base of the PR and between c2c461f and 2e2528b.

📒 Files selected for processing (10)

• tests/e2e/core/actions/api.ts
• tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts
• tests/e2e/features/virtual-keys/virtual-keys.spec.ts
• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/governance_test.go
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
• ui/components/ui/multibudgets.tsx
• ui/lib/store/apis/governanceApi.ts
• ui/lib/types/governance.ts

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

• tests/e2e/core/actions/api.ts
• ui/lib/store/apis/governanceApi.ts
• ui/components/ui/multibudgets.tsx
• ui/lib/types/governance.ts
• tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts
• ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
• transports/bifrost-http/handlers/governance.go
• ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx

[akshaydeo]

Merge activity

• May 19, 8:16 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 19, 8:19 PM UTC: Graphite rebased this pull request as part of a merge.
• May 19, 8:21 PM UTC: @akshaydeo merged this pull request with Graphite.

The base branch was changed.