fix(responses): accept assistant output_text messages without id/status in input#6599
Conversation
WalkthroughOutputMessage fields id and status are converted from required to optional types. The id field changes from String to Option, and status changes from OutputStatus to Option. Serde attributes enable deserialization when these fields are absent. All construction sites are updated to wrap values in Some(...). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/async-openai/src/types/responses/response.rs (1)
1548-1566:⚠️ Potential issue | 🟡 MinorStale doc comment on
MessageItemshould be updated to reflect optional fields.The
MessageItemdoc comment on lines 184–185 still reads:
OutputMessage: role=assistant, required id & status fieldsThis is now incorrect — both
idandstatusare optional. The comment drives incorrect expectations for future readers trying to understand the disambiguation strategy.📝 Proposed fix
-/// - OutputMessage: role=assistant, required id & status fields +/// - OutputMessage: role=assistant (optional id & status — required in model output, optional in input history)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/async-openai/src/types/responses/response.rs` around lines 1548 - 1566, Update the stale doc comment that describes OutputMessage/MessageItem to reflect that the id and status fields are optional: change any text stating "required id & status fields" or similar to indicate that id: Option<String> and status: Option<OutputStatus> are optional, and adjust phrasing around role (AssistantRole) and content (Vec<OutputMessageContent>) if needed; locate the comment near the OutputMessage struct / MessageItem description and ensure it accurately documents the optionality of id and status so readers understand the disambiguation strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/async-openai/src/types/responses/response.rs`:
- Around line 1548-1566: Update the stale doc comment that describes
OutputMessage/MessageItem to reflect that the id and status fields are optional:
change any text stating "required id & status fields" or similar to indicate
that id: Option<String> and status: Option<OutputStatus> are optional, and
adjust phrasing around role (AssistantRole) and content
(Vec<OutputMessageContent>) if needed; locate the comment near the OutputMessage
struct / MessageItem description and ensure it accurately documents the
optionality of id and status so readers understand the disambiguation strategy.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/async-openai/src/types/responses/response.rslib/llm/src/protocols/openai/responses/mod.rslib/llm/src/protocols/openai/responses/stream_converter.rs
…eserialization Clients replaying assistant turns in conversation history send output_text messages without `id` and `status` fields, which are output metadata not relevant when used as input context. The strict required fields caused 400 deserialization errors (axum Json extractor rejects before handler runs). Make both fields Option<> with serde defaults so: - Assistant messages with output_text content deserialize without id/status - Existing payloads with id/status continue to work (backwards compatible) - All construction sites (stream converter, response builder) still set both Adds serde unit tests for the exact customer reproduction cases. Signed-off-by: Marko Kosec <mkosec@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com>
Signed-off-by: Marko Kosec <mkosec@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com>
ce48b8c to
c1035ff
Compare
The `annotations` field on `OutputTextContent` is required but clients replaying assistant output_text messages as input typically omit it. Add `#[serde(default)]` so it defaults to an empty vec, allowing the OutputMessage deserialization path to succeed. Signed-off-by: Marko Kosec <mkosec@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com>
…cation, text, service_tier) (#6626) Signed-off-by: Vasilis Vagias <vvagias@nvidia.com>
…us in input (#6599) Signed-off-by: Marko Kosec <mkosec@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com> Signed-off-by: Vasilis Vagias <vvagias@nvidia.com> Co-authored-by: vvagias <vasilis.n.vagias@gmail.com> Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com>
…us in input (#6599) (#7049) Signed-off-by: Marko Kosec <mkosec@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com> Signed-off-by: Vasilis Vagias <vvagias@nvidia.com> Co-authored-by: MatejKosec <mkosec@nvidia.com> Co-authored-by: vvagias <vasilis.n.vagias@gmail.com> Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com>
…t messages Two regressions in the Responses API → Chat Completions bridge when a prior turn contains a `function_call` + assistant text + `function_call_output` sequence (emitted by OpenAI Agents SDK, Codex, etc.): 1. Tool-call chain broken by interstitial assistant text. `convert_input_items_to_messages` emitted three separate chat messages (assistant-with-tool-calls, assistant-with-text, tool). Jinja templates that require a tool message to directly follow its assistant tool_call (e.g. MiniMax) then see the interstitial assistant message reset `last_tool_call.name` and reject the payload with "Message has tool role, but there was no previous assistant message with a tool call!". Coalesce adjacent assistant-side items (OutputMessage, FunctionCall, assistant EasyMessage) into a single ChatCompletionRequestAssistantMessage carrying both `content` and `tool_calls`. This matches the Chat Completions spec and lets downstream templates pair tool calls with their outputs. 2. Strict upstream deserialization rejects bare assistant messages. After the async-openai 0.34 upgrade (#7625) the lenient fix from #6599 was dropped: upstream `OutputMessage` requires `id` + `status`, and `OutputTextContent` requires `annotations`. Clients like Codex send `{"type":"message","role":"assistant","content":[{"type":"output_text", "text":"..."}]}` without these, failing with "data did not match any variant of untagged enum InputParam". Add a custom Deserialize on NvCreateResponse that patches these items with synthetic defaults before delegating to the upstream strict types. Tested live against MiniMax-M2.7 on dynamo.frontend: - Codex `codex exec "hello minimax"` round-trips successfully - All three previously-failing repro shapes (simpler, structured w/ id+status, structured w/o id+status) now return 200 Covered by three new unit tests in responses/mod.rs.
…us in input (ai-dynamo#6599) Signed-off-by: Marko Kosec <mkosec@nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com> Signed-off-by: Vasilis Vagias <vvagias@nvidia.com> Co-authored-by: vvagias <vasilis.n.vagias@gmail.com> Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com>
Summary
OutputMessage.idandOutputMessage.statusoptional (Option<>with#[serde(default)]) so clients can replay assistant turns in conversation history without including output metadata fieldsoutput_textcontent in assistant messages withoutidandstatus(these are output-only metadata, not relevant for input context)idandstatusstill deserialize correctlyContext
An external regression report identified that assistant messages with
output_textcontent fail with 400 (InputParam deserialization error) on thenvidia_dynamoendpoint. The root cause is thatOutputMessagerequiresidandstatusas mandatory fields, but clients replaying multi-turn history naturally omit these.The serde untagged enum cascade (
InputItem→Item::Message→MessageItem::Output) fails atOutputMessagedue to missing required fields, then fails all remaining variants, producing a generic deserialization error from axum'sJsonextractor before handler code even runs.Test plan
output_textmessage withoutid/status(issue feat: nixl metadata store and retrieved from etcd #6 repro)output_texthistoryOutputMessagewithidandstatusstill workscargo test -p dynamo-async-openaicargo test -p dynamo-llm(protocol conversion tests)Summary by CodeRabbit