Skip to content

chore: remove applyListModelsVirtualKeyProviderFilter VK provider pre-filter from listModels - #4074

Merged
Pratham-Mishra04 merged 1 commit into
devfrom
06-05-feat_make_use_of_http_pre-hook_instead_of_handler_for_model_filter
Jun 5, 2026
Merged

chore: remove applyListModelsVirtualKeyProviderFilter VK provider pre-filter from listModels#4074
Pratham-Mishra04 merged 1 commit into
devfrom
06-05-feat_make_use_of_http_pre-hook_instead_of_handler_for_model_filter

Conversation

@BearTS

@BearTS BearTS commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Provider scoping for GET /v1/models with a virtual key was previously handled inside the HTTP transport layer via applyListModelsVirtualKeyProviderFilter in handlers/list_models_vk.go. This approach required direct config store access from the handler and produced noisy, expected errors in request logs whenever governance rejected providers not permitted by the virtual key. This PR removes that transport-layer implementation entirely, eliminating the filter call from handlers/inference.go and deleting the associated file and tests.

Changes

  • Removed the applyListModelsVirtualKeyProviderFilter call from handlers/inference.go.
  • Deleted transports/bifrost-http/handlers/list_models_vk.go, which contained the transport-layer VK provider scoping logic for list models.
  • Deleted transports/bifrost-http/handlers/list_models_vk_test.go and all associated unit tests.

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 ./transports/bifrost-http/handlers/...
go test ./...

Breaking changes

  • Yes
  • No

Security considerations

No changes to authentication or authorization logic. Virtual key resolution and provider scoping responsibility has been removed from the transport layer; ensure the receiving layer correctly handles provider filtering to avoid unintended access.

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

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR relocates virtual-key–based provider filtering from HTTP handler utilities into the governance plugin's request pre-hook. The filter now runs earlier, scoping /v1/models provider fan-out to a virtual key's configured providers, with new governance-layer tests and removal of the now-redundant handler-level guard logic.

Changes

Virtual-key provider filtering relocation to governance

Layer / File(s) Summary
Governance utility functions and prehook integration
plugins/governance/utils.go, plugins/governance/main.go
Adds isListModelsRequest() and applyListModelsVirtualKeyProviderFilter() helpers to detect list-models requests and scope provider fan-out via virtual-key configs, then integrates the filter into HTTPTransportPreHook to apply it for bodyless requests before other governance rules.
Governance-layer tests for virtual-key filtering
plugins/governance/httptransportprehook_test.go
Adds two test cases: one verifies an active virtual key scopes BifrostContextKeyAvailableProviders to configured providers; the other verifies an inactive virtual key does not set scoping to avoid implicit deny.
Handler-level cleanup and removal of early-return guard
transports/bifrost-http/handlers/inference.go
Removes the early-return conditional that previously invoked the handler-level applyListModelsVirtualKeyProviderFilter in listModels, since virtual-key filtering now occurs in the governance prehook.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • maximhq/bifrost#3924: Both PRs implement virtual-key–scoped provider filtering via schemas.BifrostContextKeyAvailableProviders: the main PR sets that context for bodyless list models, while the retrieved PR makes routing/fallback parsing honor/intersect it—so they're tightly connected in the same provider-selection flow.

  • maximhq/bifrost#3796: Both PRs implement the same virtual-key-based scoping of BifrostContextKeyAvailableProviders for GET /v1/models: the retrieved PR adds/apply applyListModelsVirtualKeyProviderFilter in the HTTP handlers, and the main PR moves that logic into the governance prehook while removing the handler-side filter and its related early-return behavior—so the changes overlap directly at the same scoping points.

Suggested reviewers

  • danpiths
  • akshaydeo

Poem

