refactor(provider): extract router factory closure to resolveRoutedModel

createRuleBasedRouter inlined a 25-line closure that resolves routing targets — either by looking up a name in the models map or by parsing it as an inline 'provider/model' spec. Because the closure was anonymous and only invoked through rulebased.NewClient (which wraps every error), four error paths were 0% covered:

  - the recursion-prevention branch (target itself has routing rules),

  - the ParseModelRef failure branch,

  - the named-reference factory error path,

  - the inline-spec factory error path.

Promote the closure to a package-level function resolveRoutedModel. Tests can now call it directly with a swapped factory registry to verify all four error paths and that factoryOpts (e.g. WithMaxTokens) are forwarded unchanged.

Behaviour is unchanged. createRuleBasedRouter and resolveRoutedModel both reach 100% coverage; package coverage 91.8% -> 93.9%.

Assisted-By: docker-agent

Author: dgageot

pkg/model/provider/provider.go

@@ -195,34 +195,39 @@ func NewWithModels(ctx context.Context, cfg *latest.ModelConfig, models map[stri
 
 // createRuleBasedRouter creates a rule-based routing provider.
 func createRuleBasedRouter(ctx context.Context, cfg *latest.ModelConfig, models map[string]latest.ModelConfig, env environment.Provider, opts ...options.Opt) (Provider, error) {
-	// Create a provider factory that can resolve model references
-	factory := func(ctx context.Context, modelSpec string, models map[string]latest.ModelConfig, env environment.Provider, factoryOpts ...options.Opt) (rulebased.Provider, error) {
-		// Check if modelSpec is a reference to a model in the models map
-		if modelCfg, exists := models[modelSpec]; exists {
-			// Prevent infinite recursion - referenced models cannot have routing rules
-			if len(modelCfg.Routing) > 0 {
-				return nil, fmt.Errorf("model %q has routing rules and cannot be used as a routing target", modelSpec)
-			}
-			p, err := createDirectProvider(ctx, &modelCfg, env, factoryOpts...)
-			if err != nil {
-				return nil, err
-			}
-			return p, nil
-		}
+	return rulebased.NewClient(ctx, cfg, models, env, resolveRoutedModel, opts...)
+}
 
-		// Otherwise, treat as an inline model spec (e.g., "openai/gpt-4o")
-		inlineCfg, parseErr := latest.ParseModelRef(modelSpec)
-		if parseErr != nil {
-			return nil, fmt.Errorf("invalid model spec %q: expected 'provider/model' format or a model reference", modelSpec)
-		}
-		p, err := createDirectProvider(ctx, &inlineCfg, env, factoryOpts...)
-		if err != nil {
-			return nil, err
+// resolveRoutedModel is the rulebased.ProviderFactory used by
+// createRuleBasedRouter. It resolves a routing target — which is either a name
+// from the models map or an inline "provider/model" spec — and returns the
+// provider for it. Routing targets cannot themselves have routing rules.
+//
+// Defined as a package-level function (rather than an inline closure) so the
+// recursion-prevention, parse-error and factory-error paths can be unit-tested
+// directly without going through rulebased.NewClient.
+func resolveRoutedModel(
+	ctx context.Context,
+	modelSpec string,
+	models map[string]latest.ModelConfig,
+	env environment.Provider,
+	factoryOpts ...options.Opt,
+) (rulebased.Provider, error) {
+	// Check if modelSpec is a reference to a model in the models map.
+	if modelCfg, exists := models[modelSpec]; exists {
+		// Prevent infinite recursion - referenced models cannot have routing rules.
+		if len(modelCfg.Routing) > 0 {
+			return nil, fmt.Errorf("model %q has routing rules and cannot be used as a routing target", modelSpec)
 		}
-		return p, nil
+		return createDirectProvider(ctx, &modelCfg, env, factoryOpts...)
 	}
 
-	return rulebased.NewClient(ctx, cfg, models, env, factory, opts...)
+	// Otherwise, treat as an inline model spec (e.g., "openai/gpt-4o").
+	inlineCfg, parseErr := latest.ParseModelRef(modelSpec)
+	if parseErr != nil {
+		return nil, fmt.Errorf("invalid model spec %q: expected 'provider/model' format or a model reference", modelSpec)
+	}
+	return createDirectProvider(ctx, &inlineCfg, env, factoryOpts...)
 }
 
 // createDirectProvider creates a provider without routing (direct model access).

pkg/model/provider/router_factory_test.go

@@ -0,0 +1,151 @@
+package provider
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/docker/docker-agent/pkg/config/latest"
+	"github.com/docker/docker-agent/pkg/environment"
+	"github.com/docker/docker-agent/pkg/model/provider/options"
+)
+
+// TestResolveRoutedModel_NamedReference verifies that a model name in the
+// models map is resolved to its config and dispatched through the registry.
+func TestResolveRoutedModel_NamedReference(t *testing.T) {
+	var capturedCfg *latest.ModelConfig
+	withFactories(t, map[string]providerFactory{
+		"openai": func(_ context.Context, cfg *latest.ModelConfig, _ environment.Provider, _ ...options.Opt) (Provider, error) {
+			capturedCfg = cfg
+			return &fakeProvider{id: "captured"}, nil
+		},
+	})
+
+	models := map[string]latest.ModelConfig{
+		"fast": {Provider: "openai", Model: "gpt-4o-mini"},
+	}
+
+	p, err := resolveRoutedModel(t.Context(), "fast", models, environment.NewNoEnvProvider())
+	require.NoError(t, err)
+	require.NotNil(t, p)
+
+	require.NotNil(t, capturedCfg)
+	assert.Equal(t, "openai", capturedCfg.Provider)
+	assert.Equal(t, "gpt-4o-mini", capturedCfg.Model)
+}
+
+// TestResolveRoutedModel_InlineSpec verifies that a "provider/model" string
+// not present in the models map is parsed as an inline reference.
+func TestResolveRoutedModel_InlineSpec(t *testing.T) {
+	var capturedCfg *latest.ModelConfig
+	withFactories(t, map[string]providerFactory{
+		"openai": func(_ context.Context, cfg *latest.ModelConfig, _ environment.Provider, _ ...options.Opt) (Provider, error) {
+			capturedCfg = cfg
+			return &fakeProvider{id: "captured"}, nil
+		},
+	})
+
+	p, err := resolveRoutedModel(t.Context(), "openai/gpt-4o", nil, environment.NewNoEnvProvider())
+	require.NoError(t, err)
+	require.NotNil(t, p)
+
+	require.NotNil(t, capturedCfg)
+	assert.Equal(t, "openai", capturedCfg.Provider)
+	assert.Equal(t, "gpt-4o", capturedCfg.Model)
+}
+
+// TestResolveRoutedModel_InvalidInlineSpec covers the previously-unreachable
+// ParseModelRef error branch (modelSpec not in map and not parseable).
+func TestResolveRoutedModel_InvalidInlineSpec(t *testing.T) {
+	withFactories(t, map[string]providerFactory{
+		"openai": tagFactory("openai"),
+	})
+
+	_, err := resolveRoutedModel(t.Context(), "no-slash-here", nil, environment.NewNoEnvProvider())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "invalid model spec")
+	assert.Contains(t, err.Error(), "no-slash-here")
+}
+
+// TestResolveRoutedModel_RecursiveRoutingRejected covers the recursion-prevention
+// branch: a routing target may not itself have routing rules.
+func TestResolveRoutedModel_RecursiveRoutingRejected(t *testing.T) {
+	withFactories(t, map[string]providerFactory{
+		"openai": tagFactory("openai"),
+	})
+
+	models := map[string]latest.ModelConfig{
+		"router-as-target": {
+			Provider: "openai",
+			Model:    "gpt-4o",
+			Routing:  []latest.RoutingRule{{Model: "x", Examples: []string{"hi"}}},
+		},
+	}
+
+	_, err := resolveRoutedModel(t.Context(), "router-as-target", models, environment.NewNoEnvProvider())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "routing rules")
+	assert.Contains(t, err.Error(), "router-as-target")
+}
+
+// TestResolveRoutedModel_FactoryErrorPropagated_NamedRef verifies that when a
+// named model's factory fails, the error is returned to the caller.
+func TestResolveRoutedModel_FactoryErrorPropagated_NamedRef(t *testing.T) {
+	sentinel := errors.New("named-fail")
+	withFactories(t, map[string]providerFactory{
+		"openai": func(_ context.Context, _ *latest.ModelConfig, _ environment.Provider, _ ...options.Opt) (Provider, error) {
+			return nil, sentinel
+		},
+	})
+
+	models := map[string]latest.ModelConfig{
+		"fast": {Provider: "openai", Model: "gpt-4o-mini"},
+	}
+
+	_, err := resolveRoutedModel(t.Context(), "fast", models, environment.NewNoEnvProvider())
+	require.ErrorIs(t, err, sentinel)
+}
+
+// TestResolveRoutedModel_FactoryErrorPropagated_Inline verifies the same for
+// an inline "provider/model" spec.
+func TestResolveRoutedModel_FactoryErrorPropagated_Inline(t *testing.T) {
+	sentinel := errors.New("inline-fail")
+	withFactories(t, map[string]providerFactory{
+		"openai": func(_ context.Context, _ *latest.ModelConfig, _ environment.Provider, _ ...options.Opt) (Provider, error) {
+			return nil, sentinel
+		},
+	})
+
+	_, err := resolveRoutedModel(t.Context(), "openai/gpt-4o", nil, environment.NewNoEnvProvider())
+	require.ErrorIs(t, err, sentinel)
+}
+
+// TestResolveRoutedModel_OptionsForwarded verifies that factoryOpts reach the
+// downstream factory unchanged. This guards against accidentally dropping
+// options (e.g. WithProviders) when extracting the closure.
+func TestResolveRoutedModel_OptionsForwarded(t *testing.T) {
+	var capturedOpts []options.Opt
+	withFactories(t, map[string]providerFactory{
+		"openai": func(_ context.Context, _ *latest.ModelConfig, _ environment.Provider, opts ...options.Opt) (Provider, error) {
+			capturedOpts = opts
+			return &fakeProvider{id: "ok"}, nil
+		},
+	})
+
+	maxTokens := int64(2048)
+	_, err := resolveRoutedModel(
+		t.Context(), "openai/gpt-4o", nil, environment.NewNoEnvProvider(),
+		options.WithMaxTokens(maxTokens),
+	)
+	require.NoError(t, err)
+
+	require.NotEmpty(t, capturedOpts)
+	var probe options.ModelOptions
+	for _, o := range capturedOpts {
+		o(&probe)
+	}
+	assert.Equal(t, maxTokens, probe.MaxTokens())
+}