From 08ed618f72d61702ce1e2ce6b868ddfe0c848ef5 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 13:38:18 -0700 Subject: [PATCH 001/135] chore: introduce ConversationManager as a clearinghouse for all conversations (#2240) This PR does two things because after I got deep into the first one I started pulling on the thread to the second: - Makes `ConversationManager` the place where all in-memory conversations are created and stored. Previously, `MessageProcessor` in the `codex-mcp-server` crate was doing this via its `session_map`, but this is something that should be done in `codex-core`. - It unwinds the `ctrl_c: tokio::sync::Notify` that was threaded throughout our code. I think this made sense at one time, but now that we handle Ctrl-C within the TUI and have a proper `Op::Interrupt` event, I don't think this was quite right, so I removed it. For `codex exec` and `codex proto`, we now use `tokio::signal::ctrl_c()` directly, but we no longer make `Notify` a field of `Codex` or `CodexConversation`. Changes of note: - Adds the files `conversation_manager.rs` and `codex_conversation.rs` to `codex-core`. - `Codex` and `CodexSpawnOk` are no longer exported from `codex-core`: other crates must use `CodexConversation` instead (which is created via `ConversationManager`). - `core/src/codex_wrapper.rs` has been deleted in favor of `ConversationManager`. - `ConversationManager::new_conversation()` returns `NewConversation`, which is in line with the `new_conversation` tool we want to add to the MCP server. Note `NewConversation` includes `SessionConfiguredEvent`, so we eliminate checks in cases like `codex-rs/core/tests/client.rs` to verify `SessionConfiguredEvent` is the first event because that is now internal to `ConversationManager`. - Quite a bit of code was deleted from `codex-rs/mcp-server/src/message_processor.rs` since it no longer has to manage multiple conversations itself: it goes through `ConversationManager` instead. - `core/tests/live_agent.rs` has been deleted because I had to update a bunch of tests and all the tests in here were ignored, and I don't think anyone ever ran them, so this was just technical debt, at this point. - Removed `notify_on_sigint()` from `util.rs` (and in a follow-up, I hope to refactor the blandly-named `util.rs` into more descriptive files). - In general, I started replacing local variables named `codex` as `conversation`, where appropriate, though admittedly I didn't do it through all the integration tests because that would have added a lot of noise to this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2240). * #2264 * #2263 * __->__ #2240 --- codex-rs/cli/src/proto.rs | 47 ++-- codex-rs/core/src/codex.rs | 39 +--- codex-rs/core/src/codex_conversation.rs | 30 +++ codex-rs/core/src/codex_wrapper.rs | 59 ----- codex-rs/core/src/conversation_manager.rs | 96 ++++++++ codex-rs/core/src/error.rs | 7 + codex-rs/core/src/exec.rs | 16 +- codex-rs/core/src/lib.rs | 8 +- codex-rs/core/src/shell.rs | 4 - codex-rs/core/src/util.rs | 21 -- codex-rs/core/tests/client.rs | 117 +++++----- codex-rs/core/tests/common/lib.rs | 5 +- codex-rs/core/tests/compact.rs | 17 +- codex-rs/core/tests/exec.rs | 5 +- codex-rs/core/tests/exec_stream_events.rs | 6 - codex-rs/core/tests/live_agent.rs | 209 ------------------ codex-rs/core/tests/prompt_caching.rs | 17 +- .../tests/stream_error_allows_next_turn.rs | 16 +- codex-rs/core/tests/stream_no_completed.rs | 17 +- codex-rs/exec/src/lib.rs | 42 ++-- codex-rs/linux-sandbox/tests/landlock.rs | 6 - codex-rs/mcp-server/src/codex_tool_runner.rs | 51 +++-- codex-rs/mcp-server/src/conversation_loop.rs | 4 +- codex-rs/mcp-server/src/exec_approval.rs | 6 +- codex-rs/mcp-server/src/message_processor.rs | 73 +++--- codex-rs/mcp-server/src/patch_approval.rs | 6 +- .../src/tool_handlers/create_conversation.rs | 62 ++---- .../src/tool_handlers/send_message.rs | 20 +- codex-rs/tui/src/app.rs | 8 + codex-rs/tui/src/chatwidget.rs | 5 +- codex-rs/tui/src/chatwidget/agent.rs | 38 ++-- codex-rs/tui/src/chatwidget/tests.rs | 3 +- 32 files changed, 406 insertions(+), 654 deletions(-) create mode 100644 codex-rs/core/src/codex_conversation.rs delete mode 100644 codex-rs/core/src/codex_wrapper.rs create mode 100644 codex-rs/core/src/conversation_manager.rs delete mode 100644 codex-rs/core/tests/live_agent.rs diff --git a/codex-rs/cli/src/proto.rs b/codex-rs/cli/src/proto.rs index 6c1de7eaa9e..3bc4d81618d 100644 --- a/codex-rs/cli/src/proto.rs +++ b/codex-rs/cli/src/proto.rs @@ -1,15 +1,14 @@ use std::io::IsTerminal; -use std::sync::Arc; use clap::Parser; use codex_common::CliConfigOverrides; -use codex_core::Codex; -use codex_core::CodexSpawnOk; +use codex_core::ConversationManager; +use codex_core::NewConversation; use codex_core::config::Config; use codex_core::config::ConfigOverrides; +use codex_core::protocol::Event; +use codex_core::protocol::EventMsg; use codex_core::protocol::Submission; -use codex_core::util::notify_on_sigint; -use codex_login::CodexAuth; use tokio::io::AsyncBufReadExt; use tokio::io::BufReader; use tracing::error; @@ -36,22 +35,38 @@ pub async fn run_main(opts: ProtoCli) -> anyhow::Result<()> { .map_err(anyhow::Error::msg)?; let config = Config::load_with_cli_overrides(overrides_vec, ConfigOverrides::default())?; - let auth = CodexAuth::from_codex_home(&config.codex_home)?; - let ctrl_c = notify_on_sigint(); - let CodexSpawnOk { codex, .. } = Codex::spawn(config, auth, ctrl_c.clone()).await?; - let codex = Arc::new(codex); + // Use conversation_manager API to start a conversation + let conversation_manager = ConversationManager::default(); + let NewConversation { + conversation_id: _, + conversation, + session_configured, + } = conversation_manager.new_conversation(config).await?; + + // Simulate streaming the session_configured event. + let synthetic_event = Event { + // Fake id value. + id: "".to_string(), + msg: EventMsg::SessionConfigured(session_configured), + }; + let session_configured_event = match serde_json::to_string(&synthetic_event) { + Ok(s) => s, + Err(e) => { + error!("Failed to serialize session_configured: {e}"); + return Err(anyhow::Error::from(e)); + } + }; + println!("{session_configured_event}"); // Task that reads JSON lines from stdin and forwards to Submission Queue let sq_fut = { - let codex = codex.clone(); - let ctrl_c = ctrl_c.clone(); + let conversation = conversation.clone(); async move { let stdin = BufReader::new(tokio::io::stdin()); let mut lines = stdin.lines(); loop { let result = tokio::select! { - _ = ctrl_c.notified() => { - info!("Interrupted, exiting"); + _ = tokio::signal::ctrl_c() => { break }, res = lines.next_line() => res, @@ -65,7 +80,7 @@ pub async fn run_main(opts: ProtoCli) -> anyhow::Result<()> { } match serde_json::from_str::(line) { Ok(sub) => { - if let Err(e) = codex.submit_with_id(sub).await { + if let Err(e) = conversation.submit_with_id(sub).await { error!("{e:#}"); break; } @@ -88,8 +103,8 @@ pub async fn run_main(opts: ProtoCli) -> anyhow::Result<()> { let eq_fut = async move { loop { let event = tokio::select! { - _ = ctrl_c.notified() => break, - event = codex.next_event() => event, + _ = tokio::signal::ctrl_c() => break, + event = conversation.next_event() => event, }; match event { Ok(event) => { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index b0905e34aba..5ac1392beec 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -20,7 +20,6 @@ use futures::prelude::*; use mcp_types::CallToolResult; use serde::Serialize; use serde_json; -use tokio::sync::Notify; use tokio::sync::oneshot; use tokio::task::AbortHandle; use tracing::debug; @@ -124,11 +123,7 @@ pub struct CodexSpawnOk { impl Codex { /// Spawn a new [`Codex`] and initialize the session. - pub async fn spawn( - config: Config, - auth: Option, - ctrl_c: Arc, - ) -> CodexResult { + pub async fn spawn(config: Config, auth: Option) -> CodexResult { // experimental resume path (undocumented) let resume_path = config.experimental_resume.clone(); info!("resume_path: {resume_path:?}"); @@ -156,9 +151,9 @@ impl Codex { // Generate a unique ID for the lifetime of this Codex session. let session_id = Uuid::new_v4(); - tokio::spawn(submission_loop( - session_id, config, auth, rx_sub, tx_event, ctrl_c, - )); + + // This task will run until Op::Shutdown is received. + tokio::spawn(submission_loop(session_id, config, auth, rx_sub, tx_event)); let codex = Codex { next_id: AtomicU64::new(0), tx_sub, @@ -210,7 +205,6 @@ impl Codex { pub(crate) struct Session { client: ModelClient, pub(crate) tx_event: Sender, - ctrl_c: Arc, /// The session's current working directory. All relative paths provided by /// the model as well as sandbox policies are resolved against this path @@ -493,7 +487,6 @@ impl Session { let result = process_exec_tool_call( exec_args.params, exec_args.sandbox_type, - exec_args.ctrl_c, exec_args.sandbox_policy, exec_args.codex_linux_sandbox_exe, exec_args.stdout_stream, @@ -578,7 +571,7 @@ impl Session { .await } - pub fn abort(&self) { + fn abort(&self) { info!("Aborting existing session"); let mut state = self.state.lock().unwrap(); state.pending_approvals.clear(); @@ -709,7 +702,6 @@ async fn submission_loop( auth: Option, rx_sub: Receiver, tx_event: Sender, - ctrl_c: Arc, ) { let mut sess: Option> = None; // shorthand - send an event when there is no active session @@ -724,21 +716,8 @@ async fn submission_loop( tx_event.send(event).await.ok(); }; - loop { - let interrupted = ctrl_c.notified(); - let sub = tokio::select! { - res = rx_sub.recv() => match res { - Ok(sub) => sub, - Err(_) => break, - }, - _ = interrupted => { - if let Some(sess) = sess.as_ref(){ - sess.abort(); - } - continue; - }, - }; - + // To break out of this loop, send Op::Shutdown. + while let Ok(sub) = rx_sub.recv().await { debug!(?sub, "Submission"); match sub.op { Op::Interrupt => { @@ -877,7 +856,6 @@ async fn submission_loop( config.include_plan_tool, ), tx_event: tx_event.clone(), - ctrl_c: Arc::clone(&ctrl_c), user_instructions, base_instructions, approval_policy, @@ -1787,7 +1765,6 @@ fn parse_container_exec_arguments( pub struct ExecInvokeArgs<'a> { pub params: ExecParams, pub sandbox_type: SandboxType, - pub ctrl_c: Arc, pub sandbox_policy: &'a SandboxPolicy, pub codex_linux_sandbox_exe: &'a Option, pub stdout_stream: Option, @@ -1972,7 +1949,6 @@ async fn handle_container_exec_with_params( ExecInvokeArgs { params: params.clone(), sandbox_type, - ctrl_c: sess.ctrl_c.clone(), sandbox_policy: &sess.sandbox_policy, codex_linux_sandbox_exe: &sess.codex_linux_sandbox_exe, stdout_stream: Some(StdoutStream { @@ -2104,7 +2080,6 @@ async fn handle_sandbox_error( ExecInvokeArgs { params, sandbox_type: SandboxType::None, - ctrl_c: sess.ctrl_c.clone(), sandbox_policy: &sess.sandbox_policy, codex_linux_sandbox_exe: &sess.codex_linux_sandbox_exe, stdout_stream: Some(StdoutStream { diff --git a/codex-rs/core/src/codex_conversation.rs b/codex-rs/core/src/codex_conversation.rs new file mode 100644 index 00000000000..d3b00046fc0 --- /dev/null +++ b/codex-rs/core/src/codex_conversation.rs @@ -0,0 +1,30 @@ +use crate::codex::Codex; +use crate::error::Result as CodexResult; +use crate::protocol::Event; +use crate::protocol::Op; +use crate::protocol::Submission; + +pub struct CodexConversation { + codex: Codex, +} + +/// Conduit for the bidirectional stream of messages that compose a conversation +/// in Codex. +impl CodexConversation { + pub(crate) fn new(codex: Codex) -> Self { + Self { codex } + } + + pub async fn submit(&self, op: Op) -> CodexResult { + self.codex.submit(op).await + } + + /// Use sparingly: this is intended to be removed soon. + pub async fn submit_with_id(&self, sub: Submission) -> CodexResult<()> { + self.codex.submit_with_id(sub).await + } + + pub async fn next_event(&self) -> CodexResult { + self.codex.next_event().await + } +} diff --git a/codex-rs/core/src/codex_wrapper.rs b/codex-rs/core/src/codex_wrapper.rs deleted file mode 100644 index dc10ec8d847..00000000000 --- a/codex-rs/core/src/codex_wrapper.rs +++ /dev/null @@ -1,59 +0,0 @@ -use std::sync::Arc; - -use crate::Codex; -use crate::CodexSpawnOk; -use crate::config::Config; -use crate::protocol::Event; -use crate::protocol::EventMsg; -use crate::util::notify_on_sigint; -use codex_login::CodexAuth; -use tokio::sync::Notify; -use uuid::Uuid; - -/// Represents an active Codex conversation, including the first event -/// (which is [`EventMsg::SessionConfigured`]). -pub struct CodexConversation { - pub codex: Codex, - pub session_id: Uuid, - pub session_configured: Event, - pub ctrl_c: Arc, -} - -/// Spawn a new [`Codex`] and initialize the session. -/// -/// Returns the wrapped [`Codex`] **and** the `SessionInitialized` event that -/// is received as a response to the initial `ConfigureSession` submission so -/// that callers can surface the information to the UI. -pub async fn init_codex(config: Config) -> anyhow::Result { - let ctrl_c = notify_on_sigint(); - let auth = CodexAuth::from_codex_home(&config.codex_home)?; - let CodexSpawnOk { - codex, - init_id, - session_id, - } = Codex::spawn(config, auth, ctrl_c.clone()).await?; - - // The first event must be `SessionInitialized`. Validate and forward it to - // the caller so that they can display it in the conversation history. - let event = codex.next_event().await?; - if event.id != init_id - || !matches!( - &event, - Event { - id: _id, - msg: EventMsg::SessionConfigured(_), - } - ) - { - return Err(anyhow::anyhow!( - "expected SessionInitialized but got {event:?}" - )); - } - - Ok(CodexConversation { - codex, - session_id, - session_configured: event, - ctrl_c, - }) -} diff --git a/codex-rs/core/src/conversation_manager.rs b/codex-rs/core/src/conversation_manager.rs new file mode 100644 index 00000000000..4a6cdc1bf18 --- /dev/null +++ b/codex-rs/core/src/conversation_manager.rs @@ -0,0 +1,96 @@ +use std::collections::HashMap; +use std::sync::Arc; + +use codex_login::CodexAuth; +use tokio::sync::RwLock; +use uuid::Uuid; + +use crate::codex::Codex; +use crate::codex::CodexSpawnOk; +use crate::codex_conversation::CodexConversation; +use crate::config::Config; +use crate::error::CodexErr; +use crate::error::Result as CodexResult; +use crate::protocol::Event; +use crate::protocol::EventMsg; +use crate::protocol::SessionConfiguredEvent; + +/// Represents a newly created Codex conversation, including the first event +/// (which is [`EventMsg::SessionConfigured`]). +pub struct NewConversation { + pub conversation_id: Uuid, + pub conversation: Arc, + pub session_configured: SessionConfiguredEvent, +} + +/// [`ConversationManager`] is responsible for creating conversations and +/// maintaining them in memory. +pub struct ConversationManager { + conversations: Arc>>>, +} + +impl Default for ConversationManager { + fn default() -> Self { + Self { + conversations: Arc::new(RwLock::new(HashMap::new())), + } + } +} + +impl ConversationManager { + pub async fn new_conversation(&self, config: Config) -> CodexResult { + let auth = CodexAuth::from_codex_home(&config.codex_home)?; + self.new_conversation_with_auth(config, auth).await + } + + /// Used for integration tests: should not be used by ordinary business + /// logic. + pub async fn new_conversation_with_auth( + &self, + config: Config, + auth: Option, + ) -> CodexResult { + let CodexSpawnOk { + codex, + init_id, + session_id: conversation_id, + } = Codex::spawn(config, auth).await?; + + // The first event must be `SessionInitialized`. Validate and forward it + // to the caller so that they can display it in the conversation + // history. + let event = codex.next_event().await?; + let session_configured = match event { + Event { + id, + msg: EventMsg::SessionConfigured(session_configured), + } if id == init_id => session_configured, + _ => { + return Err(CodexErr::SessionConfiguredNotFirstEvent); + } + }; + + let conversation = Arc::new(CodexConversation::new(codex)); + self.conversations + .write() + .await + .insert(conversation_id, conversation.clone()); + + Ok(NewConversation { + conversation_id, + conversation, + session_configured, + }) + } + + pub async fn get_conversation( + &self, + conversation_id: Uuid, + ) -> CodexResult> { + let conversations = self.conversations.read().await; + conversations + .get(&conversation_id) + .cloned() + .ok_or_else(|| CodexErr::ConversationNotFound(conversation_id)) + } +} diff --git a/codex-rs/core/src/error.rs b/codex-rs/core/src/error.rs index 2931d30636b..4df66633940 100644 --- a/codex-rs/core/src/error.rs +++ b/codex-rs/core/src/error.rs @@ -3,6 +3,7 @@ use serde_json; use std::io; use thiserror::Error; use tokio::task::JoinError; +use uuid::Uuid; pub type Result = std::result::Result; @@ -44,6 +45,12 @@ pub enum CodexErr { #[error("stream disconnected before completion: {0}")] Stream(String), + #[error("no conversation with id: {0}")] + ConversationNotFound(Uuid), + + #[error("session configured event was not the first event in the stream")] + SessionConfiguredNotFirstEvent, + /// Returned by run_command_stream when the spawned child process timed out (10s). #[error("timeout waiting for child process to exit")] Timeout, diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index c964466f786..50bba1b4b7a 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -6,7 +6,6 @@ use std::io; use std::path::Path; use std::path::PathBuf; use std::process::ExitStatus; -use std::sync::Arc; use std::time::Duration; use std::time::Instant; @@ -15,7 +14,6 @@ use tokio::io::AsyncRead; use tokio::io::AsyncReadExt; use tokio::io::BufReader; use tokio::process::Child; -use tokio::sync::Notify; use crate::error::CodexErr; use crate::error::Result; @@ -80,7 +78,6 @@ pub struct StdoutStream { pub async fn process_exec_tool_call( params: ExecParams, sandbox_type: SandboxType, - ctrl_c: Arc, sandbox_policy: &SandboxPolicy, codex_linux_sandbox_exe: &Option, stdout_stream: Option, @@ -89,7 +86,7 @@ pub async fn process_exec_tool_call( let raw_output_result: std::result::Result = match sandbox_type { - SandboxType::None => exec(params, sandbox_policy, ctrl_c, stdout_stream.clone()).await, + SandboxType::None => exec(params, sandbox_policy, stdout_stream.clone()).await, SandboxType::MacosSeatbelt => { let timeout = params.timeout_duration(); let ExecParams { @@ -103,7 +100,7 @@ pub async fn process_exec_tool_call( env, ) .await?; - consume_truncated_output(child, ctrl_c, timeout, stdout_stream.clone()).await + consume_truncated_output(child, timeout, stdout_stream.clone()).await } SandboxType::LinuxSeccomp => { let timeout = params.timeout_duration(); @@ -124,7 +121,7 @@ pub async fn process_exec_tool_call( ) .await?; - consume_truncated_output(child, ctrl_c, timeout, stdout_stream).await + consume_truncated_output(child, timeout, stdout_stream).await } }; let duration = start.elapsed(); @@ -286,7 +283,6 @@ pub struct ExecToolCallOutput { async fn exec( params: ExecParams, sandbox_policy: &SandboxPolicy, - ctrl_c: Arc, stdout_stream: Option, ) -> Result { let timeout = params.timeout_duration(); @@ -311,14 +307,13 @@ async fn exec( env, ) .await?; - consume_truncated_output(child, ctrl_c, timeout, stdout_stream).await + consume_truncated_output(child, timeout, stdout_stream).await } /// Consumes the output of a child process, truncating it so it is suitable for /// use as the output of a `shell` tool call. Also enforces specified timeout. pub(crate) async fn consume_truncated_output( mut child: Child, - ctrl_c: Arc, timeout: Duration, stdout_stream: Option, ) -> Result { @@ -352,7 +347,6 @@ pub(crate) async fn consume_truncated_output( true, )); - let interrupted = ctrl_c.notified(); let exit_status = tokio::select! { result = tokio::time::timeout(timeout, child.wait()) => { match result { @@ -366,7 +360,7 @@ pub(crate) async fn consume_truncated_output( } } } - _ = interrupted => { + _ = tokio::signal::ctrl_c() => { child.start_kill()?; synthetic_exit_status(128 + SIGKILL_CODE) } diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index f3247c38873..aeab49702c1 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -11,9 +11,8 @@ mod chat_completions; mod client; mod client_common; pub mod codex; -pub use codex::Codex; -pub use codex::CodexSpawnOk; -pub mod codex_wrapper; +mod codex_conversation; +pub use codex_conversation::CodexConversation; pub mod config; pub mod config_profile; pub mod config_types; @@ -34,6 +33,9 @@ pub use model_provider_info::ModelProviderInfo; pub use model_provider_info::WireApi; pub use model_provider_info::built_in_model_providers; pub use model_provider_info::create_oss_provider_with_base_url; +mod conversation_manager; +pub use conversation_manager::ConversationManager; +pub use conversation_manager::NewConversation; pub mod model_family; mod models; mod openai_model_info; diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index cc58eb74606..4a4e0146dad 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -167,9 +167,6 @@ mod tests { for (input, expected_cmd, expected_output) in cases { use std::collections::HashMap; use std::path::PathBuf; - use std::sync::Arc; - - use tokio::sync::Notify; use crate::exec::ExecParams; use crate::exec::SandboxType; @@ -219,7 +216,6 @@ mod tests { justification: None, }, SandboxType::None, - Arc::new(Notify::new()), &SandboxPolicy::DangerFullAccess, &None, None, diff --git a/codex-rs/core/src/util.rs b/codex-rs/core/src/util.rs index fb5f45de6fc..e1248a4867d 100644 --- a/codex-rs/core/src/util.rs +++ b/codex-rs/core/src/util.rs @@ -1,32 +1,11 @@ use std::path::Path; -use std::sync::Arc; use std::time::Duration; use rand::Rng; -use tokio::sync::Notify; -use tracing::debug; const INITIAL_DELAY_MS: u64 = 200; const BACKOFF_FACTOR: f64 = 2.0; -/// Make a CancellationToken that is fulfilled when SIGINT occurs. -pub fn notify_on_sigint() -> Arc { - let notify = Arc::new(Notify::new()); - - tokio::spawn({ - let notify = Arc::clone(¬ify); - async move { - loop { - tokio::signal::ctrl_c().await.ok(); - debug!("Keyboard interrupt"); - notify.notify_waiters(); - } - } - }); - - notify -} - pub(crate) fn backoff(attempt: u64) -> Duration { let exp = BACKOFF_FACTOR.powi(attempt.saturating_sub(1) as i32); let base = (INITIAL_DELAY_MS as f64 * exp) as u64; diff --git a/codex-rs/core/tests/client.rs b/codex-rs/core/tests/client.rs index bd7ad2feef9..1bcddf07961 100644 --- a/codex-rs/core/tests/client.rs +++ b/codex-rs/core/tests/client.rs @@ -1,14 +1,13 @@ #![allow(clippy::expect_used, clippy::unwrap_used)] -use codex_core::Codex; -use codex_core::CodexSpawnOk; +use codex_core::ConversationManager; use codex_core::ModelProviderInfo; +use codex_core::NewConversation; use codex_core::WireApi; use codex_core::built_in_model_providers; use codex_core::protocol::EventMsg; use codex_core::protocol::InputItem; use codex_core::protocol::Op; -use codex_core::protocol::SessionConfiguredEvent; use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_login::CodexAuth; use core_test_support::load_default_config_for_test; @@ -90,14 +89,15 @@ async fn includes_session_id_and_model_headers_in_request() { let mut config = load_default_config_for_test(&codex_home); config.model_provider = model_provider; - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("Test API Key")), - ctrl_c.clone(), - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let NewConversation { + conversation: codex, + conversation_id, + session_configured: _, + } = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .expect("create new conversation"); codex .submit(Op::UserInput { @@ -108,13 +108,6 @@ async fn includes_session_id_and_model_headers_in_request() { .await .unwrap(); - let EventMsg::SessionConfigured(SessionConfiguredEvent { session_id, .. }) = - wait_for_event(&codex, |ev| matches!(ev, EventMsg::SessionConfigured(_))).await - else { - unreachable!() - }; - - let current_session_id = Some(session_id.to_string()); wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; // get request from the server @@ -123,10 +116,9 @@ async fn includes_session_id_and_model_headers_in_request() { let request_authorization = request.headers.get("authorization").unwrap(); let request_originator = request.headers.get("originator").unwrap(); - assert!(current_session_id.is_some()); assert_eq!( request_session_id.to_str().unwrap(), - current_session_id.as_ref().unwrap() + conversation_id.to_string() ); assert_eq!(request_originator.to_str().unwrap(), "codex_cli_rs"); assert_eq!( @@ -164,14 +156,12 @@ async fn includes_base_instructions_override_in_request() { config.base_instructions = Some("test instructions".to_string()); config.model_provider = model_provider; - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("Test API Key")), - ctrl_c.clone(), - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .expect("create new conversation") + .conversation; codex .submit(Op::UserInput { @@ -223,14 +213,12 @@ async fn originator_config_override_is_used() { config.model_provider = model_provider; config.internal_originator = Some("my_override".to_string()); - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("Test API Key")), - ctrl_c.clone(), - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .expect("create new conversation") + .conversation; codex .submit(Op::UserInput { @@ -283,11 +271,15 @@ async fn chatgpt_auth_sends_correct_request() { let codex_home = TempDir::new().unwrap(); let mut config = load_default_config_for_test(&codex_home); config.model_provider = model_provider; - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = - Codex::spawn(config, Some(create_dummy_codex_auth()), ctrl_c.clone()) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let NewConversation { + conversation: codex, + conversation_id, + session_configured: _, + } = conversation_manager + .new_conversation_with_auth(config, Some(create_dummy_codex_auth())) + .await + .expect("create new conversation"); codex .submit(Op::UserInput { @@ -298,13 +290,6 @@ async fn chatgpt_auth_sends_correct_request() { .await .unwrap(); - let EventMsg::SessionConfigured(SessionConfiguredEvent { session_id, .. }) = - wait_for_event(&codex, |ev| matches!(ev, EventMsg::SessionConfigured(_))).await - else { - unreachable!() - }; - - let current_session_id = Some(session_id.to_string()); wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; // get request from the server @@ -315,10 +300,9 @@ async fn chatgpt_auth_sends_correct_request() { let request_chatgpt_account_id = request.headers.get("chatgpt-account-id").unwrap(); let request_body = request.body_json::().unwrap(); - assert!(current_session_id.is_some()); assert_eq!( request_session_id.to_str().unwrap(), - current_session_id.as_ref().unwrap() + conversation_id.to_string() ); assert_eq!(request_originator.to_str().unwrap(), "codex_cli_rs"); assert_eq!( @@ -361,14 +345,12 @@ async fn includes_user_instructions_message_in_request() { config.model_provider = model_provider; config.user_instructions = Some("be nice".to_string()); - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("Test API Key")), - ctrl_c.clone(), - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .expect("create new conversation") + .conversation; codex .submit(Op::UserInput { @@ -457,8 +439,12 @@ async fn azure_overrides_assign_properties_used_for_responses_url() { let mut config = load_default_config_for_test(&codex_home); config.model_provider = provider; - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = Codex::spawn(config, None, ctrl_c.clone()).await.unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, None) + .await + .expect("create new conversation") + .conversation; codex .submit(Op::UserInput { @@ -531,11 +517,12 @@ async fn env_var_overrides_loaded_auth() { let mut config = load_default_config_for_test(&codex_home); config.model_provider = provider; - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = - Codex::spawn(config, Some(create_dummy_codex_auth()), ctrl_c.clone()) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(create_dummy_codex_auth())) + .await + .expect("create new conversation") + .conversation; codex .submit(Op::UserInput { diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index 18bae310be9..04b6dc4b621 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -2,6 +2,7 @@ use tempfile::TempDir; +use codex_core::CodexConversation; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config::ConfigToml; @@ -72,7 +73,7 @@ pub fn load_sse_fixture_with_id(path: impl AsRef, id: &str) -> } pub async fn wait_for_event( - codex: &codex_core::Codex, + codex: &CodexConversation, predicate: F, ) -> codex_core::protocol::EventMsg where @@ -83,7 +84,7 @@ where } pub async fn wait_for_event_with_timeout( - codex: &codex_core::Codex, + codex: &CodexConversation, mut predicate: F, wait_time: tokio::time::Duration, ) -> codex_core::protocol::EventMsg diff --git a/codex-rs/core/tests/compact.rs b/codex-rs/core/tests/compact.rs index fa5c81d883a..28b1ca8d74d 100644 --- a/codex-rs/core/tests/compact.rs +++ b/codex-rs/core/tests/compact.rs @@ -1,7 +1,6 @@ #![expect(clippy::unwrap_used)] -use codex_core::Codex; -use codex_core::CodexSpawnOk; +use codex_core::ConversationManager; use codex_core::ModelProviderInfo; use codex_core::built_in_model_providers; use codex_core::protocol::EventMsg; @@ -142,14 +141,12 @@ async fn summarize_context_three_requests_and_instructions() { let home = TempDir::new().unwrap(); let mut config = load_default_config_for_test(&home); config.model_provider = model_provider; - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("dummy")), - ctrl_c.clone(), - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("dummy"))) + .await + .unwrap() + .conversation; // 1) Normal user input – should hit server once. codex diff --git a/codex-rs/core/tests/exec.rs b/codex-rs/core/tests/exec.rs index 9bead7ef60d..ff5d2b7d54d 100644 --- a/codex-rs/core/tests/exec.rs +++ b/codex-rs/core/tests/exec.rs @@ -2,7 +2,6 @@ #![expect(clippy::unwrap_used, clippy::expect_used)] use std::collections::HashMap; -use std::sync::Arc; use codex_core::exec::ExecParams; use codex_core::exec::ExecToolCallOutput; @@ -11,7 +10,6 @@ use codex_core::exec::process_exec_tool_call; use codex_core::protocol::SandboxPolicy; use codex_core::spawn::CODEX_SANDBOX_ENV_VAR; use tempfile::TempDir; -use tokio::sync::Notify; use codex_core::error::Result; @@ -39,10 +37,9 @@ async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result) -> Vec { let mut out = Vec::new(); @@ -57,13 +55,11 @@ async fn test_exec_stdout_stream_events_echo() { justification: None, }; - let ctrl_c = Arc::new(Notify::new()); let policy = SandboxPolicy::new_read_only_policy(); let result = process_exec_tool_call( params, SandboxType::None, - ctrl_c, &policy, &None, Some(stdout_stream), @@ -109,13 +105,11 @@ async fn test_exec_stderr_stream_events_echo() { justification: None, }; - let ctrl_c = Arc::new(Notify::new()); let policy = SandboxPolicy::new_read_only_policy(); let result = process_exec_tool_call( params, SandboxType::None, - ctrl_c, &policy, &None, Some(stdout_stream), diff --git a/codex-rs/core/tests/live_agent.rs b/codex-rs/core/tests/live_agent.rs deleted file mode 100644 index 81b3bb2a12f..00000000000 --- a/codex-rs/core/tests/live_agent.rs +++ /dev/null @@ -1,209 +0,0 @@ -#![expect(clippy::unwrap_used, clippy::expect_used)] - -//! Live integration tests that exercise the full [`Agent`] stack **against the real -//! OpenAI `/v1/responses` API**. These tests complement the lightweight mock‑based -//! unit tests by verifying that the agent can drive an end‑to‑end conversation, -//! stream incremental events, execute function‑call tool invocations and safely -//! chain multiple turns inside a single session – the exact scenarios that have -//! historically been brittle. -//! -//! The live tests are **ignored by default** so CI remains deterministic and free -//! of external dependencies. Developers can opt‑in locally with e.g. -//! -//! ```bash -//! OPENAI_API_KEY=sk‑... cargo test --test live_agent -- --ignored --nocapture -//! ``` -//! -//! Make sure your key has access to the experimental *Responses* API and that -//! any billable usage is acceptable. - -use std::time::Duration; - -use codex_core::Codex; -use codex_core::CodexSpawnOk; -use codex_core::error::CodexErr; -use codex_core::protocol::AgentMessageEvent; -use codex_core::protocol::ErrorEvent; -use codex_core::protocol::EventMsg; -use codex_core::protocol::InputItem; -use codex_core::protocol::Op; -use core_test_support::load_default_config_for_test; -use tempfile::TempDir; -use tokio::sync::Notify; -use tokio::time::timeout; - -fn api_key_available() -> bool { - std::env::var("OPENAI_API_KEY").is_ok() -} - -/// Helper that spawns a fresh Agent and sends the mandatory *ConfigureSession* -/// submission. The caller receives the constructed [`Agent`] plus the unique -/// submission id used for the initialization message. -async fn spawn_codex() -> Result { - assert!( - api_key_available(), - "OPENAI_API_KEY must be set for live tests" - ); - - let codex_home = TempDir::new().unwrap(); - let mut config = load_default_config_for_test(&codex_home); - config.model_provider.request_max_retries = Some(2); - config.model_provider.stream_max_retries = Some(2); - let CodexSpawnOk { codex: agent, .. } = - Codex::spawn(config, None, std::sync::Arc::new(Notify::new())).await?; - - Ok(agent) -} - -/// Verifies that the agent streams incremental *AgentMessage* events **before** -/// emitting `TaskComplete` and that a second task inside the same session does -/// not get tripped up by a stale `previous_response_id`. -#[ignore] -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn live_streaming_and_prev_id_reset() { - if !api_key_available() { - eprintln!("skipping live_streaming_and_prev_id_reset – OPENAI_API_KEY not set"); - return; - } - - let codex = spawn_codex().await.unwrap(); - - // ---------- Task 1 ---------- - codex - .submit(Op::UserInput { - items: vec![InputItem::Text { - text: "Say the words 'stream test'".into(), - }], - }) - .await - .unwrap(); - - let mut saw_message_before_complete = false; - loop { - let ev = timeout(Duration::from_secs(60), codex.next_event()) - .await - .expect("timeout waiting for task1 events") - .expect("agent closed"); - - match ev.msg { - EventMsg::AgentMessage(_) => saw_message_before_complete = true, - EventMsg::TaskComplete(_) => break, - EventMsg::Error(ErrorEvent { message }) => { - panic!("agent reported error in task1: {message}") - } - _ => { - // Ignore other events. - } - } - } - - assert!( - saw_message_before_complete, - "Agent did not stream any AgentMessage before TaskComplete" - ); - - // ---------- Task 2 (same session) ---------- - codex - .submit(Op::UserInput { - items: vec![InputItem::Text { - text: "Respond with exactly: second turn succeeded".into(), - }], - }) - .await - .unwrap(); - - let mut got_expected = false; - loop { - let ev = timeout(Duration::from_secs(60), codex.next_event()) - .await - .expect("timeout waiting for task2 events") - .expect("agent closed"); - - match &ev.msg { - EventMsg::AgentMessage(AgentMessageEvent { message }) - if message.contains("second turn succeeded") => - { - got_expected = true; - } - EventMsg::TaskComplete(_) => break, - EventMsg::Error(ErrorEvent { message }) => { - panic!("agent reported error in task2: {message}") - } - _ => { - // Ignore other events. - } - } - } - - assert!(got_expected, "second task did not receive expected answer"); -} - -/// Exercises a *function‑call → shell execution* round‑trip by instructing the -/// model to run a harmless `echo` command. The test asserts that: -/// 1. the function call is executed (we see `ExecCommandBegin`/`End` events) -/// 2. the captured stdout reaches the client unchanged. -#[ignore] -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn live_shell_function_call() { - if !api_key_available() { - eprintln!("skipping live_shell_function_call – OPENAI_API_KEY not set"); - return; - } - - let codex = spawn_codex().await.unwrap(); - - const MARKER: &str = "codex_live_echo_ok"; - - codex - .submit(Op::UserInput { - items: vec![InputItem::Text { - text: format!( - "Use the shell function to run the command `echo {MARKER}` and no other commands." - ), - }], - }) - .await - .unwrap(); - - let mut saw_begin = false; - let mut saw_end_with_output = false; - - loop { - let ev = timeout(Duration::from_secs(60), codex.next_event()) - .await - .expect("timeout waiting for function‑call events") - .expect("agent closed"); - - match ev.msg { - EventMsg::ExecCommandBegin(codex_core::protocol::ExecCommandBeginEvent { - command, - .. - }) => { - assert_eq!(command, vec!["echo", MARKER]); - saw_begin = true; - } - EventMsg::ExecCommandEnd(codex_core::protocol::ExecCommandEndEvent { - stdout, - exit_code, - .. - }) => { - assert_eq!(exit_code, 0, "echo returned non‑zero exit code"); - assert!(stdout.contains(MARKER)); - saw_end_with_output = true; - } - EventMsg::TaskComplete(_) => break, - EventMsg::Error(codex_core::protocol::ErrorEvent { message }) => { - panic!("agent error during shell test: {message}") - } - _ => { - // Ignore other events. - } - } - } - - assert!(saw_begin, "ExecCommandBegin event missing"); - assert!( - saw_end_with_output, - "ExecCommandEnd with expected output missing" - ); -} diff --git a/codex-rs/core/tests/prompt_caching.rs b/codex-rs/core/tests/prompt_caching.rs index f460fc30042..8df7ea353d4 100644 --- a/codex-rs/core/tests/prompt_caching.rs +++ b/codex-rs/core/tests/prompt_caching.rs @@ -1,7 +1,6 @@ #![allow(clippy::expect_used, clippy::unwrap_used)] -use codex_core::Codex; -use codex_core::CodexSpawnOk; +use codex_core::ConversationManager; use codex_core::ModelProviderInfo; use codex_core::built_in_model_providers; use codex_core::protocol::EventMsg; @@ -55,14 +54,12 @@ async fn prefixes_context_and_instructions_once_and_consistently_across_requests config.model_provider = model_provider; config.user_instructions = Some("be consistent and helpful".to_string()); - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("Test API Key")), - ctrl_c.clone(), - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .expect("create new conversation") + .conversation; codex .submit(Op::UserInput { diff --git a/codex-rs/core/tests/stream_error_allows_next_turn.rs b/codex-rs/core/tests/stream_error_allows_next_turn.rs index 1500c789e97..d590c433121 100644 --- a/codex-rs/core/tests/stream_error_allows_next_turn.rs +++ b/codex-rs/core/tests/stream_error_allows_next_turn.rs @@ -1,7 +1,6 @@ use std::time::Duration; -use codex_core::Codex; -use codex_core::CodexSpawnOk; +use codex_core::ConversationManager; use codex_core::ModelProviderInfo; use codex_core::WireApi; use codex_core::protocol::EventMsg; @@ -90,13 +89,12 @@ async fn continue_after_stream_error() { config.base_instructions = Some("You are a helpful assistant".to_string()); config.model_provider = provider; - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("Test API Key")), - std::sync::Arc::new(tokio::sync::Notify::new()), - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .unwrap() + .conversation; codex .submit(Op::UserInput { diff --git a/codex-rs/core/tests/stream_no_completed.rs b/codex-rs/core/tests/stream_no_completed.rs index 0ded3337abe..5fa8ab8ba15 100644 --- a/codex-rs/core/tests/stream_no_completed.rs +++ b/codex-rs/core/tests/stream_no_completed.rs @@ -3,8 +3,7 @@ use std::time::Duration; -use codex_core::Codex; -use codex_core::CodexSpawnOk; +use codex_core::ConversationManager; use codex_core::ModelProviderInfo; use codex_core::protocol::EventMsg; use codex_core::protocol::InputItem; @@ -93,17 +92,15 @@ async fn retries_on_early_close() { requires_openai_auth: false, }; - let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); let codex_home = TempDir::new().unwrap(); let mut config = load_default_config_for_test(&codex_home); config.model_provider = model_provider; - let CodexSpawnOk { codex, .. } = Codex::spawn( - config, - Some(CodexAuth::from_api_key("Test API Key")), - ctrl_c, - ) - .await - .unwrap(); + let conversation_manager = ConversationManager::default(); + let codex = conversation_manager + .new_conversation_with_auth(config, Some(CodexAuth::from_api_key("Test API Key"))) + .await + .unwrap() + .conversation; codex .submit(Op::UserInput { diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 6ed57898b2d..ff6123d74b3 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -6,12 +6,11 @@ mod event_processor_with_json_output; use std::io::IsTerminal; use std::io::Read; use std::path::PathBuf; -use std::sync::Arc; pub use cli::Cli; use codex_core::BUILT_IN_OSS_MODEL_PROVIDER_ID; -use codex_core::codex_wrapper::CodexConversation; -use codex_core::codex_wrapper::{self}; +use codex_core::ConversationManager; +use codex_core::NewConversation; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config_types::SandboxMode; @@ -185,35 +184,30 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any std::process::exit(1); } - let CodexConversation { - codex: codex_wrapper, + let conversation_manager = ConversationManager::default(); + let NewConversation { + conversation_id: _, + conversation, session_configured, - ctrl_c, - .. - } = codex_wrapper::init_codex(config).await?; - let codex = Arc::new(codex_wrapper); + } = conversation_manager.new_conversation(config).await?; info!("Codex initialized with event: {session_configured:?}"); let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel::(); { - let codex = codex.clone(); + let conversation = conversation.clone(); tokio::spawn(async move { loop { - let interrupted = ctrl_c.notified(); tokio::select! { - _ = interrupted => { - // Forward an interrupt to the codex so it can abort any in‑flight task. - let _ = codex - .submit( - Op::Interrupt, - ) - .await; + _ = tokio::signal::ctrl_c() => { + tracing::debug!("Keyboard interrupt"); + // Immediately notify Codex to abort any in‑flight task. + conversation.submit(Op::Interrupt).await.ok(); - // Exit the inner loop and return to the main input prompt. The codex + // Exit the inner loop and return to the main input prompt. The codex // will emit a `TurnInterrupted` (Error) event which is drained later. break; } - res = codex.next_event() => match res { + res = conversation.next_event() => match res { Ok(event) => { debug!("Received event: {event:?}"); @@ -243,9 +237,9 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any .into_iter() .map(|path| InputItem::LocalImage { path }) .collect(); - let initial_images_event_id = codex.submit(Op::UserInput { items }).await?; + let initial_images_event_id = conversation.submit(Op::UserInput { items }).await?; info!("Sent images with event ID: {initial_images_event_id}"); - while let Ok(event) = codex.next_event().await { + while let Ok(event) = conversation.next_event().await { if event.id == initial_images_event_id && matches!( event.msg, @@ -261,7 +255,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any // Send the prompt. let items: Vec = vec![InputItem::Text { text: prompt }]; - let initial_prompt_task_id = codex.submit(Op::UserInput { items }).await?; + let initial_prompt_task_id = conversation.submit(Op::UserInput { items }).await?; info!("Sent prompt with event ID: {initial_prompt_task_id}"); // Run the loop until the task is complete. @@ -270,7 +264,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any match shutdown { CodexStatus::Running => continue, CodexStatus::InitiateShutdown => { - codex.submit(Op::Shutdown).await?; + conversation.submit(Op::Shutdown).await?; } CodexStatus::Shutdown => { break; diff --git a/codex-rs/linux-sandbox/tests/landlock.rs b/codex-rs/linux-sandbox/tests/landlock.rs index c7081dbca2f..76de2547a6d 100644 --- a/codex-rs/linux-sandbox/tests/landlock.rs +++ b/codex-rs/linux-sandbox/tests/landlock.rs @@ -11,9 +11,7 @@ use codex_core::exec_env::create_env; use codex_core::protocol::SandboxPolicy; use std::collections::HashMap; use std::path::PathBuf; -use std::sync::Arc; use tempfile::NamedTempFile; -use tokio::sync::Notify; // At least on GitHub CI, the arm64 tests appear to need longer timeouts. @@ -59,11 +57,9 @@ async fn run_cmd(cmd: &[&str], writable_roots: &[PathBuf], timeout_ms: u64) { }; let sandbox_program = env!("CARGO_BIN_EXE_codex-linux-sandbox"); let codex_linux_sandbox_exe = Some(PathBuf::from(sandbox_program)); - let ctrl_c = Arc::new(Notify::new()); let res = process_exec_tool_call( params, SandboxType::LinuxSeccomp, - ctrl_c, &sandbox_policy, &codex_linux_sandbox_exe, None, @@ -150,13 +146,11 @@ async fn assert_network_blocked(cmd: &[&str]) { }; let sandbox_policy = SandboxPolicy::new_read_only_policy(); - let ctrl_c = Arc::new(Notify::new()); let sandbox_program = env!("CARGO_BIN_EXE_codex-linux-sandbox"); let codex_linux_sandbox_exe: Option = Some(PathBuf::from(sandbox_program)); let result = process_exec_tool_call( params, SandboxType::LinuxSeccomp, - ctrl_c, &sandbox_policy, &codex_linux_sandbox_exe, None, diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index edb5a205e8a..ced01539bb3 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -5,12 +5,13 @@ use std::collections::HashMap; use std::sync::Arc; -use codex_core::Codex; -use codex_core::codex_wrapper::CodexConversation; -use codex_core::codex_wrapper::init_codex; +use codex_core::CodexConversation; +use codex_core::ConversationManager; +use codex_core::NewConversation; use codex_core::config::Config as CodexConfig; use codex_core::protocol::AgentMessageEvent; use codex_core::protocol::ApplyPatchApprovalRequestEvent; +use codex_core::protocol::Event; use codex_core::protocol::EventMsg; use codex_core::protocol::ExecApprovalRequestEvent; use codex_core::protocol::InputItem; @@ -41,15 +42,14 @@ pub async fn run_codex_tool_session( initial_prompt: String, config: CodexConfig, outgoing: Arc, - session_map: Arc>>>, + conversation_manager: Arc, running_requests_id_to_codex_uuid: Arc>>, ) { - let CodexConversation { - codex, + let NewConversation { + conversation_id, + conversation, session_configured, - session_id, - .. - } = match init_codex(config).await { + } = match conversation_manager.new_conversation(config).await { Ok(res) => res, Err(e) => { let result = CallToolResult { @@ -65,16 +65,15 @@ pub async fn run_codex_tool_session( return; } }; - let codex = Arc::new(codex); - - // update the session map so we can retrieve the session in a reply, and then drop it, since - // we no longer need it for this function - session_map.lock().await.insert(session_id, codex.clone()); - drop(session_map); + let session_configured_event = Event { + // Use a fake id value for now. + id: "".to_string(), + msg: EventMsg::SessionConfigured(session_configured.clone()), + }; outgoing .send_event_as_notification( - &session_configured, + &session_configured_event, Some(OutgoingNotificationMeta::new(Some(id.clone()))), ) .await; @@ -89,7 +88,7 @@ pub async fn run_codex_tool_session( running_requests_id_to_codex_uuid .lock() .await - .insert(id.clone(), session_id); + .insert(id.clone(), conversation_id); let submission = Submission { id: sub_id.clone(), op: Op::UserInput { @@ -99,18 +98,24 @@ pub async fn run_codex_tool_session( }, }; - if let Err(e) = codex.submit_with_id(submission).await { + if let Err(e) = conversation.submit_with_id(submission).await { tracing::error!("Failed to submit initial prompt: {e}"); // unregister the id so we don't keep it in the map running_requests_id_to_codex_uuid.lock().await.remove(&id); return; } - run_codex_tool_session_inner(codex, outgoing, id, running_requests_id_to_codex_uuid).await; + run_codex_tool_session_inner( + conversation, + outgoing, + id, + running_requests_id_to_codex_uuid, + ) + .await; } pub async fn run_codex_tool_session_reply( - codex: Arc, + conversation: Arc, outgoing: Arc, request_id: RequestId, prompt: String, @@ -121,7 +126,7 @@ pub async fn run_codex_tool_session_reply( .lock() .await .insert(request_id.clone(), session_id); - if let Err(e) = codex + if let Err(e) = conversation .submit(Op::UserInput { items: vec![InputItem::Text { text: prompt }], }) @@ -137,7 +142,7 @@ pub async fn run_codex_tool_session_reply( } run_codex_tool_session_inner( - codex, + conversation, outgoing, request_id, running_requests_id_to_codex_uuid, @@ -146,7 +151,7 @@ pub async fn run_codex_tool_session_reply( } async fn run_codex_tool_session_inner( - codex: Arc, + codex: Arc, outgoing: Arc, request_id: RequestId, running_requests_id_to_codex_uuid: Arc>>, diff --git a/codex-rs/mcp-server/src/conversation_loop.rs b/codex-rs/mcp-server/src/conversation_loop.rs index d5c414bf745..61f0c95ad43 100644 --- a/codex-rs/mcp-server/src/conversation_loop.rs +++ b/codex-rs/mcp-server/src/conversation_loop.rs @@ -4,7 +4,7 @@ use crate::exec_approval::handle_exec_approval_request; use crate::outgoing_message::OutgoingMessageSender; use crate::outgoing_message::OutgoingNotificationMeta; use crate::patch_approval::handle_patch_approval_request; -use codex_core::Codex; +use codex_core::CodexConversation; use codex_core::protocol::AgentMessageEvent; use codex_core::protocol::ApplyPatchApprovalRequestEvent; use codex_core::protocol::EventMsg; @@ -13,7 +13,7 @@ use mcp_types::RequestId; use tracing::error; pub async fn run_conversation_loop( - codex: Arc, + codex: Arc, outgoing: Arc, request_id: RequestId, ) { diff --git a/codex-rs/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs index 54abfb35bd0..1985cc79e69 100644 --- a/codex-rs/mcp-server/src/exec_approval.rs +++ b/codex-rs/mcp-server/src/exec_approval.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use std::sync::Arc; -use codex_core::Codex; +use codex_core::CodexConversation; use codex_core::protocol::Op; use codex_core::protocol::ReviewDecision; use mcp_types::ElicitRequest; @@ -51,7 +51,7 @@ pub(crate) async fn handle_exec_approval_request( command: Vec, cwd: PathBuf, outgoing: Arc, - codex: Arc, + codex: Arc, request_id: RequestId, tool_call_id: String, event_id: String, @@ -116,7 +116,7 @@ pub(crate) async fn handle_exec_approval_request( async fn on_exec_approval_response( event_id: String, receiver: tokio::sync::oneshot::Receiver, - codex: Arc, + codex: Arc, ) { let response = receiver.await; let value = match response { diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 2f99cda7230..a3349aeb356 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -14,7 +14,7 @@ use crate::outgoing_message::OutgoingMessageSender; use crate::tool_handlers::create_conversation::handle_create_conversation; use crate::tool_handlers::send_message::handle_send_message; -use codex_core::Codex; +use codex_core::ConversationManager; use codex_core::config::Config as CodexConfig; use codex_core::protocol::Submission; use mcp_types::CallToolRequest; @@ -42,7 +42,7 @@ pub(crate) struct MessageProcessor { outgoing: Arc, initialized: bool, codex_linux_sandbox_exe: Option, - session_map: Arc>>>, + conversation_manager: Arc, running_requests_id_to_codex_uuid: Arc>>, running_session_ids: Arc>>, } @@ -58,14 +58,14 @@ impl MessageProcessor { outgoing: Arc::new(outgoing), initialized: false, codex_linux_sandbox_exe, - session_map: Arc::new(Mutex::new(HashMap::new())), + conversation_manager: Arc::new(ConversationManager::default()), running_requests_id_to_codex_uuid: Arc::new(Mutex::new(HashMap::new())), running_session_ids: Arc::new(Mutex::new(HashSet::new())), } } - pub(crate) fn session_map(&self) -> Arc>>> { - self.session_map.clone() + pub(crate) fn get_conversation_manager(&self) -> &ConversationManager { + &self.conversation_manager } pub(crate) fn outgoing(&self) -> Arc { @@ -431,9 +431,9 @@ impl MessageProcessor { } }; - // Clone outgoing and session map to move into async task. + // Clone outgoing and server to move into async task. let outgoing = self.outgoing.clone(); - let session_map = self.session_map.clone(); + let conversation_manager = self.conversation_manager.clone(); let running_requests_id_to_codex_uuid = self.running_requests_id_to_codex_uuid.clone(); // Spawn an async task to handle the Codex session so that we do not @@ -445,7 +445,7 @@ impl MessageProcessor { initial_prompt, config, outgoing, - session_map, + conversation_manager, running_requests_id_to_codex_uuid, ) .await; @@ -516,33 +516,27 @@ impl MessageProcessor { } }; - // load codex from session map - let session_map_mutex = Arc::clone(&self.session_map); - - // Clone outgoing and session map to move into async task. + // Clone outgoing to move into async task. let outgoing = self.outgoing.clone(); let running_requests_id_to_codex_uuid = self.running_requests_id_to_codex_uuid.clone(); - let codex = { - let session_map = session_map_mutex.lock().await; - match session_map.get(&session_id).cloned() { - Some(c) => c, - None => { - tracing::warn!("Session not found for session_id: {session_id}"); - let result = CallToolResult { - content: vec![ContentBlock::TextContent(TextContent { - r#type: "text".to_owned(), - text: format!("Session not found for session_id: {session_id}"), - annotations: None, - })], - is_error: Some(true), - structured_content: None, - }; - outgoing - .send_response(request_id, serde_json::to_value(result).unwrap_or_default()) - .await; - return; - } + let codex = match self.conversation_manager.get_conversation(session_id).await { + Ok(c) => c, + Err(_) => { + tracing::warn!("Session not found for session_id: {session_id}"); + let result = CallToolResult { + content: vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_owned(), + text: format!("Session not found for session_id: {session_id}"), + annotations: None, + })], + is_error: Some(true), + structured_content: None, + }; + outgoing + .send_response(request_id, serde_json::to_value(result).unwrap_or_default()) + .await; + return; } }; @@ -609,15 +603,12 @@ impl MessageProcessor { }; tracing::info!("session_id: {session_id}"); - // Obtain the Codex Arc while holding the session_map lock, then release. - let codex_arc = { - let sessions_guard = self.session_map.lock().await; - match sessions_guard.get(&session_id) { - Some(codex) => Arc::clone(codex), - None => { - tracing::warn!("Session not found for session_id: {session_id}"); - return; - } + // Obtain the Codex conversation from the server. + let codex_arc = match self.conversation_manager.get_conversation(session_id).await { + Ok(c) => c, + Err(_) => { + tracing::warn!("Session not found for session_id: {session_id}"); + return; } }; diff --git a/codex-rs/mcp-server/src/patch_approval.rs b/codex-rs/mcp-server/src/patch_approval.rs index db99ee5f278..3c614ab3317 100644 --- a/codex-rs/mcp-server/src/patch_approval.rs +++ b/codex-rs/mcp-server/src/patch_approval.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; -use codex_core::Codex; +use codex_core::CodexConversation; use codex_core::protocol::FileChange; use codex_core::protocol::Op; use codex_core::protocol::ReviewDecision; @@ -47,7 +47,7 @@ pub(crate) async fn handle_patch_approval_request( grant_root: Option, changes: HashMap, outgoing: Arc, - codex: Arc, + codex: Arc, request_id: RequestId, tool_call_id: String, event_id: String, @@ -111,7 +111,7 @@ pub(crate) async fn handle_patch_approval_request( pub(crate) async fn on_patch_approval_response( event_id: String, receiver: tokio::sync::oneshot::Receiver, - codex: Arc, + codex: Arc, ) { let response = receiver.await; let value = match response { diff --git a/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs b/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs index 559bf729055..77a68f0128d 100644 --- a/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs +++ b/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs @@ -1,16 +1,9 @@ -use std::collections::HashMap; use std::path::PathBuf; -use std::sync::Arc; -use codex_core::Codex; -use codex_core::codex_wrapper::init_codex; +use codex_core::NewConversation; use codex_core::config::Config as CodexConfig; use codex_core::config::ConfigOverrides; -use codex_core::protocol::EventMsg; -use codex_core::protocol::SessionConfiguredEvent; use mcp_types::RequestId; -use tokio::sync::Mutex; -use uuid::Uuid; use crate::conversation_loop::run_conversation_loop; use crate::json_to_toml::json_to_toml; @@ -81,8 +74,16 @@ pub(crate) async fn handle_create_conversation( } }; - // Initialize Codex session - let codex_conversation = match init_codex(cfg).await { + // 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 @@ -100,41 +101,13 @@ pub(crate) async fn handle_create_conversation( } }; - // Expect SessionConfigured; if not, return error. - let EventMsg::SessionConfigured(SessionConfiguredEvent { model, .. }) = - &codex_conversation.session_configured.msg - else { - message_processor - .send_response_with_optional_error( - id, - Some(ToolCallResponseResult::ConversationCreate( - ConversationCreateResult::Error { - message: "Expected SessionConfigured event".to_string(), - }, - )), - Some(true), - ) - .await; - return; - }; - - let effective_model = model.clone(); - - let session_id = codex_conversation.session_id; - let codex_arc = Arc::new(codex_conversation.codex); + let effective_model = session_configured.model.clone(); - // Store session for future calls - insert_session( - session_id, - codex_arc.clone(), - message_processor.session_map(), - ) - .await; // 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(codex_arc.clone(), outgoing, spawn_id).await; + run_conversation_loop(conversation.clone(), outgoing, spawn_id).await; }); // Reply with the new conversation id and effective model @@ -151,12 +124,3 @@ pub(crate) async fn handle_create_conversation( ) .await; } - -async fn insert_session( - session_id: Uuid, - codex: Arc, - session_map: Arc>>>, -) { - let mut guard = session_map.lock().await; - guard.insert(session_id, codex); -} diff --git a/codex-rs/mcp-server/src/tool_handlers/send_message.rs b/codex-rs/mcp-server/src/tool_handlers/send_message.rs index 894176bef6a..985854f8520 100644 --- a/codex-rs/mcp-server/src/tool_handlers/send_message.rs +++ b/codex-rs/mcp-server/src/tool_handlers/send_message.rs @@ -1,12 +1,6 @@ -use std::collections::HashMap; -use std::sync::Arc; - -use codex_core::Codex; use codex_core::protocol::Op; use codex_core::protocol::Submission; use mcp_types::RequestId; -use tokio::sync::Mutex; -use uuid::Uuid; use crate::mcp_protocol::ConversationSendMessageArgs; use crate::mcp_protocol::ConversationSendMessageResult; @@ -41,7 +35,11 @@ pub(crate) async fn handle_send_message( } let session_id = conversation_id.0; - let Some(codex) = get_session(session_id, message_processor.session_map()).await else { + let Ok(codex) = message_processor + .get_conversation_manager() + .get_conversation(session_id) + .await + else { message_processor .send_response_with_optional_error( id, @@ -114,11 +112,3 @@ pub(crate) async fn handle_send_message( ) .await; } - -pub(crate) async fn get_session( - session_id: Uuid, - session_map: Arc>>>, -) -> Option> { - let guard = session_map.lock().await; - guard.get(&session_id).cloned() -} diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 69d27022fa2..fb45ecfd19e 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -9,6 +9,7 @@ use crate::onboarding::onboarding_screen::OnboardingScreenArgs; use crate::should_show_login_screen; use crate::slash_command::SlashCommand; use crate::tui; +use codex_core::ConversationManager; use codex_core::config::Config; use codex_core::protocol::Event; use codex_core::protocol::Op; @@ -48,6 +49,7 @@ enum AppState<'a> { } pub(crate) struct App<'a> { + server: Arc, app_event_tx: AppEventSender, app_event_rx: Receiver, app_state: AppState<'a>, @@ -85,6 +87,8 @@ impl App<'_> { initial_images: Vec, show_trust_screen: bool, ) -> Self { + let conversation_manager = Arc::new(ConversationManager::default()); + let (app_event_tx, app_event_rx) = channel(); let app_event_tx = AppEventSender::new(app_event_tx); let pending_redraw = Arc::new(AtomicBool::new(false)); @@ -153,6 +157,7 @@ impl App<'_> { } else { let chat_widget = ChatWidget::new( config.clone(), + conversation_manager.clone(), app_event_tx.clone(), initial_prompt, initial_images, @@ -165,6 +170,7 @@ impl App<'_> { let file_search = FileSearchManager::new(config.cwd.clone(), app_event_tx.clone()); Self { + server: conversation_manager, app_event_tx, pending_history_lines: Vec::new(), app_event_rx, @@ -328,6 +334,7 @@ impl App<'_> { // User accepted – switch to chat view. let new_widget = Box::new(ChatWidget::new( self.config.clone(), + self.server.clone(), self.app_event_tx.clone(), None, Vec::new(), @@ -449,6 +456,7 @@ impl App<'_> { self.app_state = AppState::Chat { widget: Box::new(ChatWidget::new( config, + self.server.clone(), app_event_tx.clone(), initial_prompt, initial_images, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 0dc335a23a8..517a9a35f5f 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::path::PathBuf; +use std::sync::Arc; use codex_core::config::Config; use codex_core::parse_command::ParsedCommand; @@ -54,6 +55,7 @@ mod agent; use self::agent::spawn_agent; use crate::streaming::controller::AppEventHistorySink; use crate::streaming::controller::StreamController; +use codex_core::ConversationManager; use codex_file_search::FileMatch; // Track information about an in-flight exec command. @@ -498,12 +500,13 @@ impl ChatWidget<'_> { pub(crate) fn new( config: Config, + conversation_manager: Arc, app_event_tx: AppEventSender, initial_prompt: Option, initial_images: Vec, enhanced_keys_supported: bool, ) -> Self { - let codex_op_tx = spawn_agent(config.clone(), app_event_tx.clone()); + let codex_op_tx = spawn_agent(config.clone(), app_event_tx.clone(), conversation_manager); Self { app_event_tx: app_event_tx.clone(), diff --git a/codex-rs/tui/src/chatwidget/agent.rs b/codex-rs/tui/src/chatwidget/agent.rs index 69e1c19c453..eb96864f0d1 100644 --- a/codex-rs/tui/src/chatwidget/agent.rs +++ b/codex-rs/tui/src/chatwidget/agent.rs @@ -1,7 +1,7 @@ use std::sync::Arc; -use codex_core::codex_wrapper::CodexConversation; -use codex_core::codex_wrapper::init_codex; +use codex_core::ConversationManager; +use codex_core::NewConversation; use codex_core::config::Config; use codex_core::protocol::Op; use tokio::sync::mpsc::UnboundedSender; @@ -12,17 +12,21 @@ use crate::app_event_sender::AppEventSender; /// Spawn the agent bootstrapper and op forwarding loop, returning the /// `UnboundedSender` used by the UI to submit operations. -pub(crate) fn spawn_agent(config: Config, app_event_tx: AppEventSender) -> UnboundedSender { +pub(crate) fn spawn_agent( + config: Config, + app_event_tx: AppEventSender, + server: Arc, +) -> UnboundedSender { let (codex_op_tx, mut codex_op_rx) = unbounded_channel::(); let app_event_tx_clone = app_event_tx.clone(); tokio::spawn(async move { - let CodexConversation { - codex, + let NewConversation { + conversation_id: _, + conversation, session_configured, - .. - } = match init_codex(config).await { - Ok(vals) => vals, + } = match server.new_conversation(config).await { + Ok(v) => v, Err(e) => { // TODO: surface this error to the user. tracing::error!("failed to initialize codex: {e}"); @@ -30,21 +34,25 @@ pub(crate) fn spawn_agent(config: Config, app_event_tx: AppEventSender) -> Unbou } }; - // Forward the captured `SessionInitialized` event that was consumed - // inside `init_codex()` so it can be rendered in the UI. - app_event_tx_clone.send(AppEvent::CodexEvent(session_configured.clone())); - let codex = Arc::new(codex); - let codex_clone = codex.clone(); + // Forward the captured `SessionConfigured` event so it can be rendered in the UI. + let ev = codex_core::protocol::Event { + // The `id` does not matter for rendering, so we can use a fake value. + id: "".to_string(), + msg: codex_core::protocol::EventMsg::SessionConfigured(session_configured), + }; + app_event_tx_clone.send(AppEvent::CodexEvent(ev)); + + let conversation_clone = conversation.clone(); tokio::spawn(async move { while let Some(op) = codex_op_rx.recv().await { - let id = codex_clone.submit(op).await; + let id = conversation_clone.submit(op).await; if let Err(e) = id { tracing::error!("failed to submit op: {e}"); } } }); - while let Ok(event) = codex.next_event().await { + while let Ok(event) = conversation.next_event().await { app_event_tx_clone.send(AppEvent::CodexEvent(event)); } }); diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index adde76e8d6c..0a7dd93368f 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -104,7 +104,8 @@ async fn helpers_are_available_and_do_not_panic() { let (tx_raw, _rx) = channel::(); let tx = AppEventSender::new(tx_raw); let cfg = test_config(); - let mut w = ChatWidget::new(cfg, tx, None, Vec::new(), false); + let conversation_manager = Arc::new(ConversationManager::default()); + let mut w = ChatWidget::new(cfg, conversation_manager, tx, None, Vec::new(), false); // Basic construction sanity. let _ = &mut w; } From 99a242ef41129c852e62cd921ac209e2e58bcf20 Mon Sep 17 00:00:00 2001 From: Dylan Date: Wed, 13 Aug 2025 13:49:27 -0700 Subject: [PATCH 002/135] [codex-cli] Add ripgrep as a dependency for node environment (#2237) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Ripgrep is our preferred tool for file search. When users install via `brew install codex`, it's automatically installed as a dependency. We want to ensure that users running via an npm install also have this tool! Microsoft has already solved this problem for VS Code - let's not reinvent the wheel. This approach of appending to the PATH directly might be a bit heavy-handed, but feels reasonably robust to a variety of environment concerns. Open to thoughts on better approaches here! ## Testing - [x] confirmed this import approach works with `node -e "const { rgPath } = require('@vscode/ripgrep'); require('child_process').spawn(rgPath, ['--version'], { stdio: 'inherit' })"` - [x] Ran codex.js locally with `rg` uninstalled, asked it to run `which rg`. Output below: ``` ⚡ Ran command which rg; echo $? ⎿ /Users/dylan.hurd/code/dh--npm-rg/node_modules/@vscode/ripgrep/bin/rg 0 codex Re-running to confirm the path and exit code. - Path: `/Users/dylan.hurd/code/dh--npm-rg/node_modules/@vscode/ripgrep/bin/rg` - Exit code: `0` ``` --- codex-cli/bin/codex.js | 39 +++++++++++- codex-cli/package-lock.json | 119 ++++++++++++++++++++++++++++++++++++ codex-cli/package.json | 6 ++ package-lock.json | 33 ++++++++++ package.json | 4 +- 5 files changed, 196 insertions(+), 5 deletions(-) create mode 100644 codex-cli/package-lock.json create mode 100644 package-lock.json diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index d92d8f2f4ff..b22c5180c0d 100755 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -43,7 +43,7 @@ switch (platform) { targetTriple = "x86_64-pc-windows-msvc.exe"; break; case "arm64": - // We do not build this today, fall through... + // We do not build this today, fall through... default: break; } @@ -65,9 +65,43 @@ const binaryPath = path.join(__dirname, "..", "bin", `codex-${targetTriple}`); // receives a fatal signal, both processes exit in a predictable manner. const { spawn } = await import("child_process"); +async function tryImport(moduleName) { + try { + // eslint-disable-next-line node/no-unsupported-features/es-syntax + return await import(moduleName); + } catch (err) { + return null; + } +} + +async function resolveRgDir() { + const ripgrep = await tryImport("@vscode/ripgrep"); + if (!ripgrep?.rgPath) { + return null; + } + return path.dirname(ripgrep.rgPath); +} + +function getUpdatedPath(newDirs) { + const pathSep = process.platform === "win32" ? ";" : ":"; + const existingPath = process.env.PATH || ""; + const updatedPath = [ + ...newDirs, + ...existingPath.split(pathSep).filter(Boolean), + ].join(pathSep); + return updatedPath; +} + +const additionalDirs = []; +const rgDir = await resolveRgDir(); +if (rgDir) { + additionalDirs.push(rgDir); +} +const updatedPath = getUpdatedPath(additionalDirs); + const child = spawn(binaryPath, process.argv.slice(2), { stdio: "inherit", - env: { ...process.env, CODEX_MANAGED_BY_NPM: "1" }, + env: { ...process.env, PATH: updatedPath, CODEX_MANAGED_BY_NPM: "1" }, }); child.on("error", (err) => { @@ -120,4 +154,3 @@ if (childResult.type === "signal") { } else { process.exit(childResult.exitCode); } - diff --git a/codex-cli/package-lock.json b/codex-cli/package-lock.json new file mode 100644 index 00000000000..a1c840ade0e --- /dev/null +++ b/codex-cli/package-lock.json @@ -0,0 +1,119 @@ +{ + "name": "@openai/codex", + "version": "0.0.0-dev", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "@openai/codex", + "version": "0.0.0-dev", + "license": "Apache-2.0", + "dependencies": { + "@vscode/ripgrep": "^1.15.14" + }, + "bin": { + "codex": "bin/codex.js" + }, + "engines": { + "node": ">=20" + } + }, + "node_modules/@vscode/ripgrep": { + "version": "1.15.14", + "resolved": "https://registry.npmjs.org/@vscode/ripgrep/-/ripgrep-1.15.14.tgz", + "integrity": "sha512-/G1UJPYlm+trBWQ6cMO3sv6b8D1+G16WaJH1/DSqw32JOVlzgZbLkDxRyzIpTpv30AcYGMkCf5tUqGlW6HbDWw==", + "hasInstallScript": true, + "license": "MIT", + "dependencies": { + "https-proxy-agent": "^7.0.2", + "proxy-from-env": "^1.1.0", + "yauzl": "^2.9.2" + } + }, + "node_modules/agent-base": { + "version": "7.1.4", + "resolved": "https://registry.npmjs.org/agent-base/-/agent-base-7.1.4.tgz", + "integrity": "sha512-MnA+YT8fwfJPgBx3m60MNqakm30XOkyIoH1y6huTQvC0PwZG7ki8NacLBcrPbNoo8vEZy7Jpuk7+jMO+CUovTQ==", + "license": "MIT", + "engines": { + "node": ">= 14" + } + }, + "node_modules/buffer-crc32": { + "version": "0.2.13", + "resolved": "https://registry.npmjs.org/buffer-crc32/-/buffer-crc32-0.2.13.tgz", + "integrity": "sha512-VO9Ht/+p3SN7SKWqcrgEzjGbRSJYTx+Q1pTQC0wrWqHx0vpJraQ6GtHx8tvcg1rlK1byhU5gccxgOgj7B0TDkQ==", + "license": "MIT", + "engines": { + "node": "*" + } + }, + "node_modules/debug": { + "version": "4.4.1", + "resolved": "https://registry.npmjs.org/debug/-/debug-4.4.1.tgz", + "integrity": "sha512-KcKCqiftBJcZr++7ykoDIEwSa3XWowTfNPo92BYxjXiyYEVrUQh2aLyhxBCwww+heortUFxEJYcRzosstTEBYQ==", + "license": "MIT", + "dependencies": { + "ms": "^2.1.3" + }, + "engines": { + "node": ">=6.0" + }, + "peerDependenciesMeta": { + "supports-color": { + "optional": true + } + } + }, + "node_modules/fd-slicer": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/fd-slicer/-/fd-slicer-1.1.0.tgz", + "integrity": "sha512-cE1qsB/VwyQozZ+q1dGxR8LBYNZeofhEdUNGSMbQD3Gw2lAzX9Zb3uIU6Ebc/Fmyjo9AWWfnn0AUCHqtevs/8g==", + "license": "MIT", + "dependencies": { + "pend": "~1.2.0" + } + }, + "node_modules/https-proxy-agent": { + "version": "7.0.6", + "resolved": "https://registry.npmjs.org/https-proxy-agent/-/https-proxy-agent-7.0.6.tgz", + "integrity": "sha512-vK9P5/iUfdl95AI+JVyUuIcVtd4ofvtrOr3HNtM2yxC9bnMbEdp3x01OhQNnjb8IJYi38VlTE3mBXwcfvywuSw==", + "license": "MIT", + "dependencies": { + "agent-base": "^7.1.2", + "debug": "4" + }, + "engines": { + "node": ">= 14" + } + }, + "node_modules/ms": { + "version": "2.1.3", + "resolved": "https://registry.npmjs.org/ms/-/ms-2.1.3.tgz", + "integrity": "sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA==", + "license": "MIT" + }, + "node_modules/pend": { + "version": "1.2.0", + "resolved": "https://registry.npmjs.org/pend/-/pend-1.2.0.tgz", + "integrity": "sha512-F3asv42UuXchdzt+xXqfW1OGlVBe+mxa2mqI0pg5yAHZPvFmY3Y6drSf/GQ1A86WgWEN9Kzh/WrgKa6iGcHXLg==", + "license": "MIT" + }, + "node_modules/proxy-from-env": { + "version": "1.1.0", + "resolved": "https://registry.npmjs.org/proxy-from-env/-/proxy-from-env-1.1.0.tgz", + "integrity": "sha512-D+zkORCbA9f1tdWRK0RaCR3GPv50cMxcrz4X8k5LTSUD1Dkw47mKJEZQNunItRTkWwgtaUSo1RVFRIG9ZXiFYg==", + "license": "MIT" + }, + "node_modules/yauzl": { + "version": "2.10.0", + "resolved": "https://registry.npmjs.org/yauzl/-/yauzl-2.10.0.tgz", + "integrity": "sha512-p4a9I6X6nu6IhoGmBqAcbJy1mlC4j27vEPZX9F4L4/vZT3Lyq1VkFHw/V/PUcB9Buo+DG3iHkT0x3Qya58zc3g==", + "license": "MIT", + "dependencies": { + "buffer-crc32": "~0.2.3", + "fd-slicer": "~1.1.0" + } + } + } +} diff --git a/codex-cli/package.json b/codex-cli/package.json index c5464beae54..614ca1a832e 100644 --- a/codex-cli/package.json +++ b/codex-cli/package.json @@ -16,5 +16,11 @@ "repository": { "type": "git", "url": "git+https://github.com/openai/codex.git" + }, + "dependencies": { + "@vscode/ripgrep": "^1.15.14" + }, + "devDependencies": { + "prettier": "^3.3.3" } } diff --git a/package-lock.json b/package-lock.json new file mode 100644 index 00000000000..bb67c752581 --- /dev/null +++ b/package-lock.json @@ -0,0 +1,33 @@ +{ + "name": "codex-monorepo", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "codex-monorepo", + "devDependencies": { + "prettier": "^3.5.3" + }, + "engines": { + "node": ">=22", + "pnpm": ">=9.0.0" + } + }, + "node_modules/prettier": { + "version": "3.6.2", + "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.6.2.tgz", + "integrity": "sha512-I7AIg5boAr5R0FFtJ6rCfD+LFsWHp81dolrFD8S79U9tb8Az2nGrJncnMSnys+bpQJfRUzqs9hnA81OAA3hCuQ==", + "dev": true, + "license": "MIT", + "bin": { + "prettier": "bin/prettier.cjs" + }, + "engines": { + "node": ">=14" + }, + "funding": { + "url": "https://github.com/prettier/prettier?sponsor=1" + } + } + } +} diff --git a/package.json b/package.json index c13e6cb3145..606c323ff4e 100644 --- a/package.json +++ b/package.json @@ -3,8 +3,8 @@ "private": true, "description": "Tools for repo-wide maintenance.", "scripts": { - "format": "prettier --check *.json *.md .github/workflows/*.yml", - "format:fix": "prettier --write *.json *.md .github/workflows/*.yml" + "format": "prettier --check *.json *.md .github/workflows/*.yml **/*.js", + "format:fix": "prettier --write *.json *.md .github/workflows/*.yml **/*.js" }, "devDependencies": { "prettier": "^3.5.3" From d4533a0bb33f61e8214af3085eb8a95fb159dfd5 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Wed, 13 Aug 2025 14:21:24 -0700 Subject: [PATCH 003/135] TUI: change the diff preview to have color fg not bg (#2270) image --- codex-rs/tui/src/diff_render.rs | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index ada84b89d67..9b37846b3fe 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -270,13 +270,14 @@ fn push_wrapped_diff_line( // Reserve a fixed number of spaces after the line number so that content starts // at a consistent column. The sign ("+"/"-") is rendered as part of the content - // with the same background as the edit, not as a separate dimmed column. + // and colored with the same foreground as the edited text, not as a separate + // dimmed column. let gap_after_ln = SPACES_AFTER_LINE_NUMBER.saturating_sub(ln_str.len()); let first_prefix_cols = indent.len() + ln_str.len() + gap_after_ln; let cont_prefix_cols = indent.len() + ln_str.len() + gap_after_ln; let mut first = true; - let (sign_opt, bg_style) = match kind { + let (sign_opt, line_style) = match kind { DiffLineType::Insert => (Some('+'), Some(style_add())), DiffLineType::Delete => (Some('-'), Some(style_del())), DiffLineType::Context => (None, None), @@ -307,18 +308,22 @@ fn push_wrapped_diff_line( spans.push(RtSpan::raw(" ".repeat(gap_after_ln))); // Prefix the content with the sign if it is an insertion or deletion, and color - // the sign with the same background as the edited text. + // the sign and content with the same foreground as the edited text. let display_chunk = match sign_opt { Some(sign_char) => format!("{sign_char}{chunk}"), None => chunk.to_string(), }; - let content_span = match bg_style { + let content_span = match line_style { Some(style) => RtSpan::styled(display_chunk, style), None => RtSpan::raw(display_chunk), }; spans.push(content_span); - lines.push(RtLine::from(spans)); + let mut line = RtLine::from(spans); + if let Some(style) = line_style { + line.style = line.style.patch(style); + } + lines.push(line); first = false; } else { let hang_prefix = format!( @@ -326,11 +331,15 @@ fn push_wrapped_diff_line( " ".repeat(ln_str.len()), " ".repeat(gap_after_ln) ); - let content_span = match bg_style { + let content_span = match line_style { Some(style) => RtSpan::styled(chunk.to_string(), style), None => RtSpan::raw(chunk.to_string()), }; - lines.push(RtLine::from(vec![RtSpan::raw(hang_prefix), content_span])); + let mut line = RtLine::from(vec![RtSpan::raw(hang_prefix), content_span]); + if let Some(style) = line_style { + line.style = line.style.patch(style); + } + lines.push(line); } } lines @@ -341,11 +350,11 @@ fn style_dim() -> Style { } fn style_add() -> Style { - Style::default().bg(Color::Green) + Style::default().fg(Color::Green) } fn style_del() -> Style { - Style::default().bg(Color::Red) + Style::default().fg(Color::Red) } #[allow(clippy::expect_used)] From 37fc4185ef5202a6a02f769a85f56c1abb66ded0 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 14:29:13 -0700 Subject: [PATCH 004/135] fix: update `OutgoingMessageSender::send_response()` to take `Serialize` (#2263) This makes `send_response()` easier to work with. --- codex-rs/mcp-server/src/codex_tool_runner.rs | 10 +++------ codex-rs/mcp-server/src/error_code.rs | 2 ++ codex-rs/mcp-server/src/lib.rs | 1 + codex-rs/mcp-server/src/message_processor.rs | 10 +++------ codex-rs/mcp-server/src/outgoing_message.rs | 23 +++++++++++++++++--- 5 files changed, 29 insertions(+), 17 deletions(-) create mode 100644 codex-rs/mcp-server/src/error_code.rs diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index ced01539bb3..ff660167d5e 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -61,7 +61,7 @@ pub async fn run_codex_tool_session( is_error: Some(true), structured_content: None, }; - outgoing.send_response(id.clone(), result.into()).await; + outgoing.send_response(id.clone(), result).await; return; } }; @@ -235,9 +235,7 @@ async fn run_codex_tool_session_inner( is_error: None, structured_content: None, }; - outgoing - .send_response(request_id.clone(), result.into()) - .await; + outgoing.send_response(request_id.clone(), result).await; // unregister the id so we don't keep it in the map running_requests_id_to_codex_uuid .lock() @@ -296,9 +294,7 @@ async fn run_codex_tool_session_inner( // structured way. structured_content: None, }; - outgoing - .send_response(request_id.clone(), result.into()) - .await; + outgoing.send_response(request_id.clone(), result).await; break; } } diff --git a/codex-rs/mcp-server/src/error_code.rs b/codex-rs/mcp-server/src/error_code.rs new file mode 100644 index 00000000000..1ffd889d404 --- /dev/null +++ b/codex-rs/mcp-server/src/error_code.rs @@ -0,0 +1,2 @@ +pub(crate) const INVALID_REQUEST_ERROR_CODE: i64 = -32600; +pub(crate) const INTERNAL_ERROR_CODE: i64 = -32603; diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index f6d0838efcb..abbe3b94a0f 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -18,6 +18,7 @@ use tracing_subscriber::EnvFilter; 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; diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index a3349aeb356..98beb154604 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -7,6 +7,7 @@ use crate::codex_tool_config::CodexToolCallParam; 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; @@ -191,7 +192,7 @@ impl MessageProcessor { if self.initialized { // Already initialised: send JSON-RPC error response. let error = JSONRPCErrorError { - code: -32600, // Invalid Request + code: INVALID_REQUEST_ERROR_CODE, message: "initialize called more than once".to_string(), data: None, }; @@ -230,9 +231,6 @@ impl MessageProcessor { where T: ModelContextProtocolRequest, { - // result has `Serialized` instance so should never fail - #[expect(clippy::unwrap_used)] - let result = serde_json::to_value(result).unwrap(); self.outgoing.send_response(id, result).await; } @@ -533,9 +531,7 @@ impl MessageProcessor { is_error: Some(true), structured_content: None, }; - outgoing - .send_response(request_id, serde_json::to_value(result).unwrap_or_default()) - .await; + outgoing.send_response(request_id, result).await; return; } }; diff --git a/codex-rs/mcp-server/src/outgoing_message.rs b/codex-rs/mcp-server/src/outgoing_message.rs index e7b0b9b63c0..3a77cbf2a04 100644 --- a/codex-rs/mcp-server/src/outgoing_message.rs +++ b/codex-rs/mcp-server/src/outgoing_message.rs @@ -18,6 +18,8 @@ use tokio::sync::mpsc; use tokio::sync::oneshot; use tracing::warn; +use crate::error_code::INTERNAL_ERROR_CODE; + /// Sends messages to the client and manages request callbacks. pub(crate) struct OutgoingMessageSender { next_request_id: AtomicI64, @@ -74,9 +76,24 @@ impl OutgoingMessageSender { } } - pub(crate) async fn send_response(&self, id: RequestId, result: Result) { - let outgoing_message = OutgoingMessage::Response(OutgoingResponse { id, result }); - let _ = self.sender.send(outgoing_message).await; + pub(crate) async fn send_response(&self, id: RequestId, response: T) { + match serde_json::to_value(response) { + Ok(result) => { + let outgoing_message = OutgoingMessage::Response(OutgoingResponse { id, result }); + let _ = self.sender.send(outgoing_message).await; + } + Err(err) => { + self.send_error( + id, + JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to serialize response: {err}"), + data: None, + }, + ) + .await; + } + } } pub(crate) async fn send_event_as_notification( From 41eb59a07db3d515b99f288cd717878bd1a12779 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 13 Aug 2025 15:43:54 -0700 Subject: [PATCH 005/135] Wait for requested delay in rate limit errors (#2266) Fixes: https://github.com/openai/codex/issues/2131 Response doesn't have the delay in a separate field (yet) so parse the message. --- codex-rs/core/Cargo.toml | 2 +- codex-rs/core/src/chat_completions.rs | 9 +- codex-rs/core/src/client.rs | 152 +++++++++++++++++++++++--- codex-rs/core/src/codex.rs | 8 +- codex-rs/core/src/error.rs | 5 +- 5 files changed, 153 insertions(+), 23 deletions(-) diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 1ea1422bd40..3fb99d1f7f1 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -29,6 +29,7 @@ mcp-types = { path = "../mcp-types" } mime_guess = "2.0" os_info = "3.12.0" rand = "0.9" +regex-lite = "0.1.6" reqwest = { version = "0.12", features = ["json", "stream"] } serde = { version = "1", features = ["derive"] } serde_bytes = "0.11" @@ -76,7 +77,6 @@ core_test_support = { path = "tests/common" } maplit = "1.0.2" predicates = "3" pretty_assertions = "1.4.1" -regex-lite = "0.1.6" tempfile = "3" tokio-test = "0.4" walkdir = "2.5.0" diff --git a/codex-rs/core/src/chat_completions.rs b/codex-rs/core/src/chat_completions.rs index 840e808fc74..fc0ceca5ad2 100644 --- a/codex-rs/core/src/chat_completions.rs +++ b/codex-rs/core/src/chat_completions.rs @@ -213,7 +213,9 @@ async fn process_chat_sse( let sse = match timeout(idle_timeout, stream.next()).await { Ok(Some(Ok(ev))) => ev, Ok(Some(Err(e))) => { - let _ = tx_event.send(Err(CodexErr::Stream(e.to_string()))).await; + let _ = tx_event + .send(Err(CodexErr::Stream(e.to_string(), None))) + .await; return; } Ok(None) => { @@ -228,7 +230,10 @@ async fn process_chat_sse( } Err(_) => { let _ = tx_event - .send(Err(CodexErr::Stream("idle timeout waiting for SSE".into()))) + .send(Err(CodexErr::Stream( + "idle timeout waiting for SSE".into(), + None, + ))) .await; return; } diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index f0229d45aec..135782bc310 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -1,5 +1,6 @@ use std::io::BufRead; use std::path::Path; +use std::sync::OnceLock; use std::time::Duration; use bytes::Bytes; @@ -7,6 +8,7 @@ use codex_login::AuthMode; use codex_login::CodexAuth; use eventsource_stream::Eventsource; use futures::prelude::*; +use regex_lite::Regex; use reqwest::StatusCode; use serde::Deserialize; use serde::Serialize; @@ -49,7 +51,9 @@ struct ErrorResponse { #[derive(Debug, Deserialize)] struct Error { - r#type: String, + r#type: Option, + code: Option, + message: Option, } #[derive(Clone)] @@ -263,14 +267,18 @@ impl ModelClient { if status == StatusCode::TOO_MANY_REQUESTS { let body = res.json::().await.ok(); if let Some(ErrorResponse { - error: Error { r#type, .. }, + error: + Error { + r#type: Some(error_type), + .. + }, }) = body { - if r#type == "usage_limit_reached" { + if error_type == "usage_limit_reached" { return Err(CodexErr::UsageLimitReached(UsageLimitReachedError { plan_type: auth.and_then(|a| a.get_plan_type()), })); - } else if r#type == "usage_not_included" { + } else if error_type == "usage_not_included" { return Err(CodexErr::UsageNotIncluded); } } @@ -366,13 +374,14 @@ async fn process_sse( // If the stream stays completely silent for an extended period treat it as disconnected. // The response id returned from the "complete" message. let mut response_completed: Option = None; + let mut response_error: Option = None; loop { let sse = match timeout(idle_timeout, stream.next()).await { Ok(Some(Ok(sse))) => sse, Ok(Some(Err(e))) => { debug!("SSE Error: {e:#}"); - let event = CodexErr::Stream(e.to_string()); + let event = CodexErr::Stream(e.to_string(), None); let _ = tx_event.send(Err(event)).await; return; } @@ -390,9 +399,10 @@ async fn process_sse( } None => { let _ = tx_event - .send(Err(CodexErr::Stream( + .send(Err(response_error.unwrap_or(CodexErr::Stream( "stream closed before response.completed".into(), - ))) + None, + )))) .await; } } @@ -400,7 +410,10 @@ async fn process_sse( } Err(_) => { let _ = tx_event - .send(Err(CodexErr::Stream("idle timeout waiting for SSE".into()))) + .send(Err(CodexErr::Stream( + "idle timeout waiting for SSE".into(), + None, + ))) .await; return; } @@ -478,15 +491,25 @@ async fn process_sse( } "response.failed" => { if let Some(resp_val) = event.response { - let error = resp_val - .get("error") - .and_then(|v| v.get("message")) - .and_then(|v| v.as_str()) - .unwrap_or("response.failed event received"); - - let _ = tx_event - .send(Err(CodexErr::Stream(error.to_string()))) - .await; + response_error = Some(CodexErr::Stream( + "response.failed event received".to_string(), + None, + )); + + let error = resp_val.get("error"); + + if let Some(error) = error { + match serde_json::from_value::(error.clone()) { + Ok(error) => { + let delay = try_parse_retry_after(&error); + let message = error.message.unwrap_or_default(); + response_error = Some(CodexErr::Stream(message, delay)); + } + Err(e) => { + debug!("failed to parse ErrorResponse: {e}"); + } + } + } } } // Final response completed – includes array of output items & id @@ -550,6 +573,40 @@ async fn stream_from_fixture( Ok(ResponseStream { rx_event }) } +fn rate_limit_regex() -> &'static Regex { + static RE: OnceLock = OnceLock::new(); + + #[expect(clippy::unwrap_used)] + RE.get_or_init(|| Regex::new(r"Please try again in (\d+(?:\.\d+)?)(s|ms)").unwrap()) +} + +fn try_parse_retry_after(err: &Error) -> Option { + if err.code != Some("rate_limit_exceeded".to_string()) { + return None; + } + + // parse the Please try again in 1.898s format using regex + let re = rate_limit_regex(); + if let Some(message) = &err.message + && let Some(captures) = re.captures(message) + { + let seconds = captures.get(1); + let unit = captures.get(2); + + if let (Some(value), Some(unit)) = (seconds, unit) { + let value = value.as_str().parse::().ok()?; + let unit = unit.as_str(); + + if unit == "s" { + return Some(Duration::from_secs_f64(value)); + } else if unit == "ms" { + return Some(Duration::from_millis(value as u64)); + } + } + } + None +} + #[cfg(test)] mod tests { #![allow(clippy::expect_used, clippy::unwrap_used)] @@ -735,13 +792,49 @@ mod tests { matches!(events[0], Ok(ResponseEvent::OutputItemDone(_))); match &events[1] { - Err(CodexErr::Stream(msg)) => { + Err(CodexErr::Stream(msg, _)) => { assert_eq!(msg, "stream closed before response.completed") } other => panic!("unexpected second event: {other:?}"), } } + #[tokio::test] + async fn error_when_error_event() { + let raw_error = r#"{"type":"response.failed","sequence_number":3,"response":{"id":"resp_689bcf18d7f08194bf3440ba62fe05d803fee0cdac429894","object":"response","created_at":1755041560,"status":"failed","background":false,"error":{"code":"rate_limit_exceeded","message":"Rate limit reached for gpt-5 in organization org-AAA on tokens per min (TPM): Limit 30000, Used 22999, Requested 12528. Please try again in 11.054s. Visit https://platform.openai.com/account/rate-limits to learn more."}, "usage":null,"user":null,"metadata":{}}}"#; + + let sse1 = format!("event: response.failed\ndata: {raw_error}\n\n"); + let provider = ModelProviderInfo { + name: "test".to_string(), + base_url: Some("https://test.com".to_string()), + env_key: Some("TEST_API_KEY".to_string()), + env_key_instructions: None, + wire_api: WireApi::Responses, + query_params: None, + http_headers: None, + env_http_headers: None, + request_max_retries: Some(0), + stream_max_retries: Some(0), + stream_idle_timeout_ms: Some(1000), + requires_openai_auth: false, + }; + + let events = collect_events(&[sse1.as_bytes()], provider).await; + + assert_eq!(events.len(), 1); + + match &events[0] { + Err(CodexErr::Stream(msg, delay)) => { + assert_eq!( + msg, + "Rate limit reached for gpt-5 in organization org-AAA on tokens per min (TPM): Limit 30000, Used 22999, Requested 12528. Please try again in 11.054s. Visit https://platform.openai.com/account/rate-limits to learn more." + ); + assert_eq!(*delay, Some(Duration::from_secs_f64(11.054))); + } + other => panic!("unexpected second event: {other:?}"), + } + } + // ──────────────────────────── // Table-driven test from `main` // ──────────────────────────── @@ -840,4 +933,27 @@ mod tests { ); } } + + #[test] + fn test_try_parse_retry_after() { + let err = Error { + r#type: None, + message: Some("Rate limit reached for gpt-5 in organization org- on tokens per min (TPM): Limit 1, Used 1, Requested 19304. Please try again in 28ms. Visit https://platform.openai.com/account/rate-limits to learn more.".to_string()), + code: Some("rate_limit_exceeded".to_string()), + }; + + let delay = try_parse_retry_after(&err); + assert_eq!(delay, Some(Duration::from_millis(28))); + } + + #[test] + fn test_try_parse_retry_after_no_delay() { + let err = Error { + r#type: None, + message: Some("Rate limit reached for gpt-5 in organization on tokens per min (TPM): Limit 30000, Used 6899, Requested 24050. Please try again in 1.898s. Visit https://platform.openai.com/account/rate-limits to learn more.".to_string()), + code: Some("rate_limit_exceeded".to_string()), + }; + let delay = try_parse_retry_after(&err); + assert_eq!(delay, Some(Duration::from_secs_f64(1.898))); + } } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5ac1392beec..207f75a72fc 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1281,7 +1281,10 @@ async fn run_turn( let max_retries = sess.client.get_provider().stream_max_retries(); if retries < max_retries { retries += 1; - let delay = backoff(retries); + let delay = match e { + CodexErr::Stream(_, Some(delay)) => delay, + _ => backoff(retries), + }; warn!( "stream disconnected - retrying turn ({retries}/{max_retries} in {delay:?})...", ); @@ -1391,6 +1394,7 @@ async fn try_run_turn( // Treat as a disconnected stream so the caller can retry. return Err(CodexErr::Stream( "stream closed before response.completed".into(), + None, )); }; @@ -2201,6 +2205,7 @@ async fn drain_to_completed(sess: &Session, sub_id: &str, prompt: &Prompt) -> Co let Some(event) = maybe_event else { return Err(CodexErr::Stream( "stream closed before response.completed".into(), + None, )); }; match event { @@ -2218,6 +2223,7 @@ async fn drain_to_completed(sess: &Session, sub_id: &str, prompt: &Prompt) -> Co None => { return Err(CodexErr::Stream( "token_usage was None in ResponseEvent::Completed".into(), + None, )); } }; diff --git a/codex-rs/core/src/error.rs b/codex-rs/core/src/error.rs index 4df66633940..356c23011c2 100644 --- a/codex-rs/core/src/error.rs +++ b/codex-rs/core/src/error.rs @@ -1,6 +1,7 @@ use reqwest::StatusCode; use serde_json; use std::io; +use std::time::Duration; use thiserror::Error; use tokio::task::JoinError; use uuid::Uuid; @@ -42,8 +43,10 @@ pub enum CodexErr { /// handshake has succeeded but **before** it finished emitting `response.completed`. /// /// The Session loop treats this as a transient error and will automatically retry the turn. + /// + /// Optionally includes the requested delay before retrying the turn. #[error("stream disconnected before completion: {0}")] - Stream(String), + Stream(String, Option), #[error("no conversation with id: {0}")] ConversationNotFound(Uuid), From cbf972007a0e175a7c63a5d9fd98ae82a3bab7f9 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Wed, 13 Aug 2025 15:50:50 -0700 Subject: [PATCH 006/135] use modifier dim instead of gray and .dim (#2273) gray color doesn't work very well with white terminals. `.dim` doesn't have an effect for some reason. after: image Before image --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 12 ++++---- .../src/bottom_pane/selection_popup_common.rs | 6 ++-- codex-rs/tui/src/history_cell.rs | 26 ++++++++--------- codex-rs/tui/src/shimmer.rs | 14 +++++----- codex-rs/tui/src/status_indicator_widget.rs | 28 +++++++++---------- codex-rs/tui/src/user_approval_widget.rs | 4 +-- 6 files changed, 42 insertions(+), 48 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 78506f572c1..f93878f4f24 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -731,15 +731,15 @@ impl WidgetRef for &ChatComposer { .render_ref(bottom_line_rect, buf); } } + let border_style = if self.has_focus { + Style::default().fg(Color::Cyan) + } else { + Style::default().add_modifier(Modifier::DIM) + }; Block::default() - .border_style(Style::default().dim()) .borders(Borders::LEFT) .border_type(BorderType::QuadrantOutside) - .border_style(Style::default().fg(if self.has_focus { - Color::Cyan - } else { - Color::Gray - })) + .border_style(border_style) .render_ref( Rect::new(textarea_rect.x, textarea_rect.y, 1, textarea_rect.height), buf, diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 1a31115d77a..0a0574c600d 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -93,9 +93,7 @@ pub(crate) fn render_rows( spans.push(Span::raw(" ")); spans.push(Span::styled( desc.clone(), - Style::default() - .fg(Color::DarkGray) - .add_modifier(Modifier::DIM), + Style::default().add_modifier(Modifier::DIM), )); } @@ -118,7 +116,7 @@ pub(crate) fn render_rows( Block::default() .borders(Borders::LEFT) .border_type(BorderType::QuadrantOutside) - .border_style(Style::default().fg(Color::DarkGray)), + .border_style(Style::default().add_modifier(Modifier::DIM)), ) .widths([Constraint::Percentage(100)]); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index ffbc96c19cf..0fa400ca814 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -483,7 +483,7 @@ impl HistoryCell { } else { status_str.red() }, - format!(", duration: {duration}").gray(), + format!(", duration: {duration}").dim(), ]); let mut lines: Vec> = Vec::new(); @@ -526,7 +526,10 @@ impl HistoryCell { format!("link: {uri}") } }; - lines.push(Line::styled(line_text, Style::default().fg(Color::Gray))); + lines.push(Line::styled( + line_text, + Style::default().add_modifier(Modifier::DIM), + )); } } @@ -755,7 +758,7 @@ impl HistoryCell { if empty > 0 { header.push(Span::styled( "░".repeat(empty), - Style::default().fg(Color::Gray), + Style::default().add_modifier(Modifier::DIM), )); } header.push(Span::raw("] ")); @@ -767,15 +770,15 @@ impl HistoryCell { let t = s.trim().to_string(); if t.is_empty() { None } else { Some(t) } }) { - lines.push(Line::from("note".gray().italic())); + lines.push(Line::from("note".dim().italic())); for l in expl.lines() { - lines.push(Line::from(l.to_string()).gray()); + lines.push(Line::from(l.to_string()).dim()); } } // Steps styled as checkbox items if plan.is_empty() { - lines.push(Line::from("(no steps provided)".gray().italic())); + lines.push(Line::from("(no steps provided)".dim().italic())); } else { for (idx, PlanItemArg { step, status }) in plan.into_iter().enumerate() { let (box_span, text_span) = match status { @@ -783,9 +786,7 @@ impl HistoryCell { Span::styled("✔", Style::default().fg(Color::Green)), Span::styled( step, - Style::default() - .fg(Color::Gray) - .add_modifier(Modifier::CROSSED_OUT | Modifier::DIM), + Style::default().add_modifier(Modifier::CROSSED_OUT | Modifier::DIM), ), ), StepStatus::InProgress => ( @@ -799,10 +800,7 @@ impl HistoryCell { ), StepStatus::Pending => ( Span::raw("□"), - Span::styled( - step, - Style::default().fg(Color::Gray).add_modifier(Modifier::DIM), - ), + Span::styled(step, Style::default().add_modifier(Modifier::DIM)), ), }; let prefix = if idx == 0 { @@ -997,7 +995,7 @@ fn format_mcp_invocation<'a>(invocation: McpInvocation) -> Line<'a> { Span::raw("."), Span::styled(invocation.tool.clone(), Style::default().fg(Color::Blue)), Span::raw("("), - Span::styled(args_str, Style::default().fg(Color::Gray)), + Span::styled(args_str, Style::default().add_modifier(Modifier::DIM)), Span::raw(")"), ]; Line::from(invocation_spans) diff --git a/codex-rs/tui/src/shimmer.rs b/codex-rs/tui/src/shimmer.rs index 9d28b732a0a..7e0d9f572a4 100644 --- a/codex-rs/tui/src/shimmer.rs +++ b/codex-rs/tui/src/shimmer.rs @@ -66,19 +66,19 @@ pub(crate) fn shimmer_spans(text: &str, frame_idx: usize) -> Vec> .fg(Color::Rgb(level, level, level)) .add_modifier(Modifier::BOLD) } else { - Style::default().fg(color_for_level(level)) + color_for_level(level) }; spans.push(Span::styled(ch.to_string(), style)); } spans } -fn color_for_level(level: u8) -> Color { - if level < 128 { - Color::DarkGray - } else if level < 192 { - Color::Gray +fn color_for_level(level: u8) -> Style { + if level < 144 { + Style::default().add_modifier(Modifier::DIM) + } else if level < 208 { + Style::default() } else { - Color::White + Style::default().add_modifier(Modifier::BOLD) } } diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index b686f45d094..65cba425231 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -205,7 +205,7 @@ impl WidgetRef for StatusIndicatorWidget { .fg(Color::Rgb(level, level, level)) .add_modifier(Modifier::BOLD) } else { - Style::default().fg(color_for_level(level)) + color_for_level(level) }; animated_spans.push(Span::styled(ch.to_string(), style)); @@ -223,7 +223,7 @@ impl WidgetRef for StatusIndicatorWidget { let spinner_ch = spinner_frames[(idx / SPINNER_SLOWDOWN) % spinner_frames.len()]; spans.push(Span::styled( spinner_ch.to_string(), - Style::default().fg(Color::DarkGray), + Style::default().add_modifier(Modifier::DIM), )); spans.push(Span::raw(" ")); @@ -236,27 +236,25 @@ impl WidgetRef for StatusIndicatorWidget { let bracket_prefix = format!("({elapsed}s • "); spans.push(Span::styled( bracket_prefix, - Style::default().fg(Color::Gray).add_modifier(Modifier::DIM), + Style::default().add_modifier(Modifier::DIM), )); spans.push(Span::styled( "Esc", - Style::default() - .fg(Color::Gray) - .add_modifier(Modifier::DIM | Modifier::BOLD), + Style::default().add_modifier(Modifier::DIM | Modifier::BOLD), )); spans.push(Span::styled( " to interrupt)", - Style::default().fg(Color::Gray).add_modifier(Modifier::DIM), + Style::default().add_modifier(Modifier::DIM), )); // Add a space and then the log text (not animated by the gradient) if !status_prefix.is_empty() { spans.push(Span::styled( " ", - Style::default().fg(Color::Gray).add_modifier(Modifier::DIM), + Style::default().add_modifier(Modifier::DIM), )); spans.push(Span::styled( status_prefix, - Style::default().fg(Color::Gray).add_modifier(Modifier::DIM), + Style::default().add_modifier(Modifier::DIM), )); } @@ -281,13 +279,13 @@ impl WidgetRef for StatusIndicatorWidget { } } -fn color_for_level(level: u8) -> Color { - if level < 128 { - Color::DarkGray - } else if level < 192 { - Color::Gray +fn color_for_level(level: u8) -> Style { + if level < 144 { + Style::default().add_modifier(Modifier::DIM) + } else if level < 208 { + Style::default() } else { - Color::White + Style::default().add_modifier(Modifier::BOLD) } } diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index 049497f362c..78ece561ab0 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -340,7 +340,7 @@ impl WidgetRef for &UserApprovalWidget<'_> { let style = if idx == self.selected_option { Style::new().bg(Color::Cyan).fg(Color::Black) } else { - Style::new().bg(Color::DarkGray) + Style::new().add_modifier(Modifier::DIM) }; opt.label.clone().alignment(Alignment::Center).style(style) }) @@ -372,7 +372,7 @@ impl WidgetRef for &UserApprovalWidget<'_> { } Line::from(self.select_options[self.selected_option].description) - .style(Style::new().italic().fg(Color::DarkGray)) + .style(Style::new().italic().add_modifier(Modifier::DIM)) .render(description_area.inner(Margin::new(1, 0)), buf); Block::bordered() From bb9ce3cb781b6cdd3a3565566a49d2a8717313cd Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Wed, 13 Aug 2025 19:14:03 -0400 Subject: [PATCH 007/135] =?UTF-8?q?tui:=20standardize=20tree=20prefix=20gl?= =?UTF-8?q?yphs=20to=20=E2=94=94=20(#2274)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace mixed `⎿` and `L` prefixes with `└` in TUI rendering. Screenshot 2025-08-13 at 4 02 03 PM --- codex-rs/tui/src/diff_render.rs | 2 +- codex-rs/tui/src/history_cell.rs | 10 +++---- ..._tui__diff_render__tests__add_details.snap | 2 +- ...er__tests__update_details_with_rename.snap | 2 +- .../tests/fixtures/ideal-binary-response.txt | 26 +++++++++---------- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 9b37846b3fe..2433b3442e8 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -143,7 +143,7 @@ pub(crate) fn create_diff_summary( spans.push(RtSpan::raw(")")); let mut line = RtLine::from(spans); - let prefix = if idx == 0 { " ⎿ " } else { " " }; + let prefix = if idx == 0 { " └ " } else { " " }; line.spans.insert(0, prefix.into()); line.spans .iter_mut() diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 0fa400ca814..8295a8976ef 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -367,7 +367,7 @@ impl HistoryCell { ParsedCommand::Unknown { cmd } => format!("⌨️ {}", shlex_join_safe(cmd)), }; - let first_prefix = if i == 0 { " L " } else { " " }; + let first_prefix = if i == 0 { " └ " } else { " " }; for (j, line_text) in text.lines().enumerate() { let prefix = if j == 0 { first_prefix } else { " " }; lines.push(Line::from(vec![ @@ -804,7 +804,7 @@ impl HistoryCell { ), }; let prefix = if idx == 0 { - Span::raw(" ⎿ ") + Span::raw(" └ ") } else { Span::raw(" ") }; @@ -892,7 +892,7 @@ impl HistoryCell { if !stdout.trim().is_empty() { 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 prefix = if i == 0 { " └ " } else { " " }; let s = format!("{prefix}{raw}"); lines.push(ansi_escape_line(&s).dim()); } @@ -945,7 +945,7 @@ fn output_lines( for (i, raw) in lines[..head_end].iter().enumerate() { let mut line = ansi_escape_line(raw); let prefix = if i == 0 && include_angle_pipe { - " ⎿ " + " └ " } else { " " }; @@ -1019,7 +1019,7 @@ mod tests { }]; let lines = HistoryCell::exec_command_lines(&[], &parsed, None); assert!(lines.len() >= 3); - assert_eq!(lines[1].spans[0].content, " L "); + assert_eq!(lines[1].spans[0].content, " └ "); assert_eq!(lines[2].spans[0].content, " "); } } diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__add_details.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__add_details.snap index 06fc8a68e81..f86d2fb93a8 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__add_details.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__add_details.snap @@ -3,7 +3,7 @@ source: tui/src/diff_render.rs expression: terminal.backend() --- "proposed patch to 1 file (+2 -0) " -" ⎿ README.md (+2 -0) " +" └ README.md (+2 -0) " " 1 +first line " " 2 +second line " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap index 0eebe09d73b..d49ce8ad82d 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap @@ -3,7 +3,7 @@ source: tui/src/diff_render.rs expression: terminal.backend() --- "proposed patch to 1 file (+1 -1) " -" ⎿ src/lib.rs → src/lib_new.rs (+1 -1) " +" └ src/lib.rs → src/lib_new.rs (+1 -1) " " 1 line one " " 2 -line two " " 2 +line two changed " diff --git a/codex-rs/tui/tests/fixtures/ideal-binary-response.txt b/codex-rs/tui/tests/fixtures/ideal-binary-response.txt index 00c6c02b67f..bc1bb5e56d5 100644 --- a/codex-rs/tui/tests/fixtures/ideal-binary-response.txt +++ b/codex-rs/tui/tests/fixtures/ideal-binary-response.txt @@ -13,7 +13,7 @@ I’m going to scan the workspace and Cargo manifests to see build profiles and dependencies that impact binary size. Then I’ll summarize the main causes. ⚡ Running ls -la - ⎿ total 6696 + └ total 6696 drwxr-xr-x@ 39 easong staff 1248 Aug 9 08:49 . drwxr-xr-x@ 29 easong staff 928 Aug 9 08:50 .. -rw-r--r--@ 1 easong staff 10244 Aug 8 21:13 .DS_Store @@ -25,7 +25,7 @@ dependencies that impact binary size. Then I’ll summarize the main causes. -rw-r--r--@ 1 easong staff 8639 Aug 6 23:30 typewriter.md -rw-r--r--@ 1 easong staff 7673 Aug 6 21:58 wrap-plan.md ⚡ Running sed -n '1,200p' Cargo.toml - ⎿ [workspace] + └ [workspace] members = [ "ansi-escape", "apply-patch", @@ -50,7 +50,7 @@ tui, login, ollama, and mcp. ⚡ Running for d in ansi-escape apply-patch arg0 cli common core exec execpolicy file-search linux-sandbox login mcp-client mcp-server mcp-types ollama tui; do echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done - ⎿ --- ansi-escape/Cargo.toml + └ --- ansi-escape/Cargo.toml [package] edition = "2024" name = "codex-ansi-escape" @@ -62,7 +62,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done ] } tracing = { version ⚡ Running sed -n '1,200p' execpolicy/Cargo.toml - ⎿ [package] + └ [package] name = "codex-execpolicy" version = { workspace = true } edition = "2024" @@ -74,7 +74,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done [dev-dependencies] tempfile = "3.13.0" ⚡ Running sed -n '1,200p' file-search/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "codex-file-search" version = { workspace = true } @@ -86,7 +86,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done serde_json = "1.0.142" tokio = { version = "1", features = ["full"] } ⚡ Running sed -n '1,200p' linux-sandbox/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "codex-linux-sandbox" version = { workspace = true } @@ -98,7 +98,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done "signal", ] } ⚡ Running sed -n '1,200p' login/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "codex-login" version = { workspace = true } @@ -110,7 +110,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done pretty_assertions = "1.4.1" tempfile = "3" ⚡ Running sed -n '1,200p' mcp-client/Cargo.toml - ⎿ [package] + └ [package] name = "codex-mcp-client" version = { workspace = true } edition = "2024" @@ -122,7 +122,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done "time", ] } ⚡ Running sed -n '1,200p' mcp-server/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "codex-mcp-server" version = { workspace = true } @@ -134,7 +134,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done tokio-test = "0.4" wiremock = "0.6" ⚡ Running sed -n '1,200p' mcp-types/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "mcp-types" version = { workspace = true } @@ -146,7 +146,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done serde = { version = "1", features = ["derive"] } serde_json = "1" ⚡ Running sed -n '1,200p' ollama/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "codex-ollama" version = { workspace = true } @@ -158,7 +158,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done [dev-dependencies] tempfile = "3" ⚡ Running sed -n '1,200p' tui/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "codex-tui" version = { workspace = true } @@ -170,7 +170,7 @@ echo "--- $d/Cargo.toml"; sed -n '1,200p' $d/Cargo.toml; echo; done rand = "0.8" vt100 = "0.16.2" ⚡ Running sed -n '1,200p' chatgpt/Cargo.toml - ⎿ [package] + └ [package] edition = "2024" name = "codex-chatgpt" version = { workspace = true } From 3a0656df63f2806d080e2688d1d61126763c280e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 16:27:20 -0700 Subject: [PATCH 008/135] fix: skip `cargo test` for release builds on ordinary CI because it is slow, particularly with --all-features set (#2276) I put this PR together because I noticed I have to wait quite a bit longer on my PRs since we added https://github.com/openai/codex/pull/2242 to catch more build issues. I think we should think about reigning in our use of create features, but this should be good enough to speed things up for now. --- .github/workflows/rust-ci.yml | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index 0fd67175a90..735a187a789 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -53,15 +53,9 @@ jobs: - runner: macos-14 target: x86_64-apple-darwin profile: dev - - runner: macos-14 - target: aarch64-apple-darwin - profile: release - runner: ubuntu-24.04 target: x86_64-unknown-linux-musl profile: dev - - runner: ubuntu-24.04 - target: x86_64-unknown-linux-musl - profile: release - runner: ubuntu-24.04 target: x86_64-unknown-linux-gnu profile: dev @@ -75,6 +69,15 @@ jobs: target: x86_64-pc-windows-msvc profile: dev + # Also run representative release builds on Mac and Linux because + # there could be release-only build errors we want to catch. + - runner: macos-14 + target: aarch64-apple-darwin + profile: release + - runner: ubuntu-24.04 + target: x86_64-unknown-linux-musl + profile: release + steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@1.88 @@ -108,12 +111,14 @@ jobs: # slower, we only do this for the x86_64-unknown-linux-gnu target. - name: cargo build individual crates id: build - if: ${{ matrix.target == 'x86_64-unknown-linux-gnu' }} + if: ${{ matrix.target == 'x86_64-unknown-linux-gnu' && matrix.profile != 'release' }} continue-on-error: true run: find . -name Cargo.toml -mindepth 2 -maxdepth 2 -print0 | xargs -0 -n1 -I{} bash -c 'cd "$(dirname "{}")" && cargo build --profile ${{ matrix.profile }}' - name: cargo test id: test + # `cargo test` takes too long for release builds to run them on every PR + if: ${{ matrix.profile != 'release' }} continue-on-error: true run: cargo test --all-features --target ${{ matrix.target }} --profile ${{ matrix.profile }} env: From de2c6a2ce77240003dc5addf92d2681758b20088 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 13 Aug 2025 17:02:50 -0700 Subject: [PATCH 009/135] Enable reasoning for codex-prefixed models (#2275) ## Summary - enable reasoning for any model slug starting with `codex-` - provide default model info for `codex-*` slugs - test that codex models are detected and support reasoning ## Testing - `just fmt` - `just fix` *(fails: E0658 `let` expressions in this position are unstable)* - `cargo test --all-features` *(fails: E0658 `let` expressions in this position are unstable)* ------ https://chatgpt.com/codex/tasks/task_i_689d13f8705483208a6ed21c076868e1 --- codex-rs/core/src/model_family.rs | 5 +++++ codex-rs/core/src/openai_model_info.rs | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/model_family.rs b/codex-rs/core/src/model_family.rs index 1245a030c66..fa4826d76f1 100644 --- a/codex-rs/core/src/model_family.rs +++ b/codex-rs/core/src/model_family.rs @@ -78,6 +78,11 @@ pub fn find_family_for_model(slug: &str) -> Option { supports_reasoning_summaries: true, uses_local_shell_tool: true, ) + } else if slug.starts_with("codex-") { + model_family!( + slug, slug, + supports_reasoning_summaries: true, + ) } else if slug.starts_with("gpt-4.1") { model_family!( slug, "gpt-4.1", diff --git a/codex-rs/core/src/openai_model_info.rs b/codex-rs/core/src/openai_model_info.rs index a072d409c60..66f3c626ea1 100644 --- a/codex-rs/core/src/openai_model_info.rs +++ b/codex-rs/core/src/openai_model_info.rs @@ -15,7 +15,8 @@ pub(crate) struct ModelInfo { } pub(crate) fn get_model_info(model_family: &ModelFamily) -> Option { - match model_family.slug.as_str() { + let slug = model_family.slug.as_str(); + match slug { // OSS models have a 128k shared token pool. // Arbitrarily splitting it: 3/4 input context, 1/4 output. // https://openai.com/index/gpt-oss-model-card/ @@ -82,6 +83,11 @@ pub(crate) fn get_model_info(model_family: &ModelFamily) -> Option { max_output_tokens: 100_000, }), + _ if slug.starts_with("codex-") => Some(ModelInfo { + context_window: 200_000, + max_output_tokens: 100_000, + }), + _ => None, } } From e7bad650ff34580f3973b297a2626f7336a22e07 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 17:36:29 -0700 Subject: [PATCH 010/135] feat: support traditional JSON-RPC request/response in MCP server (#2264) This introduces a new set of request types that our `codex mcp` supports. Note that these do not conform to MCP tool calls so that instead of having to send something like this: ```json { "jsonrpc": "2.0", "method": "tools/call", "id": 42, "params": { "name": "newConversation", "arguments": { "model": "gpt-5", "approvalPolicy": "on-request" } } } ``` we can send something like this: ```json { "jsonrpc": "2.0", "method": "newConversation", "id": 42, "params": { "model": "gpt-5", "approvalPolicy": "on-request" } } ``` Admittedly, this new format is not a valid MCP tool call, but we are OK with that right now. (That is, not everything we might want to request of `codex mcp` is something that is appropriate for an autonomous agent to do.) To start, this introduces four request types: - `newConversation` - `sendUserMessage` - `addConversationListener` - `removeConversationListener` The new `mcp-server/tests/codex_message_processor_flow.rs` shows how these can be used. The types are defined on the `CodexRequest` enum, so we introduce a new `CodexMessageProcessor` that is responsible for dealing with requests from this enum. The top-level `MessageProcessor` has been updated so that when `process_request()` is called, it first checks whether the request conforms to `CodexRequest` and dispatches it to `CodexMessageProcessor` if so. Note that I also decided to use `camelCase` for the on-the-wire format, as that seems to be the convention for MCP. For the moment, the new protocol is defined in `wire_format.rs` within the `mcp-server` crate, but in a subsequent PR, I will probably move it to its own crate to ensure the protocol has minimal dependencies and that we can codegen a schema from it. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2264). * #2278 * __->__ #2264 --- .../mcp-server/src/codex_message_processor.rs | 269 ++++++++++++++++++ codex-rs/mcp-server/src/codex_tool_config.rs | 4 +- codex-rs/mcp-server/src/lib.rs | 2 + codex-rs/mcp-server/src/message_processor.rs | 26 +- codex-rs/mcp-server/src/wire_format.rs | 180 ++++++++++++ .../tests/codex_message_processor_flow.rs | 176 ++++++++++++ .../mcp-server/tests/common/mcp_process.rs | 45 +++ 7 files changed, 698 insertions(+), 4 deletions(-) create mode 100644 codex-rs/mcp-server/src/codex_message_processor.rs create mode 100644 codex-rs/mcp-server/src/wire_format.rs create mode 100644 codex-rs/mcp-server/tests/codex_message_processor_flow.rs diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs new file mode 100644 index 00000000000..c0a6ab287c0 --- /dev/null +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -0,0 +1,269 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; + +use codex_core::ConversationManager; +use codex_core::NewConversation; +use codex_core::config::Config; +use codex_core::config::ConfigOverrides; +use mcp_types::JSONRPCErrorError; +use mcp_types::RequestId; +use tokio::sync::oneshot; +use uuid::Uuid; + +use crate::error_code::INTERNAL_ERROR_CODE; +use crate::error_code::INVALID_REQUEST_ERROR_CODE; +use crate::json_to_toml::json_to_toml; +use crate::outgoing_message::OutgoingMessageSender; +use crate::outgoing_message::OutgoingNotificationMeta; +use crate::wire_format::AddConversationListenerParams; +use crate::wire_format::AddConversationSubscriptionResponse; +use crate::wire_format::CodexRequest; +use crate::wire_format::ConversationId; +use crate::wire_format::InputItem as WireInputItem; +use crate::wire_format::NewConversationParams; +use crate::wire_format::NewConversationResponse; +use crate::wire_format::RemoveConversationListenerParams; +use crate::wire_format::RemoveConversationSubscriptionResponse; +use crate::wire_format::SendUserMessageParams; +use crate::wire_format::SendUserMessageResponse; +use codex_core::protocol::InputItem as CoreInputItem; +use codex_core::protocol::Op; + +/// Handles JSON-RPC messages for Codex conversations. +pub(crate) struct CodexMessageProcessor { + conversation_manager: Arc, + outgoing: Arc, + codex_linux_sandbox_exe: Option, + conversation_listeners: HashMap>, +} + +impl CodexMessageProcessor { + pub fn new( + conversation_manager: Arc, + outgoing: Arc, + codex_linux_sandbox_exe: Option, + ) -> Self { + Self { + conversation_manager, + outgoing, + codex_linux_sandbox_exe, + conversation_listeners: HashMap::new(), + } + } + + pub async fn process_request(&mut self, request: CodexRequest) { + match request { + CodexRequest::NewConversation { request_id, params } => { + // Do not tokio::spawn() to process new_conversation() + // asynchronously because we need to ensure the conversation is + // created before processing any subsequent messages. + self.process_new_conversation(request_id, params).await; + } + CodexRequest::SendUserMessage { request_id, params } => { + self.send_user_message(request_id, params).await; + } + CodexRequest::AddConversationListener { request_id, params } => { + self.add_conversation_listener(request_id, params).await; + } + CodexRequest::RemoveConversationListener { request_id, params } => { + self.remove_conversation_listener(request_id, params).await; + } + } + } + + async fn process_new_conversation(&self, request_id: RequestId, params: NewConversationParams) { + let config = match derive_config_from_params(params, self.codex_linux_sandbox_exe.clone()) { + Ok(config) => config, + Err(err) => { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!("error deriving config: {err}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + } + }; + + match self.conversation_manager.new_conversation(config).await { + Ok(conversation_id) => { + let NewConversation { + conversation_id, + session_configured, + .. + } = conversation_id; + let response = NewConversationResponse { + conversation_id: ConversationId(conversation_id), + model: session_configured.model, + }; + self.outgoing.send_response(request_id, response).await; + } + Err(err) => { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("error creating conversation: {err}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + } + } + } + + async fn send_user_message(&self, request_id: RequestId, params: SendUserMessageParams) { + let SendUserMessageParams { + conversation_id, + items, + } = params; + let Ok(conversation) = self + .conversation_manager + .get_conversation(conversation_id.0) + .await + else { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!("conversation not found: {conversation_id}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + }; + + let mapped_items: Vec = items + .into_iter() + .map(|item| match item { + WireInputItem::Text { text } => CoreInputItem::Text { text }, + WireInputItem::Image { image_url } => CoreInputItem::Image { image_url }, + WireInputItem::LocalImage { path } => CoreInputItem::LocalImage { path }, + }) + .collect(); + + // Submit user input to the conversation. + let _ = conversation + .submit(Op::UserInput { + items: mapped_items, + }) + .await; + + // Acknowledge with an empty result. + self.outgoing + .send_response(request_id, SendUserMessageResponse {}) + .await; + } + + async fn add_conversation_listener( + &mut self, + request_id: RequestId, + params: AddConversationListenerParams, + ) { + let AddConversationListenerParams { conversation_id } = params; + let Ok(conversation) = self + .conversation_manager + .get_conversation(conversation_id.0) + .await + else { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!("conversation not found: {}", conversation_id.0), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + }; + + let subscription_id = Uuid::new_v4(); + let (cancel_tx, mut cancel_rx) = oneshot::channel(); + self.conversation_listeners + .insert(subscription_id, cancel_tx); + let outgoing_for_task = self.outgoing.clone(); + let add_listener_request_id = request_id.clone(); + tokio::spawn(async move { + loop { + tokio::select! { + _ = &mut cancel_rx => { + // User has unsubscribed, so exit this task. + break; + } + event = conversation.next_event() => { + let event = match event { + Ok(event) => event, + Err(err) => { + tracing::warn!("conversation.next_event() failed with: {err}"); + break; + } + }; + + outgoing_for_task.send_event_as_notification( + &event, + Some(OutgoingNotificationMeta::new(Some(add_listener_request_id.clone()))), + ) + .await; + } + } + } + }); + let response = AddConversationSubscriptionResponse { subscription_id }; + self.outgoing.send_response(request_id, response).await; + } + + async fn remove_conversation_listener( + &mut self, + request_id: RequestId, + params: RemoveConversationListenerParams, + ) { + let RemoveConversationListenerParams { subscription_id } = params; + match self.conversation_listeners.remove(&subscription_id) { + Some(sender) => { + // Signal the spawned task to exit and acknowledge. + let _ = sender.send(()); + let response = RemoveConversationSubscriptionResponse {}; + self.outgoing.send_response(request_id, response).await; + } + None => { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!("subscription not found: {subscription_id}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + } + } + } +} + +fn derive_config_from_params( + params: NewConversationParams, + codex_linux_sandbox_exe: Option, +) -> std::io::Result { + let NewConversationParams { + model, + profile, + cwd, + approval_policy, + sandbox, + config: cli_overrides, + base_instructions, + include_plan_tool, + } = params; + let overrides = ConfigOverrides { + model, + config_profile: profile, + cwd: cwd.map(PathBuf::from), + approval_policy: approval_policy.map(Into::into), + sandbox_mode: sandbox.map(Into::into), + model_provider: None, + codex_linux_sandbox_exe, + base_instructions, + include_plan_tool, + disable_response_storage: None, + show_raw_agent_reasoning: None, + }; + + let cli_overrides = cli_overrides + .unwrap_or_default() + .into_iter() + .map(|(k, v)| (k, json_to_toml(v))) + .collect(); + + Config::load_with_cli_overrides(cli_overrides, overrides) +} diff --git a/codex-rs/mcp-server/src/codex_tool_config.rs b/codex-rs/mcp-server/src/codex_tool_config.rs index 4af3e29c48e..1d77eb4b822 100644 --- a/codex-rs/mcp-server/src/codex_tool_config.rs +++ b/codex-rs/mcp-server/src/codex_tool_config.rs @@ -58,7 +58,7 @@ pub struct CodexToolCallParam { /// Custom enum mirroring [`AskForApproval`], but has an extra dependency on /// [`JsonSchema`]. -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(rename_all = "kebab-case")] pub enum CodexToolCallApprovalPolicy { Untrusted, @@ -80,7 +80,7 @@ impl From for AskForApproval { /// Custom enum mirroring [`SandboxMode`] from config_types.rs, but with /// `JsonSchema` support. -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] #[serde(rename_all = "kebab-case")] pub enum CodexToolCallSandboxMode { ReadOnly, diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index abbe3b94a0f..b6dcf8246df 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -15,6 +15,7 @@ use tracing::error; use tracing::info; use tracing_subscriber::EnvFilter; +mod codex_message_processor; mod codex_tool_config; mod codex_tool_runner; mod conversation_loop; @@ -26,6 +27,7 @@ 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; use crate::outgoing_message::OutgoingMessage; diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 98beb154604..d043493c7bc 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -3,6 +3,7 @@ use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; +use crate::codex_message_processor::CodexMessageProcessor; use crate::codex_tool_config::CodexToolCallParam; use crate::codex_tool_config::CodexToolCallReplyParam; use crate::codex_tool_config::create_tool_for_codex_tool_call_param; @@ -14,6 +15,7 @@ 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::CodexRequest; use codex_core::ConversationManager; use codex_core::config::Config as CodexConfig; @@ -40,6 +42,7 @@ use tokio::task; use uuid::Uuid; pub(crate) struct MessageProcessor { + codex_message_processor: CodexMessageProcessor, outgoing: Arc, initialized: bool, codex_linux_sandbox_exe: Option, @@ -55,11 +58,19 @@ impl MessageProcessor { outgoing: OutgoingMessageSender, codex_linux_sandbox_exe: Option, ) -> Self { + let outgoing = Arc::new(outgoing); + let conversation_manager = Arc::new(ConversationManager::default()); + let codex_message_processor = CodexMessageProcessor::new( + conversation_manager.clone(), + outgoing.clone(), + codex_linux_sandbox_exe.clone(), + ); Self { - outgoing: Arc::new(outgoing), + codex_message_processor, + outgoing, initialized: false, codex_linux_sandbox_exe, - conversation_manager: Arc::new(ConversationManager::default()), + conversation_manager, running_requests_id_to_codex_uuid: Arc::new(Mutex::new(HashMap::new())), running_session_ids: Arc::new(Mutex::new(HashSet::new())), } @@ -78,6 +89,17 @@ impl MessageProcessor { } 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) + { + // If the request is a Codex request, handle it with the Codex + // message processor. + self.codex_message_processor + .process_request(codex_request) + .await; + return; + } + // Hold on to the ID so we can respond. let request_id = request.id.clone(); diff --git a/codex-rs/mcp-server/src/wire_format.rs b/codex-rs/mcp-server/src/wire_format.rs new file mode 100644 index 00000000000..61cd881b36d --- /dev/null +++ b/codex-rs/mcp-server/src/wire_format.rs @@ -0,0 +1,180 @@ +use std::collections::HashMap; +use std::fmt::Display; + +use mcp_types::RequestId; +use serde::Deserialize; +use serde::Serialize; + +use crate::codex_tool_config::CodexToolCallApprovalPolicy; +use crate::codex_tool_config::CodexToolCallSandboxMode; +use uuid::Uuid; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(transparent)] +pub struct ConversationId(pub Uuid); + +impl Display for ConversationId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +/// Request from the client to the server. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(tag = "method", rename_all = "camelCase")] +pub enum CodexRequest { + NewConversation { + #[serde(rename = "id")] + request_id: RequestId, + params: NewConversationParams, + }, + SendUserMessage { + #[serde(rename = "id")] + request_id: RequestId, + params: SendUserMessageParams, + }, + + AddConversationListener { + #[serde(rename = "id")] + request_id: RequestId, + params: AddConversationListenerParams, + }, + RemoveConversationListener { + #[serde(rename = "id")] + request_id: RequestId, + params: RemoveConversationListenerParams, + }, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] +#[serde(rename_all = "camelCase")] +pub struct NewConversationParams { + /// Optional override for the model name (e.g. "o3", "o4-mini"). + #[serde(skip_serializing_if = "Option::is_none")] + pub model: Option, + + /// Configuration profile from config.toml to specify default options. + #[serde(skip_serializing_if = "Option::is_none")] + pub profile: Option, + + /// Working directory for the session. If relative, it is resolved against + /// the server process's current working directory. + #[serde(skip_serializing_if = "Option::is_none")] + pub cwd: Option, + + /// Approval policy for shell commands generated by the model: + /// `untrusted`, `on-failure`, `on-request`, `never`. + #[serde(skip_serializing_if = "Option::is_none")] + pub approval_policy: Option, + + /// Sandbox mode: `read-only`, `workspace-write`, or `danger-full-access`. + #[serde(skip_serializing_if = "Option::is_none")] + pub sandbox: Option, + + /// Individual config settings that will override what is in + /// CODEX_HOME/config.toml. + #[serde(skip_serializing_if = "Option::is_none")] + pub config: Option>, + + /// The set of instructions to use instead of the default ones. + #[serde(skip_serializing_if = "Option::is_none")] + pub base_instructions: Option, + + /// Whether to include the plan tool in the conversation. + #[serde(skip_serializing_if = "Option::is_none")] + pub include_plan_tool: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct NewConversationResponse { + pub conversation_id: ConversationId, + pub model: String, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct AddConversationSubscriptionResponse { + pub subscription_id: Uuid, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct RemoveConversationSubscriptionResponse {} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct SendUserMessageParams { + pub conversation_id: ConversationId, + pub items: Vec, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct SendUserMessageResponse {} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct AddConversationListenerParams { + pub conversation_id: ConversationId, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct RemoveConversationListenerParams { + pub subscription_id: Uuid, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub enum InputItem { + Text { + text: String, + }, + /// Pre‑encoded data: URI image. + Image { + image_url: String, + }, + + /// Local image path provided by the user. This will be converted to an + /// `Image` variant (base64 data URL) during request serialization. + LocalImage { + path: std::path::PathBuf, + }, +} + +#[allow(clippy::unwrap_used)] +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use serde_json::json; + + #[test] + fn serialize_new_conversation() { + let request = CodexRequest::NewConversation { + request_id: RequestId::Integer(42), + params: NewConversationParams { + model: Some("gpt-5".to_string()), + profile: None, + cwd: None, + approval_policy: Some(CodexToolCallApprovalPolicy::OnRequest), + sandbox: None, + config: None, + base_instructions: None, + include_plan_tool: None, + }, + }; + assert_eq!( + json!({ + "method": "newConversation", + "id": 42, + "params": { + "model": "gpt-5", + "approvalPolicy": "on-request" + } + }), + serde_json::to_value(&request).unwrap(), + ); + } +} diff --git a/codex-rs/mcp-server/tests/codex_message_processor_flow.rs b/codex-rs/mcp-server/tests/codex_message_processor_flow.rs new file mode 100644 index 00000000000..c81a04a5f43 --- /dev/null +++ b/codex-rs/mcp-server/tests/codex_message_processor_flow.rs @@ -0,0 +1,176 @@ +#![allow(clippy::expect_used, clippy::unwrap_used)] + +use std::path::Path; + +use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; +use codex_mcp_server::wire_format::AddConversationListenerParams; +use codex_mcp_server::wire_format::AddConversationSubscriptionResponse; +use codex_mcp_server::wire_format::NewConversationParams; +use codex_mcp_server::wire_format::NewConversationResponse; +use codex_mcp_server::wire_format::RemoveConversationListenerParams; +use codex_mcp_server::wire_format::RemoveConversationSubscriptionResponse; +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_test_support::create_shell_sse_response; +use mcp_types::JSONRPCResponse; +use mcp_types::RequestId; +use pretty_assertions::assert_eq; +use serde::de::DeserializeOwned; +use std::env; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn test_codex_jsonrpc_conversation_flow() { + if env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { + println!( + "Skipping test because it cannot execute when network is disabled in a Codex sandbox." + ); + return; + } + + let tmp = TempDir::new().expect("tmp dir"); + // Temporary Codex home with config pointing at the mock server. + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home).expect("create codex home dir"); + let working_directory = tmp.path().join("workdir"); + std::fs::create_dir(&working_directory).expect("create working directory"); + + // Create a mock model server that immediately ends each turn. + // Two turns are expected: initial session configure + one user message. + let responses = vec![ + create_shell_sse_response( + vec!["ls".to_string()], + Some(&working_directory), + Some(5000), + "call1234", + ) + .expect("create shell sse response"), + create_final_assistant_message_sse_response("Enjoy your new git repo!") + .expect("create final assistant message"), + ]; + let server = create_mock_chat_completions_server(responses).await; + create_config_toml(&codex_home, &server.uri()).expect("write config"); + + // Start MCP server and initialize. + let mut mcp = McpProcess::new(&codex_home).await.expect("spawn mcp"); + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()) + .await + .expect("init timeout") + .expect("init error"); + + // 1) newConversation + let new_conv_id = mcp + .send_new_conversation_request(NewConversationParams { + cwd: Some(working_directory.to_string_lossy().into_owned()), + ..Default::default() + }) + .await + .expect("send newConversation"); + let new_conv_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(new_conv_id)), + ) + .await + .expect("newConversation timeout") + .expect("newConversation resp"); + let new_conv_resp = to_response::(new_conv_resp) + .expect("deserialize newConversation response"); + let NewConversationResponse { + conversation_id, + model, + } = new_conv_resp; + assert_eq!(model, "mock-model"); + + // 2) addConversationListener + let add_listener_id = mcp + .send_add_conversation_listener_request(AddConversationListenerParams { conversation_id }) + .await + .expect("send addConversationListener"); + let add_listener_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(add_listener_id)), + ) + .await + .expect("addConversationListener timeout") + .expect("addConversationListener resp"); + let AddConversationSubscriptionResponse { subscription_id } = + to_response::(add_listener_resp) + .expect("deserialize addConversationListener response"); + + // 3) sendUserMessage (should trigger notifications; we only validate an OK response) + let send_user_id = mcp + .send_send_user_message_request(SendUserMessageParams { + conversation_id, + items: vec![codex_mcp_server::wire_format::InputItem::Text { + text: "text".to_string(), + }], + }) + .await + .expect("send sendUserMessage"); + let send_user_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(send_user_id)), + ) + .await + .expect("sendUserMessage timeout") + .expect("sendUserMessage resp"); + let SendUserMessageResponse {} = to_response::(send_user_resp) + .expect("deserialize sendUserMessage response"); + + // Give the server time to process the user's request. + tokio::time::sleep(std::time::Duration::from_millis(5_000)).await; + + // Could verify that some notifications were received? + + // 4) removeConversationListener + let remove_listener_id = mcp + .send_remove_conversation_listener_request(RemoveConversationListenerParams { + subscription_id, + }) + .await + .expect("send removeConversationListener"); + let remove_listener_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(remove_listener_id)), + ) + .await + .expect("removeConversationListener timeout") + .expect("removeConversationListener resp"); + let RemoveConversationSubscriptionResponse {} = + to_response(remove_listener_resp).expect("deserialize removeConversationListener response"); +} + +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: minimal config.toml pointing at mock provider. +fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write( + config_toml, + format!( + r#" +model = "mock-model" +approval_policy = "never" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "chat" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 83cf085cd43..4181479b5f9 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -18,6 +18,10 @@ 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; +use codex_mcp_server::wire_format::SendUserMessageParams; use mcp_types::CallToolRequestParams; use mcp_types::ClientCapabilities; @@ -236,6 +240,47 @@ impl McpProcess { .await } + // --------------------------------------------------------------------- + // Codex JSON-RPC (non-tool) helpers + // --------------------------------------------------------------------- + /// Send a `newConversation` JSON-RPC request. + pub async fn send_new_conversation_request( + &mut self, + params: NewConversationParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("newConversation", params).await + } + + /// Send an `addConversationListener` JSON-RPC request. + pub async fn send_add_conversation_listener_request( + &mut self, + params: AddConversationListenerParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("addConversationListener", params).await + } + + /// Send a `sendUserMessage` JSON-RPC request with a single text item. + pub async fn send_send_user_message_request( + &mut self, + params: SendUserMessageParams, + ) -> anyhow::Result { + // Wire format expects variants in camelCase; text item uses external tagging. + let params = Some(serde_json::to_value(params)?); + self.send_request("sendUserMessage", params).await + } + + /// Send a `removeConversationListener` JSON-RPC request. + pub async fn send_remove_conversation_listener_request( + &mut self, + params: RemoveConversationListenerParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("removeConversationListener", params) + .await + } + async fn send_request( &mut self, method: &str, From a62510e0ae9a95f074e99820daa1bc82f73c113e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 17:54:12 -0700 Subject: [PATCH 011/135] fix: verify notifications are sent with the conversationId set (#2278) This updates `CodexMessageProcessor` so that each notification it sends for a `EventMsg` from a `CodexConversation` such that: - The `params` always has an appropriate `conversationId` field. - The `method` is now includes the name of the `EventMsg` type rather than using `codex/event` as the `method` type for all notifications. (We currently prefix the method name with `codex/event/`, but I think that should go away once we formalize the notification schema in `wire_format.rs`.) As part of this, we update `test_codex_jsonrpc_conversation_flow()` to verify that the `task_finished` notification has made it through the system instead of sleeping for 5s and "hoping" the server finished processing the task. Note we have seen some flakiness in some of our other, similar integration tests, and I expect adding a similar check would help in those cases, as well. --- .../mcp-server/src/codex_message_processor.rs | 25 ++++++++++++----- codex-rs/mcp-server/src/outgoing_message.rs | 11 +++++--- .../tests/codex_message_processor_flow.rs | 25 ++++++++++++++--- .../mcp-server/tests/common/mcp_process.rs | 27 +++++++++++++++++++ 4 files changed, 75 insertions(+), 13 deletions(-) diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index c0a6ab287c0..5e0e4cad6ef 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -15,7 +15,7 @@ use crate::error_code::INTERNAL_ERROR_CODE; use crate::error_code::INVALID_REQUEST_ERROR_CODE; use crate::json_to_toml::json_to_toml; use crate::outgoing_message::OutgoingMessageSender; -use crate::outgoing_message::OutgoingNotificationMeta; +use crate::outgoing_message::OutgoingNotification; use crate::wire_format::AddConversationListenerParams; use crate::wire_format::AddConversationSubscriptionResponse; use crate::wire_format::CodexRequest; @@ -176,7 +176,6 @@ impl CodexMessageProcessor { self.conversation_listeners .insert(subscription_id, cancel_tx); let outgoing_for_task = self.outgoing.clone(); - let add_listener_request_id = request_id.clone(); tokio::spawn(async move { loop { tokio::select! { @@ -193,10 +192,24 @@ impl CodexMessageProcessor { } }; - outgoing_for_task.send_event_as_notification( - &event, - Some(OutgoingNotificationMeta::new(Some(add_listener_request_id.clone()))), - ) + let method = format!("codex/event/{}", event.msg); + let mut params = match serde_json::to_value(event) { + Ok(serde_json::Value::Object(map)) => map, + Ok(_) => { + tracing::error!("event did not serialize to an object"); + continue; + } + Err(err) => { + tracing::error!("failed to serialize event: {err}"); + continue; + } + }; + params.insert("conversationId".to_string(), conversation_id.to_string().into()); + + outgoing_for_task.send_notification(OutgoingNotification { + method, + params: Some(params.into()), + }) .await; } } diff --git a/codex-rs/mcp-server/src/outgoing_message.rs b/codex-rs/mcp-server/src/outgoing_message.rs index 3a77cbf2a04..f13b8b324fd 100644 --- a/codex-rs/mcp-server/src/outgoing_message.rs +++ b/codex-rs/mcp-server/src/outgoing_message.rs @@ -114,16 +114,21 @@ impl OutgoingMessageSender { event_json }; - let outgoing_message = OutgoingMessage::Notification(OutgoingNotification { + self.send_notification(OutgoingNotification { method: "codex/event".to_string(), params: Some(params.clone()), - }); - let _ = self.sender.send(outgoing_message).await; + }) + .await; self.send_event_as_notification_new_schema(event, Some(params.clone())) .await; } + pub(crate) async fn send_notification(&self, notification: OutgoingNotification) { + let outgoing_message = OutgoingMessage::Notification(notification); + let _ = self.sender.send(outgoing_message).await; + } + // should be backwards compatible. // it will replace send_event_as_notification eventually. async fn send_event_as_notification_new_schema( 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 c81a04a5f43..7b7845609e0 100644 --- a/codex-rs/mcp-server/tests/codex_message_processor_flow.rs +++ b/codex-rs/mcp-server/tests/codex_message_processor_flow.rs @@ -15,6 +15,7 @@ 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_test_support::create_shell_sse_response; +use mcp_types::JSONRPCNotification; use mcp_types::JSONRPCResponse; use mcp_types::RequestId; use pretty_assertions::assert_eq; @@ -123,10 +124,26 @@ async fn test_codex_jsonrpc_conversation_flow() { let SendUserMessageResponse {} = to_response::(send_user_resp) .expect("deserialize sendUserMessage response"); - // Give the server time to process the user's request. - tokio::time::sleep(std::time::Duration::from_millis(5_000)).await; - - // Could verify that some notifications were received? + // Verify the task_finished notification is received. + // Note this also ensures that the final request to the server was made. + let task_finished_notification: JSONRPCNotification = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await + .expect("task_finished_notification timeout") + .expect("task_finished_notification resp"); + let serde_json::Value::Object(map) = task_finished_notification + .params + .expect("notification should have params") + else { + panic!("task_finished_notification should have params"); + }; + assert_eq!( + map.get("conversationId") + .expect("should have conversationId"), + &serde_json::Value::String(conversation_id.to_string()) + ); // 4) removeConversationListener let remove_listener_id = mcp diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 4181479b5f9..a659b1d950e 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -374,6 +374,33 @@ impl McpProcess { } } + pub async fn read_stream_until_notification_message( + &mut self, + method: &str, + ) -> anyhow::Result { + loop { + let message = self.read_jsonrpc_message().await?; + eprint!("message: {message:?}"); + + match message { + JSONRPCMessage::Notification(notification) => { + if notification.method == method { + return Ok(notification); + } + } + JSONRPCMessage::Request(_) => { + anyhow::bail!("unexpected JSONRPCMessage::Request: {message:?}"); + } + JSONRPCMessage::Error(_) => { + anyhow::bail!("unexpected JSONRPCMessage::Error: {message:?}"); + } + JSONRPCMessage::Response(_) => { + anyhow::bail!("unexpected JSONRPCMessage::Response: {message:?}"); + } + } + } + } + pub async fn read_stream_until_configured_response_message( &mut self, ) -> anyhow::Result { From f1be7978cf516c393b904003eccac688c16de512 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 13 Aug 2025 18:39:58 -0700 Subject: [PATCH 012/135] Parse reasoning text content (#2277) Sometimes COT is returns as text content instead of `ReasoningText`. We should parse it but not serialize back on requests. --------- Co-authored-by: Ahmed Ibrahim --- codex-rs/core/src/codex.rs | 1 + codex-rs/core/src/models.rs | 12 +- codex-rs/tui/src/chatwidget.rs | 12 +- ...e_final_message_are_rendered_snapshot.snap | 10 ++ ...n_message_without_deltas_are_rendered.snap | 9 ++ codex-rs/tui/src/chatwidget/tests.rs | 130 ++++++++++++------ codex-rs/tui/src/streaming/controller.rs | 50 +++++-- codex-rs/tui/src/streaming/mod.rs | 3 + 8 files changed, 169 insertions(+), 58 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 207f75a72fc..e9453a2490a 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1606,6 +1606,7 @@ async fn handle_response_item( for item in content { let text = match item { ReasoningItemContent::ReasoningText { text } => text, + ReasoningItemContent::Text { text } => text, }; let event = Event { id: sub_id.to_string(), diff --git a/codex-rs/core/src/models.rs b/codex-rs/core/src/models.rs index e052bc43a41..da1e31b7062 100644 --- a/codex-rs/core/src/models.rs +++ b/codex-rs/core/src/models.rs @@ -45,7 +45,7 @@ pub enum ResponseItem { Reasoning { id: String, summary: Vec, - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "should_serialize_reasoning_content")] content: Option>, encrypted_content: Option, }, @@ -81,6 +81,15 @@ pub enum ResponseItem { Other, } +fn should_serialize_reasoning_content(content: &Option>) -> bool { + match content { + Some(content) => !content + .iter() + .any(|c| matches!(c, ReasoningItemContent::ReasoningText { .. })), + None => false, + } +} + impl From for ResponseItem { fn from(item: ResponseInputItem) -> Self { match item { @@ -142,6 +151,7 @@ pub enum ReasoningItemReasoningSummary { #[serde(tag = "type", rename_all = "snake_case")] pub enum ReasoningItemContent { ReasoningText { text: String }, + Text { text: String }, } impl From> for ResponseInputItem { diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 517a9a35f5f..9f7f0518e22 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -133,6 +133,7 @@ impl ChatWidget<'_> { fn on_agent_message(&mut self, message: String) { let sink = AppEventHistorySink(self.app_event_tx.clone()); let finished = self.stream.apply_final_answer(&message, &sink); + self.last_stream_kind = Some(StreamKind::Answer); self.handle_if_stream_finished(finished); self.mark_needs_redraw(); } @@ -145,9 +146,10 @@ impl ChatWidget<'_> { self.handle_streaming_delta(StreamKind::Reasoning, delta); } - fn on_agent_reasoning_final(&mut self) { + fn on_agent_reasoning_final(&mut self, text: String) { let sink = AppEventHistorySink(self.app_event_tx.clone()); - let finished = self.stream.finalize(StreamKind::Reasoning, false, &sink); + let finished = self.stream.apply_final_reasoning(&text, &sink); + self.last_stream_kind = Some(StreamKind::Reasoning); self.handle_if_stream_finished(finished); self.mark_needs_redraw(); } @@ -633,9 +635,9 @@ impl ChatWidget<'_> { | EventMsg::AgentReasoningRawContentDelta(AgentReasoningRawContentDeltaEvent { delta, }) => self.on_agent_reasoning_delta(delta), - EventMsg::AgentReasoning(AgentReasoningEvent { .. }) - | EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { .. }) => { - self.on_agent_reasoning_final() + EventMsg::AgentReasoning(AgentReasoningEvent { text }) + | EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { text }) => { + self.on_agent_reasoning_final(text) } EventMsg::AgentReasoningSectionBreak(_) => self.on_reasoning_section_break(), EventMsg::TaskStarted => self.on_task_started(), diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap new file mode 100644 index 00000000000..8258ea0b285 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__deltas_then_same_final_message_are_rendered_snapshot.snap @@ -0,0 +1,10 @@ +--- +source: tui/src/chatwidget/tests.rs +assertion_line: 886 +expression: combined +--- +thinking +I will first analyze the request. + +codex +Here is the result. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap new file mode 100644 index 00000000000..c90ec2733d3 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__final_reasoning_then_message_without_deltas_are_rendered.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: combined +--- +thinking +I will first analyze the request. + +codex +Here is the result. diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 0a7dd93368f..143dd4d0352 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -12,6 +12,7 @@ use codex_core::plan_tool::UpdatePlanArgs; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; use codex_core::protocol::AgentReasoningDeltaEvent; +use codex_core::protocol::AgentReasoningEvent; use codex_core::protocol::ApplyPatchApprovalRequestEvent; use codex_core::protocol::Event; use codex_core::protocol::EventMsg; @@ -24,6 +25,7 @@ use codex_core::protocol::TaskCompleteEvent; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; +use insta::assert_snapshot; use pretty_assertions::assert_eq; use std::fs::File; use std::io::BufRead; @@ -413,46 +415,6 @@ async fn binary_size_transcript_matches_ideal_fixture() { assert_eq!(visible_after, ideal); } -#[test] -fn final_longer_answer_after_single_char_delta_is_complete() { - let (mut chat, rx, _op_rx) = make_chatwidget_manual(); - - // Simulate a stray delta without newline (e.g., punctuation). - chat.handle_codex_event(Event { - id: "sub-x".into(), - msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta: "?".into() }), - }); - - // Now send the full final answer with no newline. - let full = "Hi! How can I help with codex-rs today? Want me to explore the repo, run tests, or work on a specific change?"; - chat.handle_codex_event(Event { - id: "sub-x".into(), - msg: EventMsg::AgentMessage(AgentMessageEvent { - message: full.into(), - }), - }); - - // Drain and assert the full message appears in history. - let cells = drain_insert_history(&rx); - let mut found = false; - for lines in &cells { - let s = lines - .iter() - .flat_map(|l| l.spans.iter()) - .map(|sp| sp.content.clone()) - .collect::(); - if s.contains(full) { - found = true; - break; - } - } - assert!( - found, - "expected full final message to be flushed to history, cells={:?}", - cells.len() - ); -} - #[test] fn apply_patch_events_emit_history_cells() { let (mut chat, rx, _op_rx) = make_chatwidget_manual(); @@ -923,3 +885,91 @@ fn multiple_agent_messages_in_single_turn_emit_multiple_headers() { let second_idx = combined.find("Second message").unwrap(); assert!(first_idx < second_idx, "messages out of order: {combined}"); } + +#[test] +fn final_reasoning_then_message_without_deltas_are_rendered() { + let (mut chat, rx, _op_rx) = make_chatwidget_manual(); + + // No deltas; only final reasoning followed by final message. + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentReasoning(AgentReasoningEvent { + text: "I will first analyze the request.".into(), + }), + }); + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentMessage(AgentMessageEvent { + message: "Here is the result.".into(), + }), + }); + + // Drain history and snapshot the combined visible content. + let cells = drain_insert_history(&rx); + let combined = cells + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_snapshot!(combined); +} + +#[test] +fn deltas_then_same_final_message_are_rendered_snapshot() { + let (mut chat, rx, _op_rx) = make_chatwidget_manual(); + + // Stream some reasoning deltas first. + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { + delta: "I will ".into(), + }), + }); + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { + delta: "first analyze the ".into(), + }), + }); + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { + delta: "request.".into(), + }), + }); + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentReasoning(AgentReasoningEvent { + text: "request.".into(), + }), + }); + + // Then stream answer deltas, followed by the exact same final message. + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { + delta: "Here is the ".into(), + }), + }); + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { + delta: "result.".into(), + }), + }); + + chat.handle_codex_event(Event { + id: "s1".into(), + msg: EventMsg::AgentMessage(AgentMessageEvent { + message: "Here is the result.".into(), + }), + }); + + // Snapshot the combined visible content to ensure we render as expected + // when deltas are followed by the identical final message. + let cells = drain_insert_history(&rx); + let combined = cells + .iter() + .map(|lines| lines_to_single_string(lines)) + .collect::(); + assert_snapshot!(combined); +} diff --git a/codex-rs/tui/src/streaming/controller.rs b/codex-rs/tui/src/streaming/controller.rs index 5201d359429..161a111173e 100644 --- a/codex-rs/tui/src/streaming/controller.rs +++ b/codex-rs/tui/src/streaming/controller.rs @@ -143,6 +143,10 @@ impl StreamController { }; let cfg = self.config.clone(); let state = self.state_mut(kind); + // Record that at least one delta was received for this stream + if !delta.is_empty() { + state.has_seen_delta = true; + } state.collector.push_delta(delta); if delta.contains('\n') { let newly_completed = state.collector.commit_complete_lines(&cfg); @@ -263,19 +267,41 @@ impl StreamController { /// Apply a full final answer: replace queued content with only the remaining tail, /// then finalize immediately and notify completion. pub(crate) fn apply_final_answer(&mut self, message: &str, sink: &impl HistorySink) -> bool { - self.begin(StreamKind::Answer, sink); - if !message.is_empty() { - let mut msg_with_nl = message.to_string(); - if !msg_with_nl.ends_with('\n') { - msg_with_nl.push('\n'); + self.apply_full_final(StreamKind::Answer, message, true, sink) + } + + pub(crate) fn apply_final_reasoning(&mut self, message: &str, sink: &impl HistorySink) -> bool { + self.apply_full_final(StreamKind::Reasoning, message, false, sink) + } + + fn apply_full_final( + &mut self, + kind: StreamKind, + message: &str, + immediate: bool, + sink: &impl HistorySink, + ) -> bool { + self.begin(kind, sink); + + { + let state = self.state_mut(kind); + // Only inject the final full message if we have not seen any deltas for this stream. + // If deltas were received, rely on the collector's existing buffer to avoid duplication. + if !state.has_seen_delta && !message.is_empty() { + // normalize to end with newline + let mut msg = message.to_owned(); + if !msg.ends_with('\n') { + msg.push('\n'); + } + + // replace while preserving already committed count + let committed = state.collector.committed_count(); + state + .collector + .replace_with_and_mark_committed(&msg, committed); } - let state = self.state_mut(StreamKind::Answer); - let already_committed = state.collector.committed_count(); - // Preserve previously committed count so finalize emits only the remaining tail. - state - .collector - .replace_with_and_mark_committed(&msg_with_nl, already_committed); } - self.finalize(StreamKind::Answer, true, sink) + + self.finalize(kind, immediate, sink) } } diff --git a/codex-rs/tui/src/streaming/mod.rs b/codex-rs/tui/src/streaming/mod.rs index c6f4ad21490..bb4fdcb6ffe 100644 --- a/codex-rs/tui/src/streaming/mod.rs +++ b/codex-rs/tui/src/streaming/mod.rs @@ -11,6 +11,7 @@ pub(crate) enum StreamKind { pub(crate) struct StreamState { pub(crate) collector: MarkdownStreamCollector, pub(crate) streamer: AnimatedLineStreamer, + pub(crate) has_seen_delta: bool, } impl StreamState { @@ -18,11 +19,13 @@ impl StreamState { Self { collector: MarkdownStreamCollector::new(), streamer: AnimatedLineStreamer::new(), + has_seen_delta: false, } } pub(crate) fn clear(&mut self) { self.collector.clear(); self.streamer.clear(); + self.has_seen_delta = false; } pub(crate) fn step(&mut self) -> crate::markdown_stream::StepResult { self.streamer.step() From e8ffecd632584e57bf3a670ecf46c54ad4f3e424 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Wed, 13 Aug 2025 18:56:29 -0700 Subject: [PATCH 013/135] Clarify PR/Contribution guidelines and issue templates (#2281) Co-authored-by: Dylan --- .github/ISSUE_TEMPLATE/4-feature-request.yml | 31 ++++++++++++++++++++ .github/pull_request_template.md | 6 ++++ README.md | 13 +++++--- 3 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/4-feature-request.yml create mode 100644 .github/pull_request_template.md diff --git a/.github/ISSUE_TEMPLATE/4-feature-request.yml b/.github/ISSUE_TEMPLATE/4-feature-request.yml new file mode 100644 index 00000000000..70cd7c756dc --- /dev/null +++ b/.github/ISSUE_TEMPLATE/4-feature-request.yml @@ -0,0 +1,31 @@ +name: 🎁 Feature Request +description: Propose a new feature for Codex +labels: + - feature + - needs triage +body: + - type: markdown + attributes: + value: | + Is Codex missing a feature that you'd like to see? Feel free to propose it here. + + Before you submit a feature: + 1. Search existing issues for similar features. If you find one, 👍 it rather than opening a new one. + 2. The Codex team will try to balance the varying needs of the community when prioritizing or rejecting new features. Not all features will be accepted. See [Contributing](https://github.com/openai/codex#contributing) for more details. + + - type: textarea + id: feature + attributes: + label: What feature would you like to see? + validations: + required: true + - type: textarea + id: author + attributes: + label: Are you interested in implementing this feature? + description: Please wait for acknowledgement before implementing or opening a PR. + - type: textarea + id: notes + attributes: + label: Additional information + description: Is there anything else you think we should know? diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 00000000000..03cedab29c3 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,6 @@ +# External (non-OpenAI) Pull Request Requirements + +Before opening this Pull Request, please read the "Contributing" section of the README or your PR may be closed: +https://github.com/openai/codex#contributing + +If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes. diff --git a/README.md b/README.md index 0c01654d664..596362a30a7 100644 --- a/README.md +++ b/README.md @@ -566,9 +566,13 @@ We're excited to launch a **$1 million initiative** supporting open source proje ## Contributing -This project is under active development and the code will likely change pretty significantly. We'll update this message once that's complete! +This project is under active development and the code will likely change pretty significantly. -More broadly we welcome contributions - whether you are opening your very first pull request or you're a seasoned maintainer. At the same time we care about reliability and long-term maintainability, so the bar for merging code is intentionally **high**. The guidelines below spell out what "high-quality" means in practice and should make the whole process transparent and friendly. +**At the moment, we only plan to prioritize reviewing external contributions for bugs or security fixes.** + +If you want to add a new feature or change the behavior of an existing one, please open an issue proposing the feature and get approval from an OpenAI team member before spending time building it. + +**New contributions that don't go through this process may be closed** if they aren't aligned with our current roadmap or conflict with other priorities/upcoming features. ### Development workflow @@ -593,8 +597,9 @@ More broadly we welcome contributions - whether you are opening your very first ### Review process 1. One maintainer will be assigned as a primary reviewer. -2. We may ask for changes - please do not take this personally. We value the work, we just also value consistency and long-term maintainability. -3. When there is consensus that the PR meets the bar, a maintainer will squash-and-merge. +2. If your PR adds a new feature that was not previously discussed and approved, we may choose to close your PR (see [Contributing](#contributing)). +3. We may ask for changes - please do not take this personally. We value the work, but we also value consistency and long-term maintainability. +5. When there is consensus that the PR meets the bar, a maintainer will squash-and-merge. ### Community values From 6d0eb9128e1ab1a049220000ff811334f58de74b Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Thu, 14 Aug 2025 12:08:35 +0900 Subject: [PATCH 014/135] Use enhancement tag for feature requests (#2282) --- .github/ISSUE_TEMPLATE/4-feature-request.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/4-feature-request.yml b/.github/ISSUE_TEMPLATE/4-feature-request.yml index 70cd7c756dc..fc95a67ec2d 100644 --- a/.github/ISSUE_TEMPLATE/4-feature-request.yml +++ b/.github/ISSUE_TEMPLATE/4-feature-request.yml @@ -1,7 +1,7 @@ name: 🎁 Feature Request description: Propose a new feature for Codex labels: - - feature + - enhancement - needs triage body: - type: markdown From 085f166707b4e3b4bddf34a9599201460d135ea8 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 22:53:54 -0700 Subject: [PATCH 015/135] fix: make all fields of Session private (#2285) As `Session` needs a bit of work, it will make things easier to move around if we can start by reducing the extent of its public API. This makes all the fields private, though adds three `pub(crate)` getters. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2285). * #2287 * #2286 * __->__ #2285 --- codex-rs/core/src/apply_patch.rs | 10 +++---- codex-rs/core/src/codex.rs | 46 ++++++++++++++++++++------------ codex-rs/core/src/protocol.rs | 2 +- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index dc11aed0237..21e80406e5d 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -45,17 +45,13 @@ pub(crate) async fn apply_patch( call_id: &str, action: ApplyPatchAction, ) -> InternalApplyPatchInvocation { - let writable_roots_snapshot = { - #[allow(clippy::unwrap_used)] - let guard = sess.writable_roots.lock().unwrap(); - guard.clone() - }; + let writable_roots_snapshot = sess.get_writable_roots().to_vec(); match assess_patch_safety( &action, - sess.approval_policy, + sess.get_approval_policy(), &writable_roots_snapshot, - &sess.cwd, + sess.get_cwd(), ) { SafetyCheck::AutoApprove { .. } => { InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index e9453a2490a..ff726a24263 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4,6 +4,7 @@ use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; @@ -199,23 +200,33 @@ impl Codex { } } +/// Mutable state of the agent +#[derive(Default)] +struct State { + approved_commands: HashSet>, + current_task: Option, + pending_approvals: HashMap>, + pending_input: Vec, + history: ConversationHistory, +} + /// Context for an initialized model agent /// /// A session has at most 1 running task at a time, and can be interrupted by user input. pub(crate) struct Session { client: ModelClient, - pub(crate) tx_event: Sender, + tx_event: Sender, /// The session's current working directory. All relative paths provided by /// the model as well as sandbox policies are resolved against this path /// instead of `std::env::current_dir()`. - pub(crate) cwd: PathBuf, + cwd: PathBuf, base_instructions: Option, user_instructions: Option, - pub(crate) approval_policy: AskForApproval, + approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, shell_environment_policy: ShellEnvironmentPolicy, - pub(crate) writable_roots: Mutex>, + writable_roots: Vec, disable_response_storage: bool, tools_config: ToolsConfig, @@ -236,24 +247,24 @@ pub(crate) struct Session { } impl Session { + pub(crate) fn get_writable_roots(&self) -> &[PathBuf] { + &self.writable_roots + } + + pub(crate) fn get_approval_policy(&self) -> AskForApproval { + self.approval_policy + } + + pub(crate) fn get_cwd(&self) -> &Path { + &self.cwd + } + fn resolve_path(&self, path: Option) -> PathBuf { path.as_ref() .map(PathBuf::from) .map_or_else(|| self.cwd.clone(), |p| self.cwd.join(p)) } -} -/// Mutable state of the agent -#[derive(Default)] -struct State { - approved_commands: HashSet>, - current_task: Option, - pending_approvals: HashMap>, - pending_input: Vec, - history: ConversationHistory, -} - -impl Session { pub fn set_task(&self, task: AgentTask) { let mut state = self.state.lock().unwrap(); if let Some(current_task) = state.current_task.take() { @@ -659,6 +670,7 @@ impl AgentTask { handle, } } + fn compact( sess: Arc, sub_id: String, @@ -816,7 +828,7 @@ async fn submission_loop( }, }; - let writable_roots = Mutex::new(get_writable_roots(&cwd)); + let writable_roots = get_writable_roots(&cwd); // Error messages to dispatch after SessionConfigured is sent. let mut mcp_connection_errors = Vec::::new(); diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index e984b95f98d..a7e6b1edda9 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -723,7 +723,7 @@ pub enum ReviewDecision { Abort, } -#[derive(Debug, Clone, Deserialize, Serialize)] +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)] #[serde(rename_all = "snake_case")] pub enum FileChange { Add { From 539f4b290e2e960c4347c1e9ce4648c4578e1729 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 23:00:50 -0700 Subject: [PATCH 016/135] fix: add support for exec and apply_patch approvals in the new wire format (#2286) Now when `CodexMessageProcessor` receives either a `EventMsg::ApplyPatchApprovalRequest` or a `EventMsg::ExecApprovalRequest`, it sends the appropriate request from the server to the client. When it gets a response, it forwards it on to the `CodexConversation`. Note this takes a lot of code from: https://github.com/openai/codex/blob/main/codex-rs/mcp-server/src/conversation_loop.rs https://github.com/openai/codex/blob/main/codex-rs/mcp-server/src/exec_approval.rs https://github.com/openai/codex/blob/main/codex-rs/mcp-server/src/patch_approval.rs I am copy/pasting for now because I am trying to consolidate around the new `wire_format.rs`, so I plan to delete these other files soon. Now that we have requests going both from client-to-server and server-to-client, I renamed `CodexRequest` to `ClientRequest`. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2286). * #2287 * __->__ #2286 * #2285 --- .../mcp-server/src/codex_message_processor.rs | 166 +++++++++++++++++- codex-rs/mcp-server/src/message_processor.rs | 34 ++-- codex-rs/mcp-server/src/wire_format.rs | 64 ++++++- 3 files changed, 237 insertions(+), 27 deletions(-) diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 5e0e4cad6ef..74b69fbcbfa 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -2,13 +2,20 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use codex_core::CodexConversation; use codex_core::ConversationManager; use codex_core::NewConversation; use codex_core::config::Config; use codex_core::config::ConfigOverrides; +use codex_core::protocol::ApplyPatchApprovalRequestEvent; +use codex_core::protocol::Event; +use codex_core::protocol::EventMsg; +use codex_core::protocol::ExecApprovalRequestEvent; +use codex_core::protocol::ReviewDecision; use mcp_types::JSONRPCErrorError; use mcp_types::RequestId; use tokio::sync::oneshot; +use tracing::error; use uuid::Uuid; use crate::error_code::INTERNAL_ERROR_CODE; @@ -16,10 +23,16 @@ use crate::error_code::INVALID_REQUEST_ERROR_CODE; use crate::json_to_toml::json_to_toml; use crate::outgoing_message::OutgoingMessageSender; use crate::outgoing_message::OutgoingNotification; +use crate::wire_format::APPLY_PATCH_APPROVAL_METHOD; use crate::wire_format::AddConversationListenerParams; use crate::wire_format::AddConversationSubscriptionResponse; -use crate::wire_format::CodexRequest; +use crate::wire_format::ApplyPatchApprovalParams; +use crate::wire_format::ApplyPatchApprovalResponse; +use crate::wire_format::ClientRequest; use crate::wire_format::ConversationId; +use crate::wire_format::EXEC_COMMAND_APPROVAL_METHOD; +use crate::wire_format::ExecCommandApprovalParams; +use crate::wire_format::ExecCommandApprovalResponse; use crate::wire_format::InputItem as WireInputItem; use crate::wire_format::NewConversationParams; use crate::wire_format::NewConversationResponse; @@ -52,21 +65,21 @@ impl CodexMessageProcessor { } } - pub async fn process_request(&mut self, request: CodexRequest) { + pub async fn process_request(&mut self, request: ClientRequest) { match request { - CodexRequest::NewConversation { request_id, params } => { + ClientRequest::NewConversation { request_id, params } => { // Do not tokio::spawn() to process new_conversation() // asynchronously because we need to ensure the conversation is // created before processing any subsequent messages. self.process_new_conversation(request_id, params).await; } - CodexRequest::SendUserMessage { request_id, params } => { + ClientRequest::SendUserMessage { request_id, params } => { self.send_user_message(request_id, params).await; } - CodexRequest::AddConversationListener { request_id, params } => { + ClientRequest::AddConversationListener { request_id, params } => { self.add_conversation_listener(request_id, params).await; } - CodexRequest::RemoveConversationListener { request_id, params } => { + ClientRequest::RemoveConversationListener { request_id, params } => { self.remove_conversation_listener(request_id, params).await; } } @@ -192,8 +205,12 @@ impl CodexMessageProcessor { } }; + // For now, we send a notification for every event, + // JSON-serializing the `Event` as-is, but we will move + // to creating a special enum for notifications with a + // stable wire format. let method = format!("codex/event/{}", event.msg); - let mut params = match serde_json::to_value(event) { + let mut params = match serde_json::to_value(event.clone()) { Ok(serde_json::Value::Object(map)) => map, Ok(_) => { tracing::error!("event did not serialize to an object"); @@ -211,6 +228,8 @@ impl CodexMessageProcessor { params: Some(params.into()), }) .await; + + apply_bespoke_event_handling(event, conversation_id, conversation.clone(), outgoing_for_task.clone()).await; } } } @@ -244,6 +263,61 @@ impl CodexMessageProcessor { } } +async fn apply_bespoke_event_handling( + event: Event, + conversation_id: ConversationId, + conversation: Arc, + outgoing: Arc, +) { + let Event { id: event_id, msg } = event; + match msg { + EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + call_id: _, + changes, + reason, + grant_root, + }) => { + let params = ApplyPatchApprovalParams { + conversation_id, + file_changes: changes, + reason, + grant_root, + }; + let value = serde_json::to_value(¶ms).unwrap_or_default(); + let rx = outgoing + .send_request(APPLY_PATCH_APPROVAL_METHOD, Some(value)) + .await; + // TODO(mbolin): Enforce a timeout so this task does not live indefinitely? + tokio::spawn(async move { + on_patch_approval_response(event_id, rx, conversation).await; + }); + } + EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + call_id: _, + command, + cwd, + reason, + }) => { + let params = ExecCommandApprovalParams { + conversation_id, + command, + cwd, + reason, + }; + let value = serde_json::to_value(¶ms).unwrap_or_default(); + let rx = outgoing + .send_request(EXEC_COMMAND_APPROVAL_METHOD, Some(value)) + .await; + + // TODO(mbolin): Enforce a timeout so this task does not live indefinitely? + tokio::spawn(async move { + on_exec_approval_response(event_id, rx, conversation).await; + }); + } + _ => {} + } +} + fn derive_config_from_params( params: NewConversationParams, codex_linux_sandbox_exe: Option, @@ -280,3 +354,81 @@ fn derive_config_from_params( Config::load_with_cli_overrides(cli_overrides, overrides) } + +async fn on_patch_approval_response( + event_id: String, + receiver: tokio::sync::oneshot::Receiver, + codex: Arc, +) { + let response = receiver.await; + let value = match response { + Ok(value) => value, + Err(err) => { + error!("request failed: {err:?}"); + if let Err(submit_err) = codex + .submit(Op::PatchApproval { + id: event_id.clone(), + decision: ReviewDecision::Denied, + }) + .await + { + error!("failed to submit denied PatchApproval after request failure: {submit_err}"); + } + return; + } + }; + + let response = + serde_json::from_value::(value).unwrap_or_else(|err| { + error!("failed to deserialize ApplyPatchApprovalResponse: {err}"); + ApplyPatchApprovalResponse { + decision: ReviewDecision::Denied, + } + }); + + if let Err(err) = codex + .submit(Op::PatchApproval { + id: event_id, + decision: response.decision, + }) + .await + { + error!("failed to submit PatchApproval: {err}"); + } +} + +async fn on_exec_approval_response( + event_id: String, + receiver: tokio::sync::oneshot::Receiver, + conversation: Arc, +) { + let response = receiver.await; + let value = match response { + Ok(value) => value, + Err(err) => { + tracing::error!("request failed: {err:?}"); + return; + } + }; + + // Try to deserialize `value` and then make the appropriate call to `codex`. + let response = + serde_json::from_value::(value).unwrap_or_else(|err| { + error!("failed to deserialize ExecCommandApprovalResponse: {err}"); + // If we cannot deserialize the response, we deny the request to be + // conservative. + ExecCommandApprovalResponse { + decision: ReviewDecision::Denied, + } + }); + + if let Err(err) = conversation + .submit(Op::ExecApproval { + id: event_id, + decision: response.decision, + }) + .await + { + error!("failed to submit ExecApproval: {err}"); + } +} diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index d043493c7bc..763e51bf810 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -15,7 +15,7 @@ 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::CodexRequest; +use crate::wire_format::ClientRequest; use codex_core::ConversationManager; use codex_core::config::Config as CodexConfig; @@ -23,7 +23,7 @@ use codex_core::protocol::Submission; use mcp_types::CallToolRequest; use mcp_types::CallToolRequestParams; use mcp_types::CallToolResult; -use mcp_types::ClientRequest; +use mcp_types::ClientRequest as McpClientRequest; use mcp_types::ContentBlock; use mcp_types::JSONRPCError; use mcp_types::JSONRPCErrorError; @@ -90,7 +90,7 @@ impl MessageProcessor { 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) + && let Ok(codex_request) = serde_json::from_value::(request_json) { // If the request is a Codex request, handle it with the Codex // message processor. @@ -103,7 +103,7 @@ impl MessageProcessor { // Hold on to the ID so we can respond. let request_id = request.id.clone(); - let client_request = match ClientRequest::try_from(request) { + let client_request = match McpClientRequest::try_from(request) { Ok(client_request) => client_request, Err(e) => { tracing::warn!("Failed to convert request: {e}"); @@ -113,43 +113,43 @@ impl MessageProcessor { // Dispatch to a dedicated handler for each request type. match client_request { - ClientRequest::InitializeRequest(params) => { + McpClientRequest::InitializeRequest(params) => { self.handle_initialize(request_id, params).await; } - ClientRequest::PingRequest(params) => { + McpClientRequest::PingRequest(params) => { self.handle_ping(request_id, params).await; } - ClientRequest::ListResourcesRequest(params) => { + McpClientRequest::ListResourcesRequest(params) => { self.handle_list_resources(params); } - ClientRequest::ListResourceTemplatesRequest(params) => { + McpClientRequest::ListResourceTemplatesRequest(params) => { self.handle_list_resource_templates(params); } - ClientRequest::ReadResourceRequest(params) => { + McpClientRequest::ReadResourceRequest(params) => { self.handle_read_resource(params); } - ClientRequest::SubscribeRequest(params) => { + McpClientRequest::SubscribeRequest(params) => { self.handle_subscribe(params); } - ClientRequest::UnsubscribeRequest(params) => { + McpClientRequest::UnsubscribeRequest(params) => { self.handle_unsubscribe(params); } - ClientRequest::ListPromptsRequest(params) => { + McpClientRequest::ListPromptsRequest(params) => { self.handle_list_prompts(params); } - ClientRequest::GetPromptRequest(params) => { + McpClientRequest::GetPromptRequest(params) => { self.handle_get_prompt(params); } - ClientRequest::ListToolsRequest(params) => { + McpClientRequest::ListToolsRequest(params) => { self.handle_list_tools(request_id, params).await; } - ClientRequest::CallToolRequest(params) => { + McpClientRequest::CallToolRequest(params) => { self.handle_call_tool(request_id, params).await; } - ClientRequest::SetLevelRequest(params) => { + McpClientRequest::SetLevelRequest(params) => { self.handle_set_level(params); } - ClientRequest::CompleteRequest(params) => { + McpClientRequest::CompleteRequest(params) => { self.handle_complete(params); } } diff --git a/codex-rs/mcp-server/src/wire_format.rs b/codex-rs/mcp-server/src/wire_format.rs index 61cd881b36d..5b223e604dd 100644 --- a/codex-rs/mcp-server/src/wire_format.rs +++ b/codex-rs/mcp-server/src/wire_format.rs @@ -1,6 +1,9 @@ use std::collections::HashMap; use std::fmt::Display; +use std::path::PathBuf; +use codex_core::protocol::FileChange; +use codex_core::protocol::ReviewDecision; use mcp_types::RequestId; use serde::Deserialize; use serde::Serialize; @@ -22,7 +25,7 @@ impl Display for ConversationId { /// Request from the client to the server. #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] #[serde(tag = "method", rename_all = "camelCase")] -pub enum CodexRequest { +pub enum ClientRequest { NewConversation { #[serde(rename = "id")] request_id: RequestId, @@ -139,10 +142,65 @@ pub enum InputItem { /// Local image path provided by the user. This will be converted to an /// `Image` variant (base64 data URL) during request serialization. LocalImage { - path: std::path::PathBuf, + path: PathBuf, }, } +// TODO(mbolin): Need test to ensure these constants match the enum variants. + +pub const APPLY_PATCH_APPROVAL_METHOD: &str = "applyPatchApproval"; +pub const EXEC_COMMAND_APPROVAL_METHOD: &str = "execCommandApproval"; + +/// Request initiated from the server and sent to the client. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(tag = "method", rename_all = "camelCase")] +pub enum ServerRequest { + /// Request to approve a patch. + ApplyPatchApproval { + #[serde(rename = "id")] + request_id: RequestId, + params: ApplyPatchApprovalParams, + }, + /// Request to exec a command. + ExecCommandApproval { + #[serde(rename = "id")] + request_id: RequestId, + params: ExecCommandApprovalParams, + }, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +pub struct ApplyPatchApprovalParams { + pub conversation_id: ConversationId, + pub file_changes: HashMap, + /// Optional explanatory reason (e.g. request for extra write access). + #[serde(skip_serializing_if = "Option::is_none")] + pub reason: Option, + /// When set, the agent is asking the user to allow writes under this root + /// for the remainder of the session (unclear if this is honored today). + #[serde(skip_serializing_if = "Option::is_none")] + pub grant_root: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +pub struct ExecCommandApprovalParams { + pub conversation_id: ConversationId, + pub command: Vec, + pub cwd: PathBuf, + #[serde(skip_serializing_if = "Option::is_none")] + pub reason: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +pub struct ExecCommandApprovalResponse { + pub decision: ReviewDecision, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +pub struct ApplyPatchApprovalResponse { + pub decision: ReviewDecision, +} + #[allow(clippy::unwrap_used)] #[cfg(test)] mod tests { @@ -152,7 +210,7 @@ mod tests { #[test] fn serialize_new_conversation() { - let request = CodexRequest::NewConversation { + let request = ClientRequest::NewConversation { request_id: RequestId::Integer(42), params: NewConversationParams { model: Some("gpt-5".to_string()), From f968a1327ad39a7786759ea8f1d1c088fe41e91b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 13 Aug 2025 23:12:03 -0700 Subject: [PATCH 017/135] feat: add support for an InterruptConversation request (#2287) This adds `ClientRequest::InterruptConversation`, which effectively maps directly to `Op::Interrupt`. --- * __->__ #2287 * #2286 * #2285 --- .../mcp-server/src/codex_message_processor.rs | 34 +++++++++++++++++++ codex-rs/mcp-server/src/wire_format.rs | 16 ++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 74b69fbcbfa..a13f0f2677f 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -34,6 +34,8 @@ use crate::wire_format::EXEC_COMMAND_APPROVAL_METHOD; use crate::wire_format::ExecCommandApprovalParams; use crate::wire_format::ExecCommandApprovalResponse; use crate::wire_format::InputItem as WireInputItem; +use crate::wire_format::InterruptConversationParams; +use crate::wire_format::InterruptConversationResponse; use crate::wire_format::NewConversationParams; use crate::wire_format::NewConversationResponse; use crate::wire_format::RemoveConversationListenerParams; @@ -76,6 +78,9 @@ impl CodexMessageProcessor { ClientRequest::SendUserMessage { request_id, params } => { self.send_user_message(request_id, params).await; } + ClientRequest::InterruptConversation { request_id, params } => { + self.interrupt_conversation(request_id, params).await; + } ClientRequest::AddConversationListener { request_id, params } => { self.add_conversation_listener(request_id, params).await; } @@ -164,6 +169,35 @@ impl CodexMessageProcessor { .await; } + async fn interrupt_conversation( + &mut self, + request_id: RequestId, + params: InterruptConversationParams, + ) { + let InterruptConversationParams { conversation_id } = params; + let Ok(conversation) = self + .conversation_manager + .get_conversation(conversation_id.0) + .await + else { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!("conversation not found: {conversation_id}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + }; + + let _ = conversation.submit(Op::Interrupt).await; + + // Apparently CodexConversation does not send an ack for Op::Interrupt, + // so we can reply to the request right away. + self.outgoing + .send_response(request_id, InterruptConversationResponse {}) + .await; + } + async fn add_conversation_listener( &mut self, request_id: RequestId, diff --git a/codex-rs/mcp-server/src/wire_format.rs b/codex-rs/mcp-server/src/wire_format.rs index 5b223e604dd..f2702a1d999 100644 --- a/codex-rs/mcp-server/src/wire_format.rs +++ b/codex-rs/mcp-server/src/wire_format.rs @@ -36,7 +36,11 @@ pub enum ClientRequest { request_id: RequestId, params: SendUserMessageParams, }, - + InterruptConversation { + #[serde(rename = "id")] + request_id: RequestId, + params: InterruptConversationParams, + }, AddConversationListener { #[serde(rename = "id")] request_id: RequestId, @@ -112,6 +116,16 @@ pub struct SendUserMessageParams { pub items: Vec, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct InterruptConversationParams { + pub conversation_id: ConversationId, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct InterruptConversationResponse {} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] #[serde(rename_all = "camelCase")] pub struct SendUserMessageResponse {} From cf7a7e63a376a90a51bd793319b5a323ab62834d Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 14 Aug 2025 09:55:28 -0700 Subject: [PATCH 018/135] exploration: create Session as part of Codex::spawn() (#2291) Historically, `Codex::spawn()` would create the instance of `Codex` and enforce, by construction, that `Op::ConfigureSession` was the first `Op` submitted via `submit()`. Then over in `submission_loop()`, it would handle the case for taking the parameters of `Op::ConfigureSession` and turning it into a `Session`. This approach has two challenges from a state management perspective: https://github.com/openai/codex/blob/f968a1327ad39a7786759ea8f1d1c088fe41e91b/codex-rs/core/src/codex.rs#L718 - The local `sess` variable in `submission_loop()` has to be `mut` and `Option>` because it is not invariant that a `Session` is present for the lifetime of the loop, so there is a lot of logic to deal with the case where `sess` is `None` (e.g., the `send_no_session_event` function and all of its callsites). - `submission_loop()` is written in such a way that `Op::ConfigureSession` could be observed multiple times, but in practice, it is only observed exactly once at the start of the loop. In this PR, we try to simplify the state management by _removing_ the `Op::ConfigureSession` enum variant and constructing the `Session` as part of `Codex::spawn()` so that it can be passed to `submission_loop()` as `Arc`. The original logic from the `Op::ConfigureSession` has largely been moved to the new `Session::new()` constructor. --- Incidentally, I also noticed that the handling of `Op::ConfigureSession` can result in events being dispatched in addition to `EventMsg::SessionConfigured`, as an `EventMsg::Error` is created for every MCP initialization error, so it is important to preserve that behavior: https://github.com/openai/codex/blob/f968a1327ad39a7786759ea8f1d1c088fe41e91b/codex-rs/core/src/codex.rs#L901-L916 Though admittedly, I believe this does not play nice with #2264, as these error messages will likely be dispatched before the client has a chance to call `addConversationListener`, so we likely need to make it so `newConversation` automatically creates the subscription, but we must also guarantee that the "ack" from `newConversation` is returned before any other conversation-related notifications are sent so the client knows what `conversation_id` to match on. --- codex-rs/core/src/codex.rs | 547 ++++++++++------------ codex-rs/core/src/conversation_manager.rs | 4 +- codex-rs/core/src/protocol.rs | 49 -- 3 files changed, 260 insertions(+), 340 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index ff726a24263..2ae1db5bd5c 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -30,6 +30,7 @@ use tracing::trace; use tracing::warn; use uuid::Uuid; +use crate::ModelProviderInfo; use crate::apply_patch::ApplyPatchExec; use crate::apply_patch::CODEX_APPLY_PATCH_ARG1; use crate::apply_patch::InternalApplyPatchInvocation; @@ -41,6 +42,8 @@ use crate::client_common::EnvironmentContext; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; use crate::config::Config; +use crate::config_types::ReasoningEffort as ReasoningEffortConfig; +use crate::config_types::ReasoningSummary as ReasoningSummaryConfig; use crate::config_types::ShellEnvironmentPolicy; use crate::conversation_history::ConversationHistory; use crate::error::CodexErr; @@ -118,22 +121,25 @@ pub struct Codex { /// unique session id. pub struct CodexSpawnOk { pub codex: Codex, - pub init_id: String, pub session_id: Uuid, } +pub(crate) const INITIAL_SUBMIT_ID: &str = ""; + impl Codex { /// Spawn a new [`Codex`] and initialize the session. pub async fn spawn(config: Config, auth: Option) -> CodexResult { - // experimental resume path (undocumented) - let resume_path = config.experimental_resume.clone(); - info!("resume_path: {resume_path:?}"); let (tx_sub, rx_sub) = async_channel::bounded(64); let (tx_event, rx_event) = async_channel::unbounded(); let user_instructions = get_user_instructions(&config).await; - let configure_session = Op::ConfigureSession { + let config = Arc::new(config); + let resume_path = config.experimental_resume.clone(); + + let session_id = Uuid::new_v4(); + let configure_session = ConfigureSession { + session_id, provider: config.model_provider.clone(), model: config.model.clone(), model_reasoning_effort: config.model_reasoning_effort, @@ -145,28 +151,26 @@ impl Codex { disable_response_storage: config.disable_response_storage, notify: config.notify.clone(), cwd: config.cwd.clone(), - resume_path: resume_path.clone(), + resume_path, }; - let config = Arc::new(config); - // Generate a unique ID for the lifetime of this Codex session. - let session_id = Uuid::new_v4(); + let session = Session::new(configure_session, config.clone(), auth, tx_event.clone()) + .await + .map_err(|e| { + error!("Failed to create session: {e:#}"); + CodexErr::InternalAgentDied + })?; // This task will run until Op::Shutdown is received. - tokio::spawn(submission_loop(session_id, config, auth, rx_sub, tx_event)); + tokio::spawn(submission_loop(session, config, rx_sub)); let codex = Codex { next_id: AtomicU64::new(0), tx_sub, rx_event, }; - let init_id = codex.submit(configure_session).await?; - Ok(CodexSpawnOk { - codex, - init_id, - session_id, - }) + Ok(CodexSpawnOk { codex, session_id }) } /// Submit the `op` wrapped in a `Submission` with a unique ID. @@ -214,6 +218,7 @@ struct State { /// /// A session has at most 1 running task at a time, and can be interrupted by user input. pub(crate) struct Session { + session_id: Uuid, client: ModelClient, tx_event: Sender, @@ -246,7 +251,216 @@ pub(crate) struct Session { show_raw_agent_reasoning: bool, } +/// Configure the model session. +struct ConfigureSession { + session_id: Uuid, + + /// Provider identifier ("openai", "openrouter", ...). + provider: ModelProviderInfo, + + /// If not specified, server will use its default model. + model: String, + + model_reasoning_effort: ReasoningEffortConfig, + model_reasoning_summary: ReasoningSummaryConfig, + + /// Model instructions that are appended to the base instructions. + user_instructions: Option, + + /// Base instructions override. + base_instructions: Option, + + /// When to escalate for approval for execution + approval_policy: AskForApproval, + /// How to sandbox commands executed in the system + sandbox_policy: SandboxPolicy, + /// Disable server-side response storage (send full context each request) + disable_response_storage: bool, + + /// Optional external notifier command tokens. Present only when the + /// client wants the agent to spawn a program after each completed + /// turn. + notify: Option>, + + /// Working directory that should be treated as the *root* of the + /// session. All relative paths supplied by the model as well as the + /// execution sandbox are resolved against this directory **instead** + /// of the process-wide current working directory. CLI front-ends are + /// expected to expand this to an absolute path before sending the + /// `ConfigureSession` operation so that the business-logic layer can + /// operate deterministically. + cwd: PathBuf, + + resume_path: Option, +} + impl Session { + async fn new( + configure_session: ConfigureSession, + config: Arc, + auth: Option, + tx_event: Sender, + ) -> anyhow::Result> { + let ConfigureSession { + mut session_id, + provider, + model, + model_reasoning_effort, + model_reasoning_summary, + user_instructions, + base_instructions, + approval_policy, + sandbox_policy, + disable_response_storage, + notify, + cwd, + resume_path, + } = configure_session; + debug!("Configuring session: model={model}; provider={provider:?}"); + if !cwd.is_absolute() { + return Err(anyhow::anyhow!("cwd is not absolute: {cwd:?}")); + } + + // Error messages to dispatch after SessionConfigured is sent. + let mut post_session_configured_error_events = Vec::::new(); + + // If `resume_path` is specified, then fail if we cannot resume the + // existing rollout. Though if `resume_path` is not specified and we + // fail to create a `RolloutRecorder`, potentially due to some sort of + // I/O error in attempting to create the log file, then we should still + // create the `Session`, but we do log an error. + let mut restored_items: Option> = None; + let rollout_recorder: Option = match resume_path.as_ref() { + Some(path) => match RolloutRecorder::resume(path, cwd.clone()).await { + Ok((rec, saved)) => { + session_id = saved.session_id; + if !saved.items.is_empty() { + restored_items = Some(saved.items); + } + Some(rec) + } + Err(e) => { + return Err(anyhow::anyhow!( + "failed to resume rollout from {path:?}: {e}" + )); + } + }, + None => { + match RolloutRecorder::new(&config, session_id, user_instructions.clone()).await { + Ok(r) => Some(r), + Err(e) => { + let message = format!("failed to initialise rollout recorder: {e}"); + post_session_configured_error_events.push(Event { + id: INITIAL_SUBMIT_ID.to_owned(), + msg: EventMsg::Error(ErrorEvent { + message: message.clone(), + }), + }); + warn!(message); + None + } + } + } + }; + + let client = ModelClient::new( + config.clone(), + auth.clone(), + provider.clone(), + model_reasoning_effort, + model_reasoning_summary, + session_id, + ); + + // Create the mutable state for the Session. + let mut state = State { + history: ConversationHistory::new(), + ..Default::default() + }; + if let Some(restored_items) = restored_items { + state.history.record_items(&restored_items); + } + + let writable_roots = get_writable_roots(&cwd); + + let (mcp_connection_manager, failed_clients) = + match McpConnectionManager::new(config.mcp_servers.clone()).await { + Ok((mgr, failures)) => (mgr, failures), + Err(e) => { + let message = format!("Failed to create MCP connection manager: {e:#}"); + error!("{message}"); + post_session_configured_error_events.push(Event { + id: INITIAL_SUBMIT_ID.to_owned(), + msg: EventMsg::Error(ErrorEvent { message }), + }); + (McpConnectionManager::default(), Default::default()) + } + }; + + // Surface individual client start-up failures to the user. + if !failed_clients.is_empty() { + for (server_name, err) in failed_clients { + let message = format!("MCP client for `{server_name}` failed to start: {err:#}"); + error!("{message}"); + post_session_configured_error_events.push(Event { + id: INITIAL_SUBMIT_ID.to_owned(), + msg: EventMsg::Error(ErrorEvent { message }), + }); + } + } + + let default_shell = shell::default_user_shell().await; + let sess = Arc::new(Session { + session_id, + client, + tools_config: ToolsConfig::new( + &config.model_family, + approval_policy, + sandbox_policy.clone(), + config.include_plan_tool, + ), + tx_event: tx_event.clone(), + user_instructions, + base_instructions, + approval_policy, + sandbox_policy, + shell_environment_policy: config.shell_environment_policy.clone(), + cwd, + writable_roots, + mcp_connection_manager, + notify, + state: Mutex::new(state), + rollout: Mutex::new(rollout_recorder), + codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(), + disable_response_storage, + user_shell: default_shell, + show_raw_agent_reasoning: config.show_raw_agent_reasoning, + }); + + // Gather history metadata for SessionConfiguredEvent. + let (history_log_id, history_entry_count) = + crate::message_history::history_metadata(&config).await; + + // ack + let events = std::iter::once(Event { + id: INITIAL_SUBMIT_ID.to_owned(), + msg: EventMsg::SessionConfigured(SessionConfiguredEvent { + session_id, + model, + history_log_id, + history_entry_count, + }), + }) + .chain(post_session_configured_error_events.into_iter()); + for event in events { + if let Err(e) = tx_event.send(event).await { + error!("failed to send event: {e:?}"); + } + } + + Ok(sess) + } + pub(crate) fn get_writable_roots(&self) -> &[PathBuf] { &self.writable_roots } @@ -628,16 +842,6 @@ impl Drop for Session { } } -impl State { - pub fn partial_clone(&self) -> Self { - Self { - approved_commands: self.approved_commands.clone(), - history: self.history.clone(), - ..Default::default() - } - } -} - #[derive(Clone, Debug)] pub(crate) struct ExecCommandContext { pub(crate) sub_id: String, @@ -708,262 +912,36 @@ impl AgentTask { } } -async fn submission_loop( - mut session_id: Uuid, - config: Arc, - auth: Option, - rx_sub: Receiver, - tx_event: Sender, -) { - let mut sess: Option> = None; - // shorthand - send an event when there is no active session - let send_no_session_event = |sub_id: String| async { - let event = Event { - id: sub_id, - msg: EventMsg::Error(ErrorEvent { - message: "No session initialized, expected 'ConfigureSession' as first Op" - .to_string(), - }), - }; - tx_event.send(event).await.ok(); - }; - +async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiver) { // To break out of this loop, send Op::Shutdown. while let Ok(sub) = rx_sub.recv().await { debug!(?sub, "Submission"); match sub.op { Op::Interrupt => { - let sess = match sess.as_ref() { - Some(sess) => sess, - None => { - send_no_session_event(sub.id).await; - continue; - } - }; sess.abort(); } - Op::ConfigureSession { - provider, - model, - model_reasoning_effort, - model_reasoning_summary, - user_instructions, - base_instructions, - approval_policy, - sandbox_policy, - disable_response_storage, - notify, - cwd, - resume_path, - } => { - debug!( - "Configuring session: model={model}; provider={provider:?}; resume={resume_path:?}" - ); - if !cwd.is_absolute() { - let message = format!("cwd is not absolute: {cwd:?}"); - error!(message); - let event = Event { - id: sub.id, - msg: EventMsg::Error(ErrorEvent { message }), - }; - if let Err(e) = tx_event.send(event).await { - error!("failed to send error message: {e:?}"); - } - return; - } - // Optionally resume an existing rollout. - let mut restored_items: Option> = None; - let rollout_recorder: Option = - if let Some(path) = resume_path.as_ref() { - match RolloutRecorder::resume(path, cwd.clone()).await { - Ok((rec, saved)) => { - session_id = saved.session_id; - if !saved.items.is_empty() { - restored_items = Some(saved.items); - } - Some(rec) - } - Err(e) => { - warn!("failed to resume rollout from {path:?}: {e}"); - None - } - } - } else { - None - }; - - let rollout_recorder = match rollout_recorder { - Some(rec) => Some(rec), - None => { - match RolloutRecorder::new(&config, session_id, user_instructions.clone()) - .await - { - Ok(r) => Some(r), - Err(e) => { - warn!("failed to initialise rollout recorder: {e}"); - None - } - } - } - }; - - let client = ModelClient::new( - config.clone(), - auth.clone(), - provider.clone(), - model_reasoning_effort, - model_reasoning_summary, - session_id, - ); - - // abort any current running session and clone its state - let state = match sess.take() { - Some(sess) => { - sess.abort(); - sess.state.lock().unwrap().partial_clone() - } - None => State { - history: ConversationHistory::new(), - ..Default::default() - }, - }; - - let writable_roots = get_writable_roots(&cwd); - - // Error messages to dispatch after SessionConfigured is sent. - let mut mcp_connection_errors = Vec::::new(); - let (mcp_connection_manager, failed_clients) = - match McpConnectionManager::new(config.mcp_servers.clone()).await { - Ok((mgr, failures)) => (mgr, failures), - Err(e) => { - let message = format!("Failed to create MCP connection manager: {e:#}"); - error!("{message}"); - mcp_connection_errors.push(Event { - id: sub.id.clone(), - msg: EventMsg::Error(ErrorEvent { message }), - }); - (McpConnectionManager::default(), Default::default()) - } - }; - - // Surface individual client start-up failures to the user. - if !failed_clients.is_empty() { - for (server_name, err) in failed_clients { - let message = - format!("MCP client for `{server_name}` failed to start: {err:#}"); - error!("{message}"); - mcp_connection_errors.push(Event { - id: sub.id.clone(), - msg: EventMsg::Error(ErrorEvent { message }), - }); - } - } - let default_shell = shell::default_user_shell().await; - sess = Some(Arc::new(Session { - client, - tools_config: ToolsConfig::new( - &config.model_family, - approval_policy, - sandbox_policy.clone(), - config.include_plan_tool, - ), - tx_event: tx_event.clone(), - user_instructions, - base_instructions, - approval_policy, - sandbox_policy, - shell_environment_policy: config.shell_environment_policy.clone(), - cwd, - writable_roots, - mcp_connection_manager, - notify, - state: Mutex::new(state), - rollout: Mutex::new(rollout_recorder), - codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(), - disable_response_storage, - user_shell: default_shell, - show_raw_agent_reasoning: config.show_raw_agent_reasoning, - })); - - // Patch restored state into the newly created session. - if let Some(sess_arc) = &sess { - if restored_items.is_some() { - let mut st = sess_arc.state.lock().unwrap(); - st.history.record_items(restored_items.unwrap().iter()); - } - } - - // Gather history metadata for SessionConfiguredEvent. - let (history_log_id, history_entry_count) = - crate::message_history::history_metadata(&config).await; - - // ack - let events = std::iter::once(Event { - id: sub.id.clone(), - msg: EventMsg::SessionConfigured(SessionConfiguredEvent { - session_id, - model, - history_log_id, - history_entry_count, - }), - }) - .chain(mcp_connection_errors.into_iter()); - for event in events { - if let Err(e) = tx_event.send(event).await { - error!("failed to send event: {e:?}"); - } - } - } Op::UserInput { items } => { - let sess = match sess.as_ref() { - Some(sess) => sess, - None => { - send_no_session_event(sub.id).await; - continue; - } - }; - // attempt to inject input into current task if let Err(items) = sess.inject_input(items) { // no current task, spawn a new one - let task = AgentTask::spawn(Arc::clone(sess), sub.id, items); + let task = AgentTask::spawn(sess.clone(), sub.id, items); sess.set_task(task); } } - Op::ExecApproval { id, decision } => { - let sess = match sess.as_ref() { - Some(sess) => sess, - None => { - send_no_session_event(sub.id).await; - continue; - } - }; - match decision { - ReviewDecision::Abort => { - sess.abort(); - } - other => sess.notify_approval(&id, other), + Op::ExecApproval { id, decision } => match decision { + ReviewDecision::Abort => { + sess.abort(); } - } - Op::PatchApproval { id, decision } => { - let sess = match sess.as_ref() { - Some(sess) => sess, - None => { - send_no_session_event(sub.id).await; - continue; - } - }; - match decision { - ReviewDecision::Abort => { - sess.abort(); - } - other => sess.notify_approval(&id, other), + other => sess.notify_approval(&id, other), + }, + Op::PatchApproval { id, decision } => match decision { + ReviewDecision::Abort => { + sess.abort(); } - } + other => sess.notify_approval(&id, other), + }, Op::AddToHistory { text } => { - // TODO: What should we do if we got AddToHistory before ConfigureSession? - // currently, if ConfigureSession has resume path, this history will be ignored - let id = session_id; + let id = sess.session_id; let config = config.clone(); tokio::spawn(async move { if let Err(e) = crate::message_history::append_entry(&text, &id, &config).await @@ -975,7 +953,7 @@ async fn submission_loop( Op::GetHistoryEntryRequest { offset, log_id } => { let config = config.clone(); - let tx_event = tx_event.clone(); + let tx_event = sess.tx_event.clone(); let sub_id = sub.id.clone(); tokio::spawn(async move { @@ -1003,14 +981,6 @@ async fn submission_loop( }); } Op::Compact => { - let sess = match sess.as_ref() { - Some(sess) => sess, - None => { - send_no_session_event(sub.id).await; - continue; - } - }; - // Create a summarization request as user input const SUMMARIZATION_PROMPT: &str = include_str!("prompt_for_compact_command.md"); @@ -1032,28 +1002,27 @@ async fn submission_loop( // Gracefully flush and shutdown rollout recorder on session end so tests // that inspect the rollout file do not race with the background writer. - if let Some(sess_arc) = sess { - let recorder_opt = sess_arc.rollout.lock().unwrap().take(); - if let Some(rec) = recorder_opt { - if let Err(e) = rec.shutdown().await { - warn!("failed to shutdown rollout recorder: {e}"); - let event = Event { - id: sub.id.clone(), - msg: EventMsg::Error(ErrorEvent { - message: "Failed to shutdown rollout recorder".to_string(), - }), - }; - if let Err(e) = tx_event.send(event).await { - warn!("failed to send error message: {e:?}"); - } + let recorder_opt = sess.rollout.lock().unwrap().take(); + if let Some(rec) = recorder_opt { + if let Err(e) = rec.shutdown().await { + warn!("failed to shutdown rollout recorder: {e}"); + let event = Event { + id: sub.id.clone(), + msg: EventMsg::Error(ErrorEvent { + message: "Failed to shutdown rollout recorder".to_string(), + }), + }; + if let Err(e) = sess.tx_event.send(event).await { + warn!("failed to send error message: {e:?}"); } } } + let event = Event { id: sub.id.clone(), msg: EventMsg::ShutdownComplete, }; - if let Err(e) = tx_event.send(event).await { + if let Err(e) = sess.tx_event.send(event).await { warn!("failed to send Shutdown event: {e}"); } break; diff --git a/codex-rs/core/src/conversation_manager.rs b/codex-rs/core/src/conversation_manager.rs index 4a6cdc1bf18..48ccdddf35f 100644 --- a/codex-rs/core/src/conversation_manager.rs +++ b/codex-rs/core/src/conversation_manager.rs @@ -7,6 +7,7 @@ use uuid::Uuid; use crate::codex::Codex; use crate::codex::CodexSpawnOk; +use crate::codex::INITIAL_SUBMIT_ID; use crate::codex_conversation::CodexConversation; use crate::config::Config; use crate::error::CodexErr; @@ -52,7 +53,6 @@ impl ConversationManager { ) -> CodexResult { let CodexSpawnOk { codex, - init_id, session_id: conversation_id, } = Codex::spawn(config, auth).await?; @@ -64,7 +64,7 @@ impl ConversationManager { Event { id, msg: EventMsg::SessionConfigured(session_configured), - } if id == init_id => session_configured, + } if id == INITIAL_SUBMIT_ID => session_configured, _ => { return Err(CodexErr::SessionConfiguredNotFirstEvent); } diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index a7e6b1edda9..f32d9ccf530 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -17,10 +17,7 @@ use serde_bytes::ByteBuf; use strum_macros::Display; use uuid::Uuid; -use crate::config_types::ReasoningEffort as ReasoningEffortConfig; -use crate::config_types::ReasoningSummary as ReasoningSummaryConfig; use crate::message_history::HistoryEntry; -use crate::model_provider_info::ModelProviderInfo; use crate::parse_command::ParsedCommand; use crate::plan_tool::UpdatePlanArgs; @@ -39,52 +36,6 @@ pub struct Submission { #[allow(clippy::large_enum_variant)] #[non_exhaustive] pub enum Op { - /// Configure the model session. - ConfigureSession { - /// Provider identifier ("openai", "openrouter", ...). - provider: ModelProviderInfo, - - /// If not specified, server will use its default model. - model: String, - - model_reasoning_effort: ReasoningEffortConfig, - model_reasoning_summary: ReasoningSummaryConfig, - - /// Model instructions that are appended to the base instructions. - user_instructions: Option, - - /// Base instructions override. - base_instructions: Option, - - /// When to escalate for approval for execution - approval_policy: AskForApproval, - /// How to sandbox commands executed in the system - sandbox_policy: SandboxPolicy, - /// Disable server-side response storage (send full context each request) - #[serde(default)] - disable_response_storage: bool, - - /// Optional external notifier command tokens. Present only when the - /// client wants the agent to spawn a program after each completed - /// turn. - #[serde(skip_serializing_if = "Option::is_none")] - #[serde(default)] - notify: Option>, - - /// Working directory that should be treated as the *root* of the - /// session. All relative paths supplied by the model as well as the - /// execution sandbox are resolved against this directory **instead** - /// of the process-wide current working directory. CLI front-ends are - /// expected to expand this to an absolute path before sending the - /// `ConfigureSession` operation so that the business-logic layer can - /// operate deterministically. - cwd: std::path::PathBuf, - - /// Path to a rollout file to resume from. - #[serde(skip_serializing_if = "Option::is_none")] - resume_path: Option, - }, - /// Abort current task. /// This server sends no corresponding Event Interrupt, From cdd33b2c0448b65c8c2467ccba4e45de30ddf762 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Thu, 14 Aug 2025 10:58:04 -0700 Subject: [PATCH 019/135] Tag InputItem (#2304) Instead of: ``` { Text: { text: string } } ``` It is now: ``` { type: "text", data: { text: string } } ``` which makes for cleaner discriminated unions --- codex-rs/mcp-server/src/wire_format.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/mcp-server/src/wire_format.rs b/codex-rs/mcp-server/src/wire_format.rs index f2702a1d999..4a2346cb3fc 100644 --- a/codex-rs/mcp-server/src/wire_format.rs +++ b/codex-rs/mcp-server/src/wire_format.rs @@ -144,6 +144,7 @@ pub struct RemoveConversationListenerParams { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] #[serde(rename_all = "camelCase")] +#[serde(tag = "type", content = "data")] pub enum InputItem { Text { text: String, From 585f7b06797ace32ea51d06797f7a120f0c4d441 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 14 Aug 2025 14:10:05 -0400 Subject: [PATCH 020/135] HistoryCell is a trait (#2283) refactors HistoryCell to be a trait instead of an enum. Also collapse the many "degenerate" HistoryCell enums which were just a store of lines into a single PlainHistoryCell type. The goal here is to allow more ways of rendering history cells (e.g. expanded/collapsed/"live"), and I expect we will return to more varied types of HistoryCell as we develop this area. --- codex-rs/tui/src/chatwidget.rs | 44 +- codex-rs/tui/src/diff_render.rs | 15 +- codex-rs/tui/src/history_cell.rs | 1345 +++++++++++++----------------- codex-rs/tui/src/lib.rs | 1 - codex-rs/tui/src/text_block.rs | 14 - 5 files changed, 632 insertions(+), 787 deletions(-) delete mode 100644 codex-rs/tui/src/text_block.rs diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 9f7f0518e22..fba3caf18bb 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -44,7 +44,9 @@ use crate::bottom_pane::BottomPaneParams; use crate::bottom_pane::CancellationEvent; use crate::bottom_pane::InputResult; use crate::exec_command::strip_bash_lc_and_escape; +use crate::history_cell; use crate::history_cell::CommandOutput; +use crate::history_cell::ExecCell; use crate::history_cell::HistoryCell; use crate::history_cell::PatchEventType; // streaming internals are provided by crate::streaming and crate::markdown_stream @@ -68,7 +70,7 @@ pub(crate) struct ChatWidget<'a> { app_event_tx: AppEventSender, codex_op_tx: UnboundedSender, bottom_pane: BottomPane<'a>, - active_exec_cell: Option, + active_exec_cell: Option, config: Config, initial_user_message: Option, total_token_usage: TokenUsage, @@ -123,7 +125,7 @@ impl ChatWidget<'_> { fn on_session_configured(&mut self, event: codex_core::protocol::SessionConfiguredEvent) { self.bottom_pane .set_history_metadata(event.history_log_id, event.history_entry_count); - self.add_to_history(HistoryCell::new_session_info(&self.config, event, true)); + self.add_to_history(&history_cell::new_session_info(&self.config, event, true)); if let Some(user_message) = self.initial_user_message.take() { self.submit_user_message(user_message); } @@ -195,14 +197,14 @@ impl ChatWidget<'_> { } fn on_error(&mut self, message: String) { - self.add_to_history(HistoryCell::new_error_event(message)); + self.add_to_history(&history_cell::new_error_event(message)); self.bottom_pane.set_task_running(false); self.stream.clear_all(); self.mark_needs_redraw(); } fn on_plan_update(&mut self, update: codex_core::plan_tool::UpdatePlanArgs) { - self.add_to_history(HistoryCell::new_plan_update(update)); + self.add_to_history(&history_cell::new_plan_update(update)); } fn on_exec_approval_request(&mut self, id: String, ev: ExecApprovalRequestEvent) { @@ -237,7 +239,7 @@ impl ChatWidget<'_> { } fn on_patch_apply_begin(&mut self, event: PatchApplyBeginEvent) { - self.add_to_history(HistoryCell::new_patch_event( + self.add_to_history(&history_cell::new_patch_event( PatchEventType::ApplyBegin { auto_approved: event.auto_approved, }, @@ -372,7 +374,7 @@ impl ChatWidget<'_> { self.active_exec_cell = None; let pending = std::mem::take(&mut self.pending_exec_completions); for (command, parsed, output) in pending { - self.add_to_history(HistoryCell::new_completed_exec_command( + self.add_to_history(&history_cell::new_completed_exec_command( command, parsed, output, )); } @@ -384,9 +386,9 @@ impl ChatWidget<'_> { event: codex_core::protocol::PatchApplyEndEvent, ) { if event.success { - self.add_to_history(HistoryCell::new_patch_apply_success(event.stdout)); + self.add_to_history(&history_cell::new_patch_apply_success(event.stdout)); } else { - self.add_to_history(HistoryCell::new_patch_apply_failure(event.stderr)); + self.add_to_history(&history_cell::new_patch_apply_failure(event.stderr)); } } @@ -402,7 +404,7 @@ impl ChatWidget<'_> { .map(|r| format!("\n{r}")) .unwrap_or_default() ); - self.add_to_history(HistoryCell::new_background_event(text)); + self.add_to_history(&history_cell::new_background_event(text)); let request = ApprovalRequest::Exec { id, @@ -419,7 +421,7 @@ impl ChatWidget<'_> { ev: ApplyPatchApprovalRequestEvent, ) { self.flush_answer_stream_with_separator(); - self.add_to_history(HistoryCell::new_patch_event( + self.add_to_history(&history_cell::new_patch_event( PatchEventType::ApprovalRequest, ev.changes.clone(), )); @@ -446,11 +448,11 @@ impl ChatWidget<'_> { ); // Accumulate parsed commands into a single active Exec cell so they stack match self.active_exec_cell.as_mut() { - Some(HistoryCell::Exec(exec)) => { + Some(exec) => { exec.parsed.extend(ev.parsed_cmd); } _ => { - self.active_exec_cell = Some(HistoryCell::new_active_exec_command( + self.active_exec_cell = Some(history_cell::new_active_exec_command( ev.command, ev.parsed_cmd, )); @@ -463,11 +465,11 @@ impl ChatWidget<'_> { pub(crate) fn handle_mcp_begin_now(&mut self, ev: McpToolCallBeginEvent) { self.flush_answer_stream_with_separator(); - self.add_to_history(HistoryCell::new_active_mcp_tool_call(ev.invocation)); + self.add_to_history(&history_cell::new_active_mcp_tool_call(ev.invocation)); } pub(crate) fn handle_mcp_end_now(&mut self, ev: McpToolCallEndEvent) { self.flush_answer_stream_with_separator(); - self.add_to_history(HistoryCell::new_completed_mcp_tool_call( + self.add_to_history(&*history_cell::new_completed_mcp_tool_call( 80, ev.invocation, ev.duration, @@ -564,14 +566,14 @@ impl ChatWidget<'_> { fn flush_active_exec_cell(&mut self) { if let Some(active) = self.active_exec_cell.take() { self.app_event_tx - .send(AppEvent::InsertHistory(active.plain_lines())); + .send(AppEvent::InsertHistory(active.display_lines())); } } - fn add_to_history(&mut self, cell: HistoryCell) { + fn add_to_history(&mut self, cell: &dyn HistoryCell) { self.flush_active_exec_cell(); self.app_event_tx - .send(AppEvent::InsertHistory(cell.plain_lines())); + .send(AppEvent::InsertHistory(cell.display_lines())); } fn submit_user_message(&mut self, user_message: UserMessage) { @@ -607,7 +609,7 @@ impl ChatWidget<'_> { // Only show the text portion in conversation history. if !text.is_empty() { - self.add_to_history(HistoryCell::new_user_prompt(text.clone())); + self.add_to_history(&history_cell::new_user_prompt(text.clone())); } } @@ -680,18 +682,18 @@ impl ChatWidget<'_> { } pub(crate) fn add_diff_output(&mut self, diff_output: String) { - self.add_to_history(HistoryCell::new_diff_output(diff_output.clone())); + self.add_to_history(&history_cell::new_diff_output(diff_output.clone())); } pub(crate) fn add_status_output(&mut self) { - self.add_to_history(HistoryCell::new_status_output( + self.add_to_history(&history_cell::new_status_output( &self.config, &self.total_token_usage, )); } pub(crate) fn add_prompts_output(&mut self) { - self.add_to_history(HistoryCell::new_prompts_output()); + self.add_to_history(&history_cell::new_prompts_output()); } /// Forward file-search results to the bottom pane. diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 2433b3442e8..5a3199c04b5 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -361,19 +361,22 @@ fn style_del() -> Style { #[cfg(test)] mod tests { use super::*; - use crate::history_cell::HistoryCell; - use crate::text_block::TextBlock; use insta::assert_snapshot; use ratatui::Terminal; use ratatui::backend::TestBackend; + use ratatui::text::Text; + use ratatui::widgets::Paragraph; + use ratatui::widgets::WidgetRef; + use ratatui::widgets::Wrap; fn snapshot_lines(name: &str, lines: Vec>, width: u16, height: u16) { let mut terminal = Terminal::new(TestBackend::new(width, height)).expect("terminal"); - let cell = HistoryCell::PendingPatch { - view: TextBlock::new(lines), - }; terminal - .draw(|f| f.render_widget_ref(&cell, f.area())) + .draw(|f| { + Paragraph::new(Text::from(lines)) + .wrap(Wrap { trim: false }) + .render_ref(f.area(), f.buffer_mut()) + }) .expect("draw"); assert_snapshot!(name, terminal.backend()); } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 8295a8976ef..f420065df80 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -3,7 +3,6 @@ use crate::diff_render::create_diff_summary; use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; use crate::slash_command::SlashCommand; -use crate::text_block::TextBlock; use crate::text_formatting::format_and_truncate_tool_result; use base64::Engine; use codex_ansi_escape::ansi_escape_line; @@ -50,18 +49,28 @@ pub(crate) enum PatchEventType { ApplyBegin { auto_approved: bool }, } -fn span_to_static(span: &Span) -> Span<'static> { - Span { - style: span.style, - content: std::borrow::Cow::Owned(span.content.clone().into_owned()), +/// Represents an event to display in the conversation history. Returns its +/// `Vec>` representation to make it easier to display in a +/// scrollable list. +pub(crate) trait HistoryCell { + fn display_lines(&self) -> Vec>; + + fn desired_height(&self, width: u16) -> u16 { + Paragraph::new(Text::from(self.display_lines())) + .wrap(Wrap { trim: false }) + .line_count(width) + .try_into() + .unwrap_or(0) } } -fn line_to_static(line: &Line) -> Line<'static> { - Line { - style: line.style, - alignment: line.alignment, - spans: line.spans.iter().map(span_to_static).collect(), +pub(crate) struct PlainHistoryCell { + lines: Vec>, +} + +impl HistoryCell for PlainHistoryCell { + fn display_lines(&self) -> Vec> { + self.lines.clone() } } @@ -70,92 +79,30 @@ pub(crate) struct ExecCell { pub(crate) parsed: Vec, pub(crate) output: Option, } +impl HistoryCell for ExecCell { + fn display_lines(&self) -> Vec> { + exec_command_lines(&self.command, &self.parsed, self.output.as_ref()) + } +} -/// Represents an event to display in the conversation history. Returns its -/// `Vec>` representation to make it easier to display in a -/// scrollable list. -pub(crate) enum HistoryCell { - /// Welcome message. - WelcomeMessage { - view: TextBlock, - }, - - /// Message from the user. - UserPrompt { - view: TextBlock, - }, - - Exec(ExecCell), - - /// An MCP tool call that has not finished yet. - ActiveMcpToolCall { - view: TextBlock, - }, - - /// Completed MCP tool call where we show the result serialized as JSON. - CompletedMcpToolCall { - view: TextBlock, - }, - - /// Completed MCP tool call where the result is an image. - /// Admittedly, [mcp_types::CallToolResult] can have multiple content types, - /// which could be a mix of text and images, so we need to tighten this up. - // NOTE: For image output we keep the *original* image around and lazily - // compute a resized copy that fits the available cell width. Caching the - // resized version avoids doing the potentially expensive rescale twice - // because the scroll-view first calls `height()` for layouting and then - // `render_window()` for painting. - CompletedMcpToolCallWithImageOutput { - _image: DynamicImage, - }, - - /// Background event. - BackgroundEvent { - view: TextBlock, - }, - - /// Output from the `/diff` command. - GitDiffOutput { - view: TextBlock, - }, - - /// Output from the `/status` command. - StatusOutput { - view: TextBlock, - }, - - /// Output from the `/prompts` command. - PromptsOutput { - view: TextBlock, - }, - - /// Error event from the backend. - ErrorEvent { - view: TextBlock, - }, - - /// Info describing the newly-initialized session. - SessionInfo { - view: TextBlock, - }, - - /// A pending code patch that is awaiting user approval. Mirrors the - /// behaviour of `ExecCell` so the user sees *what* patch the - /// model wants to apply before being prompted to approve or deny it. - PendingPatch { - view: TextBlock, - }, - - /// A human‑friendly rendering of the model's current plan and step - /// statuses provided via the `update_plan` tool. - PlanUpdate { - view: TextBlock, - }, - - /// Result of applying a patch (success or failure) with optional output. - PatchApplyResult { - view: TextBlock, - }, +impl WidgetRef for &ExecCell { + fn render_ref(&self, area: Rect, buf: &mut Buffer) { + Paragraph::new(Text::from(self.display_lines())) + .wrap(Wrap { trim: false }) + .render(area, buf); + } +} + +struct CompletedMcpToolCallWithImageOutput { + _image: DynamicImage, +} +impl HistoryCell for CompletedMcpToolCallWithImageOutput { + fn display_lines(&self) -> Vec> { + vec![ + Line::from("tool result (image output omitted)"), + Line::from(""), + ] + } } const TOOL_CALL_MAX_LINES: usize = 5; @@ -181,742 +128,650 @@ fn pretty_provider_name(id: &str) -> String { } } -impl HistoryCell { - /// Return a cloned, plain representation of the cell's lines suitable for - /// one‑shot insertion into the terminal scrollback. Image cells are - /// represented with a simple placeholder. - /// These lines are also rendered directly by ratatui wrapped in a Paragraph. - pub(crate) fn plain_lines(&self) -> Vec> { - match self { - HistoryCell::WelcomeMessage { view } - | HistoryCell::UserPrompt { view } - | HistoryCell::BackgroundEvent { view } - | HistoryCell::GitDiffOutput { view } - | HistoryCell::StatusOutput { view } - | HistoryCell::PromptsOutput { view } - | HistoryCell::ErrorEvent { view } - | HistoryCell::SessionInfo { view } - | HistoryCell::CompletedMcpToolCall { view } - | HistoryCell::PendingPatch { view } - | HistoryCell::PlanUpdate { view } - | HistoryCell::PatchApplyResult { view } - | HistoryCell::ActiveMcpToolCall { view, .. } => { - view.lines.iter().map(line_to_static).collect() - } - HistoryCell::Exec(ExecCell { - command, - parsed, - output, - }) => HistoryCell::exec_command_lines(command, parsed, output.as_ref()), - HistoryCell::CompletedMcpToolCallWithImageOutput { .. } => vec![ - Line::from("tool result (image output omitted)"), - Line::from(""), - ], - } - } +pub(crate) fn new_background_event(message: String) -> PlainHistoryCell { + let mut lines: Vec> = Vec::new(); + lines.push(Line::from("event".dim())); + lines.extend(message.lines().map(|line| ansi_escape_line(line).dim())); + lines.push(Line::from("")); + PlainHistoryCell { lines } +} - pub(crate) fn new_background_event(message: String) -> Self { - let mut lines: Vec> = Vec::new(); - lines.push(Line::from("event".dim())); - lines.extend(message.lines().map(|line| ansi_escape_line(line).dim())); - lines.push(Line::from("")); - HistoryCell::BackgroundEvent { - view: TextBlock::new(lines), - } - } +pub(crate) fn new_session_info( + config: &Config, + event: SessionConfiguredEvent, + is_first_event: bool, +) -> PlainHistoryCell { + let SessionConfiguredEvent { + model, + session_id: _, + history_log_id: _, + history_entry_count: _, + } = event; + if is_first_event { + let cwd_str = match relativize_to_home(&config.cwd) { + Some(rel) if !rel.as_os_str().is_empty() => format!("~/{}", rel.display()), + Some(_) => "~".to_string(), + None => config.cwd.display().to_string(), + }; - pub(crate) fn desired_height(&self, width: u16) -> u16 { - Paragraph::new(Text::from(self.plain_lines())) - .wrap(Wrap { trim: false }) - .line_count(width) - .try_into() - .unwrap_or(0) + let lines: Vec> = vec![ + Line::from(vec![ + Span::raw(">_ ").dim(), + Span::styled( + "You are using OpenAI Codex in", + Style::default().add_modifier(Modifier::BOLD), + ), + Span::raw(format!(" {cwd_str}")).dim(), + ]), + Line::from("".dim()), + Line::from(" To get started, describe a task or try one of these commands:".dim()), + Line::from("".dim()), + Line::from(format!(" /init - {}", SlashCommand::Init.description()).dim()), + Line::from(format!(" /status - {}", SlashCommand::Status.description()).dim()), + Line::from(format!(" /diff - {}", SlashCommand::Diff.description()).dim()), + Line::from(format!(" /prompts - {}", SlashCommand::Prompts.description()).dim()), + Line::from("".dim()), + ]; + PlainHistoryCell { lines } + } else if config.model == model { + PlainHistoryCell { lines: Vec::new() } + } else { + let lines = vec![ + Line::from("model changed:".magenta().bold()), + Line::from(format!("requested: {}", config.model)), + Line::from(format!("used: {model}")), + Line::from(""), + ]; + PlainHistoryCell { lines } } +} - pub(crate) fn new_session_info( - config: &Config, - event: SessionConfiguredEvent, - is_first_event: bool, - ) -> Self { - let SessionConfiguredEvent { - model, - session_id: _, - history_log_id: _, - history_entry_count: _, - } = event; - if is_first_event { - let cwd_str = match relativize_to_home(&config.cwd) { - Some(rel) if !rel.as_os_str().is_empty() => format!("~/{}", rel.display()), - Some(_) => "~".to_string(), - None => config.cwd.display().to_string(), - }; +pub(crate) fn new_user_prompt(message: String) -> PlainHistoryCell { + let mut lines: Vec> = Vec::new(); + lines.push(Line::from("user".cyan().bold())); + lines.extend(message.lines().map(|l| Line::from(l.to_string()))); + lines.push(Line::from("")); - let lines: Vec> = vec![ - Line::from(vec![ - Span::raw(">_ ").dim(), - Span::styled( - "You are using OpenAI Codex in", - Style::default().add_modifier(Modifier::BOLD), - ), - Span::raw(format!(" {cwd_str}")).dim(), - ]), - Line::from("".dim()), - Line::from(" To get started, describe a task or try one of these commands:".dim()), - Line::from("".dim()), - Line::from(format!(" /init - {}", SlashCommand::Init.description()).dim()), - Line::from(format!(" /status - {}", SlashCommand::Status.description()).dim()), - Line::from(format!(" /diff - {}", SlashCommand::Diff.description()).dim()), - Line::from(format!(" /prompts - {}", SlashCommand::Prompts.description()).dim()), - Line::from("".dim()), - ]; - HistoryCell::WelcomeMessage { - view: TextBlock::new(lines), - } - } else if config.model == model { - HistoryCell::SessionInfo { - view: TextBlock::new(Vec::new()), - } - } else { - let lines = vec![ - Line::from("model changed:".magenta().bold()), - Line::from(format!("requested: {}", config.model)), - Line::from(format!("used: {model}")), - Line::from(""), - ]; - HistoryCell::SessionInfo { - view: TextBlock::new(lines), - } - } - } + PlainHistoryCell { lines } +} - pub(crate) fn new_user_prompt(message: String) -> Self { - let mut lines: Vec> = Vec::new(); - lines.push(Line::from("user".cyan().bold())); - lines.extend(message.lines().map(|l| Line::from(l.to_string()))); - lines.push(Line::from("")); +pub(crate) fn new_active_exec_command( + command: Vec, + parsed: Vec, +) -> ExecCell { + new_exec_cell(command, parsed, None) +} - HistoryCell::UserPrompt { - view: TextBlock::new(lines), - } - } +pub(crate) fn new_completed_exec_command( + command: Vec, + parsed: Vec, + output: CommandOutput, +) -> ExecCell { + new_exec_cell(command, parsed, Some(output)) +} - pub(crate) fn new_active_exec_command( - command: Vec, - parsed: Vec, - ) -> Self { - HistoryCell::new_exec_cell(command, parsed, None) +fn new_exec_cell( + command: Vec, + parsed: Vec, + output: Option, +) -> ExecCell { + ExecCell { + command, + parsed, + output, } +} - pub(crate) fn new_completed_exec_command( - command: Vec, - parsed: Vec, - output: CommandOutput, - ) -> Self { - HistoryCell::new_exec_cell(command, parsed, Some(output)) +fn exec_command_lines( + command: &[String], + parsed: &[ParsedCommand], + output: Option<&CommandOutput>, +) -> Vec> { + match parsed.is_empty() { + true => new_exec_command_generic(command, output), + false => new_parsed_command(parsed, output), } +} - fn new_exec_cell( - command: Vec, - parsed: Vec, - output: Option, - ) -> Self { - HistoryCell::Exec(ExecCell { - command, - parsed, - output, - }) - } +fn new_parsed_command( + parsed_commands: &[ParsedCommand], + output: Option<&CommandOutput>, +) -> Vec> { + let mut lines: Vec = vec![match output { + None => Line::from("⚙︎ Working".magenta().bold()), + Some(o) if o.exit_code == 0 => Line::from("✓ Completed".green().bold()), + Some(o) => Line::from(format!("✗ Failed (exit {})", o.exit_code).red().bold()), + }]; + + for (i, parsed) in parsed_commands.iter().enumerate() { + let text = match parsed { + ParsedCommand::Read { name, .. } => format!("📖 {name}"), + ParsedCommand::ListFiles { cmd, path } => match path { + Some(p) => format!("📂 {p}"), + None => format!("📂 {}", shlex_join_safe(cmd)), + }, + ParsedCommand::Search { query, path, cmd } => match (query, path) { + (Some(q), Some(p)) => format!("🔎 {q} in {p}"), + (Some(q), None) => format!("🔎 {q}"), + (None, Some(p)) => format!("🔎 {p}"), + (None, None) => format!("🔎 {}", shlex_join_safe(cmd)), + }, + ParsedCommand::Format { .. } => "✨ Formatting".to_string(), + ParsedCommand::Test { cmd } => format!("🧪 {}", shlex_join_safe(cmd)), + ParsedCommand::Lint { cmd, .. } => format!("🧹 {}", shlex_join_safe(cmd)), + ParsedCommand::Unknown { cmd } => format!("⌨️ {}", shlex_join_safe(cmd)), + }; - fn exec_command_lines( - command: &[String], - parsed: &[ParsedCommand], - output: Option<&CommandOutput>, - ) -> Vec> { - match parsed.is_empty() { - true => HistoryCell::new_exec_command_generic(command, output), - false => HistoryCell::new_parsed_command(parsed, output), + let first_prefix = if i == 0 { " └ " } else { " " }; + for (j, line_text) in text.lines().enumerate() { + let prefix = if j == 0 { first_prefix } else { " " }; + lines.push(Line::from(vec![ + Span::styled(prefix, Style::default().add_modifier(Modifier::DIM)), + Span::styled(line_text.to_string(), Style::default().fg(LIGHT_BLUE)), + ])); } } - fn new_parsed_command( - parsed_commands: &[ParsedCommand], - output: Option<&CommandOutput>, - ) -> Vec> { - let mut lines: Vec = vec![match output { - None => Line::from("⚙︎ Working".magenta().bold()), - Some(o) if o.exit_code == 0 => Line::from("✓ Completed".green().bold()), - Some(o) => Line::from(format!("✗ Failed (exit {})", o.exit_code).red().bold()), - }]; + lines.extend(output_lines(output, true, false)); + lines.push(Line::from("")); - for (i, parsed) in parsed_commands.iter().enumerate() { - let text = match parsed { - ParsedCommand::Read { name, .. } => format!("📖 {name}"), - ParsedCommand::ListFiles { cmd, path } => match path { - Some(p) => format!("📂 {p}"), - None => format!("📂 {}", shlex_join_safe(cmd)), - }, - ParsedCommand::Search { query, path, cmd } => match (query, path) { - (Some(q), Some(p)) => format!("🔎 {q} in {p}"), - (Some(q), None) => format!("🔎 {q}"), - (None, Some(p)) => format!("🔎 {p}"), - (None, None) => format!("🔎 {}", shlex_join_safe(cmd)), - }, - ParsedCommand::Format { .. } => "✨ Formatting".to_string(), - ParsedCommand::Test { cmd } => format!("🧪 {}", shlex_join_safe(cmd)), - ParsedCommand::Lint { cmd, .. } => format!("🧹 {}", shlex_join_safe(cmd)), - ParsedCommand::Unknown { cmd } => format!("⌨️ {}", shlex_join_safe(cmd)), - }; + lines +} - let first_prefix = if i == 0 { " └ " } else { " " }; - for (j, line_text) in text.lines().enumerate() { - let prefix = if j == 0 { first_prefix } else { " " }; - lines.push(Line::from(vec![ - Span::styled(prefix, Style::default().add_modifier(Modifier::DIM)), - Span::styled(line_text.to_string(), Style::default().fg(LIGHT_BLUE)), - ])); - } - } +fn new_exec_command_generic( + command: &[String], + output: Option<&CommandOutput>, +) -> Vec> { + let mut lines: Vec> = Vec::new(); + let command_escaped = strip_bash_lc_and_escape(command); + let mut cmd_lines = command_escaped.lines(); + if let Some(first) = cmd_lines.next() { + lines.push(Line::from(vec![ + "⚡ Running ".to_string().magenta(), + first.to_string().into(), + ])); + } else { + lines.push(Line::from("⚡ Running".to_string().magenta())); + } + for cont in cmd_lines { + lines.push(Line::from(cont.to_string())); + } - lines.extend(output_lines(output, true, false)); - lines.push(Line::from("")); + lines.extend(output_lines(output, false, true)); - lines - } + lines +} - fn new_exec_command_generic( - command: &[String], - output: Option<&CommandOutput>, - ) -> Vec> { - let mut lines: Vec> = Vec::new(); - let command_escaped = strip_bash_lc_and_escape(command); - let mut cmd_lines = command_escaped.lines(); - if let Some(first) = cmd_lines.next() { - lines.push(Line::from(vec![ - "⚡ Running ".to_string().magenta(), - first.to_string().into(), - ])); - } else { - lines.push(Line::from("⚡ Running".to_string().magenta())); - } - for cont in cmd_lines { - lines.push(Line::from(cont.to_string())); - } +pub(crate) fn new_active_mcp_tool_call(invocation: McpInvocation) -> PlainHistoryCell { + let title_line = Line::from(vec!["tool".magenta(), " running...".dim()]); + let lines: Vec = vec![ + title_line, + format_mcp_invocation(invocation.clone()), + Line::from(""), + ]; - lines.extend(output_lines(output, false, true)); + PlainHistoryCell { lines } +} - lines - } +/// If the first content is an image, return a new cell with the image. +/// TODO(rgwood-dd): Handle images properly even if they're not the first result. +fn try_new_completed_mcp_tool_call_with_image_output( + result: &Result, +) -> Option { + match result { + Ok(mcp_types::CallToolResult { content, .. }) => { + if let Some(mcp_types::ContentBlock::ImageContent(image)) = content.first() { + let raw_data = match base64::engine::general_purpose::STANDARD.decode(&image.data) { + Ok(data) => data, + Err(e) => { + error!("Failed to decode image data: {e}"); + return None; + } + }; + let reader = match ImageReader::new(Cursor::new(raw_data)).with_guessed_format() { + Ok(reader) => reader, + Err(e) => { + error!("Failed to guess image format: {e}"); + return None; + } + }; - pub(crate) fn new_active_mcp_tool_call(invocation: McpInvocation) -> Self { - let title_line = Line::from(vec!["tool".magenta(), " running...".dim()]); - let lines: Vec = vec![ - title_line, - format_mcp_invocation(invocation.clone()), - Line::from(""), - ]; + let image = match reader.decode() { + Ok(image) => image, + Err(e) => { + error!("Image decoding failed: {e}"); + return None; + } + }; - HistoryCell::ActiveMcpToolCall { - view: TextBlock::new(lines), + Some(CompletedMcpToolCallWithImageOutput { _image: image }) + } else { + None + } } + _ => None, } +} - /// If the first content is an image, return a new cell with the image. - /// TODO(rgwood-dd): Handle images properly even if they're not the first result. - fn try_new_completed_mcp_tool_call_with_image_output( - result: &Result, - ) -> Option { - match result { - Ok(mcp_types::CallToolResult { content, .. }) => { - if let Some(mcp_types::ContentBlock::ImageContent(image)) = content.first() { - let raw_data = - match base64::engine::general_purpose::STANDARD.decode(&image.data) { - Ok(data) => data, - Err(e) => { - error!("Failed to decode image data: {e}"); - return None; - } - }; - let reader = match ImageReader::new(Cursor::new(raw_data)).with_guessed_format() - { - Ok(reader) => reader, - Err(e) => { - error!("Failed to guess image format: {e}"); - return None; - } - }; +pub(crate) fn new_completed_mcp_tool_call( + num_cols: usize, + invocation: McpInvocation, + duration: Duration, + success: bool, + result: Result, +) -> Box { + if let Some(cell) = try_new_completed_mcp_tool_call_with_image_output(&result) { + return Box::new(cell); + } + + let duration = format_duration(duration); + let status_str = if success { "success" } else { "failed" }; + let title_line = Line::from(vec![ + "tool".magenta(), + " ".into(), + if success { + status_str.green() + } else { + status_str.red() + }, + format!(", duration: {duration}").dim(), + ]); + + let mut lines: Vec> = Vec::new(); + lines.push(title_line); + lines.push(format_mcp_invocation(invocation)); + + match result { + Ok(mcp_types::CallToolResult { content, .. }) => { + if !content.is_empty() { + lines.push(Line::from("")); - let image = match reader.decode() { - Ok(image) => image, - Err(e) => { - error!("Image decoding failed: {e}"); - return None; + for tool_call_result in content { + let line_text = match tool_call_result { + mcp_types::ContentBlock::TextContent(text) => { + format_and_truncate_tool_result( + &text.text, + TOOL_CALL_MAX_LINES, + num_cols, + ) + } + mcp_types::ContentBlock::ImageContent(_) => { + // TODO show images even if they're not the first result, will require a refactor of `CompletedMcpToolCall` + "".to_string() + } + mcp_types::ContentBlock::AudioContent(_) => "