From 6254bd52aca0d28cf7c069c35bffb6bd5864b5e4 Mon Sep 17 00:00:00 2001 From: Vishnuvardhan Reddy Date: Thu, 26 Feb 2026 13:33:30 +0000 Subject: [PATCH] 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)