Merge pull request #2038 from dgageot/board/you-are-the-master-decoupler-you-know-ev-e75a9547

refactor: remove duplication in model resolution, thinking budget, and message construction

Author: dgageot

pkg/config/latest/model_ref.go

@@ -0,0 +1,20 @@
+package latest
+
+import (
+	"fmt"
+	"strings"
+)
+
+// ParseModelRef parses an inline "provider/model" reference into a
+// ModelConfig. It returns an error when the string does not contain
+// exactly one "/" separator or when either part is empty.
+//
+//	cfg, err := ParseModelRef("openai/gpt-4o")
+//	// cfg.Provider == "openai", cfg.Model == "gpt-4o"
+func ParseModelRef(ref string) (ModelConfig, error) {
+	providerName, model, ok := strings.Cut(ref, "/")
+	if !ok || providerName == "" || model == "" {
+		return ModelConfig{}, fmt.Errorf("invalid model reference %q: expected 'provider/model' format", ref)
+	}
+	return ModelConfig{Provider: providerName, Model: model}, nil
+}

pkg/config/latest/types.go

@@ -439,12 +439,12 @@ func (f *FlexibleModelConfig) UnmarshalYAML(unmarshal func(any) error) error {
 	// Try string shorthand first
 	var shorthand string
 	if err := unmarshal(&shorthand); err == nil && shorthand != "" {
-		provider, model, ok := strings.Cut(shorthand, "/")
-		if !ok || provider == "" || model == "" {
+		parsed, parseErr := ParseModelRef(shorthand)
+		if parseErr != nil {
 			return fmt.Errorf("invalid model shorthand %q: expected format 'provider/model'", shorthand)
 		}
-		f.Provider = provider
-		f.Model = model
+		f.Provider = parsed.Provider
+		f.Model = parsed.Model
 		return nil
 	}
 
@@ -707,6 +707,26 @@ func (t ThinkingBudget) MarshalYAML() (any, error) {
 	return t.Tokens, nil
 }
 
+// IsDisabled returns true if the thinking budget is explicitly disabled.
+// A nil receiver is treated as "not configured" (not disabled).
+//
+// Disabled when:
+//   - Tokens == 0 with no Effort (thinking_budget: 0)
+//   - Effort == "none" (thinking_budget: none)
+//
+// NOT disabled when:
+//   - Tokens > 0 or Tokens == -1 (explicit token budget)
+//   - Effort is a real level like "medium" or "high"
+func (t *ThinkingBudget) IsDisabled() bool {
+	if t == nil {
+		return false
+	}
+	if t.Tokens == 0 && t.Effort == "" {
+		return true
+	}
+	return t.Effort == "none"
+}
+
 // MarshalJSON implements custom marshaling to output simple string or int format
 // This ensures JSON and YAML have the same flattened format for consistency
 func (t ThinkingBudget) MarshalJSON() ([]byte, error) {

pkg/config/overrides.go

@@ -186,15 +186,12 @@ func ensureSingleModelExists(cfg *latest.Config, modelName, context string) erro
 		return nil
 	}
 
-	providerName, model, ok := strings.Cut(modelName, "/")
-	if !ok || providerName == "" || model == "" {
+	parsed, err := latest.ParseModelRef(modelName)
+	if err != nil {
 		return fmt.Errorf("%s references non-existent model '%s'", context, modelName)
 	}
 
-	cfg.Models[modelName] = latest.ModelConfig{
-		Provider: providerName,
-		Model:    model,
-	}
+	cfg.Models[modelName] = parsed
 
 	return nil
 }

pkg/evaluation/eval.go

@@ -597,22 +597,17 @@ func createJudgeModel(ctx context.Context, judgeModel string, runConfig *config.
 		return nil, nil
 	}
 
-	providerName, model, ok := strings.Cut(judgeModel, "/")
-	if !ok {
+	cfg, err := latest.ParseModelRef(judgeModel)
+	if err != nil {
 		return nil, fmt.Errorf("invalid judge model format %q: expected 'provider/model'", judgeModel)
 	}
 
-	cfg := &latest.ModelConfig{
-		Provider: providerName,
-		Model:    model,
-	}
-
 	var opts []options.Opt
 	if runConfig.ModelsGateway != "" {
 		opts = append(opts, options.WithGateway(runConfig.ModelsGateway))
 	}
 
-	judge, err := provider.New(ctx, cfg, runConfig.EnvProvider(), opts...)
+	judge, err := provider.New(ctx, &cfg, runConfig.EnvProvider(), opts...)
 	if err != nil {
 		return nil, fmt.Errorf("creating judge model: %w", err)
 	}

pkg/model/provider/override_test.go

@@ -501,7 +501,7 @@ func TestIsThinkingBudgetDisabled(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			assert.Equal(t, tt.expected, isThinkingBudgetDisabled(tt.budget))
+			assert.Equal(t, tt.expected, tt.budget.IsDisabled())
 		})
 	}
 }

