Skip to content

Conversation

gpeal
Copy link
Collaborator

@gpeal gpeal commented Sep 5, 2025

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:

  1. Generate more ts types
  2. Resume rollout history files rather than create a new one every time it is resumed so you don't see a duplicate conversation in history for every resume. Chatted with @aibrahim-oai to verify this
  3. Return conversation_id in conversation summaries
  4. [Cleanup] Use serde and strong types for a lot of the rollout file parsing

Copy link
Collaborator

@bolinfest bolinfest left a 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:

// 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)]
Copy link
Collaborator

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)]
Copy link
Collaborator

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?

@bolinfest
Copy link
Collaborator

I know you have preferences on which types are mcp wire types vs not. Is this change okay? There would be a massive overhead to clone all of these types to use over the wire if not.

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?

gpeal added a commit that referenced this pull request Sep 8, 2025
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.
@gpeal gpeal force-pushed the gpeal/ts-codegen branch 3 times, most recently from 1b947c8 to 14a1796 Compare September 8, 2025 04:58
@gpeal gpeal merged commit 5eaaf30 into main Sep 8, 2025
18 checks passed
@gpeal gpeal deleted the gpeal/ts-codegen branch September 8, 2025 21:54
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants