From 16309d6b68546c392fbec3d30fb65e404bd8e3fa Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 8 Sep 2025 21:39:18 -0700 Subject: [PATCH 01/49] chore: try switching to `cargo nextest` to speed up CI builds (#3323) I started looking at https://nexte.st/ because I was interested in a test harness that lets a test dynamically declare itself "skipped," which would be a nice alternative to this pattern: https://github.com/openai/codex/blob/4c46490e53076e807fd2068aa68c5a0e636d8b5f/codex-rs/core/tests/suite/cli_stream.rs#L22-L27 ChatGPT pointed me at https://nexte.st/, which also claims to be "up to 3x as fast as cargo test." Locally, in `codex-rs`, I see - `cargo nextest run` finishes in 19s - `cargo test` finishes in 37s Though looking at CI, the wins are quite as big, presumably because my laptop has more cores than our GitHub runners (which is a separate issue...). Comparing the [CI jobs from this PR](https://github.com/openai/codex/actions/runs/17561325162/job/49878216246?pr=3323) with that of a [recent open PR](https://github.com/openai/codex/actions/runs/17561066581/job/49877342753?pr=3321): | | `cargo test` | `cargo nextest` | | ----------------------------------------------- | ------------ | --------------- | | `macos-14 - aarch64-apple-darwin` | 2m16s | 1m51s | | `macos-14 - aarch64-apple-darwin` | 5m04s | 3m44s | | `ubuntu-24.04 - x86_64-unknown-linux-musl` | 2m02s | 1m56s | | `ubuntu-24.04-arm - aarch64-unknown-linux-musl` | 2m01s | 1m35s | | `windows-latest - x86_64-pc-windows-msvc` | 3m07s | 2m53s | | `windows-11-arm - aarch64-pc-windows-msvc` | 3m10s | 2m45s | I thought that, to start, we would only make this change in CI before declaring it the "official" way for the team to run the test suite. Though unfortunately, I do not believe that `cargo nextest` _actually_ supports a dynamic skip feature, so I guess I'll have to keep looking? Some related discussions: - https://internals.rust-lang.org/t/pre-rfc-skippable-tests/14611 - https://internals.rust-lang.org/t/skippable-tests/21260 --- .github/workflows/rust-ci.yml | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index e774489301..489c5bb277 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -74,7 +74,7 @@ jobs: steps: - uses: actions/checkout@v5 - uses: dtolnay/rust-toolchain@1.89 - - uses: taiki-e/install-action@v2 + - uses: taiki-e/install-action@0c5db7f7f897c03b771660e91d065338615679f4 # v2 with: tool: cargo-shear version: 1.5.1 @@ -178,12 +178,17 @@ jobs: find . -name Cargo.toml -mindepth 2 -maxdepth 2 -print0 \ | xargs -0 -n1 -I{} bash -c 'cd "$(dirname "{}")" && cargo check --profile ${{ matrix.profile }}' - - name: cargo test + - uses: taiki-e/install-action@0c5db7f7f897c03b771660e91d065338615679f4 # v2 + with: + tool: nextest + version: 0.9.103 + + - name: tests id: test - # `cargo test` takes too long for release builds to run them on every PR + # Tests take 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 }} + run: cargo nextest run --all-features --target ${{ matrix.target }} env: RUST_BACKTRACE: 1 From 2a76a08a9e048a8f2798e36b8cb2160ad59cbc2e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 9 Sep 2025 00:11:48 -0700 Subject: [PATCH 02/49] fix: include rollout_path in NewConversationResponse (#3352) Adding the `rollout_path` to the `NewConversationResponse` makes it so a client can perform subsequent operations on a `(ConversationId, PathBuf)` pair. #3353 will introduce support for `ArchiveConversation`. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/3352). * #3353 * __->__ #3352 --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/codex.rs | 2 ++ codex-rs/core/src/rollout/recorder.rs | 14 ++++++++--- .../src/event_processor_with_human_output.rs | 1 + .../mcp-server/src/codex_message_processor.rs | 1 + codex-rs/mcp-server/src/outgoing_message.rs | 6 +++++ .../suite/codex_message_processor_flow.rs | 1 + .../tests/suite/create_conversation.rs | 1 + codex-rs/protocol/Cargo.toml | 1 + codex-rs/protocol/src/mcp_protocol.rs | 1 + codex-rs/protocol/src/protocol.rs | 24 +++++++++++++++---- codex-rs/tui/src/chatwidget/tests.rs | 4 +++- codex-rs/tui/src/history_cell.rs | 1 + .../tui/tests/fixtures/binary-size-log.jsonl | 4 ++-- 14 files changed, 51 insertions(+), 11 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index e76ea6bb89..cda2e81d17 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -830,6 +830,7 @@ dependencies = [ "strum 0.27.2", "strum_macros 0.27.2", "sys-locale", + "tempfile", "tracing", "ts-rs", "uuid", diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 42960b27aa..cc8768d79b 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -416,6 +416,7 @@ impl Session { error!("failed to initialize rollout recorder: {e:#}"); anyhow::anyhow!("failed to initialize rollout recorder: {e:#}") })?; + let rollout_path = rollout_recorder.rollout_path.clone(); // Create the mutable state for the Session. let state = State { history: ConversationHistory::new(), @@ -509,6 +510,7 @@ impl Session { history_log_id, history_entry_count, initial_messages, + rollout_path, }), }) .chain(post_session_configured_error_events.into_iter()); diff --git a/codex-rs/core/src/rollout/recorder.rs b/codex-rs/core/src/rollout/recorder.rs index 907583353c..0e0af8990b 100644 --- a/codex-rs/core/src/rollout/recorder.rs +++ b/codex-rs/core/src/rollout/recorder.rs @@ -72,6 +72,7 @@ pub struct SavedSession { #[derive(Clone)] pub struct RolloutRecorder { tx: Sender, + pub(crate) rollout_path: PathBuf, } #[derive(Clone)] @@ -119,13 +120,14 @@ impl RolloutRecorder { /// cannot be created or the rollout file cannot be opened we return the /// error so the caller can decide whether to disable persistence. pub async fn new(config: &Config, params: RolloutRecorderParams) -> std::io::Result { - let (file, meta) = match params { + let (file, rollout_path, meta) = match params { RolloutRecorderParams::Create { conversation_id, instructions, } => { let LogFileInfo { file, + path, conversation_id: session_id, timestamp, } = create_log_file(config, conversation_id)?; @@ -140,6 +142,7 @@ impl RolloutRecorder { ( tokio::fs::File::from_std(file), + path, Some(SessionMeta { timestamp, id: session_id, @@ -150,8 +153,9 @@ impl RolloutRecorder { RolloutRecorderParams::Resume { path } => ( tokio::fs::OpenOptions::new() .append(true) - .open(path) + .open(&path) .await?, + path, None, ), }; @@ -169,7 +173,7 @@ impl RolloutRecorder { // driver instead of blocking the runtime. tokio::task::spawn(rollout_writer(file, rx, meta, cwd)); - Ok(Self { tx }) + Ok(Self { tx, rollout_path }) } pub(crate) async fn record_items(&self, items: &[ResponseItem]) -> std::io::Result<()> { @@ -289,6 +293,9 @@ struct LogFileInfo { /// Opened file handle to the rollout file. file: File, + /// Full path to the rollout file. + path: PathBuf, + /// Session ID (also embedded in filename). conversation_id: ConversationId, @@ -328,6 +335,7 @@ fn create_log_file( Ok(LogFileInfo { file, + path, conversation_id, timestamp, }) diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 7c5e6b147c..ae4708976f 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -523,6 +523,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { history_log_id: _, history_entry_count: _, initial_messages: _, + rollout_path: _, } = session_configured_event; ts_println!( diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index e5f44b5687..2169a8f27f 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -528,6 +528,7 @@ impl CodexMessageProcessor { let response = NewConversationResponse { conversation_id, model: session_configured.model, + rollout_path: session_configured.rollout_path, }; self.outgoing.send_response(request_id, response).await; } diff --git a/codex-rs/mcp-server/src/outgoing_message.rs b/codex-rs/mcp-server/src/outgoing_message.rs index 537d29db62..5ce2e99423 100644 --- a/codex-rs/mcp-server/src/outgoing_message.rs +++ b/codex-rs/mcp-server/src/outgoing_message.rs @@ -262,6 +262,7 @@ mod tests { use codex_protocol::mcp_protocol::LoginChatGptCompleteNotification; use pretty_assertions::assert_eq; use serde_json::json; + use tempfile::NamedTempFile; use uuid::Uuid; use super::*; @@ -272,6 +273,7 @@ mod tests { let outgoing_message_sender = OutgoingMessageSender::new(outgoing_tx); let conversation_id = ConversationId::new(); + let rollout_file = NamedTempFile::new().unwrap(); let event = Event { id: "1".to_string(), msg: EventMsg::SessionConfigured(SessionConfiguredEvent { @@ -280,6 +282,7 @@ mod tests { history_log_id: 1, history_entry_count: 1000, initial_messages: None, + rollout_path: rollout_file.path().to_path_buf(), }), }; @@ -305,12 +308,14 @@ mod tests { let outgoing_message_sender = OutgoingMessageSender::new(outgoing_tx); let conversation_id = ConversationId::new(); + let rollout_file = NamedTempFile::new().unwrap(); let session_configured_event = SessionConfiguredEvent { session_id: conversation_id, model: "gpt-4o".to_string(), history_log_id: 1, history_entry_count: 1000, initial_messages: None, + rollout_path: rollout_file.path().to_path_buf(), }; let event = Event { id: "1".to_string(), @@ -340,6 +345,7 @@ mod tests { "history_log_id": session_configured_event.history_log_id, "history_entry_count": session_configured_event.history_entry_count, "type": "session_configured", + "rollout_path": rollout_file.path().to_path_buf(), } }); assert_eq!(params.unwrap(), expected_params); diff --git a/codex-rs/mcp-server/tests/suite/codex_message_processor_flow.rs b/codex-rs/mcp-server/tests/suite/codex_message_processor_flow.rs index 092b135291..7737c49af3 100644 --- a/codex-rs/mcp-server/tests/suite/codex_message_processor_flow.rs +++ b/codex-rs/mcp-server/tests/suite/codex_message_processor_flow.rs @@ -90,6 +90,7 @@ async fn test_codex_jsonrpc_conversation_flow() { let NewConversationResponse { conversation_id, model, + rollout_path: _, } = new_conv_resp; assert_eq!(model, "mock-model"); diff --git a/codex-rs/mcp-server/tests/suite/create_conversation.rs b/codex-rs/mcp-server/tests/suite/create_conversation.rs index 81cc1f3ee7..5071868c85 100644 --- a/codex-rs/mcp-server/tests/suite/create_conversation.rs +++ b/codex-rs/mcp-server/tests/suite/create_conversation.rs @@ -59,6 +59,7 @@ async fn test_conversation_create_and_send_message_ok() { let NewConversationResponse { conversation_id, model, + rollout_path: _, } = to_response::(new_conv_resp) .expect("deserialize newConversation response"); assert_eq!(model, "o3"); diff --git a/codex-rs/protocol/Cargo.toml b/codex-rs/protocol/Cargo.toml index d77da7fc19..f88297a07b 100644 --- a/codex-rs/protocol/Cargo.toml +++ b/codex-rs/protocol/Cargo.toml @@ -28,6 +28,7 @@ uuid = { version = "1", features = ["serde", "v4"] } [dev-dependencies] pretty_assertions = "1.4.1" +tempfile = "3" [package.metadata.cargo-shear] # Required because the not imported as strum_macros in non-nightly builds. diff --git a/codex-rs/protocol/src/mcp_protocol.rs b/codex-rs/protocol/src/mcp_protocol.rs index 2af1a95166..70e334036f 100644 --- a/codex-rs/protocol/src/mcp_protocol.rs +++ b/codex-rs/protocol/src/mcp_protocol.rs @@ -203,6 +203,7 @@ pub struct NewConversationParams { pub struct NewConversationResponse { pub conversation_id: ConversationId, pub model: String, + pub rollout_path: PathBuf, } #[derive(Serialize, Deserialize, Debug, Clone, TS)] diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index eeaa72cd93..2f1c46364a 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -958,6 +958,8 @@ pub struct SessionConfiguredEvent { /// When present, UIs can use these to seed the history. #[serde(skip_serializing_if = "Option::is_none")] pub initial_messages: Option>, + + pub rollout_path: PathBuf, } /// User's decision in response to an ExecApprovalRequest. @@ -1020,12 +1022,15 @@ pub enum TurnAbortReason { #[cfg(test)] mod tests { use super::*; + use serde_json::json; + use tempfile::NamedTempFile; /// Serialize Event to verify that its JSON representation has the expected /// amount of nesting. #[test] fn serialize_event() { let conversation_id = ConversationId(uuid::uuid!("67e55044-10b1-426f-9247-bb680e5fe0c8")); + let rollout_file = NamedTempFile::new().unwrap(); let event = Event { id: "1234".to_string(), msg: EventMsg::SessionConfigured(SessionConfiguredEvent { @@ -1034,13 +1039,22 @@ mod tests { history_log_id: 0, history_entry_count: 0, initial_messages: None, + rollout_path: rollout_file.path().to_path_buf(), }), }; - let serialized = serde_json::to_string(&event).unwrap(); - assert_eq!( - serialized, - r#"{"id":"1234","msg":{"type":"session_configured","session_id":"67e55044-10b1-426f-9247-bb680e5fe0c8","model":"codex-mini-latest","history_log_id":0,"history_entry_count":0}}"# - ); + + let expected = json!({ + "id": "1234", + "msg": { + "type": "session_configured", + "session_id": "67e55044-10b1-426f-9247-bb680e5fe0c8", + "model": "codex-mini-latest", + "history_log_id": 0, + "history_entry_count": 0, + "rollout_path": format!("{}", rollout_file.path().display()), + } + }); + assert_eq!(expected, serde_json::to_value(&event).unwrap()); } #[test] diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 3b61203399..113864dba7 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -35,6 +35,7 @@ use std::fs::File; use std::io::BufRead; use std::io::BufReader; use std::path::PathBuf; +use tempfile::NamedTempFile; use tokio::sync::mpsc::unbounded_channel; fn test_config() -> Config { @@ -133,7 +134,7 @@ fn resumed_initial_messages_render_history() { let (mut chat, mut rx, _ops) = make_chatwidget_manual(); let conversation_id = ConversationId::new(); - + let rollout_file = NamedTempFile::new().unwrap(); let configured = codex_core::protocol::SessionConfiguredEvent { session_id: conversation_id, model: "test-model".to_string(), @@ -148,6 +149,7 @@ fn resumed_initial_messages_render_history() { message: "assistant reply".to_string(), }), ]), + rollout_path: rollout_file.path().to_path_buf(), }; chat.handle_codex_event(Event { diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 92bb076225..a7be1d6560 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -605,6 +605,7 @@ pub(crate) fn new_session_info( history_log_id: _, history_entry_count: _, initial_messages: _, + rollout_path: _, } = event; if is_first_event { let cwd_str = match relativize_to_home(&config.cwd) { diff --git a/codex-rs/tui/tests/fixtures/binary-size-log.jsonl b/codex-rs/tui/tests/fixtures/binary-size-log.jsonl index a02dd02f9a..98cac7c0ae 100644 --- a/codex-rs/tui/tests/fixtures/binary-size-log.jsonl +++ b/codex-rs/tui/tests/fixtures/binary-size-log.jsonl @@ -34,7 +34,7 @@ {"ts":"2025-08-09T15:51:04.829Z","dir":"to_tui","kind":"app_event","variant":"RequestRedraw"} {"ts":"2025-08-09T15:51:04.829Z","dir":"to_tui","kind":"log_line","line":"[INFO codex_core::codex] resume_path: None"} {"ts":"2025-08-09T15:51:04.830Z","dir":"to_tui","kind":"app_event","variant":"Redraw"} -{"ts":"2025-08-09T15:51:04.856Z","dir":"to_tui","kind":"codex_event","payload":{"id":"0","msg":{"type":"session_configured","session_id":"d126e3d0-80ed-480a-be8c-09d97ff602cf","model":"gpt-5","history_log_id":2532619,"history_entry_count":339}}} +{"ts":"2025-08-09T15:51:04.856Z","dir":"to_tui","kind":"codex_event","payload":{"id":"0","msg":{"type":"session_configured","session_id":"d126e3d0-80ed-480a-be8c-09d97ff602cf","model":"gpt-5","history_log_id":2532619,"history_entry_count":339,"rollout_path":"/tmp/codex-test-rollout.jsonl"}}} {"ts":"2025-08-09T15:51:04.856Z","dir":"to_tui","kind":"insert_history","lines":9} {"ts":"2025-08-09T15:51:04.857Z","dir":"to_tui","kind":"app_event","variant":"RequestRedraw"} {"ts":"2025-08-09T15:51:04.857Z","dir":"to_tui","kind":"app_event","variant":"RequestRedraw"} @@ -16447,7 +16447,7 @@ {"ts":"2025-08-09T16:06:58.083Z","dir":"to_tui","kind":"app_event","variant":"RequestRedraw"} {"ts":"2025-08-09T16:06:58.085Z","dir":"to_tui","kind":"app_event","variant":"Redraw"} {"ts":"2025-08-09T16:06:58.085Z","dir":"to_tui","kind":"log_line","line":"[INFO codex_core::codex] resume_path: None"} -{"ts":"2025-08-09T16:06:58.136Z","dir":"to_tui","kind":"codex_event","payload":{"id":"0","msg":{"type":"session_configured","session_id":"c7df96da-daec-4fe9-aed9-3cd19b7a6192","model":"gpt-5","history_log_id":2532619,"history_entry_count":342}}} +{"ts":"2025-08-09T16:06:58.136Z","dir":"to_tui","kind":"codex_event","payload":{"id":"0","msg":{"type":"session_configured","session_id":"c7df96da-daec-4fe9-aed9-3cd19b7a6192","model":"gpt-5","history_log_id":2532619,"history_entry_count":342,"rollout_path":"/tmp/codex-test-rollout.jsonl"}}} {"ts":"2025-08-09T16:06:58.136Z","dir":"to_tui","kind":"insert_history","lines":9} {"ts":"2025-08-09T16:06:58.136Z","dir":"to_tui","kind":"app_event","variant":"RequestRedraw"} {"ts":"2025-08-09T16:06:58.136Z","dir":"to_tui","kind":"app_event","variant":"RequestRedraw"} From ace14e8d36fa3e071a765737689d9853b046020f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 9 Sep 2025 08:39:00 -0700 Subject: [PATCH 03/49] feat: add ArchiveConversation to ClientRequest (#3353) Adds support for `ArchiveConversation` in the JSON-RPC server that takes a `(ConversationId, PathBuf)` pair and: - verifies the `ConversationId` corresponds to the rollout id at the `PathBuf` - if so, invokes `ConversationManager.remove_conversation(ConversationId)` - if the `CodexConversation` was in memory, send `Shutdown` and wait for `ShutdownComplete` with a timeout - moves the `.jsonl` file to `$CODEX_HOME/archived_sessions` --------- Co-authored-by: Gabriel Peal --- codex-rs/core/src/conversation_manager.rs | 11 +- codex-rs/core/src/lib.rs | 2 + codex-rs/core/src/rollout/mod.rs | 3 +- .../mcp-server/src/codex_message_processor.rs | 144 ++++++++++++++++++ .../mcp-server/tests/common/mcp_process.rs | 10 ++ .../tests/suite/archive_conversation.rs | 105 +++++++++++++ codex-rs/mcp-server/tests/suite/mod.rs | 1 + codex-rs/protocol-ts/src/lib.rs | 1 + codex-rs/protocol/src/mcp_protocol.rs | 17 +++ 9 files changed, 291 insertions(+), 3 deletions(-) create mode 100644 codex-rs/mcp-server/tests/suite/archive_conversation.rs diff --git a/codex-rs/core/src/conversation_manager.rs b/codex-rs/core/src/conversation_manager.rs index 6fac42d59f..87b60e0155 100644 --- a/codex-rs/core/src/conversation_manager.rs +++ b/codex-rs/core/src/conversation_manager.rs @@ -145,8 +145,15 @@ impl ConversationManager { self.finalize_spawn(codex, conversation_id).await } - pub async fn remove_conversation(&self, conversation_id: ConversationId) { - self.conversations.write().await.remove(&conversation_id); + /// Removes the conversation from the manager's internal map, though the + /// conversation is stored as `Arc`, it is possible that + /// other references to it exist elsewhere. Returns the conversation if the + /// conversation was found and removed. + pub async fn remove_conversation( + &self, + conversation_id: &ConversationId, + ) -> Option> { + self.conversations.write().await.remove(conversation_id) } /// Fork an existing conversation by dropping the last `drop_last_messages` diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 3b47830c50..b8cd7bc55a 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -61,7 +61,9 @@ pub mod spawn; pub mod terminal; mod tool_apply_patch; pub mod turn_diff_tracker; +pub use rollout::ARCHIVED_SESSIONS_SUBDIR; pub use rollout::RolloutRecorder; +pub use rollout::SESSIONS_SUBDIR; pub use rollout::SessionMeta; pub use rollout::list::ConversationItem; pub use rollout::list::ConversationsPage; diff --git a/codex-rs/core/src/rollout/mod.rs b/codex-rs/core/src/rollout/mod.rs index 4883517c1f..a78d760959 100644 --- a/codex-rs/core/src/rollout/mod.rs +++ b/codex-rs/core/src/rollout/mod.rs @@ -1,6 +1,7 @@ //! Rollout module: persistence and discovery of session rollout files. -pub(crate) const SESSIONS_SUBDIR: &str = "sessions"; +pub const SESSIONS_SUBDIR: &str = "sessions"; +pub const ARCHIVED_SESSIONS_SUBDIR: &str = "archived_sessions"; pub mod list; pub(crate) mod policy; diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 2169a8f27f..e6133e174b 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -35,6 +35,8 @@ use codex_protocol::mcp_protocol::AddConversationListenerParams; use codex_protocol::mcp_protocol::AddConversationSubscriptionResponse; use codex_protocol::mcp_protocol::ApplyPatchApprovalParams; use codex_protocol::mcp_protocol::ApplyPatchApprovalResponse; +use codex_protocol::mcp_protocol::ArchiveConversationParams; +use codex_protocol::mcp_protocol::ArchiveConversationResponse; use codex_protocol::mcp_protocol::AuthMode; use codex_protocol::mcp_protocol::AuthStatusChangeNotification; use codex_protocol::mcp_protocol::ClientRequest; @@ -73,12 +75,16 @@ use codex_protocol::protocol::USER_MESSAGE_BEGIN; use mcp_types::JSONRPCErrorError; use mcp_types::RequestId; use std::collections::HashMap; +use std::ffi::OsStr; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; +use tokio::select; use tokio::sync::Mutex; use tokio::sync::oneshot; use tracing::error; +use tracing::info; +use tracing::warn; use uuid::Uuid; // Duration before a ChatGPT login attempt is abandoned. @@ -142,6 +148,9 @@ impl CodexMessageProcessor { ClientRequest::ResumeConversation { request_id, params } => { self.handle_resume_conversation(request_id, params).await; } + ClientRequest::ArchiveConversation { request_id, params } => { + self.archive_conversation(request_id, params).await; + } ClientRequest::SendUserMessage { request_id, params } => { self.send_user_message(request_id, params).await; } @@ -670,6 +679,141 @@ impl CodexMessageProcessor { } } + async fn archive_conversation(&self, request_id: RequestId, params: ArchiveConversationParams) { + let ArchiveConversationParams { + conversation_id, + rollout_path, + } = params; + + // Verify that the rollout path is in the sessions directory or else + // a malicious client could specify an arbitrary path. + let rollout_folder = self.config.codex_home.join(codex_core::SESSIONS_SUBDIR); + let canonical_rollout_path = tokio::fs::canonicalize(&rollout_path).await; + let canonical_rollout_path = if let Ok(path) = canonical_rollout_path + && path.starts_with(&rollout_folder) + { + path + } else { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!( + "rollout path `{}` must be in sessions directory", + rollout_path.display() + ), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + }; + + let required_suffix = format!("{}.jsonl", conversation_id.0); + let Some(file_name) = canonical_rollout_path.file_name().map(OsStr::to_owned) else { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!( + "rollout path `{}` missing file name", + rollout_path.display() + ), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + }; + + if !file_name + .to_string_lossy() + .ends_with(required_suffix.as_str()) + { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!( + "rollout path `{}` does not match conversation id {conversation_id}", + rollout_path.display() + ), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + } + + let removed_conversation = self + .conversation_manager + .remove_conversation(&conversation_id) + .await; + if let Some(conversation) = removed_conversation { + info!("conversation {conversation_id} was active; shutting down"); + let conversation_clone = conversation.clone(); + let notify = Arc::new(tokio::sync::Notify::new()); + let notify_clone = notify.clone(); + + // Establish the listener for ShutdownComplete before submitting + // Shutdown so it is not missed. + let is_shutdown = tokio::spawn(async move { + loop { + select! { + _ = notify_clone.notified() => { + break; + } + event = conversation_clone.next_event() => { + if let Ok(event) = event && matches!(event.msg, EventMsg::ShutdownComplete) { + break; + } + } + } + } + }); + + // Request shutdown. + match conversation.submit(Op::Shutdown).await { + Ok(_) => { + // Successfully submitted Shutdown; wait before proceeding. + select! { + _ = is_shutdown => { + // Normal shutdown: proceed with archive. + } + _ = tokio::time::sleep(Duration::from_secs(10)) => { + warn!("conversation {conversation_id} shutdown timed out; proceeding with archive"); + notify.notify_one(); + } + } + } + Err(err) => { + error!("failed to submit Shutdown to conversation {conversation_id}: {err}"); + notify.notify_one(); + // Perhaps we lost a shutdown race, so let's continue to + // clean up the .jsonl file. + } + } + } + + // Move the .jsonl file to the archived sessions subdir. + let result: std::io::Result<()> = async { + let archive_folder = self + .config + .codex_home + .join(codex_core::ARCHIVED_SESSIONS_SUBDIR); + tokio::fs::create_dir_all(&archive_folder).await?; + tokio::fs::rename(&canonical_rollout_path, &archive_folder.join(&file_name)).await?; + Ok(()) + } + .await; + + match result { + Ok(()) => { + let response = ArchiveConversationResponse {}; + self.outgoing.send_response(request_id, response).await; + } + Err(err) => { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to archive 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, diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index cebc332a3c..64f2cc3852 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -13,6 +13,7 @@ use anyhow::Context; use assert_cmd::prelude::*; use codex_mcp_server::CodexToolCallParam; use codex_protocol::mcp_protocol::AddConversationListenerParams; +use codex_protocol::mcp_protocol::ArchiveConversationParams; use codex_protocol::mcp_protocol::CancelLoginChatGptParams; use codex_protocol::mcp_protocol::GetAuthStatusParams; use codex_protocol::mcp_protocol::InterruptConversationParams; @@ -186,6 +187,15 @@ impl McpProcess { self.send_request("newConversation", params).await } + /// Send an `archiveConversation` JSON-RPC request. + pub async fn send_archive_conversation_request( + &mut self, + params: ArchiveConversationParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("archiveConversation", params).await + } + /// Send an `addConversationListener` JSON-RPC request. pub async fn send_add_conversation_listener_request( &mut self, diff --git a/codex-rs/mcp-server/tests/suite/archive_conversation.rs b/codex-rs/mcp-server/tests/suite/archive_conversation.rs new file mode 100644 index 0000000000..e54a99896c --- /dev/null +++ b/codex-rs/mcp-server/tests/suite/archive_conversation.rs @@ -0,0 +1,105 @@ +use std::path::Path; + +use codex_core::ARCHIVED_SESSIONS_SUBDIR; +use codex_protocol::mcp_protocol::ArchiveConversationParams; +use codex_protocol::mcp_protocol::ArchiveConversationResponse; +use codex_protocol::mcp_protocol::NewConversationParams; +use codex_protocol::mcp_protocol::NewConversationResponse; +use mcp_test_support::McpProcess; +use mcp_test_support::to_response; +use mcp_types::JSONRPCResponse; +use mcp_types::RequestId; +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 = 2)] +async fn archive_conversation_moves_rollout_into_archived_directory() { + let codex_home = TempDir::new().expect("create temp dir"); + create_config_toml(codex_home.path()).expect("write config.toml"); + + let mut mcp = McpProcess::new(codex_home.path()) + .await + .expect("spawn mcp process"); + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()) + .await + .expect("initialize timeout") + .expect("initialize request"); + + let new_request_id = mcp + .send_new_conversation_request(NewConversationParams { + model: Some("mock-model".to_string()), + ..Default::default() + }) + .await + .expect("send newConversation"); + let new_response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(new_request_id)), + ) + .await + .expect("newConversation timeout") + .expect("newConversation response"); + + let NewConversationResponse { + conversation_id, + rollout_path, + .. + } = to_response::(new_response) + .expect("deserialize newConversation response"); + + assert!( + rollout_path.exists(), + "expected rollout path {} to exist", + rollout_path.display() + ); + + let archive_request_id = mcp + .send_archive_conversation_request(ArchiveConversationParams { + conversation_id, + rollout_path: rollout_path.clone(), + }) + .await + .expect("send archiveConversation"); + let archive_response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(archive_request_id)), + ) + .await + .expect("archiveConversation timeout") + .expect("archiveConversation response"); + + let _: ArchiveConversationResponse = + to_response::(archive_response) + .expect("deserialize archiveConversation response"); + + let archived_directory = codex_home.path().join(ARCHIVED_SESSIONS_SUBDIR); + let archived_rollout_path = + archived_directory.join(rollout_path.file_name().unwrap_or_else(|| { + panic!("rollout path {} missing file name", rollout_path.display()) + })); + + assert!( + !rollout_path.exists(), + "expected rollout path {} to be moved", + rollout_path.display() + ); + assert!( + archived_rollout_path.exists(), + "expected archived rollout path {} to exist", + archived_rollout_path.display() + ); +} + +fn create_config_toml(codex_home: &Path) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write(config_toml, config_contents()) +} + +fn config_contents() -> &'static str { + r#"model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" +"# +} diff --git a/codex-rs/mcp-server/tests/suite/mod.rs b/codex-rs/mcp-server/tests/suite/mod.rs index 4a9220da91..7f89cc3301 100644 --- a/codex-rs/mcp-server/tests/suite/mod.rs +++ b/codex-rs/mcp-server/tests/suite/mod.rs @@ -1,4 +1,5 @@ // Aggregates all former standalone integration tests as modules. +mod archive_conversation; mod auth; mod codex_message_processor_flow; mod codex_tool; diff --git a/codex-rs/protocol-ts/src/lib.rs b/codex-rs/protocol-ts/src/lib.rs index d564514747..5d8d4b17f7 100644 --- a/codex-rs/protocol-ts/src/lib.rs +++ b/codex-rs/protocol-ts/src/lib.rs @@ -39,6 +39,7 @@ pub fn generate_ts(out_dir: &Path, prettier: Option<&Path>) -> Result<()> { codex_protocol::mcp_protocol::ServerNotification::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ListConversationsResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ResumeConversationResponse::export_all_to(out_dir)?; + codex_protocol::mcp_protocol::ArchiveConversationResponse::export_all_to(out_dir)?; generate_index_ts(out_dir)?; diff --git a/codex-rs/protocol/src/mcp_protocol.rs b/codex-rs/protocol/src/mcp_protocol.rs index 70e334036f..00391c7d01 100644 --- a/codex-rs/protocol/src/mcp_protocol.rs +++ b/codex-rs/protocol/src/mcp_protocol.rs @@ -91,6 +91,11 @@ pub enum ClientRequest { request_id: RequestId, params: ResumeConversationParams, }, + ArchiveConversation { + #[serde(rename = "id")] + request_id: RequestId, + params: ArchiveConversationParams, + }, SendUserMessage { #[serde(rename = "id")] request_id: RequestId, @@ -263,6 +268,18 @@ pub struct AddConversationSubscriptionResponse { pub subscription_id: Uuid, } +/// The [`ConversationId`] must match the `rollout_path`. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] +#[serde(rename_all = "camelCase")] +pub struct ArchiveConversationParams { + pub conversation_id: ConversationId, + pub rollout_path: PathBuf, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] +#[serde(rename_all = "camelCase")] +pub struct ArchiveConversationResponse {} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] #[serde(rename_all = "camelCase")] pub struct RemoveConversationSubscriptionResponse {} From ac8a3155d64bfb502249be5666dd6c87a190510d Mon Sep 17 00:00:00 2001 From: Wang Date: Wed, 10 Sep 2025 01:37:08 +0800 Subject: [PATCH 04/49] feat(core): re-export InitialHistory from conversation_manager (#3270) This commit adds a re-export for InitialHistory from the internal conversation_manager module in codex-core's lib.rs. The `RolloutRecorder::get_rollout_history` method (exposed via `pub use rollout::RolloutRecorder;`, already present in lib.rs) returns an `InitialHistory` type, which is defined in the private conversation_manager module. Without this re-export, consumers of the public RolloutRecorder API would not be able to directly use the return type, as they cannot access the private module. This would result in an inconvenient experience where the method's return value cannot be handled without additional, non-obvious imports. By adding `pub use conversation_manager::InitialHistory;`, we make InitialHistory available as `codex_core::InitialHistory`, improving API ergonomics for users of the rollout functionality while keeping the conversation_manager module internal. No functional changes are made; this is a pure re-export for better usability. Signed-off-by: M4n5ter --- codex-rs/core/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index b8cd7bc55a..352bd48277 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -43,6 +43,7 @@ pub use model_provider_info::create_oss_provider_with_base_url; mod conversation_manager; mod event_mapping; pub use conversation_manager::ConversationManager; +pub use conversation_manager::InitialHistory; pub use conversation_manager::NewConversation; // Re-export common auth types for workspace consumers pub use auth::AuthManager; From 58bb2048ac40dfee881a679cea55aa6e2e9c5902 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 9 Sep 2025 11:06:59 -0700 Subject: [PATCH 05/49] fix: LoginChatGptCompleteNotification does not need to be listed explicitly in protocol-ts (#3222) I verified that the output of `protocol-ts$ cargo run` is unchanged by removing this line.. Added a comment on `ServerNotification` with justification to make this clear. --- codex-rs/protocol-ts/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/codex-rs/protocol-ts/src/lib.rs b/codex-rs/protocol-ts/src/lib.rs index 5d8d4b17f7..08cb940784 100644 --- a/codex-rs/protocol-ts/src/lib.rs +++ b/codex-rs/protocol-ts/src/lib.rs @@ -21,6 +21,9 @@ pub fn generate_ts(out_dir: &Path, prettier: Option<&Path>) -> Result<()> { codex_protocol::mcp_protocol::ClientRequest::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ServerRequest::export_all_to(out_dir)?; codex_protocol::mcp_protocol::NewConversationResponse::export_all_to(out_dir)?; + codex_protocol::mcp_protocol::ListConversationsResponse::export_all_to(out_dir)?; + codex_protocol::mcp_protocol::ResumeConversationResponse::export_all_to(out_dir)?; + codex_protocol::mcp_protocol::ArchiveConversationResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::AddConversationSubscriptionResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::RemoveConversationSubscriptionResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::SendUserMessageResponse::export_all_to(out_dir)?; @@ -28,7 +31,6 @@ pub fn generate_ts(out_dir: &Path, prettier: Option<&Path>) -> Result<()> { codex_protocol::mcp_protocol::InterruptConversationResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::GitDiffToRemoteResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::LoginChatGptResponse::export_all_to(out_dir)?; - codex_protocol::mcp_protocol::LoginChatGptCompleteNotification::export_all_to(out_dir)?; codex_protocol::mcp_protocol::CancelLoginChatGptResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::LogoutChatGptResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::GetAuthStatusResponse::export_all_to(out_dir)?; @@ -36,10 +38,10 @@ pub fn generate_ts(out_dir: &Path, prettier: Option<&Path>) -> Result<()> { codex_protocol::mcp_protocol::ExecCommandApprovalResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::GetUserSavedConfigResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::GetUserAgentResponse::export_all_to(out_dir)?; + + // All notification types reachable from this enum will be generated by + // induction, so they do not need to be listed individually. codex_protocol::mcp_protocol::ServerNotification::export_all_to(out_dir)?; - codex_protocol::mcp_protocol::ListConversationsResponse::export_all_to(out_dir)?; - codex_protocol::mcp_protocol::ResumeConversationResponse::export_all_to(out_dir)?; - codex_protocol::mcp_protocol::ArchiveConversationResponse::export_all_to(out_dir)?; generate_index_ts(out_dir)?; From ee5ecae7c0915ac8f478703a1171eae62d0059e3 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Tue, 9 Sep 2025 12:27:50 -0700 Subject: [PATCH 06/49] tweak "failed to find expected lines" message in apply_patch (#3374) It was hard for me to read the expected lines as a `["one", "two", "three"]` array, maybe not so hard for the model but probably not having to un-escape in its head would help it out :) Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> --- codex-rs/apply-patch/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 18d3bd4d32..1fdd6928e9 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -726,9 +726,9 @@ fn compute_replacements( line_index = start_idx + pattern.len(); } else { return Err(ApplyPatchError::ComputeReplacements(format!( - "Failed to find expected lines {:?} in {}", - chunk.old_lines, - path.display() + "Failed to find expected lines in {}:\n{}", + path.display(), + chunk.old_lines.join("\n"), ))); } } From f656e192bf2d44a34949cd87287b217105f769c6 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 9 Sep 2025 13:17:14 -0700 Subject: [PATCH 07/49] No fail fast (#3387) Add --no-fail-fast to the new `nextest` --- .github/workflows/rust-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index 489c5bb277..bae33e30d9 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -188,7 +188,7 @@ jobs: # Tests take too long for release builds to run them on every PR. if: ${{ matrix.profile != 'release' }} continue-on-error: true - run: cargo nextest run --all-features --target ${{ matrix.target }} + run: cargo nextest run --all-features --no-fail-fast --target ${{ matrix.target }} env: RUST_BACKTRACE: 1 From 5eab4c7ab41b7d07a7f0f8e0a3fa4dd1af20762e Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Tue, 9 Sep 2025 14:23:23 -0700 Subject: [PATCH 08/49] Replace config.responses_originator_header_internal_override with CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR (#3388) The previous config approach had a few issues: 1. It is part of the config but not designed to be used externally 2. It had to be wired through many places (look at the +/- on this PR 3. It wasn't guaranteed to be set consistently everywhere because we don't have a super well defined way that configs stack. For example, the extension would configure during newConversation but anything that happened outside of that (like login) wouldn't get it. This env var approach is cleaner and also creates one less thing we have to deal with when coming up with a better holistic story around configs. One downside is that I removed the unit test testing for the override because I don't want to deal with setting the global env or spawning child processes and figuring out how to introspect their originator header. The new code is sufficiently simple and I tested it e2e that I feel as if this is still worth it. --- codex-rs/chatgpt/src/apply_command.rs | 2 +- codex-rs/chatgpt/src/chatgpt_client.rs | 4 +- codex-rs/chatgpt/src/chatgpt_token.rs | 7 +- codex-rs/cli/src/login.rs | 17 +--- codex-rs/cli/src/proto.rs | 1 - codex-rs/core/src/auth.rs | 38 +++------ codex-rs/core/src/client.rs | 2 +- codex-rs/core/src/config.rs | 17 ---- codex-rs/core/src/default_client.rs | 62 +++++++++----- codex-rs/core/tests/suite/client.rs | 80 +++---------------- codex-rs/exec/src/lib.rs | 1 - codex-rs/login/src/server.rs | 17 +--- .../login/tests/suite/login_server_e2e.rs | 4 - .../mcp-server/src/codex_message_processor.rs | 8 +- codex-rs/mcp-server/src/message_processor.rs | 7 +- codex-rs/mcp-server/tests/suite/user_agent.rs | 3 +- codex-rs/tui/src/lib.rs | 12 +-- codex-rs/tui/src/onboarding/auth.rs | 8 +- .../tui/src/onboarding/onboarding_screen.rs | 1 - codex-rs/tui/src/updates.rs | 7 +- 20 files changed, 92 insertions(+), 206 deletions(-) diff --git a/codex-rs/chatgpt/src/apply_command.rs b/codex-rs/chatgpt/src/apply_command.rs index 3dbfeb1618..52ab205a0c 100644 --- a/codex-rs/chatgpt/src/apply_command.rs +++ b/codex-rs/chatgpt/src/apply_command.rs @@ -31,7 +31,7 @@ pub async fn run_apply_command( ConfigOverrides::default(), )?; - init_chatgpt_token_from_auth(&config.codex_home, &config.responses_originator_header).await?; + init_chatgpt_token_from_auth(&config.codex_home).await?; let task_response = get_task(&config, apply_cli.task_id).await?; apply_diff_from_task(task_response, cwd).await diff --git a/codex-rs/chatgpt/src/chatgpt_client.rs b/codex-rs/chatgpt/src/chatgpt_client.rs index 8991003f2b..a4545f075e 100644 --- a/codex-rs/chatgpt/src/chatgpt_client.rs +++ b/codex-rs/chatgpt/src/chatgpt_client.rs @@ -13,10 +13,10 @@ pub(crate) async fn chatgpt_get_request( path: String, ) -> anyhow::Result { let chatgpt_base_url = &config.chatgpt_base_url; - init_chatgpt_token_from_auth(&config.codex_home, &config.responses_originator_header).await?; + init_chatgpt_token_from_auth(&config.codex_home).await?; // Make direct HTTP request to ChatGPT backend API with the token - let client = create_client(&config.responses_originator_header); + let client = create_client(); let url = format!("{chatgpt_base_url}{path}"); let token = diff --git a/codex-rs/chatgpt/src/chatgpt_token.rs b/codex-rs/chatgpt/src/chatgpt_token.rs index c51be9eccf..15192ce3fb 100644 --- a/codex-rs/chatgpt/src/chatgpt_token.rs +++ b/codex-rs/chatgpt/src/chatgpt_token.rs @@ -19,11 +19,8 @@ pub fn set_chatgpt_token_data(value: TokenData) { } /// Initialize the ChatGPT token from auth.json file -pub async fn init_chatgpt_token_from_auth( - codex_home: &Path, - originator: &str, -) -> std::io::Result<()> { - let auth = CodexAuth::from_codex_home(codex_home, AuthMode::ChatGPT, originator)?; +pub async fn init_chatgpt_token_from_auth(codex_home: &Path) -> std::io::Result<()> { + let auth = CodexAuth::from_codex_home(codex_home, AuthMode::ChatGPT)?; if let Some(auth) = auth { let token_data = auth.get_token_data().await?; set_chatgpt_token_data(token_data); diff --git a/codex-rs/cli/src/login.rs b/codex-rs/cli/src/login.rs index cff03e7c16..f03509618d 100644 --- a/codex-rs/cli/src/login.rs +++ b/codex-rs/cli/src/login.rs @@ -12,8 +12,8 @@ use codex_protocol::mcp_protocol::AuthMode; use std::env; use std::path::PathBuf; -pub async fn login_with_chatgpt(codex_home: PathBuf, originator: String) -> std::io::Result<()> { - let opts = ServerOptions::new(codex_home, CLIENT_ID.to_string(), originator); +pub async fn login_with_chatgpt(codex_home: PathBuf) -> std::io::Result<()> { + let opts = ServerOptions::new(codex_home, CLIENT_ID.to_string()); let server = run_login_server(opts)?; eprintln!( @@ -27,12 +27,7 @@ pub async fn login_with_chatgpt(codex_home: PathBuf, originator: String) -> std: pub async fn run_login_with_chatgpt(cli_config_overrides: CliConfigOverrides) -> ! { let config = load_config_or_exit(cli_config_overrides); - match login_with_chatgpt( - config.codex_home, - config.responses_originator_header.clone(), - ) - .await - { + match login_with_chatgpt(config.codex_home).await { Ok(_) => { eprintln!("Successfully logged in"); std::process::exit(0); @@ -65,11 +60,7 @@ pub async fn run_login_with_api_key( pub async fn run_login_status(cli_config_overrides: CliConfigOverrides) -> ! { let config = load_config_or_exit(cli_config_overrides); - match CodexAuth::from_codex_home( - &config.codex_home, - config.preferred_auth_method, - &config.responses_originator_header, - ) { + match CodexAuth::from_codex_home(&config.codex_home, config.preferred_auth_method) { Ok(Some(auth)) => match auth.mode { AuthMode::ApiKey => match auth.get_token().await { Ok(api_key) => { diff --git a/codex-rs/cli/src/proto.rs b/codex-rs/cli/src/proto.rs index 8fd8e30df2..9b8cb92ee2 100644 --- a/codex-rs/cli/src/proto.rs +++ b/codex-rs/cli/src/proto.rs @@ -40,7 +40,6 @@ pub async fn run_main(opts: ProtoCli) -> anyhow::Result<()> { let conversation_manager = ConversationManager::new(AuthManager::shared( config.codex_home.clone(), config.preferred_auth_method, - config.responses_originator_header.clone(), )); let NewConversation { conversation_id: _, diff --git a/codex-rs/core/src/auth.rs b/codex-rs/core/src/auth.rs index 776a111cf2..1256584ce7 100644 --- a/codex-rs/core/src/auth.rs +++ b/codex-rs/core/src/auth.rs @@ -75,9 +75,8 @@ impl CodexAuth { pub fn from_codex_home( codex_home: &Path, preferred_auth_method: AuthMode, - originator: &str, ) -> std::io::Result> { - load_auth(codex_home, true, preferred_auth_method, originator) + load_auth(codex_home, true, preferred_auth_method) } pub async fn get_token_data(&self) -> Result { @@ -173,7 +172,7 @@ impl CodexAuth { mode: AuthMode::ChatGPT, auth_file: PathBuf::new(), auth_dot_json, - client: crate::default_client::create_client("codex_cli_rs"), + client: crate::default_client::create_client(), } } @@ -188,10 +187,7 @@ impl CodexAuth { } pub fn from_api_key(api_key: &str) -> Self { - Self::from_api_key_with_client( - api_key, - crate::default_client::create_client(crate::default_client::DEFAULT_ORIGINATOR), - ) + Self::from_api_key_with_client(api_key, crate::default_client::create_client()) } } @@ -232,13 +228,12 @@ fn load_auth( codex_home: &Path, include_env_var: bool, preferred_auth_method: AuthMode, - originator: &str, ) -> std::io::Result> { // First, check to see if there is a valid auth.json file. If not, we fall // back to AuthMode::ApiKey using the OPENAI_API_KEY environment variable // (if it is set). let auth_file = get_auth_file(codex_home); - let client = crate::default_client::create_client(originator); + let client = crate::default_client::create_client(); let auth_dot_json = match try_read_auth_json(&auth_file) { Ok(auth) => auth, // If auth.json does not exist, try to read the OPENAI_API_KEY from the @@ -473,7 +468,7 @@ mod tests { auth_dot_json, auth_file: _, .. - } = super::load_auth(codex_home.path(), false, AuthMode::ChatGPT, "codex_cli_rs") + } = super::load_auth(codex_home.path(), false, AuthMode::ChatGPT) .unwrap() .unwrap(); assert_eq!(None, api_key); @@ -525,7 +520,7 @@ mod tests { auth_dot_json, auth_file: _, .. - } = super::load_auth(codex_home.path(), false, AuthMode::ChatGPT, "codex_cli_rs") + } = super::load_auth(codex_home.path(), false, AuthMode::ChatGPT) .unwrap() .unwrap(); assert_eq!(None, api_key); @@ -576,7 +571,7 @@ mod tests { auth_dot_json, auth_file: _, .. - } = super::load_auth(codex_home.path(), false, AuthMode::ChatGPT, "codex_cli_rs") + } = super::load_auth(codex_home.path(), false, AuthMode::ChatGPT) .unwrap() .unwrap(); assert_eq!(Some("sk-test-key".to_string()), api_key); @@ -596,7 +591,7 @@ mod tests { ) .unwrap(); - let auth = super::load_auth(dir.path(), false, AuthMode::ChatGPT, "codex_cli_rs") + let auth = super::load_auth(dir.path(), false, AuthMode::ChatGPT) .unwrap() .unwrap(); assert_eq!(auth.mode, AuthMode::ApiKey); @@ -680,7 +675,6 @@ mod tests { #[derive(Debug)] pub struct AuthManager { codex_home: PathBuf, - originator: String, inner: RwLock, } @@ -689,13 +683,12 @@ impl AuthManager { /// preferred auth method. Errors loading auth are swallowed; `auth()` will /// simply return `None` in that case so callers can treat it as an /// unauthenticated state. - pub fn new(codex_home: PathBuf, preferred_auth_mode: AuthMode, originator: String) -> Self { - let auth = CodexAuth::from_codex_home(&codex_home, preferred_auth_mode, &originator) + pub fn new(codex_home: PathBuf, preferred_auth_mode: AuthMode) -> Self { + let auth = CodexAuth::from_codex_home(&codex_home, preferred_auth_mode) .ok() .flatten(); Self { codex_home, - originator, inner: RwLock::new(CachedAuth { preferred_auth_mode, auth, @@ -712,7 +705,6 @@ impl AuthManager { }; Arc::new(Self { codex_home: PathBuf::new(), - originator: "codex_cli_rs".to_string(), inner: RwLock::new(cached), }) } @@ -734,7 +726,7 @@ impl AuthManager { /// whether the auth value changed. pub fn reload(&self) -> bool { let preferred = self.preferred_auth_method(); - let new_auth = CodexAuth::from_codex_home(&self.codex_home, preferred, &self.originator) + let new_auth = CodexAuth::from_codex_home(&self.codex_home, preferred) .ok() .flatten(); if let Ok(mut guard) = self.inner.write() { @@ -755,12 +747,8 @@ impl AuthManager { } /// Convenience constructor returning an `Arc` wrapper. - pub fn shared( - codex_home: PathBuf, - preferred_auth_mode: AuthMode, - originator: String, - ) -> Arc { - Arc::new(Self::new(codex_home, preferred_auth_mode, originator)) + pub fn shared(codex_home: PathBuf, preferred_auth_mode: AuthMode) -> Arc { + Arc::new(Self::new(codex_home, preferred_auth_mode)) } /// Attempt to refresh the current auth token (if any). On success, reload diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 2050d8e219..1268be65c3 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -84,7 +84,7 @@ impl ModelClient { summary: ReasoningSummaryConfig, conversation_id: ConversationId, ) -> Self { - let client = create_client(&config.responses_originator_header); + let client = create_client(); Self { config, diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 1512759d87..338dd4a2dd 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -40,8 +40,6 @@ pub(crate) const PROJECT_DOC_MAX_BYTES: usize = 32 * 1024; // 32 KiB const CONFIG_TOML_FILE: &str = "config.toml"; -const DEFAULT_RESPONSES_ORIGINATOR_HEADER: &str = "codex_cli_rs"; - /// Application configuration loaded from disk and merged with overrides. #[derive(Debug, Clone, PartialEq)] pub struct Config { @@ -169,9 +167,6 @@ pub struct Config { pub tools_web_search_request: bool, - /// The value for the `originator` header included with Responses API requests. - pub responses_originator_header: String, - /// If set to `true`, the API key will be signed with the `originator` header. pub preferred_auth_method: AuthMode, @@ -478,9 +473,6 @@ pub struct ConfigToml { pub experimental_use_exec_command_tool: Option, - /// The value for the `originator` header included with Responses API requests. - pub responses_originator_header_internal_override: Option, - pub projects: Option>, /// If set to `true`, the API key will be signed with the `originator` header. @@ -773,10 +765,6 @@ impl Config { Self::get_base_instructions(experimental_instructions_path, &resolved_cwd)?; let base_instructions = base_instructions.or(file_base_instructions); - let responses_originator_header: String = cfg - .responses_originator_header_internal_override - .unwrap_or(DEFAULT_RESPONSES_ORIGINATOR_HEADER.to_owned()); - let config = Self { model, model_family, @@ -826,7 +814,6 @@ impl Config { include_plan_tool: include_plan_tool.unwrap_or(false), include_apply_patch_tool: include_apply_patch_tool.unwrap_or(false), tools_web_search_request, - responses_originator_header, preferred_auth_method: cfg.preferred_auth_method.unwrap_or(AuthMode::ChatGPT), use_experimental_streamable_shell_tool: cfg .experimental_use_exec_command_tool @@ -1203,7 +1190,6 @@ model_verbosity = "high" include_plan_tool: false, include_apply_patch_tool: false, tools_web_search_request: false, - responses_originator_header: "codex_cli_rs".to_string(), preferred_auth_method: AuthMode::ChatGPT, use_experimental_streamable_shell_tool: false, include_view_image_tool: true, @@ -1260,7 +1246,6 @@ model_verbosity = "high" include_plan_tool: false, include_apply_patch_tool: false, tools_web_search_request: false, - responses_originator_header: "codex_cli_rs".to_string(), preferred_auth_method: AuthMode::ChatGPT, use_experimental_streamable_shell_tool: false, include_view_image_tool: true, @@ -1332,7 +1317,6 @@ model_verbosity = "high" include_plan_tool: false, include_apply_patch_tool: false, tools_web_search_request: false, - responses_originator_header: "codex_cli_rs".to_string(), preferred_auth_method: AuthMode::ChatGPT, use_experimental_streamable_shell_tool: false, include_view_image_tool: true, @@ -1390,7 +1374,6 @@ model_verbosity = "high" include_plan_tool: false, include_apply_patch_tool: false, tools_web_search_request: false, - responses_originator_header: "codex_cli_rs".to_string(), preferred_auth_method: AuthMode::ChatGPT, use_experimental_streamable_shell_tool: false, include_view_image_tool: true, diff --git a/codex-rs/core/src/default_client.rs b/codex-rs/core/src/default_client.rs index d274351779..a2192ffaeb 100644 --- a/codex-rs/core/src/default_client.rs +++ b/codex-rs/core/src/default_client.rs @@ -1,11 +1,40 @@ -pub const DEFAULT_ORIGINATOR: &str = "codex_cli_rs"; +use reqwest::header::HeaderValue; +use std::sync::LazyLock; -pub fn get_codex_user_agent(originator: Option<&str>) -> String { +pub const CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR: &str = "CODEX_INTERNAL_ORIGINATOR_OVERRIDE"; + +#[derive(Debug, Clone)] +pub struct Originator { + pub value: String, + pub header_value: HeaderValue, +} + +pub static ORIGINATOR: LazyLock = LazyLock::new(|| { + let default = "codex_cli_rs"; + let value = std::env::var(CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR) + .unwrap_or_else(|_| default.to_string()); + + match HeaderValue::from_str(&value) { + Ok(header_value) => Originator { + value, + header_value, + }, + Err(e) => { + tracing::error!("Unable to turn originator override {value} into header value: {e}"); + Originator { + value: default.to_string(), + header_value: HeaderValue::from_static(default), + } + } + } +}); + +pub fn get_codex_user_agent() -> String { let build_version = env!("CARGO_PKG_VERSION"); let os_info = os_info::get(); format!( "{}/{build_version} ({} {}; {}) {}", - originator.unwrap_or(DEFAULT_ORIGINATOR), + ORIGINATOR.value.as_str(), os_info.os_type(), os_info.version(), os_info.architecture().unwrap_or("unknown"), @@ -14,25 +43,19 @@ pub fn get_codex_user_agent(originator: Option<&str>) -> String { } /// Create a reqwest client with default `originator` and `User-Agent` headers set. -pub fn create_client(originator: &str) -> reqwest::Client { +pub fn create_client() -> reqwest::Client { use reqwest::header::HeaderMap; - use reqwest::header::HeaderValue; let mut headers = HeaderMap::new(); - let originator_value = HeaderValue::from_str(originator) - .unwrap_or_else(|_| HeaderValue::from_static(DEFAULT_ORIGINATOR)); - headers.insert("originator", originator_value); - let ua = get_codex_user_agent(Some(originator)); + headers.insert("originator", ORIGINATOR.header_value.clone()); + let ua = get_codex_user_agent(); - match reqwest::Client::builder() + reqwest::Client::builder() // Set UA via dedicated helper to avoid header validation pitfalls .user_agent(ua) .default_headers(headers) .build() - { - Ok(client) => client, - Err(_) => reqwest::Client::new(), - } + .unwrap_or_else(|_| reqwest::Client::new()) } #[cfg(test)] @@ -41,7 +64,7 @@ mod tests { #[test] fn test_get_codex_user_agent() { - let user_agent = get_codex_user_agent(None); + let user_agent = get_codex_user_agent(); assert!(user_agent.starts_with("codex_cli_rs/")); } @@ -53,8 +76,7 @@ mod tests { use wiremock::matchers::method; use wiremock::matchers::path; - let originator = "test_originator"; - let client = create_client(originator); + let client = create_client(); // Spin up a local mock server and capture a request. let server = MockServer::start().await; @@ -82,10 +104,10 @@ mod tests { let originator_header = headers .get("originator") .expect("originator header missing"); - assert_eq!(originator_header.to_str().unwrap(), originator); + assert_eq!(originator_header.to_str().unwrap(), "codex_cli_rs"); // User-Agent matches the computed Codex UA for that originator - let expected_ua = get_codex_user_agent(Some(originator)); + let expected_ua = get_codex_user_agent(); let ua_header = headers .get("user-agent") .expect("user-agent header missing"); @@ -96,7 +118,7 @@ mod tests { #[cfg(target_os = "macos")] fn test_macos() { use regex_lite::Regex; - let user_agent = get_codex_user_agent(None); + let user_agent = get_codex_user_agent(); let re = Regex::new( r"^codex_cli_rs/\d+\.\d+\.\d+ \(Mac OS \d+\.\d+\.\d+; (x86_64|arm64)\) (\S+)$", ) diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 1aa1a49de7..ed609c2191 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -371,56 +371,6 @@ async fn includes_base_instructions_override_in_request() { ); } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn originator_config_override_is_used() { - // Mock server - let server = MockServer::start().await; - - let first = ResponseTemplate::new(200) - .insert_header("content-type", "text/event-stream") - .set_body_raw(sse_completed("resp1"), "text/event-stream"); - - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(first) - .expect(1) - .mount(&server) - .await; - - let model_provider = ModelProviderInfo { - base_url: Some(format!("{}/v1", server.uri())), - ..built_in_model_providers()["openai"].clone() - }; - - let codex_home = TempDir::new().unwrap(); - let mut config = load_default_config_for_test(&codex_home); - config.model_provider = model_provider; - config.responses_originator_header = "my_override".to_owned(); - - let conversation_manager = - ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key")); - let codex = conversation_manager - .new_conversation(config) - .await - .expect("create new conversation") - .conversation; - - codex - .submit(Op::UserInput { - items: vec![InputItem::Text { - text: "hello".into(), - }], - }) - .await - .unwrap(); - - wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - - let request = &server.received_requests().await.unwrap()[0]; - let request_originator = request.headers.get("originator").unwrap(); - assert_eq!(request_originator.to_str().unwrap(), "my_override"); -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn chatgpt_auth_sends_correct_request() { if std::env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { @@ -546,15 +496,12 @@ async fn prefers_chatgpt_token_when_config_prefers_chatgpt() { config.model_provider = model_provider; config.preferred_auth_method = AuthMode::ChatGPT; - let auth_manager = match CodexAuth::from_codex_home( - codex_home.path(), - config.preferred_auth_method, - &config.responses_originator_header, - ) { - Ok(Some(auth)) => codex_core::AuthManager::from_auth_for_testing(auth), - Ok(None) => panic!("No CodexAuth found in codex_home"), - Err(e) => panic!("Failed to load CodexAuth: {e}"), - }; + let auth_manager = + match CodexAuth::from_codex_home(codex_home.path(), config.preferred_auth_method) { + Ok(Some(auth)) => codex_core::AuthManager::from_auth_for_testing(auth), + Ok(None) => panic!("No CodexAuth found in codex_home"), + Err(e) => panic!("Failed to load CodexAuth: {e}"), + }; let conversation_manager = ConversationManager::new(auth_manager); let NewConversation { conversation: codex, @@ -622,15 +569,12 @@ async fn prefers_apikey_when_config_prefers_apikey_even_with_chatgpt_tokens() { config.model_provider = model_provider; config.preferred_auth_method = AuthMode::ApiKey; - let auth_manager = match CodexAuth::from_codex_home( - codex_home.path(), - config.preferred_auth_method, - &config.responses_originator_header, - ) { - Ok(Some(auth)) => codex_core::AuthManager::from_auth_for_testing(auth), - Ok(None) => panic!("No CodexAuth found in codex_home"), - Err(e) => panic!("Failed to load CodexAuth: {e}"), - }; + let auth_manager = + match CodexAuth::from_codex_home(codex_home.path(), config.preferred_auth_method) { + Ok(Some(auth)) => codex_core::AuthManager::from_auth_for_testing(auth), + Ok(None) => panic!("No CodexAuth found in codex_home"), + Err(e) => panic!("Failed to load CodexAuth: {e}"), + }; let conversation_manager = ConversationManager::new(auth_manager); let NewConversation { conversation: codex, diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 8215d7e698..66d0c09065 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -190,7 +190,6 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any let conversation_manager = ConversationManager::new(AuthManager::shared( config.codex_home.clone(), config.preferred_auth_method, - config.responses_originator_header.clone(), )); let NewConversation { conversation_id: _, diff --git a/codex-rs/login/src/server.rs b/codex-rs/login/src/server.rs index cfd1db8b4a..e267652dda 100644 --- a/codex-rs/login/src/server.rs +++ b/codex-rs/login/src/server.rs @@ -16,6 +16,7 @@ use base64::Engine; use chrono::Utc; use codex_core::auth::AuthDotJson; use codex_core::auth::get_auth_file; +use codex_core::default_client::ORIGINATOR; use codex_core::token_data::TokenData; use codex_core::token_data::parse_id_token; use rand::RngCore; @@ -35,11 +36,10 @@ pub struct ServerOptions { pub port: u16, pub open_browser: bool, pub force_state: Option, - pub originator: String, } impl ServerOptions { - pub fn new(codex_home: PathBuf, client_id: String, originator: String) -> Self { + pub fn new(codex_home: PathBuf, client_id: String) -> Self { Self { codex_home, client_id: client_id.to_string(), @@ -47,7 +47,6 @@ impl ServerOptions { port: DEFAULT_PORT, open_browser: true, force_state: None, - originator, } } } @@ -103,14 +102,7 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result { let server = Arc::new(server); let redirect_uri = format!("http://localhost:{actual_port}/auth/callback"); - let auth_url = build_authorize_url( - &opts.issuer, - &opts.client_id, - &redirect_uri, - &pkce, - &state, - &opts.originator, - ); + let auth_url = build_authorize_url(&opts.issuer, &opts.client_id, &redirect_uri, &pkce, &state); if opts.open_browser { let _ = webbrowser::open(&auth_url); @@ -311,7 +303,6 @@ fn build_authorize_url( redirect_uri: &str, pkce: &PkceCodes, state: &str, - originator: &str, ) -> String { let query = vec![ ("response_type", "code"), @@ -323,7 +314,7 @@ fn build_authorize_url( ("id_token_add_organizations", "true"), ("codex_cli_simplified_flow", "true"), ("state", state), - ("originator", originator), + ("originator", ORIGINATOR.value.as_str()), ]; let qs = query .into_iter() diff --git a/codex-rs/login/tests/suite/login_server_e2e.rs b/codex-rs/login/tests/suite/login_server_e2e.rs index 5a600a4bed..9309ef4123 100644 --- a/codex-rs/login/tests/suite/login_server_e2e.rs +++ b/codex-rs/login/tests/suite/login_server_e2e.rs @@ -102,7 +102,6 @@ async fn end_to_end_login_flow_persists_auth_json() { port: 0, open_browser: false, force_state: Some(state), - originator: "test_originator".to_string(), }; let server = run_login_server(opts).unwrap(); let login_port = server.actual_port; @@ -161,7 +160,6 @@ async fn creates_missing_codex_home_dir() { port: 0, open_browser: false, force_state: Some(state), - originator: "test_originator".to_string(), }; let server = run_login_server(opts).unwrap(); let login_port = server.actual_port; @@ -202,7 +200,6 @@ async fn cancels_previous_login_server_when_port_is_in_use() { port: 0, open_browser: false, force_state: Some("cancel_state".to_string()), - originator: "test_originator".to_string(), }; let first_server = run_login_server(first_opts).unwrap(); @@ -221,7 +218,6 @@ async fn cancels_previous_login_server_when_port_is_in_use() { port: login_port, open_browser: false, force_state: Some("cancel_state_2".to_string()), - originator: "test_originator".to_string(), }; let second_server = run_login_server(second_opts).unwrap(); diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index e6133e174b..fa84bf901b 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -198,11 +198,7 @@ impl CodexMessageProcessor { let opts = LoginServerOptions { open_browser: false, - ..LoginServerOptions::new( - config.codex_home.clone(), - CLIENT_ID.to_string(), - config.responses_originator_header.clone(), - ) + ..LoginServerOptions::new(config.codex_home.clone(), CLIENT_ID.to_string()) }; enum LoginChatGptReply { @@ -403,7 +399,7 @@ impl CodexMessageProcessor { } async fn get_user_agent(&self, request_id: RequestId) { - let user_agent = get_codex_user_agent(Some(&self.config.responses_originator_header)); + let user_agent = get_codex_user_agent(); let response = GetUserAgentResponse { user_agent }; self.outgoing.send_response(request_id, response).await; } diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 8179cdd679..dccd86e935 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -54,11 +54,8 @@ impl MessageProcessor { config: Arc, ) -> Self { let outgoing = Arc::new(outgoing); - let auth_manager = AuthManager::shared( - config.codex_home.clone(), - config.preferred_auth_method, - config.responses_originator_header.clone(), - ); + let auth_manager = + AuthManager::shared(config.codex_home.clone(), config.preferred_auth_method); let conversation_manager = Arc::new(ConversationManager::new(auth_manager.clone())); let codex_message_processor = CodexMessageProcessor::new( auth_manager, diff --git a/codex-rs/mcp-server/tests/suite/user_agent.rs b/codex-rs/mcp-server/tests/suite/user_agent.rs index 050bea7abd..19de87a59f 100644 --- a/codex-rs/mcp-server/tests/suite/user_agent.rs +++ b/codex-rs/mcp-server/tests/suite/user_agent.rs @@ -1,4 +1,3 @@ -use codex_core::default_client::DEFAULT_ORIGINATOR; use codex_core::default_client::get_codex_user_agent; use codex_protocol::mcp_protocol::GetUserAgentResponse; use mcp_test_support::McpProcess; @@ -38,7 +37,7 @@ async fn get_user_agent_returns_current_codex_user_agent() { let received: GetUserAgentResponse = to_response(response).expect("deserialize getUserAgent response"); let expected = GetUserAgentResponse { - user_agent: get_codex_user_agent(Some(DEFAULT_ORIGINATOR)), + user_agent: get_codex_user_agent(), }; assert_eq!(received, expected); diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 73e97e5392..549037462c 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -312,11 +312,7 @@ async fn run_ratatui_app( .. } = cli; - let auth_manager = AuthManager::shared( - config.codex_home.clone(), - config.preferred_auth_method, - config.responses_originator_header.clone(), - ); + let auth_manager = AuthManager::shared(config.codex_home.clone(), config.preferred_auth_method); let login_status = get_login_status(&config); let should_show_onboarding = should_show_onboarding(login_status, &config, should_show_trust_screen); @@ -400,11 +396,7 @@ fn get_login_status(config: &Config) -> LoginStatus { // Reading the OpenAI API key is an async operation because it may need // to refresh the token. Block on it. let codex_home = config.codex_home.clone(); - match CodexAuth::from_codex_home( - &codex_home, - config.preferred_auth_method, - &config.responses_originator_header, - ) { + match CodexAuth::from_codex_home(&codex_home, config.preferred_auth_method) { Ok(Some(auth)) => LoginStatus::AuthMode(auth.mode), Ok(None) => LoginStatus::NotAuthenticated, Err(err) => { diff --git a/codex-rs/tui/src/onboarding/auth.rs b/codex-rs/tui/src/onboarding/auth.rs index 90b2c994f1..c7217ac1af 100644 --- a/codex-rs/tui/src/onboarding/auth.rs +++ b/codex-rs/tui/src/onboarding/auth.rs @@ -2,7 +2,6 @@ use codex_core::AuthManager; use codex_core::auth::CLIENT_ID; -use codex_core::config::Config; use codex_login::ServerOptions; use codex_login::ShutdownHandle; use codex_login::run_login_server; @@ -114,7 +113,6 @@ pub(crate) struct AuthModeWidget { pub login_status: LoginStatus, pub preferred_auth_method: AuthMode, pub auth_manager: Arc, - pub config: Config, } impl AuthModeWidget { @@ -316,11 +314,7 @@ impl AuthModeWidget { } self.error = None; - let opts = ServerOptions::new( - self.codex_home.clone(), - CLIENT_ID.to_string(), - self.config.responses_originator_header.clone(), - ); + let opts = ServerOptions::new(self.codex_home.clone(), CLIENT_ID.to_string()); match run_login_server(opts) { Ok(child) => { let sign_in_state = self.sign_in_state.clone(); diff --git a/codex-rs/tui/src/onboarding/onboarding_screen.rs b/codex-rs/tui/src/onboarding/onboarding_screen.rs index beb96bce3f..a43b4c0f9b 100644 --- a/codex-rs/tui/src/onboarding/onboarding_screen.rs +++ b/codex-rs/tui/src/onboarding/onboarding_screen.rs @@ -85,7 +85,6 @@ impl OnboardingScreen { login_status, auth_manager, preferred_auth_method, - config, })) } let is_git_repo = get_git_repo_root(&cwd).is_some(); diff --git a/codex-rs/tui/src/updates.rs b/codex-rs/tui/src/updates.rs index 90ef6d8580..7344e24c78 100644 --- a/codex-rs/tui/src/updates.rs +++ b/codex-rs/tui/src/updates.rs @@ -24,9 +24,8 @@ pub fn get_upgrade_version(config: &Config) -> Option { // Refresh the cached latest version in the background so TUI startup // isn’t blocked by a network call. The UI reads the previously cached // value (if any) for this run; the next run shows the banner if needed. - let originator = config.responses_originator_header.clone(); tokio::spawn(async move { - check_for_update(&version_file, &originator) + check_for_update(&version_file) .await .inspect_err(|e| tracing::error!("Failed to update version: {e}")) }); @@ -65,10 +64,10 @@ fn read_version_info(version_file: &Path) -> anyhow::Result { Ok(serde_json::from_str(&contents)?) } -async fn check_for_update(version_file: &Path, originator: &str) -> anyhow::Result<()> { +async fn check_for_update(version_file: &Path) -> anyhow::Result<()> { let ReleaseInfo { tag_name: latest_tag_name, - } = create_client(originator) + } = create_client() .get(LATEST_RELEASE_URL) .send() .await? From 5bcc9d8b770d7e3c69fa37fa01f5f1fd406837e2 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 9 Sep 2025 14:47:06 -0700 Subject: [PATCH 09/49] Do not send reasoning item IDs (#3390) Response API doesn't require IDs on reasoning items anymore. Fixes: https://github.com/openai/codex/issues/3292 --- codex-rs/protocol/src/models.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 115d4596ce..23a86a594e 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -49,7 +49,7 @@ pub enum ResponseItem { content: Vec, }, Reasoning { - #[serde(default)] + #[serde(default, skip_serializing)] id: String, summary: Vec, #[serde(default, skip_serializing_if = "should_serialize_reasoning_content")] From de6559f2ab83806496851322018c75039f7eee58 Mon Sep 17 00:00:00 2001 From: Andrew Tan <9657842+andrewtlw@users.noreply.github.com> Date: Wed, 10 Sep 2025 06:02:02 +0800 Subject: [PATCH 10/49] Include apply_patch tool for oss models from gpt-oss providers with different naming convention (e.g. `openai/gpt-oss-*`) (#2811) Model providers like Groq, Openrouter, AWS Bedrock, VertexAI and others typically prefix the name of gpt-oss models with `openai`, e.g. `openai/gpt-oss-120b`. This PR is to match the model name slug using `contains` instead of `starts_with` to ensure that the `apply_patch` tool is included in the tools for models names like `openai/gpt-oss-120b` Without this, the gpt-oss models will often try to call the `apply_patch` tool directly instead of via the `shell` command, leading to validation errors. I have run all the local checks. Note: The gpt-oss models from non-Ollama providers are typically run via a profile with a different base_url (instead of with the `--oss` flag) --------- Co-authored-by: Andrew Tan --- codex-rs/core/src/model_family.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/core/src/model_family.rs b/codex-rs/core/src/model_family.rs index 5b4ca39ec0..59fd622f1c 100644 --- a/codex-rs/core/src/model_family.rs +++ b/codex-rs/core/src/model_family.rs @@ -103,7 +103,7 @@ pub fn find_family_for_model(slug: &str) -> Option { slug, "gpt-4.1", needs_special_apply_patch_instructions: true, ) - } else if slug.starts_with("gpt-oss") { + } else if slug.starts_with("gpt-oss") || slug.starts_with("openai/gpt-oss") { model_family!(slug, "gpt-oss", apply_patch_tool_type: Some(ApplyPatchToolType::Function)) } else if slug.starts_with("gpt-4o") { simple_model_family!(slug, "gpt-4o") From 5c48600bb36718cb1eeca9c9175c089cfbf77e63 Mon Sep 17 00:00:00 2001 From: dank-openai Date: Tue, 9 Sep 2025 15:41:23 -0700 Subject: [PATCH 11/49] alt+delete deletes the word to the right of the cursor (delete_forward_word) (#3394) This mirrors alt+backspace, which deletes to the left of the cursor. --- codex-rs/tui/src/bottom_pane/textarea.rs | 105 +++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index 37aa85ff18..07b6077587 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -252,6 +252,11 @@ impl TextArea { modifiers: KeyModifiers::CONTROL, .. } => self.delete_backward(1), + KeyEvent { + code: KeyCode::Delete, + modifiers: KeyModifiers::ALT, + .. + } => self.delete_forward_word(), KeyEvent { code: KeyCode::Delete, .. @@ -435,6 +440,18 @@ impl TextArea { self.replace_range(start..self.cursor_pos, ""); } + /// Delete text to the right of the cursor using "word" semantics. + /// + /// Deletes from the current cursor position through the end of the next word as determined + /// by `end_of_next_word()`. Any whitespace (including newlines) between the cursor and that + /// word is included in the deletion. + pub fn delete_forward_word(&mut self) { + let end = self.end_of_next_word(); + if end > self.cursor_pos { + self.replace_range(self.cursor_pos..end, ""); + } + } + pub fn kill_to_end_of_line(&mut self) { let eol = self.end_of_current_line(); if self.cursor_pos == eol { @@ -1104,6 +1121,79 @@ mod tests { assert_eq!(t.cursor(), 3); } + #[test] + fn delete_forward_word_variants() { + let mut t = ta_with("hello world "); + t.set_cursor(0); + t.delete_forward_word(); + assert_eq!(t.text(), " world "); + assert_eq!(t.cursor(), 0); + + let mut t = ta_with("hello world "); + t.set_cursor(1); + t.delete_forward_word(); + assert_eq!(t.text(), "h world "); + assert_eq!(t.cursor(), 1); + + let mut t = ta_with("hello world"); + t.set_cursor(t.text().len()); + t.delete_forward_word(); + assert_eq!(t.text(), "hello world"); + assert_eq!(t.cursor(), t.text().len()); + + let mut t = ta_with("foo \nbar"); + t.set_cursor(3); + t.delete_forward_word(); + assert_eq!(t.text(), "foo"); + assert_eq!(t.cursor(), 3); + + let mut t = ta_with("foo\nbar"); + t.set_cursor(3); + t.delete_forward_word(); + assert_eq!(t.text(), "foo"); + assert_eq!(t.cursor(), 3); + + let mut t = ta_with("hello world "); + t.set_cursor(t.text().len() + 10); + t.delete_forward_word(); + assert_eq!(t.text(), "hello world "); + assert_eq!(t.cursor(), t.text().len()); + } + + #[test] + fn delete_forward_word_handles_atomic_elements() { + let mut t = TextArea::new(); + t.insert_element(""); + t.insert_str(" tail"); + + t.set_cursor(0); + t.delete_forward_word(); + assert_eq!(t.text(), " tail"); + assert_eq!(t.cursor(), 0); + + let mut t = TextArea::new(); + t.insert_str(" "); + t.insert_element(""); + t.insert_str(" tail"); + + t.set_cursor(0); + t.delete_forward_word(); + assert_eq!(t.text(), " tail"); + assert_eq!(t.cursor(), 0); + + let mut t = TextArea::new(); + t.insert_str("prefix "); + t.insert_element(""); + t.insert_str(" tail"); + + // cursor in the middle of the element, delete_forward_word deletes the element + let elem_range = t.elements[0].range.clone(); + t.cursor_pos = elem_range.start + (elem_range.len() / 2); + t.delete_forward_word(); + assert_eq!(t.text(), "prefix tail"); + assert_eq!(t.cursor(), elem_range.start); + } + #[test] fn cursor_left_and_right_handle_graphemes() { let mut t = ta_with("a👍b"); @@ -1174,6 +1264,21 @@ mod tests { assert_eq!(t.cursor(), 6); } + #[test] + fn delete_forward_word_with_without_alt_modifier() { + let mut t = ta_with("hello world"); + t.set_cursor(0); + t.input(KeyEvent::new(KeyCode::Delete, KeyModifiers::ALT)); + assert_eq!(t.text(), " world"); + assert_eq!(t.cursor(), 0); + + let mut t = ta_with("hello"); + t.set_cursor(0); + t.input(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); + assert_eq!(t.text(), "ello"); + assert_eq!(t.cursor(), 0); + } + #[test] fn control_h_backspace() { // Test Ctrl+H as backspace From 43809a454e5c6348418fc2d5ace1eb0a98f59847 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Sep 2025 16:52:33 -0700 Subject: [PATCH 12/49] Introduce rollout items (#3380) This PR introduces Rollout items. This enable us to rollout eventmsgs and session meta. This is mostly #3214 with rebase on main --- codex-rs/core/src/codex.rs | 238 +++++++++--------- codex-rs/core/src/conversation_manager.rs | 85 ++++++- codex-rs/core/src/rollout/list.rs | 52 +++- codex-rs/core/src/rollout/mod.rs | 1 - codex-rs/core/src/rollout/policy.rs | 56 ++++- codex-rs/core/src/rollout/recorder.rs | 150 ++++++----- codex-rs/core/src/rollout/tests.rs | 155 ++++++++---- codex-rs/core/tests/suite/cli_stream.rs | 40 ++- codex-rs/core/tests/suite/client.rs | 59 ++++- .../mcp-server/src/codex_message_processor.rs | 9 +- .../mcp-server/tests/suite/list_resume.rs | 43 +++- codex-rs/tui/src/resume_picker.rs | 36 +-- 12 files changed, 615 insertions(+), 309 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index cc8768d79b..5bd5c50413 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -10,6 +10,7 @@ use std::time::Duration; use crate::AuthManager; use crate::event_mapping::map_response_item_to_event_messages; +use crate::rollout::recorder::RolloutItem; use async_channel::Receiver; use async_channel::Sender; use codex_apply_patch::ApplyPatchAction; @@ -203,9 +204,6 @@ impl Codex { error!("Failed to create session: {e:#}"); CodexErr::InternalAgentDied })?; - session - .record_initial_history(&turn_context, conversation_history) - .await; let conversation_id = session.conversation_id; // This task will run until Op::Shutdown is received. @@ -494,13 +492,9 @@ impl Session { // Dispatch the SessionConfiguredEvent first and then report any errors. // If resuming, include converted initial messages in the payload so UIs can render them immediately. - let initial_messages = match &initial_history { - InitialHistory::New => None, - InitialHistory::Forked(items) => Some(sess.build_initial_messages(items)), - InitialHistory::Resumed(resumed_history) => { - Some(sess.build_initial_messages(&resumed_history.history)) - } - }; + let initial_messages = initial_history.get_event_msgs(); + sess.record_initial_history(&turn_context, initial_history) + .await; let events = std::iter::once(Event { id: INITIAL_SUBMIT_ID.to_owned(), @@ -515,9 +509,7 @@ impl Session { }) .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:?}"); - } + sess.send_event(event).await; } Ok((sess, turn_context)) @@ -547,53 +539,33 @@ impl Session { ) { match conversation_history { InitialHistory::New => { - self.record_initial_history_new(turn_context).await; + // Build and record initial items (user instructions + environment context) + let items = self.build_initial_context(turn_context); + self.record_conversation_items(&items).await; } - InitialHistory::Forked(items) => { - self.record_initial_history_from_items(items).await; - } - InitialHistory::Resumed(resumed_history) => { - self.record_initial_history_from_items(resumed_history.history) - .await; + InitialHistory::Resumed(_) | InitialHistory::Forked(_) => { + let rollout_items = conversation_history.get_rollout_items(); + let persist = matches!(conversation_history, InitialHistory::Forked(_)); + + // Always add response items to conversation history + let response_items = conversation_history.get_response_items(); + if !response_items.is_empty() { + self.record_into_history(&response_items); + } + + // If persisting, persist all rollout items as-is (recorder filters) + if persist && !rollout_items.is_empty() { + self.persist_rollout_items(&rollout_items).await; + } } } } - async fn record_initial_history_new(&self, turn_context: &TurnContext) { - // record the initial user instructions and environment context, - // regardless of whether we restored items. - // TODO: Those items shouldn't be "user messages" IMO. Maybe developer messages. - let mut conversation_items = Vec::::with_capacity(2); - if let Some(user_instructions) = turn_context.user_instructions.as_deref() { - conversation_items.push(UserInstructions::new(user_instructions.to_string()).into()); - } - conversation_items.push(ResponseItem::from(EnvironmentContext::new( - Some(turn_context.cwd.clone()), - Some(turn_context.approval_policy), - Some(turn_context.sandbox_policy.clone()), - Some(self.user_shell.clone()), - ))); - self.record_conversation_items(&conversation_items).await; - } - - async fn record_initial_history_from_items(&self, items: Vec) { - self.record_conversation_items_internal(&items, false).await; - } - - /// build the initial messages vector for SessionConfigured by converting - /// ResponseItems into EventMsg. - fn build_initial_messages(&self, items: &[ResponseItem]) -> Vec { - items - .iter() - .flat_map(|item| { - map_response_item_to_event_messages(item, self.show_raw_agent_reasoning) - }) - .collect() - } - - /// Sends the given event to the client and swallows the send event, if - /// any, logging it as an error. + /// Persist the event to rollout and send it to clients. pub(crate) async fn send_event(&self, event: Event) { + // Persist the event into rollout (recorder filters as needed) + let rollout_items = vec![RolloutItem::EventMsg(event.msg.clone())]; + self.persist_rollout_items(&rollout_items).await; if let Err(e) = self.tx_event.send(event).await { error!("failed to send tool call event: {e}"); } @@ -627,7 +599,7 @@ impl Session { reason, }), }; - let _ = self.tx_event.send(event).await; + self.send_event(event).await; rx_approve } @@ -659,7 +631,7 @@ impl Session { grant_root, }), }; - let _ = self.tx_event.send(event).await; + self.send_event(event).await; rx_approve } @@ -683,36 +655,76 @@ impl Session { state.approved_commands.insert(cmd); } - /// Records items to both the rollout and the chat completions/ZDR - /// transcript, if enabled. + /// Records input items: always append to conversation history and + /// persist these response items to rollout. async fn record_conversation_items(&self, items: &[ResponseItem]) { - self.record_conversation_items_internal(items, true).await; + self.record_into_history(items); + self.persist_rollout_response_items(items).await; } - async fn record_conversation_items_internal(&self, items: &[ResponseItem], persist: bool) { - debug!("Recording items for conversation: {items:?}"); - if persist { - self.record_state_snapshot(items).await; + /// Append ResponseItems to the in-memory conversation history only. + fn record_into_history(&self, items: &[ResponseItem]) { + self.state + .lock_unchecked() + .history + .record_items(items.iter()); + } + + async fn persist_rollout_response_items(&self, items: &[ResponseItem]) { + let rollout_items: Vec = items + .iter() + .cloned() + .map(RolloutItem::ResponseItem) + .collect(); + self.persist_rollout_items(&rollout_items).await; + } + + fn build_initial_context(&self, turn_context: &TurnContext) -> Vec { + let mut items = Vec::::with_capacity(2); + if let Some(user_instructions) = turn_context.user_instructions.as_deref() { + items.push(UserInstructions::new(user_instructions.to_string()).into()); } - - self.state.lock_unchecked().history.record_items(items); + items.push(ResponseItem::from(EnvironmentContext::new( + Some(turn_context.cwd.clone()), + Some(turn_context.approval_policy), + Some(turn_context.sandbox_policy.clone()), + Some(self.user_shell.clone()), + ))); + items } - async fn record_state_snapshot(&self, items: &[ResponseItem]) { - let snapshot = { crate::rollout::SessionStateSnapshot {} }; - + async fn persist_rollout_items(&self, items: &[RolloutItem]) { let recorder = { let guard = self.rollout.lock_unchecked(); guard.as_ref().cloned() }; + if let Some(rec) = recorder + && let Err(e) = rec.record_items(items).await + { + error!("failed to record rollout items: {e:#}"); + } + } - if let Some(rec) = recorder { - if let Err(e) = rec.record_state(snapshot).await { - error!("failed to record rollout state: {e:#}"); - } - if let Err(e) = rec.record_items(items).await { - error!("failed to record rollout items: {e:#}"); - } + /// Record a user input item to conversation history and also persist a + /// corresponding UserMessage EventMsg to rollout. + async fn record_input_and_rollout_usermsg(&self, response_input: &ResponseInputItem) { + let response_item: ResponseItem = response_input.clone().into(); + // Add to conversation history and persist response item to rollout + self.record_conversation_items(std::slice::from_ref(&response_item)) + .await; + + // Derive user message events and persist only UserMessage to rollout + let msgs = + map_response_item_to_event_messages(&response_item, self.show_raw_agent_reasoning); + let user_msgs: Vec = msgs + .into_iter() + .filter_map(|m| match m { + EventMsg::UserMessage(ev) => Some(RolloutItem::EventMsg(EventMsg::UserMessage(ev))), + _ => None, + }) + .collect(); + if !user_msgs.is_empty() { + self.persist_rollout_items(&user_msgs).await; } } @@ -755,7 +767,7 @@ impl Session { id: sub_id.to_string(), msg, }; - let _ = self.tx_event.send(event).await; + self.send_event(event).await; } async fn on_exec_command_end( @@ -802,7 +814,7 @@ impl Session { id: sub_id.to_string(), msg, }; - let _ = self.tx_event.send(event).await; + self.send_event(event).await; // If this is an apply_patch, after we emit the end patch, emit a second event // with the full turn diff if there is one. @@ -814,7 +826,7 @@ impl Session { id: sub_id.into(), msg, }; - let _ = self.tx_event.send(event).await; + self.send_event(event).await; } } } @@ -880,7 +892,7 @@ impl Session { message: message.into(), }), }; - let _ = self.tx_event.send(event).await; + self.send_event(event).await; } async fn notify_stream_error(&self, sub_id: &str, message: impl Into) { @@ -890,7 +902,7 @@ impl Session { message: message.into(), }), }; - let _ = self.tx_event.send(event).await; + self.send_event(event).await; } /// Build the full turn input by concatenating the current conversation @@ -1053,9 +1065,9 @@ impl AgentTask { id: self.sub_id, msg: EventMsg::TurnAborted(TurnAbortedEvent { reason }), }; - let tx_event = self.sess.tx_event.clone(); + let sess = self.sess.clone(); tokio::spawn(async move { - tx_event.send(event).await.ok(); + sess.send_event(event).await; }); } } @@ -1260,7 +1272,7 @@ async fn submission_loop( Op::GetHistoryEntryRequest { offset, log_id } => { let config = config.clone(); - let tx_event = sess.tx_event.clone(); + let sess_clone = sess.clone(); let sub_id = sub.id.clone(); tokio::spawn(async move { @@ -1288,13 +1300,10 @@ async fn submission_loop( ), }; - if let Err(e) = tx_event.send(event).await { - warn!("failed to send GetHistoryEntryResponse event: {e}"); - } + sess_clone.send_event(event).await; }); } Op::ListMcpTools => { - let tx_event = sess.tx_event.clone(); let sub_id = sub.id.clone(); // This is a cheap lookup from the connection manager's cache. @@ -1305,12 +1314,9 @@ async fn submission_loop( crate::protocol::McpListToolsResponseEvent { tools }, ), }; - if let Err(e) = tx_event.send(event).await { - warn!("failed to send McpListToolsResponse event: {e}"); - } + sess.send_event(event).await; } Op::ListCustomPrompts => { - let tx_event = sess.tx_event.clone(); let sub_id = sub.id.clone(); let custom_prompts: Vec = @@ -1326,9 +1332,7 @@ async fn submission_loop( custom_prompts, }), }; - if let Err(e) = tx_event.send(event).await { - warn!("failed to send ListCustomPromptsResponse event: {e}"); - } + sess.send_event(event).await; } Op::Compact => { // Create a summarization request as user input @@ -1364,22 +1368,17 @@ async fn submission_loop( 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:?}"); - } + sess.send_event(event).await; } let event = Event { id: sub.id.clone(), msg: EventMsg::ShutdownComplete, }; - if let Err(e) = sess.tx_event.send(event).await { - warn!("failed to send Shutdown event: {e}"); - } + sess.send_event(event).await; break; } Op::GetHistory => { - let tx_event = sess.tx_event.clone(); let sub_id = sub.id.clone(); let event = Event { @@ -1389,9 +1388,7 @@ async fn submission_loop( entries: sess.state.lock_unchecked().history.contents(), }), }; - if let Err(e) = tx_event.send(event).await { - warn!("failed to send ConversationHistory event: {e}"); - } + sess.send_event(event).await; } _ => { // Ignore unknown ops; enum is non_exhaustive to allow extensions. @@ -1429,12 +1426,10 @@ async fn run_task( model_context_window: turn_context.client.get_model_context_window(), }), }; - if sess.tx_event.send(event).await.is_err() { - return; - } + sess.send_event(event).await; let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); - sess.record_conversation_items(&[initial_input_for_turn.clone().into()]) + sess.record_input_and_rollout_usermsg(&initial_input_for_turn) .await; let mut last_agent_message: Option = None; @@ -1603,7 +1598,7 @@ async fn run_task( message: e.to_string(), }), }; - sess.tx_event.send(event).await.ok(); + sess.send_event(event).await; // let the user continue the conversation break; } @@ -1614,7 +1609,7 @@ async fn run_task( id: sub_id, msg: EventMsg::TaskComplete(TaskCompleteEvent { last_agent_message }), }; - sess.tx_event.send(event).await.ok(); + sess.send_event(event).await; } async fn run_turn( @@ -1812,13 +1807,12 @@ async fn try_run_turn( st.token_info = info.clone(); info }; - sess.tx_event - .send(Event { + let _ = sess + .send_event(Event { id: sub_id.to_string(), msg: EventMsg::TokenCount(crate::protocol::TokenCountEvent { info }), }) - .await - .ok(); + .await; let unified_diff = turn_diff_tracker.get_unified_diff(); if let Ok(Some(unified_diff)) = unified_diff { @@ -1827,7 +1821,7 @@ async fn try_run_turn( id: sub_id.to_string(), msg, }; - let _ = sess.tx_event.send(event).await; + sess.send_event(event).await; } return Ok(output); @@ -1837,21 +1831,21 @@ async fn try_run_turn( id: sub_id.to_string(), msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta }), }; - sess.tx_event.send(event).await.ok(); + sess.send_event(event).await; } ResponseEvent::ReasoningSummaryDelta(delta) => { let event = Event { id: sub_id.to_string(), msg: EventMsg::AgentReasoningDelta(AgentReasoningDeltaEvent { delta }), }; - sess.tx_event.send(event).await.ok(); + sess.send_event(event).await; } ResponseEvent::ReasoningSummaryPartAdded => { let event = Event { id: sub_id.to_string(), msg: EventMsg::AgentReasoningSectionBreak(AgentReasoningSectionBreakEvent {}), }; - sess.tx_event.send(event).await.ok(); + sess.send_event(event).await; } ResponseEvent::ReasoningContentDelta(delta) => { if sess.show_raw_agent_reasoning { @@ -1861,7 +1855,7 @@ async fn try_run_turn( AgentReasoningRawContentDeltaEvent { delta }, ), }; - sess.tx_event.send(event).await.ok(); + sess.send_event(event).await; } } } @@ -1882,9 +1876,7 @@ async fn run_compact_task( model_context_window, }), }; - if sess.tx_event.send(start_event).await.is_err() { - return; - } + sess.send_event(start_event).await; let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); let turn_input: Vec = @@ -2062,7 +2054,7 @@ async fn handle_response_item( id: sub_id.to_string(), msg, }; - sess.tx_event.send(event).await.ok(); + sess.send_event(event).await; } None } diff --git a/codex-rs/core/src/conversation_manager.rs b/codex-rs/core/src/conversation_manager.rs index 87b60e0155..e352680ca8 100644 --- a/codex-rs/core/src/conversation_manager.rs +++ b/codex-rs/core/src/conversation_manager.rs @@ -11,6 +11,7 @@ use crate::protocol::Event; use crate::protocol::EventMsg; use crate::protocol::SessionConfiguredEvent; use crate::rollout::RolloutRecorder; +use crate::rollout::recorder::RolloutItem; use codex_protocol::mcp_protocol::ConversationId; use codex_protocol::models::ResponseItem; use std::collections::HashMap; @@ -18,18 +19,72 @@ use std::path::PathBuf; use std::sync::Arc; use tokio::sync::RwLock; -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone)] pub struct ResumedHistory { pub conversation_id: ConversationId, - pub history: Vec, + pub history: Vec, pub rollout_path: PathBuf, } -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone)] pub enum InitialHistory { New, Resumed(ResumedHistory), - Forked(Vec), + Forked(Vec), +} + +impl InitialHistory { + pub(crate) fn get_rollout_items(&self) -> Vec { + match self { + InitialHistory::New => Vec::new(), + InitialHistory::Resumed(resumed) => resumed.history.clone(), + InitialHistory::Forked(items) => items.clone(), + } + } + pub fn get_response_items(&self) -> Vec { + match self { + InitialHistory::New => Vec::new(), + InitialHistory::Resumed(resumed) => resumed + .history + .iter() + .filter_map(|ri| match ri { + RolloutItem::ResponseItem(item) => Some(item.clone()), + _ => None, + }) + .collect(), + InitialHistory::Forked(items) => items + .iter() + .filter_map(|ri| match ri { + RolloutItem::ResponseItem(item) => Some(item.clone()), + _ => None, + }) + .collect(), + } + } + pub fn get_event_msgs(&self) -> Option> { + match self { + InitialHistory::New => None, + InitialHistory::Resumed(resumed) => Some( + resumed + .history + .iter() + .filter_map(|ri| match ri { + RolloutItem::EventMsg(ev) => Some(ev.clone()), + _ => None, + }) + .collect(), + ), + InitialHistory::Forked(items) => Some( + items + .iter() + .filter_map(|ri| match ri { + RolloutItem::EventMsg(ev) => Some(ev.clone()), + _ => None, + }) + .collect(), + ), + } + } } /// Represents a newly created Codex conversation, including the first event @@ -185,7 +240,8 @@ impl ConversationManager { /// and all items that follow them. fn truncate_after_dropping_last_messages(items: Vec, n: usize) -> InitialHistory { if n == 0 { - return InitialHistory::Forked(items); + let rolled: Vec = items.into_iter().map(RolloutItem::ResponseItem).collect(); + return InitialHistory::Forked(rolled); } // Walk backwards counting only `user` Message items, find cut index. @@ -207,7 +263,12 @@ fn truncate_after_dropping_last_messages(items: Vec, n: usize) -> // No prefix remains after dropping; start a new conversation. InitialHistory::New } else { - InitialHistory::Forked(items.into_iter().take(cut_index).collect()) + let rolled: Vec = items + .into_iter() + .take(cut_index) + .map(RolloutItem::ResponseItem) + .collect(); + InitialHistory::Forked(rolled) } } @@ -263,12 +324,18 @@ mod tests { ]; let truncated = truncate_after_dropping_last_messages(items.clone(), 1); + let got_items = truncated.get_rollout_items(); + let expected_items = vec![ + RolloutItem::ResponseItem(items[0].clone()), + RolloutItem::ResponseItem(items[1].clone()), + RolloutItem::ResponseItem(items[2].clone()), + ]; assert_eq!( - truncated, - InitialHistory::Forked(vec![items[0].clone(), items[1].clone(), items[2].clone(),]) + serde_json::to_value(&got_items).unwrap(), + serde_json::to_value(&expected_items).unwrap() ); let truncated2 = truncate_after_dropping_last_messages(items, 2); - assert_eq!(truncated2, InitialHistory::New); + assert!(matches!(truncated2, InitialHistory::New)); } } diff --git a/codex-rs/core/src/rollout/list.rs b/codex-rs/core/src/rollout/list.rs index 44f08e67a4..c4feb62958 100644 --- a/codex-rs/core/src/rollout/list.rs +++ b/codex-rs/core/src/rollout/list.rs @@ -10,6 +10,9 @@ use time::macros::format_description; use uuid::Uuid; use super::SESSIONS_SUBDIR; +use super::recorder::RolloutItem; +use super::recorder::RolloutLine; +use crate::protocol::EventMsg; /// Returned page of conversation summaries. #[derive(Debug, Default, PartialEq)] @@ -34,7 +37,7 @@ pub struct ConversationItem { } /// Hard cap to bound worst‑case work per request. -const MAX_SCAN_FILES: usize = 10_000; +const MAX_SCAN_FILES: usize = 100; const HEAD_RECORD_LIMIT: usize = 10; /// Pagination cursor identifying a file by timestamp and UUID. @@ -167,10 +170,16 @@ async fn traverse_directories_for_paths( if items.len() == page_size { break 'outer; } - let head = read_first_jsonl_records(&path, HEAD_RECORD_LIMIT) - .await - .unwrap_or_default(); - items.push(ConversationItem { path, head }); + // Read head and simultaneously detect message events within the same + // first N JSONL records to avoid a second file read. + let (head, saw_session_meta, saw_user_event) = + read_head_and_flags(&path, HEAD_RECORD_LIMIT) + .await + .unwrap_or((Vec::new(), false, false)); + // Apply filters: must have session meta and at least one user message event + if saw_session_meta && saw_user_event { + items.push(ConversationItem { path, head }); + } } } } @@ -273,16 +282,19 @@ fn parse_timestamp_uuid_from_filename(name: &str) -> Option<(OffsetDateTime, Uui Some((ts, uuid)) } -async fn read_first_jsonl_records( +async fn read_head_and_flags( path: &Path, max_records: usize, -) -> io::Result> { +) -> io::Result<(Vec, bool, bool)> { use tokio::io::AsyncBufReadExt; let file = tokio::fs::File::open(path).await?; let reader = tokio::io::BufReader::new(file); let mut lines = reader.lines(); let mut head: Vec = Vec::new(); + let mut saw_session_meta = false; + let mut saw_user_event = false; + while head.len() < max_records { let line_opt = lines.next_line().await?; let Some(line) = line_opt else { break }; @@ -290,9 +302,29 @@ async fn read_first_jsonl_records( if trimmed.is_empty() { continue; } - if let Ok(v) = serde_json::from_str::(trimmed) { - head.push(v); + + let parsed: Result = serde_json::from_str(trimmed); + let Ok(rollout_line) = parsed else { continue }; + + match rollout_line.item { + RolloutItem::SessionMeta(session_meta_line) => { + if let Ok(val) = serde_json::to_value(session_meta_line) { + head.push(val); + saw_session_meta = true; + } + } + RolloutItem::ResponseItem(item) => { + if let Ok(val) = serde_json::to_value(item) { + head.push(val); + } + } + RolloutItem::EventMsg(ev) => { + if matches!(ev, EventMsg::UserMessage(_)) { + saw_user_event = true; + } + } } } - Ok(head) + + Ok((head, saw_session_meta, saw_user_event)) } diff --git a/codex-rs/core/src/rollout/mod.rs b/codex-rs/core/src/rollout/mod.rs index a78d760959..341d39ae29 100644 --- a/codex-rs/core/src/rollout/mod.rs +++ b/codex-rs/core/src/rollout/mod.rs @@ -10,7 +10,6 @@ pub mod recorder; pub use recorder::RolloutRecorder; pub use recorder::RolloutRecorderParams; pub use recorder::SessionMeta; -pub use recorder::SessionStateSnapshot; #[cfg(test)] pub mod tests; diff --git a/codex-rs/core/src/rollout/policy.rs b/codex-rs/core/src/rollout/policy.rs index 3eb5290321..676f8e908d 100644 --- a/codex-rs/core/src/rollout/policy.rs +++ b/codex-rs/core/src/rollout/policy.rs @@ -1,8 +1,21 @@ +use crate::protocol::EventMsg; +use crate::rollout::recorder::RolloutItem; use codex_protocol::models::ResponseItem; +/// Whether a rollout `item` should be persisted in rollout files. +#[inline] +pub(crate) fn is_persisted_response_item(item: &RolloutItem) -> bool { + match item { + RolloutItem::ResponseItem(item) => should_persist_response_item(item), + RolloutItem::EventMsg(ev) => should_persist_event_msg(ev), + // Always persist session meta + RolloutItem::SessionMeta(_) => true, + } +} + /// Whether a `ResponseItem` should be persisted in rollout files. #[inline] -pub(crate) fn is_persisted_response_item(item: &ResponseItem) -> bool { +pub(crate) fn should_persist_response_item(item: &ResponseItem) -> bool { match item { ResponseItem::Message { .. } | ResponseItem::Reasoning { .. } @@ -14,3 +27,44 @@ pub(crate) fn is_persisted_response_item(item: &ResponseItem) -> bool { ResponseItem::WebSearchCall { .. } | ResponseItem::Other => false, } } + +/// Whether an `EventMsg` should be persisted in rollout files. +#[inline] +pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool { + match ev { + EventMsg::UserMessage(_) + | EventMsg::AgentMessage(_) + | EventMsg::AgentReasoning(_) + | EventMsg::AgentReasoningRawContent(_) + | EventMsg::TokenCount(_) => true, + EventMsg::Error(_) + | EventMsg::TaskStarted(_) + | EventMsg::TaskComplete(_) + | EventMsg::AgentMessageDelta(_) + | EventMsg::AgentReasoningDelta(_) + | EventMsg::AgentReasoningRawContentDelta(_) + | EventMsg::AgentReasoningSectionBreak(_) + | EventMsg::SessionConfigured(_) + | EventMsg::McpToolCallBegin(_) + | EventMsg::McpToolCallEnd(_) + | EventMsg::WebSearchBegin(_) + | EventMsg::WebSearchEnd(_) + | EventMsg::ExecCommandBegin(_) + | EventMsg::ExecCommandOutputDelta(_) + | EventMsg::ExecCommandEnd(_) + | EventMsg::ExecApprovalRequest(_) + | EventMsg::ApplyPatchApprovalRequest(_) + | EventMsg::BackgroundEvent(_) + | EventMsg::StreamError(_) + | EventMsg::PatchApplyBegin(_) + | EventMsg::PatchApplyEnd(_) + | EventMsg::TurnDiff(_) + | EventMsg::GetHistoryEntryResponse(_) + | EventMsg::McpListToolsResponse(_) + | EventMsg::ListCustomPromptsResponse(_) + | EventMsg::PlanUpdate(_) + | EventMsg::TurnAborted(_) + | EventMsg::ShutdownComplete + | EventMsg::ConversationHistory(_) => false, + } +} diff --git a/codex-rs/core/src/rollout/recorder.rs b/codex-rs/core/src/rollout/recorder.rs index 0e0af8990b..9954c4b1ca 100644 --- a/codex-rs/core/src/rollout/recorder.rs +++ b/codex-rs/core/src/rollout/recorder.rs @@ -28,25 +28,45 @@ use super::policy::is_persisted_response_item; use crate::config::Config; use crate::conversation_manager::InitialHistory; use crate::conversation_manager::ResumedHistory; +use crate::default_client::ORIGINATOR; use crate::git_info::GitInfo; use crate::git_info::collect_git_info; +use crate::protocol::EventMsg; use codex_protocol::models::ResponseItem; -#[derive(Serialize, Deserialize, Clone, Default)] +#[derive(Serialize, Deserialize, Clone, Default, Debug)] pub struct SessionMeta { pub id: ConversationId, pub timestamp: String, + pub cwd: PathBuf, + pub originator: String, + pub cli_version: String, pub instructions: Option, } -#[derive(Serialize)] -struct SessionMetaWithGit { +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct SessionMetaLine { #[serde(flatten)] meta: SessionMeta, #[serde(skip_serializing_if = "Option::is_none")] git: Option, } +#[derive(Serialize, Deserialize, Debug, Clone)] +#[serde(tag = "type", content = "payload", rename_all = "snake_case")] +pub enum RolloutItem { + SessionMeta(SessionMetaLine), + ResponseItem(ResponseItem), + EventMsg(EventMsg), +} + +#[derive(Serialize, Deserialize, Clone)] +pub(crate) struct RolloutLine { + pub(crate) timestamp: String, + #[serde(flatten)] + pub(crate) item: RolloutItem, +} + #[derive(Serialize, Deserialize, Default, Clone)] pub struct SessionStateSnapshot {} @@ -87,8 +107,7 @@ pub enum RolloutRecorderParams { } enum RolloutCmd { - AddItems(Vec), - UpdateState(SessionStateSnapshot), + AddItems(Vec), Shutdown { ack: oneshot::Sender<()> }, } @@ -144,8 +163,11 @@ impl RolloutRecorder { tokio::fs::File::from_std(file), path, Some(SessionMeta { - timestamp, id: session_id, + timestamp, + cwd: config.cwd.clone(), + originator: ORIGINATOR.value.clone(), + cli_version: env!("CARGO_PKG_VERSION").to_string(), instructions, }), ) @@ -176,7 +198,7 @@ impl RolloutRecorder { Ok(Self { tx, rollout_path }) } - pub(crate) async fn record_items(&self, items: &[ResponseItem]) -> std::io::Result<()> { + pub(crate) async fn record_items(&self, items: &[RolloutItem]) -> std::io::Result<()> { let mut filtered = Vec::new(); for item in items { // Note that function calls may look a bit strange if they are @@ -195,60 +217,48 @@ impl RolloutRecorder { .map_err(|e| IoError::other(format!("failed to queue rollout items: {e}"))) } - pub(crate) async fn record_state(&self, state: SessionStateSnapshot) -> std::io::Result<()> { - self.tx - .send(RolloutCmd::UpdateState(state)) - .await - .map_err(|e| IoError::other(format!("failed to queue rollout state: {e}"))) - } - - pub async fn get_rollout_history(path: &Path) -> std::io::Result { + pub(crate) async fn get_rollout_history(path: &Path) -> std::io::Result { info!("Resuming rollout from {path:?}"); tracing::error!("Resuming rollout from {path:?}"); let text = tokio::fs::read_to_string(path).await?; - let mut lines = text.lines(); - let first_line = lines - .next() - .ok_or_else(|| IoError::other("empty session file"))?; - let conversation_id = match serde_json::from_str::(first_line) { - Ok(rollout_session_meta) => { - tracing::error!( - "Parsed conversation ID from rollout file: {:?}", - rollout_session_meta.id - ); - Some(rollout_session_meta.id) - } - Err(e) => { - return Err(IoError::other(format!( - "failed to parse first line of rollout file as SessionMeta: {e}" - ))); - } - }; + if text.trim().is_empty() { + return Err(IoError::other("empty session file")); + } - let mut items = Vec::new(); - for line in lines { + let mut items: Vec = Vec::new(); + let mut conversation_id: Option = None; + for line in text.lines() { if line.trim().is_empty() { continue; } let v: Value = match serde_json::from_str(line) { Ok(v) => v, - Err(_) => continue, - }; - if v.get("record_type") - .and_then(|rt| rt.as_str()) - .map(|s| s == "state") - .unwrap_or(false) - { - continue; - } - match serde_json::from_value::(v.clone()) { - Ok(item) => { - if is_persisted_response_item(&item) { - items.push(item); - } - } Err(e) => { - warn!("failed to parse item: {v:?}, error: {e}"); + warn!("failed to parse line as JSON: {line:?}, error: {e}"); + continue; + } + }; + + // Parse the rollout line structure + match serde_json::from_value::(v.clone()) { + Ok(rollout_line) => match rollout_line.item { + RolloutItem::SessionMeta(session_meta_line) => { + tracing::error!( + "Parsed conversation ID from rollout file: {:?}", + session_meta_line.meta.id + ); + conversation_id = Some(session_meta_line.meta.id); + items.push(RolloutItem::SessionMeta(session_meta_line)); + } + RolloutItem::ResponseItem(item) => { + items.push(RolloutItem::ResponseItem(item)); + } + RolloutItem::EventMsg(_ev) => { + items.push(RolloutItem::EventMsg(_ev)); + } + }, + Err(e) => { + warn!("failed to parse rollout line: {v:?}, error: {e}"); } } } @@ -352,13 +362,15 @@ async fn rollout_writer( // If we have a meta, collect git info asynchronously and write meta first if let Some(session_meta) = meta.take() { let git_info = collect_git_info(&cwd).await; - let session_meta_with_git = SessionMetaWithGit { + let session_meta_line = SessionMetaLine { meta: session_meta, git: git_info, }; - // Write the SessionMeta as the first item in the file - writer.write_line(&session_meta_with_git).await?; + // Write the SessionMeta as the first item in the file, wrapped in a rollout line + writer + .write_rollout_item(RolloutItem::SessionMeta(session_meta_line)) + .await?; } // Process rollout commands @@ -367,24 +379,10 @@ async fn rollout_writer( RolloutCmd::AddItems(items) => { for item in items { if is_persisted_response_item(&item) { - writer.write_line(&item).await?; + writer.write_rollout_item(item).await?; } } } - RolloutCmd::UpdateState(state) => { - #[derive(Serialize)] - struct StateLine<'a> { - record_type: &'static str, - #[serde(flatten)] - state: &'a SessionStateSnapshot, - } - writer - .write_line(&StateLine { - record_type: "state", - state: &state, - }) - .await?; - } RolloutCmd::Shutdown { ack } => { let _ = ack.send(()); } @@ -399,6 +397,20 @@ struct JsonlWriter { } impl JsonlWriter { + async fn write_rollout_item(&mut self, rollout_item: RolloutItem) -> std::io::Result<()> { + let timestamp_format: &[FormatItem] = format_description!( + "[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:3]Z" + ); + let timestamp = OffsetDateTime::now_utc() + .format(timestamp_format) + .map_err(|e| IoError::other(format!("failed to format timestamp: {e}")))?; + + let line = RolloutLine { + timestamp, + item: rollout_item, + }; + self.write_line(&line).await + } async fn write_line(&mut self, item: &impl serde::Serialize) -> std::io::Result<()> { let mut json = serde_json::to_string(item)?; json.push('\n'); diff --git a/codex-rs/core/src/rollout/tests.rs b/codex-rs/core/src/rollout/tests.rs index 52c121e772..ecff658d29 100644 --- a/codex-rs/core/src/rollout/tests.rs +++ b/codex-rs/core/src/rollout/tests.rs @@ -42,10 +42,30 @@ fn write_session_file( let meta = serde_json::json!({ "timestamp": ts_str, - "id": uuid.to_string() + "type": "session_meta", + "payload": { + "id": uuid, + "timestamp": ts_str, + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + } }); writeln!(file, "{meta}")?; + // Include at least one user message event to satisfy listing filters + let user_event = serde_json::json!({ + "timestamp": ts_str, + "type": "event_msg", + "payload": { + "type": "user_message", + "message": "Hello from user", + "kind": "plain" + } + }); + writeln!(file, "{user_event}")?; + for i in 0..num_records { let rec = serde_json::json!({ "record_type": "response", @@ -93,24 +113,30 @@ async fn test_list_conversations_latest_first() { .join("01") .join(format!("rollout-2025-01-01T12-00-00-{u1}.jsonl")); - let head_3 = vec![ - serde_json::json!({"timestamp": "2025-01-03T12-00-00", "id": u3.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - serde_json::json!({"record_type": "response", "index": 1}), - serde_json::json!({"record_type": "response", "index": 2}), - ]; - let head_2 = vec![ - serde_json::json!({"timestamp": "2025-01-02T12-00-00", "id": u2.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - serde_json::json!({"record_type": "response", "index": 1}), - serde_json::json!({"record_type": "response", "index": 2}), - ]; - let head_1 = vec![ - serde_json::json!({"timestamp": "2025-01-01T12-00-00", "id": u1.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - serde_json::json!({"record_type": "response", "index": 1}), - serde_json::json!({"record_type": "response", "index": 2}), - ]; + let head_3 = vec![serde_json::json!({ + "id": u3, + "timestamp": "2025-01-03T12-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; + let head_2 = vec![serde_json::json!({ + "id": u2, + "timestamp": "2025-01-02T12-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; + let head_1 = vec![serde_json::json!({ + "id": u1, + "timestamp": "2025-01-01T12-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; let expected_cursor: Cursor = serde_json::from_str(&format!("\"2025-01-01T12-00-00|{u1}\"")).unwrap(); @@ -170,14 +196,22 @@ async fn test_pagination_cursor() { .join("03") .join("04") .join(format!("rollout-2025-03-04T09-00-00-{u4}.jsonl")); - let head_5 = vec![ - serde_json::json!({"timestamp": "2025-03-05T09-00-00", "id": u5.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - ]; - let head_4 = vec![ - serde_json::json!({"timestamp": "2025-03-04T09-00-00", "id": u4.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - ]; + let head_5 = vec![serde_json::json!({ + "id": u5, + "timestamp": "2025-03-05T09-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; + let head_4 = vec![serde_json::json!({ + "id": u4, + "timestamp": "2025-03-04T09-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; let expected_cursor1: Cursor = serde_json::from_str(&format!("\"2025-03-04T09-00-00|{u4}\"")).unwrap(); let expected_page1 = ConversationsPage { @@ -212,14 +246,22 @@ async fn test_pagination_cursor() { .join("03") .join("02") .join(format!("rollout-2025-03-02T09-00-00-{u2}.jsonl")); - let head_3 = vec![ - serde_json::json!({"timestamp": "2025-03-03T09-00-00", "id": u3.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - ]; - let head_2 = vec![ - serde_json::json!({"timestamp": "2025-03-02T09-00-00", "id": u2.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - ]; + let head_3 = vec![serde_json::json!({ + "id": u3, + "timestamp": "2025-03-03T09-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; + let head_2 = vec![serde_json::json!({ + "id": u2, + "timestamp": "2025-03-02T09-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; let expected_cursor2: Cursor = serde_json::from_str(&format!("\"2025-03-02T09-00-00|{u2}\"")).unwrap(); let expected_page2 = ConversationsPage { @@ -248,10 +290,14 @@ async fn test_pagination_cursor() { .join("03") .join("01") .join(format!("rollout-2025-03-01T09-00-00-{u1}.jsonl")); - let head_1 = vec![ - serde_json::json!({"timestamp": "2025-03-01T09-00-00", "id": u1.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - ]; + let head_1 = vec![serde_json::json!({ + "id": u1, + "timestamp": "2025-03-01T09-00-00", + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; let expected_cursor3: Cursor = serde_json::from_str(&format!("\"2025-03-01T09-00-00|{u1}\"")).unwrap(); let expected_page3 = ConversationsPage { @@ -287,11 +333,14 @@ async fn test_get_conversation_contents() { .join("04") .join("01") .join(format!("rollout-2025-04-01T10-30-00-{uuid}.jsonl")); - let expected_head = vec![ - serde_json::json!({"timestamp": ts, "id": uuid.to_string()}), - serde_json::json!({"record_type": "response", "index": 0}), - serde_json::json!({"record_type": "response", "index": 1}), - ]; + let expected_head = vec![serde_json::json!({ + "id": uuid, + "timestamp": ts, + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })]; let expected_cursor: Cursor = serde_json::from_str(&format!("\"{ts}|{uuid}\"")).unwrap(); let expected_page = ConversationsPage { items: vec![ConversationItem { @@ -305,10 +354,15 @@ async fn test_get_conversation_contents() { assert_eq!(page, expected_page); // Entire file contents equality - let meta = serde_json::json!({"timestamp": ts, "id": uuid.to_string()}); + let meta = serde_json::json!({"timestamp": ts, "type": "session_meta", "payload": {"id": uuid, "timestamp": ts, "instructions": null, "cwd": ".", "originator": "test_originator", "cli_version": "test_version"}}); + let user_event = serde_json::json!({ + "timestamp": ts, + "type": "event_msg", + "payload": {"type": "user_message", "message": "Hello from user", "kind": "plain"} + }); let rec0 = serde_json::json!({"record_type": "response", "index": 0}); let rec1 = serde_json::json!({"record_type": "response", "index": 1}); - let expected_content = format!("{meta}\n{rec0}\n{rec1}\n"); + let expected_content = format!("{meta}\n{user_event}\n{rec0}\n{rec1}\n"); assert_eq!(content, expected_content); } @@ -341,7 +395,14 @@ async fn test_stable_ordering_same_second_pagination() { .join("01") .join(format!("rollout-2025-07-01T00-00-00-{u2}.jsonl")); let head = |u: Uuid| -> Vec { - vec![serde_json::json!({"timestamp": ts, "id": u.to_string()})] + vec![serde_json::json!({ + "id": u, + "timestamp": ts, + "instructions": null, + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + })] }; let expected_cursor1: Cursor = serde_json::from_str(&format!("\"{ts}|{u2}\"")).unwrap(); let expected_page1 = ConversationsPage { diff --git a/codex-rs/core/tests/suite/cli_stream.rs b/codex-rs/core/tests/suite/cli_stream.rs index ee4eef7027..716247393a 100644 --- a/codex-rs/core/tests/suite/cli_stream.rs +++ b/codex-rs/core/tests/suite/cli_stream.rs @@ -1,4 +1,5 @@ use assert_cmd::Command as AssertCommand; +use codex_core::RolloutRecorder; use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use std::time::Duration; use std::time::Instant; @@ -77,6 +78,22 @@ async fn chat_mode_stream_cli() { assert_eq!(hi_lines, 1, "Expected exactly one line with 'hi'"); server.verify().await; + + // Verify a new session rollout was created and is discoverable via list_conversations + let page = RolloutRecorder::list_conversations(home.path(), 10, None) + .await + .expect("list conversations"); + assert!( + !page.items.is_empty(), + "expected at least one session to be listed" + ); + // First line of head must be the SessionMeta payload (id/timestamp) + let head0 = page.items[0].head.first().expect("missing head record"); + assert!(head0.get("id").is_some(), "head[0] missing id"); + assert!( + head0.get("timestamp").is_some(), + "head[0] missing timestamp" + ); } /// Verify that passing `-c experimental_instructions_file=...` to the CLI @@ -297,8 +314,10 @@ async fn integration_creates_and_checks_session_file() { Ok(v) => v, Err(_) => continue, }; - if item.get("type").and_then(|t| t.as_str()) == Some("message") - && let Some(c) = item.get("content") + if item.get("type").and_then(|t| t.as_str()) == Some("response_item") + && let Some(payload) = item.get("payload") + && payload.get("type").and_then(|t| t.as_str()) == Some("message") + && let Some(c) = payload.get("content") && c.to_string().contains(&marker) { matching_path = Some(path.to_path_buf()); @@ -361,9 +380,16 @@ async fn integration_creates_and_checks_session_file() { .unwrap_or_else(|_| panic!("missing session meta line")); let meta: serde_json::Value = serde_json::from_str(meta_line) .unwrap_or_else(|_| panic!("Failed to parse session meta line as JSON")); - assert!(meta.get("id").is_some(), "SessionMeta missing id"); + assert_eq!( + meta.get("type").and_then(|v| v.as_str()), + Some("session_meta") + ); + let payload = meta + .get("payload") + .unwrap_or_else(|| panic!("Missing payload in meta line")); + assert!(payload.get("id").is_some(), "SessionMeta missing id"); assert!( - meta.get("timestamp").is_some(), + payload.get("timestamp").is_some(), "SessionMeta missing timestamp" ); @@ -375,8 +401,10 @@ async fn integration_creates_and_checks_session_file() { let Ok(item) = serde_json::from_str::(line) else { continue; }; - if item.get("type").and_then(|t| t.as_str()) == Some("message") - && let Some(c) = item.get("content") + if item.get("type").and_then(|t| t.as_str()) == Some("response_item") + && let Some(payload) = item.get("payload") + && payload.get("type").and_then(|t| t.as_str()) == Some("message") + && let Some(c) = payload.get("content") && c.to_string().contains(&marker) { found_message = true; diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index ed609c2191..32466295e5 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -123,10 +123,22 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { let tmpdir = TempDir::new().unwrap(); let session_path = tmpdir.path().join("resume-session.jsonl"); let mut f = std::fs::File::create(&session_path).unwrap(); + let convo_id = Uuid::new_v4(); writeln!( f, "{}", - json!({"meta":"test","instructions":"be nice", "id": Uuid::new_v4(), "timestamp": "2024-01-01T00:00:00Z"}) + json!({ + "timestamp": "2024-01-01T00:00:00.000Z", + "type": "session_meta", + "payload": { + "id": convo_id, + "timestamp": "2024-01-01T00:00:00Z", + "instructions": "be nice", + "cwd": ".", + "originator": "test_originator", + "cli_version": "test_version" + } + }) ) .unwrap(); @@ -138,7 +150,17 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { text: "resumed user message".to_string(), }], }; - writeln!(f, "{}", serde_json::to_string(&prior_user).unwrap()).unwrap(); + let prior_user_json = serde_json::to_value(&prior_user).unwrap(); + writeln!( + f, + "{}", + json!({ + "timestamp": "2024-01-01T00:00:01.000Z", + "type": "response_item", + "payload": prior_user_json + }) + ) + .unwrap(); // Prior item: system message (excluded from API history) let prior_system = codex_protocol::models::ResponseItem::Message { @@ -148,7 +170,17 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { text: "resumed system instruction".to_string(), }], }; - writeln!(f, "{}", serde_json::to_string(&prior_system).unwrap()).unwrap(); + let prior_system_json = serde_json::to_value(&prior_system).unwrap(); + writeln!( + f, + "{}", + json!({ + "timestamp": "2024-01-01T00:00:02.000Z", + "type": "response_item", + "payload": prior_system_json + }) + ) + .unwrap(); // Prior item: assistant message let prior_item = codex_protocol::models::ResponseItem::Message { @@ -158,7 +190,17 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { text: "resumed assistant message".to_string(), }], }; - writeln!(f, "{}", serde_json::to_string(&prior_item).unwrap()).unwrap(); + let prior_item_json = serde_json::to_value(&prior_item).unwrap(); + writeln!( + f, + "{}", + json!({ + "timestamp": "2024-01-01T00:00:03.000Z", + "type": "response_item", + "payload": prior_item_json + }) + ) + .unwrap(); drop(f); // Mock server that will receive the resumed request @@ -196,16 +238,13 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { .await .expect("create new conversation"); - // 1) Assert initial_messages contains the prior user + assistant messages as EventMsg entries + // 1) Assert initial_messages only includes existing EventMsg entries; response items are not converted let initial_msgs = session_configured .initial_messages .clone() - .expect("expected initial messages for resumed session"); + .expect("expected initial messages option for resumed session"); let initial_json = serde_json::to_value(&initial_msgs).unwrap(); - let expected_initial_json = json!([ - { "type": "user_message", "message": "resumed user message", "kind": "plain" }, - { "type": "agent_message", "message": "resumed assistant message" } - ]); + let expected_initial_json = json!([]); assert_eq!(initial_json, expected_initial_json); // 2) Submit new input; the request body must include the prior item followed by the new user input. diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index fa84bf901b..53e0b9d5a6 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -1251,10 +1251,7 @@ fn extract_conversation_summary( head: &[serde_json::Value], ) -> Option { let session_meta = match head.first() { - Some(first_line) => match serde_json::from_value::(first_line.clone()) { - Ok(session_meta) => session_meta, - Err(..) => return None, - }, + Some(first_line) => serde_json::from_value::(first_line.clone()).ok()?, None => return None, }; @@ -1312,6 +1309,10 @@ mod tests { json!({ "id": conversation_id.0, "timestamp": timestamp, + "cwd": "/", + "originator": "codex", + "cli_version": "0.0.0", + "instructions": null }), json!({ "type": "message", diff --git a/codex-rs/mcp-server/tests/suite/list_resume.rs b/codex-rs/mcp-server/tests/suite/list_resume.rs index eef69c6d96..9b5748cab5 100644 --- a/codex-rs/mcp-server/tests/suite/list_resume.rs +++ b/codex-rs/mcp-server/tests/suite/list_resume.rs @@ -156,14 +156,45 @@ fn create_fake_rollout(codex_home: &Path, filename_ts: &str, meta_rfc3339: &str, let file_path = dir.join(format!("rollout-{filename_ts}-{uuid}.jsonl")); let mut lines = Vec::new(); - // Meta line with timestamp - lines.push(json!({"timestamp": meta_rfc3339, "id": uuid}).to_string()); - // Minimal user message entry as a persisted response item + // Meta line with timestamp (flattened meta in payload for new schema) lines.push( json!({ - "type":"message", - "role":"user", - "content":[{"type":"input_text","text": preview}] + "timestamp": meta_rfc3339, + "type": "session_meta", + "payload": { + "id": uuid, + "timestamp": meta_rfc3339, + "cwd": "/", + "originator": "codex", + "cli_version": "0.0.0", + "instructions": null + } + }) + .to_string(), + ); + // Minimal user message entry as a persisted response item (with envelope timestamp) + lines.push( + json!({ + "timestamp": meta_rfc3339, + "type":"response_item", + "payload": { + "type":"message", + "role":"user", + "content":[{"type":"input_text","text": preview}] + } + }) + .to_string(), + ); + // Add a matching user message event line to satisfy filters + lines.push( + json!({ + "timestamp": meta_rfc3339, + "type":"event_msg", + "payload": { + "type":"user_message", + "message": preview, + "kind": "plain" + } }) .to_string(), ); diff --git a/codex-rs/tui/src/resume_picker.rs b/codex-rs/tui/src/resume_picker.rs index f34a9a5516..8609ab0c4d 100644 --- a/codex-rs/tui/src/resume_picker.rs +++ b/codex-rs/tui/src/resume_picker.rs @@ -2,7 +2,6 @@ use std::path::Path; use std::path::PathBuf; use chrono::DateTime; -use chrono::TimeZone; use chrono::Utc; use codex_core::ConversationItem; use codex_core::ConversationsPage; @@ -255,19 +254,10 @@ impl PickerState { } fn to_rows(page: ConversationsPage) -> Vec { - use std::cmp::Reverse; - let mut rows: Vec = page - .items - .into_iter() - .filter_map(|it| head_to_row(&it)) - .collect(); - // Ensure newest-first ordering within the page by timestamp when available. - let epoch = Utc.timestamp_opt(0, 0).single().unwrap_or_else(Utc::now); - rows.sort_by_key(|r| Reverse(r.ts.unwrap_or(epoch))); - rows + page.items.into_iter().map(|it| head_to_row(&it)).collect() } -fn head_to_row(item: &ConversationItem) -> Option { +fn head_to_row(item: &ConversationItem) -> Row { let mut ts: Option> = None; if let Some(first) = item.head.first() && let Some(t) = first.get("timestamp").and_then(|v| v.as_str()) @@ -276,16 +266,16 @@ fn head_to_row(item: &ConversationItem) -> Option { ts = Some(parsed.with_timezone(&Utc)); } - let preview = preview_from_head(&item.head)?; - let preview = preview.trim().to_string(); - if preview.is_empty() { - return None; - } - Some(Row { + let preview = preview_from_head(&item.head) + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + .unwrap_or_else(|| String::from("(no message yet)")); + + Row { path: item.path.clone(), preview, ts, - }) + } } fn preview_from_head(head: &[serde_json::Value]) -> Option { @@ -483,7 +473,7 @@ mod tests { } #[test] - fn to_rows_sorts_descending_by_timestamp() { + fn to_rows_preserves_backend_order() { // Construct two items with different timestamps and real user text. let a = ConversationItem { path: PathBuf::from("/tmp/a.jsonl"), @@ -500,8 +490,8 @@ mod tests { reached_scan_cap: false, }); assert_eq!(rows.len(), 2); - // Expect the newer timestamp (B) first - assert!(rows[0].preview.contains('B')); - assert!(rows[1].preview.contains('A')); + // Preserve the given order; backend already provides newest-first + assert!(rows[0].preview.contains('A')); + assert!(rows[1].preview.contains('B')); } } From 8636bff46dfce1ad4aee1bca71447796380292dc Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Tue, 9 Sep 2025 19:32:57 -0700 Subject: [PATCH 13/49] Set a user agent suffix when used as a mcp server (#3395) This automatically adds a user agent suffix whenever the CLI is used as a MCP server --- codex-rs/Cargo.lock | 4 + codex-rs/core/src/default_client.rs | 88 ++++++++++++++++++- codex-rs/core/src/mcp_connection_manager.rs | 4 +- codex-rs/mcp-client/src/main.rs | 4 +- codex-rs/mcp-server/Cargo.toml | 1 + codex-rs/mcp-server/src/message_processor.rs | 13 ++- codex-rs/mcp-server/tests/common/Cargo.toml | 2 + .../mcp-server/tests/common/mcp_process.rs | 15 +++- codex-rs/mcp-server/tests/suite/user_agent.rs | 14 ++- codex-rs/mcp-types/src/lib.rs | 16 +++- codex-rs/mcp-types/tests/suite/initialize.rs | 4 +- codex-rs/protocol-ts/Cargo.toml | 1 + codex-rs/protocol-ts/src/lib.rs | 1 + 13 files changed, 148 insertions(+), 19 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index cda2e81d17..d1b9cb391a 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -785,6 +785,7 @@ dependencies = [ "codex-protocol", "mcp-types", "mcp_test_support", + "os_info", "pretty_assertions", "schemars 0.8.22", "serde", @@ -843,6 +844,7 @@ dependencies = [ "anyhow", "clap", "codex-protocol", + "mcp-types", "ts-rs", ] @@ -2680,9 +2682,11 @@ version = "0.0.0" dependencies = [ "anyhow", "assert_cmd", + "codex-core", "codex-mcp-server", "codex-protocol", "mcp-types", + "os_info", "pretty_assertions", "serde", "serde_json", diff --git a/codex-rs/core/src/default_client.rs b/codex-rs/core/src/default_client.rs index a2192ffaeb..36212f2599 100644 --- a/codex-rs/core/src/default_client.rs +++ b/codex-rs/core/src/default_client.rs @@ -1,5 +1,23 @@ use reqwest::header::HeaderValue; use std::sync::LazyLock; +use std::sync::Mutex; + +/// Set this to add a suffix to the User-Agent string. +/// +/// It is not ideal that we're using a global singleton for this. +/// This is primarily designed to differentiate MCP clients from each other. +/// Because there can only be one MCP server per process, it should be safe for this to be a global static. +/// However, future users of this should use this with caution as a result. +/// In addition, we want to be confident that this value is used for ALL clients and doing that requires a +/// lot of wiring and it's easy to miss code paths by doing so. +/// See https://github.com/openai/codex/pull/3388/files for an example of what that would look like. +/// Finally, we want to make sure this is set for ALL mcp clients without needing to know a special env var +/// or having to set data that they already specified in the mcp initialize request somewhere else. +/// +/// A space is automatically added between the suffix and the rest of the User-Agent string. +/// The full user agent string is returned from the mcp initialize response. +/// Parenthesis will be added by Codex. This should only specify what goes inside of the parenthesis. +pub static USER_AGENT_SUFFIX: LazyLock>> = LazyLock::new(|| Mutex::new(None)); pub const CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR: &str = "CODEX_INTERNAL_ORIGINATOR_OVERRIDE"; @@ -32,14 +50,58 @@ pub static ORIGINATOR: LazyLock = LazyLock::new(|| { pub fn get_codex_user_agent() -> String { let build_version = env!("CARGO_PKG_VERSION"); let os_info = os_info::get(); - format!( + let prefix = format!( "{}/{build_version} ({} {}; {}) {}", ORIGINATOR.value.as_str(), os_info.os_type(), os_info.version(), os_info.architecture().unwrap_or("unknown"), crate::terminal::user_agent() - ) + ); + let suffix = USER_AGENT_SUFFIX + .lock() + .ok() + .and_then(|guard| guard.clone()); + let suffix = suffix + .as_deref() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map_or_else(String::new, |value| format!(" ({value})")); + + let candidate = format!("{prefix}{suffix}"); + sanitize_user_agent(candidate, &prefix) +} + +/// Sanitize the user agent string. +/// +/// Invalid characters are replaced with an underscore. +/// +/// If the user agent fails to parse, it falls back to fallback and then to ORIGINATOR. +fn sanitize_user_agent(candidate: String, fallback: &str) -> String { + if HeaderValue::from_str(candidate.as_str()).is_ok() { + return candidate; + } + + let sanitized: String = candidate + .chars() + .map(|ch| if matches!(ch, ' '..='~') { ch } else { '_' }) + .collect(); + if !sanitized.is_empty() && HeaderValue::from_str(sanitized.as_str()).is_ok() { + tracing::warn!( + "Sanitized Codex user agent because provided suffix contained invalid header characters" + ); + sanitized + } else if HeaderValue::from_str(fallback).is_ok() { + tracing::warn!( + "Falling back to base Codex user agent because provided suffix could not be sanitized" + ); + fallback.to_string() + } else { + tracing::warn!( + "Falling back to default Codex originator because base user agent string is invalid" + ); + ORIGINATOR.value.clone() + } } /// Create a reqwest client with default `originator` and `User-Agent` headers set. @@ -114,6 +176,28 @@ mod tests { assert_eq!(ua_header.to_str().unwrap(), expected_ua); } + #[test] + fn test_invalid_suffix_is_sanitized() { + let prefix = "codex_cli_rs/0.0.0"; + let suffix = "bad\rsuffix"; + + assert_eq!( + sanitize_user_agent(format!("{prefix} ({suffix})"), prefix), + "codex_cli_rs/0.0.0 (bad_suffix)" + ); + } + + #[test] + fn test_invalid_suffix_is_sanitized2() { + let prefix = "codex_cli_rs/0.0.0"; + let suffix = "bad\0suffix"; + + assert_eq!( + sanitize_user_agent(format!("{prefix} ({suffix})"), prefix), + "codex_cli_rs/0.0.0 (bad_suffix)" + ); + } + #[test] #[cfg(target_os = "macos")] fn test_macos() { diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 387ea1730c..9fc7275583 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -17,7 +17,7 @@ use anyhow::Result; use anyhow::anyhow; use codex_mcp_client::McpClient; use mcp_types::ClientCapabilities; -use mcp_types::Implementation; +use mcp_types::McpClientInfo; use mcp_types::Tool; use serde_json::json; @@ -159,7 +159,7 @@ impl McpConnectionManager { // indicates this should be an empty object. elicitation: Some(json!({})), }, - client_info: Implementation { + client_info: McpClientInfo { name: "codex-mcp-client".to_owned(), version: env!("CARGO_PKG_VERSION").to_owned(), title: Some("Codex".into()), diff --git a/codex-rs/mcp-client/src/main.rs b/codex-rs/mcp-client/src/main.rs index 10cfe389bf..a9f4e335d1 100644 --- a/codex-rs/mcp-client/src/main.rs +++ b/codex-rs/mcp-client/src/main.rs @@ -17,10 +17,10 @@ use anyhow::Context; use anyhow::Result; use codex_mcp_client::McpClient; use mcp_types::ClientCapabilities; -use mcp_types::Implementation; use mcp_types::InitializeRequestParams; use mcp_types::ListToolsRequestParams; use mcp_types::MCP_SCHEMA_VERSION; +use mcp_types::McpClientInfo; use tracing_subscriber::EnvFilter; #[tokio::main] @@ -60,7 +60,7 @@ async fn main() -> Result<()> { sampling: None, elicitation: None, }, - client_info: Implementation { + client_info: McpClientInfo { name: "codex-mcp-client".to_owned(), version: env!("CARGO_PKG_VERSION").to_owned(), title: Some("Codex".to_string()), diff --git a/codex-rs/mcp-server/Cargo.toml b/codex-rs/mcp-server/Cargo.toml index 335e4ce6d3..c219c72955 100644 --- a/codex-rs/mcp-server/Cargo.toml +++ b/codex-rs/mcp-server/Cargo.toml @@ -41,6 +41,7 @@ uuid = { version = "1", features = ["serde", "v4"] } [dev-dependencies] assert_cmd = "2" mcp_test_support = { path = "tests/common" } +os_info = "3.12.0" pretty_assertions = "1.4.1" tempfile = "3" wiremock = "0.6" diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index dccd86e935..8018095d2a 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -14,6 +14,8 @@ use codex_protocol::mcp_protocol::ConversationId; use codex_core::AuthManager; use codex_core::ConversationManager; use codex_core::config::Config; +use codex_core::default_client::USER_AGENT_SUFFIX; +use codex_core::default_client::get_codex_user_agent; use codex_core::protocol::Submission; use mcp_types::CallToolRequestParams; use mcp_types::CallToolResult; @@ -208,6 +210,14 @@ impl MessageProcessor { return; } + let client_info = params.client_info; + let name = client_info.name; + let version = client_info.version; + let user_agent_suffix = format!("{name}; {version}"); + if let Ok(mut suffix) = USER_AGENT_SUFFIX.lock() { + *suffix = Some(user_agent_suffix); + } + self.initialized = true; // Build a minimal InitializeResult. Fill with placeholders. @@ -224,10 +234,11 @@ impl MessageProcessor { }, instructions: None, protocol_version: params.protocol_version.clone(), - server_info: mcp_types::Implementation { + server_info: mcp_types::McpServerInfo { name: "codex-mcp-server".to_string(), version: env!("CARGO_PKG_VERSION").to_string(), title: Some("Codex".to_string()), + user_agent: get_codex_user_agent(), }, }; diff --git a/codex-rs/mcp-server/tests/common/Cargo.toml b/codex-rs/mcp-server/tests/common/Cargo.toml index 88ad93f573..6bdef423cc 100644 --- a/codex-rs/mcp-server/tests/common/Cargo.toml +++ b/codex-rs/mcp-server/tests/common/Cargo.toml @@ -9,9 +9,11 @@ path = "lib.rs" [dependencies] anyhow = "1" assert_cmd = "2" +codex-core = { path = "../../../core" } codex-mcp-server = { path = "../.." } codex-protocol = { path = "../../../protocol" } mcp-types = { path = "../../../mcp-types" } +os_info = "3.12.0" pretty_assertions = "1.4.1" serde = { version = "1" } serde_json = "1" diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 64f2cc3852..66d546e243 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -26,13 +26,13 @@ use codex_protocol::mcp_protocol::SendUserTurnParams; use mcp_types::CallToolRequestParams; use mcp_types::ClientCapabilities; -use mcp_types::Implementation; use mcp_types::InitializeRequestParams; use mcp_types::JSONRPC_VERSION; use mcp_types::JSONRPCMessage; use mcp_types::JSONRPCNotification; use mcp_types::JSONRPCRequest; use mcp_types::JSONRPCResponse; +use mcp_types::McpClientInfo; use mcp_types::ModelContextProtocolNotification; use mcp_types::ModelContextProtocolRequest; use mcp_types::RequestId; @@ -111,7 +111,7 @@ impl McpProcess { roots: None, sampling: None, }, - client_info: Implementation { + client_info: McpClientInfo { name: "elicitation test".into(), title: Some("Elicitation Test".into()), version: "0.0.0".into(), @@ -129,6 +129,14 @@ impl McpProcess { .await?; let initialized = self.read_jsonrpc_message().await?; + let os_info = os_info::get(); + let user_agent = format!( + "codex_cli_rs/0.0.0 ({} {}; {}) {} (elicitation test; 0.0.0)", + os_info.os_type(), + os_info.version(), + os_info.architecture().unwrap_or("unknown"), + codex_core::terminal::user_agent() + ); assert_eq!( JSONRPCMessage::Response(JSONRPCResponse { jsonrpc: JSONRPC_VERSION.into(), @@ -142,7 +150,8 @@ impl McpProcess { "serverInfo": { "name": "codex-mcp-server", "title": "Codex", - "version": "0.0.0" + "version": "0.0.0", + "user_agent": user_agent }, "protocolVersion": mcp_types::MCP_SCHEMA_VERSION }) diff --git a/codex-rs/mcp-server/tests/suite/user_agent.rs b/codex-rs/mcp-server/tests/suite/user_agent.rs index 19de87a59f..718e145250 100644 --- a/codex-rs/mcp-server/tests/suite/user_agent.rs +++ b/codex-rs/mcp-server/tests/suite/user_agent.rs @@ -1,4 +1,3 @@ -use codex_core::default_client::get_codex_user_agent; use codex_protocol::mcp_protocol::GetUserAgentResponse; use mcp_test_support::McpProcess; use mcp_test_support::to_response; @@ -34,11 +33,18 @@ async fn get_user_agent_returns_current_codex_user_agent() { .expect("getUserAgent timeout") .expect("getUserAgent response"); + let os_info = os_info::get(); + let user_agent = format!( + "codex_cli_rs/0.0.0 ({} {}; {}) {} (elicitation test; 0.0.0)", + os_info.os_type(), + os_info.version(), + os_info.architecture().unwrap_or("unknown"), + codex_core::terminal::user_agent() + ); + let received: GetUserAgentResponse = to_response(response).expect("deserialize getUserAgent response"); - let expected = GetUserAgentResponse { - user_agent: get_codex_user_agent(), - }; + let expected = GetUserAgentResponse { user_agent }; assert_eq!(received, expected); } diff --git a/codex-rs/mcp-types/src/lib.rs b/codex-rs/mcp-types/src/lib.rs index 988bec2ade..2f862be897 100644 --- a/codex-rs/mcp-types/src/lib.rs +++ b/codex-rs/mcp-types/src/lib.rs @@ -482,13 +482,23 @@ pub struct ImageContent { /// Describes the name and version of an MCP implementation, with an optional title for UI representation. #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, TS)] -pub struct Implementation { +pub struct McpClientInfo { pub name: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub title: Option, pub version: String, } +/// Describes the name and version of an MCP implementation, with an optional title for UI representation. +#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, TS)] +pub struct McpServerInfo { + pub name: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub title: Option, + pub version: String, + pub user_agent: String, +} + #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, TS)] pub enum InitializeRequest {} @@ -502,7 +512,7 @@ impl ModelContextProtocolRequest for InitializeRequest { pub struct InitializeRequestParams { pub capabilities: ClientCapabilities, #[serde(rename = "clientInfo")] - pub client_info: Implementation, + pub client_info: McpClientInfo, #[serde(rename = "protocolVersion")] pub protocol_version: String, } @@ -516,7 +526,7 @@ pub struct InitializeResult { #[serde(rename = "protocolVersion")] pub protocol_version: String, #[serde(rename = "serverInfo")] - pub server_info: Implementation, + pub server_info: McpServerInfo, } impl From for serde_json::Value { diff --git a/codex-rs/mcp-types/tests/suite/initialize.rs b/codex-rs/mcp-types/tests/suite/initialize.rs index 04778f2a90..2bd3c789c9 100644 --- a/codex-rs/mcp-types/tests/suite/initialize.rs +++ b/codex-rs/mcp-types/tests/suite/initialize.rs @@ -1,10 +1,10 @@ use mcp_types::ClientCapabilities; use mcp_types::ClientRequest; -use mcp_types::Implementation; use mcp_types::InitializeRequestParams; use mcp_types::JSONRPC_VERSION; use mcp_types::JSONRPCMessage; use mcp_types::JSONRPCRequest; +use mcp_types::McpClientInfo; use mcp_types::RequestId; use serde_json::json; @@ -58,7 +58,7 @@ fn deserialize_initialize_request() { sampling: None, elicitation: None, }, - client_info: Implementation { + client_info: McpClientInfo { name: "acme-client".into(), title: Some("Acme".to_string()), version: "1.2.3".into(), diff --git a/codex-rs/protocol-ts/Cargo.toml b/codex-rs/protocol-ts/Cargo.toml index 9faa9344e9..1131a62196 100644 --- a/codex-rs/protocol-ts/Cargo.toml +++ b/codex-rs/protocol-ts/Cargo.toml @@ -16,6 +16,7 @@ path = "src/main.rs" [dependencies] anyhow = "1" +mcp-types = { path = "../mcp-types" } codex-protocol = { path = "../protocol" } ts-rs = "11" clap = { version = "4", features = ["derive"] } diff --git a/codex-rs/protocol-ts/src/lib.rs b/codex-rs/protocol-ts/src/lib.rs index 08cb940784..776c8ba370 100644 --- a/codex-rs/protocol-ts/src/lib.rs +++ b/codex-rs/protocol-ts/src/lib.rs @@ -16,6 +16,7 @@ pub fn generate_ts(out_dir: &Path, prettier: Option<&Path>) -> Result<()> { ensure_dir(out_dir)?; // Generate TS bindings + mcp_types::InitializeResult::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ConversationId::export_all_to(out_dir)?; codex_protocol::mcp_protocol::InputItem::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ClientRequest::export_all_to(out_dir)?; From c13c3dadbf68d5369f24a20594cedd6b3973516b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 10 Sep 2025 08:19:05 -0700 Subject: [PATCH 14/49] fix: remove unnecessary #[allow(dead_code)] annotation (#3357) --- codex-rs/core/src/rollout/recorder.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/codex-rs/core/src/rollout/recorder.rs b/codex-rs/core/src/rollout/recorder.rs index 9954c4b1ca..5451af0dfe 100644 --- a/codex-rs/core/src/rollout/recorder.rs +++ b/codex-rs/core/src/rollout/recorder.rs @@ -125,7 +125,6 @@ impl RolloutRecorderParams { } impl RolloutRecorder { - #[allow(dead_code)] /// List conversations (rollout files) under the provided Codex home directory. pub async fn list_conversations( codex_home: &Path, From 45bd5ca4b90c636b0717178cfdcb862a3f145773 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 10 Sep 2025 10:17:24 -0700 Subject: [PATCH 15/49] Move initial history to protocol (#3422) To fix an edge case of forking then resuming #3419 --- codex-rs/core/src/codex.rs | 4 +- codex-rs/core/src/conversation_manager.rs | 71 +------------- codex-rs/core/src/git_info.rs | 14 +-- codex-rs/core/src/lib.rs | 2 +- codex-rs/core/src/rollout/list.rs | 4 +- codex-rs/core/src/rollout/mod.rs | 2 +- codex-rs/core/src/rollout/policy.rs | 2 +- codex-rs/core/src/rollout/recorder.rs | 43 ++------ codex-rs/core/tests/suite/cli_stream.rs | 3 +- codex-rs/protocol/src/protocol.rs | 114 ++++++++++++++++++++++ 10 files changed, 132 insertions(+), 127 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5bd5c50413..dcdad6c1d1 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -10,7 +10,6 @@ use std::time::Duration; use crate::AuthManager; use crate::event_mapping::map_response_item_to_event_messages; -use crate::rollout::recorder::RolloutItem; use async_channel::Receiver; use async_channel::Sender; use codex_apply_patch::ApplyPatchAction; @@ -18,6 +17,7 @@ use codex_apply_patch::MaybeApplyPatchVerified; use codex_apply_patch::maybe_parse_apply_patch_verified; use codex_protocol::mcp_protocol::ConversationId; use codex_protocol::protocol::ConversationHistoryResponseEvent; +use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::TaskStartedEvent; use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::TurnAbortedEvent; @@ -45,7 +45,6 @@ use crate::client_common::ResponseEvent; use crate::config::Config; use crate::config_types::ShellEnvironmentPolicy; use crate::conversation_history::ConversationHistory; -use crate::conversation_manager::InitialHistory; use crate::environment_context::EnvironmentContext; use crate::error::CodexErr; use crate::error::Result as CodexResult; @@ -122,6 +121,7 @@ use codex_protocol::models::LocalShellAction; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; use codex_protocol::models::ShellToolCallParams; +use codex_protocol::protocol::InitialHistory; // A convenience extension trait for acquiring mutex locks where poisoning is // unrecoverable and should abort the program. This avoids scattered `.unwrap()` diff --git a/codex-rs/core/src/conversation_manager.rs b/codex-rs/core/src/conversation_manager.rs index e352680ca8..c0695f16f3 100644 --- a/codex-rs/core/src/conversation_manager.rs +++ b/codex-rs/core/src/conversation_manager.rs @@ -11,82 +11,15 @@ use crate::protocol::Event; use crate::protocol::EventMsg; use crate::protocol::SessionConfiguredEvent; use crate::rollout::RolloutRecorder; -use crate::rollout::recorder::RolloutItem; use codex_protocol::mcp_protocol::ConversationId; use codex_protocol::models::ResponseItem; +use codex_protocol::protocol::InitialHistory; +use codex_protocol::protocol::RolloutItem; use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; use tokio::sync::RwLock; -#[derive(Debug, Clone)] -pub struct ResumedHistory { - pub conversation_id: ConversationId, - pub history: Vec, - pub rollout_path: PathBuf, -} - -#[derive(Debug, Clone)] -pub enum InitialHistory { - New, - Resumed(ResumedHistory), - Forked(Vec), -} - -impl InitialHistory { - pub(crate) fn get_rollout_items(&self) -> Vec { - match self { - InitialHistory::New => Vec::new(), - InitialHistory::Resumed(resumed) => resumed.history.clone(), - InitialHistory::Forked(items) => items.clone(), - } - } - pub fn get_response_items(&self) -> Vec { - match self { - InitialHistory::New => Vec::new(), - InitialHistory::Resumed(resumed) => resumed - .history - .iter() - .filter_map(|ri| match ri { - RolloutItem::ResponseItem(item) => Some(item.clone()), - _ => None, - }) - .collect(), - InitialHistory::Forked(items) => items - .iter() - .filter_map(|ri| match ri { - RolloutItem::ResponseItem(item) => Some(item.clone()), - _ => None, - }) - .collect(), - } - } - pub fn get_event_msgs(&self) -> Option> { - match self { - InitialHistory::New => None, - InitialHistory::Resumed(resumed) => Some( - resumed - .history - .iter() - .filter_map(|ri| match ri { - RolloutItem::EventMsg(ev) => Some(ev.clone()), - _ => None, - }) - .collect(), - ), - InitialHistory::Forked(items) => Some( - items - .iter() - .filter_map(|ri| match ri { - RolloutItem::EventMsg(ev) => Some(ev.clone()), - _ => None, - }) - .collect(), - ), - } - } -} - /// Represents a newly created Codex conversation, including the first event /// (which is [`EventMsg::SessionConfigured`]). pub struct NewConversation { diff --git a/codex-rs/core/src/git_info.rs b/codex-rs/core/src/git_info.rs index 79a8647a2b..619f4f52de 100644 --- a/codex-rs/core/src/git_info.rs +++ b/codex-rs/core/src/git_info.rs @@ -3,6 +3,7 @@ use std::path::Path; use std::path::PathBuf; use codex_protocol::mcp_protocol::GitSha; +use codex_protocol::protocol::GitInfo; use futures::future::join_all; use serde::Deserialize; use serde::Serialize; @@ -43,19 +44,6 @@ pub fn get_git_repo_root(base_dir: &Path) -> Option { /// Timeout for git commands to prevent freezing on large repositories const GIT_COMMAND_TIMEOUT: TokioDuration = TokioDuration::from_secs(5); -#[derive(Serialize, Deserialize, Clone, Debug)] -pub struct GitInfo { - /// Current commit hash (SHA) - #[serde(skip_serializing_if = "Option::is_none")] - pub commit_hash: Option, - /// Current branch name - #[serde(skip_serializing_if = "Option::is_none")] - pub branch: Option, - /// Repository URL (if available from remote) - #[serde(skip_serializing_if = "Option::is_none")] - pub repository_url: Option, -} - #[derive(Serialize, Deserialize, Clone, Debug)] pub struct GitDiffToRemote { pub sha: GitSha, diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 352bd48277..c053153086 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -42,8 +42,8 @@ pub use model_provider_info::built_in_model_providers; pub use model_provider_info::create_oss_provider_with_base_url; mod conversation_manager; mod event_mapping; +pub use codex_protocol::protocol::InitialHistory; pub use conversation_manager::ConversationManager; -pub use conversation_manager::InitialHistory; pub use conversation_manager::NewConversation; // Re-export common auth types for workspace consumers pub use auth::AuthManager; diff --git a/codex-rs/core/src/rollout/list.rs b/codex-rs/core/src/rollout/list.rs index c4feb62958..417638e781 100644 --- a/codex-rs/core/src/rollout/list.rs +++ b/codex-rs/core/src/rollout/list.rs @@ -10,9 +10,9 @@ use time::macros::format_description; use uuid::Uuid; use super::SESSIONS_SUBDIR; -use super::recorder::RolloutItem; -use super::recorder::RolloutLine; use crate::protocol::EventMsg; +use codex_protocol::protocol::RolloutItem; +use codex_protocol::protocol::RolloutLine; /// Returned page of conversation summaries. #[derive(Debug, Default, PartialEq)] diff --git a/codex-rs/core/src/rollout/mod.rs b/codex-rs/core/src/rollout/mod.rs index 341d39ae29..6bf1cf9429 100644 --- a/codex-rs/core/src/rollout/mod.rs +++ b/codex-rs/core/src/rollout/mod.rs @@ -7,9 +7,9 @@ pub mod list; pub(crate) mod policy; pub mod recorder; +pub use codex_protocol::protocol::SessionMeta; pub use recorder::RolloutRecorder; pub use recorder::RolloutRecorderParams; -pub use recorder::SessionMeta; #[cfg(test)] pub mod tests; diff --git a/codex-rs/core/src/rollout/policy.rs b/codex-rs/core/src/rollout/policy.rs index 676f8e908d..7c58260479 100644 --- a/codex-rs/core/src/rollout/policy.rs +++ b/codex-rs/core/src/rollout/policy.rs @@ -1,5 +1,5 @@ use crate::protocol::EventMsg; -use crate::rollout::recorder::RolloutItem; +use crate::protocol::RolloutItem; use codex_protocol::models::ResponseItem; /// Whether a rollout `item` should be persisted in rollout files. diff --git a/codex-rs/core/src/rollout/recorder.rs b/codex-rs/core/src/rollout/recorder.rs index 5451af0dfe..f5609f7206 100644 --- a/codex-rs/core/src/rollout/recorder.rs +++ b/codex-rs/core/src/rollout/recorder.rs @@ -26,46 +26,15 @@ use super::list::Cursor; use super::list::get_conversations; use super::policy::is_persisted_response_item; use crate::config::Config; -use crate::conversation_manager::InitialHistory; -use crate::conversation_manager::ResumedHistory; use crate::default_client::ORIGINATOR; -use crate::git_info::GitInfo; use crate::git_info::collect_git_info; -use crate::protocol::EventMsg; use codex_protocol::models::ResponseItem; - -#[derive(Serialize, Deserialize, Clone, Default, Debug)] -pub struct SessionMeta { - pub id: ConversationId, - pub timestamp: String, - pub cwd: PathBuf, - pub originator: String, - pub cli_version: String, - pub instructions: Option, -} - -#[derive(Serialize, Deserialize, Debug, Clone)] -pub struct SessionMetaLine { - #[serde(flatten)] - meta: SessionMeta, - #[serde(skip_serializing_if = "Option::is_none")] - git: Option, -} - -#[derive(Serialize, Deserialize, Debug, Clone)] -#[serde(tag = "type", content = "payload", rename_all = "snake_case")] -pub enum RolloutItem { - SessionMeta(SessionMetaLine), - ResponseItem(ResponseItem), - EventMsg(EventMsg), -} - -#[derive(Serialize, Deserialize, Clone)] -pub(crate) struct RolloutLine { - pub(crate) timestamp: String, - #[serde(flatten)] - pub(crate) item: RolloutItem, -} +use codex_protocol::protocol::InitialHistory; +use codex_protocol::protocol::ResumedHistory; +use codex_protocol::protocol::RolloutItem; +use codex_protocol::protocol::RolloutLine; +use codex_protocol::protocol::SessionMeta; +use codex_protocol::protocol::SessionMetaLine; #[derive(Serialize, Deserialize, Default, Clone)] pub struct SessionStateSnapshot {} diff --git a/codex-rs/core/tests/suite/cli_stream.rs b/codex-rs/core/tests/suite/cli_stream.rs index 716247393a..01b9d6bfc1 100644 --- a/codex-rs/core/tests/suite/cli_stream.rs +++ b/codex-rs/core/tests/suite/cli_stream.rs @@ -1,5 +1,6 @@ use assert_cmd::Command as AssertCommand; use codex_core::RolloutRecorder; +use codex_core::protocol::GitInfo; use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use std::time::Duration; use std::time::Instant; @@ -617,7 +618,7 @@ async fn integration_git_info_unit_test() { // 5. Test serialization to ensure it works in SessionMeta let serialized = serde_json::to_string(&git_info).unwrap(); - let deserialized: codex_core::git_info::GitInfo = serde_json::from_str(&serialized).unwrap(); + let deserialized: GitInfo = serde_json::from_str(&serialized).unwrap(); assert_eq!(git_info.commit_hash, deserialized.commit_hash); assert_eq!(git_info.branch, deserialized.branch); diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 2f1c46364a..860bb59f91 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -804,6 +804,120 @@ pub struct ConversationHistoryResponseEvent { pub entries: Vec, } +#[derive(Debug, Clone, Deserialize, Serialize, TS)] +pub struct ResumedHistory { + pub conversation_id: ConversationId, + pub history: Vec, + pub rollout_path: PathBuf, +} + +#[derive(Debug, Clone, Deserialize, Serialize, TS)] +pub enum InitialHistory { + New, + Resumed(ResumedHistory), + Forked(Vec), +} + +impl InitialHistory { + pub fn get_rollout_items(&self) -> Vec { + match self { + InitialHistory::New => Vec::new(), + InitialHistory::Resumed(resumed) => resumed.history.clone(), + InitialHistory::Forked(items) => items.clone(), + } + } + pub fn get_response_items(&self) -> Vec { + match self { + InitialHistory::New => Vec::new(), + InitialHistory::Resumed(resumed) => resumed + .history + .iter() + .filter_map(|ri| match ri { + RolloutItem::ResponseItem(item) => Some(item.clone()), + _ => None, + }) + .collect(), + InitialHistory::Forked(items) => items + .iter() + .filter_map(|ri| match ri { + RolloutItem::ResponseItem(item) => Some(item.clone()), + _ => None, + }) + .collect(), + } + } + pub fn get_event_msgs(&self) -> Option> { + match self { + InitialHistory::New => None, + InitialHistory::Resumed(resumed) => Some( + resumed + .history + .iter() + .filter_map(|ri| match ri { + RolloutItem::EventMsg(ev) => Some(ev.clone()), + _ => None, + }) + .collect(), + ), + InitialHistory::Forked(items) => Some( + items + .iter() + .filter_map(|ri| match ri { + RolloutItem::EventMsg(ev) => Some(ev.clone()), + _ => None, + }) + .collect(), + ), + } + } +} + +#[derive(Serialize, Deserialize, Clone, Default, Debug, TS)] +pub struct SessionMeta { + pub id: ConversationId, + pub timestamp: String, + pub cwd: PathBuf, + pub originator: String, + pub cli_version: String, + pub instructions: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, TS)] +pub struct SessionMetaLine { + #[serde(flatten)] + pub meta: SessionMeta, + #[serde(skip_serializing_if = "Option::is_none")] + pub git: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, TS)] +#[serde(tag = "type", content = "payload", rename_all = "snake_case")] +pub enum RolloutItem { + SessionMeta(SessionMetaLine), + ResponseItem(ResponseItem), + EventMsg(EventMsg), +} + +#[derive(Serialize, Deserialize, Clone)] +pub struct RolloutLine { + pub timestamp: String, + #[serde(flatten)] + pub item: RolloutItem, +} + +#[derive(Serialize, Deserialize, Clone, Debug, TS)] +pub struct GitInfo { + /// Current commit hash (SHA) + #[serde(skip_serializing_if = "Option::is_none")] + pub commit_hash: Option, + /// Current branch name + #[serde(skip_serializing_if = "Option::is_none")] + pub branch: Option, + /// Repository URL (if available from remote) + #[serde(skip_serializing_if = "Option::is_none")] + pub repository_url: Option, +} + #[derive(Debug, Clone, Deserialize, Serialize, TS)] pub struct ExecCommandBeginEvent { /// Identifier so this can be paired with the ExecCommandEnd event. From 39db113cc95cf864ae07b966d07132579df05788 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 10 Sep 2025 10:18:43 -0700 Subject: [PATCH 16/49] Added images to `UserMessageEvent` (#3400) This PR adds an `images` field to the existing `UserMessageEvent` so we can encode zero or more images associated with a user message. This allows images to be restored when conversations are restored. --- codex-rs/core/src/event_mapping.rs | 115 +++++++++++++++++++++------ codex-rs/protocol/src/protocol.rs | 2 + codex-rs/tui/src/chatwidget/tests.rs | 1 + 3 files changed, 95 insertions(+), 23 deletions(-) diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index c9ad8a16b8..2628de4281 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -25,31 +25,56 @@ pub(crate) fn map_response_item_to_event_messages( return Vec::new(); } - let events: Vec = content - .iter() - .filter_map(|content_item| match content_item { - ContentItem::OutputText { text } => { - Some(EventMsg::AgentMessage(AgentMessageEvent { - message: text.clone(), - })) - } + let mut events: Vec = Vec::new(); + let mut message_parts: Vec = Vec::new(); + let mut images: Vec = Vec::new(); + let mut kind: Option = None; + + for content_item in content.iter() { + match content_item { ContentItem::InputText { text } => { - let trimmed = text.trim_start(); - let kind = if trimmed.starts_with("") { - Some(InputMessageKind::EnvironmentContext) - } else if trimmed.starts_with("") { - Some(InputMessageKind::UserInstructions) - } else { - Some(InputMessageKind::Plain) - }; - Some(EventMsg::UserMessage(UserMessageEvent { - message: text.clone(), - kind, - })) + if kind.is_none() { + let trimmed = text.trim_start(); + kind = if trimmed.starts_with("") { + Some(InputMessageKind::EnvironmentContext) + } else if trimmed.starts_with("") { + Some(InputMessageKind::UserInstructions) + } else { + Some(InputMessageKind::Plain) + }; + } + message_parts.push(text.clone()); } - _ => None, - }) - .collect(); + ContentItem::InputImage { image_url } => { + images.push(image_url.clone()); + } + ContentItem::OutputText { text } => { + events.push(EventMsg::AgentMessage(AgentMessageEvent { + message: text.clone(), + })); + } + } + } + + if !message_parts.is_empty() || !images.is_empty() { + let message = if message_parts.is_empty() { + String::new() + } else { + message_parts.join("") + }; + let images = if images.is_empty() { + None + } else { + Some(images) + }; + + events.push(EventMsg::UserMessage(UserMessageEvent { + message, + kind, + images, + })); + } + events } @@ -96,3 +121,47 @@ pub(crate) fn map_response_item_to_event_messages( | ResponseItem::Other => Vec::new(), } } + +#[cfg(test)] +mod tests { + use super::map_response_item_to_event_messages; + use crate::protocol::EventMsg; + use crate::protocol::InputMessageKind; + use codex_protocol::models::ContentItem; + use codex_protocol::models::ResponseItem; + use pretty_assertions::assert_eq; + + #[test] + fn maps_user_message_with_text_and_two_images() { + let img1 = "https://example.com/one.png".to_string(); + let img2 = "https://example.com/two.jpg".to_string(); + + let item = ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ + ContentItem::InputText { + text: "Hello world".to_string(), + }, + ContentItem::InputImage { + image_url: img1.clone(), + }, + ContentItem::InputImage { + image_url: img2.clone(), + }, + ], + }; + + let events = map_response_item_to_event_messages(&item, false); + assert_eq!(events.len(), 1, "expected a single user message event"); + + match &events[0] { + EventMsg::UserMessage(user) => { + assert_eq!(user.message, "Hello world"); + assert!(matches!(user.kind, Some(InputMessageKind::Plain))); + assert_eq!(user.images, Some(vec![img1.clone(), img2.clone()])); + } + other => panic!("expected UserMessage, got {other:?}"), + } + } +} diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 860bb59f91..7e358f12e2 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -695,6 +695,8 @@ pub struct UserMessageEvent { pub message: String, #[serde(skip_serializing_if = "Option::is_none")] pub kind: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub images: Option>, } impl From<(T, U)> for InputMessageKind diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 113864dba7..e097c2014e 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -144,6 +144,7 @@ fn resumed_initial_messages_render_history() { EventMsg::UserMessage(UserMessageEvent { message: "hello from user".to_string(), kind: Some(InputMessageKind::Plain), + images: None, }), EventMsg::AgentMessage(AgentMessageEvent { message: "assistant reply".to_string(), From 64e6c4afbbcb154d331d64a682b5b641c8007271 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 10 Sep 2025 10:35:24 -0700 Subject: [PATCH 17/49] fix: remove empty file: chatwidget_stream_tests.rs (#3356) Originally added in https://github.com/openai/codex/pull/2029. --- codex-rs/tui/src/chatwidget_stream_tests.rs | 1 - codex-rs/tui/src/lib.rs | 5 ----- 2 files changed, 6 deletions(-) delete mode 100644 codex-rs/tui/src/chatwidget_stream_tests.rs diff --git a/codex-rs/tui/src/chatwidget_stream_tests.rs b/codex-rs/tui/src/chatwidget_stream_tests.rs deleted file mode 100644 index 8b13789179..0000000000 --- a/codex-rs/tui/src/chatwidget_stream_tests.rs +++ /dev/null @@ -1 +0,0 @@ - diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 549037462c..5f002e53db 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -61,11 +61,6 @@ mod user_approval_widget; mod version; mod wrapping; -// Internal vt100-based replay tests live as a separate source file to keep them -// close to the widget code. Include them in unit tests. -#[cfg(test)] -mod chatwidget_stream_tests; - #[cfg(not(debug_assertions))] mod updates; From 5200b7a95ddaa6a2d4a536f21b2cd58911fd5498 Mon Sep 17 00:00:00 2001 From: katyhshi <115659651+katyhshi@users.noreply.github.com> Date: Wed, 10 Sep 2025 10:39:53 -0700 Subject: [PATCH 18/49] docs: fix codex exec heading typo (#2703) # 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. --- codex-rs/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/README.md b/codex-rs/README.md index 390f5d31aa..043d872afe 100644 --- a/codex-rs/README.md +++ b/codex-rs/README.md @@ -35,7 +35,7 @@ npx @modelcontextprotocol/inspector codex mcp You can enable notifications by configuring a script that is run whenever the agent finishes a turn. The [notify documentation](../docs/config.md#notify) includes a detailed example that explains how to get desktop notifications via [terminal-notifier](https://github.com/julienXX/terminal-notifier) on macOS. -### `codex exec` to run Codex programmatially/non-interactively +### `codex exec` to run Codex programmatically/non-interactively To run Codex non-interactively, run `codex exec PROMPT` (you can also pass the prompt via `stdin`) and Codex will work on your task until it decides that it is done and exits. Output is printed to the terminal directly. You can set the `RUST_LOG` environment variable to see more about what's going on. From 97338de5780e0f11b89cf5c3a51ddc4e1c2e75eb Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Thu, 11 Sep 2025 02:52:50 +0900 Subject: [PATCH 19/49] Remove a broken link to prompting_guide.md in docs/getting-started.md (#2858) The file no longer exists. We've been receiving this feedback several times. - https://github.com/openai/codex/issues/2374 - https://github.com/openai/codex/issues/2810 - https://github.com/openai/codex/issues/2826 My previous PR https://github.com/openai/codex/pull/2413 for this issue restored the file but now it's compatible with the current file structure. Thus, let's simply delete the link. --- docs/getting-started.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/getting-started.md b/docs/getting-started.md index 3f7473fd33..cc7dd1293c 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -42,7 +42,7 @@ they'll be committed to your working directory. ### Example prompts -Below are a few bite-size examples you can copy-paste. Replace the text in quotes with your own task. See the [prompting guide](https://github.com/openai/codex/blob/main/codex-cli/examples/prompting_guide.md) for more tips and usage patterns. +Below are a few bite-size examples you can copy-paste. Replace the text in quotes with your own task. | ✨ | What you type | What happens | | --- | ------------------------------------------------------------------------------- | -------------------------------------------------------------------------- | From acb28bf91402b600e0735f25b151cd87b89636e3 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 10 Sep 2025 11:46:02 -0700 Subject: [PATCH 20/49] Improved resiliency of two auth-related tests (#3427) This PR improves two existing auth-related tests. They were failing when run in an environment where an `OPENAI_API_KEY` env variable was defined. The change makes them more resilient. --- .../mcp-server/tests/common/mcp_process.rs | 23 +++++++++++++++++++ codex-rs/mcp-server/tests/suite/auth.rs | 2 +- codex-rs/mcp-server/tests/suite/login.rs | 2 +- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 66d546e243..2deb5f612d 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -54,6 +54,18 @@ pub struct McpProcess { impl McpProcess { pub async fn new(codex_home: &Path) -> anyhow::Result { + Self::new_with_env(codex_home, &[]).await + } + + /// Creates a new MCP process, allowing tests to override or remove + /// specific environment variables for the child process only. + /// + /// Pass a tuple of (key, Some(value)) to set/override, or (key, None) to + /// remove a variable from the child's environment. + pub async fn new_with_env( + codex_home: &Path, + env_overrides: &[(&str, Option<&str>)], + ) -> anyhow::Result { // Use assert_cmd to locate the binary path and then switch to tokio::process::Command let std_cmd = StdCommand::cargo_bin("codex-mcp-server") .context("should find binary for codex-mcp-server")?; @@ -68,6 +80,17 @@ impl McpProcess { cmd.env("CODEX_HOME", codex_home); cmd.env("RUST_LOG", "debug"); + for (k, v) in env_overrides { + match v { + Some(val) => { + cmd.env(k, val); + } + None => { + cmd.env_remove(k); + } + } + } + let mut process = cmd .kill_on_drop(true) .spawn() diff --git a/codex-rs/mcp-server/tests/suite/auth.rs b/codex-rs/mcp-server/tests/suite/auth.rs index 3bf972842a..415392780f 100644 --- a/codex-rs/mcp-server/tests/suite/auth.rs +++ b/codex-rs/mcp-server/tests/suite/auth.rs @@ -41,7 +41,7 @@ async fn get_auth_status_no_auth() { let codex_home = TempDir::new().unwrap_or_else(|e| panic!("create tempdir: {e}")); create_config_toml(codex_home.path()).expect("write config.toml"); - let mut mcp = McpProcess::new(codex_home.path()) + let mut mcp = McpProcess::new_with_env(codex_home.path(), &[("OPENAI_API_KEY", None)]) .await .expect("spawn mcp process"); timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()) diff --git a/codex-rs/mcp-server/tests/suite/login.rs b/codex-rs/mcp-server/tests/suite/login.rs index ba5021c1fa..bbc0558777 100644 --- a/codex-rs/mcp-server/tests/suite/login.rs +++ b/codex-rs/mcp-server/tests/suite/login.rs @@ -46,7 +46,7 @@ async fn logout_chatgpt_removes_auth() { login_with_api_key(codex_home.path(), "sk-test-key").expect("seed api key"); assert!(codex_home.path().join("auth.json").exists()); - let mut mcp = McpProcess::new(codex_home.path()) + let mut mcp = McpProcess::new_with_env(codex_home.path(), &[("OPENAI_API_KEY", None)]) .await .expect("spawn mcp process"); timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()) From 8068cc75f8ad6d71c0c35b4b3109633b6edb7269 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Wed, 10 Sep 2025 12:13:53 -0700 Subject: [PATCH 21/49] replace tui_markdown with a custom markdown renderer (#3396) Also, simplify the streaming behavior. This fixes a number of display issues with streaming markdown, and paves the way for better markdown features (e.g. customizable styles, syntax highlighting, markdown-aware wrapping). Not currently supported: - footnotes - tables - reference-style links --- codex-rs/Cargo.lock | 189 +--- codex-rs/tui/Cargo.toml | 2 +- codex-rs/tui/src/bin/md-events.rs | 15 + ...t_markdown_code_blocks_vt100_snapshot.snap | 18 + codex-rs/tui/src/chatwidget/tests.rs | 120 +++ codex-rs/tui/src/insert_history.rs | 252 ++++- codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/markdown.rs | 461 +------- codex-rs/tui/src/markdown_render.rs | 566 ++++++++++ codex-rs/tui/src/markdown_render_tests.rs | 995 ++++++++++++++++++ codex-rs/tui/src/markdown_stream.rs | 502 ++++----- codex-rs/tui/src/render/markdown_utils.rs | 72 -- codex-rs/tui/src/render/mod.rs | 1 - ...sts__markdown_render_complex_snapshot.snap | 62 ++ codex-rs/tui/src/streaming/controller.rs | 14 +- codex-rs/tui/src/wrapping.rs | 22 +- 16 files changed, 2309 insertions(+), 983 deletions(-) create mode 100644 codex-rs/tui/src/bin/md-events.rs create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_markdown_code_blocks_vt100_snapshot.snap create mode 100644 codex-rs/tui/src/markdown_render.rs create mode 100644 codex-rs/tui/src/markdown_render_tests.rs delete mode 100644 codex-rs/tui/src/render/markdown_utils.rs create mode 100644 codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__markdown_render_complex_snapshot.snap diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index d1b9cb391a..7bf6c2a7a6 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -311,15 +311,6 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a8241f3ebb85c056b509d4327ad0358fbbba6ffb340bf388f26350aeda225b1" -[[package]] -name = "bincode" -version = "1.3.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b1f45e9417d87227c7a56d22e471c6206462cba514c7590c09aff4cf6d1ddcad" -dependencies = [ - "serde", -] - [[package]] name = "bit-set" version = "0.5.3" @@ -655,7 +646,7 @@ dependencies = [ "tokio-test", "tokio-util", "toml", - "toml_edit 0.23.4", + "toml_edit", "tracing", "tree-sitter", "tree-sitter-bash", @@ -879,6 +870,7 @@ dependencies = [ "path-clean", "pathdiff", "pretty_assertions", + "pulldown-cmark", "rand 0.9.2", "ratatui", "regex-lite", @@ -895,7 +887,6 @@ dependencies = [ "tracing", "tracing-appender", "tracing-subscriber", - "tui-markdown", "unicode-segmentation", "unicode-width 0.1.14", "url", @@ -1763,12 +1754,6 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" -[[package]] -name = "futures-timer" -version = "3.0.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" - [[package]] name = "futures-util" version = "0.3.31" @@ -1854,12 +1839,6 @@ version = "0.31.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" -[[package]] -name = "glob" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a8d1add55171497b4705a648c6b583acafb01d58050a51727785f0b2c8e0a2b2" - [[package]] name = "globset" version = "0.4.16" @@ -2567,12 +2546,6 @@ dependencies = [ "libc", ] -[[package]] -name = "linked-hash-map" -version = "0.5.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f" - [[package]] name = "linux-raw-sys" version = "0.4.15" @@ -3014,28 +2987,6 @@ version = "1.70.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a4895175b425cb1f87721b59f0f286c2092bd4af812243672510e1ac53e2e0ad" -[[package]] -name = "onig" -version = "6.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "336b9c63443aceef14bea841b899035ae3abe89b7c486aaf4c5bd8aafedac3f0" -dependencies = [ - "bitflags 2.9.1", - "libc", - "once_cell", - "onig_sys", -] - -[[package]] -name = "onig_sys" -version = "69.9.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c7f86c6eef3d6df15f23bcfb6af487cbd2fed4e5581d58d5bf1f5f8b7f6727dc" -dependencies = [ - "cc", - "pkg-config", -] - [[package]] name = "openssl" version = "0.10.73" @@ -3361,15 +3312,6 @@ dependencies = [ "yansi", ] -[[package]] -name = "proc-macro-crate" -version = "3.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "edce586971a4dfaa28950c6f18ed55e0406c1ab88bbce2c6f6293a7aaba73d35" -dependencies = [ - "toml_edit 0.22.27", -] - [[package]] name = "proc-macro2" version = "1.0.95" @@ -3381,9 +3323,9 @@ dependencies = [ [[package]] name = "pulldown-cmark" -version = "0.13.0" +version = "0.10.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e8bbe1a966bd2f362681a44f6edce3c2310ac21e4d5067a6e7ec396297a6ea0" +checksum = "76979bea66e7875e7509c4ec5300112b316af87fa7a252ca91c448b32dfe3993" dependencies = [ "bitflags 2.9.1", "getopts", @@ -3394,9 +3336,9 @@ dependencies = [ [[package]] name = "pulldown-cmark-escape" -version = "0.11.0" +version = "0.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "007d8adb5ddab6f8e3f491ac63566a7d5002cc7ed73901f72057943fa71ae1ae" +checksum = "bd348ff538bc9caeda7ee8cad2d1d48236a1f443c1fa3913c6a02fe0043b1dd3" [[package]] name = "pxfm" @@ -3627,12 +3569,6 @@ version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c" -[[package]] -name = "relative-path" -version = "1.9.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ba39f3699c378cd8970968dcbff9c43159ea4cfbd88d43c00b22f2ef10a435d2" - [[package]] name = "reqwest" version = "0.12.23" @@ -3691,51 +3627,12 @@ dependencies = [ "windows-sys 0.52.0", ] -[[package]] -name = "rstest" -version = "0.25.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6fc39292f8613e913f7df8fa892b8944ceb47c247b78e1b1ae2f09e019be789d" -dependencies = [ - "futures-timer", - "futures-util", - "rstest_macros", - "rustc_version", -] - -[[package]] -name = "rstest_macros" -version = "0.25.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1f168d99749d307be9de54d23fd226628d99768225ef08f6ffb52e0182a27746" -dependencies = [ - "cfg-if", - "glob", - "proc-macro-crate", - "proc-macro2", - "quote", - "regex", - "relative-path", - "rustc_version", - "syn 2.0.104", - "unicode-ident", -] - [[package]] name = "rustc-demangle" version = "0.1.25" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "989e6739f80c4ad5b13e0fd7fe89531180375b18520cc8c82080e4dc4035b84f" -[[package]] -name = "rustc_version" -version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cfcb3a22ef46e85b45de6ee7e79d063319ebb6594faafcf1c225ea92ab6e9b92" -dependencies = [ - "semver", -] - [[package]] name = "rustix" version = "0.38.44" @@ -3975,12 +3872,6 @@ dependencies = [ "libc", ] -[[package]] -name = "semver" -version = "1.0.26" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "56e6fa9c48d24d85fb3de5ad847117517440f6beceb7798af16b4a87d616b8d0" - [[package]] name = "serde" version = "1.0.219" @@ -4464,28 +4355,6 @@ dependencies = [ "syn 2.0.104", ] -[[package]] -name = "syntect" -version = "5.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "874dcfa363995604333cf947ae9f751ca3af4522c60886774c4963943b4746b1" -dependencies = [ - "bincode", - "bitflags 1.3.2", - "flate2", - "fnv", - "once_cell", - "onig", - "plist", - "regex-syntax 0.8.5", - "serde", - "serde_derive", - "serde_json", - "thiserror 1.0.69", - "walkdir", - "yaml-rust", -] - [[package]] name = "sys-locale" version = "0.3.2" @@ -4809,18 +4678,12 @@ dependencies = [ "indexmap 2.10.0", "serde", "serde_spanned", - "toml_datetime 0.7.0", + "toml_datetime", "toml_parser", "toml_writer", "winnow", ] -[[package]] -name = "toml_datetime" -version = "0.6.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22cddaf88f4fbc13c51aebbf5f8eceb5c7c5a9da2ac40a13519eb5b0a0e8f11c" - [[package]] name = "toml_datetime" version = "0.7.0" @@ -4830,17 +4693,6 @@ dependencies = [ "serde", ] -[[package]] -name = "toml_edit" -version = "0.22.27" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41fe8c660ae4257887cf66394862d21dbca4a6ddd26f04a3560410406a2f819a" -dependencies = [ - "indexmap 2.10.0", - "toml_datetime 0.6.11", - "winnow", -] - [[package]] name = "toml_edit" version = "0.23.4" @@ -4848,7 +4700,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7211ff1b8f0d3adae1663b7da9ffe396eabe1ca25f0b0bee42b0da29a9ddce93" dependencies = [ "indexmap 2.10.0", - "toml_datetime 0.7.0", + "toml_datetime", "toml_parser", "toml_writer", "winnow", @@ -5058,22 +4910,6 @@ dependencies = [ "termcolor", ] -[[package]] -name = "tui-markdown" -version = "0.3.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d10648c25931bfaaf5334ff4e7dc5f3d830e0c50d7b0119b1d5cfe771f540536" -dependencies = [ - "ansi-to-tui", - "itertools 0.14.0", - "pretty_assertions", - "pulldown-cmark", - "ratatui", - "rstest", - "syntect", - "tracing", -] - [[package]] name = "typenum" version = "1.18.0" @@ -5855,15 +5691,6 @@ version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ec107c4503ea0b4a98ef47356329af139c0a4f7750e621cf2973cd3385ebcb3d" -[[package]] -name = "yaml-rust" -version = "0.4.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "56c1936c4cc7a1c9ab21a1ebb602eb942ba868cbd44a99cb7cdc5892335e1c85" -dependencies = [ - "linked-hash-map", -] - [[package]] name = "yansi" version = "1.0.1" diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index bedcf827be..88e68875d4 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -79,7 +79,7 @@ tokio-stream = "0.1.17" tracing = { version = "0.1.41", features = ["log"] } tracing-appender = "0.2.3" tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } -tui-markdown = "0.3.3" +pulldown-cmark = "0.10" unicode-segmentation = "1.12.0" unicode-width = "0.1" url = "2" diff --git a/codex-rs/tui/src/bin/md-events.rs b/codex-rs/tui/src/bin/md-events.rs new file mode 100644 index 0000000000..f1117fad91 --- /dev/null +++ b/codex-rs/tui/src/bin/md-events.rs @@ -0,0 +1,15 @@ +use std::io::Read; +use std::io::{self}; + +fn main() { + let mut input = String::new(); + if let Err(err) = io::stdin().read_to_string(&mut input) { + eprintln!("failed to read stdin: {err}"); + std::process::exit(1); + } + + let parser = pulldown_cmark::Parser::new(&input); + for event in parser { + println!("{event:?}"); + } +} diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_markdown_code_blocks_vt100_snapshot.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_markdown_code_blocks_vt100_snapshot.snap new file mode 100644 index 0000000000..ca4e72a94b --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chatwidget_markdown_code_blocks_vt100_snapshot.snap @@ -0,0 +1,18 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: visual +--- +> -- Indented code block (4 spaces) + SELECT * + FROM "users" + WHERE "email" LIKE '%@example.com'; + + ```sh + printf 'fenced within fenced\n' + ``` + + { + // comment allowed in jsonc + "path": "C:\\Program Files\\App", + "regex": "^foo.*(bar)?$" + } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index e097c2014e..aad6e83913 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1756,3 +1756,123 @@ fn chatwidget_exec_and_status_layout_vt100_snapshot() { let visual = vt_lines.join("\n"); assert_snapshot!(visual); } + +// E2E vt100 snapshot for complex markdown with indented and nested fenced code blocks +#[test] +fn chatwidget_markdown_code_blocks_vt100_snapshot() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + + // Simulate a final agent message via streaming deltas instead of a single message + + chat.handle_codex_event(Event { + id: "t1".into(), + msg: EventMsg::TaskStarted(TaskStartedEvent { + model_context_window: None, + }), + }); + // Build a vt100 visual from the history insertions only (no UI overlay) + let width: u16 = 80; + let height: u16 = 50; + let backend = ratatui::backend::TestBackend::new(width, height); + let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal"); + // Place viewport at the last line so that history lines insert above it + term.set_viewport_area(Rect::new(0, height - 1, width, 1)); + + let mut ansi: Vec = Vec::new(); + + // Simulate streaming via AgentMessageDelta in 2-character chunks (no final AgentMessage). + let source: &str = r#" + + -- Indented code block (4 spaces) + SELECT * + FROM "users" + WHERE "email" LIKE '%@example.com'; + +````markdown +```sh +printf 'fenced within fenced\n' +``` +```` + +```jsonc +{ + // comment allowed in jsonc + "path": "C:\\Program Files\\App", + "regex": "^foo.*(bar)?$" +} +``` +"#; + + let mut it = source.chars(); + loop { + let mut delta = String::new(); + match it.next() { + Some(c) => delta.push(c), + None => break, + } + if let Some(c2) = it.next() { + delta.push(c2); + } + + chat.handle_codex_event(Event { + id: "t1".into(), + msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { delta }), + }); + // Drive commit ticks and drain emitted history lines into the vt100 buffer. + loop { + chat.on_commit_tick(); + let mut inserted_any = false; + while let Ok(app_ev) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = app_ev { + let lines = cell.display_lines(width); + crate::insert_history::insert_history_lines_to_writer( + &mut term, &mut ansi, lines, + ); + inserted_any = true; + } + } + if !inserted_any { + break; + } + } + } + + // Finalize the stream without sending a final AgentMessage, to flush any tail. + chat.handle_codex_event(Event { + id: "t1".into(), + msg: EventMsg::TaskComplete(TaskCompleteEvent { + last_agent_message: None, + }), + }); + for lines in drain_insert_history(&mut rx) { + crate::insert_history::insert_history_lines_to_writer(&mut term, &mut ansi, lines); + } + + let mut parser = vt100::Parser::new(height, width, 0); + parser.process(&ansi); + + let mut vt_lines: Vec = (0..height) + .map(|row| { + let mut s = String::with_capacity(width as usize); + for col in 0..width { + if let Some(cell) = parser.screen().cell(row, col) { + if let Some(ch) = cell.contents().chars().next() { + s.push(ch); + } else { + s.push(' '); + } + } else { + s.push(' '); + } + } + s.trim_end().to_string() + }) + .collect(); + + // Compact trailing blank rows for a stable snapshot + while matches!(vt_lines.last(), Some(l) if l.trim().is_empty()) { + vt_lines.pop(); + } + let visual = vt_lines.join("\n"); + assert_snapshot!(visual); +} diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index bffd3f2b40..75731c7f55 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -97,7 +97,17 @@ pub fn insert_history_lines_to_writer( for line in wrapped { queue!(writer, Print("\r\n")).ok(); - write_spans(writer, &line).ok(); + // Merge line-level style into each span so that ANSI colors reflect + // line styles (e.g., blockquotes with green fg). + let merged_spans: Vec = line + .spans + .iter() + .map(|s| Span { + style: s.style.patch(line.style), + content: s.content.clone(), + }) + .collect(); + write_spans(writer, merged_spans.iter()).ok(); } queue!(writer, ResetScrollRegion).ok(); @@ -264,6 +274,10 @@ where #[cfg(test)] mod tests { use super::*; + use crate::markdown_render::render_markdown_text; + use ratatui::layout::Rect; + use ratatui::style::Color; + use vt100::Parser; #[test] fn writes_bold_then_regular_spans() { @@ -292,4 +306,240 @@ mod tests { String::from_utf8(expected).unwrap() ); } + + #[test] + fn vt100_blockquote_line_emits_green_fg() { + // Set up a small off-screen terminal + let width: u16 = 40; + let height: u16 = 10; + let backend = ratatui::backend::TestBackend::new(width, height); + let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal"); + // Place viewport on the last line so history inserts scroll upward + let viewport = Rect::new(0, height - 1, width, 1); + term.set_viewport_area(viewport); + + // Build a blockquote-like line: apply line-level green style and prefix "> " + let mut line: Line<'static> = Line::from(vec!["> ".into(), "Hello world".into()]); + line = line.style(Color::Green); + let mut ansi: Vec = Vec::new(); + insert_history_lines_to_writer(&mut term, &mut ansi, vec![line]); + + // Parse ANSI using vt100 and assert at least one non-default fg color appears + let mut parser = Parser::new(height, width, 0); + parser.process(&ansi); + + let mut saw_colored = false; + 'outer: for row in 0..height { + for col in 0..width { + if let Some(cell) = parser.screen().cell(row, col) + && cell.has_contents() + && cell.fgcolor() != vt100::Color::Default + { + saw_colored = true; + break 'outer; + } + } + } + assert!( + saw_colored, + "expected at least one colored cell in vt100 output" + ); + } + + #[test] + fn vt100_blockquote_wrap_preserves_color_on_all_wrapped_lines() { + // Force wrapping by using a narrow viewport width and a long blockquote line. + let width: u16 = 20; + let height: u16 = 8; + let backend = ratatui::backend::TestBackend::new(width, height); + let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal"); + // Viewport is the last line so history goes directly above it. + let viewport = Rect::new(0, height - 1, width, 1); + term.set_viewport_area(viewport); + + // Create a long blockquote with a distinct prefix and enough text to wrap. + let mut line: Line<'static> = Line::from(vec![ + "> ".into(), + "This is a long quoted line that should wrap".into(), + ]); + line = line.style(Color::Green); + + let mut ansi: Vec = Vec::new(); + insert_history_lines_to_writer(&mut term, &mut ansi, vec![line]); + + // Parse and inspect the final screen buffer. + let mut parser = Parser::new(height, width, 0); + parser.process(&ansi); + let screen = parser.screen(); + + // Collect rows that are non-empty; these should correspond to our wrapped lines. + let mut non_empty_rows: Vec = Vec::new(); + for row in 0..height { + let mut any = false; + for col in 0..width { + if let Some(cell) = screen.cell(row, col) + && cell.has_contents() + && cell.contents() != "\0" + && cell.contents() != " " + { + any = true; + break; + } + } + if any { + non_empty_rows.push(row); + } + } + + // Expect at least two rows due to wrapping. + assert!( + non_empty_rows.len() >= 2, + "expected wrapped output to span >=2 rows, got {non_empty_rows:?}", + ); + + // For each non-empty row, ensure all non-space cells are using a non-default fg color. + for row in non_empty_rows { + for col in 0..width { + if let Some(cell) = screen.cell(row, col) { + let contents = cell.contents(); + if !contents.is_empty() && contents != " " { + assert!( + cell.fgcolor() != vt100::Color::Default, + "expected non-default fg on row {row} col {col}, got {:?}", + cell.fgcolor() + ); + } + } + } + } + } + + #[test] + fn vt100_colored_prefix_then_plain_text_resets_color() { + let width: u16 = 40; + let height: u16 = 6; + let backend = ratatui::backend::TestBackend::new(width, height); + let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal"); + let viewport = Rect::new(0, height - 1, width, 1); + term.set_viewport_area(viewport); + + // First span colored, rest plain. + let line: Line<'static> = Line::from(vec![ + Span::styled("1. ", ratatui::style::Style::default().fg(Color::LightBlue)), + Span::raw("Hello world"), + ]); + + let mut ansi: Vec = Vec::new(); + insert_history_lines_to_writer(&mut term, &mut ansi, vec![line]); + + let mut parser = Parser::new(height, width, 0); + parser.process(&ansi); + let screen = parser.screen(); + + // Find the first non-empty row; verify first three cells are colored, following cells default. + 'rows: for row in 0..height { + let mut has_text = false; + for col in 0..width { + if let Some(cell) = screen.cell(row, col) + && cell.has_contents() + && cell.contents() != " " + { + has_text = true; + break; + } + } + if !has_text { + continue; + } + + // Expect "1. Hello world" starting at col 0. + for col in 0..3 { + let cell = screen.cell(row, col).unwrap(); + assert!( + cell.fgcolor() != vt100::Color::Default, + "expected colored prefix at col {col}, got {:?}", + cell.fgcolor() + ); + } + for col in 3..(3 + "Hello world".len() as u16) { + let cell = screen.cell(row, col).unwrap(); + assert_eq!( + cell.fgcolor(), + vt100::Color::Default, + "expected default color for plain text at col {col}, got {:?}", + cell.fgcolor() + ); + } + break 'rows; + } + } + + #[test] + fn vt100_deep_nested_mixed_list_third_level_marker_is_colored() { + // Markdown with five levels (ordered → unordered → ordered → unordered → unordered). + let md = "1. First\n - Second level\n 1. Third level (ordered)\n - Fourth level (bullet)\n - Fifth level to test indent consistency\n"; + let text = render_markdown_text(md); + let lines: Vec> = text.lines.clone(); + + let width: u16 = 60; + let height: u16 = 12; + let backend = ratatui::backend::TestBackend::new(width, height); + let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal"); + let viewport = ratatui::layout::Rect::new(0, height - 1, width, 1); + term.set_viewport_area(viewport); + + let mut ansi: Vec = Vec::new(); + insert_history_lines_to_writer(&mut term, &mut ansi, lines); + + let mut parser = Parser::new(height, width, 0); + parser.process(&ansi); + let screen = parser.screen(); + + // Reconstruct screen rows as strings to locate the 3rd level line. + let mut rows: Vec = Vec::with_capacity(height as usize); + for row in 0..height { + let mut s = String::with_capacity(width as usize); + for col in 0..width { + if let Some(cell) = screen.cell(row, col) { + if let Some(ch) = cell.contents().chars().next() { + s.push(ch); + } else { + s.push(' '); + } + } else { + s.push(' '); + } + } + rows.push(s.trim_end().to_string()); + } + + let needle = "1. Third level (ordered)"; + let row_idx = rows + .iter() + .position(|r| r.contains(needle)) + .unwrap_or_else(|| { + panic!("expected to find row containing {needle:?}, have rows: {rows:?}") + }); + let col_start = rows[row_idx].find(needle).unwrap() as u16; // column where '1' starts + + // Verify that the numeric marker ("1.") at the third level is colored + // (non-default fg) and the content after the following space resets to default. + for c in [col_start, col_start + 1] { + let cell = screen.cell(row_idx as u16, c).unwrap(); + assert!( + cell.fgcolor() != vt100::Color::Default, + "expected colored 3rd-level marker at row {row_idx} col {c}, got {:?}", + cell.fgcolor() + ); + } + let content_col = col_start + 3; // skip '1', '.', and the space + if let Some(cell) = screen.cell(row_idx as u16, content_col) { + assert_eq!( + cell.fgcolor(), + vt100::Color::Default, + "expected default color for 3rd-level content at row {row_idx} col {content_col}, got {:?}", + cell.fgcolor() + ); + } + } } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5f002e53db..710312cebc 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -45,6 +45,7 @@ pub mod insert_history; mod key_hint; pub mod live_wrap; mod markdown; +mod markdown_render; mod markdown_stream; pub mod onboarding; mod pager_overlay; diff --git a/codex-rs/tui/src/markdown.rs b/codex-rs/tui/src/markdown.rs index 6aff205a35..010ff78ab5 100644 --- a/codex-rs/tui/src/markdown.rs +++ b/codex-rs/tui/src/markdown.rs @@ -1,8 +1,6 @@ -use crate::citation_regex::CITATION_REGEX; use codex_core::config::Config; use codex_core::config_types::UriBasedFileOpener; use ratatui::text::Line; -use std::borrow::Cow; use std::path::Path; pub(crate) fn append_markdown( @@ -19,238 +17,13 @@ fn append_markdown_with_opener_and_cwd( file_opener: UriBasedFileOpener, cwd: &Path, ) { - // Historically, we fed the entire `markdown_source` into the renderer in - // one pass. However, fenced code blocks sometimes lost leading whitespace - // when formatted by the markdown renderer/highlighter. To preserve code - // block content exactly, split the source into "text" and "code" segments: - // - Render non-code text through `tui_markdown` (with citation rewrite). - // - Render code block content verbatim as plain lines without additional - // formatting, preserving leading spaces. - for seg in split_text_and_fences(markdown_source) { - match seg { - Segment::Text(s) => { - let processed = rewrite_file_citations(&s, file_opener, cwd); - let rendered = tui_markdown::from_str(&processed); - crate::render::line_utils::push_owned_lines(&rendered.lines, lines); - } - Segment::Code { content, .. } => { - // Emit the code content exactly as-is, line by line. - // We don't attempt syntax highlighting to avoid whitespace bugs. - for line in content.split_inclusive('\n') { - // split_inclusive keeps the trailing \n; we want lines without it. - let line = if let Some(stripped) = line.strip_suffix('\n') { - stripped - } else { - line - }; - let owned_line: Line<'static> = line.to_string().into(); - lines.push(owned_line); - } - } - } - } -} - -/// Rewrites file citations in `src` into markdown hyperlinks using the -/// provided `scheme` (`vscode`, `cursor`, etc.). The resulting URI follows the -/// format expected by VS Code-compatible file openers: -/// -/// ```text -/// ://file: -/// ``` -fn rewrite_file_citations<'a>( - src: &'a str, - file_opener: UriBasedFileOpener, - cwd: &Path, -) -> Cow<'a, str> { - // Map enum values to the corresponding URI scheme strings. - let scheme: &str = match file_opener.get_scheme() { - Some(scheme) => scheme, - None => return Cow::Borrowed(src), - }; - - CITATION_REGEX.replace_all(src, |caps: ®ex_lite::Captures<'_>| { - let file = &caps[1]; - let start_line = &caps[2]; - - // Resolve the path against `cwd` when it is relative. - let absolute_path = { - let p = Path::new(file); - let absolute_path = if p.is_absolute() { - path_clean::clean(p) - } else { - path_clean::clean(cwd.join(p)) - }; - // VS Code expects forward slashes even on Windows because URIs use - // `/` as the path separator. - absolute_path.to_string_lossy().replace('\\', "/") - }; - - // Render as a normal markdown link so the downstream renderer emits - // the hyperlink escape sequence (when supported by the terminal). - // - // In practice, sometimes multiple citations for the same file, but with a - // different line number, are shown sequentially, so we: - // - include the line number in the label to disambiguate them - // - add a space after the link to make it easier to read - format!("[{file}:{start_line}]({scheme}://file{absolute_path}:{start_line}) ") - }) -} - -// use shared helper from `line_utils` - -// Minimal code block splitting. -// - Recognizes fenced blocks opened by ``` or ~~~ (allowing leading whitespace). -// The opening fence may include a language string which we ignore. -// The closing fence must be on its own line (ignoring surrounding whitespace). -// - Additionally recognizes indented code blocks that begin after a blank line -// with a line starting with at least 4 spaces or a tab, and continue for -// consecutive lines that are blank or also indented by >= 4 spaces or a tab. -enum Segment { - Text(String), - Code { - _lang: Option, - content: String, - }, -} - -fn split_text_and_fences(src: &str) -> Vec { - let mut segments = Vec::new(); - let mut curr_text = String::new(); - #[derive(Copy, Clone, PartialEq)] - enum CodeMode { - None, - Fenced, - Indented, - } - let mut code_mode = CodeMode::None; - let mut fence_token = ""; - let mut code_lang: Option = None; - let mut code_content = String::new(); - // We intentionally do not require a preceding blank line for indented code blocks, - // since streamed model output often omits it. This favors preserving indentation. - - for line in src.split_inclusive('\n') { - let line_no_nl = line.strip_suffix('\n'); - let trimmed_start = match line_no_nl { - Some(l) => l.trim_start(), - None => line.trim_start(), - }; - if code_mode == CodeMode::None { - let open = if trimmed_start.starts_with("```") { - Some("```") - } else if trimmed_start.starts_with("~~~") { - Some("~~~") - } else { - None - }; - if let Some(tok) = open { - // Flush pending text segment. - if !curr_text.is_empty() { - segments.push(Segment::Text(curr_text.clone())); - curr_text.clear(); - } - fence_token = tok; - // Capture language after the token on this line (before newline). - let after = &trimmed_start[tok.len()..]; - let lang = after.trim(); - code_lang = if lang.is_empty() { - None - } else { - Some(lang.to_string()) - }; - code_mode = CodeMode::Fenced; - code_content.clear(); - // Do not include the opening fence line in output. - continue; - } - // Check for start of an indented code block: only after a blank line - // (or at the beginning), and the line must start with >=4 spaces or a tab. - let raw_line = match line_no_nl { - Some(l) => l, - None => line, - }; - let leading_spaces = raw_line.chars().take_while(|c| *c == ' ').count(); - let starts_with_tab = raw_line.starts_with('\t'); - // Consider any line that begins with >=4 spaces or a tab to start an - // indented code block. This favors preserving indentation even when a - // preceding blank line is omitted (common in streamed model output). - let starts_indented_code = (leading_spaces >= 4) || starts_with_tab; - if starts_indented_code { - // Flush pending text and begin an indented code block. - if !curr_text.is_empty() { - segments.push(Segment::Text(curr_text.clone())); - curr_text.clear(); - } - code_mode = CodeMode::Indented; - code_content.clear(); - code_content.push_str(line); - // Inside code now; do not treat this line as normal text. - continue; - } - // Normal text line. - curr_text.push_str(line); - } else { - match code_mode { - CodeMode::Fenced => { - // inside fenced code: check for closing fence on its own line - let trimmed = match line_no_nl { - Some(l) => l.trim(), - None => line.trim(), - }; - if trimmed == fence_token { - // End code block: emit segment without fences - segments.push(Segment::Code { - _lang: code_lang.take(), - content: code_content.clone(), - }); - code_content.clear(); - code_mode = CodeMode::None; - fence_token = ""; - continue; - } - // Accumulate code content exactly as-is. - code_content.push_str(line); - } - CodeMode::Indented => { - // Continue while the line is blank, or starts with >=4 spaces, or a tab. - let raw_line = match line_no_nl { - Some(l) => l, - None => line, - }; - let is_blank = raw_line.trim().is_empty(); - let leading_spaces = raw_line.chars().take_while(|c| *c == ' ').count(); - let starts_with_tab = raw_line.starts_with('\t'); - if is_blank || leading_spaces >= 4 || starts_with_tab { - code_content.push_str(line); - } else { - // Close the indented code block and reprocess this line as normal text. - segments.push(Segment::Code { - _lang: None, - content: code_content.clone(), - }); - code_content.clear(); - code_mode = CodeMode::None; - // Now handle current line as text. - curr_text.push_str(line); - } - } - CodeMode::None => unreachable!(), - } - } - } - - if code_mode != CodeMode::None { - // Unterminated code fence: treat accumulated content as a code segment. - segments.push(Segment::Code { - _lang: code_lang.take(), - content: code_content.clone(), - }); - } else if !curr_text.is_empty() { - segments.push(Segment::Text(curr_text.clone())); - } - - segments + // Render via pulldown-cmark and rewrite citations during traversal (outside code blocks). + let rendered = crate::markdown_render::render_markdown_text_with_citations( + markdown_source, + file_opener.get_scheme(), + cwd, + ); + crate::render::line_utils::push_owned_lines(&rendered.lines, lines); } #[cfg(test)] @@ -258,88 +31,6 @@ mod tests { use super::*; use pretty_assertions::assert_eq; - #[test] - fn citation_is_rewritten_with_absolute_path() { - let markdown = "See 【F:/src/main.rs†L42-L50】 for details."; - let cwd = Path::new("/workspace"); - let result = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); - - assert_eq!( - "See [/src/main.rs:42](vscode://file/src/main.rs:42) for details.", - result - ); - } - - #[test] - fn citation_is_rewritten_with_relative_path() { - let markdown = "Refer to 【F:lib/mod.rs†L5】 here."; - let cwd = Path::new("/home/user/project"); - let result = rewrite_file_citations(markdown, UriBasedFileOpener::Windsurf, cwd); - - assert_eq!( - "Refer to [lib/mod.rs:5](windsurf://file/home/user/project/lib/mod.rs:5) here.", - result - ); - } - - #[test] - fn citation_followed_by_space_so_they_do_not_run_together() { - let markdown = "References on lines 【F:src/foo.rs†L24】【F:src/foo.rs†L42】"; - let cwd = Path::new("/home/user/project"); - let result = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); - - assert_eq!( - "References on lines [src/foo.rs:24](vscode://file/home/user/project/src/foo.rs:24) [src/foo.rs:42](vscode://file/home/user/project/src/foo.rs:42) ", - result - ); - } - - #[test] - fn citation_unchanged_without_file_opener() { - let markdown = "Look at 【F:file.rs†L1】."; - let cwd = Path::new("/"); - let unchanged = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); - // The helper itself always rewrites – this test validates behaviour of - // append_markdown when `file_opener` is None. - let mut out = Vec::new(); - append_markdown_with_opener_and_cwd(markdown, &mut out, UriBasedFileOpener::None, cwd); - // Convert lines back to string for comparison. - let rendered: String = out - .iter() - .flat_map(|l| l.spans.iter()) - .map(|s| s.content.clone()) - .collect::>() - .join(""); - assert_eq!(markdown, rendered); - // Ensure helper rewrites. - assert_ne!(markdown, unchanged); - } - - #[test] - fn fenced_code_blocks_preserve_leading_whitespace() { - let src = "```\n indented\n\t\twith tabs\n four spaces\n```\n"; - let cwd = Path::new("/"); - let mut out = Vec::new(); - append_markdown_with_opener_and_cwd(src, &mut out, UriBasedFileOpener::None, cwd); - let rendered: Vec = out - .iter() - .map(|l| { - l.spans - .iter() - .map(|s| s.content.clone()) - .collect::() - }) - .collect(); - assert_eq!( - rendered, - vec![ - " indented".to_string(), - "\t\twith tabs".to_string(), - " four spaces".to_string() - ] - ); - } - #[test] fn citations_not_rewritten_inside_code_blocks() { let src = "Before 【F:/x.rs†L1】\n```\nInside 【F:/x.rs†L2】\n```\nAfter 【F:/x.rs†L3】\n"; @@ -355,19 +46,31 @@ mod tests { .collect::() }) .collect(); - // Expect first and last lines rewritten, middle line unchanged. - assert!(rendered[0].contains("vscode://file")); - assert_eq!(rendered[1], "Inside 【F:/x.rs†L2】"); - assert!(matches!(rendered.last(), Some(s) if s.contains("vscode://file"))); + // Expect a line containing the inside text unchanged. + assert!(rendered.iter().any(|s| s.contains("Inside 【F:/x.rs†L2】"))); + // And first/last sections rewritten. + assert!( + rendered + .first() + .map(|s| s.contains("vscode://file")) + .unwrap_or(false) + ); + assert!( + rendered + .last() + .map(|s| s.contains("vscode://file")) + .unwrap_or(false) + ); } #[test] fn indented_code_blocks_preserve_leading_whitespace() { - let src = "Before\n code 1\n\tcode with tab\n code 2\nAfter\n"; + // Basic sanity: indented code with surrounding blank lines should produce the indented line. + let src = "Before\n\n code 1\n\nAfter\n"; let cwd = Path::new("/"); let mut out = Vec::new(); append_markdown_with_opener_and_cwd(src, &mut out, UriBasedFileOpener::None, cwd); - let rendered: Vec = out + let lines: Vec = out .iter() .map(|l| { l.spans @@ -376,16 +79,7 @@ mod tests { .collect::() }) .collect(); - assert_eq!( - rendered, - vec![ - "Before".to_string(), - " code 1".to_string(), - "\tcode with tab".to_string(), - " code 2".to_string(), - "After".to_string() - ] - ); + assert_eq!(lines, vec!["Before", "", " code 1", "", "After"]); } #[test] @@ -403,11 +97,17 @@ mod tests { .collect::() }) .collect(); - // Expect first and last lines rewritten, and the indented code line present - // unchanged (citations inside not rewritten). We do not assert on blank - // separator lines since the markdown renderer may normalize them. - assert!(rendered.iter().any(|s| s.contains("vscode://file"))); - assert!(rendered.iter().any(|s| s == " Inside 【F:/x.rs†L2】")); + assert!( + rendered + .iter() + .any(|s| s.contains("Start") && s.contains("vscode://file")) + ); + assert!( + rendered + .iter() + .any(|s| s.contains("End") && s.contains("vscode://file")) + ); + assert!(rendered.iter().any(|s| s.contains("Inside 【F:/x.rs†L2】"))); } #[test] @@ -435,27 +135,6 @@ mod tests { ); } - #[test] - fn tui_markdown_splits_ordered_marker_and_text() { - // With marker and content on the same line, tui_markdown keeps it as one line - // even in the surrounding section context. - let rendered = tui_markdown::from_str("Loose vs. tight list items:\n1. Tight item\n"); - let lines: Vec = rendered - .lines - .iter() - .map(|l| { - l.spans - .iter() - .map(|s| s.content.clone()) - .collect::() - }) - .collect(); - assert!( - lines.iter().any(|w| w == "1. Tight item"), - "expected single line '1. Tight item' in context: {lines:?}" - ); - } - #[test] fn append_markdown_matches_tui_markdown_for_ordered_item() { use codex_core::config_types::UriBasedFileOpener; @@ -480,72 +159,6 @@ mod tests { assert_eq!(lines, vec!["1. Tight item".to_string()]); } - #[test] - fn tui_markdown_shape_for_loose_tight_section() { - // Use the exact source from the session deltas used in tests. - let source = r#" -Loose vs. tight list items: -1. Tight item -2. Another tight item - -3. - Loose item -"#; - - let rendered = tui_markdown::from_str(source); - let lines: Vec = rendered - .lines - .iter() - .map(|l| { - l.spans - .iter() - .map(|s| s.content.clone()) - .collect::() - }) - .collect(); - // Join into a single string and assert the exact shape we observe - // from tui_markdown in this larger context (marker and content split). - let joined = { - let mut s = String::new(); - for (i, l) in lines.iter().enumerate() { - s.push_str(l); - if i + 1 < lines.len() { - s.push('\n'); - } - } - s - }; - let expected = r#"Loose vs. tight list items: - -1. -Tight item -2. -Another tight item -3. -Loose item"#; - assert_eq!( - joined, expected, - "unexpected tui_markdown shape: {joined:?}" - ); - } - - #[test] - fn split_text_and_fences_keeps_ordered_list_line_as_text() { - // No fences here; expect a single Text segment containing the full input. - let src = "Loose vs. tight list items:\n1. Tight item\n"; - let segs = super::split_text_and_fences(src); - assert_eq!( - segs.len(), - 1, - "expected single text segment, got {}", - segs.len() - ); - match &segs[0] { - super::Segment::Text(s) => assert_eq!(s, src), - _ => panic!("expected Text segment for non-fence input"), - } - } - #[test] fn append_markdown_keeps_ordered_list_line_unsplit_in_context() { use codex_core::config_types::UriBasedFileOpener; diff --git a/codex-rs/tui/src/markdown_render.rs b/codex-rs/tui/src/markdown_render.rs new file mode 100644 index 0000000000..f20bfd75c0 --- /dev/null +++ b/codex-rs/tui/src/markdown_render.rs @@ -0,0 +1,566 @@ +use crate::citation_regex::CITATION_REGEX; +use pulldown_cmark::CodeBlockKind; +use pulldown_cmark::CowStr; +use pulldown_cmark::Event; +use pulldown_cmark::HeadingLevel; +use pulldown_cmark::Options; +use pulldown_cmark::Parser; +use pulldown_cmark::Tag; +use pulldown_cmark::TagEnd; +use ratatui::style::Style; +use ratatui::style::Stylize; +use ratatui::text::Line; +use ratatui::text::Span; +use ratatui::text::Text; +use std::borrow::Cow; +use std::path::Path; + +#[derive(Clone, Debug)] +struct IndentContext { + prefix: Vec>, + marker: Option>>, + is_list: bool, +} + +impl IndentContext { + fn new(prefix: Vec>, marker: Option>>, is_list: bool) -> Self { + Self { + prefix, + marker, + is_list, + } + } +} + +#[allow(dead_code)] +pub(crate) fn render_markdown_text(input: &str) -> Text<'static> { + let mut options = Options::empty(); + options.insert(Options::ENABLE_STRIKETHROUGH); + let parser = Parser::new_ext(input, options); + let mut w = Writer::new(parser, None, None); + w.run(); + w.text +} + +pub(crate) fn render_markdown_text_with_citations( + input: &str, + scheme: Option<&str>, + cwd: &Path, +) -> Text<'static> { + let mut options = Options::empty(); + options.insert(Options::ENABLE_STRIKETHROUGH); + let parser = Parser::new_ext(input, options); + let mut w = Writer::new( + parser, + scheme.map(|s| s.to_string()), + Some(cwd.to_path_buf()), + ); + w.run(); + w.text +} + +struct Writer<'a, I> +where + I: Iterator>, +{ + iter: I, + text: Text<'static>, + inline_styles: Vec