feat: add per-request context to the /v1/messages loop (#137) - #249
Open
Zheng-Lu wants to merge 1 commit into
Open
feat: add per-request context to the /v1/messages loop (#137)#249Zheng-Lu wants to merge 1 commit into
Zheng-Lu wants to merge 1 commit into
Conversation
Closes vllm-project#137. Follow-up to vllm-project#131, addressing the code review concern that the Messages loop took a bare `serde_json::Value` at a public API boundary while the handler separately parsed a `MessagesRequest` for typed field access. Introduces `MessagesRequestContext { typed: MessagesRequest, raw: Value }`, built once per request, and threads it through `run_messages_loop` and `run_messages_stream` in place of the untyped body. The raw body stays the thing forwarded upstream, and is deliberately not re-serialized from the typed view. `ContentBlock` catches unmodeled block types in `#[serde(other)] Unknown` and several variants model only the fields the gateway reads, so a typed round-trip would drop `cache_control` and `is_error` and collapse `image`/`redacted_thinking` into a literal `{"type":"unknown"}` block. - The typed view exposes only `tools()`, `stream()` and `model()` — the fields the loops read and never mutate. `messages` and `system` are unreachable through it, so a stale typed view cannot be read back after a round is appended. - The context owns every mutation to the upstream body (`force_stream`, `append_round`) plus the native web-search budget, replacing the two duplicated `append_round_to_history` copies with one method. - The two views diverge only where the gateway rewrites for upstream: `normalize_native_web_search` rewrites the native `web_search_20250305` declaration in the raw body, while the typed view keeps the client's original so the tool seam can still classify it. - `MessagesRequestContext::new` reuses the handler's routing parse, so neither view is built twice and a proxied request never pays for a raw parse it does not use. Native web-search validation folds into the constructor, still ahead of a streaming response committing its status, and `validate_native_web_search_request` drops off the public API. Also removes the single-field `ResolvedCall`/`ResolvedStreamCall` wrappers, which every caller immediately unwrapped, and a redundant deep clone of the assistant turn per round in the non-streaming loop. Note this keeps routing on the full `MessagesRequest` parse, so a body that declares a gateway tool but fails that parse on an unrelated field still falls through to the proxy silently. That behaviour is unchanged here and is worth a separate issue. Test Plan: - `cargo fmt -- --check`, `cargo clippy --all-targets -- -D warnings` and `cargo test --workspace` all clean: 889 passed, 0 failed, cassette replays included. - New `messages_loop_preserves_unmodeled_blocks_and_tool_result_fields_across_rounds` covers `image`, `redacted_thinking`, `citations` and a `tool_result` carrying `is_error` and `cache_control`, asserting they reach upstream unchanged across a gateway round. - Negative control: pointing `upstream_body` at the typed view fails 8 tests, including the existing Claude Code cache_control replays, which confirms the guard is not vacuous. Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Zheng-Lu
requested review from
bbrowning,
franciscojavierarceo,
jiahuei,
leseb,
maralbahari,
noobHappylife,
qandrew and
tjtanaa
as code owners
September 6, 2026 01:53
franciscojavierarceo
left a comment
Collaborator
There was a problem hiding this comment.
the handler builds both views from the same request, and the loops keep forwarding the raw body when appending tool rounds. i didn't find a behavior regression in the refactor. this was a source review; i haven't independently run the tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #137. Follow-up to #131, addressing the code review concern that the Messages loop took a bare
serde_json::Valueat a public API boundary while the handler separately parsed aMessagesRequestfor typed field access.Technical Details
Introduces
MessagesRequestContext { typed: MessagesRequest, raw: Value }, built once per request, and threads it throughrun_messages_loopandrun_messages_streamin place of the untyped body.The raw body stays the thing forwarded upstream, and is deliberately not re-serialized from the typed view.
ContentBlockcatches unmodeled block types in#[serde(other)] Unknownand several variants model only the fields the gateway reads, so a typed round-trip would dropcache_controlandis_errorand collapseimage/redacted_thinkinginto a literal{"type":"unknown"}block.tools(),stream()andmodel()— the fields the loops read and never mutate.messagesandsystemare unreachable through it, so a stale typed view cannot be read back after a round is appended.force_stream,append_round) plus the native web-search budget, replacing the two duplicatedappend_round_to_historycopies with one method.normalize_native_web_searchrewrites the nativeweb_search_20250305declaration in the raw body, while the typed view keeps the client's original so the tool seam can still classify it.MessagesRequestContext::newreuses the handler's routing parse, so neither view is built twice and a proxied request never pays for a raw parse it does not use. Native web-search validation folds into the constructor, still ahead of a streaming response committing its status, andvalidate_native_web_search_requestdrops off the public API.Also removes the single-field
ResolvedCall/ResolvedStreamCallwrappers, which every caller immediately unwrapped, and a redundant deep clone of the assistant turn per round in the non-streaming loop.Note this keeps routing on the full
MessagesRequestparse, so a body that declares a gateway tool but fails that parse on an unrelated field still falls through to the proxy silently. That behaviour is unchanged here and is worth a separate issue.Test Plan
cargo fmt -- --check,cargo clippy --all-targets -- -D warningsandcargo test --workspaceall clean: 889 passed, 0 failed, cassette replays included.messages_loop_preserves_unmodeled_blocks_and_tool_result_fields_across_roundscoversimage,redacted_thinking,citationsand atool_resultcarryingis_errorandcache_control, asserting they reach upstream unchanged across a gateway round.upstream_bodyat the typed view fails 8 tests, including the existing Claude Code cache_control replays, which confirms the guard is not vacuous.