From 6f72f0f528205c2478baf28588ef68d645f54f2f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 26 Feb 2026 15:09:15 +0000 Subject: [PATCH] refactor: reduce code duplication across providers, status, tools, and auth - Extract applyProviderConfig() helper to eliminate repeated apiKey/apiBase/proxy assignment pattern in factory.go (reduced from 323 to 223 lines) - Replace repetitive provider status checks with data-driven loops in status display - Deduplicate filesystem tool constructors via shared newFileSystem() helper - Unify model type-check functions (isAntigravityModel, isOpenAIModel, isAnthropicModel) with generic isProviderModel() helper - Fix bug in cronSetJobEnabled() that always printed "enabled" regardless of the actual enabled/disabled state https://claude.ai/code/session_01M1YgMhxq3coXK2hGmUvfFX --- cmd/picoclaw/internal/auth/helpers.go | 25 ++-- cmd/picoclaw/internal/cron/helpers.go | 6 +- cmd/picoclaw/internal/status/helpers.go | 71 +++++----- pkg/providers/factory.go | 169 +++++------------------- pkg/tools/filesystem.go | 30 ++--- 5 files changed, 97 insertions(+), 204 deletions(-) diff --git a/cmd/picoclaw/internal/auth/helpers.go b/cmd/picoclaw/internal/auth/helpers.go index 633ce8740..5ddf43939 100644 --- a/cmd/picoclaw/internal/auth/helpers.go +++ b/cmd/picoclaw/internal/auth/helpers.go @@ -416,22 +416,25 @@ func authModelsCmd() error { return nil } -// isAntigravityModel checks if a model string belongs to antigravity provider +// isProviderModel checks if a model string matches any of the given provider prefixes. +// It returns true if the model equals a prefix or starts with "prefix/". +func isProviderModel(model string, providers ...string) bool { + for _, p := range providers { + if model == p || strings.HasPrefix(model, p+"/") { + return true + } + } + return false +} + func isAntigravityModel(model string) bool { - return model == "antigravity" || - model == "google-antigravity" || - strings.HasPrefix(model, "antigravity/") || - strings.HasPrefix(model, "google-antigravity/") + return isProviderModel(model, "antigravity", "google-antigravity") } -// isOpenAIModel checks if a model string belongs to openai provider func isOpenAIModel(model string) bool { - return model == "openai" || - strings.HasPrefix(model, "openai/") + return isProviderModel(model, "openai") } -// isAnthropicModel checks if a model string belongs to anthropic provider func isAnthropicModel(model string) bool { - return model == "anthropic" || - strings.HasPrefix(model, "anthropic/") + return isProviderModel(model, "anthropic") } diff --git a/cmd/picoclaw/internal/cron/helpers.go b/cmd/picoclaw/internal/cron/helpers.go index 88bdf1bf7..8fcf4f90f 100644 --- a/cmd/picoclaw/internal/cron/helpers.go +++ b/cmd/picoclaw/internal/cron/helpers.go @@ -59,7 +59,11 @@ func cronSetJobEnabled(storePath, jobID string, enabled bool) { cs := cron.NewCronService(storePath, nil) job := cs.EnableJob(jobID, enabled) if job != nil { - fmt.Printf("✓ Job '%s' enabled\n", job.Name) + status := "enabled" + if !enabled { + status = "disabled" + } + fmt.Printf("✓ Job '%s' %s\n", job.Name, status) } else { fmt.Printf("✗ Job %s not found\n", jobID) } diff --git a/cmd/picoclaw/internal/status/helpers.go b/cmd/picoclaw/internal/status/helpers.go index ab28f4885..a54069048 100644 --- a/cmd/picoclaw/internal/status/helpers.go +++ b/cmd/picoclaw/internal/status/helpers.go @@ -41,46 +41,43 @@ func statusCmd() { if _, err := os.Stat(configPath); err == nil { fmt.Printf("Model: %s\n", cfg.Agents.Defaults.GetModelName()) - hasOpenRouter := cfg.Providers.OpenRouter.APIKey != "" - hasAnthropic := cfg.Providers.Anthropic.APIKey != "" - hasOpenAI := cfg.Providers.OpenAI.APIKey != "" - hasGemini := cfg.Providers.Gemini.APIKey != "" - hasZhipu := cfg.Providers.Zhipu.APIKey != "" - hasQwen := cfg.Providers.Qwen.APIKey != "" - hasGroq := cfg.Providers.Groq.APIKey != "" - hasVLLM := cfg.Providers.VLLM.APIBase != "" - hasMoonshot := cfg.Providers.Moonshot.APIKey != "" - hasDeepSeek := cfg.Providers.DeepSeek.APIKey != "" - hasVolcEngine := cfg.Providers.VolcEngine.APIKey != "" - hasNvidia := cfg.Providers.Nvidia.APIKey != "" - hasOllama := cfg.Providers.Ollama.APIBase != "" - - status := func(enabled bool) string { - if enabled { - return "✓" + apiKeyProviders := []struct { + name string + hasKey bool + }{ + {"OpenRouter API", cfg.Providers.OpenRouter.APIKey != ""}, + {"Anthropic API", cfg.Providers.Anthropic.APIKey != ""}, + {"OpenAI API", cfg.Providers.OpenAI.APIKey != ""}, + {"Gemini API", cfg.Providers.Gemini.APIKey != ""}, + {"Zhipu API", cfg.Providers.Zhipu.APIKey != ""}, + {"Qwen API", cfg.Providers.Qwen.APIKey != ""}, + {"Groq API", cfg.Providers.Groq.APIKey != ""}, + {"Moonshot API", cfg.Providers.Moonshot.APIKey != ""}, + {"DeepSeek API", cfg.Providers.DeepSeek.APIKey != ""}, + {"VolcEngine API", cfg.Providers.VolcEngine.APIKey != ""}, + {"Nvidia API", cfg.Providers.Nvidia.APIKey != ""}, + } + for _, p := range apiKeyProviders { + if p.hasKey { + fmt.Printf("%s: ✓\n", p.name) + } else { + fmt.Printf("%s: not set\n", p.name) } - return "not set" } - fmt.Println("OpenRouter API:", status(hasOpenRouter)) - fmt.Println("Anthropic API:", status(hasAnthropic)) - fmt.Println("OpenAI API:", status(hasOpenAI)) - fmt.Println("Gemini API:", status(hasGemini)) - fmt.Println("Zhipu API:", status(hasZhipu)) - fmt.Println("Qwen API:", status(hasQwen)) - fmt.Println("Groq API:", status(hasGroq)) - fmt.Println("Moonshot API:", status(hasMoonshot)) - fmt.Println("DeepSeek API:", status(hasDeepSeek)) - fmt.Println("VolcEngine API:", status(hasVolcEngine)) - fmt.Println("Nvidia API:", status(hasNvidia)) - if hasVLLM { - fmt.Printf("vLLM/Local: ✓ %s\n", cfg.Providers.VLLM.APIBase) - } else { - fmt.Println("vLLM/Local: not set") + + urlProviders := []struct { + name string + apiBase string + }{ + {"vLLM/Local", cfg.Providers.VLLM.APIBase}, + {"Ollama", cfg.Providers.Ollama.APIBase}, } - if hasOllama { - fmt.Printf("Ollama: ✓ %s\n", cfg.Providers.Ollama.APIBase) - } else { - fmt.Println("Ollama: not set") + for _, p := range urlProviders { + if p.apiBase != "" { + fmt.Printf("%s: ✓ %s\n", p.name, p.apiBase) + } else { + fmt.Printf("%s: not set\n", p.name) + } } store, _ := auth.LoadStore() diff --git a/pkg/providers/factory.go b/pkg/providers/factory.go index 11af14da4..d28ca145f 100644 --- a/pkg/providers/factory.go +++ b/pkg/providers/factory.go @@ -35,6 +35,17 @@ type providerSelection struct { enableWebSearch bool } +// applyProviderConfig copies the standard provider config fields into the selection. +// If the resolved apiBase is empty, defaultBase is used as fallback. +func applyProviderConfig(sel *providerSelection, pc config.ProviderConfig, defaultBase string) { + sel.apiKey = pc.APIKey + sel.apiBase = pc.APIBase + sel.proxy = pc.Proxy + if sel.apiBase == "" && defaultBase != "" { + sel.apiBase = defaultBase + } +} + func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { model := cfg.Agents.Defaults.GetModelName() providerName := strings.ToLower(cfg.Agents.Defaults.Provider) @@ -50,12 +61,7 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { switch providerName { case "groq": if cfg.Providers.Groq.APIKey != "" { - sel.apiKey = cfg.Providers.Groq.APIKey - sel.apiBase = cfg.Providers.Groq.APIBase - sel.proxy = cfg.Providers.Groq.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.groq.com/openai/v1" - } + applyProviderConfig(&sel, cfg.Providers.Groq, "https://api.groq.com/openai/v1") } case "openai", "gpt": if cfg.Providers.OpenAI.APIKey != "" || cfg.Providers.OpenAI.AuthMethod != "" { @@ -68,12 +74,7 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { sel.providerType = providerTypeCodexAuth return sel, nil } - sel.apiKey = cfg.Providers.OpenAI.APIKey - sel.apiBase = cfg.Providers.OpenAI.APIBase - sel.proxy = cfg.Providers.OpenAI.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.openai.com/v1" - } + applyProviderConfig(&sel, cfg.Providers.OpenAI.ProviderConfig, "https://api.openai.com/v1") } case "anthropic", "claude": if cfg.Providers.Anthropic.APIKey != "" || cfg.Providers.Anthropic.AuthMethod != "" { @@ -85,64 +86,31 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { sel.providerType = providerTypeClaudeAuth return sel, nil } - sel.apiKey = cfg.Providers.Anthropic.APIKey - sel.apiBase = cfg.Providers.Anthropic.APIBase - sel.proxy = cfg.Providers.Anthropic.Proxy - if sel.apiBase == "" { - sel.apiBase = defaultAnthropicAPIBase - } + applyProviderConfig(&sel, cfg.Providers.Anthropic, defaultAnthropicAPIBase) } case "openrouter": if cfg.Providers.OpenRouter.APIKey != "" { - sel.apiKey = cfg.Providers.OpenRouter.APIKey - sel.proxy = cfg.Providers.OpenRouter.Proxy - if cfg.Providers.OpenRouter.APIBase != "" { - sel.apiBase = cfg.Providers.OpenRouter.APIBase - } else { - sel.apiBase = "https://openrouter.ai/api/v1" - } + applyProviderConfig(&sel, cfg.Providers.OpenRouter, "https://openrouter.ai/api/v1") } case "zhipu", "glm": if cfg.Providers.Zhipu.APIKey != "" { - sel.apiKey = cfg.Providers.Zhipu.APIKey - sel.apiBase = cfg.Providers.Zhipu.APIBase - sel.proxy = cfg.Providers.Zhipu.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://open.bigmodel.cn/api/paas/v4" - } + applyProviderConfig(&sel, cfg.Providers.Zhipu, "https://open.bigmodel.cn/api/paas/v4") } case "gemini", "google": if cfg.Providers.Gemini.APIKey != "" { - sel.apiKey = cfg.Providers.Gemini.APIKey - sel.apiBase = cfg.Providers.Gemini.APIBase - sel.proxy = cfg.Providers.Gemini.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://generativelanguage.googleapis.com/v1beta" - } + applyProviderConfig(&sel, cfg.Providers.Gemini, "https://generativelanguage.googleapis.com/v1beta") } case "vllm": if cfg.Providers.VLLM.APIBase != "" { - sel.apiKey = cfg.Providers.VLLM.APIKey - sel.apiBase = cfg.Providers.VLLM.APIBase - sel.proxy = cfg.Providers.VLLM.Proxy + applyProviderConfig(&sel, cfg.Providers.VLLM, "") } case "shengsuanyun": if cfg.Providers.ShengSuanYun.APIKey != "" { - sel.apiKey = cfg.Providers.ShengSuanYun.APIKey - sel.apiBase = cfg.Providers.ShengSuanYun.APIBase - sel.proxy = cfg.Providers.ShengSuanYun.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://router.shengsuanyun.com/api/v1" - } + applyProviderConfig(&sel, cfg.Providers.ShengSuanYun, "https://router.shengsuanyun.com/api/v1") } case "nvidia": if cfg.Providers.Nvidia.APIKey != "" { - sel.apiKey = cfg.Providers.Nvidia.APIKey - sel.apiBase = cfg.Providers.Nvidia.APIBase - sel.proxy = cfg.Providers.Nvidia.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://integrate.api.nvidia.com/v1" - } + applyProviderConfig(&sel, cfg.Providers.Nvidia, "https://integrate.api.nvidia.com/v1") } case "claude-cli", "claude-code", "claudecode": workspace := cfg.WorkspacePath() @@ -162,24 +130,14 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { return sel, nil case "deepseek": if cfg.Providers.DeepSeek.APIKey != "" { - sel.apiKey = cfg.Providers.DeepSeek.APIKey - sel.apiBase = cfg.Providers.DeepSeek.APIBase - sel.proxy = cfg.Providers.DeepSeek.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.deepseek.com/v1" - } + applyProviderConfig(&sel, cfg.Providers.DeepSeek, "https://api.deepseek.com/v1") if model != "deepseek-chat" && model != "deepseek-reasoner" { sel.model = "deepseek-chat" } } case "mistral": if cfg.Providers.Mistral.APIKey != "" { - sel.apiKey = cfg.Providers.Mistral.APIKey - sel.apiBase = cfg.Providers.Mistral.APIBase - sel.proxy = cfg.Providers.Mistral.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.mistral.ai/v1" - } + applyProviderConfig(&sel, cfg.Providers.Mistral, "https://api.mistral.ai/v1") } case "github_copilot", "copilot": sel.providerType = providerTypeGitHubCopilot @@ -197,25 +155,14 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { if sel.apiKey == "" && sel.apiBase == "" { switch { case (strings.Contains(lowerModel, "kimi") || strings.Contains(lowerModel, "moonshot") || strings.HasPrefix(model, "moonshot/")) && cfg.Providers.Moonshot.APIKey != "": - sel.apiKey = cfg.Providers.Moonshot.APIKey - sel.apiBase = cfg.Providers.Moonshot.APIBase - sel.proxy = cfg.Providers.Moonshot.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.moonshot.cn/v1" - } + applyProviderConfig(&sel, cfg.Providers.Moonshot, "https://api.moonshot.cn/v1") case strings.HasPrefix(model, "openrouter/") || strings.HasPrefix(model, "anthropic/") || strings.HasPrefix(model, "openai/") || strings.HasPrefix(model, "meta-llama/") || strings.HasPrefix(model, "deepseek/") || strings.HasPrefix(model, "google/"): - sel.apiKey = cfg.Providers.OpenRouter.APIKey - sel.proxy = cfg.Providers.OpenRouter.Proxy - if cfg.Providers.OpenRouter.APIBase != "" { - sel.apiBase = cfg.Providers.OpenRouter.APIBase - } else { - sel.apiBase = "https://openrouter.ai/api/v1" - } + applyProviderConfig(&sel, cfg.Providers.OpenRouter, "https://openrouter.ai/api/v1") case (strings.Contains(lowerModel, "claude") || strings.HasPrefix(model, "anthropic/")) && (cfg.Providers.Anthropic.APIKey != "" || cfg.Providers.Anthropic.AuthMethod != ""): if cfg.Providers.Anthropic.AuthMethod == "oauth" || cfg.Providers.Anthropic.AuthMethod == "token" { @@ -226,12 +173,7 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { sel.providerType = providerTypeClaudeAuth return sel, nil } - sel.apiKey = cfg.Providers.Anthropic.APIKey - sel.apiBase = cfg.Providers.Anthropic.APIBase - sel.proxy = cfg.Providers.Anthropic.Proxy - if sel.apiBase == "" { - sel.apiBase = defaultAnthropicAPIBase - } + applyProviderConfig(&sel, cfg.Providers.Anthropic, defaultAnthropicAPIBase) case (strings.Contains(lowerModel, "gpt") || strings.HasPrefix(model, "openai/")) && (cfg.Providers.OpenAI.APIKey != "" || cfg.Providers.OpenAI.AuthMethod != ""): sel.enableWebSearch = cfg.Providers.OpenAI.WebSearch @@ -243,67 +185,24 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) { sel.providerType = providerTypeCodexAuth return sel, nil } - sel.apiKey = cfg.Providers.OpenAI.APIKey - sel.apiBase = cfg.Providers.OpenAI.APIBase - sel.proxy = cfg.Providers.OpenAI.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.openai.com/v1" - } + applyProviderConfig(&sel, cfg.Providers.OpenAI.ProviderConfig, "https://api.openai.com/v1") case (strings.Contains(lowerModel, "gemini") || strings.HasPrefix(model, "google/")) && cfg.Providers.Gemini.APIKey != "": - sel.apiKey = cfg.Providers.Gemini.APIKey - sel.apiBase = cfg.Providers.Gemini.APIBase - sel.proxy = cfg.Providers.Gemini.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://generativelanguage.googleapis.com/v1beta" - } + applyProviderConfig(&sel, cfg.Providers.Gemini, "https://generativelanguage.googleapis.com/v1beta") case (strings.Contains(lowerModel, "glm") || strings.Contains(lowerModel, "zhipu") || strings.Contains(lowerModel, "zai")) && cfg.Providers.Zhipu.APIKey != "": - sel.apiKey = cfg.Providers.Zhipu.APIKey - sel.apiBase = cfg.Providers.Zhipu.APIBase - sel.proxy = cfg.Providers.Zhipu.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://open.bigmodel.cn/api/paas/v4" - } + applyProviderConfig(&sel, cfg.Providers.Zhipu, "https://open.bigmodel.cn/api/paas/v4") case (strings.Contains(lowerModel, "groq") || strings.HasPrefix(model, "groq/")) && cfg.Providers.Groq.APIKey != "": - sel.apiKey = cfg.Providers.Groq.APIKey - sel.apiBase = cfg.Providers.Groq.APIBase - sel.proxy = cfg.Providers.Groq.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.groq.com/openai/v1" - } + applyProviderConfig(&sel, cfg.Providers.Groq, "https://api.groq.com/openai/v1") case (strings.Contains(lowerModel, "nvidia") || strings.HasPrefix(model, "nvidia/")) && cfg.Providers.Nvidia.APIKey != "": - sel.apiKey = cfg.Providers.Nvidia.APIKey - sel.apiBase = cfg.Providers.Nvidia.APIBase - sel.proxy = cfg.Providers.Nvidia.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://integrate.api.nvidia.com/v1" - } + applyProviderConfig(&sel, cfg.Providers.Nvidia, "https://integrate.api.nvidia.com/v1") case (strings.Contains(lowerModel, "ollama") || strings.HasPrefix(model, "ollama/")) && cfg.Providers.Ollama.APIKey != "": - sel.apiKey = cfg.Providers.Ollama.APIKey - sel.apiBase = cfg.Providers.Ollama.APIBase - sel.proxy = cfg.Providers.Ollama.Proxy - if sel.apiBase == "" { - sel.apiBase = "http://localhost:11434/v1" - } + applyProviderConfig(&sel, cfg.Providers.Ollama, "http://localhost:11434/v1") case (strings.Contains(lowerModel, "mistral") || strings.HasPrefix(model, "mistral/")) && cfg.Providers.Mistral.APIKey != "": - sel.apiKey = cfg.Providers.Mistral.APIKey - sel.apiBase = cfg.Providers.Mistral.APIBase - sel.proxy = cfg.Providers.Mistral.Proxy - if sel.apiBase == "" { - sel.apiBase = "https://api.mistral.ai/v1" - } + applyProviderConfig(&sel, cfg.Providers.Mistral, "https://api.mistral.ai/v1") case cfg.Providers.VLLM.APIBase != "": - sel.apiKey = cfg.Providers.VLLM.APIKey - sel.apiBase = cfg.Providers.VLLM.APIBase - sel.proxy = cfg.Providers.VLLM.Proxy + applyProviderConfig(&sel, cfg.Providers.VLLM, "") default: if cfg.Providers.OpenRouter.APIKey != "" { - sel.apiKey = cfg.Providers.OpenRouter.APIKey - sel.proxy = cfg.Providers.OpenRouter.Proxy - if cfg.Providers.OpenRouter.APIBase != "" { - sel.apiBase = cfg.Providers.OpenRouter.APIBase - } else { - sel.apiBase = "https://openrouter.ai/api/v1" - } + applyProviderConfig(&sel, cfg.Providers.OpenRouter, "https://openrouter.ai/api/v1") } else { return providerSelection{}, fmt.Errorf("no API key configured for model: %s", model) } diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index e3a167ccf..fd3223f14 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -85,14 +85,16 @@ type ReadFileTool struct { fs fileSystem } -func NewReadFileTool(workspace string, restrict bool) *ReadFileTool { - var fs fileSystem +// newFileSystem creates the appropriate fileSystem based on sandbox settings. +func newFileSystem(workspace string, restrict bool) fileSystem { if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} + return &sandboxFs{workspace: workspace} } - return &ReadFileTool{fs: fs} + return &hostFs{} +} + +func NewReadFileTool(workspace string, restrict bool) *ReadFileTool { + return &ReadFileTool{fs: newFileSystem(workspace, restrict)} } func (t *ReadFileTool) Name() string { @@ -138,13 +140,7 @@ type WriteFileTool struct { } func NewWriteFileTool(workspace string, restrict bool) *WriteFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} - } - return &WriteFileTool{fs: fs} + return &WriteFileTool{fs: newFileSystem(workspace, restrict)} } func (t *WriteFileTool) Name() string { @@ -195,13 +191,7 @@ type ListDirTool struct { } func NewListDirTool(workspace string, restrict bool) *ListDirTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} - } - return &ListDirTool{fs: fs} + return &ListDirTool{fs: newFileSystem(workspace, restrict)} } func (t *ListDirTool) Name() string {