diff --git a/internal/llm/client.go b/internal/llm/client.go index cb4f967..c277bf2 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -10,6 +10,7 @@ import ( "net/http" "strconv" "strings" + "sync/atomic" "time" "github.com/BackendStack21/odek/internal/transport" @@ -25,6 +26,11 @@ type Client struct { MaxTokens int // max output tokens (0 = provider default) Temperature float64 // 0 = use provider default, <0 = omit from request http *http.Client + + // forceNoneEffort is learned at runtime: set when the provider rejects + // reasoning_effort combined with function tools (e.g. gpt-5.6-luna), + // so subsequent calls pin effort to "none" without a failed round-trip. + forceNoneEffort atomic.Bool } // maxResponseSize limits the LLM response body read to prevent DoS/OOM. @@ -54,8 +60,44 @@ func NewWithMaxTokens(baseURL, apiKey, model, thinking string, thinkingBudget in } } +// IsAnthropic reports whether the client's base URL targets the Anthropic +// API. Anthropic-specific request features (the top-level "system" field, +// cache_control markers) must only be sent when this is true — other +// providers reject them (OpenAI answers 400 unknown_parameter). +func (c *Client) IsAnthropic() bool { + return strings.Contains(c.BaseURL, "anthropic") +} + +// sendsThinkingObject reports whether the provider accepts the +// Anthropic-style "thinking" request object. Anthropic requires it for +// extended thinking and DeepSeek supports it natively (deepseek-reasoner); +// other providers (OpenAI and compatibles) reject unknown top-level +// parameters, so thinking intent must be mapped to reasoning_effort instead. +func (c *Client) sendsThinkingObject() bool { + return c.IsAnthropic() || strings.Contains(c.BaseURL, "deepseek") +} + +// modelForbidsTemperature reports whether the model rejects an explicit +// temperature parameter. OpenAI reasoning models (o1/o3/o4 families and the +// gpt-5 series) only accept the default temperature (1); sending any other +// value — including odek's deterministic default 0 — returns a 400 +// "unsupported_value" error. Matching is model-name-based and +// provider-agnostic, since OpenAI-compatible proxies serving these model +// IDs enforce the same constraint. +func modelForbidsTemperature(model string) bool { + m := strings.ToLower(model) + for _, prefix := range []string{"o1", "o3", "o4", "gpt-5"} { + if strings.HasPrefix(m, prefix) { + return true + } + } + return false +} + // CacheControl marks a message or system block as cacheable by Anthropic. -// Providers that don't support it (OpenAI, DeepSeek) silently ignore the field. +// Only send it to Anthropic endpoints: OpenAI rejects Anthropic-style +// request shapes with a 400 (e.g. the top-level "system" field), so the +// loop only applies cache markers when Client.IsAnthropic reports true. type CacheControl struct { Type string `json:"type"` // "ephemeral" } @@ -154,7 +196,8 @@ var toolChoiceNone = "none" // // Returns the updated messages and a System field (populated if the system // message was moved out of the messages array for Anthropic compatibility). -// Providers that don't support prompt caching silently ignore these fields. +// Callers must only use this when the target provider is Anthropic +// (see Client.IsAnthropic) — OpenAI rejects the resulting request shape. func ApplyCacheMarkers(messages []Message) ([]Message, []SystemBlock) { var systemBlocks []SystemBlock annotated := make([]Message, 0, len(messages)) @@ -238,12 +281,11 @@ func (c *Client) SimpleCall(ctx context.Context, systemPrompt, userPrompt string return raw.Choices[0].Message.Content, nil } -// Call sends a chat completion request and returns the result. -// systemBlocks is optional — pass nil for providers that don't support -// the separate System field (OpenAI, DeepSeek). When non-nil, the system -// prompt is sent in the "system" field instead of as a system message in -// the messages array (Anthropic format for prompt caching). -func (c *Client) Call(ctx context.Context, messages []Message, systemBlocks []SystemBlock, tools []ToolDef) (*CallResult, error) { +// buildCallParams assembles the request body for a Call, mapping the +// thinking configuration onto whatever shape the target provider accepts: +// the Anthropic-style "thinking" object for Anthropic/DeepSeek, +// reasoning_effort for OpenAI reasoning models, and nothing otherwise. +func (c *Client) buildCallParams(messages []Message, systemBlocks []SystemBlock, tools []ToolDef) CallParams { body := CallParams{ Model: c.Model, Messages: messages, @@ -255,26 +297,44 @@ func (c *Client) Call(ctx context.Context, messages []Message, systemBlocks []Sy switch c.Thinking { case "enabled": - // Anthropic requires budget_tokens when enabling thinking. - // 5000 is a safe default: leaves ample room for the text response - // even on models with 8K max output (e.g. Claude Haiku). - // DeepSeek silently ignores the field. - budget := c.ThinkingBudget - if budget <= 0 { - budget = 5000 + if c.sendsThinkingObject() { + // Anthropic requires budget_tokens when enabling thinking. + // 5000 is a safe default: leaves ample room for the text response + // even on models with 8K max output (e.g. Claude Haiku). + // DeepSeek silently ignores the field. + budget := c.ThinkingBudget + if budget <= 0 { + budget = 5000 + } + body.Thinking = &ThinkingConfig{Type: "enabled", BudgetTokens: budget} + // Anthropic also requires temperature=1 when thinking is enabled. + // Force it regardless of the configured temperature to avoid a 400. + one := float64(1) + body.Temperature = &one + } else if modelForbidsTemperature(c.Model) { + // OpenAI reasoning models have no "thinking" object; the closest + // mapping for enabled extended thinking is maximum effort. + // Temperature stays omitted — only the provider default is accepted. + body.ReasoningEffort = "high" + } else if c.Temperature >= 0 { + // Non-reasoning models (e.g. gpt-4o) have no thinking to enable; + // just honor the configured temperature. + body.Temperature = &c.Temperature } - body.Thinking = &ThinkingConfig{Type: "enabled", BudgetTokens: budget} - // Anthropic also requires temperature=1 when thinking is enabled. - // Force it regardless of the configured temperature to avoid a 400. - one := float64(1) - body.Temperature = &one case "disabled": - body.Thinking = &ThinkingConfig{Type: "disabled"} - if c.Temperature >= 0 { + if c.sendsThinkingObject() { + body.Thinking = &ThinkingConfig{Type: "disabled"} + } else if modelForbidsTemperature(c.Model) { + // OpenAI reasoning models: disabling thinking maps to effort + // "none" (accepted on the gpt-5 series). Non-reasoning models + // have nothing to disable, so the field is omitted entirely. + body.ReasoningEffort = "none" + } + if c.Temperature >= 0 && !modelForbidsTemperature(c.Model) { body.Temperature = &c.Temperature } default: - if c.Temperature >= 0 { + if c.Temperature >= 0 && !modelForbidsTemperature(c.Model) { body.Temperature = &c.Temperature } if c.Thinking == "low" || c.Thinking == "medium" || c.Thinking == "high" { @@ -282,18 +342,59 @@ func (c *Client) Call(ctx context.Context, messages []Message, systemBlocks []Sy } } + // Some models (e.g. gpt-5.6-luna) reject function tools combined with + // any reasoning_effort other than "none" on /chat/completions — and + // their DEFAULT effort is not "none", so merely omitting the field is + // not enough. Once that constraint has been learned from a 400 (see + // Call), pin effort to "none" whenever tools are present. + if c.forceNoneEffort.Load() && len(tools) > 0 { + body.ReasoningEffort = "none" + } + + return body +} + +// Call sends a chat completion request and returns the result. +// systemBlocks is optional — pass nil for providers that don't support +// the separate System field (OpenAI, DeepSeek). When non-nil, the system +// prompt is sent in the "system" field instead of as a system message in +// the messages array (Anthropic format for prompt caching). +func (c *Client) Call(ctx context.Context, messages []Message, systemBlocks []SystemBlock, tools []ToolDef) (*CallResult, error) { + body := c.buildCallParams(messages, systemBlocks, tools) + reqBytes, err := json.Marshal(body) if err != nil { return nil, fmt.Errorf("llm: marshal request: %w", err) } respBytes, err := c.postChatWithRetry(ctx, reqBytes) + if err != nil && len(tools) > 0 && !c.forceNoneEffort.Load() && reasoningEffortRejected(err) { + // Learn the constraint once, then every later call sends + // reasoning_effort "none" directly (no more failed round-trips). + c.forceNoneEffort.Store(true) + body.ReasoningEffort = "none" + reqBytes, mErr := json.Marshal(body) + if mErr != nil { + return nil, fmt.Errorf("llm: marshal request: %w", mErr) + } + respBytes, err = c.postChatWithRetry(ctx, reqBytes) + } if err != nil { return nil, err } return parseResponse(respBytes) } +// reasoningEffortRejected reports whether err is a 400 whose provider +// response names reasoning_effort as the offending parameter. +func reasoningEffortRejected(err error) bool { + if err == nil { + return false + } + msg := err.Error() + return strings.Contains(msg, "400") && strings.Contains(msg, `"reasoning_effort"`) +} + // postChatWithRetry POSTs reqBytes to /chat/completions and returns the raw 200 // response body, retrying transient network errors and retryable HTTP statuses // (429, 502, 503, 504) with exponential backoff. Shared by every chat call so diff --git a/internal/llm/client_test.go b/internal/llm/client_test.go index df13ade..de9a578 100644 --- a/internal/llm/client_test.go +++ b/internal/llm/client_test.go @@ -3,8 +3,11 @@ package llm import ( "context" "encoding/json" + "fmt" + "io" "net/http" "net/http/httptest" + "sync" "testing" "time" ) @@ -406,30 +409,11 @@ func TestClient_Call_HTTPError(t *testing.T) { } func TestClient_Call_WithThinking(t *testing.T) { - var receivedBody map[string]any - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - json.NewDecoder(r.Body).Decode(&receivedBody) - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"choices":[{"message":{"content":"thinking response"}}]}`)) - })) - defer server.Close() - - c := New(server.URL, "sk-test", "deepseek-chat", "enabled", 0, 0) - result, err := c.Call(context.Background(), []Message{{Role: "user", Content: "think"}}, nil, nil) - if err != nil { - t.Fatalf("Call() error: %v", err) - } - if result.Content != "thinking response" { - t.Errorf("Content = %q", result.Content) - } - // Verify thinking was sent in the request - thinking, ok := receivedBody["thinking"] - if !ok { - t.Error("thinking field not sent in request") - } - thinkingMap, ok := thinking.(map[string]any) - if !ok || thinkingMap["type"] != "enabled" { - t.Errorf("thinking = %v, want {type: enabled}", thinking) + // DeepSeek supports the Anthropic-style thinking object natively. + c := New("https://api.deepseek.com/v1", "sk-test", "deepseek-chat", "enabled", 0, 0) + body := c.buildCallParams([]Message{{Role: "user", Content: "think"}}, nil, nil) + if body.Thinking == nil || body.Thinking.Type != "enabled" { + t.Errorf("thinking = %v, want {type: enabled}", body.Thinking) } } @@ -720,29 +704,14 @@ func TestClient_Call_FlashVsProThinkingContrast(t *testing.T) { }) t.Run("pro_thinking_enabled", func(t *testing.T) { - var body map[string]any - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - json.NewDecoder(r.Body).Decode(&body) - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"choices":[{"message":{"content":"ok"}}]}`)) - })) - defer server.Close() - - c := New(server.URL, "sk-test", "deepseek-v4-pro", "enabled", 0, 0) - _, err := c.Call(context.Background(), []Message{{Role: "user", Content: "hi"}}, nil, nil) - if err != nil { - t.Fatal(err) - } - thinking, ok := body["thinking"] - if !ok { + // DeepSeek supports the Anthropic-style thinking object natively. + c := New("https://api.deepseek.com/v1", "sk-test", "deepseek-v4-pro", "enabled", 0, 0) + body := c.buildCallParams([]Message{{Role: "user", Content: "hi"}}, nil, nil) + if body.Thinking == nil { t.Fatal("Pro: thinking field should be present") } - thinkingMap, ok := thinking.(map[string]any) - if !ok { - t.Fatal("Pro: thinking should be an object") - } - if thinkingMap["type"] != "enabled" { - t.Errorf("Pro: thinking.type = %v, want 'enabled'", thinkingMap["type"]) + if body.Thinking.Type != "enabled" { + t.Errorf("Pro: thinking.type = %v, want 'enabled'", body.Thinking.Type) } }) } @@ -1050,3 +1019,175 @@ func TestParseResponse_AnthropicAndOpenAICache(t *testing.T) { t.Errorf("CachedTokens = %d, want 999", result.CachedTokens) } } + +func TestClient_IsAnthropic(t *testing.T) { + cases := []struct { + baseURL string + want bool + }{ + {"https://api.anthropic.com/v1", true}, + {"https://api.openai.com/v1", false}, + {"https://api.deepseek.com/v1", false}, + {"http://localhost:11434/v1", false}, + } + for _, tc := range cases { + c := New(tc.baseURL, "sk-test", "model", "", 0, 0) + if got := c.IsAnthropic(); got != tc.want { + t.Errorf("IsAnthropic(%q) = %v, want %v", tc.baseURL, got, tc.want) + } + } +} + +// OpenAI reasoning models (o1/o3/o4, gpt-5 family) reject any explicit +// temperature other than the default (1) with a 400. The client must omit +// the field for those models while still sending odek's deterministic +// default (0) to models that accept it. +func TestCall_OmitsTemperatureForReasoningModels(t *testing.T) { + cases := []struct { + model string + wantTempSent bool + }{ + {"gpt-5-nano", false}, + {"gpt-5", false}, + {"o3-mini", false}, + {"o1-preview", false}, + {"o4-mini", false}, + {"GPT-5-MINI", false}, // case-insensitive + {"gpt-4o-mini", true}, + {"deepseek-chat", true}, + {"claude-sonnet-4-5", true}, + } + + for _, tc := range cases { + t.Run(tc.model, func(t *testing.T) { + var captured []byte + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured, _ = io.ReadAll(r.Body) + fmt.Fprint(w, `{"choices":[{"message":{"content":"ok"}}]}`) + })) + defer server.Close() + + c := New(server.URL, "sk-test", tc.model, "", 0, 0) + c.Temperature = 0 // odek's deterministic default + if _, err := c.Call(context.Background(), []Message{{Role: "user", Content: "hi"}}, nil, nil); err != nil { + t.Fatalf("Call: %v", err) + } + + var body map[string]any + if err := json.Unmarshal(captured, &body); err != nil { + t.Fatalf("captured request is not JSON: %v", err) + } + _, sent := body["temperature"] + if sent != tc.wantTempSent { + t.Errorf("model %q: temperature sent = %v, want %v (body: %s)", tc.model, sent, tc.wantTempSent, captured) + } + }) + } +} + +// Models like gpt-5.6-luna reject function tools combined with any +// reasoning_effort other than "none" — and their default effort is not +// "none", so omitting the field still 400s. The client must learn the +// constraint from the 400, retry with reasoning_effort "none", and pin it +// for subsequent calls without further failed round-trips. +func TestCall_LearnsReasoningEffortNoneWithTools(t *testing.T) { + tools := []ToolDef{{ + Type: "function", + Function: FunctionDef{ + Name: "echo", + Description: "echoes input", + Parameters: map[string]any{"type": "object"}, + }, + }} + + var mu sync.Mutex + var efforts []string // recorded reasoning_effort per request ("" = absent) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + var parsed map[string]any + _ = json.Unmarshal(body, &parsed) + effort, _ := parsed["reasoning_effort"].(string) + mu.Lock() + efforts = append(efforts, effort) + mu.Unlock() + toolsArr, _ := parsed["tools"].([]any) + if len(toolsArr) > 0 && effort != "none" { + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `{"error":{"message":"Function tools with reasoning_effort are not supported","type":"invalid_request_error","param":"reasoning_effort"}}`) + return + } + fmt.Fprint(w, `{"choices":[{"message":{"content":"ok"}}]}`) + })) + defer server.Close() + + c := New(server.URL, "sk-test", "gpt-5.6-luna", "high", 0, 0) + msgs := []Message{{Role: "user", Content: "hi"}} + + // First call: 400 (effort "high") → learned retry with "none" → success. + if _, err := c.Call(context.Background(), msgs, nil, tools); err != nil { + t.Fatalf("first Call: %v", err) + } + // Second call: must send "none" immediately, no failed attempt first. + if _, err := c.Call(context.Background(), msgs, nil, tools); err != nil { + t.Fatalf("second Call: %v", err) + } + // Calls without tools keep the configured effort. + if _, err := c.Call(context.Background(), msgs, nil, nil); err != nil { + t.Fatalf("tool-less Call: %v", err) + } + + mu.Lock() + defer mu.Unlock() + want := []string{"high", "none", "none", "high"} + if len(efforts) != len(want) { + t.Fatalf("requests = %v, want %v", efforts, want) + } + for i := range want { + if efforts[i] != want[i] { + t.Fatalf("requests = %v, want %v", efforts, want) + } + } +} + +// The thinking configuration must be mapped onto the shape each provider +// accepts: the Anthropic-style "thinking" object for Anthropic/DeepSeek, +// reasoning_effort for OpenAI reasoning models, and nothing otherwise — +// OpenAI rejects unknown top-level parameters with a 400. +func TestBuildCallParams_ThinkingMapping(t *testing.T) { + cases := []struct { + name string + baseURL string + model string + thinking string + wantThinking string // "" = no thinking object + wantEffort string // "" = no reasoning_effort + }{ + {"deepseek enabled", "https://api.deepseek.com/v1", "deepseek-chat", "enabled", "enabled", ""}, + {"deepseek disabled", "https://api.deepseek.com/v1", "deepseek-chat", "disabled", "disabled", ""}, + {"anthropic enabled", "https://api.anthropic.com/v1", "claude-sonnet-4-5", "enabled", "enabled", ""}, + {"anthropic disabled", "https://api.anthropic.com/v1", "claude-sonnet-4-5", "disabled", "disabled", ""}, + {"openai reasoning disabled", "https://api.openai.com/v1", "gpt-5-nano", "disabled", "", "none"}, + {"openai reasoning enabled", "https://api.openai.com/v1", "gpt-5.6-luna", "enabled", "", "high"}, + {"openai reasoning effort passthrough", "https://api.openai.com/v1", "gpt-5-nano", "medium", "", "medium"}, + {"openai non-reasoning disabled", "https://api.openai.com/v1", "gpt-4o-mini", "disabled", "", ""}, + {"openai non-reasoning enabled", "https://api.openai.com/v1", "gpt-4o-mini", "enabled", "", ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := New(tc.baseURL, "sk-test", tc.model, tc.thinking, 0, 0) + body := c.buildCallParams([]Message{{Role: "user", Content: "hi"}}, nil, nil) + + gotThinking := "" + if body.Thinking != nil { + gotThinking = body.Thinking.Type + } + if gotThinking != tc.wantThinking { + t.Errorf("thinking object = %q, want %q", gotThinking, tc.wantThinking) + } + if body.ReasoningEffort != tc.wantEffort { + t.Errorf("reasoning_effort = %q, want %q", body.ReasoningEffort, tc.wantEffort) + } + }) + } +} diff --git a/internal/loop/loop.go b/internal/loop/loop.go index c29f630..2a53bc1 100644 --- a/internal/loop/loop.go +++ b/internal/loop/loop.go @@ -149,10 +149,12 @@ type Engine struct { // across turns — only the memory message changes each iteration. memMsgIdx int - // PromptCaching enables Anthropic/OpenAI/DeepSeek prompt caching markers. - // When enabled, the system prompt and first user message are annotated - // with cache_control markers, and the system prompt is moved to the - // dedicated "system" field for Anthropic compatibility. + // PromptCaching enables Anthropic prompt caching markers. When enabled + // and the LLM endpoint is Anthropic, the system prompt and first user + // message are annotated with cache_control markers, and the system + // prompt is moved to the dedicated "system" field. For non-Anthropic + // endpoints (OpenAI, DeepSeek) the markers are skipped entirely — those + // providers cache automatically or reject the Anthropic request shape. PromptCaching bool // MaxToolParallel controls how many tool calls run concurrently per @@ -818,10 +820,13 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ // THINK (timed) start := time.Now() - // Apply prompt caching markers when enabled + // Apply prompt caching markers when enabled — but only for Anthropic + // endpoints. OpenAI rejects the Anthropic request shape (top-level + // "system" field) with a 400, and DeepSeek caches automatically, + // so markers would be harmful or useless there. var systemBlocks []llm.SystemBlock callMsgs := messages - if e.PromptCaching { + if e.PromptCaching && e.client.IsAnthropic() { callMsgs, systemBlocks = llm.ApplyCacheMarkers(messages) } diff --git a/internal/loop/loop_test.go b/internal/loop/loop_test.go index f3dd77e..0d4e565 100644 --- a/internal/loop/loop_test.go +++ b/internal/loop/loop_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "sort" @@ -2519,3 +2520,46 @@ func TestClassifyToolCall_ShellStillWorks(t *testing.T) { t.Errorf("shell resource = %q, want command", resource) } } + +// Regression test: prompt caching markers are Anthropic-only. When +// PromptCaching is enabled against a non-Anthropic endpoint (e.g. OpenAI), +// the request must not contain the Anthropic-style top-level "system" field +// or cache_control markers — OpenAI rejects them with 400 unknown_parameter. +func TestEngine_PromptCaching_NonAnthropicSkipsMarkers(t *testing.T) { + var captured []byte + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured, _ = io.ReadAll(r.Body) + fmt.Fprint(w, `{"choices":[{"message":{"content":"done"}}]}`) + })) + defer server.Close() + + // server.URL (127.0.0.1) is not an Anthropic endpoint. + client := llm.New(server.URL, "sk-test", "test-model", "", 0, 0) + registry := tool.NewRegistry(nil) + engine := New(client, registry, 10, "You are a test agent.", nil, 0) + engine.PromptCaching = true + + if _, err := engine.Run(context.Background(), "hi"); err != nil { + t.Fatalf("Run() error: %v", err) + } + + var body map[string]any + if err := json.Unmarshal(captured, &body); err != nil { + t.Fatalf("captured request is not JSON: %v", err) + } + if _, ok := body["system"]; ok { + t.Errorf("non-Anthropic request must not contain top-level system field: %s", captured) + } + if strings.Contains(string(captured), "cache_control") { + t.Errorf("non-Anthropic request must not contain cache_control markers: %s", captured) + } + // The system prompt must still reach the model as a system message. + msgs, _ := body["messages"].([]any) + if len(msgs) == 0 { + t.Fatalf("no messages in request: %s", captured) + } + first, _ := msgs[0].(map[string]any) + if first["role"] != "system" { + t.Errorf("first message role = %v, want system", first["role"]) + } +}