Merge pull request #2547 from dgageot/board/improving-provider-package-test-coverage-afd6319e

refactor(provider): improve testability and split provider.go

Author: dgageot

pkg/config/gather.go

@@ -106,7 +106,7 @@ func addEnvVarsForModelConfig(model *latest.ModelConfig, customProviders map[str
 				addEnvVarsForCoreProvider(effective, model, requiredEnv)
 			}
 		}
-	} else if alias, exists := provider.Aliases[model.Provider]; exists {
+	} else if alias, exists := provider.LookupAlias(model.Provider); exists {
 		// Check built-in aliases
 		if alias.TokenEnvVar != "" {
 			requiredEnv[alias.TokenEnvVar] = true

pkg/model/provider/aliases.go

@@ -0,0 +1,141 @@
+package provider
+
+import (
+	"iter"
+	"maps"
+	"slices"
+	"strings"
+)
+
+// Alias defines the configuration for a provider alias.
+type Alias struct {
+	APIType     string // The actual API type to use (openai, anthropic, etc.)
+	BaseURL     string // Default base URL for the provider
+	TokenEnvVar string // Environment variable name for the API token
+}
+
+// CoreProviders lists all natively implemented provider types.
+// These are the provider types that have direct implementations (not aliases).
+var CoreProviders = []string{
+	"openai",
+	"anthropic",
+	"google",
+	"dmr",
+	"amazon-bedrock",
+}
+
+// Aliases maps provider names to their corresponding configurations.
+//
+// Most consumers should call [LookupAlias] for a single lookup or [EachAlias]
+// to iterate, both of which keep the rest of the codebase decoupled from this
+// concrete map. Direct mutation of Aliases is not supported.
+var Aliases = map[string]Alias{
+	"requesty": {
+		APIType:     "openai",
+		BaseURL:     "https://router.requesty.ai/v1",
+		TokenEnvVar: "REQUESTY_API_KEY",
+	},
+	"azure": {
+		APIType:     "openai",
+		TokenEnvVar: "AZURE_API_KEY",
+	},
+	"xai": {
+		APIType:     "openai",
+		BaseURL:     "https://api.x.ai/v1",
+		TokenEnvVar: "XAI_API_KEY",
+	},
+	"nebius": {
+		APIType:     "openai",
+		BaseURL:     "https://api.studio.nebius.com/v1",
+		TokenEnvVar: "NEBIUS_API_KEY",
+	},
+	"mistral": {
+		APIType:     "openai",
+		BaseURL:     "https://api.mistral.ai/v1",
+		TokenEnvVar: "MISTRAL_API_KEY",
+	},
+	"ollama": {
+		APIType: "openai",
+		BaseURL: "http://localhost:11434/v1",
+	},
+	"minimax": {
+		APIType:     "openai",
+		BaseURL:     "https://api.minimax.io/v1",
+		TokenEnvVar: "MINIMAX_API_KEY",
+	},
+	"github-copilot": {
+		APIType:     "openai",
+		BaseURL:     "https://api.githubcopilot.com",
+		TokenEnvVar: "GITHUB_TOKEN",
+	},
+}
+
+// LookupAlias returns the Alias registered for the given name (if any).
+// Lookup is case-sensitive; callers that need case-insensitive matching
+// should normalise the name first (e.g. [strings.ToLower]).
+func LookupAlias(name string) (Alias, bool) {
+	alias, ok := Aliases[name]
+	return alias, ok
+}
+
+// EachAlias returns an iterator over every registered (name, Alias) pair.
+// Iteration order is not guaranteed; callers that need a deterministic order
+// should sort by name.
+func EachAlias() iter.Seq2[string, Alias] {
+	return func(yield func(string, Alias) bool) {
+		for name, alias := range Aliases {
+			if !yield(name, alias) {
+				return
+			}
+		}
+	}
+}
+
+// AllProviders returns all known provider names (core providers + aliases),
+// sorted for deterministic output.
+func AllProviders() []string {
+	providers := slices.Concat(CoreProviders, slices.Collect(maps.Keys(Aliases)))
+	slices.Sort(providers)
+	return providers
+}
+
+// IsKnownProvider returns true if the provider name is a core provider or an alias.
+func IsKnownProvider(name string) bool {
+	if slices.Contains(CoreProviders, strings.ToLower(name)) {
+		return true
+	}
+	_, exists := LookupAlias(strings.ToLower(name))
+	return exists
+}
+
+// CatalogProviders returns the list of provider names that should be shown in the model catalog.
+// This includes core providers and aliases that have a defined BaseURL (self-contained endpoints).
+// Aliases without a BaseURL (like azure) require user configuration and are excluded.
+func CatalogProviders() []string {
+	providers := make([]string, 0, len(CoreProviders)+len(Aliases))
+
+	// Add all core providers
+	providers = append(providers, CoreProviders...)
+
+	// Add aliases that have a defined BaseURL (they work out of the box)
+	for name, alias := range EachAlias() {
+		if alias.BaseURL != "" {
+			providers = append(providers, name)
+		}
+	}
+
+	return providers
+}
+
+// IsCatalogProvider returns true if the provider name is valid for the model catalog.
+func IsCatalogProvider(name string) bool {
+	// Check core providers
+	if slices.Contains(CoreProviders, name) {
+		return true
+	}
+	// Check aliases with BaseURL
+	if alias, exists := LookupAlias(name); exists && alias.BaseURL != "" {
+		return true
+	}
+	return false
+}

pkg/model/provider/aliases_test.go

@@ -0,0 +1,54 @@
+package provider
+
+import (
+	"maps"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLookupAlias(t *testing.T) {
+	t.Parallel()
+
+	// Every entry in the table is reachable.
+	for name, expected := range Aliases {
+		got, ok := LookupAlias(name)
+		assert.True(t, ok, "alias %q should be found", name)
+		assert.Equal(t, expected, got)
+	}
+
+	// Unknown name yields the zero Alias and false.
+	got, ok := LookupAlias("does-not-exist")
+	assert.False(t, ok)
+	assert.Equal(t, Alias{}, got)
+
+	// Lookup is case-sensitive (callers normalise themselves).
+	if _, ok := LookupAlias("MISTRAL"); ok {
+		t.Errorf("LookupAlias should be case-sensitive")
+	}
+}
+
+func TestEachAlias(t *testing.T) {
+	t.Parallel()
+
+	// Iterator yields every entry exactly once.
+	collected := maps.Collect(EachAlias())
+	assert.Equal(t, Aliases, collected)
+}
+
+func TestEachAlias_EarlyTermination(t *testing.T) {
+	t.Parallel()
+
+	// Iterator must respect a false return from the yield function.
+	require.NotEmpty(t, Aliases, "test requires the alias table to be non-empty")
+
+	count := 0
+	for range EachAlias() {
+		count++
+		if count == 1 {
+			break
+		}
+	}
+	assert.Equal(t, 1, count, "iteration should stop when consumer breaks out")
+}

pkg/model/provider/clone.go

@@ -4,41 +4,57 @@ import (
 	"context"
 	"log/slog"
 
+	"github.com/docker/docker-agent/pkg/config/latest"
+	"github.com/docker/docker-agent/pkg/model/provider/base"
 	"github.com/docker/docker-agent/pkg/model/provider/options"
 )
 
 // CloneWithOptions returns a new Provider instance using the same provider/model
 // as the base provider, applying the provided options. If cloning fails, the
 // original base provider is returned.
-func CloneWithOptions(ctx context.Context, base Provider, opts ...options.Opt) Provider {
-	config := base.BaseConfig()
+func CloneWithOptions(ctx context.Context, baseProvider Provider, opts ...options.Opt) Provider {
+	cfg := baseProvider.BaseConfig()
+	modelConfig, mergedOpts := mergeCloneOptions(cfg, opts)
 
+	// Use NewWithModels to support cloning routers that reference other models.
+	// cfg.Models is populated by routers; for other providers it's nil (which is fine).
+	clone, err := NewWithModels(ctx, &modelConfig, cfg.Models, cfg.Env, mergedOpts...)
+	if err != nil {
+		slog.Debug("Failed to clone provider; using base provider", "error", err, "id", baseProvider.ID())
+		return baseProvider
+	}
+
+	return clone
+}
+
+// mergeCloneOptions is the pure half of CloneWithOptions. Given the base
+// provider's configuration and the user-supplied overrides, it returns:
+//
+//   - a copy of the base ModelConfig with explicit overrides applied (currently
+//     MaxTokens and the no-thinking flag), and
+//   - the full ordered slice of options that should be passed to NewWithModels
+//     (existing options first, then user overrides; later opts win).
+//
+// Splitting this out from the impure NewWithModels call lets us table-test the
+// option-merging logic without spinning up an HTTP server.
+func mergeCloneOptions(cfg base.Config, opts []options.Opt) (latest.ModelConfig, []options.Opt) {
 	// Preserve existing options, then apply overrides. Later opts take precedence.
-	baseOpts := options.FromModelOptions(config.ModelOptions)
+	baseOpts := options.FromModelOptions(cfg.ModelOptions)
 	mergedOpts := append(baseOpts, opts...)
 
-	// Apply max_tokens override if present in options
-	// We need to apply it to the ModelConfig itself since that's what providers use
-	// Only update MaxTokens if an option explicitly sets it (non-zero value)
-	modelConfig := config.ModelConfig
+	// Apply every option to a single accumulator so we can read the final
+	// effective values directly. "Later opt wins" semantics fall out naturally.
+	var merged options.ModelOptions
 	for _, opt := range mergedOpts {
-		tempOpts := &options.ModelOptions{}
-		opt(tempOpts)
-		if mt := tempOpts.MaxTokens(); mt != 0 {
-			modelConfig.MaxTokens = &mt
-		}
-		if tempOpts.NoThinking() {
-			modelConfig.ThinkingBudget = nil
-		}
+		opt(&merged)
 	}
 
-	// Use NewWithModels to support cloning routers that reference other models.
-	// config.Models is populated by routers; for other providers it's nil (which is fine).
-	clone, err := NewWithModels(ctx, &modelConfig, config.Models, config.Env, mergedOpts...)
-	if err != nil {
-		slog.Debug("Failed to clone provider; using base provider", "error", err, "id", base.ID())
-		return base
+	modelConfig := cfg.ModelConfig
+	if mt := merged.MaxTokens(); mt != 0 {
+		modelConfig.MaxTokens = &mt
 	}
-
-	return clone
+	if merged.NoThinking() {
+		modelConfig.ThinkingBudget = nil
+	}
+	return modelConfig, mergedOpts
 }