🐰 From handlers to hooks, the filter now hops,
Virtual keys scoped where governance stops,
Early-return guards fade away with grace,
Provider filtering finds its rightful place! 🔑

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ⚠️ Warning The PR title describes only a removal operation (chore: remove...) but the actual primary change is a refactor that moves functionality from HTTP handler into governance plugin. Update title to reflect the main architectural change, e.g., 'refactor: move list-models VK provider scoping from HTTP handler to governance pre-hook' to better represent the full scope of work.
Linked Issues check ❓ Inconclusive The linked issue #2516 concerns /genai routing-rule target loss before governance load balancing. This PR moves VK provider scoping into governance pre-hook for list models, which is tangential to #2516 but not directly addressing the /genai target preservation problem. Clarify whether this refactoring addresses or contributes to fixing #2516's /genai routing-rule target issue, or if it is a separate governance improvement. The PR closes #2516 but the actual code changes do not appear to implement any of the three stated fix options for the /genai problem.
✅ Passed checks (2 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to moving VK provider scoping logic from HTTP handlers into the governance plugin; no unrelated modifications to other areas are present.
Description check ✅ Passed The PR description is well-structured and covers all major template sections including summary, changes, type of change, affected areas, testing instructions, and security considerations.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 06-05-feat_make_use_of_http_pre-hook_instead_of_handler_for_model_filter

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

BearTS commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

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

@BearTS BearTS changed the title feat: make use of http pre-hook instead of handler for model filter refactor: move list-models VK provider scoping from HTTP handler into governance plugin pre-hook Jun 5, 2026
@BearTS

BearTS commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@BearTS
BearTS marked this pull request as ready for review June 5, 2026 06:28
@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 3/5

Not safe to merge in its current state — the handler-layer VK pre-filter was deleted but the governance plugin replacement was never committed, leaving list-models fan-out unscoped for virtual keys.

The only behavioral change in this diff is deletion: the VK provider-scoping logic that populated BifrostContextKeyAvailableProviders before ListAllModels is gone, and nothing in the governance plugin's HTTPTransportPreHook fills that gap for bodyless requests. Every GET /v1/models call made with a virtual key will now query every configured provider, and governance will reject the non-VK providers, producing exactly the noisy log errors the PR set out to eliminate.

transports/bifrost-http/handlers/inference.go — the removed pre-filter call is the center of the regression; plugins/governance/main.go — the HTTPTransportPreHook needs the bodyless list-models path added before this deletion can land safely.

Important Files Changed

Filename Overview
transports/bifrost-http/handlers/inference.go Removes the VK pre-filter call for list-models without a working replacement in the governance pre-hook, leaving GET /v1/models requests to fan out to all providers regardless of virtual key restrictions.
transports/bifrost-http/handlers/list_models_vk.go Deleted handler-layer VK provider scoping implementation; deletion is intentional as part of the refactor, but the governance plugin replacement logic described in the PR was not committed.
transports/bifrost-http/handlers/list_models_vk_test.go Deleted tests for the removed handler; test coverage for the replacement pre-hook behavior (list-models + VK with no body) was not added to the governance test suite.

Reviews (3): Last reviewed commit: "feat: make use of http pre-hook instead ..." | Re-trigger Greptile

Comment thread plugins/governance/httptransportprehook_test.go
@BearTS
BearTS force-pushed the 06-05-feat_make_use_of_http_pre-hook_instead_of_handler_for_model_filter branch 3 times, most recently from da8a906 to d2f170f Compare June 5, 2026 07:39
@BearTS
BearTS force-pushed the 06-05-feat_make_use_of_http_pre-hook_instead_of_handler_for_model_filter branch from d2f170f to 78b230d Compare June 5, 2026 07:40
Comment thread transports/bifrost-http/handlers/inference.go
@BearTS BearTS changed the title refactor: move list-models VK provider scoping from HTTP handler into governance plugin pre-hook chore: remove applyListModelsVirtualKeyProviderFilter VK provider pre-filter from listModels Jun 5, 2026

Pratham-Mishra04 commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Merge activity

  • Jun 5, 1:41 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 5, 1:41 PM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

@Pratham-Mishra04
Pratham-Mishra04 merged commit 8924180 into dev Jun 5, 2026
16 checks passed
@Pratham-Mishra04
Pratham-Mishra04 deleted the 06-05-feat_make_use_of_http_pre-hook_instead_of_handler_for_model_filter branch June 5, 2026 13:41
akshaydeo pushed a commit that referenced this pull request Jun 7, 2026
…re-filter from `listModels` (#4074)

## Summary

Provider scoping for `GET /v1/models` with a virtual key was previously handled inside the HTTP transport layer via `applyListModelsVirtualKeyProviderFilter` in `handlers/list_models_vk.go`. This approach required direct config store access from the handler and produced noisy, expected errors in request logs whenever governance rejected providers not permitted by the virtual key. This PR removes that transport-layer implementation entirely, eliminating the filter call from `handlers/inference.go` and deleting the associated file and tests.

## Changes

- Removed the `applyListModelsVirtualKeyProviderFilter` call from `handlers/inference.go`.
- Deleted `transports/bifrost-http/handlers/list_models_vk.go`, which contained the transport-layer VK provider scoping logic for list models.
- Deleted `transports/bifrost-http/handlers/list_models_vk_test.go` and all associated unit tests.

## Type of change

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

## Affected areas

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

## How to test

```sh
go test ./transports/bifrost-http/handlers/...
go test ./...
```

## Breaking changes

- [ ] Yes
- [x] No

## Security considerations

No changes to authentication or authorization logic. Virtual key resolution and provider scoping responsibility has been removed from the transport layer; ensure the receiving layer correctly handles provider filtering to avoid unintended access.

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
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.

2 participants