fix(bedrock): add Claude Sonnet/Opus/Haiku 5 to Bedrock context-length table - #66167
fix(bedrock): add Claude Sonnet/Opus/Haiku 5 to Bedrock context-length table#66167Polyhistor wants to merge 3 commits into
Conversation
…h table
au.anthropic.claude-sonnet-5 (and opus-5/haiku-5) had no entry in
BEDROCK_CONTEXT_LENGTHS, so get_bedrock_context_length() silently fell
back to BEDROCK_DEFAULT_CONTEXT_LENGTH (128_000) instead of the actual
200_000 token window AWS documents for this model family on Bedrock.
Impact: ContextCompressor's small-context floor treats any resolved
window under 512K as 'small' and raises the compaction threshold to
75% of window. With the wrong 128K window that floor fires at 96,000
tokens instead of 150,000 tokens at the correct 200K window -- roughly
36% less usable headroom before Hermes force-compacts the conversation,
causing more frequent/premature compaction on long agentic sessions.
Verified locally:
get_bedrock_context_length('au.anthropic.claude-sonnet-5') -> 200000
(was 128000 before this change)
Related: #24059 corrects Claude 4.x 1M Bedrock entries; this independently adds the 200K Claude 5 identifiers. Not a duplicate. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Context length table update for Claude 5 family on Bedrock. Simple safe addition. No security concerns.
Reviewed by Hermes Agent
Bedrock Converse rejects text content blocks that are empty OR
whitespace-only (ValidationException: "text content blocks must
contain non-whitespace text"). The prior fix attempt substituted a
single space (" ") for missing content -- but a lone space IS
whitespace, so it was rejected by the exact same validation rule it
was meant to satisfy. This caused a deterministic, unrecoverable
retry-loop failure once any blank/whitespace assistant, tool, or user
turn entered history (most commonly via context-compaction rewriting
a turn to a blank string).
Adds _safe_text()/_EMPTY_TEXT_PLACEHOLDER ("(empty)") and applies it
everywhere a blank text block could reach the wire: user/assistant
content conversion, tool results, the assistant-empty-turn fallback,
and the first/last-message user-alternation padding. System-prompt
blocks are the one exception: blank parts are dropped entirely rather
than placeholder-filled, since a system prompt block should never
carry meaningless placeholder text.
Adds tests/agent/test_bedrock_empty_text_blocks.py (11 tests, was
already present uncommitted -- codifies the exact failing history
from issue NousResearch#9486 and asserts no blank block ever reaches Bedrock).
Verified against the actual failed request dump from this session
(27-message payload) -- replaying it through the fixed converter now
produces zero blank/whitespace-only blocks.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the missing Claude 5 Bedrock context entries. The premise is present on current main: agent/model_metadata.py:2259-2266 routes Bedrock models to get_bedrock_context_length(), while agent/bedrock_adapter.py:1307-1350 has no Claude 5 keys and returns the 128K default for unmatched IDs.
Problems
- The placeholder change will fail current tests:
tests/agent/test_bedrock_adapter.py:1310-1323asserts thatNone, empty, and whitespace-only content produce a literal space, while this PR changes those outputs to(empty)without updating the assertions. - The new
_safe_text()is not applied to supported string entries in list content.agent/bedrock_adapter.py:511-512still emits{"text": part}directly, so[" "]can still send a whitespace-only block to Bedrock.
Suggested changes
- Update the existing placeholder assertions and route list string items through
_safe_text(); add a list-string whitespace regression case.
Automated hermes-sweeper review.
| # Bedrock's Converse API rejects any text content block whose text is empty | ||
| # OR whitespace-only (ValidationException: "text content blocks must contain | ||
| # non-whitespace text"). A lone space is whitespace and is rejected too — the | ||
| # placeholder MUST itself be non-whitespace. Ref: issue #9486. |
There was a problem hiding this comment.
Please update tests/agent/test_bedrock_adapter.py:1310-1323 in this PR: those existing tests still assert the old literal-space placeholder and will fail after this constant changes the converter output.
There was a problem hiding this comment.
Addressed in bbb233e:
Updated tests/agent/test_bedrock_adapter.py::TestEmptyTextBlockFix — the placeholder assertions were stale (still expecting the old literal-space behavior); now assert _EMPTY_TEXT_PLACEHOLDER (non-whitespace) instead.
Fixed _convert_content_to_converse(): plain-string items inside a content list (e.g. [" "]) were emitted as {"text": part}" directly, bypassing _safe_text(). Now routed through _safe_text() like every other path. Added a regression test (test_whitespace_only_list_string_item_gets_placeholder`) plus a happy-path counterpart.
Full adapter test suite: 135 passed, 10 skipped (pre-existing, unrelated botocore-version-gated skips).
…text, update stale placeholder assertions Addresses hermes-sweeper review on PR NousResearch#66167: 1. _convert_content_to_converse() still emitted {"text": part} directly for plain-string items inside a content list (as opposed to {"type": "text"} dicts), bypassing _safe_text() entirely. A whitespace-only string item (e.g. [" "]) could still reach Bedrock as a blank block. Now routed through _safe_text(). 2. tests/agent/test_bedrock_adapter.py::TestEmptyTextBlockFix asserted the pre-fix behavior (whitespace -> literal space " "), contradicting the new _safe_text()/_EMPTY_TEXT_PLACEHOLDER behavior added in 4618095. Updated assertions to expect the non-whitespace placeholder, plus added a regression test for the list-string-item case above.
|
Addressed in bbb233e:
Full adapter test suite: 135 passed, 10 skipped (pre-existing, unrelated botocore-version-gated skips). |
…text, update stale placeholder assertions Addresses hermes-sweeper review on PR #66167: 1. _convert_content_to_converse() still emitted {"text": part} directly for plain-string items inside a content list (as opposed to {"type": "text"} dicts), bypassing _safe_text() entirely. A whitespace-only string item (e.g. [" "]) could still reach Bedrock as a blank block. Now routed through _safe_text(). 2. tests/agent/test_bedrock_adapter.py::TestEmptyTextBlockFix asserted the pre-fix behavior (whitespace -> literal space " "), contradicting the new _safe_text()/_EMPTY_TEXT_PLACEHOLDER behavior added in 4618095. Updated assertions to expect the non-whitespace placeholder, plus added a regression test for the list-string-item case above.
|
Merged the fix half via PR #67978 — both of your The context-table half of this PR was superseded by #67977: current main already carried |
…text, update stale placeholder assertions Addresses hermes-sweeper review on PR NousResearch#66167: 1. _convert_content_to_converse() still emitted {"text": part} directly for plain-string items inside a content list (as opposed to {"type": "text"} dicts), bypassing _safe_text() entirely. A whitespace-only string item (e.g. [" "]) could still reach Bedrock as a blank block. Now routed through _safe_text(). 2. tests/agent/test_bedrock_adapter.py::TestEmptyTextBlockFix asserted the pre-fix behavior (whitespace -> literal space " "), contradicting the new _safe_text()/_EMPTY_TEXT_PLACEHOLDER behavior added in 4618095. Updated assertions to expect the non-whitespace placeholder, plus added a regression test for the list-string-item case above.
Problem
au.anthropic.claude-sonnet-5(and theopus-5/haiku-5siblings) have no entry inBEDROCK_CONTEXT_LENGTHSinagent/bedrock_adapter.py. Since Bedrock'sListFoundationModelsAPI doesn't expose context window sizes,get_bedrock_context_length()falls back toBEDROCK_DEFAULT_CONTEXT_LENGTH(128,000) for any unmatched model — silently under-reporting the real window.AWS documents a 200,000 token default context window for Claude Sonnet 5 on Bedrock (same as the Sonnet 4.5/Opus 4 family already in the table), with a 1M beta available via header — not the 128K fallback.
Impact
ContextCompressorapplies a small-context floor: any model resolved under 512K is treated as 'small' and its compaction threshold is raised to 75% of window (to avoid thrashing on tight windows). With the wrong 128K window this floor fires at 96,000 tokens; at the correct 200K window it fires at 150,000 tokens — about 36% less usable headroom before Hermes force-compacts the conversation. On long agentic sessions (e.g. heavyread_file/grep/web_searchtool output) this causes materially more frequent compaction than intended.Fix
Add
anthropic.claude-opus-5,anthropic.claude-sonnet-5,anthropic.claude-haiku-5→ 200,000 toBEDROCK_CONTEXT_LENGTHS. Minimal, additive, no behavior change for existing entries.Verification
Also confirmed downstream in
ContextCompressor: with this fix (or an equivalentmodel.context_length: 200000config override),threshold_tokensfor this model correctly resolves to 150,000 instead of 96,000.