Repository navigation
Conversation
Since vllm-project#464 an input item with an unmodeled type fails at the request boundary, so a request that changes the reasoning effort with a configuration_update item before the next user message got a 400. InputItem gains a typed ConfigurationUpdate variant. The item keeps its position in the stored input items, in history loaded through previous_response_id or conversation, and in the input sent upstream. The request-level reasoning.effort is not rewritten. RequestPayload::effective_reasoning_effort returns the effort of the last update in the input, or else reasoning.effort. An update must carry a non-empty reasoning.effort and no other reasoning field. Upstream capability checks, adjacent updates and the WebSocket and compaction cases are left for the next changes. Part of vllm-project#331. Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd83ed73a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Json(json!({"id": "resp_upstream", "object": "response", "created_at": 1, "model": "test-model", | ||
| "status": "completed", "usage": {"input_tokens": 3, "output_tokens": 1, "total_tokens": 4}, | ||
| "output": [{"type": "message", "id": format!("msg_answer_{id}"), "role": "assistant", | ||
| "status": "completed", "content": [{"type": "output_text", "text": format!("answer {id}")}]}]})) |
There was a problem hiding this comment.
For a real-provider turn containing a configuration_update and a subsequent previous_response_id continuation, this stub unconditionally returns success, so the new tests cannot detect provider rejection or different update semantics. A repository-wide rg configuration_update finds no recorder scenario or cassette. The cassette rule in .agents/skills/pr-review/SKILL.md, required via AGENTS.md, requires recorded replay coverage at this material wire/continuation boundary. Extend the existing Responses recorder with a stored low-effort turn, a high-effort update before the next user message, and continuation/input-items checks, capturing the actual OpenAI and gateway outcomes and asserting them in replay.
AGENTS.md reference: AGENTS.md:L7-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There's no recording for this yet. The OpenAI side has to be a real capture against a model that supports the item (the reasoning guide names GPT-6 and later), and a gateway capture in front of vLLM would only record vLLM rejecting it, since vLLM main has no configuration_update handling yet; a hand-written cassette wouldn't count. The PR changes only the gateway's own handling: configuration_update_test.rs asserts the exact upstream input on each continuation path, and that a compacting request with an update is rejected before any upstream call. I'll add a recorder scenario once I can run the capture.
franciscojavierarceo
left a comment
franciscojavierarceo
left a comment
There was a problem hiding this comment.
i found one compaction continuation issue below.
| | InputItem::AgentMessage(_) | ||
| | InputItem::McpListTools(_) | ||
| | InputItem::CompactionTrigger | ||
| | InputItem::ConfigurationUpdate(_) |
There was a problem hiding this comment.
configuration_update is accepted on a request that can also compact, but retained_user_window keeps only user messages and the summary. after compaction, the update disappears from the stored window, so a later continuation can lose the effective reasoning effort. we should reject this combination until compaction can preserve the update's position and semantics, or retain it in the compacted window with a regression covering the next turn.
There was a problem hiding this comment.
Reproduced on fd83ed7: with automatic compaction the update was gone from the compacted input, from the stored window and from the next turn, and compaction_trigger and /v1/responses/compact dropped it the same way. I went with rejecting the combination for now, in 6f667b2. When the input or history has a configuration_update, a request that can compact gets a 400 before anything reaches the upstream: a compaction entry in context_management, a compaction_trigger, or multi_agent (each agent compacts automatically). /v1/responses/compact rejects it too, as #331 describes for the reference.
an_update_is_rejected_where_compaction_would_drop_it covers six cases, including a next turn that turns on compaction with the update only in history; all six returned 200 on fd83ed7. an_update_sent_after_compaction_reaches_the_next_turn checks that an update sent after a compaction still reaches the turn after it.
Keeping the update in the compacted window needs more than retained_user_window: the window rules in types/io/input/window.rs hide everything before a checkpoint except retained user messages and open async calls, so the next turn would still lose it. I'd rather do that with the compaction_trigger step of #331.
Compaction keeps user messages and its summary only, so a configuration_update in the compacted input disappeared from the input after compaction, from the stored window and from later turns, and a continuation lost the effective reasoning effort. Until compaction keeps the item, a request that can compact rejects an update in its input or history with a 400 before any upstream call: a context_management compaction entry, a compaction_trigger item, or multi_agent, which compacts each agent automatically. /v1/responses/compact rejects it too. An update sent after a compaction still reaches later turns. Part of vllm-project#331. Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.com>
Summary
Part of #331 (step 1 of the split in my comment there).
Since #464, an input item with an unmodeled
typefails at the request boundary, andconfiguration_updateis not modeled. A request that changes the reasoning effort as the reasoning guide describes, with{"type": "configuration_update", "reasoning": {"effort": "high"}}before the next user message, gets a 400 today, with or withoutprevious_response_id.This PR:
InputItem::ConfigurationUpdatewith a typedConfigurationUpdateItem(optionalid,reasoning.effort).effortmust be a non-empty string. Any other field fails parsing instead of being dropped: newer OpenAI SDKs also allow atextupdate and"effort": null, which stay rejected for now.previous_response_idorconversation, and in the input sent upstream. The request-levelreasoning.effortis not rewritten, and the response'sreasoningstill reports it.RequestPayload::effective_reasoning_effort(): the effort of the last update in the input, history included, or elsereasoning.effort. Nothing acts on it yet; step 2 uses it.InputItemOpenAPI schema (one closed object, like thecode_interpretertool),docs/api/index.mdand the CHANGELOG.compactionentry incontext_management, acompaction_trigger, ormulti_agent(each agent compacts automatically), and at/v1/responses/compact. Compaction keeps only user messages and its summary, so the update would drop out of the input after compaction and out of every later turn (from the review).Not in this PR (steps 2 and 3): checking upstream support before inference, rejecting adjacent updates, WebSocket continuation, and keeping an update through a compaction checkpoint instead of rejecting it. Until then the gateway forwards the item as is; vLLM main does not convert this item type and answers 400 (read in
responses/utils.pyandharmony.py, not run).Test Plan
types::io::input::configuration_updateunit tests: payload and position survive parsing, serialization andmodel_input; eight malformed shapes are rejected; with two updates in the history, the last one is the effective effort while the upstream request keepsreasoning.effortand both items in place.tests/configuration_update_test.rs(HTTP, in-memory SQLite, an upstream that records each request): a stored turn, thenprevious_response_id(the first turn's three input items reach the upstream again unchanged, andinput_itemslists the update in place); a branch from an earlier response does not inherit it;conversationhistory; astore: falsereplay; malformed updates get 400 before the upstream is called.an_update_is_rejected_where_compaction_would_drop_itsends six requests (automatic compaction with the update in the input or only in history,compaction_trigger,multi_agent, and/v1/responses/compactwith either); each gets 400 and the upstream is not called. All six got 200 on fd83ed7.an_update_sent_after_compaction_reaches_the_next_turnsends an update after acompaction_triggerturn and finds it in the next turn's upstream input.openapiunit tests: the fixtures and the strict history schema include the item.cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test -p agentic-server-core -p agentic-server(2937 passed) andpre-commit run --all-filespass on Rust 1.98.1, the pinned toolchain.AI-assisted; reviewed and tested by me.