Repository navigation
fix(frontend): return the Anthropic error envelope on bad request bodies - #13698
ayaangazali wants to merge 2 commits into
Conversation
`/v1/messages` and `/v1/messages/count_tokens` answer every failure they
produce themselves with the nested Anthropic envelope:
{"type":"error","error":{"type":"invalid_request_error","message":"..."}}
ai-dynamo#13102 added the same envelope for unmatched Anthropic routes, noting that
Anthropic clients expect that shape. Requests that fail before reaching the
handler did not get it. Both handlers used Axum's `Json` extractor, whose
rejections are `text/plain`:
Content-Type: text/plain -> 415 text/plain Expected request with `Content-Type: application/json`
body "{not json" -> 400 text/plain Failed to parse the request body as JSON: ...
`anthropic_error_middleware` does not cover these. It only rewrites 422 to
400, and neither rejection is a 422.
Read the body explicitly and report unsupported media type, oversized body and
malformed JSON through `anthropic_error`. `handler_count_tokens` gains the
`HeaderMap` it needs to see the content type.
Content-type parsing reuses `is_json_content_type` from the openai module
rather than reimplementing it. The policy is unchanged, only the envelope: an
absent `Content-Type` is still rejected.
Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
|
👋 Hi ayaangazali! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
external-contribution
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughAnthropic message and token-count handlers now use centralized request parsing. The parser validates JSON content types, enforces body limits, parses JSON, and returns Anthropic-formatted errors. Integration tests cover unsupported media types and malformed JSON. ChangesAnthropic request handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change makes malformed or unsupported Anthropic request bodies return the expected JSON error envelope without changing the existing status-code policy. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
|
Heads up on a coupling between this and its two siblings, which is my fault for not checking when I opened them. This PR, #13696 #13699 each add the same The route handlers themselves do not overlap ( Two ways out, your call:
Happy to do either, or to close two of them if you would rather see one. |
| .and_then(|value| value.to_str().ok()) | ||
| .is_some_and(is_json_content_type); | ||
| if !is_json { | ||
| return Err(anthropic_error( |
There was a problem hiding this comment.
[P1] Supply the required ErrorClass in all four new calls
anthropic_error now requires (ErrorClass, StatusCode, &str, &str), but the new calls at lines 163, 178, 187, and 196 supply only three arguments. cargo check --locked --offline -p dynamo-llm --no-default-features --lib fails with four E0061 errors at this head. Pass UnsupportedMedia, PayloadTooLarge, and InvalidRequest as appropriate so this compiles and records the correct local-failure classifications.
| { | ||
| anthropic_error( | ||
| StatusCode::PAYLOAD_TOO_LARGE, | ||
| "invalid_request_error", |
There was a problem hiding this comment.
[P2] Use request_too_large for the 413 envelope
An oversized body currently emits HTTP 413 with error.type = "invalid_request_error". Both existing Anthropic mappings in this module map payload-too-large failures to "request_too_large", and anthropic_error passes the supplied string through unchanged. Use that type here and add a regression assertion for the oversized-body branch; the two new tests only cover 400 and 415.
| } | ||
|
|
||
| /// Anthropic clients parse the nested `{"type": "error", "error": {...}}` | ||
| /// envelope, which #13102 established for unmatched routes. The Messages |
There was a problem hiding this comment.
[P3, optional] Keep the comment focused on the contract
This repeats the PR history, including the previous extractor and #13102. Trim it to the behavior being asserted, or let the test name and assertions explain it. The same historical explanation above read_anthropic_request can be shortened to its first two lines.
Summary
/v1/messagesand/v1/messages/count_tokensanswer every failure they produce themselves with the nested Anthropic envelope, and #13102 added the same envelope for unmatched Anthropic routes, with the note that Anthropic clients expect that shape:{"type":"error","error":{"type":"not_found_error","message":"Route not found: GET /v1/messages/missing"}}Requests that fail before reaching the handler did not get it. Both handlers used Axum's
Jsonextractor, whose rejections aretext/plain. Measured onmainwith the Anthropic endpoints enabled:So a client written against the documented envelope parses an unmatched route fine and throws on the two most common request mistakes.
I checked the obvious counter-hypothesis before concluding anything: this router layers
anthropic_error_middleware, so it looked like this might already be handled. It is not. That middleware only rewrites 422 to 400, and neither rejection is a 422.The fix reads the body explicitly and reports unsupported media type, oversized body and malformed JSON through
anthropic_error.handler_count_tokensgains theHeaderMapit needs to see the content type.Content-type parsing reuses
is_json_content_typefrom the openai module rather than reimplementing it, so this route keeps the same notion of what counts as JSON, including parameters and+jsonsuffixes. The policy is unchanged, only the envelope: an absentContent-Typeis still rejected, matchingensure_json_content_typetoday.Overlap worth flagging
Two things this touches are also touched elsewhere, so whoever merges second should expect a trivial rebase rather than a surprise:
is_json_content_typeaspub(super), for the same reason on/inference/v1/generate. Whichever lands first, the other drops that hunk.Content-Typeas JSON. If that lands, this route should follow it, and reusing the shared helper is what keeps that a one-place change.This is the same class as #13685 and #13696 but a third module with a third envelope, so it is deliberately not a copy of either: Anthropic nests under
{"type":"error","error":{...}},generateuses{"error":{...}}, and the openai routes use a flat{message,type,code}.Validation
Measured before and after by driving the route through
spawn_service(|b| b.enable_anthropic_endpoints(true)):Content-Type: text/plain415 text/plain415 application/json,{"type":"error","error":{"type":"invalid_request_error",...}}{not json400 text/plain400 application/json, same envelopeAdded
test_anthropic_bad_content_type_returns_anthropic_envelopeandtest_anthropic_malformed_json_returns_anthropic_envelope, asserting the responsecontent-typeas well as the body, because the status codes were already correct onmainand only the shape was wrong. Both confirmed red first by restoringanthropic.rsfromupstream/mainand re-running:main's 2041 plus the two added, withcargo clippyandcargo fmt --all -- --checkclean.test_oversized_body_returns_json_413, the pre-existing intermittent I reported on fix(frontend): preserve chat_template_args through Responses conversion #13624. It is unrelated to this change and fails at the same rate onmain.Not verified here: no GPU, so the success path of
/v1/messageswas not exercised end to end. This change only affects how the request body is read and how that read is reported when it fails.Summary by CodeRabbit