mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-10 13:31:38 +00:00
fix(compressor): strip orphan tool_calls instead of inserting stubs (#51218)
_sanitize_tool_pairs inserted stub role="tool" results for orphaned
tool_calls. The pre-API repair_message_sequence() tracks known call IDs by
tc.get("id") while this sanitizer keys on call_id||id; when they disagree
(Codex Responses API: id != call_id) the stubs are silently dropped by the
repair pass, re-exposing the original orphans. Strip the orphaned tool_calls
at the source instead (preserving any text content, adding a placeholder for
an otherwise-empty assistant turn) to avoid the mismatch class entirely.
Salvaged from #51225.
Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
This commit is contained in:
parent
58ea7f9071
commit
32b23bfb08
2 changed files with 138 additions and 16 deletions
|
|
@ -2092,8 +2092,16 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
The API rejects this because every tool_call must be followed by
|
||||
a tool result with the matching call_id.
|
||||
|
||||
This method removes orphaned results and inserts stub results for
|
||||
orphaned calls so the message list is always well-formed.
|
||||
This method removes orphaned results and strips orphaned tool_calls
|
||||
from assistant messages so the message list is always well-formed.
|
||||
|
||||
Previous approach inserted stub ``role="tool"`` results for orphaned
|
||||
tool_calls. That caused a secondary failure: the pre-API
|
||||
``repair_message_sequence()`` uses ``tc.get("id")`` to track known
|
||||
call IDs while this sanitizer uses ``call_id || id``. When the two
|
||||
disagree (Codex Responses API format: ``id != call_id``), stubs get
|
||||
silently dropped by the repair pass, re-exposing the original orphans.
|
||||
Stripping at the source avoids this entire class of mismatch.
|
||||
"""
|
||||
surviving_call_ids: set = set()
|
||||
for msg in messages:
|
||||
|
|
@ -2120,24 +2128,34 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
if not self.quiet_mode:
|
||||
logger.info("Compression sanitizer: removed %d orphaned tool result(s)", len(orphaned_results))
|
||||
|
||||
# 2. Add stub results for assistant tool_calls whose results were dropped
|
||||
# 2. Strip orphaned tool_calls from assistant messages whose results
|
||||
# were dropped. Stripping is preferred over inserting stub results
|
||||
# because stubs can be dropped by downstream repair_message_sequence
|
||||
# when call_id != id (Codex Responses API format), re-exposing orphans.
|
||||
missing_results = surviving_call_ids - result_call_ids
|
||||
if missing_results:
|
||||
patched: List[Dict[str, Any]] = []
|
||||
for msg in messages:
|
||||
patched.append(msg)
|
||||
if msg.get("role") == "assistant":
|
||||
for tc in msg.get("tool_calls") or []:
|
||||
cid = self._get_tool_call_id(tc)
|
||||
if cid in missing_results:
|
||||
patched.append({
|
||||
"role": "tool",
|
||||
"content": "[Result from earlier conversation — see context summary above]",
|
||||
"tool_call_id": cid,
|
||||
})
|
||||
messages = patched
|
||||
if msg.get("role") != "assistant":
|
||||
continue
|
||||
tcs = msg.get("tool_calls")
|
||||
if not tcs:
|
||||
continue
|
||||
kept = [tc for tc in tcs if self._get_tool_call_id(tc) not in missing_results]
|
||||
if len(kept) != len(tcs):
|
||||
if kept:
|
||||
msg["tool_calls"] = kept
|
||||
else:
|
||||
msg.pop("tool_calls", None)
|
||||
# Ensure the assistant message still has visible
|
||||
# content so the API does not reject an empty turn.
|
||||
content = msg.get("content")
|
||||
if not content or (isinstance(content, str) and not content.strip()):
|
||||
msg["content"] = "(tool call removed)"
|
||||
if not self.quiet_mode:
|
||||
logger.info("Compression sanitizer: added %d stub tool result(s)", len(missing_results))
|
||||
logger.info(
|
||||
"Compression sanitizer: stripped %d orphaned tool_call(s) from assistant messages",
|
||||
len(missing_results),
|
||||
)
|
||||
|
||||
return messages
|
||||
|
||||
|
|
|
|||
|
|
@ -2919,3 +2919,107 @@ class TestTurnPairPreservation:
|
|||
f"Orphan user turn at tail start: {tail[0]['content']!r} — "
|
||||
f"next role is {tail[1].get('role') if len(tail) > 1 else 'nothing'}"
|
||||
)
|
||||
|
||||
|
||||
|
||||
|
||||
class TestSanitizerStripsOrphanedToolCalls:
|
||||
"""PR #51218 (salvaged from #51225): orphaned tool_calls are stripped from
|
||||
assistant messages instead of having stub tool results inserted, avoiding
|
||||
the call_id != id mismatch that let downstream repair_message_sequence drop
|
||||
the stubs and re-expose orphans."""
|
||||
|
||||
def test_sanitizer_strips_orphaned_tool_calls(self, compressor):
|
||||
"""Orphaned tool_calls (no matching tool result) are stripped from
|
||||
assistant messages instead of having stubs inserted. #51218"""
|
||||
msgs = [
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "",
|
||||
"tool_calls": [
|
||||
{"id": "tc_orphan", "function": {"name": "search", "arguments": "{}"}},
|
||||
],
|
||||
},
|
||||
{"role": "user", "content": "never mind"},
|
||||
]
|
||||
|
||||
sanitized = compressor._sanitize_tool_pairs(msgs)
|
||||
|
||||
# Orphaned tool_call should be stripped, not stub-inserted
|
||||
asst = next(m for m in sanitized if m.get("role") == "assistant")
|
||||
assert not asst.get("tool_calls"), "orphaned tool_calls should be stripped"
|
||||
# No stub tool messages should be added
|
||||
assert not any(m.get("role") == "tool" for m in sanitized)
|
||||
# Empty assistant should get placeholder content
|
||||
assert asst.get("content") == "(tool call removed)"
|
||||
|
||||
def test_sanitizer_strips_orphaned_keeps_valid(self, compressor):
|
||||
"""When an assistant has both valid and orphaned tool_calls, only
|
||||
the orphans are stripped. #51218"""
|
||||
msgs = [
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "",
|
||||
"tool_calls": [
|
||||
{"id": "tc_valid", "function": {"name": "read_file", "arguments": "{}"}},
|
||||
{"id": "tc_orphan", "function": {"name": "search", "arguments": "{}"}},
|
||||
],
|
||||
},
|
||||
{"role": "tool", "tool_call_id": "tc_valid", "content": "file content"},
|
||||
]
|
||||
|
||||
sanitized = compressor._sanitize_tool_pairs(msgs)
|
||||
|
||||
asst = next(m for m in sanitized if m.get("role") == "assistant")
|
||||
assert len(asst["tool_calls"]) == 1
|
||||
assert asst["tool_calls"][0]["id"] == "tc_valid"
|
||||
# Valid tool result preserved
|
||||
tool_msgs = [m for m in sanitized if m.get("role") == "tool"]
|
||||
assert len(tool_msgs) == 1
|
||||
assert tool_msgs[0]["tool_call_id"] == "tc_valid"
|
||||
|
||||
def test_sanitizer_strips_orphaned_preserves_text_content(self, compressor):
|
||||
"""When an assistant has text content AND orphaned tool_calls,
|
||||
the text is preserved and only tool_calls are stripped. #51218"""
|
||||
msgs = [
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "Let me search for that.",
|
||||
"tool_calls": [
|
||||
{"id": "tc_orphan", "function": {"name": "search", "arguments": "{}"}},
|
||||
],
|
||||
},
|
||||
{"role": "user", "content": "thanks"},
|
||||
]
|
||||
|
||||
sanitized = compressor._sanitize_tool_pairs(msgs)
|
||||
|
||||
asst = next(m for m in sanitized if m.get("role") == "assistant")
|
||||
assert asst["content"] == "Let me search for that."
|
||||
assert not asst.get("tool_calls")
|
||||
|
||||
def test_sanitizer_strips_orphaned_with_call_id_mismatch(self, compressor):
|
||||
"""Stubs with call_id != id used to be dropped by downstream
|
||||
repair_message_sequence, re-exposing orphans. Stripping avoids
|
||||
this entirely. #51218"""
|
||||
msgs = [
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "",
|
||||
"tool_calls": [
|
||||
{
|
||||
"id": "fc_abc",
|
||||
"call_id": "call_abc",
|
||||
"function": {"name": "search", "arguments": "{}"},
|
||||
},
|
||||
],
|
||||
},
|
||||
# No tool result for call_abc — orphaned
|
||||
{"role": "user", "content": "next"},
|
||||
]
|
||||
|
||||
sanitized = compressor._sanitize_tool_pairs(msgs)
|
||||
|
||||
asst = next(m for m in sanitized if m.get("role") == "assistant")
|
||||
assert not asst.get("tool_calls")
|
||||
# No stub tool messages (which would have call_id != id mismatch)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue