mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
fix: sanitize malformed tool_calls and orphan tool results in outbound messages
When a streamed assistant response is cut off mid input_json_delta, the
resulting tool_call.arguments can be a truncated (non-JSON) string. Every
subsequent API request re-sends that history, and Anthropic-compatible
proxies (LiteLLM in particular) reject it with HTTP 400. The retry loop
cannot recover because the bad message is pinned in history.
This commit adds sanitize_tool_calls_in_messages() in transports/base.py
and calls it from both:
- ChatCompletionsTransport.convert_messages (covers OpenAI-compatible
providers including LiteLLM, OpenRouter, etc.)
- anthropic_adapter.convert_messages_to_anthropic (covers direct
Anthropic path)
The sanitizer:
1. Drops assistant messages whose tool_calls contain malformed
function.arguments (not valid JSON).
2. Drops tool result messages whose tool_call_id no longer has a
matching assistant tool_call.
Dropping (rather than repairing to {}) is intentional — a {}-arg tool
call would execute a garbage edit; dropping lets the model re-attempt
on the next turn.
Fixes #14443
This commit is contained in:
parent
d1ce358646
commit
076e8bc4d1
4 changed files with 224 additions and 11 deletions
|
|
@ -1093,6 +1093,10 @@ def convert_messages_to_anthropic(
|
|||
Anthropic-proprietary — third-party endpoints cannot validate them and will
|
||||
reject them with HTTP 400 "Invalid signature in thinking block".
|
||||
"""
|
||||
from agent.transports.base import sanitize_tool_calls_in_messages
|
||||
|
||||
messages = sanitize_tool_calls_in_messages(messages)
|
||||
|
||||
system = None
|
||||
result = []
|
||||
|
||||
|
|
|
|||
|
|
@ -7,12 +7,121 @@ It does NOT own: client construction, streaming, credential refresh,
|
|||
prompt caching, interrupt handling, or retry logic. Those stay on AIAgent.
|
||||
"""
|
||||
|
||||
import copy
|
||||
import json
|
||||
from abc import ABC, abstractmethod
|
||||
from typing import Any, Dict, List, Optional
|
||||
from typing import Any, Dict, List, Optional, Set
|
||||
|
||||
from agent.transports.types import NormalizedResponse
|
||||
|
||||
|
||||
def _is_valid_json(s: str) -> bool:
|
||||
"""Return True if *s* is valid JSON."""
|
||||
try:
|
||||
json.loads(s)
|
||||
return True
|
||||
except (json.JSONDecodeError, ValueError, TypeError):
|
||||
return False
|
||||
|
||||
|
||||
def sanitize_tool_calls_in_messages(messages: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
|
||||
"""Sanitize messages by dropping malformed tool_calls and orphan tool results.
|
||||
|
||||
If a streamed assistant response is cut off mid ``input_json_delta``, the
|
||||
resulting ``tool_call.arguments`` can be a truncated (non-JSON) string.
|
||||
Re-sending that history to Anthropic-compatible proxies (LiteLLM, etc.)
|
||||
causes a deterministic 400 that retries cannot recover from.
|
||||
|
||||
This function:
|
||||
|
||||
1. Drops assistant messages whose ``tool_calls`` contain malformed
|
||||
``function.arguments`` (not valid JSON).
|
||||
2. Drops ``tool`` result messages whose ``tool_call_id`` no longer has a
|
||||
matching assistant tool_call (orphaned results).
|
||||
|
||||
Dropping (rather than repairing to ``{}``) is intentional — a ``{}``-arg
|
||||
tool call would execute a garbage edit; dropping lets the model re-attempt
|
||||
on the next turn.
|
||||
|
||||
Returns a shallow copy of the list when mutations are needed; returns the
|
||||
original list when no sanitization is required.
|
||||
"""
|
||||
# First pass: collect valid tool_call_ids and detect malformed tool_calls
|
||||
valid_tool_call_ids: Set[str] = set()
|
||||
needs_sanitize = False
|
||||
|
||||
for msg in messages:
|
||||
if not isinstance(msg, dict):
|
||||
continue
|
||||
if msg.get("role") != "assistant":
|
||||
continue
|
||||
tool_calls = msg.get("tool_calls")
|
||||
if not isinstance(tool_calls, list):
|
||||
continue
|
||||
valid_tcs = []
|
||||
for tc in tool_calls:
|
||||
if not isinstance(tc, dict):
|
||||
needs_sanitize = True
|
||||
continue
|
||||
fn = tc.get("function", {}) if isinstance(tc.get("function"), dict) else {}
|
||||
args = fn.get("arguments", "{}")
|
||||
if isinstance(args, str) and not _is_valid_json(args):
|
||||
needs_sanitize = True
|
||||
continue
|
||||
valid_tcs.append(tc)
|
||||
tc_id = tc.get("id")
|
||||
if isinstance(tc_id, str):
|
||||
valid_tool_call_ids.add(tc_id)
|
||||
if len(valid_tcs) != len(tool_calls):
|
||||
needs_sanitize = True
|
||||
# Also mark for sanitize if assistant message has no valid tool_calls
|
||||
# but originally had some (all were malformed)
|
||||
if tool_calls and not valid_tcs:
|
||||
needs_sanitize = True
|
||||
|
||||
if not needs_sanitize:
|
||||
return messages
|
||||
|
||||
# Second pass: rebuild messages, dropping malformed assistant messages
|
||||
# and orphan tool results
|
||||
sanitized = []
|
||||
for msg in messages:
|
||||
if not isinstance(msg, dict):
|
||||
sanitized.append(msg)
|
||||
continue
|
||||
|
||||
if msg.get("role") == "assistant":
|
||||
tool_calls = msg.get("tool_calls")
|
||||
if isinstance(tool_calls, list):
|
||||
valid_tcs = []
|
||||
for tc in tool_calls:
|
||||
if not isinstance(tc, dict):
|
||||
continue
|
||||
fn = tc.get("function", {}) if isinstance(tc.get("function"), dict) else {}
|
||||
args = fn.get("arguments", "{}")
|
||||
if isinstance(args, str) and not _is_valid_json(args):
|
||||
continue
|
||||
valid_tcs.append(tc)
|
||||
if valid_tcs:
|
||||
new_msg = copy.copy(msg)
|
||||
new_msg["tool_calls"] = valid_tcs
|
||||
sanitized.append(new_msg)
|
||||
else:
|
||||
# All tool_calls malformed — drop the whole assistant msg
|
||||
pass
|
||||
continue
|
||||
|
||||
if msg.get("role") == "tool":
|
||||
tc_id = msg.get("tool_call_id")
|
||||
if isinstance(tc_id, str) and tc_id not in valid_tool_call_ids:
|
||||
# Orphan tool result — drop it
|
||||
continue
|
||||
|
||||
sanitized.append(msg)
|
||||
|
||||
return sanitized
|
||||
|
||||
|
||||
class ProviderTransport(ABC):
|
||||
"""Base class for provider-specific format conversion and normalization."""
|
||||
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ import copy
|
|||
from typing import Any, Dict, List, Optional
|
||||
|
||||
from agent.prompt_builder import DEVELOPER_ROLE_MODELS
|
||||
from agent.transports.base import ProviderTransport
|
||||
from agent.transports.base import ProviderTransport, sanitize_tool_calls_in_messages
|
||||
from agent.transports.types import NormalizedResponse, ToolCall, Usage
|
||||
|
||||
|
||||
|
|
@ -28,32 +28,41 @@ class ChatCompletionsTransport(ProviderTransport):
|
|||
return "chat_completions"
|
||||
|
||||
def convert_messages(self, messages: List[Dict[str, Any]], **kwargs) -> List[Dict[str, Any]]:
|
||||
"""Messages are already in OpenAI format — sanitize Codex leaks only.
|
||||
"""Messages are already in OpenAI format — sanitize Codex leaks and malformed tool_calls.
|
||||
|
||||
Strips Codex Responses API fields (``codex_reasoning_items`` on the
|
||||
message, ``call_id``/``response_item_id`` on tool_calls) that strict
|
||||
chat-completions providers reject with 400/422.
|
||||
|
||||
Also drops assistant messages with malformed (non-JSON)
|
||||
``tool_calls[*].function.arguments`` and orphan ``tool`` result
|
||||
messages, preventing deterministic 400 retries when a truncated
|
||||
stream leaves bad JSON in history. See hermes-agent#14443.
|
||||
"""
|
||||
needs_sanitize = False
|
||||
for msg in messages:
|
||||
# First: sanitize malformed tool_calls and orphan tool results
|
||||
sanitized = sanitize_tool_calls_in_messages(messages)
|
||||
|
||||
# Second: strip Codex-specific fields
|
||||
needs_codex_sanitize = False
|
||||
for msg in sanitized:
|
||||
if not isinstance(msg, dict):
|
||||
continue
|
||||
if "codex_reasoning_items" in msg:
|
||||
needs_sanitize = True
|
||||
needs_codex_sanitize = True
|
||||
break
|
||||
tool_calls = msg.get("tool_calls")
|
||||
if isinstance(tool_calls, list):
|
||||
for tc in tool_calls:
|
||||
if isinstance(tc, dict) and ("call_id" in tc or "response_item_id" in tc):
|
||||
needs_sanitize = True
|
||||
needs_codex_sanitize = True
|
||||
break
|
||||
if needs_sanitize:
|
||||
if needs_codex_sanitize:
|
||||
break
|
||||
|
||||
if not needs_sanitize:
|
||||
return messages
|
||||
if not needs_codex_sanitize:
|
||||
return sanitized
|
||||
|
||||
sanitized = copy.deepcopy(messages)
|
||||
sanitized = copy.deepcopy(sanitized)
|
||||
for msg in sanitized:
|
||||
if not isinstance(msg, dict):
|
||||
continue
|
||||
|
|
|
|||
|
|
@ -332,6 +332,97 @@ class TestChatCompletionsNormalize:
|
|||
assert nr.provider_data == {"reasoning_content": "detailed scratchpad"}
|
||||
|
||||
|
||||
class TestChatCompletionsSanitizeMalformedToolCalls:
|
||||
"""Regression tests for hermes-agent#14443.
|
||||
|
||||
Truncated tool_call.arguments (non-JSON) in history must be dropped
|
||||
before the message leaves the process, or Anthropic-compatible proxies
|
||||
(LiteLLM) reject every retry with HTTP 400.
|
||||
"""
|
||||
|
||||
def test_drops_assistant_with_truncated_tool_call(self, transport):
|
||||
msgs = [
|
||||
{"role": "user", "content": "patch file.py"},
|
||||
{"role": "assistant", "content": None, "tool_calls": [
|
||||
{"id": "call_1", "type": "function", "function": {
|
||||
"name": "patch",
|
||||
# Truncated JSON — unterminated string
|
||||
"arguments": '{"path": "file.py", "old_string": "',
|
||||
}},
|
||||
]},
|
||||
]
|
||||
result = transport.convert_messages(msgs)
|
||||
# Assistant message with malformed tool_calls is dropped entirely
|
||||
assert len(result) == 1
|
||||
assert result[0]["role"] == "user"
|
||||
|
||||
def test_drops_orphan_tool_result(self, transport):
|
||||
msgs = [
|
||||
{"role": "user", "content": "patch file.py"},
|
||||
{"role": "assistant", "content": None, "tool_calls": [
|
||||
{"id": "call_1", "type": "function", "function": {
|
||||
"name": "patch",
|
||||
# Truncated JSON
|
||||
"arguments": '{"path": "file.py", "old_string": "',
|
||||
}},
|
||||
]},
|
||||
{"role": "tool", "tool_call_id": "call_1", "content": "ok"},
|
||||
]
|
||||
result = transport.convert_messages(msgs)
|
||||
# Both assistant and orphan tool result are dropped
|
||||
assert len(result) == 1
|
||||
assert result[0]["role"] == "user"
|
||||
|
||||
def test_keeps_valid_tool_calls_and_results(self, transport):
|
||||
msgs = [
|
||||
{"role": "user", "content": "patch file.py"},
|
||||
{"role": "assistant", "content": None, "tool_calls": [
|
||||
{"id": "call_1", "type": "function", "function": {
|
||||
"name": "patch",
|
||||
"arguments": '{"path": "file.py", "old_string": "x", "new_string": "y"}',
|
||||
}},
|
||||
]},
|
||||
{"role": "tool", "tool_call_id": "call_1", "content": "done"},
|
||||
]
|
||||
result = transport.convert_messages(msgs)
|
||||
assert len(result) == 3
|
||||
assert result[1]["role"] == "assistant"
|
||||
assert result[2]["role"] == "tool"
|
||||
|
||||
def test_mixed_valid_and_invalid_tool_calls(self, transport):
|
||||
msgs = [
|
||||
{"role": "assistant", "content": None, "tool_calls": [
|
||||
{"id": "call_good", "type": "function", "function": {
|
||||
"name": "read",
|
||||
"arguments": '{"path": "/etc/hosts"}',
|
||||
}},
|
||||
{"id": "call_bad", "type": "function", "function": {
|
||||
"name": "patch",
|
||||
# Truncated
|
||||
"arguments": '{"path": "file.py", "old_string": "',
|
||||
}},
|
||||
]},
|
||||
{"role": "tool", "tool_call_id": "call_good", "content": "127.0.0.1 localhost"},
|
||||
{"role": "tool", "tool_call_id": "call_bad", "content": "ok"},
|
||||
]
|
||||
result = transport.convert_messages(msgs)
|
||||
# Only valid tool_call and its result remain
|
||||
assert len(result) == 2
|
||||
assert result[0]["role"] == "assistant"
|
||||
assert len(result[0]["tool_calls"]) == 1
|
||||
assert result[0]["tool_calls"][0]["id"] == "call_good"
|
||||
assert result[1]["role"] == "tool"
|
||||
assert result[1]["tool_call_id"] == "call_good"
|
||||
|
||||
def test_no_sanitize_when_all_valid(self, transport):
|
||||
msgs = [
|
||||
{"role": "user", "content": "hi"},
|
||||
]
|
||||
result = transport.convert_messages(msgs)
|
||||
# Identity — same list returned when no sanitization needed
|
||||
assert result is msgs
|
||||
|
||||
|
||||
class TestChatCompletionsCacheStats:
|
||||
|
||||
def test_no_usage(self, transport):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue