fix(bedrock): use non-whitespace placeholder for empty Converse content blocks - #64858
fix(bedrock): use non-whitespace placeholder for empty Converse content blocks#64858taoxee wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the Converse-only validation path. The whitespace-content premise is present on current main: agent/bedrock_adapter.py:504-550, 586-600, and 637-668 can emit whitespace-only text blocks.
Problems
- No regression tests are included. Current
tests/agent/test_bedrock_adapter.py:325-330explicitly permits the single-space placeholder, despite the PR body claiming unit and integration coverage. - The added
text.strip()at the changed text-part path assumespart["text"]is a string. ANoneor other non-string value raises instead of being filtered; apply the same type guard in the new system-list branch. - The bundled runtime-provider hunk is stale: current main already has bearer-token routing in
hermes_cli/runtime_provider.py:1979-2008from5e6a0d9ee, covered bytests/agent/test_bedrock_adapter.py:1756-1796. Preserve that behavior when salvaging the whitespace fix.
Suggested changes
- Add recursive assertions that all emitted Converse text blocks, including nested tool results, are non-whitespace strings.
- Use
isinstance(value, str) and value.strip()for text-part and system-part filtering.
Automated hermes-sweeper review.
| 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", "") |
There was a problem hiding this comment.
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.
…nt blocks
Bedrock's Converse/ConverseStream API rejects text content blocks whose
text is empty OR whitespace-only:
ValidationException: ... messages: text content blocks must contain
non-whitespace text
convert_messages_to_converse() used a single space {"text": " "} as its
placeholder for otherwise-empty content in six places, and appended
empty/whitespace text blocks for system messages, list string parts, and
tool results. A single space is whitespace-only, so it trips the exact
error above. The AnthropicBedrock SDK path tolerated these blocks, which
is why this only surfaces on the Converse path.
- Introduce _EMPTY_TEXT_PLACEHOLDER = "(empty)" and use it for all
empty-content fallbacks (replacing the six {"text": " "} spots).
- Add _nonblank_text(value) — isinstance(value, str) and value.strip() —
and gate every text-block append with it, so empty/whitespace/non-string
text (None, ints) is dropped or replaced, never emitted or raised on.
Tests: update the pre-existing TestEmptyTextBlockFix / placeholder tests
to assert the non-whitespace invariant, and add
TestConverseNoWhitespaceOnlyTextBlocks with a recursive walk asserting no
emitted text block (including nested toolResult content) is blank, plus a
non-string text-part case that must be filtered rather than crash.
c0e26c8 to
2b5a2af
Compare
|
Rebased onto current
|
SummaryFourteen PRs address or reference this three-issue complex. #9491 targets the Anthropic assistant-list cause of #9486; the Bedrock Converse variants replace invalid blank placeholders or sanitize additional system/list/tool-result paths for #39829 and #55092, with merged #67978 providing the recorded best fix for the latter two issues; #20366/#21202 instead concern response reasoning normalization. Related pull requests
Duplicates#39850, #39871, #40075, and #41985 are direct placeholder/padding variants; #60628, #63983, and #64858 are broader competing implementations superseded by merged #67978. #55093 and the native Bedrock portion of #24743 overlap subsets of #67978; #20366 is superseded by #21202, while #9491 and #57985 remain a separate Anthropic-path cluster. Suggested consolidationClose #39850, #39871, #41985, #55093, #60628, #63983, and #64858 as duplicates of merged #67978; #40075 is already closed in that chain. This departs from their visible keep_open reviews because #67978's merged diff covers the cited list-string, system, padding, non-string, and nested toolResult gaps with regression tests; for #24743, author action: rebase onto main and split out only the copied outbound-only custom-proxy sanitizer, while #57985 should keep open with a salvage path for ordered replay and cache-control-safe normalization. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I39829(["issue #39829 (open)"])
I55092(["issue #55092 (open)"])
subgraph Dup60628 ["PRs duplicating each other"]
P60628["PR #60628 (open)"]
P63983["PR #63983 (open)"]
P64858["PR #64858 (open)"]
end
P64858 -->|fixes| I39829
P64858 -->|fixes| I55092
class I39829 open
class I55092 open
class P60628 open
class P63983 open
class P64858 open
class P64858 target
click I39829 "https://github.com/NousResearch/hermes-agent/issues/39829"
click I55092 "https://github.com/NousResearch/hermes-agent/issues/55092"
click P60628 "https://github.com/NousResearch/hermes-agent/pull/60628"
click P63983 "https://github.com/NousResearch/hermes-agent/pull/63983"
click P64858 "https://github.com/NousResearch/hermes-agent/pull/64858"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 14 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 103 kB of PR diffs, 35 kB of issue/PR text, 13 kB of discussion (19 comments), 26 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Problem
Bedrock's Converse/ConverseStream API rejects text content blocks whose text is empty or whitespace-only:
convert_messages_to_converse()(agent/bedrock_adapter.py) used a single space{"text": " "}as its placeholder for otherwise-empty content in six places, and appended empty/whitespace text blocks for system messages, list string parts, and tool results. A single space is whitespace-only, so it trips the exact error above.The AnthropicBedrock SDK path tolerated these blocks, which is why this only surfaces on the Converse path — e.g. once bearer-token users route Claude through Converse (see #64857).
Fix
_EMPTY_TEXT_PLACEHOLDER = "(empty)"and use it for all empty-content fallbacks (replacing the six{"text": " "}spots)..strip()-guard every text-block append (list string parts,type:textparts, system-message list branch, tool-result content) so empty/whitespace blocks are dropped or replaced — never emitted.Testing
convert_messages_to_converse()on messages containing empty strings, whitespace-only strings, empty assistant content, and empty tool results produces zero whitespace-only text blocks.ValidationException.Note
Surfaces most directly on top of #64857 (which routes Claude through the Converse path for bearer-token auth). This fix is independent and correct on its own, but the two are best reviewed together; suggest merging #64857 first.