Repository navigation
Conversation
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
|
Merging this PR will not alter performance1 warning
Comparing Footnotes |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
31c03bc to
a41fc2b
Compare
| nonlocal calls | ||
| calls += 1 | ||
| messages = json.loads(request.content)["guard_input"]["messages"] | ||
| scanned.append("".join(message["content"] for message in messages)) |
There was a problem hiding this comment.
Mutable scan collection The updated test appends to
scanned, violating the repository requirement to avoid mutation in updated code. Please resolve this before merging
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!
VANDRANKI
left a comment
There was a problem hiding this comment.
Community review, does not clear the merge gate.
I read the diff, the PR description, and the full parametrized test to check both the behavior-default change and the code pattern used to implement it.
The PR flips the CrowdStrike AIDR guardrail's default for streaming_end_of_stream_only from False to True, per the description because this guardrail was "always intended to work with the complete response," and mid-stream chunked scanning produces multiple incomplete findings on CrowdStrike's own platform that look like false regressions to customers. I flag the tradeoff up front since it's real and the updated docstring says so honestly: with the new default, a flagged/blocked response may already have reached the client before the guard runs, versus the old per-chunk mode which could cut a stream short mid-flight. That's a deliberate, documented tradeoff for this specific vendor integration, not a silent weakening.
The implementation detail I specifically checked: the old code used streaming_params.streaming_end_of_stream_only or False. That pattern only works when the default is falsy. Simply changing the fallback to or True would have been a real bug, since False or True evaluates to True, silently overriding a user's explicit streaming_end_of_stream_only=False back to True. The PR instead uses True if streaming_params.streaming_end_of_stream_only is None else streaming_params.streaming_end_of_stream_only, which correctly distinguishes "unset" (defaults to the new True) from "explicitly False" (stays False).
I traced the new parametrized test against this: {"streaming_end_of_stream_only": False} expects two scans ("ABCDE", "ABCDEFGHIJ", at the default streaming_sampling_rate=5), confirming explicit False still activates the old chunked-scan behavior rather than being coerced to True. {} (unset) expects exactly one scan of the full assembled text, confirming the new default. The two streaming_end_of_stream_only=True cases (with and without a sampling rate) both correctly ignore the sampling rate and scan once. Every case also asserts scanned[-1] == "ABCDEFGHIJ", i.e. the final scan always sees the complete accumulated output regardless of mode.
This is a narrowly scoped default change (only this guardrail's own _set_streaming_params, not a shared base-class default affecting other guardrail integrations), correctly avoids the truthy-default or footgun, and the tests exercise the actual edge case that pattern would have gotten wrong. Looks correct to me.
981452e to
5990653
Compare
5990653 to
5b457f3
Compare
5b457f3 to
0b7b79b
Compare
…stream The CrowdStrike AIDR guardrail has always been intended to work with the complete response, not with streamed chunks. The introduction of `streaming_end_of_stream_only` and the like have caused the guardrail to begin receiving chunked content, which then gets reflected in the CrowdStrike AIDR platform as multiple incomplete findings/events, which looks like a regression to customers. This patch updates the `streaming_end_of_stream_only` default to `True` for this guardrail.
0b7b79b to
d045668
Compare
TLDR
The CrowdStrike AIDR guardrail has always been intended to work with the
complete response, not with streamed chunks. The introduction of
streaming_end_of_stream_onlyand the like have caused the guardrail to beginreceiving chunked content, which then gets reflected in the CrowdStrike AIDR
platform as multiple incomplete findings/events, which looks like a regression
to customers. This patch updates the
streaming_end_of_stream_onlydefault toTruefor this guardrail.Affected release
Regressed at some point after v1.84.6?
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Type
🐛 Bug Fix
Final Attestation