diff --git a/.github/pr-assets/590/index.html b/.github/pr-assets/590/index.html new file mode 100644 index 00000000..4aa70cd7 --- /dev/null +++ b/.github/pr-assets/590/index.html @@ -0,0 +1,790 @@ + + + + + + PR #590 — OpenAI Tool Call Compatibility Before / After + + + + +
+ + +
+
+
PR #590 • reviewer artifact
+

OpenAI compatibility fix, shown as behavior not prose.

+

+ This PR has no product UI. The reviewer surface is wire behavior: + how long tool-call IDs are serialized and how prefixed OpenAI-compatible + model IDs are classified. The visuals below show the exact before/after + deltas that matter for issue #532. +

+
+
+
Primary bug
+
Legacy or replayed tool IDs could alias after a 40-char prefix cut.
+
+
+
Secondary bug
+
Prefixed models like openai/o3-mini bypassed capability gates.
+
+
+
Fix shape
+
Hash long IDs at the serializer boundary and normalize model family once.
+
+
+
Scope
+
Covers main loop, resumed history, memory flush, and subagent replay through one path.
+
+
+
+ +
+
01 • Tool ID shortening
+

Before: prefix cut preserved length, not uniqueness.

+

+ The original defensive fallback capped IDs by slicing the first 40 characters. + That stops the HTTP 400, but it still lets two distinct long IDs collapse into + the same wire value if they only diverge after character 40. +

+ +
+
+ Why this was still a blocker +

Fresh live tool calls were already hashed upstream, but resumed history and replay paths still depended on the serializer. That meant the bug could survive in exactly the sessions reviewers care about: legacy, resumed, or replayed state.

+
+
+ Why the serializer fix is the right layer +

Every OpenAI-compatible request path uses the same request builder. Fixing the boundary closes the main loop and the side loops together instead of patching each caller independently.

+
+
+ +
+
+
+
+ Before +

Shared-prefix IDs collide after truncation

+
+
+
+
Collision risk remains on replay
+

+ Two distinct IDs from a replayed or deduped session can become the same + outbound value. The request is shorter, but the assistant tool calls and + tool results are no longer uniquely identifiable. +

+ 
{
+  "tool_calls": [
+    { "id": "call_0123456789abcdef0123456789abcdef012" },
+    { "id": "call_0123456789abcdef0123456789abcdef012" }
+  ],
+  "messages": [
+    { "role": "tool", "tool_call_id": "call_0123456789abcdef0123456789abcdef012" },
+    { "role": "tool", "tool_call_id": "call_0123456789abcdef0123456789abcdef012" }
+  ]
+}
+
    +
  • Distinct legacy IDs differ only after byte 40.
    • +
  • Prefix slicing makes both values identical on the wire.
    • +
  • Parallel tool results can no longer be correlated safely.
    • +
+
+
+ +
+
+
+ After +

Long IDs shorten to stable hashed values

+
+
+
+
40-char cap without aliasing
+

+ IDs longer than 40 characters are now re-encoded as deterministic + call_-prefixed hashes. The same original ID always maps + to the same shortened value, and different long IDs stay different. +

+ 
{
+  "tool_calls": [
+    { "id": "call_6625d2d3e5ec2c6c86f70fd9a7064cc77a1" },
+    { "id": "call_60b8d9101dbdde451723a0888a84317d14f" }
+  ],
+  "messages": [
+    { "role": "tool", "tool_call_id": "call_6625d2d3e5ec2c6c86f70fd9a7064cc77a1" },
+    { "role": "tool", "tool_call_id": "call_60b8d9101dbdde451723a0888a84317d14f" }
+  ]
+}
+
    +
  • Still meets the 40-character API limit.
    • +
  • Preserves assistant/tool-result correlation because the same helper is used for both fields.
    • +
  • Protects replay and legacy sessions, not just newly generated tool calls.
    • +
+
+
+
+
+ +
+
02 • Model capability gating
+

Before: prefixed model IDs skipped the intended reasoning-model rules.

+

+ The request builder already supports provider-prefixed IDs such as + openai/o3-mini. The bug was that capability checks ran on the + full string, so prefixed reasoning models could keep temperature + and use the wrong token field. +

