fix: content-filter responses raise instead of normalizing - #358
Open
andrewwhitecdw wants to merge 1 commit into
Open
Conversation
## Summary
`_response()` raised `ValueError("LiteLLM returned no text content")` for any
response with empty `content`, including content-filter responses where empty
output is expected. This caused legitimate content-filter results to fail
instead of being normalized.
## Root cause
The guard at the end of `_response()` did not distinguish between a truly
empty/invalid response and a `content_filter` finish reason, which by design
returns no content.
## Fix
Allow empty `content` when the finish reason is `content_filter`; keep the
existing raise for all other empty-content cases so unexpected responses still
fail fast.
```diff
- if not content:
- raise ValueError("LiteLLM returned no text content")
+ if not content and choice.finish_reason != "content_filter":
+ raise ValueError("LiteLLM returned no text content")
```
## Testing
Added `test_response_content_filter_empty_content` to
`examples/experimental/litellm/tests/test_client.py` and verified the full
non-e2e suite passes:
```
uv run --project examples/experimental/litellm --python 3.12 \
pytest examples/experimental/litellm/tests/test_client.py -m "not e2e" -v
# 34 passed
```
## Contributor guidelines
- DCO sign-off included.
- One focused commit per PR.
Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
WalkthroughThe LiteLLM response parser now accepts empty content when the finish reason is ChangesLiteLLM content filter handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/experimental/litellm/tests/test_client.py (1)
557-576: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the preserved non-filter error path.
This test covers the allowed
content_filterpath. It does not verify that empty content with anotherfinish_reasonstill raisesValueError("LiteLLM returned no text content"). Add a companion test forfinish_reason="stop"withcontent=None.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/experimental/litellm/tests/test_client.py` around lines 557 - 576, Add a companion test alongside test_response_content_filter_empty_content using finish_reason="stop" and content=None, then assert that _response raises ValueError with the exact message "LiteLLM returned no text content". Keep the existing content_filter test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/experimental/litellm/tests/test_client.py`:
- Around line 563-576: Update the _response regression test fixture to construct
a typed ModelResponse instead of SimpleNamespace, preserving the content_filter
response values. Narrow or validate the returned outputs collection before
indexing it so the dict[str, object] result passes strict mypy checks.
---
Nitpick comments:
In `@examples/experimental/litellm/tests/test_client.py`:
- Around line 557-576: Add a companion test alongside
test_response_content_filter_empty_content using finish_reason="stop" and
content=None, then assert that _response raises ValueError with the exact
message "LiteLLM returned no text content". Keep the existing content_filter
test unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee5a76f1-690f-44cf-81a4-ce4df4182125
📒 Files selected for processing (2)
examples/experimental/litellm/src/switchyard_litellm/client.pyexamples/experimental/litellm/tests/test_client.py
Comment on lines
+563
to
+576
| response = SimpleNamespace( | ||
| id="chatcmpl-test", | ||
| model="openai/strong", | ||
| choices=[ | ||
| SimpleNamespace( | ||
| message=SimpleNamespace(content=None, tool_calls=None), | ||
| finish_reason="content_filter", | ||
| ) | ||
| ], | ||
| usage=None, | ||
| ) | ||
| result = _response(response) | ||
| assert result["outputs"][0]["content"] == [] | ||
| assert result["outputs"][0]["stop_reason"] == "content_filter" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
uv run mypy --strict \
examples/experimental/litellm/src/switchyard_litellm/client.py \
examples/experimental/litellm/tests/test_client.pyRepository: NVIDIA-NeMo/Switchyard
Length of output: 200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target test ---'
sed -n '1,120p' examples/experimental/litellm/tests/test_client.py
sed -n '520,610p' examples/experimental/litellm/tests/test_client.py
printf '%s\n' '--- client response definitions and call sites ---'
rg -n -C 5 'def _response|ModelResponse|content_filter|stop_reason|outputs' \
examples/experimental/litellm/src examples/experimental/litellm/tests
printf '%s\n' '--- project typing configuration ---'
fd -HI 'pyproject.toml|mypy.ini|setup.cfg|tox.ini' . -x sh -c 'echo "--- $1"; sed -n "1,240p" "$1"' sh {}Repository: NVIDIA-NeMo/Switchyard
Length of output: 34012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- available type-checking tools ---'
command -v mypy || true
command -v pyright || true
python3 - <<'PY'
import importlib.util
for name in ("mypy", "pyright", "litellm"):
print(f"{name}: {importlib.util.find_spec(name)}")
PY
printf '%s\n' '--- LiteLLM dependency and lock references ---'
rg -n -C 3 'litellm|1\.92\.0|ModelResponse' \
examples/experimental/litellm uv.lock requirements*.txt 2>/dev/null || true
printf '%s\n' '--- test module declarations and type-check exclusions ---'
rg -n -C 4 'mypy|strict|ignore_errors|test_client|examples/experimental/litellm' \
pyproject.toml examples/experimental/litellmRepository: NVIDIA-NeMo/Switchyard
Length of output: 48555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
client_path = Path("examples/experimental/litellm/src/switchyard_litellm/client.py")
test_path = Path("examples/experimental/litellm/tests/test_client.py")
client = ast.parse(client_path.read_text())
test = ast.parse(test_path.read_text())
def find_function(tree: ast.AST, name: str) -> ast.FunctionDef:
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name:
return node
raise AssertionError(f"missing function: {name}")
response_fn = find_function(client, "_response")
arg = response_fn.args.args[0]
print(f"_response parameter annotation: {ast.unparse(arg.annotation)}")
print(f"_response return annotation: {ast.unparse(response_fn.returns)}")
test_fn = find_function(test, "test_response_content_filter_empty_content")
for node in ast.walk(test_fn):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Name):
if node.func.id in {"SimpleNamespace", "_response"}:
print(f"{node.func.id} call at line {node.lineno}: {ast.unparse(node)}")
if isinstance(node, ast.Subscript):
print(f"subscript at line {node.lineno}: {ast.unparse(node)}")
config = ast.parse(Path("pyproject.toml").read_text()) if False else None
print("SimpleNamespace fixture present: " + str(
any(
isinstance(node, ast.Call)
and isinstance(node.func, ast.Name)
and node.func.id == "SimpleNamespace"
for node in ast.walk(test_fn)
)
))
print("result output is indexed twice: " + str(sum(
isinstance(node, ast.Subscript)
and isinstance(node.value, ast.Subscript)
and isinstance(node.value.value, ast.Name)
and node.value.value.id == "result"
for node in ast.walk(test_fn)
) == 2))
PYRepository: NVIDIA-NeMo/Switchyard
Length of output: 1099
Make the regression fixture type-safe under strict mypy.
_response requires ModelResponse, but this test passes a SimpleNamespace. Its dict[str, object] result is also indexed without narrowing. Use a typed ModelResponse fixture and narrow outputs before indexing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/experimental/litellm/tests/test_client.py` around lines 563 - 576,
Update the _response regression test fixture to construct a typed ModelResponse
instead of SimpleNamespace, preserving the content_filter response values.
Narrow or validate the returned outputs collection before indexing it so the
dict[str, object] result passes strict mypy checks.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: content-filter responses raise instead of normalizing
Summary
_response()raisedValueError("LiteLLM returned no text content")for anyresponse with empty
content, including content-filter responses where emptyoutput is expected. This caused legitimate content-filter results to fail
instead of being normalized.
Root cause
The guard at the end of
_response()did not distinguish between a trulyempty/invalid response and a
content_filterfinish reason, which by designreturns no content.
Fix
Allow empty
contentwhen the finish reason iscontent_filter; keep theexisting raise for all other empty-content cases so unexpected responses still
fail fast.
Testing
Added
test_response_content_filter_empty_contenttoexamples/experimental/litellm/tests/test_client.pyand verified the fullnon-e2e suite passes:
Contributor guidelines
Signed-off-by: andrewwhitecdw andrewwhitecdw@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests