Skip to content

allow catalog opaque providers in unknown models - #4124

Merged
akshaydeo merged 1 commit into
devfrom
06-07-allow_catalog_opaque_providers_in_unknown_models
Jun 7, 2026
Merged

allow catalog opaque providers in unknown models#4124
akshaydeo merged 1 commit into
devfrom
06-07-allow_catalog_opaque_providers_in_unknown_models

Conversation

@akshaydeo

@akshaydeo akshaydeo commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Virtual keys with a wildcard ("*") allow-list on catalog-opaque providers (vLLM, Ollama, SGL, and other self-hosted or custom providers whose models are never indexed in the bundled model catalog) were being incorrectly dropped from BifrostContextKeyAvailableProviders and subsequently 403'd by the budget resolver. Because GetProvidersForModel returns empty for these providers, the catalog cross-check treated every uncatalogued model as forbidden — dead-ending requests before they reached the backend. This fix introduces a isCatalogOpaqueProvider guard that short-circuits the catalog check when a wildcard is paired with a provider the catalog cannot enumerate, honoring the intent of "*" as allow-all.

Changes

  • Introduced isCatalogOpaqueProvider in main.go to identify providers whose model set the catalog cannot enumerate (custom providers with a CustomProviderConfig, or native providers with an empty catalog model list).
  • In loadBalanceProvider, when AllowedModels.IsUnrestricted() and the provider is catalog-opaque, isProviderAllowed is set to true directly, bypassing the catalog cross-check that would otherwise exclude the provider.
  • In resolver.go (BudgetResolver.isModelAllowed), the same guard is applied so that a routed request is not 403'd after passing the AvailableProviders gate.
  • Added TestHTTPTransportPreHook_WildcardKeepsCatalogOpaqueProvider_VLLM to verify the end-to-end pre-hook behavior for a vLLM virtual key with a wildcard and an uncatalogued model.
  • Added TestBudgetResolver_EvaluateRequest_WildcardAllowsCatalogOpaqueProvider to verify that the resolver allows wildcard + catalog-opaque (vLLM/uncatalogued model), while catalog-known providers (OpenAI) remain fully catalog-cross-checked under the same wildcard.

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

go test ./plugins/governance/...

Expected: all tests pass, including the two new tests:

  • TestHTTPTransportPreHook_WildcardKeepsCatalogOpaqueProvider_VLLM
  • TestBudgetResolver_EvaluateRequest_WildcardAllowsCatalogOpaqueProvider

To validate the fix manually, configure a virtual key with a vLLM provider and allowed_models: ["*"], then send a chat completion request with a self-hosted model name. The request should route successfully instead of returning a 403 or empty provider error.

Screenshots/Recordings

N/A

Breaking changes

  • Yes
  • No

Related issues

Closes #4122
Closes #3282

Security considerations

The wildcard bypass is scoped exclusively to providers the catalog has no knowledge of. For all catalog-known providers (e.g., OpenAI, Anthropic), the existing catalog cross-check is preserved, so there is no regression in model-level access control for first-party providers.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

Summary by CodeRabbit

  • Tests

    • Added coverage for wildcard model provider access patterns when specific providers lack enumerated model catalogs.
    • Added validation tests ensuring configured blacklist restrictions are properly enforced in conjunction with wildcard access permissions.
  • Improvements

    • Enhanced model catalog resolution logic to enable wildcard access for providers without catalogued model information while maintaining compliance with configured blacklist restrictions.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@akshaydeo, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 40 minutes and 40 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 90b50db3-a230-4eda-b803-afc374206ba7

📥 Commits

Reviewing files that changed from the base of the PR and between 03f583a and 72124ed.

📒 Files selected for processing (3)
  • framework/modelcatalog/models.go
  • plugins/governance/httptransportprehook_test.go
  • plugins/governance/resolver_test.go
📝 Walkthrough

Walkthrough

