Read conversation summaries through thread store#18716
Conversation
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da18087cb2
ℹ️ 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".
| GetConversationSummaryParams::ThreadId { conversation_id } => self | ||
| .thread_store | ||
| .read_thread(StoreReadThreadParams { | ||
| thread_id: conversation_id, | ||
| include_archived: true, | ||
| include_history: false, | ||
| }) | ||
| .await | ||
| { | ||
| Ok(Some(p)) => p, | ||
| _ => { | ||
| let error = JSONRPCErrorError { | ||
| code: INVALID_REQUEST_ERROR_CODE, | ||
| message: format!( | ||
| "no rollout found for conversation id {conversation_id}" | ||
| ), | ||
| data: None, | ||
| }; | ||
| self.outgoing.send_error(request_id, error).await; | ||
| return; | ||
| } | ||
| } | ||
| .map_err(|err| conversation_summary_thread_id_read_error(conversation_id, err)), |
There was a problem hiding this comment.
Guard ThreadId summaries when remote store lacks read_thread
get_thread_summary now always calls thread_store.read_thread for ThreadId requests. In remote mode, RemoteThreadStore::read_thread still returns not_implemented, so getConversationSummary by thread id now fails with an internal error instead of a usable/not-found response. This is a functional regression for configs using experimental_thread_store_endpoint.
Useful? React with 👍 / 👎.
6205503 to
d035ab9
Compare
da18087 to
ec3765e
Compare
ec3765e to
2cf2f43
Compare
Migrate the conversation summary App Server methods to ThreadStore
Because this app server api allows explicitly fetching the thread by rollout path, intercept that case in the app server code and (a) route directly to underlying local thread store methods if we're using a local thread store, or (b) throw an unsupported error if we're using a remote thread store. This keeps the thread store API clean and all filesystem operations inside of the local thread store, which pushing the "fundamental incompatibility" check as early as possible.