Skip to content

Commit 3f4888c

Browse files
committed
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.
1 parent 6a90bc4 commit 3f4888c

4 files changed

Lines changed: 255 additions & 10 deletions

File tree

internal/llm/client.go

Lines changed: 68 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"net/http"
1111
"strconv"
1212
"strings"
13+
"sync/atomic"
1314
"time"
1415

1516
"github.com/BackendStack21/odek/internal/transport"
@@ -25,6 +26,11 @@ type Client struct {
2526
MaxTokens int // max output tokens (0 = provider default)
2627
Temperature float64 // 0 = use provider default, <0 = omit from request
2728
http *http.Client
29+
30+
// forceNoneEffort is learned at runtime: set when the provider rejects
31+
// reasoning_effort combined with function tools (e.g. gpt-5.6-luna),
32+
// so subsequent calls pin effort to "none" without a failed round-trip.
33+
forceNoneEffort atomic.Bool
2834
}
2935

3036
// maxResponseSize limits the LLM response body read to prevent DoS/OOM.
@@ -54,8 +60,35 @@ func NewWithMaxTokens(baseURL, apiKey, model, thinking string, thinkingBudget in
5460
}
5561
}
5662

63+
// IsAnthropic reports whether the client's base URL targets the Anthropic
64+
// API. Anthropic-specific request features (the top-level "system" field,
65+
// cache_control markers) must only be sent when this is true — other
66+
// providers reject them (OpenAI answers 400 unknown_parameter).
67+
func (c *Client) IsAnthropic() bool {
68+
return strings.Contains(c.BaseURL, "anthropic")
69+
}
70+
71+
// modelForbidsTemperature reports whether the model rejects an explicit
72+
// temperature parameter. OpenAI reasoning models (o1/o3/o4 families and the
73+
// gpt-5 series) only accept the default temperature (1); sending any other
74+
// value — including odek's deterministic default 0 — returns a 400
75+
// "unsupported_value" error. Matching is model-name-based and
76+
// provider-agnostic, since OpenAI-compatible proxies serving these model
77+
// IDs enforce the same constraint.
78+
func modelForbidsTemperature(model string) bool {
79+
m := strings.ToLower(model)
80+
for _, prefix := range []string{"o1", "o3", "o4", "gpt-5"} {
81+
if strings.HasPrefix(m, prefix) {
82+
return true
83+
}
84+
}
85+
return false
86+
}
87+
5788
// CacheControl marks a message or system block as cacheable by Anthropic.
58-
// Providers that don't support it (OpenAI, DeepSeek) silently ignore the field.
89+
// Only send it to Anthropic endpoints: OpenAI rejects Anthropic-style
90+
// request shapes with a 400 (e.g. the top-level "system" field), so the
91+
// loop only applies cache markers when Client.IsAnthropic reports true.
5992
type CacheControl struct {
6093
Type string `json:"type"` // "ephemeral"
6194
}
@@ -154,7 +187,8 @@ var toolChoiceNone = "none"
154187
//
155188
// Returns the updated messages and a System field (populated if the system
156189
// message was moved out of the messages array for Anthropic compatibility).
157-
// Providers that don't support prompt caching silently ignore these fields.
190+
// Callers must only use this when the target provider is Anthropic
191+
// (see Client.IsAnthropic) — OpenAI rejects the resulting request shape.
158192
func ApplyCacheMarkers(messages []Message) ([]Message, []SystemBlock) {
159193
var systemBlocks []SystemBlock
160194
annotated := make([]Message, 0, len(messages))
@@ -270,30 +304,60 @@ func (c *Client) Call(ctx context.Context, messages []Message, systemBlocks []Sy
270304
body.Temperature = &one
271305
case "disabled":
272306
body.Thinking = &ThinkingConfig{Type: "disabled"}
273-
if c.Temperature >= 0 {
307+
if c.Temperature >= 0 && !modelForbidsTemperature(c.Model) {
274308
body.Temperature = &c.Temperature
275309
}
276310
default:
277-
if c.Temperature >= 0 {
311+
if c.Temperature >= 0 && !modelForbidsTemperature(c.Model) {
278312
body.Temperature = &c.Temperature
279313
}
280314
if c.Thinking == "low" || c.Thinking == "medium" || c.Thinking == "high" {
281315
body.ReasoningEffort = c.Thinking
282316
}
283317
}
284318

319+
// Some models (e.g. gpt-5.6-luna) reject function tools combined with
320+
// any reasoning_effort other than "none" on /chat/completions — and
321+
// their DEFAULT effort is not "none", so merely omitting the field is
322+
// not enough. Once that constraint has been learned from a 400 (see
323+
// below), pin effort to "none" whenever tools are present.
324+
if c.forceNoneEffort.Load() && len(tools) > 0 {
325+
body.ReasoningEffort = "none"
326+
}
327+
285328
reqBytes, err := json.Marshal(body)
286329
if err != nil {
287330
return nil, fmt.Errorf("llm: marshal request: %w", err)
288331
}
289332

290333
respBytes, err := c.postChatWithRetry(ctx, reqBytes)
334+
if err != nil && len(tools) > 0 && !c.forceNoneEffort.Load() && reasoningEffortRejected(err) {
335+
// Learn the constraint once, then every later call sends
336+
// reasoning_effort "none" directly (no more failed round-trips).
337+
c.forceNoneEffort.Store(true)
338+
body.ReasoningEffort = "none"
339+
reqBytes, mErr := json.Marshal(body)
340+
if mErr != nil {
341+
return nil, fmt.Errorf("llm: marshal request: %w", mErr)
342+
}
343+
respBytes, err = c.postChatWithRetry(ctx, reqBytes)
344+
}
291345
if err != nil {
292346
return nil, err
293347
}
294348
return parseResponse(respBytes)
295349
}
296350

351+
// reasoningEffortRejected reports whether err is a 400 whose provider
352+
// response names reasoning_effort as the offending parameter.
353+
func reasoningEffortRejected(err error) bool {
354+
if err == nil {
355+
return false
356+
}
357+
msg := err.Error()
358+
return strings.Contains(msg, "400") && strings.Contains(msg, `"reasoning_effort"`)
359+
}
360+
297361
// postChatWithRetry POSTs reqBytes to /chat/completions and returns the raw 200
298362
// response body, retrying transient network errors and retryable HTTP statuses
299363
// (429, 502, 503, 504) with exponential backoff. Shared by every chat call so

internal/llm/client_test.go

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@ package llm
33
import (
44
"context"
55
"encoding/json"
6+
"fmt"
7+
"io"
68
"net/http"
79
"net/http/httptest"
10+
"sync"
811
"testing"
912
"time"
1013
)
@@ -1050,3 +1053,132 @@ func TestParseResponse_AnthropicAndOpenAICache(t *testing.T) {
10501053
t.Errorf("CachedTokens = %d, want 999", result.CachedTokens)
10511054
}
10521055
}
1056+
1057+
func TestClient_IsAnthropic(t *testing.T) {
1058+
cases := []struct {
1059+
baseURL string
1060+
want bool
1061+
}{
1062+
{"https://api.anthropic.com/v1", true},
1063+
{"https://api.openai.com/v1", false},
1064+
{"https://api.deepseek.com/v1", false},
1065+
{"http://localhost:11434/v1", false},
1066+
}
1067+
for _, tc := range cases {
1068+
c := New(tc.baseURL, "sk-test", "model", "", 0, 0)
1069+
if got := c.IsAnthropic(); got != tc.want {
1070+
t.Errorf("IsAnthropic(%q) = %v, want %v", tc.baseURL, got, tc.want)
1071+
}
1072+
}
1073+
}
1074+
1075+
// OpenAI reasoning models (o1/o3/o4, gpt-5 family) reject any explicit
1076+
// temperature other than the default (1) with a 400. The client must omit
1077+
// the field for those models while still sending odek's deterministic
1078+
// default (0) to models that accept it.
1079+
func TestCall_OmitsTemperatureForReasoningModels(t *testing.T) {
1080+
cases := []struct {
1081+
model string
1082+
wantTempSent bool
1083+
}{
1084+
{"gpt-5-nano", false},
1085+
{"gpt-5", false},
1086+
{"o3-mini", false},
1087+
{"o1-preview", false},
1088+
{"o4-mini", false},
1089+
{"GPT-5-MINI", false}, // case-insensitive
1090+
{"gpt-4o-mini", true},
1091+
{"deepseek-chat", true},
1092+
{"claude-sonnet-4-5", true},
1093+
}
1094+
1095+
for _, tc := range cases {
1096+
t.Run(tc.model, func(t *testing.T) {
1097+
var captured []byte
1098+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1099+
captured, _ = io.ReadAll(r.Body)
1100+
fmt.Fprint(w, `{"choices":[{"message":{"content":"ok"}}]}`)
1101+
}))
1102+
defer server.Close()
1103+
1104+
c := New(server.URL, "sk-test", tc.model, "", 0, 0)
1105+
c.Temperature = 0 // odek's deterministic default
1106+
if _, err := c.Call(context.Background(), []Message{{Role: "user", Content: "hi"}}, nil, nil); err != nil {
1107+
t.Fatalf("Call: %v", err)
1108+
}
1109+
1110+
var body map[string]any
1111+
if err := json.Unmarshal(captured, &body); err != nil {
1112+
t.Fatalf("captured request is not JSON: %v", err)
1113+
}
1114+
_, sent := body["temperature"]
1115+
if sent != tc.wantTempSent {
1116+
t.Errorf("model %q: temperature sent = %v, want %v (body: %s)", tc.model, sent, tc.wantTempSent, captured)
1117+
}
1118+
})
1119+
}
1120+
}
1121+
1122+
// Models like gpt-5.6-luna reject function tools combined with any
1123+
// reasoning_effort other than "none" — and their default effort is not
1124+
// "none", so omitting the field still 400s. The client must learn the
1125+
// constraint from the 400, retry with reasoning_effort "none", and pin it
1126+
// for subsequent calls without further failed round-trips.
1127+
func TestCall_LearnsReasoningEffortNoneWithTools(t *testing.T) {
1128+
tools := []ToolDef{{
1129+
Type: "function",
1130+
Function: FunctionDef{
1131+
Name: "echo",
1132+
Description: "echoes input",
1133+
Parameters: map[string]any{"type": "object"},
1134+
},
1135+
}}
1136+
1137+
var mu sync.Mutex
1138+
var efforts []string // recorded reasoning_effort per request ("" = absent)
1139+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1140+
body, _ := io.ReadAll(r.Body)
1141+
var parsed map[string]any
1142+
_ = json.Unmarshal(body, &parsed)
1143+
effort, _ := parsed["reasoning_effort"].(string)
1144+
mu.Lock()
1145+
efforts = append(efforts, effort)
1146+
mu.Unlock()
1147+
toolsArr, _ := parsed["tools"].([]any)
1148+
if len(toolsArr) > 0 && effort != "none" {
1149+
w.WriteHeader(http.StatusBadRequest)
1150+
fmt.Fprint(w, `{"error":{"message":"Function tools with reasoning_effort are not supported","type":"invalid_request_error","param":"reasoning_effort"}}`)
1151+
return
1152+
}
1153+
fmt.Fprint(w, `{"choices":[{"message":{"content":"ok"}}]}`)
1154+
}))
1155+
defer server.Close()
1156+
1157+
c := New(server.URL, "sk-test", "gpt-5.6-luna", "high", 0, 0)
1158+
msgs := []Message{{Role: "user", Content: "hi"}}
1159+
1160+
// First call: 400 (effort "high") → learned retry with "none" → success.
1161+
if _, err := c.Call(context.Background(), msgs, nil, tools); err != nil {
1162+
t.Fatalf("first Call: %v", err)
1163+
}
1164+
// Second call: must send "none" immediately, no failed attempt first.
1165+
if _, err := c.Call(context.Background(), msgs, nil, tools); err != nil {
1166+
t.Fatalf("second Call: %v", err)
1167+
}
1168+
// Calls without tools keep the configured effort.
1169+
if _, err := c.Call(context.Background(), msgs, nil, nil); err != nil {
1170+
t.Fatalf("tool-less Call: %v", err)
1171+
}
1172+
1173+
mu.Lock()
1174+
defer mu.Unlock()
1175+
want := []string{"high", "none", "none", "high"}
1176+
if len(efforts) != len(want) {
1177+
t.Fatalf("requests = %v, want %v", efforts, want)
1178+
}
1179+
for i := range want {
1180+
if efforts[i] != want[i] {
1181+
t.Fatalf("requests = %v, want %v", efforts, want)
1182+
}
1183+
}
1184+
}

internal/loop/loop.go

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,12 @@ type Engine struct {
149149
// across turns — only the memory message changes each iteration.
150150
memMsgIdx int
151151

152-
// PromptCaching enables Anthropic/OpenAI/DeepSeek prompt caching markers.
153-
// When enabled, the system prompt and first user message are annotated
154-
// with cache_control markers, and the system prompt is moved to the
155-
// dedicated "system" field for Anthropic compatibility.
152+
// PromptCaching enables Anthropic prompt caching markers. When enabled
153+
// and the LLM endpoint is Anthropic, the system prompt and first user
154+
// message are annotated with cache_control markers, and the system
155+
// prompt is moved to the dedicated "system" field. For non-Anthropic
156+
// endpoints (OpenAI, DeepSeek) the markers are skipped entirely — those
157+
// providers cache automatically or reject the Anthropic request shape.
156158
PromptCaching bool
157159

158160
// MaxToolParallel controls how many tool calls run concurrently per
@@ -818,10 +820,13 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [
818820
// THINK (timed)
819821
start := time.Now()
820822

821-
// Apply prompt caching markers when enabled
823+
// Apply prompt caching markers when enabled — but only for Anthropic
824+
// endpoints. OpenAI rejects the Anthropic request shape (top-level
825+
// "system" field) with a 400, and DeepSeek caches automatically,
826+
// so markers would be harmful or useless there.
822827
var systemBlocks []llm.SystemBlock
823828
callMsgs := messages
824-
if e.PromptCaching {
829+
if e.PromptCaching && e.client.IsAnthropic() {
825830
callMsgs, systemBlocks = llm.ApplyCacheMarkers(messages)
826831
}
827832

internal/loop/loop_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"context"
66
"encoding/json"
77
"fmt"
8+
"io"
89
"net/http"
910
"net/http/httptest"
1011
"sort"
@@ -2519,3 +2520,46 @@ func TestClassifyToolCall_ShellStillWorks(t *testing.T) {
25192520
t.Errorf("shell resource = %q, want command", resource)
25202521
}
25212522
}
2523+
2524+
// Regression test: prompt caching markers are Anthropic-only. When
2525+
// PromptCaching is enabled against a non-Anthropic endpoint (e.g. OpenAI),
2526+
// the request must not contain the Anthropic-style top-level "system" field
2527+
// or cache_control markers — OpenAI rejects them with 400 unknown_parameter.
2528+
func TestEngine_PromptCaching_NonAnthropicSkipsMarkers(t *testing.T) {
2529+
var captured []byte
2530+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
2531+
captured, _ = io.ReadAll(r.Body)
2532+
fmt.Fprint(w, `{"choices":[{"message":{"content":"done"}}]}`)
2533+
}))
2534+
defer server.Close()
2535+
2536+
// server.URL (127.0.0.1) is not an Anthropic endpoint.
2537+
client := llm.New(server.URL, "sk-test", "test-model", "", 0, 0)
2538+
registry := tool.NewRegistry(nil)
2539+
engine := New(client, registry, 10, "You are a test agent.", nil, 0)
2540+
engine.PromptCaching = true
2541+
2542+
if _, err := engine.Run(context.Background(), "hi"); err != nil {
2543+
t.Fatalf("Run() error: %v", err)
2544+
}
2545+
2546+
var body map[string]any
2547+
if err := json.Unmarshal(captured, &body); err != nil {
2548+
t.Fatalf("captured request is not JSON: %v", err)
2549+
}
2550+
if _, ok := body["system"]; ok {
2551+
t.Errorf("non-Anthropic request must not contain top-level system field: %s", captured)
2552+
}
2553+
if strings.Contains(string(captured), "cache_control") {
2554+
t.Errorf("non-Anthropic request must not contain cache_control markers: %s", captured)
2555+
}
2556+
// The system prompt must still reach the model as a system message.
2557+
msgs, _ := body["messages"].([]any)
2558+
if len(msgs) == 0 {
2559+
t.Fatalf("no messages in request: %s", captured)
2560+
}
2561+
first, _ := msgs[0].(map[string]any)
2562+
if first["role"] != "system" {
2563+
t.Errorf("first message role = %v, want system", first["role"])
2564+
}
2565+
}

0 commit comments

Comments
 (0)