From 7c26c8e091851957d58da7b82f390a3de9bc7e80 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 15 Aug 2025 13:55:44 -0400 Subject: [PATCH 1/6] tui: skip identical consecutive entries in local composer history (#2352) This PR avoids inserting duplicate consecutive messages into the Chat Composer's local history. --- .../src/bottom_pane/chat_composer_history.rs | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs index 93accfcd1a..04b745d1ff 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -55,11 +55,18 @@ impl ChatComposerHistory { /// Record a message submitted by the user in the current session so it can /// be recalled later. pub fn record_local_submission(&mut self, text: &str) { - if !text.is_empty() { - self.local_history.push(text.to_string()); - self.history_cursor = None; - self.last_history_text = None; + if text.is_empty() { + return; } + + // Avoid inserting a duplicate if identical to the previous entry. + if self.local_history.last().is_some_and(|prev| prev == text) { + return; + } + + self.local_history.push(text.to_string()); + self.history_cursor = None; + self.last_history_text = None; } /// Should Up/Down key presses be interpreted as history navigation given @@ -187,6 +194,29 @@ mod tests { use codex_core::protocol::Op; use std::sync::mpsc::channel; + #[test] + fn duplicate_submissions_are_not_recorded() { + let mut history = ChatComposerHistory::new(); + + // Empty submissions are ignored. + history.record_local_submission(""); + assert_eq!(history.local_history.len(), 0); + + // First entry is recorded. + history.record_local_submission("hello"); + assert_eq!(history.local_history.len(), 1); + assert_eq!(history.local_history.last().unwrap(), "hello"); + + // Identical consecutive entry is skipped. + history.record_local_submission("hello"); + assert_eq!(history.local_history.len(), 1); + + // Different entry is recorded. + history.record_local_submission("world"); + assert_eq!(history.local_history.len(), 2); + assert_eq!(history.local_history.last().unwrap(), "world"); + } + #[test] fn navigation_with_async_fetch() { let (tx, rx) = channel::(); From d262244725282e30a6b8537c257a73394d74461a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 15 Aug 2025 12:44:40 -0700 Subject: [PATCH 2/6] fix: introduce codex-protocol crate (#2355) --- codex-rs/Cargo.lock | 15 ++++++++ codex-rs/Cargo.toml | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/codex.rs | 20 ++++++++-- codex-rs/core/src/config_types.rs | 24 ++++++++++++ codex-rs/core/src/lib.rs | 7 +++- codex-rs/core/src/models.rs | 1 + codex-rs/core/src/parse_command.rs | 18 +++++++++ codex-rs/core/src/plan_tool.rs | 31 +++------------ codex-rs/mcp-server/src/wire_format.rs | 4 +- .../tests/codex_message_processor_flow.rs | 4 +- codex-rs/protocol/Cargo.toml | 20 ++++++++++ codex-rs/protocol/README.md | 7 ++++ codex-rs/protocol/src/config_types.rs | 31 +++++++++++++++ codex-rs/protocol/src/lib.rs | 5 +++ codex-rs/protocol/src/message_history.rs | 9 +++++ codex-rs/protocol/src/parse_command.rs | 38 +++++++++++++++++++ codex-rs/protocol/src/plan_tool.rs | 26 +++++++++++++ codex-rs/{core => protocol}/src/protocol.rs | 2 +- codex-rs/tui/Cargo.toml | 1 + codex-rs/tui/src/chatwidget.rs | 2 +- codex-rs/tui/src/chatwidget/tests.rs | 18 ++++++--- codex-rs/tui/src/history_cell.rs | 2 +- 23 files changed, 244 insertions(+), 43 deletions(-) create mode 100644 codex-rs/protocol/Cargo.toml create mode 100644 codex-rs/protocol/README.md create mode 100644 codex-rs/protocol/src/config_types.rs create mode 100644 codex-rs/protocol/src/lib.rs create mode 100644 codex-rs/protocol/src/message_history.rs create mode 100644 codex-rs/protocol/src/parse_command.rs create mode 100644 codex-rs/protocol/src/plan_tool.rs rename codex-rs/{core => protocol}/src/protocol.rs (99%) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a0dd913374..4eefc375ac 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -695,6 +695,7 @@ dependencies = [ "codex-apply-patch", "codex-login", "codex-mcp-client", + "codex-protocol", "core_test_support", "dirs", "env-flags", @@ -889,6 +890,19 @@ dependencies = [ "wiremock", ] +[[package]] +name = "codex-protocol" +version = "0.0.0" +dependencies = [ + "mcp-types", + "serde", + "serde_bytes", + "serde_json", + "strum 0.27.2", + "strum_macros 0.27.2", + "uuid", +] + [[package]] name = "codex-tui" version = "0.0.0" @@ -904,6 +918,7 @@ dependencies = [ "codex-file-search", "codex-login", "codex-ollama", + "codex-protocol", "color-eyre", "crossterm", "diffy", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 0ed8852228..2fb9b9271e 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -15,6 +15,7 @@ members = [ "mcp-server", "mcp-types", "ollama", + "protocol", "tui", ] resolver = "2" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 3fb99d1f7f..74eaf6704b 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -19,6 +19,7 @@ chrono = { version = "0.4", features = ["serde"] } codex-apply-patch = { path = "../apply-patch" } codex-login = { path = "../login" } codex-mcp-client = { path = "../mcp-client" } +codex-protocol = { path = "../protocol" } dirs = "6" env-flags = "0.1.1" eventsource-stream = "0.2.3" diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index c683f187eb..020acd045e 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -677,7 +677,10 @@ impl Session { call_id, command: command_for_display.clone(), cwd, - parsed_cmd: parse_command(&command_for_display), + parsed_cmd: parse_command(&command_for_display) + .into_iter() + .map(Into::into) + .collect(), }), }; let event = Event { @@ -1031,8 +1034,8 @@ async fn submission_loop( Arc::new(per_turn_config), None, provider, - effort, - summary, + effort.into(), + summary.into(), sess.session_id, ); @@ -1102,7 +1105,13 @@ async fn submission_loop( crate::protocol::GetHistoryEntryResponseEvent { offset, log_id, - entry: entry_opt, + entry: entry_opt.map(|e| { + codex_protocol::message_history::HistoryEntry { + session_id: e.session_id, + ts: e.ts, + text: e.text, + } + }), }, ), }; @@ -1160,6 +1169,9 @@ async fn submission_loop( } break; } + _ => { + // Ignore unknown ops; enum is non_exhaustive to allow extensions. + } } } debug!("Agent loop exited"); diff --git a/codex-rs/core/src/config_types.rs b/codex-rs/core/src/config_types.rs index cbbc6b4923..bb61f820d3 100644 --- a/codex-rs/core/src/config_types.rs +++ b/codex-rs/core/src/config_types.rs @@ -228,3 +228,27 @@ pub enum ReasoningSummary { /// Option to disable reasoning summaries. None, } + +// Conversions from protocol enums to core config enums used where protocol +// values are supplied by clients and core needs its internal representations. +impl From for ReasoningEffort { + fn from(v: codex_protocol::config_types::ReasoningEffort) -> Self { + match v { + codex_protocol::config_types::ReasoningEffort::Low => ReasoningEffort::Low, + codex_protocol::config_types::ReasoningEffort::Medium => ReasoningEffort::Medium, + codex_protocol::config_types::ReasoningEffort::High => ReasoningEffort::High, + codex_protocol::config_types::ReasoningEffort::None => ReasoningEffort::None, + } + } +} + +impl From for ReasoningSummary { + fn from(v: codex_protocol::config_types::ReasoningSummary) -> Self { + match v { + codex_protocol::config_types::ReasoningSummary::Auto => ReasoningSummary::Auto, + codex_protocol::config_types::ReasoningSummary::Concise => ReasoningSummary::Concise, + codex_protocol::config_types::ReasoningSummary::Detailed => ReasoningSummary::Detailed, + codex_protocol::config_types::ReasoningSummary::None => ReasoningSummary::None, + } + } +} diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index e895377b74..28d35f5376 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -44,7 +44,6 @@ mod openai_model_info; mod openai_tools; pub mod plan_tool; mod project_doc; -pub mod protocol; mod rollout; pub(crate) mod safety; pub mod seatbelt; @@ -56,3 +55,9 @@ mod user_notification; pub mod util; pub use apply_patch::CODEX_APPLY_PATCH_ARG1; pub use safety::get_platform_sandbox; +// Re-export the protocol types from the standalone `codex-protocol` crate so existing +// `codex_core::protocol::...` references continue to work across the workspace. +pub use codex_protocol::protocol; +// Re-export protocol config enums to ensure call sites can use the same types +// as those in the protocol crate when constructing protocol messages. +pub use codex_protocol::config_types as protocol_config_types; diff --git a/codex-rs/core/src/models.rs b/codex-rs/core/src/models.rs index 5e67fc381a..f6323e2724 100644 --- a/codex-rs/core/src/models.rs +++ b/codex-rs/core/src/models.rs @@ -183,6 +183,7 @@ impl From> for ResponseInputItem { None } }, + _ => None, }) .collect::>(), } diff --git a/codex-rs/core/src/parse_command.rs b/codex-rs/core/src/parse_command.rs index 6436ce4039..6ea0626814 100644 --- a/codex-rs/core/src/parse_command.rs +++ b/codex-rs/core/src/parse_command.rs @@ -41,6 +41,24 @@ pub enum ParsedCommand { }, } +// Convert core's parsed command enum into the protocol's simplified type so +// events can carry the canonical representation across process boundaries. +impl From for codex_protocol::parse_command::ParsedCommand { + fn from(v: ParsedCommand) -> Self { + use codex_protocol::parse_command::ParsedCommand as P; + match v { + ParsedCommand::Read { cmd, name } => P::Read { cmd, name }, + ParsedCommand::ListFiles { cmd, path } => P::ListFiles { cmd, path }, + ParsedCommand::Search { cmd, query, path } => P::Search { cmd, query, path }, + ParsedCommand::Format { cmd, tool, targets } => P::Format { cmd, tool, targets }, + ParsedCommand::Test { cmd } => P::Test { cmd }, + ParsedCommand::Lint { cmd, tool, targets } => P::Lint { cmd, tool, targets }, + ParsedCommand::Noop { cmd } => P::Noop { cmd }, + ParsedCommand::Unknown { cmd } => P::Unknown { cmd }, + } + } +} + fn shlex_join(tokens: &[String]) -> String { shlex_try_join(tokens.iter().map(|s| s.as_str())) .unwrap_or_else(|_| "".to_string()) diff --git a/codex-rs/core/src/plan_tool.rs b/codex-rs/core/src/plan_tool.rs index 9e81abcd88..bc39e4f6ce 100644 --- a/codex-rs/core/src/plan_tool.rs +++ b/codex-rs/core/src/plan_tool.rs @@ -1,9 +1,6 @@ use std::collections::BTreeMap; use std::sync::LazyLock; -use serde::Deserialize; -use serde::Serialize; - use crate::codex::Session; use crate::models::FunctionCallOutputPayload; use crate::models::ResponseInputItem; @@ -13,29 +10,13 @@ use crate::openai_tools::ResponsesApiTool; use crate::protocol::Event; use crate::protocol::EventMsg; +// Use the canonical plan tool types from the protocol crate to ensure +// type-identity matches events transported via `codex_protocol`. +pub use codex_protocol::plan_tool::PlanItemArg; +pub use codex_protocol::plan_tool::StepStatus; +pub use codex_protocol::plan_tool::UpdatePlanArgs; + // Types for the TODO tool arguments matching codex-vscode/todo-mcp/src/main.rs -#[derive(Debug, Clone, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -pub enum StepStatus { - Pending, - InProgress, - Completed, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -#[serde(deny_unknown_fields)] -pub struct PlanItemArg { - pub step: String, - pub status: StepStatus, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -#[serde(deny_unknown_fields)] -pub struct UpdatePlanArgs { - #[serde(default)] - pub explanation: Option, - pub plan: Vec, -} pub(crate) static PLAN_TOOL: LazyLock = LazyLock::new(|| { let mut plan_item_props = BTreeMap::new(); diff --git a/codex-rs/mcp-server/src/wire_format.rs b/codex-rs/mcp-server/src/wire_format.rs index 68d9aeb9eb..2dca1b79b7 100644 --- a/codex-rs/mcp-server/src/wire_format.rs +++ b/codex-rs/mcp-server/src/wire_format.rs @@ -2,12 +2,12 @@ use std::collections::HashMap; use std::fmt::Display; use std::path::PathBuf; -use codex_core::config_types::ReasoningEffort; -use codex_core::config_types::ReasoningSummary; use codex_core::protocol::AskForApproval; use codex_core::protocol::FileChange; use codex_core::protocol::ReviewDecision; use codex_core::protocol::SandboxPolicy; +use codex_core::protocol_config_types::ReasoningEffort; +use codex_core::protocol_config_types::ReasoningSummary; use mcp_types::RequestId; use serde::Deserialize; use serde::Serialize; diff --git a/codex-rs/mcp-server/tests/codex_message_processor_flow.rs b/codex-rs/mcp-server/tests/codex_message_processor_flow.rs index e0c7a83209..5b89f3fe42 100644 --- a/codex-rs/mcp-server/tests/codex_message_processor_flow.rs +++ b/codex-rs/mcp-server/tests/codex_message_processor_flow.rs @@ -1,9 +1,9 @@ use std::path::Path; -use codex_core::config_types::ReasoningEffort; -use codex_core::config_types::ReasoningSummary; use codex_core::protocol::AskForApproval; use codex_core::protocol::SandboxPolicy; +use codex_core::protocol_config_types::ReasoningEffort; +use codex_core::protocol_config_types::ReasoningSummary; use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_mcp_server::wire_format::AddConversationListenerParams; use codex_mcp_server::wire_format::AddConversationSubscriptionResponse; diff --git a/codex-rs/protocol/Cargo.toml b/codex-rs/protocol/Cargo.toml new file mode 100644 index 0000000000..43c5eac8f1 --- /dev/null +++ b/codex-rs/protocol/Cargo.toml @@ -0,0 +1,20 @@ +[package] +edition = "2024" +name = "codex-protocol" +version = { workspace = true } + +[lib] +name = "codex_protocol" +path = "src/lib.rs" + +[lints] +workspace = true + +[dependencies] +mcp-types = { path = "../mcp-types" } +serde = { version = "1", features = ["derive"] } +serde_bytes = "0.11" +serde_json = "1" +strum = "0.27.2" +strum_macros = "0.27.2" +uuid = { version = "1", features = ["serde", "v4"] } diff --git a/codex-rs/protocol/README.md b/codex-rs/protocol/README.md new file mode 100644 index 0000000000..384d0b4859 --- /dev/null +++ b/codex-rs/protocol/README.md @@ -0,0 +1,7 @@ +# codex-protocol + +This crate defines the "types" for the protocol used by Codex CLI, which includes both "internal types" for communication between `codex-core` and `codex-tui`, as well as "external types" used with `codex mcp`. + +This crate should have minimal dependencies. + +Ideally, we should avoid "material business logic" in this crate, as we can always introduce `Ext`-style traits to add functionality to types in other crates. diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs new file mode 100644 index 0000000000..b4525d79ec --- /dev/null +++ b/codex-rs/protocol/src/config_types.rs @@ -0,0 +1,31 @@ +use serde::Deserialize; +use serde::Serialize; +use strum_macros::Display; + +/// See https://platform.openai.com/docs/guides/reasoning?api-mode=responses#get-started-with-reasoning +#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display)] +#[serde(rename_all = "lowercase")] +#[strum(serialize_all = "lowercase")] +pub enum ReasoningEffort { + Low, + #[default] + Medium, + High, + /// Option to disable reasoning. + None, +} + +/// A summary of the reasoning performed by the model. This can be useful for +/// debugging and understanding the model's reasoning process. +/// See https://platform.openai.com/docs/guides/reasoning?api-mode=responses#reasoning-summaries +#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display)] +#[serde(rename_all = "lowercase")] +#[strum(serialize_all = "lowercase")] +pub enum ReasoningSummary { + #[default] + Auto, + Concise, + Detailed, + /// Option to disable reasoning summaries. + None, +} diff --git a/codex-rs/protocol/src/lib.rs b/codex-rs/protocol/src/lib.rs new file mode 100644 index 0000000000..ec6a4195d9 --- /dev/null +++ b/codex-rs/protocol/src/lib.rs @@ -0,0 +1,5 @@ +pub mod config_types; +pub mod message_history; +pub mod parse_command; +pub mod plan_tool; +pub mod protocol; diff --git a/codex-rs/protocol/src/message_history.rs b/codex-rs/protocol/src/message_history.rs new file mode 100644 index 0000000000..3a561df7bf --- /dev/null +++ b/codex-rs/protocol/src/message_history.rs @@ -0,0 +1,9 @@ +use serde::Deserialize; +use serde::Serialize; + +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct HistoryEntry { + pub session_id: String, + pub ts: u64, + pub text: String, +} diff --git a/codex-rs/protocol/src/parse_command.rs b/codex-rs/protocol/src/parse_command.rs new file mode 100644 index 0000000000..495562a765 --- /dev/null +++ b/codex-rs/protocol/src/parse_command.rs @@ -0,0 +1,38 @@ +use serde::Deserialize; +use serde::Serialize; + +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +pub enum ParsedCommand { + Read { + cmd: String, + name: String, + }, + ListFiles { + cmd: String, + path: Option, + }, + Search { + cmd: String, + query: Option, + path: Option, + }, + Format { + cmd: String, + tool: Option, + targets: Option>, + }, + Test { + cmd: String, + }, + Lint { + cmd: String, + tool: Option, + targets: Option>, + }, + Noop { + cmd: String, + }, + Unknown { + cmd: String, + }, +} diff --git a/codex-rs/protocol/src/plan_tool.rs b/codex-rs/protocol/src/plan_tool.rs new file mode 100644 index 0000000000..78ef9cd48f --- /dev/null +++ b/codex-rs/protocol/src/plan_tool.rs @@ -0,0 +1,26 @@ +use serde::Deserialize; +use serde::Serialize; + +// Types for the TODO tool arguments matching codex-vscode/todo-mcp/src/main.rs +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum StepStatus { + Pending, + InProgress, + Completed, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct PlanItemArg { + pub step: String, + pub status: StepStatus, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct UpdatePlanArgs { + #[serde(default)] + pub explanation: Option, + pub plan: Vec, +} diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/protocol/src/protocol.rs similarity index 99% rename from codex-rs/core/src/protocol.rs rename to codex-rs/protocol/src/protocol.rs index d334c2eb86..c4f50b4ffa 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -193,7 +193,7 @@ pub struct WritableRoot { } impl WritableRoot { - pub(crate) fn is_path_writable(&self, path: &Path) -> bool { + pub fn is_path_writable(&self, path: &Path) -> bool { // Check if the path is under the root. if !path.starts_with(&self.root) { return false; diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 182667a060..75aa6f8ecd 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -33,6 +33,7 @@ codex-common = { path = "../common", features = [ "sandbox_summary", ] } codex-core = { path = "../core" } +codex-protocol = { path = "../protocol" } codex-file-search = { path = "../file-search" } codex-login = { path = "../login" } codex-ollama = { path = "../ollama" } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c39edb8229..637d50bd33 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -3,7 +3,6 @@ use std::path::PathBuf; use std::sync::Arc; use codex_core::config::Config; -use codex_core::parse_command::ParsedCommand; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; use codex_core::protocol::AgentReasoningDeltaEvent; @@ -26,6 +25,7 @@ use codex_core::protocol::PatchApplyBeginEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TokenUsage; use codex_core::protocol::TurnDiffEvent; +use codex_protocol::parse_command::ParsedCommand; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; use ratatui::buffer::Buffer; diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index bf03a8ed05..eb7142ac6d 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -204,9 +204,12 @@ fn exec_history_cell_shows_working_then_completed() { call_id: "call-1".into(), command: vec!["bash".into(), "-lc".into(), "echo done".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), - parsed_cmd: vec![codex_core::parse_command::ParsedCommand::Unknown { - cmd: "echo done".into(), - }], + parsed_cmd: vec![ + codex_core::parse_command::ParsedCommand::Unknown { + cmd: "echo done".into(), + } + .into(), + ], }), }); @@ -246,9 +249,12 @@ fn exec_history_cell_shows_working_then_failed() { call_id: "call-2".into(), command: vec!["bash".into(), "-lc".into(), "false".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), - parsed_cmd: vec![codex_core::parse_command::ParsedCommand::Unknown { - cmd: "false".into(), - }], + parsed_cmd: vec![ + codex_core::parse_command::ParsedCommand::Unknown { + cmd: "false".into(), + } + .into(), + ], }), }); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 36d412cec8..37a9d66f7e 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -9,7 +9,6 @@ use codex_ansi_escape::ansi_escape_line; use codex_common::create_config_summary_entries; use codex_common::elapsed::format_duration; use codex_core::config::Config; -use codex_core::parse_command::ParsedCommand; use codex_core::plan_tool::PlanItemArg; use codex_core::plan_tool::StepStatus; use codex_core::plan_tool::UpdatePlanArgs; @@ -20,6 +19,7 @@ use codex_core::protocol::SessionConfiguredEvent; use codex_core::protocol::TokenUsage; use codex_login::get_auth_file; use codex_login::try_read_auth_json; +use codex_protocol::parse_command::ParsedCommand; use image::DynamicImage; use image::ImageReader; use mcp_types::EmbeddedResourceResource; From dcfdd2faf51f00a60ca294db8c2aa82f07abf750 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Sat, 16 Aug 2025 04:59:52 +0900 Subject: [PATCH 3/6] Fix #2296 Add "minimal" reasoning effort for GPT 5 models (#2326) This pull request resolves #2296; I've confirmed if it works by: 1. Add settings to ~/.codex/config.toml: ```toml model_reasoning_effort = "minimal" ``` 2. Run the CLI: ``` cd codex-rs cargo build && RUST_LOG=trace cargo run --bin codex /status tail -f ~/.codex/log/codex-tui.log ``` Co-authored-by: pakrym-oai --- codex-rs/core/src/client_common.rs | 2 ++ codex-rs/core/src/config_types.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index c3489200c9..67fcb7cd84 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -94,6 +94,7 @@ pub(crate) struct Reasoning { #[derive(Debug, Serialize, Default, Clone, Copy)] #[serde(rename_all = "lowercase")] pub(crate) enum OpenAiReasoningEffort { + Minimal, Low, #[default] Medium, @@ -103,6 +104,7 @@ pub(crate) enum OpenAiReasoningEffort { impl From for Option { fn from(effort: ReasoningEffortConfig) -> Self { match effort { + ReasoningEffortConfig::Minimal => Some(OpenAiReasoningEffort::Minimal), ReasoningEffortConfig::Low => Some(OpenAiReasoningEffort::Low), ReasoningEffortConfig::Medium => Some(OpenAiReasoningEffort::Medium), ReasoningEffortConfig::High => Some(OpenAiReasoningEffort::High), diff --git a/codex-rs/core/src/config_types.rs b/codex-rs/core/src/config_types.rs index bb61f820d3..17fd610420 100644 --- a/codex-rs/core/src/config_types.rs +++ b/codex-rs/core/src/config_types.rs @@ -206,6 +206,7 @@ impl From for ShellEnvironmentPolicy { #[serde(rename_all = "lowercase")] #[strum(serialize_all = "lowercase")] pub enum ReasoningEffort { + Minimal, Low, #[default] Medium, From c1156a878b1e45353554e2ed19ae94ac1c83c33f Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 15 Aug 2025 13:01:27 -0700 Subject: [PATCH 4/6] Remove duplicated "Successfully logged in message" (#2357) --- codex-rs/cli/src/login.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/codex-rs/cli/src/login.rs b/codex-rs/cli/src/login.rs index a5ee7fa430..5f9dc5f908 100644 --- a/codex-rs/cli/src/login.rs +++ b/codex-rs/cli/src/login.rs @@ -21,10 +21,7 @@ pub async fn login_with_chatgpt(codex_home: PathBuf) -> std::io::Result<()> { server.actual_port, server.auth_url, ); - server.block_until_done()?; - - eprintln!("Successfully logged in"); - Ok(()) + server.block_until_done() } pub async fn run_login_with_chatgpt(cli_config_overrides: CliConfigOverrides) -> ! { From 1ad8ae2579fd9e1db4b549150f5ec9c3905b9900 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 15 Aug 2025 16:25:48 -0400 Subject: [PATCH 5/6] color the status letter in apply patch summary (#2337) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Screenshot 2025-08-14 at 8 30 30 PM --- AGENTS.md | 9 +++++++++ codex-rs/tui/src/history_cell.rs | 28 ++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 264b1e0448..b2f21c7c6b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,6 +12,15 @@ Before finalizing a change to `codex-rs`, run `just fmt` (in `codex-rs` director 1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`. 2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`. +## TUI code conventions + +- Use concise styling helpers from ratatui’s Stylize trait. + - Basic spans: use "text".into() + - Styled spans: use "text".red(), "text".green(), "text".magenta(), "text".dim(), etc. + - Prefer these over constructing styles with `Span::styled` and `Style` directly. + - Example: patch summary file lines + - Desired: vec![" └ ".into(), "M".red(), " ".dim(), "tui/src/app.rs".dim()] + ## Snapshot tests This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to validate rendered output. When UI or text output changes intentionally, update the snapshots as follows: diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 37a9d66f7e..bc90ad3455 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -818,8 +818,32 @@ pub(crate) fn new_patch_apply_success(stdout: String) -> PlainHistoryCell { let mut iter = stdout.lines(); for (i, raw) in iter.by_ref().take(TOOL_CALL_MAX_LINES).enumerate() { let prefix = if i == 0 { " └ " } else { " " }; - let s = format!("{prefix}{raw}"); - lines.push(ansi_escape_line(&s).dim()); + + // First line is the header; dim it entirely. + if i == 0 { + let s = format!("{prefix}{raw}"); + lines.push(ansi_escape_line(&s).dim()); + continue; + } + + // Subsequent lines should look like: "M path/to/file". + // Colorize the status letter like `git status` (e.g., M red). + let status = raw.chars().next(); + let rest = raw.get(1..).unwrap_or(""); + + let status_span = match status { + Some('M') => "M".red(), + Some('A') => "A".green(), + Some('D') => "D".red(), + Some(other) => other.to_string().into(), + None => "".into(), + }; + + lines.push(Line::from(vec![ + prefix.into(), + status_span, + ansi_escape_line(rest).to_string().into(), + ])); } let remaining = iter.count(); if remaining > 0 { From 8b1b3f135686a0d379cb34c29d01ec9c0c5ba5c8 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 15 Aug 2025 13:38:33 -0700 Subject: [PATCH 6/6] remove mcp-server/src/mcp_protocol.rs and the code that depends on it --- codex-rs/mcp-server/src/lib.rs | 3 - codex-rs/mcp-server/src/mcp_protocol.rs | 1054 ----------------- codex-rs/mcp-server/src/message_processor.rs | 70 +- .../src/tool_handlers/create_conversation.rs | 127 -- .../src/tool_handlers/send_message.rs | 114 -- .../mcp-server/tests/common/mcp_process.rs | 88 +- .../mcp-server/tests/create_conversation.rs | 84 +- codex-rs/mcp-server/tests/send_message.rs | 110 +- 8 files changed, 139 insertions(+), 1511 deletions(-) delete mode 100644 codex-rs/mcp-server/src/mcp_protocol.rs delete mode 100644 codex-rs/mcp-server/src/tool_handlers/create_conversation.rs delete mode 100644 codex-rs/mcp-server/src/tool_handlers/send_message.rs diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index b6dcf8246d..f30daa927c 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -18,15 +18,12 @@ use tracing_subscriber::EnvFilter; mod codex_message_processor; mod codex_tool_config; mod codex_tool_runner; -mod conversation_loop; mod error_code; mod exec_approval; mod json_to_toml; -pub mod mcp_protocol; pub(crate) mod message_processor; mod outgoing_message; mod patch_approval; -pub(crate) mod tool_handlers; pub mod wire_format; use crate::message_processor::MessageProcessor; diff --git a/codex-rs/mcp-server/src/mcp_protocol.rs b/codex-rs/mcp-server/src/mcp_protocol.rs deleted file mode 100644 index 26c6655f3b..0000000000 --- a/codex-rs/mcp-server/src/mcp_protocol.rs +++ /dev/null @@ -1,1054 +0,0 @@ -use codex_core::config_types::SandboxMode; -use codex_core::protocol::AskForApproval; -use codex_core::protocol::EventMsg; -use codex_core::protocol::InputItem; -use serde::Deserialize; -use serde::Serialize; -use strum_macros::Display; -use uuid::Uuid; - -use mcp_types::CallToolResult; -use mcp_types::ContentBlock; -use mcp_types::RequestId; -use mcp_types::TextContent; - -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] -#[serde(transparent)] -pub struct ConversationId(pub Uuid); - -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] -#[serde(transparent)] -pub struct MessageId(pub Uuid); - -// Requests -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ToolCallRequest { - #[serde(rename = "jsonrpc")] - pub jsonrpc: &'static str, - pub id: RequestId, - pub method: &'static str, - pub params: ToolCallRequestParams, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(tag = "name", content = "arguments", rename_all = "camelCase")] -pub enum ToolCallRequestParams { - ConversationCreate(ConversationCreateArgs), - ConversationStream(ConversationStreamArgs), - ConversationSendMessage(ConversationSendMessageArgs), - ConversationsList(ConversationsListArgs), -} - -impl ToolCallRequestParams { - /// Wrap this request in a JSON-RPC request. - #[allow(dead_code)] - pub fn into_request(self, id: RequestId) -> ToolCallRequest { - ToolCallRequest { - jsonrpc: "2.0", - id, - method: "tools/call", - params: self, - } - } -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationCreateArgs { - pub prompt: String, - pub model: String, - pub cwd: String, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub approval_policy: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub sandbox: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub config: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub profile: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub base_instructions: Option, -} - -/// Optional overrides for an existing conversation's execution context when sending a message. -/// Fields left as `None` inherit the current conversation/session settings. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationOverrides { - #[serde(default, skip_serializing_if = "Option::is_none")] - pub model: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub cwd: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub approval_policy: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub sandbox: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub config: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub profile: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub base_instructions: Option, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationStreamArgs { - pub conversation_id: ConversationId, -} - -/// If omitted, the message continues from the latest turn. -/// Set to resume/edit from an earlier parent message in the thread. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationSendMessageArgs { - pub conversation_id: ConversationId, - pub content: Vec, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub parent_message_id: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - #[serde(flatten)] - pub conversation_overrides: Option, -} -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationsListArgs { - #[serde(default, skip_serializing_if = "Option::is_none")] - pub limit: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub cursor: Option, -} - -// Responses -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct ToolCallResponse { - pub request_id: RequestId, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub is_error: Option, - #[serde(default, skip_serializing_if = "Option::is_none", flatten)] - pub result: Option, -} - -impl From for CallToolResult { - fn from(val: ToolCallResponse) -> Self { - let ToolCallResponse { - request_id: _request_id, - is_error, - result, - } = val; - match result { - Some(res) => match serde_json::to_value(&res) { - Ok(v) => CallToolResult { - content: vec![ContentBlock::TextContent(TextContent { - r#type: "text".to_string(), - text: v.to_string(), - annotations: None, - })], - is_error, - structured_content: Some(v), - }, - Err(e) => CallToolResult { - content: vec![ContentBlock::TextContent(TextContent { - r#type: "text".to_string(), - text: format!("Failed to serialize tool result: {e}"), - annotations: None, - })], - is_error: Some(true), - structured_content: None, - }, - }, - None => CallToolResult { - content: vec![], - is_error, - structured_content: None, - }, - } - } -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(untagged)] -pub enum ToolCallResponseResult { - ConversationCreate(ConversationCreateResult), - ConversationStream(ConversationStreamResult), - ConversationSendMessage(ConversationSendMessageResult), - ConversationsList(ConversationsListResult), -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(untagged)] -pub enum ConversationCreateResult { - Ok { - conversation_id: ConversationId, - model: String, - }, - Error { - message: String, - }, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationStreamResult {} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -// TODO: remove this status because we have is_error field in the response. -#[serde(tag = "status", rename_all = "camelCase")] -pub enum ConversationSendMessageResult { - Ok, - Error { message: String }, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationsListResult { - pub conversations: Vec, - #[serde(skip_serializing_if = "Option::is_none")] - pub next_cursor: Option, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationSummary { - pub conversation_id: ConversationId, - pub title: String, -} - -// Notifications -#[derive(Debug, Clone, Deserialize, Display)] -pub enum ServerNotification { - InitialState(InitialStateNotificationParams), - StreamDisconnected(StreamDisconnectedNotificationParams), - CodexEvent(Box), -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct NotificationMeta { - #[serde(skip_serializing_if = "Option::is_none")] - pub conversation_id: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub request_id: Option, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct InitialStateNotificationParams { - #[serde(rename = "_meta", skip_serializing_if = "Option::is_none")] - pub meta: Option, - pub initial_state: InitialStatePayload, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct InitialStatePayload { - #[serde(default)] - pub events: Vec, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct StreamDisconnectedNotificationParams { - #[serde(rename = "_meta", skip_serializing_if = "Option::is_none")] - pub meta: Option, - pub reason: String, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct CodexEventNotificationParams { - #[serde(rename = "_meta", skip_serializing_if = "Option::is_none")] - pub meta: Option, - pub msg: EventMsg, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] -pub struct CancelNotificationParams { - pub request_id: RequestId, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub reason: Option, -} - -impl Serialize for ServerNotification { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - use serde::ser::SerializeMap; - - let mut map = serializer.serialize_map(Some(2))?; - match self { - ServerNotification::CodexEvent(p) => { - map.serialize_entry("method", &format!("notifications/{}", p.msg))?; - map.serialize_entry("params", p)?; - } - ServerNotification::InitialState(p) => { - map.serialize_entry("method", "notifications/initial_state")?; - map.serialize_entry("params", p)?; - } - ServerNotification::StreamDisconnected(p) => { - map.serialize_entry("method", "notifications/stream_disconnected")?; - map.serialize_entry("params", p)?; - } - } - map.end() - } -} - -#[derive(Debug, Clone, Deserialize, Serialize)] -#[serde(tag = "method", content = "params", rename_all = "camelCase")] -pub enum ClientNotification { - #[serde(rename = "notifications/cancelled")] - Cancelled(CancelNotificationParams), -} - -#[cfg(test)] -mod tests { - use std::path::PathBuf; - - use super::*; - use codex_core::protocol::McpInvocation; - use codex_core::protocol::McpToolCallBeginEvent; - use pretty_assertions::assert_eq; - use serde::Serialize; - use serde_json::Value; - use serde_json::json; - use uuid::uuid; - - fn to_val(v: &T) -> Value { - serde_json::to_value(v).expect("serialize to Value") - } - - // ----- Requests ----- - - #[test] - fn serialize_tool_call_request_params_conversation_create_minimal() { - let req = ToolCallRequestParams::ConversationCreate(ConversationCreateArgs { - prompt: "".into(), - model: "o3".into(), - cwd: "/repo".into(), - approval_policy: None, - sandbox: None, - config: None, - profile: None, - base_instructions: None, - }); - - let observed = to_val(&req.into_request(mcp_types::RequestId::Integer(2))); - let expected = json!({ - "jsonrpc": "2.0", - "id": 2, - "method": "tools/call", - "params": { - "name": "conversationCreate", - "arguments": { - "prompt": "", - "model": "o3", - "cwd": "/repo" - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_tool_call_request_params_conversation_send_message_with_overrides_and_parent_message_id() - { - let req = ToolCallRequestParams::ConversationSendMessage(ConversationSendMessageArgs { - conversation_id: ConversationId(uuid!("d0f6ecbe-84a2-41c1-b23d-b20473b25eab")), - content: vec![ - InputItem::Text { text: "Hi".into() }, - InputItem::Image { - image_url: "https://example.com/cat.jpg".into(), - }, - InputItem::LocalImage { - path: "notes.txt".into(), - }, - ], - parent_message_id: Some(MessageId(uuid!("67e55044-10b1-426f-9247-bb680e5fe0c8"))), - conversation_overrides: Some(ConversationOverrides { - model: Some("o4-mini".into()), - cwd: Some("/workdir".into()), - approval_policy: None, - sandbox: Some(SandboxMode::DangerFullAccess), - config: Some(json!({"temp": 0.2})), - profile: Some("eng".into()), - base_instructions: Some("Be terse".into()), - }), - }); - - let observed = to_val(&req.into_request(mcp_types::RequestId::Integer(2))); - let expected = json!({ - "jsonrpc": "2.0", - "id": 2, - "method": "tools/call", - "params": { - "name": "conversationSendMessage", - "arguments": { - "conversation_id": "d0f6ecbe-84a2-41c1-b23d-b20473b25eab", - "content": [ - { "type": "text", "text": "Hi" }, - { "type": "image", "image_url": "https://example.com/cat.jpg" }, - { "type": "local_image", "path": "notes.txt" } - ], - "parent_message_id": "67e55044-10b1-426f-9247-bb680e5fe0c8", - "model": "o4-mini", - "cwd": "/workdir", - "sandbox": "danger-full-access", - "config": { "temp": 0.2 }, - "profile": "eng", - "base_instructions": "Be terse" - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_tool_call_request_params_conversations_list_with_opts() { - let req = ToolCallRequestParams::ConversationsList(ConversationsListArgs { - limit: Some(50), - cursor: Some("abc".into()), - }); - - let observed = to_val(&req.into_request(RequestId::Integer(2))); - let expected = json!({ - "jsonrpc": "2.0", - "id": 2, - "method": "tools/call", - "params": { - "name": "conversationsList", - "arguments": { - "limit": 50, - "cursor": "abc" - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_tool_call_request_params_conversation_stream() { - let req = ToolCallRequestParams::ConversationStream(ConversationStreamArgs { - conversation_id: ConversationId(uuid!("67e55044-10b1-426f-9247-bb680e5fe0c8")), - }); - - let observed = to_val(&req.into_request(mcp_types::RequestId::Integer(2))); - let expected = json!({ - "jsonrpc": "2.0", - "id": 2, - "method": "tools/call", - "params": { - "name": "conversationStream", - "arguments": { - "conversation_id": "67e55044-10b1-426f-9247-bb680e5fe0c8" - } - } - }); - assert_eq!(observed, expected); - } - - // ----- Message inputs / sources ----- - - #[test] - fn serialize_message_input_image_url() { - let item = InputItem::Image { - image_url: "https://example.com/x.png".into(), - }; - let observed = to_val(&item); - let expected = json!({ - "type": "image", - "image_url": "https://example.com/x.png" - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_message_input_local_image_path() { - let url = InputItem::LocalImage { - path: PathBuf::from("https://example.com/a.pdf"), - }; - let id = InputItem::LocalImage { - path: PathBuf::from("file_456"), - }; - let observed_url = to_val(&url); - let expected_url = json!({"type":"local_image","path":"https://example.com/a.pdf"}); - assert_eq!( - observed_url, expected_url, - "LocalImage with URL path should serialize as image_url" - ); - let observed_id = to_val(&id); - let expected_id = json!({"type":"local_image","path":"file_456"}); - assert_eq!( - observed_id, expected_id, - "LocalImage with file id should serialize as image_url" - ); - } - - #[test] - fn serialize_message_input_image_url_without_detail() { - let item = InputItem::Image { - image_url: "https://example.com/x.png".into(), - }; - let observed = to_val(&item); - let expected = json!({ - "type": "image", - "image_url": "https://example.com/x.png" - }); - assert_eq!(observed, expected); - } - - // ----- Responses ----- - - #[test] - fn response_success_conversation_create_full_schema() { - let env = ToolCallResponse { - request_id: RequestId::Integer(1), - is_error: None, - result: Some(ToolCallResponseResult::ConversationCreate( - ConversationCreateResult::Ok { - conversation_id: ConversationId(uuid!("d0f6ecbe-84a2-41c1-b23d-b20473b25eab")), - model: "o3".into(), - }, - )), - }; - let req_id = env.request_id.clone(); - let observed = to_val(&CallToolResult::from(env)); - let expected = json!({ - "content": [ - { "type": "text", "text": "{\"conversation_id\":\"d0f6ecbe-84a2-41c1-b23d-b20473b25eab\",\"model\":\"o3\"}" } - ], - "structuredContent": { - "conversation_id": "d0f6ecbe-84a2-41c1-b23d-b20473b25eab", - "model": "o3" - } - }); - assert_eq!( - observed, expected, - "response (ConversationCreate) must match" - ); - assert_eq!(req_id, RequestId::Integer(1)); - } - - #[test] - fn response_error_conversation_create_full_schema() { - let env = ToolCallResponse { - request_id: RequestId::Integer(2), - is_error: Some(true), - result: Some(ToolCallResponseResult::ConversationCreate( - ConversationCreateResult::Error { - message: "Failed to initialize session".into(), - }, - )), - }; - let req_id = env.request_id.clone(); - let observed = to_val(&CallToolResult::from(env)); - let expected = json!({ - "content": [ - { "type": "text", "text": "{\"message\":\"Failed to initialize session\"}" } - ], - "isError": true, - "structuredContent": { - "message": "Failed to initialize session" - } - }); - assert_eq!( - observed, expected, - "error response (ConversationCreate) must match" - ); - assert_eq!(req_id, RequestId::Integer(2)); - } - - #[test] - fn response_success_conversation_stream_empty_result_object() { - let env = ToolCallResponse { - request_id: RequestId::Integer(2), - is_error: None, - result: Some(ToolCallResponseResult::ConversationStream( - ConversationStreamResult {}, - )), - }; - let req_id = env.request_id.clone(); - let observed = to_val(&CallToolResult::from(env)); - let expected = json!({ - "content": [ { "type": "text", "text": "{}" } ], - "structuredContent": {} - }); - assert_eq!( - observed, expected, - "response (ConversationStream) must have empty object result" - ); - assert_eq!(req_id, RequestId::Integer(2)); - } - - #[test] - fn response_success_send_message_accepted_full_schema() { - let env = ToolCallResponse { - request_id: RequestId::Integer(3), - is_error: None, - result: Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult::Ok, - )), - }; - let req_id = env.request_id.clone(); - let observed = to_val(&CallToolResult::from(env)); - let expected = json!({ - "content": [ { "type": "text", "text": "{\"status\":\"ok\"}" } ], - "structuredContent": { "status": "ok" } - }); - assert_eq!( - observed, expected, - "response (ConversationSendMessageAccepted) must match" - ); - assert_eq!(req_id, RequestId::Integer(3)); - } - - #[test] - fn response_success_conversations_list_with_next_cursor_full_schema() { - let env = ToolCallResponse { - request_id: RequestId::Integer(4), - is_error: None, - result: Some(ToolCallResponseResult::ConversationsList( - ConversationsListResult { - conversations: vec![ConversationSummary { - conversation_id: ConversationId(uuid!( - "67e55044-10b1-426f-9247-bb680e5fe0c8" - )), - title: "Refactor config loader".into(), - }], - next_cursor: Some("next123".into()), - }, - )), - }; - let req_id = env.request_id.clone(); - let observed = to_val(&CallToolResult::from(env)); - let expected = json!({ - "content": [ - { "type": "text", "text": "{\"conversations\":[{\"conversation_id\":\"67e55044-10b1-426f-9247-bb680e5fe0c8\",\"title\":\"Refactor config loader\"}],\"next_cursor\":\"next123\"}" } - ], - "structuredContent": { - "conversations": [ - { - "conversation_id": "67e55044-10b1-426f-9247-bb680e5fe0c8", - "title": "Refactor config loader" - } - ], - "next_cursor": "next123" - } - }); - assert_eq!( - observed, expected, - "response (ConversationsList with cursor) must match" - ); - assert_eq!(req_id, RequestId::Integer(4)); - } - - #[test] - fn response_error_only_is_error_and_request_id_string() { - let env = ToolCallResponse { - request_id: RequestId::Integer(4), - is_error: Some(true), - result: None, - }; - let req_id = env.request_id.clone(); - let observed = to_val(&CallToolResult::from(env)); - let expected = json!({ - "content": [], - "isError": true - }); - assert_eq!( - observed, expected, - "error response must omit `result` and include `isError`" - ); - assert_eq!(req_id, RequestId::Integer(4)); - } - - // ----- Notifications ----- - - #[test] - fn serialize_notification_initial_state_minimal() { - let params = InitialStateNotificationParams { - meta: Some(NotificationMeta { - conversation_id: Some(ConversationId(uuid!( - "67e55044-10b1-426f-9247-bb680e5fe0c8" - ))), - request_id: Some(RequestId::Integer(44)), - }), - initial_state: InitialStatePayload { - events: vec![ - CodexEventNotificationParams { - meta: None, - msg: EventMsg::TaskStarted, - }, - CodexEventNotificationParams { - meta: None, - msg: EventMsg::AgentMessageDelta( - codex_core::protocol::AgentMessageDeltaEvent { - delta: "Loading...".into(), - }, - ), - }, - ], - }, - }; - - let observed = to_val(&ServerNotification::InitialState(params.clone())); - let expected = json!({ - "method": "notifications/initial_state", - "params": { - "_meta": { - "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8", - "requestId": 44 - }, - "initial_state": { - "events": [ - { "msg": { "type": "task_started" } }, - { "msg": { "type": "agent_message_delta", "delta": "Loading..." } } - ] - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_initial_state_omits_empty_events_full_json() { - let params = InitialStateNotificationParams { - meta: None, - initial_state: InitialStatePayload { events: vec![] }, - }; - - let observed = to_val(&ServerNotification::InitialState(params)); - let expected = json!({ - "method": "notifications/initial_state", - "params": { - "initial_state": { "events": [] } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_stream_disconnected() { - let params = StreamDisconnectedNotificationParams { - meta: Some(NotificationMeta { - conversation_id: Some(ConversationId(uuid!( - "67e55044-10b1-426f-9247-bb680e5fe0c8" - ))), - request_id: None, - }), - reason: "New stream() took over".into(), - }; - - let observed = to_val(&ServerNotification::StreamDisconnected(params)); - let expected = json!({ - "method": "notifications/stream_disconnected", - "params": { - "_meta": { "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8" }, - "reason": "New stream() took over" - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_uses_eventmsg_type_in_method() { - let params = CodexEventNotificationParams { - meta: Some(NotificationMeta { - conversation_id: Some(ConversationId(uuid!( - "67e55044-10b1-426f-9247-bb680e5fe0c8" - ))), - request_id: Some(RequestId::Integer(44)), - }), - msg: EventMsg::AgentMessage(codex_core::protocol::AgentMessageEvent { - message: "hi".into(), - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/agent_message", - "params": { - "_meta": { - "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8", - "requestId": 44 - }, - "msg": { "type": "agent_message", "message": "hi" } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_task_started_full_json() { - let params = CodexEventNotificationParams { - meta: Some(NotificationMeta { - conversation_id: Some(ConversationId(uuid!( - "67e55044-10b1-426f-9247-bb680e5fe0c8" - ))), - request_id: Some(RequestId::Integer(7)), - }), - msg: EventMsg::TaskStarted, - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/task_started", - "params": { - "_meta": { - "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8", - "requestId": 7 - }, - "msg": { "type": "task_started" } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_agent_message_delta_full_json() { - let params = CodexEventNotificationParams { - meta: None, - msg: EventMsg::AgentMessageDelta(codex_core::protocol::AgentMessageDeltaEvent { - delta: "stream...".into(), - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/agent_message_delta", - "params": { - "msg": { "type": "agent_message_delta", "delta": "stream..." } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_agent_message_full_json() { - let params = CodexEventNotificationParams { - meta: Some(NotificationMeta { - conversation_id: Some(ConversationId(uuid!( - "67e55044-10b1-426f-9247-bb680e5fe0c8" - ))), - request_id: Some(RequestId::Integer(44)), - }), - msg: EventMsg::AgentMessage(codex_core::protocol::AgentMessageEvent { - message: "hi".into(), - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/agent_message", - "params": { - "_meta": { - "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8", - "requestId": 44 - }, - "msg": { "type": "agent_message", "message": "hi" } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_agent_reasoning_full_json() { - let params = CodexEventNotificationParams { - meta: None, - msg: EventMsg::AgentReasoning(codex_core::protocol::AgentReasoningEvent { - text: "thinking…".into(), - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/agent_reasoning", - "params": { - "msg": { "type": "agent_reasoning", "text": "thinking…" } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_token_count_full_json() { - let usage = codex_core::protocol::TokenUsage { - input_tokens: 10, - cached_input_tokens: Some(2), - output_tokens: 5, - reasoning_output_tokens: Some(1), - total_tokens: 16, - }; - let params = CodexEventNotificationParams { - meta: None, - msg: EventMsg::TokenCount(usage), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/token_count", - "params": { - "msg": { - "type": "token_count", - "input_tokens": 10, - "cached_input_tokens": 2, - "output_tokens": 5, - "reasoning_output_tokens": 1, - "total_tokens": 16 - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_session_configured_full_json() { - let params = CodexEventNotificationParams { - meta: Some(NotificationMeta { - conversation_id: Some(ConversationId(uuid!( - "67e55044-10b1-426f-9247-bb680e5fe0c8" - ))), - request_id: None, - }), - msg: EventMsg::SessionConfigured(codex_core::protocol::SessionConfiguredEvent { - session_id: uuid!("67e55044-10b1-426f-9247-bb680e5fe0c8"), - model: "codex-mini-latest".into(), - history_log_id: 42, - history_entry_count: 3, - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/session_configured", - "params": { - "_meta": { "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8" }, - "msg": { - "type": "session_configured", - "session_id": "67e55044-10b1-426f-9247-bb680e5fe0c8", - "model": "codex-mini-latest", - "history_log_id": 42, - "history_entry_count": 3 - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_exec_command_begin_full_json() { - let params = CodexEventNotificationParams { - meta: None, - msg: EventMsg::ExecCommandBegin(codex_core::protocol::ExecCommandBeginEvent { - call_id: "c1".into(), - command: vec!["bash".into(), "-lc".into(), "echo hi".into()], - cwd: std::path::PathBuf::from("/work"), - parsed_cmd: vec![], - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/exec_command_begin", - "params": { - "msg": { - "type": "exec_command_begin", - "call_id": "c1", - "command": ["bash", "-lc", "echo hi"], - "cwd": "/work", - "parsed_cmd": [] - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_mcp_tool_call_begin_full_json() { - let params = CodexEventNotificationParams { - meta: None, - msg: EventMsg::McpToolCallBegin(McpToolCallBeginEvent { - call_id: "m1".into(), - invocation: McpInvocation { - server: "calc".into(), - tool: "add".into(), - arguments: Some(json!({"a":1,"b":2})), - }, - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/mcp_tool_call_begin", - "params": { - "msg": { - "type": "mcp_tool_call_begin", - "call_id": "m1", - "invocation": { - "server": "calc", - "tool": "add", - "arguments": { "a": 1, "b": 2 } - } - } - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_codex_event_patch_apply_end_full_json() { - let params = CodexEventNotificationParams { - meta: None, - msg: EventMsg::PatchApplyEnd(codex_core::protocol::PatchApplyEndEvent { - call_id: "p1".into(), - stdout: "ok".into(), - stderr: "".into(), - success: true, - }), - }; - - let observed = to_val(&ServerNotification::CodexEvent(Box::new(params))); - let expected = json!({ - "method": "notifications/patch_apply_end", - "params": { - "msg": { - "type": "patch_apply_end", - "call_id": "p1", - "stdout": "ok", - "stderr": "", - "success": true - } - } - }); - assert_eq!(observed, expected); - } - - // ----- Cancelled notifications ----- - - #[test] - fn serialize_notification_cancelled_with_reason_full_json() { - let params = CancelNotificationParams { - request_id: RequestId::String("r-123".into()), - reason: Some("user_cancelled".into()), - }; - - let observed = to_val(&ClientNotification::Cancelled(params)); - let expected = json!({ - "method": "notifications/cancelled", - "params": { - "requestId": "r-123", - "reason": "user_cancelled" - } - }); - assert_eq!(observed, expected); - } - - #[test] - fn serialize_notification_cancelled_without_reason_full_json() { - let params = CancelNotificationParams { - request_id: RequestId::Integer(77), - reason: None, - }; - - let observed = to_val(&ClientNotification::Cancelled(params)); - - // Check exact structure: reason must be omitted. - assert_eq!(observed["method"], "notifications/cancelled"); - assert_eq!(observed["params"]["requestId"], 77); - assert!( - observed["params"].get("reason").is_none(), - "reason must be omitted when None" - ); - } -} diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 763e51bf81..f6450f4438 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -1,5 +1,4 @@ use std::collections::HashMap; -use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; @@ -9,18 +8,12 @@ use crate::codex_tool_config::CodexToolCallReplyParam; use crate::codex_tool_config::create_tool_for_codex_tool_call_param; use crate::codex_tool_config::create_tool_for_codex_tool_call_reply_param; use crate::error_code::INVALID_REQUEST_ERROR_CODE; -use crate::mcp_protocol::ToolCallRequestParams; -use crate::mcp_protocol::ToolCallResponse; -use crate::mcp_protocol::ToolCallResponseResult; use crate::outgoing_message::OutgoingMessageSender; -use crate::tool_handlers::create_conversation::handle_create_conversation; -use crate::tool_handlers::send_message::handle_send_message; use crate::wire_format::ClientRequest; use codex_core::ConversationManager; use codex_core::config::Config as CodexConfig; use codex_core::protocol::Submission; -use mcp_types::CallToolRequest; use mcp_types::CallToolRequestParams; use mcp_types::CallToolResult; use mcp_types::ClientRequest as McpClientRequest; @@ -48,7 +41,7 @@ pub(crate) struct MessageProcessor { codex_linux_sandbox_exe: Option, conversation_manager: Arc, running_requests_id_to_codex_uuid: Arc>>, - running_session_ids: Arc>>, + // Tracks in-flight MCP request IDs to Codex session UUIDs. } impl MessageProcessor { @@ -72,22 +65,9 @@ impl MessageProcessor { codex_linux_sandbox_exe, conversation_manager, running_requests_id_to_codex_uuid: Arc::new(Mutex::new(HashMap::new())), - running_session_ids: Arc::new(Mutex::new(HashSet::new())), } } - pub(crate) fn get_conversation_manager(&self) -> &ConversationManager { - &self.conversation_manager - } - - pub(crate) fn outgoing(&self) -> Arc { - self.outgoing.clone() - } - - pub(crate) fn running_session_ids(&self) -> Arc>> { - self.running_session_ids.clone() - } - pub(crate) async fn process_request(&mut self, request: JSONRPCRequest) { if let Ok(request_json) = serde_json::to_value(request.clone()) && let Ok(codex_request) = serde_json::from_value::(request_json) @@ -341,14 +321,6 @@ impl MessageProcessor { params: ::Params, ) { tracing::info!("tools/call -> params: {:?}", params); - // Serialize params into JSON and try to parse as new type - if let Ok(new_params) = - serde_json::to_value(¶ms).and_then(serde_json::from_value::) - { - // New tool call matched → forward - self.handle_new_tool_calls(id, new_params).await; - return; - } let CallToolRequestParams { name, arguments } = params; match name.as_str() { @@ -372,30 +344,6 @@ impl MessageProcessor { } } } - async fn handle_new_tool_calls(&self, request_id: RequestId, params: ToolCallRequestParams) { - match params { - ToolCallRequestParams::ConversationCreate(args) => { - handle_create_conversation(self, request_id, args).await; - } - ToolCallRequestParams::ConversationSendMessage(args) => { - handle_send_message(self, request_id, args).await; - } - _ => { - let result = CallToolResult { - content: vec![ContentBlock::TextContent(TextContent { - r#type: "text".to_string(), - text: "Unknown tool".to_string(), - annotations: None, - })], - is_error: Some(true), - structured_content: None, - }; - self.send_response::(request_id, result) - .await; - } - } - } - async fn handle_tool_call_codex(&self, id: RequestId, arguments: Option) { let (initial_prompt, config): (String, CodexConfig) = match arguments { Some(json_val) => match serde_json::from_value::(json_val) { @@ -692,20 +640,4 @@ impl MessageProcessor { ) { tracing::info!("notifications/message -> params: {:?}", params); } - - pub(crate) async fn send_response_with_optional_error( - &self, - id: RequestId, - message: Option, - error: Option, - ) { - let response = ToolCallResponse { - request_id: id.clone(), - is_error: error, - result: message, - }; - let result: CallToolResult = response.into(); - self.send_response::(id.clone(), result) - .await; - } } diff --git a/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs b/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs deleted file mode 100644 index eee2e1d5f4..0000000000 --- a/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs +++ /dev/null @@ -1,127 +0,0 @@ -use std::path::PathBuf; - -use codex_core::NewConversation; -use codex_core::config::Config as CodexConfig; -use codex_core::config::ConfigOverrides; -use mcp_types::RequestId; - -use crate::conversation_loop::run_conversation_loop; -use crate::json_to_toml::json_to_toml; -use crate::mcp_protocol::ConversationCreateArgs; -use crate::mcp_protocol::ConversationCreateResult; -use crate::mcp_protocol::ConversationId; -use crate::mcp_protocol::ToolCallResponseResult; -use crate::message_processor::MessageProcessor; - -pub(crate) async fn handle_create_conversation( - message_processor: &MessageProcessor, - id: RequestId, - args: ConversationCreateArgs, -) { - // Build ConfigOverrides from args - let ConversationCreateArgs { - prompt: _, // not used here; creation only establishes the session - model, - cwd, - approval_policy, - sandbox, - config, - profile, - base_instructions, - } = args; - - // Convert config overrides JSON into CLI-style TOML overrides - let cli_overrides: Vec<(String, toml::Value)> = match config { - Some(v) => match v.as_object() { - Some(map) => map - .into_iter() - .map(|(k, v)| (k.clone(), json_to_toml(v.clone()))) - .collect(), - None => Vec::new(), - }, - None => Vec::new(), - }; - - let overrides = ConfigOverrides { - model: Some(model.clone()), - cwd: Some(PathBuf::from(cwd)), - approval_policy, - sandbox_mode: sandbox, - model_provider: None, - config_profile: profile, - codex_linux_sandbox_exe: None, - base_instructions, - include_plan_tool: None, - include_apply_patch_tool: None, - disable_response_storage: None, - show_raw_agent_reasoning: None, - }; - - let cfg: CodexConfig = match CodexConfig::load_with_cli_overrides(cli_overrides, overrides) { - Ok(cfg) => cfg, - Err(e) => { - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationCreate( - ConversationCreateResult::Error { - message: format!("Failed to load config: {e}"), - }, - )), - Some(true), - ) - .await; - return; - } - }; - - // Initialize Codex session via server API - let NewConversation { - conversation_id: session_id, - conversation, - session_configured, - } = match message_processor - .get_conversation_manager() - .new_conversation(cfg) - .await - { - Ok(conv) => conv, - Err(e) => { - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationCreate( - ConversationCreateResult::Error { - message: format!("Failed to initialize session: {e}"), - }, - )), - Some(true), - ) - .await; - return; - } - }; - - let effective_model = session_configured.model.clone(); - - // Run the conversation loop in the background so this request can return immediately. - let outgoing = message_processor.outgoing(); - let spawn_id = id.clone(); - tokio::spawn(async move { - run_conversation_loop(conversation.clone(), outgoing, spawn_id).await; - }); - - // Reply with the new conversation id and effective model - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationCreate( - ConversationCreateResult::Ok { - conversation_id: ConversationId(session_id), - model: effective_model, - }, - )), - Some(false), - ) - .await; -} diff --git a/codex-rs/mcp-server/src/tool_handlers/send_message.rs b/codex-rs/mcp-server/src/tool_handlers/send_message.rs deleted file mode 100644 index 985854f852..0000000000 --- a/codex-rs/mcp-server/src/tool_handlers/send_message.rs +++ /dev/null @@ -1,114 +0,0 @@ -use codex_core::protocol::Op; -use codex_core::protocol::Submission; -use mcp_types::RequestId; - -use crate::mcp_protocol::ConversationSendMessageArgs; -use crate::mcp_protocol::ConversationSendMessageResult; -use crate::mcp_protocol::ToolCallResponseResult; -use crate::message_processor::MessageProcessor; - -pub(crate) async fn handle_send_message( - message_processor: &MessageProcessor, - id: RequestId, - arguments: ConversationSendMessageArgs, -) { - let ConversationSendMessageArgs { - conversation_id, - content: items, - parent_message_id: _, - conversation_overrides: _, - } = arguments; - - if items.is_empty() { - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult::Error { - message: "No content items provided".to_string(), - }, - )), - Some(true), - ) - .await; - return; - } - - let session_id = conversation_id.0; - let Ok(codex) = message_processor - .get_conversation_manager() - .get_conversation(session_id) - .await - else { - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult::Error { - message: "Session does not exist".to_string(), - }, - )), - Some(true), - ) - .await; - return; - }; - - let running = { - let running_sessions = message_processor.running_session_ids(); - let mut running_sessions = running_sessions.lock().await; - !running_sessions.insert(session_id) - }; - - if running { - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult::Error { - message: "Session is already running".to_string(), - }, - )), - Some(true), - ) - .await; - return; - } - - let request_id_string = match &id { - RequestId::String(s) => s.clone(), - RequestId::Integer(i) => i.to_string(), - }; - - let submit_res = codex - .submit_with_id(Submission { - id: request_id_string, - op: Op::UserInput { items }, - }) - .await; - - if let Err(e) = submit_res { - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult::Error { - message: format!("Failed to submit user input: {e}"), - }, - )), - Some(true), - ) - .await; - return; - } - - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult::Ok, - )), - Some(false), - ) - .await; -} diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index dc7833441c..df1b2b82ec 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -11,13 +11,8 @@ use tokio::process::ChildStdout; use anyhow::Context; use assert_cmd::prelude::*; -use codex_core::protocol::InputItem; use codex_mcp_server::CodexToolCallParam; use codex_mcp_server::CodexToolCallReplyParam; -use codex_mcp_server::mcp_protocol::ConversationCreateArgs; -use codex_mcp_server::mcp_protocol::ConversationId; -use codex_mcp_server::mcp_protocol::ConversationSendMessageArgs; -use codex_mcp_server::mcp_protocol::ToolCallRequestParams; use codex_mcp_server::wire_format::AddConversationListenerParams; use codex_mcp_server::wire_format::NewConversationParams; use codex_mcp_server::wire_format::RemoveConversationListenerParams; @@ -40,7 +35,6 @@ use pretty_assertions::assert_eq; use serde_json::json; use std::process::Command as StdCommand; use tokio::process::Command; -use uuid::Uuid; pub struct McpProcess { next_request_id: AtomicI64, @@ -186,60 +180,7 @@ impl McpProcess { .await } - pub async fn send_user_message_tool_call( - &mut self, - message: &str, - session_id: &str, - ) -> anyhow::Result { - let params = ToolCallRequestParams::ConversationSendMessage(ConversationSendMessageArgs { - conversation_id: ConversationId(Uuid::parse_str(session_id)?), - content: vec![InputItem::Text { - text: message.to_string(), - }], - parent_message_id: None, - conversation_overrides: None, - }); - self.send_request( - mcp_types::CallToolRequest::METHOD, - Some(serde_json::to_value(params)?), - ) - .await - } - - pub async fn send_conversation_create_tool_call( - &mut self, - prompt: &str, - model: &str, - cwd: &str, - ) -> anyhow::Result { - let params = ToolCallRequestParams::ConversationCreate(ConversationCreateArgs { - prompt: prompt.to_string(), - model: model.to_string(), - cwd: cwd.to_string(), - approval_policy: None, - sandbox: None, - config: None, - profile: None, - base_instructions: None, - }); - self.send_request( - mcp_types::CallToolRequest::METHOD, - Some(serde_json::to_value(params)?), - ) - .await - } - - pub async fn send_conversation_create_with_args( - &mut self, - args: ConversationCreateArgs, - ) -> anyhow::Result { - let params = ToolCallRequestParams::ConversationCreate(args); - self.send_request( - mcp_types::CallToolRequest::METHOD, - Some(serde_json::to_value(params)?), - ) - .await - } + // Deprecated tool-call helpers removed. Use newConversation/sendUserMessage APIs instead. // --------------------------------------------------------------------- // Codex JSON-RPC (non-tool) helpers @@ -384,6 +325,33 @@ impl McpProcess { } } + pub async fn read_stream_until_error_message( + &mut self, + request_id: RequestId, + ) -> anyhow::Result { + loop { + let message = self.read_jsonrpc_message().await?; + eprint!("message: {message:?}"); + + match message { + JSONRPCMessage::Notification(_) => { + eprintln!("notification: {message:?}"); + } + JSONRPCMessage::Request(_) => { + anyhow::bail!("unexpected JSONRPCMessage::Request: {message:?}"); + } + JSONRPCMessage::Response(_) => { + // Keep scanning; we're waiting for an error with matching id. + } + JSONRPCMessage::Error(err) => { + if err.id == request_id { + return Ok(err); + } + } + } + } + } + pub async fn read_stream_until_notification_message( &mut self, method: &str, diff --git a/codex-rs/mcp-server/tests/create_conversation.rs b/codex-rs/mcp-server/tests/create_conversation.rs index 2349b0b9e4..5430fd243b 100644 --- a/codex-rs/mcp-server/tests/create_conversation.rs +++ b/codex-rs/mcp-server/tests/create_conversation.rs @@ -1,5 +1,12 @@ use std::path::Path; +use codex_mcp_server::wire_format::AddConversationListenerParams; +use codex_mcp_server::wire_format::AddConversationSubscriptionResponse; +use codex_mcp_server::wire_format::InputItem; +use codex_mcp_server::wire_format::NewConversationParams; +use codex_mcp_server::wire_format::NewConversationResponse; +use codex_mcp_server::wire_format::SendUserMessageParams; +use codex_mcp_server::wire_format::SendUserMessageResponse; use mcp_test_support::McpProcess; use mcp_test_support::create_final_assistant_message_sse_response; use mcp_test_support::create_mock_chat_completions_server; @@ -33,43 +40,64 @@ async fn test_conversation_create_and_send_message_ok() { .expect("init timeout") .expect("init failed"); - // Create a conversation via the new tool. - let req_id = mcp - .send_conversation_create_tool_call("", "o3", "/repo") + // Create a conversation via the new JSON-RPC API. + let new_conv_id = mcp + .send_new_conversation_request(NewConversationParams { + model: Some("o3".to_string()), + ..Default::default() + }) .await - .expect("send conversationCreate"); - - let resp: JSONRPCResponse = timeout( + .expect("send newConversation"); + let new_conv_resp: JSONRPCResponse = timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(req_id)), + mcp.read_stream_until_response_message(RequestId::Integer(new_conv_id)), ) .await - .expect("create response timeout") - .expect("create response error"); + .expect("newConversation timeout") + .expect("newConversation resp"); + let NewConversationResponse { + conversation_id, + model, + } = to_response::(new_conv_resp) + .expect("deserialize newConversation response"); + assert_eq!(model, "o3"); - // Structured content must include status=ok, a UUID conversation_id and the model we passed. - let sc = &resp.result["structuredContent"]; - let conv_id = sc["conversation_id"].as_str().expect("uuid string"); - assert!(!conv_id.is_empty()); - assert_eq!(sc["model"], json!("o3")); - - // Now send a message to the created conversation and expect an OK result. - let send_id = mcp - .send_user_message_tool_call("Hello", conv_id) + // Add a listener so we receive notifications for this conversation (not strictly required for this test). + let add_listener_id = mcp + .send_add_conversation_listener_request(AddConversationListenerParams { conversation_id }) .await - .expect("send message"); + .expect("send addConversationListener"); + let _sub: AddConversationSubscriptionResponse = + to_response::( + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(add_listener_id)), + ) + .await + .expect("addConversationListener timeout") + .expect("addConversationListener resp"), + ) + .expect("deserialize addConversationListener response"); + // Now send a user message via the wire API and expect an OK (empty object) result. + let send_id = mcp + .send_send_user_message_request(SendUserMessageParams { + conversation_id, + items: vec![InputItem::Text { + text: "Hello".to_string(), + }], + }) + .await + .expect("send sendUserMessage"); let send_resp: JSONRPCResponse = timeout( DEFAULT_READ_TIMEOUT, mcp.read_stream_until_response_message(RequestId::Integer(send_id)), ) .await - .expect("send response timeout") - .expect("send response error"); - assert_eq!( - send_resp.result["structuredContent"], - json!({ "status": "ok" }) - ); + .expect("sendUserMessage timeout") + .expect("sendUserMessage resp"); + let _ok: SendUserMessageResponse = to_response::(send_resp) + .expect("deserialize sendUserMessage response"); // avoid race condition by waiting for the mock server to receive the chat.completions request let deadline = std::time::Instant::now() + DEFAULT_READ_TIMEOUT; @@ -101,6 +129,12 @@ async fn test_conversation_create_and_send_message_ok() { drop(server); } +fn to_response(response: JSONRPCResponse) -> anyhow::Result { + let value = serde_json::to_value(response.result)?; + let codex_response = serde_json::from_value(value)?; + Ok(codex_response) +} + // Helper to create a config.toml pointing at the mock model server. fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { let config_toml = codex_home.join("config.toml"); diff --git a/codex-rs/mcp-server/tests/send_message.rs b/codex-rs/mcp-server/tests/send_message.rs index bf2966ef05..686e75709a 100644 --- a/codex-rs/mcp-server/tests/send_message.rs +++ b/codex-rs/mcp-server/tests/send_message.rs @@ -2,15 +2,18 @@ use std::path::Path; use std::thread::sleep; use std::time::Duration; -use codex_mcp_server::CodexToolCallParam; +use codex_mcp_server::wire_format::ConversationId; +use codex_mcp_server::wire_format::InputItem; +use codex_mcp_server::wire_format::NewConversationParams; +use codex_mcp_server::wire_format::NewConversationResponse; +use codex_mcp_server::wire_format::SendUserMessageParams; +use codex_mcp_server::wire_format::SendUserMessageResponse; use mcp_test_support::McpProcess; use mcp_test_support::create_final_assistant_message_sse_response; use mcp_test_support::create_mock_chat_completions_server; -use mcp_types::JSONRPC_VERSION; use mcp_types::JSONRPCResponse; use mcp_types::RequestId; use pretty_assertions::assert_eq; -use serde_json::json; use tempfile::TempDir; use tokio::time::timeout; @@ -39,63 +42,44 @@ async fn test_send_message_success() { .expect("init timed out") .expect("init failed"); - // Kick off a Codex session so we have a valid session_id. - let codex_request_id = mcp_process - .send_codex_tool_call(CodexToolCallParam { - prompt: "Start a session".to_string(), - ..Default::default() - }) + // Start a conversation using the new wire API. + let new_conv_id = mcp_process + .send_new_conversation_request(NewConversationParams::default()) .await - .expect("send codex tool call"); - - // Wait for the session_configured event to get the session_id. - let session_id = mcp_process - .read_stream_until_configured_response_message() - .await - .expect("read session_configured"); - - // The original codex call will finish quickly given our mock; consume its response. - timeout( + .expect("send newConversation"); + let new_conv_resp: JSONRPCResponse = timeout( DEFAULT_READ_TIMEOUT, - mcp_process.read_stream_until_response_message(RequestId::Integer(codex_request_id)), + mcp_process.read_stream_until_response_message(RequestId::Integer(new_conv_id)), ) .await - .expect("codex response timeout") - .expect("codex response error"); + .expect("newConversation timeout") + .expect("newConversation resp"); + let NewConversationResponse { + conversation_id, .. + } = to_response::(new_conv_resp) + .expect("deserialize newConversation response"); - // Now exercise the send-user-message tool. - let send_msg_request_id = mcp_process - .send_user_message_tool_call("Hello again", &session_id) + // Now exercise sendUserMessage. + let send_id = mcp_process + .send_send_user_message_request(SendUserMessageParams { + conversation_id, + items: vec![InputItem::Text { + text: "Hello again".to_string(), + }], + }) .await - .expect("send send-message tool call"); + .expect("send sendUserMessage"); let response: JSONRPCResponse = timeout( DEFAULT_READ_TIMEOUT, - mcp_process.read_stream_until_response_message(RequestId::Integer(send_msg_request_id)), + mcp_process.read_stream_until_response_message(RequestId::Integer(send_id)), ) .await - .expect("send-user-message response timeout") - .expect("send-user-message response error"); + .expect("sendUserMessage response timeout") + .expect("sendUserMessage response error"); - assert_eq!( - JSONRPCResponse { - jsonrpc: JSONRPC_VERSION.into(), - id: RequestId::Integer(send_msg_request_id), - result: json!({ - "content": [ - { - "text": "{\"status\":\"ok\"}", - "type": "text", - } - ], - "isError": false, - "structuredContent": { - "status": "ok" - } - }), - }, - response - ); + let _ok: SendUserMessageResponse = to_response::(response) + .expect("deserialize sendUserMessage response"); // wait for the server to hear the user message sleep(Duration::from_secs(5)); @@ -113,24 +97,26 @@ async fn test_send_message_session_not_found() { .expect("timeout") .expect("init"); - let unknown = uuid::Uuid::new_v4().to_string(); + let unknown = ConversationId(uuid::Uuid::new_v4()); let req_id = mcp - .send_user_message_tool_call("ping", &unknown) + .send_send_user_message_request(SendUserMessageParams { + conversation_id: unknown, + items: vec![InputItem::Text { + text: "ping".to_string(), + }], + }) .await - .expect("send tool"); + .expect("send sendUserMessage"); - let resp: JSONRPCResponse = timeout( + // Expect an error response for unknown conversation. + let err = timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(req_id)), + mcp.read_stream_until_error_message(RequestId::Integer(req_id)), ) .await .expect("timeout") - .expect("resp"); - - let result = resp.result.clone(); - let content = result["content"][0]["text"].as_str().unwrap_or(""); - assert!(content.contains("Session does not exist")); - assert_eq!(result["isError"], json!(true)); + .expect("error"); + assert_eq!(err.id, RequestId::Integer(req_id)); } // --------------------------------------------------------------------------- @@ -159,3 +145,9 @@ stream_max_retries = 0 ), ) } + +fn to_response(response: JSONRPCResponse) -> anyhow::Result { + let value = serde_json::to_value(response.result)?; + let codex_response = serde_json::from_value(value)?; + Ok(codex_response) +}