test(bedrock): add regression test for cachePoint as separate block#1570
test(bedrock): add regression test for cachePoint as separate block#1570minorun365 wants to merge 1 commit intostrands-agents:mainfrom
Conversation
src/strands/models/bedrock.py
Outdated
| # See: https://github.com/strands-agents/sdk-python/issues/1219 | ||
| if "cachePoint" in content_block: | ||
| if cleaned_content: | ||
| cleaned_content[-1]["cachePoint"] = {"type": content_block["cachePoint"]["type"]} |
There was a problem hiding this comment.
Hey thanks for raising this but it seems like this would still fail if we have
content = [
{"document": {"format": "pdf", ...}},
{"document": {"format": "pdf", ...}},
{"cachePoint": {"type": "default"}} # ← standalone, causes ValidationException
]
transformed into
content = [
{"document": {"format": "pdf", ...}},
{"document": {"format": "pdf", ...}, "cachePoint": {"type": "default"}} # ← merged
]
no?
If not, great but lets add/modify integ tests to prove that.
There was a problem hiding this comment.
Thanks for the review! You were absolutely right - my original approach was incorrect.
After investigating further, I discovered:
-
The merging approach violates Bedrock's tagged union structure - content blocks can only have one key (
text,document,cachePoint, etc.), so mergingcachePointinto a document block like{"document": {...}, "cachePoint": {...}}causesParamValidationError -
Issue [BUG] BedrockModel doesn't handle cachePoint content blocks in messages, causes ValidationException #1219 was already fixed by PR feat(bedrock): add automatic prompt caching support #1438 - The
_format_request_message_contentmethod already handlescachePointblocks correctly by returning{"cachePoint": {"type": "..."}}as a standalone block (line 478-479)
I've updated this PR to:
- Remove my incorrect merging logic
- Add a regression test to verify
cachePointblocks are correctly formatted as separate blocks
The test confirms that:
content = [
{"text": "Some text"},
{"cachePoint": {"type": "default"}}
]is correctly preserved as two separate blocks, not merged.
Thanks for catching this! 🙏
Add test to verify cachePoint blocks are formatted as standalone blocks and not merged into previous content blocks. This confirms the fix from PR strands-agents#1438 works correctly for the scenario reported in Issue strands-agents#1219. Closes strands-agents#1219
e1930f7 to
45c2488
Compare
Summary
Add regression test to verify that
cachePointcontent blocks are correctly formatted as standalone blocks (not merged into previous content blocks).This test confirms that the fix from PR #1438 works correctly for the scenario reported in Issue #1219, where
cachePointblocks followingtextblocks were causingValidationExceptionerrors.What this PR does:
test_format_request_cachepoint_after_text_separate_blocksthat verifies:cachePointblocks remain as separate content blocksBackground
Issue #1219 reported that
cachePointblocks after text/document content causedValidationException. This was already fixed by PR #1438 (which added propercachePointhandling in_format_request_message_content).This PR adds a regression test to ensure the fix continues to work.
Test plan
test_format_request_cachepoint_after_text_separate_blockshatch test tests/strands/models/test_bedrock.py::test_format_request_cachepoint_after_text_separate_blocksCloses #1219
🤖 Generated with Claude Code