bedrock api response null type handling#24147
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge — the fix is correct, well-documented, and comprehensively tested with mock unit tests. The null-safety fix is technically sound (Python dict.get() semantics are correctly handled), all three affected methods are patched consistently, and the new test classes cover every null-field combination. The previously flagged missing ANONYMIZED/BLOCKED test has been re-added to the unit test suite. No P0 or P1 issues remain. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/guardrails/guardrail_hooks/bedrock_guardrails.py | Applies or [] null-safety fix across _redact_pii_matches, _should_raise_guardrail_blocked_exception, and apply_guardrail; fix is correct and well-documented with inline comments explaining Python dict.get() semantics |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.py | New comprehensive mock unit tests covering all null-safety scenarios; HTTPException import at top level is correctly used at line 1179; test_bedrock_guardrail_blocked_vs_anonymized_actions re-added here |
| tests/guardrails_tests/test_bedrock_guardrails.py | Integration test file; some tests reorganized to the unit test suite; existing real-network-call tests remain in their appropriate integration test location |
Sequence Diagram
sequenceDiagram
participant C as Client
participant BG as BedrockGuardrail
participant BA as Bedrock API
C->>BG: apply_guardrail(inputs)
note over BG: texts = inputs.get("texts") or []
alt texts is empty (None → [])
BG-->>C: return inputs unchanged (no API call)
else texts present
BG->>BA: POST /guardrail/{id}/version/{v}/apply
BA-->>BG: response (may contain null list fields)
note over BG: assessments = response.get("assessments") or []
loop for each assessment
note over BG: piiEntities/regexes/topics/filters/customWords/managedWordLists\n= .get(key) or [] ← null-safe
end
alt any field action == BLOCKED
BG-->>C: raise HTTPException(400)
else ANONYMIZED or no match
note over BG: _redact_pii_matches(response)\nsame or [] pattern — replace match → [REDACTED]
BG-->>C: return masked inputs
end
end
Reviews (3): Last reviewed commit: "added changes based on the feedback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @pytest.mark.asyncio | ||
| async def test_bedrock_guardrails_block_responses_api(): | ||
| """ | ||
| Test that guardrails block responses API requests containing 'coffee' and raise the expected exception. | ||
| """ | ||
| from fastapi import HTTPException | ||
| async def test__redact_pii_matches_malformed_response(): | ||
| """Test _redact_pii_matches with malformed response (should not crash)""" | ||
|
|
||
| # Create proper mock objects | ||
| mock_user_api_key_dict = UserAPIKeyAuth() | ||
| # Test with completely malformed response | ||
| malformed_response = { | ||
| "action": "GUARDRAIL_INTERVENED", | ||
| "assessments": "not_a_list", # This should cause an exception | ||
| } | ||
|
|
||
| guardrail = BedrockGuardrail( | ||
| guardrailIdentifier="ff6ujrregl1q", | ||
| guardrailVersion="DRAFT", | ||
| ) | ||
| # Should not crash and return original response | ||
| redacted_response = _redact_pii_matches(malformed_response) | ||
| assert redacted_response == malformed_response | ||
|
|
||
| request_data = { | ||
| "model": "gpt-4.1", | ||
| "input": "Tell me a three sentence bedtime story about a unicorn drinking coffee", | ||
| "stream": False, | ||
| # Test with missing keys | ||
| missing_keys_response = { | ||
| "action": "GUARDRAIL_INTERVENED" | ||
| # Missing assessments key | ||
| } | ||
|
|
||
| with pytest.raises(HTTPException) as exc_info: | ||
| await guardrail.async_pre_call_hook( | ||
| data=request_data, | ||
| user_api_key_dict=mock_user_api_key_dict, | ||
| call_type="responses", | ||
| cache=MagicMock(spec=DualCache), | ||
| ) | ||
| redacted_response = _redact_pii_matches(missing_keys_response) | ||
| assert redacted_response == missing_keys_response | ||
|
|
||
| exception = exc_info.value | ||
| assert exception.status_code == 400 | ||
| detail = exception.detail | ||
| assert isinstance(detail, dict) | ||
| assert detail["error"] == "Violated guardrail policy" | ||
| assert ( | ||
| detail["bedrock_guardrail_response"] | ||
| == "Sorry, the model cannot answer this question. coffee guardrail applied " | ||
| ) | ||
| print("Malformed response redaction test passed") |
There was a problem hiding this comment.
The core fix in this PR handles Bedrock returning explicit null for list fields (e.g. "piiEntities": null, "regexes": null, "assessments": null). However, none of the new tests exercise this exact code path. test__redact_pii_matches_malformed_response tests "not_a_list" (a truthy string that goes through the exception handler), but it does not test None (the actual Bedrock null scenario that the or [] pattern is intended to fix).
A specific test like the following would directly validate the fix:
@pytest.mark.asyncio
async def test__redact_pii_matches_null_list_fields():
"""Test that explicit null values from Bedrock API are handled correctly"""
response_with_null_fields = {
"action": "GUARDRAIL_INTERVENED",
"assessments": [
{
"sensitiveInformationPolicy": {
"piiEntities": None, # Bedrock can return explicit null
"regexes": None, # same
}
}
],
}
# Should not raise TypeError: 'NoneType' object is not iterable
redacted_response = _redact_pii_matches(response_with_null_fields)
assert redacted_response is not NoneWithout this, the specific regression that motivated this PR is not validated by the test suite.
| import pytest | ||
| from fastapi import HTTPException |
There was a problem hiding this comment.
HTTPException is imported at the module level but is not used by any of the new test functions. The tests that previously referenced it (test_bedrock_guardrails_block_messages_api, test_bedrock_guardrails_block_responses_api) were removed.
| from fastapi import HTTPException |
(Remove the line entirely.)
| """ | ||
| Unit tests for Bedrock Guardrails | ||
| """ | ||
| import json | ||
| import os | ||
| import io, asyncio | ||
| import sys | ||
| from unittest.mock import AsyncMock, MagicMock, patch | ||
|
|
||
| import pytest | ||
| from fastapi import HTTPException | ||
|
|
||
| sys.path.insert(0, os.path.abspath("../../../../../..")) | ||
|
|
||
| sys.path.insert(0, os.path.abspath("../..")) | ||
| import litellm | ||
| from litellm.proxy.guardrails.guardrail_hooks.bedrock_guardrails import BedrockGuardrail | ||
| from litellm.proxy._types import UserAPIKeyAuth | ||
| from litellm.caching import DualCache | ||
| from unittest.mock import MagicMock, AsyncMock, patch | ||
| from litellm.proxy.guardrails.guardrail_hooks.bedrock_guardrails import ( | ||
| BedrockGuardrail, | ||
| _redact_pii_matches, | ||
| ) |
There was a problem hiding this comment.
_should_raise_guardrail_blocked_exception
This PR also patches _should_raise_guardrail_blocked_exception with the same null-handling fix (for topics, filters, customWords, managedWordLists, piiEntities, regexes, grounding_filters), but the old test test_bedrock_guardrail_blocked_vs_anonymized_actions that covered this method was removed and not replaced.
The deleted test validated three distinct cases that are still important:
ANONYMIZEDactions should NOT raise an exception (returnsFalse)BLOCKEDactions SHOULD raise an exception (returnsTrue)- Mixed ANONYMIZED + BLOCKED should raise if ANY action is
BLOCKED
These scenarios are critical to the guardrail's correctness and are now untested. Per the mock test integrity rule, a mock test should only be removed when it no longer reflects the intended behavior — these cases still represent correct expected behavior.
Rule Used: # Code Review Rule: Mock Test Integrity
What:... (source)
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
kothamah
force-pushed
the
litellm_bedrock_api_response_null_type_handling
branch
from
f775c79 to
168b0a0
Compare
|
@copilot resolve the merge conflicts in this pull request |
ishaan-berri
merged commit aaf169c
into
BerriAI:litellm_ishaan_april15
3 participants
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes