fix(guardrails): expand os.environ/ references in every litellm_params field - #34570
fix(guardrails): expand os.environ/ references in every litellm_params field#34570mubashir1osmani wants to merge 1 commit into
Conversation
…s field initialize_guardrail only expanded os.environ/ for api_key and api_base, so every other field kept the literal reference string. A guardrail registered with aws_region_name="os.environ/AWS_REGION" handed boto3 the string "os.environ/AWS_REGION", which failed the request with "Invalid AWS region format: 'os.environ/AWS_REGION'. Region names must contain only lowercase letters, digits, and hyphens." The same silently broke aws_access_key_id, aws_secret_access_key, the presidio analyzer/anonymizer bases, and the rest of the credential fields, none of which are reachable through api_key/api_base. Resolve every string field that starts with os.environ/ instead of two named ones. A reference that resolves to nothing now becomes None rather than the string "None", so the downstream client falls back to its own credential chain (pod identity, for example) instead of authenticating with a literal "None". Reproduced against the live stage proxy before the fix: registering a bedrock guardrail with the three os.environ/ AWS params returned 500 with the invalid region message, while the same guardrail without them blocked correctly.
Greptile SummaryThis PR generalizes environment-reference resolution for guardrail parameters.
Confidence Score: 3/5This PR should not merge until environment references are resolved before typed guardrail parameters are validated. Numeric guardrail fields still receive the literal Files Needing Attention: litellm/proxy/guardrails/guardrail_registry.py, tests/test_litellm/proxy/guardrails/test_guardrail_registry.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/guardrails/guardrail_registry.py | Generalizes secret resolution, but performs it after model validation, leaving typed numeric parameters unable to use environment references. |
| tests/test_litellm/proxy/guardrails/test_guardrail_registry.py | Covers optional and literal string fields but does not exercise environment references for typed fields such as timeout or Bedrock thresholds. |
Reviews (1): Last reviewed commit: "fix(guardrails): expand os.environ/ refe..." | Re-trigger Greptile
| for name, value in litellm_params.model_dump().items() | ||
| if isinstance(value, str) and value.startswith("os.environ/") | ||
| } |
There was a problem hiding this comment.
Typed environment references fail
When a guardrail uses an environment reference for a typed parameter such as timeout or content_filter_threshold, LitellmParams validates the literal os.environ/... string before this resolver runs, causing guardrail initialization to fail even when the environment value is valid.
Knowledge Base Used: Guardrails
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TLDR
Problem this solves:
initialize_guardrailonly expandedos.environ/forapi_keyandapi_base, so every other guardrail field kept the literal reference string. A guardrail registered withaws_region_name="os.environ/AWS_REGION"handed boto3 the string"os.environ/AWS_REGION"and failed the requestaws_access_key_id,aws_secret_access_key, the presidio analyzer/anonymizer bases, and the rest of the credential fields, none of which are reachable throughapi_key/api_baseHow it solves it:
os.environ/instead of two named onesNonerather than the string"None", so the downstream client falls back to its own credential chain (pod identity, for example) instead of authenticating with a literal"None"Relevant issues
Linear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
Reproduced against the live stage proxy before the fix. Registering a bedrock guardrail with the three
os.environ/AWS params and sending a chat request returned:The same guardrail registered without those params blocked correctly with
400 "Violated guardrail policy", confirming the literal string was the only difference.LitellmParamshas 20+str | Nonefields that were all affected, includingaws_region_name,aws_access_key_id,aws_secret_access_key,aws_session_token,aws_role_name,presidio_analyzer_api_base,presidio_anonymizer_api_base,auth_token, andcredentials.Type
🐛 Bug Fix
Changes
Adds
_resolve_env_references()and calls it in place of the two hardcoded field checks. The helper builds the resolved values with a comprehension and returnsmodel_copy(update=...), so nothing is mutated in place.QA runbook
The new test is a real regression test, not coverage padding. Reverting the helper to the old two-field behavior makes it fail on the exact symptom:
It also pins the
None-not-"None"behavior for an unset reference, and asserts literal values and unrelated fields survive the copy.Final Attestation