Skip to content

refactor(virtualmodels): replace dead Provider decorator with one-method chat executor #477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SantiagoDePolonia merged 1 commit into from
Jul 4, 2026
Merged

refactor(virtualmodels): replace dead Provider decorator with one-method chat executor #477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func New(ctx context.Context, cfg Config) (*App, error) {
refreshInterval := workflowRefreshInterval(appCfg)
var guardrailExecutor guardrails.ChatCompletionExecutor = app.providers.Router
if vm != nil {
guardrailExecutor = virtualmodels.NewProviderWithOptions(app.providers.Router, vm, virtualmodels.Options{})
guardrailExecutor = virtualmodels.NewChatExecutor(app.providers.Router, vm)
}

// Initialize reusable guardrail definitions using shared storage when already available.
Expand Down
232 changes: 8 additions & 224 deletions internal/server/handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,124 +1526,6 @@ func TestChatCompletion_NormalizesSemanticSelectorHints(t *testing.T) {
}
}

func TestChatCompletion_ResolvesQualifiedMaskingAliasBeforeHandlerRouting(t *testing.T) {
catalog := aliasesTestCatalog{
supported: map[string]bool{
"anthropic/claude-opus-4-6": true,
"openai/gpt-5-nano": true,
},
providerTypes: map[string]string{
"anthropic/claude-opus-4-6": "anthropic",
"openai/gpt-5-nano": "openai",
},
models: map[string]core.Model{
"anthropic/claude-opus-4-6": {ID: "claude-opus-4-6", Object: "model"},
"openai/gpt-5-nano": {ID: "gpt-5-nano", Object: "model"},
},
}

service, err := virtualmodels.NewService(newAliasesTestStore(
redirectVM("anthropic/claude-opus-4-6", "gpt-5-nano", "openai", true),
), &catalog, true)
if err != nil {
t.Fatalf("NewService() error = %v", err)
}
if err := service.Refresh(context.Background()); err != nil {
t.Fatalf("Refresh() error = %v", err)
}

inner := &capturingProvider{
mockProvider: mockProvider{
supportedModels: []string{"gpt-5-nano"},
providerTypes: map[string]string{
"openai/gpt-5-nano": "openai",
},
response: &core.ChatResponse{
ID: "chatcmpl_alias_123",
Object: "chat.completion",
Model: "gpt-5-nano",
Provider: "openai",
Choices: []core.Choice{
{
Index: 0,
FinishReason: "stop",
Message: core.ResponseMessage{
Role: "assistant",
Content: "ok",
},
},
},
},
},
}

provider := virtualmodels.NewProviderWithOptions(inner, service, virtualmodels.Options{})

e := echo.New()
handler := NewHandler(provider, nil, nil, nil)

req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil)
req.Header.Set("Content-Type", "application/json")
req.Body = &explodingReadCloser{}

frame := core.NewRequestSnapshot(
http.MethodPost,
"/v1/chat/completions",
nil,
nil,
nil,
"application/json",
[]byte(`{
"model":"anthropic/claude-opus-4-6",
"messages":[{"role":"user","content":"return json"}]
}`),
false,
"",
nil,
)
req = withRequestSnapshotAndPrompt(req, frame)

rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

err = handler.ChatCompletion(c)
if err != nil {
t.Fatalf("handler returned error: %v", err)
}
if rec.Code != http.StatusOK {
t.Fatalf("expected status 200, got %d (%s)", rec.Code, rec.Body.String())
}
if inner.capturedChatReq == nil {
t.Fatal("expected chat request to be captured")
}
if inner.capturedChatReq.Model != "gpt-5-nano" {
t.Fatalf("captured model = %q, want gpt-5-nano", inner.capturedChatReq.Model)
}
if inner.capturedChatReq.Provider != "openai" {
t.Fatalf("captured provider = %q, want openai", inner.capturedChatReq.Provider)
}

workflow := core.GetWorkflow(c.Request().Context())
if workflow == nil {
t.Fatal("expected workflow in context")
}
if workflow.Mode != core.ExecutionModeTranslated {
t.Fatalf("workflow mode = %q, want %q", workflow.Mode, core.ExecutionModeTranslated)
}
if workflow.ProviderType != "openai" {
t.Fatalf("workflow provider type = %q, want openai", workflow.ProviderType)
}
if workflow.Resolution == nil {
t.Fatal("expected workflow resolution")
}
if !workflow.Resolution.AliasApplied {
t.Fatal("expected alias resolution to be marked as applied")
}
if workflow.ResolvedQualifiedModel() != "openai/gpt-5-nano" {
t.Fatalf("workflow resolved model = %q, want openai/gpt-5-nano", workflow.ResolvedQualifiedModel())
}
}

