Feat/conversion endpoints - #166
Conversation
Signed-off-by: prasanna <prasannajaga9@gmail.com>
Signed-off-by: prasanna <prasannajaga9@gmail.com>
![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: 5ab2fe5446
ℹ️ 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".
| } | ||
| if let Some(conversation_id) = &ctx.conversation_id { | ||
| response.insert("conversation_id".to_owned(), Value::String(conversation_id.clone())); | ||
| response.insert("conversation".to_owned(), Value::String(conversation_id.clone())); |
There was a problem hiding this comment.
For streamed conversation continuations, forwarded non-terminal events such as response.created pass through emit_stream_frame, so this writes response.conversation as a bare string while blocking responses and the terminal stream chunk serialize the same field as an object with an id. Clients that consume response.conversation.id uniformly across streamed events will only break on these forwarded streaming events; use the same {"id": ...} shape here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for contributing to agentic-api! I’ve left a few inline comments covering some of the maintenance practices we follow across the repository. We also use the rust-skills guidelines as a reference for Rust best practices, including addressing Clippy warnings.
Could you please take a look at these items first? Once they’re addressed, I’ll continue reviewing the implementation and logic. If anything in the feedback is unclear, please don’t hesitate to ask. Thanks again for your contribution!
| @@ -1,7 +1,12 @@ | |||
| //! Conversation storage handler — owns all conversation store operations. | |||
|
|
|||
| #![allow(clippy::missing_errors_doc)] | |||
There was a problem hiding this comment.
it's repo policy to not ignore any clippy warning. need to remove this and add error docs.
There was a problem hiding this comment.
please address all clippy warning. I see many of them I tried to add them inline comments but I might have missed some of them.
There was a problem hiding this comment.
checking it! I was mixed with the existing ones little bit I move that cleanly now
| @@ -0,0 +1,125 @@ | |||
| """Conformance checks using the public OpenAI Python Conversations and Responses clients. | |||
There was a problem hiding this comment.
we do not check compatibility between openai and gateway in this format. we have cassettes recorder where we have a listener to record request and responses payload for both conversation and responses api. after recording cassettes from openai and gateway you compare gateway behavior to be compatible with openai not only the logical but also the the SSE lifecycle in case of streaming events.
please refer to https://github.com/vllm-project/agentic-api/blob/main/crates/agentic-server-core/tests/cassettes/README.md
There was a problem hiding this comment.
got it! Ill remove this and follow cassettes order
There was a problem hiding this comment.
The current Python recorder does not cover the conversation-management or conversation-item routes introduced here. Its conv mode only calls POST /v1/conversations and then uses the returned ID with /v1/responses. It never calls:
GET/POST/DELETE /v1/conversations/{conversation_id}GET/POST /v1/conversations/{conversation_id}/itemsGET/DELETE /v1/conversations/{conversation_id}/items/{item_id}
Therefore, both split PRs (one PR conversation/ another conversation/items) need new cassette scenarios.
For the conversation-management PR, record create, retrieve, metadata update, retrieve-after-update, delete, and retrieve-after-delete.
For the items PR, add a conversation_items recorder scenario covering create, list, retrieve, and delete. It must capture generated IDs and reuse them in later requests. Cover all supported item formats, pagination, include, missing IDs, invalid requests, and OpenAI error responses.
Record every scenario twice:
- Against OpenAI as the ground truth.
- Against agentic-api with its database, gateway, and vLLM running.
Use identical requests and prompts for both recordings. Normalize dynamic IDs and timestamps, but compare status codes, response shapes, ordering, pagination, ID relationships, and history behavior.
The following stateful prompt scenarios are especially important.
Items added through the API affect model context
Add this item through /v1/conversations/{conversation_id}/items:
Remember exactly: MANUAL=30.
Then call /v1/responses with the conversation ID:
Using only the conversation history, reply exactly: MANUAL=<value-or-MISSING>
This verifies that an item added through the items API is actually persisted and passed to the model.
Deleting an item changes conversation history
Create two turns:
- Turn 1:
Remember exactly: ALPHA=314159. Reply only OK. - Turn 2:
Remember exactly: BETA=271828. Reply only OK.
List the conversation items, delete the first user message, and then continue with the conversation ID:
Using only the visible conversation history, reply exactly: ALPHA=<value-or-MISSING>;BETA=<value-or-MISSING>
Also ask the same question using the second turn’s previous_response_id.
This records whether OpenAI treats deletion differently for:
- The conversation’s current history.
- A previous-response checkpoint created before the deletion.
The gateway must reproduce OpenAI’s recorded behavior rather than assume both histories behave identically.
Branching after adding an item
Create this history:
- Turn 1:
Remember exactly: ROOT=10. Reply only OK. - Turn 2:
Remember exactly: MAIN=20. Reply only OK. - Directly add through the items endpoint:
Remember exactly: MANUAL=30.
Then ask this question in three ways:
Using only the history visible to this request, reply exactly: ROOT=<value-or-MISSING>;MAIN=<value-or-MISSING>;MANUAL=<value-or-MISSING>
Run it:
- With
previous_response_idfrom turn 1. - With
previous_response_idfrom turn 2. - With the conversation ID.
This determines which earlier turns and manually added items are visible on each branch.
Follow the existing mathematical branching examples in:
conv-multi-turn-single-branch-gpt-4o-nonstreaming.yamlconv-multi-branch-multi-turn-gpt-4o-nonstreaming.yamlrecord_text_only_cassettes.shstateful_conversation_integration.rs
Those tests use ordered arithmetic to make incorrect history obvious—for example, a branch from the response containing 4 should produce 5, not the main branch’s later result.
Add repeatable recording scripts following record_text_only_cassettes.sh and record_web_search_cassettes.sh. The Python recorder must generate the YAML; cassettes must not be written or edited manually. Finally, add Rust integration tests comparing gateway behavior with the OpenAI reference.
| ) | ||
| .route( | ||
| "/v1/conversations/{conversation_id}/items/{item_id}", | ||
| get(retrieve_item).delete(delete_item), |
There was a problem hiding this comment.
I think for each route you can try to open separate PR instead of all in one PR ?
There was a problem hiding this comment.
is it because of the file huge file changes will be hard to track or something we follow as guidelines ?
I would definitely do it in separate PR just curious to know
There was a problem hiding this comment.
I suggest to split this into two focused PRs:
Conversation resource:
GET/POST/DELETE /v1/conversations/{conversation_id}
Only the handler, storage/model changes, tenant handling, tests, and cassettes required by those operations.
Conversation items:
GET/POST /v1/conversations/{conversation_id}/items
GET/DELETE /v1/conversations/{conversation_id}/items/{item_id}
Item serialization, pagination, storage changes, tests, and cassettes.
Changes to POST /v1/conversations should follow the same ownership: metadata/resource behavior belongs with PR 1, while initial-item behavior belongs with PR 2. PR 2 may be stacked on PR 1 if there is a genuine dependency, but each commit/PR should contain only logic needed for its route group.
The reason for splitting is concrete: this PR currently adds 2,861 lines across 41 files and combines two independently testable public API contracts. Separate PRs give each contract its own OpenAI reference recordings, review boundary, and rollback boundary.
Signed-off-by: prasanna <prasannajaga9@gmail.com>
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
These dead_code suppressions show that this fixture is not actually common. Every file in tests/ is compiled as a separate integration-test crate with its own mod common, while storage_backed_state is used only by conversations_test.rs. Move the route-specific state helper next to that test, or introduce a real shared test-support crate; do not hide the unused code from clippy.
| ) | ||
| .route( | ||
| "/v1/conversations/{conversation_id}/items/{item_id}", | ||
| get(retrieve_item).delete(delete_item), |
There was a problem hiding this comment.
I suggest to split this into two focused PRs:
Conversation resource:
GET/POST/DELETE /v1/conversations/{conversation_id}
Only the handler, storage/model changes, tenant handling, tests, and cassettes required by those operations.
Conversation items:
GET/POST /v1/conversations/{conversation_id}/items
GET/DELETE /v1/conversations/{conversation_id}/items/{item_id}
Item serialization, pagination, storage changes, tests, and cassettes.
Changes to POST /v1/conversations should follow the same ownership: metadata/resource behavior belongs with PR 1, while initial-item behavior belongs with PR 2. PR 2 may be stacked on PR 1 if there is a genuine dependency, but each commit/PR should contain only logic needed for its route group.
The reason for splitting is concrete: this PR currently adds 2,861 lines across 41 files and combines two independently testable public API contracts. Separate PRs give each contract its own OpenAI reference recordings, review boundary, and rollback boundary.
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| struct TestDb { |
There was a problem hiding this comment.
This custom temporary database and Drop cleanup are unnecessary. Test storage is already created and migrated with one call:
let pool = create_pool_with_schema(Some("sqlite://?mode=memory")).await?The function is in agentic-server-core/src/storage/pool.rs and is already used this way in executor/request.rs and executor/modes/conversation.rs. Use that pool directly to construct the conversation and response stores.
| /// # Errors | ||
| /// Returns `DbResult::Err` if the database insertion fails. | ||
| pub async fn create(pool: &DbPool, id: &str) -> DbResult<Conversation> { | ||
| create_with_metadata(pool, id, None, None).await |
There was a problem hiding this comment.
create is now only a stale compatibility wrapper around the new function. Since this PR changes creation semantics and all internal call sites are in-tree, change create itself to accept tenant_id and metadata, update the callers, and name the transaction form create_in_tx. Please do not introduce a permanent create_with_metadata/create_with_metadata_in_tx parallel API.
if you find any other such surface API pattern introduced, try to avoid them
| items: Vec<(String, String)>, | ||
| conversation_id: Option<&str>, | ||
| ) -> DbResult<Vec<Item>> { | ||
| create_in_tx_with_tenant(tx, items, conversation_id, None).await |
There was a problem hiding this comment.
Same issue here: add tenant_id to the existing create_in_tx signature and update its call sites. Keeping create_in_tx only to pass None into create_in_tx_with_tenant leaves two creation paths and makes the old path encode an increasingly incorrect default.
| .await | ||
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
avoid the suppression.
| ) -> StoreResult<ConversationItemPage> { | ||
| let pool = self.pool()?; | ||
| self.get_for_tenant(conversation_id, tenant_id).await?; | ||
| let mut rows = item::get_items_by_conversation_for_tenant(pool, conversation_id, tenant_id).await?; |
There was a problem hiding this comment.
limit does not bound database work or memory here. This loads the entire conversation, may reverse it, scans it for the cursor, and then clones a slice of at most 100 rows. A sufficiently long conversation can make one list request allocate its complete history. Push tenant filtering, ordering, the cursor predicate, and LIMIT limit + 1 into SQL; use the extra row to derive has_more.
|
|
||
| impl From<StorageDbItem> for ConversationItemData { | ||
| fn from(row: StorageDbItem) -> Self { | ||
| let mut item = deserialize_from_str_opt::<Value>(&row.data).unwrap_or(Value::Null); |
There was a problem hiding this comment.
Persisted JSON and seq are storage invariants for a conversation item. Converting malformed JSON to null and a missing sequence to 0 turns corruption into a successful but invalid API response. Make this a fallible TryFrom<StorageDbItem> returning StorageError, and propagate failures from append/list/retrieve rather than silently manufacturing values.
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct ConversationItemData { |
There was a problem hiding this comment.
ConversationItemData is being used as a shortcut between the database row and the HTTP response:
idpreserves the database row ID for pagination and retrieval.item: Valuelets the handler inject that ID and remove_agentic_item_kind.created_atandsequencecarry database metadata, although the HTTP handler discards both.
This bypasses the existing typed Item → InOutItem conversion. It also creates two item identities: the ID already inside a typed message/tool/reasoning item and the generated database row ID that later overwrites it.
OpenAI models conversation items as a typed discriminated union, and its list response contains List[ConversationItem], not arbitrary JSON values. Individual returned variants define their typed id, status, and other fields. ConversationItem union, ConversationItemList.
Please remove ConversationItemData. This creates a second, untyped database-to-domain conversion path even though models::Item already maps stored data into the typed InOutItem union.
The current implementation parses the stored payload as an arbitrary Value, removes the private storage marker, and overwrites the item’s existing ID with the database row ID. This can change the identity of model-generated message, function_call, custom_tool_call, and reasoning items. It also allows malformed stored data to escape the business layer without typed validation. created_at is discarded by the HTTP handler, while sequence is a database pagination detail and should not be part of the returned business object.
Please preserve one conversion contract:
models::Item → TryFrom<Item> for InOutItem → typed HTTP serialization
Make the database conversion fallible and propagate StorageError. The append and retrieve operations should return InOutItem, and the list operation should use a typed page containing Vec<InOutItem>, first_id, last_id, and has_more. Keep seq inside the storage query.
If the existing input/output variants cannot represent fields that OpenAI returns—particularly generated id or status on tool outputs—extend those typed variants according to the OpenAI cassettes. Assign the canonical item ID once when the item is stored; do not repair or overwrite arbitrary JSON during reads.
A suitable direction would be:
pub struct ConversationItemPage {
pub data: Vec<InOutItem>,
pub first_id: Option<String>,
pub last_id: Option<String>,
pub has_more: bool,
}The important point is that InOutItem remains the business contract. If it is currently insufficient to serialize the OpenAI conversation-item response, its typed variants should be completed rather than introducing serde_json::Value as an escape hatch.
| @@ -0,0 +1,125 @@ | |||
| """Conformance checks using the public OpenAI Python Conversations and Responses clients. | |||
There was a problem hiding this comment.
The current Python recorder does not cover the conversation-management or conversation-item routes introduced here. Its conv mode only calls POST /v1/conversations and then uses the returned ID with /v1/responses. It never calls:
GET/POST/DELETE /v1/conversations/{conversation_id}GET/POST /v1/conversations/{conversation_id}/itemsGET/DELETE /v1/conversations/{conversation_id}/items/{item_id}
Therefore, both split PRs (one PR conversation/ another conversation/items) need new cassette scenarios.
For the conversation-management PR, record create, retrieve, metadata update, retrieve-after-update, delete, and retrieve-after-delete.
For the items PR, add a conversation_items recorder scenario covering create, list, retrieve, and delete. It must capture generated IDs and reuse them in later requests. Cover all supported item formats, pagination, include, missing IDs, invalid requests, and OpenAI error responses.
Record every scenario twice:
- Against OpenAI as the ground truth.
- Against agentic-api with its database, gateway, and vLLM running.
Use identical requests and prompts for both recordings. Normalize dynamic IDs and timestamps, but compare status codes, response shapes, ordering, pagination, ID relationships, and history behavior.
The following stateful prompt scenarios are especially important.
Items added through the API affect model context
Add this item through /v1/conversations/{conversation_id}/items:
Remember exactly: MANUAL=30.
Then call /v1/responses with the conversation ID:
Using only the conversation history, reply exactly: MANUAL=<value-or-MISSING>
This verifies that an item added through the items API is actually persisted and passed to the model.
Deleting an item changes conversation history
Create two turns:
- Turn 1:
Remember exactly: ALPHA=314159. Reply only OK. - Turn 2:
Remember exactly: BETA=271828. Reply only OK.
List the conversation items, delete the first user message, and then continue with the conversation ID:
Using only the visible conversation history, reply exactly: ALPHA=<value-or-MISSING>;BETA=<value-or-MISSING>
Also ask the same question using the second turn’s previous_response_id.
This records whether OpenAI treats deletion differently for:
- The conversation’s current history.
- A previous-response checkpoint created before the deletion.
The gateway must reproduce OpenAI’s recorded behavior rather than assume both histories behave identically.
Branching after adding an item
Create this history:
- Turn 1:
Remember exactly: ROOT=10. Reply only OK. - Turn 2:
Remember exactly: MAIN=20. Reply only OK. - Directly add through the items endpoint:
Remember exactly: MANUAL=30.
Then ask this question in three ways:
Using only the history visible to this request, reply exactly: ROOT=<value-or-MISSING>;MAIN=<value-or-MISSING>;MANUAL=<value-or-MISSING>
Run it:
- With
previous_response_idfrom turn 1. - With
previous_response_idfrom turn 2. - With the conversation ID.
This determines which earlier turns and manually added items are visible on each branch.
Follow the existing mathematical branching examples in:
conv-multi-turn-single-branch-gpt-4o-nonstreaming.yamlconv-multi-branch-multi-turn-gpt-4o-nonstreaming.yamlrecord_text_only_cassettes.shstateful_conversation_integration.rs
Those tests use ordered arithmetic to make incorrect history obvious—for example, a branch from the response containing 4 should produce 5, not the main branch’s later result.
Add repeatable recording scripts following record_text_only_cassettes.sh and record_web_search_cassettes.sh. The Python recorder must generate the YAML; cassettes must not be written or edited manually. Finally, add Rust integration tests comparing gateway behavior with the OpenAI reference.
|
|
||
| pub async fn conversations(State(state): State<AppState>, req: Request) -> Response { | ||
| const MAX_ITEMS_PER_REQUEST: usize = 20; | ||
|
|
There was a problem hiding this comment.
overall feedback on this file from architectural POV.
Keep storage behind the executor boundary
The HTTP conversation handler currently depends on storage types such as ConversationData, ConversationItemData, and ConversationItemPage. Although it calls conv_handler, that handler passes storage results back to Axum, so the database representation still leaks into the HTTP layer.
Please preserve this layering:
HTTP handler
→ typed Conversation API requests/responses
ConversationHandler in executor/modes/conversation.rs
→ internal storage operations
ConversationStore / database
Define the public payloads under agentic-server-core/src/types/conversations/. Then expose typed operations from ConversationHandler, for example:
pub async fn create(
&self,
tenant_id: Option<&str>,
request: CreateConversationRequest,
) -> ExecutorResult<ConversationResponse>;
pub async fn retrieve(
&self,
tenant_id: Option<&str>,
conversation_id: &str,
) -> ExecutorResult<ConversationResponse>;
pub async fn update(
&self,
tenant_id: Option<&str>,
conversation_id: &str,
request: UpdateConversationRequest,
) -> ExecutorResult<ConversationResponse>;
pub async fn create_items(
&self,
tenant_id: Option<&str>,
conversation_id: &str,
request: CreateItemsRequest,
) -> ExecutorResult<ConversationItemListResponse>;
pub async fn list_items(
&self,
tenant_id: Option<&str>,
conversation_id: &str,
query: ListItemsQuery,
) -> ExecutorResult<ConversationItemListResponse>;ConversationHandler should call ConversationStore, validate requests, convert StorageError into ExecutorError, and map storage results into typed API responses.
The Axum handler should then remain small:
let request: CreateConversationRequest = read_json(body).await?;
match state
.exec_ctx
.conv_handler
.create(tenant_id.as_deref(), request)
.await
{
Ok(response) => Json(response).into_response(),
Err(error) => executor_error_response(error),
}The HTTP layer should not deserialize stored metadata, build responses using json!, manipulate storage markers, or reference database-specific item/page types. This keeps storage private and makes the executor conversation mode the single application boundary.
|
Hey @maralbahari , thanks for the detailed review. I’ll close this PR since it was getting a bit confusing to alter the requested changes, and I’ll create 2 new PRs as we planned. so moving this to draft |
2 participants
Fixes #155 by adding complete stateful Conversations API support and integration coverage.
This change adds support for:
conversationandprevious_response_id.store=falsecorrectly when an existing conversation is provided.conversation_idinput alias.The existing stateful conversation tests remain as regression coverage and now use the standard
conversationrequest field. This is needed so conversations can be managed independently from response generation and reused consistently across all supported transports. It also ensures that persisted conversation data is correctly ordered, isolated by tenant, and compatible with the public OpenAI SDK.Test Plan
Executed the relevant Rust tests covering:
cargo test --workspaceFormatting and lint checks:
OpenAI SDK conformance test:
The SDK test requires the gateway and the configured LLM server to be running.