[codex] Add local thread store listing#17824
Conversation
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-abstraction
branch
from
645e9e5 to
4edef55
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-listing
branch
from
0abeaaf to
9309c3f
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-abstraction
branch
from
4edef55 to
0c6fd51
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-listing
branch
from
9309c3f to
01fa182
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-listing
branch
5 times, most recently
from
0f2240a to
90c87a4
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-listing
branch
from
90c87a4 to
86ff861
Compare
| let state_db_ctx = state_db::get_state_db(config).await; | ||
|
|
||
| if search_term.is_some() | ||
| if search_term.is_none() |
There was a problem hiding this comment.
this early return was only safe for unsearched listing; for searched listing it could return a partially populated SQLite result before the existing rollout repair pass had a chance to backfill older matching threads
| /// Local rollout path when the backing store is filesystem-based. | ||
| pub rollout_path: Option<PathBuf>, |
There was a problem hiding this comment.
It's temporary in the sense that an optional path is necessary in the ThreadStore until we make the path non-required in the AppServer protocol. The app server currently has a non-nullable path field when returning summaries of listed threads, so the ThreadStore sort of has to provide it.
I'm trying to do as much of this migration as possible underneath the current app server shape, but we will eventually need to change that layer too.
Do you have a preferred approach for how to handle that?
There was a problem hiding this comment.
I think keeping it here for a bit is fine especially considering rollout paths are all over app-server.
| /*search_term*/ None, | ||
| ) | ||
| async fn load_recent_threads(sess: &Session) -> Vec<StoredThread> { | ||
| let config = sess.get_config().await; |
There was a problem hiding this comment.
any reason not to toss LocalThreadStore onto sess.services?
There was a problem hiding this comment.
don't think so, can do
| sort_key: codex_rollout::ThreadSortKey, | ||
| ) -> ThreadStoreResult<codex_rollout::ThreadsPage> { | ||
| let page = if params.archived { | ||
| RolloutRecorder::list_archived_threads( |
There was a problem hiding this comment.
maybe worth inlining later separately
There was a problem hiding this comment.
by inline do you mean moving the archived/unarchived distinction into a param on the rollout recorder methods?
There was a problem hiding this comment.
I mean getting rid of RolloutRecorder and having methods be native on LocalThreadStorage.
RolloutRecorder was the previous place where we were abstracting away rollout management.
There was a problem hiding this comment.
ah I see. Yes we can do that over time!
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-listing
branch
3 times, most recently
from
e8c948c to
874e83c
Compare
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-store-listing
branch
from
874e83c to
6676bbd
Compare
2 participants
Builds on top of #17659
Move the filesystem + sqlite thread listing-related operations inside of a local ThreadStore implementation and call ThreadStore from the places that used to perform these filesystem/sqlite operations.
This is the first of a series of PRs that will implement the rest of the local ThreadStore.
Testing: