feat(guardrails): optional skip system message in unified guardrail inputs#25481
feat(guardrails): optional skip system message in unified guardrail inputs#25481Sameerlite wants to merge 1 commit intomainfrom
Conversation
…nputs Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR adds an optional flag to exclude Confidence Score: 5/5This PR is safe to merge; all remaining findings are P2 style suggestions that do not affect correctness or runtime behavior. The implementation is logically sound: filtering is applied consistently to both texts and structured_messages, the three-state per-guardrail/global precedence model is correct, and the original request payload to the LLM is never mutated. The only findings are a missing inline-import comment and an absent Anthropic handler test — neither blocks merge. litellm/llms/base_llm/guardrail_translation/utils.py — inline import should carry a circular-import comment; tests/ — Anthropic handler path lacks a skip_system test.
|
| Filename | Overview |
|---|---|
| litellm/llms/base_llm/guardrail_translation/utils.py | New utility file with resolution helper and filter function; contains an inline import litellm inside the function body that should be documented as a circular-import workaround. |
| litellm/llms/openai/chat/guardrail_translation/handler.py | Correctly applies skip_system filtering to both texts_to_check (via _extract_inputs) and structured_messages (via openai_messages_without_system); logic is sound and consistent. |
| litellm/llms/anthropic/chat/guardrail_translation/handler.py | Correctly filters structured_messages (OpenAI-converted) and applies guard in _extract_input_text_and_images; Anthropic messages don't carry system in the messages array so the texts filtering guard is safe but effectively a no-op in normal Anthropic usage. |
| litellm/proxy/guardrails/guardrail_registry.py | Uses setattr to stamp the per-guardrail flag onto the callback after initialization; design is correct since effective_skip_system_message_for_guardrail reads at request time. |
| litellm/types/guardrails.py | Adds skip_system_message_in_guardrail: Optional[bool] = None to BaseLitellmParams with clear three-state semantics (True/False/None=inherit); well-documented. |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/unified_guardrails/test_unified_guardrail.py | Good unit tests for OpenAI handler and helper functions; no Anthropic-specific test for the new skip_system feature despite Anthropic support being claimed in the PR description. |
| litellm/init.py | Adds global skip_system_message_in_guardrail: bool = False; correctly placed near other guardrail-related settings. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request arrives at handler
process_input_messages] --> B[effective_skip_system_message_for_guardrail]
B --> C{per-guardrail attr
skip_system_message_in_guardrail}
C -- not None --> D[return bool per-guardrail value]
C -- None --> E[read litellm.skip_system_message_in_guardrail global]
E --> F[return global bool]
D --> G{skip_system = True?}
F --> G
G -- Yes --> H[_extract_inputs skips system msgs
from texts_to_check]
G -- No --> I[_extract_inputs includes all msgs
in texts_to_check]
H --> J[structured_messages = openai_messages_without_system]
I --> K[structured_messages = messages unchanged]
J --> L{texts_to_check or
tool_calls_to_check?}
K --> L
L -- Yes --> M[apply_guardrail with filtered inputs]
L -- No --> N[return data unchanged
no guardrail call]
M --> O[Map responses back to
original messages]
Reviews (1): Last reviewed commit: "feat(guardrails): optional skip system m..." | Re-trigger Greptile
| per = getattr(guardrail_to_apply, "skip_system_message_in_guardrail", None) | ||
| if per is not None: | ||
| return bool(per) | ||
| import litellm |
There was a problem hiding this comment.
Inline import inside function body
Per the project style guide, module-level imports are required; the only accepted exception is avoiding circular imports. The import litellm here is almost certainly justified (this submodule is transitively imported by litellm/__init__.py), but the intent isn't obvious to future readers. A brief comment would clarify this:
| import litellm | |
| import litellm # deferred to avoid circular import (this module is loaded during litellm package init) |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @@ -68,6 +76,109 @@ def _inject_mcp_handler_mapping(): | |||
|
|
|||
There was a problem hiding this comment.
No Anthropic handler test for skip_system feature
The PR description explicitly states the feature applies to "OpenAI Chat Completions and Anthropic Messages," but TestSkipSystemMessageForChatCompletions only exercises OpenAIChatCompletionsHandler. The Anthropic path (AnthropicMessagesHandler) differs meaningfully — it iterates the raw Anthropic messages array for texts_to_check and filters structured_messages from the OpenAI-converted request separately. A parallel test using AnthropicMessagesHandler (or at minimum AnthropicMessagesHandler._extract_input_text_and_images) would ensure that path stays covered as both handlers evolve.
Summary
Adds optional exclusion of
role: systemmessages from unified guardrail evaluation inputs (flattenedtextsandstructured_messages) on OpenAI Chat Completions and Anthropic Messages, while leaving the request payload to the LLM unchanged.Fixes LIT-2382
Configuration
litellm_settings.skip_system_message_in_guardrail(also setslitellm.skip_system_message_in_guardrail).litellm_params.skip_system_message_in_guardrail(true/false/ omit to inherit). Registry sets the attribute on the callback after init.Implementation
litellm/llms/base_llm/guardrail_translation/utils.py— resolution helper and OpenAI-shaped message filter.litellm_settingstable in config reference.