fix(bedrock): guard against null url in image_url parts and lifecycle status (#55686) - #55996
fix(bedrock): guard against null url in image_url parts and lifecycle status (#55686)#55996AlexFucuson9 wants to merge 1 commit into
Conversation
… status
Two .get(key, "") patterns crash when the key exists with value None
(the default only applies when the key is ABSENT, not when it is None):
1. image_url.get("url", "") → None when url is null →
url.startswith("data:") raises AttributeError
2. lifecycle.get("status", "").upper() → None when status is null →
.upper() raises AttributeError
Fix: use (x.get("key") or "") which coalesces both absent-key and
None-value to empty string.
Fixes NousResearch#55686
Related: this fixes the same |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Bedrock hardening. The stated null cases remain reproducible from current main: agent/bedrock_adapter.py:522-523 can call startswith on a null URL, and agent/bedrock_adapter.py:1147-1148 can call upper on a null lifecycle status.
Problems
- The PR changes only
agent/bedrock_adapter.py; it adds no regression coverage. Existing tests cover a valid image data URL attests/agent/test_bedrock_adapter.py:332-348and ACTIVE/LEGACY lifecycle states at:711-773, not either null case. - The proposed
or ""expressions fixNone, but truthy non-string values can still reachstartswithorupper. The analogous Gemini converter uses an explicit string guard atagent/gemini_native_adapter.py:213-215.
Suggested changes
- Add direct null-URL and null-lifecycle-status regression tests in
tests/agent/test_bedrock_adapter.py. - Use explicit string normalization for both values before their string-method calls.
Automated hermes-sweeper review.
| elif part_type == "image_url": | ||
| image_url = part.get("image_url", {}) | ||
| url = image_url.get("url", "") if isinstance(image_url, dict) else "" | ||
| url = (image_url.get("url") or "") if isinstance(image_url, dict) else "" |
There was a problem hiding this comment.
This fixes None, but a truthy non-string value such as 123 still reaches url.startswith(...) below. Please normalize with an explicit isinstance(url, str) guard, matching the defensive Gemini converter.
| # Only include active, streaming-capable, text-output models | ||
| lifecycle = summary.get("modelLifecycle", {}) | ||
| if lifecycle.get("status", "").upper() != "ACTIVE": | ||
| if (lifecycle.get("status") or "").upper() != "ACTIVE": |
There was a problem hiding this comment.
This coalesces null status values, but truthy non-strings still reach .upper(). Normalize to a string before comparison so malformed lifecycle payloads cannot retain the same crash class.
Summary
Prevent
AttributeErrorcrash when Bedrock Converse encounters animage_urlpart with a nullurlfield, or a model lifecycle entry with a nullstatus.Problem
Two
.get(key, "")patterns crash when the key exists with valueNone:image_url.get("url", "")returnsNone(not"") whenurlis explicitly null →url.startswith("data:")raisesAttributeError: 'NoneType' object has no attribute 'startswith'lifecycle.get("status", "").upper()returnsNonewhenstatusis null →.upper()raisesAttributeErrorThe
dict.get(key, default)default only applies when the key is absent, not when it is present with valueNone.Fix
Use
(x.get("key") or "")which coalesces both absent-key and None-value to empty string:image_url.get("url") or ""— null URL parts are safely skipped (empty string doesn't matchdata:prefix)lifecycle.get("status") or ""— null status is treated as non-ACTIVE (model skipped)Fixes #55686