Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 40 additions & 13 deletions agent/bedrock_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -490,33 +490,54 @@ def convert_tools_to_converse(tools: List[Dict]) -> List[Dict]:
return result


# Placeholder for otherwise-empty content blocks. Bedrock's Converse API
# rejects text content blocks that are empty OR whitespace-only with
# ``ValidationException: text content blocks must contain non-whitespace
# text``. A single space (the previous placeholder) is whitespace-only and
# still trips this — so the placeholder must contain a non-whitespace char.
_EMPTY_TEXT_PLACEHOLDER = "(empty)"


def _nonblank_text(value) -> bool:
"""True only when ``value`` is a string with non-whitespace content.

Used to gate every Converse text-block emit. Guards against non-string
``text`` payloads (None, ints, nested dicts) that would raise on
``.strip()`` — those are simply treated as blank and dropped/replaced.
"""
return isinstance(value, str) and bool(value.strip())


def _convert_content_to_converse(content) -> List[Dict]:
"""Convert OpenAI message content (string or list) to Converse content blocks.

Handles:
- Plain text strings → [{"text": "..."}]
- Content arrays with text/image_url parts → mixed text/image blocks

Filters out empty text blocks — Bedrock's Converse API rejects messages
where a text content block has an empty ``text`` field (ValidationException:
"text content blocks must be non-empty"). Ref: issue #9486.
Filters out empty/whitespace-only text blocks — Bedrock's Converse API
rejects messages where a text content block is empty or whitespace-only
(ValidationException: "text content blocks must contain non-whitespace
text"). Ref: issue #9486.
"""
if content is None:
return [{"text": " "}]
return [{"text": _EMPTY_TEXT_PLACEHOLDER}]
if isinstance(content, str):
return [{"text": content}] if content.strip() else [{"text": " "}]
return [{"text": content}] if content.strip() else [{"text": _EMPTY_TEXT_PLACEHOLDER}]
if isinstance(content, list):
blocks = []
for part in content:
if isinstance(part, str):
blocks.append({"text": part})
if part.strip():
blocks.append({"text": part})
continue
if not isinstance(part, dict):
continue
part_type = part.get("type", "")
if part_type == "text":
text = part.get("text", "")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part.get("text", "") is not guaranteed to return a string; for example, a text part with "text": null reaches this line and raises AttributeError. Please use an isinstance(text, str) and text.strip() guard here and in the analogous system-message branch.

blocks.append({"text": text if text else " "})
if _nonblank_text(text):
blocks.append({"text": text})
elif part_type == "image_url":
image_url = part.get("image_url", {})
url = image_url.get("url", "") if isinstance(image_url, dict) else ""
Expand Down Expand Up @@ -547,7 +568,7 @@ def _convert_content_to_converse(content) -> List[Dict]:
# Remote URL — Converse doesn't support URLs directly,
# include as text reference for the model.
blocks.append({"text": f"[Image: {url}]"})
return blocks if blocks else [{"text": " "}]
return blocks if blocks else [{"text": _EMPTY_TEXT_PLACEHOLDER}]
return [{"text": str(content)}]


