refactor: use resolvedModelConfig() instead of buildModelIndex()

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)
+	populateCandidateProvidersFromNames(cfg, workspace, fallbacks, candidateProviders)
-	populateCandidateProviders(modelIndex, cfg.WorkspacePath(), candidates, 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
+// populateCandidateProvidersFromNames resolves each model name (alias or
-// cfg.ModelList. First entry wins for duplicate keys. Returns nil when the list is empty.
+// "provider/model") via resolvedModelConfig and creates a dedicated LLMProvider
-//
+// for it. This reuses the canonical config resolution path (GetModelConfig) so
-// Uses ExtractProtocol + NormalizeProvider (not cfg.GetModelConfig) because candidates
+// alias handling and load-balancing stay consistent with the rest of the codebase.
-// hold resolved (Provider, Model) pairs that must be matched against the Model field,
+func populateCandidateProvidersFromNames(
-// not the model_name alias.
+	cfg *config.Config,
-func buildModelIndex(cfg *config.Config) map[string]*config.ModelConfig {
+	workspace string,
-	if cfg == nil || len(cfg.ModelList) == 0 {
+	names []string,
-		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,
 	out map[string]providers.LLMProvider,
 ) {
-	for _, c := range candidates {
+	if cfg == nil || len(names) == 0 {
-		key := providers.ModelKey(c.Provider, c.Model)
+		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]
+		p, _, err := providers.CreateProviderFromConfig(mc)
-		if !found {
+		if err != nil {
-			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 {
 			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) != "" {

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
+// TestPopulateCandidateProviders_ResolvesAlias verifies that a model_name
-// model_list entry without a provider prefix (e.g. "gpt-4o") still matches a
+// alias (e.g. "my-gpt") is resolved via GetModelConfig and the provider
-// candidate whose provider is "openai" — the default protocol that
+// is created using the underlying model's config.
-// ExtractProtocol assigns to bare names.
+func TestPopulateCandidateProviders_ResolvesAlias(t *testing.T) {
-func TestRegisterCandidateProviders_MatchesBareModelName(t *testing.T) {
 	workspace := t.TempDir()
 	out := map[string]providers.LLMProvider{}
 
 	cfg := &config.Config{
 		ModelList: []*config.ModelConfig{
-			// Bare name — no "openai/" prefix. ExtractProtocol should
+			{ModelName: "my-gpt", Model: "openai/gpt-4o", APIBase: "https://api.openai.com/v1", Workspace: workspace},
-			// assign "openai" as the default protocol.
-			{Model: "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.
+// "gemini/gemma-3-27b-it") is matched correctly when referenced by model_name.
-func TestRegisterCandidateProviders_MatchesWithProtocolPrefix(t *testing.T) {
+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
+// TestPopulateCandidateProviders_EmptyNamesIsNoop verifies the early-exit
-// path when the candidates slice is empty — no index is built and the map
+// path when the names slice is empty.
-// remains unchanged.
+func TestPopulateCandidateProviders_EmptyNamesIsNoop(t *testing.T) {
-func TestRegisterCandidateProviders_EmptyCandidatesIsNoop(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
+// TestPopulateCandidateProviders_UnmatchedNameIsSkipped verifies that a
-// that when model_list contains two entries with the same normalised
+// name with no matching model_list entry is skipped and does not
-// 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
 // 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.
+	populateCandidateProvidersFromNames(cfg, t.TempDir(), []string{"nonexistent-model"}, out)
-	registerCandidateProviders(cfg, []providers.FallbackCandidate{{Provider: "anthropic", Model: "claude-3-opus"}}, 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"),
 	}