refactor(virtualmodels): replace dead Provider decorator with one-method chat executor#477
Merged
SantiagoDePolonia merged 1 commit intoJul 4, 2026
Conversation
…hod chat executor The 572-line redirect-aware Provider decorator mirrored the full ~30-method routable capability surface, but its only production construction was as the guardrail auxiliary-LLM executor (app.go), a single-method seam superseded by SetExecutor during wiring. The main request path resolves redirects through Service seams (gateway/interfaces.go), and native batch preparation through BatchPreparer — never through the decorator. Replace it with ChatExecutor (redirect-rewrite + delegate, the one behavior the bootstrap needs) and keep the shared redirect-rewrite helpers that BatchPreparer uses in redirect_rewrite.go. Delete the decorator-only helpers (resolveRedirectModel, rewriteResponsesRequest, rewriteEmbeddingRequest) and the standing obligation to mirror every new capability method. Tests: decorator-based server tests either had explicit-resolver twins (...WithoutProviderDecorator) and are deleted, or are converted to the production mechanisms (WorkflowResolutionWithResolver, NewBatchPreparer + handler.modelResolver). New unit tests cover ChatExecutor redirect rewrite, concrete-model passthrough, and model-not-found; the rewriteChatRequest helper test moves from provider_test.go. Net: -294 lines, no production behavior change (the executor performs the same rewriteChatRequest the decorator's ChatCompletion did). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR removes the ChangesProvider decorator replacement with ChatExecutor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ChatExecutor
participant Service
participant InnerProvider
Caller->>ChatExecutor: ChatCompletion(req)
ChatExecutor->>Service: ResolveModelForUserPath(selector)
Service-->>ChatExecutor: resolved model/provider
ChatExecutor->>ChatExecutor: validate provider type / support
alt resolution succeeds
ChatExecutor->>InnerProvider: ChatCompletion(rewritten req)
InnerProvider-->>ChatExecutor: ChatResponse
ChatExecutor-->>Caller: ChatResponse
else resolution fails
ChatExecutor-->>Caller: GatewayError (invalid_request/model_not_found)
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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: 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 `@internal/server/handlers_test.go`:
- Around line 4096-4098: The test setup for Handler wiring is duplicated in
multiple batch tests, so extract the repeated
NewHandler/modelResolver/batchRequestPreparer initialization into a small helper
such as newBatchTestHandler(inner *mockProvider, service *virtualmodels.Service)
*Handler. Update the affected tests to call that helper instead of repeating the
same three-line block, using the existing Handler and
virtualmodels.NewBatchPreparer setup to keep the wiring in one place.
🪄 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: 4fe47690-c13e-46e7-950b-0566d20cf9cb
📒 Files selected for processing (7)
internal/app/app.gointernal/server/handlers_test.gointernal/server/model_validation_test.gointernal/virtualmodels/provider.gointernal/virtualmodels/provider_test.gointernal/virtualmodels/redirect_rewrite.gointernal/virtualmodels/redirect_rewrite_test.go
💤 Files with no reviewable changes (2)
- internal/virtualmodels/provider_test.go
- internal/virtualmodels/provider.go
Comment on lines
+4096
to
+4098
| handler := NewHandler(inner, nil, nil, nil) | ||
| handler.modelResolver = service | ||
| handler.batchRequestPreparer = virtualmodels.NewBatchPreparer(inner, service) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Minor duplication in test wiring.
The 3-line NewHandler + modelResolver + batchRequestPreparer wiring block is repeated verbatim in both TestBatches_InputFileRewritesAliasesAndPersistsBatchPreparation and TestBatches_InputFileRejectsDisabledAlias. Consider extracting a small helper (e.g. newBatchTestHandler(inner *mockProvider, service *virtualmodels.Service) *Handler) to avoid repeating this pattern if more call sites are added later.
Also applies to: 4265-4267
🤖 Prompt for 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.
In `@internal/server/handlers_test.go` around lines 4096 - 4098, The test setup
for Handler wiring is duplicated in multiple batch tests, so extract the
repeated NewHandler/modelResolver/batchRequestPreparer initialization into a
small helper such as newBatchTestHandler(inner *mockProvider, service
*virtualmodels.Service) *Handler. Update the affected tests to call that helper
instead of repeating the same three-line block, using the existing Handler and
virtualmodels.NewBatchPreparer setup to keep the wiring in one place.
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
2 participants
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
Next architecture-review item (doc in #472): the 572-line
virtualmodels.Providerdecorator mirrored the full ~30-method routable capability surface, but production only ever constructed it once — as the guardrail auxiliary-LLM executor bootstrap inapp.go, where the consumed interface (guardrails.ChatCompletionExecutor) has exactly one method. The main request path resolves redirects throughServiceseams (gateway/interfaces.go), and native batch preparation throughBatchPreparer— never through the decorator.ChatExecutor(redirect rewrite + delegate) replaces the decorator inapp.go; it performs the identicalrewriteChatRequestthe decorator'sChatCompletiondid, so guardrail aux calls keep resolving virtual models.BatchPrepareruses move toredirect_rewrite.go; decorator-only helpers (resolveRedirectModel,rewriteResponsesRequest,rewriteEmbeddingRequest) are deleted.Test migration
The decorator was load-bearing in six server test harnesses:
...WithoutProviderDecorator) — the decorator variants are deleted, twins keep the coverage.WorkflowResolutionWithResolver(inner, service), andNewBatchPreparer+handler.modelResolver(matching the pattern neighboring batch tests already use).ChatExecutor(redirect rewrite, concrete-model passthrough, model-not-found short-circuit); therewriteChatRequesthelper test moves over fromprovider_test.go.Net −294 lines.
User-visible impact
None. No production behavior change.
Testing
Full
go build ./...andgo test ./...;go test -raceon virtualmodels + server;golangci-lint0 issues; pre-commitmake test-raceon the commit.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests