From 9d3f0afcc6486fd42bfcdbd379f611007aa2f6c5 Mon Sep 17 00:00:00 2001 From: Yiliu Date: Wed, 25 Feb 2026 14:02:24 +0800 Subject: [PATCH] refactor(providers): adopt functional options and preserve timeout migration --- pkg/config/config.go | 11 +- pkg/config/migration.go | 180 ++++++++++--------- pkg/config/migration_test.go | 21 +++ pkg/providers/http_provider.go | 7 +- pkg/providers/openai_compat/provider.go | 66 ++++--- pkg/providers/openai_compat/provider_test.go | 21 +++ 6 files changed, 195 insertions(+), 111 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 391d6a672..c92e46a1b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -371,11 +371,12 @@ func (p ProvidersConfig) MarshalJSON() ([]byte, error) { } type ProviderConfig struct { - APIKey string `json:"api_key" env:"PICOCLAW_PROVIDERS_{{.Name}}_API_KEY"` - APIBase string `json:"api_base" env:"PICOCLAW_PROVIDERS_{{.Name}}_API_BASE"` - Proxy string `json:"proxy,omitempty" env:"PICOCLAW_PROVIDERS_{{.Name}}_PROXY"` - AuthMethod string `json:"auth_method,omitempty" env:"PICOCLAW_PROVIDERS_{{.Name}}_AUTH_METHOD"` - ConnectMode string `json:"connect_mode,omitempty" env:"PICOCLAW_PROVIDERS_{{.Name}}_CONNECT_MODE"` // only for Github Copilot, `stdio` or `grpc` + APIKey string `json:"api_key" env:"PICOCLAW_PROVIDERS_{{.Name}}_API_KEY"` + APIBase string `json:"api_base" env:"PICOCLAW_PROVIDERS_{{.Name}}_API_BASE"` + Proxy string `json:"proxy,omitempty" env:"PICOCLAW_PROVIDERS_{{.Name}}_PROXY"` + RequestTimeout int `json:"request_timeout,omitempty" env:"PICOCLAW_PROVIDERS_{{.Name}}_REQUEST_TIMEOUT"` + AuthMethod string `json:"auth_method,omitempty" env:"PICOCLAW_PROVIDERS_{{.Name}}_AUTH_METHOD"` + ConnectMode string `json:"connect_mode,omitempty" env:"PICOCLAW_PROVIDERS_{{.Name}}_CONNECT_MODE"` // only for Github Copilot, `stdio` or `grpc` } type OpenAIProviderConfig struct { diff --git a/pkg/config/migration.go b/pkg/config/migration.go index 70e1de438..5deb09270 100644 --- a/pkg/config/migration.go +++ b/pkg/config/migration.go @@ -60,12 +60,13 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "openai", - Model: "openai/gpt-5.2", - APIKey: p.OpenAI.APIKey, - APIBase: p.OpenAI.APIBase, - Proxy: p.OpenAI.Proxy, - AuthMethod: p.OpenAI.AuthMethod, + ModelName: "openai", + Model: "openai/gpt-5.2", + APIKey: p.OpenAI.APIKey, + APIBase: p.OpenAI.APIBase, + Proxy: p.OpenAI.Proxy, + RequestTimeout: p.OpenAI.RequestTimeout, + AuthMethod: p.OpenAI.AuthMethod, }, true }, }, @@ -77,12 +78,13 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "anthropic", - Model: "anthropic/claude-sonnet-4.6", - APIKey: p.Anthropic.APIKey, - APIBase: p.Anthropic.APIBase, - Proxy: p.Anthropic.Proxy, - AuthMethod: p.Anthropic.AuthMethod, + ModelName: "anthropic", + Model: "anthropic/claude-sonnet-4.6", + APIKey: p.Anthropic.APIKey, + APIBase: p.Anthropic.APIBase, + Proxy: p.Anthropic.Proxy, + RequestTimeout: p.Anthropic.RequestTimeout, + AuthMethod: p.Anthropic.AuthMethod, }, true }, }, @@ -94,11 +96,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "openrouter", - Model: "openrouter/auto", - APIKey: p.OpenRouter.APIKey, - APIBase: p.OpenRouter.APIBase, - Proxy: p.OpenRouter.Proxy, + ModelName: "openrouter", + Model: "openrouter/auto", + APIKey: p.OpenRouter.APIKey, + APIBase: p.OpenRouter.APIBase, + Proxy: p.OpenRouter.Proxy, + RequestTimeout: p.OpenRouter.RequestTimeout, }, true }, }, @@ -110,11 +113,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "groq", - Model: "groq/llama-3.1-70b-versatile", - APIKey: p.Groq.APIKey, - APIBase: p.Groq.APIBase, - Proxy: p.Groq.Proxy, + ModelName: "groq", + Model: "groq/llama-3.1-70b-versatile", + APIKey: p.Groq.APIKey, + APIBase: p.Groq.APIBase, + Proxy: p.Groq.Proxy, + RequestTimeout: p.Groq.RequestTimeout, }, true }, }, @@ -126,11 +130,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "zhipu", - Model: "zhipu/glm-4", - APIKey: p.Zhipu.APIKey, - APIBase: p.Zhipu.APIBase, - Proxy: p.Zhipu.Proxy, + ModelName: "zhipu", + Model: "zhipu/glm-4", + APIKey: p.Zhipu.APIKey, + APIBase: p.Zhipu.APIBase, + Proxy: p.Zhipu.Proxy, + RequestTimeout: p.Zhipu.RequestTimeout, }, true }, }, @@ -142,11 +147,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "vllm", - Model: "vllm/auto", - APIKey: p.VLLM.APIKey, - APIBase: p.VLLM.APIBase, - Proxy: p.VLLM.Proxy, + ModelName: "vllm", + Model: "vllm/auto", + APIKey: p.VLLM.APIKey, + APIBase: p.VLLM.APIBase, + Proxy: p.VLLM.Proxy, + RequestTimeout: p.VLLM.RequestTimeout, }, true }, }, @@ -158,11 +164,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "gemini", - Model: "gemini/gemini-pro", - APIKey: p.Gemini.APIKey, - APIBase: p.Gemini.APIBase, - Proxy: p.Gemini.Proxy, + ModelName: "gemini", + Model: "gemini/gemini-pro", + APIKey: p.Gemini.APIKey, + APIBase: p.Gemini.APIBase, + Proxy: p.Gemini.Proxy, + RequestTimeout: p.Gemini.RequestTimeout, }, true }, }, @@ -174,11 +181,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "nvidia", - Model: "nvidia/meta/llama-3.1-8b-instruct", - APIKey: p.Nvidia.APIKey, - APIBase: p.Nvidia.APIBase, - Proxy: p.Nvidia.Proxy, + ModelName: "nvidia", + Model: "nvidia/meta/llama-3.1-8b-instruct", + APIKey: p.Nvidia.APIKey, + APIBase: p.Nvidia.APIBase, + Proxy: p.Nvidia.Proxy, + RequestTimeout: p.Nvidia.RequestTimeout, }, true }, }, @@ -190,11 +198,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "ollama", - Model: "ollama/llama3", - APIKey: p.Ollama.APIKey, - APIBase: p.Ollama.APIBase, - Proxy: p.Ollama.Proxy, + ModelName: "ollama", + Model: "ollama/llama3", + APIKey: p.Ollama.APIKey, + APIBase: p.Ollama.APIBase, + Proxy: p.Ollama.Proxy, + RequestTimeout: p.Ollama.RequestTimeout, }, true }, }, @@ -206,11 +215,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "moonshot", - Model: "moonshot/kimi", - APIKey: p.Moonshot.APIKey, - APIBase: p.Moonshot.APIBase, - Proxy: p.Moonshot.Proxy, + ModelName: "moonshot", + Model: "moonshot/kimi", + APIKey: p.Moonshot.APIKey, + APIBase: p.Moonshot.APIBase, + Proxy: p.Moonshot.Proxy, + RequestTimeout: p.Moonshot.RequestTimeout, }, true }, }, @@ -222,11 +232,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "shengsuanyun", - Model: "shengsuanyun/auto", - APIKey: p.ShengSuanYun.APIKey, - APIBase: p.ShengSuanYun.APIBase, - Proxy: p.ShengSuanYun.Proxy, + ModelName: "shengsuanyun", + Model: "shengsuanyun/auto", + APIKey: p.ShengSuanYun.APIKey, + APIBase: p.ShengSuanYun.APIBase, + Proxy: p.ShengSuanYun.Proxy, + RequestTimeout: p.ShengSuanYun.RequestTimeout, }, true }, }, @@ -238,11 +249,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "deepseek", - Model: "deepseek/deepseek-chat", - APIKey: p.DeepSeek.APIKey, - APIBase: p.DeepSeek.APIBase, - Proxy: p.DeepSeek.Proxy, + ModelName: "deepseek", + Model: "deepseek/deepseek-chat", + APIKey: p.DeepSeek.APIKey, + APIBase: p.DeepSeek.APIBase, + Proxy: p.DeepSeek.Proxy, + RequestTimeout: p.DeepSeek.RequestTimeout, }, true }, }, @@ -254,11 +266,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "cerebras", - Model: "cerebras/llama-3.3-70b", - APIKey: p.Cerebras.APIKey, - APIBase: p.Cerebras.APIBase, - Proxy: p.Cerebras.Proxy, + ModelName: "cerebras", + Model: "cerebras/llama-3.3-70b", + APIKey: p.Cerebras.APIKey, + APIBase: p.Cerebras.APIBase, + Proxy: p.Cerebras.Proxy, + RequestTimeout: p.Cerebras.RequestTimeout, }, true }, }, @@ -270,11 +283,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "volcengine", - Model: "volcengine/doubao-pro", - APIKey: p.VolcEngine.APIKey, - APIBase: p.VolcEngine.APIBase, - Proxy: p.VolcEngine.Proxy, + ModelName: "volcengine", + Model: "volcengine/doubao-pro", + APIKey: p.VolcEngine.APIKey, + APIBase: p.VolcEngine.APIBase, + Proxy: p.VolcEngine.Proxy, + RequestTimeout: p.VolcEngine.RequestTimeout, }, true }, }, @@ -316,11 +330,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "qwen", - Model: "qwen/qwen-max", - APIKey: p.Qwen.APIKey, - APIBase: p.Qwen.APIBase, - Proxy: p.Qwen.Proxy, + ModelName: "qwen", + Model: "qwen/qwen-max", + APIKey: p.Qwen.APIKey, + APIBase: p.Qwen.APIBase, + Proxy: p.Qwen.Proxy, + RequestTimeout: p.Qwen.RequestTimeout, }, true }, }, @@ -332,11 +347,12 @@ func ConvertProvidersToModelList(cfg *Config) []ModelConfig { return ModelConfig{}, false } return ModelConfig{ - ModelName: "mistral", - Model: "mistral/mistral-small-latest", - APIKey: p.Mistral.APIKey, - APIBase: p.Mistral.APIBase, - Proxy: p.Mistral.Proxy, + ModelName: "mistral", + Model: "mistral/mistral-small-latest", + APIKey: p.Mistral.APIKey, + APIBase: p.Mistral.APIBase, + Proxy: p.Mistral.Proxy, + RequestTimeout: p.Mistral.RequestTimeout, }, true }, }, diff --git a/pkg/config/migration_test.go b/pkg/config/migration_test.go index 42165cb71..db8f4657d 100644 --- a/pkg/config/migration_test.go +++ b/pkg/config/migration_test.go @@ -166,6 +166,27 @@ func TestConvertProvidersToModelList_Proxy(t *testing.T) { } } +func TestConvertProvidersToModelList_RequestTimeout(t *testing.T) { + cfg := &Config{ + Providers: ProvidersConfig{ + Ollama: ProviderConfig{ + APIKey: "ollama-key", + RequestTimeout: 300, + }, + }, + } + + result := ConvertProvidersToModelList(cfg) + + if len(result) != 1 { + t.Fatalf("len(result) = %d, want 1", len(result)) + } + + if result[0].RequestTimeout != 300 { + t.Errorf("RequestTimeout = %d, want %d", result[0].RequestTimeout, 300) + } +} + func TestConvertProvidersToModelList_AuthMethod(t *testing.T) { cfg := &Config{ Providers: ProvidersConfig{ diff --git a/pkg/providers/http_provider.go b/pkg/providers/http_provider.go index 7631734d7..5c328f418 100644 --- a/pkg/providers/http_provider.go +++ b/pkg/providers/http_provider.go @@ -8,6 +8,7 @@ package providers import ( "context" + "time" "github.com/sipeed/picoclaw/pkg/providers/openai_compat" ) @@ -31,12 +32,12 @@ func NewHTTPProviderWithMaxTokensFieldAndRequestTimeout( requestTimeoutSeconds int, ) *HTTPProvider { return &HTTPProvider{ - delegate: openai_compat.NewProviderWithMaxTokensFieldAndTimeout( + delegate: openai_compat.NewProvider( apiKey, apiBase, proxy, - maxTokensField, - requestTimeoutSeconds, + openai_compat.WithMaxTokensField(maxTokensField), + openai_compat.WithRequestTimeout(time.Duration(requestTimeoutSeconds)*time.Second), ), } } diff --git a/pkg/providers/openai_compat/provider.go b/pkg/providers/openai_compat/provider.go index 0af14b49f..3aa81b303 100644 --- a/pkg/providers/openai_compat/provider.go +++ b/pkg/providers/openai_compat/provider.go @@ -34,27 +34,27 @@ type Provider struct { httpClient *http.Client } +type Option func(*Provider) + const defaultRequestTimeout = 120 * time.Second -func NewProvider(apiKey, apiBase, proxy string) *Provider { - return NewProviderWithMaxTokensField(apiKey, apiBase, proxy, "") -} - -func NewProviderWithMaxTokensField(apiKey, apiBase, proxy, maxTokensField string) *Provider { - return NewProviderWithMaxTokensFieldAndTimeout(apiKey, apiBase, proxy, maxTokensField, 0) -} - -func NewProviderWithMaxTokensFieldAndTimeout( - apiKey, apiBase, proxy, maxTokensField string, - requestTimeoutSeconds int, -) *Provider { - timeout := defaultRequestTimeout - if requestTimeoutSeconds > 0 { - timeout = time.Duration(requestTimeoutSeconds) * time.Second +func WithMaxTokensField(maxTokensField string) Option { + return func(p *Provider) { + p.maxTokensField = maxTokensField } +} +func WithRequestTimeout(timeout time.Duration) Option { + return func(p *Provider) { + if timeout > 0 { + p.httpClient.Timeout = timeout + } + } +} + +func NewProvider(apiKey, apiBase, proxy string, opts ...Option) *Provider { client := &http.Client{ - Timeout: timeout, + Timeout: defaultRequestTimeout, } if proxy != "" { @@ -68,12 +68,36 @@ func NewProviderWithMaxTokensFieldAndTimeout( } } - return &Provider{ - apiKey: apiKey, - apiBase: strings.TrimRight(apiBase, "/"), - maxTokensField: maxTokensField, - httpClient: client, + p := &Provider{ + apiKey: apiKey, + apiBase: strings.TrimRight(apiBase, "/"), + httpClient: client, } + + for _, opt := range opts { + if opt != nil { + opt(p) + } + } + + return p +} + +func NewProviderWithMaxTokensField(apiKey, apiBase, proxy, maxTokensField string) *Provider { + return NewProvider(apiKey, apiBase, proxy, WithMaxTokensField(maxTokensField)) +} + +func NewProviderWithMaxTokensFieldAndTimeout( + apiKey, apiBase, proxy, maxTokensField string, + requestTimeoutSeconds int, +) *Provider { + return NewProvider( + apiKey, + apiBase, + proxy, + WithMaxTokensField(maxTokensField), + WithRequestTimeout(time.Duration(requestTimeoutSeconds)*time.Second), + ) } func (p *Provider) Chat( diff --git a/pkg/providers/openai_compat/provider_test.go b/pkg/providers/openai_compat/provider_test.go index 25ee7ff87..7247fea3e 100644 --- a/pkg/providers/openai_compat/provider_test.go +++ b/pkg/providers/openai_compat/provider_test.go @@ -340,3 +340,24 @@ func TestProvider_RequestTimeoutOverride(t *testing.T) { t.Fatalf("http timeout = %v, want %v", p.httpClient.Timeout, 300*time.Second) } } + +func TestProvider_FunctionalOptionMaxTokensField(t *testing.T) { + p := NewProvider("key", "https://example.com/v1", "", WithMaxTokensField("max_completion_tokens")) + if p.maxTokensField != "max_completion_tokens" { + t.Fatalf("maxTokensField = %q, want %q", p.maxTokensField, "max_completion_tokens") + } +} + +func TestProvider_FunctionalOptionRequestTimeout(t *testing.T) { + p := NewProvider("key", "https://example.com/v1", "", WithRequestTimeout(45*time.Second)) + if p.httpClient.Timeout != 45*time.Second { + t.Fatalf("http timeout = %v, want %v", p.httpClient.Timeout, 45*time.Second) + } +} + +func TestProvider_FunctionalOptionRequestTimeoutNonPositive(t *testing.T) { + p := NewProvider("key", "https://example.com/v1", "", WithRequestTimeout(-1*time.Second)) + if p.httpClient.Timeout != defaultRequestTimeout { + t.Fatalf("http timeout = %v, want %v", p.httpClient.Timeout, defaultRequestTimeout) + } +}