-
Notifications
You must be signed in to change notification settings - Fork 175
feat(protocols): implement P2 top-level ResponsesRequest fields #1278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,9 +9,7 @@ | |
|
|
||
| use openai_protocol::{ | ||
| chat::{ChatCompletionRequest, ChatCompletionResponse, ChatMessage, MessageContent}, | ||
| common::{ | ||
| FunctionCallResponse, JsonSchemaFormat, ResponseFormat, StreamOptions, ToolCall, UsageInfo, | ||
| }, | ||
| common::{FunctionCallResponse, JsonSchemaFormat, ResponseFormat, ToolCall, UsageInfo}, | ||
| responses::{ | ||
| ResponseContentPart, ResponseInput, ResponseInputOutputItem, ResponseOutputItem, | ||
| ResponseReasoningContent::ReasoningText, ResponseStatus, ResponsesRequest, | ||
|
|
@@ -184,10 +182,15 @@ pub(crate) fn responses_to_chat(req: &ResponsesRequest) -> Result<ChatCompletion | |
| temperature: req.temperature, | ||
| max_completion_tokens: req.max_output_tokens, | ||
| stream: is_streaming, | ||
| // Preserve caller-provided stream_options (e.g. `include_obfuscation: false` | ||
| // on the Responses API) and only default `include_usage` when the caller | ||
| // did not set it. Non-streaming requests intentionally drop stream_options. | ||
| stream_options: if is_streaming { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When building Useful? React with 👍 / 👎. |
||
| Some(StreamOptions { | ||
| include_usage: Some(true), | ||
| }) | ||
| let mut opts = req.stream_options.clone().unwrap_or_default(); | ||
| if opts.include_usage.is_none() { | ||
| opts.include_usage = Some(true); | ||
| } | ||
| Some(opts) | ||
| } else { | ||
| None | ||
| }, | ||
|
|
@@ -372,6 +375,8 @@ pub(crate) fn chat_to_responses( | |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use openai_protocol::common::StreamOptions; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
|
|
@@ -433,4 +438,66 @@ mod tests { | |
| let result = responses_to_chat(&req); | ||
| assert!(result.is_ok()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_stream_options_include_obfuscation_roundtrip() { | ||
| // Regression: ensure caller-provided stream_options (e.g. `include_obfuscation`) | ||
| // are preserved through the Responses → Chat conversion when streaming. | ||
| let req = ResponsesRequest { | ||
| input: ResponseInput::Text("hi".to_string()), | ||
| stream: Some(true), | ||
| stream_options: Some(StreamOptions { | ||
| include_usage: None, | ||
| include_obfuscation: Some(false), | ||
| }), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let chat_req = responses_to_chat(&req).unwrap(); | ||
| assert!(chat_req.stream); | ||
| let opts = chat_req | ||
| .stream_options | ||
| .expect("stream_options populated when streaming"); | ||
| // Caller-provided value is preserved verbatim. | ||
| assert_eq!(opts.include_obfuscation, Some(false)); | ||
| // include_usage defaults to true when absent so downstream consumers | ||
| // still emit the usage block at end-of-stream. | ||
| assert_eq!(opts.include_usage, Some(true)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_stream_options_caller_include_usage_preserved() { | ||
| // Caller-set `include_usage` must not be clobbered by the conversion layer. | ||
| let req = ResponsesRequest { | ||
| input: ResponseInput::Text("hi".to_string()), | ||
| stream: Some(true), | ||
| stream_options: Some(StreamOptions { | ||
| include_usage: Some(false), | ||
| include_obfuscation: Some(true), | ||
| }), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let opts = responses_to_chat(&req).unwrap().stream_options.unwrap(); | ||
| assert_eq!(opts.include_usage, Some(false)); | ||
| assert_eq!(opts.include_obfuscation, Some(true)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_stream_options_non_streaming_dropped() { | ||
| // stream=false must produce None stream_options even if caller set it. | ||
| let req = ResponsesRequest { | ||
| input: ResponseInput::Text("hi".to_string()), | ||
| stream: Some(false), | ||
| stream_options: Some(StreamOptions { | ||
| include_usage: Some(true), | ||
| include_obfuscation: Some(false), | ||
| }), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let chat_req = responses_to_chat(&req).unwrap(); | ||
| assert!(!chat_req.stream); | ||
| assert!(chat_req.stream_options.is_none()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
stream_optionsfield is documented as streaming-only, but no cross-parameter validation was added for Responses requests. As a result, requests withstream: falseandstream_optionsset are accepted and then silently dropped during chat conversion, which differs from the explicit validation behavior already enforced in Chat/Completions. Adding astream_options-requires-streamcheck invalidate_responses_cross_parameterswould prevent this silent no-op.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching. This is legitimate validation hygiene, but cross-parameter validation is deliberately out of P2's schema-only scope — the P-group tasks close wire-format gaps, while semantic validation (including cross-param rules like "stream_options requires stream=true") belongs to a separate validation pass that hasn't been carved as an audit task yet. Filing as a follow-up rather than expanding P2 mid-flight.