Add Responses request translation core#702
Conversation
franciscojavierarceo
requested review from
leseb,
shaneutt and
twghu
as code owners
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Review: Add Responses request translation core
Translates OpenAI Responses API create requests into Chat Completions wire format. Clean decomposition with ConvertedContentParts for text-collapse logic and good separation of tool/tool-choice conversion.
| Severity | Count |
|---|---|
| Medium | 3 |
Non-inline findings
[Medium] Test coverage is thin for a translation layer that will carry production traffic. Missing tests for: TranslationError::ExpectedObject (non-object input), string-only input, single-object input (not wrapped in array), empty instructions (should be skipped), tool_choice as a typed object ({"type": "function", "name": "foo"}), non-function tools being dropped, file_id fallback in image parts, empty tools array after all are filtered, and no-input-no-instructions producing an empty messages array. The two existing tests cover the happy path well but do not exercise any error or edge-case branches.
| let mut mapped = Vec::new(); | ||
|
|
||
| for tool in tools { | ||
| let Some(tool_obj) = tool.as_object() else { |
There was a problem hiding this comment.
[Medium] file_id is an opaque identifier (e.g. file-abc123), not a URL. Falling back to it for the image_url.url field produces an invalid Chat Completions image part that downstream providers will reject. Either skip this fallback (with a warn!) or translate file_id into the appropriate provider-specific URL if that is the intent.
| /// Convert one `Responses` function tool into Chat Completions function shape. | ||
| fn convert_function_tool(tool: &Map<String, Value>) -> Value { | ||
| let mut function = Map::new(); | ||
| copy_field(tool, &mut function, "name"); |
There was a problem hiding this comment.
[Medium] When every tool in the array is non-function (e.g. code_interpreter, file_search), this returns Some(Value::Array([])), inserting an empty "tools": [] into the Chat Completions request. Some providers reject empty tool arrays. Return None when mapped is empty:
if mapped.is_empty() {
return None;
}| Value::Object(_) => append_input_item(messages, input), | ||
| _ => {}, | ||
| } | ||
| } |
There was a problem hiding this comment.
[Medium] When input is an unexpected JSON type (number, bool, null), it is silently discarded. This is inconsistent with build_chat_tools and build_chat_tool_choice, which both warn! on unsupported variants. Add a warn! on the catch-all arm for parity and to make debugging easier when a caller sends a malformed request.
franciscojavierarceo
force-pushed
the
codex/issue-213-chat-translation-core
branch
from
77e19aa to
6568916
Compare
| @@ -0,0 +1,265 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
should this be in filter/src/builtins/http/ai/openai/translation/ instead?
| copy_field(obj, &mut chat, "model"); | ||
| copy_field(obj, &mut chat, "stream"); | ||
| copy_field(obj, &mut chat, "temperature"); | ||
| copy_field(obj, &mut chat, "top_p"); |
There was a problem hiding this comment.
copy more fields like frequency_penalty, presence_penalty, parallel_tool_calls, service_tier, reasoning_effort and top_logprobs?
| /// Push one Responses content part. | ||
| fn push(&mut self, part: &Value) { | ||
| match part.get("type").and_then(Value::as_str) { | ||
| Some("input_text" | "output_text" | "text") => self.push_text(part), |
| let mut function = Map::new(); | ||
| copy_field(tool, &mut function, "name"); | ||
| copy_field(tool, &mut function, "description"); | ||
| copy_field(tool, &mut function, "parameters"); |
| } | ||
|
|
||
| /// Convert simple `Responses` tool-choice values into Chat Completions shape. | ||
| fn build_chat_tool_choice(obj: &Map<String, Value>) -> Option<Value> { |
| } | ||
|
|
||
| /// Convert a single `Responses` input item into one Chat Completions message. | ||
| fn append_input_item(messages: &mut Vec<Value>, item: &Value) { |
There was a problem hiding this comment.
function_call_output input items have call_id and output, not role and content, so this path emits {"role":"user","content":""}.
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
franciscojavierarceo
force-pushed
the
codex/issue-213-chat-translation-core
branch
from
6568916 to
4eb496c
Compare
|
PR too large: 967 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add |
4 participants
Part of #213.
Summary
Stack
Validation