fix(llm): OpenAI provider compatibility for caching, temperature, and tool calls (#98)

* fix(llm): OpenAI provider compatibility for caching, temperature, and tool calls

Three request-shape bugs surfaced when running against the real OpenAI
API (DeepSeek tolerates all three, which is why they went unnoticed):

1. Anthropic prompt-caching shape sent to every provider. With
   prompt_caching enabled, the loop moved the system prompt into the
   Anthropic-only top-level "system" field and added cache_control
   markers; OpenAI rejects this with 400 unknown_parameter. Markers are
   now only applied when the endpoint is Anthropic (new
   Client.IsAnthropic). OpenAI and DeepSeek cache automatically.

2. temperature:0 sent to reasoning models. odek defaults to
   temperature 0 for determinism, but OpenAI reasoning models
   (o1/o3/o4, gpt-5 series) only accept the default (1) and answer 400
   unsupported_value. The field is now omitted for those models;
   behavior for all other providers/models is unchanged.

3. Function tools + reasoning_effort rejected by gpt-5.6 models.
   gpt-5.6-luna/sol reject tools combined with any reasoning_effort
   other than "none" — and their default effort is not "none", so
   omitting the field still 400s. The client now learns the constraint
   from the 400 (atomic flag), retries once with effort "none", and
   pins it for subsequent tool-bearing calls; tool-less calls keep the
   configured effort.

Verified live against api.openai.com (gpt-4o-mini, gpt-5-nano,
gpt-5.6-luna, gpt-5.6-sol — basic runs, tool round-trips, automatic
prompt caching) and regression-checked against DeepSeek v4 Flash
(native cache metrics and temperature:0 default intact).

Regression tests: TestClient_IsAnthropic,
TestCall_OmitsTemperatureForReasoningModels,
TestCall_LearnsReasoningEffortNoneWithTools,
TestEngine_PromptCaching_NonAnthropicSkipsMarkers.

* fix(llm): map thinking enabled/disabled per provider instead of always sending the Anthropic object

The thinking object ({"type":"enabled"/"disabled"}) was sent
unconditionally; OpenAI rejects unknown top-level parameters with a
400, so thinking: disabled (and enabled) was broken against OpenAI —
same family as the system-field bug fixed earlier in this branch.

The shaping logic now lives in an extracted, directly testable
buildCallParams method:

- Anthropic/DeepSeek (new sendsThinkingObject gate): unchanged,
  thinking object sent as before.
- OpenAI reasoning models (o1/o3/o4, gpt-5 series): enabled maps to
  reasoning_effort "high", disabled to "none" (verified accepted on
  the gpt-5 series); temperature stays omitted per the forbidlist.
- Non-reasoning models (e.g. gpt-4o): no thinking capability exists,
  so both fields are omitted and temperature is honored as configured.

The two existing thinking tests asserted the old unconditional
behavior through an httptest server whose URL is never a recognized
provider; they now exercise buildCallParams directly. New table test
TestBuildCallParams_ThinkingMapping covers all provider/model/thinking
combinations.

Author: jkyberneees

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,45 +297,104 @@ 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" {
 			body.ReasoningEffort = c.Thinking
 		}
 	}
 
+	// 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