From 29f9cfeb4a3966e744ff63e6941264483fecbcc1 Mon Sep 17 00:00:00 2001 From: ygd58 Date: Thu, 23 Jul 2026 05:56:59 +0000 Subject: [PATCH] fix(anthropic): don't fall back to raw content when all blocks filtered Follow-up per independent review of #68633 (GPT-5.6-sol-xhigh in Codex, reviewer egilewski) on this PR. Two real bugs in the blank-text-block filtering added by that fix: 1. `effective = blocks or content` fell back to the RAW, unfiltered `content` variable whenever every block was filtered out as blank -- which happens precisely when the entire message content WAS the blank/whitespace payload the filter exists to remove (a sole blank text block, a sole cache-marked blank block, or standalone whitespace scalar content with no tool_calls). The fallback silently restored the exact invalid content the filtering just stripped, leaving the message provider-invalid. Fixed: `effective = blocks if blocks else [{"type": "text", "text": "(empty)"}]` -- never falls back to raw `content`. Also moved the cache_control application (both the relocated-from-a-dropped-block marker and the message-level marker) to run against `effective` instead of the pre-fallback `blocks`, so a cache marker on a block that was the ONLY content still lands on the (empty) placeholder rather than being silently lost when `blocks` was empty at the point it would otherwise have been applied. 2. The normal-path blank-text check used `(blk.get("text") or "").strip()`, which is not type-safe for a truthy NON-string, non-None text value (e.g. an int or dict from an invalid upstream payload) -- `or` doesn't substitute for a truthy value, so `(7 or "").strip()` still raises AttributeError. Now checks `isinstance(text, str)` first, matching the replay path's `_sanitize_replay_block()`, which the reviewer confirmed was already correctly type-safe. Added regression tests for: sole blank list block, sole whitespace scalar content, sole cache-marked blank block (marker relocation to the placeholder), a truthy non-string (int) text value both mixed with a surviving tool_use and as the sole content, and a dict-valued text field. 7/7 new tests pass; 193/193 in the full tests/agent/test_anthropic_adapter.py file; 23/23 in tests/agent/test_prompt_caching.py (unaffected, confirmed). --- agent/anthropic_adapter.py | 53 ++++++------ tests/agent/test_anthropic_adapter.py | 116 ++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 25 deletions(-) diff --git a/agent/anthropic_adapter.py b/agent/anthropic_adapter.py index 9341f842f911..756dd62c4e9d 100644 --- a/agent/anthropic_adapter.py +++ b/agent/anthropic_adapter.py @@ -2061,15 +2061,18 @@ def _convert_assistant_message(m: Dict[str, Any]) -> Dict[str, Any]: # Bedrock and strict Anthropic-compatible endpoints reject # text blocks where "text" is empty or whitespace-only. The # ordered-replay path enforces the same invariant via - # _sanitize_replay_block(). Type-safe: input text blocks can - # carry text=None (_convert_content_part_to_anthropic - # preserves it from an invalid upstream payload), which a - # bare .strip() would crash on. + # _sanitize_replay_block(). Type-safe against ANY invalid + # "text" value from an upstream payload -- None, or a + # truthy non-string like an int -- not just None: checking + # isinstance() first (rather than `blk.get("text") or ""`) + # means a non-string value is treated as blank/invalid + # instead of reaching .strip() and raising AttributeError. for blk in converted_content: + _blk_text = blk.get("text") if isinstance(blk, dict) else None if ( isinstance(blk, dict) and blk.get("type") == "text" - and not (blk.get("text") or "").strip() + and (not isinstance(_blk_text, str) or not _blk_text.strip()) ): if isinstance(blk.get("cache_control"), dict): _relocated_cache_control = blk["cache_control"] @@ -2097,13 +2100,6 @@ def _convert_assistant_message(m: Dict[str, Any]) -> Dict[str, Any]: "name": fn.get("name", ""), "input": parsed_args, }) - if _relocated_cache_control is not None: - _apply_assistant_cache_control_to_last_cacheable_block( - blocks, _relocated_cache_control - ) - _apply_assistant_cache_control_to_last_cacheable_block( - blocks, m.get("cache_control") - ) # Kimi's /coding endpoint (Anthropic protocol) requires assistant # tool-call messages to carry reasoning_content when thinking is # enabled server-side. Preserve it as a thinking block so Kimi @@ -2129,19 +2125,26 @@ def _convert_assistant_message(m: Dict[str, Any]) -> Dict[str, Any]: ) if isinstance(reasoning_content, str) and not _already_has_thinking: blocks.insert(0, {"type": "thinking", "thinking": reasoning_content}) - # Anthropic rejects empty assistant content - effective = blocks or content - if not effective or effective == "": - effective = [{"type": "text", "text": _EMPTY_TEXT_PLACEHOLDER}] - elif isinstance(effective, list): - # The all-empty guard above misses a list that still contains a - # whitespace-only text block (e.g. from a content array of blank parts, - # or compression). Those also trip "text content blocks must contain - # non-whitespace text" (#69512). Coerce text blocks in place; other - # block types (thinking/tool_use/image) are left untouched. - for blk in effective: - if isinstance(blk, dict) and blk.get("type") == "text": - blk["text"] = _safe_text(blk.get("text", "")) + # Anthropic rejects empty assistant content. IMPORTANT: fall back only + # to the placeholder, never to the raw `content` variable -- `content` + # is the UNFILTERED original message content, and can itself be exactly + # the blank/whitespace-only payload the filtering above just removed + # (a sole blank text block, or scalar whitespace with no tool_calls). + # `blocks or content` there would silently restore the invalid provider + # payload this function exists to prevent (#69512). + effective = blocks if blocks else [{"type": "text", "text": _EMPTY_TEXT_PLACEHOLDER}] + # Applied here (after the empty-fallback resolution) rather than + # earlier against `blocks` directly, so a cache_control relocated from + # a dropped blank block that was the ONLY block still lands on the + # (empty) placeholder instead of being silently lost when blocks was + # empty at the point the marker would otherwise have been applied. + if _relocated_cache_control is not None: + _apply_assistant_cache_control_to_last_cacheable_block( + effective, _relocated_cache_control + ) + _apply_assistant_cache_control_to_last_cacheable_block( + effective, m.get("cache_control") + ) return {"role": "assistant", "content": effective} diff --git a/tests/agent/test_anthropic_adapter.py b/tests/agent/test_anthropic_adapter.py index 405e8804f70c..4661073277a0 100644 --- a/tests/agent/test_anthropic_adapter.py +++ b/tests/agent/test_anthropic_adapter.py @@ -2818,3 +2818,119 @@ class TestBlankTextBlockFiltering: cacheable_with_marker = [b for b in blocks if isinstance(b.get("cache_control"), dict)] assert len(cacheable_with_marker) == 1 assert cacheable_with_marker[0]["type"] == "tool_use" + + +class TestAllBlankFallbackAndNonStringText: + """Regression tests for the two bugs found in independent review of + #68633 (GPT-5.6-sol-xhigh in Codex, egilewski): + + 1. `effective = blocks or content` fell back to the RAW, unfiltered + `content` when every block was filtered out as blank -- restoring + exactly the invalid (blank/whitespace) payload the filter exists to + remove, for any message where blank content is the ONLY content + (no surviving tool_use/text/thinking block). + 2. The normal-path blank-text check used `(blk.get("text") or "").strip()`, + which is not type-safe for a truthy NON-string, non-None text value + (e.g. an int) -- `or` doesn't substitute for a truthy value, so + `(7 or "").strip()` still raises AttributeError. + """ + + def _convert(self, message): + from agent.anthropic_adapter import _convert_assistant_message + return _convert_assistant_message(message) + + def test_sole_blank_list_block_falls_back_to_placeholder_not_raw_content(self): + """A message whose ONLY content is a single blank text block (no + tool_calls, nothing else) must resolve to the "(empty)" placeholder, + not silently restore the raw blank content list.""" + msg = {"role": "assistant", "content": [{"type": "text", "text": " "}]} + result = self._convert(msg) + blocks = result["content"] + assert blocks == [{"type": "text", "text": "(empty)"}], ( + f"Must fall back to the placeholder, not restore raw blank content: {blocks}" + ) + + def test_sole_whitespace_scalar_content_falls_back_to_placeholder(self): + """Same guarantee for scalar (non-list) whitespace-only content + with no tool_calls -- the earlier scalar-whitespace fix filters it + out of `blocks`, but the empty-fallback previously restored the raw + `content` string, which is itself the invalid whitespace payload.""" + msg = {"role": "assistant", "content": " \n\t "} + result = self._convert(msg) + blocks = result["content"] + assert blocks == [{"type": "text", "text": "(empty)"}], ( + f"Must fall back to the placeholder, not restore raw whitespace content: {blocks}" + ) + + def test_sole_cache_marked_blank_block_relocates_marker_to_placeholder(self): + """A message whose ONLY content is a blank text block that also + carries cache_control: the marker must not be silently dropped just + because there's nothing else to relocate it onto -- it must land on + the (empty) placeholder that replaces the dropped block.""" + msg = { + "role": "assistant", + "content": [{"type": "text", "text": "", "cache_control": {"type": "ephemeral"}}], + } + result = self._convert(msg) + blocks = result["content"] + assert blocks == [ + {"type": "text", "text": "(empty)", "cache_control": {"type": "ephemeral"}} + ], f"cache_control must relocate onto the (empty) placeholder: {blocks}" + + def test_mixed_blank_plus_survivor_still_drops_blank_and_keeps_survivor(self): + """Sanity check the fix didn't regress the already-covered mixed + case: a blank block alongside a real tool_use must still just drop + the blank one, not fall back to the placeholder.""" + msg = { + "role": "assistant", + "content": [{"type": "text", "text": ""}], + "tool_calls": [ + {"id": "call_1", "function": {"name": "web_search", + "arguments": '{"query": "test"}'}}, + ], + } + result = self._convert(msg) + blocks = result["content"] + assert not any(b.get("type") == "text" for b in blocks) + assert any(b.get("type") == "tool_use" for b in blocks) + + def test_non_string_truthy_text_treated_as_invalid_not_crash(self): + """Regression: text=7 (a truthy int, not None) must not reach + .strip() and raise AttributeError -- it must be treated the same as + blank/invalid text and dropped.""" + msg = { + "role": "assistant", + "content": [ + {"type": "text", "text": 7}, + {"type": "tool_use", "id": "call_int", "name": "web_search", + "input": {"query": "test"}}, + ], + } + result = self._convert(msg) # must not raise + blocks = result["content"] + text_blocks = [b for b in blocks if b.get("type") == "text"] + tool_blocks = [b for b in blocks if b.get("type") == "tool_use"] + assert len(text_blocks) == 0, f"Non-string text value must be dropped, not kept: {text_blocks}" + assert len(tool_blocks) == 1 + + def test_non_string_truthy_text_as_sole_content_falls_back_to_placeholder(self): + """Combines both bugs: a truthy non-string text value as the ONLY + content must not crash, and must resolve to the placeholder.""" + msg = {"role": "assistant", "content": [{"type": "text", "text": 7}]} + result = self._convert(msg) # must not raise + blocks = result["content"] + assert blocks == [{"type": "text", "text": "(empty)"}], blocks + + def test_dict_valued_text_treated_as_invalid_not_crash(self): + """Another truthy non-string shape (dict) must also be safely dropped.""" + msg = { + "role": "assistant", + "content": [{"type": "text", "text": {"nested": "garbage"}}], + "tool_calls": [ + {"id": "call_d", "function": {"name": "web_search", + "arguments": '{"query": "test"}'}}, + ], + } + result = self._convert(msg) # must not raise + blocks = result["content"] + assert not any(b.get("type") == "text" for b in blocks)