feat: adds rate limits to customers and teams#1619
Conversation
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Team and Customer rate-limit support across schema, DB models, store APIs, governance in-memory synchronization, HTTP handlers, and UI (dialogs/tables), including a migration, transactional delete/get helpers, and UI fields/visualizations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as HTTP Handler
participant DB as Database (Tx)
participant Store as Governance Store
Client->>API: POST /teams (body includes rate_limit)
activate API
API->>API: validate request & rate_limit
API->>DB: BEGIN TX
activate DB
API->>DB: INSERT team
API->>DB: INSERT rate_limit
DB-->>API: return rate_limit_id
API->>DB: UPDATE team SET rate_limit_id
API->>DB: COMMIT
deactivate DB
API->>Store: sync team + rate_limit into in-memory structures
activate Store
Store->>Store: propagate rate_limit/budget references (VKs, teams, customers)
Store-->>API: sync complete
API-->>Client: 200 OK (team with rate_limit)
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ui/lib/types/governance.ts (1)
175-223:⚠️ Potential issue | 🟠 MajorUpdateRateLimitRequest supports
nullclears — make sure UI emits those when clearing fields.The
| nullshapes are great for “clear/unset” semantics, but the dialogs currently only omit fields (or omit the entirerate_limit) when users clear inputs, which won’t clear server-side state in most APIs.ui/app/workspace/user-groups/views/teamDialog.tsx (2)
76-209:⚠️ Potential issue | 🟠 MajorUpdate flow can’t clear rate limits; dirty-tracking can allow “no-op saves”.
Two related issues:
- If a team already has token/request limits, clearing the inputs results in
rate_limitbeing omitted (Line 165), so the server never receives a “clear” instruction even thoughUpdateRateLimitRequestsupportsnullclears.- Reset-duration changes can make the form dirty even when the corresponding max limit is empty, but those changes are dropped from the submission payload — leading to “saved successfully” with no actual change.
Please confirm backend update semantics for “clear” (null vs empty object vs omission) in the team update handler/store layer before merging.
Proposed fix (emit nulls on clear + align dirty tracking)
useEffect(() => { const currentData = { name: formData.name, customerId: formData.customerId, budgetMaxLimit: formData.budgetMaxLimit, - budgetResetDuration: formData.budgetResetDuration, + budgetResetDuration: formData.budgetMaxLimit ? formData.budgetResetDuration : initialState.budgetResetDuration, tokenMaxLimit: formData.tokenMaxLimit, - tokenResetDuration: formData.tokenResetDuration, + tokenResetDuration: formData.tokenMaxLimit ? formData.tokenResetDuration : initialState.tokenResetDuration, requestMaxLimit: formData.requestMaxLimit, - requestResetDuration: formData.requestResetDuration, + requestResetDuration: formData.requestMaxLimit ? formData.requestResetDuration : initialState.requestResetDuration, }; setFormData((prev) => ({ ...prev, isDirty: !isEqual(initialState, currentData), })); }, [..., initialState]); ... // Add rate limit if enabled (token or request limits) -if (tokenMaxLimitNum || requestMaxLimitNum) { - updateData.rate_limit = { - token_max_limit: tokenMaxLimitNum, - token_reset_duration: tokenMaxLimitNum ? formData.tokenResetDuration : undefined, - request_max_limit: requestMaxLimitNum, - request_reset_duration: requestMaxLimitNum ? formData.requestResetDuration : undefined, - }; -} +const rateLimitTouched = + formData.tokenMaxLimit !== initialState.tokenMaxLimit || + (formData.tokenMaxLimit && formData.tokenResetDuration !== initialState.tokenResetDuration) || + formData.requestMaxLimit !== initialState.requestMaxLimit || + (formData.requestMaxLimit && formData.requestResetDuration !== initialState.requestResetDuration); + +if (rateLimitTouched) { + updateData.rate_limit = { + token_max_limit: formData.tokenMaxLimit ? tokenMaxLimitNum : null, + token_reset_duration: formData.tokenMaxLimit ? formData.tokenResetDuration : null, + request_max_limit: formData.requestMaxLimit ? requestMaxLimitNum : null, + request_reset_duration: formData.requestMaxLimit ? formData.requestResetDuration : null, + }; +}
151-155:⚠️ Potential issue | 🟠 MajorInconsistent
customer_idhandling between create and update—empty string can persist via update.Create prevents sending empty
customer_id(uses|| undefined), but update sends the raw value directly, allowing an empty string to reach the backend. The backend has no validation to reject or convert emptycustomer_idto null, so it persists the invalid state to the database. Align update to match create by converting empty strings toundefined:customer_id: formData.customerId || undefined,Also applies to: lines 179–182
ui/app/workspace/user-groups/views/customerDialog.tsx (1)
71-201:⚠️ Potential issue | 🟠 MajorCustomer rate limits cannot be cleared, and dirty-tracking allows orphaned duration changes.
Same pattern as TeamDialog:
- Clearing token/request inputs omits
rate_limitentirely (line 158), making it impossible to remove existing limits even thoughUpdateRateLimitRequestsupports null values in the type definition.- Resetting duration inputs can mark the form dirty when their max limit is empty, but will never be submitted because the condition checks are at submission time (line 158).
Additionally, the backend
updateCustomerhandler lacks the deletion logic present in other handlers (ModelConfig, Provider), so even if the UI sends null values, the rate limit relationship would not be cleared.Align UI dirty-tracking to only include reset-duration when its paired max-limit is set, and ensure the backend properly handles null rate_limit fields by setting
RateLimitID = niland deleting orphaned rate limit records.
🤖 Fix all issues with AI agents
In `@framework/configstore/store.go`:
- Around line 105-110: MockConfigStore has mismatched/missing method signatures:
update MockConfigStore's GetRateLimit to accept the variadic tx ...*gorm.DB
parameter, and add implementations for DeleteRateLimit and DeleteBudget that
match the ConfigStore interface signatures (e.g., GetRateLimit(ctx
context.Context, id string, tx ...*gorm.DB) (*tables.TableRateLimit, error),
DeleteRateLimit(ctx context.Context, id string, tx ...*gorm.DB) error,
DeleteBudget(ctx context.Context, id string, tx ...*gorm.DB) error); ensure
method receivers and return types follow the interface and that tests/uses
calling these methods (e.g., places that pass tx) compile cleanly.
In `@plugins/governance/store.go`:
- Around line 227-244: The build fails because tables.TableTeam is missing the
RateLimitID (and possibly RateLimit) fields referenced when cloning a team in
the iteration (the code uses clone.RateLimitID, clone.RateLimit, gs.rateLimits
and assigns to teams[key.(string)] = &clone). Fix by adding the missing fields
to the TableTeam struct (e.g., RateLimitID *<appropriate-type> and RateLimit
*configstoreTables.TableRateLimit) or by regenerating the generated tables code
so TableTeam includes RateLimitID/RateLimit, then recompile to ensure the clone
assignments compile against the updated struct.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1267-1311: The update branch for rate limits currently only
overwrites non-nil fields and skips validateRateLimit, preventing callers from
clearing fields and allowing invalid values; change the update flow in the
handler so that when req.RateLimit != nil you assign every RateLimit field from
req (setting fields to nil when req has nil to clear them) instead of
conditional updates, call validateRateLimit(rateLimit) before persisting, and
then call h.configStore.UpdateRateLimit(ctx, rateLimit, tx); mirror the same
“assign all then validate then persist” pattern used in
updateModelConfig/updateProviderGovernance and keep the create path’s
validateRateLimit call intact.
- Around line 1579-1623: The handler currently ignores nil fields when updating
an existing rate limit; change the update logic for req.RateLimit so you perform
full assignment of each nullable field onto the fetched rateLimit (assigning nil
to clear values) instead of skipping on nil, then call
validateRateLimit(rateLimit) before calling h.configStore.UpdateRateLimit(ctx,
rateLimit, tx) to prevent persisting invalid values; do the same validation flow
used when creating a new rate limit (use validateRateLimit and then
h.configStore.CreateRateLimit) and keep the same assignments to
customer.RateLimitID and customer.RateLimit so callers see the updated/cleared
state.
In `@ui/app/workspace/user-groups/views/customerTable.tsx`:
- Around line 312-326: The action buttons are hidden via "opacity-0 ...
group-hover:opacity-100", which prevents keyboard users from revealing them;
update the container that wraps the actions (the div with className "opacity-0
transition-opacity group-hover:opacity-100") to also include the focus-visible
variant used in this repo (e.g., add "group-focus-within:opacity-100" or
equivalent) so the actions become visible when any child (like the
TooltipTrigger/Button that uses handleEditCustomer) receives keyboard focus;
apply the same change to the matching container in teamsTable.tsx to ensure
keyboard accessibility across both tables.
In `@ui/app/workspace/user-groups/views/teamsTable.tsx`:
- Around line 303-317: The action buttons container in teamsTable.tsx is hidden
with opacity-0 and only becomes visible on hover; update the container's class
(the div wrapping the Tooltip/Button in the row) to also include
group-focus-within:opacity-100 so keyboard focus reveals the actions (e.g.,
change the div className to include "group-focus-within:opacity-100"); ensure
the same update is applied to the equivalent containers in customerTable.tsx,
providerGovernanceTable.tsx, modelLimitsTable.tsx, and devProfiler.tsx so
keyboard users can access the handleEditTeam/Edit Button and other action
buttons via tab focus.
🧹 Nitpick comments (7)
framework/configstore/tables/customer.go (1)
5-24: RateLimit relationship wiring matches existing Budget pattern.
RateLimitID+RateLimitrelationship is consistent with howBudgetID/Budgetis modeled, so preloading/JSON should stay predictable across Customer/Team/VK.If you expect DB-enforced one-to-one ownership (no sharing), consider adding a UNIQUE constraint on
rate_limit_id(and similarlybudget_id) at the migration level. This aligns with the stated “1:1 ownership” invariant but does require schema changes.framework/configstore/tables/team.go (1)
11-23: Team RateLimit fields look consistent; consider enforcing 1:1 at DB layer.The
RateLimitID/RateLimitadditions mirror Customer and should compose cleanly with existing GORM hooks.Same as Customer: if rate limits are strictly owned and never shared, a UNIQUE constraint on
governance_teams.rate_limit_id(nullable unique) would prevent accidental reuse.ui/app/workspace/user-groups/views/teamsTable.tsx (2)
30-33: DRY: extractformatResetDuration(duplicated in customers table).This helper is repeated in
customerTable.tsx; consider moving it toui/lib/utils/governance.tsnext toformatCurrencyfor consistency.
210-281: Avoid defaulting missing reset durations to"1h"in display.
token_reset_duration || "1h"(and request) can silently misrepresent data if the backend returnsnull/missing durations. Prefer showing “—”/“Unknown” (or render only when duration exists) rather than inventing a value.Please double-check backend guarantees around
*_reset_durationwhen*_max_limitis set; if it’s guaranteed, then drop the fallback entirely.Proposed tweak (display “—” instead of inventing “1h”)
- {formatResetDuration(team.rate_limit.token_reset_duration || "1h")} + {formatResetDuration(team.rate_limit.token_reset_duration ?? "—")} ... - Resets {formatResetDuration(team.rate_limit.token_reset_duration || "1h")} + Resets {formatResetDuration(team.rate_limit.token_reset_duration ?? "—")}ui/app/workspace/user-groups/views/customerTable.tsx (2)
30-33: DRY:formatResetDurationduplicated with teams table.Recommend centralizing this helper (and possibly the “limits cell” UI) to keep Teams/Customers tables consistent.
219-290: Avoid defaulting missing reset durations to"1h"in display.Same concern as TeamsTable: the fallback can mask missing/invalid data and show the wrong reset cadence.
Please confirm backend guarantees for
*_reset_durationwhen*_max_limitis present; if guaranteed, remove fallback; otherwise show “—/Unknown”.framework/configstore/migrations.go (1)
3330-3377: Add indexes for the new rate_limit_id columns.
gorm:"index"tags won’t backfill indexes on existing DBs viaAddColumn, so consider explicit index creation for query performance and parity with new installs.💡 Suggested migration enhancement
// Add rate_limit_id to governance_teams table if !migrator.HasColumn(&tables.TableTeam{}, "rate_limit_id") { if err := migrator.AddColumn(&tables.TableTeam{}, "rate_limit_id"); err != nil { return fmt.Errorf("failed to add rate_limit_id column to teams: %w", err) } } + if !migrator.HasIndex(&tables.TableTeam{}, "idx_team_rate_limit_id") { + if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_team_rate_limit_id ON governance_teams (rate_limit_id)").Error; err != nil { + return fmt.Errorf("failed to create rate_limit_id index on teams: %w", err) + } + } // Add rate_limit_id to governance_customers table if !migrator.HasColumn(&tables.TableCustomer{}, "rate_limit_id") { if err := migrator.AddColumn(&tables.TableCustomer{}, "rate_limit_id"); err != nil { return fmt.Errorf("failed to add rate_limit_id column to customers: %w", err) } } + if !migrator.HasIndex(&tables.TableCustomer{}, "idx_customer_rate_limit_id") { + if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_customer_rate_limit_id ON governance_customers (rate_limit_id)").Error; err != nil { + return fmt.Errorf("failed to create rate_limit_id index on customers: %w", err) + } + }if migrator.HasColumn(&tables.TableTeam{}, "rate_limit_id") { if err := migrator.DropColumn(&tables.TableTeam{}, "rate_limit_id"); err != nil { return fmt.Errorf("failed to drop rate_limit_id column from teams: %w", err) } } + if err := tx.Exec("DROP INDEX IF EXISTS idx_team_rate_limit_id").Error; err != nil { + return fmt.Errorf("failed to drop team rate_limit_id index: %w", err) + } if migrator.HasColumn(&tables.TableCustomer{}, "rate_limit_id") { if err := migrator.DropColumn(&tables.TableCustomer{}, "rate_limit_id"); err != nil { return fmt.Errorf("failed to drop rate_limit_id column from customers: %w", err) } } + if err := tx.Exec("DROP INDEX IF EXISTS idx_customer_rate_limit_id").Error; err != nil { + return fmt.Errorf("failed to drop customer rate_limit_id index: %w", err) + }
danpiths
force-pushed
the
feature/02-12-feat_adds_rate_limits_to_customers_and_teams
branch
from
1148916 to
ae9e103
Compare
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 (2)
ui/app/workspace/user-groups/views/teamDialog.tsx (1)
156-199:⚠️ Potential issue | 🟠 MajorClearing budget/rate_limit fields doesn't remove existing limits; both teamDialog and customerDialog have this parity gap.
When users clear the budget or rate_limit fields and submit, the form omits these fields from the request payload (since the parsed values are
undefined), rather than sending explicit null or deletion signals. The backend treats omitted optional fields as "no change," leaving existing limits intact on the server. AlthoughDeleteBudgetandDeleteRateLimitoperations exist in the handlers, they're never invoked by this UI pattern. Consider explicitly sending nulls or a deletion flag when fields are cleared to enable proper removal.ui/app/workspace/user-groups/views/customerDialog.tsx (1)
150-191:⚠️ Potential issue | 🟠 MajorClearing limits won't remove existing budget/rate-limit from customers and teams.
When editing a customer or team, cleared max-limit inputs omit
budget/rate_limitfrom the update payload entirely (not sent as null). The backend handlers skip these omitted fields without attempting deletion, leaving existingBudgetIDandRateLimitIDreferences unchanged. This creates orphaned limits that remain active.The correct pattern already exists in the codebase for ModelConfig and Provider handlers: check if
req.Budget.MaxLimit == niland explicitly set the ID field to nil to trigger deletion. Apply this same logic toupdateCustomerandupdateTeamhandlers for parity.
🧹 Nitpick comments (6)
framework/configstore/migrations.go (1)
3330-3377: Consider explicit indexes for newrate_limit_idcolumns.
AddColumnmay not create thegorm:"index"indexes automatically, so adding explicit indexes here would keep team/customer lookups consistent with other governance tables.🛠️ Suggested index additions
// Add rate_limit_id to governance_teams table if !migrator.HasColumn(&tables.TableTeam{}, "rate_limit_id") { if err := migrator.AddColumn(&tables.TableTeam{}, "rate_limit_id"); err != nil { return fmt.Errorf("failed to add rate_limit_id column to teams: %w", err) } } + if !migrator.HasIndex(&tables.TableTeam{}, "idx_team_rate_limit") { + if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_team_rate_limit ON governance_teams (rate_limit_id)").Error; err != nil { + return fmt.Errorf("failed to create rate_limit_id index for teams: %w", err) + } + } // Add rate_limit_id to governance_customers table if !migrator.HasColumn(&tables.TableCustomer{}, "rate_limit_id") { if err := migrator.AddColumn(&tables.TableCustomer{}, "rate_limit_id"); err != nil { return fmt.Errorf("failed to add rate_limit_id column to customers: %w", err) } } + if !migrator.HasIndex(&tables.TableCustomer{}, "idx_customer_rate_limit") { + if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_customer_rate_limit ON governance_customers (rate_limit_id)").Error; err != nil { + return fmt.Errorf("failed to create rate_limit_id index for customers: %w", err) + } + }Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() + if err := tx.Exec("DROP INDEX IF EXISTS idx_team_rate_limit").Error; err != nil { + return fmt.Errorf("failed to drop rate_limit_id index for teams: %w", err) + } + if err := tx.Exec("DROP INDEX IF EXISTS idx_customer_rate_limit").Error; err != nil { + return fmt.Errorf("failed to drop rate_limit_id index for customers: %w", err) + } + if migrator.HasColumn(&tables.TableTeam{}, "rate_limit_id") { if err := migrator.DropColumn(&tables.TableTeam{}, "rate_limit_id"); err != nil { return fmt.Errorf("failed to drop rate_limit_id column from teams: %w", err) } }Does GORM Migrator.AddColumn create indexes defined via `gorm:"index"` tags, or must indexes be created explicitly (v1.31.1)?transports/bifrost-http/lib/config_test.go (1)
575-581: Consider mutating mock governance state on DeleteRateLimit/DeleteBudget.
Right now these are no-ops; if future tests depend on deletions, the mock will mask bugs. Consider trimmingm.governanceConfigto reflect removals.♻️ Proposed update
func (m *MockConfigStore) DeleteRateLimit(ctx context.Context, id string, tx ...*gorm.DB) error { - return nil + if m.governanceConfig == nil { + return nil + } + filtered := m.governanceConfig.RateLimits[:0] + for _, rl := range m.governanceConfig.RateLimits { + if rl.ID != id { + filtered = append(filtered, rl) + } + } + m.governanceConfig.RateLimits = filtered + return nil } func (m *MockConfigStore) DeleteBudget(ctx context.Context, id string, tx ...*gorm.DB) error { - return nil + if m.governanceConfig == nil { + return nil + } + filtered := m.governanceConfig.Budgets[:0] + for _, b := range m.governanceConfig.Budgets { + if b.ID != id { + filtered = append(filtered, b) + } + } + m.governanceConfig.Budgets = filtered + return nil }ui/app/workspace/user-groups/views/teamsTable.tsx (1)
16-33: Consider centralizingformatResetDurationto avoid drift across the stack.This helper is duplicated in
ui/app/workspace/user-groups/views/customerTable.tsx; extracting it to a shared util keeps team/customer tables aligned.framework/configstore/rdb.go (1)
2078-2090: Consider returning ErrNotFound when delete affects zero rows.Keeps delete semantics consistent with other Delete* helpers and avoids silent success on unknown IDs.
Suggested pattern (apply similarly to DeleteBudget)
- if err := txDB.WithContext(ctx).Delete(&tables.TableRateLimit{}, "id = ?", id).Error; err != nil { - return s.parseGormError(err) - } - return nil + result := txDB.WithContext(ctx).Delete(&tables.TableRateLimit{}, "id = ?", id) + if result.Error != nil { + return s.parseGormError(result.Error) + } + if result.RowsAffected == 0 { + return ErrNotFound + } + return nilAlso applies to: 2163-2175
plugins/governance/store.go (2)
1916-1992: Consider de-duplicating rate limits during hierarchy traversal.If future configs allow overlapping references (or transient data inconsistencies), duplicate checks can produce repeated violations. A small
seenset avoids that.♻️ Optional deduplication helper
func (gs *LocalGovernanceStore) collectRateLimitsFromHierarchy(vk *configstoreTables.TableVirtualKey, requestedProvider schemas.ModelProvider) ([]*configstoreTables.TableRateLimit, []string) { if vk == nil { return nil, nil } var rateLimits []*configstoreTables.TableRateLimit var rateLimitNames []string + seen := make(map[string]struct{}) + add := func(rl *configstoreTables.TableRateLimit, name string) { + if rl == nil { + return + } + if _, ok := seen[rl.ID]; ok { + return + } + seen[rl.ID] = struct{}{} + rateLimits = append(rateLimits, rl) + rateLimitNames = append(rateLimitNames, name) + } for _, pc := range vk.ProviderConfigs { if pc.RateLimitID != nil && pc.Provider == string(requestedProvider) { if rateLimitValue, exists := gs.rateLimits.Load(*pc.RateLimitID); exists && rateLimitValue != nil { if rateLimit, ok := rateLimitValue.(*configstoreTables.TableRateLimit); ok && rateLimit != nil { - rateLimits = append(rateLimits, rateLimit) - rateLimitNames = append(rateLimitNames, pc.Provider) + add(rateLimit, pc.Provider) } } } } ... - rateLimits = append(rateLimits, rateLimit) - rateLimitNames = append(rateLimitNames, "Team") + add(rateLimit, "Team") ... - rateLimits = append(rateLimits, rateLimit) - rateLimitNames = append(rateLimitNames, "Customer") + add(rateLimit, "Customer") ... - rateLimits = append(rateLimits, rateLimit) - rateLimitNames = append(rateLimitNames, "Customer") + add(rateLimit, "Customer")
2365-2375: Optional: clean LastDBUsagesRateLimits entries on delete.This prevents stale usage maps from growing across deletes.
♻️ Optional cleanup
if team.RateLimitID != nil { - gs.rateLimits.Delete(*team.RateLimitID) + rateLimitID := *team.RateLimitID + gs.rateLimits.Delete(rateLimitID) + gs.LastDBUsagesRateLimitsRequestsMu.Lock() + delete(gs.LastDBUsagesRequestsRateLimits, rateLimitID) + gs.LastDBUsagesRateLimitsRequestsMu.Unlock() + gs.LastDBUsagesRateLimitsTokensMu.Lock() + delete(gs.LastDBUsagesTokensRateLimits, rateLimitID) + gs.LastDBUsagesRateLimitsTokensMu.Unlock() }if customer.RateLimitID != nil { - gs.rateLimits.Delete(*customer.RateLimitID) + rateLimitID := *customer.RateLimitID + gs.rateLimits.Delete(rateLimitID) + gs.LastDBUsagesRateLimitsRequestsMu.Lock() + delete(gs.LastDBUsagesRequestsRateLimits, rateLimitID) + gs.LastDBUsagesRateLimitsRequestsMu.Unlock() + gs.LastDBUsagesRateLimitsTokensMu.Lock() + delete(gs.LastDBUsagesTokensRateLimits, rateLimitID) + gs.LastDBUsagesRateLimitsTokensMu.Unlock() }Also applies to: 2477-2487
danpiths
force-pushed
the
feature/02-12-feat_adds_rate_limits_to_customers_and_teams
branch
from
ae9e103 to
40c695f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugins/governance/store.go`:
- Around line 1945-1991: The code appends the same customer rate limit twice
when vk.TeamID points to a team whose CustomerID equals vk.CustomerID; update
the logic in the block around gs.teams / gs.customers resolution (the code that
fills rateLimits and rateLimitNames for team/customer) to dedupe before
appending: track seen customer IDs (or seen rateLimit IDs) in a local set, check
collectRateLimitIDsFromMemory / UpdateVirtualKeyRateLimitUsageInMemory callers'
expectations, and only append a customer rate limit if its customer ID (or
rateLimit.ID) has not already been added; reference vk, gs.teams, gs.customers,
gs.rateLimits, rateLimits, and rateLimitNames when applying the check.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 571-581: The mock deletion methods
(MockConfigStore.DeleteRateLimit and MockConfigStore.DeleteBudget) currently
no-op and should mutate the in-memory governance state so tests can observe
deletions; update each method to remove the entry with the matching id from the
corresponding slice on MockConfigStore (e.g., RateLimits, Budgets or whatever
slice fields hold the data), handle optional tx param as before, and maintain
any existing concurrency protection (locks) used by MockConfigStore so removal
is safe; also ensure DeleteRateLimit returns an error when the id is not found
if the real store would do so.
🧹 Nitpick comments (4)
plugins/governance/store.go (1)
2732-2792: Comment mentions “users” but no user updates occur here.
Consider removing “users” to match actual behavior and avoid confusion.ui/app/workspace/user-groups/views/teamsTable.tsx (1)
30-33: Consider centralizingformatResetDurationfor the stacked views.If customer/other tables in the stack use the same formatting, a shared util would prevent drift.
ui/app/workspace/user-groups/views/customerDialog.tsx (1)
88-91: Consider usingparseFloatfor token/request limits to handle edge cases.Using
parseIntfortokenMaxLimitNumandrequestMaxLimitNumwill silently truncate decimal inputs (e.g., "100.5" becomes 100). While the UI likely constrains to integers, this could cause confusion if a user enters a decimal value. The budget correctly usesparseFloat.This is minor since rate limits are typically integers, but worth noting for consistency.
ui/app/workspace/user-groups/views/customerTable.tsx (1)
30-33: Consider extractingformatResetDurationto a shared utility.This helper function is duplicated across
customerTable.tsxand likelyteamsTable.tsx. Consider moving it to@/lib/utils/governance.tsalongsideformatCurrencyfor better reusability and consistency.
danpiths
force-pushed
the
feature/02-12-feat_adds_rate_limits_to_customers_and_teams
branch
2 times, most recently
from
f317171 to
16da591
Compare
danpiths
force-pushed
the
feature/02-12-feat_adds_rate_limits_to_customers_and_teams
branch
from
16da591 to
95af7f5
Compare
akshaydeo
deleted the
feature/02-12-feat_adds_rate_limits_to_customers_and_teams
branch
2 participants
Summary
Added rate limit support to teams and customers, allowing for hierarchical rate limiting at the organization level in addition to the existing virtual key level rate limits.
Changes
rate_limit_idcolumn to bothgovernance_teamsandgovernance_customerstablesType of change
Affected areas
How to test
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
Rate limits provide an additional layer of security by preventing abuse of the API.
Checklist