+ +
+
+
+
+ Before +

Prefixed reasoning models look like unknown families

+
+
+
+
Wrong request shape for openai/o3-mini
+

+ Because the gate looked at the full model string, the builder missed + both the reasoning-model token routing and the temperature skip. +

+ 
{
+  "model": "openai/o3-mini",
+  "max_tokens": 123,
+  "temperature": 0.7
+}
+
    +
  • Contradicted the “model-level, not provider-specific” finding.
    • +
  • Left OpenRouter-style OpenAI IDs outside the intended guardrail.
    • +
+
+
+ +
+
+
+ After +

Capability checks use normalized model family

+
+
+
+
Same rules for bare and prefixed IDs
+

+ The builder now strips transport prefixes once, then applies the same + capability logic to both bare and prefixed model IDs. +

+ 
{
+  "model": "openai/o3-mini",
+  "max_completion_tokens": 123
+}
+
    +
  • openai/o3-mini now follows the same route as o3-mini.
    • +
  • openai/gpt-5.4 still keeps temperature, matching the flagship-model rule.
    • +
  • Azure and non-Azure paths are validated as model-based, not URL-based.
    • +
+
+
+
+
+ +
+
03 • Validation
+

Tests and reviewer-facing outcomes

+

+ The changes above were verified with focused provider and agent tests, plus + a full-suite spot check to expose unrelated red tests explicitly rather than + hide them under a green subset. +

+ +
+
+ Passed +
    +
  • go test ./internal/providers ./internal/agent
    • +
  • go test ./internal/providers -run 'TestTruncateToolCallID|TestBuildRequestBody' -count=1
    • +
  • go test ./internal/agent -run '^TestUniquifyToolCallIDs$' -count=1
    • +
+
+
+ Known unrelated failure +

+ go test ./... still fails in pkg/browser at + TestResolveRemoteCDP_DefaultPort with + /json/version returned HTTP 404. This pre-exists the provider + changes and is unrelated to the OpenAI compatibility patch. +

+
+
+ +

+ Review note: these visuals intentionally show behavior and request-shape + diffs rather than product UI, because this PR does not alter application screens. +

