Repository navigation
feat: add DELETE /v1/responses/{id} and GET /v1/responses/{id}/input_items - #466
Conversation
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: b9a8429d53
ℹ️ 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".
maralbahari
left a comment
maralbahari
left a comment
There was a problem hiding this comment.
Thanks for contributing! I’ve added inline comments with a few concerns and suggestions. Like the Conversations API, these endpoints need cassette recordings to verify OpenAI behavior and gateway parity. If you need help recording the cassettes, please let me know.
| Some(cursor) => { | ||
| items | ||
| .iter() | ||
| .position(|item| item.id == cursor) |
There was a problem hiding this comment.
The cursor lookup uses .position() on the public item ID, which always selects the first occurrence. Public IDs can occur more than once in conversation history.
I reproduced this by creating a conversation containing two occurrences of the same public item ID and generating a response from it. With order=asc&limit=1, the next page returns the same ID with has_more: true. Subsequent requests using that last_id repeat the same page indefinitely.
Please establish OpenAI's cursor behavior for repeated IDs through a reference recording, then implement that behavior and add a regression test proving pagination makes forward progress. The current test only covers unique IDs.
Like the Conversations API, this feature requires OpenAI and gateway cassette recordings to verify behavior. See the Conversations edge-case YAML and the corresponding record_conversations_api_cassettes.sh as examples.
There was a problem hiding this comment.
Code fixed in a31b6d2. A cursor naming a repeated public ID now resumes after its furthest occurrence in the requested order, the same rule item::list_for_conversation applies, so paging always advances. Regression: input_item_pages_make_progress_past_a_repeated_public_id walks both orders with limit=1 over a history containing msg_reused twice. Reverting to .position() makes it fail. Consequence of matching conversations: items between two occurrences are skipped by that cursor. I don't have OpenAI API access to record the reference behavior, so I'm leaving this thread open until someone with access can record it. I can prepare the recorder scenario if that helps.
| pub fn response(id: String) -> Self { | ||
| Self { | ||
| id, | ||
| object: "response".to_string(), |
There was a problem hiding this comment.
The new constructor returns "object": "response", while OpenAI's current DELETE reference specifies "object": "response.deleted".
The integration test and API documentation also assert the current implementation's value, so passing those tests does not establish compatibility.
Please verify the actual response through an OpenAI recording and align the constructor, test assertions, and documentation with the verified contract.
Like the Conversations API, this feature requires OpenAI and gateway cassette recordings to verify behavior. See the Conversations deletion YAML and the corresponding record_conversations_api_cassettes.sh as examples.
There was a problem hiding this comment.
Changed in a31b6d2 to "object": "response.deleted", together with the test assertion and docs/api/index.md. I verified it against OpenAI's published OpenAPI definition (https://github.com/openai/openai-openapi/blob/master/openapi.yaml): deleteResponse declares object as the const response.deleted, and the example body is {"id": "resp_123", "object": "response.deleted", "deleted": true}. That is the spec, not a live recording. I don't have OpenAI API access to record one, so I'm leaving this open for a cassette.
| pub async fn list_input_items( | ||
| State(state): State<AppState>, | ||
| Path(response_id): Path<String>, | ||
| axum::extract::Query(query): axum::extract::Query<super::conversation_items::ListItemsQuery>, |
There was a problem hiding this comment.
OpenAI defines include as an array and documents values beyond message.output_text.logprobs. This route reuses an Option<String> query field and validation that only accepts that one expansion.
I reproduced both failures:
include[]=message.input_image.image_urlreturns 400 with"unsupported include value".- Multiple
include[]parameters return 400 with the plain-text bodyFailed to deserialize query string: duplicate field `include`.
Please look at ListItemsQuery in conversation_items.rs: it already defines include and is reused by this handler. Extend that shared query type to represent supported include values with an enum and accept a collection of them, with structured API errors for malformed queries. At this revision, include is an Option<String> field on a struct, not an enum.
Like the Conversations API, this feature requires OpenAI and gateway cassette recordings to verify behavior. See the Conversations pagination YAML and the corresponding record_conversations_api_cassettes.sh as examples.
There was a problem hiding this comment.
Changed in a31b6d2. ListItemsQuery is parsed from the raw query into typed fields: include: Vec<ItemInclude>, repeatable and accepted as both include and include[]. ItemInclude has the 8 values of OpenAI's IncludeEnum (https://github.com/openai/openai-openapi/blob/master/openapi.yaml). The values stored items already carry are accepted: message.input_image.image_url, computer_call_output.output.image_url, reasoning.encrypted_content and message.output_text.logprobs. Hosted tool results and unknown values return a structured invalid_request 400. A repeated limit/order/after also returns a structured 400 instead of the plain-text deserializer error. Conversation item listing uses the same parser. Tests cover a repeated include[] with image_url, and the 400 bodies. Which hosted-tool expansions should be servable is open to discussion, and recordings are pending as noted on the other threads.
| } | ||
|
|
||
| /// Like [`answering_upstream`], but each turn's output starts with `extra_output`. | ||
| fn answering_upstream_with(extra_output: Value) -> Router { |
There was a problem hiding this comment.
The PR adds mock-based tests but no OpenAI reference cassettes. Its description explicitly derives the inherited-history behavior from the SDK's ResponseItem union. That union describes permissible item shapes; it does not establish which historical occurrences this endpoint returns.
Like the Conversations API, this feature requires OpenAI and gateway cassette recordings to verify history, pagination, include parameters, and deletion behavior, including cached WebSocket continuations. Mock-based tests alone do not establish parity.
There was a problem hiding this comment.
Agreed that mock tests don't establish parity. I don't have OpenAI API access, so I can't produce the OpenAI side of these cassettes. This revision checks the DELETE shape and the include values against OpenAI's published OpenAPI definition, and it adds regressions for the gateway-side behavior: repeated-ID pagination, include arrays, and structured query errors. History contents, cursor semantics for repeated IDs, and WebSocket continuations after deletion still need a reference recording. If a maintainer can run the recorder, I can add the scenarios to a recorder script modeled on record_conversations_api_cassettes.sh for that. I'm leaving this open until then.
There was a problem hiding this comment.
@ENCHIGO I will help you to add those yamls and test your branch.
There was a problem hiding this comment.
Thank you, @maralbahari! The branch is rebased on current main at f5b8d7e. If it helps the recorder, the cases worth capturing are: DELETE body and a continuation after deletion (400 previous_response_not_found), input_items for a previous_response_id chain and for a conversation turn, a history where one public ID repeats with limit=1 paging, and the include values.
aeea461 to
a8f9e38
Compare
|
Rebased onto main (62471db) to resolve the conflicts with #465 and #455.
|
a8f9e38 to
a31b6d2
Compare
a31b6d2 to
f5b8d7e
Compare
Stored responses could be retrieved but not deleted (405), unlike
Conversations, which already support deletion.
Deleting removes the response row only. A response-history item id is
copied into the history_item_ids of every descendant, so items are not
owned by a single response and stay in place. The existing foreign keys
clear responses.previous_response_id and conversations.latest_response_id
references to the deleted row, so descendants stay retrievable and can
still be continued with their full history, while a new request naming the
deleted id as previous_response_id is rejected as not found.
The route shares retrieval's credential check because both act on local
storage. The response body matches the OpenAI deletion object:
{"id": ..., "object": "response", "deleted": true}.
Refs vllm-project#462
Signed-off-by: fengjiaqi <enchiigo@gmail.com>
Lists the context a stored response was generated from: items inherited through previous_response_id or its conversation, including earlier turns' outputs, followed by that turn's input. The response's own output is excluded, identified by the output item IDs in its stored snapshot. This matches the OpenAI listing, whose item type also covers prior outputs. Every stored response already records its full history_item_ids, on both the previous_response_id and the conversation path, so the listing reads that snapshot rather than the live conversation: a turn lists the conversation as it was at that turn, and a deleted parent does not break a child's listing. Pagination, validation and errors follow conversation item listing: limit 1..=100 (default 20), order desc by default, an unknown after cursor is a 404 naming `after`. The shared query validation moves onto ListItemsQuery so both listings use it. Records without a snapshot return 409, as retrieval does. Authentication matches retrieval. Closes vllm-project#462 Signed-off-by: fengjiaqi <enchiigo@gmail.com>
input_items excluded a response's own output by the wire IDs in its snapshot. Output kinds whose wire id is optional (compaction, shell_call) get a generated ID when stored, so they were missing from that set and were listed as input. And since vllm-project#455 a public ID can recur in history, so matching by public ID would also drop an earlier turn's output that shares this turn's ID. Persistence now records the row IDs of the rows it inserts as output in ResponseMetadata.output_item_ids, on both the previous_response_id and conversation paths, and input_items matches rows by row ID. Records written before this fall back to the snapshot's public IDs. Regression tests cover an ID-less compaction and an upstream reusing one message ID across turns; each fails when its half of the fix is removed. Deleting a response does not invalidate a WebSocket session that caches it as its latest checkpoint. Checking storage before reusing a cached durable parent was tried and reverted: the extra await reordered automatic steering successors, and the edge-input-lifecycle/old-parent-before-successor replay failed 5 of 6 runs (6/6 on main and after the revert). Sessions are per connection and may be on another replica, so the exception is documented on ResponseStore::delete and in the API docs. Signed-off-by: fengjiaqi <enchiigo@gmail.com>
- Parse item listing queries once into a typed ItemLimit, ItemOrder and a
repeatable ItemInclude list, shared by conversation item listing and
response input items; malformed queries return structured 400 errors.
- Resume an input_items cursor after the furthest occurrence of a repeated
public ID, as conversation item listing does, so paging always advances.
- Return "object": "response.deleted" from DELETE /v1/responses/{id},
per OpenAI's published OpenAPI definition.
- Serialize new items into NewItem (public ID, data, input/output role) and
return WrittenItem rows that keep that role, deriving output row IDs
from them instead of a parallel boolean array.
- Record output row IDs through the one sanitizing metadata conversion,
now consuming, so persistence clones the metadata once.
Signed-off-by: fengjiaqi <enchiigo@gmail.com>
- Delete the deleted response's history rows that nothing else keeps: rows another response lists, rows that belong to a conversation, and rows a conversation item references stay. - When the response was its conversation's latest, point the conversation at its newest remaining response instead of clearing it, so the next turn keeps the closest stored state. - Report an unknown previous_response_id on the HTTP path as 400 previous_response_not_found, as OpenAI does and the WebSocket path already did. Signed-off-by: fengjiaqi <enchiigo@gmail.com>
f5b8d7e to
fd0cdef
Compare
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
|
@ENCHIGOI recorded OpenAI’s behavior and replayed the cases through the gateway. The recordings exposed gaps in deletion, pagination, tool item IDs/statuses, and error responses. Some failures returned different HTTP statuses, error codes, or error metadata ( For tool item IDs and statuses, some result types did not retain these fields, and conversions could discard them before persistence. This left The larger number of changed files comes partly from closing these gaps: preserving IDs and statuses requires updating the relevant item types, tool-result creation paths, conversions, and their callers. The additional recordings and regression tests also account for a substantial part of the changes. I also simplified the implementation:
Tests now check recorded behavior and replay requests through the gateway with mocked upstream responses. All local tests passed. The gateway tool recordings still need refreshing after the final listing changes before claiming full parity for those cases. For future PRs, please reuse existing types, conversions, and storage operations before introducing new representations or metadata. Preserve identity and status throughout conversions, keep responsibilities in their existing layers, and verify both successful responses and error details against OpenAI recordings. |
|
Thank you, @maralbahari, for recording the OpenAI cases and closing the gaps they exposed. Noted on reusing existing types, conversions and storage operations, and on verifying error details against recordings; I'll follow that in future PRs. I won't push to this branch further unless you ask, so your changes stay as they are. |
## Summary Fixes #479. On PostgreSQL, every stored conversation turn returned 500 before inference ran. The turn calls `conversation::get_or_create`, whose upsert ends in `ON CONFLICT (id) DO UPDATE SET created_at = created_at`. In that clause an unqualified `created_at` could mean the existing row or `EXCLUDED`, so PostgreSQL rejects the statement as ambiguous (SQLSTATE 42702). SQLite accepts it. The update now reads `SET created_at = conversations.created_at`. It is still a no-op whose only job is to make `RETURNING` yield the existing row. `DO NOTHING` would not work here, because it returns no row on conflict. This is the only `ON CONFLICT` in production code. CI's `postgres` job already runs `postgres_storage_integration`, so the new test runs there without any workflow change. ## Test Plan - New `postgres_get_or_create_creates_then_returns_the_existing_conversation` in `postgres_storage_integration.rs`: 1. The first `get_or_create` creates the conversation, and a turn is persisted into it. 2. The second call returns the same conversation and `created_at`. 3. The conversation version (including `latest_response_id`) and its item are unchanged. - **RED** on a7dc7b5 against `postgres:17-alpine`: the first call fails with `42702 column reference "created_at" is ambiguous`. - **GREEN** with the fix. All three of CI's PostgreSQL steps pass locally against `postgres:17-alpine`: - `storage::schema::tests::` (3); - `postgres_storage_integration` (6, including the new test); - `postgres_tenant_isolation_test` (6). - End to end: while reviewing #466 against PostgreSQL, a `POST /v1/responses` turn with `conversation` returned this 500. With the same one-line change, the turn and that PR's conversation listing test passed. - The SQLite `test_conversation_get_or_create_same_id` still passes. - `cargo fmt -- --check`, `cargo clippy --workspace --all-targets -- -D warnings`, the Rust file-size ratchet and `pre-commit run --all-files` are clean. - `cargo test --workspace --no-fail-fast`: 111 test binaries ran, with 2719 tests passing and 1 failing. The failure is `tool::web_fetch::http::tests::the_deadline_bounds_a_slow_origin`, a wall-clock bound in code this PR doesn't touch. It also fails on `main` on my machine when the whole suite runs in parallel, and it passes 3 of 3 runs in isolation. Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
Brings in vllm-project#466 (DELETE /v1/responses/{id} and GET .../input_items; tool outputs carry ids and statuses) and vllm-project#475 (HTTP store:false runs through the executor). Lowered search outputs and replayed gateway calls carry no id or status; replayed search history is checked on vllm-project#466's resolved items; a replayed gateway record resolved to its stored output stays an input record, so store:false replays of stored MCP, web search and multi-agent hosted items still reach the model. Tests updated for vllm-project#466's duplicate-item check and Signed-off-by: haoshan98 <haoshanw@gmail.com> vllm-project#475's unstored conversation turns.
Summary
Adds the two stored-response routes that #462 reports missing. Both act on local storage and use the same authentication layer as
GET /v1/responses/{response_id}.DELETE /v1/responses/{response_id}returns{"id": ..., "object": "response", "deleted": true}.responsesrow is removed. The schema already provides for this:responses.previous_response_idandconversations.latest_response_idare bothON DELETE SET NULL.persist_with_conversation_idcopies the parent'shistory_item_idsinto every child, so a child never reads its parent row.previous_response_idgets 404, and no inference runs.history_item_idsarray, so reclamation is out of scope here (see the questions on AddDELETE /v1/responses/{id}andGET /v1/responses/{id}/input_items#462).GET /v1/responses/{response_id}/input_itemslists the context the response was generated from: items inherited throughprevious_response_idor its conversation, including earlier turns' outputs, followed by this turn's input. The response's own output is excluded, identified by the output item IDs in its stored snapshot.ResponseItem, which covers output items as well as inputs. If you'd rather list this turn's input only, that needs a boundary recorded at persist time, and I'm happy to switch.history_item_idssnapshot, which both theprevious_response_idand conversation paths record. A conversation turn therefore lists the conversation as it was at that turn, and deleting a parent does not break a child's listing.GET /v1/conversations/{id}/items:limit1..=100, default 20orderdefaults todescafterreturns 404 namingafterListItemsQueryso both listings use the same checks.README.mdanddocs/api/index.mddocument both routes.Closes #462
Test Plan
tests/response_input_items_test.rs:previous_response_idchain listsfirst, answer 0, secondand excludes its ownmsg_answer_1; the defaultdescorder is reversed; walkinglimit=1pages withafterreproduces the same sequencelimit,orderorinclude; 404 withparam: afterfor an unknown cursor; 409 for a legacy record; 404 afterDELETEtests/response_retrieval_test.rs: deleting a parent keeps the child retrievable and lets it be continued with full history (the upstream receivesfirst, answer, second, answer, third); continuing from the deleted ID returns 404 with no inference; a repeat delete returns 404; 401 without a key.cargo test --workspace(all 106 test binaries pass),cargo clippy --all-targets -- -D warnings,cargo fmt -- --check, andpre-commit run --all-filesare clean.