Merge pull request #5740 from nesquena/release/stage-5737

Release: drop empty tool_calls arrays before API send (#5737)

CHANGELOG.md

@@ -5,6 +5,8 @@
 
 ### Fixed
 
+- **Turns no longer fail with an HTTP 400 from strict providers on empty tool-call history.** Some stored assistant messages carry an empty `tool_calls: []` array, which DeepSeek v4 and newer OpenAI models reject outright ("empty array. Expected an array with minimum length 1"). The model-facing history sanitizer now drops an empty `tool_calls` key on every path that builds the API payload, so a session that accumulated such a message can still send. Populated tool-call chains and their linked results are untouched. Thanks @Swanzb. (#5737)
+
 - **Switching profiles no longer occasionally sticks a session to the wrong provider.** When you switched profiles or tabs while the model dropdown was mid-rebuild, the app could read the *previous* profile's selected model and stamp its provider (e.g. `ollama`) onto a model that doesn't belong to it. That wrong provider got saved to the session and re-sent on every turn, bricking it with a "Provider 'X' is set… but no API key was found" error for a provider the session never used. The provider is now resolved from the dropdown option whose value actually matches the model being sent, so the mismatch can't happen. Credit @b3nw for the root-cause trace. (#5567)
 
 - **Clicking a session link that belongs to another profile now switches to that profile instead of failing.** A valid `session://` deep link pointing at a session owned by a different Hermes profile used to look identical to a deleted session — the UI showed "Session not available in web UI" and self-healed away. Now the WebUI recognizes a valid-but-wrong-profile session, offers to switch to the owning profile, and retries the load once; truly missing/deleted sessions still self-heal as before. No foreign-profile transcript is ever returned — the mismatch response carries only the owning profile name. Thanks @harcek. (#5419)

api/streaming.py

@@ -4025,6 +4025,10 @@ def _sanitize_messages_for_api(
                 # Orphaned tool result — skip to avoid 400 from strict providers.
                 continue
         sanitized = {k: v for k, v in msg.items() if k in _API_SAFE_MSG_KEYS}
+        # Drop empty tool_calls — strict providers (DeepSeek, newer OpenAI)
+        # reject tool_calls: [] with HTTP 400 even when no orphaned calls exist.
+        if 'tool_calls' in sanitized and not sanitized['tool_calls']:
+            del sanitized['tool_calls']
         # Provider-aware reasoning_content stripping from model-facing history.
         # Historical assistant reasoning_content is stripped only when the user
         # explicitly requests strip mode or auto mode identifies a local/generic
@@ -4136,6 +4140,8 @@ def _api_safe_message_positions(messages):
             if not tid or tid not in valid_tool_call_ids:
                 continue
         sanitized = {k: v for k, v in msg.items() if k in _API_SAFE_MSG_KEYS}
+        if 'tool_calls' in sanitized and not sanitized['tool_calls']:
+            del sanitized['tool_calls']
         if is_recovered:
             sanitized['_recovered'] = True  # temporary marker — stripped before return
         if 'content' in sanitized:
@@ -4315,6 +4321,12 @@ def _restore_reasoning_metadata(previous_messages, updated_messages):
         if not isinstance(msg, dict):
             return None
         projected = {k: v for k, v in msg.items() if k in _API_SAFE_MSG_KEYS and msg.get('role')}
+        # Mirror the empty-tool_calls drop applied by _api_safe_message_positions
+        # (#5737) so this projection matches the API-safe positions it's aligned
+        # against — otherwise a row stored with tool_calls: [] projects
+        # differently here than in prev_safe and loses its metadata carry-forward.
+        if 'tool_calls' in projected and not projected['tool_calls']:
+            del projected['tool_calls']
         if 'content' in projected:
             projected['content'] = _strip_oob_blocks(projected['content'])
         return projected

tests/test_context_history_sanitization.py

@@ -6,7 +6,11 @@ import threading
 from types import SimpleNamespace
 from unittest.mock import patch
 
-from api.streaming import _sanitize_messages_for_api, _strip_oob_blocks
+from api.streaming import (
+    _restore_reasoning_metadata,
+    _sanitize_messages_for_api,
+    _strip_oob_blocks,
+)
 
 
 OOB_BLOCK = (
@@ -99,6 +103,45 @@ def test_sanitize_messages_for_api_preserves_tool_chains():
     assert not _contains_oob(sanitized)
 
 
+def test_sanitize_messages_drops_empty_tool_calls_array():
+    """#5737: strict providers (DeepSeek v4, newer OpenAI) reject an assistant
+    message carrying `tool_calls: []` with HTTP 400. A stored assistant message
+    can literally have an empty list (not None, not missing), which the orphan
+    linker doesn't catch. The sanitizer must drop the empty key while leaving a
+    populated tool_calls chain intact."""
+    messages = [
+        {"role": "user", "content": "hi"},
+        # Empty tool_calls: [] must be dropped (would 400 on strict providers).
+        {"role": "assistant", "content": "thinking out loud", "tool_calls": []},
+        {"role": "user", "content": "go on"},
+        # Populated tool_calls (+ its tool result) must be preserved untouched.
+        {
+            "role": "assistant",
+            "content": "",
+            "tool_calls": [
+                {"type": "function", "id": "call-9",
+                 "function": {"name": "terminal", "arguments": "{}"}},
+            ],
+        },
+        {"role": "tool", "tool_call_id": "call-9", "content": "ok"},
+    ]
+
+    sanitized = _sanitize_messages_for_api(messages)
+
+    # The empty-tool_calls assistant message survives, but WITHOUT the key.
+    empty_asst = next(m for m in sanitized if m.get("content") == "thinking out loud")
+    assert empty_asst["role"] == "assistant"
+    assert "tool_calls" not in empty_asst, "empty tool_calls: [] must be dropped (strict-provider 400)"
+    # The populated chain is preserved.
+    populated = next(m for m in sanitized if m.get("role") == "assistant" and m.get("tool_calls"))
+    assert populated["tool_calls"][0]["id"] == "call-9"
+    assert any(m.get("tool_call_id") == "call-9" for m in sanitized)
+    # No message that reaches the API carries an empty tool_calls array.
+    for m in sanitized:
+        assert not ("tool_calls" in m and not m["tool_calls"]), \
+            "no message may ship an empty tool_calls array"
+
+
 def test_gateway_conversation_history_no_oob():
     from api.config import STREAM_PARTIAL_TEXT, STREAM_REASONING_TEXT
     from api.gateway_chat import _STREAM_RUN_IDS, _run_gateway_runs_api_streaming
@@ -261,3 +304,34 @@ def test_strip_oob_blocks_incomplete_block_preserved():
 
     # Incomplete block should remain untouched
     assert "[OUT-OF-BAND USER MESSAGE" in cleaned
+
+
+def test_restore_metadata_aligns_row_with_empty_tool_calls():
+    """#5737 third site: _restore_reasoning_metadata aligns previous vs updated
+    rows by their API-safe projection. A row stored with tool_calls: [] must
+    project identically on both sides (empty key dropped) so its reasoning /
+    stable id / timestamp still carry forward — otherwise the projections diverge
+    and the row silently loses its metadata on every turn."""
+    previous = [
+        {"role": "user", "content": "q"},
+        {
+            "role": "assistant",
+            "content": "answer",
+            "tool_calls": [],            # empty — the strict-provider case
+            "reasoning": "prior reasoning",
+            "id": "msg-42",
+            "timestamp": 1234,
+        },
+    ]
+    # The agent echoes the row back WITHOUT our metadata (and without tool_calls).
+    updated = [
+        {"role": "user", "content": "q"},
+        {"role": "assistant", "content": "answer"},
+    ]
+
+    restored = _restore_reasoning_metadata(previous, updated)
+
+    asst = restored[1]
+    assert asst["reasoning"] == "prior reasoning", "reasoning must carry forward across the empty-tool_calls row"
+    assert asst["id"] == "msg-42", "stable id must carry forward"
+    assert asst["timestamp"] == 1234, "timestamp must carry forward (row must not re-mint / drift)"