fix: cap tool call IDs to 40 chars via hash-based uniquification (#590)

Closes #532

- Replace prefix truncation with SHA-256 hash-based shortening for oversized tool call IDs (40-char OpenAI/Azure limit)
- Normalize provider-prefixed model IDs (e.g. openai/o3-mini) before capability checks for temperature and max_completion_tokens
- Add regression tests for ID collision, correlation, and prefixed model routing
This commit is contained in:
Kai (Tam Nhu) Tran authored and GitHub committed 2026-03-31 08:10:01 +07:00
1 parent 3ca3bb2062
commit 343b530480
5 files changed
+1133 -23

No files matched your search

+790
View File
@@ -0,0 +1,790 @@
<!doctype html>
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>PR #590 — OpenAI Tool Call Compatibility Before / After</title>
<style>
:root {
--font-body: "DM Sans", "Segoe UI", sans-serif;
--font-mono: "Fira Code", "SFMono-Regular", monospace;
--bg: #f0f4f8;
--surface: #ffffff;
--surface2: #e8eef4;
--surface-elevated: #f7fbff;
--border: rgba(30, 60, 100, 0.12);
--border-bright: rgba(30, 60, 100, 0.24);
--text: #16202d;
--text-dim: #5a7090;
--text-bright: #0c1622;
--accent: #1a5fa8;
--accent-dim: rgba(26, 95, 168, 0.1);
--green: #157347;
--green-dim: rgba(21, 115, 71, 0.1);
--red: #b42318;
--red-dim: rgba(180, 35, 24, 0.1);
--amber: #b7791f;
--amber-dim: rgba(183, 121, 31, 0.1);
--teal: #0f766e;
--teal-dim: rgba(15, 118, 110, 0.1);
--shadow: 0 18px 54px rgba(15, 23, 42, 0.1);
}
@media (prefers-color-scheme: dark) {
:root:not([data-theme="light"]) {
--bg: #0d1421;
--surface: #111d2e;
--surface2: #162438;
--surface-elevated: #1b2b42;
--border: rgba(100, 160, 220, 0.14);
--border-bright: rgba(100, 160, 220, 0.28);
--text: #d9e5f2;
--text-dim: #8ea3bb;
--text-bright: #f4f8fc;
--accent: #6eb4ff;
--accent-dim: rgba(110, 180, 255, 0.12);
--green: #4ade80;
--green-dim: rgba(74, 222, 128, 0.12);
--red: #ff8a8a;
--red-dim: rgba(255, 138, 138, 0.12);
--amber: #f6c453;
--amber-dim: rgba(246, 196, 83, 0.12);
--teal: #67e8f9;
--teal-dim: rgba(103, 232, 249, 0.12);
--shadow: 0 20px 64px rgba(2, 8, 23, 0.42);
}
}
[data-theme="dark"] {
--bg: #0d1421;
--surface: #111d2e;
--surface2: #162438;
--surface-elevated: #1b2b42;
--border: rgba(100, 160, 220, 0.14);
--border-bright: rgba(100, 160, 220, 0.28);
--text: #d9e5f2;
--text-dim: #8ea3bb;
--text-bright: #f4f8fc;
--accent: #6eb4ff;
--accent-dim: rgba(110, 180, 255, 0.12);
--green: #4ade80;
--green-dim: rgba(74, 222, 128, 0.12);
--red: #ff8a8a;
--red-dim: rgba(255, 138, 138, 0.12);
--amber: #f6c453;
--amber-dim: rgba(246, 196, 83, 0.12);
--teal: #67e8f9;
--teal-dim: rgba(103, 232, 249, 0.12);
--shadow: 0 20px 64px rgba(2, 8, 23, 0.42);
}
* {
box-sizing: border-box;
}
html {
scroll-behavior: smooth;
}
body {
margin: 0;
padding: 24px;
font-family: var(--font-body);
color: var(--text);
background:
radial-gradient(circle at top left, var(--accent-dim) 0%, transparent 26%),
linear-gradient(180deg, rgba(255, 255, 255, 0.12), rgba(255, 255, 255, 0) 24%),
var(--bg);
background-image:
radial-gradient(circle, var(--border) 1px, transparent 1px),
radial-gradient(circle at top left, var(--accent-dim) 0%, transparent 26%),
linear-gradient(180deg, rgba(255, 255, 255, 0.12), rgba(255, 255, 255, 0) 24%);
background-size: 24px 24px, auto, auto;
}
a {
color: inherit;
}
.theme-toggle {
position: fixed;
top: 16px;
right: 16px;
z-index: 300;
width: 36px;
height: 36px;
border-radius: 8px;
border: 1px solid var(--border);
background: var(--surface);
color: var(--text-dim);
cursor: pointer;
display: flex;
align-items: center;
justify-content: center;
font-size: 16px;
transition: background 0.15s, color 0.15s;
box-shadow: 0 1px 4px rgba(0, 0, 0, 0.06);
}
.theme-toggle:hover {
background: var(--surface2);
color: var(--text);
}
.wrap {
max-width: 1400px;
margin: 0 auto;
display: grid;
grid-template-columns: 190px 1fr;
gap: 0 36px;
}
.toc {
position: sticky;
top: 24px;
align-self: start;
padding: 14px 0;
max-height: calc(100dvh - 48px);
overflow-y: auto;
}
.toc-title {
font-family: var(--font-mono);
font-size: 11px;
font-weight: 700;
letter-spacing: 0.16em;
text-transform: uppercase;
color: var(--text-dim);
padding-bottom: 10px;
margin-bottom: 8px;
border-bottom: 1px solid var(--border);
}
.toc a {
display: block;
padding: 6px 8px;
margin-bottom: 2px;
border-left: 2px solid transparent;
border-radius: 6px;
text-decoration: none;
color: var(--text-dim);
font-size: 12px;
}
.toc a:hover,
.toc a.active {
color: var(--text);
background: var(--surface);
border-left-color: var(--accent);
}
.main {
min-width: 0;
}
.hero,
.section,
.comparison-card {
border: 1px solid var(--border);
background: linear-gradient(180deg, var(--surface), var(--surface-elevated));
box-shadow: var(--shadow);
}
.hero {
padding: 28px;
border-radius: 28px;
}
.eyebrow {
display: inline-flex;
align-items: center;
gap: 10px;
padding: 7px 12px;
border-radius: 999px;
border: 1px solid var(--border-bright);
background: var(--accent-dim);
color: var(--accent);
font-family: var(--font-mono);
font-size: 12px;
font-weight: 700;
letter-spacing: 0.08em;
text-transform: uppercase;
}
.hero h1 {
margin: 16px 0 12px;
font-size: clamp(34px, 5vw, 58px);
line-height: 0.98;
letter-spacing: -0.05em;
color: var(--text-bright);
}
.hero p {
max-width: 980px;
margin: 0;
font-size: 17px;
line-height: 1.7;
color: var(--text-dim);
}
.hero-grid {
display: grid;
grid-template-columns: repeat(4, minmax(0, 1fr));
gap: 14px;
margin-top: 22px;
}
.hero-stat {
padding: 15px 16px;
border-radius: 18px;
background: var(--surface2);
border: 1px solid var(--border);
}
.hero-stat__label {
font-family: var(--font-mono);
font-size: 11px;
font-weight: 700;
letter-spacing: 0.12em;
text-transform: uppercase;
color: var(--text-dim);
}
.hero-stat__value {
margin-top: 8px;
font-size: 15px;
line-height: 1.5;
}
.section {
margin-top: 24px;
border-radius: 24px;
padding: 24px;
}
.section-label {
display: inline-flex;
align-items: center;
gap: 9px;
margin-bottom: 10px;
color: var(--text-dim);
font-family: var(--font-mono);
font-size: 12px;
font-weight: 700;
letter-spacing: 0.12em;
text-transform: uppercase;
}
.section-label::before {
content: "";
width: 8px;
height: 8px;
border-radius: 999px;
background: var(--accent);
box-shadow: 0 0 0 6px var(--accent-dim);
}
.section h2 {
margin: 0;
font-size: 28px;
letter-spacing: -0.04em;
}
.section-intro {
margin: 10px 0 0;
font-size: 15px;
line-height: 1.7;
color: var(--text-dim);
}
.callout-grid,
.validation-grid {
display: grid;
grid-template-columns: repeat(2, minmax(0, 1fr));
gap: 14px;
margin-top: 18px;
}
.callout,
.validation-card {
border-radius: 18px;
padding: 16px;
border: 1px solid var(--border);
background: var(--surface2);
}
.callout strong,
.validation-card strong {
display: block;
margin-bottom: 8px;
font-size: 14px;
}
.callout p,
.validation-card p,
.validation-card ul {
margin: 0;
color: var(--text-dim);
font-size: 14px;
line-height: 1.65;
}
.comparison-grid {
display: grid;
grid-template-columns: repeat(2, minmax(0, 1fr));
gap: 18px;
margin-top: 22px;
}
.comparison-card {
border-radius: 22px;
overflow: hidden;
}
.comparison-head {
display: flex;
align-items: center;
justify-content: space-between;
gap: 12px;
padding: 16px 18px;
border-bottom: 1px solid var(--border);
background: rgba(255, 255, 255, 0.02);
}
.comparison-title {
display: flex;
align-items: center;
gap: 10px;
}
.comparison-title h3 {
margin: 0;
font-size: 19px;
letter-spacing: -0.03em;
}
.tag {
display: inline-flex;
align-items: center;
gap: 8px;
padding: 6px 10px;
border-radius: 999px;
font-family: var(--font-mono);
font-size: 11px;
font-weight: 700;
letter-spacing: 0.08em;
text-transform: uppercase;
}
.tag.before {
color: var(--red);
background: var(--red-dim);
border: 1px solid rgba(180, 35, 24, 0.18);
}
.tag.after {
color: var(--green);
background: var(--green-dim);
border: 1px solid rgba(21, 115, 71, 0.18);
}
.comparison-body {
padding: 18px;
}
.comparison-body p {
margin: 0 0 12px;
color: var(--text-dim);
font-size: 14px;
line-height: 1.65;
}
.status-line {
display: flex;
align-items: center;
gap: 10px;
padding: 10px 12px;
margin-bottom: 14px;
border-radius: 14px;
font-family: var(--font-mono);
font-size: 12px;
font-weight: 700;
letter-spacing: 0.04em;
}
.status-line.bad {
background: var(--red-dim);
color: var(--red);
}
.status-line.good {
background: var(--green-dim);
color: var(--green);
}
pre {
margin: 0;
padding: 16px;
border-radius: 16px;
border: 1px solid var(--border);
background: #0b1320;
color: #d9e5f2;
overflow-x: auto;
font-family: var(--font-mono);
font-size: 12px;
line-height: 1.7;
}
[data-theme="light"] pre,
:root pre {
background: #102033;
}
.bullet-list {
margin: 14px 0 0;
padding-left: 18px;
color: var(--text-dim);
font-size: 14px;
line-height: 1.65;
}
.bullet-list li + li {
margin-top: 8px;
}
.validation-list {
margin: 0;
padding-left: 18px;
}
.footer-note {
margin-top: 20px;
font-size: 13px;
color: var(--text-dim);
}
@media (max-width: 1000px) {
body {
padding: 16px;
}
.wrap {
grid-template-columns: 1fr;
}
.toc {
position: sticky;
top: 0;
z-index: 200;
display: flex;
gap: 6px;
overflow-x: auto;
max-height: none;
padding: 10px 0 12px;
margin: 0 -16px 12px;
padding-left: 16px;
padding-right: 16px;
background: var(--bg);
border-bottom: 1px solid var(--border);
}
.toc-title {
display: none;
}
.toc a {
white-space: nowrap;
border-left: none;
border-bottom: 2px solid transparent;
}
.hero-grid,
.callout-grid,
.validation-grid,
.comparison-grid {
grid-template-columns: 1fr;
}
}
</style>
</head>
<body>
<button class="theme-toggle" id="themeToggle" title="Toggle theme" aria-label="Toggle light/dark theme"></button>
<div class="wrap">
<nav class="toc" id="toc">
<div class="toc-title">Contents</div>
<a href="#overview">Overview</a>
<a href="#tool-id-fix">Tool ID shortening</a>
<a href="#model-gating-fix">Model gating</a>
<a href="#validation">Validation</a>
</nav>
<main class="main">
<section class="hero" id="overview">
<div class="eyebrow">PR #590 • reviewer artifact</div>
<h1>OpenAI compatibility fix, shown as behavior not prose.</h1>
<p>
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.
</p>
<div class="hero-grid">
<div class="hero-stat">
<div class="hero-stat__label">Primary bug</div>
<div class="hero-stat__value">Legacy or replayed tool IDs could alias after a 40-char prefix cut.</div>
</div>
<div class="hero-stat">
<div class="hero-stat__label">Secondary bug</div>
<div class="hero-stat__value">Prefixed models like <code>openai/o3-mini</code> bypassed capability gates.</div>
</div>
<div class="hero-stat">
<div class="hero-stat__label">Fix shape</div>
<div class="hero-stat__value">Hash long IDs at the serializer boundary and normalize model family once.</div>
</div>
<div class="hero-stat">
<div class="hero-stat__label">Scope</div>
<div class="hero-stat__value">Covers main loop, resumed history, memory flush, and subagent replay through one path.</div>
</div>
</div>
</section>
<section class="section" id="tool-id-fix">
<div class="section-label">01 • Tool ID shortening</div>
<h2>Before: prefix cut preserved length, not uniqueness.</h2>
<p class="section-intro">
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.
</p>
<div class="callout-grid">
<div class="callout">
<strong>Why this was still a blocker</strong>
<p>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.</p>
</div>
<div class="callout">
<strong>Why the serializer fix is the right layer</strong>
<p>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.</p>
</div>
</div>
<div class="comparison-grid">
<article class="comparison-card" id="before-tool-id-card">
<div class="comparison-head">
<div class="comparison-title">
<span class="tag before">Before</span>
<h3>Shared-prefix IDs collide after truncation</h3>
</div>
</div>
<div class="comparison-body">
<div class="status-line bad">Collision risk remains on replay</div>
<p>
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.
</p>
<pre>{
"tool_calls": [
{ "id": "call_0123456789abcdef0123456789abcdef012" },
{ "id": "call_0123456789abcdef0123456789abcdef012" }
],
"messages": [
{ "role": "tool", "tool_call_id": "call_0123456789abcdef0123456789abcdef012" },
{ "role": "tool", "tool_call_id": "call_0123456789abcdef0123456789abcdef012" }
]
}</pre>
<ul class="bullet-list">
<li>Distinct legacy IDs differ only after byte 40.</li>
<li>Prefix slicing makes both values identical on the wire.</li>
<li>Parallel tool results can no longer be correlated safely.</li>
</ul>
</div>
</article>
<article class="comparison-card" id="after-tool-id-card">
<div class="comparison-head">
<div class="comparison-title">
<span class="tag after">After</span>
<h3>Long IDs shorten to stable hashed values</h3>
</div>
</div>
<div class="comparison-body">
<div class="status-line good">40-char cap without aliasing</div>
<p>
IDs longer than 40 characters are now re-encoded as deterministic
<code>call_</code>-prefixed hashes. The same original ID always maps
to the same shortened value, and different long IDs stay different.
</p>
<pre>{
"tool_calls": [
{ "id": "call_6625d2d3e5ec2c6c86f70fd9a7064cc77a1" },
{ "id": "call_60b8d9101dbdde451723a0888a84317d14f" }
],
"messages": [
{ "role": "tool", "tool_call_id": "call_6625d2d3e5ec2c6c86f70fd9a7064cc77a1" },
{ "role": "tool", "tool_call_id": "call_60b8d9101dbdde451723a0888a84317d14f" }
]
}</pre>
<ul class="bullet-list">
<li>Still meets the 40-character API limit.</li>
<li>Preserves assistant/tool-result correlation because the same helper is used for both fields.</li>
<li>Protects replay and legacy sessions, not just newly generated tool calls.</li>
</ul>
</div>
</article>
</div>
</section>
<section class="section" id="model-gating-fix">
<div class="section-label">02 • Model capability gating</div>
<h2>Before: prefixed model IDs skipped the intended reasoning-model rules.</h2>
<p class="section-intro">
The request builder already supports provider-prefixed IDs such as
<code>openai/o3-mini</code>. The bug was that capability checks ran on the
full string, so prefixed reasoning models could keep <code>temperature</code>
and use the wrong token field.
</p>
<div class="comparison-grid">
<article class="comparison-card" id="before-gating-card">
<div class="comparison-head">
<div class="comparison-title">
<span class="tag before">Before</span>
<h3>Prefixed reasoning models look like unknown families</h3>
</div>
</div>
<div class="comparison-body">
<div class="status-line bad">Wrong request shape for <code>openai/o3-mini</code></div>
<p>
Because the gate looked at the full model string, the builder missed
both the reasoning-model token routing and the temperature skip.
</p>
<pre>{
"model": "openai/o3-mini",
"max_tokens": 123,
"temperature": 0.7
}</pre>
<ul class="bullet-list">
<li>Contradicted the “model-level, not provider-specific” finding.</li>
<li>Left OpenRouter-style OpenAI IDs outside the intended guardrail.</li>
</ul>
</div>
</article>
<article class="comparison-card" id="after-gating-card">
<div class="comparison-head">
<div class="comparison-title">
<span class="tag after">After</span>
<h3>Capability checks use normalized model family</h3>
</div>
</div>
<div class="comparison-body">
<div class="status-line good">Same rules for bare and prefixed IDs</div>
<p>
The builder now strips transport prefixes once, then applies the same
capability logic to both bare and prefixed model IDs.
</p>
<pre>{
"model": "openai/o3-mini",
"max_completion_tokens": 123
}</pre>
<ul class="bullet-list">
<li><code>openai/o3-mini</code> now follows the same route as <code>o3-mini</code>.</li>
<li><code>openai/gpt-5.4</code> still keeps <code>temperature</code>, matching the flagship-model rule.</li>
<li>Azure and non-Azure paths are validated as model-based, not URL-based.</li>
</ul>
</div>
</article>
</div>
</section>
<section class="section" id="validation">
<div class="section-label">03 • Validation</div>
<h2>Tests and reviewer-facing outcomes</h2>
<p class="section-intro">
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.
</p>
<div class="validation-grid">
<div class="validation-card">
<strong>Passed</strong>
<ul class="validation-list">
<li><code>go test ./internal/providers ./internal/agent</code></li>
<li><code>go test ./internal/providers -run 'TestTruncateToolCallID|TestBuildRequestBody' -count=1</code></li>
<li><code>go test ./internal/agent -run '^TestUniquifyToolCallIDs$' -count=1</code></li>
</ul>
</div>
<div class="validation-card">
<strong>Known unrelated failure</strong>
<p>
<code>go test ./...</code> still fails in <code>pkg/browser</code> at
<code>TestResolveRemoteCDP_DefaultPort</code> with
<code>/json/version returned HTTP 404</code>. This pre-exists the provider
changes and is unrelated to the OpenAI compatibility patch.
</p>
</div>
</div>
<p class="footer-note">
Review note: these visuals intentionally show behavior and request-shape
diffs rather than product UI, because this PR does not alter application screens.
</p>
</section>
</main>
</div>
<script>
(function() {
const toggle = document.getElementById("themeToggle");
const saved = localStorage.getItem("theme");
const initial = saved || (window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light");
document.documentElement.setAttribute("data-theme", initial);
toggle.textContent = initial === "dark" ? "\u2600" : "\u263E";
toggle.addEventListener("click", function() {
const current = document.documentElement.getAttribute("data-theme") || "light";
const next = current === "light" ? "dark" : "light";
document.documentElement.setAttribute("data-theme", next);
localStorage.setItem("theme", next);
toggle.textContent = next === "dark" ? "\u2600" : "\u263E";
});
const toc = document.getElementById("toc");
const links = toc.querySelectorAll("a");
const sections = [];
links.forEach((link) => {
const id = link.getAttribute("href").slice(1);
const el = document.getElementById(id);
if (el) sections.push({ link, el });
});
const observer = new IntersectionObserver(
(entries) => {
entries.forEach((entry) => {
if (!entry.isIntersecting) return;
links.forEach((link) => link.classList.remove("active"));
const match = sections.find((section) => section.el === entry.target);
if (match) match.link.classList.add("active");
});
},
{ rootMargin: "-10% 0px -70% 0px" },
);
sections.forEach((section) => observer.observe(section.el));
links.forEach((link) => {
link.addEventListener("click", (event) => {
event.preventDefault();
const id = link.getAttribute("href").slice(1);
const el = document.getElementById(id);
if (el) {
el.scrollIntoView({ behavior: "smooth", block: "start" });
history.replaceState(null, "", "#" + id);
}
});
});
})();
</script>
</body>
</html>
+35 -8
View File
@@ -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)
+11 -10
View File
@@ -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
}
+37 -5
View File
@@ -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_")]
}
+260
View File
@@ -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)
}
}