From be23fe1353cd97384824962351a2c743b5d3ee2c Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 4 Sep 2025 12:37:43 -0700 Subject: [PATCH 01/13] Pause status timer while modals are open (#3131) Summary: - pause the status timer while waiting on approval modals - expose deterministic pause/resume helpers to avoid sleep-based tests - simplify bottom pane timer handling now that the widget owns the clock --- codex-rs/tui/src/bottom_pane/mod.rs | 21 +++++++ codex-rs/tui/src/status_indicator_widget.rs | 70 ++++++++++++++++++++- 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 5ab32849b1..d5daea2ebe 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -162,6 +162,8 @@ impl BottomPane { view.handle_key_event(self, key_event); if !view.is_complete() { self.active_view = Some(view); + } else { + self.on_active_view_complete(); } self.request_redraw(); InputResult::None @@ -201,6 +203,8 @@ impl BottomPane { CancellationEvent::Handled => { if !view.is_complete() { self.active_view = Some(view); + } else { + self.on_active_view_complete(); } self.show_ctrl_c_quit_hint(); } @@ -381,10 +385,27 @@ impl BottomPane { // Otherwise create a new approval modal overlay. let modal = ApprovalModalView::new(request, self.app_event_tx.clone()); + self.pause_status_timer_for_modal(); self.active_view = Some(Box::new(modal)); self.request_redraw() } + fn on_active_view_complete(&mut self) { + self.resume_status_timer_after_modal(); + } + + fn pause_status_timer_for_modal(&mut self) { + if let Some(status) = self.status.as_mut() { + status.pause_timer(); + } + } + + fn resume_status_timer_after_modal(&mut self) { + if let Some(status) = self.status.as_mut() { + status.resume_timer(); + } + } + /// Height (terminal rows) required by the current bottom pane. pub(crate) fn request_redraw(&self) { self.frame_requester.schedule_frame(); diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index 126f3d1221..07ecba0c2b 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -26,7 +26,9 @@ pub(crate) struct StatusIndicatorWidget { /// Queued user messages to display under the status line. queued_messages: Vec, - start_time: Instant, + elapsed_running: Duration, + last_resume_at: Instant, + is_paused: bool, app_event_tx: AppEventSender, frame_requester: FrameRequester, } @@ -36,7 +38,9 @@ impl StatusIndicatorWidget { Self { header: String::from("Working"), queued_messages: Vec::new(), - start_time: Instant::now(), + elapsed_running: Duration::ZERO, + last_resume_at: Instant::now(), + is_paused: false, app_event_tx, frame_requester, @@ -88,6 +92,43 @@ impl StatusIndicatorWidget { // Ensure a redraw so changes are visible. self.frame_requester.schedule_frame(); } + + pub(crate) fn pause_timer(&mut self) { + self.pause_timer_at(Instant::now()); + } + + pub(crate) fn resume_timer(&mut self) { + self.resume_timer_at(Instant::now()); + } + + pub(crate) fn pause_timer_at(&mut self, now: Instant) { + if self.is_paused { + return; + } + self.elapsed_running += now.saturating_duration_since(self.last_resume_at); + self.is_paused = true; + } + + pub(crate) fn resume_timer_at(&mut self, now: Instant) { + if !self.is_paused { + return; + } + self.last_resume_at = now; + self.is_paused = false; + self.frame_requester.schedule_frame(); + } + + fn elapsed_seconds_at(&self, now: Instant) -> u64 { + let mut elapsed = self.elapsed_running; + if !self.is_paused { + elapsed += now.saturating_duration_since(self.last_resume_at); + } + elapsed.as_secs() + } + + fn elapsed_seconds(&self) -> u64 { + self.elapsed_seconds_at(Instant::now()) + } } impl WidgetRef for StatusIndicatorWidget { @@ -99,7 +140,7 @@ impl WidgetRef for StatusIndicatorWidget { // Schedule next animation frame. self.frame_requester .schedule_frame_in(Duration::from_millis(32)); - let elapsed = self.start_time.elapsed().as_secs(); + let elapsed = self.elapsed_seconds(); // Plain rendering: no borders or padding so the live cell is visually indistinguishable from terminal scrollback. let mut spans = vec![" ".into()]; @@ -147,6 +188,8 @@ mod tests { use crate::app_event_sender::AppEventSender; use ratatui::Terminal; use ratatui::backend::TestBackend; + use std::time::Duration; + use std::time::Instant; use tokio::sync::mpsc::unbounded_channel; #[test] @@ -191,4 +234,25 @@ mod tests { .expect("draw"); insta::assert_snapshot!(terminal.backend()); } + + #[test] + fn timer_pauses_when_requested() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut widget = StatusIndicatorWidget::new(tx, crate::tui::FrameRequester::test_dummy()); + + let baseline = Instant::now(); + widget.last_resume_at = baseline; + + let before_pause = widget.elapsed_seconds_at(baseline + Duration::from_secs(5)); + assert_eq!(before_pause, 5); + + widget.pause_timer_at(baseline + Duration::from_secs(5)); + let paused_elapsed = widget.elapsed_seconds_at(baseline + Duration::from_secs(10)); + assert_eq!(paused_elapsed, before_pause); + + widget.resume_timer_at(baseline + Duration::from_secs(10)); + let after_resume = widget.elapsed_seconds_at(baseline + Duration::from_secs(13)); + assert_eq!(after_resume, before_pause + 3); + } } From bef7ed0ccc563e61fac5bef811c6079d9d65ce60 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 4 Sep 2025 13:30:12 -0700 Subject: [PATCH 02/13] prompt to read AGENTS.md files (#3122) --- codex-rs/core/prompt.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/codex-rs/core/prompt.md b/codex-rs/core/prompt.md index 4b39ed6bbe..e18327b46b 100644 --- a/codex-rs/core/prompt.md +++ b/codex-rs/core/prompt.md @@ -14,6 +14,18 @@ Within this context, Codex refers to the open-source agentic coding interface (n Your default personality and tone is concise, direct, and friendly. You communicate efficiently, always keeping the user clearly informed about ongoing actions without unnecessary detail. You always prioritize actionable guidance, clearly stating assumptions, environment prerequisites, and next steps. Unless explicitly asked, you avoid excessively verbose explanations about your work. +# AGENTS.md spec +- Repos often contain AGENTS.md files. These files can appear anywhere within the repository. +- These files are a way for humans to give you (the agent) instructions or tips for working within the container. +- Some examples might be: coding conventions, info about how code is organized, or instructions for how to run or test code. +- Instructions in AGENTS.md files: + - The scope of an AGENTS.md file is the entire directory tree rooted at the folder that contains it. + - For every file you touch in the final patch, you must obey instructions in any AGENTS.md file whose scope includes that file. + - Instructions about code style, structure, naming, etc. apply only to code within the AGENTS.md file's scope, unless the file states otherwise. + - More-deeply-nested AGENTS.md files take precedence in the case of conflicting instructions. + - Direct system/developer/user instructions (as part of a prompt) take precedence over AGENTS.md instructions. +- The contents of the AGENTS.md file at the root of the repo and any directories from the CWD up to the root are included with the developer message and don't need to be re-read. When working in a subdirectory of CWD, or a directory outside the CWD, check for any AGENTS.md files that may be applicable. + ## Responsiveness ### Preamble messages From 1c04e1314daf2b31a160d69f667fc002a9e1adf8 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 4 Sep 2025 13:36:12 -0700 Subject: [PATCH 03/13] AGENTS.md: clarify test approvals for codex-rs (#3132) Clarifies codex-rs testing approvals in AGENTS.md: - Allow running project-specific or individual tests without asking. - Require asking before running the complete test suite. - Keep `just fmt` always allowed without approval. --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 1c3f8ad825..83fd2a3ec4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,7 +11,7 @@ In the codex-rs folder where the rust code lives: Run `just fmt` (in `codex-rs` directory) automatically after making Rust code changes; do not ask for approval to run it. Before finalizing a change to `codex-rs`, run `just fix -p ` (in `codex-rs` directory) to fix any linter issues in the code. Prefer scoping with `-p` to avoid slow workspace‑wide Clippy builds; only run `just fix` without `-p` if you changed shared crates. Additionally, run the tests: 1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`. 2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`. -When running interactively, ask the user before running `just fix` and tests to finalize; `just fmt` does not require approval. +When running interactively, ask the user before running `just fix` to finalize. `just fmt` does not require approval. project-specific or individual tests can be run without asking the user, but do ask the user before running the complete test suite. ## TUI style conventions From 82ed7bd2852f17898f1f0a59f543a5ba12172b72 Mon Sep 17 00:00:00 2001 From: Dylan Date: Thu, 4 Sep 2025 16:26:41 -0700 Subject: [PATCH 04/13] [mcp-server] Update read config interface (#3093) ## Summary Follow-up to #3056 This PR updates the mcp-server interface for reading the config settings saved by the user. At risk of introducing _another_ Config struct, I think it makes sense to avoid tying our protocol to ConfigToml, as its become a bit unwieldy. GetConfigTomlResponse was a de-facto struct for this already - better to make it explicit, in my opinion. This is technically a breaking change of the mcp-server protocol, but given the previous interface was introduced so recently in #2725, and we have not yet even started to call it, I propose proceeding with the breaking change - but am open to preserving the old endpoint. ## Testing - [x] Added additional integration test coverage --- codex-rs/core/src/config.rs | 34 +++++ codex-rs/core/src/config_profile.rs | 15 ++ codex-rs/core/src/config_types.rs | 11 ++ .../mcp-server/src/codex_message_processor.rs | 36 ++--- .../mcp-server/tests/common/mcp_process.rs | 6 +- codex-rs/mcp-server/tests/suite/config.rs | 131 +++++++++++++++--- codex-rs/protocol-ts/src/lib.rs | 1 + codex-rs/protocol/src/config_types.rs | 14 +- codex-rs/protocol/src/mcp_protocol.rs | 70 +++++++++- 9 files changed, 246 insertions(+), 72 deletions(-) diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 2b8d7d92d5..993dc5a2ba 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -20,6 +20,8 @@ use codex_protocol::config_types::ReasoningSummary; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::Verbosity; use codex_protocol::mcp_protocol::AuthMode; +use codex_protocol::mcp_protocol::Tools; +use codex_protocol::mcp_protocol::UserSavedConfig; use dirs::home_dir; use serde::Deserialize; use std::collections::HashMap; @@ -503,6 +505,29 @@ pub struct ConfigToml { pub disable_paste_burst: Option, } +impl From for UserSavedConfig { + fn from(config_toml: ConfigToml) -> Self { + let profiles = config_toml + .profiles + .into_iter() + .map(|(k, v)| (k, v.into())) + .collect(); + + Self { + approval_policy: config_toml.approval_policy, + sandbox_mode: config_toml.sandbox_mode, + sandbox_settings: config_toml.sandbox_workspace_write.map(From::from), + model: config_toml.model, + model_reasoning_effort: config_toml.model_reasoning_effort, + model_reasoning_summary: config_toml.model_reasoning_summary, + model_verbosity: config_toml.model_verbosity, + tools: config_toml.tools.map(From::from), + profile: config_toml.profile, + profiles, + } + } +} + #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] pub struct ProjectConfig { pub trust_level: Option, @@ -518,6 +543,15 @@ pub struct ToolsToml { pub view_image: Option, } +impl From for Tools { + fn from(tools_toml: ToolsToml) -> Self { + Self { + web_search: tools_toml.web_search, + view_image: tools_toml.view_image, + } + } +} + impl ConfigToml { /// Derive the effective sandbox policy from the configuration. fn derive_sandbox_policy(&self, sandbox_mode_override: Option) -> SandboxPolicy { diff --git a/codex-rs/core/src/config_profile.rs b/codex-rs/core/src/config_profile.rs index 0f25d3168d..d6ab03f60f 100644 --- a/codex-rs/core/src/config_profile.rs +++ b/codex-rs/core/src/config_profile.rs @@ -22,3 +22,18 @@ pub struct ConfigProfile { pub chatgpt_base_url: Option, pub experimental_instructions_file: Option, } + +impl From for codex_protocol::mcp_protocol::Profile { + fn from(config_profile: ConfigProfile) -> Self { + Self { + model: config_profile.model, + model_provider: config_profile.model_provider, + approval_policy: config_profile.approval_policy, + disable_response_storage: config_profile.disable_response_storage, + model_reasoning_effort: config_profile.model_reasoning_effort, + model_reasoning_summary: config_profile.model_reasoning_summary, + model_verbosity: config_profile.model_verbosity, + chatgpt_base_url: config_profile.chatgpt_base_url, + } + } +} diff --git a/codex-rs/core/src/config_types.rs b/codex-rs/core/src/config_types.rs index 7335fa7e85..27c2712be7 100644 --- a/codex-rs/core/src/config_types.rs +++ b/codex-rs/core/src/config_types.rs @@ -88,6 +88,17 @@ pub struct SandboxWorkspaceWrite { pub exclude_slash_tmp: bool, } +impl From for codex_protocol::mcp_protocol::SandboxSettings { + fn from(sandbox_workspace_write: SandboxWorkspaceWrite) -> Self { + Self { + writable_roots: sandbox_workspace_write.writable_roots, + network_access: Some(sandbox_workspace_write.network_access), + exclude_tmpdir_env_var: Some(sandbox_workspace_write.exclude_tmpdir_env_var), + exclude_slash_tmp: Some(sandbox_workspace_write.exclude_slash_tmp), + } + } +} + #[derive(Deserialize, Debug, Clone, PartialEq, Default)] #[serde(rename_all = "kebab-case")] pub enum ShellEnvironmentPolicyInherit { diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 9174403e52..1216e55922 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -45,7 +45,7 @@ use codex_protocol::mcp_protocol::ExecArbitraryCommandResponse; use codex_protocol::mcp_protocol::ExecCommandApprovalParams; use codex_protocol::mcp_protocol::ExecCommandApprovalResponse; use codex_protocol::mcp_protocol::ExecOneOffCommandParams; -use codex_protocol::mcp_protocol::GetConfigTomlResponse; +use codex_protocol::mcp_protocol::GetUserSavedConfigResponse; use codex_protocol::mcp_protocol::GitDiffToRemoteResponse; use codex_protocol::mcp_protocol::InputItem as WireInputItem; use codex_protocol::mcp_protocol::InterruptConversationParams; @@ -61,6 +61,7 @@ use codex_protocol::mcp_protocol::SendUserMessageResponse; use codex_protocol::mcp_protocol::SendUserTurnParams; use codex_protocol::mcp_protocol::SendUserTurnResponse; use codex_protocol::mcp_protocol::ServerNotification; +use codex_protocol::mcp_protocol::UserSavedConfig; use mcp_types::JSONRPCErrorError; use mcp_types::RequestId; use tokio::sync::Mutex; @@ -153,8 +154,8 @@ impl CodexMessageProcessor { ClientRequest::GetAuthStatus { request_id, params } => { self.get_auth_status(request_id, params).await; } - ClientRequest::GetConfigToml { request_id } => { - self.get_config_toml(request_id).await; + ClientRequest::GetUserSavedConfig { request_id } => { + self.get_user_saved_config(request_id).await; } ClientRequest::ExecOneOffCommand { request_id, params } => { self.exec_one_off_command(request_id, params).await; @@ -371,7 +372,7 @@ impl CodexMessageProcessor { self.outgoing.send_response(request_id, response).await; } - async fn get_config_toml(&self, request_id: RequestId) { + async fn get_user_saved_config(&self, request_id: RequestId) { let toml_value = match load_config_as_toml(&self.config.codex_home) { Ok(val) => val, Err(err) => { @@ -398,32 +399,11 @@ impl CodexMessageProcessor { } }; - let profiles: HashMap = cfg - .profiles - .into_iter() - .map(|(k, v)| { - ( - k, - // Define this explicitly here to avoid the need to - // implement `From` - // for the `ConfigProfile` type and introduce a dependency on codex_core - codex_protocol::config_types::ConfigProfile { - model: v.model, - approval_policy: v.approval_policy, - model_reasoning_effort: v.model_reasoning_effort, - }, - ) - }) - .collect(); + let user_saved_config: UserSavedConfig = cfg.into(); - let response = GetConfigTomlResponse { - approval_policy: cfg.approval_policy, - sandbox_mode: cfg.sandbox_mode, - model_reasoning_effort: cfg.model_reasoning_effort, - profile: cfg.profile, - profiles: Some(profiles), + let response = GetUserSavedConfigResponse { + config: user_saved_config, }; - self.outgoing.send_response(request_id, response).await; } diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 14939156bb..1355446342 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -240,9 +240,9 @@ impl McpProcess { self.send_request("getAuthStatus", params).await } - /// Send a `getConfigToml` JSON-RPC request. - pub async fn send_get_config_toml_request(&mut self) -> anyhow::Result { - self.send_request("getConfigToml", None).await + /// Send a `getUserSavedConfig` JSON-RPC request. + pub async fn send_get_user_saved_config_request(&mut self) -> anyhow::Result { + self.send_request("getUserSavedConfig", None).await } /// Send a `loginChatGpt` JSON-RPC request. diff --git a/codex-rs/mcp-server/tests/suite/config.rs b/codex-rs/mcp-server/tests/suite/config.rs index cec41b25e0..809d426b24 100644 --- a/codex-rs/mcp-server/tests/suite/config.rs +++ b/codex-rs/mcp-server/tests/suite/config.rs @@ -2,10 +2,15 @@ use std::collections::HashMap; use std::path::Path; use codex_core::protocol::AskForApproval; -use codex_protocol::config_types::ConfigProfile; use codex_protocol::config_types::ReasoningEffort; +use codex_protocol::config_types::ReasoningSummary; use codex_protocol::config_types::SandboxMode; -use codex_protocol::mcp_protocol::GetConfigTomlResponse; +use codex_protocol::config_types::Verbosity; +use codex_protocol::mcp_protocol::GetUserSavedConfigResponse; +use codex_protocol::mcp_protocol::Profile; +use codex_protocol::mcp_protocol::SandboxSettings; +use codex_protocol::mcp_protocol::Tools; +use codex_protocol::mcp_protocol::UserSavedConfig; use mcp_test_support::McpProcess; use mcp_test_support::to_response; use mcp_types::JSONRPCResponse; @@ -21,22 +26,39 @@ fn create_config_toml(codex_home: &Path) -> std::io::Result<()> { std::fs::write( config_toml, r#" +model = "gpt-5" approval_policy = "on-request" sandbox_mode = "workspace-write" +model_reasoning_summary = "detailed" model_reasoning_effort = "high" +model_verbosity = "medium" profile = "test" +[sandbox_workspace_write] +writable_roots = ["/tmp"] +network_access = true +exclude_tmpdir_env_var = true +exclude_slash_tmp = true + +[tools] +web_search = false +view_image = true + [profiles.test] model = "gpt-4o" approval_policy = "on-request" model_reasoning_effort = "high" model_reasoning_summary = "detailed" +model_verbosity = "medium" +model_provider = "openai" +disable_response_storage = false +chatgpt_base_url = "https://api.chatgpt.com" "#, ) } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn get_config_toml_returns_subset() { +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn get_config_toml_parses_all_fields() { let codex_home = TempDir::new().unwrap_or_else(|e| panic!("create tempdir: {e}")); create_config_toml(codex_home.path()).expect("write config.toml"); @@ -49,32 +71,95 @@ async fn get_config_toml_returns_subset() { .expect("init failed"); let request_id = mcp - .send_get_config_toml_request() + .send_get_user_saved_config_request() .await - .expect("send getConfigToml"); + .expect("send getUserSavedConfig"); let resp: JSONRPCResponse = timeout( DEFAULT_READ_TIMEOUT, mcp.read_stream_until_response_message(RequestId::Integer(request_id)), ) .await - .expect("getConfigToml timeout") - .expect("getConfigToml response"); + .expect("getUserSavedConfig timeout") + .expect("getUserSavedConfig response"); - let config: GetConfigTomlResponse = to_response(resp).expect("deserialize config"); - let expected = GetConfigTomlResponse { - approval_policy: Some(AskForApproval::OnRequest), - sandbox_mode: Some(SandboxMode::WorkspaceWrite), - model_reasoning_effort: Some(ReasoningEffort::High), - profile: Some("test".to_string()), - profiles: Some(HashMap::from([( - "test".into(), - ConfigProfile { - model: Some("gpt-4o".into()), - approval_policy: Some(AskForApproval::OnRequest), - model_reasoning_effort: Some(ReasoningEffort::High), - }, - )])), + let config: GetUserSavedConfigResponse = to_response(resp).expect("deserialize config"); + let expected = GetUserSavedConfigResponse { + config: UserSavedConfig { + approval_policy: Some(AskForApproval::OnRequest), + sandbox_mode: Some(SandboxMode::WorkspaceWrite), + sandbox_settings: Some(SandboxSettings { + writable_roots: vec!["/tmp".into()], + network_access: Some(true), + exclude_tmpdir_env_var: Some(true), + exclude_slash_tmp: Some(true), + }), + model: Some("gpt-5".into()), + model_reasoning_effort: Some(ReasoningEffort::High), + model_reasoning_summary: Some(ReasoningSummary::Detailed), + model_verbosity: Some(Verbosity::Medium), + tools: Some(Tools { + web_search: Some(false), + view_image: Some(true), + }), + profile: Some("test".to_string()), + profiles: HashMap::from([( + "test".into(), + Profile { + model: Some("gpt-4o".into()), + approval_policy: Some(AskForApproval::OnRequest), + model_reasoning_effort: Some(ReasoningEffort::High), + model_reasoning_summary: Some(ReasoningSummary::Detailed), + model_verbosity: Some(Verbosity::Medium), + model_provider: Some("openai".into()), + disable_response_storage: Some(false), + chatgpt_base_url: Some("https://api.chatgpt.com".into()), + }, + )]), + }, }; - assert_eq!(expected, config); + assert_eq!(config, expected); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn get_config_toml_empty() { + let codex_home = TempDir::new().unwrap_or_else(|e| panic!("create tempdir: {e}")); + + let mut mcp = McpProcess::new(codex_home.path()) + .await + .expect("spawn mcp process"); + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()) + .await + .expect("init timeout") + .expect("init failed"); + + let request_id = mcp + .send_get_user_saved_config_request() + .await + .expect("send getUserSavedConfig"); + let resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await + .expect("getUserSavedConfig timeout") + .expect("getUserSavedConfig response"); + + let config: GetUserSavedConfigResponse = to_response(resp).expect("deserialize config"); + let expected = GetUserSavedConfigResponse { + config: UserSavedConfig { + approval_policy: None, + sandbox_mode: None, + sandbox_settings: None, + model: None, + model_reasoning_effort: None, + model_reasoning_summary: None, + model_verbosity: None, + tools: None, + profile: None, + profiles: HashMap::new(), + }, + }; + + assert_eq!(config, expected); } diff --git a/codex-rs/protocol-ts/src/lib.rs b/codex-rs/protocol-ts/src/lib.rs index 67c8b156ad..cdc63e4efa 100644 --- a/codex-rs/protocol-ts/src/lib.rs +++ b/codex-rs/protocol-ts/src/lib.rs @@ -46,6 +46,7 @@ pub fn generate_ts(out_dir: &Path, prettier: Option<&Path>) -> Result<()> { codex_protocol::mcp_protocol::ApplyPatchApprovalResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ExecCommandApprovalParams::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ExecCommandApprovalResponse::export_all_to(out_dir)?; + codex_protocol::mcp_protocol::GetUserSavedConfigResponse::export_all_to(out_dir)?; codex_protocol::mcp_protocol::ServerNotification::export_all_to(out_dir)?; generate_index_ts(out_dir)?; diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs index 510c64090c..633c845dd2 100644 --- a/codex-rs/protocol/src/config_types.rs +++ b/codex-rs/protocol/src/config_types.rs @@ -4,8 +4,6 @@ use strum_macros::Display; use strum_macros::EnumIter; use ts_rs::TS; -use crate::protocol::AskForApproval; - /// See https://platform.openai.com/docs/guides/reasoning?api-mode=responses#get-started-with-reasoning #[derive( Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display, TS, EnumIter, @@ -37,7 +35,7 @@ pub enum ReasoningSummary { /// Controls output length/detail on GPT-5 models via the Responses API. /// Serialized with lowercase values to match the OpenAI API. -#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display)] +#[derive(Debug, Serialize, Deserialize, Default, Clone, Copy, PartialEq, Eq, Display, TS)] #[serde(rename_all = "lowercase")] #[strum(serialize_all = "lowercase")] pub enum Verbosity { @@ -61,13 +59,3 @@ pub enum SandboxMode { #[serde(rename = "danger-full-access")] DangerFullAccess, } - -/// Collection of common configuration options that a user can define as a unit -/// in `config.toml`. Currently only a subset of the fields are supported. -#[derive(Deserialize, Debug, Clone, PartialEq, Serialize, TS)] -#[serde(rename_all = "camelCase")] -pub struct ConfigProfile { - pub model: Option, - pub approval_policy: Option, - pub model_reasoning_effort: Option, -} diff --git a/codex-rs/protocol/src/mcp_protocol.rs b/codex-rs/protocol/src/mcp_protocol.rs index e1797b91ad..7083bb1ad3 100644 --- a/codex-rs/protocol/src/mcp_protocol.rs +++ b/codex-rs/protocol/src/mcp_protocol.rs @@ -2,10 +2,10 @@ use std::collections::HashMap; use std::fmt::Display; use std::path::PathBuf; -use crate::config_types::ConfigProfile; use crate::config_types::ReasoningEffort; use crate::config_types::ReasoningSummary; use crate::config_types::SandboxMode; +use crate::config_types::Verbosity; use crate::protocol::AskForApproval; use crate::protocol::FileChange; use crate::protocol::ReviewDecision; @@ -102,7 +102,7 @@ pub enum ClientRequest { request_id: RequestId, params: GetAuthStatusParams, }, - GetConfigToml { + GetUserSavedConfig { #[serde(rename = "id")] request_id: RequestId, }, @@ -260,22 +260,82 @@ pub struct GetAuthStatusResponse { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] #[serde(rename_all = "camelCase")] -pub struct GetConfigTomlResponse { +pub struct GetUserSavedConfigResponse { + pub config: UserSavedConfig, +} + +/// UserSavedConfig contains a subset of the config. It is meant to expose mcp +/// client-configurable settings that can be specified in the NewConversation +/// and SendUserTurn requests. +#[derive(Deserialize, Debug, Clone, PartialEq, Serialize, TS)] +#[serde(rename_all = "camelCase")] +pub struct UserSavedConfig { /// Approvals #[serde(skip_serializing_if = "Option::is_none")] pub approval_policy: Option, #[serde(skip_serializing_if = "Option::is_none")] pub sandbox_mode: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub sandbox_settings: Option, - /// Relevant model configuration + /// Model-specific configuration + #[serde(skip_serializing_if = "Option::is_none")] + pub model: Option, #[serde(skip_serializing_if = "Option::is_none")] pub model_reasoning_effort: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub model_reasoning_summary: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub model_verbosity: Option, + + /// Tools + #[serde(skip_serializing_if = "Option::is_none")] + pub tools: Option, /// Profiles #[serde(skip_serializing_if = "Option::is_none")] pub profile: Option, + #[serde(default)] + pub profiles: HashMap, +} + +/// MCP representation of a [`codex_core::config_profile::ConfigProfile`]. +#[derive(Deserialize, Debug, Clone, PartialEq, Serialize, TS)] +#[serde(rename_all = "camelCase")] +pub struct Profile { + pub model: Option, + /// The key in the `model_providers` map identifying the + /// [`ModelProviderInfo`] to use. + pub model_provider: Option, + pub approval_policy: Option, + pub disable_response_storage: Option, + pub model_reasoning_effort: Option, + pub model_reasoning_summary: Option, + pub model_verbosity: Option, + pub chatgpt_base_url: Option, +} +/// MCP representation of a [`codex_core::config::ToolsToml`]. +#[derive(Deserialize, Debug, Clone, PartialEq, Serialize, TS)] +#[serde(rename_all = "camelCase")] +pub struct Tools { #[serde(skip_serializing_if = "Option::is_none")] - pub profiles: Option>, + pub web_search: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub view_image: Option, +} + +/// MCP representation of a [`codex_core::config_types::SandboxWorkspaceWrite`]. +#[derive(Deserialize, Debug, Clone, PartialEq, Serialize, TS)] +#[serde(rename_all = "camelCase")] +pub struct SandboxSettings { + #[serde(default)] + pub writable_roots: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + pub network_access: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub exclude_tmpdir_env_var: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub exclude_slash_tmp: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] From b795fbe244e8774c9ff72332c71e4d2bbff6ce13 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 4 Sep 2025 16:30:34 -0700 Subject: [PATCH 05/13] chore(deps): bump uuid from 1.17.0 to 1.18.0 in /codex-rs (#2493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [uuid](https://github.com/uuid-rs/uuid) from 1.17.0 to 1.18.0.
Release notes

Sourced from uuid's releases.

v1.18.0

What's Changed

New Contributors

Full Changelog: https://github.com/uuid-rs/uuid/compare/v1.17.0...v1.18.0

Commits
  • 60a49eb Merge pull request #839 from uuid-rs/cargo/v1.18.0
  • eb8c697 prepare for 1.18.0 release
  • 281f26f Merge pull request #838 from uuid-rs/chore/time-conversion
  • 2d67ab2 don't use allocated values in errors
  • c284ed5 wrap the error type used in time conversions
  • 87a4359 Merge pull request #835 from dcormier/main
  • 8927396 Merge pull request #837 from uuid-rs/fix/lifetime-syntaxes
  • 6dfb4b1 Conversions between Timestamp and std::time::SystemTime
  • b508383 fix up mismatched_lifetime_syntaxes lint
  • See full diff in compare view

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=uuid&package-manager=cargo&previous-version=1.17.0&new-version=1.18.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- codex-rs/Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 83eac74d3e..eda83be76b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -5535,9 +5535,9 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" [[package]] name = "uuid" -version = "1.17.0" +version = "1.18.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3cf4199d1e5d15ddd86a694e4d0dffa9c323ce759fea589f00fef9d81cc1931d" +checksum = "2f87b8aa10b915a06587d0dec516c282ff295b475d94abf425d62b57710070a2" dependencies = [ "getrandom 0.3.3", "js-sys", From 7df9e9c6645823b27e460e99ee44d88334cf5c9b Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 4 Sep 2025 16:34:14 -0700 Subject: [PATCH 06/13] Correctly calculate remaining context size (#3190) We had multiple issues with context size calculation: 1. `initial_prompt_tokens` calculation based on cache size is not reliable, cache misses might set it to much higher value. For now hardcoded to a safer constant. 2. Input context size for GPT-5 is 272k (that's where 33% came from). Fixes. --- codex-rs/core/src/config.rs | 2 +- codex-rs/core/src/openai_model_info.rs | 4 ++-- codex-rs/protocol/src/protocol.rs | 17 +++++++-------- codex-rs/tui/src/bottom_pane/chat_composer.rs | 21 +------------------ 4 files changed, 12 insertions(+), 32 deletions(-) diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 993dc5a2ba..e022ba0188 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -1382,7 +1382,7 @@ model_verbosity = "high" let expected_gpt5_profile_config = Config { model: "gpt-5".to_string(), model_family: find_family_for_model("gpt-5").expect("known model slug"), - model_context_window: Some(400_000), + model_context_window: Some(272_000), model_max_output_tokens: Some(128_000), model_provider_id: "openai".to_string(), model_provider: fixture.openai_provider.clone(), diff --git a/codex-rs/core/src/openai_model_info.rs b/codex-rs/core/src/openai_model_info.rs index bf67ec4a11..9c2f24214e 100644 --- a/codex-rs/core/src/openai_model_info.rs +++ b/codex-rs/core/src/openai_model_info.rs @@ -79,12 +79,12 @@ pub(crate) fn get_model_info(model_family: &ModelFamily) -> Option { }), "gpt-5" => Some(ModelInfo { - context_window: 400_000, + context_window: 272_000, max_output_tokens: 128_000, }), _ if slug.starts_with("codex-") => Some(ModelInfo { - context_window: 400_000, + context_window: 272_000, max_output_tokens: 128_000, }), diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 3c40b33c1b..2308b7d212 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -527,6 +527,9 @@ pub struct TokenUsage { pub total_tokens: u64, } +// Includes prompts, tools and space to call compact. +const BASELINE_TOKENS: u64 = 12000; + impl TokenUsage { pub fn is_zero(&self) -> bool { self.total_tokens == 0 @@ -557,26 +560,22 @@ impl TokenUsage { /// Estimate the remaining user-controllable percentage of the model's context window. /// /// `context_window` is the total size of the model's context window. - /// `baseline_used_tokens` should capture tokens that are always present in + /// `BASELINE_TOKENS` should capture tokens that are always present in /// the context (e.g., system prompt and fixed tool instructions) so that /// the percentage reflects the portion the user can influence. /// /// This normalizes both the numerator and denominator by subtracting the /// baseline, so immediately after the first prompt the UI shows 100% left /// and trends toward 0% as the user fills the effective window. - pub fn percent_of_context_window_remaining( - &self, - context_window: u64, - baseline_used_tokens: u64, - ) -> u8 { - if context_window <= baseline_used_tokens { + pub fn percent_of_context_window_remaining(&self, context_window: u64) -> u8 { + if context_window <= BASELINE_TOKENS { return 0; } - let effective_window = context_window - baseline_used_tokens; + let effective_window = context_window - BASELINE_TOKENS; let used = self .tokens_in_context_window() - .saturating_sub(baseline_used_tokens); + .saturating_sub(BASELINE_TOKENS); let remaining = effective_window.saturating_sub(used); ((remaining as f32 / effective_window as f32) * 100.0).clamp(0.0, 100.0) as u8 } diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index dd784f9260..528290fa03 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -67,15 +67,6 @@ struct TokenUsageInfo { total_token_usage: TokenUsage, last_token_usage: TokenUsage, model_context_window: Option, - /// Baseline token count present in the context before the user's first - /// message content is considered. This is used to normalize the - /// "context left" percentage so it reflects the portion the user can - /// influence rather than fixed prompt overhead (system prompt, tool - /// instructions, etc.). - /// - /// Preferred source is `cached_input_tokens` from the first turn (when - /// available), otherwise we fall back to 0. - initial_prompt_tokens: u64, } pub(crate) struct ChatComposer { @@ -181,17 +172,10 @@ impl ChatComposer { last_token_usage: TokenUsage, model_context_window: Option, ) { - let initial_prompt_tokens = self - .token_usage_info - .as_ref() - .map(|info| info.initial_prompt_tokens) - .unwrap_or_else(|| last_token_usage.cached_input_tokens.unwrap_or(0)); - self.token_usage_info = Some(TokenUsageInfo { total_token_usage, last_token_usage, model_context_window, - initial_prompt_tokens, }); } @@ -1302,10 +1286,7 @@ impl WidgetRef for ChatComposer { let last_token_usage = &token_usage_info.last_token_usage; if let Some(context_window) = token_usage_info.model_context_window { let percent_remaining: u8 = if context_window > 0 { - last_token_usage.percent_of_context_window_remaining( - context_window, - token_usage_info.initial_prompt_tokens, - ) + last_token_usage.percent_of_context_window_remaining(context_window) } else { 100 }; From 907d3dd3488375c8630fc0759c339c941f4fdd57 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Thu, 4 Sep 2025 16:44:18 -0700 Subject: [PATCH 07/13] MCP: add session resume + history listing; (#3185) # External (non-OpenAI) Pull Request Requirements Before opening this Pull Request, please read the dedicated "Contributing" markdown file or your PR may be closed: https://github.com/openai/codex/blob/main/docs/contributing.md If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes. --- .../mcp-server/src/codex_message_processor.rs | 169 +++++++++++++++++ .../mcp-server/tests/common/mcp_process.rs | 20 ++ .../mcp-server/tests/suite/list_resume.rs | 172 ++++++++++++++++++ codex-rs/mcp-server/tests/suite/mod.rs | 1 + codex-rs/protocol/src/mcp_protocol.rs | 63 +++++++ 5 files changed, 425 insertions(+) create mode 100644 codex-rs/mcp-server/tests/suite/list_resume.rs diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 1216e55922..7de12febc2 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -11,7 +11,9 @@ use crate::outgoing_message::OutgoingNotification; use codex_core::AuthManager; use codex_core::CodexConversation; use codex_core::ConversationManager; +use codex_core::Cursor as RolloutCursor; use codex_core::NewConversation; +use codex_core::RolloutRecorder; use codex_core::auth::CLIENT_ID; use codex_core::config::Config; use codex_core::config::ConfigOverrides; @@ -40,6 +42,7 @@ use codex_protocol::mcp_protocol::AuthMode; use codex_protocol::mcp_protocol::AuthStatusChangeNotification; use codex_protocol::mcp_protocol::ClientRequest; use codex_protocol::mcp_protocol::ConversationId; +use codex_protocol::mcp_protocol::ConversationSummary; use codex_protocol::mcp_protocol::EXEC_COMMAND_APPROVAL_METHOD; use codex_protocol::mcp_protocol::ExecArbitraryCommandResponse; use codex_protocol::mcp_protocol::ExecCommandApprovalParams; @@ -50,12 +53,15 @@ use codex_protocol::mcp_protocol::GitDiffToRemoteResponse; use codex_protocol::mcp_protocol::InputItem as WireInputItem; use codex_protocol::mcp_protocol::InterruptConversationParams; use codex_protocol::mcp_protocol::InterruptConversationResponse; +use codex_protocol::mcp_protocol::ListConversationsParams; +use codex_protocol::mcp_protocol::ListConversationsResponse; use codex_protocol::mcp_protocol::LoginChatGptCompleteNotification; use codex_protocol::mcp_protocol::LoginChatGptResponse; use codex_protocol::mcp_protocol::NewConversationParams; use codex_protocol::mcp_protocol::NewConversationResponse; use codex_protocol::mcp_protocol::RemoveConversationListenerParams; use codex_protocol::mcp_protocol::RemoveConversationSubscriptionResponse; +use codex_protocol::mcp_protocol::ResumeConversationParams; use codex_protocol::mcp_protocol::SendUserMessageParams; use codex_protocol::mcp_protocol::SendUserMessageResponse; use codex_protocol::mcp_protocol::SendUserTurnParams; @@ -124,6 +130,12 @@ impl CodexMessageProcessor { // created before processing any subsequent messages. self.process_new_conversation(request_id, params).await; } + ClientRequest::ListConversations { request_id, params } => { + self.handle_list_conversations(request_id, params).await; + } + ClientRequest::ResumeConversation { request_id, params } => { + self.handle_resume_conversation(request_id, params).await; + } ClientRequest::SendUserMessage { request_id, params } => { self.send_user_message(request_id, params).await; } @@ -515,6 +527,128 @@ impl CodexMessageProcessor { } } + async fn handle_list_conversations( + &self, + request_id: RequestId, + params: ListConversationsParams, + ) { + let page_size = params.page_size.unwrap_or(25); + // Decode the optional cursor string to a Cursor via serde (Cursor implements Deserialize from string) + let cursor_obj: Option = match params.cursor { + Some(s) => serde_json::from_str::(&format!("\"{s}\"")).ok(), + None => None, + }; + let cursor_ref = cursor_obj.as_ref(); + + let page = match RolloutRecorder::list_conversations( + &self.config.codex_home, + page_size, + cursor_ref, + ) + .await + { + Ok(p) => p, + Err(err) => { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to list conversations: {err}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + } + }; + + // Build summaries + let mut items: Vec = Vec::new(); + for it in page.items.into_iter() { + let (timestamp, preview) = extract_ts_and_preview(&it.head); + items.push(ConversationSummary { + path: it.path, + preview, + timestamp, + }); + } + + // Encode next_cursor as a plain string + let next_cursor = match page.next_cursor { + Some(c) => match serde_json::to_value(&c) { + Ok(serde_json::Value::String(s)) => Some(s), + _ => None, + }, + None => None, + }; + + let response = ListConversationsResponse { items, next_cursor }; + self.outgoing.send_response(request_id, response).await; + } + + async fn handle_resume_conversation( + &self, + request_id: RequestId, + params: ResumeConversationParams, + ) { + // Derive a Config using the same logic as new conversation, honoring overrides if provided. + let config = match params.overrides { + Some(overrides) => { + derive_config_from_params(overrides, self.codex_linux_sandbox_exe.clone()) + } + None => Ok(self.config.as_ref().clone()), + }; + let config = match config { + Ok(cfg) => cfg, + Err(err) => { + let error = JSONRPCErrorError { + code: INVALID_REQUEST_ERROR_CODE, + message: format!("error deriving config: {err}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + } + }; + + match self + .conversation_manager + .resume_conversation_from_rollout( + config, + params.path.clone(), + self.auth_manager.clone(), + ) + .await + { + Ok(NewConversation { + conversation_id, + session_configured, + .. + }) => { + let event = codex_core::protocol::Event { + id: "".to_string(), + msg: codex_core::protocol::EventMsg::SessionConfigured( + session_configured.clone(), + ), + }; + self.outgoing.send_event_as_notification(&event, None).await; + + // Reply with conversation id + model and initial messages (when present) + let response = codex_protocol::mcp_protocol::ResumeConversationResponse { + conversation_id: ConversationId(conversation_id), + model: session_configured.model.clone(), + initial_messages: session_configured.initial_messages.clone(), + }; + self.outgoing.send_response(request_id, response).await; + } + Err(err) => { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("error resuming 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, @@ -951,3 +1085,38 @@ async fn on_exec_approval_response( error!("failed to submit ExecApproval: {err}"); } } + +fn extract_ts_and_preview(head: &[serde_json::Value]) -> (Option, String) { + let ts = head + .first() + .and_then(|v| v.get("timestamp")) + .and_then(|v| v.as_str()) + .map(|s| s.to_string()); + let preview = find_first_user_text(head).unwrap_or_default(); + (ts, preview) +} + +fn find_first_user_text(head: &[serde_json::Value]) -> Option { + use codex_core::protocol::InputMessageKind; + for v in head.iter() { + let t = v.get("type").and_then(|x| x.as_str()).unwrap_or(""); + if t != "message" { + continue; + } + if v.get("role").and_then(|x| x.as_str()) != Some("user") { + continue; + } + if let Some(arr) = v.get("content").and_then(|c| c.as_array()) { + for c in arr.iter() { + if let (Some("input_text"), Some(txt)) = + (c.get("type").and_then(|t| t.as_str()), c.get("text")) + && let Some(s) = txt.as_str() + && matches!(InputMessageKind::from(("user", s)), InputMessageKind::Plain) + { + return Some(s.to_string()); + } + } + } + } + None +} diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 1355446342..34b24ac171 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -16,8 +16,10 @@ use codex_protocol::mcp_protocol::AddConversationListenerParams; use codex_protocol::mcp_protocol::CancelLoginChatGptParams; use codex_protocol::mcp_protocol::GetAuthStatusParams; use codex_protocol::mcp_protocol::InterruptConversationParams; +use codex_protocol::mcp_protocol::ListConversationsParams; use codex_protocol::mcp_protocol::NewConversationParams; use codex_protocol::mcp_protocol::RemoveConversationListenerParams; +use codex_protocol::mcp_protocol::ResumeConversationParams; use codex_protocol::mcp_protocol::SendUserMessageParams; use codex_protocol::mcp_protocol::SendUserTurnParams; @@ -245,6 +247,24 @@ impl McpProcess { self.send_request("getUserSavedConfig", None).await } + /// Send a `listConversations` JSON-RPC request. + pub async fn send_list_conversations_request( + &mut self, + params: ListConversationsParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("listConversations", params).await + } + + /// Send a `resumeConversation` JSON-RPC request. + pub async fn send_resume_conversation_request( + &mut self, + params: ResumeConversationParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("resumeConversation", params).await + } + /// Send a `loginChatGpt` JSON-RPC request. pub async fn send_login_chat_gpt_request(&mut self) -> anyhow::Result { self.send_request("loginChatGpt", None).await diff --git a/codex-rs/mcp-server/tests/suite/list_resume.rs b/codex-rs/mcp-server/tests/suite/list_resume.rs new file mode 100644 index 0000000000..d835e5ca77 --- /dev/null +++ b/codex-rs/mcp-server/tests/suite/list_resume.rs @@ -0,0 +1,172 @@ +use std::fs; +use std::path::Path; + +use codex_protocol::mcp_protocol::ListConversationsParams; +use codex_protocol::mcp_protocol::ListConversationsResponse; +use codex_protocol::mcp_protocol::NewConversationParams; // reused for overrides shape +use codex_protocol::mcp_protocol::ResumeConversationParams; +use codex_protocol::mcp_protocol::ResumeConversationResponse; +use mcp_test_support::McpProcess; +use mcp_test_support::to_response; +use mcp_types::JSONRPCNotification; +use mcp_types::JSONRPCResponse; +use mcp_types::RequestId; +use pretty_assertions::assert_eq; +use serde_json::json; +use tempfile::TempDir; +use tokio::time::timeout; +use uuid::Uuid; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_list_and_resume_conversations() { + // Prepare a temporary CODEX_HOME with a few fake rollout files. + let codex_home = TempDir::new().expect("create temp dir"); + create_fake_rollout( + codex_home.path(), + "2025-01-02T12-00-00", + "2025-01-02T12:00:00Z", + "Hello A", + ); + create_fake_rollout( + codex_home.path(), + "2025-01-01T13-00-00", + "2025-01-01T13:00:00Z", + "Hello B", + ); + create_fake_rollout( + codex_home.path(), + "2025-01-01T12-00-00", + "2025-01-01T12:00:00Z", + "Hello C", + ); + + let mut mcp = McpProcess::new(codex_home.path()) + .await + .expect("spawn mcp process"); + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()) + .await + .expect("init timeout") + .expect("init failed"); + + // Request first page with size 2 + let req_id = mcp + .send_list_conversations_request(ListConversationsParams { + page_size: Some(2), + cursor: None, + }) + .await + .expect("send listConversations"); + let resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(req_id)), + ) + .await + .expect("listConversations timeout") + .expect("listConversations resp"); + let ListConversationsResponse { items, next_cursor } = + to_response::(resp).expect("deserialize response"); + + assert_eq!(items.len(), 2); + // Newest first; preview text should match + assert_eq!(items[0].preview, "Hello A"); + assert_eq!(items[1].preview, "Hello B"); + assert!(items[0].path.is_absolute()); + assert!(next_cursor.is_some()); + + // Request the next page using the cursor + let req_id2 = mcp + .send_list_conversations_request(ListConversationsParams { + page_size: Some(2), + cursor: next_cursor, + }) + .await + .expect("send listConversations page 2"); + let resp2: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(req_id2)), + ) + .await + .expect("listConversations page 2 timeout") + .expect("listConversations page 2 resp"); + let ListConversationsResponse { + items: items2, + next_cursor: next2, + .. + } = to_response::(resp2).expect("deserialize response"); + assert_eq!(items2.len(), 1); + assert_eq!(items2[0].preview, "Hello C"); + assert!(next2.is_some()); + + // Now resume one of the sessions and expect a SessionConfigured notification and response. + let resume_req_id = mcp + .send_resume_conversation_request(ResumeConversationParams { + path: items[0].path.clone(), + overrides: Some(NewConversationParams { + model: Some("o3".to_string()), + ..Default::default() + }), + }) + .await + .expect("send resumeConversation"); + + // Expect a codex/event notification with msg.type == session_configured + let notification: JSONRPCNotification = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event"), + ) + .await + .expect("session_configured notification timeout") + .expect("session_configured notification"); + // Basic shape assertion: ensure event type is session_configured + let msg_type = notification + .params + .as_ref() + .and_then(|p| p.get("msg")) + .and_then(|m| m.get("type")) + .and_then(|t| t.as_str()) + .unwrap_or(""); + assert_eq!(msg_type, "session_configured"); + + // Then the response for resumeConversation + let resume_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(resume_req_id)), + ) + .await + .expect("resumeConversation timeout") + .expect("resumeConversation resp"); + let ResumeConversationResponse { + conversation_id, .. + } = to_response::(resume_resp) + .expect("deserialize resumeConversation response"); + // conversation id should be a valid UUID + let _ = uuid::Uuid::from_bytes(conversation_id.0.into_bytes()); +} + +fn create_fake_rollout(codex_home: &Path, filename_ts: &str, meta_rfc3339: &str, preview: &str) { + let uuid = Uuid::new_v4(); + // sessions/YYYY/MM/DD/ derived from filename_ts (YYYY-MM-DDThh-mm-ss) + let year = &filename_ts[0..4]; + let month = &filename_ts[5..7]; + let day = &filename_ts[8..10]; + let dir = codex_home.join("sessions").join(year).join(month).join(day); + fs::create_dir_all(&dir).unwrap_or_else(|e| panic!("create sessions dir: {e}")); + + 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}).to_string()); + // Minimal user message entry as a persisted response item + lines.push( + json!({ + "type":"message", + "role":"user", + "content":[{"type":"input_text","text": preview}] + }) + .to_string(), + ); + fs::write(file_path, lines.join("\n") + "\n") + .unwrap_or_else(|e| panic!("write rollout file: {e}")); +} diff --git a/codex-rs/mcp-server/tests/suite/mod.rs b/codex-rs/mcp-server/tests/suite/mod.rs index fb6d1cefac..138e11a4af 100644 --- a/codex-rs/mcp-server/tests/suite/mod.rs +++ b/codex-rs/mcp-server/tests/suite/mod.rs @@ -5,5 +5,6 @@ mod codex_tool; mod config; mod create_conversation; mod interrupt; +mod list_resume; mod login; mod send_message; diff --git a/codex-rs/protocol/src/mcp_protocol.rs b/codex-rs/protocol/src/mcp_protocol.rs index 7083bb1ad3..a5a646fabf 100644 --- a/codex-rs/protocol/src/mcp_protocol.rs +++ b/codex-rs/protocol/src/mcp_protocol.rs @@ -7,6 +7,7 @@ use crate::config_types::ReasoningSummary; use crate::config_types::SandboxMode; use crate::config_types::Verbosity; use crate::protocol::AskForApproval; +use crate::protocol::EventMsg; use crate::protocol::FileChange; use crate::protocol::ReviewDecision; use crate::protocol::SandboxPolicy; @@ -54,6 +55,18 @@ pub enum ClientRequest { request_id: RequestId, params: NewConversationParams, }, + /// List recorded Codex conversations (rollouts) with optional pagination and search. + ListConversations { + #[serde(rename = "id")] + request_id: RequestId, + params: ListConversationsParams, + }, + /// Resume a recorded Codex conversation from a rollout file. + ResumeConversation { + #[serde(rename = "id")] + request_id: RequestId, + params: ResumeConversationParams, + }, SendUserMessage { #[serde(rename = "id")] request_id: RequestId, @@ -164,6 +177,56 @@ pub struct NewConversationResponse { pub model: String, } +#[derive(Serialize, Deserialize, Debug, Clone)] +#[serde(rename_all = "camelCase")] +pub struct ResumeConversationResponse { + pub conversation_id: ConversationId, + pub model: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub initial_messages: Option>, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, TS)] +#[serde(rename_all = "camelCase")] +pub struct ListConversationsParams { + /// Optional page size; defaults to a reasonable server-side value. + #[serde(skip_serializing_if = "Option::is_none")] + pub page_size: Option, + /// Opaque pagination cursor returned by a previous call. + #[serde(skip_serializing_if = "Option::is_none")] + pub cursor: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] +#[serde(rename_all = "camelCase")] +pub struct ConversationSummary { + pub path: PathBuf, + pub preview: String, + /// RFC3339 timestamp string for the session start, if available. + #[serde(skip_serializing_if = "Option::is_none")] + pub timestamp: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] +#[serde(rename_all = "camelCase")] +pub struct ListConversationsResponse { + pub items: Vec, + /// Opaque cursor to pass to the next call to continue after the last item. + /// if None, there are no more items to return. + #[serde(skip_serializing_if = "Option::is_none")] + pub next_cursor: Option, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] +#[serde(rename_all = "camelCase")] +pub struct ResumeConversationParams { + /// Absolute path to the rollout JSONL file. + pub path: PathBuf, + /// Optional overrides to apply when spawning the resumed session. + #[serde(skip_serializing_if = "Option::is_none")] + pub overrides: Option, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS)] #[serde(rename_all = "camelCase")] pub struct AddConversationSubscriptionResponse { From 742feaf40f5fd03796afcf17f8f500c0706cc78c Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 4 Sep 2025 16:54:53 -0700 Subject: [PATCH 08/13] tui: fix approval dialog for large commands (#3087) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Summary - Emit a “Proposed Command” history cell when an ExecApprovalRequest arrives (parity with proposed patches). - Simplify the approval dialog: show only the reason/instructions; move the command preview into history. - Make approval/abort decision history concise: - Single line snippet; if multiline, show first line + " ...". - Truncate to 80 graphemes with ellipsis for very long commands. #### Details - History - Add `new_proposed_command` to render a header and indented command preview. - Use shared `prefix_lines` helper for first/subsequent line prefixes. - Approval UI - `UserApprovalWidget` no longer renders the command in the modal; shows optional `reason` text only. - Decision history renders an inline, dimmed snippet per rules above. - Tests (snapshot-based) - Proposed/decision flow for short command. - Proposed multi-line + aborted decision snippet with “ ...”. - Very long one-line command -> truncated snippet with “…”. - Updated existing exec approval snapshots and test reasons. Screenshot 2025-09-03 at 11 57 35 AM after approving: Screenshot 2025-09-03 at 11 58 18 AM rejection: Screenshot 2025-09-03 at 11 58 45 AM big command: https://github.com/user-attachments/assets/2dd976e5-799f-4af7-9682-a046e66cc494 --- codex-rs/tui/src/chatwidget.rs | 2 + ...hatwidget__tests__approval_modal_exec.snap | 4 +- ..._tests__approval_modal_exec_no_reason.snap | 2 - ...pproval_history_decision_aborted_long.snap | 5 + ...al_history_decision_aborted_multiline.snap | 6 + ...roval_history_decision_approved_short.snap | 6 + ...c_approval_history_proposed_multiline.snap | 7 + ..._exec_approval_history_proposed_short.snap | 6 + ...sts__status_widget_and_approval_modal.snap | 4 +- codex-rs/tui/src/chatwidget/tests.rs | 106 ++++++++++++++- codex-rs/tui/src/history_cell.rs | 49 ++++--- codex-rs/tui/src/render/line_utils.rs | 23 ++++ codex-rs/tui/src/user_approval_widget.rs | 126 +++++++----------- 13 files changed, 230 insertions(+), 116 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e8381c6f4d..7acb802c73 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -524,6 +524,8 @@ impl ChatWidget { pub(crate) fn handle_exec_approval_now(&mut self, id: String, ev: ExecApprovalRequestEvent) { self.flush_answer_stream_with_separator(); + // Emit the proposed command into history (like proposed patches) + self.add_to_history(history_cell::new_proposed_command(&ev.command)); let request = ApprovalRequest::Exec { id, diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap index e190ef4d0c..f306381f4a 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap @@ -4,9 +4,7 @@ assertion_line: 728 expression: terminal.backend() --- " " -"? Codex wants to run echo hello world " -" " -"Model wants to run a command " +"this is a test reason such as one that would be produced by the model " " " "▌Allow command? " "▌ Yes Always No, provide feedback " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap index f8aee6393d..fb1ecce7a6 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap @@ -3,8 +3,6 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend() --- " " -"? Codex wants to run echo hello world " -" " "▌Allow command? " "▌ Yes Always No, provide feedback " "▌ Approve and run the command " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap new file mode 100644 index 0000000000..524e096824 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&aborted_long) +--- +✗ You canceled the request to run echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa... diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap new file mode 100644 index 0000000000..d35cb17597 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&aborted_multi) +--- +✗ You canceled the request to run echo line1 ... + diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap new file mode 100644 index 0000000000..1b18a23d4d --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&decision) +--- +✔ You approved codex to run echo hello world this time + diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap new file mode 100644 index 0000000000..119965df12 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap @@ -0,0 +1,7 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&proposed_multi) +--- +• Proposed Command + └ echo line1 + echo line2 diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap new file mode 100644 index 0000000000..dfc52c0162 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&proposed) +--- +• Proposed Command + └ echo hello world diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap index 645ddb8e9f..63fb213e1b 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap @@ -4,9 +4,7 @@ assertion_line: 921 expression: terminal.backend() --- " " -"? Codex wants to run echo 'hello world' " -" " -"Codex wants to run a command " +"this is a test reason such as one that would be produced by the model " " " "▌Allow command? " "▌ Yes Always No, provide feedback " diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 287ce70664..555ce50dbc 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -265,6 +265,104 @@ fn lines_to_single_string(lines: &[ratatui::text::Line<'static>]) -> String { s } +// (removed experimental resize snapshot test) + +#[test] +fn exec_approval_emits_proposed_command_and_decision_history() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + + // Trigger an exec approval request with a short, single-line command + let ev = ExecApprovalRequestEvent { + call_id: "call-short".into(), + command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), + }; + chat.handle_codex_event(Event { + id: "sub-short".into(), + msg: EventMsg::ExecApprovalRequest(ev), + }); + + // Snapshot the Proposed Command cell emitted into history + let proposed = drain_insert_history(&mut rx) + .pop() + .expect("expected proposed command cell"); + assert_snapshot!( + "exec_approval_history_proposed_short", + lines_to_single_string(&proposed) + ); + + // Approve via keyboard and verify a concise decision history line is added + chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "exec_approval_history_decision_approved_short", + lines_to_single_string(&decision) + ); +} + +#[test] +fn exec_approval_decision_truncates_multiline_and_long_commands() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + + // Multiline command: should render proposed command fully in history with prefixes + let ev_multi = ExecApprovalRequestEvent { + call_id: "call-multi".into(), + command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), + }; + chat.handle_codex_event(Event { + id: "sub-multi".into(), + msg: EventMsg::ExecApprovalRequest(ev_multi), + }); + let proposed_multi = drain_insert_history(&mut rx) + .pop() + .expect("expected proposed multiline command cell"); + assert_snapshot!( + "exec_approval_history_proposed_multiline", + lines_to_single_string(&proposed_multi) + ); + + // Deny via keyboard; decision snippet should be single-line and elided with " ..." + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + let aborted_multi = drain_insert_history(&mut rx) + .pop() + .expect("expected aborted decision cell (multiline)"); + assert_snapshot!( + "exec_approval_history_decision_aborted_multiline", + lines_to_single_string(&aborted_multi) + ); + + // Very long single-line command: decision snippet should be truncated <= 80 chars with trailing ... + let long = format!("echo {}", "a".repeat(200)); + let ev_long = ExecApprovalRequestEvent { + call_id: "call-long".into(), + command: vec!["bash".into(), "-lc".into(), long.clone()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: None, + }; + chat.handle_codex_event(Event { + id: "sub-long".into(), + msg: EventMsg::ExecApprovalRequest(ev_long), + }); + drain_insert_history(&mut rx); // proposed cell not needed for this assertion + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + let aborted_long = drain_insert_history(&mut rx) + .pop() + .expect("expected aborted decision cell (long)"); + assert_snapshot!( + "exec_approval_history_decision_aborted_long", + lines_to_single_string(&aborted_long) + ); +} + // --- Small helpers to tersely drive exec begin/end and snapshot active cell --- fn begin_exec(chat: &mut ChatWidget, call_id: &str, raw_cmd: &str) { // Build the full command vec and parse it using core's parser, @@ -714,7 +812,9 @@ fn approval_modal_exec_snapshot() { call_id: "call-approve-cmd".into(), command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), - reason: Some("Model wants to run a command".into()), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), }; chat.handle_codex_event(Event { id: "sub-approve".into(), @@ -907,7 +1007,9 @@ fn status_widget_and_approval_modal_snapshot() { call_id: "call-approve-exec".into(), command: vec!["echo".into(), "hello world".into()], cwd: std::path::PathBuf::from("/tmp"), - reason: Some("Codex wants to run a command".into()), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), }; chat.handle_codex_event(Event { id: "sub-approve-exec".into(), diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 798a87dc11..ce048f4875 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -2,6 +2,7 @@ use crate::diff_render::create_diff_summary; use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; use crate::markdown::append_markdown; +use crate::render::line_utils::prefix_lines; use crate::slash_command::SlashCommand; use crate::text_formatting::format_and_truncate_tool_result; use base64::Engine; @@ -1140,33 +1141,9 @@ impl HistoryCell for PlanUpdateCell { .into_iter() .map(|s| s.to_string().set_style(step_style).into()) .collect(); - prefix_lines(step_text, &box_str.into(), &" ".into()) + prefix_lines(step_text, box_str.into(), " ".into()) }; - fn prefix_lines( - lines: Vec>, - initial_prefix: &Span<'static>, - subsequent_prefix: &Span<'static>, - ) -> Vec> { - lines - .into_iter() - .enumerate() - .map(|(i, l)| { - Line::from( - [ - vec![if i == 0 { - initial_prefix.clone() - } else { - subsequent_prefix.clone() - }], - l.spans, - ] - .concat(), - ) - }) - .collect() - } - let mut lines: Vec> = vec![]; lines.push(vec!["• ".into(), "Updated Plan".bold()].into()); @@ -1187,7 +1164,7 @@ impl HistoryCell for PlanUpdateCell { indented_lines.extend(render_step(status, step)); } } - lines.extend(prefix_lines(indented_lines, &" └ ".into(), &" ".into())); + lines.extend(prefix_lines(indented_lines, " └ ".into(), " ".into())); lines } @@ -1231,6 +1208,26 @@ pub(crate) fn new_patch_apply_failure(stderr: String) -> PlainHistoryCell { PlainHistoryCell { lines } } +/// Create a new history cell for a proposed command approval. +/// Renders a header and the command preview similar to how proposed patches +/// show a header and summary. +pub(crate) fn new_proposed_command(command: &[String]) -> PlainHistoryCell { + let cmd = strip_bash_lc_and_escape(command); + + let mut lines: Vec> = Vec::new(); + lines.push(Line::from(vec!["• ".into(), "Proposed Command".bold()])); + + let cmd_lines: Vec> = cmd + .lines() + .map(|part| Line::from(part.to_string())) + .collect(); + let initial_prefix: Span<'static> = " └ ".dim(); + let subsequent_prefix: Span<'static> = " ".into(); + lines.extend(prefix_lines(cmd_lines, initial_prefix, subsequent_prefix)); + + PlainHistoryCell { lines } +} + pub(crate) fn new_reasoning_block( full_reasoning_buffer: String, config: &Config, diff --git a/codex-rs/tui/src/render/line_utils.rs b/codex-rs/tui/src/render/line_utils.rs index 1cc9d5c95a..5a8e09068a 100644 --- a/codex-rs/tui/src/render/line_utils.rs +++ b/codex-rs/tui/src/render/line_utils.rs @@ -34,3 +34,26 @@ pub fn is_blank_line_spaces_only(line: &Line<'_>) -> bool { .iter() .all(|s| s.content.is_empty() || s.content.chars().all(|c| c == ' ')) } + +/// Prefix each line with `initial_prefix` for the first line and +/// `subsequent_prefix` for following lines. Returns a new Vec of owned lines. +pub fn prefix_lines( + lines: Vec>, + initial_prefix: Span<'static>, + subsequent_prefix: Span<'static>, +) -> Vec> { + lines + .into_iter() + .enumerate() + .map(|(i, l)| { + let mut spans = Vec::with_capacity(l.spans.len() + 1); + spans.push(if i == 0 { + initial_prefix.clone() + } else { + subsequent_prefix.clone() + }); + spans.extend(l.spans); + Line::from(spans) + }) + .collect() +} diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index 9ea2c0bc46..4f1739554a 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -30,6 +30,7 @@ use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::exec_command::strip_bash_lc_and_escape; use crate::history_cell; +use crate::text_formatting::truncate_text; /// Request coming from the agent that needs user approval. pub(crate) enum ApprovalRequest { @@ -110,45 +111,11 @@ pub(crate) struct UserApprovalWidget { done: bool, } -fn to_command_display<'a>( - first_line: Vec>, - cmd: String, - last_line: Vec>, -) -> Vec> { - let command_lines: Vec = cmd.lines().map(|line| line.to_string().dim()).collect(); - - let mut lines: Vec> = vec![]; - - let mut first_line = first_line.clone(); - if command_lines.len() == 1 { - first_line.push(command_lines[0].clone()); - first_line.extend(last_line); - } else { - for line in command_lines { - lines.push(vec![" ".into(), line].into()); - } - let last_line = last_line.clone(); - lines.push(Line::from(last_line)); - } - lines.insert(0, Line::from(first_line)); - - lines -} - impl UserApprovalWidget { pub(crate) fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self { let confirmation_prompt = match &approval_request { - ApprovalRequest::Exec { - command, reason, .. - } => { - let cmd = strip_bash_lc_and_escape(command); - let mut contents: Vec = to_command_display( - vec!["? ".fg(Color::Cyan), "Codex wants to run ".bold()], - cmd, - vec![], - ); - - contents.push(Line::from("")); + ApprovalRequest::Exec { reason, .. } => { + let mut contents: Vec = vec![]; if let Some(reason) = reason { contents.push(Line::from(reason.clone().italic())); contents.push(Line::from("")); @@ -258,62 +225,61 @@ impl UserApprovalWidget { fn send_decision_with_feedback(&mut self, decision: ReviewDecision, feedback: String) { match &self.approval_request { ApprovalRequest::Exec { command, .. } => { - let cmd = strip_bash_lc_and_escape(command); - // TODO: move this rendering into history_cell. - let mut lines: Vec> = vec![]; + let full_cmd = strip_bash_lc_and_escape(command); + // Construct a concise, single-line summary of the command: + // - If multi-line, take the first line and append " ...". + // - Truncate to 80 graphemes. + let mut snippet = match full_cmd.split_once('\n') { + Some((first, _)) => format!("{first} ..."), + None => full_cmd.clone(), + }; + // Enforce the 80 character length limit. + snippet = truncate_text(&snippet, 80); - // Result line based on decision. + let mut result_spans: Vec> = Vec::new(); match decision { ReviewDecision::Approved => { - lines.extend(to_command_display( - vec![ - "✔ ".fg(Color::Green), - "You ".into(), - "approved".bold(), - " codex to run ".into(), - ], - cmd, - vec![" this time".bold()], - )); + result_spans.extend(vec![ + "✔ ".fg(Color::Green), + "You ".into(), + "approved".bold(), + " codex to run ".into(), + snippet.clone().dim(), + " this time".bold(), + ]); } ReviewDecision::ApprovedForSession => { - lines.extend(to_command_display( - vec![ - "✔ ".fg(Color::Green), - "You ".into(), - "approved".bold(), - " codex to run ".into(), - ], - cmd, - vec![" every time this session".bold()], - )); + result_spans.extend(vec![ + "✔ ".fg(Color::Green), + "You ".into(), + "approved".bold(), + " codex to run ".into(), + snippet.clone().dim(), + " every time this session".bold(), + ]); } ReviewDecision::Denied => { - lines.extend(to_command_display( - vec![ - "✗ ".fg(Color::Red), - "You ".into(), - "did not approve".bold(), - " codex to run ".into(), - ], - cmd, - vec![], - )); + result_spans.extend(vec![ + "✗ ".fg(Color::Red), + "You ".into(), + "did not approve".bold(), + " codex to run ".into(), + snippet.clone().dim(), + ]); } ReviewDecision::Abort => { - lines.extend(to_command_display( - vec![ - "✗ ".fg(Color::Red), - "You ".into(), - "canceled".bold(), - " the request to run ".into(), - ], - cmd, - vec![], - )); + result_spans.extend(vec![ + "✗ ".fg(Color::Red), + "You ".into(), + "canceled".bold(), + " the request to run ".into(), + snippet.clone().dim(), + ]); } } + let mut lines: Vec> = vec![Line::from(result_spans)]; + if !feedback.trim().is_empty() { lines.push(Line::from("feedback:")); for l in feedback.lines() { From 3f40fbc0a886ef54f494a1ff5963971bc42c036a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 4 Sep 2025 17:49:50 -0700 Subject: [PATCH 09/13] chore: improve serialization of ServerNotification (#3193) This PR introduces introduces a new `OutgoingMessage::AppServerNotification` variant that is designed to wrap a `ServerNotification`, which makes the serialization more straightforward compared to `OutgoingMessage::Notification(OutgoingNotification)`. We still use the latter for serializing an `Event` as a `JSONRPCMessage::Notification`, but I will try to get away from that in the near future. With this change, now the generated TypeScript type for `ServerNotification` is: ```typescript export type ServerNotification = | { "method": "authStatusChange", "params": AuthStatusChangeNotification } | { "method": "loginChatGptComplete", "params": LoginChatGptCompleteNotification }; ``` whereas before it was: ```typescript export type ServerNotification = | { type: "auth_status_change"; data: AuthStatusChangeNotification } | { type: "login_chat_gpt_complete"; data: LoginChatGptCompleteNotification }; ``` Once the `Event`s are migrated to the `ServerNotification` enum in Rust, it should be considerably easier to work with notifications on the TypeScript side, as it will be possible to `switch (message.method)` and check for exhaustiveness. Though we will probably need to introduce: ```typescript export type ServerMessage = ServerRequest | ServerNotification; ``` and then we still need to group all of the `ServerResponse` types together, as well. --- .../mcp-server/src/codex_message_processor.rs | 6 +- codex-rs/mcp-server/src/outgoing_message.rs | 58 ++++++++++++++++--- codex-rs/protocol/src/mcp_protocol.rs | 13 ++++- 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 7de12febc2..2309e01dcd 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -815,9 +815,9 @@ impl CodexMessageProcessor { }; // For now, we send a notification for every event, - // JSON-serializing the `Event` as-is, but we will move - // to creating a special enum for notifications with a - // stable wire format. + // JSON-serializing the `Event` as-is, but these should + // be migrated to be variants of `ServerNotification` + // instead. let method = format!("codex/event/{}", event.msg); let mut params = match serde_json::to_value(event.clone()) { Ok(serde_json::Value::Object(map)) => map, diff --git a/codex-rs/mcp-server/src/outgoing_message.rs b/codex-rs/mcp-server/src/outgoing_message.rs index c26c8e3dec..b735f2bf9c 100644 --- a/codex-rs/mcp-server/src/outgoing_message.rs +++ b/codex-rs/mcp-server/src/outgoing_message.rs @@ -97,6 +97,9 @@ impl OutgoingMessageSender { } } + /// This is used with the MCP server, but not the more general JSON-RPC app + /// server. Prefer [`OutgoingMessageSender::send_server_notification`] where + /// possible. pub(crate) async fn send_event_as_notification( &self, event: &Event, @@ -123,14 +126,9 @@ impl OutgoingMessageSender { } pub(crate) async fn send_server_notification(&self, notification: ServerNotification) { - let method = format!("codex/event/{notification}"); - let params = match serde_json::to_value(¬ification) { - Ok(serde_json::Value::Object(mut map)) => map.remove("data"), - _ => None, - }; - let outgoing_message = - OutgoingMessage::Notification(OutgoingNotification { method, params }); - let _ = self.sender.send(outgoing_message); + let _ = self + .sender + .send(OutgoingMessage::AppServerNotification(notification)); } pub(crate) async fn send_notification(&self, notification: OutgoingNotification) { @@ -148,6 +146,9 @@ impl OutgoingMessageSender { pub(crate) enum OutgoingMessage { Request(OutgoingRequest), Notification(OutgoingNotification), + /// AppServerNotification is specific to the case where this is run as an + /// "app server" as opposed to an MCP server. + AppServerNotification(ServerNotification), Response(OutgoingResponse), Error(OutgoingError), } @@ -171,6 +172,21 @@ impl From for JSONRPCMessage { params, }) } + AppServerNotification(notification) => { + let method = notification.to_string(); + let params = match notification.to_params() { + Ok(params) => Some(params), + Err(err) => { + warn!("failed to serialize notification params: {err}"); + None + } + }; + JSONRPCMessage::Notification(JSONRPCNotification { + jsonrpc: JSONRPC_VERSION.into(), + method, + params, + }) + } Response(OutgoingResponse { id, result }) => { JSONRPCMessage::Response(JSONRPCResponse { jsonrpc: JSONRPC_VERSION.into(), @@ -242,6 +258,7 @@ pub(crate) struct OutgoingError { mod tests { use codex_core::protocol::EventMsg; use codex_core::protocol::SessionConfiguredEvent; + use codex_protocol::mcp_protocol::LoginChatGptCompleteNotification; use pretty_assertions::assert_eq; use serde_json::json; use uuid::Uuid; @@ -324,4 +341,29 @@ mod tests { }); assert_eq!(params.unwrap(), expected_params); } + + #[test] + fn verify_server_notification_serialization() { + let notification = + ServerNotification::LoginChatGptComplete(LoginChatGptCompleteNotification { + login_id: Uuid::nil(), + success: true, + error: None, + }); + + let jsonrpc_notification: JSONRPCMessage = + OutgoingMessage::AppServerNotification(notification).into(); + assert_eq!( + JSONRPCMessage::Notification(JSONRPCNotification { + jsonrpc: "2.0".into(), + method: "loginChatGptComplete".into(), + params: Some(json!({ + "loginId": Uuid::nil(), + "success": true, + })), + }), + jsonrpc_notification, + "ensure the strum macros serialize the method field correctly" + ); + } } diff --git a/codex-rs/protocol/src/mcp_protocol.rs b/codex-rs/protocol/src/mcp_protocol.rs index a5a646fabf..3980de0ae1 100644 --- a/codex-rs/protocol/src/mcp_protocol.rs +++ b/codex-rs/protocol/src/mcp_protocol.rs @@ -551,8 +551,8 @@ pub struct AuthStatusChangeNotification { } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS, Display)] -#[serde(tag = "type", content = "data", rename_all = "snake_case")] -#[strum(serialize_all = "snake_case")] +#[serde(tag = "method", content = "params", rename_all = "camelCase")] +#[strum(serialize_all = "camelCase")] pub enum ServerNotification { /// Authentication status changed AuthStatusChange(AuthStatusChangeNotification), @@ -561,6 +561,15 @@ pub enum ServerNotification { LoginChatGptComplete(LoginChatGptCompleteNotification), } +impl ServerNotification { + pub fn to_params(self) -> Result { + match self { + ServerNotification::AuthStatusChange(params) => serde_json::to_value(params), + ServerNotification::LoginChatGptComplete(params) => serde_json::to_value(params), + } + } +} + #[cfg(test)] mod tests { use super::*; From 323a5cb7e70cc9ae815dc43edb4d2292437483b3 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 5 Sep 2025 07:02:11 -0700 Subject: [PATCH 10/13] refactor: remove AttachImage tui event (#3191) TuiEvent is supposed to be purely events that come from the "driver", i.e. events from the terminal. Everything app-specific should be an AppEvent. In this case, it didn't need to be an event at all. --- codex-rs/tui/src/app.rs | 9 --------- codex-rs/tui/src/chatwidget.rs | 12 ++++++++++++ codex-rs/tui/src/tui.rs | 34 ---------------------------------- 3 files changed, 12 insertions(+), 43 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 40a7f20886..12cd789c5f 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -183,15 +183,6 @@ impl App { }, )?; } - TuiEvent::AttachImage { - path, - width, - height, - format_label, - } => { - self.chat_widget - .attach_image(path, width, height, format_label); - } } } Ok(true) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 7acb802c73..65ecbf749f 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -57,6 +57,7 @@ use crate::bottom_pane::CancellationEvent; use crate::bottom_pane::InputResult; use crate::bottom_pane::SelectionAction; use crate::bottom_pane::SelectionItem; +use crate::clipboard_paste::paste_image_to_temp_png; use crate::get_git_diff::get_git_diff; use crate::history_cell; use crate::history_cell::CommandOutput; @@ -745,6 +746,17 @@ impl ChatWidget { self.on_ctrl_c(); return; } + KeyEvent { + code: KeyCode::Char('v'), + modifiers: KeyModifiers::CONTROL, + kind: KeyEventKind::Press, + .. + } => { + if let Ok((path, info)) = paste_image_to_temp_png() { + self.attach_image(path, info.width, info.height, info.encoded_format.label()); + } + return; + } other if other.kind == KeyEventKind::Press => { self.bottom_pane.clear_ctrl_c_quit_hint(); } diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index df957fee87..26c188668d 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -1,7 +1,6 @@ use std::io::Result; use std::io::Stdout; use std::io::stdout; -use std::path::PathBuf; use std::pin::Pin; use std::sync::Arc; use std::sync::atomic::AtomicBool; @@ -20,10 +19,7 @@ use crossterm::cursor::MoveTo; use crossterm::event::DisableBracketedPaste; use crossterm::event::EnableBracketedPaste; use crossterm::event::Event; -use crossterm::event::KeyCode; use crossterm::event::KeyEvent; -use crossterm::event::KeyEventKind; -use crossterm::event::KeyModifiers; use crossterm::event::KeyboardEnhancementFlags; use crossterm::event::PopKeyboardEnhancementFlags; use crossterm::event::PushKeyboardEnhancementFlags; @@ -38,7 +34,6 @@ use ratatui::crossterm::terminal::enable_raw_mode; use ratatui::layout::Offset; use ratatui::text::Line; -use crate::clipboard_paste::paste_image_to_temp_png; use crate::custom_terminal; use crate::custom_terminal::Terminal as CustomTerminal; use tokio::select; @@ -154,12 +149,6 @@ pub enum TuiEvent { Key(KeyEvent), Paste(String), Draw, - AttachImage { - path: PathBuf, - width: u32, - height: u32, - format_label: &'static str, - }, } pub struct Tui { @@ -305,29 +294,6 @@ impl Tui { select! { Some(Ok(event)) = crossterm_events.next() => { match event { - // Detect Ctrl+V to attach an image from the clipboard. - Event::Key(key_event @ KeyEvent { - code: KeyCode::Char('v'), - modifiers: KeyModifiers::CONTROL, - kind: KeyEventKind::Press, - .. - }) => { - match paste_image_to_temp_png() { - Ok((path, info)) => { - yield TuiEvent::AttachImage { - path, - width: info.width, - height: info.height, - format_label: info.encoded_format.label(), - }; - } - Err(_) => { - // Fall back to normal key handling if no image is available. - yield TuiEvent::Key(key_event); - } - } - } - crossterm::event::Event::Key(key_event) => { #[cfg(unix)] if matches!( From d6182becbe155776b159270fe4a5e88d607f0c5e Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 5 Sep 2025 07:10:32 -0700 Subject: [PATCH 11/13] syntax-highlight bash lines (#3142) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit i'm not yet convinced i have the best heuristics for what to highlight, but this feels like a useful step towards something a bit easier to read, esp. when the model is producing large commands. Screenshot 2025-09-03 at 8 21 56 PM also a fairly significant refactor of our line wrapping logic. --- AGENTS.md | 14 +- codex-rs/core/src/lib.rs | 2 +- codex-rs/tui/src/backtrack_helpers.rs | 2 +- codex-rs/tui/src/bottom_pane/textarea.rs | 19 +- ...et__tests__binary_size_ideal_response.snap | 19 +- codex-rs/tui/src/history_cell.rs | 141 ++--- codex-rs/tui/src/insert_history.rs | 166 +----- codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/pager_overlay.rs | 6 +- codex-rs/tui/src/render/highlight.rs | 145 +++++ codex-rs/tui/src/render/mod.rs | 1 + ...both_lines_wrap_with_correct_prefixes.snap | 9 +- ...with_extra_indent_on_subsequent_lines.snap | 1 + ...nd_wraps_with_four_space_continuation.snap | 8 +- codex-rs/tui/src/status_indicator_widget.rs | 12 +- codex-rs/tui/src/wrapping.rs | 545 ++++++++++++++++++ 16 files changed, 786 insertions(+), 305 deletions(-) create mode 100644 codex-rs/tui/src/render/highlight.rs create mode 100644 codex-rs/tui/src/wrapping.rs diff --git a/AGENTS.md b/AGENTS.md index 83fd2a3ec4..697de7009f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,7 +37,15 @@ See `codex-rs/tui/styles.md`. - Avoid churn: don’t refactor between equivalent forms (Span::styled ↔ set_style, Line::from ↔ .into()) without a clear readability or functional gain; follow file‑local conventions and do not introduce type annotations solely to satisfy .into(). - Compactness: prefer the form that stays on one line after rustfmt; if only one of Line::from(vec![…]) or vec![…].into() avoids wrapping, choose that. If both wrap, pick the one with fewer wrapped lines. -## Snapshot tests +### Text wrapping +- Always use textwrap::wrap to wrap plain strings. +- If you have a ratatui Line and you want to wrap it, use the helpers in tui/src/wrapping.rs, e.g. word_wrap_lines / word_wrap_line. +- If you need to indent wrapped lines, use the initial_indent / subsequent_indent options from RtOptions if you can, rather than writing custom logic. +- If you have a list of lines and you need to prefix them all with some prefix (optionally different on the first vs subsequent lines), use the `prefix_lines` helper from line_utils. + +## Tests + +### Snapshot tests This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to validate rendered output. When UI or text output changes intentionally, update the snapshots as follows: @@ -52,3 +60,7 @@ This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to va If you don’t have the tool: - `cargo install cargo-insta` + +### Test assertions + +- Tests should use pretty_assertions::assert_eq for clearer diffs. Import this at the top of the test module if it isn't already. diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 358c58de0c..6d26ad2e8a 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -7,7 +7,7 @@ mod apply_patch; pub mod auth; -mod bash; +pub mod bash; mod chat_completions; mod client; mod client_common; diff --git a/codex-rs/tui/src/backtrack_helpers.rs b/codex-rs/tui/src/backtrack_helpers.rs index b006ae38eb..e2306b80b1 100644 --- a/codex-rs/tui/src/backtrack_helpers.rs +++ b/codex-rs/tui/src/backtrack_helpers.rs @@ -12,7 +12,7 @@ pub(crate) fn highlight_range_for_nth_last_user( /// Compute the wrapped display-line offset before `header_idx`, for a given width. pub(crate) fn wrapped_offset_before(lines: &[Line<'_>], header_idx: usize, width: u16) -> usize { let before = &lines[0..header_idx]; - crate::insert_history::word_wrap_lines(before, width).len() + crate::wrapping::word_wrap_lines(before, width as usize).len() } /// Find the header index for the Nth last user message in the transcript. diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index cb6ef501a1..37aa85ff18 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -832,25 +832,10 @@ impl TextArea { None => true, }; if needs_recalc { - let mut lines: Vec> = Vec::new(); - for line in textwrap::wrap( + let lines = crate::wrapping::wrap_ranges( &self.text, Options::new(width as usize).wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), - ) - .iter() - { - match line { - std::borrow::Cow::Borrowed(slice) => { - let start = - unsafe { slice.as_ptr().offset_from(self.text.as_ptr()) as usize }; - let end = start + slice.len(); - let trailing_spaces = - self.text[end..].chars().take_while(|c| *c == ' ').count(); - lines.push(start..end + trailing_spaces + 1); - } - std::borrow::Cow::Owned(_) => unreachable!(), - } - } + ); *cache = Some(WrapCache { width, lines }); } } diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__binary_size_ideal_response.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__binary_size_ideal_response.snap index 5d6d5cb455..0b6ec3b392 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__binary_size_ideal_response.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__binary_size_ideal_response.snap @@ -1,5 +1,6 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 648 expression: visible_after --- > I’m going to scan the workspace and Cargo manifests to see build profiles and @@ -22,9 +23,9 @@ expression: visible_after Main Causes - - Static linking style: Each bin (codex, codex-tui, codex-exec, - codex-mcp-server, etc.) statically links its full dependency graph, so common - code isn’t shared at runtime across executables. + - Static linking style: Each bin (codex, codex-tui, codex-exec, codex-mcp- + server, etc.) statically links its full dependency graph, so common code isn’t + shared at runtime across executables. - Heavy deps (HTTP/TLS): reqwest brings in Hyper, HTTP/2, compressors, and a TLS stack (rustls by default; OpenSSL on musl). In core, login, tui, and ollama you enable reqwest with json/stream, which still pulls a large @@ -39,9 +40,9 @@ expression: visible_after per bin. - Panic + backtraces: Default panic = unwind and backtrace support keep unwinding tables and symbols that add weight. - - Per-target OpenSSL (musl): For *-unknown-linux-musl, core enables - openssl-sys with vendored, compiling OpenSSL into the binary—this adds - multiple megabytes per executable. + - Per-target OpenSSL (musl): For *-unknown-linux-musl, core enables openssl- + sys with vendored, compiling OpenSSL into the binary—this adds multiple + megabytes per executable. Build-Mode Notes @@ -52,6 +53,6 @@ expression: visible_after - Debug builds: cargo build (dev profile) includes full debuginfo, no LTO, and assertions—outputs are much larger than cargo build --release. - If you want, I can outline targeted trims (e.g., strip = "debuginfo", - opt-level = "z", panic abort, tighter tokio/reqwest features) and estimate - impact per binary. + If you want, I can outline targeted trims (e.g., strip = "debuginfo", opt- + level = "z", panic abort, tighter tokio/reqwest features) and estimate impact + per binary. diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index ce048f4875..01583958aa 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -2,9 +2,14 @@ use crate::diff_render::create_diff_summary; use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; use crate::markdown::append_markdown; +use crate::render::line_utils::line_to_static; use crate::render::line_utils::prefix_lines; +use crate::render::line_utils::push_owned_lines; use crate::slash_command::SlashCommand; use crate::text_formatting::format_and_truncate_tool_result; +use crate::wrapping::RtOptions; +use crate::wrapping::word_wrap_line; +use crate::wrapping::word_wrap_lines; use base64::Engine; use codex_ansi_escape::ansi_escape_line; use codex_common::create_config_summary_entries; @@ -97,8 +102,7 @@ impl HistoryCell for UserHistoryCell { let wrapped = textwrap::wrap( &self.message, textwrap::Options::new(wrap_width as usize) - .wrap_algorithm(textwrap::WrapAlgorithm::FirstFit) // Match textarea wrap - .word_splitter(textwrap::WordSplitter::NoHyphenation), + .wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), // Match textarea wrap ); for line in wrapped { @@ -132,28 +136,16 @@ impl AgentMessageCell { impl HistoryCell for AgentMessageCell { fn display_lines(&self, width: u16) -> Vec> { - let mut out: Vec> = Vec::new(); - // We want: - // - First visual line: "> " prefix (collapse with header logic) - // - All subsequent visual lines: two-space prefix - let mut is_first_visual = true; - let wrap_width = width.saturating_sub(2); // account for prefix - for line in &self.lines { - let wrapped = - crate::insert_history::word_wrap_lines(std::slice::from_ref(line), wrap_width); - for (i, piece) in wrapped.into_iter().enumerate() { - let mut spans = Vec::with_capacity(piece.spans.len() + 1); - spans.push(if is_first_visual && i == 0 && self.is_first_line { + word_wrap_lines( + &self.lines, + RtOptions::new(width as usize) + .initial_indent(if self.is_first_line { "> ".into() } else { " ".into() - }); - spans.extend(piece.spans.into_iter()); - out.push(spans.into()); - } - is_first_visual = false; - } - out + }) + .subsequent_indent(" ".into()), + ) } fn transcript_lines(&self) -> Vec> { @@ -278,13 +270,13 @@ impl ExecCell { } fn exploring_display_lines(&self, width: u16) -> Vec> { - let mut lines: Vec> = Vec::new(); + let mut out: Vec> = Vec::new(); let active_start_time = self .calls .iter() .find(|c| c.output.is_none()) .and_then(|c| c.start_time); - lines.push(Line::from(vec![ + out.push(Line::from(vec![ if self.is_active() { // Show an animated spinner while exploring spinner(active_start_time) @@ -299,7 +291,7 @@ impl ExecCell { }, ])); let mut calls = self.calls.clone(); - let mut first = true; + let mut out_indented = Vec::new(); while !calls.is_empty() { let mut call = calls.remove(0); if call @@ -372,39 +364,24 @@ impl ExecCell { lines }; for (title, line) in call_lines { - let prefix_len = 4 + title.len() + 1; // " └ " + title + " " - let wrapped = crate::insert_history::word_wrap_lines( - &[line.into()], - width.saturating_sub(prefix_len as u16), + let line = Line::from(line); + let initial_indent = Line::from(vec![title.cyan(), " ".into()]); + let subsequent_indent = " ".repeat(initial_indent.width()).into(); + let wrapped = word_wrap_line( + &line, + RtOptions::new(width as usize) + .initial_indent(initial_indent) + .subsequent_indent(subsequent_indent), ); - let mut first_sub = true; - for mut line in wrapped { - let mut spans = Vec::with_capacity(line.spans.len() + 1); - spans.push(if first { - first = false; - " └ ".dim() - } else { - " ".into() - }); - if first_sub { - first_sub = false; - spans.push(title.cyan()); - spans.push(" ".into()); - } else { - spans.push(" ".repeat(title.width() + 1).into()); - } - spans.extend(line.spans.into_iter()); - line.spans = spans; - lines.push(line); - } + push_owned_lines(&wrapped, &mut out_indented); } } - lines + out.extend(prefix_lines(out_indented, " └ ".dim(), " ".into())); + out } fn command_display_lines(&self, width: u16) -> Vec> { use textwrap::Options as TwOptions; - use textwrap::WordSplitter; let mut lines: Vec> = Vec::new(); let [call] = &self.calls.as_slice() else { @@ -424,38 +401,28 @@ impl ExecCell { // "• Running " (including trailing space) as the reserved prefix width. // If the command contains newlines, always use the multi-line variant. let reserved = "• Running ".width(); - let mut branch_consumed = false; - if !cmd_display.contains('\n') - && cmd_display.width() < (width as usize).saturating_sub(reserved) + let mut body_lines: Vec> = Vec::new(); + + let highlighted_lines = crate::render::highlight::highlight_bash_to_lines(&cmd_display); + + if highlighted_lines.len() == 1 + && highlighted_lines[0].width() < (width as usize).saturating_sub(reserved) { - lines.push(Line::from(vec![ - bullet, - " ".into(), - title.bold(), - " ".into(), - cmd_display.clone().into(), - ])); + let mut line = Line::from(vec![bullet, " ".into(), title.bold(), " ".into()]); + line.extend(highlighted_lines[0].clone()); + lines.push(line); } else { - branch_consumed = true; lines.push(vec![bullet, " ".into(), title.bold()].into()); - // Wrap the command line. - for (i, line) in cmd_display.lines().enumerate() { - let wrapped = textwrap::wrap( - line, - TwOptions::new(width as usize) - .initial_indent(" ") - .subsequent_indent(" ") - .word_splitter(WordSplitter::NoHyphenation), - ); - lines.extend(wrapped.into_iter().enumerate().map(|(j, l)| { - if i == 0 && j == 0 { - vec![" └ ".dim(), l[4..].to_string().into()].into() - } else { - l.to_string().into() - } - })); + for hl_line in highlighted_lines.iter() { + let opts = crate::wrapping::RtOptions::new((width as usize).saturating_sub(4)) + .initial_indent("".into()) + .subsequent_indent(" ".into()) + // Hyphenation likes to break words on hyphens, which is bad for bash scripts --because-of-flags. + .word_splitter(textwrap::WordSplitter::NoHyphenation); + let wrapped_borrowed = crate::wrapping::word_wrap_line(hl_line, opts); + body_lines.extend(wrapped_borrowed.iter().map(|l| line_to_static(l))); } } if let Some(output) = call.output.as_ref() @@ -466,25 +433,13 @@ impl ExecCell { .join("\n"); if !out.trim().is_empty() { // Wrap the output. - for (i, line) in out.lines().enumerate() { - let wrapped = textwrap::wrap( - line, - TwOptions::new(width as usize - 4) - .word_splitter(WordSplitter::NoHyphenation), - ); - lines.extend(wrapped.into_iter().map(|l| { - Line::from(vec![ - if i == 0 && !branch_consumed { - " └ ".dim() - } else { - " ".dim() - }, - l.to_string().dim(), - ]) - })); + for line in out.lines() { + let wrapped = textwrap::wrap(line, TwOptions::new(width as usize - 4)); + body_lines.extend(wrapped.into_iter().map(|l| Line::from(l.to_string().dim()))); } } } + lines.extend(prefix_lines(body_lines, " └ ".dim(), " ".into())); lines } } diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index 1ecbd3448b..bffd3f2b40 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -3,6 +3,7 @@ use std::io; use std::io::Write; use crate::tui; +use crate::wrapping::word_wrap_lines_borrowed; use crossterm::Command; use crossterm::cursor::MoveTo; use crossterm::queue; @@ -18,8 +19,6 @@ use ratatui::style::Color; use ratatui::style::Modifier; use ratatui::text::Line; use ratatui::text::Span; -use textwrap::Options as TwOptions; -use textwrap::WordSplitter; /// Insert `lines` above the viewport using the terminal's backend writer /// (avoids direct stdout references). @@ -44,7 +43,7 @@ pub fn insert_history_lines_to_writer( // Pre-wrap lines using word-aware wrapping so terminal scrollback sees the same // formatting as the TUI. This avoids character-level hard wrapping by the terminal. - let wrapped = word_wrap_lines(&lines, area.width.max(1)); + let wrapped = word_wrap_lines_borrowed(&lines, area.width.max(1) as usize); let wrapped_lines = wrapped.len() as u16; let cursor_top = if area.bottom() < screen_size.height { // If the viewport is not at the bottom of the screen, scroll it down to make room. @@ -98,7 +97,7 @@ pub fn insert_history_lines_to_writer( for line in wrapped { queue!(writer, Print("\r\n")).ok(); - write_spans(writer, line.iter()).ok(); + write_spans(writer, &line).ok(); } queue!(writer, ResetScrollRegion).ok(); @@ -223,7 +222,7 @@ impl ModifierDiff { fn write_spans<'a, I>(mut writer: &mut impl Write, content: I) -> io::Result<()> where - I: Iterator>, + I: IntoIterator>, { let mut fg = Color::Reset; let mut bg = Color::Reset; @@ -262,129 +261,6 @@ where ) } -/// Word-aware wrapping for a list of `Line`s preserving styles. -pub(crate) fn word_wrap_lines<'a, I>(lines: I, width: u16) -> Vec> -where - I: IntoIterator>, -{ - let mut out = Vec::new(); - let w = width.max(1) as usize; - for line in lines { - out.extend(word_wrap_line(line, w)); - } - out -} - -fn word_wrap_line(line: &Line, width: usize) -> Vec> { - if width == 0 { - return vec![to_owned_line(line)]; - } - // Concatenate content and keep span boundaries for later re-slicing. - let mut flat = String::new(); - let mut span_bounds = Vec::new(); // (start_byte, end_byte, style) - let mut cursor = 0usize; - for s in &line.spans { - let text = s.content.as_ref(); - let start = cursor; - flat.push_str(text); - cursor += text.len(); - span_bounds.push((start, cursor, s.style)); - } - - // Use textwrap for robust word-aware wrapping; no hyphenation, no breaking words. - let opts = TwOptions::new(width) - .break_words(false) - .word_splitter(WordSplitter::NoHyphenation); - let wrapped = textwrap::wrap(&flat, &opts); - - if wrapped.len() <= 1 { - return vec![to_owned_line(line)]; - } - - // Map wrapped pieces back to byte ranges in `flat` sequentially. - let mut start_cursor = 0usize; - let mut out: Vec> = Vec::with_capacity(wrapped.len()); - for piece in wrapped { - let piece_str: &str = &piece; - if piece_str.is_empty() { - out.push(Line { - style: line.style, - alignment: line.alignment, - spans: Vec::new(), - }); - continue; - } - // Find the next occurrence of piece_str at or after start_cursor. - // textwrap preserves order, so a linear scan is sufficient. - if let Some(rel) = flat[start_cursor..].find(piece_str) { - let s = start_cursor + rel; - let e = s + piece_str.len(); - out.push(slice_line_spans(line, &span_bounds, s, e)); - start_cursor = e; - } else { - // Fallback: slice by length from cursor. - let s = start_cursor; - let e = (start_cursor + piece_str.len()).min(flat.len()); - out.push(slice_line_spans(line, &span_bounds, s, e)); - start_cursor = e; - } - } - - out -} - -fn to_owned_line(l: &Line<'_>) -> Line<'static> { - Line { - style: l.style, - alignment: l.alignment, - spans: l - .spans - .iter() - .map(|s| Span { - style: s.style, - content: std::borrow::Cow::Owned(s.content.to_string()), - }) - .collect(), - } -} - -fn slice_line_spans( - original: &Line<'_>, - span_bounds: &[(usize, usize, ratatui::style::Style)], - start_byte: usize, - end_byte: usize, -) -> Line<'static> { - let mut acc: Vec> = Vec::new(); - for (i, (s, e, style)) in span_bounds.iter().enumerate() { - if *e <= start_byte { - continue; - } - if *s >= end_byte { - break; - } - let seg_start = start_byte.max(*s); - let seg_end = end_byte.min(*e); - if seg_end > seg_start { - let local_start = seg_start - *s; - let local_end = seg_end - *s; - let content = original.spans[i].content.as_ref(); - let slice = &content[local_start..local_end]; - acc.push(Span { - style: *style, - content: std::borrow::Cow::Owned(slice.to_string()), - }); - } - if *e >= end_byte { - break; - } - } - Line { - style: original.style, - alignment: original.alignment, - spans: acc, - } -} - #[cfg(test)] mod tests { use super::*; @@ -416,38 +292,4 @@ mod tests { String::from_utf8(expected).unwrap() ); } - - #[test] - fn line_height_counts_double_width_emoji() { - let line = "😀😀😀".into(); // each emoji ~ width 2 - assert_eq!(word_wrap_line(&line, 4).len(), 2); - assert_eq!(word_wrap_line(&line, 2).len(), 3); - assert_eq!(word_wrap_line(&line, 6).len(), 1); - } - - #[test] - fn word_wrap_does_not_split_words_simple_english() { - let sample = "Years passed, and Willowmere thrived in peace and friendship. Mira’s herb garden flourished with both ordinary and enchanted plants, and travelers spoke of the kindness of the woman who tended them."; - let line = sample.into(); - // Force small width to exercise wrapping at spaces. - let wrapped = word_wrap_lines(&[line], 40); - let joined: String = wrapped - .iter() - .map(|l| { - l.spans - .iter() - .map(|s| s.content.clone()) - .collect::() - }) - .collect::>() - .join("\n"); - assert!( - !joined.contains("bo\nth"), - "word 'both' should not be split across lines:\n{joined}" - ); - assert!( - !joined.contains("Willowm\nere"), - "should not split inside words:\n{joined}" - ); - } } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index c52f48e62f..c43572dd81 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -58,6 +58,7 @@ mod streaming; mod text_formatting; mod tui; mod user_approval_widget; +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. diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index 70365aed1d..0a7e54f06f 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -1,7 +1,7 @@ use std::io::Result; use std::time::Duration; -use crate::insert_history; +use crate::render::line_utils::push_owned_lines; use crate::tui; use crate::tui::TuiEvent; use crossterm::event::KeyCode; @@ -287,9 +287,9 @@ impl PagerView { let mut wrapped: Vec> = Vec::new(); let mut src_idx: Vec = Vec::new(); for (i, line) in self.lines.iter().enumerate() { - let ws = insert_history::word_wrap_lines(std::slice::from_ref(line), width); + let ws = crate::wrapping::word_wrap_line(line, width as usize); src_idx.extend(std::iter::repeat_n(i, ws.len())); - wrapped.extend(ws); + push_owned_lines(&ws, &mut wrapped); } self.wrap_cache = Some(WrapCache { width, diff --git a/codex-rs/tui/src/render/highlight.rs b/codex-rs/tui/src/render/highlight.rs new file mode 100644 index 0000000000..393aa33705 --- /dev/null +++ b/codex-rs/tui/src/render/highlight.rs @@ -0,0 +1,145 @@ +use codex_core::bash::try_parse_bash; +use ratatui::style::Stylize; +use ratatui::text::Line; +use ratatui::text::Span; + +/// Convert the full bash script into per-line styled content by first +/// computing operator-dimmed spans across the entire script, then splitting +/// by newlines and dimming heredoc body lines. Performs a single parse and +/// reuses it for both highlighting and heredoc detection. +pub(crate) fn highlight_bash_to_lines(script: &str) -> Vec> { + // Parse once; use the tree for both highlighting and heredoc body detection. + let spans: Vec> = if let Some(tree) = try_parse_bash(script) { + // Single walk: collect operator ranges and heredoc rows. + let root = tree.root_node(); + let mut cursor = root.walk(); + let mut stack = vec![root]; + let mut ranges: Vec<(usize, usize)> = Vec::new(); + while let Some(node) = stack.pop() { + if !node.is_named() && !node.is_extra() { + let kind = node.kind(); + let is_quote = matches!(kind, "\"" | "'" | "`"); + let is_whitespace = kind.trim().is_empty(); + if !is_quote && !is_whitespace { + ranges.push((node.start_byte(), node.end_byte())); + } + } else if node.kind() == "heredoc_body" { + ranges.push((node.start_byte(), node.end_byte())); + } + for child in node.children(&mut cursor) { + stack.push(child); + } + } + if ranges.is_empty() { + ranges.push((script.len(), script.len())); + } + ranges.sort_by_key(|(st, _)| *st); + let mut spans: Vec> = Vec::new(); + let mut i = 0usize; + for (start, end) in ranges.into_iter() { + let dim_start = start.max(i); + let dim_end = end; + if dim_start < dim_end { + if dim_start > i { + spans.push(script[i..dim_start].to_string().into()); + } + spans.push(script[dim_start..dim_end].to_string().dim()); + i = dim_end; + } + } + if i < script.len() { + spans.push(script[i..].to_string().into()); + } + spans + } else { + vec![script.to_string().into()] + }; + // Split spans into lines preserving style boundaries and highlights across newlines. + let mut lines: Vec> = vec![Line::from("")]; + for sp in spans { + let style = sp.style; + let text = sp.content.into_owned(); + for (i, part) in text.split('\n').enumerate() { + if i > 0 { + lines.push(Line::from("")); + } + if part.is_empty() { + continue; + } + let span = Span { + style, + content: std::borrow::Cow::Owned(part.to_string()), + }; + if let Some(last) = lines.last_mut() { + last.spans.push(span); + } + } + } + lines +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use ratatui::style::Modifier; + + #[test] + fn dims_expected_bash_operators() { + let s = "echo foo && bar || baz | qux & (echo hi)"; + let lines = highlight_bash_to_lines(s); + let reconstructed: String = lines + .iter() + .map(|l| { + l.spans + .iter() + .map(|sp| sp.content.clone()) + .collect::() + }) + .collect::>() + .join("\n"); + assert_eq!(reconstructed, s); + + fn is_dim(span: &Span<'_>) -> bool { + span.style.add_modifier.contains(Modifier::DIM) + } + let dimmed: Vec = lines + .iter() + .flat_map(|l| l.spans.iter()) + .filter(|sp| is_dim(sp)) + .map(|sp| sp.content.clone().into_owned()) + .collect(); + assert_eq!(dimmed, vec!["&&", "||", "|", "&", "(", ")"]); + } + + #[test] + fn does_not_dim_quotes_but_dims_other_punct() { + let s = "echo \"hi\" > out.txt; echo 'ok'"; + let lines = highlight_bash_to_lines(s); + let reconstructed: String = lines + .iter() + .map(|l| { + l.spans + .iter() + .map(|sp| sp.content.clone()) + .collect::() + }) + .collect::>() + .join("\n"); + assert_eq!(reconstructed, s); + + fn is_dim(span: &Span<'_>) -> bool { + span.style.add_modifier.contains(Modifier::DIM) + } + let dimmed: Vec = lines + .iter() + .flat_map(|l| l.spans.iter()) + .filter(|sp| is_dim(sp)) + .map(|sp| sp.content.clone().into_owned()) + .collect(); + assert!(dimmed.contains(&">".to_string())); + assert!(dimmed.contains(&";".to_string())); + assert!(!dimmed.contains(&"\"".to_string())); + assert!(!dimmed.contains(&"'".to_string())); + } +} diff --git a/codex-rs/tui/src/render/mod.rs b/codex-rs/tui/src/render/mod.rs index 71ae62dba1..ed1e215e65 100644 --- a/codex-rs/tui/src/render/mod.rs +++ b/codex-rs/tui/src/render/mod.rs @@ -1,2 +1,3 @@ +pub mod highlight; pub mod line_utils; pub mod markdown_utils; diff --git a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_both_lines_wrap_with_correct_prefixes.snap b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_both_lines_wrap_with_correct_prefixes.snap index 99d6579fe4..0964b64aec 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_both_lines_wrap_with_correct_prefixes.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_both_lines_wrap_with_correct_prefixes.snap @@ -1,9 +1,10 @@ --- source: tui/src/history_cell.rs +assertion_line: 1942 expression: rendered --- • Ran - └ first_token_is_long_ - enough_to_wrap - second_token_is_also - _long_enough_to_wrap + └ first_token_is_long_enou + gh_to_wrap + second_token_is_also_lon + g_enough_to_wrap diff --git a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_wraps_with_extra_indent_on_subsequent_lines.snap b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_wraps_with_extra_indent_on_subsequent_lines.snap index 4e383a5aa2..de36cd92af 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_wraps_with_extra_indent_on_subsequent_lines.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__multiline_command_wraps_with_extra_indent_on_subsequent_lines.snap @@ -1,5 +1,6 @@ --- source: tui/src/history_cell.rs +assertion_line: 1797 expression: rendered --- • Ran diff --git a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__single_line_command_wraps_with_four_space_continuation.snap b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__single_line_command_wraps_with_four_space_continuation.snap index 548d50cea2..5ca1330757 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__single_line_command_wraps_with_four_space_continuation.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__single_line_command_wraps_with_four_space_continuation.snap @@ -1,9 +1,9 @@ --- source: tui/src/history_cell.rs +assertion_line: 1869 expression: rendered --- • Ran - └ a_very_long_toke - n_without_spaces - _to_force_wrappi - ng + └ a_very_long_token_wi + thout_spaces_to_ + force_wrapping diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index 07ecba0c2b..8f492c062b 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -17,8 +17,6 @@ use crate::app_event_sender::AppEventSender; use crate::key_hint; use crate::shimmer::shimmer_spans; use crate::tui::FrameRequester; -use textwrap::Options as TwOptions; -use textwrap::WordSplitter; pub(crate) struct StatusIndicatorWidget { /// Animated header text (defaults to "Working"). @@ -54,11 +52,8 @@ impl StatusIndicatorWidget { let mut total: u16 = 1; // status line let text_width = inner_width.saturating_sub(3); // account for " ↳ " prefix if text_width > 0 { - let opts = TwOptions::new(text_width) - .break_words(false) - .word_splitter(WordSplitter::NoHyphenation); for q in &self.queued_messages { - let wrapped = textwrap::wrap(q, &opts); + let wrapped = textwrap::wrap(q, text_width); let lines = wrapped.len().min(3) as u16; total = total.saturating_add(lines); if wrapped.len() > 3 { @@ -157,11 +152,8 @@ impl WidgetRef for StatusIndicatorWidget { lines.push(Line::from(spans)); // Wrap queued messages using textwrap and show up to the first 3 lines per message. let text_width = area.width.saturating_sub(3); // " ↳ " prefix - let opts = TwOptions::new(text_width as usize) - .break_words(false) - .word_splitter(WordSplitter::NoHyphenation); for q in &self.queued_messages { - let wrapped = textwrap::wrap(q, &opts); + let wrapped = textwrap::wrap(q, text_width as usize); for (i, piece) in wrapped.iter().take(3).enumerate() { let prefix = if i == 0 { " ↳ " } else { " " }; let content = format!("{prefix}{piece}"); diff --git a/codex-rs/tui/src/wrapping.rs b/codex-rs/tui/src/wrapping.rs new file mode 100644 index 0000000000..508c6a45f4 --- /dev/null +++ b/codex-rs/tui/src/wrapping.rs @@ -0,0 +1,545 @@ +use ratatui::text::Line; +use ratatui::text::Span; +use std::ops::Range; +use textwrap::Options; + +use crate::render::line_utils::push_owned_lines; + +pub(crate) fn wrap_ranges<'a, O>(text: &str, width_or_options: O) -> Vec> +where + O: Into>, +{ + let opts = width_or_options.into(); + let mut lines: Vec> = Vec::new(); + for line in textwrap::wrap(text, opts).iter() { + match line { + std::borrow::Cow::Borrowed(slice) => { + let start = unsafe { slice.as_ptr().offset_from(text.as_ptr()) as usize }; + let end = start + slice.len(); + let trailing_spaces = text[end..].chars().take_while(|c| *c == ' ').count(); + lines.push(start..end + trailing_spaces + 1); + } + std::borrow::Cow::Owned(_) => panic!("wrap_ranges: unexpected owned string"), + } + } + lines +} + +/// Like `wrap_ranges` but returns ranges without trailing whitespace and +/// without the sentinel extra byte. Suitable for general wrapping where +/// trailing spaces should not be preserved. +pub(crate) fn wrap_ranges_trim<'a, O>(text: &str, width_or_options: O) -> Vec> +where + O: Into>, +{ + let opts = width_or_options.into(); + let mut lines: Vec> = Vec::new(); + for line in textwrap::wrap(text, opts).iter() { + match line { + std::borrow::Cow::Borrowed(slice) => { + let start = unsafe { slice.as_ptr().offset_from(text.as_ptr()) as usize }; + let end = start + slice.len(); + lines.push(start..end); + } + std::borrow::Cow::Owned(_) => panic!("wrap_ranges_trim: unexpected owned string"), + } + } + lines +} + +#[derive(Debug, Clone)] +pub struct RtOptions<'a> { + /// The width in columns at which the text will be wrapped. + pub width: usize, + /// Line ending used for breaking lines. + pub line_ending: textwrap::LineEnding, + /// Indentation used for the first line of output. See the + /// [`Options::initial_indent`] method. + pub initial_indent: Line<'a>, + /// Indentation used for subsequent lines of output. See the + /// [`Options::subsequent_indent`] method. + pub subsequent_indent: Line<'a>, + /// Allow long words to be broken if they cannot fit on a line. + /// When set to `false`, some lines may be longer than + /// `self.width`. See the [`Options::break_words`] method. + pub break_words: bool, + /// Wrapping algorithm to use, see the implementations of the + /// [`WrapAlgorithm`] trait for details. + pub wrap_algorithm: textwrap::WrapAlgorithm, + /// The line breaking algorithm to use, see the [`WordSeparator`] + /// trait for an overview and possible implementations. + pub word_separator: textwrap::WordSeparator, + /// The method for splitting words. This can be used to prohibit + /// splitting words on hyphens, or it can be used to implement + /// language-aware machine hyphenation. + pub word_splitter: textwrap::WordSplitter, +} +impl From for RtOptions<'_> { + fn from(width: usize) -> Self { + RtOptions::new(width) + } +} + +#[allow(dead_code)] +impl<'a> RtOptions<'a> { + pub fn new(width: usize) -> Self { + RtOptions { + width, + line_ending: textwrap::LineEnding::LF, + initial_indent: Line::default(), + subsequent_indent: Line::default(), + break_words: true, + word_separator: textwrap::WordSeparator::new(), + wrap_algorithm: textwrap::WrapAlgorithm::new(), + word_splitter: textwrap::WordSplitter::HyphenSplitter, + } + } + + pub fn line_ending(self, line_ending: textwrap::LineEnding) -> Self { + RtOptions { + line_ending, + ..self + } + } + + pub fn width(self, width: usize) -> Self { + RtOptions { width, ..self } + } + + pub fn initial_indent(self, initial_indent: Line<'a>) -> Self { + RtOptions { + initial_indent, + ..self + } + } + + pub fn subsequent_indent(self, subsequent_indent: Line<'a>) -> Self { + RtOptions { + subsequent_indent, + ..self + } + } + + pub fn break_words(self, break_words: bool) -> Self { + RtOptions { + break_words, + ..self + } + } + + pub fn word_separator(self, word_separator: textwrap::WordSeparator) -> RtOptions<'a> { + RtOptions { + word_separator, + ..self + } + } + + pub fn wrap_algorithm(self, wrap_algorithm: textwrap::WrapAlgorithm) -> RtOptions<'a> { + RtOptions { + wrap_algorithm, + ..self + } + } + + pub fn word_splitter(self, word_splitter: textwrap::WordSplitter) -> RtOptions<'a> { + RtOptions { + word_splitter, + ..self + } + } +} + +pub(crate) fn word_wrap_line<'a, O>(line: &'a Line<'a>, width_or_options: O) -> Vec> +where + O: Into>, +{ + // Flatten the line and record span byte ranges. + let mut flat = String::new(); + let mut span_bounds = Vec::new(); + let mut acc = 0usize; + for s in &line.spans { + let text = s.content.as_ref(); + let start = acc; + flat.push_str(text); + acc += text.len(); + span_bounds.push((start..acc, s.style)); + } + + let rt_opts: RtOptions<'a> = width_or_options.into(); + let opts = Options::new(rt_opts.width) + .line_ending(rt_opts.line_ending) + .break_words(rt_opts.break_words) + .wrap_algorithm(rt_opts.wrap_algorithm) + .word_separator(rt_opts.word_separator) + .word_splitter(rt_opts.word_splitter); + + let mut out: Vec> = Vec::new(); + + // Compute first line range with reduced width due to initial indent. + let initial_width_available = opts + .width + .saturating_sub(rt_opts.initial_indent.width()) + .max(1); + let initial_wrapped = wrap_ranges_trim(&flat, opts.clone().width(initial_width_available)); + let Some(first_line_range) = initial_wrapped.first() else { + return vec![rt_opts.initial_indent.clone()]; + }; + + // Build first wrapped line with initial indent. + let mut first_line = rt_opts.initial_indent.clone(); + { + let mut sliced = slice_line_spans(line, &span_bounds, first_line_range); + let mut spans = first_line.spans; + spans.append(&mut sliced.spans); + first_line.spans = spans; + out.push(first_line); + } + + // Wrap the remainder using subsequent indent width and map back to original indices. + let base = first_line_range.end; + let skip_leading_spaces = flat[base..].chars().take_while(|c| *c == ' ').count(); + let base = base + skip_leading_spaces; + let subsequent_width_available = opts + .width + .saturating_sub(rt_opts.subsequent_indent.width()) + .max(1); + let remaining_wrapped = wrap_ranges_trim(&flat[base..], opts.width(subsequent_width_available)); + for r in &remaining_wrapped { + if r.is_empty() { + continue; + } + let mut subsequent_line = rt_opts.subsequent_indent.clone(); + let offset_range = (r.start + base)..(r.end + base); + let mut sliced = slice_line_spans(line, &span_bounds, &offset_range); + let mut spans = subsequent_line.spans; + spans.append(&mut sliced.spans); + subsequent_line.spans = spans; + out.push(subsequent_line); + } + + out +} + +/// Wrap a sequence of lines, applying the initial indent only to the very first +/// output line, and using the subsequent indent for all later wrapped pieces. +#[allow(dead_code)] +pub(crate) fn word_wrap_lines<'a, I, O>(lines: I, width_or_options: O) -> Vec> +where + I: IntoIterator>, + O: Into>, +{ + let base_opts: RtOptions<'a> = width_or_options.into(); + let mut out: Vec> = Vec::new(); + + for (idx, line) in lines.into_iter().enumerate() { + let opts = if idx == 0 { + base_opts.clone() + } else { + let mut o = base_opts.clone(); + let sub = o.subsequent_indent.clone(); + o = o.initial_indent(sub); + o + }; + let wrapped = word_wrap_line(line, opts); + push_owned_lines(&wrapped, &mut out); + } + + out +} + +#[allow(dead_code)] +pub(crate) fn word_wrap_lines_borrowed<'a, I, O>(lines: I, width_or_options: O) -> Vec> +where + I: IntoIterator>, + O: Into>, +{ + let base_opts: RtOptions<'a> = width_or_options.into(); + let mut out: Vec> = Vec::new(); + let mut first = true; + for line in lines.into_iter() { + let opts = if first { + base_opts.clone() + } else { + base_opts + .clone() + .initial_indent(base_opts.subsequent_indent.clone()) + }; + out.extend(word_wrap_line(line, opts)); + first = false; + } + out +} + +fn slice_line_spans<'a>( + original: &'a Line<'a>, + span_bounds: &[(Range, ratatui::style::Style)], + range: &Range, +) -> Line<'a> { + let start_byte = range.start; + let end_byte = range.end; + let mut acc: Vec> = Vec::new(); + for (i, (range, style)) in span_bounds.iter().enumerate() { + let s = range.start; + let e = range.end; + if e <= start_byte { + continue; + } + if s >= end_byte { + break; + } + let seg_start = start_byte.max(s); + let seg_end = end_byte.min(e); + if seg_end > seg_start { + let local_start = seg_start - s; + let local_end = seg_end - s; + let content = original.spans[i].content.as_ref(); + let slice = &content[local_start..local_end]; + acc.push(Span { + style: *style, + content: std::borrow::Cow::Borrowed(slice), + }); + } + if e >= end_byte { + break; + } + } + Line { + style: original.style, + alignment: original.alignment, + spans: acc, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use itertools::Itertools as _; + use pretty_assertions::assert_eq; + use ratatui::style::Color; + use ratatui::style::Stylize; + + fn concat_line(line: &Line) -> String { + line.spans + .iter() + .map(|s| s.content.as_ref()) + .collect::() + } + + #[test] + fn trivial_unstyled_no_indents_wide_width() { + let line = Line::from("hello"); + let out = word_wrap_line(&line, 10); + assert_eq!(out.len(), 1); + assert_eq!(concat_line(&out[0]), "hello"); + } + + #[test] + fn simple_unstyled_wrap_narrow_width() { + let line = Line::from("hello world"); + let out = word_wrap_line(&line, 5); + assert_eq!(out.len(), 2); + assert_eq!(concat_line(&out[0]), "hello"); + assert_eq!(concat_line(&out[1]), "world"); + } + + #[test] + fn simple_styled_wrap_preserves_styles() { + let line = Line::from(vec!["hello ".red(), "world".into()]); + let out = word_wrap_line(&line, 6); + assert_eq!(out.len(), 2); + // First line should carry the red style + assert_eq!(concat_line(&out[0]), "hello"); + assert_eq!(out[0].spans.len(), 1); + assert_eq!(out[0].spans[0].style.fg, Some(Color::Red)); + // Second line is unstyled + assert_eq!(concat_line(&out[1]), "world"); + assert_eq!(out[1].spans.len(), 1); + assert_eq!(out[1].spans[0].style.fg, None); + } + + #[test] + fn with_initial_and_subsequent_indents() { + let opts = RtOptions::new(8) + .initial_indent(Line::from("- ")) + .subsequent_indent(Line::from(" ")); + let line = Line::from("hello world foo"); + let out = word_wrap_line(&line, opts); + // Expect three lines with proper prefixes + assert!(concat_line(&out[0]).starts_with("- ")); + assert!(concat_line(&out[1]).starts_with(" ")); + assert!(concat_line(&out[2]).starts_with(" ")); + // And content roughly segmented + assert_eq!(concat_line(&out[0]), "- hello"); + assert_eq!(concat_line(&out[1]), " world"); + assert_eq!(concat_line(&out[2]), " foo"); + } + + #[test] + fn empty_initial_indent_subsequent_spaces() { + let opts = RtOptions::new(8) + .initial_indent(Line::from("")) + .subsequent_indent(Line::from(" ")); + let line = Line::from("hello world foobar"); + let out = word_wrap_line(&line, opts); + assert!(concat_line(&out[0]).starts_with("hello")); + for l in &out[1..] { + assert!(concat_line(l).starts_with(" ")); + } + } + + #[test] + fn empty_input_yields_single_empty_line() { + let line = Line::from(""); + let out = word_wrap_line(&line, 10); + assert_eq!(out.len(), 1); + assert_eq!(concat_line(&out[0]), ""); + } + + #[test] + fn leading_spaces_preserved_on_first_line() { + let line = Line::from(" hello"); + let out = word_wrap_line(&line, 8); + assert_eq!(out.len(), 1); + assert_eq!(concat_line(&out[0]), " hello"); + } + + #[test] + fn multiple_spaces_between_words_dont_start_next_line_with_spaces() { + let line = Line::from("hello world"); + let out = word_wrap_line(&line, 8); + assert_eq!(out.len(), 2); + assert_eq!(concat_line(&out[0]), "hello"); + assert_eq!(concat_line(&out[1]), "world"); + } + + #[test] + fn break_words_false_allows_overflow_for_long_word() { + let opts = RtOptions::new(5).break_words(false); + let line = Line::from("supercalifragilistic"); + let out = word_wrap_line(&line, opts); + assert_eq!(out.len(), 1); + assert_eq!(concat_line(&out[0]), "supercalifragilistic"); + } + + #[test] + fn hyphen_splitter_breaks_at_hyphen() { + let line = Line::from("hello-world"); + let out = word_wrap_line(&line, 7); + assert_eq!(out.len(), 2); + assert_eq!(concat_line(&out[0]), "hello-"); + assert_eq!(concat_line(&out[1]), "world"); + } + + #[test] + fn indent_consumes_width_leaving_one_char_space() { + let opts = RtOptions::new(4) + .initial_indent(Line::from(">>>>")) + .subsequent_indent(Line::from("--")); + let line = Line::from("hello"); + let out = word_wrap_line(&line, opts); + assert_eq!(out.len(), 3); + assert_eq!(concat_line(&out[0]), ">>>>h"); + assert_eq!(concat_line(&out[1]), "--el"); + assert_eq!(concat_line(&out[2]), "--lo"); + } + + #[test] + fn wide_unicode_wraps_by_display_width() { + let line = Line::from("😀😀😀"); + let out = word_wrap_line(&line, 4); + assert_eq!(out.len(), 2); + assert_eq!(concat_line(&out[0]), "😀😀"); + assert_eq!(concat_line(&out[1]), "😀"); + } + + #[test] + fn styled_split_within_span_preserves_style() { + use ratatui::style::Stylize; + let line = Line::from(vec!["abcd".red()]); + let out = word_wrap_line(&line, 2); + assert_eq!(out.len(), 2); + assert_eq!(out[0].spans.len(), 1); + assert_eq!(out[1].spans.len(), 1); + assert_eq!(out[0].spans[0].style.fg, Some(Color::Red)); + assert_eq!(out[1].spans[0].style.fg, Some(Color::Red)); + assert_eq!(concat_line(&out[0]), "ab"); + assert_eq!(concat_line(&out[1]), "cd"); + } + + #[test] + fn wrap_lines_applies_initial_indent_only_once() { + let opts = RtOptions::new(8) + .initial_indent(Line::from("- ")) + .subsequent_indent(Line::from(" ")); + + let lines = vec![Line::from("hello world"), Line::from("foo bar baz")]; + let out = word_wrap_lines(&lines, opts); + + // Expect: first line prefixed with "- ", subsequent wrapped pieces with " " + // and for the second input line, there should be no "- " prefix on its first piece + let rendered: Vec = out.iter().map(concat_line).collect(); + assert!(rendered[0].starts_with("- ")); + for r in rendered.iter().skip(1) { + assert!(r.starts_with(" ")); + } + } + + #[test] + fn wrap_lines_without_indents_is_concat_of_single_wraps() { + let lines = vec![Line::from("hello"), Line::from("world!")]; + let out = word_wrap_lines(&lines, 10); + let rendered: Vec = out.iter().map(concat_line).collect(); + assert_eq!(rendered, vec!["hello", "world!"]); + } + + #[test] + fn wrap_lines_borrowed_applies_initial_indent_only_once() { + let opts = RtOptions::new(8) + .initial_indent(Line::from("- ")) + .subsequent_indent(Line::from(" ")); + + let lines = [Line::from("hello world"), Line::from("foo bar baz")]; + let out = word_wrap_lines_borrowed(lines.iter().collect::>(), opts); + + let rendered: Vec = out.iter().map(concat_line).collect(); + assert!(rendered.first().unwrap().starts_with("- ")); + for r in rendered.iter().skip(1) { + assert!(r.starts_with(" ")); + } + } + + #[test] + fn wrap_lines_borrowed_without_indents_is_concat_of_single_wraps() { + let lines = [Line::from("hello"), Line::from("world!")]; + let out = word_wrap_lines_borrowed(lines.iter().collect::>(), 10); + let rendered: Vec = out.iter().map(concat_line).collect(); + assert_eq!(rendered, vec!["hello", "world!"]); + } + + #[test] + fn line_height_counts_double_width_emoji() { + let line = "😀😀😀".into(); // each emoji ~ width 2 + assert_eq!(word_wrap_line(&line, 4).len(), 2); + assert_eq!(word_wrap_line(&line, 2).len(), 3); + assert_eq!(word_wrap_line(&line, 6).len(), 1); + } + + #[test] + fn word_wrap_does_not_split_words_simple_english() { + let sample = "Years passed, and Willowmere thrived in peace and friendship. Mira’s herb garden flourished with both ordinary and enchanted plants, and travelers spoke of the kindness of the woman who tended them."; + let line = Line::from(sample); + let lines = [line]; + // Force small width to exercise wrapping at spaces. + let wrapped = word_wrap_lines_borrowed(&lines, 40); + let joined: String = wrapped.iter().map(|l| l.to_string()).join("\n"); + assert_eq!( + joined, + r#"Years passed, and Willowmere thrived +in peace and friendship. Mira’s herb +garden flourished with both ordinary and +enchanted plants, and travelers spoke +of the kindness of the woman who tended +them."# + ); + } +} From db3834733acb46c83922573e4a161d6553346a42 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 5 Sep 2025 09:10:41 -0700 Subject: [PATCH 12/13] [BREAKING] Stop loading project .env files (#3184) Loading project local .env often loads settings that break codex cli. Fixes: https://github.com/openai/codex/issues/3174 --- codex-rs/arg0/src/lib.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index fc66f978a5..d5df68e554 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -21,8 +21,7 @@ const MISSPELLED_APPLY_PATCH_ARG0: &str = "applypatch"; /// `codex-linux-sandbox` we *directly* execute /// [`codex_linux_sandbox::run_main`] (which never returns). Otherwise we: /// -/// 1. Use [`dotenvy::from_path`] and [`dotenvy::dotenv`] to modify the -/// environment before creating any threads. +/// 1. Load `.env` values from `~/.codex/.env` before creating any threads. /// 2. Construct a Tokio multi-thread runtime. /// 3. Derive the path to the current executable (so children can re-invoke the /// sandbox) when running on Linux. @@ -106,7 +105,7 @@ where const ILLEGAL_ENV_VAR_PREFIX: &str = "CODEX_"; -/// Load env vars from ~/.codex/.env and `$(pwd)/.env`. +/// Load env vars from ~/.codex/.env. /// /// Security: Do not allow `.env` files to create or modify any variables /// with names starting with `CODEX_`. @@ -116,10 +115,6 @@ fn load_dotenv() { { set_filtered(iter); } - - if let Ok(iter) = dotenvy::dotenv_iter() { - set_filtered(iter); - } } /// Helper to set vars from a dotenvy iterator while filtering out `CODEX_` keys. From ba631e79281bd57dbfd53966ceb0273dc265b4e6 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Fri, 5 Sep 2025 09:51:01 -0700 Subject: [PATCH 13/13] ZSH on UNIX system and better detection (#3187) --- codex-rs/core/src/shell.rs | 233 +++++++++++++++++++++++++++++-------- 1 file changed, 185 insertions(+), 48 deletions(-) diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 3a69874eb2..7ef79076e0 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -9,6 +9,12 @@ pub struct ZshShell { zshrc_path: String, } +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] +pub struct BashShell { + shell_path: String, + bashrc_path: String, +} + #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct PowerShellConfig { exe: String, // Executable name or path, e.g. "pwsh" or "powershell.exe". @@ -18,6 +24,7 @@ pub struct PowerShellConfig { #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub enum Shell { Zsh(ZshShell), + Bash(BashShell), PowerShell(PowerShellConfig), Unknown, } @@ -26,22 +33,10 @@ impl Shell { pub fn format_default_shell_invocation(&self, command: Vec) -> Option> { match self { Shell::Zsh(zsh) => { - if !std::path::Path::new(&zsh.zshrc_path).exists() { - return None; - } - - let mut result = vec![zsh.shell_path.clone()]; - result.push("-lc".to_string()); - - let joined = strip_bash_lc(&command) - .or_else(|| shlex::try_join(command.iter().map(|s| s.as_str())).ok()); - - if let Some(joined) = joined { - result.push(format!("source {} && ({joined})", zsh.zshrc_path)); - } else { - return None; - } - Some(result) + format_shell_invocation_with_rc(&command, &zsh.shell_path, &zsh.zshrc_path) + } + Shell::Bash(bash) => { + format_shell_invocation_with_rc(&command, &bash.shell_path, &bash.bashrc_path) } Shell::PowerShell(ps) => { // If model generated a bash command, prefer a detected bash fallback @@ -97,12 +92,32 @@ impl Shell { Shell::Zsh(zsh) => std::path::Path::new(&zsh.shell_path) .file_name() .map(|s| s.to_string_lossy().to_string()), + Shell::Bash(bash) => std::path::Path::new(&bash.shell_path) + .file_name() + .map(|s| s.to_string_lossy().to_string()), Shell::PowerShell(ps) => Some(ps.exe.clone()), Shell::Unknown => None, } } } +fn format_shell_invocation_with_rc( + command: &Vec, + shell_path: &str, + rc_path: &str, +) -> Option> { + let joined = strip_bash_lc(command) + .or_else(|| shlex::try_join(command.iter().map(|s| s.as_str())).ok())?; + + let rc_command = if std::path::Path::new(rc_path).exists() { + format!("source {rc_path} && ({joined})") + } else { + joined + }; + + Some(vec![shell_path.to_string(), "-lc".to_string(), rc_command]) +} + fn strip_bash_lc(command: &Vec) -> Option { match command.as_slice() { // exactly three items @@ -116,44 +131,43 @@ fn strip_bash_lc(command: &Vec) -> Option { } } -#[cfg(target_os = "macos")] -pub async fn default_user_shell() -> Shell { - use tokio::process::Command; - use whoami; +#[cfg(unix)] +fn detect_default_user_shell() -> Shell { + use libc::getpwuid; + use libc::getuid; + use std::ffi::CStr; - let user = whoami::username(); - let home = format!("/Users/{user}"); - let output = Command::new("dscl") - .args([".", "-read", &home, "UserShell"]) - .output() - .await - .ok(); - match output { - Some(o) => { - if !o.status.success() { - return Shell::Unknown; - } - let stdout = String::from_utf8_lossy(&o.stdout); - for line in stdout.lines() { - if let Some(shell_path) = line.strip_prefix("UserShell: ") - && shell_path.ends_with("/zsh") - { - return Shell::Zsh(ZshShell { - shell_path: shell_path.to_string(), - zshrc_path: format!("{home}/.zshrc"), - }); - } + unsafe { + let uid = getuid(); + let pw = getpwuid(uid); + + if !pw.is_null() { + let shell_path = CStr::from_ptr((*pw).pw_shell) + .to_string_lossy() + .into_owned(); + let home_path = CStr::from_ptr((*pw).pw_dir).to_string_lossy().into_owned(); + + if shell_path.ends_with("/zsh") { + return Shell::Zsh(ZshShell { + shell_path, + zshrc_path: format!("{home_path}/.zshrc"), + }); } - Shell::Unknown + if shell_path.ends_with("/bash") { + return Shell::Bash(BashShell { + shell_path, + bashrc_path: format!("{home_path}/.bashrc"), + }); + } } - _ => Shell::Unknown, } + Shell::Unknown } -#[cfg(all(not(target_os = "macos"), not(target_os = "windows")))] +#[cfg(unix)] pub async fn default_user_shell() -> Shell { - Shell::Unknown + detect_default_user_shell() } #[cfg(target_os = "windows")] @@ -196,8 +210,13 @@ pub async fn default_user_shell() -> Shell { } } +#[cfg(all(not(target_os = "windows"), not(unix)))] +pub async fn default_user_shell() -> Shell { + Shell::Unknown +} + #[cfg(test)] -#[cfg(target_os = "macos")] +#[cfg(unix)] mod tests { use super::*; use std::process::Command; @@ -230,9 +249,127 @@ mod tests { zshrc_path: "/does/not/exist/.zshrc".to_string(), }); let actual_cmd = shell.format_default_shell_invocation(vec!["myecho".to_string()]); - assert_eq!(actual_cmd, None); + assert_eq!( + actual_cmd, + Some(vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "myecho".to_string() + ]) + ); } + #[tokio::test] + async fn test_run_with_profile_bashrc_not_exists() { + let shell = Shell::Bash(BashShell { + shell_path: "/bin/bash".to_string(), + bashrc_path: "/does/not/exist/.bashrc".to_string(), + }); + let actual_cmd = shell.format_default_shell_invocation(vec!["myecho".to_string()]); + assert_eq!( + actual_cmd, + Some(vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "myecho".to_string() + ]) + ); + } + + #[tokio::test] + async fn test_run_with_profile_bash_escaping_and_execution() { + let shell_path = "/bin/bash"; + + let cases = vec![ + ( + vec!["myecho"], + vec![shell_path, "-lc", "source BASHRC_PATH && (myecho)"], + Some("It works!\n"), + ), + ( + vec!["bash", "-lc", "echo 'single' \"double\""], + vec![ + shell_path, + "-lc", + "source BASHRC_PATH && (echo 'single' \"double\")", + ], + Some("single double\n"), + ), + ]; + + for (input, expected_cmd, expected_output) in cases { + use std::collections::HashMap; + + use crate::exec::ExecParams; + use crate::exec::SandboxType; + use crate::exec::process_exec_tool_call; + use crate::protocol::SandboxPolicy; + + let temp_home = tempfile::tempdir().unwrap(); + let bashrc_path = temp_home.path().join(".bashrc"); + std::fs::write( + &bashrc_path, + r#" + set -x + function myecho { + echo 'It works!' + } + "#, + ) + .unwrap(); + let shell = Shell::Bash(BashShell { + shell_path: shell_path.to_string(), + bashrc_path: bashrc_path.to_str().unwrap().to_string(), + }); + + let actual_cmd = shell + .format_default_shell_invocation(input.iter().map(|s| s.to_string()).collect()); + let expected_cmd = expected_cmd + .iter() + .map(|s| { + s.replace("BASHRC_PATH", bashrc_path.to_str().unwrap()) + .to_string() + }) + .collect(); + + assert_eq!(actual_cmd, Some(expected_cmd)); + + let output = process_exec_tool_call( + ExecParams { + command: actual_cmd.unwrap(), + cwd: PathBuf::from(temp_home.path()), + timeout_ms: None, + env: HashMap::from([( + "HOME".to_string(), + temp_home.path().to_str().unwrap().to_string(), + )]), + with_escalated_permissions: None, + justification: None, + }, + SandboxType::None, + &SandboxPolicy::DangerFullAccess, + &None, + None, + ) + .await + .unwrap(); + + assert_eq!(output.exit_code, 0, "input: {input:?} output: {output:?}"); + if let Some(expected) = expected_output { + assert_eq!( + output.stdout.text, expected, + "input: {input:?} output: {output:?}" + ); + } + } + } +} + +#[cfg(test)] +#[cfg(target_os = "macos")] +mod macos_tests { + use super::*; + #[tokio::test] async fn test_run_with_profile_escaping_and_execution() { let shell_path = "/bin/zsh";