From a849e02917e6c3c6a52a0e2c27e19552cc91f88d Mon Sep 17 00:00:00 2001 From: Lixeer <1612655510@qq.com> Date: Sun, 22 Feb 2026 22:30:53 +0800 Subject: [PATCH 01/11] fix: better session management for `github_copilot_provider` --- cmd/picoclaw/cmd_gateway.go | 3 + pkg/providers/github_copilot_provider.go | 74 ++++++++++++++++-------- pkg/providers/types.go | 5 ++ 3 files changed, 59 insertions(+), 23 deletions(-) diff --git a/cmd/picoclaw/cmd_gateway.go b/cmd/picoclaw/cmd_gateway.go index 28ef76ad3..30d61aec3 100644 --- a/cmd/picoclaw/cmd_gateway.go +++ b/cmd/picoclaw/cmd_gateway.go @@ -212,6 +212,9 @@ func gatewayCmd() { fmt.Println("\nShutting down...") cancel() + if cp, ok := provider.(providers.SessionProvider); ok { + cp.Close() + } healthServer.Stop(context.Background()) deviceService.Stop() heartbeatService.Stop() diff --git a/pkg/providers/github_copilot_provider.go b/pkg/providers/github_copilot_provider.go index 6124881f7..8131b76fc 100644 --- a/pkg/providers/github_copilot_provider.go +++ b/pkg/providers/github_copilot_provider.go @@ -4,60 +4,75 @@ import ( "context" "encoding/json" "fmt" + "sync" copilot "github.com/github/copilot-sdk/go" ) type GitHubCopilotProvider struct { uri string - connectMode string // `stdio` or `grpc`` + connectMode string // "stdio" or "grpc" + client *copilot.Client session *copilot.Session + + mu sync.Mutex } func NewGitHubCopilotProvider(uri string, connectMode string, model string) (*GitHubCopilotProvider, error) { - var session *copilot.Session if connectMode == "" { connectMode = "grpc" } - switch connectMode { + switch connectMode { case "stdio": - // todo + // TODO: + return nil, fmt.Errorf("stdio mode not implemented") case "grpc": client := copilot.NewClient(&copilot.ClientOptions{ CLIUrl: uri, }) if err := client.Start(context.Background()); err != nil { - return nil, fmt.Errorf( - "Can't connect to Github Copilot, https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md#connecting-to-an-external-cli-server for details", - ) + return nil, fmt.Errorf("can't connect to Github Copilot: %w; `https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md#connecting-to-an-external-cli-server` for details", err) } - defer client.Stop() - session, _ = client.CreateSession(context.Background(), &copilot.SessionConfig{ + + session, err := client.CreateSession(context.Background(), &copilot.SessionConfig{ Model: model, Hooks: &copilot.SessionHooks{}, }) + if err != nil { + client.Stop() + return nil, fmt.Errorf("create session failed: %w", err) + } + + return &GitHubCopilotProvider{ + uri: uri, + connectMode: connectMode, + client: client, + session: session, + }, nil + default: + return nil, fmt.Errorf("unknown connect mode: %s", connectMode) } - - return &GitHubCopilotProvider{ - uri: uri, - connectMode: connectMode, - session: session, - }, nil } -// Chat sends a chat request to GitHub Copilot -func (p *GitHubCopilotProvider) Chat( - ctx context.Context, messages []Message, tools []ToolDefinition, model string, options map[string]any, -) (*LLMResponse, error) { +func (p *GitHubCopilotProvider) Close() { + p.mu.Lock() + defer p.mu.Unlock() + if p.client != nil { + p.client.Stop() + p.client = nil + p.session = nil + } +} + +func (p *GitHubCopilotProvider) Chat(ctx context.Context, messages []Message, tools []ToolDefinition, model string, options map[string]interface{}) (*LLMResponse, error) { type tempMessage struct { Role string `json:"role"` Content string `json:"content"` } out := make([]tempMessage, 0, len(messages)) - for _, msg := range messages { out = append(out, tempMessage{ Role: msg.Role, @@ -65,18 +80,31 @@ func (p *GitHubCopilotProvider) Chat( }) } - fullcontent, _ := json.Marshal(out) + fullcontent, err := json.Marshal(out) + if err != nil { + return nil, fmt.Errorf("marshal messages: %w", err) + } + p.mu.Lock() + defer p.mu.Unlock() - content, _ := p.session.Send(ctx, copilot.MessageOptions{ + resp, err := p.session.SendAndWait(ctx, copilot.MessageOptions{ Prompt: string(fullcontent), }) + if err != nil { + return nil, err + } + + var content string + if resp != nil && resp.Data.Content != nil { + content = *resp.Data.Content + } return &LLMResponse{ FinishReason: "stop", Content: content, }, nil } - func (p *GitHubCopilotProvider) GetDefaultModel() string { + return "gpt-4.1" } diff --git a/pkg/providers/types.go b/pkg/providers/types.go index f711e7803..40ff6f7c8 100644 --- a/pkg/providers/types.go +++ b/pkg/providers/types.go @@ -30,6 +30,11 @@ type LLMProvider interface { GetDefaultModel() string } +type SessionProvider interface { + LLMProvider + Close() +} + // FailoverReason classifies why an LLM request failed for fallback decisions. type FailoverReason string From 3d605a4f537507713706aa626b74ad8506e6e12a Mon Sep 17 00:00:00 2001 From: Lixeer <1612655510@qq.com> Date: Sun, 22 Feb 2026 23:02:29 +0800 Subject: [PATCH 02/11] fix: run fmt and lint --- pkg/providers/github_copilot_provider.go | 15 ++++++++++++--- pkg/tools/registry_test.go | 20 ++++++++++---------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/pkg/providers/github_copilot_provider.go b/pkg/providers/github_copilot_provider.go index 8131b76fc..c69658b44 100644 --- a/pkg/providers/github_copilot_provider.go +++ b/pkg/providers/github_copilot_provider.go @@ -33,7 +33,10 @@ func NewGitHubCopilotProvider(uri string, connectMode string, model string) (*Gi CLIUrl: uri, }) if err := client.Start(context.Background()); err != nil { - return nil, fmt.Errorf("can't connect to Github Copilot: %w; `https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md#connecting-to-an-external-cli-server` for details", err) + return nil, fmt.Errorf( + "can't connect to Github Copilot: %w; `https://github.com/github/copilot-sdk/blob/main/docs/getting-started.md#connecting-to-an-external-cli-server` for details", + err, + ) } session, err := client.CreateSession(context.Background(), &copilot.SessionConfig{ @@ -67,7 +70,13 @@ func (p *GitHubCopilotProvider) Close() { } } -func (p *GitHubCopilotProvider) Chat(ctx context.Context, messages []Message, tools []ToolDefinition, model string, options map[string]interface{}) (*LLMResponse, error) { +func (p *GitHubCopilotProvider) Chat( + ctx context.Context, + messages []Message, + tools []ToolDefinition, + model string, + options map[string]any, +) (*LLMResponse, error) { type tempMessage struct { Role string `json:"role"` Content string `json:"content"` @@ -104,7 +113,7 @@ func (p *GitHubCopilotProvider) Chat(ctx context.Context, messages []Message, to Content: content, }, nil } -func (p *GitHubCopilotProvider) GetDefaultModel() string { +func (p *GitHubCopilotProvider) GetDefaultModel() string { return "gpt-4.1" } diff --git a/pkg/tools/registry_test.go b/pkg/tools/registry_test.go index 33978e543..8ae13b20c 100644 --- a/pkg/tools/registry_test.go +++ b/pkg/tools/registry_test.go @@ -14,14 +14,14 @@ import ( type mockRegistryTool struct { name string desc string - params map[string]interface{} + params map[string]any result *ToolResult } -func (m *mockRegistryTool) Name() string { return m.name } -func (m *mockRegistryTool) Description() string { return m.desc } -func (m *mockRegistryTool) Parameters() map[string]interface{} { return m.params } -func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]interface{}) *ToolResult { +func (m *mockRegistryTool) Name() string { return m.name } +func (m *mockRegistryTool) Description() string { return m.desc } +func (m *mockRegistryTool) Parameters() map[string]any { return m.params } +func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]any) *ToolResult { return m.result } @@ -51,7 +51,7 @@ func newMockTool(name, desc string) *mockRegistryTool { return &mockRegistryTool{ name: name, desc: desc, - params: map[string]interface{}{"type": "object"}, + params: map[string]any{"type": "object"}, result: SilentResult("ok"), } } @@ -109,7 +109,7 @@ func TestToolRegistry_Execute_Success(t *testing.T) { r.Register(&mockRegistryTool{ name: "greet", desc: "says hello", - params: map[string]interface{}{}, + params: map[string]any{}, result: SilentResult("hello"), }) @@ -203,7 +203,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { if defs[0]["type"] != "function" { t.Errorf("expected type 'function', got %v", defs[0]["type"]) } - fn, ok := defs[0]["function"].(map[string]interface{}) + fn, ok := defs[0]["function"].(map[string]any) if !ok { t.Fatal("expected 'function' key to be a map") } @@ -217,7 +217,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { func TestToolRegistry_ToProviderDefs(t *testing.T) { r := NewToolRegistry() - params := map[string]interface{}{"type": "object", "properties": map[string]interface{}{}} + params := map[string]any{"type": "object", "properties": map[string]any{}} r.Register(&mockRegistryTool{ name: "beta", desc: "tool B", @@ -310,7 +310,7 @@ func TestToolToSchema(t *testing.T) { if schema["type"] != "function" { t.Errorf("expected type 'function', got %v", schema["type"]) } - fn, ok := schema["function"].(map[string]interface{}) + fn, ok := schema["function"].(map[string]any) if !ok { t.Fatal("expected 'function' to be a map") } From 712f5a8300f0ce9d9d22352a1bc92bed3a0c8b88 Mon Sep 17 00:00:00 2001 From: yinwm Date: Mon, 23 Feb 2026 16:55:06 +0800 Subject: [PATCH 03/11] refactor(config): rename model field to model_name The configuration field for specifying the model has been renamed from "model" to "model_name" for better clarity and consistency with the model_list configuration. A GetModelName() accessor method has been added to maintain backward compatibility. Existing configurations using the old "model" field will continue to work correctly. This change affects: - Configuration structure (AgentDefaults struct) - All references across the codebase - Documentation in all language variants - Example configuration files --- README.fr.md | 2 +- README.ja.md | 2 +- README.md | 2 +- README.pt-br.md | 2 +- README.vi.md | 2 +- README.zh.md | 2 +- cmd/picoclaw/cmd_agent.go | 4 +- cmd/picoclaw/cmd_auth.go | 10 +-- cmd/picoclaw/cmd_gateway.go | 2 +- cmd/picoclaw/cmd_status.go | 2 +- config/config.example.json | 2 +- pkg/agent/instance.go | 2 +- pkg/channels/telegram_commands.go | 4 +- pkg/config/config.go | 12 ++- pkg/config/migration.go | 2 +- pkg/config/model_config_test.go | 132 ++++++++++++++++++++++++++++++ pkg/migrate/config.go | 5 +- pkg/providers/factory.go | 2 +- pkg/providers/legacy_provider.go | 2 +- 19 files changed, 169 insertions(+), 24 deletions(-) diff --git a/README.fr.md b/README.fr.md index 7199f7098..04f40e022 100644 --- a/README.fr.md +++ b/README.fr.md @@ -222,7 +222,7 @@ picoclaw onboard ], "agents": { "defaults": { - "model": "gpt4" + "model_name": "gpt4" } }, "channels": { diff --git a/README.ja.md b/README.ja.md index bb0bdfb28..b379bc2a7 100644 --- a/README.ja.md +++ b/README.ja.md @@ -184,7 +184,7 @@ picoclaw onboard ], "agents": { "defaults": { - "model": "gpt4" + "model_name": "gpt4" } }, "channels": { diff --git a/README.md b/README.md index 7bc7b1089..058e16a2b 100644 --- a/README.md +++ b/README.md @@ -209,7 +209,7 @@ picoclaw onboard "agents": { "defaults": { "workspace": "~/.picoclaw/workspace", - "model": "gpt4", + "model_name": "gpt4", "max_tokens": 8192, "temperature": 0.7, "max_tool_iterations": 20 diff --git a/README.pt-br.md b/README.pt-br.md index ec8fe8e1c..cfa5b801b 100644 --- a/README.pt-br.md +++ b/README.pt-br.md @@ -223,7 +223,7 @@ picoclaw onboard ], "agents": { "defaults": { - "model": "gpt4" + "model_name": "gpt4" } }, "tools": { diff --git a/README.vi.md b/README.vi.md index 161842933..1d0084aa3 100644 --- a/README.vi.md +++ b/README.vi.md @@ -203,7 +203,7 @@ picoclaw onboard ], "agents": { "defaults": { - "model": "gpt4" + "model_name": "gpt4" } }, "channels": { diff --git a/README.zh.md b/README.zh.md index 4d739c5eb..dca149fa9 100644 --- a/README.zh.md +++ b/README.zh.md @@ -221,7 +221,7 @@ picoclaw onboard "agents": { "defaults": { "workspace": "~/.picoclaw/workspace", - "model": "gpt4", + "model_name": "gpt4", "max_tokens": 8192, "temperature": 0.7, "max_tool_iterations": 20 diff --git a/cmd/picoclaw/cmd_agent.go b/cmd/picoclaw/cmd_agent.go index 6d6ff935f..6331fdd4a 100644 --- a/cmd/picoclaw/cmd_agent.go +++ b/cmd/picoclaw/cmd_agent.go @@ -56,7 +56,7 @@ func agentCmd() { } if modelOverride != "" { - cfg.Agents.Defaults.Model = modelOverride + cfg.Agents.Defaults.ModelName = modelOverride } provider, modelID, err := providers.CreateProvider(cfg) @@ -66,7 +66,7 @@ func agentCmd() { } // Use the resolved model ID from provider creation if modelID != "" { - cfg.Agents.Defaults.Model = modelID + cfg.Agents.Defaults.ModelName = modelID } msgBus := bus.NewMessageBus() diff --git a/cmd/picoclaw/cmd_auth.go b/cmd/picoclaw/cmd_auth.go index 729c56177..55eb3cec3 100644 --- a/cmd/picoclaw/cmd_auth.go +++ b/cmd/picoclaw/cmd_auth.go @@ -144,7 +144,7 @@ func authLoginOpenAI(useDeviceCode bool) { } // Update default model to use OpenAI - appCfg.Agents.Defaults.Model = "gpt-5.2" + appCfg.Agents.Defaults.ModelName = "gpt-5.2" if err := config.SaveConfig(getConfigPath(), appCfg); err != nil { fmt.Printf("Warning: could not update config: %v\n", err) @@ -218,7 +218,7 @@ func authLoginGoogleAntigravity() { } // Update default model - appCfg.Agents.Defaults.Model = "gemini-flash" + appCfg.Agents.Defaults.ModelName = "gemini-flash" if err := config.SaveConfig(getConfigPath(), appCfg); err != nil { fmt.Printf("Warning: could not update config: %v\n", err) @@ -292,7 +292,7 @@ func authLoginPasteToken(provider string) { }) } // Update default model - appCfg.Agents.Defaults.Model = "claude-sonnet-4.6" + appCfg.Agents.Defaults.ModelName = "claude-sonnet-4.6" case "openai": appCfg.Providers.OpenAI.AuthMethod = "token" // Update ModelList @@ -312,7 +312,7 @@ func authLoginPasteToken(provider string) { }) } // Update default model - appCfg.Agents.Defaults.Model = "gpt-5.2" + appCfg.Agents.Defaults.ModelName = "gpt-5.2" } if err := config.SaveConfig(getConfigPath(), appCfg); err != nil { fmt.Printf("Warning: could not update config: %v\n", err) @@ -320,7 +320,7 @@ func authLoginPasteToken(provider string) { } fmt.Printf("Token saved for %s!\n", provider) - fmt.Printf("Default model set to: %s\n", appCfg.Agents.Defaults.Model) + fmt.Printf("Default model set to: %s\n", appCfg.Agents.Defaults.GetModelName()) } func authLogoutCmd() { diff --git a/cmd/picoclaw/cmd_gateway.go b/cmd/picoclaw/cmd_gateway.go index 9a3b6aa19..bd1cdd7a8 100644 --- a/cmd/picoclaw/cmd_gateway.go +++ b/cmd/picoclaw/cmd_gateway.go @@ -51,7 +51,7 @@ func gatewayCmd() { } // Use the resolved model ID from provider creation if modelID != "" { - cfg.Agents.Defaults.Model = modelID + cfg.Agents.Defaults.ModelName = modelID } msgBus := bus.NewMessageBus() diff --git a/cmd/picoclaw/cmd_status.go b/cmd/picoclaw/cmd_status.go index 07296784e..6a117bd17 100644 --- a/cmd/picoclaw/cmd_status.go +++ b/cmd/picoclaw/cmd_status.go @@ -41,7 +41,7 @@ func statusCmd() { } if _, err := os.Stat(configPath); err == nil { - fmt.Printf("Model: %s\n", cfg.Agents.Defaults.Model) + fmt.Printf("Model: %s\n", cfg.Agents.Defaults.GetModelName()) hasOpenRouter := cfg.Providers.OpenRouter.APIKey != "" hasAnthropic := cfg.Providers.Anthropic.APIKey != "" diff --git a/config/config.example.json b/config/config.example.json index 77a8c0683..1795fed9f 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -3,7 +3,7 @@ "defaults": { "workspace": "~/.picoclaw/workspace", "restrict_to_workspace": true, - "model": "gpt4", + "model_name": "gpt4", "max_tokens": 8192, "temperature": 0.7, "max_tool_iterations": 20 diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index dfbef9fbc..c6a54c7d2 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -133,7 +133,7 @@ func resolveAgentModel(agentCfg *config.AgentConfig, defaults *config.AgentDefau if agentCfg != nil && agentCfg.Model != nil && strings.TrimSpace(agentCfg.Model.Primary) != "" { return strings.TrimSpace(agentCfg.Model.Primary) } - return defaults.Model + return defaults.GetModelName() } // resolveAgentFallbacks resolves the fallback models for an agent. diff --git a/pkg/channels/telegram_commands.go b/pkg/channels/telegram_commands.go index a084b641b..f28434f46 100644 --- a/pkg/channels/telegram_commands.go +++ b/pkg/channels/telegram_commands.go @@ -81,7 +81,7 @@ func (c *cmd) Show(ctx context.Context, message telego.Message) error { switch args { case "model": response = fmt.Sprintf("Current Model: %s (Provider: %s)", - c.config.Agents.Defaults.Model, + c.config.Agents.Defaults.GetModelName(), c.config.Agents.Defaults.Provider) case "channel": response = "Current Channel: telegram" @@ -120,7 +120,7 @@ func (c *cmd) List(ctx context.Context, message telego.Message) error { provider = "configured default" } response = fmt.Sprintf("Configured Model: %s\nProvider: %s\n\nTo change models, update config.yaml", - c.config.Agents.Defaults.Model, provider) + c.config.Agents.Defaults.GetModelName(), provider) case "channels": var enabled []string diff --git a/pkg/config/config.go b/pkg/config/config.go index 20556011a..c103963c8 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -170,7 +170,8 @@ type AgentDefaults struct { Workspace string `json:"workspace" env:"PICOCLAW_AGENTS_DEFAULTS_WORKSPACE"` RestrictToWorkspace bool `json:"restrict_to_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_RESTRICT_TO_WORKSPACE"` Provider string `json:"provider" env:"PICOCLAW_AGENTS_DEFAULTS_PROVIDER"` - Model string `json:"model" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL"` + ModelName string `json:"model_name,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL_NAME"` + Model string `json:"model,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MODEL"` // Deprecated: use model_name instead ModelFallbacks []string `json:"model_fallbacks,omitempty"` ImageModel string `json:"image_model,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_IMAGE_MODEL"` ImageModelFallbacks []string `json:"image_model_fallbacks,omitempty"` @@ -179,6 +180,15 @@ type AgentDefaults struct { MaxToolIterations int `json:"max_tool_iterations" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOOL_ITERATIONS"` } +// GetModelName returns the effective model name for the agent defaults. +// It prefers the new "model_name" field but falls back to "model" for backward compatibility. +func (d *AgentDefaults) GetModelName() string { + if d.ModelName != "" { + return d.ModelName + } + return d.Model +} + type ChannelsConfig struct { WhatsApp WhatsAppConfig `json:"whatsapp"` Telegram TelegramConfig `json:"telegram"` diff --git a/pkg/config/migration.go b/pkg/config/migration.go index 689e2312f..e51d4efb2 100644 --- a/pkg/config/migration.go +++ b/pkg/config/migration.go @@ -41,7 +41,7 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { // Get user's configured provider and model userProvider := strings.ToLower(cfg.Agents.Defaults.Provider) - userModel := cfg.Agents.Defaults.Model + userModel := cfg.Agents.Defaults.GetModelName() p := cfg.Providers diff --git a/pkg/config/model_config_test.go b/pkg/config/model_config_test.go index 3c411dc0f..c89029e8c 100644 --- a/pkg/config/model_config_test.go +++ b/pkg/config/model_config_test.go @@ -6,6 +6,7 @@ package config import ( + "encoding/json" "strings" "sync" "testing" @@ -114,6 +115,137 @@ func TestGetModelConfig_Concurrent(t *testing.T) { } } +func TestAgentDefaults_GetModelName_BackwardCompat(t *testing.T) { + tests := []struct { + name string + defaults AgentDefaults + wantName string + }{ + { + name: "new model_name field only", + defaults: AgentDefaults{ModelName: "new-model"}, + wantName: "new-model", + }, + { + name: "old model field only", + defaults: AgentDefaults{Model: "legacy-model"}, + wantName: "legacy-model", + }, + { + name: "both fields - model_name takes precedence", + defaults: AgentDefaults{ModelName: "new-model", Model: "old-model"}, + wantName: "new-model", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.defaults.GetModelName(); got != tt.wantName { + t.Errorf("GetModelName() = %q, want %q", got, tt.wantName) + } + }) + } +} + +func TestAgentDefaults_JSON_BackwardCompat(t *testing.T) { + tests := []struct { + name string + json string + wantName string + }{ + { + name: "new model_name field", + json: `{"model_name": "gpt4"}`, + wantName: "gpt4", + }, + { + name: "old model field", + json: `{"model": "gpt4"}`, + wantName: "gpt4", + }, + { + name: "both fields - model_name wins", + json: `{"model_name": "new", "model": "old"}`, + wantName: "new", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var defaults AgentDefaults + if err := json.Unmarshal([]byte(tt.json), &defaults); err != nil { + t.Fatalf("Unmarshal error: %v", err) + } + if got := defaults.GetModelName(); got != tt.wantName { + t.Errorf("GetModelName() = %q, want %q", got, tt.wantName) + } + }) + } +} + +func TestFullConfig_JSON_BackwardCompat(t *testing.T) { + // Test complete config with both old and new formats + oldFormat := `{ + "agents": { + "defaults": { + "workspace": "~/.picoclaw/workspace", + "model": "gpt4", + "max_tokens": 4096 + } + }, + "model_list": [ + { + "model_name": "gpt4", + "model": "openai/gpt-4o", + "api_key": "test-key" + } + ] + }` + + newFormat := `{ + "agents": { + "defaults": { + "workspace": "~/.picoclaw/workspace", + "model_name": "gpt4", + "max_tokens": 4096 + } + }, + "model_list": [ + { + "model_name": "gpt4", + "model": "openai/gpt-4o", + "api_key": "test-key" + } + ] + }` + + for name, jsonStr := range map[string]string{ + "old format (model)": oldFormat, + "new format (model_name)": newFormat, + } { + t.Run(name, func(t *testing.T) { + cfg := &Config{} + if err := json.Unmarshal([]byte(jsonStr), cfg); err != nil { + t.Fatalf("Unmarshal error: %v", err) + } + + // Check that GetModelName returns correct value + if got := cfg.Agents.Defaults.GetModelName(); got != "gpt4" { + t.Errorf("GetModelName() = %q, want %q", got, "gpt4") + } + + // Check that GetModelConfig works + modelCfg, err := cfg.GetModelConfig("gpt4") + if err != nil { + t.Fatalf("GetModelConfig error: %v", err) + } + if modelCfg.Model != "openai/gpt-4o" { + t.Errorf("Model = %q, want %q", modelCfg.Model, "openai/gpt-4o") + } + }) + } +} + func TestModelConfig_Validate(t *testing.T) { tests := []struct { name string diff --git a/pkg/migrate/config.go b/pkg/migrate/config.go index 2237a1429..d9552db9f 100644 --- a/pkg/migrate/config.go +++ b/pkg/migrate/config.go @@ -72,7 +72,10 @@ func ConvertConfig(data map[string]any) (*config.Config, []string, error) { if agents, ok := getMap(data, "agents"); ok { if defaults, ok := getMap(agents, "defaults"); ok { - if v, ok := getString(defaults, "model"); ok { + // Prefer model_name, fallback to model for backward compatibility + if v, ok := getString(defaults, "model_name"); ok { + cfg.Agents.Defaults.ModelName = v + } else if v, ok := getString(defaults, "model"); ok { cfg.Agents.Defaults.Model = v } if v, ok := getFloat(defaults, "max_tokens"); ok { diff --git a/pkg/providers/factory.go b/pkg/providers/factory.go index b6f1b5e21..b9abfdc61 100644 --- a/pkg/providers/factory.go +++ b/pkg/providers/factory.go @@ -36,7 +36,7 @@ type providerSelection struct { } func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { - model := cfg.Agents.Defaults.Model + model := cfg.Agents.Defaults.GetModelName() providerName := strings.ToLower(cfg.Agents.Defaults.Provider) lowerModel := strings.ToLower(model) diff --git a/pkg/providers/legacy_provider.go b/pkg/providers/legacy_provider.go index eb13cec65..23f137538 100644 --- a/pkg/providers/legacy_provider.go +++ b/pkg/providers/legacy_provider.go @@ -16,7 +16,7 @@ import ( // The old providers config is automatically converted to model_list during config loading. // Returns the provider, the model ID to use, and any error. func CreateProvider(cfg *config.Config) (LLMProvider, string, error) { - model := cfg.Agents.Defaults.Model + model := cfg.Agents.Defaults.GetModelName() // Ensure model_list is populated (should be done by LoadConfig, but handle edge cases) if len(cfg.ModelList) == 0 && cfg.HasProvidersConfig() { From 76f2b42d5b88bebec2a247b180cf1f19bc69464c Mon Sep 17 00:00:00 2001 From: nayihz Date: Tue, 24 Feb 2026 17:16:16 +0800 Subject: [PATCH 04/11] feat: improve web proxy handling and coverage --- config/config.example.json | 3 +- pkg/agent/loop.go | 3 +- pkg/config/config.go | 3 + pkg/config/config_test.go | 21 +++++ pkg/tools/web.go | 101 +++++++++++++++++----- pkg/tools/web_test.go | 167 +++++++++++++++++++++++++++++++++++++ 6 files changed, 275 insertions(+), 23 deletions(-) diff --git a/config/config.example.json b/config/config.example.json index 555509732..c73d327d6 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -217,7 +217,8 @@ "enabled": false, "api_key": "pplx-xxx", "max_results": 5 - } + }, + "proxy": "" }, "cron": { "exec_timeout_minutes": 5 diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 9a2bb1198..dbc4a9b87 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -106,10 +106,11 @@ func registerSharedTools( PerplexityAPIKey: cfg.Tools.Web.Perplexity.APIKey, PerplexityMaxResults: cfg.Tools.Web.Perplexity.MaxResults, PerplexityEnabled: cfg.Tools.Web.Perplexity.Enabled, + Proxy: cfg.Tools.Web.Proxy, }); searchTool != nil { agent.Tools.Register(searchTool) } - agent.Tools.Register(tools.NewWebFetchTool(50000)) + agent.Tools.Register(tools.NewWebFetchToolWithProxy(50000, cfg.Tools.Web.Proxy)) // Hardware tools (I2C, SPI) - Linux only, returns error on other platforms agent.Tools.Register(tools.NewI2CTool()) diff --git a/pkg/config/config.go b/pkg/config/config.go index 2595398c7..67adca5c3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -443,6 +443,9 @@ type WebToolsConfig struct { Tavily TavilyConfig `json:"tavily"` DuckDuckGo DuckDuckGoConfig `json:"duckduckgo"` Perplexity PerplexityConfig `json:"perplexity"` + // Proxy is an optional proxy URL for web tools (http/https/socks5/socks5h). + // For authenticated proxies, prefer HTTP_PROXY/HTTPS_PROXY env vars instead of embedding credentials in config. + Proxy string `json:"proxy,omitempty" env:"PICOCLAW_TOOLS_WEB_PROXY"` } type CronToolsConfig struct { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index f88c0269c..223ac798d 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -392,3 +392,24 @@ func TestLoadConfig_OpenAIWebSearchCanBeDisabled(t *testing.T) { t.Fatal("OpenAI codex web search should be false when disabled in config file") } } + +func TestLoadConfig_WebToolsProxy(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.json") + configJSON := `{ + "agents": {"defaults":{"workspace":"./workspace","model":"gpt4","max_tokens":8192,"max_tool_iterations":20}}, + "model_list": [{"model_name":"gpt4","model":"openai/gpt-5.2","api_key":"x"}], + "tools": {"web":{"proxy":"http://127.0.0.1:7890"}} +}` + if err := os.WriteFile(configPath, []byte(configJSON), 0o600); err != nil { + t.Fatalf("os.WriteFile() error: %v", err) + } + + cfg, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error: %v", err) + } + if cfg.Tools.Web.Proxy != "http://127.0.0.1:7890" { + t.Fatalf("Tools.Web.Proxy = %q, want %q", cfg.Tools.Web.Proxy, "http://127.0.0.1:7890") + } +} diff --git a/pkg/tools/web.go b/pkg/tools/web.go index 452e95e0f..968579dea 100644 --- a/pkg/tools/web.go +++ b/pkg/tools/web.go @@ -17,12 +17,50 @@ const ( userAgent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" ) +// createHTTPClient creates an HTTP client with optional proxy support +func createHTTPClient(proxyURL string, timeout time.Duration) (*http.Client, error) { + client := &http.Client{ + Timeout: timeout, + Transport: &http.Transport{ + MaxIdleConns: 10, + IdleConnTimeout: 30 * time.Second, + DisableCompression: false, + TLSHandshakeTimeout: 15 * time.Second, + }, + } + + if proxyURL != "" { + proxy, err := url.Parse(proxyURL) + if err != nil { + return nil, fmt.Errorf("invalid proxy URL: %w", err) + } + scheme := strings.ToLower(proxy.Scheme) + switch scheme { + case "http", "https", "socks5", "socks5h": + default: + return nil, fmt.Errorf( + "unsupported proxy scheme %q (supported: http, https, socks5, socks5h)", + proxy.Scheme, + ) + } + if proxy.Host == "" { + return nil, fmt.Errorf("invalid proxy URL: missing host") + } + client.Transport.(*http.Transport).Proxy = http.ProxyURL(proxy) + } else { + client.Transport.(*http.Transport).Proxy = http.ProxyFromEnvironment + } + + return client, nil +} + type SearchProvider interface { Search(ctx context.Context, query string, count int) (string, error) } type BraveSearchProvider struct { apiKey string + proxy string } func (p *BraveSearchProvider) Search(ctx context.Context, query string, count int) (string, error) { @@ -37,7 +75,10 @@ func (p *BraveSearchProvider) Search(ctx context.Context, query string, count in req.Header.Set("Accept", "application/json") req.Header.Set("X-Subscription-Token", p.apiKey) - client := &http.Client{Timeout: 10 * time.Second} + client, err := createHTTPClient(p.proxy, 10*time.Second) + if err != nil { + return "", fmt.Errorf("failed to create HTTP client: %w", err) + } resp, err := client.Do(req) if err != nil { return "", fmt.Errorf("request failed: %w", err) @@ -167,7 +208,9 @@ func (p *TavilySearchProvider) Search(ctx context.Context, query string, count i return strings.Join(lines, "\n"), nil } -type DuckDuckGoSearchProvider struct{} +type DuckDuckGoSearchProvider struct { + proxy string +} func (p *DuckDuckGoSearchProvider) Search(ctx context.Context, query string, count int) (string, error) { searchURL := fmt.Sprintf("https://html.duckduckgo.com/html/?q=%s", url.QueryEscape(query)) @@ -179,7 +222,10 @@ func (p *DuckDuckGoSearchProvider) Search(ctx context.Context, query string, cou req.Header.Set("User-Agent", userAgent) - client := &http.Client{Timeout: 10 * time.Second} + client, err := createHTTPClient(p.proxy, 10*time.Second) + if err != nil { + return "", fmt.Errorf("failed to create HTTP client: %w", err) + } resp, err := client.Do(req) if err != nil { return "", fmt.Errorf("request failed: %w", err) @@ -261,6 +307,7 @@ func stripTags(content string) string { type PerplexitySearchProvider struct { apiKey string + proxy string } func (p *PerplexitySearchProvider) Search(ctx context.Context, query string, count int) (string, error) { @@ -295,7 +342,10 @@ func (p *PerplexitySearchProvider) Search(ctx context.Context, query string, cou req.Header.Set("Authorization", "Bearer "+p.apiKey) req.Header.Set("User-Agent", userAgent) - client := &http.Client{Timeout: 30 * time.Second} + client, err := createHTTPClient(p.proxy, 30*time.Second) + if err != nil { + return "", fmt.Errorf("failed to create HTTP client: %w", err) + } resp, err := client.Do(req) if err != nil { return "", fmt.Errorf("request failed: %w", err) @@ -348,6 +398,7 @@ type WebSearchToolOptions struct { PerplexityAPIKey string PerplexityMaxResults int PerplexityEnabled bool + Proxy string } func NewWebSearchTool(opts WebSearchToolOptions) *WebSearchTool { @@ -356,12 +407,12 @@ func NewWebSearchTool(opts WebSearchToolOptions) *WebSearchTool { // Priority: Perplexity > Brave > Tavily > DuckDuckGo if opts.PerplexityEnabled && opts.PerplexityAPIKey != "" { - provider = &PerplexitySearchProvider{apiKey: opts.PerplexityAPIKey} + provider = &PerplexitySearchProvider{apiKey: opts.PerplexityAPIKey, proxy: opts.Proxy} if opts.PerplexityMaxResults > 0 { maxResults = opts.PerplexityMaxResults } } else if opts.BraveEnabled && opts.BraveAPIKey != "" { - provider = &BraveSearchProvider{apiKey: opts.BraveAPIKey} + provider = &BraveSearchProvider{apiKey: opts.BraveAPIKey, proxy: opts.Proxy} if opts.BraveMaxResults > 0 { maxResults = opts.BraveMaxResults } @@ -374,7 +425,7 @@ func NewWebSearchTool(opts WebSearchToolOptions) *WebSearchTool { maxResults = opts.TavilyMaxResults } } else if opts.DuckDuckGoEnabled { - provider = &DuckDuckGoSearchProvider{} + provider = &DuckDuckGoSearchProvider{proxy: opts.Proxy} if opts.DuckDuckGoMaxResults > 0 { maxResults = opts.DuckDuckGoMaxResults } @@ -441,6 +492,7 @@ func (t *WebSearchTool) Execute(ctx context.Context, args map[string]any) *ToolR type WebFetchTool struct { maxChars int + proxy string } func NewWebFetchTool(maxChars int) *WebFetchTool { @@ -452,6 +504,16 @@ func NewWebFetchTool(maxChars int) *WebFetchTool { } } +func NewWebFetchToolWithProxy(maxChars int, proxy string) *WebFetchTool { + if maxChars <= 0 { + maxChars = 50000 + } + return &WebFetchTool{ + maxChars: maxChars, + proxy: proxy, + } +} + func (t *WebFetchTool) Name() string { return "web_fetch" } @@ -511,20 +573,17 @@ func (t *WebFetchTool) Execute(ctx context.Context, args map[string]any) *ToolRe req.Header.Set("User-Agent", userAgent) - client := &http.Client{ - Timeout: 60 * time.Second, - Transport: &http.Transport{ - MaxIdleConns: 10, - IdleConnTimeout: 30 * time.Second, - DisableCompression: false, - TLSHandshakeTimeout: 15 * time.Second, - }, - CheckRedirect: func(req *http.Request, via []*http.Request) error { - if len(via) >= 5 { - return fmt.Errorf("stopped after 5 redirects") - } - return nil - }, + client, err := createHTTPClient(t.proxy, 60*time.Second) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to create HTTP client: %v", err)) + } + + // Configure redirect handling + client.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if len(via) >= 5 { + return fmt.Errorf("stopped after 5 redirects") + } + return nil } resp, err := client.Do(req) diff --git a/pkg/tools/web_test.go b/pkg/tools/web_test.go index 75e0d8d16..2cd79eb24 100644 --- a/pkg/tools/web_test.go +++ b/pkg/tools/web_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" ) // TestWebTool_WebFetch_Success verifies successful URL fetching @@ -334,6 +335,172 @@ func TestWebTool_WebFetch_MissingDomain(t *testing.T) { } } +func TestCreateHTTPClient_ProxyConfigured(t *testing.T) { + client, err := createHTTPClient("http://127.0.0.1:7890", 12*time.Second) + if err != nil { + t.Fatalf("createHTTPClient() error: %v", err) + } + if client.Timeout != 12*time.Second { + t.Fatalf("client.Timeout = %v, want %v", client.Timeout, 12*time.Second) + } + + tr, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("client.Transport type = %T, want *http.Transport", client.Transport) + } + if tr.Proxy == nil { + t.Fatal("transport.Proxy is nil, want non-nil") + } + + req, err := http.NewRequest("GET", "https://example.com", nil) + if err != nil { + t.Fatalf("http.NewRequest() error: %v", err) + } + proxyURL, err := tr.Proxy(req) + if err != nil { + t.Fatalf("transport.Proxy(req) error: %v", err) + } + if proxyURL == nil || proxyURL.String() != "http://127.0.0.1:7890" { + t.Fatalf("proxy URL = %v, want %q", proxyURL, "http://127.0.0.1:7890") + } +} + +func TestCreateHTTPClient_InvalidProxy(t *testing.T) { + _, err := createHTTPClient("://bad-proxy", 10*time.Second) + if err == nil { + t.Fatal("createHTTPClient() expected error for invalid proxy URL, got nil") + } +} + +func TestCreateHTTPClient_Socks5ProxyConfigured(t *testing.T) { + client, err := createHTTPClient("socks5://127.0.0.1:1080", 8*time.Second) + if err != nil { + t.Fatalf("createHTTPClient() error: %v", err) + } + + tr, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("client.Transport type = %T, want *http.Transport", client.Transport) + } + req, err := http.NewRequest("GET", "https://example.com", nil) + if err != nil { + t.Fatalf("http.NewRequest() error: %v", err) + } + proxyURL, err := tr.Proxy(req) + if err != nil { + t.Fatalf("transport.Proxy(req) error: %v", err) + } + if proxyURL == nil || proxyURL.String() != "socks5://127.0.0.1:1080" { + t.Fatalf("proxy URL = %v, want %q", proxyURL, "socks5://127.0.0.1:1080") + } +} + +func TestCreateHTTPClient_UnsupportedProxyScheme(t *testing.T) { + _, err := createHTTPClient("ftp://127.0.0.1:21", 10*time.Second) + if err == nil { + t.Fatal("createHTTPClient() expected error for unsupported scheme, got nil") + } + if !strings.Contains(err.Error(), "unsupported proxy scheme") { + t.Fatalf("error = %q, want to contain %q", err.Error(), "unsupported proxy scheme") + } +} + +func TestCreateHTTPClient_ProxyFromEnvironmentWhenConfigEmpty(t *testing.T) { + t.Setenv("HTTP_PROXY", "http://127.0.0.1:8888") + t.Setenv("http_proxy", "http://127.0.0.1:8888") + t.Setenv("HTTPS_PROXY", "http://127.0.0.1:8888") + t.Setenv("https_proxy", "http://127.0.0.1:8888") + t.Setenv("ALL_PROXY", "") + t.Setenv("all_proxy", "") + t.Setenv("NO_PROXY", "") + t.Setenv("no_proxy", "") + + client, err := createHTTPClient("", 10*time.Second) + if err != nil { + t.Fatalf("createHTTPClient() error: %v", err) + } + + tr, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("client.Transport type = %T, want *http.Transport", client.Transport) + } + if tr.Proxy == nil { + t.Fatal("transport.Proxy is nil, want proxy function from environment") + } + + req, err := http.NewRequest("GET", "https://example.com", nil) + if err != nil { + t.Fatalf("http.NewRequest() error: %v", err) + } + if _, err := tr.Proxy(req); err != nil { + t.Fatalf("transport.Proxy(req) error: %v", err) + } +} + +func TestNewWebFetchToolWithProxy(t *testing.T) { + tool := NewWebFetchToolWithProxy(1024, "http://127.0.0.1:7890") + if tool.maxChars != 1024 { + t.Fatalf("maxChars = %d, want %d", tool.maxChars, 1024) + } + if tool.proxy != "http://127.0.0.1:7890" { + t.Fatalf("proxy = %q, want %q", tool.proxy, "http://127.0.0.1:7890") + } + + tool = NewWebFetchToolWithProxy(0, "http://127.0.0.1:7890") + if tool.maxChars != 50000 { + t.Fatalf("default maxChars = %d, want %d", tool.maxChars, 50000) + } +} + +func TestNewWebSearchTool_PropagatesProxy(t *testing.T) { + t.Run("perplexity", func(t *testing.T) { + tool := NewWebSearchTool(WebSearchToolOptions{ + PerplexityEnabled: true, + PerplexityAPIKey: "k", + PerplexityMaxResults: 3, + Proxy: "http://127.0.0.1:7890", + }) + p, ok := tool.provider.(*PerplexitySearchProvider) + if !ok { + t.Fatalf("provider type = %T, want *PerplexitySearchProvider", tool.provider) + } + if p.proxy != "http://127.0.0.1:7890" { + t.Fatalf("provider proxy = %q, want %q", p.proxy, "http://127.0.0.1:7890") + } + }) + + t.Run("brave", func(t *testing.T) { + tool := NewWebSearchTool(WebSearchToolOptions{ + BraveEnabled: true, + BraveAPIKey: "k", + BraveMaxResults: 3, + Proxy: "http://127.0.0.1:7890", + }) + p, ok := tool.provider.(*BraveSearchProvider) + if !ok { + t.Fatalf("provider type = %T, want *BraveSearchProvider", tool.provider) + } + if p.proxy != "http://127.0.0.1:7890" { + t.Fatalf("provider proxy = %q, want %q", p.proxy, "http://127.0.0.1:7890") + } + }) + + t.Run("duckduckgo", func(t *testing.T) { + tool := NewWebSearchTool(WebSearchToolOptions{ + DuckDuckGoEnabled: true, + DuckDuckGoMaxResults: 3, + Proxy: "http://127.0.0.1:7890", + }) + p, ok := tool.provider.(*DuckDuckGoSearchProvider) + if !ok { + t.Fatalf("provider type = %T, want *DuckDuckGoSearchProvider", tool.provider) + } + if p.proxy != "http://127.0.0.1:7890" { + t.Fatalf("provider proxy = %q, want %q", p.proxy, "http://127.0.0.1:7890") + } + }) +} + // TestWebTool_TavilySearch_Success verifies successful Tavily search func TestWebTool_TavilySearch_Success(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 8405d390dfaca26fba80fd63728d5f87353e1b92 Mon Sep 17 00:00:00 2001 From: root Date: Tue, 24 Feb 2026 04:19:18 -0500 Subject: [PATCH 05/11] fix: add CGO_ENABLED=0 for static build to fix cross-platform GLIBC errors --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 29e2fc964..111908a79 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ GO_VERSION=$(shell $(GO) version | awk '{print $$3}') LDFLAGS=-ldflags "-X main.version=$(VERSION) -X main.gitCommit=$(GIT_COMMIT) -X main.buildTime=$(BUILD_TIME) -X main.goVersion=$(GO_VERSION) -s -w" # Go variables -GO?=go +GO?=CGO_ENABLED=0 go GOFLAGS?=-v -tags stdjson # Golangci-lint From 0d761ca6084efb1344591870ce58d96b6a9be051 Mon Sep 17 00:00:00 2001 From: yangmanqing Date: Tue, 24 Feb 2026 17:57:28 +0800 Subject: [PATCH 06/11] fix: prevent DefaultConfig template values from leaking into user model_list entries --- pkg/config/config.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pkg/config/config.go b/pkg/config/config.go index 2595398c7..a15a19b91 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -499,6 +499,20 @@ func LoadConfig(path string) (*Config, error) { return nil, err } + // Pre-scan the JSON to check how many model_list entries the user provided. + // Go's JSON decoder reuses existing slice backing-array elements rather than + // zero-initializing them, so fields absent from the user's JSON (e.g. api_base) + // would silently inherit values from the DefaultConfig template at the same + // index position. We only reset cfg.ModelList when the user actually provides + // entries; when count is 0 we keep DefaultConfig's built-in list as fallback. + var tmp Config + if err := json.Unmarshal(data, &tmp); err != nil { + return nil, err + } + if len(tmp.ModelList) > 0 { + cfg.ModelList = nil + } + if err := json.Unmarshal(data, cfg); err != nil { return nil, err } From 78e5bdad29c9c927b72b99b40b3da87d5dc28431 Mon Sep 17 00:00:00 2001 From: Kai Xia Date: Tue, 24 Feb 2026 22:04:23 +1100 Subject: [PATCH 07/11] minor improvements in Makefile Signed-off-by: Kai Xia --- Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 29e2fc964..30522e7b9 100644 --- a/Makefile +++ b/Makefile @@ -144,6 +144,10 @@ fmt: lint: @$(GOLANGCI_LINT) run +## fix: Fix linting issues +fix: + @$(GOLANGCI_LINT) run --fix + ## deps: Download dependencies deps: @$(GO) mod download @@ -169,7 +173,7 @@ help: @echo " make [target]" @echo "" @echo "Targets:" - @grep -E '^## ' $(MAKEFILE_LIST) | sed 's/## / /' + @grep -E '^## ' $(MAKEFILE_LIST) | sort | awk -F': ' '{printf " %-16s %s\n", substr($$1, 4), $$2}' @echo "" @echo "Examples:" @echo " make build # Build for current platform" From 01e2354b977d580679871379beef24f371bf521e Mon Sep 17 00:00:00 2001 From: yinwm Date: Tue, 24 Feb 2026 19:04:38 +0800 Subject: [PATCH 08/11] fix: align map values for proper formatting --- pkg/config/model_config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/config/model_config_test.go b/pkg/config/model_config_test.go index c89029e8c..99eea2782 100644 --- a/pkg/config/model_config_test.go +++ b/pkg/config/model_config_test.go @@ -220,7 +220,7 @@ func TestFullConfig_JSON_BackwardCompat(t *testing.T) { }` for name, jsonStr := range map[string]string{ - "old format (model)": oldFormat, + "old format (model)": oldFormat, "new format (model_name)": newFormat, } { t.Run(name, func(t *testing.T) { From 100356e8ec029f689a6b43363ada3556a2163365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kai=20Xia=28=E5=A4=8F=E6=81=BA=29?= Date: Wed, 25 Feb 2026 00:52:25 +1100 Subject: [PATCH 09/11] refactor: cleanup dead code and turn on dead code detection in CI (#515) * cleanup dead code. Signed-off-by: Kai Xia * add these two back with flag. Signed-off-by: Kai Xia * fix ci Signed-off-by: Kai Xia * remove this confusing line Signed-off-by: Kai Xia * make fmt Signed-off-by: Kai Xia * remove unused method. picked up by golangci-lint run Signed-off-by: Kai Xia --------- Signed-off-by: Kai Xia --- .golangci.yaml | 4 +- pkg/agent/context.go | 19 --------- pkg/channels/wecom_app.go | 55 ------------------------- pkg/devices/sources/usb_linux.go | 5 +-- pkg/providers/antigravity_provider.go | 58 --------------------------- pkg/providers/fallback_test.go | 6 --- pkg/tools/i2c.go | 8 ++++ pkg/tools/spi.go | 4 ++ 8 files changed, 17 insertions(+), 142 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index d45d69e67..dd3cbae19 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -66,7 +66,6 @@ linters: - testifylint - thelper - unparam - - unused - usestdlibvars - usetesting - wastedassign @@ -152,6 +151,9 @@ linters: - gocognit - gocyclo path: _test\.go$ + - linters: + - nolintlint + path: 'pkg/tools/(i2c\.go|spi\.go)$' issues: max-issues-per-linter: 0 diff --git a/pkg/agent/context.go b/pkg/agent/context.go index a9db5afdd..ba07e33d3 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -288,25 +288,6 @@ func (cb *ContextBuilder) AddAssistantMessage( return messages } -func (cb *ContextBuilder) loadSkills() string { - allSkills := cb.skillsLoader.ListSkills() - if len(allSkills) == 0 { - return "" - } - - var skillNames []string - for _, s := range allSkills { - skillNames = append(skillNames, s.Name) - } - - content := cb.skillsLoader.LoadSkillsForContext(skillNames) - if content == "" { - return "" - } - - return "# Skill Definitions\n\n" + content -} - // GetSkillsInfo returns information about loaded skills. func (cb *ContextBuilder) GetSkillsInfo() map[string]any { allSkills := cb.skillsLoader.ListSkills() diff --git a/pkg/channels/wecom_app.go b/pkg/channels/wecom_app.go index 715c48707..302603445 100644 --- a/pkg/channels/wecom_app.go +++ b/pkg/channels/wecom_app.go @@ -571,61 +571,6 @@ func (c *WeComAppChannel) sendTextMessage(ctx context.Context, accessToken, user return nil } -// sendMarkdownMessage sends a markdown message to a user -func (c *WeComAppChannel) sendMarkdownMessage(ctx context.Context, accessToken, userID, content string) error { - apiURL := fmt.Sprintf("%s/cgi-bin/message/send?access_token=%s", wecomAPIBase, accessToken) - - msg := WeComMarkdownMessage{ - ToUser: userID, - MsgType: "markdown", - AgentID: c.config.AgentID, - } - msg.Markdown.Content = content - - jsonData, err := json.Marshal(msg) - if err != nil { - return fmt.Errorf("failed to marshal message: %w", err) - } - - // Use configurable timeout (default 5 seconds) - timeout := c.config.ReplyTimeout - if timeout <= 0 { - timeout = 5 - } - - reqCtx, cancel := context.WithTimeout(ctx, time.Duration(timeout)*time.Second) - defer cancel() - - req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, apiURL, bytes.NewBuffer(jsonData)) - if err != nil { - return fmt.Errorf("failed to create request: %w", err) - } - req.Header.Set("Content-Type", "application/json") - - client := &http.Client{Timeout: time.Duration(timeout) * time.Second} - resp, err := client.Do(req) - if err != nil { - return fmt.Errorf("failed to send message: %w", err) - } - defer resp.Body.Close() - - body, err := io.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("failed to read response: %w", err) - } - - var sendResp WeComSendMessageResponse - if err := json.Unmarshal(body, &sendResp); err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - if sendResp.ErrCode != 0 { - return fmt.Errorf("API error: %s (code: %d)", sendResp.ErrMsg, sendResp.ErrCode) - } - - return nil -} - // handleHealth handles health check requests func (c *WeComAppChannel) handleHealth(w http.ResponseWriter, r *http.Request) { status := map[string]any{ diff --git a/pkg/devices/sources/usb_linux.go b/pkg/devices/sources/usb_linux.go index be0193cfb..2bb38941f 100644 --- a/pkg/devices/sources/usb_linux.go +++ b/pkg/devices/sources/usb_linux.go @@ -35,9 +35,8 @@ var usbClassToCapability = map[string]string{ } type USBMonitor struct { - cmd *exec.Cmd - cancel context.CancelFunc - mu sync.Mutex + cmd *exec.Cmd + mu sync.Mutex } func NewUSBMonitor() *USBMonitor { diff --git a/pkg/providers/antigravity_provider.go b/pkg/providers/antigravity_provider.go index cff67c88c..d4ee528b7 100644 --- a/pkg/providers/antigravity_provider.go +++ b/pkg/providers/antigravity_provider.go @@ -404,64 +404,6 @@ type antigravityJSONResponse struct { } `json:"usageMetadata"` } -func (p *AntigravityProvider) parseJSONResponse(body []byte) (*LLMResponse, error) { - var resp antigravityJSONResponse - if err := json.Unmarshal(body, &resp); err != nil { - return nil, fmt.Errorf("parsing antigravity response: %w", err) - } - - if len(resp.Candidates) == 0 { - return nil, fmt.Errorf("antigravity: no candidates in response") - } - - candidate := resp.Candidates[0] - var contentParts []string - var toolCalls []ToolCall - - for _, part := range candidate.Content.Parts { - if part.Text != "" { - contentParts = append(contentParts, part.Text) - } - if part.FunctionCall != nil { - argumentsJSON, _ := json.Marshal(part.FunctionCall.Args) - toolCalls = append(toolCalls, ToolCall{ - ID: fmt.Sprintf("call_%s_%d", part.FunctionCall.Name, time.Now().UnixNano()), - Name: part.FunctionCall.Name, - Arguments: part.FunctionCall.Args, - Function: &FunctionCall{ - Name: part.FunctionCall.Name, - Arguments: string(argumentsJSON), - ThoughtSignature: extractPartThoughtSignature(part.ThoughtSignature, part.ThoughtSignatureSnake), - }, - }) - } - } - - finishReason := "stop" - if len(toolCalls) > 0 { - finishReason = "tool_calls" - } - if candidate.FinishReason == "MAX_TOKENS" { - finishReason = "length" - } - - var usage *UsageInfo - if resp.UsageMetadata.TotalTokenCount > 0 { - usage = &UsageInfo{ - PromptTokens: resp.UsageMetadata.PromptTokenCount, - CompletionTokens: resp.UsageMetadata.CandidatesTokenCount, - TotalTokens: resp.UsageMetadata.TotalTokenCount, - } - } - - return &LLMResponse{ - Content: strings.Join(contentParts, ""), - ToolCalls: toolCalls, - FinishReason: finishReason, - Usage: usage, - }, nil -} - func (p *AntigravityProvider) parseSSEResponse(body string) (*LLMResponse, error) { var contentParts []string var toolCalls []ToolCall diff --git a/pkg/providers/fallback_test.go b/pkg/providers/fallback_test.go index e872c672e..ebba054ef 100644 --- a/pkg/providers/fallback_test.go +++ b/pkg/providers/fallback_test.go @@ -17,12 +17,6 @@ func successRun(content string) func(ctx context.Context, provider, model string } } -func failRun(err error) func(ctx context.Context, provider, model string) (*LLMResponse, error) { - return func(ctx context.Context, provider, model string) (*LLMResponse, error) { - return nil, err - } -} - func TestFallback_SingleCandidate_Success(t *testing.T) { ct := NewCooldownTracker() fc := NewFallbackChain(ct) diff --git a/pkg/tools/i2c.go b/pkg/tools/i2c.go index 0387a26d3..779b1d5a7 100644 --- a/pkg/tools/i2c.go +++ b/pkg/tools/i2c.go @@ -117,13 +117,19 @@ func (t *I2CTool) detect() *ToolResult { return SilentResult(fmt.Sprintf("Found %d I2C bus(es):\n%s", len(buses), string(result))) } +// Helper functions for I2C operations (used by platform-specific implementations) + // isValidBusID checks that a bus identifier is a simple number (prevents path injection) +// +//nolint:unused // Used by i2c_linux.go func isValidBusID(id string) bool { matched, _ := regexp.MatchString(`^\d+$`, id) return matched } // parseI2CAddress extracts and validates an I2C address from args +// +//nolint:unused // Used by i2c_linux.go func parseI2CAddress(args map[string]any) (int, *ToolResult) { addrFloat, ok := args["address"].(float64) if !ok { @@ -137,6 +143,8 @@ func parseI2CAddress(args map[string]any) (int, *ToolResult) { } // parseI2CBus extracts and validates an I2C bus from args +// +//nolint:unused // Used by i2c_linux.go func parseI2CBus(args map[string]any) (string, *ToolResult) { bus, ok := args["bus"].(string) if !ok || bus == "" { diff --git a/pkg/tools/spi.go b/pkg/tools/spi.go index d6a88a5b0..0ca17e84f 100644 --- a/pkg/tools/spi.go +++ b/pkg/tools/spi.go @@ -119,7 +119,11 @@ func (t *SPITool) list() *ToolResult { return SilentResult(fmt.Sprintf("Found %d SPI device(s):\n%s", len(devices), string(result))) } +// Helper function for SPI operations (used by platform-specific implementations) + // parseSPIArgs extracts and validates common SPI parameters +// +//nolint:unused // Used by spi_linux.go func parseSPIArgs(args map[string]any) (device string, speed uint32, mode uint8, bits uint8, errMsg string) { dev, ok := args["device"].(string) if !ok || dev == "" { From d09c64fcee20e9eabb13ca9c8de483747aafa930 Mon Sep 17 00:00:00 2001 From: Lixeer <1612655510@qq.com> Date: Tue, 24 Feb 2026 22:33:04 +0800 Subject: [PATCH 10/11] fix: implement code review suggestions Address all feedback from PR review: - Lock granularity - Empty response handling - Shutdown race condition - Interface naming --- cmd/picoclaw/cmd_gateway.go | 4 ++-- pkg/providers/github_copilot_provider.go | 21 +++++++++++++-------- pkg/providers/types.go | 2 +- pkg/tools/registry_test.go | 20 ++++++++++---------- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/cmd/picoclaw/cmd_gateway.go b/cmd/picoclaw/cmd_gateway.go index 30d61aec3..fea569381 100644 --- a/cmd/picoclaw/cmd_gateway.go +++ b/cmd/picoclaw/cmd_gateway.go @@ -211,10 +211,10 @@ func gatewayCmd() { <-sigChan fmt.Println("\nShutting down...") - cancel() - if cp, ok := provider.(providers.SessionProvider); ok { + if cp, ok := provider.(providers.StatefulProvider); ok { cp.Close() } + cancel() healthServer.Stop(context.Background()) deviceService.Stop() heartbeatService.Stop() diff --git a/pkg/providers/github_copilot_provider.go b/pkg/providers/github_copilot_provider.go index c69658b44..9210021e1 100644 --- a/pkg/providers/github_copilot_provider.go +++ b/pkg/providers/github_copilot_provider.go @@ -94,19 +94,24 @@ func (p *GitHubCopilotProvider) Chat( return nil, fmt.Errorf("marshal messages: %w", err) } p.mu.Lock() - defer p.mu.Unlock() + session := p.session + p.mu.Unlock() - resp, err := p.session.SendAndWait(ctx, copilot.MessageOptions{ + if session == nil { + return nil, fmt.Errorf("provider closed") + } + + resp, err := session.SendAndWait(ctx, copilot.MessageOptions{ Prompt: string(fullcontent), }) - if err != nil { - return nil, err - } - var content string - if resp != nil && resp.Data.Content != nil { - content = *resp.Data.Content + if resp == nil { + return nil, fmt.Errorf("empty response from copilot") } + if resp.Data.Content == nil { + return nil, fmt.Errorf("no content in copilot response") + } + content := *resp.Data.Content return &LLMResponse{ FinishReason: "stop", diff --git a/pkg/providers/types.go b/pkg/providers/types.go index 40ff6f7c8..b2dda04a5 100644 --- a/pkg/providers/types.go +++ b/pkg/providers/types.go @@ -30,7 +30,7 @@ type LLMProvider interface { GetDefaultModel() string } -type SessionProvider interface { +type StatefulProvider interface { LLMProvider Close() } diff --git a/pkg/tools/registry_test.go b/pkg/tools/registry_test.go index 8ae13b20c..33978e543 100644 --- a/pkg/tools/registry_test.go +++ b/pkg/tools/registry_test.go @@ -14,14 +14,14 @@ import ( type mockRegistryTool struct { name string desc string - params map[string]any + params map[string]interface{} result *ToolResult } -func (m *mockRegistryTool) Name() string { return m.name } -func (m *mockRegistryTool) Description() string { return m.desc } -func (m *mockRegistryTool) Parameters() map[string]any { return m.params } -func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]any) *ToolResult { +func (m *mockRegistryTool) Name() string { return m.name } +func (m *mockRegistryTool) Description() string { return m.desc } +func (m *mockRegistryTool) Parameters() map[string]interface{} { return m.params } +func (m *mockRegistryTool) Execute(_ context.Context, _ map[string]interface{}) *ToolResult { return m.result } @@ -51,7 +51,7 @@ func newMockTool(name, desc string) *mockRegistryTool { return &mockRegistryTool{ name: name, desc: desc, - params: map[string]any{"type": "object"}, + params: map[string]interface{}{"type": "object"}, result: SilentResult("ok"), } } @@ -109,7 +109,7 @@ func TestToolRegistry_Execute_Success(t *testing.T) { r.Register(&mockRegistryTool{ name: "greet", desc: "says hello", - params: map[string]any{}, + params: map[string]interface{}{}, result: SilentResult("hello"), }) @@ -203,7 +203,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { if defs[0]["type"] != "function" { t.Errorf("expected type 'function', got %v", defs[0]["type"]) } - fn, ok := defs[0]["function"].(map[string]any) + fn, ok := defs[0]["function"].(map[string]interface{}) if !ok { t.Fatal("expected 'function' key to be a map") } @@ -217,7 +217,7 @@ func TestToolRegistry_GetDefinitions(t *testing.T) { func TestToolRegistry_ToProviderDefs(t *testing.T) { r := NewToolRegistry() - params := map[string]any{"type": "object", "properties": map[string]any{}} + params := map[string]interface{}{"type": "object", "properties": map[string]interface{}{}} r.Register(&mockRegistryTool{ name: "beta", desc: "tool B", @@ -310,7 +310,7 @@ func TestToolToSchema(t *testing.T) { if schema["type"] != "function" { t.Errorf("expected type 'function', got %v", schema["type"]) } - fn, ok := schema["function"].(map[string]any) + fn, ok := schema["function"].(map[string]interface{}) if !ok { t.Fatal("expected 'function' to be a map") } From ec6da7a530ce5da48df422b38be697afad36192b Mon Sep 17 00:00:00 2001 From: Achton Smidt Winther Date: Tue, 24 Feb 2026 21:39:49 +0100 Subject: [PATCH 11/11] fix: reject empty task in spawn tool (#740) The spawn tool accepts empty strings as valid task arguments, which causes a subagent to run with no meaningful work. The subagent's completion message is then routed back to the originating channel (e.g. Signal, Discord), where the main agent processes it and may hallucinate an unrelated response that gets sent to users. Validate that the task parameter is non-empty after trimming whitespace. Related: #545 Co-authored-by: Claude Opus 4.6 --- pkg/tools/spawn.go | 5 +-- pkg/tools/spawn_test.go | 79 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 pkg/tools/spawn_test.go diff --git a/pkg/tools/spawn.go b/pkg/tools/spawn.go index 73d385cb0..8b166b41f 100644 --- a/pkg/tools/spawn.go +++ b/pkg/tools/spawn.go @@ -3,6 +3,7 @@ package tools import ( "context" "fmt" + "strings" ) type SpawnTool struct { @@ -66,8 +67,8 @@ func (t *SpawnTool) SetAllowlistChecker(check func(targetAgentID string) bool) { func (t *SpawnTool) Execute(ctx context.Context, args map[string]any) *ToolResult { task, ok := args["task"].(string) - if !ok { - return ErrorResult("task is required") + if !ok || strings.TrimSpace(task) == "" { + return ErrorResult("task is required and must be a non-empty string") } label, _ := args["label"].(string) diff --git a/pkg/tools/spawn_test.go b/pkg/tools/spawn_test.go new file mode 100644 index 000000000..0646c82a9 --- /dev/null +++ b/pkg/tools/spawn_test.go @@ -0,0 +1,79 @@ +package tools + +import ( + "context" + "strings" + "testing" +) + +func TestSpawnTool_Execute_EmptyTask(t *testing.T) { + provider := &MockLLMProvider{} + manager := NewSubagentManager(provider, "test-model", "/tmp/test", nil) + tool := NewSpawnTool(manager) + + ctx := context.Background() + + tests := []struct { + name string + args map[string]any + }{ + {"empty string", map[string]any{"task": ""}}, + {"whitespace only", map[string]any{"task": " "}}, + {"tabs and newlines", map[string]any{"task": "\t\n "}}, + {"missing task key", map[string]any{"label": "test"}}, + {"wrong type", map[string]any{"task": 123}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tool.Execute(ctx, tt.args) + if result == nil { + t.Fatal("Result should not be nil") + } + if !result.IsError { + t.Error("Expected error for invalid task parameter") + } + if !strings.Contains(result.ForLLM, "task is required") { + t.Errorf("Error message should mention 'task is required', got: %s", result.ForLLM) + } + }) + } +} + +func TestSpawnTool_Execute_ValidTask(t *testing.T) { + provider := &MockLLMProvider{} + manager := NewSubagentManager(provider, "test-model", "/tmp/test", nil) + tool := NewSpawnTool(manager) + + ctx := context.Background() + args := map[string]any{ + "task": "Write a haiku about coding", + "label": "haiku-task", + } + + result := tool.Execute(ctx, args) + if result == nil { + t.Fatal("Result should not be nil") + } + if result.IsError { + t.Errorf("Expected success for valid task, got error: %s", result.ForLLM) + } + if !result.Async { + t.Error("SpawnTool should return async result") + } +} + +func TestSpawnTool_Execute_NilManager(t *testing.T) { + tool := NewSpawnTool(nil) + + ctx := context.Background() + args := map[string]any{"task": "test task"} + + result := tool.Execute(ctx, args) + if !result.IsError { + t.Error("Expected error for nil manager") + } + if !strings.Contains(result.ForLLM, "Subagent manager not configured") { + t.Errorf("Error message should mention manager not configured, got: %s", result.ForLLM) + } +}