feat: Add v2 API for governance and update UI to use the multi budget lines - #3960
Conversation
|
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces versioned v2 API endpoints for provider governance with multi-budget support and calendar alignment. The backend refactors route registration to support versioned handlers, adds v2 response/request types, implements GET/PUT handlers with multi-budget semantics, and includes comprehensive tests. The frontend updates type definitions, API layer endpoints, form component logic, and display components to work with multi-budget governance. ChangesMulti-Budget Provider Governance v2 Support
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend Client
participant APILayer as API Layer
participant BackendV2 as Backend v2 Handler
participant Storage as ConfigStore
participant Mapper as Model Mapper
Frontend->>APILayer: getProviderGovernance()
APILayer->>BackendV2: GET /api/v2/governance/providers
BackendV2->>Storage: GetProviderGovernanceModelConfigs()
Storage-->>BackendV2: TableModelConfig[]
BackendV2->>Mapper: modelConfigToProviderGovernanceV2()
Mapper-->>BackendV2: ProviderGovernanceResponseV2[]
BackendV2-->>APILayer: JSON response
APILayer-->>Frontend: ProviderGovernance {budgets[], rate_limit, calendar_aligned}
Frontend->>APILayer: updateProviderGovernance(provider, updates)
APILayer->>BackendV2: PUT /api/v2/governance/providers/{provider}
BackendV2->>Storage: UpdateProviderGovernanceModelConfig()
alt Budgets Changed
BackendV2->>Storage: reconcileModelConfigBudgets()
end
alt CalendarAligned Changed
BackendV2->>Storage: Update calendar_aligned field
end
Storage-->>BackendV2: Updated config
BackendV2->>Mapper: modelConfigToProviderGovernanceV2()
Mapper-->>BackendV2: ProviderGovernanceResponseV2
BackendV2-->>APILayer: JSON response
APILayer-->>Frontend: Updated ProviderGovernance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
37612ab to
10da233
Compare
096e68f to
e63c607
Compare
10da233 to
d7f6f08
Compare
Confidence Score: 4/5Safe to merge after fixing the rate limit dropdown regression; the Go backend and test additions are solid. The Go backend changes are well-structured and the orphaned-budget deletion is correctly handled. The one concrete defect is in the UI form: the rate limit duration dropdowns were silently narrowed from 9 options to 4, meaning any operator whose saved rate limit uses a sub-hour or 6-hour interval will see that value absent from the select options — re-saving the form would silently overwrite the stored duration with whichever option the control falls back to. ui/app/workspace/providers/fragments/governanceFormFragment.tsx — rate limit NumberAndSelect options need to be restored to the full set (resetDurationOptions or equivalent). Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/governance_test.go (1)
1860-1860: ⚡ Quick winUse existing
schemas.Ptrinstead of introducing a newstrPtrhelper.This file already uses
schemas.Ptrextensively (lines 135, 136, 137, 143, 182, etc.). Introducing a newstrPtrhelper creates inconsistency. ReplacestrPtr("openai")calls withschemas.Ptr("openai")throughout the new tests and remove this helper.♻️ Proposed fix
-func strPtr(s string) *string { return &s }Then update line 1556 and similar usages:
- mc: &configstoreTables.TableModelConfig{Scope: "virtual_key", ModelName: "*", Provider: strPtr("openai")}, + mc: &configstoreTables.TableModelConfig{Scope: "virtual_key", ModelName: "*", Provider: schemas.Ptr("openai")},🤖 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_test.go` at line 1860, Replace the local helper function strPtr and all its usages with the existing schemas.Ptr helper to keep consistency: remove the strPtr declaration (func strPtr(s string) *string { return &s }) and change each call like strPtr("openai") in the new tests to schemas.Ptr("openai"); ensure imports/reference to schemas remain intact and run tests to confirm no regressions.
🤖 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 3671-3687: When deleting a model config in the branch handling
"!hasGovernance && !isNew" you must delete its owned budgets before removing the
config to avoid orphaned rows; before the
tx.Delete(&configstoreTables.TableModelConfig{}, "id = ?", mc.ID) call, run the
same budget-delete logic used in deleteVKModelConfig (delete budgets where
model_config_id = mc.ID within the same tx) or call the existing configStore
method that removes model config budgets, then proceed to delete the model
config; this ensures reconcileModelConfigBudgets (which is skipped when deleted
== true) cannot leave budget rows dangling.
In `@ui/app/workspace/providers/fragments/governanceFormFragment.tsx`:
- Around line 26-30: The budgetLineSchema lacks user-facing Zod error messages;
update the fields in budgetLineSchema (the z.object with keys id, max_limit,
reset_duration) to include meaningful .error messages and validators: e.g., for
max_limit (currently z.number().nonnegative().optional()) add a .nonnegative({
message: "Maximum limit must be 0 or greater" }) and .optional() handling error
text, for reset_duration (currently z.string()) require nonempty with
z.string().nonempty({ message: "Reset duration is required" }) or a
z.enum/refinement with a clear message, and for id keep z.string().optional()
but supply a validation/message if you expect a UUID (z.string().uuid({ message:
"Invalid id format" }).optional()) or at least .optional().refine if needed;
ensure all validators on budgetLineSchema provide explicit message strings to
satisfy the UI Zod schema guideline.
In `@ui/app/workspace/providers/views/providerGovernanceTable.tsx`:
- Around line 188-190: The sort currently uses new Date(b.created_at ??
0).getTime() which can misplace items with missing created_at; explicitly map
created_at to a numeric timestamp (e.g., const ts = created_at ?
Date.parse(created_at) : Number.NEGATIVE_INFINITY) and use those timestamps in
the comparator so missing created_at are treated as oldest and appear last in
the newest-first sort; update the budgets creation that references
providerGovernance?.budgets to use this explicit timestamp parsing in the (a,b)
=> comparator.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/governance_test.go`:
- Line 1860: Replace the local helper function strPtr and all its usages with
the existing schemas.Ptr helper to keep consistency: remove the strPtr
declaration (func strPtr(s string) *string { return &s }) and change each call
like strPtr("openai") in the new tests to schemas.Ptr("openai"); ensure
imports/reference to schemas remain intact and run tests to confirm no
regressions.
🪄 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: 0942185f-4fac-4d21-86c3-a5c8fea0c790
📒 Files selected for processing (7)
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.goui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
d7f6f08 to
5ced207
Compare
2 participants
Summary
This PR introduces a v2 API for provider governance that supports multiple budgets per provider, alongside a
calendar_alignedfield. Previously, the provider governance API exposed only a single budget; the v2 endpoints expose the full list. The UI is updated to consume the v2 endpoints and render all budgets.Changes
versionedRoutestruct and aroutes()method onGovernanceHandlerto declare the full route table in one place. Each route registers under/api(compat),/api/v1, and optionally/api/v2when a v2 handler is explicitly provided — no implicit v1 fallback under/api/v2.getProviderGovernanceV2andupdateProviderGovernanceV2handlers. The v2 GET returnsProviderGovernanceResponseV2(all budgets +calendar_aligned); the v2 PUT acceptsUpdateProviderGovernanceRequestV2whereBudgetsis*[]so a nil field means "no change" and a non-nil empty slice means "remove all".modelConfigToProviderGovernanceV2which returns all budgets (copied, not aliased) andcalendar_aligned, replacing the v1 single-budget surface.UpdateProviderGovernanceRequestV2.Budgetsuses a pointer-to-slice to distinguish "absent" from "empty array" after JSON unmarshal.governanceApiGET and PUT provider governance queries now target/v2/governance/providers.ProviderGovernancetype updated tobudgets?: Budget[](dropping the singlebudgetfield) andUpdateProviderGovernanceRequestupdated tobudgets?: CreateBudgetRequest[].created_at. Budget cards are labeled with their reset duration when multiple budgets exist.GovernanceFormFragmentreplaced the single-budgetFormFieldwith theMultiBudgetLinescomponent and simplified the twouseEffectreset blocks via a sharedgovernanceToFormValueshelper.modelConfigToProviderGovernanceV2(filter logic, field mapping, copy semantics),getProviderGovernanceV2(from-memory, nil data, DB path, DB error),UpdateProviderGovernanceRequestV2pointer semantics, and the routes table contract (every route has a v1 handler, provider GET/PUT have v2 handlers, DELETE does not, no duplicates).Type of change
Affected areas
How to test
To validate the v2 endpoints manually:
Screenshots/Recordings
Provider governance table and form now display one card/row per budget, labeled with the reset duration when more than one budget is present.
Breaking changes
The
ProviderGovernanceTypeScript type drops the singlebudgetfield in favour ofbudgets: Budget[]. Any UI code referencingproviderGovernance.budgetdirectly will need to be updated to useproviderGovernance.budgets?.[0]or iterate the array. The backend v1 endpoint is unchanged and continues to return a singlebudgetfield; only the UI now calls v2.Related issues
Security considerations
No new auth surfaces. The v2 endpoints are registered with the same admin middleware chain as v1. The self-service
/api/governance/virtual-keys/quotaendpoint continues to bypass admin middlewares as before.Checklist
docs/contributing/README.mdand followed the guidelines