feat(models): add db-backed model access overrides#218
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
💤 Files with no reviewable changes (5)
📝 WalkthroughWalkthroughAdds a model-override access-control subsystem: configuration, persistence backends, in-memory service with background refresh, admin UI/API for CRUD, request-time authorizer integrated across listing, resolution, passthrough, and batch flows, plus tests and app/server wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Resolver
participant Authorizer
participant Provider
Client->>Handler: Send inference request (with requested model)
Handler->>Resolver: Resolve requested model selector
Resolver-->>Handler: Resolved selector
Handler->>Authorizer: ValidateModelAccess(ctx, resolved selector)
alt denied
Authorizer-->>Handler: error (model_access_denied)
Handler-->>Client: HTTP 400 (denied)
else allowed
Authorizer-->>Handler: ok
Handler->>Provider: Execute request with model
Provider-->>Handler: Response
Handler-->>Client: HTTP 200
end
sequenceDiagram
participant Admin
participant AdminUI
participant AdminHandler
participant OverrideService
participant Store
Admin->>AdminUI: Open model overrides editor
AdminUI->>AdminHandler: GET /admin/api/v1/models
AdminHandler->>OverrideService: List views / effective state
OverrideService-->>AdminHandler: Models + access state
AdminHandler-->>AdminUI: Render list
Admin->>AdminUI: Submit override (PUT)
AdminUI->>AdminHandler: PUT /admin/api/v1/model-overrides/{selector}
AdminHandler->>OverrideService: Upsert override
OverrideService->>Store: Persist override
Store-->>OverrideService: OK
OverrideService->>OverrideService: Refresh snapshot
OverrideService-->>AdminHandler: Success
AdminHandler-->>AdminUI: HTTP 200
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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 |
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/modeloverrides/batch_preparer.go`:
- Around line 88-90: The code currently discards the second return value from
resolver.ResolveModel; update the branch that checks
p.provider.(selectorResolver) to capture the three returns (selector, changed,
err) from ResolveModel and return both selector and changed along with err so
the "changed" flag is propagated (match the pattern used in
internal/server/request_model_resolution.go); specifically modify the block
using p.provider, selectorResolver and ResolveModel to stop ignoring the second
value and pass it through.
In `@internal/modeloverrides/service.go`:
- Around line 185-205: The code currently calls s.store.Upsert / s.store.Delete
and only then s.Refresh, which can leave storage mutated while the in-memory
snapshot fails to update; instead, compute and validate the new snapshot before
committing storage and only persist if that validation succeeds. Concretely: in
Service.Upsert and Service.Delete (after normalizeSelectorInput /
selectorProviderNames), construct the prospective in-memory snapshot by applying
the normalized change to the current snapshot (or use a helper that mirrors
Refresh logic to validate the new state), ensure that snapshot build/validation
succeeds, and only then call s.store.Upsert or s.store.Delete followed by
s.Refresh; if you can’t build the snapshot beforehand, persist inside a
transactional API on s.store (begin/commit/rollback) or persist then roll back
(call s.store.Delete or restore previous entry) on Refresh failure so storage
and in-memory state remain consistent.
- Around line 328-350: The conflict resolution can leave state.Enabled true
after state.ForceDisabled was set because later rules set Enabled without
respecting ForceDisabled; update the apply function used for compiledOverride
(and any places merging modelWide/providerWide/exact) so that once
state.ForceDisabled is true it cannot be undone — either exit early from apply
when state.ForceDisabled is already set or guard the Enabled assignment
(rule.override.Enabled) to skip setting state.Enabled if state.ForceDisabled is
true; ensure state.ForceDisabled is still set when rule.override.ForceDisabled
is true and that addAllowed still runs.
- Around line 219-233: Replace the discarded call to s.Refresh(refreshCtx) with
error handling: capture the returned error (err := s.Refresh(refreshCtx)); if
non-nil, log it the same way registry.go does (e.g., s.logger.Errorf or
equivalent) and/or send it to a supervisory channel instead of dropping it; if
you choose to surface the error to callers, wrap/convert it into a
core.GatewayError instance per guidelines before propagating. Ensure you update
the anonymous goroutine around s.Refresh and keep done/ticker behavior
unchanged.
In `@internal/server/handlers.go`:
- Around line 245-252: The current branch merges unfiltered ExposedModels() when
modelAuthorizer is present but exposedModelLister does not implement
FilteredExposedModelLister; instead, when modelAuthorizer != nil you must filter
the results from exposedModelLister.ExposedModels() using
modelAuthorizer.AllowsModel before calling mergeExposedModelsResponse so denied
aliases are not reintroduced. Update the else-path to obtain the slice from
exposedModelLister.ExposedModels(), iterate and retain only models where
h.modelAuthorizer.AllowsModel(ctx, selector) returns true (using the same
selector logic as ExposedModelsFiltered), then pass the filtered slice to
mergeExposedModelsResponse; keep existing behavior unchanged when
modelAuthorizer is nil or when exposedModelLister already implements
FilteredExposedModelLister.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5383edf9-d7d2-4a8c-adcd-e188ced58649
📒 Files selected for processing (29)
config/config.example.yamlconfig/config.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/templates/index.htmlinternal/admin/handler.gointernal/admin/handler_model_overrides_test.gointernal/aliases/service.gointernal/app/app.gointernal/modeloverrides/batch_preparer.gointernal/modeloverrides/factory.gointernal/modeloverrides/service.gointernal/modeloverrides/service_test.gointernal/modeloverrides/store.gointernal/modeloverrides/store_mongodb.gointernal/modeloverrides/store_postgresql.gointernal/modeloverrides/store_sqlite.gointernal/modeloverrides/types.gointernal/server/execution_plan_helpers.gointernal/server/exposed_model_lister.gointernal/server/handlers.gointernal/server/http.gointernal/server/internal_chat_completion_executor.gointernal/server/model_access.gointernal/server/native_batch_service.gointernal/server/native_batch_support.gointernal/server/passthrough_service.gointernal/server/request_model_resolution.gointernal/server/translated_inference_service.go
Comment on lines
+88
to
+90
| if resolver, ok := p.provider.(selectorResolver); ok { | ||
| selector, _, err := resolver.ResolveModel(requested) | ||
| return selector, err |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect resolver implementations and existing call sites to confirm whether
# the boolean return is used as a "handled/resolved" signal anywhere else.
rg -n -C4 --type go 'func .*ResolveModel\s*\('
rg -n -C4 --type go '\.ResolveModel\s*\('Repository: ENTERPILOT/GoModel
Length of output: 11866
Respect the resolver's second return value to match established patterns in the codebase.
ResolveModel returns (selector, changed, err). While the second value indicates whether the model was changed (not whether it was handled), the pattern in internal/server/request_model_resolution.go (lines 119, 128) shows this value should be captured and propagated, not discarded. Lines 88–90 should capture and return it instead of silently dropping it.
Suggested fix
func (p *BatchPreparer) resolveSelector(requested core.RequestedModelSelector) (core.ModelSelector, error) {
if p == nil || p.provider == nil {
return requested.Normalize()
}
if resolver, ok := p.provider.(selectorResolver); ok {
- selector, _, err := resolver.ResolveModel(requested)
+ selector, _, err := resolver.ResolveModel(requested)
+ if err != nil {
+ return core.ModelSelector{}, err
+ }
return selector, err
}
return requested.Normalize()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if resolver, ok := p.provider.(selectorResolver); ok { | |
| selector, _, err := resolver.ResolveModel(requested) | |
| return selector, err | |
| if resolver, ok := p.provider.(selectorResolver); ok { | |
| selector, _, err := resolver.ResolveModel(requested) | |
| if err != nil { | |
| return core.ModelSelector{}, err | |
| } | |
| return selector, err |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/modeloverrides/batch_preparer.go` around lines 88 - 90, The code
currently discards the second return value from resolver.ResolveModel; update
the branch that checks p.provider.(selectorResolver) to capture the three
returns (selector, changed, err) from ResolveModel and return both selector and
changed along with err so the "changed" flag is propagated (match the pattern
used in internal/server/request_model_resolution.go); specifically modify the
block using p.provider, selectorResolver and ResolveModel to stop ignoring the
second value and pass it through.
Comment on lines
+328
to
+350
| apply := func(rule compiledOverride, ok bool) { | ||
| if !ok { | ||
| return | ||
| } | ||
| if rule.override.Enabled != nil && *rule.override.Enabled { | ||
| state.Enabled = true | ||
| } | ||
| if rule.override.ForceDisabled { | ||
| state.Enabled = false | ||
| state.ForceDisabled = true | ||
| } | ||
| addAllowed(rule.override.AllowedOnlyForUserPaths) | ||
| } | ||
|
|
||
| if modelWide, ok := snap.modelWide[model]; ok { | ||
| apply(modelWide, true) | ||
| } | ||
| if providerWide, ok := snap.providerWide[providerName]; ok { | ||
| apply(providerWide, true) | ||
| } | ||
| if exact, ok := snap.exact[exactMatchKey(providerName, model)]; ok { | ||
| apply(exact, true) | ||
| } |
Contributor
There was a problem hiding this comment.
Make force_disabled conflict resolution deterministic.
Lines 332-338 let a later enabled=true rule turn Enabled back on after an earlier rule has already set ForceDisabled=true. That can produce Enabled=true and ForceDisabled=true simultaneously, and AllowsModel / ValidateModelAccess will currently allow the model because they only consult Enabled.
Suggested fix
apply := func(rule compiledOverride, ok bool) {
if !ok {
return
}
- if rule.override.Enabled != nil && *rule.override.Enabled {
- state.Enabled = true
- }
if rule.override.ForceDisabled {
state.Enabled = false
state.ForceDisabled = true
}
+ if !state.ForceDisabled && rule.override.Enabled != nil && *rule.override.Enabled {
+ state.Enabled = true
+ }
addAllowed(rule.override.AllowedOnlyForUserPaths)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| apply := func(rule compiledOverride, ok bool) { | |
| if !ok { | |
| return | |
| } | |
| if rule.override.Enabled != nil && *rule.override.Enabled { | |
| state.Enabled = true | |
| } | |
| if rule.override.ForceDisabled { | |
| state.Enabled = false | |
| state.ForceDisabled = true | |
| } | |
| addAllowed(rule.override.AllowedOnlyForUserPaths) | |
| } | |
| if modelWide, ok := snap.modelWide[model]; ok { | |
| apply(modelWide, true) | |
| } | |
| if providerWide, ok := snap.providerWide[providerName]; ok { | |
| apply(providerWide, true) | |
| } | |
| if exact, ok := snap.exact[exactMatchKey(providerName, model)]; ok { | |
| apply(exact, true) | |
| } | |
| apply := func(rule compiledOverride, ok bool) { | |
| if !ok { | |
| return | |
| } | |
| if rule.override.ForceDisabled { | |
| state.Enabled = false | |
| state.ForceDisabled = true | |
| } | |
| if !state.ForceDisabled && rule.override.Enabled != nil && *rule.override.Enabled { | |
| state.Enabled = true | |
| } | |
| addAllowed(rule.override.AllowedOnlyForUserPaths) | |
| } | |
| if modelWide, ok := snap.modelWide[model]; ok { | |
| apply(modelWide, true) | |
| } | |
| if providerWide, ok := snap.providerWide[providerName]; ok { | |
| apply(providerWide, true) | |
| } | |
| if exact, ok := snap.exact[exactMatchKey(providerName, model)]; ok { | |
| apply(exact, true) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/modeloverrides/service.go` around lines 328 - 350, The conflict
resolution can leave state.Enabled true after state.ForceDisabled was set
because later rules set Enabled without respecting ForceDisabled; update the
apply function used for compiledOverride (and any places merging
modelWide/providerWide/exact) so that once state.ForceDisabled is true it cannot
be undone — either exit early from apply when state.ForceDisabled is already set
or guard the Enabled assignment (rule.override.Enabled) to skip setting
state.Enabled if state.ForceDisabled is true; ensure state.ForceDisabled is
still set when rule.override.ForceDisabled is true and that addAllowed still
runs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/passthrough_provider_resolution.go`:
- Around line 40-76: Add a short doc comment above the function
passthroughAccessSelector explaining that it may return a core.ModelSelector
with an empty Provider field when resolvePassthroughProvider yields an empty
ProviderName and none of the candidates from
ProviderNameResolver.GetProviderName resolve to a non-empty canonical name;
state that this is intentional and that downstream authorization/validation is
expected to handle empty Provider values, and mention the roles of parameters
(provider core.RoutableProvider, info *core.PassthroughRouteInfo) and the
returned (core.ModelSelector, bool) to clarify the contract.
In `@internal/server/request_model_resolution_test.go`:
- Around line 40-52: The test double
canonicalizingProvider.GetProviderTypeForName currently returns
p.types[qualifiedModel] without trimming; change it to return
strings.TrimSpace(p.types[qualifiedModel]) so its output matches production
behavior (similar to registry.go's strings.TrimSpace(r.providerTypes[provider])
usage), ensuring you import/use strings and preserve existing early returns and
loop logic.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9f73c7f6-998d-49ec-908c-9116258a26a9
📒 Files selected for processing (18)
internal/core/interfaces.gointernal/modeloverrides/service.gointernal/modeloverrides/service_test.gointernal/providers/registry.gointernal/providers/router.gointernal/server/handlers.gointernal/server/handlers_test.gointernal/server/http.gointernal/server/model_validation.gointernal/server/model_validation_test.gointernal/server/passthrough_execution_helpers.gointernal/server/passthrough_execution_helpers_test.gointernal/server/passthrough_provider_resolution.gointernal/server/passthrough_semantic_enrichment.gointernal/server/passthrough_semantic_enrichment_test.gointernal/server/passthrough_service.gointernal/server/request_model_resolution_test.gotests/integration/setup_test.go
5 tasks
This was referenced Jun 22, 2026
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
1 participant
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
model_overridessubsystem with DB-backed storage and atomic in-memory refreshMODELS_ENABLED_BY_DEFAULT, explicit enable, global force-disable, and managed API-keyuser_pathrestrictionsMerge
origin/maininto this branch and resolved the request model resolution conflictVerification
Summary by CodeRabbit
New Features
Tests