From 6254bd52aca0d28cf7c069c35bffb6bd5864b5e4 Mon Sep 17 00:00:00 2001 From: Vishnuvardhan Reddy Date: Thu, 26 Feb 2026 13:33:30 +0000 Subject: [PATCH 1/2] fix(agent,gateway): don't overwrite ModelName with protocol-stripped modelID Combined with test documentation --- cmd/picoclaw/internal/agent/helpers.go | 9 +- cmd/picoclaw/internal/agent/helpers_test.go | 182 ++++++++++++++++++++ cmd/picoclaw/internal/gateway/helpers.go | 9 +- 3 files changed, 190 insertions(+), 10 deletions(-) create mode 100644 cmd/picoclaw/internal/agent/helpers_test.go diff --git a/cmd/picoclaw/internal/agent/helpers.go b/cmd/picoclaw/internal/agent/helpers.go index 746e9755e..e599da805 100644 --- a/cmd/picoclaw/internal/agent/helpers.go +++ b/cmd/picoclaw/internal/agent/helpers.go @@ -37,15 +37,14 @@ func agentCmd(message, sessionKey, model string, debug bool) error { cfg.Agents.Defaults.ModelName = model } - provider, modelID, err := providers.CreateProvider(cfg) + provider, _, err := providers.CreateProvider(cfg) if err != nil { return fmt.Errorf("error creating provider: %w", err) } - // Use the resolved model ID from provider creation - if modelID != "" { - cfg.Agents.Defaults.ModelName = modelID - } + // Don't overwrite ModelName with modelID - modelID is just the protocol-stripped + // model identifier, but ModelName should remain as the model_list entry name + // for proper fallback resolution. msgBus := bus.NewMessageBus() agentLoop := agent.NewAgentLoop(cfg, msgBus, provider) diff --git a/cmd/picoclaw/internal/agent/helpers_test.go b/cmd/picoclaw/internal/agent/helpers_test.go new file mode 100644 index 000000000..eea376549 --- /dev/null +++ b/cmd/picoclaw/internal/agent/helpers_test.go @@ -0,0 +1,182 @@ +package agent + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestModelNameNotOverwritten_Documentation documents the expected behavior +// after the fix for the ModelName overwrite bug. +// +// BUG DESCRIPTION: +// Previously, after CreateProvider returned, the code would overwrite +// cfg.Agents.Defaults.ModelName with modelID (the second return value). +// +// The modelID is the protocol-stripped model identifier. For example: +// - model_list has: model = "openrouter/free" +// - ExtractProtocol returns: ("openrouter", "free") +// - CreateProvider returns: modelID = "free" +// - BUG: Code set ModelName = "free" +// +// This broke fallback because: +// 1. Agent's Model field became "free" instead of "openrouter-free" +// 2. ParseModelRef("free", "openrouter") created wrong candidate +// 3. Error showed provider=openrouter model=free (lost context) +// +// EXPECTED BEHAVIOR AFTER FIX: +// - ModelName should remain as the model_list entry name +// - For example: ModelName = "openrouter-free" (NOT "free") +// - This ensures fallback candidates resolve correctly +// +// TEST SCENARIO: +// GIVEN config has: +// { +// "agents": { +// "defaults": { +// "model_name": "openrouter-free" +// } +// }, +// "model_list": [ +// { +// "model_name": "openrouter-free", +// "model": "openrouter/free" +// } +// ] +// } +// +// WHEN CreateProvider is called: +// - GetModelConfig("openrouter-free") finds the entry +// - CreateProviderFromConfig gets model = "openrouter/free" +// - ExtractProtocol("openrouter/free") returns ("openrouter", "free") +// - Returns: (provider, "free", nil) +// +// THEN (the fix): +// - ModelName should STILL be "openrouter-free" +// - ModelName is NOT overwritten to "free" +// +// VERIFICATION: +// - Agent instance is created with Model = "openrouter-free" +// - ResolveCandidates looks up "openrouter-free" in model_list +// - Gets full model string "openrouter/free" +// - ParseModelRef("openrouter/free", "") returns (Provider: "openrouter", Model: "free") +// - Candidate has correct provider and model info +func TestModelNameNotOverwritten_Documentation(t *testing.T) { + // This test documents the contract. The actual verification happens + // at the integration level through the agent creation flow. + + testCases := []struct { + name string + modelName string // model_list entry name (e.g., "openrouter-free") + modelString string // model field (e.g., "openrouter/free") + expectedModelID string // ExtractProtocol result (e.g., "free") + expectedFinalName string // Should remain as modelName (NOT modelID) + }{ + { + name: "openrouter-free model", + modelName: "openrouter-free", + modelString: "openrouter/free", + expectedModelID: "free", + expectedFinalName: "openrouter-free", // Should NOT become "free" + }, + { + name: "openrouter nested protocol", + modelName: "openrouter-nested", + modelString: "openrouter/openrouter/free", + expectedModelID: "openrouter/free", + expectedFinalName: "openrouter-nested", // Should NOT become "openrouter/free" + }, + { + name: "anthropic model", + modelName: "claude-sonnet", + modelString: "anthropic/claude-sonnet-4-20250514", + expectedModelID: "claude-sonnet-4-20250514", + expectedFinalName: "claude-sonnet", // Should NOT become "claude-sonnet-4-20250514" + }, + { + name: "openai model", + modelName: "gpt4o", + modelString: "openai/gpt-4o", + expectedModelID: "gpt-4o", + expectedFinalName: "gpt4o", // Should NOT become "gpt-4o" + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Verify the modelID extraction + parts := strings.SplitN(tc.modelString, "/", 2) + if len(parts) == 2 { + modelID := parts[1] + assert.Equal(t, tc.expectedModelID, modelID, + "ExtractProtocol should extract correct modelID") + } + + // The key assertion: ModelName should NOT be overwritten + // This is enforced by NOT having the code: + // if modelID != "" { cfg.Agents.Defaults.ModelName = modelID } + t.Logf("Model '%s' with protocol '%s' should keep name as '%s', NOT overwrite to '%s'", + tc.modelString, parts[0], tc.modelName, tc.expectedModelID) + + assert.NotEqual(t, tc.expectedModelID, tc.expectedFinalName, + "ModelName should NOT be the same as modelID (this is the bug we fixed)") + assert.Equal(t, tc.modelName, tc.expectedFinalName, + "ModelName should remain as the model_list entry name") + }) + } +} + +// TestParseModelRefBehavior documents how ParseModelRef handles model strings +func TestParseModelRefBehavior(t *testing.T) { + testCases := []struct { + name string + modelString string + expectedProvider string + expectedModel string + }{ + { + name: "simple protocol/model", + modelString: "openrouter/free", + expectedProvider: "openrouter", + expectedModel: "free", + }, + { + name: "nested protocol openrouter/openrouter/free", + modelString: "openrouter/openrouter/free", + expectedProvider: "openrouter", + expectedModel: "openrouter/free", // Everything after first / + }, + { + name: "anthropic model", + modelString: "anthropic/claude-sonnet-4-20250514", + expectedProvider: "anthropic", + expectedModel: "claude-sonnet-4-20250514", + }, + { + name: "openai model", + modelString: "openai/gpt-4o", + expectedProvider: "openai", + expectedModel: "gpt-4o", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // ParseModelRef splits on the first / + idx := strings.Index(tc.modelString, "/") + require.Greater(t, idx, 0, "Model string should contain /") + + provider := tc.modelString[:idx] + model := tc.modelString[idx+1:] + + assert.Equal(t, tc.expectedProvider, provider, + "Provider should be everything before first /") + assert.Equal(t, tc.expectedModel, model, + "Model should be everything after first /") + + t.Logf("ModelRef parsed: Provider=%s, Model=%s", provider, model) + }) + } +} diff --git a/cmd/picoclaw/internal/gateway/helpers.go b/cmd/picoclaw/internal/gateway/helpers.go index a06625dc9..257b38f4e 100644 --- a/cmd/picoclaw/internal/gateway/helpers.go +++ b/cmd/picoclaw/internal/gateway/helpers.go @@ -38,15 +38,14 @@ func gatewayCmd(debug bool) error { return fmt.Errorf("error loading config: %w", err) } - provider, modelID, err := providers.CreateProvider(cfg) + provider, _, err := providers.CreateProvider(cfg) if err != nil { return fmt.Errorf("error creating provider: %w", err) } - // Use the resolved model ID from provider creation - if modelID != "" { - cfg.Agents.Defaults.ModelName = modelID - } + // Don't overwrite ModelName with modelID - modelID is just the protocol-stripped + // model identifier, but ModelName should remain as the model_list entry name + // for proper fallback resolution. msgBus := bus.NewMessageBus() agentLoop := agent.NewAgentLoop(cfg, msgBus, provider) From 6e48d3766788f7325fbb93e9eca53694473fa88b Mon Sep 17 00:00:00 2001 From: Vishnuvardhan Reddy Date: Thu, 26 Feb 2026 12:35:54 +0000 Subject: [PATCH 2/2] fix(agent): resolve model names to full model strings before fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, ResolveCandidates was using model_names (like "gemini-flash") directly as model strings, without looking them up in model_list first. This caused issues where: 1. model_name: "gemini-flash" → used as model: "gemini-flash" directly 2. ParseModelRef("gemini-flash", "") → Provider: "", Model: "gemini-flash" 3. Error showed: provider= model=gemini-flash (empty provider!) 4. The actual model string "antigravity/gemini-3-flash" was never used The fix adds two helper functions: 1. resolveModelString(cfg, modelName) - looks up model_name in model_list and returns the full model string (e.g., "antigravity/gemini-3-flash") 2. resolveFallbackModelStrings(cfg, modelNames) - resolves multiple names Now the agent instance creation flow: 1. Gets model_name (e.g., "gemini-flash") from config 2. Looks it up in model_list to get full model (e.g., "antigravity/gemini-3-flash") 3. Passes full model string to ResolveCandidates 4. ParseModelRef correctly parses: Provider="antigravity", Model="gemini-3-flash" 5. Fallback candidates have correct provider and model info This also fixes nested protocol models like "openrouter/openrouter/free": - Provider: "openrouter" - Model: "openrouter/free" (everything after first /) Tests added: - TestResolveModelString: verifies model name lookup - TestResolveFallbackModelStrings: verifies batch lookup - TestNewAgentInstance_ModelResolution: integration test Fixes the issue where model changes weren't being picked up after restart. --- pkg/agent/instance.go | 37 +++++- pkg/agent/instance_resolution_test.go | 158 ++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 2 deletions(-) create mode 100644 pkg/agent/instance_resolution_test.go diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index a6fd365c7..df6b8ba0d 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -88,9 +88,13 @@ func NewAgentInstance( } // Resolve fallback candidates + // First, look up model names in model_list to get full model strings + primaryModelString := resolveModelString(cfg, model) + fallbackModelStrings := resolveFallbackModelStrings(cfg, fallbacks) + modelCfg := providers.ModelConfig{ - Primary: model, - Fallbacks: fallbacks, + Primary: primaryModelString, + Fallbacks: fallbackModelStrings, } candidates := providers.ResolveCandidates(modelCfg, defaults.Provider) @@ -156,3 +160,32 @@ func expandHome(path string) string { } return path } + +// resolveModelString looks up a model name in model_list and returns the full model string. +// If the model name already contains a "/" (like "openrouter/free"), it's returned as-is. +// If the model name is not found in model_list, it's returned as-is (for backward compatibility). +func resolveModelString(cfg *config.Config, modelName string) string { + // If it already looks like a full model string (protocol/model), return it as-is + if strings.Contains(modelName, "/") { + return modelName + } + + // Look up in model_list + modelCfg, err := cfg.GetModelConfig(modelName) + if err != nil { + // Model not found in model_list, return as-is for backward compatibility + return modelName + } + + // Return the full model string (e.g., "antigravity/gemini-3-flash") + return modelCfg.Model +} + +// resolveFallbackModelStrings looks up multiple model names in model_list and returns their full model strings. +func resolveFallbackModelStrings(cfg *config.Config, modelNames []string) []string { + result := make([]string, 0, len(modelNames)) + for _, name := range modelNames { + result = append(result, resolveModelString(cfg, name)) + } + return result +} diff --git a/pkg/agent/instance_resolution_test.go b/pkg/agent/instance_resolution_test.go new file mode 100644 index 000000000..e61ffdc2c --- /dev/null +++ b/pkg/agent/instance_resolution_test.go @@ -0,0 +1,158 @@ +package agent + +import ( + "testing" + + "github.com/sipeed/picoclaw/pkg/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestResolveModelString tests that model names are correctly resolved to full model strings +func TestResolveModelString(t *testing.T) { + tests := []struct { + name string + modelName string + modelListEntry *config.ModelConfig + expectedResult string + }{ + { + name: "model name without slash - looks up in model_list", + modelName: "gemini-flash", + modelListEntry: &config.ModelConfig{Model: "antigravity/gemini-3-flash"}, + expectedResult: "antigravity/gemini-3-flash", + }, + { + name: "model name with slash - returned as-is", + modelName: "openrouter/free", + modelListEntry: nil, + expectedResult: "openrouter/free", + }, + { + name: "model name not in model_list - returned as-is", + modelName: "unknown-model", + modelListEntry: nil, + expectedResult: "unknown-model", + }, + { + name: "nested protocol model", + modelName: "openrouter-nested", + modelListEntry: &config.ModelConfig{Model: "openrouter/openrouter/free"}, + expectedResult: "openrouter/openrouter/free", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &config.Config{} + if tt.modelListEntry != nil { + tt.modelListEntry.ModelName = tt.modelName + cfg.ModelList = []config.ModelConfig{*tt.modelListEntry} + } + + result := resolveModelString(cfg, tt.modelName) + assert.Equal(t, tt.expectedResult, result) + }) + } +} + +// TestResolveFallbackModelStrings tests that multiple model names are resolved correctly +func TestResolveFallbackModelStrings(t *testing.T) { + cfg := &config.Config{ + ModelList: []config.ModelConfig{ + {ModelName: "gemini-flash", Model: "antigravity/gemini-3-flash"}, + {ModelName: "openrouter-free", Model: "openrouter/free"}, + {ModelName: "claude-sonnet", Model: "anthropic/claude-sonnet-4-20250514"}, + }, + } + + modelNames := []string{"gemini-flash", "openrouter-free", "claude-sonnet"} + result := resolveFallbackModelStrings(cfg, modelNames) + + expected := []string{ + "antigravity/gemini-3-flash", + "openrouter/free", + "anthropic/claude-sonnet-4-20250514", + } + + assert.Equal(t, expected, result) +} + +// TestResolveFallbackModelStrings_MixedInput tests resolution with mixed input +func TestResolveFallbackModelStrings_MixedInput(t *testing.T) { + cfg := &config.Config{ + ModelList: []config.ModelConfig{ + {ModelName: "gemini-flash", Model: "antigravity/gemini-3-flash"}, + }, + } + + // Mix of model names and full model strings + modelNames := []string{ + "gemini-flash", // Should resolve to "antigravity/gemini-3-flash" + "openrouter/free", // Already a full string, kept as-is + "anthropic/claude-3-5", // Already a full string, kept as-is + } + result := resolveFallbackModelStrings(cfg, modelNames) + + expected := []string{ + "antigravity/gemini-3-flash", + "openrouter/free", + "anthropic/claude-3-5", + } + + assert.Equal(t, expected, result) +} + +// TestNewAgentInstance_ModelResolution is an integration test that verifies +// the full model resolution flow when creating an agent instance +func TestNewAgentInstance_ModelResolution(t *testing.T) { + cfg := &config.Config{ + ModelList: []config.ModelConfig{ + { + ModelName: "gemini-flash", + Model: "antigravity/gemini-3-flash", + APIKey: "test-key", + }, + { + ModelName: "openrouter-free", + Model: "openrouter/free", + APIKey: "sk-or-test", + APIBase: "https://openrouter.ai/api/v1", + }, + }, + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: t.TempDir(), + ModelName: "gemini-flash", + ModelFallbacks: []string{"openrouter-free"}, + }, + }, + } + + // We don't need an actual provider for this test since we're not calling Chat + // Use the existing mockProvider from mock_provider_test.go + provider := &mockProvider{} + + instance := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, provider) + + require.NotNil(t, instance) + + // Verify the agent's Model field is still the model_name (not overwritten) + assert.Equal(t, "gemini-flash", instance.Model, + "Agent's Model field should remain as the model_list entry name") + + // Verify the Candidates have been resolved to full model strings + require.Len(t, instance.Candidates, 2, "Should have 2 candidates (primary + fallback)") + + // First candidate should be the resolved primary model + assert.Equal(t, "antigravity", instance.Candidates[0].Provider, + "Primary candidate provider should be 'antigravity'") + assert.Equal(t, "gemini-3-flash", instance.Candidates[0].Model, + "Primary candidate model should be 'gemini-3-flash' (not 'antigravity/gemini-3-flash')") + + // Second candidate should be the resolved fallback model + assert.Equal(t, "openrouter", instance.Candidates[1].Provider, + "Fallback candidate provider should be 'openrouter'") + assert.Equal(t, "free", instance.Candidates[1].Model, + "Fallback candidate model should be 'free' (not 'openrouter/free')") +}