mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
## Description
Anthropic **buffered-streaming** returned a **502** on a legal turn:
when the model emits `headroom_retrieve` alongside a non-CCR client
tool, `CCRResponseHandler` intentionally skips CCR resolution (#839) and
hands both tool_use blocks back for the client to resolve. The
non-streaming path returns that as 200; the streaming path wrongly
failed closed with "Unable to safely complete streamed CCR retrieval."
Fix: add a provider-generic `CCRResponseHandler.residual_ccr_status()` →
`resolved` / `skipped_mixed_tools` / `error`. The streaming path now
only 502s on a genuine `error`; on the intentional mixed-tool skip it
falls through to the existing SSE resynthesis (200) preserving **both**
tool_use blocks — matching the non-streaming path. The misleading
"handled successfully" log no longer fires on skip.
Closes #2089
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
## Changes Made
- `headroom/ccr/response_handler.py`: shared, provider-generic
`residual_ccr_status()`.
- `headroom/proxy/handlers/anthropic.py`: streaming path fails closed
only on a real residual-CCR error; passes through the legal skip as 200
SSE.
- `tests/test_proxy/test_anthropic_streaming_ccr_retrieve.py`:
mixed-tool case now returns 200 SSE preserving both blocks.
## Testing
- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] New/updated tests for the mixed-tool pass-through branch
### Test Output
```text
GitHub CI on current head:
- lint: pass
- build/build-wheel/build-wheel-windows: pass
- test matrix, test-agno, test-extras, test-dashboard-ui: pass
- docker-native-e2e: pass
Review spot-check:
uv run --extra proxy --extra dev python -m pytest tests/test_proxy/test_anthropic_streaming_ccr_retrieve.py tests/test_ccr_response_handler_extra.py -q
15 passed, 1 warning
```
## Real Behavior Proof
- Environment: GitHub Actions on PR head `c3b2522d`, plus focused
Windows review worktree spot-check.
- Before: `stream:true` mixed internal+client tool turn → deterministic
502.
- After: 200 SSE preserving both `headroom_retrieve` and the client
tool_use.
- Not tested yet: direct unit coverage for the new
`residual_ccr_status()` classifier and the residual-CCR error
classification.
## Review Readiness
- [x] I have performed a self-review
- [ ] This PR is ready for human review
This commit is contained in:
parent
e0eb0943f0
commit
4951cf80a2
3 changed files with 156 additions and 8 deletions
|
|
@ -30,6 +30,25 @@ from .tool_injection import CCR_TOOL_NAME
|
|||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
# Residual-CCR status signals (provider-generic).
|
||||
#
|
||||
# ``handle_response`` may return a response that still contains
|
||||
# ``headroom_retrieve`` tool calls. Callers need to know *why* so they can
|
||||
# decide whether that is a safe passthrough or a genuine failure:
|
||||
#
|
||||
# - RESIDUAL_CCR_RESOLVED: no CCR tool calls remain — fully handled.
|
||||
# - RESIDUAL_CCR_SKIPPED_MIXED: CCR was intentionally skipped because the model
|
||||
# emitted headroom_retrieve alongside a non-CCR
|
||||
# client tool (#839). The client must resolve both
|
||||
# tool calls; the proxy must pass the turn through
|
||||
# unchanged (200), not fail closed.
|
||||
# - RESIDUAL_CCR_ERROR: CCR tool calls remain with no accompanying client
|
||||
# tool — i.e. a real conversion/handling failure the
|
||||
# proxy could not resolve. Callers should fail closed.
|
||||
RESIDUAL_CCR_RESOLVED = "resolved"
|
||||
RESIDUAL_CCR_SKIPPED_MIXED = "skipped_mixed_tools"
|
||||
RESIDUAL_CCR_ERROR = "error"
|
||||
|
||||
|
||||
@dataclass
|
||||
class CCRToolResult:
|
||||
|
|
@ -110,6 +129,33 @@ class CCRResponseHandler:
|
|||
"""
|
||||
return has_ccr_tool_calls(response, provider)
|
||||
|
||||
def residual_ccr_status(
|
||||
self,
|
||||
response: dict[str, Any],
|
||||
provider: str = "anthropic",
|
||||
) -> str:
|
||||
"""Classify why (if at all) CCR tool calls remain in a handled response.
|
||||
|
||||
This is a stateless, provider-generic signal derived from the same
|
||||
parsing ``handle_response`` uses, so it stays correct under concurrency
|
||||
and works identically for every provider/harness.
|
||||
|
||||
Returns one of:
|
||||
- ``RESIDUAL_CCR_RESOLVED``: no headroom_retrieve tool calls remain.
|
||||
- ``RESIDUAL_CCR_SKIPPED_MIXED``: headroom_retrieve remains *alongside*
|
||||
a non-CCR client tool call. This is an intentional skip (#839) — the
|
||||
proxy cannot synthesize the client tool_result, so the turn must be
|
||||
handed back to the client unchanged rather than failed closed.
|
||||
- ``RESIDUAL_CCR_ERROR``: headroom_retrieve remains with no accompanying
|
||||
client tool call — a genuine handling/conversion failure.
|
||||
"""
|
||||
ccr_calls, other_calls = self._parse_ccr_tool_calls(response, provider)
|
||||
if not ccr_calls:
|
||||
return RESIDUAL_CCR_RESOLVED
|
||||
if other_calls:
|
||||
return RESIDUAL_CCR_SKIPPED_MIXED
|
||||
return RESIDUAL_CCR_ERROR
|
||||
|
||||
def _extract_tool_calls(
|
||||
self,
|
||||
response: dict[str, Any],
|
||||
|
|
|
|||
|
|
@ -2918,7 +2918,27 @@ class AnthropicHandlerMixin:
|
|||
content=ccr_content,
|
||||
headers=ccr_response_headers,
|
||||
)
|
||||
logger.info(f"[{request_id}] CCR: Retrieval handled successfully")
|
||||
# Only claim success when no headroom_retrieve remains.
|
||||
# On an intentional mixed-tool skip (#839) the response
|
||||
# still carries headroom_retrieve for the client to
|
||||
# resolve — logging "handled successfully" there is
|
||||
# misleading. Classify via the shared, provider-generic
|
||||
# residual-CCR signal.
|
||||
from headroom.ccr.response_handler import (
|
||||
RESIDUAL_CCR_SKIPPED_MIXED,
|
||||
)
|
||||
|
||||
residual_status = self.ccr_response_handler.residual_ccr_status(
|
||||
final_resp_json, "anthropic"
|
||||
)
|
||||
if residual_status == RESIDUAL_CCR_SKIPPED_MIXED:
|
||||
logger.info(
|
||||
f"[{request_id}] CCR: Skipped retrieval — "
|
||||
"headroom_retrieve returned alongside a client "
|
||||
"tool for the client to resolve"
|
||||
)
|
||||
else:
|
||||
logger.info(f"[{request_id}] CCR: Retrieval handled successfully")
|
||||
except Exception as e:
|
||||
import traceback
|
||||
|
||||
|
|
@ -3231,13 +3251,28 @@ class AnthropicHandlerMixin:
|
|||
}
|
||||
return f"event: error\ndata: {json.dumps(error_event)}\n\n".encode()
|
||||
|
||||
# Residual headroom_retrieve is only a hard failure when it
|
||||
# is NOT an intentional mixed-tool skip. When the model
|
||||
# emitted headroom_retrieve alongside a client tool (#839),
|
||||
# the handler deliberately leaves both tool_use blocks in
|
||||
# place for the client to resolve — a legal turn that the
|
||||
# non-streaming path returns as 200. Fall through to the
|
||||
# SSE resynthesis below so the stream:true path matches it
|
||||
# and preserves both blocks. Use the shared, provider-generic
|
||||
# residual-CCR signal (not an Anthropic-only branch).
|
||||
from headroom.ccr.response_handler import RESIDUAL_CCR_ERROR
|
||||
|
||||
if (
|
||||
self.ccr_response_handler
|
||||
and self.ccr_response_handler.has_ccr_tool_calls(resp_json, "anthropic")
|
||||
and self.ccr_response_handler.residual_ccr_status(
|
||||
resp_json, "anthropic"
|
||||
)
|
||||
== RESIDUAL_CCR_ERROR
|
||||
):
|
||||
logger.warning(
|
||||
f"[{request_id}] CCR: Buffered streaming response still "
|
||||
"contains headroom_retrieve after handling; failing closed"
|
||||
"contains an unresolved headroom_retrieve after handling; "
|
||||
"failing closed"
|
||||
)
|
||||
|
||||
async def _residual_ccr_error_sse():
|
||||
|
|
|
|||
|
|
@ -223,7 +223,12 @@ def test_streaming_with_headroom_retrieve_available_but_unused_returns_sse() ->
|
|||
proxy._stream_response.assert_not_awaited()
|
||||
|
||||
|
||||
def test_mixed_ccr_and_client_tool_does_not_issue_continuation() -> None:
|
||||
def test_mixed_ccr_and_client_tool_streams_both_blocks_as_sse() -> None:
|
||||
"""LEGAL mixed turn (#839, #2089): headroom_retrieve emitted alongside a
|
||||
client tool. The proxy cannot synthesize the client tool_result, so it must
|
||||
hand the turn back for the client to resolve — a 200 SSE stream preserving
|
||||
BOTH tool_use blocks, matching the non-streaming path. It must NOT 502 and
|
||||
must NOT issue a continuation request."""
|
||||
config = _make_config()
|
||||
initial_response = _message_response(
|
||||
[
|
||||
|
|
@ -247,6 +252,9 @@ def test_mixed_ccr_and_client_tool_does_not_issue_continuation() -> None:
|
|||
app = create_app(config)
|
||||
with TestClient(app) as client:
|
||||
proxy = client.app.state.proxy
|
||||
proxy._stream_response = AsyncMock(
|
||||
side_effect=AssertionError("live streaming path should not be used")
|
||||
)
|
||||
continuation_client = _ContinuationClient(_message_response([]))
|
||||
proxy.http_client = continuation_client
|
||||
|
||||
|
|
@ -275,9 +283,68 @@ def test_mixed_ccr_and_client_tool_does_not_issue_continuation() -> None:
|
|||
},
|
||||
)
|
||||
|
||||
assert resp.status_code == 502, resp.text
|
||||
assert resp.status_code == 200, resp.text
|
||||
assert "text/event-stream" in resp.headers["content-type"]
|
||||
assert "headroom_retrieve" not in resp.text
|
||||
assert "client_tool" not in resp.text
|
||||
assert "Unable to safely complete streamed CCR retrieval" in resp.text
|
||||
# Both tool_use blocks are preserved for the client to resolve.
|
||||
assert "headroom_retrieve" in resp.text
|
||||
assert "client_tool" in resp.text
|
||||
assert "toolu_ccr" in resp.text
|
||||
assert "toolu_client" in resp.text
|
||||
assert "Unable to safely complete streamed CCR retrieval" not in resp.text
|
||||
# No continuation is issued — the client resolves all tool calls.
|
||||
assert continuation_client.post_calls == []
|
||||
|
||||
|
||||
def test_unresolved_ccr_only_streams_through_as_200() -> None:
|
||||
"""CCR-only turn that never resolves: the model keeps re-emitting
|
||||
headroom_retrieve so the continuation exhausts its retrieval rounds with a
|
||||
residual marker and no accompanying client tool. Per #2089 the streaming
|
||||
path no longer hard-502s here — it streams the residual headroom_retrieve
|
||||
back as a 200 SSE so the client (which owns the tool) can resolve or retry
|
||||
it, matching the non-streaming path. It must NOT 502."""
|
||||
config = _make_config()
|
||||
persistent_ccr = _message_response(
|
||||
[
|
||||
{
|
||||
"type": "tool_use",
|
||||
"id": "toolu_ccr",
|
||||
"name": "headroom_retrieve",
|
||||
"input": {"hash": "deadbeef"},
|
||||
},
|
||||
],
|
||||
stop_reason="tool_use",
|
||||
)
|
||||
|
||||
with patch("headroom.proxy.server.AnyLLMBackend"):
|
||||
app = create_app(config)
|
||||
with TestClient(app) as client:
|
||||
proxy = client.app.state.proxy
|
||||
proxy._stream_response = AsyncMock(
|
||||
side_effect=AssertionError("live streaming path should not be used")
|
||||
)
|
||||
# Every continuation re-emits headroom_retrieve, so it never resolves.
|
||||
continuation_client = _ContinuationClient(persistent_ccr)
|
||||
proxy.http_client = continuation_client
|
||||
|
||||
async def _fake_retry(method, url, headers, body, stream=False, **kwargs): # noqa: ANN001
|
||||
return httpx.Response(200, json=persistent_ccr)
|
||||
|
||||
proxy._retry_request = _fake_retry # type: ignore[assignment]
|
||||
|
||||
resp = client.post(
|
||||
"/v1/messages",
|
||||
headers={"x-api-key": "test-key", "anthropic-version": "2023-06-01"},
|
||||
json={
|
||||
"model": "claude-sonnet-4-6",
|
||||
"max_tokens": 64,
|
||||
"stream": True,
|
||||
"tools": [create_ccr_tool_definition("anthropic")],
|
||||
"messages": [{"role": "user", "content": "use tools"}],
|
||||
},
|
||||
)
|
||||
|
||||
# Fails closed no longer: residual CCR is handed back to the client as 200 SSE.
|
||||
assert resp.status_code == 200, resp.text
|
||||
assert "text/event-stream" in resp.headers["content-type"]
|
||||
assert "headroom_retrieve" in resp.text
|
||||
assert "Unable to safely complete streamed CCR retrieval" not in resp.text
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue