Conversation
… any model rather than restrict it on vk
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughA parameter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Confidence Score: 4/5
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.
🧹 Nitpick comments (2)
plugins/governance/resolver_test.go (1)
26-26: The new resolver dependency is still untested.All updated constructor call sites pass
nilfor the added store argument, so none of these tests exercise the provider-config-aware path that motivated this change. Please add at least oneEvaluateVirtualKeyRequestcase with a real custom-provider config andListModels=falseso the new behavior is covered end to end.Also applies to: 41-41, 59-59, 82-82, 114-114, 137-137, 198-198, 220-220, 247-247, 281-281, 315-315, 338-338, 353-353, 401-401, 476-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/resolver_test.go` at line 26, Tests currently construct the resolver with NewBudgetResolver(store, nil, logger, nil) leaving the new provider-config store untested; add at least one EvaluateVirtualKeyRequest test case in resolver_test.go that constructs the resolver with a real provider-config (non-nil store) and supplies an EvaluateVirtualKeyRequest containing a custom-provider config and ListModels=false so the provider-config-aware path in methods like EvaluateVirtualKeyRequest is executed end-to-end; update the table-driven tests (the cases referenced around NewBudgetResolver calls) to include this case and assert the expected provider-specific behavior/output.framework/modelcatalog/main_test.go (1)
163-209: Add coverage for the empty-allowlist branch.The new logic treats both
[]and["*"]as unrestricted, but these additions only exercise the wildcard branch. A regression in thelen(allowedModels) == 0path would still pass this suite.🧪 Suggested test
+func TestIsModelAllowedForProvider_CustomProviderListModelsDisabled_EmptyAllowedModels(t *testing.T) { + mc := newTestCatalog(nil, nil) + + providerConfig := configstore.ProviderConfig{ + CustomProviderConfig: &schemas.CustomProviderConfig{ + AllowedRequests: &schemas.AllowedRequests{ + ListModels: false, + }, + }, + } + + assert.True(t, mc.IsModelAllowedForProvider("custom-provider", "any-model", &providerConfig, []string{})) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main_test.go` around lines 163 - 209, Add a test covering the empty-allowlist branch by calling mc.IsModelAllowedForProvider with allowedModels set to an empty slice (e.g., []string{}) and verifying it behaves like the wildcard case; specifically, add a test (similar to TestIsModelAllowedForProvider_CustomProviderListModelsEnabled or TestIsModelAllowedForProvider_NilProviderConfig) that uses newTestCatalog and providerConfig as appropriate and asserts that an in-catalog model returns true and an out-of-catalog model returns false when allowedModels is []string{} to ensure the len(allowedModels) == 0 path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/modelcatalog/main_test.go`:
- Around line 163-209: Add a test covering the empty-allowlist branch by calling
mc.IsModelAllowedForProvider with allowedModels set to an empty slice (e.g.,
[]string{}) and verifying it behaves like the wildcard case; specifically, add a
test (similar to TestIsModelAllowedForProvider_CustomProviderListModelsEnabled
or TestIsModelAllowedForProvider_NilProviderConfig) that uses newTestCatalog and
providerConfig as appropriate and asserts that an in-catalog model returns true
and an out-of-catalog model returns false when allowedModels is []string{} to
ensure the len(allowedModels) == 0 path is exercised.
In `@plugins/governance/resolver_test.go`:
- Line 26: Tests currently construct the resolver with NewBudgetResolver(store,
nil, logger, nil) leaving the new provider-config store untested; add at least
one EvaluateVirtualKeyRequest test case in resolver_test.go that constructs the
resolver with a real provider-config (non-nil store) and supplies an
EvaluateVirtualKeyRequest containing a custom-provider config and
ListModels=false so the provider-config-aware path in methods like
EvaluateVirtualKeyRequest is executed end-to-end; update the table-driven tests
(the cases referenced around NewBudgetResolver calls) to include this case and
assert the expected provider-specific behavior/output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc47d535-7027-4211-b3e7-4be196c3a76b
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
framework/modelcatalog/main.goframework/modelcatalog/main_test.goplugins/governance/main.goplugins/governance/model_provider_governance_test.goplugins/governance/resolver.goplugins/governance/resolver_test.goplugins/governance/tracker_test.goplugins/governance/utils.gotransports/bifrost-http/handlers/providers.go
akshaydeo
deleted the
fix/allow_custom_providers_without_a_list_models_endpoint_to_pass_in_any_model_rather_than_restrict_it_on_vk
branch
3 participants
Summary
Fixes model allowance checking for custom providers by passing provider
configuration to the model catalog. This enables proper handling of custom
providers that have the list-models endpoint disabled, allowing them to bypass
catalog validation when using unrestricted model lists.
Changes
IsModelAllowedForProviderto accept aProviderConfigparameterand handle custom providers without list-models endpoints
IsModelAllowedForProviderto pass the providerconfiguration
list-models when using unrestricted allowed models (
[]or["*"])BudgetResolverto accept and use an in-memory store for providerconfiguration access
Type of change
Affected areas
How to test
Validate that custom providers with disabled list-models endpoints can use
unrestricted model lists:
Test scenarios:
["*"]→ should allow any model["*"]→ should use catalogvalidation
Screenshots/Recordings
N/A - Backend logic changes only
Breaking changes
The API signature change is internal and all callers have been updated in this
PR.
Related issues
#2351
Security considerations
This change relaxes validation for a specific case (custom providers without
list-models + unrestricted model lists), but maintains security by:
Checklist
docs/contributing/README.mdand followed the guidelines