Mcp protocol by aibrahim-oai · Pull Request #1715 · openai/codex

aibrahim-oai · 2025-07-28T22:28:09Z

Add typed MCP protocol surface in codex-rs/mcp-server/src/mcp_protocol.rs for requests, responses, and notifications
Requests: NewConversation, Connect, SendUserMessage, GetConversations
Message content parts: Text, Image (ImageUrl/FileId, optional ImageDetail), File (Url/Id/inline Data)
Responses: ToolCallResponseEnvelope with optional isError and structuredContent variants (NewConversation, Connect, SendUserMessageAccepted, GetConversations)
Notifications: InitialState, ConnectionRevoked, CodexEvent, Cancelled
Uniform _meta on notifications via NotificationMeta (conversationId, requestId)
Unit tests validate JSON wire shapes for key requests/responses/notifications

github-actions · 2025-07-28T22:54:56Z

Summary

Adds mcp_protocol.rs to codex-rs/mcp-server with the full set of request/response/notification structs (incl. serde tags & helpers) plus unit tests, and wires the new module into lib.rs.

Review

Nice, well-structured type definitions and exhaustive tests—build should stay green.

Naming / serde attributes follow the spec, tests cover key paths.
Consider re-exporting the main enums so callers don’t need the full path.
Minor: some TODO-style comments or inline docs would help future maintainers, but not blocking.

View workflow run

bolinfest · 2025-07-28T22:40:03Z

+    pub model: String,
+    pub cwd: String,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub approval_policy: Option<String>,


Why is this a String instead of the enum value?

bolinfest · 2025-07-28T22:40:11Z

+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub approval_policy: Option<String>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub sandbox: Option<String>,


bolinfest · 2025-07-28T22:46:00Z

+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct SendUserMessageArgs {
+    pub conversation_id: Uuid,


I am still inclined to use a u32 instead of a Uuid for this.

FYI, in Rust, there is a thing called the "newtype" idiom: https://doc.rust-lang.org/rust-by-example/generics/new_types.html

In our case, I would be inclined to do:

struct ConversationId(u32)

so that we use the ConversationId throughout and if we want to change how we represent it, we can do it all in one place.

Rust is designed so that there is no extra allocation because this is a "struct:" it just uses the four bytes for the u32.

FWIW I do think UUID is the right call here, even if it's extra space. These will essentially serve as primary keys for sessions stored at clients, right? Feels like a good use of bytes to store and transport a very resilient id format.

bolinfest · 2025-07-28T22:50:17Z

+}
+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct ConnectResult {}


Can you define the result closer to the request? The request is ConnectArgs in this case, right?

I am probably in the minority, but I would favor using Uuid in the request and then "exchanging" it for u32 in the response.

I don't get this part:
I am probably in the minority, but I would favor using Uuid in the request and then "exchanging" it for u32 in the response.

How are we exchanging them from the request to response and why?

Not sure I follow. Can you explain?

bolinfest · 2025-07-28T22:51:22Z

+pub enum ToolCallResponseData {
+    NewConversation(NewConversationResult),
+    Connect(ConnectResult),
+    SendUserMessage(SendUserMessageAccepted),


Can/should we use a consistent suffix for all these names? The others are all Result?

Rust was saying remove any suffix because it's part of an enum.

bolinfest · 2025-07-28T22:53:55Z

+// Notifications
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(tag = "type", content = "data", rename_all = "snake_case")]
+pub enum ConversationNotificationParams {


I think we want one enum for all notifications, not just conversation notifications?

On the client side, I think we want to be able to dispatch based on the type field.

What other types did I miss? most of it is included in CodexEvent

bolinfest · 2025-07-28T22:55:14Z

+            }),
+            reason: "New connect() took over".into(),
+        };
+        let got = match serde_json::to_value(&params) {


Note "observed" is generally the preferred term compared to "got."

bolinfest · 2025-07-28T22:56:01Z

+
+        let got = match serde_json::to_value(&params) {
+            Ok(v) => v,
+            Err(e) => panic!("failed to serialize InitialStateNotificationParams: {e}"),


If you use expect() you can still specify the message but avoid the extra lines from the match.

gpeal · 2025-07-28T23:46:44Z

+    NewConversation(NewConversationArgs),
+    Connect(ConnectArgs),
+    SendUserMessage(SendUserMessageArgs),


I still want to tighten up these names.

The fact that the resource in new is Conversation and follow-up is UserMessage feels makes them feel unrelated but they're very related

Connect is not descriptive enough

how about now?

gpeal · 2025-07-28T23:47:15Z

+    pub conversation_id: Uuid,
+    pub content: Vec<InputMessageContentPart>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub message_id: Option<String>,


This is going to need ~all of the fields from NewConversation (model, cwd, etc). They can be optional and inherit the parent message but they need to be there

gpeal · 2025-07-28T23:50:50Z

+    Data {
+        #[serde(default, skip_serializing_if = "Option::is_none")]
+        filename: Option<String>,
+        file_data: String,


Is this base64? Can you document that? Does this only support PDF? Admittedly, the API is confusing. It keeps referencing PDFs but I'd be shocked if it didn't support other file types.
https://platform.openai.com/docs/guides/pdf-files?api-mode=responses#base64-encoded-files

It's base64. I am not sure if it only supports PDF or what else.

gpeal · 2025-07-28T23:54:17Z

+        file_url: String,
+    },
+    Id {
+        file_id: String,


Maybe document that this is from the files api
https://platform.openai.com/docs/guides/pdf-files?api-mode=responses#uploading-files

gpeal · 2025-07-28T23:56:51Z

+    pub history_log_id: u64,
+    pub history_entry_count: usize,


What would a caller do with the history log id and entry count?

What is the entry count for?

Is there a reason log id would be different than conversation id?

They are currently part of sessionConfigured I can remove them if they aren't useful.

If we can't think of a good use case for it, let's keep it as an implementation detail for now. If we need to add it later, we can

Did you want to remove these?

gpeal · 2025-07-28T23:57:13Z

+}
+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct ConnectResult {}


Not sure I follow. Can you explain?

gpeal · 2025-07-28T23:58:09Z

+#[serde(rename_all = "camelCase")]
+pub struct NotificationMeta {
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub conversation_id: Option<Uuid>,


Since we have RequestId as a type, let's define ConversationId (tbd on uuid vs u32 depending on your conversation with Michael)

gpeal · 2025-07-28T23:58:57Z

+#[serde(tag = "type", content = "data", rename_all = "snake_case")]
+pub enum ConversationNotificationParams {
+    InitialState(InitialStateNotificationParams),
+    ConnectionRevoked(ConnectionRevokedNotificationParams),


What is the difference between ConnectionRevoked and Cancelled? I think you can combine them.

ConnectionRevoked tells a client “the stream for this conversation was taken over by a newer connect(); stop listening to this one.” cancelled should be sent on a cancellation notification.

I think it's okay to send Cancellation for both. Maybe you can add reason to cancellation or something but from the calling side, I think it's okay for them to be the same. @bolinfest wdyt?

Wdyt about this?

I used poor wording. cancellation here is the cancellation that a client sends to the server. there is no response for a cancellation notification.
ConnectionRevoked will be from the server to the client (opposite direction). I don't think the server can send a cancel notification to the client. I renamed it to CancellNotificationParams

dylan-hurd-oai · 2025-07-29T05:00:56Z

+    pub limit: Option<u32>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub cursor: Option<String>,
+}


simple, i like it

dylan-hurd-oai · 2025-07-29T05:01:36Z

+    #[serde(rename = "isError", default, skip_serializing_if = "Option::is_none")]
+    pub is_error: Option<bool>,
+    #[serde(
+        rename = "structuredContent",


non-blocking: why not use rename_all="camelCase" on the struct?

dylan-hurd-oai · 2025-07-29T05:03:06Z

+pub struct InitialStateNotificationParams {
+    #[serde(rename = "_meta", skip_serializing_if = "Option::is_none")]
+    pub meta: Option<NotificationMeta>,
+    pub initial_state: InitialStatePayload,


Why have a nested struct here?

I wanted to define the initial_state on its own.

gpeal · 2025-07-29T00:40:42Z

+    pub approval_policy: Option<codex_core::protocol::AskForApproval>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub sandbox: Option<codex_core::config_types::SandboxMode>,


Suggested change

pub approval_policy: Option<codex_core::protocol::AskForApproval>,

#[serde(default, skip_serializing_if = "Option::is_none")]

pub sandbox: Option<codex_core::config_types::SandboxMode>,

pub approval_policy: Option<AskForApproval>,

#[serde(default, skip_serializing_if = "Option::is_none")]

pub sandbox: Option<SandboxMode>,

gpeal · 2025-07-29T00:40:53Z

+    pub approval_policy: Option<codex_core::protocol::AskForApproval>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub sandbox: Option<codex_core::config_types::SandboxMode>,


Suggested change

pub approval_policy: Option<codex_core::protocol::AskForApproval>,

#[serde(default, skip_serializing_if = "Option::is_none")]

pub sandbox: Option<codex_core::config_types::SandboxMode>,

pub approval_policy: Option<AskForApproval>,

#[serde(default, skip_serializing_if = "Option::is_none")]

pub sandbox: Option<SandboxMode>,

gpeal · 2025-07-29T00:42:18Z

+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(tag = "type", rename_all = "snake_case")]
+pub enum InputMessageContentPart {
+    // following OpenAI's Responses API: https://platform.openai.com/docs/api-reference/responses


Is this comment associated with Text or InputMessageContentPart? If the former, it should be /// and above the derive. 2 / is a regular comment 3, is documentation and will show up in the IDE documentation popup

gpeal · 2025-07-29T00:42:54Z

+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(tag = "type", rename_all = "snake_case")]
+pub enum InputMessageContentPart {


Wdyt about this name?

Suggested change

pub enum InputMessageContentPart {

pub enum MessageInputItem {

gpeal · 2025-07-29T00:43:26Z

+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(untagged)]
+pub enum FileSource {
+    // following OpenAI's Responses API: https://platform.openai.com/docs/guides/pdf-files?api-mode=responses#uploading-files


Same comment as above regarding doc comment + position

gpeal · 2025-07-29T00:43:57Z

+    Id {
+        file_id: String,
+    },
+    Data {


I think this is a bit clearer. Wdyt?

Suggested change

Data {

Base64 {

feels better but a bit misleading because the struct has filename as well. Changed it anyways.

gpeal · 2025-07-29T00:44:40Z

+    pub history_log_id: u64,
+    pub history_entry_count: usize,


Did you want to remove these?

gpeal · 2025-07-29T00:45:21Z

+#[serde(tag = "type", content = "data", rename_all = "snake_case")]
+pub enum ConversationNotificationParams {
+    InitialState(InitialStateNotificationParams),
+    // sent when a second client connects to the same conversation


Suggested change

// sent when a second client connects to the same conversation

// Sent when a second client connects to the same conversation.

nit (for this and for others): For documentation comments, let's use sentence case.

I think you forgot to address this one

gpeal · 2025-07-29T00:45:32Z

+#[serde(tag = "type", content = "data", rename_all = "snake_case")]
+pub enum ConversationNotificationParams {
+    InitialState(InitialStateNotificationParams),
+    ConnectionRevoked(ConnectionRevokedNotificationParams),


Wdyt about this?

dylan-hurd-oai

I like what we're standardizing on! My primary remaining concern is just mixing snake_case and camelCase.

dylan-hurd-oai · 2025-07-29T19:47:04Z

    pub reason: Option<String>,
 }

+/// Strongly-typed notification envelope (no unwraps/expect, no serde_json::Value payloads).


dylan-hurd-oai · 2025-07-29T19:55:41Z

+}
+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct CancellNotificationParams {


typo

Suggested change

pub struct CancellNotificationParams {

pub struct CancelNotificationParams {

dylan-hurd-oai · 2025-07-29T20:02:18Z

+}
+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(tag = "name", content = "arguments", rename_all = "snake_case")]


We seem to use a mix of snake_case and camelCase in this protocol. I'd strongly suggest we standardize on either snake_case or camelCase. I have a light preference for camelCase to match MCP patterns, but it's much more important to use one consistent casing. Using snake_case everywhere would be strictly better than an approach of "camelCase when we can"

gpeal · 2025-07-29T20:25:59Z

+
+// Requests
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct ToolCallRequestEnvelope {


What does Envelope mean here? Why is it not ToolCallRequest?

gpeal · 2025-07-29T20:29:05Z

+    pub conversation_id: ConversationId,
+    pub content: Vec<MessageInputItem>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub message_id: Option<String>,


Should this be parent_message_id and document that, by default, it will continue from the last message but this can be specified to edit a or resume from an earlier message.

Could you define a MessageId struct like you did for ConversationId?

gpeal · 2025-07-29T20:30:29Z

+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(tag = "type", rename_all = "camelCase")]
+pub enum MessageInputItem {
+    /// Following OpenAI's Responses API: https://platform.openai.com/docs/api-reference/responses


This should be above the #derive because this is a doc for MessageInputItem not Text. Same below

gpeal · 2025-07-29T20:32:21Z

+// Responses
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct ToolCallResponseEnvelope {


Same question re: Envelope

gpeal · 2025-07-29T20:33:20Z

+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub struct ConversationSendMessageAccepted {
+    pub accepted: bool,


Suggested change

pub accepted: bool,

pub success: bool,

nit: I think success is a bit more conventional here

gpeal · 2025-07-29T20:33:58Z

+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(tag = "type", rename_all = "camelCase")]
+pub enum ToolCallResponseContent {


Is this used?

it's remaining from previous edits. nice catch.

gpeal · 2025-07-29T20:34:15Z

+#[serde(tag = "type", content = "data", rename_all = "snake_case")]
+pub enum ConversationNotificationParams {
+    InitialState(InitialStateNotificationParams),
+    // sent when a second client connects to the same conversation


I think you forgot to address this one

gpeal · 2025-07-29T20:35:52Z

+}
+
+#[derive(Debug, Clone)]
+pub enum NotificationEnvelope {


Same question re: Envelope

gpeal · 2025-07-29T20:36:08Z

+    CodexEvent(CodexEventNotificationParams),
+}
+
+impl From<ConversationNotificationParams> for NotificationEnvelope {


Why is ConversationNotificationParams prefixed with Conversation but NotificationEnvelope isn't?

added prefix.

gpeal · 2025-07-29T22:09:14Z

+
+use mcp_types::RequestId;
+
+pub type ConversationId = Uuid;


Can you make a struct for ConversationId, too?

Michael may have a preference here. I think we want struct so that it's actually type safe but we certainly want them to be consistent

gpeal · 2025-07-29T22:09:58Z

+    ConversationsList(ConversationsListArgs),
+}
+
+/// Wrap this request in a JSON-RPC request.


Is this the doc for into_request? You have it as the doc for the whole struct impl

gpeal · 2025-07-29T22:56:15Z

+    // Sent when a second client connects to the same conversation.
+    ConnectionRevoked(ConnectionRevokedNotificationParams),
+    CodexEvent(CodexEventNotificationParams),
+    Cancelled(CancelNotificationParams),


Let's split Cancel into a separate enum so we have separate types for server -> client and vice versa

gpeal · 2025-07-29T22:56:49Z

+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct InitialStatePayload {
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]


Suggested change

#[serde(default, skip_serializing_if = "Vec::is_empty")]

#[serde(default)]

Let's not skip this if it's empty so clients can always work with an array

gpeal · 2025-07-29T22:57:16Z

+pub struct CancelNotificationParams {
+    pub request_id: RequestId,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub reason: Option<String>,


Give reason an enum so it's clear what the String options are

discussed in person. We don't need an enum because the client sends it.

gpeal · 2025-07-29T23:01:49Z

+}
+
+#[derive(Debug, Clone)]
+pub enum ConversationNotification {


Why do this and ConversationNotificationParams need to exist? It looks like they're roughly the same

gpeal

🚢🚢🚢🚢🚢🚢🚢🚢🚢

aibrahim-oai added 2 commits July 28, 2025 15:11

initialize types

d585bf5

initialize types

203d4db

aibrahim-oai requested review from bolinfest, dylan-hurd-oai, gpeal and pakrym-oai July 28, 2025 22:28

aibrahim-oai added the code-review Issues relating to code reviews performed by codex label Jul 28, 2025

github-actions Bot added codex-review-in-progress and removed code-review Issues relating to code reviews performed by codex labels Jul 28, 2025

github-actions Bot added codex-review-completed and removed codex-review-in-progress labels Jul 28, 2025

bolinfest requested changes Jul 28, 2025

View reviewed changes

review

dcfe1bb

gpeal reviewed Jul 28, 2025

View reviewed changes

Gabe review

83dd81b

aibrahim-oai requested review from bolinfest and gpeal July 29, 2025 00:37

aibrahim-oai added 5 commits July 28, 2025 17:40

Gabe review

3ca31ac

Merge branch 'main' into mcp-protocol

4daa61e

Gabe review

1a06b0c

Merge branch 'mcp-protocol' of github.com:openai/codex into mcp-protocol

478de10

Gabe review

50d3408

dylan-hurd-oai reviewed Jul 29, 2025

View reviewed changes

gpeal reviewed Jul 29, 2025

View reviewed changes

aibrahim-oai added 3 commits July 29, 2025 10:04

Merge branch 'main' into mcp-protocol

6d62043

review

988e058

Merge branch 'mcp-protocol' of github.com:openai/codex into mcp-protocol

4513f1a

aibrahim-oai added 4 commits July 29, 2025 10:46

Merge branch 'main' into mcp-protocol

f99c948

review

4fdd135

add more tests

00175a8

add more tests

144f805

dylan-hurd-oai reviewed Jul 29, 2025

View reviewed changes

aibrahim-oai added 2 commits July 29, 2025 13:20

dylan

02f56fc

Merge branch 'main' into mcp-protocol

b1c413f

gpeal reviewed Jul 29, 2025

View reviewed changes

aibrahim-oai added 2 commits July 29, 2025 14:22

gab

1c13837

gab

7d2bd6c

aibrahim-oai requested review from dylan-hurd-oai and gpeal July 29, 2025 21:23

gpeal reviewed Jul 29, 2025

View reviewed changes

aibrahim-oai added 3 commits July 29, 2025 16:41

review

d4210de

review

1015860

Merge branch 'main' into mcp-protocol

a4a660d

aibrahim-oai requested a review from gpeal July 30, 2025 00:15

aibrahim-oai added 4 commits July 29, 2025 18:21

remove option on prompt create message

5821d8e

Merge branch 'mcp-protocol' of github.com:openai/codex into mcp-protocol

3e44f55

tests

c3a1040

rename data to result

1366a08

gpeal approved these changes Jul 30, 2025

View reviewed changes

aibrahim-oai merged commit 3823b32 into main Jul 30, 2025
11 checks passed

aibrahim-oai deleted the mcp-protocol branch July 30, 2025 03:14

github-actions Bot locked and limited conversation to collaborators Jul 30, 2025

	pub enum InputMessageContentPart {
	pub enum MessageInputItem {

	// sent when a second client connects to the same conversation
	// Sent when a second client connects to the same conversation.

	pub struct CancellNotificationParams {
	pub struct CancelNotificationParams {

	#[serde(default, skip_serializing_if = "Vec::is_empty")]
	#[serde(default)]

Conversation

aibrahim-oai commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 28, 2025

Summary

Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylan-hurd-oai Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylan-hurd-oai Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylan-hurd-oai Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

aibrahim-oai commented Jul 28, 2025 •

edited

Loading

dylan-hurd-oai Jul 29, 2025 •

edited

Loading

dylan-hurd-oai Jul 29, 2025 •

edited

Loading

dylan-hurd-oai Jul 29, 2025 •

edited

Loading