-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Generate more typescript types and return conversation id with ConversationSummary #3219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a good first step, though ultimately I would like to stop sending each EventMsg
as a codex/event/XYZ
and just name things XYZ
as part of the ServerNotification
enum so it's easier to reason about the JSON-RPC protocol:
codex/codex-rs/mcp-server/src/codex_message_processor.rs
Lines 817 to 820 in 17a80d4
// For now, we send a notification for every event, | |
// JSON-serializing the `Event` as-is, but these should | |
// be migrated to be variants of `ServerNotification` | |
// instead. |
Maybe the tuple/serde cleanup can be done in a follow-up just to move things forward here?
use crate::protocol::InputItem; | ||
|
||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] | ||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, TS)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike most of the other things we generate as TypeScript types, I think we need to stick with snake_case
in this file because this has to conform to the OpenAI API.
|
||
/// Response event from the agent | ||
#[derive(Debug, Clone, Deserialize, Serialize, Display)] | ||
#[derive(Debug, Clone, Deserialize, Serialize, Display, TS)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this file, I would favor switching to snakeCase
, but we can do this work in phases?
It looks like about ~7/25 events are not used on the extension side? We could do the thing where, to start, we just send 2x notifications (one in the new format; one in the old format) so that we don't have to do the upgrade as one massive change? |
f12f7e7
to
8f09e9e
Compare
We're trying to migrate from `session_id: Uuid` to `conversation_id: ConversationId`. Not only does this give us more type safety but it unifies our terminology across Codex and with the implementation of session resuming, a conversation (which can span multiple sessions) is more appropriate. I started this impl on #3219 as part of getting resume working in the extension but it's big enough that it should be broken out.
1b947c8
to
14a1796
Compare
14a1796
to
fbffc33
Compare
This PR does multiple things that are necessary for conversation resume to work from the extension. I wanted to make sure everything worked so these changes wound up in one PR: