fix(guardrails): re-emit chunks in tool_permission streaming hook when no tool_calls found - #26585
Conversation
Greptile SummaryFixes a silent response-drop bug in
Confidence Score: 5/5Safe to merge — a minimal, well-scoped fix that restores plain-text streaming responses in the tool_permission guardrail without touching any other code paths. The change is exactly five lines in the production file, directly mirrors the already-tested pattern two dozen lines below it in the same method, and is covered by a new regression test that checks both chunk presence and content fidelity. The else fallback for non-ModelResponse types and the None return from stream_chunk_builder are both handled by the existing outer branch structure, so no new edge cases are introduced. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/guardrails/guardrail_hooks/tool_permission.py | Adds 5 lines to re-emit assembled chunks via MockResponseIterator in the no-tool-calls branch, fixing silent response drops; mirrors the identical pattern already used in the allowed-tools path |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.py | Adds regression test verifying that the hook yields at least one chunk AND that the chunk content matches the assembled response for plain-text (no-tool-call) responses; uses mocks only, no real network calls |
Reviews (5): Last reviewed commit: "test(guardrails): strengthen plain-text ..." | Re-trigger Greptile
| assert len(chunks) >= 1, ( | ||
| "Hook must yield at least one chunk for plain-text responses; " | ||
| "got none — bare return bug" | ||
| ) |
There was a problem hiding this comment.
len(chunks) >= 1 only proves that something was yielded; it doesn't verify that the yielded chunk carries the expected content ("Hello, world!"). A stronger assertion would also confirm that the content attribute in at least one chunk matches the assembled response, making the test a true guard against silent content corruption in addition to the dropped-stream bug.
| assert len(chunks) >= 1, ( | |
| "Hook must yield at least one chunk for plain-text responses; " | |
| "got none — bare return bug" | |
| ) | |
| assert len(chunks) >= 1, ( | |
| "Hook must yield at least one chunk for plain-text responses; " | |
| "got none — bare return bug" | |
| ) | |
| # Verify the content of the yielded chunks matches the assembled response. | |
| content = "".join( | |
| getattr(c.choices[0].delta, "content", "") or "" | |
| for c in chunks | |
| if c.choices | |
| ) | |
| assert "Hello, world!" in content, ( | |
| f"Expected plain-text content to be re-emitted; got: {content!r}" | |
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
005f143 to
e533f6a
Compare
1 similar comment
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 0/5 ❌ Why blocked:
Details: Score docked for: merge conflicts (rebase against base branch); 1 PR-related CI failure (This PR will be auto-closed as it lacks a screenshot for proof of fix. Please include one in the PR description. Add the Fix the issues above and push an update — the bot will re-review automatically.
|
…n no tool_calls found async_post_call_streaming_iterator_hook is an async generator. The `if not tool_calls:` branch (plain-text LLM replies) did a bare `return`, which terminates the generator without yielding anything. Clients received only `data: [DONE]` with empty content — the entire response was silently dropped. Fix: pass the assembled ModelResponse through MockResponseIterator and yield every chunk before returning, mirroring the allowed-tool code path that already exists a few lines below. Closes BerriAI#26547 Re-submits after BerriAI#26551 (auto-closed when litellm_oss_branch was deleted)
… content fidelity Previously the regression test only checked that at least one chunk was yielded; now it also asserts that the chunk content matches the original assembled response, ensuring the fix preserves response data end-to-end.
e533f6a to
cefd032
Compare
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 3/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (This PR will be auto-closed as it lacks a screenshot for proof of fix. Please include one in the PR description. Add the Fix the issues above and push an update — the bot will re-review automatically.
|
Relevant issues
Fixes #26547
Re-submission of #26551 (auto-closed when
litellm_oss_branchwas deleted)Pre-Submission checklist
tests/test_litellm/—test_async_post_call_streaming_iterator_hook_plain_text_yields_chunksmake test-unitpasses locally; all CI checks pass on this PR@greptileaireview requested — Confidence Score: 5/5 — "Safe to merge"Type
🐛 Bug Fix
Root cause
ToolPermissionGuardrail.async_post_call_streaming_iterator_hookis an async generator (it containsyieldstatements). In theif not tool_calls:branch — the path taken when the LLM replies with plain text — the original code did a barereturn.In an async generator,
returnis equivalent toraise StopAsyncIteration. Nothing is yielded. The client receives onlydata: [DONE]with empty content. The entire plain-text response is silently dropped whenever thetool_permissionguardrail is active withmode: post_callon a streaming request.User-visible impact: Any chat client using LiteLLM proxy with
tool_permissionguardrail enabled gets a blank reply whenever the LLM decides not to call a tool (a very common case for conversational queries).Fix
Before returning, re-emit the assembled response through
MockResponseIterator— the same pattern already used in the allowed-tool path a few lines below in the same function:Five lines, scoped to the exact location of the bug. Mirrors an existing, well-tested pattern in the same method, so risk of behavioural surprise is minimal.
Screenshots / Proof of Fix
Reproduced against a local LiteLLM proxy (Docker) with the
tool_permissionguardrail configured inmode: post_call. The same byte-identical streaming request was sent in both runs — the only variable is whether the 5-line fix is present intool_permission.py.Reproduction command:
1. Wire-level — Before fix (BUG)
Client receives only
data: [DONE]. No content chunk is ever emitted, so the user sees a blank response.2. Wire-level — After fix (FIX)
Same request, fix applied. A
chat.completion.chunkcarryingdelta.content: "FOUR"is now delivered before[DONE].3. Server-side control (LiteLLM Logs, from the BUG run)
Even in the bug case, the LLM returned content (
ASSISTANT: "FOUR.") and both guardrails passed — the proxy logged the request as Success. This pinpoints the drop: it happens strictly insideasync_post_call_streaming_iterator_hook'sif not tool_calls:branch, between guardrail evaluation and the SSE response written to the client. That is exactly where this PR's 5 lines live.Changes
litellm/proxy/guardrails/guardrail_hooks/tool_permission.pytests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.pytest_async_post_call_streaming_iterator_hook_plain_text_yields_chunks) asserts both that ≥1 chunk is yielded and thatchunk.delta.contentmatches the assembled response, guarding against silent drops and content-mangling regressionsPR raised by Someswar at Incubyte