This PR adds catalog-opaque provider support to wildcard model allow-lists, enabling requests for uncatalogued self-hosted models (like vLLM) when configured with wildcard access. A new IsCatalogOpaqueProvider helper detects providers whose models cannot be enumerated from the catalog, and the wildcard logic now allows models immediately for opaque providers rather than consulting the catalog. Blacklist restrictions are tested to ensure they take precedence.

Changes

Wildcard Model Allowance with Catalog-Opaque Providers

Layer / File(s) Summary
Catalog-opaque detection and wildcard allowance logic
framework/modelcatalog/models.go
New IsCatalogOpaqueProvider method detects custom and unlisted providers; IsModelAllowedForProvider wildcard branch now delegates opacity checks to this helper, allowing models immediately for opaque providers instead of catalog-based validation.
Tests for opaque and catalog-known providers
plugins/governance/resolver_test.go, plugins/governance/httptransportprehook_test.go
Budget resolver and HTTP pre-hook tests verify wildcard allows uncatalogued models for opaque providers while preserving catalog checks for known providers; additional tests confirm blacklist exclusions apply regardless of wildcard configuration.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • maximhq/bifrost#3718: Both PRs modify the governance wildcard and blacklist enforcement path, with this PR adding test coverage for the existing blacklist-precedence behavior.
  • maximhq/bifrost#3653: Related through blacklist enforcement logic; this PR's tests validate that wildcard allow-lists still honor blacklist restrictions in alignment with the precedence semantics from #3653.

Suggested reviewers

  • danpiths
  • roroghost17

🐰 A wildcard wish for catalog-opaque dreams,
Where vLLM models bloom in untamed schemes,
Tests confirm the blacklist holds the line,
Self-hosted models now can shine!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the primary fix: allowing catalog-opaque providers to work with wildcard allow-lists for unknown models.
Description check ✅ Passed The PR description is comprehensive and follows the template structure, including summary, changes, type, affected areas, testing instructions, and issue references.
Linked Issues check ✅ Passed The PR successfully addresses both linked issues: #4122 (v1.5.9 regression where wildcard keys trigger 403) and #3282 (provider-specific model identifiers rejected despite allow-all keys), with appropriate code fixes and tests.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing wildcard behavior for catalog-opaque providers. New tests verify the fixes, and the implementation is limited to governance and model catalog logic without extraneous modifications.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 06-07-allow_catalog_opaque_providers_in_unknown_models

Comment @coderabbitai help to get the list of available commands and usage tips.

@akshaydeo
akshaydeo marked this pull request as ready for review June 7, 2026 15:14

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@greptile-apps

greptile-apps Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

The change is narrowly scoped to the wildcard + catalog-opaque case; catalog-known providers (OpenAI, Anthropic, etc.) continue to be fully cross-checked, and the blacklist pass runs before the new shortcut on both affected paths.

The logic change is minimal and well-contained: a single new predicate gates a previously missing shortcut. The two-pass ordering (blacklist first, then wildcard-opaque) is preserved in both isModelAllowed and loadBalanceProvider, and is explicitly exercised by the new regression tests. No unguarded nil dereferences are introduced; the nil-receiver handling in IsCatalogOpaqueProvider is consistent with the rest of ModelCatalog. The test suite now covers the primary fix, mixed opaque/catalog provider interplay, VK exclusion scoping, and the blacklist-wins-over-opaque-wildcard ordering on both the pre-hook and resolver paths.

No files require special attention; all changes are in framework/modelcatalog/models.go (the new predicate) and test files.

Important Files Changed

Filename Overview
framework/modelcatalog/models.go Extracts catalog-opaque provider check into exported IsCatalogOpaqueProvider; updates IsModelAllowedForProvider wildcard branch to use it; nil-receiver handling and RLock usage are correct
plugins/governance/httptransportprehook_test.go Adds four new tests: vLLM wildcard fix, mixed opaque/catalog provider, VK-scoped exclusion, and blacklist-wins-over-opaque-wildcard; all cases are well-structured and cover the regression scenarios
plugins/governance/resolver_test.go Adds resolver-level tests for wildcard + opaque provider (allowed) and wildcard + opaque + blacklisted model (blocked); symmetric with the pre-hook tests and guards the two-pass ordering in isModelAllowed

Reviews (5): Last reviewed commit: "allow catalog opaque providers in unknow..." | Re-trigger Greptile

Comment thread plugins/governance/main.go Outdated
Comment thread plugins/governance/resolver_test.go
@akshaydeo
akshaydeo force-pushed the 06-07-allow_catalog_opaque_providers_in_unknown_models branch 2 times, most recently from 150b8c5 to 03f583a Compare June 7, 2026 16:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@framework/modelcatalog/models.go`:
- Around line 248-258: IsCatalogOpaqueProvider currently calls
GetModelsForProvider(provider) and uses len(...)==0 which forces allocation/copy
of the provider slice on the hot request path; change this to an allocation-free
existence/count check by adding and using a lightweight accessor (e.g.,
HasModelsForProvider(provider) bool or GetModelCountForProvider(provider) int)
that scans/reads the underlying index without allocating a full slice, then
replace the len(mc.GetModelsForProvider(provider)) == 0 check with the new
HasModelsForProvider/GetModelCountForProvider call inside
IsCatalogOpaqueProvider; keep the nil mc guard and existing logic otherwise.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8c2fb72e-0de2-4878-ad6c-b6b925af1be3

📥 Commits

Reviewing files that changed from the base of the PR and between dfd10e0 and 03f583a.

📒 Files selected for processing (3)
  • framework/modelcatalog/models.go
  • plugins/governance/httptransportprehook_test.go
  • plugins/governance/resolver_test.go

Comment thread framework/modelcatalog/models.go
@akshaydeo
akshaydeo force-pushed the 06-07-allow_catalog_opaque_providers_in_unknown_models branch from 03f583a to fd4768c Compare June 7, 2026 16:22
@akshaydeo
akshaydeo force-pushed the 06-07-allow_catalog_opaque_providers_in_unknown_models branch from fd4768c to 72124ed Compare June 7, 2026 16:28

akshaydeo commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jun 7, 4:39 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 7, 4:39 PM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo
akshaydeo merged commit f40e86a into dev Jun 7, 2026
15 of 16 checks passed
@akshaydeo
akshaydeo deleted the 06-07-allow_catalog_opaque_providers_in_unknown_models branch June 7, 2026 16:39
@akshaydeo akshaydeo mentioned this pull request Jun 7, 2026
18 tasks
akshaydeo added a commit that referenced this pull request Jun 7, 2026
## Summary

This is a hotfix release (v1.5.10) on top of v1.5.9 that addresses two targeted bugs in governance and provider model allow-listing. No new features, migrations, or behavioral opt-ins are introduced beyond the explicit opt-in flag described below.

## Changes

- **Virtual key usage dropped under user attribution (#4123):** When a request carried user attribution, virtual-key usage accounting was silently skipped, causing VK budget and rate-limit tracking to be incomplete. Governance now tracks both the virtual-key and user scopes by default. Callers that intentionally want user-only accounting can opt in by setting the new `BifrostContextKeySkipVirtualKeyUsageTracking` (`bifrost-skip-virtual-key-usage-tracking`) context key.

- **Wildcard allow-lists rejected models on catalog-opaque providers (#4124):** A `*` allowed-models entry was incorrectly blocking requests to providers whose model list cannot be enumerated by the catalog — custom providers without list-models support and keyless self-hosted deployments (vLLM, Ollama, SGL). A new shared `IsCatalogOpaqueProvider` helper is used to resolve wildcard allow-lists to allow-all for these providers.

## Type of change

- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
# Core/Transports
go version
go test ./...
```

**Virtual key usage tracking:** Send a request with user attribution set and verify that both the virtual-key and user usage scopes are incremented. Then set `bifrost-skip-virtual-key-usage-tracking` on the context and confirm only the user scope is tracked.

