fix(ccr): tolerate null/malformed OpenAI data in response handling (#2467)
## Description
Two sibling spots in the CCR OpenAI response handling assumed
well-formed provider data and crash on the present-but-null shapes some
OpenAI-compatible gateways send.
**1. Streaming reconstruction (`_reconstruct_openai_response`).**
Tool-call deltas were accumulated on key presence only:
```python
if "tool_calls" in delta:
for tc_delta in delta["tool_calls"]:
...
if "function" in tc_delta:
fn = tc_delta["function"]
if "name" in fn:
...
```
A delta with `"tool_calls": null` (or `"function": null`) has the key
present with a null value, so `for tc_delta in None` raises `TypeError:
'NoneType' object is not iterable`, aborting the whole CCR round. The
sibling line just above already value-guards content (`if "content" in
delta and delta["content"]:`).
**2. Responses assistant extraction (`_extract_assistant_message`).**
The `openai_responses` branch returned `response.get("output", [])`,
which only falls back when the key is absent. A present-but-null
`output` returned None, and `handle_response` then did
`current_messages.extend(None)`, the same `TypeError`. The `choices`
branch right above already guards this with `isinstance`.
## Fix
Guard the values, not just the keys:
- Iterate `tool_calls` only when it is a list, skip a non-dict entry,
and read `function` only when it is a dict.
- Coerce `output` to a list when it is not one.
Well-formed streams and responses reconstruct exactly as before.
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Code refactoring (no functional changes)
## Changes Made
- `headroom/ccr/response_handler.py`: value-guard
`tool_calls`/`function` (and skip non-dict tool-call entries) in
`_reconstruct_openai_response`; coerce a null/absent `output` to a list
in `_extract_assistant_message`.
- `tests/test_ccr_response_handler_extra.py`: regressions for null
`tool_calls`/`function` in the stream reconstruction and for a null
`output` in the Responses assistant extraction.
## Testing
- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [ ] Manual testing performed
### Test Output
```text
$ python -m pytest "tests/test_ccr_response_handler_extra.py::test_reconstruct_openai_response_tolerates_null_tool_calls_and_function" "tests/test_ccr_response_handler_extra.py::test_extract_assistant_message_responses_output_null_coerces_to_list" -q
2 passed
# with the reconstruction fix reverted, the first test fails with
# TypeError: 'NoneType' object is not iterable
$ uvx ruff@0.15.17 check headroom/ccr/response_handler.py tests/test_ccr_response_handler_extra.py
All checks passed!
$ uvx mypy@1.20.2 --ignore-missing-imports headroom/ccr/response_handler.py
Success: no issues found in 1 source file
```
Note: a handful of pre-existing async tests in this file fail in my
local venv because `pytest-asyncio` is not configured there (`Unknown
config option: asyncio_mode`); they fail identically on a clean `main`
without my change. The tests I added are synchronous.
## Real Behavior Proof
- Environment: Windows 11, Python 3.12, project venv (`uv sync --extra
proxy`), `uvx ruff@0.15.17` / `uvx mypy@1.20.2`, pytest in the venv.
- Exact command / steps: called the real
`StreamingCCRHandler._reconstruct_openai_response` with deltas carrying
`"tool_calls": null` and `"function": null`, and the real
`CCRResponseHandler._extract_assistant_message` with `{"output": None}`;
reverted the reconstruction fix and re-ran.
- Observed result: with the fixes the reconstruction returns the
concatenated content and the accumulated tool call, and the extraction
returns `{"_openai_responses_output_items": []}`; with the
reconstruction fix reverted the same input raises `TypeError: 'NoneType'
object is not iterable`. Ran against the actual module.
- Not tested: a live end-to-end CCR round against a provider that emits
these null frames.
## Review Readiness
- [x] I have performed a self-review
- [x] This PR is ready for human review
## Checklist
- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] I have updated the CHANGELOG.md if applicable
This commit is contained in:
@@ -403,7 +403,16 @@ class CCRResponseHandler:
|
||||
# echoed back verbatim as `input[]` items — not a single
|
||||
# role/content dict like chat completions. Sentinel key mirrors
|
||||
# `_openai_tool_results`; handle_response() extends on it.
|
||||
return {"_openai_responses_output_items": response.get("output", [])}
|
||||
# `.get("output", [])` only falls back when the key is absent, so a
|
||||
# present-but-null `output` would return None and make the
|
||||
# `current_messages.extend(...)` in handle_response raise TypeError;
|
||||
# coerce to a list like the choices branch above.
|
||||
output_items = response.get("output")
|
||||
return {
|
||||
"_openai_responses_output_items": output_items
|
||||
if isinstance(output_items, list)
|
||||
else []
|
||||
}
|
||||
elif provider == "google":
|
||||
# Google/Gemini format: role is "model", content is in candidates[0].content.parts
|
||||
candidates = response.get("candidates", [])
|
||||
@@ -905,8 +914,17 @@ class StreamingCCRHandler:
|
||||
if "content" in delta and delta["content"]:
|
||||
message["content"] = (message.get("content") or "") + delta["content"]
|
||||
|
||||
if "tool_calls" in delta:
|
||||
for tc_delta in delta["tool_calls"]:
|
||||
# Guard the value, not just the key: some OpenAI-compatible
|
||||
# providers include ``"tool_calls": null`` (and ``"function": null``)
|
||||
# in a delta rather than omitting the key, which would make the
|
||||
# iteration below raise ``TypeError: 'NoneType' object is not
|
||||
# iterable`` and abort the whole reconstruction. Mirrors the
|
||||
# ``and delta["content"]`` value-guard above.
|
||||
tool_calls = delta.get("tool_calls")
|
||||
if isinstance(tool_calls, list):
|
||||
for tc_delta in tool_calls:
|
||||
if not isinstance(tc_delta, dict):
|
||||
continue
|
||||
idx = tc_delta.get("index", 0)
|
||||
if idx not in tool_calls_map:
|
||||
tool_calls_map[idx] = {
|
||||
@@ -918,8 +936,8 @@ class StreamingCCRHandler:
|
||||
tc = tool_calls_map[idx]
|
||||
if "id" in tc_delta:
|
||||
tc["id"] = tc_delta["id"]
|
||||
if "function" in tc_delta:
|
||||
fn = tc_delta["function"]
|
||||
fn = tc_delta.get("function")
|
||||
if isinstance(fn, dict):
|
||||
if "name" in fn:
|
||||
tc["function"]["name"] = fn["name"]
|
||||
if "arguments" in fn:
|
||||
|
||||
@@ -320,6 +320,60 @@ def test_streaming_buffer_and_parse_sse_helpers() -> None:
|
||||
)
|
||||
|
||||
|
||||
def test_reconstruct_openai_response_tolerates_null_tool_calls_and_function() -> None:
|
||||
# Some OpenAI-compatible providers put ``"tool_calls": null`` (and
|
||||
# ``"function": null``) in a streaming delta instead of omitting the key.
|
||||
# Only checking key presence made the reconstruction iterate ``None`` and
|
||||
# raise ``TypeError``, aborting the whole CCR round.
|
||||
handler = StreamingCCRHandler(CCRResponseHandler(), provider="openai")
|
||||
|
||||
parsed = handler._reconstruct_openai_response(
|
||||
[
|
||||
{"choices": [{"delta": {"content": "Hi", "tool_calls": None}}]},
|
||||
{"choices": [{"delta": {"tool_calls": [{"index": 0, "function": None}]}}]},
|
||||
{
|
||||
"choices": [
|
||||
{
|
||||
"delta": {
|
||||
"tool_calls": [
|
||||
{
|
||||
"index": 0,
|
||||
"id": "call_1",
|
||||
"function": {"name": "f", "arguments": "{}"},
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
]
|
||||
},
|
||||
]
|
||||
)
|
||||
|
||||
message = parsed["choices"][0]["message"]
|
||||
# The null frames did not crash, and the real tool call still reconstructs.
|
||||
assert message["content"] == "Hi"
|
||||
assert message["tool_calls"][0]["id"] == "call_1"
|
||||
assert message["tool_calls"][0]["function"] == {"name": "f", "arguments": "{}"}
|
||||
|
||||
|
||||
def test_extract_assistant_message_responses_output_null_coerces_to_list() -> None:
|
||||
# A Responses turn with a present-but-null `output` (some gateways send this
|
||||
# on an empty/filtered turn) must not become None: handle_response later
|
||||
# does `current_messages.extend(...)` on it, which would raise TypeError.
|
||||
handler = CCRResponseHandler()
|
||||
|
||||
assert handler._extract_assistant_message({"output": None}, "openai_responses") == {
|
||||
"_openai_responses_output_items": []
|
||||
}
|
||||
# An absent output is also an empty list, and a real output passes through.
|
||||
assert handler._extract_assistant_message({}, "openai_responses") == {
|
||||
"_openai_responses_output_items": []
|
||||
}
|
||||
assert handler._extract_assistant_message(
|
||||
{"output": [{"type": "message"}]}, "openai_responses"
|
||||
) == {"_openai_responses_output_items": [{"type": "message"}]}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_streaming_handler_process_stream_pass_through_and_ccr(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
|
||||
Reference in New Issue
Block a user