diff --git a/src/context_compactor.py b/src/context_compactor.py index 09e2e59f6..6fb93cedc 100644 --- a/src/context_compactor.py +++ b/src/context_compactor.py @@ -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 diff --git a/tests/test_context_compactor.py b/tests/test_context_compactor.py index 08f1728e3..237c307de 100644 --- a/tests/test_context_compactor.py +++ b/tests/test_context_compactor.py @@ -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