refactor(provider): split CloneWithOptions into a pure mergeCloneOptions helper

CloneWithOptions mixed two responsibilities: (1) merge base options with caller-supplied overrides into a (ModelConfig, []options.Opt) pair, and (2) actually call NewWithModels and gracefully fall back if it errors. The pure half was untestable in isolation, and two branches were uncovered:

  - the WithNoThinking() override that nils out ThinkingBudget,

  - the failure-fallback that returns the base provider unchanged.

Extract the merging logic into a pure mergeCloneOptions(base.Config, []Opt) function. Tests can now table-drive every override path (no-overrides, MaxTokens override, NoThinking override, base-options preservation, later-wins precedence) without standing up an httptest.Server, and the fallback path is exercised with a fakeProvider whose zero-valued Config naturally fails the registry lookup.

Behaviour is unchanged. CloneWithOptions and mergeCloneOptions both reach 100% coverage; package coverage 93.9% -> 95.8%.

Assisted-By: docker-agent

Author: dgageot

pkg/model/provider/clone.go

@@ -4,23 +4,48 @@ 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
+	modelConfig := cfg.ModelConfig
 	for _, opt := range mergedOpts {
 		tempOpts := &options.ModelOptions{}
 		opt(tempOpts)
@@ -32,13 +57,5 @@ func CloneWithOptions(ctx context.Context, base Provider, opts ...options.Opt) P
 		}
 	}
 
-	// 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
-	}
-
-	return clone
+	return modelConfig, mergedOpts
 }

pkg/model/provider/clone_merge_test.go

@@ -0,0 +1,128 @@
+package provider
+
+import (
+	"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/model/provider/base"
+	"github.com/docker/docker-agent/pkg/model/provider/options"
+)
+
+func TestMergeCloneOptions_NoOverrides(t *testing.T) {
+	t.Parallel()
+
+	originalMax := int64(8192)
+	cfg := base.Config{
+		ModelConfig: latest.ModelConfig{
+			Provider:  "openai",
+			Model:     "gpt-4o",
+			MaxTokens: &originalMax,
+			ThinkingBudget: &latest.ThinkingBudget{
+				Effort: "high",
+			},
+		},
+	}
+
+	got, mergedOpts := mergeCloneOptions(cfg, nil)
+
+	require.NotNil(t, got.MaxTokens)
+	assert.Equal(t, originalMax, *got.MaxTokens, "MaxTokens should be untouched when no overrides")
+	require.NotNil(t, got.ThinkingBudget)
+	assert.Equal(t, "high", got.ThinkingBudget.Effort, "ThinkingBudget should be preserved")
+	assert.Empty(t, mergedOpts, "no base opts and no overrides -> empty merged slice")
+}
+
+func TestMergeCloneOptions_MaxTokensOverride(t *testing.T) {
+	t.Parallel()
+
+	originalMax := int64(8192)
+	cfg := base.Config{
+		ModelConfig: latest.ModelConfig{MaxTokens: &originalMax},
+	}
+
+	newMax := int64(2048)
+	got, mergedOpts := mergeCloneOptions(cfg, []options.Opt{options.WithMaxTokens(newMax)})
+
+	require.NotNil(t, got.MaxTokens)
+	assert.Equal(t, newMax, *got.MaxTokens, "MaxTokens should follow explicit override")
+	assert.Len(t, mergedOpts, 1, "user-supplied option should appear in mergedOpts")
+}
+
+// TestMergeCloneOptions_NoThinking covers the previously-uncovered branch in
+// the option-merge loop that nils out ThinkingBudget when WithNoThinking is set.
+func TestMergeCloneOptions_NoThinking(t *testing.T) {
+	t.Parallel()
+
+	cfg := base.Config{
+		ModelConfig: latest.ModelConfig{
+			Provider:       "openai",
+			Model:          "o3-mini",
+			ThinkingBudget: &latest.ThinkingBudget{Effort: "medium"},
+		},
+	}
+
+	got, _ := mergeCloneOptions(cfg, []options.Opt{options.WithNoThinking()})
+
+	assert.Nil(t, got.ThinkingBudget, "WithNoThinking must clear ThinkingBudget")
+}
+
+func TestMergeCloneOptions_PreservesBaseOptions(t *testing.T) {
+	t.Parallel()
+
+	// ModelOptions on the base config should be reconstituted as Opts and
+	// included in the merged slice ahead of user-supplied overrides.
+	var baseOpts options.ModelOptions
+	options.WithGateway("https://gw.example.com")(&baseOpts)
+	options.WithGeneratingTitle()(&baseOpts)
+
+	cfg := base.Config{
+		ModelOptions: baseOpts,
+		ModelConfig:  latest.ModelConfig{Provider: "openai", Model: "gpt-4o"},
+	}
+
+	_, mergedOpts := mergeCloneOptions(cfg, []options.Opt{options.WithMaxTokens(1024)})
+
+	// Replay the merged opts and check all three flags survived.
+	var probe options.ModelOptions
+	for _, opt := range mergedOpts {
+		opt(&probe)
+	}
+	assert.Equal(t, "https://gw.example.com", probe.Gateway())
+	assert.True(t, probe.GeneratingTitle())
+	assert.Equal(t, int64(1024), probe.MaxTokens())
+}
+
+// TestMergeCloneOptions_LaterOverridesWin covers the documented "later opts
+// take precedence" contract: a user-supplied MaxTokens overrides the base one.
+func TestMergeCloneOptions_LaterOverridesWin(t *testing.T) {
+	t.Parallel()
+
+	var baseOpts options.ModelOptions
+	options.WithMaxTokens(int64(512))(&baseOpts)
+
+	cfg := base.Config{
+		ModelOptions: baseOpts,
+		ModelConfig:  latest.ModelConfig{Provider: "openai", Model: "gpt-4o"},
+	}
+
+	got, _ := mergeCloneOptions(cfg, []options.Opt{options.WithMaxTokens(int64(4096))})
+
+	require.NotNil(t, got.MaxTokens)
+	assert.Equal(t, int64(4096), *got.MaxTokens, "later opt must override earlier opt")
+}
+
+// TestCloneWithOptions_FallbackOnError verifies the previously-uncovered
+// fallback path: when NewWithModels fails, CloneWithOptions returns the
+// original base provider unchanged.
+func TestCloneWithOptions_FallbackOnError(t *testing.T) {
+	// fakeProvider returns a zero-valued base.Config, so its Provider type is
+	// empty; that always fails the factory-registry lookup in createDirectProvider.
+	original := &fakeProvider{id: "original"}
+
+	got := CloneWithOptions(t.Context(), original, options.WithMaxTokens(int64(2048)))
+
+	assert.Same(t, original, got, "should fall back to base provider when cloning fails")
+}