mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-11-09 22:52:21 +01:00
* fix(agent): normalize tool-call message order without dropping history Restore the context compactor to the current dev implementation so the PR no longer removes unrelated helpers, then repair the predecessor contract on top of it. The sanitizer now iterates one normalization pass to a fixpoint, so it validates each batch against its own effective output and a second sanitation run is a no-op. Assistant text turns immediately before a tool_calls message are merged into it, preserving completed call/result evidence when the transcript was trimmed mid-history. Batches that still lack a user or tool predecessor are stripped or dropped as before. * fix(agent): preserve multimodal content when dropping invalid tool calls --------- Co-authored-by: ShahabAhmed01 <192491541+ShahabAhmed01@users.noreply.github.com> Co-authored-by: Alexandre Teixeira <general@alexandreteixeira.dev>
This commit is contained in:
Shahab Ahmed
co-authored by
ShahabAhmed01
Alexandre Teixeira
ShahabAhmed01
Alexandre Teixeira
parent
1c15090949
commit
34e4666f35
+89
-13
@@ -42,6 +42,15 @@ def _content_as_text(content: Any) -> str:
|
||||
_MULTIMODAL_IMAGE_TYPES = {"image_url", "input_image", "image"}
|
||||
|
||||
|
||||
def _has_preservable_content(content: Any) -> bool:
|
||||
"""Retain nonblank text or nonempty multimodal blocks after tool-call removal."""
|
||||
if isinstance(content, str):
|
||||
return bool(content.strip())
|
||||
if isinstance(content, list):
|
||||
return bool(content)
|
||||
return False
|
||||
|
||||
|
||||
def prune_multimodal_images(
|
||||
messages: List[Dict],
|
||||
*,
|
||||
@@ -222,22 +231,81 @@ def normalize_compaction_summary(summary: str) -> str:
|
||||
|
||||
|
||||
def _sanitize_tool_messages(msgs: List[Dict]) -> List[Dict]:
|
||||
"""Drop orphaned `tool` messages and dangling assistant `tool_calls`.
|
||||
"""Normalize tool-call message ordering so strict providers accept the transcript.
|
||||
|
||||
OpenAI's API requires every `role:"tool"` message to immediately
|
||||
follow an assistant message that carries `tool_calls` (or another
|
||||
tool message in the same batch). Front-trimming the history can cut
|
||||
the assistant `tool_calls` parent while keeping its tool responses,
|
||||
which triggers: "messages with role 'tool' must be a response to a
|
||||
preceding message with 'tool_calls'". This pass repairs that:
|
||||
- drops `tool` messages with no valid preceding tool_calls
|
||||
- drops assistant `tool_calls` messages whose tool responses were
|
||||
all trimmed away (some providers reject unanswered tool_calls)
|
||||
tool message in the same batch). Gemini and some other providers
|
||||
additionally require a function call turn to come immediately after
|
||||
a user turn or a function response turn. Trimming the history can
|
||||
break either rule, which triggers errors such as "messages with role
|
||||
'tool' must be a response to a preceding message with 'tool_calls'"
|
||||
and "Please ensure that function call turn comes immediately after
|
||||
a user turn or after a function response turn."
|
||||
|
||||
The transcript is normalized against its own effective output: each
|
||||
pass repairs the violations it can see and the passes repeat until
|
||||
the result is stable, so the returned transcript itself satisfies
|
||||
the ordering contract and sanitizing it again is a no-op. A
|
||||
completed call/result batch whose predecessor is an adjacent
|
||||
assistant text turn keeps its calls by merging that text into the
|
||||
call message instead of dropping executed-tool evidence; a batch
|
||||
that cannot get a user/tool predecessor (e.g. a transcript that
|
||||
starts mid-history) is the only one dropped.
|
||||
"""
|
||||
# Pass 1: drop orphan tool messages.
|
||||
work = list(msgs)
|
||||
for _ in range(2 * len(work) + 2):
|
||||
normalized = _normalize_tool_messages_once(work)
|
||||
if normalized == work:
|
||||
break
|
||||
work = normalized
|
||||
return work
|
||||
|
||||
|
||||
def _normalize_tool_messages_once(msgs: List[Dict]) -> List[Dict]:
|
||||
"""One normalization pass over an already-trimmed message list."""
|
||||
# Merge the run of assistant text turns directly before an assistant
|
||||
# tool_calls turn into a single assistant message carrying both, so the
|
||||
# batch inherits the text run's predecessor (a user or tool message)
|
||||
# instead of losing its tool_calls. Only plain-text content is merged;
|
||||
# multimodal content stays untouched and falls through to the strip logic.
|
||||
merged: List[Dict] = []
|
||||
for m in msgs:
|
||||
cur_text = m.get("content")
|
||||
if (
|
||||
m.get("role") == "assistant"
|
||||
and m.get("tool_calls")
|
||||
and (cur_text is None or isinstance(cur_text, str))
|
||||
):
|
||||
text_run: List[Dict] = []
|
||||
while (
|
||||
merged
|
||||
and merged[-1].get("role") == "assistant"
|
||||
and not merged[-1].get("tool_calls")
|
||||
and isinstance(merged[-1].get("content"), str)
|
||||
):
|
||||
text_run.append(merged.pop())
|
||||
if text_run:
|
||||
text_parts = [
|
||||
msg["content"].strip()
|
||||
for msg in reversed(text_run)
|
||||
if msg["content"].strip()
|
||||
]
|
||||
if isinstance(cur_text, str) and cur_text.strip():
|
||||
text_parts.append(cur_text.strip())
|
||||
merged_msg = dict(m)
|
||||
if text_parts:
|
||||
merged_msg["content"] = "\n\n".join(text_parts)
|
||||
merged.append(merged_msg)
|
||||
continue
|
||||
merged.append(m)
|
||||
|
||||
# Strip tool_calls from assistant messages whose predecessor is not a
|
||||
# user or tool message, then drop orphan tool messages and dangling
|
||||
# tool_calls whose responses are gone.
|
||||
cleaned: List[Dict] = []
|
||||
in_batch = False # are we right after an assistant tool_calls (or mid-batch)?
|
||||
for m in msgs:
|
||||
for m in merged:
|
||||
role = m.get("role")
|
||||
if role == "tool":
|
||||
if in_batch:
|
||||
@@ -245,13 +313,21 @@ def _sanitize_tool_messages(msgs: List[Dict]) -> List[Dict]:
|
||||
# else: orphan — drop
|
||||
continue
|
||||
if role == "assistant" and m.get("tool_calls"):
|
||||
prev_role = cleaned[-1].get("role") if cleaned else None
|
||||
if prev_role not in ("user", "tool"):
|
||||
# Invalid predecessor — strip tool_calls to avoid provider
|
||||
# errors; keep the message only if it still carries content.
|
||||
m = {k: v for k, v in m.items() if k != "tool_calls"}
|
||||
in_batch = False
|
||||
if not _has_preservable_content(m.get("content")):
|
||||
continue # nothing left worth keeping
|
||||
cleaned.append(m)
|
||||
continue
|
||||
in_batch = True
|
||||
else:
|
||||
in_batch = False
|
||||
cleaned.append(m)
|
||||
|
||||
# Pass 2: drop assistant tool_calls messages that have NO following
|
||||
# tool response (dangling) — walk backwards so we know what follows.
|
||||
out: List[Dict] = []
|
||||
for i, m in enumerate(cleaned):
|
||||
if m.get("role") == "assistant" and m.get("tool_calls"):
|
||||
@@ -261,7 +337,7 @@ def _sanitize_tool_messages(msgs: List[Dict]) -> List[Dict]:
|
||||
# tool_calls so it's a plain assistant turn (preserves any
|
||||
# text content the model produced alongside the calls).
|
||||
m = {k: v for k, v in m.items() if k != "tool_calls"}
|
||||
if not (m.get("content") or "").strip():
|
||||
if not _has_preservable_content(m.get("content")):
|
||||
continue # nothing left worth keeping
|
||||
out.append(m)
|
||||
return out
|
||||
|
||||
@@ -516,3 +516,235 @@ messages = [
|
||||
assert '[Image]' in keep_recent_image_turns(messages, keep=1)[0]['content'][0]['text']
|
||||
'''
|
||||
subprocess.run([sys.executable, '-c', script], check=True, timeout=8)
|
||||
|
||||
|
||||
class TestSanitizeToolMessagesPredecessor:
|
||||
"""``_sanitize_tool_messages`` must return a transcript that satisfies the
|
||||
provider ordering contract and is stable under repeated sanitation.
|
||||
|
||||
Contract under test:
|
||||
- an assistant ``tool_calls`` turn follows a user or tool turn;
|
||||
- every tool message belongs to a preceding ``tool_calls`` batch;
|
||||
- an assistant ``tool_calls`` turn has at least one tool response.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _violations(msgs):
|
||||
problems = []
|
||||
in_batch = False
|
||||
for i, m in enumerate(msgs):
|
||||
role = m.get("role")
|
||||
if role == "tool":
|
||||
if not in_batch:
|
||||
problems.append(f"[{i}] orphan tool message")
|
||||
continue
|
||||
prev = msgs[i - 1] if i else None
|
||||
prev_role = prev.get("role") if prev else None
|
||||
if role == "assistant" and m.get("tool_calls"):
|
||||
if prev_role not in ("user", "tool"):
|
||||
problems.append(f"[{i}] tool_calls after {prev_role!r}")
|
||||
nxt = msgs[i + 1] if i + 1 < len(msgs) else None
|
||||
if not (nxt and nxt.get("role") == "tool"):
|
||||
problems.append(f"[{i}] dangling tool_calls")
|
||||
in_batch = True
|
||||
else:
|
||||
in_batch = False
|
||||
return problems
|
||||
|
||||
def _sanitize(self, msgs):
|
||||
result = cc._sanitize_tool_messages(msgs)
|
||||
assert self._violations(result) == [], (
|
||||
"sanitized transcript still violates the ordering contract: "
|
||||
f"{self._violations(result)}"
|
||||
)
|
||||
assert cc._sanitize_tool_messages(result) == result, (
|
||||
"sanitizing the sanitized transcript changed it"
|
||||
)
|
||||
return result
|
||||
|
||||
def test_user_then_calls_is_unchanged(self):
|
||||
msgs = [
|
||||
{"role": "user", "content": "search for X"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert len(result) == 3
|
||||
assert result[1].get("tool_calls") is not None
|
||||
|
||||
def test_second_batch_after_tool_response_is_unchanged(self):
|
||||
"""A call batch whose predecessor is a tool response is valid."""
|
||||
msgs = [
|
||||
{"role": "user", "content": "search for X"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c2", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c2", "content": "more results"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert len(result) == 5
|
||||
assert result[3].get("tool_calls") is not None
|
||||
|
||||
def test_two_batch_mid_history_reaches_stable_result(self):
|
||||
"""A transcript that starts mid-history has no user/tool predecessor
|
||||
for its first call batch. Dropping that batch leaves the second one
|
||||
with no predecessor either, so the contract-valid, stable result is
|
||||
empty; the sanitizer must not return the half-repaired transcript
|
||||
where the second batch sits at the start without a predecessor.
|
||||
"""
|
||||
msgs = [
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c2", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c2", "content": "more results"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert result == []
|
||||
|
||||
def test_assistant_text_then_completed_call_is_preserved(self):
|
||||
"""An assistant text turn directly before a completed call batch must
|
||||
not cost the batch its tool_calls: the text is merged into the call
|
||||
message so the executed call/result pair stays in the transcript.
|
||||
"""
|
||||
msgs = [
|
||||
{"role": "user", "content": "plan my day"},
|
||||
{"role": "assistant", "content": "I'll check the calendar.",
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "manage_calendar"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "[]"},
|
||||
{"role": "assistant", "content": "Found it."},
|
||||
{"role": "assistant", "content": "Let me look at day two.",
|
||||
"tool_calls": [{"id": "c2", "function": {"name": "manage_calendar"}}]},
|
||||
{"role": "tool", "tool_call_id": "c2", "content": "[]"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert len(result) == 5
|
||||
assert result[1].get("tool_calls") is not None
|
||||
merged = result[3]
|
||||
assert merged.get("content") == "Found it.\n\nLet me look at day two."
|
||||
assert merged.get("tool_calls") is not None
|
||||
assert merged["tool_calls"][0]["id"] == "c2"
|
||||
|
||||
def test_assistant_text_directly_before_calls_merges(self):
|
||||
msgs = [
|
||||
{"role": "user", "content": "hi"},
|
||||
{"role": "assistant", "content": "Thinking out loud"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert len(result) == 3
|
||||
assert result[1]["content"] == "Thinking out loud"
|
||||
assert result[1]["tool_calls"][0]["id"] == "c1"
|
||||
|
||||
def test_chain_of_text_turns_before_calls_merges_into_one(self):
|
||||
msgs = [
|
||||
{"role": "user", "content": "hi"},
|
||||
{"role": "assistant", "content": "one"},
|
||||
{"role": "assistant", "content": "two"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert len(result) == 3
|
||||
assert result[1]["content"] == "one\n\ntwo"
|
||||
assert result[1]["tool_calls"][0]["id"] == "c1"
|
||||
|
||||
def test_system_predecessor_calls_dropped(self):
|
||||
msgs = [
|
||||
{"role": "system", "content": "You are helpful"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert [m["role"] for m in result] == ["system"]
|
||||
|
||||
def test_first_message_calls_dropped(self):
|
||||
msgs = [
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
]
|
||||
assert self._sanitize(msgs) == []
|
||||
|
||||
def test_calls_with_content_after_system_keep_text(self):
|
||||
msgs = [
|
||||
{"role": "system", "content": "You are helpful"},
|
||||
{"role": "assistant", "content": "I'll search for you",
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "web_search"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "results"},
|
||||
]
|
||||
result = self._sanitize(msgs)
|
||||
assert [m["role"] for m in result] == ["system", "assistant"]
|
||||
assert result[1].get("tool_calls") is None
|
||||
assert "I'll search for you" in result[1]["content"]
|
||||
|
||||
def test_repeated_sanitation_is_stable_across_shapes(self):
|
||||
shapes = [
|
||||
[],
|
||||
[{"role": "user", "content": "hi"}],
|
||||
[{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "f"}}]}],
|
||||
[{"role": "tool", "tool_call_id": "c1", "content": "x"}],
|
||||
[{"role": "user", "content": "hi"},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "x"}],
|
||||
[{"role": "assistant", "content": "a"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "f"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "x"},
|
||||
{"role": "assistant", "content": "b"},
|
||||
{"role": "assistant", "content": "c",
|
||||
"tool_calls": [{"id": "c2", "function": {"name": "f"}}]},
|
||||
{"role": "tool", "tool_call_id": "c2", "content": "y"}],
|
||||
]
|
||||
for msgs in shapes:
|
||||
once = cc._sanitize_tool_messages(msgs)
|
||||
assert self._violations(once) == []
|
||||
assert cc._sanitize_tool_messages(once) == once
|
||||
|
||||
def test_input_messages_are_not_mutated(self):
|
||||
import copy
|
||||
|
||||
msgs = [
|
||||
{"role": "user", "content": "hi"},
|
||||
{"role": "assistant", "content": "text"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "f"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "x"},
|
||||
]
|
||||
before = copy.deepcopy(msgs)
|
||||
cc._sanitize_tool_messages(msgs)
|
||||
assert msgs == before
|
||||
|
||||
@pytest.mark.parametrize("content", [
|
||||
[{"type": "text", "text": "Keep observation"}],
|
||||
[{"type": "image_url", "image_url": {"url": "https://example.com/image.png"}}],
|
||||
[],
|
||||
])
|
||||
@pytest.mark.parametrize("dangling", [False, True])
|
||||
def test_stripped_calls_preserve_multimodal_content(self, content, dangling):
|
||||
if dangling:
|
||||
messages = [
|
||||
{"role": "user", "content": "Inspect image"},
|
||||
{"role": "assistant", "content": content,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "inspect"}}]},
|
||||
]
|
||||
else:
|
||||
messages = [
|
||||
{"role": "system", "content": "Instructions"},
|
||||
{"role": "assistant", "content": content,
|
||||
"tool_calls": [{"id": "c1", "function": {"name": "inspect"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "orphan"},
|
||||
]
|
||||
output = self._sanitize(messages)
|
||||
expected = [messages[0]]
|
||||
if content:
|
||||
expected.append({"role": "assistant", "content": content})
|
||||
assert output == expected
|
||||
|
||||
Reference in New Issue
Block a user