Expand Down Expand Up @@ -584,15 +605,21 @@ def convert_messages_to_converse(
elif isinstance(content, list):
for part in content:
if isinstance(part, dict) and part.get("type") == "text":
system_blocks.append({"text": part.get("text", "")})
elif isinstance(part, str):
_t = part.get("text", "")
if _nonblank_text(_t):
system_blocks.append({"text": _t})
elif _nonblank_text(part):
system_blocks.append({"text": part})
continue

if role == "tool":
# Tool result messages → merge into the preceding user turn
tool_call_id = msg.get("tool_call_id", "")
result_content = content if isinstance(content, str) else json.dumps(content)
# Converse rejects empty/whitespace-only text blocks (incl. tool
# results), so substitute a non-whitespace placeholder.
if not _nonblank_text(result_content):
result_content = _EMPTY_TEXT_PLACEHOLDER
tool_result_block = {
"toolResult": {
"toolUseId": tool_call_id,
Expand Down Expand Up @@ -635,7 +662,7 @@ def convert_messages_to_converse(
})

if not content_blocks:
content_blocks = [{"text": " "}]
content_blocks = [{"text": _EMPTY_TEXT_PLACEHOLDER}]

# Merge with previous assistant message if needed (strict alternation)
if converse_msgs and converse_msgs[-1]["role"] == "assistant":
Expand All @@ -661,11 +688,11 @@ def convert_messages_to_converse(

# Converse requires the first message to be from the user
if converse_msgs and converse_msgs[0]["role"] != "user":
converse_msgs.insert(0, {"role": "user", "content": [{"text": " "}]})
converse_msgs.insert(0, {"role": "user", "content": [{"text": _EMPTY_TEXT_PLACEHOLDER}]})

# Converse requires the last message to be from the user
if converse_msgs and converse_msgs[-1]["role"] != "user":
converse_msgs.append({"role": "user", "content": [{"text": " "}]})
converse_msgs.append({"role": "user", "content": [{"text": _EMPTY_TEXT_PLACEHOLDER}]})

return (system_blocks if system_blocks else None, converse_msgs)

Expand Down
159 changes: 150 additions & 9 deletions tests/agent/test_bedrock_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,10 @@ def test_empty_content_gets_placeholder(self):
from agent.bedrock_adapter import convert_messages_to_converse
messages = [{"role": "user", "content": ""}]
system, msgs = convert_messages_to_converse(messages)
# Empty string should get a space placeholder
assert msgs[0]["content"][0]["text"].strip() != "" or msgs[0]["content"][0]["text"] == " "
# Empty string must get a NON-whitespace placeholder — Converse rejects
# empty/whitespace-only text blocks (must contain non-whitespace text).
placeholder = msgs[0]["content"][0]["text"]
assert placeholder.strip() != ""

def test_image_data_url_converted(self):
from agent.bedrock_adapter import convert_messages_to_converse
Expand Down Expand Up @@ -361,6 +363,142 @@ def test_multiple_system_messages_merged(self):
assert system[1]["text"] == "Rule 2"


def _iter_converse_text_blocks(node):
"""Yield every ``text`` value emitted anywhere in a Converse payload.

Walks system blocks, message content blocks, and nested toolResult
content so the whitespace invariant can be asserted recursively.
"""
if isinstance(node, dict):
for key, value in node.items():
if key == "text":
yield value
else:
yield from _iter_converse_text_blocks(value)
elif isinstance(node, list):
for item in node:
yield from _iter_converse_text_blocks(item)


class TestConverseNoWhitespaceOnlyTextBlocks:
"""Bedrock's Converse API rejects empty/whitespace-only text blocks with
``ValidationException: text content blocks must contain non-whitespace
text``. No emitted text block — including nested tool results — may be
blank. Ref: issue #9486, PR #64858."""

def _assert_all_text_nonblank(self, system, msgs):
texts = list(_iter_converse_text_blocks(system)) + list(
_iter_converse_text_blocks(msgs)
)
# There must be at least one text block (or the walk is vacuous), and
# every one must be a non-whitespace string.
for text in texts:
assert isinstance(text, str), f"non-string text block: {text!r}"
assert text.strip() != "", f"whitespace-only text block: {text!r}"

def test_empty_and_whitespace_user_content(self):
from agent.bedrock_adapter import convert_messages_to_converse

for content in ("", " ", "\n\t ", None):
system, msgs = convert_messages_to_converse(
[{"role": "user", "content": content}]
)
self._assert_all_text_nonblank(system, msgs)

def test_whitespace_only_list_parts_dropped(self):
from agent.bedrock_adapter import convert_messages_to_converse

messages = [{
"role": "user",
"content": [
{"type": "text", "text": " "},
{"type": "text", "text": "\n"},
" ",
{"type": "text", "text": "real content"},
],
}]
system, msgs = convert_messages_to_converse(messages)
self._assert_all_text_nonblank(system, msgs)
# The one real block survives; the blanks are dropped.
user_texts = [
b["text"] for b in msgs[0]["content"] if "text" in b
]
assert user_texts == ["real content"]

def test_all_blank_list_parts_get_placeholder(self):
from agent.bedrock_adapter import convert_messages_to_converse

messages = [{
"role": "user",
"content": [{"type": "text", "text": " "}, {"type": "text", "text": ""}],
}]
system, msgs = convert_messages_to_converse(messages)
self._assert_all_text_nonblank(system, msgs)

def test_empty_tool_result(self):
from agent.bedrock_adapter import convert_messages_to_converse

for result in ("", " ", None):
messages = [
{"role": "user", "content": "run it"},
{"role": "tool", "tool_call_id": "t1", "content": result},
]
system, msgs = convert_messages_to_converse(messages)
self._assert_all_text_nonblank(system, msgs)
# The tool result block itself must be non-blank.
tool_texts = [
inner["text"]
for m in msgs
for block in m["content"]
if "toolResult" in block
for inner in block["toolResult"]["content"]
if "text" in inner
]
assert tool_texts and all(t.strip() for t in tool_texts)

def test_empty_assistant_content(self):
from agent.bedrock_adapter import convert_messages_to_converse

messages = [
{"role": "user", "content": "hi"},
{"role": "assistant", "content": ""},
{"role": "user", "content": "still there?"},
]
system, msgs = convert_messages_to_converse(messages)
self._assert_all_text_nonblank(system, msgs)

def test_whitespace_system_message(self):
from agent.bedrock_adapter import convert_messages_to_converse

messages = [
{"role": "system", "content": [
{"type": "text", "text": " "},
{"type": "text", "text": "Be concise."},
" ",
]},
{"role": "user", "content": "hi"},
]
system, msgs = convert_messages_to_converse(messages)
self._assert_all_text_nonblank(system, msgs)
assert [b["text"] for b in (system or [])] == ["Be concise."]

def test_non_string_text_part_does_not_raise(self):
"""A non-string ``text`` payload must be filtered, not crash on .strip()."""
from agent.bedrock_adapter import convert_messages_to_converse

messages = [{
"role": "user",
"content": [
{"type": "text", "text": None},
{"type": "text", "text": 123},
{"type": "text", "text": "ok"},
],
}]
system, msgs = convert_messages_to_converse(messages)
self._assert_all_text_nonblank(system, msgs)
assert [b["text"] for b in msgs[0]["content"] if "text" in b] == ["ok"]


# ---------------------------------------------------------------------------
# Response normalization: Converse → OpenAI
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1305,22 +1443,25 @@ def test_eu_claude(self):


class TestEmptyTextBlockFix:
"""Test that empty text blocks are replaced with space placeholders."""
"""Empty/whitespace-only content is replaced with a NON-whitespace
placeholder. Bedrock's Converse API rejects text blocks that are empty or
whitespace-only ("must contain non-whitespace text"), so a single space
is not a valid placeholder. Ref: PR #64858."""

def test_none_content_gets_space(self):
def test_none_content_gets_nonblank_placeholder(self):
from agent.bedrock_adapter import _convert_content_to_converse
blocks = _convert_content_to_converse(None)
assert blocks[0]["text"] == " "
assert blocks[0]["text"].strip() != ""

def test_empty_string_gets_space(self):
def test_empty_string_gets_nonblank_placeholder(self):
from agent.bedrock_adapter import _convert_content_to_converse
blocks = _convert_content_to_converse("")
assert blocks[0]["text"] == " "
assert blocks[0]["text"].strip() != ""

def test_whitespace_only_gets_space(self):
def test_whitespace_only_gets_nonblank_placeholder(self):
from agent.bedrock_adapter import _convert_content_to_converse
blocks = _convert_content_to_converse(" ")
assert blocks[0]["text"] == " "
assert blocks[0]["text"].strip() != ""

def test_real_text_preserved(self):
from agent.bedrock_adapter import _convert_content_to_converse
Expand Down