+
+
+
+ + + + diff --git a/internal/agent/loop_history_test.go b/internal/agent/loop_history_test.go index e95daa25..33290ff0 100644 --- a/internal/agent/loop_history_test.go +++ b/internal/agent/loop_history_test.go @@ -1,6 +1,7 @@ package agent import ( + "strings" "testing" "github.com/nextlevelbuilder/goclaw/internal/config" @@ -335,17 +336,24 @@ func TestUniquifyToolCallIDs(t *testing.T) { } }) - t.Run("appends run prefix", func(t *testing.T) { + t.Run("produces fixed-length hashed IDs", func(t *testing.T) { calls := []providers.ToolCall{ {ID: "call_123", Name: "read_file"}, {ID: "call_456", Name: "write_file"}, } got := uniquifyToolCallIDs(calls, runID, 2) - if got[0].ID != "call_123_abcdef12_2_0" { - t.Errorf("unexpected ID: %s", got[0].ID) + // IDs must be exactly 40 chars: "call_" (5) + 35 hex chars + for i, tc := range got { + if len(tc.ID) != 40 { + t.Errorf("call %d: ID length = %d, want 40: %s", i, len(tc.ID), tc.ID) + } + if !strings.HasPrefix(tc.ID, "call_") { + t.Errorf("call %d: ID should start with call_, got: %s", i, tc.ID) + } } - if got[1].ID != "call_456_abcdef12_2_1" { - t.Errorf("unexpected ID: %s", got[1].ID) + // Different original IDs must produce different hashed IDs + if got[0].ID == got[1].ID { + t.Errorf("different inputs should produce different IDs: %s", got[0].ID) } }) @@ -354,8 +362,11 @@ func TestUniquifyToolCallIDs(t *testing.T) { {ID: "", Name: "read_file"}, } got := uniquifyToolCallIDs(calls, runID, 0) - if got[0].ID != "call_abcdef12_0_0" { - t.Errorf("unexpected ID for empty: %s", got[0].ID) + if len(got[0].ID) != 40 { + t.Errorf("empty input: ID length = %d, want 40: %s", len(got[0].ID), got[0].ID) + } + if !strings.HasPrefix(got[0].ID, "call_") { + t.Errorf("empty input: ID should start with call_, got: %s", got[0].ID) } }) @@ -382,11 +393,27 @@ func TestUniquifyToolCallIDs(t *testing.T) { t.Errorf("IDs should be unique, both are: %s", got[0].ID) } }) + + t.Run("includes runID and iteration in hash", func(t *testing.T) { + calls := []providers.ToolCall{ + {ID: "same_id", Name: "read_file"}, + } + iter0 := uniquifyToolCallIDs(calls, runID, 0)[0].ID + iter1 := uniquifyToolCallIDs(calls, runID, 1)[0].ID + otherRun := uniquifyToolCallIDs(calls, "12345678-1234-5678-1234-567812345678", 0)[0].ID + + if iter0 == iter1 { + t.Errorf("iteration should affect hashed ID: %s", iter0) + } + if iter0 == otherRun { + t.Errorf("runID should affect hashed ID: %s", iter0) + } + }) } func TestEstimateTokens(t *testing.T) { msgs := []providers.Message{ - {Role: "user", Content: "Hello world!"}, // 12 chars → ~4 tokens + {Role: "user", Content: "Hello world!"}, // 12 chars → ~4 tokens {Role: "assistant", Content: "Hi there, how are you?"}, // 22 chars → ~7 tokens } got := EstimateTokens(msgs) diff --git a/internal/agent/loop_utils.go b/internal/agent/loop_utils.go index e9947003..19c6233f 100644 --- a/internal/agent/loop_utils.go +++ b/internal/agent/loop_utils.go @@ -2,6 +2,8 @@ package agent import ( "context" + "crypto/sha256" + "encoding/hex" "fmt" "log/slog" "path/filepath" @@ -153,9 +155,12 @@ func (l *Loop) ProviderName() string { } // uniquifyToolCallIDs ensures all tool call IDs are globally unique across the -// transcript by appending a short run-ID prefix and iteration index. +// transcript by hashing the original ID with run-ID, iteration, and index. // Returns a new slice (does not mutate the input). // +// IDs are capped at 40 characters to comply with the OpenAI/Azure API limit +// on tool_calls[].id and tool_call_id fields (undocumented, returns HTTP 400). +// // Some OpenAI-compatible APIs (OpenRouter, vLLM, DeepSeek) return duplicate IDs // within a single response or reuse IDs from earlier turns, causing HTTP 400. // Using the run UUID guarantees cross-turn uniqueness without history rewriting. @@ -163,18 +168,14 @@ func uniquifyToolCallIDs(calls []providers.ToolCall, runID string, iteration int if len(calls) == 0 { return calls } - short := runID - if len(short) > 8 { - short = short[:8] - } out := make([]providers.ToolCall, len(calls)) copy(out, calls) for i := range out { - if out[i].ID == "" { - out[i].ID = fmt.Sprintf("call_%s_%d_%d", short, iteration, i) - } else { - out[i].ID = fmt.Sprintf("%s_%s_%d_%d", out[i].ID, short, iteration, i) - } + // Hash all discriminating components into a fixed-length ID: + // "call_" (5 chars) + hex(sha256(id:runID:iter:idx))[:35] = 40 chars exactly. + raw := fmt.Sprintf("%s:%s:%d:%d", out[i].ID, runID, iteration, i) + h := sha256.Sum256([]byte(raw)) + out[i].ID = "call_" + hex.EncodeToString(h[:])[:35] } return out } diff --git a/internal/providers/openai.go b/internal/providers/openai.go index 4ee77f81..ed29e666 100644 --- a/internal/providers/openai.go +++ b/internal/providers/openai.go @@ -4,6 +4,8 @@ import ( "bufio" "bytes" "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -347,7 +349,7 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream } } toolCalls[i] = map[string]any{ - "id": tc.ID, + "id": truncateToolCallID(tc.ID), "type": "function", "function": fn, } @@ -356,7 +358,7 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream } if m.ToolCallID != "" { - msg["tool_call_id"] = m.ToolCallID + msg["tool_call_id"] = truncateToolCallID(m.ToolCallID) } msgs = append(msgs, msg) @@ -391,16 +393,20 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream } // Merge options + capabilityModel := modelFamily(model) if v, ok := req.Options[OptMaxTokens]; ok { - if strings.HasPrefix(model, "gpt-5") || strings.HasPrefix(model, "o1") || strings.HasPrefix(model, "o3") || strings.HasPrefix(model, "o4") { + if strings.HasPrefix(capabilityModel, "gpt-5") || strings.HasPrefix(capabilityModel, "o1") || strings.HasPrefix(capabilityModel, "o3") || strings.HasPrefix(capabilityModel, "o4") { body["max_completion_tokens"] = v } else { body["max_tokens"] = v } } if v, ok := req.Options[OptTemperature]; ok { - // GPT-5 mini/nano and o-series models only support default temperature - skipTemp := strings.HasPrefix(model, "gpt-5-mini") || strings.HasPrefix(model, "gpt-5-nano") || strings.HasPrefix(model, "o1") || strings.HasPrefix(model, "o3") || strings.HasPrefix(model, "o4") + // Certain model families don't support custom temperature (locked to default). + // This is a model-level constraint, not provider-specific — applies to both OpenAI and Azure. + // Note: gpt-5.X flagship models (gpt-5.1, gpt-5.4) DO support temperature; + // only the mini/nano reasoning variants reject it. + skipTemp := strings.HasPrefix(capabilityModel, "gpt-5-mini") || strings.HasPrefix(capabilityModel, "gpt-5-nano") || strings.HasPrefix(capabilityModel, "o1") || strings.HasPrefix(capabilityModel, "o3") || strings.HasPrefix(capabilityModel, "o4") if !skipTemp { body["temperature"] = v } @@ -422,6 +428,15 @@ func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream return body } +// modelFamily strips provider prefixes (for example "openai/o3-mini") so capability +// gates apply to the actual model family rather than the transport-specific wrapper. +func modelFamily(model string) string { + if idx := strings.LastIndex(model, "/"); idx >= 0 && idx < len(model)-1 { + return model[idx+1:] + } + return model +} + func (p *OpenAIProvider) doRequest(ctx context.Context, body any) (io.ReadCloser, error) { data, err := json.Marshal(body) if err != nil { @@ -548,3 +563,20 @@ func clampedLimit(body map[string]any) any { } return body["max_tokens"] } + +const maxToolCallIDLen = 40 + +// truncateToolCallID deterministically fits tool call IDs into OpenAI's 40-char +// limit. Prefix truncation can alias distinct legacy IDs that only diverge after +// byte 40, so we hash the full original ID when shortening is needed. +// +// Fresh tool calls from the agent loop already go through uniquifyToolCallIDs +// (which produces 40-char hashed IDs), so this is a no-op for those. This +// function catches replayed/legacy history entries that bypassed uniquification. +func truncateToolCallID(id string) string { + if len(id) <= maxToolCallIDLen { + return id + } + hash := sha256.Sum256([]byte(id)) + return "call_" + hex.EncodeToString(hash[:])[:maxToolCallIDLen-len("call_")] +} diff --git a/internal/providers/openai_test.go b/internal/providers/openai_test.go new file mode 100644 index 00000000..b80658cf --- /dev/null +++ b/internal/providers/openai_test.go @@ -0,0 +1,260 @@ +package providers + +import ( + "strings" + "testing" +) + +func TestTruncateToolCallID(t *testing.T) { + t.Run("short IDs stay unchanged", func(t *testing.T) { + ids := []string{ + "", + "call_abc123", + "call_0123456789abcdef0123456789abcdef012", // exactly 40 chars + } + for _, id := range ids { + if got := truncateToolCallID(id); got != id { + t.Errorf("truncateToolCallID(%q) = %q, want unchanged", id, got) + } + } + }) + + t.Run("long IDs are shortened deterministically", func(t *testing.T) { + id := "call_0123456789abcdef0123456789abcdef01234" + got1 := truncateToolCallID(id) + got2 := truncateToolCallID(id) + if got1 != got2 { + t.Fatalf("truncateToolCallID(%q) should be deterministic: %q != %q", id, got1, got2) + } + if len(got1) != maxToolCallIDLen { + t.Fatalf("truncateToolCallID(%q) length = %d, want %d", id, len(got1), maxToolCallIDLen) + } + if got1 == id { + t.Fatalf("truncateToolCallID(%q) should shorten long IDs", id) + } + if !strings.HasPrefix(got1, "call_") { + t.Fatalf("truncateToolCallID(%q) should preserve call_ prefix, got %q", id, got1) + } + }) + + t.Run("shared-prefix long IDs stay unique", func(t *testing.T) { + prefix40 := "call_0123456789abcdef0123456789abcdef012" + id1 := prefix40 + "_0" + id2 := prefix40 + "_1" + got1 := truncateToolCallID(id1) + got2 := truncateToolCallID(id2) + if got1 == got2 { + t.Fatalf("shared-prefix IDs collided after shortening: %q", got1) + } + if len(got1) > maxToolCallIDLen || len(got2) > maxToolCallIDLen { + t.Fatalf("shared-prefix IDs should be <= %d chars: %q / %q", maxToolCallIDLen, got1, got2) + } + }) +} + +func TestBuildRequestBody_TemperatureSkippedForReasoningModels(t *testing.T) { + p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4") + + // These model families don't support custom temperature (locked to default). + // This is a model-level constraint, not provider-specific. + models := []string{ + "gpt-5-mini", + "gpt-5-mini-2025-01", + "gpt-5-nano", + "o1", + "o1-mini", + "o1-preview", + "o3", + "o3-mini", + "o4-mini", + "openai/gpt-5-mini", + "openai/o3-mini", + } + + for _, model := range models { + t.Run(model, func(t *testing.T) { + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "test"}}, + Options: map[string]any{ + OptTemperature: 0.7, + }, + } + body := p.buildRequestBody(model, req, false) + if _, hasTemp := body["temperature"]; hasTemp { + t.Errorf("model %q: temperature should be skipped but was included", model) + } + }) + } +} + +func TestBuildRequestBody_TemperatureKeptForNonReasoningModels(t *testing.T) { + p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4") + + // These models DO support custom temperature -- must not be suppressed. + models := []string{ + "gpt-4", + "gpt-4o", + "gpt-4-turbo", + "gpt-5", + "gpt-5.1", + "gpt-5.4", + "openai/gpt-5.4", + "claude-3-sonnet", + "llama-3", + } + + for _, model := range models { + t.Run(model, func(t *testing.T) { + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "test"}}, + Options: map[string]any{ + OptTemperature: 0.7, + }, + } + body := p.buildRequestBody(model, req, false) + if _, hasTemp := body["temperature"]; !hasTemp { + t.Errorf("model %q: temperature should be included but was skipped", model) + } + }) + } +} + +func TestBuildRequestBody_TemperatureDependsOnModelNotAPIBase(t *testing.T) { + p := NewOpenAIProvider("azure", "key", "https://example.openai.azure.com/openai/deployments/test", "gpt-4") + + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "test"}}, + Options: map[string]any{ + OptTemperature: 0.7, + }, + } + + body := p.buildRequestBody("gpt-5.4", req, false) + if _, hasTemp := body["temperature"]; !hasTemp { + t.Fatal("azure-backed gpt-5.4 should keep temperature; gating must stay model-based") + } +} + +func TestBuildRequestBody_PrefixedModelsUseCorrectTokenField(t *testing.T) { + p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4") + + tests := []struct { + model string + wantCompletionKey bool + }{ + {model: "openai/o3-mini", wantCompletionKey: true}, + {model: "openai/gpt-5.4", wantCompletionKey: true}, + {model: "openai/gpt-4o", wantCompletionKey: false}, + } + + for _, tt := range tests { + t.Run(tt.model, func(t *testing.T) { + req := ChatRequest{ + Messages: []Message{{Role: "user", Content: "test"}}, + Options: map[string]any{ + OptMaxTokens: 123, + }, + } + body := p.buildRequestBody(tt.model, req, false) + _, hasCompletionKey := body["max_completion_tokens"] + _, hasMaxTokens := body["max_tokens"] + if hasCompletionKey != tt.wantCompletionKey { + t.Fatalf("model %q: max_completion_tokens present = %v, want %v", tt.model, hasCompletionKey, tt.wantCompletionKey) + } + if hasMaxTokens == tt.wantCompletionKey { + t.Fatalf("model %q: max_tokens/max_completion_tokens routing incorrect: body=%v", tt.model, body) + } + }) + } +} + +func TestBuildRequestBody_ToolCallIDsTruncated(t *testing.T) { + p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4") + + longID := "call_0123456789abcdef0123456789abcdef01234" // 42 chars + + req := ChatRequest{ + Messages: []Message{ + { + Role: "assistant", + ToolCalls: []ToolCall{ + {ID: longID, Name: "test_fn", Arguments: map[string]any{"arg": "val"}}, + }, + }, + { + Role: "tool", + ToolCallID: longID, + Content: "result", + }, + {Role: "user", Content: "continue"}, + }, + } + + body := p.buildRequestBody("gpt-4", req, false) + msgs := body["messages"].([]map[string]any) + + var assistantID, toolResultID string + for _, msg := range msgs { + if tcs, ok := msg["tool_calls"]; ok { + toolCalls := tcs.([]map[string]any) + assistantID = toolCalls[0]["id"].(string) + if len(assistantID) > 40 { + t.Errorf("tool_calls[0].id length = %d, want <= 40", len(assistantID)) + } + } + if tcid, ok := msg["tool_call_id"]; ok { + toolResultID = tcid.(string) + if len(toolResultID) > 40 { + t.Errorf("tool_call_id length = %d, want <= 40", len(toolResultID)) + } + } + } + + // Critical: truncated IDs must match for API correlation + if assistantID != toolResultID { + t.Errorf("ID correlation broken: tool_calls.id=%q != tool_call_id=%q", assistantID, toolResultID) + } +} + +func TestBuildRequestBody_LegacyLongToolCallIDsStayUnique(t *testing.T) { + p := NewOpenAIProvider("test", "key", "https://api.openai.com/v1", "gpt-4") + + prefix40 := "call_0123456789abcdef0123456789abcdef012" + id1 := prefix40 + "_0" + id2 := prefix40 + "_1" + + req := ChatRequest{ + Messages: []Message{ + { + Role: "assistant", + ToolCalls: []ToolCall{ + {ID: id1, Name: "fn1", Arguments: map[string]any{}}, + {ID: id2, Name: "fn2", Arguments: map[string]any{}}, + }, + }, + {Role: "tool", ToolCallID: id1, Content: "result-1"}, + {Role: "tool", ToolCallID: id2, Content: "result-2"}, + {Role: "user", Content: "continue"}, + }, + } + + body := p.buildRequestBody("gpt-4", req, false) + msgs := body["messages"].([]map[string]any) + + toolCalls := msgs[0]["tool_calls"].([]map[string]any) + assistantID1 := toolCalls[0]["id"].(string) + assistantID2 := toolCalls[1]["id"].(string) + if assistantID1 == assistantID2 { + t.Fatalf("legacy long IDs collided after shortening: %q", assistantID1) + } + if len(assistantID1) > maxToolCallIDLen || len(assistantID2) > maxToolCallIDLen { + t.Fatalf("assistant IDs should be <= %d chars: %q / %q", maxToolCallIDLen, assistantID1, assistantID2) + } + + if got := msgs[1]["tool_call_id"].(string); got != assistantID1 { + t.Fatalf("first tool result ID = %q, want %q", got, assistantID1) + } + if got := msgs[2]["tool_call_id"].(string); got != assistantID2 { + t.Fatalf("second tool result ID = %q, want %q", got, assistantID2) + } +}