[app-server] populate thread>turns>items on thread/resume #6848
Conversation
owenlin0
force-pushed
the
owen/return_items_on_thread_resume
branch
3 times, most recently
from
ad05a7f to
4f4d6ff
Compare
|
@codex review this |
![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.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| EventMsg::TokenCount(_) => true, | ||
| EventMsg::EnteredReviewMode(_) => true, | ||
| EventMsg::ExitedReviewMode(_) => true, |
There was a problem hiding this comment.
When a session entered review mode before being persisted, should_persist_event_msg records both EnteredReviewMode and ExitedReviewMode events in the rollout, but ThreadHistoryBuilder::handle_event just returns true for those variants without emitting any ThreadItem. As a result, resuming a thread that contains a completed review will produce thread.turns with the entire review interaction missing, so clients cannot render the prior review turn even though the data was recorded in the rollout. This affects any thread/resume response for threads that used review mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is explicit - we need a bit more thought on how to represent review mode and will handle this as a followup
|
|
||
| /// This function should handle all EventMsg variants that can be persisted in a rollout file. | ||
| /// See `should_persist_event_msg` in `codex-rs/core/rollout/policy.rs`. | ||
| fn handle_event(&mut self, event: &EventMsg) -> bool { |
There was a problem hiding this comment.
Maintaining this bool return value feels like quite a bit of work. I feel like it's mainly for the benefit of the tests internally, so it's not clear to me that it's worth it, but I'll defer to you whether you think it helps ensure correctness of the code.
There was a problem hiding this comment.
yeah feels annoying, removed!
| /// Only populated on a `thread/resume` response for now. For all other | ||
| /// responses and notifications returning a Thread, the turns field | ||
| /// will be an empty list. | ||
| pub turns: Vec<Turn>, |
There was a problem hiding this comment.
I don't think this should live on Thread. We should not feel obligated to load the full list of items for a thread/list request, for example.
Building on #6847, I think this should go on ThreadResumeResponse.
There was a problem hiding this comment.
For turns I think it'd be nice to follow the include pattern in the future, if we wanted to: https://platform.openai.com/docs/api-reference/responses/create#responses_create-include
so by default we don't return Turns on thread/list which I agree we shouldn't do (unless the client specifically asked for it)
|
|
||
| let thread = match read_summary_from_rollout( | ||
| session_configured.rollout_path.as_path(), | ||
| let mut thread = match read_summary_from_rollout( |
owenlin0
force-pushed
the
owen/return_items_on_thread_resume
branch
2 times, most recently
from
b13567e to
7c7ce49
Compare
owenlin0
force-pushed
the
owen/return_items_on_thread_resume
branch
from
554705f to
b776b18
Compare
This PR allows clients to render historical messages when resuming a thread via
thread/resumeby reading from the list ofEventMsgpayloads loaded from the rollout, and then transforming them into Turns and ThreadItems to be returned on theThreadobject.This is implemented by leveraging
SessionConfiguredNotificationwhich returns this list ofEventMsgobjects when resuming a conversation, and then applying a statefulThreadHistoryBuilderthat parses from this EventMsg log and transforms it into Turns and ThreadItems.Note that we only persist a subset of
EventMsgs in a rollout as defined inpolicy.rs, so we lose fidelity whenever we resume a thread compared to when we streamed the thread's turns originally. However, this behavior is at parity with the legacy API.