Fix #1564 Add conversations API support #1587

seratch · 2025-08-26T12:16:13Z

This pull request resolves #1564

chatgpt-codex-connector

Codex Review: Here are some suggestions.

[P1] Conversation IDs are never surfaced to callers

[openai_responses.py:140-142] The OpenAIResponsesModel now accepts a conversation_id and passes it to _client.responses.create, but the response’s generated conversation_id is discarded when wrapping the Response object in ModelResponse (lines 140‑143). Only the response_id is returned. When a new conversation is created, callers have no way to retrieve the generated conversation ID to supply on later turns, which makes stored conversations unusable from the SDK. The model should capture response.conversation_id (or equivalent) and return it through ModelResponse/RunResult so that a conversation can be resumed.

Reply with @codex fix comments to fix any unresolved comments.

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, or 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 fix this CI failure" or "@codex address that feedback".

rm-openai

This generally looks good to me. A couple of quick questions in case you've thought about them:

It's possible that the user doesn't want every LLM call or every agent to read from or write to the conversation. e.g. imagine the setup is classify the query -> route to an agent. Maybe you only want the 2nd one to be reading/writing from conversation. How would that be enabled?
This is somewhat tangential but curious if you think the Sessions concept should have a conversations based wrapped?

seratch · 2025-08-27T09:06:06Z

It's possible that the user doesn't want every LLM call or every agent to read from or write to the conversation. e.g. imagine the setup is classify the query -> route to an agent. Maybe you only want the 2nd one to be reading/writing from conversation. How would that be enabled?

If these two runs are separate (i.e., deterministic workflow that just sequentially runs two agents), that's simple. If you mean handoffs, indeed it's not feasible with this feature addition. That's the same for session as well. I am not sure if attaching conversation_id when instantiating an Agent object is a right solution here. Probably not. Then, I don't have any other ideas.

I think the immediate suggestion from us would be you can use conversation_id, previous_response_id, and session per an agent run.

This is somewhat tangential but curious if you think the Sessions concept should have a conversations based wrapped?

Conversations API based sessions implementation is worth considering, and it should be feasible. If a user is fine with frequently calling the API endpoints, it should be an good option. I can explore the implementation tomorrow or later this week.

rm-openai

small comment but otherwise this looks fine to ship as is

src/agents/run.py

pyproject.toml

seratch · 2025-08-28T10:37:59Z

src/agents/memory/session.py

@@ -102,268 +97,3 @@ async def pop_item(self) -> TResponseInputItem | None:
    async def clear_session(self) -> None:
        """Clear all items for this session."""
        ...
-
-
-class SQLiteSession(SessionABC):


moved this to sqlite_session.py

seratch · 2025-08-28T10:38:55Z

src/agents/memory/openai_session.py

+class OpenAISession(SessionABC):
+    def __init__(
+        self,
+        session_id: str | None = None,


use conversation_id as session_id; you can pass the id if you already have, but if you don't, this class creates a new one under the hood

seratch

comments for reviwers

seratch · 2025-08-28T10:58:19Z

examples/memory/sqlalchemy_session_example.py

@@ -20,28 +20,56 @@ async def main():
        create_tables=True,


moved to memory dir and made the code consistent with others

seratch · 2025-08-28T10:59:05Z

src/agents/memory/openai_session.py

+        openai_client: Optional[AsyncOpenAI] = None,
+    ):
+        # this implementation allows to set this value later
+        self.session_id = session_id  # type: ignore


using conversation_id as session_id; you can pass if you already have; otherwise, this class generates a new one under the hood

seratch · 2025-08-28T10:59:36Z

src/agents/memory/sqlite_session.py

+from .session import SessionABC
+
+
+class SQLiteSession(SessionABC):


moved from session.py

rm-openai

this generally looks good but feels like there are too many type:ignores here

rm-openai · 2025-08-28T15:49:09Z

src/agents/memory/openai_session.py

+    return response.id
+
+
+class OpenAISession(SessionABC):


Suggested change

class OpenAISession(SessionABC):

class OpenAIConversationsSession(SessionABC):

rm-openai · 2025-08-28T15:49:22Z

src/agents/memory/openai_session.py

+class OpenAISession(SessionABC):
+    def __init__(
+        self,
+        session_id: Optional[str] = None,


Suggested change

session_id: Optional[str] = None,

conversation_id: str | None = None,

somehow, this fails with python 3.9 tests; I had to use optional in this file

i don't know what's the trigger of the A | B error with python 3.9 but for this one, let me go with Optional[A]. When we drop python 3.9 support, we can make it consistent

You can just add from __future__ import annotations to the top of the file for these

Ah I see. will do

OK, this is done ✅

rm-openai · 2025-08-28T15:49:50Z

src/agents/memory/openai_session.py

+    def __init__(
+        self,
+        session_id: Optional[str] = None,
+        openai_client: Optional[AsyncOpenAI] = None,


Suggested change

openai_client: Optional[AsyncOpenAI] = None,

openai_client: AsyncOpenAI | None = None,

Also feels kinda weird to ask for a client here when in most places we don't require this?

having to create an instance isn't not great when you're using default openai client

forgot to mention other patterns; if you want to pass a client for a certain reason (say, you manage multiple instances for multi tenant apps?), accepting this should be helpful for those.

seratch · 2025-08-28T18:14:26Z

feels like there are too many type:ignores here

yeah, but it was inevitable to call model_dump() to make the returned objects to be serializable

seratch · 2025-08-29T00:25:37Z

@rm-openai renamed the class and resolved most of the type ignores. let me know if you see anything else to change; otherwise, you can merge at any time

Co-authored-by: Rohan Mehta <rm@openai.com>

rm-openai · 2025-08-29T04:23:22Z

src/agents/memory/openai_conversations_session.py

+    def __init__(
+        self,
+        *,
+        session_id: str | None = None,


can you rename to conversation_id please?

rm-openai · 2025-08-29T04:24:32Z

src/agents/memory/openai_conversations_session.py

+        openai_client: AsyncOpenAI | None = None,
+    ):
+        # this implementation allows to set this value later
+        self.session_id = session_id or _EMPTY_SESSION_ID


a better pattern is:

self._session_id: str | None

with

async def _get_session_id(self) -> str: if not self._session_id: self._session_id = await ... return self._session_id

rm-openai · 2025-08-29T04:25:02Z

src/agents/memory/openai_conversations_session.py

+        if _openai_client is None:
+            _openai_client = get_default_openai_client() or AsyncOpenAI()
+        # this never be None here
+        self.openai_client: AsyncOpenAI = _openai_client


generally prefix with _

rm-openai · 2025-08-29T04:26:22Z

src/agents/models/openai_responses.py

+        previous_response_id: str | None = None,  # unused
+        conversation_id: str | None = None,  # unused


seems used to me? line 170/171

ah, ai editor did extra...

seratch

all the comments should be resolved now; if you see anything extra tomorrow, please feel free to modify this branch before merging

seratch · 2025-08-29T05:35:52Z

src/agents/models/openai_responses.py

+        previous_response_id: str | None = None,  # unused
+        conversation_id: str | None = None,  # unused


ah, ai editor did extra...

surfingdev · 2025-09-02T16:51:06Z

This is exactly what I was looking for. I see this is already out but there is no documentation for it right? cc @seratch

seratch requested a review from rm-openai August 26, 2025 12:16

seratch added enhancement New feature or request feature:core labels Aug 26, 2025

chatgpt-codex-connector bot reviewed Aug 26, 2025

View reviewed changes

rm-openai reviewed Aug 26, 2025

View reviewed changes

rm-openai approved these changes Aug 27, 2025

View reviewed changes

src/agents/run.py Outdated Show resolved Hide resolved

seratch commented Aug 28, 2025

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

seratch force-pushed the issue-1564-conversation branch from 4760aff to 977843c Compare August 28, 2025 04:10

seratch commented Aug 28, 2025

View reviewed changes

rm-openai reviewed Aug 28, 2025

View reviewed changes

seratch force-pushed the issue-1564-conversation branch from 1bce3f4 to c531e96 Compare August 29, 2025 00:16

seratch mentioned this pull request Aug 29, 2025

v0.2.10 #1607

Merged

seratch and others added 13 commits August 29, 2025 10:37

Fix #1564 Add conversations API support

e4b7816

fix mypy errors

f54bdb0

Update src/agents/run.py

32e7c57

Co-authored-by: Rohan Mehta <rm@openai.com>

fix lint

c919f6e

Add OpenAISession

be053ff

fix

6b5277f

fix

ab847ff

fix

8577c17

fix

038ea00

fix

1f24ff5

rename

e9fcdd0

fix type ignores

0f1c99d

fix

d9cd39b

seratch force-pushed the issue-1564-conversation branch from 13c9fbd to d9cd39b Compare August 29, 2025 01:37

rm-openai approved these changes Aug 29, 2025

View reviewed changes

rm-openai reviewed Aug 29, 2025

View reviewed changes

fix

c225c55

seratch commented Aug 29, 2025

View reviewed changes

rm-openai merged commit 164acb5 into main Aug 29, 2025
5 checks passed

rm-openai deleted the issue-1564-conversation branch August 29, 2025 14:06

		from .session import SessionABC


		class SQLiteSession(SessionABC):

	class OpenAISession(SessionABC):
	class OpenAIConversationsSession(SessionABC):

	session_id: Optional[str] = None,
	conversation_id: str \| None = None,

	openai_client: Optional[AsyncOpenAI] = None,
	openai_client: AsyncOpenAI \| None = None,

		previous_response_id: str \| None = None, # unused
		conversation_id: str \| None = None, # unused

Fix #1564 Add conversations API support #1587

Fix #1564 Add conversations API support #1587

Uh oh!

Conversation

seratch commented Aug 26, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

Uh oh!

rm-openai left a comment

Choose a reason for hiding this comment

Uh oh!

seratch commented Aug 27, 2025

Uh oh!

rm-openai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rm-openai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch commented Aug 28, 2025

Uh oh!

seratch commented Aug 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

surfingdev commented Sep 2, 2025

Uh oh!

Uh oh!