diff --git a/internal/agent/sanitize.go b/internal/agent/sanitize.go index decd0d70..885471d5 100644 --- a/internal/agent/sanitize.go +++ b/internal/agent/sanitize.go @@ -168,21 +168,23 @@ func stripDowngradedToolCallText(content string) string { // --- 3. Thinking/reasoning tags --- // Matches TS stripThinkingTagsFromText() with strict mode. -// Strips: ..., ..., ..., +// Strips: ..., ..., +// ..., ..., // ... // Go regexp doesn't support backreferences, so we use separate patterns. var thinkingTagPatterns = []*regexp.Regexp{ - regexp.MustCompile(`(?is).*?`), - regexp.MustCompile(`(?is).*?`), - regexp.MustCompile(`(?is).*?`), - regexp.MustCompile(`(?is).*?`), - regexp.MustCompile(`(?is).*?`), + regexp.MustCompile(`(?is)]*>.*?`), + regexp.MustCompile(`(?is)]*>.*?`), + regexp.MustCompile(`(?is)]*>.*?`), + regexp.MustCompile(`(?is)]*>.*?`), + regexp.MustCompile(`(?is)]*>.*?`), + regexp.MustCompile(`(?is)]*>.*?`), } func stripThinkingTags(content string) string { lower := strings.ToLower(content) if !strings.Contains(lower, "secret reasoning world", + want: "Hello world", + }, + { + name: "strips redacted_thinking with attributes", + input: `Before hidden after`, + want: "Before after", + }, + { + name: "strips redacted_thinking with whitespace in closing tag", + input: "A x B", + want: "A B", + }, + { + name: "strips redacted_thinking multiline content", + input: "Start\n\nline1\nline2\n\nEnd", + want: "Start\n\nEnd", + }, + { + name: "content with only redacted_thinking (no other thinking tags)", + input: "all hidden", + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := stripThinkingTags(tt.input) + if got != tt.want { + t.Errorf("stripThinkingTags() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestStripThinkingTags_ExistingPatterns(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + { + name: "strips think tags", + input: "Hello reasoning world", + want: "Hello world", + }, + { + name: "strips thinking tags", + input: "A step by step B", + want: "A B", + }, + { + name: "strips thought tags", + input: "A hmm B", + want: "A B", + }, + { + name: "strips antThinking tags", + input: "A internal B", + want: "A B", + }, + { + name: "strips think with attributes", + input: `A stuff B`, + want: "A B", + }, + { + name: "no thinking tags — returns unchanged", + input: "Just normal content here", + want: "Just normal content here", + }, + { + name: "mixed redacted_thinking and think", + input: "a middle b", + want: "middle", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := stripThinkingTags(tt.input) + if got != tt.want { + t.Errorf("stripThinkingTags() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestSanitizeAssistantContent_RedactedThinking(t *testing.T) { + input := "Hello secret world" + got := SanitizeAssistantContent(input) + if got != "Hello world" { + t.Errorf("SanitizeAssistantContent() = %q, want %q", got, "Hello world") + } +} diff --git a/internal/providers/openai.go b/internal/providers/openai.go index 721bc179..67dc882e 100644 --- a/internal/providers/openai.go +++ b/internal/providers/openai.go @@ -46,6 +46,43 @@ func (p *OpenAIProvider) isFireworksEndpoint() bool { return strings.Contains(strings.ToLower(p.apiBase), "fireworks.ai") } +// isTogetherEndpoint returns true for Together AI inference hosts. +// Together rejects some OpenAI extensions (e.g. stream_options, reasoning_effort) with HTTP 400. +// Uses URL, provider_type, and name so reverse-proxied Together endpoints are also detected. +func (p *OpenAIProvider) isTogetherEndpoint() bool { + b := strings.ToLower(p.apiBase) + if strings.Contains(b, "together.xyz") || strings.Contains(b, "together.ai") { + return true + } + if strings.Contains(strings.ToLower(strings.TrimSpace(p.providerType)), "together") { + return true + } + if strings.Contains(strings.ToLower(p.name), "together") { + return true + } + return false +} + +// isDashScopeAPIBase returns true for Alibaba DashScope OpenAI-compatible endpoints. +func isDashScopeAPIBase(apiBase string) bool { + return strings.Contains(strings.ToLower(apiBase), "dashscope") +} + +// dashScopePassthroughKeys is true when enable_thinking / thinking_budget may be added to the JSON body. +// Uses URL, provider_type, and name so httptest DashScope URLs still work in tests. +func (p *OpenAIProvider) dashScopePassthroughKeys() bool { + if isDashScopeAPIBase(p.apiBase) { + return true + } + if strings.Contains(strings.ToLower(strings.TrimSpace(p.providerType)), "dashscope") { + return true + } + if strings.Contains(strings.ToLower(p.name), "dashscope") { + return true + } + return false +} + func NewOpenAIProvider(name, apiKey, apiBase, defaultModel string) *OpenAIProvider { if apiBase == "" { apiBase = "https://api.openai.com/v1" @@ -325,8 +362,9 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream "role": role, } - // Echo reasoning_content for assistant messages (required by Kimi, DeepSeek when thinking is enabled) - if m.Thinking != "" && m.Role == "assistant" { + // Echo reasoning_content only for APIs/models that accept it on assistant history. + // Together Qwen and many OpenAI-compat gateways reject unknown message fields → HTTP 400. + if m.Thinking != "" && m.Role == "assistant" && openAIWireAssistantReasoningContent(model) { msg["reasoning_content"] = m.Thinking } @@ -334,6 +372,13 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream // (Gemini rejects empty content → "must include at least one parts field"). if m.Role == "user" && len(m.Images) > 0 { var parts []map[string]any + // Text before images — Together / Qwen vision examples use this order; OpenAI accepts both. + if m.Content != "" { + parts = append(parts, map[string]any{ + "type": "text", + "text": m.Content, + }) + } for _, img := range m.Images { parts = append(parts, map[string]any{ "type": "image_url", @@ -342,12 +387,6 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream }, }) } - if m.Content != "" { - parts = append(parts, map[string]any{ - "type": "text", - "text": m.Content, - }) - } msg["content"] = parts } else if m.Content != "" || len(m.ToolCalls) == 0 { msg["content"] = m.Content @@ -408,7 +447,8 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream body["tool_choice"] = "auto" } - if stream { + // Together returns HTTP 400 on some requests when stream_options is present. + if stream && !p.isTogetherEndpoint() { body["stream_options"] = map[string]any{ "include_usage": true, } @@ -443,17 +483,21 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream } } - // Inject reasoning_effort for o-series models (ignored by models that don't support it) + // reasoning_effort is OpenAI-specific; do not send to third-party OpenAI-compatible APIs. if level, ok := req.Options[OptThinkingLevel].(string); ok && level != "" && level != "off" { - body[OptReasoningEffort] = level + if openAIModelSupportsReasoningEffort(model) { + body[OptReasoningEffort] = level + } } - // DashScope-specific passthrough keys - if v, ok := req.Options[OptEnableThinking]; ok { - body[OptEnableThinking] = v - } - if v, ok := req.Options[OptThinkingBudget]; ok { - body[OptThinkingBudget] = v + // DashScope-specific passthrough keys — never send to other OpenAI-compat hosts. + if p.dashScopePassthroughKeys() { + if v, ok := req.Options[OptEnableThinking]; ok { + body[OptEnableThinking] = v + } + if v, ok := req.Options[OptThinkingBudget]; ok { + body[OptThinkingBudget] = v + } } return body @@ -468,6 +512,39 @@ func modelFamily(model string) string { return model } +// openAIModelSupportsReasoningEffort is true when the Chat Completions request may include +// the top-level "reasoning_effort" field (OpenAI o-series / GPT-5 family). +// Other OpenAI-compatible hosts (Together, Groq, vLLM, etc.) often reject unknown fields with HTTP 400. +func openAIModelSupportsReasoningEffort(model string) bool { + if LookupReasoningCapability(model) != nil { + return true + } + fam := strings.ToLower(modelFamily(model)) + for _, prefix := range []string{"gpt-5", "o1", "o3", "o4"} { + if strings.HasPrefix(fam, prefix) { + return true + } + } + return false +} + +// openAIWireAssistantReasoningContent is true when assistant message objects may include +// "reasoning_content" (thinking replay). Narrow allowlist — most OpenAI-compat hosts reject it. +func openAIWireAssistantReasoningContent(model string) bool { + if openAIModelSupportsReasoningEffort(model) { + return true + } + fam := strings.ToLower(modelFamily(model)) + full := strings.ToLower(model) + if strings.Contains(fam, "deepseek") || strings.Contains(full, "deepseek") { + return true + } + if strings.Contains(fam, "kimi") || strings.Contains(full, "kimi") { + return true + } + return false +} + func (p *OpenAIProvider) doRequest(ctx context.Context, body any) (io.ReadCloser, error) { data, err := json.Marshal(body) if err != nil { diff --git a/internal/providers/openai_reasoning_alias_test.go b/internal/providers/openai_reasoning_alias_test.go index 50509ac1..6d92d79a 100644 --- a/internal/providers/openai_reasoning_alias_test.go +++ b/internal/providers/openai_reasoning_alias_test.go @@ -228,6 +228,36 @@ func TestBuildRequestBody_NoReasoningAliasInOutput(t *testing.T) { } } +// TestBuildRequestBody_TogetherQwenOmitsAssistantReasoningContent verifies Together-hosted Qwen +// does not get reasoning_content on assistant history (HTTP 400 input validation otherwise). +func TestBuildRequestBody_TogetherQwenOmitsAssistantReasoningContent(t *testing.T) { + p := NewOpenAIProvider("togetherai", "key", "https://api.together.xyz/v1", "") + + req := ChatRequest{ + Messages: []Message{ + { + Role: "assistant", + Content: "Dạ em có thể thấy images...", + Thinking: "User is asking if I can read images.", + }, + {Role: "user", Content: "hello"}, + }, + } + + body := p.buildRequestBody("Qwen/Qwen3.5-397B-A17B", req, false) + msgs, ok := body["messages"].([]map[string]any) + if !ok { + t.Fatal("messages missing") + } + for _, m := range msgs { + if role, _ := m["role"].(string); role == "assistant" { + if _, has := m["reasoning_content"]; has { + t.Fatalf("Together Qwen must omit reasoning_content, got msg=%v", m) + } + } + } +} + // TestBuildRequestBody_NoReasoningContentWhenThinkingEmpty confirms that assistant messages // without Thinking content do not emit a reasoning_content key at all. func TestBuildRequestBody_NoReasoningContentWhenThinkingEmpty(t *testing.T) { diff --git a/internal/providers/openai_test.go b/internal/providers/openai_test.go index faa2194e..ccf6528d 100644 --- a/internal/providers/openai_test.go +++ b/internal/providers/openai_test.go @@ -136,6 +136,257 @@ func TestBuildRequestBody_TemperatureDependsOnModelNotAPIBase(t *testing.T) { } } +func TestBuildRequestBody_ReasoningEffortOpenAIOnly(t *testing.T) { + thinkOpts := map[string]any{OptThinkingLevel: "medium"} + + t.Run("Together Qwen omits reasoning_effort", func(t *testing.T) { + p := NewOpenAIProvider("togetherai", "key", "https://api.together.xyz/v1", "") + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: thinkOpts, + } + body := p.buildRequestBody("Qwen/Qwen3.5-397B-A17B", req, false) + if _, ok := body[OptReasoningEffort]; ok { + t.Fatalf("Qwen on Together must not send %q (HTTP 400 on unknown fields), body=%v", OptReasoningEffort, body) + } + }) + + t.Run("OpenAI gpt-5 keeps reasoning_effort", func(t *testing.T) { + p := NewOpenAIProvider("openai", "key", "https://api.openai.com/v1", "gpt-5") + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: thinkOpts, + } + body := p.buildRequestBody("gpt-5.4", req, false) + if got, ok := body[OptReasoningEffort].(string); !ok || got != "medium" { + t.Fatalf("gpt-5.4 should send reasoning_effort medium, got body=%v", body) + } + }) + + t.Run("OpenAI o3-mini keeps reasoning_effort", func(t *testing.T) { + p := NewOpenAIProvider("openai", "key", "https://api.openai.com/v1", "") + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: thinkOpts, + } + body := p.buildRequestBody("o3-mini", req, false) + if got, ok := body[OptReasoningEffort].(string); !ok || got != "medium" { + t.Fatalf("o3-mini should send reasoning_effort medium, got body=%v", body) + } + }) +} + +func TestBuildRequestBody_TogetherOmitsStreamOptions(t *testing.T) { + together := NewOpenAIProvider("togetherai", "key", "https://api.together.xyz/v1", "") + openai := NewOpenAIProvider("openai", "key", "https://api.openai.com/v1", "gpt-4") + + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + } + + gotTogether := together.buildRequestBody("Qwen/Qwen3.5-397B-A17B", req, true) + if _, ok := gotTogether["stream_options"]; ok { + t.Fatalf("Together streaming must omit stream_options, got %v", gotTogether["stream_options"]) + } + + gotOpenAI := openai.buildRequestBody("gpt-4o", req, true) + if _, ok := gotOpenAI["stream_options"]; !ok { + t.Fatal("OpenAI streaming should include stream_options for usage") + } +} + +func TestBuildRequestBody_TogetherOmitsStrayDashScopeKeys(t *testing.T) { + p := NewOpenAIProvider("togetherai", "key", "https://api.together.xyz/v1", "") + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: map[string]any{ + OptEnableThinking: true, + OptThinkingBudget: 4096, + }, + } + body := p.buildRequestBody("Qwen/Qwen3.5-397B-A17B", req, false) + if _, ok := body[OptEnableThinking]; ok { + t.Fatalf("Together must not send %q", OptEnableThinking) + } + if _, ok := body[OptThinkingBudget]; ok { + t.Fatalf("Together must not send %q", OptThinkingBudget) + } +} + +func TestBuildRequestBody_MultimodalTextBeforeImages(t *testing.T) { + p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4") + req := ChatRequest{ + Messages: []Message{ + { + Role: "user", + Content: "describe", + Images: []ImageContent{ + {MimeType: "image/jpeg", Data: "qq=="}, + }, + }, + }, + } + body := p.buildRequestBody("gpt-4o", req, false) + msgs := body["messages"].([]map[string]any) + parts, ok := msgs[0]["content"].([]map[string]any) + if !ok || len(parts) < 2 { + t.Fatalf("want multimodal parts, got %v", msgs[0]["content"]) + } + if typ, _ := parts[0]["type"].(string); typ != "text" { + t.Fatalf("first part should be text, got type=%q parts=%v", typ, parts) + } + if typ, _ := parts[1]["type"].(string); typ != "image_url" { + t.Fatalf("second part should be image_url, got type=%q", typ) + } +} + +func TestBuildRequestBody_TogetherDetectedByProviderType(t *testing.T) { + // Together behind reverse proxy — detected by providerType, not URL. + p := NewOpenAIProvider("my-proxy", "key", "https://proxy.internal/v1", "") + p.WithProviderType("together") + + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: map[string]any{OptThinkingLevel: "medium"}, + } + + body := p.buildRequestBody("Qwen/Qwen3.5-397B-A17B", req, true) + if _, ok := body["stream_options"]; ok { + t.Fatal("Together (via providerType) must omit stream_options") + } + if _, ok := body[OptReasoningEffort]; ok { + t.Fatal("Together (via providerType) must omit reasoning_effort") + } +} + +func TestBuildRequestBody_TogetherDetectedByName(t *testing.T) { + // Together detected by provider name. + p := NewOpenAIProvider("together-prod", "key", "https://proxy.internal/v1", "") + + body := p.buildRequestBody("Qwen/Qwen3.5-397B-A17B", ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + }, true) + if _, ok := body["stream_options"]; ok { + t.Fatal("Together (via name) must omit stream_options") + } +} + +func TestBuildRequestBody_MultimodalImagesOnlyNoEmptyTextPart(t *testing.T) { + p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4") + req := ChatRequest{ + Messages: []Message{ + { + Role: "user", + Content: "", // no text + Images: []ImageContent{{MimeType: "image/png", Data: "abc=="}}, + }, + }, + } + body := p.buildRequestBody("gpt-4o", req, false) + msgs := body["messages"].([]map[string]any) + parts, ok := msgs[0]["content"].([]map[string]any) + if !ok { + t.Fatal("want multimodal parts array") + } + // Only image parts — no empty text part should be present + for _, part := range parts { + if typ, _ := part["type"].(string); typ == "text" { + t.Fatalf("images-only message should not have text part, got %v", parts) + } + } + if len(parts) != 1 { + t.Fatalf("want 1 image part, got %d parts: %v", len(parts), parts) + } +} + +func TestBuildRequestBody_OllamaModelOmitsReasoningContent(t *testing.T) { + // Ollama/vLLM self-hosted models should NOT get reasoning_content + // since most don't support it and it causes HTTP 400. + p := NewOpenAIProvider("ollama", "key", "http://localhost:11434/v1", "") + + req := ChatRequest{ + Messages: []Message{ + {Role: "assistant", Content: "sure", Thinking: "let me think..."}, + {Role: "user", Content: "continue"}, + }, + } + + body := p.buildRequestBody("qwen2:7b", req, false) + msgs := body["messages"].([]map[string]any) + for _, m := range msgs { + if role, _ := m["role"].(string); role == "assistant" { + if _, has := m["reasoning_content"]; has { + t.Fatalf("Ollama qwen2:7b must omit reasoning_content, got %v", m) + } + } + } +} + +func TestBuildRequestBody_DeepSeekKeepsReasoningContent(t *testing.T) { + // DeepSeek models must keep reasoning_content (required for thinking replay). + p := NewOpenAIProvider("deepseek", "key", "https://api.deepseek.com/v1", "") + + req := ChatRequest{ + Messages: []Message{ + {Role: "assistant", Content: "result", Thinking: "step by step"}, + {Role: "user", Content: "next"}, + }, + } + + body := p.buildRequestBody("deepseek-r1", req, false) + msgs := body["messages"].([]map[string]any) + for _, m := range msgs { + if role, _ := m["role"].(string); role == "assistant" { + if _, has := m["reasoning_content"]; !has { + t.Fatalf("DeepSeek must keep reasoning_content, got %v", m) + } + } + } +} + +func TestBuildRequestBody_KimiKeepsReasoningContent(t *testing.T) { + p := NewOpenAIProvider("kimi", "key", "https://api.moonshot.cn/v1", "") + + req := ChatRequest{ + Messages: []Message{ + {Role: "assistant", Content: "ok", Thinking: "reasoning"}, + {Role: "user", Content: "next"}, + }, + } + + body := p.buildRequestBody("kimi-k2", req, false) + msgs := body["messages"].([]map[string]any) + for _, m := range msgs { + if role, _ := m["role"].(string); role == "assistant" { + if _, has := m["reasoning_content"]; !has { + t.Fatalf("Kimi must keep reasoning_content, got %v", m) + } + } + } +} + +func TestBuildRequestBody_DashScopePassthroughByProviderType(t *testing.T) { + // DashScope behind proxy — detected by providerType. + p := NewOpenAIProvider("my-qwen", "key", "https://proxy.internal/v1", "") + p.WithProviderType("dashscope") + + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: map[string]any{ + OptEnableThinking: true, + OptThinkingBudget: 8192, + }, + } + + body := p.buildRequestBody("qwen3-max", req, false) + if _, ok := body[OptEnableThinking]; !ok { + t.Fatal("DashScope (via providerType) must pass through enable_thinking") + } + if _, ok := body[OptThinkingBudget]; !ok { + t.Fatal("DashScope (via providerType) must pass through thinking_budget") + } +} + func TestBuildRequestBody_PrefixedModelsUseCorrectTokenField(t *testing.T) { p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4")