pkg/model/provider/provider.go

@@ -205,16 +205,11 @@ func createRuleBasedRouter(ctx context.Context, cfg *latest.ModelConfig, models
 		}
 
 		// Otherwise, treat as an inline model spec (e.g., "openai/gpt-4o")
-		providerName, model, ok := strings.Cut(modelSpec, "/")
-		if !ok {
+		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)
 		}
-
-		inlineCfg := &latest.ModelConfig{
-			Provider: providerName,
-			Model:    model,
-		}
-		p, err := createDirectProvider(ctx, inlineCfg, env, factoryOpts...)
+		p, err := createDirectProvider(ctx, &inlineCfg, env, factoryOpts...)
 		if err != nil {
 			return nil, err
 		}
@@ -394,7 +389,7 @@ func applyOverrides(cfg *latest.ModelConfig, opts *options.ModelOptions) *latest
 	// 1. ThinkingBudget is nil (not configured) - apply defaults to enable thinking
 	// 2. ThinkingBudget is explicitly disabled (Tokens == 0 or Effort == "none") - clear and re-apply defaults
 	// This allows /think to enable thinking with provider defaults even when config had thinking_budget: 0
-	if enhancedCfg.ThinkingBudget == nil || isThinkingBudgetDisabled(enhancedCfg.ThinkingBudget) {
+	if enhancedCfg.ThinkingBudget == nil || enhancedCfg.ThinkingBudget.IsDisabled() {
 		enhancedCfg.ThinkingBudget = nil
 		applyModelDefaults(&enhancedCfg)
 		slog.Debug("Override: thinking enabled - applied default thinking configuration",
@@ -407,22 +402,6 @@ func applyOverrides(cfg *latest.ModelConfig, opts *options.ModelOptions) *latest
 	return &enhancedCfg
 }
 
-// isThinkingBudgetDisabled returns true if the thinking budget is explicitly disabled.
-// NOT disabled when:
-// - Tokens > 0 or Tokens == -1 (explicit token budget)
-// - Effort is set to something other than "none" (e.g., "medium", "high")
-func isThinkingBudgetDisabled(tb *latest.ThinkingBudget) bool {
-	if tb == nil {
-		return false
-	}
-	if tb.Effort == "none" {
-		return true
-	}
-	// Tokens == 0 with no Effort means explicitly disabled (thinking_budget: 0)
-	// Tokens == 0 with Effort set (e.g., "medium") means Effort-based config, not disabled
-	return tb.Tokens == 0 && tb.Effort == ""
-}
-
 // applyModelDefaults applies provider-specific default values for model configuration.
 // These defaults are applied only if the user hasn't explicitly set the values.
 //
@@ -441,7 +420,7 @@ func applyModelDefaults(cfg *latest.ModelConfig) {
 	// If thinking is explicitly disabled (thinking_budget: 0 or thinking_budget: none),
 	// set ThinkingBudget to nil to completely disable thinking.
 	// This ensures no thinking config is sent to the provider.
-	if isThinkingBudgetDisabled(cfg.ThinkingBudget) {
+	if cfg.ThinkingBudget.IsDisabled() {
 		cfg.ThinkingBudget = nil
 		slog.Debug("Thinking explicitly disabled via thinking_budget: 0 or none",
 			"provider", cfg.Provider,

pkg/runtime/model_switcher.go

@@ -88,7 +88,7 @@ func (r *LocalRuntime) SetAgentModel(ctx context.Context, agentName, modelRef st
 		modelConfig.Name = modelRef
 		// Check if this is an alloy model (no provider, comma-separated models)
 		if isAlloyModelConfig(modelConfig) {
-			providers, err := r.createProvidersFromAlloyConfig(ctx, modelConfig)
+			providers, err := r.resolveModelRefs(ctx, modelConfig.Model)
 			if err != nil {
 				return fmt.Errorf("failed to create alloy model from config: %w", err)
 			}
@@ -109,7 +109,7 @@ func (r *LocalRuntime) SetAgentModel(ctx context.Context, agentName, modelRef st
 	// Check if this is an inline alloy spec (comma-separated provider/model specs)
 	// e.g., "openai/gpt-4o,anthropic/claude-sonnet-4-0"
 	if isInlineAlloySpec(modelRef) {
-		providers, err := r.createProvidersFromInlineAlloy(ctx, modelRef)
+		providers, err := r.resolveModelRefs(ctx, modelRef)
 		if err != nil {
 			return fmt.Errorf("failed to create inline alloy model: %w", err)
 		}
@@ -146,17 +146,12 @@ func (r *LocalRuntime) resolveModelRef(ctx context.Context, modelRef string) (pr
 	}
 
 	// Try inline "provider/model" format.
-	providerName, modelName, ok := strings.Cut(modelRef, "/")
-	if !ok || providerName == "" || modelName == "" {
+	inlineCfg, err := latest.ParseModelRef(modelRef)
+	if err != nil {
 		return nil, fmt.Errorf("invalid model reference %q: expected a model name from config or 'provider/model' format", modelRef)
 	}
 
-	inlineCfg := &latest.ModelConfig{
-		Provider: providerName,
-		Model:    modelName,
-	}
-
-	return r.createProviderFromConfig(ctx, inlineCfg)
+	return r.createProviderFromConfig(ctx, &inlineCfg)
 }
 
 // isAlloyModelConfig checks if a model config is an alloy model (multiple models).
@@ -186,92 +181,44 @@ func isInlineAlloySpec(modelRef string) bool {
 	return validParts >= 2
 }
 
-// createProvidersFromInlineAlloy creates providers from an inline alloy spec.
-// An inline alloy is comma-separated provider/model specs like "openai/gpt-4o,anthropic/claude-sonnet-4-0".
-func (r *LocalRuntime) createProvidersFromInlineAlloy(ctx context.Context, modelRef string) ([]provider.Provider, error) {
+// resolveModelRefs resolves a comma-separated list of model references into
+// providers. Each reference is first looked up in the config by name; if not
+// found it is parsed as an inline "provider/model" spec.
+func (r *LocalRuntime) resolveModelRefs(ctx context.Context, commaSeparatedRefs string) ([]provider.Provider, error) {
 	var providers []provider.Provider
 
-	for part := range strings.SplitSeq(modelRef, ",") {
-		part = strings.TrimSpace(part)
-		if part == "" {
+	for ref := range strings.SplitSeq(commaSeparatedRefs, ",") {
+		ref = strings.TrimSpace(ref)
+		if ref == "" {
 			continue
 		}
 
-		// Check if this part exists as a named model in config
-		if modelCfg, exists := r.modelSwitcherCfg.Models[part]; exists {
-			modelCfg.Name = part
+		// Check if this ref exists as a named model in config
+		if modelCfg, exists := r.modelSwitcherCfg.Models[ref]; exists {
+			modelCfg.Name = ref
 			prov, err := r.createProviderFromConfig(ctx, &modelCfg)
 			if err != nil {
-				return nil, fmt.Errorf("failed to create provider for %q: %w", part, err)
+				return nil, fmt.Errorf("failed to create provider for %q: %w", ref, err)
 			}
 			providers = append(providers, prov)
 			continue
 		}
 
 		// Parse as provider/model
-		providerName, modelName, ok := strings.Cut(part, "/")
-		if !ok {
-			return nil, fmt.Errorf("invalid model reference %q in inline alloy: expected 'provider/model' format", part)
-		}
-
-		inlineCfg := &latest.ModelConfig{
-			Provider: providerName,
-			Model:    modelName,
-		}
-		prov, err := r.createProviderFromConfig(ctx, inlineCfg)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create provider for %q: %w", part, err)
-		}
-		providers = append(providers, prov)
-	}
-
-	if len(providers) == 0 {
-		return nil, errors.New("inline alloy spec has no valid models")
-	}
-
-	return providers, nil
-}
-
-// createProvidersFromAlloyConfig creates providers for each model in an alloy configuration.
-func (r *LocalRuntime) createProvidersFromAlloyConfig(ctx context.Context, alloyCfg latest.ModelConfig) ([]provider.Provider, error) {
-	var providers []provider.Provider
-
-	for modelRef := range strings.SplitSeq(alloyCfg.Model, ",") {
-		modelRef = strings.TrimSpace(modelRef)
-		if modelRef == "" {
-			continue
+		inlineCfg, parseErr := latest.ParseModelRef(ref)
+		if parseErr != nil {
+			return nil, fmt.Errorf("invalid model reference %q: expected 'provider/model' format or a named model from config", ref)
 		}
 
-		// Check if this model reference exists in the config
-		if modelCfg, exists := r.modelSwitcherCfg.Models[modelRef]; exists {
-			modelCfg.Name = modelRef
-			prov, err := r.createProviderFromConfig(ctx, &modelCfg)
-			if err != nil {
-				return nil, fmt.Errorf("failed to create provider for %q: %w", modelRef, err)
-			}
-			providers = append(providers, prov)
-			continue
-		}
-
-		// Try parsing as inline spec (provider/model)
-		providerName, modelName, ok := strings.Cut(modelRef, "/")
-		if !ok {
-			return nil, fmt.Errorf("invalid model reference %q in alloy config: expected 'provider/model' format", modelRef)
-		}
-
-		inlineCfg := &latest.ModelConfig{
-			Provider: providerName,
-			Model:    modelName,
-		}
-		prov, err := r.createProviderFromConfig(ctx, inlineCfg)
+		prov, err := r.createProviderFromConfig(ctx, &inlineCfg)
 		if err != nil {
-			return nil, fmt.Errorf("failed to create provider for %q: %w", modelRef, err)
+			return nil, fmt.Errorf("failed to create provider for %q: %w", ref, err)
 		}
 		providers = append(providers, prov)
 	}
 
 	if len(providers) == 0 {
-		return nil, errors.New("alloy model config has no valid models")
+		return nil, errors.New("no valid models found in model reference list")
 	}
 
 	return providers, nil

pkg/runtime/tool_dispatch.go

@@ -424,7 +424,7 @@ func (r *LocalRuntime) runAgentTool(ctx context.Context, handler ToolHandlerFunc
 }
 
 func addAgentMessage(sess *session.Session, a *agent.Agent, msg *chat.Message, events chan Event) {
-	agentMsg := session.NewAgentMessage(a, msg)
+	agentMsg := session.NewAgentMessage(a.Name(), msg)
 	sess.AddMessage(agentMsg)
 	events <- MessageAdded(sess.ID, agentMsg, a.Name())
 }

pkg/session/session.go

@@ -190,9 +190,9 @@ func UserMessage(content string, multiContent ...chat.MessagePart) *Message {
 	}
 }
 
-func NewAgentMessage(a *agent.Agent, message *chat.Message) *Message {
+func NewAgentMessage(agentName string, message *chat.Message) *Message {
 	return &Message{
-		AgentName: a.Name(),
+		AgentName: agentName,
 		Message:   *message,
 	}
 }