From 32b23bfb08138a8010dadd7b568d3aad40c17284 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Wed, 1 Jul 2026 14:11:40 +0530 Subject: [PATCH] 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 --- agent/context_compressor.py | 50 ++++++++---- tests/agent/test_context_compressor.py | 104 +++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 16 deletions(-) diff --git a/agent/context_compressor.py b/agent/context_compressor.py index 48b97bda787..7b6dfe68e63 100644 --- a/agent/context_compressor.py +++ b/agent/context_compressor.py @@ -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 diff --git a/tests/agent/test_context_compressor.py b/tests/agent/test_context_compressor.py index fe0bf3b4b5e..beb9c3ae39d 100644 --- a/tests/agent/test_context_compressor.py +++ b/tests/agent/test_context_compressor.py @@ -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)