func TestChatCompletion_UsesExplicitAliasResolverWithoutProviderDecorator(t *testing.T) {
catalog := aliasesTestCatalog{
supported: map[string]bool{
Expand Down Expand Up @@ -1751,106 +1633,6 @@ func TestChatCompletion_UsesExplicitAliasResolverWithoutProviderDecorator(t *tes
}
}

func TestChatCompletion_UsesExplicitAliasResolverWithAliasProviderInventoryOnly(t *testing.T) {
catalog := aliasesTestCatalog{
supported: map[string]bool{
"anthropic/claude-opus-4-6": true,
"openai/gpt-5-nano": true,
},
providerTypes: map[string]string{
"anthropic/claude-opus-4-6": "anthropic",
"openai/gpt-5-nano": "openai",
},
models: map[string]core.Model{
"anthropic/claude-opus-4-6": {ID: "claude-opus-4-6", Object: "model"},
"openai/gpt-5-nano": {ID: "gpt-5-nano", Object: "model"},
},
}

service, err := virtualmodels.NewService(newAliasesTestStore(
redirectVM("anthropic/claude-opus-4-6", "gpt-5-nano", "openai", true),
), &catalog, true)
if err != nil {
t.Fatalf("NewService() error = %v", err)
}
if err := service.Refresh(context.Background()); err != nil {
t.Fatalf("Refresh() error = %v", err)
}

inner := &capturingProvider{
mockProvider: mockProvider{
supportedModels: []string{"gpt-5-nano"},
providerTypes: map[string]string{
"openai/gpt-5-nano": "openai",
},
response: &core.ChatResponse{
ID: "chatcmpl_alias_inventory_123",
Object: "chat.completion",
Model: "gpt-5-nano",
Provider: "openai",
Choices: []core.Choice{
{
Index: 0,
FinishReason: "stop",
Message: core.ResponseMessage{
Role: "assistant",
Content: "ok",
},
},
},
},
},
}
provider := virtualmodels.NewProviderWithOptions(inner, service, virtualmodels.Options{
DisableTranslatedRequestProcessing: true,
DisableNativeBatchPreparation: true,
})

e := echo.New()
handler := newHandler(provider, nil, nil, nil, service, nil, nil, nil)

req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", nil)
req.Header.Set("Content-Type", "application/json")
req.Body = &explodingReadCloser{}

frame := core.NewRequestSnapshot(
http.MethodPost,
"/v1/chat/completions",
nil,
nil,
nil,
"application/json",
[]byte(`{
"model":"anthropic/claude-opus-4-6",
"messages":[{"role":"user","content":"return json"}]
}`),
false,
"",
nil,
)
req = withRequestSnapshotAndPrompt(req, frame)

rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

err = handler.ChatCompletion(c)
if err != nil {
t.Fatalf("handler returned error: %v", err)
}
if rec.Code != http.StatusOK {
t.Fatalf("expected status 200, got %d (%s)", rec.Code, rec.Body.String())
}
if inner.capturedChatReq == nil {
t.Fatal("expected chat request to be captured")
}
if inner.capturedChatReq.Model != "gpt-5-nano" {
t.Fatalf("captured model = %q, want gpt-5-nano", inner.capturedChatReq.Model)
}
if inner.capturedChatReq.Provider != "openai" {
t.Fatalf("captured provider = %q, want openai", inner.capturedChatReq.Provider)
}
}

func TestChatCompletion_UsesExplicitTranslatedRequestPatcher(t *testing.T) {
pipeline := guardrails.NewPipeline()
systemPrompt, err := guardrails.NewSystemPromptGuardrail("test", guardrails.SystemPromptInject, "guardrail system")
Expand Down Expand Up @@ -4311,8 +4093,9 @@ func TestBatches_InputFileRewritesAliasesAndPersistsBatchPreparation(t *testing.
},
}

provider := virtualmodels.NewProvider(inner, service)
handler := NewHandler(provider, nil, nil, nil)
handler := NewHandler(inner, nil, nil, nil)
handler.modelResolver = service
handler.batchRequestPreparer = virtualmodels.NewBatchPreparer(inner, service)
Comment on lines +4096 to +4098

@coderabbitai coderabbitai Bot Jul 4, 2026

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.

📐 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.

batchStore := batchstore.NewMemoryStore()
handler.SetBatchStore(batchStore)

Expand Down Expand Up @@ -4479,8 +4262,9 @@ func TestBatches_InputFileRejectsDisabledAlias(t *testing.T) {
},
}

provider := virtualmodels.NewProvider(inner, service)
handler := NewHandler(provider, nil, nil, nil)
handler := NewHandler(inner, nil, nil, nil)
handler.modelResolver = service
handler.batchRequestPreparer = virtualmodels.NewBatchPreparer(inner, service)

e := echo.New()
req := httptest.NewRequest(http.MethodPost, "/v1/batches", strings.NewReader(`{
Expand Down Expand Up @@ -6444,9 +6228,9 @@ func TestGetFileWithoutProviderUsesProviderInventoryWhenAliasMasksModel(t *testi
},
}

provider := virtualmodels.NewProvider(mock, service)
e := echo.New()
handler := NewHandler(provider, nil, nil, nil)
handler := NewHandler(mock, nil, nil, nil)
handler.modelResolver = service

req := httptest.NewRequest(http.MethodGet, "/v1/files/file_ok_1", nil)
rec := httptest.NewRecorder()
Expand Down
70 changes: 1 addition & 69 deletions internal/server/model_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,12 +736,10 @@ func TestModelValidation_EnrichesAuditEntryWithRequestedModelOnResolutionError(t
"openai/gpt-4o": "openai",
},
}
provider := virtualmodels.NewProvider(inner, service)

e := echo.New()
handlerCalled := false

middleware := WorkflowResolution(provider)
middleware := WorkflowResolutionWithResolver(inner, service)
handler := middleware(func(c *echo.Context) error {
handlerCalled = true
return c.String(http.StatusOK, "ok")
Expand Down Expand Up @@ -981,72 +979,6 @@ func BenchmarkSelectorHintsFromJSONGJSON(b *testing.B) {
}
}

func TestModelValidation_ResolvesQualifiedMaskingAliasBeforeProviderParsing(t *testing.T) {
catalog := aliasesTestCatalog{
supported: map[string]bool{
"anthropic/claude-opus-4-6": true,
"openai/gpt-5-nano": true,
},
providerTypes: map[string]string{
"anthropic/claude-opus-4-6": "anthropic",
"openai/gpt-5-nano": "openai",
},
models: map[string]core.Model{
"anthropic/claude-opus-4-6": {ID: "claude-opus-4-6", Object: "model"},
"openai/gpt-5-nano": {ID: "gpt-5-nano", Object: "model"},
},
}

service, err := virtualmodels.NewService(newAliasesTestStore(
redirectVM("anthropic/claude-opus-4-6", "gpt-5-nano", "openai", true),
), &catalog, true)
if err != nil {
t.Fatalf("NewService() error = %v", err)
}
if err := service.Refresh(context.Background()); err != nil {
t.Fatalf("Refresh() error = %v", err)
}

inner := &mockProvider{
supportedModels: []string{"claude-opus-4-6", "gpt-5-nano"},
providerTypes: map[string]string{
"anthropic/claude-opus-4-6": "anthropic",
"openai/gpt-5-nano": "openai",
},
}
provider := virtualmodels.NewProvider(inner, service)

e := echo.New()
var (
capturedProviderType string
capturedWorkflow *core.Workflow
)

middleware := WorkflowResolution(provider)
handler := middleware(func(c *echo.Context) error {
capturedProviderType = GetProviderType(c)
capturedWorkflow = core.GetWorkflow(c.Request().Context())
return c.String(http.StatusOK, "ok")
})

req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions",
strings.NewReader(`{"model":"anthropic/claude-opus-4-6","messages":[{"role":"user","content":"hi"}]}`))
req.Header.Set("Content-Type", "application/json")
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

err = handler(c)
require.NoError(t, err)

assert.Equal(t, http.StatusOK, rec.Code)
assert.Equal(t, "openai", capturedProviderType)
if assert.NotNil(t, capturedWorkflow) && assert.NotNil(t, capturedWorkflow.Resolution) {
assert.True(t, capturedWorkflow.Resolution.AliasApplied)
assert.Equal(t, "anthropic/claude-opus-4-6", capturedWorkflow.RequestedQualifiedModel())
assert.Equal(t, "openai/gpt-5-nano", capturedWorkflow.ResolvedQualifiedModel())
}
}

func TestWorkflowResolutionWithResolver_UsesExplicitAliasResolverWithoutProviderDecorator(t *testing.T) {
catalog := aliasesTestCatalog{
supported: map[string]bool{
Expand Down
Loading