fix: preserve Claude Code Messages transport fidelity - #161
fix: preserve Claude Code Messages transport fidelity#161franciscojavierarceo wants to merge 1 commit into
Conversation
b8c8b43 to
315e4f0
Compare
315e4f0 to
971afa9
Compare
ashwing
left a comment
There was a problem hiding this comment.
First pass — the transport-fidelity direction is right, and priming the first upstream request before the handler commits an HTTP 200 is the correct fix for preserving streaming error status. One correctness issue on the open-ended header forwarding, plus the test gap around it.
| /// open-ended, and the configured credential is injected only when the client | ||
| /// did not supply one. | ||
| #[must_use] | ||
| pub fn upstream_request_headers(headers: &HeaderMap, config: &Config, auth: ProxyAuth) -> reqwest::header::HeaderMap { |
There was a problem hiding this comment.
Anchoring on the forwarding entry point — the issue is the REQUEST_DROP_EXTRA list (["host", "content-length"]): accept-encoding isn't dropped there and isn't hop-by-hop, so open-ended forwarding now sends the client's accept-encoding: gzip, br upstream.
The proxy path gets away with this because it streams the upstream bytes back verbatim (ProxyBody::Stream) and the client decompresses. But #161 routes the gateway-tool loop through response_lines / fetch_response_json_with_headers, which parse the body in-process — and our reqwest is built default-features = false with no gzip/brotli feature, so it won't auto-decompress. If vLLM (or an intermediary) honors the forwarded accept-encoding, the loop parses compressed bytes and every SSE/JSON parse fails.
The old extract_client_key path sent only Content-Type, so upstream always replied uncompressed — this is a regression from the switch to open-ended forwarding. Add accept-encoding to REQUEST_DROP_EXTRA (the gateway advertises an encoding it can't decode). content-encoding on the response side is worth the same thought.
| ); | ||
| assert_eq!(requests[0].headers["x-claude-code-session-id"], "session-loop"); | ||
| assert_eq!(requests[0].headers["x-claude-code-agent-id"], "agent-loop"); | ||
| assert_eq!(requests[0].headers["x-api-key"], "anthropic-key"); |
There was a problem hiding this comment.
These assert the allowlisted headers (anthropic-version, anthropic-beta, x-claude-code-*, x-api-key) survive — good — but nothing asserts that headers which would break in-process parsing get stripped. Once accept-encoding is dropped, add a negative case here: client sends accept-encoding: gzip, assert it's absent from requests[0].headers. That pins the parse-path contract so a later header-forwarding change can't silently reintroduce it.
maralbahari
left a comment
There was a problem hiding this comment.
I left some inline comments.
couldnt add this as inline but worth addressing:
inference.rs (line 88) retains only status/body, and messages.rs (line 44) reconstructs only Content-Type. A 429 therefore loses retry-after, while request-id and rate-limit headers disappear from both error and successful loop responses. Preserve filtered upstream response headers, at least from the terminal round. Anthropic rate-limit headers, request IDs
| fn messages_error_response(err: ExecutorError) -> Response { | ||
| if let ExecutorError::LLMRequest { status, body } = err { | ||
| return (status, [(http::header::CONTENT_TYPE, "application/json")], body).into_response(); | ||
| } |
There was a problem hiding this comment.
treats every LLMRequest as an upstream response, but connection failures and timeouts use the same variant with plain bodies such as upstream unavailable. Gateway-tool requests therefore return Content-Type: application/json with non-JSON text, breaking Anthropic SDK error parsing. Anthropic specifies that API errors are always JSON envelopes. Use a distinct transport-error variant, or only forward bodies that parse as Anthropic errors
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn messages_stream_preserves_error_event_after_gateway_tool_round() { |
There was a problem hiding this comment.
this test simulates a second-round HTTP 400 response, not a documented SSE error received after HTTP 200.
When the upstream sends:
event: error
data: {"type":"error","error":{"type":"overloaded_error","message":"Overloaded"}}
MessagesStreamAccumulator::push falls through to the wildcard branch and drops the event. The loop then calls finish(), which emits message_stop. Clients can therefore interpret partial or empty output as a successful completion instead of handling or retrying the error.
Please forward type: "error" events and terminate the synthetic stream. https://platform.claude.com/docs/en/build-with-claude/streaming.
| "text": "<attribution>claude-code-session</attribution>", | ||
| "cache_control": { | ||
| "type": "ephemeral", | ||
| "ttl": "5m" |
There was a problem hiding this comment.
contains five explicit breakpoints, while Anthropic allows four. It also places 5m entries before later 1h entries; longer TTLs must precede shorter TTLs in the effective tools → system → messages prompt order. The replay server accepts this invalid shape, so the tests provide false compatibility confidence. Anthropic prompt-caching constraints
971afa9 to
506ed5a
Compare
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
506ed5a to
8311bab
Compare
Summary
The E2E job uses localhost services and a placeholder token. It requires no Anthropic credential, GPU, model download, or paid inference. This change does not add conversation persistence or other server-side client state; the request-scoped transport context is reused only for the gateway-owned tool loop.
For non-streaming requests, response metadata comes from the terminal upstream round. For streaming requests, HTTP response metadata comes from the initial upstream response because later rounds occur after the client response has started.
The visible reasoning double-render remains a vLLM-side issue tracked separately; this PR closes the agentic-api transport and error-fidelity gaps found by running Claude Code.
Related to #116.
Test Plan