**Wildcard allow-lists:** Configure a virtual key with `allowed_models: ["*"]` against a custom or keyless self-hosted provider (vLLM/Ollama/SGL). Confirm requests are permitted rather than rejected.

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

- Closes #4123
- Closes #4124

## Security considerations

The new `BifrostContextKeySkipVirtualKeyUsageTracking` context key bypasses virtual-key usage accounting. Callers should ensure this flag is only set in controlled, intentional scenarios to avoid unintentionally circumventing budget and rate-limit enforcement on virtual keys.

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
@akshaydeo akshaydeo mentioned this pull request Jun 7, 2026
akshaydeo added a commit that referenced this pull request Jun 7, 2026
This is a hotfix release (v1.5.10) on top of v1.5.9 that addresses two targeted bugs in governance and provider model allow-listing. No new features, migrations, or behavioral opt-ins are introduced beyond the explicit opt-in flag described below.

- **Virtual key usage dropped under user attribution (#4123):** When a request carried user attribution, virtual-key usage accounting was silently skipped, causing VK budget and rate-limit tracking to be incomplete. Governance now tracks both the virtual-key and user scopes by default. Callers that intentionally want user-only accounting can opt in by setting the new `BifrostContextKeySkipVirtualKeyUsageTracking` (`bifrost-skip-virtual-key-usage-tracking`) context key.

- **Wildcard allow-lists rejected models on catalog-opaque providers (#4124):** A `*` allowed-models entry was incorrectly blocking requests to providers whose model list cannot be enumerated by the catalog — custom providers without list-models support and keyless self-hosted deployments (vLLM, Ollama, SGL). A new shared `IsCatalogOpaqueProvider` helper is used to resolve wildcard allow-lists to allow-all for these providers.

- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs

```sh
go version
go test ./...
```

**Virtual key usage tracking:** Send a request with user attribution set and verify that both the virtual-key and user usage scopes are incremented. Then set `bifrost-skip-virtual-key-usage-tracking` on the context and confirm only the user scope is tracked.

**Wildcard allow-lists:** Configure a virtual key with `allowed_models: ["*"]` against a custom or keyless self-hosted provider (vLLM/Ollama/SGL). Confirm requests are permitted rather than rejected.

- [ ] Yes
- [x] No

- Closes #4123
- Closes #4124

The new `BifrostContextKeySkipVirtualKeyUsageTracking` context key bypasses virtual-key usage accounting. Callers should ensure this flag is only set in controlled, intentional scenarios to avoid unintentionally circumventing budget and rate-limit enforcement on virtual keys.

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
akshaydeo added a commit that referenced this pull request Jun 7, 2026
<Note>
**Hotfix on [v1.5.9](https://docs.getbifrost.ai/changelogs/v1.5.9).**
The key fix corrects wildcard (`*`) allowed-models handling: for
providers whose models the catalog cannot enumerate, a wildcard now
correctly allows any model instead of rejecting it. This affected
keyless self-hosted providers (vLLM/Ollama/SGL) and custom providers
without list-models support.
</Note>

## 🐞 Fixed

- **Virtual Key Usage Tracking Under User Attribution** - Usage is no
longer silently dropped from virtual-key accounting whenever a user is
attributed on a request. Governance now tracks both the virtual-key and
user scopes by default; callers that deliberately want user-only
accounting can opt in with the new
`bifrost-skip-virtual-key-usage-tracking` context flag (#4123)
- **Wildcard Allow-Lists for Catalog-Opaque Providers** - A wildcard
(`*`) allowed-models list now correctly permits any model for providers
whose models the catalog cannot enumerate - custom providers without
list-models support, and keyless self-hosted vLLM/Ollama/SGL - instead
of incorrectly rejecting them (#4124)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: v1.5.9 all model request trigger 403 [Bug]: After upgrading to v1.5.0, <model> is not allowed for this virtual key

2 participants