codex: route thread/read persistence through thread store#18352
codex: route thread/read persistence through thread store#18352wiltzius-openai merged 3 commits intomainfrom
Conversation
![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: 287445efa4
ℹ️ 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".
wiltzius-openai
force-pushed
the
wiltzius/codex/thread-read-store-migration
branch
from
7e4c50f to
b1b11a4
Compare
|
|
||
| fn thread_from_stored_thread( |
There was a problem hiding this comment.
if we decided to go with 2 types, let's do From<StoredThread> for Thread
There was a problem hiding this comment.
I agree with the direction, but a literal impl From<StoredThread> for Thread cannot live in codex-app-server: From is a std trait and both Thread (codex-app-server-protocol) and StoredThread (codex-thread-store) are foreign types to this crate, so the orphan rules reject it. Putting the impl in either defining crate would force a dependency between the protocol and persistence crates that we have been trying to avoid. This helper is intentionally the app-server-local adapter boundary; once it has a second caller, I think the better cleanup is to move it into a small private thread-view module rather than making the protocol or store crate depend on the other.
There was a problem hiding this comment.
lol sorry @aibrahim-oai I had the pr babysitter skill running and codex wrote this itself to respond to comments (I just wanted it to retry the failing CI build)
...but fwiw I think its true
3 participants
Summary
Context