Skip to content

Commit 7ef736f

Browse files
Parideboyclaude
andauthored
fix(ccr): make StreamingCCRHandler work on OpenAI streams (#3069)
## Description `StreamingCCRHandler` (`headroom/ccr/response_handler.py`) was written against the Anthropic wire format. Constructed with `provider="openai"` it does not work: it silently drops the response, reports the wrong `finish_reason`, and emits a stream shape no OpenAI client can read. This PR fixes all three. **Reachability, stated up front:** `StreamingCCRHandler` is exported from `headroom/ccr/__init__.py` but no proxy handler instantiates it today. Every live CCR path (`handlers/openai.py:4276`, `handlers/openai.py:5936`, `handlers/anthropic.py`, `handlers/gemini.py`) calls `CCRResponseHandler.handle_response` on a non-streaming body instead. So these defects are not currently hit by proxy traffic. They bite anyone importing the public `headroom.ccr.StreamingCCRHandler` export, and they would bite the moment streaming CCR gets wired up. I would rather fix them while they are cheap than have them surface as a mysterious truncation bug later. **This PR does not fix #1026.** I found these while investigating that issue and they turned out to be unrelated to it. #1026 needs information from the reporter before anyone can say whether Headroom is even in the request path; I have asked for it there. ### The three defects **1. The whole OpenAI response was dropped.** `StreamingCCRBuffer.add_chunk` detected a tool call by scanning the accumulated bytes for the literal `"type":"tool_use"`. That is Anthropic-only. An OpenAI-compatible stream carries tool calls as a `tool_calls` array inside `choices[].delta` and never emits that marker, so `detected_ccr` could never become `True`. Independently, `process_stream` decided the stream had ended by scanning for `"stop_reason"`, another Anthropic-only field. An OpenAI stream has no such field; it terminates with the `[DONE]` sentinel. With neither marker ever matching, and nothing flushing the buffer once the source iterator ran out, the outcome was: - OpenAI stream under 10 000 bytes: **nothing at all was yielded**. The client got an empty response. - OpenAI stream over 10 000 bytes: chunks flushed in ~10 KB batches, and the final sub-threshold batch was never flushed. The response visibly stopped mid-sentence. **2. `finish_reason` was hardcoded.** `_reconstruct_openai_response` always returned `"finish_reason": "stop"`, even when it had just finished reconstructing a non-empty `tool_calls` array, where the OpenAI API requires `"tool_calls"`. A client that drives its agent loop off `finish_reason` reads `stop`, concludes the turn is over, and never executes the tool calls. The Anthropic sibling `_reconstruct_anthropic_response` does this correctly, carrying `stop_reason` through from `message_delta`. It also discarded `id`, `object`, `created`, `model`, and `usage`, returning a bare `choices` list that is not a valid `chat.completion`. **3. `_response_to_sse` emitted the wrong shape.** The OpenAI branch serialised the reconstructed **non-streaming** body into a single SSE frame. A streaming client parses `choices[].delta`; this frame has `choices[].message`. Both the text and the tool calls were invisible to it. ### Why CI did not catch it `tests/test_ccr_response_handler_extra.py` exercised `_reconstruct_openai_response` but never asserted `finish_reason`, and the one `process_stream` test that passed `provider="openai"` fed it Anthropic-shaped bytes (`"type":"tool_use"` plus `"stop_reason"`). No test had ever run a real OpenAI stream through this class. That test now uses the real OpenAI wire shape, so it actually covers the path it claims to. ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update - [ ] Refactor / internal change ## Changes Made All in `headroom/ccr/response_handler.py`: - `StreamingCCRBuffer` gained a `provider` field (defaults to `"anthropic"`, so existing construction is unchanged) and picks its tool-call marker from it: `"type":"tool_use"` for Anthropic, `"tool_calls"` for everything else. `StreamingCCRHandler.__init__` now passes its own provider down. - `process_stream` selects the end-of-stream marker by provider (`"stop_reason"` for Anthropic, `data: [DONE]` for OpenAI), and **always flushes whatever is still buffered once the source iterator is exhausted**. That second part is deliberately unconditional on the marker: upstream can truncate, a gateway can omit the sentinel, and a future stream shape may not be recognised. Buffered bytes at that point are real response data, so they get flushed rather than dropped. - Removed the dead re-iteration block that followed the detection loop. Its guard was `not detection_complete and not self.buffer.detected_ccr`, and the only `break` out of the loop above required `detected_ccr` to be `True`, so it could only ever be reached with an already-exhausted iterator. The new flush takes its place. - `_reconstruct_openai_response` derives `finish_reason`: `"tool_calls"` when the message carries tool calls, otherwise the last non-null upstream value (so a truncated turn stays reported as `"length"`), defaulting to `"stop"`. It carries `id` / `created` / `model` / `system_fingerprint` / `usage` through from the chunk envelope and stamps `"object": "chat.completion"`. It also tolerates `"delta": null` on a terminal chunk, which some OpenAI-compatible providers send instead of `{}`, in the same spirit as #2467. - New `_openai_response_to_chunks` splits a non-streaming `chat.completion` body into proper `chat.completion.chunk` frames (a role delta, a content delta, one delta per tool call, then a terminal frame carrying `finish_reason`). `_response_to_sse` uses it and then emits `[DONE]`. The Anthropic branch still delegates to `StreamingMixin._response_to_sse` and is untouched. Tests in `tests/test_ccr_response_handler_extra.py`: - Seven new tests: OpenAI CCR detection on a `tool_calls` delta (plus a non-CCR negative case), a short OpenAI stream passing through byte for byte, a stream past the 10 000-byte flush threshold keeping its tail, a stream with no `[DONE]` sentinel still flushing, `finish_reason` becoming `"tool_calls"` with the envelope preserved, the upstream `finish_reason` being kept when there are no tool calls, and `_response_to_sse` emitting parseable chunk frames. - `test_streaming_handler_falls_back_to_buffer_on_processing_error` now feeds genuine OpenAI SSE bytes instead of Anthropic ones, so it exercises the OpenAI detection path it was always meant to. - `test_response_to_sse_formats` asserts the new chunk-frame shape for OpenAI. The Anthropic half is unchanged. No behaviour change for `provider="anthropic"` beyond the end-of-iterator flush, which can only add data that was previously discarded. ## Testing - [x] Unit tests added/updated - [x] Existing tests pass - [ ] Manual testing performed - [ ] Integration tests added Each of the seven new tests was confirmed to fail against the unmodified source (`git stash` on `response_handler.py` alone, tests untouched), so they are genuine regression tests rather than assertions written to match current behaviour: ``` $ git stash push -- headroom/ccr/response_handler.py $ python -m pytest tests/test_ccr_response_handler_extra.py -q -k openai FAILED tests/test_ccr_response_handler_extra.py::test_streaming_buffer_detects_ccr_in_openai_tool_calls_delta FAILED tests/test_ccr_response_handler_extra.py::test_openai_stream_without_ccr_yields_every_chunk FAILED tests/test_ccr_response_handler_extra.py::test_openai_stream_past_flush_threshold_keeps_the_tail FAILED tests/test_ccr_response_handler_extra.py::test_openai_stream_without_done_sentinel_still_flushes FAILED tests/test_ccr_response_handler_extra.py::test_reconstruct_openai_response_marks_tool_calls_finish_reason FAILED tests/test_ccr_response_handler_extra.py::test_reconstruct_openai_response_keeps_upstream_finish_reason FAILED tests/test_ccr_response_handler_extra.py::test_response_to_sse_emits_openai_chunk_frames 7 failed, 2 passed, 13 deselected in 0.79s ``` With the fix applied, the full CCR response-handler suite passes: ``` $ python -m pytest tests/test_ccr_response_handler_extra.py tests/test_ccr_response_handler.py -q collected 57 items tests\test_ccr_response_handler_extra.py ...................... [ 38%] tests\test_ccr_response_handler.py ................................... [100%] ============================= 57 passed in 1.74s ============================== ``` Wider CCR and streaming surface: ``` $ python -m pytest tests/ -k "ccr or streaming" -q 4 failed, 696 passed, 73 skipped, 10949 deselected, 2 warnings in 175.80s (0:02:55) ``` The 4 failures are pre-existing on a clean `upstream/main` and unrelated to this change (verified by stashing both changed files and re-running exactly those four): `test_ccr_mcp_http.py::test_streamable_http_initialize_and_list_tools`, `test_cli_proxy_env.py::TestCLICompressionOnlyFlags::test_ccr_defaults_on`, and two in `test_transforms/test_smart_crusher_ccr_roundtrip.py`. Lint and types: ``` $ python -m ruff check . All checks passed! $ python -m ruff format --check . 1505 files already formatted $ python -m mypy headroom --ignore-missing-imports Found 12 errors in 3 files (checked 521 source files) ``` Zero mypy errors in `headroom/ccr/response_handler.py`. The 12 are pre-existing, in `ccr/mcp_server.py`, `memory/mcp_server.py`, and `release_version.py`, none of which this PR touches (they come from a locally installed `mcp` whose stubs differ from CI's). ## Real Behavior Proof - Environment: Windows 11, Python 3.13.11, pytest 9.1.1, ruff and mypy from the repo's pinned config, branch `fix/ccr-streaming-openai-path` off `upstream/main` at `cbb950a4`. - Exact command / steps: `python -m pytest tests/test_ccr_response_handler_extra.py tests/test_ccr_response_handler.py -q`; then `git stash push -- headroom/ccr/response_handler.py` and `python -m pytest tests/test_ccr_response_handler_extra.py -q -k openai` to confirm the new tests fail without the source fix; then `python -m pytest tests/ -k "ccr or streaming" -q`; then `python -m ruff check .`, `python -m ruff format --check .`, `python -m mypy headroom --ignore-missing-imports`. - Observed result: 57/57 pass in the CCR response-handler suites with the fix; all 7 new tests fail without it. The wider run is 696 passed with 4 failures that reproduce identically on an unmodified tree. Ruff clean, mypy clean on the changed file. In `test_openai_stream_without_ccr_yields_every_chunk` the handler now returns every input chunk byte for byte, where before it returned an empty list. - Not tested: no end-to-end run against a live OpenAI-compatible backend, because no proxy handler instantiates `StreamingCCRHandler` today, so there is no wired path to drive. Coverage is at the class level using recorded-shape SSE frames. The Anthropic path is covered only by the existing tests, which still pass unchanged. ## Runtime Rollout Safety - Rollout-managed feature(s): none. `StreamingCCRHandler` is not gated by a rollout feature and is not reachable from any proxy handler. - Minimum rollout channel: not applicable; no rollout gate is involved. - Stable/default behavior changed: no. For `provider="anthropic"` the only behavioural difference is that bytes left buffered when the source iterator ends are now flushed instead of discarded, which can only add data the client previously lost. For `provider="openai"` the class was non-functional, so there is no prior behaviour to preserve. - Kill switch / disable path: not applicable; no new configuration, env var, or feature flag is introduced. - Unsafe override required: no. - Qualification impact: none. No qualification-gated surface is touched. - Rollback path: revert this commit. It is self-contained in `headroom/ccr/response_handler.py` and `tests/test_ccr_response_handler_extra.py`, with no schema, config, or persisted-state changes. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review Two judgement calls worth a reviewer's attention: 1. **Removing the dead re-iteration block** in `process_stream`. I am confident it was unreachable (the only `break` above it requires `detected_ccr`, which its own guard excludes), but it is the one deletion in this diff rather than an addition, so it is worth a second pair of eyes. 2. **The unconditional end-of-iterator flush.** I chose to flush regardless of whether an end marker matched, rather than only fixing the OpenAI marker. That makes the truncation bug unreachable even if a future provider uses a shape neither marker recognises. The cost is that a stream whose trailing bytes are genuinely not meant for the client would now be forwarded. Given the buffer only ever holds upstream response bytes, forwarding is the safer default, but flag it if you disagree. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent eeb038b commit 7ef736f

2 files changed

Lines changed: 388 additions & 25 deletions

File tree

headroom/ccr/response_handler.py

Lines changed: 145 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -571,11 +571,23 @@ class StreamingCCRBuffer:
571571
chunks: list[bytes] = field(default_factory=list)
572572
detected_ccr: bool = False
573573
complete_response: dict[str, Any] | None = None
574+
provider: str = "anthropic"
574575

575-
# Patterns to detect tool_use in stream
576+
# Wire markers for the start of a tool call. Anthropic streams
577+
# `"type":"tool_use"` content blocks; OpenAI-compatible streams carry a
578+
# `"tool_calls"` array inside `choices[].delta` and never emit the
579+
# Anthropic marker, so scanning only for the latter meant CCR was never
580+
# detected on an OpenAI stream.
576581
_tool_use_start: bytes = b'"type":"tool_use"'
582+
_openai_tool_use_start: bytes = b'"tool_calls"'
577583
_ccr_tool_pattern: bytes = f'"{CCR_TOOL_NAME}"'.encode()
578584

585+
def _tool_call_marker(self) -> bytes:
586+
"""The provider's on-the-wire marker for the start of a tool call."""
587+
if self.provider == "anthropic":
588+
return self._tool_use_start
589+
return self._openai_tool_use_start
590+
579591
def add_chunk(self, chunk: bytes) -> bool:
580592
"""Add a chunk and check for CCR tool calls.
581593
@@ -587,7 +599,7 @@ def add_chunk(self, chunk: bytes) -> bool:
587599
# Quick check: does accumulated content contain CCR tool?
588600
accumulated = b"".join(self.chunks)
589601

590-
if self._tool_use_start in accumulated and self._ccr_tool_pattern in accumulated:
602+
if self._tool_call_marker() in accumulated and self._ccr_tool_pattern in accumulated:
591603
self.detected_ccr = True
592604
return True
593605

@@ -622,7 +634,7 @@ def __init__(
622634
) -> None:
623635
self.response_handler = response_handler
624636
self.provider = provider
625-
self.buffer = StreamingCCRBuffer()
637+
self.buffer = StreamingCCRBuffer(provider=provider)
626638

627639
async def process_stream(
628640
self,
@@ -648,8 +660,12 @@ async def process_stream(
648660
Response chunks (possibly from continuation response).
649661
"""
650662
# Phase 1: Initial detection
651-
# Buffer chunks until we can determine if there's a CCR call
652-
detection_complete = False
663+
# Buffer chunks until we can determine if there's a CCR call.
664+
#
665+
# The end-of-stream marker is provider-specific. Anthropic signals the
666+
# terminal state with `stop_reason` in `message_delta`; OpenAI-compatible
667+
# streams have no such field and terminate with the `[DONE]` sentinel.
668+
end_marker = b'"stop_reason"' if self.provider == "anthropic" else b"data: [DONE]"
653669

654670
async for chunk in stream_iterator:
655671
self.buffer.add_chunk(chunk)
@@ -660,9 +676,7 @@ async def process_stream(
660676
accumulated = self.buffer.get_accumulated()
661677

662678
# Look for stream end markers
663-
if b'"stop_reason"' in accumulated:
664-
detection_complete = True
665-
679+
if end_marker in accumulated:
666680
if self.buffer.detected_ccr:
667681
# CCR detected - need to handle
668682
break
@@ -679,13 +693,15 @@ async def process_stream(
679693
yield buffered_chunk
680694
self.buffer.clear()
681695

682-
# Continue streaming rest of response
683-
if not detection_complete and not self.buffer.detected_ccr:
684-
async for chunk in stream_iterator:
685-
if self.buffer.detected_ccr:
686-
self.buffer.add_chunk(chunk)
687-
else:
688-
yield chunk
696+
# The end marker is not guaranteed to arrive: upstream can truncate, a
697+
# provider can omit the sentinel, or the stream can be a shape this
698+
# detector does not recognise. Anything still buffered once the source
699+
# iterator is exhausted is real response data the client has never
700+
# seen, so flush it instead of dropping it.
701+
if not self.buffer.detected_ccr and self.buffer.chunks:
702+
for buffered_chunk in self.buffer.chunks:
703+
yield buffered_chunk
704+
self.buffer.clear()
689705

690706
# Phase 2: Handle CCR if detected
691707
if self.buffer.detected_ccr:
@@ -903,13 +919,38 @@ def _reconstruct_openai_response(
903919
}
904920

905921
tool_calls_map: dict[int, dict[str, Any]] = {}
922+
finish_reason: str | None = None
923+
envelope: dict[str, Any] = {}
924+
usage: Any = None
906925

907926
for event in events:
908-
choices = event.get("choices", [])
909-
if not choices:
927+
# Carry the chunk envelope through. Dropping it left the
928+
# reconstructed body without `id`, `model`, `created` or `usage`,
929+
# which downstream middleware reads for routing and metering.
930+
for key in ("id", "created", "model", "system_fingerprint"):
931+
value = event.get(key)
932+
if value is not None:
933+
envelope[key] = value
934+
if event.get("usage") is not None:
935+
usage = event["usage"]
936+
937+
choices = event.get("choices")
938+
if not isinstance(choices, list) or not choices:
910939
continue
940+
choice = choices[0]
941+
if not isinstance(choice, dict):
942+
continue
943+
944+
# `finish_reason` is null on every chunk but the last, so keep the
945+
# most recent non-null value rather than the first one seen.
946+
if choice.get("finish_reason") is not None:
947+
finish_reason = choice["finish_reason"]
911948

912-
delta = choices[0].get("delta", {})
949+
# Some OpenAI-compatible providers send `"delta": null` on the
950+
# terminal chunk instead of an empty object.
951+
delta = choice.get("delta")
952+
if not isinstance(delta, dict):
953+
delta = {}
913954

914955
if "content" in delta and delta["content"]:
915956
message["content"] = (message.get("content") or "") + delta["content"]
@@ -944,14 +985,94 @@ def _reconstruct_openai_response(
944985
tc["function"]["arguments"] += fn["arguments"]
945986

946987
message["tool_calls"] = [tool_calls_map[i] for i in sorted(tool_calls_map.keys())]
947-
if not message["tool_calls"]:
988+
has_tool_calls = bool(message["tool_calls"])
989+
if not has_tool_calls:
948990
del message["tool_calls"]
949991
if not message["content"]:
950992
message["content"] = None
951993

952-
return {
953-
"choices": [{"message": message, "finish_reason": "stop"}],
994+
# OpenAI requires `finish_reason: "tool_calls"` whenever the message
995+
# carries tool calls. This was hardcoded to "stop", which tells any
996+
# client that drives its agent loop off `finish_reason` that the turn
997+
# is over, so the reconstructed tool calls were never executed.
998+
if has_tool_calls:
999+
finish_reason = "tool_calls"
1000+
elif finish_reason is None:
1001+
finish_reason = "stop"
1002+
1003+
response: dict[str, Any] = {
1004+
"object": "chat.completion",
1005+
**envelope,
1006+
"choices": [{"index": 0, "message": message, "finish_reason": finish_reason}],
9541007
}
1008+
if usage is not None:
1009+
response["usage"] = usage
1010+
return response
1011+
1012+
def _openai_response_to_chunks(self, response: dict[str, Any]) -> list[bytes]:
1013+
"""Split a non-streaming ``chat.completion`` body into SSE chunk frames.
1014+
1015+
A streaming client reads ``choices[].delta``, not ``choices[].message``.
1016+
Serialising the reconstructed non-streaming body into a single SSE frame
1017+
produced a stream in which both the text and the tool calls were
1018+
invisible to the client.
1019+
"""
1020+
choices = response.get("choices")
1021+
choice = choices[0] if isinstance(choices, list) and choices else {}
1022+
if not isinstance(choice, dict):
1023+
choice = {}
1024+
message = choice.get("message")
1025+
if not isinstance(message, dict):
1026+
message = {}
1027+
finish_reason = choice.get("finish_reason") or "stop"
1028+
1029+
base: dict[str, Any] = {"object": "chat.completion.chunk"}
1030+
for key in ("id", "created", "model", "system_fingerprint"):
1031+
if response.get(key) is not None:
1032+
base[key] = response[key]
1033+
1034+
def frame(delta: dict[str, Any], reason: str | None) -> bytes:
1035+
payload = {
1036+
**base,
1037+
"choices": [{"index": 0, "delta": delta, "finish_reason": reason}],
1038+
}
1039+
return f"data: {json.dumps(payload)}\n\n".encode()
1040+
1041+
frames = [frame({"role": message.get("role") or "assistant"}, None)]
1042+
1043+
content = message.get("content")
1044+
if content:
1045+
frames.append(frame({"content": content}, None))
1046+
1047+
tool_calls = message.get("tool_calls")
1048+
if isinstance(tool_calls, list):
1049+
for index, tool_call in enumerate(tool_calls):
1050+
if not isinstance(tool_call, dict):
1051+
continue
1052+
function = tool_call.get("function")
1053+
if not isinstance(function, dict):
1054+
function = {}
1055+
frames.append(
1056+
frame(
1057+
{
1058+
"tool_calls": [
1059+
{
1060+
"index": index,
1061+
"id": tool_call.get("id", ""),
1062+
"type": tool_call.get("type", "function"),
1063+
"function": {
1064+
"name": function.get("name", ""),
1065+
"arguments": function.get("arguments", ""),
1066+
},
1067+
}
1068+
]
1069+
},
1070+
None,
1071+
)
1072+
)
1073+
1074+
frames.append(frame({}, finish_reason))
1075+
return frames
9551076

9561077
async def _response_to_sse(
9571078
self,
@@ -968,6 +1089,7 @@ async def _response_to_sse(
9681089
for chunk in StreamingMixin()._response_to_sse(response, "anthropic"):
9691090
yield chunk
9701091
else:
971-
# OpenAI SSE format
972-
yield f"data: {json.dumps(response)}\n\n".encode()
1092+
# OpenAI SSE format: `chat.completion.chunk` frames, then [DONE].
1093+
for chunk in self._openai_response_to_chunks(response):
1094+
yield chunk
9731095
yield b"data: [DONE]\n\n"

0 commit comments

Comments
 (0)