From c1d0b376759b1912c71941ce6e83e80e2da91559 Mon Sep 17 00:00:00 2001 From: Equent Date: Sat, 28 Mar 2026 22:01:29 +0800 Subject: [PATCH] fix(providers): narrow responses routing to gpt-5 Keep automatic /responses routing limited to gpt-5 models so OpenAI-compatible endpoints continue to use chat/completions by default, and document the shared request-body conversion path used with openai_responses_common. --- pkg/providers/factory_provider.go | 2 - pkg/providers/factory_provider_test.go | 20 ++++-- pkg/providers/http_provider.go | 7 +- pkg/providers/openai_compat/provider.go | 43 ++++++------ pkg/providers/openai_compat/provider_test.go | 67 ++++++++++++++++--- .../responses_common.go | 4 +- 6 files changed, 105 insertions(+), 38 deletions(-) diff --git a/pkg/providers/factory_provider.go b/pkg/providers/factory_provider.go index 70ed0b8ca..962e6ae19 100644 --- a/pkg/providers/factory_provider.go +++ b/pkg/providers/factory_provider.go @@ -15,7 +15,6 @@ import ( anthropicmessages "github.com/sipeed/picoclaw/pkg/providers/anthropic_messages" "github.com/sipeed/picoclaw/pkg/providers/azure" "github.com/sipeed/picoclaw/pkg/providers/bedrock" - "github.com/sipeed/picoclaw/pkg/providers/openai_compat" ) // createClaudeAuthProvider creates a Claude provider using OAuth credentials from auth store. @@ -99,7 +98,6 @@ func CreateProviderFromConfig(cfg *config.ModelConfig) (LLMProvider, string, err cfg.MaxTokensField, cfg.RequestTimeout, cfg.ExtraBody, - openai_compat.WithResponsesPreferred(), ), modelID, nil case "azure", "azure-openai": diff --git a/pkg/providers/factory_provider_test.go b/pkg/providers/factory_provider_test.go index bc88ce80c..0b91992be 100644 --- a/pkg/providers/factory_provider_test.go +++ b/pkg/providers/factory_provider_test.go @@ -107,7 +107,7 @@ func TestCreateProviderFromConfig_OpenAI(t *testing.T) { } } -func TestCreateProviderFromConfig_OpenAIUsesResponsesFirst(t *testing.T) { +func TestCreateProviderFromConfig_OpenAIGPT5UsesResponsesFirst(t *testing.T) { var paths []string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -116,10 +116,19 @@ func TestCreateProviderFromConfig_OpenAIUsesResponsesFirst(t *testing.T) { switch r.URL.Path { case "/responses": w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"status":"completed","output":[{"type":"message","content":[{"type":"output_text","text":"from responses"}]}]}`)) + _, _ = w.Write([]byte(`{ + "status": "completed", + "output": [ + {"type": "message", "content": [{"type": "output_text", "text": "from responses"}]} + ] + }`)) case "/chat/completions": w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"choices":[{"message":{"content":"from chat completions"},"finish_reason":"stop"}]}`)) + _, _ = w.Write([]byte(`{ + "choices": [ + {"message": {"content": "from chat completions"}, "finish_reason": "stop"} + ] + }`)) default: http.Error(w, "not found", http.StatusNotFound) } @@ -128,7 +137,7 @@ func TestCreateProviderFromConfig_OpenAIUsesResponsesFirst(t *testing.T) { cfg := &config.ModelConfig{ ModelName: "test-openai", - Model: "openai/gpt-4o", + Model: "openai/gpt-5.2", APIKeys: config.SimpleSecureStrings("test-key"), APIBase: server.URL, } @@ -137,6 +146,9 @@ func TestCreateProviderFromConfig_OpenAIUsesResponsesFirst(t *testing.T) { if err != nil { t.Fatalf("CreateProviderFromConfig() error = %v", err) } + if modelID != "gpt-5.2" { + t.Fatalf("modelID = %q, want %q", modelID, "gpt-5.2") + } out, err := provider.Chat( t.Context(), diff --git a/pkg/providers/http_provider.go b/pkg/providers/http_provider.go index f9a3d4be5..eeb2fa8f9 100644 --- a/pkg/providers/http_provider.go +++ b/pkg/providers/http_provider.go @@ -33,11 +33,12 @@ func NewHTTPProviderWithMaxTokensFieldAndRequestTimeout( extraBody map[string]any, opts ...openai_compat.Option, ) *HTTPProvider { - providerOpts := []openai_compat.Option{ + providerOpts := make([]openai_compat.Option, 0, 3+len(opts)) + providerOpts = append(providerOpts, openai_compat.WithMaxTokensField(maxTokensField), - openai_compat.WithRequestTimeout(time.Duration(requestTimeoutSeconds) * time.Second), + openai_compat.WithRequestTimeout(time.Duration(requestTimeoutSeconds)*time.Second), openai_compat.WithExtraBody(extraBody), - } + ) providerOpts = append(providerOpts, opts...) return &HTTPProvider{ diff --git a/pkg/providers/openai_compat/provider.go b/pkg/providers/openai_compat/provider.go index 8690ba655..295322e95 100644 --- a/pkg/providers/openai_compat/provider.go +++ b/pkg/providers/openai_compat/provider.go @@ -16,6 +16,7 @@ import ( "github.com/openai/openai-go/v3" "github.com/openai/openai-go/v3/responses" + "github.com/sipeed/picoclaw/pkg/providers/common" orc "github.com/sipeed/picoclaw/pkg/providers/openai_responses_common" "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" @@ -34,12 +35,11 @@ type ( ) type Provider struct { - apiKey string - apiBase string - maxTokensField string // Field name for max tokens (e.g., "max_completion_tokens" for o1/glm models) - httpClient *http.Client - extraBody map[string]any // Additional fields to inject into request body - preferResponses bool // Prefer /responses for OpenAI-native configs. + apiKey string + apiBase string + maxTokensField string // Field name for max tokens (e.g., "max_completion_tokens" for o1/glm models) + httpClient *http.Client + extraBody map[string]any // Additional fields to inject into request body } type Option func(*Provider) @@ -60,12 +60,6 @@ func WithRequestTimeout(timeout time.Duration) Option { } } -func WithResponsesPreferred() Option { - return func(p *Provider) { - p.preferResponses = true - } -} - func WithExtraBody(extraBody map[string]any) Option { return func(p *Provider) { p.extraBody = extraBody @@ -173,13 +167,14 @@ func requestTemperature(model string, options map[string]any) (float64, bool) { return temperature, true } -func shouldPreferResponses(rawModel, normalizedModel string, preferOpenAIModels bool) bool { +func shouldPreferResponses(rawModel, normalizedModel string) bool { rawModel = strings.ToLower(strings.TrimSpace(rawModel)) normalizedModel = strings.ToLower(strings.TrimSpace(normalizedModel)) - return preferOpenAIModels || - strings.HasPrefix(rawModel, "gpt-5") || - strings.HasPrefix(normalizedModel, "gpt-5") + // Keep the automatic route conservative: only gpt-5 models are forced + // onto /responses, and all other model families stay on chat/completions + // unless they are explicitly routed elsewhere by the caller. + return strings.HasPrefix(rawModel, "gpt-5") || strings.HasPrefix(normalizedModel, "gpt-5") } func hasReasoningContentHistory(messages []Message) bool { @@ -233,6 +228,8 @@ func (p *Provider) buildResponsesRequestBody( } } + // Marshal through the SDK type first so we keep its validation/defaulting for + // Responses API fields, then convert back to a generic map to merge extraBody. jsonData, err := json.Marshal(requestBody) if err != nil { return nil, fmt.Errorf("failed to marshal responses request: %w", err) @@ -262,7 +259,7 @@ func (p *Provider) Chat( } normalizedModel := normalizeModel(model, p.apiBase) - if shouldPreferResponses(model, normalizedModel, p.preferResponses) && !hasReasoningContentHistory(messages) { + if shouldPreferResponses(model, normalizedModel) && !hasReasoningContentHistory(messages) { out, err := p.chatResponses(ctx, messages, tools, normalizedModel, options) if err == nil { return out, nil @@ -270,11 +267,19 @@ func (p *Provider) Chat( if ctx.Err() != nil { return nil, err } - log.Printf("openai_compat: /responses failed for %q, falling back to /chat/completions: %v", normalizedModel, err) + log.Printf( + "openai_compat: /responses failed for %q, falling back to /chat/completions: %v", + normalizedModel, + err, + ) fallbackOut, fallbackErr := p.chatCompletions(ctx, messages, tools, normalizedModel, options) if fallbackErr != nil { - return nil, fmt.Errorf("responses request failed; fallback chat/completions failed: %w", errors.Join(err, fallbackErr)) + joinedErr := errors.Join(err, fallbackErr) + return nil, fmt.Errorf( + "responses request failed; fallback chat/completions failed: %w", + joinedErr, + ) } return fallbackOut, nil } diff --git a/pkg/providers/openai_compat/provider_test.go b/pkg/providers/openai_compat/provider_test.go index e9e238a94..db70e8dae 100644 --- a/pkg/providers/openai_compat/provider_test.go +++ b/pkg/providers/openai_compat/provider_test.go @@ -17,7 +17,7 @@ import ( "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" ) -func TestProviderChat_PrefersResponsesWhenConfigured(t *testing.T) { +func TestProviderChat_PrefersResponsesForGPT5Models(t *testing.T) { var paths []string var responsesBody map[string]any @@ -64,12 +64,12 @@ func TestProviderChat_PrefersResponsesWhenConfigured(t *testing.T) { })) defer server.Close() - p := NewProvider("key", server.URL, "", WithResponsesPreferred()) + p := NewProvider("key", server.URL, "") out, err := p.Chat( t.Context(), []Message{{Role: "user", Content: "hi"}}, nil, - "gpt-4o", + "gpt-5.2", map[string]any{"max_tokens": 256}, ) if err != nil { @@ -82,8 +82,8 @@ func TestProviderChat_PrefersResponsesWhenConfigured(t *testing.T) { if !reflect.DeepEqual(paths, []string{"/responses"}) { t.Fatalf("paths = %v, want [/responses]", paths) } - if responsesBody["model"] != "gpt-4o" { - t.Fatalf("model = %v, want gpt-4o", responsesBody["model"]) + if responsesBody["model"] != "gpt-5.2" { + t.Fatalf("model = %v, want gpt-5.2", responsesBody["model"]) } if _, ok := responsesBody["input"]; !ok { t.Fatalf("expected responses request body to contain input") @@ -124,7 +124,7 @@ func TestProviderChat_ResponsesBodyUsesSharedTranslatorSemantics(t *testing.T) { })) defer server.Close() - p := NewProvider("key", server.URL, "", WithResponsesPreferred()) + p := NewProvider("key", server.URL, "") _, err := p.Chat( t.Context(), []Message{ @@ -132,7 +132,7 @@ func TestProviderChat_ResponsesBodyUsesSharedTranslatorSemantics(t *testing.T) { {Role: "user", Content: "Transcribe this", Media: []string{"data:audio/wav;base64,AAAA"}}, }, nil, - "gpt-4o", + "gpt-5.2", nil, ) if err != nil { @@ -154,6 +154,49 @@ func TestProviderChat_ResponsesBodyUsesSharedTranslatorSemantics(t *testing.T) { } } +func TestProviderChat_UsesChatCompletionsForNonGPT5Models(t *testing.T) { + var paths []string + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.URL.Path) + + switch r.URL.Path { + case "/chat/completions": + resp := map[string]any{ + "choices": []map[string]any{{ + "message": map[string]any{"content": "from chat completions"}, + "finish_reason": "stop", + }}, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(resp) + case "/responses": + http.Error(w, "responses should not be used", http.StatusBadRequest) + default: + http.Error(w, "not found", http.StatusNotFound) + } + })) + defer server.Close() + + p := NewProvider("key", server.URL, "") + out, err := p.Chat( + t.Context(), + []Message{{Role: "user", Content: "hi"}}, + nil, + "Qwen3.5-35B-A3B", + nil, + ) + if err != nil { + t.Fatalf("Chat() error = %v", err) + } + if out.Content != "from chat completions" { + t.Fatalf("Content = %q, want %q", out.Content, "from chat completions") + } + if !reflect.DeepEqual(paths, []string{"/chat/completions"}) { + t.Fatalf("paths = %v, want [/chat/completions]", paths) + } +} + func TestProviderChat_FallsBackToChatCompletionsWhenResponsesFails(t *testing.T) { var paths []string @@ -616,7 +659,9 @@ func TestProviderChat_ParsesRefusalFromResponses(t *testing.T) { } func TestParseResponsesResponse_FailedStatusUsesServerMessage(t *testing.T) { - _, err := parseResponsesResponse(strings.NewReader(`{"status":" failed ","error":{"message":"responses failed"}}`)) + _, err := parseResponsesResponse( + strings.NewReader(`{"status":" failed ","error":{"message":"responses failed"}}`), + ) if err == nil { t.Fatal("expected error, got nil") } @@ -626,7 +671,11 @@ func TestParseResponsesResponse_FailedStatusUsesServerMessage(t *testing.T) { } func TestParseResponsesResponse_UsesNormalizedIncompleteStatus(t *testing.T) { - out, err := parseResponsesResponse(strings.NewReader(`{"status":" incomplete ","output":[{"type":"message","content":[{"type":"output_text","text":"partial answer"}]}],"incomplete_details":{"reason":"content_filter"}}`)) + out, err := parseResponsesResponse( + strings.NewReader( + `{"status":" incomplete ","output":[{"type":"message","content":[{"type":"output_text","text":"partial answer"}]}],"incomplete_details":{"reason":"content_filter"}}`, + ), + ) if err != nil { t.Fatalf("parseResponsesResponse() error = %v", err) } diff --git a/pkg/providers/openai_responses_common/responses_common.go b/pkg/providers/openai_responses_common/responses_common.go index 5ddb6041b..057ebbd57 100644 --- a/pkg/providers/openai_responses_common/responses_common.go +++ b/pkg/providers/openai_responses_common/responses_common.go @@ -360,7 +360,9 @@ func parseResponseEnvelope(apiResp *responseEnvelope) (*protocoltypes.LLMRespons finishReason = "tool_calls" } else if status == "incomplete" { finishReason = "length" - if apiResp.IncompleteDetails != nil && apiResp.IncompleteDetails.Reason != "" && apiResp.IncompleteDetails.Reason != "max_output_tokens" { + if apiResp.IncompleteDetails != nil && + apiResp.IncompleteDetails.Reason != "" && + apiResp.IncompleteDetails.Reason != "max_output_tokens" { finishReason = apiResp.IncompleteDetails.Reason } }