diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index ec59a8e19..562692e97 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -175,8 +175,7 @@ func NewAgentInstance( candidates := resolveModelCandidates(cfg, defaults.Provider, model, fallbacks) candidateProviders := make(map[string]providers.LLMProvider) - modelIndex := buildModelIndex(cfg) - populateCandidateProviders(modelIndex, cfg.WorkspacePath(), candidates, candidateProviders) + populateCandidateProvidersFromNames(cfg, workspace, fallbacks, candidateProviders) // Model routing setup: pre-resolve light model candidates at creation time // to avoid repeated model_list lookups on every incoming message. @@ -202,7 +201,7 @@ func NewAgentInstance( }) lightCandidates = resolved lightProvider = lp - populateCandidateProviders(modelIndex, cfg.WorkspacePath(), resolved, candidateProviders) + populateCandidateProvidersFromNames(cfg, workspace, []string{rc.LightModel}, candidateProviders) } } } else { @@ -238,81 +237,42 @@ func NewAgentInstance( } } -// buildModelIndex returns a normalised "provider/model" → *ModelConfig lookup for -// cfg.ModelList. First entry wins for duplicate keys. Returns nil when the list is empty. -// -// Uses ExtractProtocol + NormalizeProvider (not cfg.GetModelConfig) because candidates -// hold resolved (Provider, Model) pairs that must be matched against the Model field, -// not the model_name alias. -func buildModelIndex(cfg *config.Config) map[string]*config.ModelConfig { - if cfg == nil || len(cfg.ModelList) == 0 { - return nil - } - index := make(map[string]*config.ModelConfig, len(cfg.ModelList)) - for _, mc := range cfg.ModelList { - entry := strings.TrimSpace(mc.Model) - if entry == "" { - continue - } - protocol, modelID := providers.ExtractProtocol(entry) - k := providers.ModelKey(providers.NormalizeProvider(protocol), modelID) - if _, exists := index[k]; !exists { - index[k] = mc - } - } - return index -} - -// populateCandidateProviders creates an LLMProvider for each candidate using the -// pre-built model index and stores it in out. Candidates absent from the index are -// skipped with a warning; they inherit the primary provider's credentials at runtime. -func populateCandidateProviders( - index map[string]*config.ModelConfig, - workspacePath string, - candidates []providers.FallbackCandidate, +// populateCandidateProvidersFromNames resolves each model name (alias or +// "provider/model") via resolvedModelConfig and creates a dedicated LLMProvider +// for it. This reuses the canonical config resolution path (GetModelConfig) so +// alias handling and load-balancing stay consistent with the rest of the codebase. +func populateCandidateProvidersFromNames( + cfg *config.Config, + workspace string, + names []string, out map[string]providers.LLMProvider, ) { - for _, c := range candidates { - key := providers.ModelKey(c.Provider, c.Model) + if cfg == nil || len(names) == 0 { + return + } + for _, name := range names { + mc, err := resolvedModelConfig(cfg, strings.TrimSpace(name), workspace) + if err != nil { + logger.WarnCF("agent", + "fallback provider: no model_list entry found; will inherit primary provider credentials", + map[string]any{"name": name, "error": err.Error()}) + continue + } + protocol, modelID := providers.ExtractProtocol(strings.TrimSpace(mc.Model)) + key := providers.ModelKey(providers.NormalizeProvider(protocol), modelID) if _, exists := out[key]; exists { continue } - mc, found := index[key] - if !found { - logger.WarnCF( - "agent", - "fallback provider: no model_list entry found; will inherit primary provider credentials", - map[string]any{"provider": c.Provider, "model": c.Model}, - ) - continue - } - mCopy := *mc - if mCopy.Workspace == "" { - mCopy.Workspace = workspacePath - } - p, _, err := providers.CreateProviderFromConfig(&mCopy) - if err == nil { - out[key] = p - } else { + p, _, err := providers.CreateProviderFromConfig(mc) + if err != nil { logger.WarnCF("agent", "fallback provider: failed to create provider", map[string]any{"model": mc.Model, "error": err.Error()}) + continue } + out[key] = p } } -// registerCandidateProviders is the test-facing entry point that wraps -// buildModelIndex + populateCandidateProviders in a single call. -func registerCandidateProviders( - cfg *config.Config, - candidates []providers.FallbackCandidate, - out map[string]providers.LLMProvider, -) { - if cfg == nil || len(cfg.ModelList) == 0 || len(candidates) == 0 { - return - } - populateCandidateProviders(buildModelIndex(cfg), cfg.WorkspacePath(), candidates, out) -} - // resolveAgentWorkspace determines the workspace directory for an agent. func resolveAgentWorkspace(agentCfg *config.AgentConfig, defaults *config.AgentDefaults) string { if agentCfg != nil && strings.TrimSpace(agentCfg.Workspace) != "" { diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index 78da6ed19..f03c66c5e 100644 --- a/pkg/agent/instance_test.go +++ b/pkg/agent/instance_test.go @@ -249,149 +249,120 @@ func TestNewAgentInstance_AllowsMediaTempDirForReadListAndExec(t *testing.T) { } } -// TestRegisterCandidateProviders_NilCfgIsNoop verifies that passing a nil +// TestPopulateCandidateProviders_NilCfgIsNoop verifies that passing a nil // config does not panic and leaves the output map empty. -func TestRegisterCandidateProviders_NilCfgIsNoop(t *testing.T) { +func TestPopulateCandidateProviders_NilCfgIsNoop(t *testing.T) { out := map[string]providers.LLMProvider{} - registerCandidateProviders(nil, []providers.FallbackCandidate{{Provider: "openai", Model: "gpt-4o"}}, out) + populateCandidateProvidersFromNames(nil, t.TempDir(), []string{"gpt-4o"}, out) if len(out) != 0 { t.Fatalf("expected empty map, got %d entries", len(out)) } } -// TestRegisterCandidateProviders_SkipsExistingKeys verifies that a key already +// TestPopulateCandidateProviders_SkipsExistingKeys verifies that a key already // present in the output map is not overwritten. -func TestRegisterCandidateProviders_SkipsExistingKeys(t *testing.T) { +func TestPopulateCandidateProviders_SkipsExistingKeys(t *testing.T) { existing := &mockProvider{} key := providers.ModelKey("openai", "gpt-4o") out := map[string]providers.LLMProvider{key: existing} cfg := &config.Config{ ModelList: []*config.ModelConfig{ - {Model: "openai/gpt-4o", APIKeys: config.SimpleSecureStrings("test-key")}, + {ModelName: "my-gpt", Model: "openai/gpt-4o", APIKeys: config.SimpleSecureStrings("test-key")}, }, } - registerCandidateProviders(cfg, []providers.FallbackCandidate{{Provider: "openai", Model: "gpt-4o"}}, out) + populateCandidateProvidersFromNames(cfg, t.TempDir(), []string{"my-gpt"}, out) if out[key] != existing { t.Fatal("existing provider entry was overwritten; expected it to be preserved") } } -// TestRegisterCandidateProviders_MatchesBareModelName verifies that a -// model_list entry without a provider prefix (e.g. "gpt-4o") still matches a -// candidate whose provider is "openai" — the default protocol that -// ExtractProtocol assigns to bare names. -func TestRegisterCandidateProviders_MatchesBareModelName(t *testing.T) { +// TestPopulateCandidateProviders_ResolvesAlias verifies that a model_name +// alias (e.g. "my-gpt") is resolved via GetModelConfig and the provider +// is created using the underlying model's config. +func TestPopulateCandidateProviders_ResolvesAlias(t *testing.T) { workspace := t.TempDir() out := map[string]providers.LLMProvider{} cfg := &config.Config{ ModelList: []*config.ModelConfig{ - // Bare name — no "openai/" prefix. ExtractProtocol should - // assign "openai" as the default protocol. - {Model: "gpt-4o", APIBase: "https://api.openai.com/v1", Workspace: workspace}, + {ModelName: "my-gpt", Model: "openai/gpt-4o", APIBase: "https://api.openai.com/v1", Workspace: workspace}, }, } - registerCandidateProviders(cfg, []providers.FallbackCandidate{{Provider: "openai", Model: "gpt-4o"}}, out) + populateCandidateProvidersFromNames(cfg, workspace, []string{"my-gpt"}, out) key := providers.ModelKey("openai", "gpt-4o") if out[key] == nil { - t.Fatalf("expected CandidateProviders[%q] to be populated for bare model name", key) + t.Fatalf("expected CandidateProviders[%q] to be populated for alias", key) } } -// TestRegisterCandidateProviders_MatchesWithProtocolPrefix verifies that a +// TestPopulateCandidateProviders_ResolvesProtocolPrefix verifies that a // model_list entry using full "provider/model" notation (e.g. -// "gemini/gemma-3-27b-it") is matched correctly. -func TestRegisterCandidateProviders_MatchesWithProtocolPrefix(t *testing.T) { +// "gemini/gemma-3-27b-it") is matched correctly when referenced by model_name. +func TestPopulateCandidateProviders_ResolvesProtocolPrefix(t *testing.T) { workspace := t.TempDir() out := map[string]providers.LLMProvider{} cfg := &config.Config{ ModelList: []*config.ModelConfig{ { + ModelName: "gemma", Model: "gemini/gemma-3-27b-it", APIKeys: config.SimpleSecureStrings("gemini-test-key"), Workspace: workspace, }, }, } - registerCandidateProviders(cfg, []providers.FallbackCandidate{{Provider: "gemini", Model: "gemma-3-27b-it"}}, out) + populateCandidateProvidersFromNames(cfg, workspace, []string{"gemma"}, out) key := providers.ModelKey("gemini", "gemma-3-27b-it") if out[key] == nil { - t.Fatalf("expected CandidateProviders[%q] to be populated for protocol-prefixed model name", key) + t.Fatalf("expected CandidateProviders[%q] to be populated for protocol-prefixed model", key) } } -// TestRegisterCandidateProviders_EmptyCandidatesIsNoop verifies the early-exit -// path when the candidates slice is empty — no index is built and the map -// remains unchanged. -func TestRegisterCandidateProviders_EmptyCandidatesIsNoop(t *testing.T) { +// TestPopulateCandidateProviders_EmptyNamesIsNoop verifies the early-exit +// path when the names slice is empty. +func TestPopulateCandidateProviders_EmptyNamesIsNoop(t *testing.T) { out := map[string]providers.LLMProvider{} cfg := &config.Config{ ModelList: []*config.ModelConfig{ - {Model: "openai/gpt-4o", APIKeys: config.SimpleSecureStrings("key")}, + {ModelName: "my-gpt", Model: "openai/gpt-4o", APIKeys: config.SimpleSecureStrings("key")}, }, } - registerCandidateProviders(cfg, nil, out) + populateCandidateProvidersFromNames(cfg, t.TempDir(), nil, out) if len(out) != 0 { t.Fatalf("expected empty map, got %d entries", len(out)) } } -// TestRegisterCandidateProviders_EmptyModelListIsNoop verifies the early-exit +// TestPopulateCandidateProviders_EmptyModelListIsNoop verifies the early-exit // path when model_list is empty — no provider can be created. -func TestRegisterCandidateProviders_EmptyModelListIsNoop(t *testing.T) { +func TestPopulateCandidateProviders_EmptyModelListIsNoop(t *testing.T) { out := map[string]providers.LLMProvider{} cfg := &config.Config{} - registerCandidateProviders(cfg, []providers.FallbackCandidate{{Provider: "openai", Model: "gpt-4o"}}, out) + populateCandidateProvidersFromNames(cfg, t.TempDir(), []string{"gpt-4o"}, out) if len(out) != 0 { t.Fatalf("expected empty map, got %d entries", len(out)) } } -// TestRegisterCandidateProviders_FirstModelListEntryWinsForDuplicates verifies -// that when model_list contains two entries with the same normalised -// provider/model key, the first one is used (mirrors model_list precedence). -func TestRegisterCandidateProviders_FirstModelListEntryWinsForDuplicates(t *testing.T) { - workspace := t.TempDir() - out := map[string]providers.LLMProvider{} - - cfg := &config.Config{ - ModelList: []*config.ModelConfig{ - {Model: "openai/gpt-4o", APIBase: "https://first.example.com/v1", Workspace: workspace}, - {Model: "openai/gpt-4o", APIBase: "https://second.example.com/v1", Workspace: workspace}, - }, - } - registerCandidateProviders(cfg, []providers.FallbackCandidate{{Provider: "openai", Model: "gpt-4o"}}, out) - - key := providers.ModelKey("openai", "gpt-4o") - if out[key] == nil { - t.Fatalf("expected CandidateProviders[%q] to be populated", key) - } - // Only one entry should be registered despite two model_list entries. - if len(out) != 1 { - t.Fatalf("expected 1 entry, got %d", len(out)) - } -} - -// TestRegisterCandidateProviders_UnmatchedCandidateIsSkipped verifies that a -// candidate with no matching model_list entry is silently skipped and does not +// TestPopulateCandidateProviders_UnmatchedNameIsSkipped verifies that a +// name with no matching model_list entry is skipped and does not // cause a panic or leave a nil entry in the map. -func TestRegisterCandidateProviders_UnmatchedCandidateIsSkipped(t *testing.T) { +func TestPopulateCandidateProviders_UnmatchedNameIsSkipped(t *testing.T) { out := map[string]providers.LLMProvider{} cfg := &config.Config{ ModelList: []*config.ModelConfig{ - {Model: "openai/gpt-4o", APIKeys: config.SimpleSecureStrings("key")}, + {ModelName: "my-gpt", Model: "openai/gpt-4o", APIKeys: config.SimpleSecureStrings("key")}, }, } - // "anthropic/claude-3-opus" has no matching model_list entry. - registerCandidateProviders(cfg, []providers.FallbackCandidate{{Provider: "anthropic", Model: "claude-3-opus"}}, out) + populateCandidateProvidersFromNames(cfg, t.TempDir(), []string{"nonexistent-model"}, out) if len(out) != 0 { - t.Fatalf("expected empty map for unmatched candidate, got %d entries", len(out)) + t.Fatalf("expected empty map for unmatched name, got %d entries", len(out)) } } @@ -436,8 +407,8 @@ func TestNewAgentInstance_CandidateProvidersPopulatedForCrossProviderFallbacks(t primaryProvider := &mockProvider{} agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, primaryProvider) + // Only fallback models need entries — the primary uses the injected provider directly. wantKeys := []string{ - providers.ModelKey("openrouter", "mistralai/mistral-small-3.1-24b-instruct:free"), providers.ModelKey("gemini", "gemma-3-27b-it"), providers.ModelKey("gemini", "gemini-2.5-flash-lite"), }