-
Notifications
You must be signed in to change notification settings - Fork 150
feat: adds backend go functions for model/provider governance configuration #1169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/12-17-chore_adds_test_for_governance_plugin_s_model_configs_and_providers_budgeting_and_rate_limiting
Are you sure you want to change the base?
Conversation
|
Warning Rate limit exceeded@danpiths has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds HTTP governance endpoints and server callbacks to manage model configs and providers; implements CRUD and provider-governance handlers, new request/response types, and synchronizes DB mutations with in-memory governance state via reload/remove operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as HTTP Handler
participant DB as Database
participant Gov as Governance Plugin
participant Mem as In-Memory Store
rect rgb(235,245,255)
note over Client,HTTP: Create or Update ModelConfig / Provider
Client->>HTTP: POST/PUT /api/governance/...
activate HTTP
HTTP->>DB: persist create/update/delete
DB-->>HTTP: confirmation
HTTP->>Gov: ReloadModelConfig(id) / ReloadProvider(name)
activate Gov
Gov->>DB: fetch entity + budget/rate_limit
DB-->>Gov: entity record(s)
Gov->>Mem: update in-memory store
Mem-->>Gov: ack
Gov-->>HTTP: updated entity
deactivate Gov
alt budget/rate_limit usage changed
HTTP->>DB: sync updated budget/rate_limit usage
DB-->>HTTP: persisted
end
HTTP-->>Client: response
deactivate HTTP
end
rect rgb(235,255,240)
note over Client,HTTP: Delete ModelConfig or Provider
Client->>HTTP: DELETE /api/governance/...
activate HTTP
HTTP->>DB: delete entity
DB-->>HTTP: confirmation
HTTP->>Gov: RemoveModelConfig(id) / RemoveProvider(name)
Gov->>Mem: delete from in-memory store
Mem-->>Gov: ack
Gov-->>HTTP: confirmation
HTTP-->>Client: success
deactivate HTTP
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/server/server.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/governance.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/governance.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/governance.go
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (16)
transports/bifrost-http/server/server.go (5)
76-79: LGTM!The new interface methods follow the established pattern for reload/remove operations and are consistent with similar methods like
ReloadVirtualKeyandReloadTeam.
652-689: LGTM! Sync pattern mirrors existing implementations.The method correctly loads the model config, updates in-memory state, and conditionally syncs changed usage values back to the database. The pattern is consistent with similar reload methods for virtual keys and teams.
691-699: LGTM!The method correctly removes the model config from in-memory store only, following the established pattern where DB deletion is handled separately by the caller.
701-738: LGTM! Consistent with ReloadModelConfig pattern.The method follows the same structure as
ReloadModelConfig, loading the provider by name, updating in-memory state, and syncing modified usage values back to the database.
740-748: LGTM!The method correctly removes the provider from in-memory store, consistent with the pattern used in
RemoveModelConfigand other remove operations.transports/bifrost-http/handlers/governance.go (11)
32-35: LGTM!The interface additions are consistent with the
ServerCallbacksinterface in server.go and follow the established pattern.
160-181: LGTM!The request/response types are well-structured and follow the established patterns for CRUD operations. Optional fields are correctly represented as pointers.
209-219: LGTM!The route registrations follow RESTful conventions and are consistent with the existing governance routes. Path parameters are appropriately named.
1586-1598: LGTM!The handler correctly retrieves and returns all model configs. Simple and straightforward implementation.
1600-1615: LGTM!The handler follows the standard pattern for retrieving a single resource by ID with appropriate error handling.
1617-1706: LGTM! Proper validation and transaction handling.The handler correctly validates input, creates the model config with optional budget and rate limit within a transaction, and reloads it into memory. The pattern is consistent with other create handlers.
1880-1911: LGTM!The handler follows the standard delete pattern: verify existence, delete from database, then remove from in-memory store. Error handling is appropriate.
1915-1920: LGTM!The response type appropriately represents a provider with its governance settings.
1922-1945: LGTM! Appropriate filtering for governance endpoint.The handler correctly retrieves all providers and filters to only return those with governance settings (budget or rate limit), which makes sense for a governance-focused API.
1947-2117: LGTM! Consistent with updateModelConfig pattern.The handler properly implements create/update/delete logic for provider governance settings. The implementation mirrors
updateModelConfig, maintaining consistency across the codebase.
2119-2185: LGTM! Proper cleanup of governance settings.The handler correctly removes budget and rate limit references from the provider, deletes the orphaned records within a transaction, and updates the in-memory state. The pattern is consistent with other deletion handlers.
7167ba6 to
ed45bf7
Compare
3c984ca to
548cfc9
Compare
ed45bf7 to
85fa076
Compare
548cfc9 to
d9d7bcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/governance.go (1)
1919-1942: Optional: Consider documenting provider filtering behavior.The handler only returns providers that have governance settings (
p.Budget != nil || p.RateLimit != nil, line 1930). While this reduces response size, users might expect to see all providers with an indication of which have governance. Consider adding a query parameter to optionally include all providers or documenting this filtering behavior in API docs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/server/server.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/server/server.go
🧠 Learnings (3)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/server/server.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/server/server.go
📚 Learning: 2025-12-24T04:36:57.698Z
Learnt from: danpiths
Repo: maximhq/bifrost PR: 1169
File: transports/bifrost-http/handlers/governance.go:1708-1878
Timestamp: 2025-12-24T04:36:57.698Z
Learning: In governance update handlers (e.g., updateModelConfig, updateProviderGovernance), design updates to support clearing individual fields by sending null/empty values (e.g., {"rate_limit": {"token_max_limit": null}} clears token_max_limit). Follow this pattern for partial updates so users can remove specific governance settings without deleting the whole entity. Ensure budget updates follow the same approach using direct field assignment. Review input validation, JSON decoding (e.g., pointers vs values in Go structs), and API documentation to reflect nullable fields and expected behavior.
Applied to files:
transports/bifrost-http/handlers/governance.go
🧬 Code graph analysis (1)
transports/bifrost-http/handlers/governance.go (6)
framework/configstore/tables/provider.go (2)
TableProvider(15-52)TableProvider(55-55)core/schemas/provider.go (1)
Provider(315-362)transports/bifrost-http/lib/middleware.go (1)
ChainMiddlewares(12-24)transports/bifrost-http/handlers/utils.go (2)
SendError(35-44)SendJSON(16-22)framework/configstore/tables/budget.go (2)
TableBudget(11-27)TableBudget(30-30)framework/configstore/tables/ratelimit.go (2)
TableRateLimit(11-36)TableRateLimit(39-39)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (8)
transports/bifrost-http/handlers/governance.go (5)
32-35: LGTM! Interface additions follow existing patterns.The new methods for model config and provider operations are consistent with the existing VirtualKey, Team, and Customer operations, maintaining a uniform interface design.
160-181: LGTM! Request/response types are well-structured.The new types follow the established patterns for governance entities, with appropriate use of pointers to support optional fields in update requests.
209-219: LGTM! RESTful route structure.The new routes follow REST conventions and integrate properly with the middleware chain. Note that provider governance uses PUT/DELETE only (no POST), which is correct since providers are managed via provider configuration.
1793-1857: LGTM! Rate limit updates and cleanup logic are correct.The rate limit update logic (lines 1810-1813) correctly uses direct assignment to support clearing individual fields via nil values, as intended per the established design pattern. The deferred deletion approach (lines 1847-1857) properly removes orphaned budget/rate limit records after clearing FK references to avoid constraint violations.
Based on learnings, this pattern allows users to remove specific governance settings without deleting the entire entity.
2023-2087: LGTM! Rate limit updates and cleanup follow established patterns.The rate limit update logic uses direct assignment to support clearing fields (lines 2040-2043), and the deferred deletion properly handles FK constraints (lines 2077-2087). Both are consistent with the model config implementation and the established design pattern.
Based on learnings, this implementation correctly supports clearing individual governance settings.
transports/bifrost-http/server/server.go (3)
76-79: LGTM! Interface extensions maintain consistency.The new callback methods follow the same pattern as existing entity operations (VirtualKey, Team, Customer). Note that
ReloadProviderandRemoveProvidercorrectly usenameas the identifier instead ofid, matching how providers are identified in the system.
652-699: LGTM! Model config reload/remove implementations are correct.Both methods follow the established pattern from VirtualKey operations:
ReloadModelConfigproperly loads from DB, updates in-memory state, and conditionally syncs modified usage values back to storageRemoveModelConfigcleanly removes from in-memory storeThe conditional persistence of usage changes (lines 671-686) ensures database updates only occur when values actually change, which is an appropriate optimization.
701-748: LGTM! Provider reload/remove implementations mirror model config pattern.Both methods correctly adapt the reload/remove pattern for providers:
ReloadProviderusesGetProviderByNameand properly syncs usage changesRemoveProvideruses the provider name for in-memory deletionThe implementation is consistent with the model config methods while appropriately handling provider-specific identification by name rather than ID.
85fa076 to
ff67ef4
Compare
d9d7bcc to
f2e6066
Compare
f2e6066 to
ed6660d
Compare
b128177 to
0ba0d94
Compare
ed6660d to
54081dd
Compare
5ff01f7 to
80f2779
Compare
0ba0d94 to
ffb1435
Compare
ffb1435 to
03f1cc1
Compare
80f2779 to
5c8aba4
Compare

Summary
Add API endpoints for model config and provider governance management, enabling
fine-grained control over model usage and provider-level governance settings.
Changes
/api/governance/model-configs)(
/api/governance/providers)in-memory store
provider operations
Type of change
Affected areas
How to test
Test the new API endpoints for model config management:
Test provider governance endpoints:
Breaking changes
Related issues
Implements model-level and provider-level governance controls
Security considerations
These endpoints should be properly secured as they control access to models and
providers.
Checklist
docs/contributing/README.mdand followed the guidelines