server: improve Responses API compliance and Codex CLI compatibility#21174
server: improve Responses API compliance and Codex CLI compatibility#21174krystophny wants to merge 24 commits into
Conversation
|
please add proper testings for it |
530e13c to
467266b
Compare
Done! |
|
According to the responses API reference for streaming events, the It seems like the current implementation just returns a minimal response object in those events. This causes issues with certain spec-compliant client libraries like async-openai. Would it be possible to add the missing streaming event fields here as well? |
- Add sequence_number to ALL streaming events (created, in_progress,
output_item.added, content_part.added, all delta events)
- Add output_index to all events referencing output items
- Add content_index to content-related events
- Populate full response object in response.created and
response.in_progress events (was only {id, object, status})
- Add id field to function_call output_item.added events
- Add status: completed to reasoning output_item.done events
- Counter state persisted across streaming chunks via task_result_state
Fixes: spec-compliant client libraries (async-openai) that require
these fields can now parse all streaming events without error.
Refs: ggml-org#21174 (fumlig review comment)
Code fixes: - build_oai_resp_metadata accepts status param; completed_at is null when status is in_progress (was always set to timestamp) - response.created/in_progress events use zeroed usage (was passing actual prompt tokens before response was logically started) - Function call item IDs are now generated once per tool call in update() and reused consistently across output_item.added, function_call_arguments.delta, and output_item.done events (was generating independent random IDs in each path) - Clean up commented-out status checks in server-common.cpp Test fixes: - Assert sequence_number on every event unconditionally (was using weak "if present" guard) - Check actual values not just key presence in streaming created event test (completed_at is None, usage tokens are 0, etc.) Refs: ggml-org#21174 (patrick review)
|
@fumlig Thanks for the feedback. The streaming events now include the full response object in Tested with the async OpenAI Python SDK (which validates event schemas similarly to async-openai on the Rust side). @ngxson Tests added: 14 pytest tests covering schema fields, streaming compliance, tool type skipping, developer role merging, key stripping, multi-turn input, and output_text consistency. Plus E2E tests with async OpenAI SDK against Qwen3.5-9B. If you'd prefer this split into smaller PRs for faster review, happy to do so. |
|
Additional E2E testing (async OpenAI SDK, Codex CLI, concurrent stress tests, multiple Qwen3.5 models) is documented in the companion meta-repo: https://github.com/krystophny/llama.cpp-dev The meta-repo includes a Nix flake for reproducible test environments and scripted test harnesses under |
Codex CLI compatibility: - Skip non-function tool types (web_search, code_interpreter) - Merge developer/system messages into position 0 for Qwen templates - Strip Responses-only request keys (store, include, prompt_cache_key) - Restore refusal content type handling Responses API compliance (ideas from ggml-org#19720 by riskywindow, adapted): - Add 24 missing Response object fields per OpenAI spec - Fix function_call id/call_id field mapping - Add sequence_number, output_index, content_index to ALL streaming events - Full response object in response.created/in_progress events - Accept input_text type and EasyInputMessage for multi-turn input - output_text convenience field, output_tokens_details 14 pytest tests, E2E tested with async OpenAI SDK and Codex CLI. Refs: ggml-org#19138, ggml-org#19720, ggml-org#21174
|
I'm ok with the current PR, but could you let us know when you are finally ready for review? I have been re-running the CI each time you pushed a new PR, and without CI passed, I cannot merge it |
@ngxson thanks! I marked it as draft and iterate a bit and test locally more with https://github.com/lazy-fortran/fortbench during the coming days and see if any problems pop up on the codex path compared to opencode. Then I'll let you know. |
adef64c to
7229135
Compare
- Add sequence_number to ALL streaming events (created, in_progress,
output_item.added, content_part.added, all delta events)
- Add output_index to all events referencing output items
- Add content_index to content-related events
- Populate full response object in response.created and
response.in_progress events (was only {id, object, status})
- Add id field to function_call output_item.added events
- Add status: completed to reasoning output_item.done events
- Counter state persisted across streaming chunks via task_result_state
Fixes: spec-compliant client libraries (async-openai) that require
these fields can now parse all streaming events without error.
Refs: ggml-org#21174 (fumlig review comment)
Code fixes: - build_oai_resp_metadata accepts status param; completed_at is null when status is in_progress (was always set to timestamp) - response.created/in_progress events use zeroed usage (was passing actual prompt tokens before response was logically started) - Function call item IDs are now generated once per tool call in update() and reused consistently across output_item.added, function_call_arguments.delta, and output_item.done events (was generating independent random IDs in each path) - Clean up commented-out status checks in server-common.cpp Test fixes: - Assert sequence_number on every event unconditionally (was using weak "if present" guard) - Check actual values not just key presence in streaming created event test (completed_at is None, usage tokens are 0, etc.) Refs: ggml-org#21174 (patrick review)
@ngxson I saw no more issues in practical usage with Codex CLI and the smaller Qwen 3.5 models with the responses API. Rebased once more and set ready to review. |
|
Thank you for this!!! I ran into this problem today when I was trying codex with llama.cpp and this change makes it work :) |
|
Nice work, thank you! I tried using it with the Codex IDE extension and almost everything works, but still had a few issues causing errors that would freeze up the entire thread, making it unusable to continue from where it was left off. I'll share some small suggested changes soon. |
|
Fixed a small issue that caused tests to fail. Please approve the workflows once more and provide some final review approval if everything is fine @michaelw9999 . |
@krystophny I am actually using the PR and this is great (plus I've made some other tricks too!) but but I am just a user and contributor, not an admin, so I cannot approve anything... |
根因分析（对比 ai-adapter + llama.cpp PR #21174）： 1. sequence_number 缺失 — 非 delta 事件(response.created/in_progress/ output_item.added/content_part.added/output_text.done/content_part.done/ output_item.done/completed)均缺少 sequence_number 字段。Codex CLI 兼容性要求所有流式事件包含此字段。 2. created → created_at — response 对象中使用 Chat Completions API 的 created 字段名，而 Responses API 规范要求 created_at。 3. content_part.added 多余 item_id — 该事件不应包含 item_id， 应通过 output_index 关联到所属 output item。 附加修复: - 添加 SSE keepalive 心跳(3s 间隔)防止 Codex CLI 超时断开 - function_call_arguments.done 字段 delta → arguments 对齐规范 参考: dyrnq/ai-adapter, ggml-org/llama.cpp#21174, OmniRoute#2599
|
OK sorry for the mess. Now CI checks should pass. |
|
Update once more from upstream/master . This should fix last remaining real CI failure and re-run of checks hopefully one flaky download. |
…ex-compat # Conflicts: # tools/server/server-common.cpp # tools/server/server-task.cpp
yes, I don't think any of the maintainers can review this PR as-is also, there seems to be some unrelated changes included here. please review your PR again before requesting a maintainer's review |
The synchronize-on-teardown fix (src/llama-context.cpp) and the phi3 meta split-state fix (src/llama-model.cpp) entered this branch via an upstream merge and are unrelated to the Responses API work. They are now filed independently as ggml-org#24935 and ggml-org#24936. Remove them here so this PR is limited to the server Responses changes.
Moved two unrelated changes to #24935 and #24936 . Once those land, I will continue slicing the PR to smaller pieces. |
pwilkin
left a comment
There was a problem hiding this comment.
I can't approve the PR as it is because it has a lot of content that I find questionable. While I have no problems with adding fields that are needed for Responses API compliance, I don't understand the behavior here with reporting a lot of native tools, then providing implementations that assume a specific Unix shell environment? I'm not even sure how this is supposed to work because it's completely uncommented, but that's certainly not something that we can merge.
Thank you for taking a look into this. This was indeed nonsense. Sorry about it. I remove it now. |
The Responses-to-chat-completions path synthesized local_shell exec commands (a ripgrep invocation for file_search, a wrapper CLI for web_search) and parsed them back, gated on the X-Llama-Responses-*-Wrapper request headers. This assumed a specific client shell environment and had no test coverage. Remove the bridge end to end: the two wrapper request headers and their internal __responses_*_wrapper fields, the local_shell command builders, the inbound local_shell_call to web/file search parsing, and the shell-output scraping. A local_shell_call is now relayed verbatim for the client to execute. Hosted web_search, file_search and image_generation have no server-side backend, so they are declared to the model only when tool_choice explicitly selects them, and skipped otherwise.
| static std::string responses_custom_tool_description(const json & resp_tool, const std::string & name) { | ||
| std::string description = json_value(resp_tool, "description", std::string("Freeform Codex tool input.")); | ||
|
|
||
| if (name == "apply_patch") { |
There was a problem hiding this comment.
Still see some hardcoded stuff that I'm not sure belongs here. Serving tool descriptions should always be the client's responsibility, not the server's (unless they're server-side tools).
| name = sanitize_tool_name(json_value(resp_tool, "name", std::string("local_shell")), "local_shell"); | ||
| fn = responses_function_tool( | ||
| name, | ||
| "Run a local shell command.", |
There was a problem hiding this comment.
These ones as well, doesn't look like something a server should be handling at all.
| name = sanitize_tool_name(json_value(resp_tool, "name", std::string("update_plan")), "update_plan"); | ||
| fn = responses_function_tool( | ||
| name, | ||
| json_value(resp_tool, "description", std::string("Updates the task plan.")), |
| name = sanitize_tool_name(json_value(resp_tool, "name", std::string("tool_search")), "tool_search"); | ||
| fn = responses_function_tool( | ||
| name, | ||
| json_value(resp_tool, "description", std::string("Search for tools available to this Codex session.")), |
| } | ||
| fn = responses_function_tool( | ||
| name, | ||
| "Search files in the current workspace. Use mode='files' to find paths by name, or mode='content' to search file contents. Prefer this over shell commands such as find.", |
| } | ||
| fn = responses_function_tool( | ||
| name, | ||
| "Generate an image from a prompt.", |
| item.at("call_id").get<std::string>(), | ||
| parse_arguments_best_effort(item.at("arguments")))); | ||
| } else if (exists_and_is_string(item, "type") && | ||
| (item.at("type") == "function_call" || |
There was a problem hiding this comment.
I take it those are actually separate calls, but we should treat them just like all function_call instances, i.e. pass them on without modifications.
| return current_input.substr(previous_input.size()); | ||
| } | ||
|
|
||
| static json build_local_shell_action(const json & args, const std::string & raw_arguments) { |
Drop the server-side fabrication of native tool types (local_shell, custom/apply_patch, web_search, file_search, image_generation, tool_search, task_list): their hardcoded descriptions and JSON schemas, the bash -lc shell synthesis, apply_patch body normalization, and the per-type reconstruction of tool-call items. Non-function Responses tool types are now skipped instead of fabricated; inbound and outbound tool calls relay as plain function_call. Removes the responses_tool_metadata plumbing, an orphaned header helper, and the stale README block on the removed search wrappers.
Stop injecting "[responses recovery: ...]" placeholder text into the converted prompt, and drop the per-item warning log, when an input content part is malformed or of an unknown type. A diagnostic in the prompt only confuses the model, wastes context, and (with Codex replaying full history) gets re-logged every turn. Such parts are now dropped quietly and the request still completes; the tests assert the converted prompt is unchanged.
Drop the "[unsupported Responses item: ...]" and "[unsupported Responses input item: ...]" placeholders injected for unknown or non-object top-level input items; they are skipped quietly, like the content parts. Folds the now-redundant ghost_snapshot special case into the default skip.
|
@pwilkin thanks for the remarks. I removed the tool-related code you flagged:
So the converter only relays |
|
Yeah, this is starting to resemble something merge'able, let's see the test results :) |
Summary
Improve Responses API (
/v1/responses) compliance and Codex CLI compatibility.Response object (non-streaming and streaming):
id/call_idfield mapping (idgets unique fc_ ID,call_idgets the model's tool_call.id)output_texttop-level convenience fieldoutput_tokens_detailswithreasoning_tokensto usagerefusalcontent type handling (was broken in upstream, unreachable code after throw)Streaming events:
sequence_numberto ALL streaming events (created, in_progress, added, delta, done, completed)output_indexto all events referencing output itemscontent_indexto content-related eventsresponse.createdandresponse.in_progress(was only{id, object, status})output_item.added,function_call_arguments.delta, andoutput_item.donetask_result_stateCodex CLI compatibility:
input_texttype alongsideoutput_textfor EasyInputMessage / AssistantMessageItemParamPrior art: #19720 by @riskywindow (stale, 500+ commits behind). This PR incorporates applicable ideas adapted to the current codebase.
Fixes #19138. Related: #20156, #20733, #20607.
Verification
Post-merge local verification (2026-06-18)
Focused failures fixed during the merge:
Passing checks after the fixes:
Local all-CTest without exclusions was not the final gate:
test-backend-opsis a long backend matrix, andtest-quant-type-selectionfailed because live model metadata no longer matched the upstream snapshots. The quant snapshot files are unchanged againstupstream/master.pytest (14 tests, tinyllama2)
E2E with async OpenAI SDK + Qwen3.5-9B Q4_K_M
Codex CLI E2E
Test plan
Requirements
If reviewers prefer, this can be split into smaller PRs for faster review. Let me know.