From 0935e6a875e22c33073f738e4b64292bc643bc90 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 31 Jul 2025 15:40:19 -0700 Subject: [PATCH 01/22] Send account id when available (#1767) For users with multiple accounts we need to specify the account to use. --- codex-rs/core/src/client.rs | 8 +++++++- codex-rs/core/tests/client.rs | 4 +++- codex-rs/login/src/lib.rs | 11 +++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 4e0e62c0f7..0f7f51c2fc 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -192,7 +192,7 @@ impl ModelClient { loop { attempt += 1; - let req_builder = self + let mut req_builder = self .client .post(format!("{base_url}/responses")) .header("OpenAI-Beta", "responses=experimental") @@ -201,6 +201,12 @@ impl ModelClient { .header(reqwest::header::ACCEPT, "text/event-stream") .json(&payload); + if auth.mode == AuthMode::ChatGPT { + if let Some(account_id) = auth.get_account_id().await { + req_builder = req_builder.header("chatgpt-account-id", account_id); + } + } + let req_builder = self.provider.apply_http_headers(req_builder); let res = req_builder.send().await; diff --git a/codex-rs/core/tests/client.rs b/codex-rs/core/tests/client.rs index ab8ad6833a..bfa29657d5 100644 --- a/codex-rs/core/tests/client.rs +++ b/codex-rs/core/tests/client.rs @@ -237,6 +237,7 @@ async fn chatgpt_auth_sends_correct_request() { let request_session_id = request.headers.get("session_id").unwrap(); let request_originator = request.headers.get("originator").unwrap(); let request_authorization = request.headers.get("authorization").unwrap(); + let request_chatgpt_account_id = request.headers.get("chatgpt-account-id").unwrap(); let request_body = request.body_json::().unwrap(); assert!(current_session_id.is_some()); @@ -249,6 +250,7 @@ async fn chatgpt_auth_sends_correct_request() { request_authorization.to_str().unwrap(), "Bearer Access Token" ); + assert_eq!(request_chatgpt_account_id.to_str().unwrap(), "account_id"); assert!(!request_body["store"].as_bool().unwrap()); assert!(request_body["stream"].as_bool().unwrap()); assert_eq!( @@ -332,7 +334,7 @@ fn auth_from_token(id_token: String) -> CodexAuth { id_token, access_token: "Access Token".to_string(), refresh_token: "test".to_string(), - account_id: None, + account_id: Some("account_id".to_string()), }), last_refresh: Some(Utc::now()), }), diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index 2f0aeb60bd..3d55c20264 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -123,6 +123,17 @@ impl CodexAuth { } } } + + pub async fn get_account_id(&self) -> Option { + match self.mode { + AuthMode::ApiKey => None, + AuthMode::ChatGPT => { + let token_data = self.get_token_data().await.ok()?; + + token_data.account_id.clone() + } + } + } } // Loads the available auth information from the auth.json or OPENAI_API_KEY environment variable. From 610addbc2e28159ebaf9b33e77778ba04de588ab Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 31 Jul 2025 17:00:48 -0700 Subject: [PATCH 02/22] do not dispatch key releases (#1771) when we enabled KKP in https://github.com/openai/codex/pull/1743, we started receiving keyup events, but didn't expect them anywhere in our code. for now, just don't dispatch them at all. --- codex-rs/tui/src/app.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 3013cb6df0..86c042f813 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -15,6 +15,7 @@ use color_eyre::eyre::Result; use crossterm::SynchronizedUpdate; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; use ratatui::layout::Offset; use ratatui::prelude::Backend; use std::path::PathBuf; @@ -213,6 +214,7 @@ impl App<'_> { KeyEvent { code: KeyCode::Char('c'), modifiers: crossterm::event::KeyModifiers::CONTROL, + kind: KeyEventKind::Press, .. } => { match &mut self.app_state { @@ -227,6 +229,7 @@ impl App<'_> { KeyEvent { code: KeyCode::Char('d'), modifiers: crossterm::event::KeyModifiers::CONTROL, + kind: KeyEventKind::Press, .. } => { match &mut self.app_state { @@ -245,9 +248,15 @@ impl App<'_> { } } } - _ => { + KeyEvent { + kind: KeyEventKind::Press | KeyEventKind::Repeat, + .. + } => { self.dispatch_key_event(key_event); } + _ => { + // Ignore Release key events for now. + } }; } AppEvent::Paste(text) => { From ea69a1d72f11d0b1d24f9722dd3c31ff0fe69763 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 31 Jul 2025 17:10:52 -0700 Subject: [PATCH 03/22] lighter approval modal (#1768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The yellow hazard stripes were too scary :) This also has the added benefit of not rendering anything at the full width of the terminal, so resizing is a little easier to handle. Screenshot 2025-07-31 at 4 03 29 PM Screenshot 2025-07-31 at 4 05 03 PM --- codex-rs/tui/src/app.rs | 40 ++- codex-rs/tui/src/chatwidget.rs | 5 +- codex-rs/tui/src/user_approval_widget.rs | 306 +++++++++-------------- 3 files changed, 157 insertions(+), 194 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 86c042f813..7454a1ca11 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -10,7 +10,6 @@ use crate::tui; use codex_core::config::Config; use codex_core::protocol::Event; use codex_core::protocol::EventMsg; -use codex_core::protocol::ExecApprovalRequestEvent; use color_eyre::eyre::Result; use crossterm::SynchronizedUpdate; use crossterm::event::KeyCode; @@ -313,14 +312,41 @@ impl App<'_> { } #[cfg(debug_assertions)] SlashCommand::TestApproval => { + use std::collections::HashMap; + + use codex_core::protocol::ApplyPatchApprovalRequestEvent; + use codex_core::protocol::FileChange; + self.app_event_tx.send(AppEvent::CodexEvent(Event { id: "1".to_string(), - msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { - call_id: "1".to_string(), - command: vec!["git".into(), "apply".into()], - cwd: self.config.cwd.clone(), - reason: Some("test".to_string()), - }), + // msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + // call_id: "1".to_string(), + // command: vec!["git".into(), "apply".into()], + // cwd: self.config.cwd.clone(), + // reason: Some("test".to_string()), + // }), + msg: EventMsg::ApplyPatchApprovalRequest( + ApplyPatchApprovalRequestEvent { + call_id: "1".to_string(), + changes: HashMap::from([ + ( + PathBuf::from("/tmp/test.txt"), + FileChange::Add { + content: "test".to_string(), + }, + ), + ( + PathBuf::from("/tmp/test2.txt"), + FileChange::Update { + unified_diff: "+test\n-test2".to_string(), + move_path: None, + }, + ), + ]), + reason: None, + grant_root: Some(PathBuf::from("/tmp")), + }, + ), })); } }, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 50488f8b30..b0a6fb44e1 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -25,6 +25,7 @@ use codex_core::protocol::PatchApplyBeginEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TokenUsage; use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; use ratatui::buffer::Buffer; use ratatui::layout::Rect; use ratatui::widgets::Widget; @@ -157,7 +158,9 @@ impl ChatWidget<'_> { } pub(crate) fn handle_key_event(&mut self, key_event: KeyEvent) { - self.bottom_pane.clear_ctrl_c_quit_hint(); + if key_event.kind == KeyEventKind::Press { + self.bottom_pane.clear_ctrl_c_quit_hint(); + } match self.bottom_pane.handle_key_event(key_event) { InputResult::Submitted(text) => { diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index 855a7ea3db..91febde208 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -7,23 +7,24 @@ //! driven workflow – a fully‑fledged visual match is not required. use std::path::PathBuf; +use std::sync::LazyLock; use codex_core::protocol::Op; use codex_core::protocol::ReviewDecision; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; +use crossterm::event::KeyEventKind; use ratatui::buffer::Buffer; use ratatui::layout::Rect; use ratatui::prelude::*; use ratatui::text::Line; -use ratatui::text::Span; -use ratatui::widgets::List; +use ratatui::widgets::Block; +use ratatui::widgets::BorderType; +use ratatui::widgets::Borders; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use ratatui::widgets::WidgetRef; use ratatui::widgets::Wrap; -use tui_input::Input; -use tui_input::backend::crossterm::EventHandler; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; @@ -47,68 +48,62 @@ pub(crate) enum ApprovalRequest { /// Options displayed in the *select* mode. struct SelectOption { - label: &'static str, - decision: Option, - /// `true` when this option switches the widget to *input* mode. - enters_input_mode: bool, + label: Line<'static>, + description: &'static str, + key: KeyCode, + decision: ReviewDecision, } -// keep in same order as in the TS implementation -const SELECT_OPTIONS: &[SelectOption] = &[ - SelectOption { - label: "Yes (y)", - decision: Some(ReviewDecision::Approved), +static COMMAND_SELECT_OPTIONS: LazyLock> = LazyLock::new(|| { + vec![ + SelectOption { + label: Line::from(vec!["Y".underlined(), "es".into()]), + description: "Approve and run the command", + key: KeyCode::Char('y'), + decision: ReviewDecision::Approved, + }, + SelectOption { + label: Line::from(vec!["A".underlined(), "lways".into()]), + description: "Approve the command for the remainder of this session", + key: KeyCode::Char('a'), + decision: ReviewDecision::ApprovedForSession, + }, + SelectOption { + label: Line::from(vec!["N".underlined(), "o".into()]), + description: "Do not run the command", + key: KeyCode::Char('n'), + decision: ReviewDecision::Denied, + }, + ] +}); - enters_input_mode: false, - }, - SelectOption { - label: "Yes, always approve this exact command for this session (a)", - decision: Some(ReviewDecision::ApprovedForSession), - - enters_input_mode: false, - }, - SelectOption { - label: "Edit or give feedback (e)", - decision: None, - - enters_input_mode: true, - }, - SelectOption { - label: "No, and keep going (n)", - decision: Some(ReviewDecision::Denied), - - enters_input_mode: false, - }, - SelectOption { - label: "No, and stop for now (esc)", - decision: Some(ReviewDecision::Abort), - - enters_input_mode: false, - }, -]; - -/// Internal mode the widget is in – mirrors the TypeScript component. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum Mode { - Select, - Input, -} +static PATCH_SELECT_OPTIONS: LazyLock> = LazyLock::new(|| { + vec![ + SelectOption { + label: Line::from(vec!["Y".underlined(), "es".into()]), + description: "Approve and apply the changes", + key: KeyCode::Char('y'), + decision: ReviewDecision::Approved, + }, + SelectOption { + label: Line::from(vec!["N".underlined(), "o".into()]), + description: "Do not apply the changes", + key: KeyCode::Char('n'), + decision: ReviewDecision::Denied, + }, + ] +}); /// A modal prompting the user to approve or deny the pending request. pub(crate) struct UserApprovalWidget<'a> { approval_request: ApprovalRequest, app_event_tx: AppEventSender, confirmation_prompt: Paragraph<'a>, + select_options: &'a Vec, /// Currently selected index in *select* mode. selected_option: usize, - /// State for the optional input widget. - input: Input, - - /// Current mode. - mode: Mode, - /// Set to `true` once a decision has been sent – the parent view can then /// remove this widget from its queue. done: bool, @@ -116,7 +111,6 @@ pub(crate) struct UserApprovalWidget<'a> { impl UserApprovalWidget<'_> { pub(crate) fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self { - let input = Input::default(); let confirmation_prompt = match &approval_request { ApprovalRequest::Exec { command, @@ -132,25 +126,20 @@ impl UserApprovalWidget<'_> { None => cwd.display().to_string(), }; let mut contents: Vec = vec![ - Line::from(vec![ - Span::from(cwd_str).dim(), - Span::from("$"), - Span::from(format!(" {cmd}")), - ]), + Line::from(vec!["codex".bold().magenta(), " wants to run:".into()]), + Line::from(vec![cwd_str.dim(), "$".into(), format!(" {cmd}").into()]), Line::from(""), ]; if let Some(reason) = reason { contents.push(Line::from(reason.clone().italic())); contents.push(Line::from("")); } - contents.extend(vec![Line::from("Allow command?"), Line::from("")]); Paragraph::new(contents).wrap(Wrap { trim: false }) } ApprovalRequest::ApplyPatch { reason, grant_root, .. } => { - let mut contents: Vec = - vec![Line::from("Apply patch".bold()), Line::from("")]; + let mut contents: Vec = vec![]; if let Some(r) = reason { contents.push(Line::from(r.clone().italic())); @@ -165,20 +154,19 @@ impl UserApprovalWidget<'_> { contents.push(Line::from("")); } - contents.push(Line::from("Allow changes?")); - contents.push(Line::from("")); - - Paragraph::new(contents) + Paragraph::new(contents).wrap(Wrap { trim: false }) } }; Self { + select_options: match &approval_request { + ApprovalRequest::Exec { .. } => &COMMAND_SELECT_OPTIONS, + ApprovalRequest::ApplyPatch { .. } => &PATCH_SELECT_OPTIONS, + }, approval_request, app_event_tx, confirmation_prompt, selected_option: 0, - input, - mode: Mode::Select, done: false, } } @@ -194,9 +182,8 @@ impl UserApprovalWidget<'_> { /// captures input while visible, we don’t need to report whether the event /// was consumed—callers can assume it always is. pub(crate) fn handle_key_event(&mut self, key: KeyEvent) { - match self.mode { - Mode::Select => self.handle_select_key(key), - Mode::Input => self.handle_input_key(key), + if key.kind == KeyEventKind::Press { + self.handle_select_key(key); } } @@ -208,58 +195,24 @@ impl UserApprovalWidget<'_> { fn handle_select_key(&mut self, key_event: KeyEvent) { match key_event.code { - KeyCode::Up => { - if self.selected_option == 0 { - self.selected_option = SELECT_OPTIONS.len() - 1; - } else { - self.selected_option -= 1; - } + KeyCode::Left => { + self.selected_option = (self.selected_option + self.select_options.len() - 1) + % self.select_options.len(); } - KeyCode::Down => { - self.selected_option = (self.selected_option + 1) % SELECT_OPTIONS.len(); - } - KeyCode::Char('y') => { - self.send_decision(ReviewDecision::Approved); - } - KeyCode::Char('a') => { - self.send_decision(ReviewDecision::ApprovedForSession); - } - KeyCode::Char('n') => { - self.send_decision(ReviewDecision::Denied); - } - KeyCode::Char('e') => { - self.mode = Mode::Input; + KeyCode::Right => { + self.selected_option = (self.selected_option + 1) % self.select_options.len(); } KeyCode::Enter => { - let opt = &SELECT_OPTIONS[self.selected_option]; - if opt.enters_input_mode { - self.mode = Mode::Input; - } else if let Some(decision) = opt.decision { - self.send_decision(decision); - } + let opt = &self.select_options[self.selected_option]; + self.send_decision(opt.decision); } KeyCode::Esc => { self.send_decision(ReviewDecision::Abort); } - _ => {} - } - } - - fn handle_input_key(&mut self, key_event: KeyEvent) { - // Handle special keys first. - match key_event.code { - KeyCode::Enter => { - let feedback = self.input.value().to_string(); - self.send_decision_with_feedback(ReviewDecision::Denied, feedback); - } - KeyCode::Esc => { - // Cancel input – treat as deny without feedback. - self.send_decision(ReviewDecision::Denied); - } - _ => { - // Feed into input widget for normal editing. - let ct_event = crossterm::event::Event::Key(key_event); - self.input.handle_event(&ct_event); + other => { + if let Some(opt) = self.select_options.iter().find(|opt| opt.key == other) { + self.send_decision(opt.decision); + } } } } @@ -312,87 +265,68 @@ impl UserApprovalWidget<'_> { } pub(crate) fn desired_height(&self, width: u16) -> u16 { - self.get_confirmation_prompt_height(width - 2) + SELECT_OPTIONS.len() as u16 + 2 + self.get_confirmation_prompt_height(width) + self.select_options.len() as u16 } } -const PLAIN: Style = Style::new(); -const BLUE_FG: Style = Style::new().fg(Color::LightCyan); - impl WidgetRef for &UserApprovalWidget<'_> { fn render_ref(&self, area: Rect, buf: &mut Buffer) { - // Take the area, wrap it in a block with a border, and divide up the - // remaining area into two chunks: one for the confirmation prompt and - // one for the response. - let inner = area.inner(Margin::new(0, 2)); - - // Determine how many rows we can allocate for the static confirmation - // prompt while *always* keeping enough space for the interactive - // response area (select list or input field). When the full prompt - // would exceed the available height we truncate it so the response - // options never get pushed out of view. This keeps the approval modal - // usable even when the overall bottom viewport is small. - - // Full height of the prompt (may be larger than the available area). - let full_prompt_height = self.get_confirmation_prompt_height(inner.width); - - // Minimum rows that must remain for the interactive section. - let min_response_rows = match self.mode { - Mode::Select => SELECT_OPTIONS.len() as u16, - // In input mode we need exactly two rows: one for the guidance - // prompt and one for the single-line input field. - Mode::Input => 2, - }; - - // Clamp prompt height so confirmation + response never exceed the - // available space. `saturating_sub` avoids underflow when the area is - // too small even for the minimal layout – in this unlikely case we - // fall back to zero-height prompt so at least the options are - // visible. - let prompt_height = full_prompt_height.min(inner.height.saturating_sub(min_response_rows)); - - let chunks = Layout::default() + let prompt_height = self.get_confirmation_prompt_height(area.width); + let [prompt_chunk, response_chunk] = Layout::default() .direction(Direction::Vertical) .constraints([Constraint::Length(prompt_height), Constraint::Min(0)]) - .split(inner); - let prompt_chunk = chunks[0]; - let response_chunk = chunks[1]; + .areas(area); - // Build the inner lines based on the mode. Collect them into a List of - // non-wrapping lines rather than a Paragraph for predictable layout. - let lines = match self.mode { - Mode::Select => SELECT_OPTIONS - .iter() - .enumerate() - .map(|(idx, opt)| { - let (prefix, style) = if idx == self.selected_option { - ("▶", BLUE_FG) - } else { - (" ", PLAIN) - }; - Line::styled(format!(" {prefix} {}", opt.label), style) - }) - .collect(), - Mode::Input => { - vec![ - Line::from("Give the model feedback on this command:"), - Line::from(self.input.value()), - ] - } + let lines: Vec = self + .select_options + .iter() + .enumerate() + .map(|(idx, opt)| { + let style = if idx == self.selected_option { + Style::new().bg(Color::Cyan).fg(Color::Black) + } else { + Style::new().bg(Color::DarkGray) + }; + opt.label.clone().alignment(Alignment::Center).style(style) + }) + .collect(); + + let [title_area, button_area, description_area] = Layout::vertical([ + Constraint::Length(1), + Constraint::Length(1), + Constraint::Min(0), + ]) + .areas(response_chunk.inner(Margin::new(1, 0))); + let title = match &self.approval_request { + ApprovalRequest::Exec { .. } => "Allow command?", + ApprovalRequest::ApplyPatch { .. } => "Apply changes?", }; - - let border = ("◢◤") - .repeat((area.width / 2).into()) - .fg(Color::LightYellow); - - border.render_ref(area, buf); - Paragraph::new(" Execution Request ".bold().black().on_light_yellow()) - .alignment(Alignment::Center) - .render_ref(area, buf); + Line::from(title).render(title_area, buf); self.confirmation_prompt.clone().render(prompt_chunk, buf); - List::new(lines).render_ref(response_chunk, buf); + let areas = Layout::horizontal( + lines + .iter() + .map(|l| Constraint::Length(l.width() as u16 + 2)), + ) + .spacing(1) + .split(button_area); + for (idx, area) in areas.iter().enumerate() { + let line = &lines[idx]; + line.render(*area, buf); + } - border.render_ref(Rect::new(0, area.y + area.height - 1, area.width, 1), buf); + Line::from(self.select_options[self.selected_option].description) + .style(Style::new().italic().fg(Color::DarkGray)) + .render(description_area.inner(Margin::new(1, 0)), buf); + + Block::bordered() + .border_type(BorderType::QuadrantOutside) + .border_style(Style::default().fg(Color::Cyan)) + .borders(Borders::LEFT) + .render_ref( + Rect::new(0, response_chunk.y, 1, response_chunk.height), + buf, + ); } } From d787434aa81c097f242b39c1101ec7f09afa8a72 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 31 Jul 2025 17:13:36 -0700 Subject: [PATCH 04/22] fix: always send KeyEvent, we now check kind in the handler (#1772) https://github.com/openai/codex/pull/1754 and #1771 fixed the same thing in colliding ways. --- codex-rs/tui/src/app.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 7454a1ca11..24b2301bb9 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -96,9 +96,7 @@ impl App<'_> { if let Ok(event) = crossterm::event::read() { match event { crossterm::event::Event::Key(key_event) => { - if key_event.kind == crossterm::event::KeyEventKind::Press { - app_event_tx.send(AppEvent::KeyEvent(key_event)); - } + app_event_tx.send(AppEvent::KeyEvent(key_event)); } crossterm::event::Event::Resize(_, _) => { app_event_tx.send(AppEvent::RequestRedraw); From 4aca3e46c84431b6afd883e00baf6189805e2ab1 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 31 Jul 2025 17:15:26 -0700 Subject: [PATCH 05/22] insert history lines with redraw (#1769) This delays the call to insert_history_lines until a redraw is happening. Crucially, the new lines are inserted _after the viewport is resized_. This results in fewer stray blank lines below the viewport when modals (e.g. user approval) are closed. --- codex-rs/tui/src/app.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 24b2301bb9..a03cc89983 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -17,6 +17,7 @@ use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; use ratatui::layout::Offset; use ratatui::prelude::Backend; +use ratatui::text::Line; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicBool; @@ -55,6 +56,8 @@ pub(crate) struct App<'a> { /// True when a redraw has been scheduled but not yet executed. pending_redraw: Arc, + pending_history_lines: Vec>, + /// Stored parameters needed to instantiate the ChatWidget later, e.g., /// after dismissing the Git-repo warning. chat_args: Option, @@ -152,6 +155,7 @@ impl App<'_> { let file_search = FileSearchManager::new(config.cwd.clone(), app_event_tx.clone()); Self { app_event_tx, + pending_history_lines: Vec::new(), app_event_rx, app_state, config, @@ -197,7 +201,7 @@ impl App<'_> { while let Ok(event) = self.app_event_rx.recv() { match event { AppEvent::InsertHistory(lines) => { - crate::insert_history::insert_history_lines(terminal, lines); + self.pending_history_lines.extend(lines); self.app_event_tx.send(AppEvent::RequestRedraw); } AppEvent::RequestRedraw => { @@ -409,6 +413,13 @@ impl App<'_> { terminal.clear()?; terminal.set_viewport_area(area); } + if !self.pending_history_lines.is_empty() { + crate::insert_history::insert_history_lines( + terminal, + self.pending_history_lines.clone(), + ); + self.pending_history_lines.clear(); + } match &mut self.app_state { AppState::Chat { widget } => { terminal.draw(|frame| frame.render_widget_ref(&**widget, frame.area()))?; From 575590e4c2e432e8cf5392a5214da577621d4c8d Mon Sep 17 00:00:00 2001 From: easong-openai Date: Thu, 31 Jul 2025 17:30:44 -0700 Subject: [PATCH 06/22] Detect kitty terminals (#1748) We want to detect kitty terminals so we can preferentially upgrade their UX without degrading older terminals. --- codex-rs/tui/Cargo.toml | 2 ++ codex-rs/tui/src/app.rs | 12 +++++++ .../src/bottom_pane/approval_modal_view.rs | 1 + codex-rs/tui/src/bottom_pane/chat_composer.rs | 31 +++++++++++++------ codex-rs/tui/src/bottom_pane/mod.rs | 9 +++++- ...mposer__tests__backspace_after_pastes.snap | 2 +- ...tom_pane__chat_composer__tests__empty.snap | 2 +- ...tom_pane__chat_composer__tests__large.snap | 2 +- ...chat_composer__tests__multiple_pastes.snap | 2 +- ...tom_pane__chat_composer__tests__small.snap | 2 +- codex-rs/tui/src/chatwidget.rs | 2 ++ 11 files changed, 52 insertions(+), 15 deletions(-) diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 63d287ca11..468f2f3be6 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -62,6 +62,8 @@ unicode-segmentation = "1.12.0" unicode-width = "0.1" uuid = "1" + + [dev-dependencies] insta = "1.43.1" pretty_assertions = "1" diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index a03cc89983..cc3118e860 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -15,6 +15,7 @@ use crossterm::SynchronizedUpdate; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; +use crossterm::terminal::supports_keyboard_enhancement; use ratatui::layout::Offset; use ratatui::prelude::Backend; use ratatui::text::Line; @@ -61,6 +62,8 @@ pub(crate) struct App<'a> { /// Stored parameters needed to instantiate the ChatWidget later, e.g., /// after dismissing the Git-repo warning. chat_args: Option, + + enhanced_keys_supported: bool, } /// Aggregate parameters needed to create a `ChatWidget`, as creation may be @@ -70,6 +73,7 @@ struct ChatWidgetArgs { config: Config, initial_prompt: Option, initial_images: Vec, + enhanced_keys_supported: bool, } impl App<'_> { @@ -83,6 +87,8 @@ impl App<'_> { let app_event_tx = AppEventSender::new(app_event_tx); let pending_redraw = Arc::new(AtomicBool::new(false)); + let enhanced_keys_supported = supports_keyboard_enhancement().unwrap_or(false); + // Spawn a dedicated thread for reading the crossterm event loop and // re-publishing the events as AppEvents, as appropriate. { @@ -135,6 +141,7 @@ impl App<'_> { config: config.clone(), initial_prompt, initial_images, + enhanced_keys_supported, }), ) } else { @@ -143,6 +150,7 @@ impl App<'_> { app_event_tx.clone(), initial_prompt, initial_images, + enhanced_keys_supported, ); ( AppState::Chat { @@ -162,6 +170,7 @@ impl App<'_> { file_search, pending_redraw, chat_args, + enhanced_keys_supported, } } @@ -284,6 +293,7 @@ impl App<'_> { self.app_event_tx.clone(), None, Vec::new(), + self.enhanced_keys_supported, )); self.app_state = AppState::Chat { widget: new_widget }; self.app_event_tx.send(AppEvent::RequestRedraw); @@ -400,6 +410,7 @@ impl App<'_> { AppState::Chat { widget } => widget.desired_height(size.width), AppState::GitWarning { .. } => 10, }; + let mut area = terminal.viewport_area; area.height = desired_height.min(size.height); area.width = size.width; @@ -451,6 +462,7 @@ impl App<'_> { self.app_event_tx.clone(), args.initial_prompt, args.initial_images, + args.enhanced_keys_supported, )); self.app_state = AppState::Chat { widget }; self.app_event_tx.send(AppEvent::RequestRedraw); diff --git a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs index 4cd952f9eb..8e9ff6d936 100644 --- a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs +++ b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs @@ -99,6 +99,7 @@ mod tests { let mut pane = BottomPane::new(super::super::BottomPaneParams { app_event_tx: AppEventSender::new(tx_raw2), has_input_focus: true, + enhanced_keys_supported: false, }); assert_eq!(CancellationEvent::Handled, view.on_ctrl_c(&mut pane)); assert!(view.queue.is_empty()); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 3bc573a003..7e187bec8b 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -41,6 +41,7 @@ pub(crate) struct ChatComposer<'a> { app_event_tx: AppEventSender, history: ChatComposerHistory, ctrl_c_quit_hint: bool, + use_shift_enter_hint: bool, dismissed_file_popup_token: Option, current_file_query: Option, pending_pastes: Vec<(String, String)>, @@ -54,17 +55,24 @@ enum ActivePopup { } impl ChatComposer<'_> { - pub fn new(has_input_focus: bool, app_event_tx: AppEventSender) -> Self { + pub fn new( + has_input_focus: bool, + app_event_tx: AppEventSender, + enhanced_keys_supported: bool, + ) -> Self { let mut textarea = TextArea::default(); textarea.set_placeholder_text(BASE_PLACEHOLDER_TEXT); textarea.set_cursor_line_style(ratatui::style::Style::default()); + let use_shift_enter_hint = enhanced_keys_supported; + let mut this = Self { textarea, active_popup: ActivePopup::None, app_event_tx, history: ChatComposerHistory::new(), ctrl_c_quit_hint: false, + use_shift_enter_hint, dismissed_file_popup_token: None, current_file_query: None, pending_pastes: Vec::new(), @@ -712,11 +720,16 @@ impl WidgetRef for &ChatComposer<'_> { Span::from(" to quit"), ] } else { + let newline_hint_key = if self.use_shift_enter_hint { + "Shift+⏎" + } else { + "Ctrl+J" + }; vec![ Span::from(" "), "⏎".set_style(key_hint_style), Span::from(" send "), - "Shift+⏎".set_style(key_hint_style), + newline_hint_key.set_style(key_hint_style), Span::from(" newline "), "Ctrl+C".set_style(key_hint_style), Span::from(" quit"), @@ -890,7 +903,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); let needs_redraw = composer.handle_paste("hello".to_string()); assert!(needs_redraw); @@ -913,7 +926,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); let large = "x".repeat(LARGE_PASTE_CHAR_THRESHOLD + 10); let needs_redraw = composer.handle_paste(large.clone()); @@ -942,7 +955,7 @@ mod tests { let large = "y".repeat(LARGE_PASTE_CHAR_THRESHOLD + 1); let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); composer.handle_paste(large); assert_eq!(composer.pending_pastes.len(), 1); @@ -978,7 +991,7 @@ mod tests { for (name, input) in test_cases { // Create a fresh composer for each test case - let mut composer = ChatComposer::new(true, sender.clone()); + let mut composer = ChatComposer::new(true, sender.clone(), false); if let Some(text) = input { composer.handle_paste(text); @@ -1015,7 +1028,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); // Define test cases: (paste content, is_large) let test_cases = [ @@ -1088,7 +1101,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); // Define test cases: (content, is_large) let test_cases = [ @@ -1161,7 +1174,7 @@ mod tests { let (tx, _rx) = std::sync::mpsc::channel(); let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new(true, sender); + let mut composer = ChatComposer::new(true, sender, false); // Define test cases: (cursor_position_from_end, expected_pending_count) let test_cases = [ diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 2710a3e997..281f0859a5 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -50,12 +50,18 @@ pub(crate) struct BottomPane<'a> { pub(crate) struct BottomPaneParams { pub(crate) app_event_tx: AppEventSender, pub(crate) has_input_focus: bool, + pub(crate) enhanced_keys_supported: bool, } impl BottomPane<'_> { pub fn new(params: BottomPaneParams) -> Self { + let enhanced_keys_supported = params.enhanced_keys_supported; Self { - composer: ChatComposer::new(params.has_input_focus, params.app_event_tx.clone()), + composer: ChatComposer::new( + params.has_input_focus, + params.app_event_tx.clone(), + enhanced_keys_supported, + ), active_view: None, app_event_tx: params.app_event_tx, has_input_focus: params.has_input_focus, @@ -298,6 +304,7 @@ mod tests { let mut pane = BottomPane::new(BottomPaneParams { app_event_tx: tx, has_input_focus: true, + enhanced_keys_supported: false, }); pane.push_approval_request(exec_request()); assert_eq!(CancellationEvent::Handled, pane.on_ctrl_c()); diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap index 4f155dab30..178e2cc40f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__backspace_after_pastes.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap index 4e8371f177..7a1a7f4ef3 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__empty.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap index 80fea40d5f..d3f67c9219 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__large.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap index 26e8d26733..999e7b2e3f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__multiple_pastes.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap index 0f1b9e6426..68d7711aab 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__small.snap @@ -11,4 +11,4 @@ expression: terminal.backend() "▌ " "▌ " "▌ " -" ⏎ send Shift+⏎ newline Ctrl+C quit " +" ⏎ send Ctrl+J newline Ctrl+C quit " diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index b0a6fb44e1..4c415777a9 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -95,6 +95,7 @@ impl ChatWidget<'_> { app_event_tx: AppEventSender, initial_prompt: Option, initial_images: Vec, + enhanced_keys_supported: bool, ) -> Self { let (codex_op_tx, mut codex_op_rx) = unbounded_channel::(); @@ -140,6 +141,7 @@ impl ChatWidget<'_> { bottom_pane: BottomPane::new(BottomPaneParams { app_event_tx, has_input_focus: true, + enhanced_keys_supported, }), config, initial_user_message: create_initial_user_message( From d3aa5f46b78e48fc25b9d867740b7315a0c28cdd Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Thu, 31 Jul 2025 19:30:03 -0700 Subject: [PATCH 07/22] MCP Protocol: Align tool-call response with CallToolResult [Stack 1/3] (#1750) # Summary - Align MCP server responses with mcp_types by emitting [CallToolResult, RequestId] instead of an object. Update send-message result to a tagged enum: Ok or Error { message }. # Why Protocol compliance with current MCP schema. # Tests - Updated assertions in mcp_protocol.rs for create/stream/send/list and error cases. This is the first PR in a stack. Stack: Final: #1686 Intermediate: #1751 First: #1750 --- codex-rs/mcp-server/src/mcp_protocol.rs | 92 ++++++++++++++++++++----- 1 file changed, 74 insertions(+), 18 deletions(-) diff --git a/codex-rs/mcp-server/src/mcp_protocol.rs b/codex-rs/mcp-server/src/mcp_protocol.rs index e507376c16..23304dc43d 100644 --- a/codex-rs/mcp-server/src/mcp_protocol.rs +++ b/codex-rs/mcp-server/src/mcp_protocol.rs @@ -7,7 +7,10 @@ use serde::Serialize; use strum_macros::Display; use uuid::Uuid; +use mcp_types::CallToolResult; +use mcp_types::ContentBlock; use mcp_types::RequestId; +use mcp_types::TextContent; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(transparent)] @@ -118,10 +121,47 @@ pub struct ToolCallResponse { pub request_id: RequestId, #[serde(default, skip_serializing_if = "Option::is_none")] pub is_error: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none", flatten)] pub result: Option, } +impl From for CallToolResult { + fn from(val: ToolCallResponse) -> Self { + let ToolCallResponse { + request_id: _request_id, + is_error, + result, + } = val; + let (content, structured_content, is_error_out) = match result { + Some(res) => match serde_json::to_value(&res) { + Ok(v) => { + let content = vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_string(), + text: v.to_string(), + annotations: None, + })]; + (content, Some(v), is_error) + } + Err(e) => { + let content = vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_string(), + text: format!("Failed to serialize tool result: {e}"), + annotations: None, + })]; + (content, None, Some(true)) + } + }, + None => (vec![], None, is_error), + }; + + CallToolResult { + content, + is_error: is_error_out, + structured_content, + } + } +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(untagged)] pub enum ToolCallResponseResult { @@ -141,8 +181,10 @@ pub struct ConversationCreateResult { pub struct ConversationStreamResult {} #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationSendMessageResult { - pub success: bool, +#[serde(tag = "status", rename_all = "camelCase")] +pub enum ConversationSendMessageResult { + Ok, + Error { message: String }, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -455,10 +497,13 @@ mod tests { }, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 1, - "result": { + "content": [ + { "type": "text", "text": "{\"conversation_id\":\"d0f6ecbe-84a2-41c1-b23d-b20473b25eab\",\"model\":\"o3\"}" } + ], + "structuredContent": { "conversation_id": "d0f6ecbe-84a2-41c1-b23d-b20473b25eab", "model": "o3" } @@ -467,6 +512,7 @@ mod tests { observed, expected, "response (ConversationCreate) must match" ); + assert_eq!(req_id, RequestId::Integer(1)); } #[test] @@ -478,15 +524,17 @@ mod tests { ConversationStreamResult {}, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 2, - "result": {} + "content": [ { "type": "text", "text": "{}" } ], + "structuredContent": {} }); assert_eq!( observed, expected, "response (ConversationStream) must have empty object result" ); + assert_eq!(req_id, RequestId::Integer(2)); } #[test] @@ -495,18 +543,20 @@ mod tests { request_id: RequestId::Integer(3), is_error: None, result: Some(ToolCallResponseResult::ConversationSendMessage( - ConversationSendMessageResult { success: true }, + ConversationSendMessageResult::Ok, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 3, - "result": { "success": true } + "content": [ { "type": "text", "text": "{\"status\":\"ok\"}" } ], + "structuredContent": { "status": "ok" } }); assert_eq!( observed, expected, "response (ConversationSendMessageAccepted) must match" ); + assert_eq!(req_id, RequestId::Integer(3)); } #[test] @@ -526,10 +576,13 @@ mod tests { }, )), }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 4, - "result": { + "content": [ + { "type": "text", "text": "{\"conversations\":[{\"conversation_id\":\"67e55044-10b1-426f-9247-bb680e5fe0c8\",\"title\":\"Refactor config loader\"}],\"next_cursor\":\"next123\"}" } + ], + "structuredContent": { "conversations": [ { "conversation_id": "67e55044-10b1-426f-9247-bb680e5fe0c8", @@ -543,6 +596,7 @@ mod tests { observed, expected, "response (ConversationsList with cursor) must match" ); + assert_eq!(req_id, RequestId::Integer(4)); } #[test] @@ -552,15 +606,17 @@ mod tests { is_error: Some(true), result: None, }; - let observed = to_val(&env); + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); let expected = json!({ - "requestId": 4, + "content": [], "isError": true }); assert_eq!( observed, expected, "error response must omit `result` and include `isError`" ); + assert_eq!(req_id, RequestId::Integer(4)); } // ----- Notifications ----- From ad0295b89336d0264cd68d1389e977239e4751f0 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Thu, 31 Jul 2025 19:46:04 -0700 Subject: [PATCH 08/22] MCP server: route structured tool-call requests and expose mcp_protocol [Stack 2/3] (#1751) - Expose mcp_protocol from mcp-server for reuse in tests and callers. - In MessageProcessor, detect structured ToolCallRequestParams in tools/call and forward to a new handler. - Add handle_new_tool_calls scaffold (returns error for now). - Test helper: add send_send_user_message_tool_call to McpProcess to send ConversationSendMessage requests; This is the second PR in a stack. Stack: Final: #1686 Intermediate: #1751 First: #1750 --- codex-rs/Cargo.lock | 2 ++ codex-rs/mcp-server/src/lib.rs | 2 +- codex-rs/mcp-server/src/message_processor.rs | 23 ++++++++++++++++ codex-rs/mcp-server/tests/common/Cargo.toml | 2 ++ .../mcp-server/tests/common/mcp_process.rs | 26 +++++++++++++++++++ 5 files changed, 54 insertions(+), 1 deletion(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 87d59b21be..9123bd6374 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2643,6 +2643,7 @@ version = "0.0.0" dependencies = [ "anyhow", "assert_cmd", + "codex-core", "codex-mcp-server", "mcp-types", "pretty_assertions", @@ -2650,6 +2651,7 @@ dependencies = [ "shlex", "tempfile", "tokio", + "uuid", "wiremock", ] diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index 0912fed118..ebef8ca903 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -19,7 +19,7 @@ mod codex_tool_config; mod codex_tool_runner; mod exec_approval; mod json_to_toml; -mod mcp_protocol; +pub mod mcp_protocol; mod message_processor; mod outgoing_message; mod patch_approval; diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 7ba827d60b..a4013cc3a3 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -6,6 +6,7 @@ use crate::codex_tool_config::CodexToolCallParam; use crate::codex_tool_config::CodexToolCallReplyParam; use crate::codex_tool_config::create_tool_for_codex_tool_call_param; use crate::codex_tool_config::create_tool_for_codex_tool_call_reply_param; +use crate::mcp_protocol::ToolCallRequestParams; use crate::outgoing_message::OutgoingMessageSender; use codex_core::Codex; @@ -300,6 +301,14 @@ impl MessageProcessor { params: ::Params, ) { tracing::info!("tools/call -> params: {:?}", params); + // Serialize params into JSON and try to parse as new type + if let Ok(new_params) = + serde_json::to_value(¶ms).and_then(serde_json::from_value::) + { + // New tool call matched → forward + self.handle_new_tool_calls(id, new_params).await; + return; + } let CallToolRequestParams { name, arguments } = params; match name.as_str() { @@ -323,6 +332,20 @@ impl MessageProcessor { } } } + async fn handle_new_tool_calls(&self, request_id: RequestId, _params: ToolCallRequestParams) { + // TODO: implement the new tool calls + let result = CallToolResult { + content: vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_string(), + text: "Unknown tool".to_string(), + annotations: None, + })], + is_error: Some(true), + structured_content: None, + }; + self.send_response::(request_id, result) + .await; + } async fn handle_tool_call_codex(&self, id: RequestId, arguments: Option) { let (initial_prompt, config): (String, CodexConfig) = match arguments { diff --git a/codex-rs/mcp-server/tests/common/Cargo.toml b/codex-rs/mcp-server/tests/common/Cargo.toml index 3aa246f154..420b97811d 100644 --- a/codex-rs/mcp-server/tests/common/Cargo.toml +++ b/codex-rs/mcp-server/tests/common/Cargo.toml @@ -10,6 +10,7 @@ path = "lib.rs" anyhow = "1" assert_cmd = "2" codex-mcp-server = { path = "../.." } +codex-core = { path = "../../../core" } mcp-types = { path = "../../../mcp-types" } pretty_assertions = "1.4.1" serde_json = "1" @@ -22,3 +23,4 @@ tokio = { version = "1", features = [ "rt-multi-thread", ] } wiremock = "0.6" +uuid = { version = "1", features = ["serde", "v4"] } \ No newline at end of file diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index 8138749c40..d7144af480 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -11,8 +11,13 @@ use tokio::process::ChildStdout; use anyhow::Context; use assert_cmd::prelude::*; +use codex_core::protocol::InputItem; use codex_mcp_server::CodexToolCallParam; use codex_mcp_server::CodexToolCallReplyParam; +use codex_mcp_server::mcp_protocol::ConversationId; +use codex_mcp_server::mcp_protocol::ConversationSendMessageArgs; +use codex_mcp_server::mcp_protocol::ToolCallRequestParams; + use mcp_types::CallToolRequestParams; use mcp_types::ClientCapabilities; use mcp_types::Implementation; @@ -29,6 +34,7 @@ use pretty_assertions::assert_eq; use serde_json::json; use std::process::Command as StdCommand; use tokio::process::Command; +use uuid::Uuid; pub struct McpProcess { next_request_id: AtomicI64, @@ -174,6 +180,26 @@ impl McpProcess { .await } + pub async fn send_user_message_tool_call( + &mut self, + message: &str, + session_id: &str, + ) -> anyhow::Result { + let params = ToolCallRequestParams::ConversationSendMessage(ConversationSendMessageArgs { + conversation_id: ConversationId(Uuid::parse_str(session_id)?), + content: vec![InputItem::Text { + text: message.to_string(), + }], + parent_message_id: None, + conversation_overrides: None, + }); + self.send_request( + mcp_types::CallToolRequest::METHOD, + Some(serde_json::to_value(params)?), + ) + .await + } + async fn send_request( &mut self, method: &str, From e2c994e32a31415e87070bef28ed698968d2e549 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Thu, 31 Jul 2025 21:34:32 -0700 Subject: [PATCH 09/22] Add /compact (#1527) - Add operation to summarize the context so far. - The operation runs a compact task that summarizes the context. - The operation clear the previous context to free the context window - The operation didn't use `run_task` to avoid corrupting the session - Add /compact in the tui https://github.com/user-attachments/assets/e06c24e5-dcfb-4806-934a-564d425a919c --- SUMMARY.md | 21 ++ codex-rs/core/src/codex.rs | 154 ++++++++++++- codex-rs/core/src/conversation_history.rs | 28 +++ codex-rs/core/src/protocol.rs | 4 + codex-rs/core/tests/compact.rs | 254 ++++++++++++++++++++++ codex-rs/tui/src/app.rs | 8 + codex-rs/tui/src/chatwidget.rs | 6 + codex-rs/tui/src/slash_command.rs | 2 + 8 files changed, 474 insertions(+), 3 deletions(-) create mode 100644 SUMMARY.md create mode 100644 codex-rs/core/tests/compact.rs diff --git a/SUMMARY.md b/SUMMARY.md new file mode 100644 index 0000000000..5cdecdbf4c --- /dev/null +++ b/SUMMARY.md @@ -0,0 +1,21 @@ +You are a summarization assistant. A conversation follows between a user and a coding-focused AI (Codex). Your task is to generate a clear summary capturing: + +• High-level objective or problem being solved +• Key instructions or design decisions given by the user +• Main code actions or behaviors from the AI +• Important variables, functions, modules, or outputs discussed +• Any unresolved questions or next steps + +Produce the summary in a structured format like: + +**Objective:** … + +**User instructions:** … (bulleted) + +**AI actions / code behavior:** … (bulleted) + +**Important entities:** … (e.g. function names, variables, files) + +**Open issues / next steps:** … (if any) + +**Summary (concise):** (one or two sentences) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3dd1d513a2..5d48cd457f 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -442,6 +442,12 @@ impl Session { let _ = self.tx_event.send(event).await; } + /// Build the full turn input by concatenating the current conversation + /// history with additional items for this turn. + pub fn turn_input_with_history(&self, extra: Vec) -> Vec { + [self.state.lock().unwrap().history.contents(), extra].concat() + } + /// Returns the input if there was no task running to inject into pub fn inject_input(&self, input: Vec) -> Result<(), Vec> { let mut state = self.state.lock().unwrap(); @@ -564,6 +570,25 @@ impl AgentTask { handle, } } + fn compact( + sess: Arc, + sub_id: String, + input: Vec, + compact_instructions: String, + ) -> Self { + let handle = tokio::spawn(run_compact_task( + Arc::clone(&sess), + sub_id.clone(), + input, + compact_instructions, + )) + .abort_handle(); + Self { + sess, + sub_id, + handle, + } + } fn abort(self) { if !self.handle.is_finished() { @@ -884,6 +909,31 @@ async fn submission_loop( } }); } + Op::Compact => { + let sess = match sess.as_ref() { + Some(sess) => sess, + None => { + send_no_session_event(sub.id).await; + continue; + } + }; + + // Create a summarization request as user input + const SUMMARIZATION_PROMPT: &str = include_str!("../../../SUMMARY.md"); + + // Attempt to inject input into current task + if let Err(items) = sess.inject_input(vec![InputItem::Text { + text: "Start Summarization".to_string(), + }]) { + let task = AgentTask::compact( + sess.clone(), + sub.id, + items, + SUMMARIZATION_PROMPT.to_string(), + ); + sess.set_task(task); + } + } Op::Shutdown => { info!("Shutting down Codex instance"); @@ -945,7 +995,7 @@ async fn run_task(sess: Arc, sub_id: String, input: Vec) { return; } - let initial_input_for_turn = ResponseInputItem::from(input); + let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); sess.record_conversation_items(&[initial_input_for_turn.clone().into()]) .await; @@ -966,8 +1016,7 @@ async fn run_task(sess: Arc, sub_id: String, input: Vec) { // conversation history on each turn. The rollout file, however, should // only record the new items that originated in this turn so that it // represents an append-only log without duplicates. - let turn_input: Vec = - [sess.state.lock().unwrap().history.contents(), pending_input].concat(); + let turn_input: Vec = sess.turn_input_with_history(pending_input); let turn_input_messages: Vec = turn_input .iter() @@ -1293,6 +1342,88 @@ async fn try_run_turn( } } +async fn run_compact_task( + sess: Arc, + sub_id: String, + input: Vec, + compact_instructions: String, +) { + let start_event = Event { + id: sub_id.clone(), + msg: EventMsg::TaskStarted, + }; + if sess.tx_event.send(start_event).await.is_err() { + return; + } + + let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input); + let turn_input: Vec = + sess.turn_input_with_history(vec![initial_input_for_turn.clone().into()]); + + let prompt = Prompt { + input: turn_input, + user_instructions: None, + store: !sess.disable_response_storage, + extra_tools: HashMap::new(), + base_instructions_override: Some(compact_instructions.clone()), + }; + + let max_retries = sess.client.get_provider().stream_max_retries(); + let mut retries = 0; + + loop { + let attempt_result = drain_to_completed(&sess, &prompt).await; + + match attempt_result { + Ok(()) => break, + Err(CodexErr::Interrupted) => return, + Err(e) => { + if retries < max_retries { + retries += 1; + let delay = backoff(retries); + sess.notify_background_event( + &sub_id, + format!( + "stream error: {e}; retrying {retries}/{max_retries} in {delay:?}…" + ), + ) + .await; + tokio::time::sleep(delay).await; + continue; + } else { + let event = Event { + id: sub_id.clone(), + msg: EventMsg::Error(ErrorEvent { + message: e.to_string(), + }), + }; + sess.send_event(event).await; + return; + } + } + } + } + + sess.remove_task(&sub_id); + let event = Event { + id: sub_id.clone(), + msg: EventMsg::AgentMessage(AgentMessageEvent { + message: "Compact task completed".to_string(), + }), + }; + sess.send_event(event).await; + let event = Event { + id: sub_id.clone(), + msg: EventMsg::TaskComplete(TaskCompleteEvent { + last_agent_message: None, + }), + }; + sess.send_event(event).await; + + let mut state = sess.state.lock().unwrap(); + state.history.keep_last_messages(1); +} + async fn handle_response_item( sess: &Session, sub_id: &str, @@ -1858,3 +1989,20 @@ fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) -> Option CodexResult<()> { + let mut stream = sess.client.clone().stream(prompt).await?; + loop { + let maybe_event = stream.next().await; + let Some(event) = maybe_event else { + return Err(CodexErr::Stream( + "stream closed before response.completed".into(), + )); + }; + match event { + Ok(ResponseEvent::Completed { .. }) => return Ok(()), + Ok(_) => continue, + Err(e) => return Err(e), + } + } +} diff --git a/codex-rs/core/src/conversation_history.rs b/codex-rs/core/src/conversation_history.rs index 4cd989cbd9..f5254f339e 100644 --- a/codex-rs/core/src/conversation_history.rs +++ b/codex-rs/core/src/conversation_history.rs @@ -30,6 +30,34 @@ impl ConversationHistory { } } } + + pub(crate) fn keep_last_messages(&mut self, n: usize) { + if n == 0 { + self.items.clear(); + return; + } + + // Collect the last N message items (assistant/user), newest to oldest. + let mut kept: Vec = Vec::with_capacity(n); + for item in self.items.iter().rev() { + if let ResponseItem::Message { role, content, .. } = item { + kept.push(ResponseItem::Message { + // we need to remove the id or the model will complain that messages are sent without + // their reasonings + id: None, + role: role.clone(), + content: content.clone(), + }); + if kept.len() == n { + break; + } + } + } + + // Preserve chronological order (oldest to newest) within the kept slice. + kept.reverse(); + self.items = kept; + } } /// Anything that is not a system message or "reasoning" message is considered diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index bc922eb0e2..1b4832a273 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -121,6 +121,10 @@ pub enum Op { /// Request a single history entry identified by `log_id` + `offset`. GetHistoryEntryRequest { offset: usize, log_id: u64 }, + /// Request the agent to summarize the current conversation context. + /// The agent will use its existing context (either conversation history or previous response id) + /// to generate a summary which will be returned as an AgentMessage event. + Compact, /// Request to shut down codex instance. Shutdown, } diff --git a/codex-rs/core/tests/compact.rs b/codex-rs/core/tests/compact.rs new file mode 100644 index 0000000000..8967c86a8f --- /dev/null +++ b/codex-rs/core/tests/compact.rs @@ -0,0 +1,254 @@ +#![expect(clippy::unwrap_used)] + +use codex_core::Codex; +use codex_core::CodexSpawnOk; +use codex_core::ModelProviderInfo; +use codex_core::built_in_model_providers; +use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; +use codex_core::protocol::EventMsg; +use codex_core::protocol::InputItem; +use codex_core::protocol::Op; +use codex_login::CodexAuth; +use core_test_support::load_default_config_for_test; +use core_test_support::wait_for_event; +use serde_json::Value; +use tempfile::TempDir; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; +use wiremock::matchers::path; + +use pretty_assertions::assert_eq; + +// --- Test helpers ----------------------------------------------------------- + +/// Build an SSE stream body from a list of JSON events. +fn sse(events: Vec) -> String { + use std::fmt::Write as _; + let mut out = String::new(); + for ev in events { + let kind = ev.get("type").and_then(|v| v.as_str()).unwrap(); + writeln!(&mut out, "event: {kind}").unwrap(); + if !ev.as_object().map(|o| o.len() == 1).unwrap_or(false) { + write!(&mut out, "data: {ev}\n\n").unwrap(); + } else { + out.push('\n'); + } + } + out +} + +/// Convenience: SSE event for a completed response with a specific id. +fn ev_completed(id: &str) -> Value { + serde_json::json!({ + "type": "response.completed", + "response": { + "id": id, + "usage": {"input_tokens":0,"input_tokens_details":null,"output_tokens":0,"output_tokens_details":null,"total_tokens":0} + } + }) +} + +/// Convenience: SSE event for a single assistant message output item. +fn ev_assistant_message(id: &str, text: &str) -> Value { + serde_json::json!({ + "type": "response.output_item.done", + "item": { + "type": "message", + "role": "assistant", + "id": id, + "content": [{"type": "output_text", "text": text}] + } + }) +} + +fn sse_response(body: String) -> ResponseTemplate { + ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(body, "text/event-stream") +} + +async fn mount_sse_once(server: &MockServer, matcher: M, body: String) +where + M: wiremock::Match + Send + Sync + 'static, +{ + Mock::given(method("POST")) + .and(path("/v1/responses")) + .and(matcher) + .respond_with(sse_response(body)) + .expect(1) + .mount(server) + .await; +} + +const FIRST_REPLY: &str = "FIRST_REPLY"; +const SUMMARY_TEXT: &str = "SUMMARY_ONLY_CONTEXT"; +const SUMMARIZE_TRIGGER: &str = "Start Summarization"; +const THIRD_USER_MSG: &str = "next turn"; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn summarize_context_three_requests_and_instructions() { + if std::env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { + println!( + "Skipping test because it cannot execute when network is disabled in a Codex sandbox." + ); + return; + } + + // Set up a mock server that we can inspect after the run. + let server = MockServer::start().await; + + // SSE 1: assistant replies normally so it is recorded in history. + let sse1 = sse(vec![ + ev_assistant_message("m1", FIRST_REPLY), + ev_completed("r1"), + ]); + + // SSE 2: summarizer returns a summary message. + let sse2 = sse(vec![ + ev_assistant_message("m2", SUMMARY_TEXT), + ev_completed("r2"), + ]); + + // SSE 3: minimal completed; we only need to capture the request body. + let sse3 = sse(vec![ev_completed("r3")]); + + // Mount three expectations, one per request, matched by body content. + let first_matcher = |req: &wiremock::Request| { + let body = std::str::from_utf8(&req.body).unwrap_or(""); + body.contains("\"text\":\"hello world\"") + && !body.contains(&format!("\"text\":\"{SUMMARIZE_TRIGGER}\"")) + }; + mount_sse_once(&server, first_matcher, sse1).await; + + let second_matcher = |req: &wiremock::Request| { + let body = std::str::from_utf8(&req.body).unwrap_or(""); + body.contains(&format!("\"text\":\"{SUMMARIZE_TRIGGER}\"")) + }; + mount_sse_once(&server, second_matcher, sse2).await; + + let third_matcher = |req: &wiremock::Request| { + let body = std::str::from_utf8(&req.body).unwrap_or(""); + body.contains(&format!("\"text\":\"{THIRD_USER_MSG}\"")) + }; + mount_sse_once(&server, third_matcher, sse3).await; + + // Build config pointing to the mock server and spawn Codex. + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + let home = TempDir::new().unwrap(); + let mut config = load_default_config_for_test(&home); + config.model_provider = model_provider; + let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); + let CodexSpawnOk { codex, .. } = Codex::spawn( + config, + Some(CodexAuth::from_api_key("dummy".to_string())), + ctrl_c.clone(), + ) + .await + .unwrap(); + + // 1) Normal user input – should hit server once. + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: "hello world".into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + // 2) Summarize – second hit with summarization instructions. + codex.submit(Op::Compact).await.unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + // 3) Next user input – third hit; history should include only the summary. + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: THIRD_USER_MSG.into(), + }], + }) + .await + .unwrap(); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + // Inspect the three captured requests. + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 3, "expected exactly three requests"); + + let req1 = &requests[0]; + let req2 = &requests[1]; + let req3 = &requests[2]; + + let body1 = req1.body_json::().unwrap(); + let body2 = req2.body_json::().unwrap(); + let body3 = req3.body_json::().unwrap(); + + // System instructions should change for the summarization turn. + let instr1 = body1.get("instructions").and_then(|v| v.as_str()).unwrap(); + let instr2 = body2.get("instructions").and_then(|v| v.as_str()).unwrap(); + assert_ne!( + instr1, instr2, + "summarization should override base instructions" + ); + assert!( + instr2.contains("You are a summarization assistant"), + "summarization instructions not applied" + ); + + // The summarization request should include the injected user input marker. + let input2 = body2.get("input").and_then(|v| v.as_array()).unwrap(); + // The last item is the user message created from the injected input. + let last2 = input2.last().unwrap(); + assert_eq!(last2.get("type").unwrap().as_str().unwrap(), "message"); + assert_eq!(last2.get("role").unwrap().as_str().unwrap(), "user"); + let text2 = last2["content"][0]["text"].as_str().unwrap(); + assert!(text2.contains(SUMMARIZE_TRIGGER)); + + // Third request must contain only the summary from step 2 as prior history plus new user msg. + let input3 = body3.get("input").and_then(|v| v.as_array()).unwrap(); + println!("third request body: {body3}"); + assert!( + input3.len() >= 2, + "expected summary + new user message in third request" + ); + + // Collect all (role, text) message tuples. + let mut messages: Vec<(String, String)> = Vec::new(); + for item in input3 { + if item["type"].as_str() == Some("message") { + let role = item["role"].as_str().unwrap_or_default().to_string(); + let text = item["content"][0]["text"] + .as_str() + .unwrap_or_default() + .to_string(); + messages.push((role, text)); + } + } + + // Exactly one assistant message should remain after compaction and the new user message is present. + let assistant_count = messages.iter().filter(|(r, _)| r == "assistant").count(); + assert_eq!( + assistant_count, 1, + "exactly one assistant message should remain after compaction" + ); + assert!( + messages + .iter() + .any(|(r, t)| r == "user" && t == THIRD_USER_MSG), + "third request should include the new user message" + ); + assert!( + !messages.iter().any(|(_, t)| t.contains("hello world")), + "third request should not include the original user input" + ); + assert!( + !messages.iter().any(|(_, t)| t.contains(SUMMARIZE_TRIGGER)), + "third request should not include the summarize trigger" + ); +} diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index cc3118e860..d4c2f3c12d 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -10,6 +10,8 @@ use crate::tui; use codex_core::config::Config; use codex_core::protocol::Event; use codex_core::protocol::EventMsg; +use codex_core::protocol::ExecApprovalRequestEvent; +use codex_core::protocol::Op; use color_eyre::eyre::Result; use crossterm::SynchronizedUpdate; use crossterm::event::KeyCode; @@ -298,6 +300,12 @@ impl App<'_> { self.app_state = AppState::Chat { widget: new_widget }; self.app_event_tx.send(AppEvent::RequestRedraw); } + SlashCommand::Compact => { + if let AppState::Chat { widget } = &mut self.app_state { + widget.clear_token_usage(); + self.app_event_tx.send(AppEvent::CodexOp(Op::Compact)); + } + } SlashCommand::Quit => { break; } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 4c415777a9..c777af5264 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -502,6 +502,12 @@ impl ChatWidget<'_> { pub(crate) fn token_usage(&self) -> &TokenUsage { &self.token_usage } + + pub(crate) fn clear_token_usage(&mut self) { + self.token_usage = TokenUsage::default(); + self.bottom_pane + .set_token_usage(self.token_usage.clone(), self.config.model_context_window); + } } impl WidgetRef for &ChatWidget<'_> { diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index 7df1bcbdec..d82a16608f 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -13,6 +13,7 @@ pub enum SlashCommand { // DO NOT ALPHA-SORT! Enum order is presentation order in the popup, so // more frequently used commands should be listed first. New, + Compact, Diff, Quit, #[cfg(debug_assertions)] @@ -24,6 +25,7 @@ impl SlashCommand { pub fn description(self) -> &'static str { match self { SlashCommand::New => "Start a new chat.", + SlashCommand::Compact => "Compact the chat history.", SlashCommand::Quit => "Exit the application.", SlashCommand::Diff => { "Show git diff of the working directory (including untracked files)" From b67c485d84eba0d087203a850762da0cfe3361f9 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Fri, 1 Aug 2025 09:17:13 -0700 Subject: [PATCH 10/22] ci fix (#1782) --- codex-rs/core/tests/compact.rs | 2 +- codex-rs/tui/src/app.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/codex-rs/core/tests/compact.rs b/codex-rs/core/tests/compact.rs index 8967c86a8f..bf47feed1f 100644 --- a/codex-rs/core/tests/compact.rs +++ b/codex-rs/core/tests/compact.rs @@ -4,10 +4,10 @@ use codex_core::Codex; use codex_core::CodexSpawnOk; use codex_core::ModelProviderInfo; use codex_core::built_in_model_providers; -use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_core::protocol::EventMsg; use codex_core::protocol::InputItem; use codex_core::protocol::Op; +use codex_core::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_login::CodexAuth; use core_test_support::load_default_config_for_test; use core_test_support::wait_for_event; diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index d4c2f3c12d..efaec7f19a 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -10,7 +10,6 @@ use crate::tui; use codex_core::config::Config; use codex_core::protocol::Event; use codex_core::protocol::EventMsg; -use codex_core::protocol::ExecApprovalRequestEvent; use codex_core::protocol::Op; use color_eyre::eyre::Result; use crossterm::SynchronizedUpdate; From 88ea215c80e4fd33dc802b31cd20f8894c60dd6b Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 1 Aug 2025 09:55:23 -0700 Subject: [PATCH 11/22] Add a custom originator setting (#1781) --- codex-rs/core/src/client.rs | 9 +++- codex-rs/core/src/config.rs | 10 +++++ codex-rs/core/src/model_provider_info.rs | 16 ++----- codex-rs/core/tests/client.rs | 57 +++++++++++++++++++++++- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 0f7f51c2fc..b9ea6b13f4 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -207,7 +207,14 @@ impl ModelClient { } } - let req_builder = self.provider.apply_http_headers(req_builder); + req_builder = self.provider.apply_http_headers(req_builder); + + let originator = self + .config + .internal_originator + .as_deref() + .unwrap_or("codex_cli_rs"); + req_builder = req_builder.header("originator", originator); let res = req_builder.send().await; if let Ok(resp) = &res { diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index a65ec09674..1b8da89419 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -146,6 +146,9 @@ pub struct Config { /// Include an experimental plan tool that the model can use to update its current plan and status of each step. pub include_plan_tool: bool, + + /// The value for the `originator` header included with Responses API requests. + pub internal_originator: Option, } impl Config { @@ -336,6 +339,9 @@ pub struct ConfigToml { /// Experimental path to a file whose contents replace the built-in BASE_INSTRUCTIONS. pub experimental_instructions_file: Option, + + /// The value for the `originator` header included with Responses API requests. + pub internal_originator: Option, } impl ConfigToml { @@ -529,6 +535,7 @@ impl Config { experimental_resume, include_plan_tool: include_plan_tool.unwrap_or(false), + internal_originator: cfg.internal_originator, }; Ok(config) } @@ -887,6 +894,7 @@ disable_response_storage = true experimental_resume: None, base_instructions: None, include_plan_tool: false, + internal_originator: None, }, o3_profile_config ); @@ -936,6 +944,7 @@ disable_response_storage = true experimental_resume: None, base_instructions: None, include_plan_tool: false, + internal_originator: None, }; assert_eq!(expected_gpt3_profile_config, gpt3_profile_config); @@ -1000,6 +1009,7 @@ disable_response_storage = true experimental_resume: None, base_instructions: None, include_plan_tool: false, + internal_originator: None, }; assert_eq!(expected_zdr_profile_config, zdr_profile_config); diff --git a/codex-rs/core/src/model_provider_info.rs b/codex-rs/core/src/model_provider_info.rs index 4640f53ad7..2936637779 100644 --- a/codex-rs/core/src/model_provider_info.rs +++ b/codex-rs/core/src/model_provider_info.rs @@ -12,10 +12,6 @@ use std::env::VarError; use std::time::Duration; use crate::error::EnvVarError; - -/// Value for the `OpenAI-Originator` header that is sent with requests to -/// OpenAI. -const OPENAI_ORIGINATOR_HEADER: &str = "codex_cli_rs"; const DEFAULT_STREAM_IDLE_TIMEOUT_MS: u64 = 300_000; const DEFAULT_STREAM_MAX_RETRIES: u64 = 10; const DEFAULT_REQUEST_MAX_RETRIES: u64 = 4; @@ -229,15 +225,9 @@ pub fn built_in_model_providers() -> HashMap { wire_api: WireApi::Responses, query_params: None, http_headers: Some( - [ - ( - "originator".to_string(), - OPENAI_ORIGINATOR_HEADER.to_string(), - ), - ("version".to_string(), env!("CARGO_PKG_VERSION").to_string()), - ] - .into_iter() - .collect(), + [("version".to_string(), env!("CARGO_PKG_VERSION").to_string())] + .into_iter() + .collect(), ), env_http_headers: Some( [ diff --git a/codex-rs/core/tests/client.rs b/codex-rs/core/tests/client.rs index bfa29657d5..a22a94388b 100644 --- a/codex-rs/core/tests/client.rs +++ b/codex-rs/core/tests/client.rs @@ -95,8 +95,8 @@ async fn includes_session_id_and_model_headers_in_request() { // get request from the server let request = &server.received_requests().await.unwrap()[0]; let request_session_id = request.headers.get("session_id").unwrap(); - let request_originator = request.headers.get("originator").unwrap(); let request_authorization = request.headers.get("authorization").unwrap(); + let request_originator = request.headers.get("originator").unwrap(); assert!(current_session_id.is_some()); assert_eq!( @@ -170,6 +170,59 @@ async fn includes_base_instructions_override_in_request() { ); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn originator_config_override_is_used() { + #![allow(clippy::unwrap_used)] + + // Mock server + let server = MockServer::start().await; + + let first = ResponseTemplate::new(200) + .insert_header("content-type", "text/event-stream") + .set_body_raw(sse_completed("resp1"), "text/event-stream"); + + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with(first) + .expect(1) + .mount(&server) + .await; + + let model_provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + + let codex_home = TempDir::new().unwrap(); + let mut config = load_default_config_for_test(&codex_home); + config.model_provider = model_provider; + config.internal_originator = Some("my_override".to_string()); + + let ctrl_c = std::sync::Arc::new(tokio::sync::Notify::new()); + let CodexSpawnOk { codex, .. } = Codex::spawn( + config, + Some(CodexAuth::from_api_key("Test API Key".to_string())), + ctrl_c.clone(), + ) + .await + .unwrap(); + + codex + .submit(Op::UserInput { + items: vec![InputItem::Text { + text: "hello".into(), + }], + }) + .await + .unwrap(); + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let request = &server.received_requests().await.unwrap()[0]; + let request_originator = request.headers.get("originator").unwrap(); + assert_eq!(request_originator.to_str().unwrap(), "my_override"); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn chatgpt_auth_sends_correct_request() { #![allow(clippy::unwrap_used)] @@ -235,8 +288,8 @@ async fn chatgpt_auth_sends_correct_request() { // get request from the server let request = &server.received_requests().await.unwrap()[0]; let request_session_id = request.headers.get("session_id").unwrap(); - let request_originator = request.headers.get("originator").unwrap(); let request_authorization = request.headers.get("authorization").unwrap(); + let request_originator = request.headers.get("originator").unwrap(); let request_chatgpt_account_id = request.headers.get("chatgpt-account-id").unwrap(); let request_body = request.body_json::().unwrap(); From f918198bbbfa95e20c629eed758cbd53ad38ab97 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Fri, 1 Aug 2025 10:04:12 -0700 Subject: [PATCH 12/22] Introduce a new function to just send user message [Stack 3/3] (#1686) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - MCP server: add send-user-message tool to send user input to a running Codex session - Added an integration tests for the happy and sad paths Changes: • Add tool definition and schema. • Expose tool in capabilities. • Route and handle tool requests with validation. • Tests for success, bad UUID, and missing session. follow‑ups • Listen path not implemented yet; the tool is present but marked “don’t use yet” in code comments. • Session run flag reset: clear running_session_id_set appropriately after turn completion/errors. This is the third PR in a stack. Stack: Final: #1686 Intermediate: #1751 First: #1750 --- codex-rs/mcp-server/src/lib.rs | 3 +- codex-rs/mcp-server/src/mcp_protocol.rs | 36 ++-- codex-rs/mcp-server/src/message_processor.rs | 63 +++++-- codex-rs/mcp-server/src/tool_handlers/mod.rs | 1 + .../src/tool_handlers/send_message.rs | 124 +++++++++++++ codex-rs/mcp-server/tests/send_message.rs | 163 ++++++++++++++++++ 6 files changed, 358 insertions(+), 32 deletions(-) create mode 100644 codex-rs/mcp-server/src/tool_handlers/mod.rs create mode 100644 codex-rs/mcp-server/src/tool_handlers/send_message.rs create mode 100644 codex-rs/mcp-server/tests/send_message.rs diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index ebef8ca903..6b3c0ddbe6 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -20,9 +20,10 @@ mod codex_tool_runner; mod exec_approval; mod json_to_toml; pub mod mcp_protocol; -mod message_processor; +pub(crate) mod message_processor; mod outgoing_message; mod patch_approval; +pub(crate) mod tool_handlers; use crate::message_processor::MessageProcessor; use crate::outgoing_message::OutgoingMessage; diff --git a/codex-rs/mcp-server/src/mcp_protocol.rs b/codex-rs/mcp-server/src/mcp_protocol.rs index 23304dc43d..287890bf5e 100644 --- a/codex-rs/mcp-server/src/mcp_protocol.rs +++ b/codex-rs/mcp-server/src/mcp_protocol.rs @@ -132,32 +132,32 @@ impl From for CallToolResult { is_error, result, } = val; - let (content, structured_content, is_error_out) = match result { + match result { Some(res) => match serde_json::to_value(&res) { - Ok(v) => { - let content = vec![ContentBlock::TextContent(TextContent { + Ok(v) => CallToolResult { + content: vec![ContentBlock::TextContent(TextContent { r#type: "text".to_string(), text: v.to_string(), annotations: None, - })]; - (content, Some(v), is_error) - } - Err(e) => { - let content = vec![ContentBlock::TextContent(TextContent { + })], + is_error, + structured_content: Some(v), + }, + Err(e) => CallToolResult { + content: vec![ContentBlock::TextContent(TextContent { r#type: "text".to_string(), text: format!("Failed to serialize tool result: {e}"), annotations: None, - })]; - (content, None, Some(true)) - } + })], + is_error: Some(true), + structured_content: None, + }, + }, + None => CallToolResult { + content: vec![], + is_error, + structured_content: None, }, - None => (vec![], None, is_error), - }; - - CallToolResult { - content, - is_error: is_error_out, - structured_content, } } } diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index a4013cc3a3..14e9bb3887 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; @@ -7,11 +8,15 @@ use crate::codex_tool_config::CodexToolCallReplyParam; use crate::codex_tool_config::create_tool_for_codex_tool_call_param; use crate::codex_tool_config::create_tool_for_codex_tool_call_reply_param; use crate::mcp_protocol::ToolCallRequestParams; +use crate::mcp_protocol::ToolCallResponse; +use crate::mcp_protocol::ToolCallResponseResult; use crate::outgoing_message::OutgoingMessageSender; +use crate::tool_handlers::send_message::handle_send_message; use codex_core::Codex; use codex_core::config::Config as CodexConfig; use codex_core::protocol::Submission; +use mcp_types::CallToolRequest; use mcp_types::CallToolRequestParams; use mcp_types::CallToolResult; use mcp_types::ClientRequest; @@ -38,6 +43,7 @@ pub(crate) struct MessageProcessor { codex_linux_sandbox_exe: Option, session_map: Arc>>>, running_requests_id_to_codex_uuid: Arc>>, + running_session_ids: Arc>>, } impl MessageProcessor { @@ -53,9 +59,18 @@ impl MessageProcessor { codex_linux_sandbox_exe, session_map: Arc::new(Mutex::new(HashMap::new())), running_requests_id_to_codex_uuid: Arc::new(Mutex::new(HashMap::new())), + running_session_ids: Arc::new(Mutex::new(HashSet::new())), } } + pub(crate) fn session_map(&self) -> Arc>>> { + self.session_map.clone() + } + + pub(crate) fn running_session_ids(&self) -> Arc>> { + self.running_session_ids.clone() + } + pub(crate) async fn process_request(&mut self, request: JSONRPCRequest) { // Hold on to the ID so we can respond. let request_id = request.id.clone(); @@ -332,19 +347,25 @@ impl MessageProcessor { } } } - async fn handle_new_tool_calls(&self, request_id: RequestId, _params: ToolCallRequestParams) { - // TODO: implement the new tool calls - let result = CallToolResult { - content: vec![ContentBlock::TextContent(TextContent { - r#type: "text".to_string(), - text: "Unknown tool".to_string(), - annotations: None, - })], - is_error: Some(true), - structured_content: None, - }; - self.send_response::(request_id, result) - .await; + async fn handle_new_tool_calls(&self, request_id: RequestId, params: ToolCallRequestParams) { + match params { + ToolCallRequestParams::ConversationSendMessage(args) => { + handle_send_message(self, request_id, args).await; + } + _ => { + let result = CallToolResult { + content: vec![ContentBlock::TextContent(TextContent { + r#type: "text".to_string(), + text: "Unknown tool".to_string(), + annotations: None, + })], + is_error: Some(true), + structured_content: None, + }; + self.send_response::(request_id, result) + .await; + } + } } async fn handle_tool_call_codex(&self, id: RequestId, arguments: Option) { @@ -654,4 +675,20 @@ impl MessageProcessor { ) { tracing::info!("notifications/message -> params: {:?}", params); } + + pub(crate) async fn send_response_with_optional_error( + &self, + id: RequestId, + message: Option, + error: Option, + ) { + let response = ToolCallResponse { + request_id: id.clone(), + is_error: error, + result: message, + }; + let result: CallToolResult = response.into(); + self.send_response::(id.clone(), result) + .await; + } } diff --git a/codex-rs/mcp-server/src/tool_handlers/mod.rs b/codex-rs/mcp-server/src/tool_handlers/mod.rs new file mode 100644 index 0000000000..1907ec64e0 --- /dev/null +++ b/codex-rs/mcp-server/src/tool_handlers/mod.rs @@ -0,0 +1 @@ +pub(crate) mod send_message; diff --git a/codex-rs/mcp-server/src/tool_handlers/send_message.rs b/codex-rs/mcp-server/src/tool_handlers/send_message.rs new file mode 100644 index 0000000000..894176bef6 --- /dev/null +++ b/codex-rs/mcp-server/src/tool_handlers/send_message.rs @@ -0,0 +1,124 @@ +use std::collections::HashMap; +use std::sync::Arc; + +use codex_core::Codex; +use codex_core::protocol::Op; +use codex_core::protocol::Submission; +use mcp_types::RequestId; +use tokio::sync::Mutex; +use uuid::Uuid; + +use crate::mcp_protocol::ConversationSendMessageArgs; +use crate::mcp_protocol::ConversationSendMessageResult; +use crate::mcp_protocol::ToolCallResponseResult; +use crate::message_processor::MessageProcessor; + +pub(crate) async fn handle_send_message( + message_processor: &MessageProcessor, + id: RequestId, + arguments: ConversationSendMessageArgs, +) { + let ConversationSendMessageArgs { + conversation_id, + content: items, + parent_message_id: _, + conversation_overrides: _, + } = arguments; + + if items.is_empty() { + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationSendMessage( + ConversationSendMessageResult::Error { + message: "No content items provided".to_string(), + }, + )), + Some(true), + ) + .await; + return; + } + + let session_id = conversation_id.0; + let Some(codex) = get_session(session_id, message_processor.session_map()).await else { + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationSendMessage( + ConversationSendMessageResult::Error { + message: "Session does not exist".to_string(), + }, + )), + Some(true), + ) + .await; + return; + }; + + let running = { + let running_sessions = message_processor.running_session_ids(); + let mut running_sessions = running_sessions.lock().await; + !running_sessions.insert(session_id) + }; + + if running { + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationSendMessage( + ConversationSendMessageResult::Error { + message: "Session is already running".to_string(), + }, + )), + Some(true), + ) + .await; + return; + } + + let request_id_string = match &id { + RequestId::String(s) => s.clone(), + RequestId::Integer(i) => i.to_string(), + }; + + let submit_res = codex + .submit_with_id(Submission { + id: request_id_string, + op: Op::UserInput { items }, + }) + .await; + + if let Err(e) = submit_res { + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationSendMessage( + ConversationSendMessageResult::Error { + message: format!("Failed to submit user input: {e}"), + }, + )), + Some(true), + ) + .await; + return; + } + + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationSendMessage( + ConversationSendMessageResult::Ok, + )), + Some(false), + ) + .await; +} + +pub(crate) async fn get_session( + session_id: Uuid, + session_map: Arc>>>, +) -> Option> { + let guard = session_map.lock().await; + guard.get(&session_id).cloned() +} diff --git a/codex-rs/mcp-server/tests/send_message.rs b/codex-rs/mcp-server/tests/send_message.rs new file mode 100644 index 0000000000..fd4b210b0b --- /dev/null +++ b/codex-rs/mcp-server/tests/send_message.rs @@ -0,0 +1,163 @@ +#![allow(clippy::expect_used)] + +use std::path::Path; +use std::thread::sleep; +use std::time::Duration; + +use codex_mcp_server::CodexToolCallParam; +use mcp_test_support::McpProcess; +use mcp_test_support::create_final_assistant_message_sse_response; +use mcp_test_support::create_mock_chat_completions_server; +use mcp_types::JSONRPC_VERSION; +use mcp_types::JSONRPCResponse; +use mcp_types::RequestId; +use pretty_assertions::assert_eq; +use serde_json::json; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_send_message_success() { + // Spin up a mock completions server that immediately ends the Codex turn. + // Two Codex turns hit the mock model (session start + send-user-message). Provide two SSE responses. + let responses = vec![ + create_final_assistant_message_sse_response("Done").expect("build mock assistant message"), + create_final_assistant_message_sse_response("Done").expect("build mock assistant message"), + ]; + let server = create_mock_chat_completions_server(responses).await; + + // Create a temporary Codex home with config pointing at the mock server. + let codex_home = TempDir::new().expect("create temp dir"); + create_config_toml(codex_home.path(), &server.uri()).expect("write config.toml"); + + // Start MCP server process and initialize. + let mut mcp_process = McpProcess::new(codex_home.path()) + .await + .expect("spawn mcp process"); + timeout(DEFAULT_READ_TIMEOUT, mcp_process.initialize()) + .await + .expect("init timed out") + .expect("init failed"); + + // Kick off a Codex session so we have a valid session_id. + let codex_request_id = mcp_process + .send_codex_tool_call(CodexToolCallParam { + prompt: "Start a session".to_string(), + ..Default::default() + }) + .await + .expect("send codex tool call"); + + // Wait for the session_configured event to get the session_id. + let session_id = mcp_process + .read_stream_until_configured_response_message() + .await + .expect("read session_configured"); + + // The original codex call will finish quickly given our mock; consume its response. + timeout( + DEFAULT_READ_TIMEOUT, + mcp_process.read_stream_until_response_message(RequestId::Integer(codex_request_id)), + ) + .await + .expect("codex response timeout") + .expect("codex response error"); + + // Now exercise the send-user-message tool. + let send_msg_request_id = mcp_process + .send_user_message_tool_call("Hello again", &session_id) + .await + .expect("send send-message tool call"); + + let response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp_process.read_stream_until_response_message(RequestId::Integer(send_msg_request_id)), + ) + .await + .expect("send-user-message response timeout") + .expect("send-user-message response error"); + + assert_eq!( + JSONRPCResponse { + jsonrpc: JSONRPC_VERSION.into(), + id: RequestId::Integer(send_msg_request_id), + result: json!({ + "content": [ + { + "text": "{\"status\":\"ok\"}", + "type": "text", + } + ], + "isError": false, + "structuredContent": { + "status": "ok" + } + }), + }, + response + ); + // wait for the server to hear the user message + sleep(Duration::from_secs(1)); + + // Ensure the server and tempdir live until end of test + drop(server); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_send_message_session_not_found() { + // Start MCP without creating a Codex session + let codex_home = TempDir::new().expect("tempdir"); + let mut mcp = McpProcess::new(codex_home.path()).await.expect("spawn"); + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()) + .await + .expect("timeout") + .expect("init"); + + let unknown = uuid::Uuid::new_v4().to_string(); + let req_id = mcp + .send_user_message_tool_call("ping", &unknown) + .await + .expect("send tool"); + + let resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(req_id)), + ) + .await + .expect("timeout") + .expect("resp"); + + let result = resp.result.clone(); + let content = result["content"][0]["text"].as_str().unwrap_or(""); + assert!(content.contains("Session does not exist")); + assert_eq!(result["isError"], json!(true)); +} + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write( + config_toml, + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "danger-full-access" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "chat" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} From 8360c6a3ec83d0aa5d208b7a7eeb619c4ce27cad Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 1 Aug 2025 10:37:43 -0700 Subject: [PATCH 13/22] fix insert_history modifier handling (#1774) This fixes a bug in insert_history_lines where writing `Line::From(vec!["A".bold(), "B".into()])` would write "B" as bold, because "B" didn't explicitly subtract bold. --- codex-rs/tui/src/history_cell.rs | 27 ++++++---------- codex-rs/tui/src/insert_history.rs | 50 +++++++++++++++++++++++++----- 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 0e9f25332d..c2aafdd522 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -545,24 +545,17 @@ impl HistoryCell { } else { for (idx, PlanItemArg { step, status }) in plan.into_iter().enumerate() { let num = idx + 1; - let (icon, style): (&str, Style) = match status { - StepStatus::Completed => ("✓", Style::default().fg(Color::Green)), - StepStatus::InProgress => ( - "▶", - Style::default() - .fg(Color::Yellow) - .add_modifier(Modifier::BOLD), - ), - StepStatus::Pending => ("○", Style::default().fg(Color::Gray)), + let icon_span: Span = match status { + StepStatus::Completed => Span::from("✓").fg(Color::Green), + StepStatus::InProgress => Span::from("▶").fg(Color::Yellow).bold(), + StepStatus::Pending => Span::from("○").fg(Color::Gray), }; - let prefix = vec![ - Span::raw(format!("{num:>2}. [")), - Span::styled(icon.to_string(), style), - Span::raw("] "), - ]; - let mut spans = prefix; - spans.push(Span::raw(step)); - lines.push(Line::from(spans)); + lines.push(Line::from(vec![ + format!("{num:>2}. [").into(), + icon_span, + "] ".into(), + step.into(), + ])); } } diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index efd08a71c2..87d88b7f03 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -216,18 +216,18 @@ where { let mut fg = Color::Reset; let mut bg = Color::Reset; - let mut modifier = Modifier::empty(); + let mut last_modifier = Modifier::empty(); for span in content { - let mut next_modifier = modifier; - next_modifier.insert(span.style.add_modifier); - next_modifier.remove(span.style.sub_modifier); - if next_modifier != modifier { + let mut modifier = Modifier::empty(); + modifier.insert(span.style.add_modifier); + modifier.remove(span.style.sub_modifier); + if modifier != last_modifier { let diff = ModifierDiff { - from: modifier, - to: next_modifier, + from: last_modifier, + to: modifier, }; diff.queue(&mut writer)?; - modifier = next_modifier; + last_modifier = modifier; } let next_fg = span.style.fg.unwrap_or(Color::Reset); let next_bg = span.style.bg.unwrap_or(Color::Reset); @@ -250,3 +250,37 @@ where SetAttribute(crossterm::style::Attribute::Reset), ) } + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used)] + use super::*; + + #[test] + fn writes_bold_then_regular_spans() { + use ratatui::style::Stylize; + + let spans = ["A".bold(), "B".into()]; + + let mut actual: Vec = Vec::new(); + write_spans(&mut actual, spans.iter()).unwrap(); + + let mut expected: Vec = Vec::new(); + queue!( + expected, + SetAttribute(crossterm::style::Attribute::Bold), + Print("A"), + SetAttribute(crossterm::style::Attribute::NormalIntensity), + Print("B"), + SetForegroundColor(CColor::Reset), + SetBackgroundColor(CColor::Reset), + SetAttribute(crossterm::style::Attribute::Reset), + ) + .unwrap(); + + assert_eq!( + String::from_utf8(actual).unwrap(), + String::from_utf8(expected).unwrap() + ); + } +} From bc7beddaa2d9fab9cea376c4a6d8a26bfac3b349 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Fri, 1 Aug 2025 13:04:34 -0700 Subject: [PATCH 14/22] feat: stream exec stdout events (#1786) ## Summary - stream command stdout as `ExecCommandStdout` events - forward streamed stdout to clients and ignore in human output processor - adjust call sites for new streaming API --- codex-rs/Cargo.lock | 10 ++ codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/codex.rs | 11 ++ codex-rs/core/src/exec.rs | 53 ++++++- codex-rs/core/src/protocol.rs | 25 ++++ codex-rs/core/src/shell.rs | 1 + codex-rs/core/tests/exec_stream_events.rs | 135 ++++++++++++++++++ .../src/event_processor_with_human_output.rs | 2 + codex-rs/linux-sandbox/tests/landlock.rs | 2 + codex-rs/mcp-server/src/codex_tool_runner.rs | 2 + codex-rs/tui/src/chatwidget.rs | 1 + 11 files changed, 238 insertions(+), 5 deletions(-) create mode 100644 codex-rs/core/tests/exec_stream_events.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 9123bd6374..460a440e63 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -695,6 +695,7 @@ dependencies = [ "reqwest", "seccompiler", "serde", + "serde_bytes", "serde_json", "sha1", "shlex", @@ -3951,6 +3952,15 @@ dependencies = [ "serde_derive", ] +[[package]] +name = "serde_bytes" +version = "0.11.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8437fd221bde2d4ca316d61b90e337e9e702b3820b87d63caa9ba6c02bd06d96" +dependencies = [ + "serde", +] + [[package]] name = "serde_derive" version = "1.0.219" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index ecc904cd5e..db3fd4f834 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -31,6 +31,7 @@ rand = "0.9" reqwest = { version = "0.12", features = ["json", "stream"] } serde = { version = "1", features = ["derive"] } serde_json = "1" +serde_bytes = "0.11" sha1 = "0.10.6" shlex = "1.3.0" strum_macros = "0.27.2" diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5d48cd457f..6e21642da1 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -48,6 +48,7 @@ use crate::error::SandboxErr; use crate::exec::ExecParams; use crate::exec::ExecToolCallOutput; use crate::exec::SandboxType; +use crate::exec::StdoutStream; use crate::exec::process_exec_tool_call; use crate::exec_env::create_env; use crate::mcp_connection_manager::McpConnectionManager; @@ -1759,6 +1760,11 @@ async fn handle_container_exec_with_params( sess.ctrl_c.clone(), &sess.sandbox_policy, &sess.codex_linux_sandbox_exe, + Some(StdoutStream { + sub_id: sub_id.clone(), + call_id: call_id.clone(), + tx_event: sess.tx_event.clone(), + }), ) .await; @@ -1879,6 +1885,11 @@ async fn handle_sandbox_error( sess.ctrl_c.clone(), &sess.sandbox_policy, &sess.codex_linux_sandbox_exe, + Some(StdoutStream { + sub_id: sub_id.clone(), + call_id: call_id.clone(), + tx_event: sess.tx_event.clone(), + }), ) .await; diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 06416e6768..a5a88ee9c0 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -10,6 +10,7 @@ use std::sync::Arc; use std::time::Duration; use std::time::Instant; +use async_channel::Sender; use tokio::io::AsyncRead; use tokio::io::AsyncReadExt; use tokio::io::BufReader; @@ -19,10 +20,15 @@ use tokio::sync::Notify; use crate::error::CodexErr; use crate::error::Result; use crate::error::SandboxErr; +use crate::protocol::Event; +use crate::protocol::EventMsg; +use crate::protocol::ExecCommandStderrDeltaEvent; +use crate::protocol::ExecCommandStdoutDeltaEvent; use crate::protocol::SandboxPolicy; use crate::seatbelt::spawn_command_under_seatbelt; use crate::spawn::StdioPolicy; use crate::spawn::spawn_child_async; +use serde_bytes::ByteBuf; // Maximum we send for each stream, which is either: // - 10KiB OR @@ -56,18 +62,26 @@ pub enum SandboxType { LinuxSeccomp, } +#[derive(Clone)] +pub struct StdoutStream { + pub sub_id: String, + pub call_id: String, + pub tx_event: Sender, +} + pub async fn process_exec_tool_call( params: ExecParams, sandbox_type: SandboxType, ctrl_c: Arc, sandbox_policy: &SandboxPolicy, codex_linux_sandbox_exe: &Option, + stdout_stream: Option, ) -> Result { let start = Instant::now(); let raw_output_result: std::result::Result = match sandbox_type { - SandboxType::None => exec(params, sandbox_policy, ctrl_c).await, + SandboxType::None => exec(params, sandbox_policy, ctrl_c, stdout_stream.clone()).await, SandboxType::MacosSeatbelt => { let ExecParams { command, @@ -83,7 +97,7 @@ pub async fn process_exec_tool_call( env, ) .await?; - consume_truncated_output(child, ctrl_c, timeout_ms).await + consume_truncated_output(child, ctrl_c, timeout_ms, stdout_stream.clone()).await } SandboxType::LinuxSeccomp => { let ExecParams { @@ -106,7 +120,7 @@ pub async fn process_exec_tool_call( ) .await?; - consume_truncated_output(child, ctrl_c, timeout_ms).await + consume_truncated_output(child, ctrl_c, timeout_ms, stdout_stream).await } }; let duration = start.elapsed(); @@ -233,6 +247,7 @@ async fn exec( }: ExecParams, sandbox_policy: &SandboxPolicy, ctrl_c: Arc, + stdout_stream: Option, ) -> Result { let (program, args) = command.split_first().ok_or_else(|| { CodexErr::Io(io::Error::new( @@ -251,7 +266,7 @@ async fn exec( env, ) .await?; - consume_truncated_output(child, ctrl_c, timeout_ms).await + consume_truncated_output(child, ctrl_c, timeout_ms, stdout_stream).await } /// Consumes the output of a child process, truncating it so it is suitable for @@ -260,6 +275,7 @@ pub(crate) async fn consume_truncated_output( mut child: Child, ctrl_c: Arc, timeout_ms: Option, + stdout_stream: Option, ) -> Result { // Both stdout and stderr were configured with `Stdio::piped()` // above, therefore `take()` should normally return `Some`. If it doesn't @@ -280,11 +296,15 @@ pub(crate) async fn consume_truncated_output( BufReader::new(stdout_reader), MAX_STREAM_OUTPUT, MAX_STREAM_OUTPUT_LINES, + stdout_stream.clone(), + false, )); let stderr_handle = tokio::spawn(read_capped( BufReader::new(stderr_reader), MAX_STREAM_OUTPUT, MAX_STREAM_OUTPUT_LINES, + stdout_stream.clone(), + true, )); let interrupted = ctrl_c.notified(); @@ -318,10 +338,12 @@ pub(crate) async fn consume_truncated_output( }) } -async fn read_capped( +async fn read_capped( mut reader: R, max_output: usize, max_lines: usize, + stream: Option, + is_stderr: bool, ) -> io::Result> { let mut buf = Vec::with_capacity(max_output.min(8 * 1024)); let mut tmp = [0u8; 8192]; @@ -335,6 +357,27 @@ async fn read_capped( break; } + if let Some(stream) = &stream { + let chunk = tmp[..n].to_vec(); + let msg = if is_stderr { + EventMsg::ExecCommandStderrDelta(ExecCommandStderrDeltaEvent { + call_id: stream.call_id.clone(), + chunk: ByteBuf::from(chunk), + }) + } else { + EventMsg::ExecCommandStdoutDelta(ExecCommandStdoutDeltaEvent { + call_id: stream.call_id.clone(), + chunk: ByteBuf::from(chunk), + }) + }; + let event = Event { + id: stream.sub_id.clone(), + msg, + }; + #[allow(clippy::let_unit_value)] + let _ = stream.tx_event.send(event).await; + } + // Copy into the buffer only while we still have byte and line budget. if remaining_bytes > 0 && remaining_lines > 0 { let mut copy_len = 0; diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 1b4832a273..8d26eda98e 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -13,6 +13,7 @@ use std::time::Duration; use mcp_types::CallToolResult; use serde::Deserialize; use serde::Serialize; +use serde_bytes::ByteBuf; use strum_macros::Display; use uuid::Uuid; @@ -323,6 +324,12 @@ pub enum EventMsg { /// Notification that the server is about to execute a command. ExecCommandBegin(ExecCommandBeginEvent), + /// Incremental chunk of stdout from a running command. + ExecCommandStdoutDelta(ExecCommandStdoutDeltaEvent), + + /// Incremental chunk of stderr from a running command. + ExecCommandStderrDelta(ExecCommandStderrDeltaEvent), + ExecCommandEnd(ExecCommandEndEvent), ExecApprovalRequest(ExecApprovalRequestEvent), @@ -476,6 +483,24 @@ pub struct ExecCommandEndEvent { pub exit_code: i32, } +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct ExecCommandStdoutDeltaEvent { + /// Identifier for the ExecCommandBegin that produced this chunk. + pub call_id: String, + /// Raw stdout bytes (may not be valid UTF-8). + #[serde(with = "serde_bytes")] + pub chunk: ByteBuf, +} + +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct ExecCommandStderrDeltaEvent { + /// Identifier for the ExecCommandBegin that produced this chunk. + pub call_id: String, + /// Raw stderr bytes (may not be valid UTF-8). + #[serde(with = "serde_bytes")] + pub chunk: ByteBuf, +} + #[derive(Debug, Clone, Deserialize, Serialize)] pub struct ExecApprovalRequestEvent { /// Identifier for the associated exec call, if available. diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 98addffce2..1e895a3701 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -220,6 +220,7 @@ mod tests { Arc::new(Notify::new()), &SandboxPolicy::DangerFullAccess, &None, + None, ) .await .unwrap(); diff --git a/codex-rs/core/tests/exec_stream_events.rs b/codex-rs/core/tests/exec_stream_events.rs new file mode 100644 index 0000000000..059b69a064 --- /dev/null +++ b/codex-rs/core/tests/exec_stream_events.rs @@ -0,0 +1,135 @@ +#![cfg(unix)] + +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; + +use async_channel::Receiver; +use codex_core::exec::ExecParams; +use codex_core::exec::SandboxType; +use codex_core::exec::StdoutStream; +use codex_core::exec::process_exec_tool_call; +use codex_core::protocol::Event; +use codex_core::protocol::EventMsg; +use codex_core::protocol::ExecCommandStderrDeltaEvent; +use codex_core::protocol::ExecCommandStdoutDeltaEvent; +use codex_core::protocol::SandboxPolicy; +use tokio::sync::Notify; + +fn collect_stdout_events(rx: Receiver) -> Vec { + let mut out = Vec::new(); + while let Ok(ev) = rx.try_recv() { + if let EventMsg::ExecCommandStdoutDelta(ExecCommandStdoutDeltaEvent { chunk, .. }) = ev.msg + { + out.extend_from_slice(&chunk); + } + } + out +} + +#[tokio::test] +async fn test_exec_stdout_stream_events_echo() { + let (tx, rx) = async_channel::unbounded::(); + + let stdout_stream = StdoutStream { + sub_id: "test-sub".to_string(), + call_id: "call-1".to_string(), + tx_event: tx, + }; + + let cmd = vec![ + "/bin/sh".to_string(), + "-c".to_string(), + // Use printf for predictable behavior across shells + "printf 'hello-world\n'".to_string(), + ]; + + let params = ExecParams { + command: cmd, + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + timeout_ms: Some(5_000), + env: HashMap::new(), + }; + + let ctrl_c = Arc::new(Notify::new()); + let policy = SandboxPolicy::new_read_only_policy(); + + let result = process_exec_tool_call( + params, + SandboxType::None, + ctrl_c, + &policy, + &None, + Some(stdout_stream), + ) + .await; + + let result = match result { + Ok(r) => r, + Err(e) => panic!("process_exec_tool_call failed: {e}"), + }; + + assert_eq!(result.exit_code, 0); + assert_eq!(result.stdout, "hello-world\n"); + + let streamed = collect_stdout_events(rx); + // We should have received at least the same contents (possibly in one chunk) + assert_eq!(String::from_utf8_lossy(&streamed), "hello-world\n"); +} + +#[tokio::test] +async fn test_exec_stderr_stream_events_echo() { + let (tx, rx) = async_channel::unbounded::(); + + let stdout_stream = StdoutStream { + sub_id: "test-sub".to_string(), + call_id: "call-2".to_string(), + tx_event: tx, + }; + + let cmd = vec![ + "/bin/sh".to_string(), + "-c".to_string(), + // Write to stderr explicitly + "printf 'oops\n' 1>&2".to_string(), + ]; + + let params = ExecParams { + command: cmd, + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + timeout_ms: Some(5_000), + env: HashMap::new(), + }; + + let ctrl_c = Arc::new(Notify::new()); + let policy = SandboxPolicy::new_read_only_policy(); + + let result = process_exec_tool_call( + params, + SandboxType::None, + ctrl_c, + &policy, + &None, + Some(stdout_stream), + ) + .await; + + let result = match result { + Ok(r) => r, + Err(e) => panic!("process_exec_tool_call failed: {e}"), + }; + + assert_eq!(result.exit_code, 0); + assert_eq!(result.stdout, ""); + assert_eq!(result.stderr, "oops\n"); + + // Collect only stderr delta events + let mut err = Vec::new(); + while let Ok(ev) = rx.try_recv() { + if let EventMsg::ExecCommandStderrDelta(ExecCommandStderrDeltaEvent { chunk, .. }) = ev.msg + { + err.extend_from_slice(&chunk); + } + } + assert_eq!(String::from_utf8_lossy(&err), "oops\n"); +} diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 54604d538b..05643c6282 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -239,6 +239,8 @@ impl EventProcessor for EventProcessorWithHumanOutput { cwd.to_string_lossy(), ); } + EventMsg::ExecCommandStdoutDelta(_) => {} + EventMsg::ExecCommandStderrDelta(_) => {} EventMsg::ExecCommandEnd(ExecCommandEndEvent { call_id, stdout, diff --git a/codex-rs/linux-sandbox/tests/landlock.rs b/codex-rs/linux-sandbox/tests/landlock.rs index 7eacda46c1..c16e9227df 100644 --- a/codex-rs/linux-sandbox/tests/landlock.rs +++ b/codex-rs/linux-sandbox/tests/landlock.rs @@ -59,6 +59,7 @@ async fn run_cmd(cmd: &[&str], writable_roots: &[PathBuf], timeout_ms: u64) { ctrl_c, &sandbox_policy, &codex_linux_sandbox_exe, + None, ) .await .unwrap(); @@ -149,6 +150,7 @@ async fn assert_network_blocked(cmd: &[&str]) { ctrl_c, &sandbox_policy, &codex_linux_sandbox_exe, + None, ) .await; diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index f25659b284..94d47f7497 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -258,6 +258,8 @@ async fn run_codex_tool_session_inner( | EventMsg::McpToolCallBegin(_) | EventMsg::McpToolCallEnd(_) | EventMsg::ExecCommandBegin(_) + | EventMsg::ExecCommandStdoutDelta(_) + | EventMsg::ExecCommandStderrDelta(_) | EventMsg::ExecCommandEnd(_) | EventMsg::BackgroundEvent(_) | EventMsg::PatchApplyBegin(_) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c777af5264..d6454c82a7 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -374,6 +374,7 @@ impl ChatWidget<'_> { ); self.add_to_history(HistoryCell::new_active_exec_command(command)); } + EventMsg::ExecCommandStdoutDelta(_) => {} EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: _, auto_approved, From f20de21cb608feef4fe295f434bf22498821bdcd Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Fri, 1 Aug 2025 14:00:19 -0700 Subject: [PATCH 15/22] collabse `stdout` and `stderr` delta events into one (#1787) --- codex-rs/core/src/exec.rs | 24 +++++++++---------- codex-rs/core/src/protocol.rs | 23 ++++++++---------- codex-rs/core/tests/exec_stream_events.rs | 16 +++++++++---- .../src/event_processor_with_human_output.rs | 3 +-- codex-rs/mcp-server/src/codex_tool_runner.rs | 3 +-- codex-rs/tui/src/chatwidget.rs | 2 +- 6 files changed, 36 insertions(+), 35 deletions(-) diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index a5a88ee9c0..5301f0220d 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -22,8 +22,8 @@ use crate::error::Result; use crate::error::SandboxErr; use crate::protocol::Event; use crate::protocol::EventMsg; -use crate::protocol::ExecCommandStderrDeltaEvent; -use crate::protocol::ExecCommandStdoutDeltaEvent; +use crate::protocol::ExecCommandOutputDeltaEvent; +use crate::protocol::ExecOutputStream; use crate::protocol::SandboxPolicy; use crate::seatbelt::spawn_command_under_seatbelt; use crate::spawn::StdioPolicy; @@ -359,17 +359,15 @@ async fn read_capped( if let Some(stream) = &stream { let chunk = tmp[..n].to_vec(); - let msg = if is_stderr { - EventMsg::ExecCommandStderrDelta(ExecCommandStderrDeltaEvent { - call_id: stream.call_id.clone(), - chunk: ByteBuf::from(chunk), - }) - } else { - EventMsg::ExecCommandStdoutDelta(ExecCommandStdoutDeltaEvent { - call_id: stream.call_id.clone(), - chunk: ByteBuf::from(chunk), - }) - }; + let msg = EventMsg::ExecCommandOutputDelta(ExecCommandOutputDeltaEvent { + call_id: stream.call_id.clone(), + stream: if is_stderr { + ExecOutputStream::Stderr + } else { + ExecOutputStream::Stdout + }, + chunk: ByteBuf::from(chunk), + }); let event = Event { id: stream.sub_id.clone(), msg, diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 8d26eda98e..3be0c92ceb 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -324,11 +324,8 @@ pub enum EventMsg { /// Notification that the server is about to execute a command. ExecCommandBegin(ExecCommandBeginEvent), - /// Incremental chunk of stdout from a running command. - ExecCommandStdoutDelta(ExecCommandStdoutDeltaEvent), - - /// Incremental chunk of stderr from a running command. - ExecCommandStderrDelta(ExecCommandStderrDeltaEvent), + /// Incremental chunk of output from a running command. + ExecCommandOutputDelta(ExecCommandOutputDeltaEvent), ExecCommandEnd(ExecCommandEndEvent), @@ -484,19 +481,19 @@ pub struct ExecCommandEndEvent { } #[derive(Debug, Clone, Deserialize, Serialize)] -pub struct ExecCommandStdoutDeltaEvent { - /// Identifier for the ExecCommandBegin that produced this chunk. - pub call_id: String, - /// Raw stdout bytes (may not be valid UTF-8). - #[serde(with = "serde_bytes")] - pub chunk: ByteBuf, +#[serde(rename_all = "snake_case")] +pub enum ExecOutputStream { + Stdout, + Stderr, } #[derive(Debug, Clone, Deserialize, Serialize)] -pub struct ExecCommandStderrDeltaEvent { +pub struct ExecCommandOutputDeltaEvent { /// Identifier for the ExecCommandBegin that produced this chunk. pub call_id: String, - /// Raw stderr bytes (may not be valid UTF-8). + /// Which stream produced this chunk. + pub stream: ExecOutputStream, + /// Raw bytes from the stream (may not be valid UTF-8). #[serde(with = "serde_bytes")] pub chunk: ByteBuf, } diff --git a/codex-rs/core/tests/exec_stream_events.rs b/codex-rs/core/tests/exec_stream_events.rs index 059b69a064..50f6888f73 100644 --- a/codex-rs/core/tests/exec_stream_events.rs +++ b/codex-rs/core/tests/exec_stream_events.rs @@ -11,15 +11,19 @@ use codex_core::exec::StdoutStream; use codex_core::exec::process_exec_tool_call; use codex_core::protocol::Event; use codex_core::protocol::EventMsg; -use codex_core::protocol::ExecCommandStderrDeltaEvent; -use codex_core::protocol::ExecCommandStdoutDeltaEvent; +use codex_core::protocol::ExecCommandOutputDeltaEvent; +use codex_core::protocol::ExecOutputStream; use codex_core::protocol::SandboxPolicy; use tokio::sync::Notify; fn collect_stdout_events(rx: Receiver) -> Vec { let mut out = Vec::new(); while let Ok(ev) = rx.try_recv() { - if let EventMsg::ExecCommandStdoutDelta(ExecCommandStdoutDeltaEvent { chunk, .. }) = ev.msg + if let EventMsg::ExecCommandOutputDelta(ExecCommandOutputDeltaEvent { + stream: ExecOutputStream::Stdout, + chunk, + .. + }) = ev.msg { out.extend_from_slice(&chunk); } @@ -126,7 +130,11 @@ async fn test_exec_stderr_stream_events_echo() { // Collect only stderr delta events let mut err = Vec::new(); while let Ok(ev) = rx.try_recv() { - if let EventMsg::ExecCommandStderrDelta(ExecCommandStderrDeltaEvent { chunk, .. }) = ev.msg + if let EventMsg::ExecCommandOutputDelta(ExecCommandOutputDeltaEvent { + stream: ExecOutputStream::Stderr, + chunk, + .. + }) = ev.msg { err.extend_from_slice(&chunk); } diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 05643c6282..7393ab72a0 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -239,8 +239,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { cwd.to_string_lossy(), ); } - EventMsg::ExecCommandStdoutDelta(_) => {} - EventMsg::ExecCommandStderrDelta(_) => {} + EventMsg::ExecCommandOutputDelta(_) => {} EventMsg::ExecCommandEnd(ExecCommandEndEvent { call_id, stdout, diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 94d47f7497..d489ffe076 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -258,8 +258,7 @@ async fn run_codex_tool_session_inner( | EventMsg::McpToolCallBegin(_) | EventMsg::McpToolCallEnd(_) | EventMsg::ExecCommandBegin(_) - | EventMsg::ExecCommandStdoutDelta(_) - | EventMsg::ExecCommandStderrDelta(_) + | EventMsg::ExecCommandOutputDelta(_) | EventMsg::ExecCommandEnd(_) | EventMsg::BackgroundEvent(_) | EventMsg::PatchApplyBegin(_) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index d6454c82a7..374a4b5688 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -374,7 +374,7 @@ impl ChatWidget<'_> { ); self.add_to_history(HistoryCell::new_active_exec_command(command)); } - EventMsg::ExecCommandStdoutDelta(_) => {} + EventMsg::ExecCommandOutputDelta(_) => {} EventMsg::PatchApplyBegin(PatchApplyBeginEvent { call_id: _, auto_approved, From 92f3566d78fa95b90ccafbb0083e0e6a599f87c5 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 1 Aug 2025 14:15:55 -0700 Subject: [PATCH 16/22] chore: introduce SandboxPolicy::WorkspaceWrite::include_default_writable_roots (#1785) Without this change, it is challenging to create integration tests to verify that the folders not included in `writable_roots` in `SandboxPolicy::WorkspaceWrite` are read-only because, by default, `get_writable_roots_with_cwd()` includes `TMPDIR`, which is where most integrationt tests do their work. This introduces a `use_exact_writable_roots` option to disable the default includes returned by `get_writable_roots_with_cwd()`. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/1785). * #1765 * __->__ #1785 --- codex-rs/common/src/sandbox_summary.rs | 4 ++++ codex-rs/core/src/config.rs | 2 ++ codex-rs/core/src/protocol.rs | 21 ++++++++++++++++++++- codex-rs/linux-sandbox/tests/landlock.rs | 1 + 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/codex-rs/common/src/sandbox_summary.rs b/codex-rs/common/src/sandbox_summary.rs index 3d33d92836..e0e309a9d9 100644 --- a/codex-rs/common/src/sandbox_summary.rs +++ b/codex-rs/common/src/sandbox_summary.rs @@ -7,6 +7,7 @@ pub fn summarize_sandbox_policy(sandbox_policy: &SandboxPolicy) -> String { SandboxPolicy::WorkspaceWrite { writable_roots, network_access, + include_default_writable_roots, } => { let mut summary = "workspace-write".to_string(); if !writable_roots.is_empty() { @@ -19,6 +20,9 @@ pub fn summarize_sandbox_policy(sandbox_policy: &SandboxPolicy) -> String { .join(", ") )); } + if !*include_default_writable_roots { + summary.push_str(" (exact writable roots)"); + } if *network_access { summary.push_str(" (network access enabled)"); } diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 1b8da89419..87629d821a 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -356,6 +356,7 @@ impl ConfigToml { Some(s) => SandboxPolicy::WorkspaceWrite { writable_roots: s.writable_roots.clone(), network_access: s.network_access, + include_default_writable_roots: true, }, None => SandboxPolicy::new_workspace_write_policy(), }, @@ -727,6 +728,7 @@ writable_roots = [ SandboxPolicy::WorkspaceWrite { writable_roots: vec![PathBuf::from("/tmp")], network_access: false, + include_default_writable_roots: true, }, sandbox_workspace_write_cfg.derive_sandbox_policy(sandbox_mode_override) ); diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 3be0c92ceb..af65f4d3a8 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -180,9 +180,19 @@ pub enum SandboxPolicy { /// default. #[serde(default)] network_access: bool, + + /// When set to `true`, will include defaults like the current working + /// directory and TMPDIR (on macOS). When `false`, only `writable_roots` + /// are used. (Mainly used for testing.) + #[serde(default = "default_true")] + include_default_writable_roots: bool, }, } +fn default_true() -> bool { + true +} + impl FromStr for SandboxPolicy { type Err = serde_json::Error; @@ -204,6 +214,7 @@ impl SandboxPolicy { SandboxPolicy::WorkspaceWrite { writable_roots: vec![], network_access: false, + include_default_writable_roots: true, } } @@ -235,7 +246,15 @@ impl SandboxPolicy { match self { SandboxPolicy::DangerFullAccess => Vec::new(), SandboxPolicy::ReadOnly => Vec::new(), - SandboxPolicy::WorkspaceWrite { writable_roots, .. } => { + SandboxPolicy::WorkspaceWrite { + writable_roots, + include_default_writable_roots, + .. + } => { + if !*include_default_writable_roots { + return writable_roots.clone(); + } + let mut roots = writable_roots.clone(); roots.push(cwd.to_path_buf()); diff --git a/codex-rs/linux-sandbox/tests/landlock.rs b/codex-rs/linux-sandbox/tests/landlock.rs index c16e9227df..1375a4c686 100644 --- a/codex-rs/linux-sandbox/tests/landlock.rs +++ b/codex-rs/linux-sandbox/tests/landlock.rs @@ -49,6 +49,7 @@ async fn run_cmd(cmd: &[&str], writable_roots: &[PathBuf], timeout_ms: u64) { let sandbox_policy = SandboxPolicy::WorkspaceWrite { writable_roots: writable_roots.to_vec(), network_access: false, + include_default_writable_roots: true, }; let sandbox_program = env!("CARGO_BIN_EXE_codex-linux-sandbox"); let codex_linux_sandbox_exe = Some(PathBuf::from(sandbox_program)); From fe62f859a68ade8e3b63bf626472a0adbda867e3 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Fri, 1 Aug 2025 15:13:53 -0700 Subject: [PATCH 17/22] Add Error variant to ConversationCreateResult [Stack 1/2] (#1784) Switch ConversationCreateResult from a struct to a tagged enum (Ok | Error) Stack: Top: #1783 Bottom: #1784 --- codex-rs/mcp-server/src/mcp_protocol.rs | 44 ++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/codex-rs/mcp-server/src/mcp_protocol.rs b/codex-rs/mcp-server/src/mcp_protocol.rs index 287890bf5e..2f8858a37b 100644 --- a/codex-rs/mcp-server/src/mcp_protocol.rs +++ b/codex-rs/mcp-server/src/mcp_protocol.rs @@ -172,15 +172,22 @@ pub enum ToolCallResponseResult { } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct ConversationCreateResult { - pub conversation_id: ConversationId, - pub model: String, +#[serde(untagged)] +pub enum ConversationCreateResult { + Ok { + conversation_id: ConversationId, + model: String, + }, + Error { + message: String, + }, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct ConversationStreamResult {} #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +// TODO: remove this status because we have is_error field in the response. #[serde(tag = "status", rename_all = "camelCase")] pub enum ConversationSendMessageResult { Ok, @@ -491,7 +498,7 @@ mod tests { request_id: RequestId::Integer(1), is_error: None, result: Some(ToolCallResponseResult::ConversationCreate( - ConversationCreateResult { + ConversationCreateResult::Ok { conversation_id: ConversationId(uuid!("d0f6ecbe-84a2-41c1-b23d-b20473b25eab")), model: "o3".into(), }, @@ -515,6 +522,35 @@ mod tests { assert_eq!(req_id, RequestId::Integer(1)); } + #[test] + fn response_error_conversation_create_full_schema() { + let env = ToolCallResponse { + request_id: RequestId::Integer(2), + is_error: Some(true), + result: Some(ToolCallResponseResult::ConversationCreate( + ConversationCreateResult::Error { + message: "Failed to initialize session".into(), + }, + )), + }; + let req_id = env.request_id.clone(); + let observed = to_val(&CallToolResult::from(env)); + let expected = json!({ + "content": [ + { "type": "text", "text": "{\"message\":\"Failed to initialize session\"}" } + ], + "isError": true, + "structuredContent": { + "message": "Failed to initialize session" + } + }); + assert_eq!( + observed, expected, + "error response (ConversationCreate) must match" + ); + assert_eq!(req_id, RequestId::Integer(2)); + } + #[test] fn response_success_conversation_stream_empty_result_object() { let env = ToolCallResponse { From 97ab8fb6101dc5203104117023453862320e91ca Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Fri, 1 Aug 2025 15:18:36 -0700 Subject: [PATCH 18/22] MCP: add conversation.create tool [Stack 2/2] (#1783) Introduce conversation.create handler (handle_create_conversation) and wire it in MessageProcessor. Stack: Top: #1783 Bottom: #1784 --------- Co-authored-by: Gabriel Peal --- codex-rs/mcp-server/src/conversation_loop.rs | 121 +++++++++++++ codex-rs/mcp-server/src/lib.rs | 1 + codex-rs/mcp-server/src/message_processor.rs | 8 + .../src/tool_handlers/create_conversation.rs | 160 ++++++++++++++++++ codex-rs/mcp-server/src/tool_handlers/mod.rs | 1 + .../mcp-server/tests/common/mcp_process.rs | 36 ++++ .../mcp-server/tests/create_conversation.rs | 128 ++++++++++++++ 7 files changed, 455 insertions(+) create mode 100644 codex-rs/mcp-server/src/conversation_loop.rs create mode 100644 codex-rs/mcp-server/src/tool_handlers/create_conversation.rs create mode 100644 codex-rs/mcp-server/tests/create_conversation.rs diff --git a/codex-rs/mcp-server/src/conversation_loop.rs b/codex-rs/mcp-server/src/conversation_loop.rs new file mode 100644 index 0000000000..534275181a --- /dev/null +++ b/codex-rs/mcp-server/src/conversation_loop.rs @@ -0,0 +1,121 @@ +use std::sync::Arc; + +use crate::exec_approval::handle_exec_approval_request; +use crate::outgoing_message::OutgoingMessageSender; +use crate::outgoing_message::OutgoingNotificationMeta; +use crate::patch_approval::handle_patch_approval_request; +use codex_core::Codex; +use codex_core::protocol::AgentMessageEvent; +use codex_core::protocol::ApplyPatchApprovalRequestEvent; +use codex_core::protocol::EventMsg; +use codex_core::protocol::ExecApprovalRequestEvent; +use mcp_types::RequestId; +use tracing::error; + +pub async fn run_conversation_loop( + codex: Arc, + outgoing: Arc, + request_id: RequestId, +) { + let request_id_str = match &request_id { + RequestId::String(s) => s.clone(), + RequestId::Integer(n) => n.to_string(), + }; + + // Stream events until the task needs to pause for user interaction or + // completes. + loop { + match codex.next_event().await { + Ok(event) => { + outgoing + .send_event_as_notification( + &event, + Some(OutgoingNotificationMeta::new(Some(request_id.clone()))), + ) + .await; + + match event.msg { + EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + command, + cwd, + call_id, + reason: _, + }) => { + handle_exec_approval_request( + command, + cwd, + outgoing.clone(), + codex.clone(), + request_id.clone(), + request_id_str.clone(), + event.id.clone(), + call_id, + ) + .await; + continue; + } + EventMsg::Error(_) => { + error!("Codex runtime error"); + } + EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + call_id, + reason, + grant_root, + changes, + }) => { + handle_patch_approval_request( + call_id, + reason, + grant_root, + changes, + outgoing.clone(), + codex.clone(), + request_id.clone(), + request_id_str.clone(), + event.id.clone(), + ) + .await; + continue; + } + EventMsg::TaskComplete(_) => {} + EventMsg::SessionConfigured(_) => { + tracing::error!("unexpected SessionConfigured event"); + } + EventMsg::AgentMessageDelta(_) => { + // TODO: think how we want to support this in the MCP + } + EventMsg::AgentReasoningDelta(_) => { + // TODO: think how we want to support this in the MCP + } + EventMsg::AgentMessage(AgentMessageEvent { .. }) => { + // TODO: think how we want to support this in the MCP + } + EventMsg::TaskStarted + | EventMsg::TokenCount(_) + | EventMsg::AgentReasoning(_) + | EventMsg::McpToolCallBegin(_) + | EventMsg::McpToolCallEnd(_) + | EventMsg::ExecCommandBegin(_) + | EventMsg::ExecCommandEnd(_) + | EventMsg::BackgroundEvent(_) + | EventMsg::ExecCommandOutputDelta(_) + | EventMsg::PatchApplyBegin(_) + | EventMsg::PatchApplyEnd(_) + | EventMsg::GetHistoryEntryResponse(_) + | EventMsg::PlanUpdate(_) + | EventMsg::ShutdownComplete => { + // For now, we do not do anything extra for these + // events. Note that + // send(codex_event_to_notification(&event)) above has + // already dispatched these events as notifications, + // though we may want to do give different treatment to + // individual events in the future. + } + } + } + Err(e) => { + error!("Codex runtime error: {e}"); + } + } + } +} diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index 6b3c0ddbe6..f6d0838efc 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -17,6 +17,7 @@ use tracing_subscriber::EnvFilter; mod codex_tool_config; mod codex_tool_runner; +mod conversation_loop; mod exec_approval; mod json_to_toml; pub mod mcp_protocol; diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index 14e9bb3887..2f99cda723 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -11,6 +11,7 @@ use crate::mcp_protocol::ToolCallRequestParams; use crate::mcp_protocol::ToolCallResponse; use crate::mcp_protocol::ToolCallResponseResult; use crate::outgoing_message::OutgoingMessageSender; +use crate::tool_handlers::create_conversation::handle_create_conversation; use crate::tool_handlers::send_message::handle_send_message; use codex_core::Codex; @@ -67,6 +68,10 @@ impl MessageProcessor { self.session_map.clone() } + pub(crate) fn outgoing(&self) -> Arc { + self.outgoing.clone() + } + pub(crate) fn running_session_ids(&self) -> Arc>> { self.running_session_ids.clone() } @@ -349,6 +354,9 @@ impl MessageProcessor { } async fn handle_new_tool_calls(&self, request_id: RequestId, params: ToolCallRequestParams) { match params { + ToolCallRequestParams::ConversationCreate(args) => { + handle_create_conversation(self, request_id, args).await; + } ToolCallRequestParams::ConversationSendMessage(args) => { handle_send_message(self, request_id, args).await; } diff --git a/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs b/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs new file mode 100644 index 0000000000..28a8965115 --- /dev/null +++ b/codex-rs/mcp-server/src/tool_handlers/create_conversation.rs @@ -0,0 +1,160 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; + +use codex_core::Codex; +use codex_core::codex_wrapper::init_codex; +use codex_core::config::Config as CodexConfig; +use codex_core::config::ConfigOverrides; +use codex_core::protocol::EventMsg; +use codex_core::protocol::SessionConfiguredEvent; +use mcp_types::RequestId; +use tokio::sync::Mutex; +use uuid::Uuid; + +use crate::conversation_loop::run_conversation_loop; +use crate::json_to_toml::json_to_toml; +use crate::mcp_protocol::ConversationCreateArgs; +use crate::mcp_protocol::ConversationCreateResult; +use crate::mcp_protocol::ConversationId; +use crate::mcp_protocol::ToolCallResponseResult; +use crate::message_processor::MessageProcessor; + +pub(crate) async fn handle_create_conversation( + message_processor: &MessageProcessor, + id: RequestId, + args: ConversationCreateArgs, +) { + // Build ConfigOverrides from args + let ConversationCreateArgs { + prompt: _, // not used here; creation only establishes the session + model, + cwd, + approval_policy, + sandbox, + config, + profile, + base_instructions, + } = args; + + // Convert config overrides JSON into CLI-style TOML overrides + let cli_overrides: Vec<(String, toml::Value)> = match config { + Some(v) => match v.as_object() { + Some(map) => map + .into_iter() + .map(|(k, v)| (k.clone(), json_to_toml(v.clone()))) + .collect(), + None => Vec::new(), + }, + None => Vec::new(), + }; + + let overrides = ConfigOverrides { + model: Some(model.clone()), + cwd: Some(PathBuf::from(cwd)), + approval_policy, + sandbox_mode: sandbox, + model_provider: None, + config_profile: profile, + codex_linux_sandbox_exe: None, + base_instructions, + include_plan_tool: None, + }; + + let cfg: CodexConfig = match CodexConfig::load_with_cli_overrides(cli_overrides, overrides) { + Ok(cfg) => cfg, + Err(e) => { + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationCreate( + ConversationCreateResult::Error { + message: format!("Failed to load config: {e}"), + }, + )), + Some(true), + ) + .await; + return; + } + }; + + // Initialize Codex session + let codex_conversation = match init_codex(cfg).await { + Ok(conv) => conv, + Err(e) => { + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationCreate( + ConversationCreateResult::Error { + message: format!("Failed to initialize session: {e}"), + }, + )), + Some(true), + ) + .await; + return; + } + }; + + // Expect SessionConfigured; if not, return error. + let EventMsg::SessionConfigured(SessionConfiguredEvent { model, .. }) = + &codex_conversation.session_configured.msg + else { + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationCreate( + ConversationCreateResult::Error { + message: "Expected SessionConfigured event".to_string(), + }, + )), + Some(true), + ) + .await; + return; + }; + + let effective_model = model.clone(); + + let session_id = codex_conversation.session_id; + let codex_arc = Arc::new(codex_conversation.codex); + + // Store session for future calls + insert_session( + session_id, + codex_arc.clone(), + message_processor.session_map(), + ) + .await; + // Run the conversation loop in the background so this request can return immediately. + let outgoing = message_processor.outgoing(); + let spawn_id = id.clone(); + tokio::spawn(async move { + run_conversation_loop(codex_arc.clone(), outgoing, spawn_id).await; + }); + + // Reply with the new conversation id and effective model + message_processor + .send_response_with_optional_error( + id, + Some(ToolCallResponseResult::ConversationCreate( + ConversationCreateResult::Ok { + conversation_id: ConversationId(session_id), + model: effective_model, + }, + )), + Some(false), + ) + .await; +} + +async fn insert_session( + session_id: Uuid, + codex: Arc, + session_map: Arc>>>, +) { + let mut guard = session_map.lock().await; + guard.insert(session_id, codex); +} diff --git a/codex-rs/mcp-server/src/tool_handlers/mod.rs b/codex-rs/mcp-server/src/tool_handlers/mod.rs index 1907ec64e0..5863bdc288 100644 --- a/codex-rs/mcp-server/src/tool_handlers/mod.rs +++ b/codex-rs/mcp-server/src/tool_handlers/mod.rs @@ -1 +1,2 @@ +pub(crate) mod create_conversation; pub(crate) mod send_message; diff --git a/codex-rs/mcp-server/tests/common/mcp_process.rs b/codex-rs/mcp-server/tests/common/mcp_process.rs index d7144af480..83cf085cd4 100644 --- a/codex-rs/mcp-server/tests/common/mcp_process.rs +++ b/codex-rs/mcp-server/tests/common/mcp_process.rs @@ -14,6 +14,7 @@ use assert_cmd::prelude::*; use codex_core::protocol::InputItem; use codex_mcp_server::CodexToolCallParam; use codex_mcp_server::CodexToolCallReplyParam; +use codex_mcp_server::mcp_protocol::ConversationCreateArgs; use codex_mcp_server::mcp_protocol::ConversationId; use codex_mcp_server::mcp_protocol::ConversationSendMessageArgs; use codex_mcp_server::mcp_protocol::ToolCallRequestParams; @@ -200,6 +201,41 @@ impl McpProcess { .await } + pub async fn send_conversation_create_tool_call( + &mut self, + prompt: &str, + model: &str, + cwd: &str, + ) -> anyhow::Result { + let params = ToolCallRequestParams::ConversationCreate(ConversationCreateArgs { + prompt: prompt.to_string(), + model: model.to_string(), + cwd: cwd.to_string(), + approval_policy: None, + sandbox: None, + config: None, + profile: None, + base_instructions: None, + }); + self.send_request( + mcp_types::CallToolRequest::METHOD, + Some(serde_json::to_value(params)?), + ) + .await + } + + pub async fn send_conversation_create_with_args( + &mut self, + args: ConversationCreateArgs, + ) -> anyhow::Result { + let params = ToolCallRequestParams::ConversationCreate(args); + self.send_request( + mcp_types::CallToolRequest::METHOD, + Some(serde_json::to_value(params)?), + ) + .await + } + async fn send_request( &mut self, method: &str, diff --git a/codex-rs/mcp-server/tests/create_conversation.rs b/codex-rs/mcp-server/tests/create_conversation.rs new file mode 100644 index 0000000000..41052ea7b3 --- /dev/null +++ b/codex-rs/mcp-server/tests/create_conversation.rs @@ -0,0 +1,128 @@ +#![allow(clippy::expect_used, clippy::unwrap_used)] + +use std::path::Path; + +use mcp_test_support::McpProcess; +use mcp_test_support::create_final_assistant_message_sse_response; +use mcp_test_support::create_mock_chat_completions_server; +use mcp_types::JSONRPCResponse; +use mcp_types::RequestId; +use pretty_assertions::assert_eq; +use serde_json::json; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_conversation_create_and_send_message_ok() { + // Mock server – we won't strictly rely on it, but provide one to satisfy any model wiring. + let responses = vec![ + create_final_assistant_message_sse_response("Done").expect("build mock assistant message"), + ]; + let server = create_mock_chat_completions_server(responses).await; + + // Temporary Codex home with config pointing at the mock server. + let codex_home = TempDir::new().expect("create temp dir"); + create_config_toml(codex_home.path(), &server.uri()).expect("write config.toml"); + + // Start MCP server process and initialize. + 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"); + + // Create a conversation via the new tool. + let req_id = mcp + .send_conversation_create_tool_call("", "o3", "/repo") + .await + .expect("send conversationCreate"); + + let resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(req_id)), + ) + .await + .expect("create response timeout") + .expect("create response error"); + + // Structured content must include status=ok, a UUID conversation_id and the model we passed. + let sc = &resp.result["structuredContent"]; + let conv_id = sc["conversation_id"].as_str().expect("uuid string"); + assert!(!conv_id.is_empty()); + assert_eq!(sc["model"], json!("o3")); + + // Now send a message to the created conversation and expect an OK result. + let send_id = mcp + .send_user_message_tool_call("Hello", conv_id) + .await + .expect("send message"); + + let send_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(send_id)), + ) + .await + .expect("send response timeout") + .expect("send response error"); + assert_eq!( + send_resp.result["structuredContent"], + json!({ "status": "ok" }) + ); + + // avoid race condition by waiting for the mock server to receive the chat.completions request + let deadline = std::time::Instant::now() + DEFAULT_READ_TIMEOUT; + loop { + let requests = server.received_requests().await.unwrap_or_default(); + if !requests.is_empty() { + break; + } + if std::time::Instant::now() >= deadline { + panic!("mock server did not receive the chat.completions request in time"); + } + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + + // Verify the outbound request body matches expectations for Chat Completions. + let request = &server.received_requests().await.unwrap()[0]; + let body = request + .body_json::() + .expect("parse request body as JSON"); + assert_eq!(body["model"], json!("o3")); + assert!(body["stream"].as_bool().unwrap_or(false)); + let messages = body["messages"] + .as_array() + .expect("messages should be array"); + let last = messages.last().expect("at least one message"); + assert_eq!(last["role"], json!("user")); + assert_eq!(last["content"], json!("Hello")); + + drop(server); +} + +// Helper to create a config.toml pointing at the mock model server. +fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write( + config_toml, + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "danger-full-access" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "chat" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} From 80555d4ff25b8d7af34be207436c786de4fbaf71 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 1 Aug 2025 16:11:24 -0700 Subject: [PATCH 19/22] feat: make .git read-only within a writable root when using Seatbelt (#1765) To make `--full-auto` safer, this PR updates the Seatbelt policy so that a `SandboxPolicy` with a `writable_root` that contains a `.git/` _directory_ will make `.git/` _read-only_ (though as a follow-up, we should also consider the case where `.git` is a _file_ with a `gitdir: /path/to/actual/repo/.git` entry that should also be protected). The two major changes in this PR: - Updating `SandboxPolicy::get_writable_roots_with_cwd()` to return a `Vec` instead of a `Vec` where a `WritableRoot` can specify a list of read-only subpaths. - Updating `create_seatbelt_command_args()` to honor the read-only subpaths in `WritableRoot`. The logic to update the policy is a fairly straightforward update to `create_seatbelt_command_args()`, but perhaps the more interesting part of this PR is the introduction of an integration test in `tests/sandbox.rs`. Leveraging the new API in #1785, we test `SandboxPolicy` under various conditions, including ones where `$TMPDIR` is not readable, which is critical for verifying the new behavior. To ensure that Codex can run its own tests, e.g.: ``` just codex debug seatbelt --full-auto -- cargo test if_git_repo_is_writable_root_then_dot_git_folder_is_read_only ``` I had to introduce the use of `CODEX_SANDBOX=sandbox`, which is comparable to how `CODEX_SANDBOX_NETWORK_DISABLED=1` was already being used. Adding a comparable change for Landlock will be done in a subsequent PR. --- AGENTS.md | 4 +- codex-rs/config.md | 2 + codex-rs/core/src/protocol.rs | 56 ++++-- codex-rs/core/src/seatbelt.rs | 238 +++++++++++++++++++++++-- codex-rs/core/src/spawn.rs | 5 + codex-rs/core/tests/sandbox.rs | 195 ++++++++++++++++++++ codex-rs/linux-sandbox/src/landlock.rs | 6 +- 7 files changed, 478 insertions(+), 28 deletions(-) create mode 100644 codex-rs/core/tests/sandbox.rs diff --git a/AGENTS.md b/AGENTS.md index 27af48ae60..5c3f659c35 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,9 @@ In the codex-rs folder where the rust code lives: -- Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR`. You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. +- Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` or `CODEX_SANDBOX_ENV_VAR`. + - You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. + - Similarly, when you spawn a process using Seatbelt (`/usr/bin/sandbox-exec`), `CODEX_SANDBOX=seatbelt` will be set on the child process. Integration tests that want to run Seatbelt themselves cannot be run under Seatbelt, so checks for `CODEX_SANDBOX=seatbelt` are also often used to early exit out of tests, as appropriate. Before creating a pull request with changes to `codex-rs`, run `just fmt` (in `codex-rs` directory) to format the code and `just fix` (in `codex-rs` directory) to fix any linter issues in the code, ensure the test suite passes by running `cargo test --all-features` in the `codex-rs` directory. diff --git a/codex-rs/config.md b/codex-rs/config.md index 1a407a239b..c7dfe42a75 100644 --- a/codex-rs/config.md +++ b/codex-rs/config.md @@ -259,6 +259,8 @@ disk, but attempts to write a file or access the network will be blocked. A more relaxed policy is `workspace-write`. When specified, the current working directory for the Codex task will be writable (as well as `$TMPDIR` on macOS). Note that the CLI defaults to using the directory where it was spawned as `cwd`, though this can be overridden using `--cwd/-C`. +On macOS (and soon Linux), all writable roots (including `cwd`) that contain a `.git/` folder _as an immediate child_ will configure the `.git/` folder to be read-only while the rest of the Git repository will be writable. This means that commands like `git commit` will fail, by default (as it entails writing to `.git/`), and will require Codex to ask for permission. + ```toml # same as `--sandbox workspace-write` sandbox_mode = "workspace-write" diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index af65f4d3a8..1bfeee56ab 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -189,6 +189,16 @@ pub enum SandboxPolicy { }, } +/// A writable root path accompanied by a list of subpaths that should remain +/// read‑only even when the root is writable. This is primarily used to ensure +/// top‑level VCS metadata directories (e.g. `.git`) under a writable root are +/// not modified by the agent. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct WritableRoot { + pub root: PathBuf, + pub read_only_subpaths: Vec, +} + fn default_true() -> bool { true } @@ -240,9 +250,10 @@ impl SandboxPolicy { } } - /// Returns the list of writable roots that should be passed down to the - /// Landlock rules installer, tailored to the current working directory. - pub fn get_writable_roots_with_cwd(&self, cwd: &Path) -> Vec { + /// Returns the list of writable roots (tailored to the current working + /// directory) together with subpaths that should remain read‑only under + /// each writable root. + pub fn get_writable_roots_with_cwd(&self, cwd: &Path) -> Vec { match self { SandboxPolicy::DangerFullAccess => Vec::new(), SandboxPolicy::ReadOnly => Vec::new(), @@ -251,24 +262,39 @@ impl SandboxPolicy { include_default_writable_roots, .. } => { - if !*include_default_writable_roots { - return writable_roots.clone(); - } + // Start from explicitly configured writable roots. + let mut roots: Vec = writable_roots.clone(); - let mut roots = writable_roots.clone(); - roots.push(cwd.to_path_buf()); + // Optionally include defaults (cwd and TMPDIR on macOS). + if *include_default_writable_roots { + roots.push(cwd.to_path_buf()); - // Also include the per-user tmp dir on macOS. - // Note this is added dynamically rather than storing it in - // writable_roots because writable_roots contains only static - // values deserialized from the config file. - if cfg!(target_os = "macos") { - if let Some(tmpdir) = std::env::var_os("TMPDIR") { - roots.push(PathBuf::from(tmpdir)); + // Also include the per-user tmp dir on macOS. + // Note this is added dynamically rather than storing it in + // `writable_roots` because `writable_roots` contains only static + // values deserialized from the config file. + if cfg!(target_os = "macos") { + if let Some(tmpdir) = std::env::var_os("TMPDIR") { + roots.push(PathBuf::from(tmpdir)); + } } } + // For each root, compute subpaths that should remain read-only. roots + .into_iter() + .map(|writable_root| { + let mut subpaths = Vec::new(); + let top_level_git = writable_root.join(".git"); + if top_level_git.is_dir() { + subpaths.push(top_level_git); + } + WritableRoot { + root: writable_root, + read_only_subpaths: subpaths, + } + }) + .collect() } } } diff --git a/codex-rs/core/src/seatbelt.rs b/codex-rs/core/src/seatbelt.rs index be2acb1bdc..0364840b1a 100644 --- a/codex-rs/core/src/seatbelt.rs +++ b/codex-rs/core/src/seatbelt.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use tokio::process::Child; use crate::protocol::SandboxPolicy; +use crate::spawn::CODEX_SANDBOX_ENV_VAR; use crate::spawn::StdioPolicy; use crate::spawn::spawn_child_async; @@ -20,10 +21,11 @@ pub async fn spawn_command_under_seatbelt( sandbox_policy: &SandboxPolicy, cwd: PathBuf, stdio_policy: StdioPolicy, - env: HashMap, + mut env: HashMap, ) -> std::io::Result { let args = create_seatbelt_command_args(command, sandbox_policy, &cwd); let arg0 = None; + env.insert(CODEX_SANDBOX_ENV_VAR.to_string(), "seatbelt".to_string()); spawn_child_async( PathBuf::from(MACOS_PATH_TO_SEATBELT_EXECUTABLE), args, @@ -50,16 +52,38 @@ fn create_seatbelt_command_args( ) } else { let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); - let (writable_folder_policies, cli_args): (Vec, Vec) = writable_roots - .iter() - .enumerate() - .map(|(index, root)| { - let param_name = format!("WRITABLE_ROOT_{index}"); - let policy: String = format!("(subpath (param \"{param_name}\"))"); - let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); - (policy, cli_arg) - }) - .unzip(); + + let mut writable_folder_policies: Vec = Vec::new(); + let mut cli_args: Vec = Vec::new(); + + for (index, wr) in writable_roots.iter().enumerate() { + // Canonicalize to avoid mismatches like /var vs /private/var on macOS. + let canonical_root = wr.root.canonicalize().unwrap_or_else(|_| wr.root.clone()); + let root_param = format!("WRITABLE_ROOT_{index}"); + cli_args.push(format!( + "-D{root_param}={}", + canonical_root.to_string_lossy() + )); + + if wr.read_only_subpaths.is_empty() { + writable_folder_policies.push(format!("(subpath (param \"{root_param}\"))")); + } else { + // Add parameters for each read-only subpath and generate + // the `(require-not ...)` clauses. + let mut require_parts: Vec = Vec::new(); + require_parts.push(format!("(subpath (param \"{root_param}\"))")); + for (subpath_index, ro) in wr.read_only_subpaths.iter().enumerate() { + let canonical_ro = ro.canonicalize().unwrap_or_else(|_| ro.clone()); + let ro_param = format!("WRITABLE_ROOT_{index}_RO_{subpath_index}"); + cli_args.push(format!("-D{ro_param}={}", canonical_ro.to_string_lossy())); + require_parts + .push(format!("(require-not (subpath (param \"{ro_param}\")))")); + } + let policy_component = format!("(require-all {} )", require_parts.join(" ")); + writable_folder_policies.push(policy_component); + } + } + if writable_folder_policies.is_empty() { ("".to_string(), Vec::::new()) } else { @@ -88,9 +112,201 @@ fn create_seatbelt_command_args( let full_policy = format!( "{MACOS_SEATBELT_BASE_POLICY}\n{file_read_policy}\n{file_write_policy}\n{network_policy}" ); + let mut seatbelt_args: Vec = vec!["-p".to_string(), full_policy]; seatbelt_args.extend(extra_cli_args); seatbelt_args.push("--".to_string()); seatbelt_args.extend(command); seatbelt_args } + +#[cfg(test)] +mod tests { + #![expect(clippy::expect_used)] + use super::MACOS_SEATBELT_BASE_POLICY; + use super::create_seatbelt_command_args; + use crate::protocol::SandboxPolicy; + use pretty_assertions::assert_eq; + use std::fs; + use std::path::Path; + use std::path::PathBuf; + use tempfile::TempDir; + + #[test] + fn create_seatbelt_args_with_read_only_git_subpath() { + // Create a temporary workspace with two writable roots: one containing + // a top-level .git directory and one without it. + let tmp = TempDir::new().expect("tempdir"); + let PopulatedTmp { + root_with_git, + root_without_git, + root_with_git_canon, + root_with_git_git_canon, + root_without_git_canon, + } = populate_tmpdir(tmp.path()); + + // Build a policy that only includes the two test roots as writable and + // does not automatically include defaults like cwd or TMPDIR. + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![root_with_git.clone(), root_without_git.clone()], + network_access: false, + include_default_writable_roots: false, + }; + + let args = create_seatbelt_command_args( + vec!["/bin/echo".to_string(), "hello".to_string()], + &policy, + tmp.path(), + ); + + // Build the expected policy text using a raw string for readability. + // Note that the policy includes: + // - the base policy, + // - read-only access to the filesystem, + // - write access to WRITABLE_ROOT_0 (but not its .git) and WRITABLE_ROOT_1. + let expected_policy = format!( + r#"{MACOS_SEATBELT_BASE_POLICY} +; allow read-only file operations +(allow file-read*) +(allow file-write* +(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) ) (subpath (param "WRITABLE_ROOT_1")) +) +"#, + ); + + let expected_args = vec![ + "-p".to_string(), + expected_policy, + format!( + "-DWRITABLE_ROOT_0={}", + root_with_git_canon.to_string_lossy() + ), + format!( + "-DWRITABLE_ROOT_0_RO_0={}", + root_with_git_git_canon.to_string_lossy() + ), + format!( + "-DWRITABLE_ROOT_1={}", + root_without_git_canon.to_string_lossy() + ), + "--".to_string(), + "/bin/echo".to_string(), + "hello".to_string(), + ]; + + assert_eq!(args, expected_args); + } + + #[test] + fn create_seatbelt_args_for_cwd_as_git_repo() { + // Create a temporary workspace with two writable roots: one containing + // a top-level .git directory and one without it. + let tmp = TempDir::new().expect("tempdir"); + let PopulatedTmp { + root_with_git, + root_with_git_canon, + root_with_git_git_canon, + .. + } = populate_tmpdir(tmp.path()); + + // Build a policy that does not specify any writable_roots, but does + // use the default ones (cwd and TMPDIR) and verifies the `.git` check + // is done properly for cwd. + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + include_default_writable_roots: true, + }; + + let args = create_seatbelt_command_args( + vec!["/bin/echo".to_string(), "hello".to_string()], + &policy, + root_with_git.as_path(), + ); + + let tmpdir_env_var = if cfg!(target_os = "macos") { + std::env::var("TMPDIR") + .ok() + .map(PathBuf::from) + .and_then(|p| p.canonicalize().ok()) + .map(|p| p.to_string_lossy().to_string()) + } else { + None + }; + let tempdir_policy_entry = if tmpdir_env_var.is_some() { + " (subpath (param \"WRITABLE_ROOT_1\"))" + } else { + "" + }; + + // Build the expected policy text using a raw string for readability. + // Note that the policy includes: + // - the base policy, + // - read-only access to the filesystem, + // - write access to WRITABLE_ROOT_0 (but not its .git) and WRITABLE_ROOT_1. + let expected_policy = format!( + r#"{MACOS_SEATBELT_BASE_POLICY} +; allow read-only file operations +(allow file-read*) +(allow file-write* +(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) ){tempdir_policy_entry} +) +"#, + ); + + let mut expected_args = vec![ + "-p".to_string(), + expected_policy, + format!( + "-DWRITABLE_ROOT_0={}", + root_with_git_canon.to_string_lossy() + ), + format!( + "-DWRITABLE_ROOT_0_RO_0={}", + root_with_git_git_canon.to_string_lossy() + ), + ]; + + if let Some(p) = tmpdir_env_var { + expected_args.push(format!("-DWRITABLE_ROOT_1={p}")); + } + + expected_args.extend(vec![ + "--".to_string(), + "/bin/echo".to_string(), + "hello".to_string(), + ]); + + assert_eq!(args, expected_args); + } + + struct PopulatedTmp { + root_with_git: PathBuf, + root_without_git: PathBuf, + root_with_git_canon: PathBuf, + root_with_git_git_canon: PathBuf, + root_without_git_canon: PathBuf, + } + + fn populate_tmpdir(tmp: &Path) -> PopulatedTmp { + let root_with_git = tmp.join("with_git"); + let root_without_git = tmp.join("no_git"); + fs::create_dir_all(&root_with_git).expect("create with_git"); + fs::create_dir_all(&root_without_git).expect("create no_git"); + fs::create_dir_all(root_with_git.join(".git")).expect("create .git"); + + // Ensure we have canonical paths for -D parameter matching. + let root_with_git_canon = root_with_git.canonicalize().expect("canonicalize with_git"); + let root_with_git_git_canon = root_with_git_canon.join(".git"); + let root_without_git_canon = root_without_git + .canonicalize() + .expect("canonicalize no_git"); + PopulatedTmp { + root_with_git, + root_without_git, + root_with_git_canon, + root_with_git_git_canon, + root_without_git_canon, + } + } +} diff --git a/codex-rs/core/src/spawn.rs b/codex-rs/core/src/spawn.rs index 9fde26539b..1c82df3180 100644 --- a/codex-rs/core/src/spawn.rs +++ b/codex-rs/core/src/spawn.rs @@ -17,6 +17,11 @@ use crate::protocol::SandboxPolicy; /// attributes, so this may change in the future. pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED"; +/// Should be set when the process is spawned under a sandbox. Currently, the +/// value is "seatbelt" for macOS, but it may change in the future to +/// accommodate sandboxing configuration and other sandboxing mechanisms. +pub const CODEX_SANDBOX_ENV_VAR: &str = "CODEX_SANDBOX"; + #[derive(Debug, Clone, Copy)] pub enum StdioPolicy { RedirectForShellTool, diff --git a/codex-rs/core/tests/sandbox.rs b/codex-rs/core/tests/sandbox.rs new file mode 100644 index 0000000000..e85156bf05 --- /dev/null +++ b/codex-rs/core/tests/sandbox.rs @@ -0,0 +1,195 @@ +#![cfg(target_os = "macos")] +#![expect(clippy::expect_used)] + +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +use codex_core::protocol::SandboxPolicy; +use codex_core::seatbelt::spawn_command_under_seatbelt; +use codex_core::spawn::CODEX_SANDBOX_ENV_VAR; +use codex_core::spawn::StdioPolicy; +use tempfile::TempDir; + +struct TestScenario { + repo_parent: PathBuf, + file_outside_repo: PathBuf, + repo_root: PathBuf, + file_in_repo_root: PathBuf, + file_in_dot_git_dir: PathBuf, +} + +struct TestExpectations { + file_outside_repo_is_writable: bool, + file_in_repo_root_is_writable: bool, + file_in_dot_git_dir_is_writable: bool, +} + +impl TestScenario { + async fn run_test(&self, policy: &SandboxPolicy, expectations: TestExpectations) { + if std::env::var(CODEX_SANDBOX_ENV_VAR) == Ok("seatbelt".to_string()) { + eprintln!("{CODEX_SANDBOX_ENV_VAR} is set to 'seatbelt', skipping test."); + return; + } + + assert_eq!( + touch(&self.file_outside_repo, policy).await, + expectations.file_outside_repo_is_writable + ); + assert_eq!( + self.file_outside_repo.exists(), + expectations.file_outside_repo_is_writable + ); + + assert_eq!( + touch(&self.file_in_repo_root, policy).await, + expectations.file_in_repo_root_is_writable + ); + assert_eq!( + self.file_in_repo_root.exists(), + expectations.file_in_repo_root_is_writable + ); + + assert_eq!( + touch(&self.file_in_dot_git_dir, policy).await, + expectations.file_in_dot_git_dir_is_writable + ); + assert_eq!( + self.file_in_dot_git_dir.exists(), + expectations.file_in_dot_git_dir_is_writable + ); + } +} + +/// If the user has added a workspace root that is not a Git repo root, then +/// the user has to specify `--skip-git-repo-check` or go through some +/// interstitial that indicates they are taking on some risk because Git +/// cannot be used to backup their work before the agent begins. +/// +/// Because the user has agreed to this risk, we do not try find all .git +/// folders in the workspace and block them (though we could change our +/// position on this in the future). +#[tokio::test] +async fn if_parent_of_repo_is_writable_then_dot_git_folder_is_writable() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![test_scenario.repo_parent.clone()], + network_access: false, + include_default_writable_roots: false, + }; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: true, + file_in_repo_root_is_writable: true, + file_in_dot_git_dir_is_writable: true, + }, + ) + .await; +} + +/// When the writable root is the root of a Git repository (as evidenced by the +/// presence of a .git folder), then the .git folder should be read-only if +/// the policy is `WorkspaceWrite`. +#[tokio::test] +async fn if_git_repo_is_writable_root_then_dot_git_folder_is_read_only() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![test_scenario.repo_root.clone()], + network_access: false, + include_default_writable_roots: false, + }; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: false, + file_in_repo_root_is_writable: true, + file_in_dot_git_dir_is_writable: false, + }, + ) + .await; +} + +/// Under DangerFullAccess, all writes should be permitted anywhere on disk, +/// including inside the .git folder. +#[tokio::test] +async fn danger_full_access_allows_all_writes() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::DangerFullAccess; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: true, + file_in_repo_root_is_writable: true, + file_in_dot_git_dir_is_writable: true, + }, + ) + .await; +} + +/// Under ReadOnly, writes should not be permitted anywhere on disk. +#[tokio::test] +async fn read_only_forbids_all_writes() { + let tmp = TempDir::new().expect("should be able to create temp dir"); + let test_scenario = create_test_scenario(&tmp); + let policy = SandboxPolicy::ReadOnly; + + test_scenario + .run_test( + &policy, + TestExpectations { + file_outside_repo_is_writable: false, + file_in_repo_root_is_writable: false, + file_in_dot_git_dir_is_writable: false, + }, + ) + .await; +} + +fn create_test_scenario(tmp: &TempDir) -> TestScenario { + let repo_parent = tmp.path().to_path_buf(); + let repo_root = repo_parent.join("repo"); + let dot_git_dir = repo_root.join(".git"); + + std::fs::create_dir(&repo_root).expect("should be able to create repo root"); + std::fs::create_dir(&dot_git_dir).expect("should be able to create .git dir"); + + TestScenario { + file_outside_repo: repo_parent.join("outside.txt"), + repo_parent, + file_in_repo_root: repo_root.join("repo_file.txt"), + repo_root, + file_in_dot_git_dir: dot_git_dir.join("dot_git_file.txt"), + } +} + +/// Note that `path` must be absolute. +async fn touch(path: &Path, policy: &SandboxPolicy) -> bool { + assert!(path.is_absolute(), "Path must be absolute: {path:?}"); + let mut child = spawn_command_under_seatbelt( + vec![ + "/usr/bin/touch".to_string(), + path.to_string_lossy().to_string(), + ], + policy, + std::env::current_dir().expect("should be able to get current dir"), + StdioPolicy::RedirectForShellTool, + HashMap::new(), + ) + .await + .expect("should be able to spawn command under seatbelt"); + child + .wait() + .await + .expect("should be able to wait for child process") + .success() +} diff --git a/codex-rs/linux-sandbox/src/landlock.rs b/codex-rs/linux-sandbox/src/landlock.rs index 326e2cb487..e13e3c8b75 100644 --- a/codex-rs/linux-sandbox/src/landlock.rs +++ b/codex-rs/linux-sandbox/src/landlock.rs @@ -36,7 +36,11 @@ pub(crate) fn apply_sandbox_policy_to_current_thread( } if !sandbox_policy.has_full_disk_write_access() { - let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); + let writable_roots = sandbox_policy + .get_writable_roots_with_cwd(cwd) + .into_iter() + .map(|writable_root| writable_root.root) + .collect(); install_filesystem_landlock_rules_on_current_thread(writable_roots)?; } From 929ba50adce36c3b3e5cf0af42e38347a57bb09b Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 1 Aug 2025 16:30:15 -0700 Subject: [PATCH 20/22] Update succesfull login page look (#1789) --- codex-rs/login/src/login_with_chatgpt.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/codex-rs/login/src/login_with_chatgpt.py b/codex-rs/login/src/login_with_chatgpt.py index 2dbf5be58a..4c07feeba0 100644 --- a/codex-rs/login/src/login_with_chatgpt.py +++ b/codex-rs/login/src/login_with_chatgpt.py @@ -686,6 +686,7 @@ LOGIN_SUCCESS_HTML = """ justify-content: center; position: relative; background: white; + font-family: system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, 'Open Sans', 'Helvetica Neue', sans-serif; } .inner-container { @@ -703,6 +704,7 @@ LOGIN_SUCCESS_HTML = """ align-items: center; gap: 20px; display: flex; + margin-top: 15vh; } .svg-wrapper { position: relative; @@ -710,9 +712,9 @@ LOGIN_SUCCESS_HTML = """ .title { text-align: center; color: var(--text-primary, #0D0D0D); - font-size: 28px; + font-size: 32px; font-weight: 400; - line-height: 36.40px; + line-height: 40px; word-wrap: break-word; } .setup-box { @@ -785,16 +787,26 @@ LOGIN_SUCCESS_HTML = """ word-wrap: break-word; text-decoration: none; } + .logo { + display: flex; + align-items: center; + justify-content: center; + width: 4rem; + height: 4rem; + border-radius: 16px; + border: .5px solid rgba(0, 0, 0, 0.1); + box-shadow: rgba(0, 0, 0, 0.1) 0px 4px 16px 0px; + box-sizing: border-box; + background-color: rgb(255, 255, 255); + }
-
- - - +
Signed in to Codex CLI
From 7e0f506da208d6ee9c289cc28b13532226d5abdb Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 1 Aug 2025 17:31:38 -0700 Subject: [PATCH 21/22] check for updates (#1764) 1. Ping https://api.github.com/repos/openai/codex/releases/latest (at most once every 20 hrs) 2. Store the result in ~/.codex/version.jsonl 3. If CARGO_PKG_VERSION < latest_version, print a message at boot. --------- Co-authored-by: easong-openai --- codex-cli/bin/codex.js | 1 + codex-rs/Cargo.lock | 3 + codex-rs/tui/Cargo.toml | 3 + codex-rs/tui/src/lib.rs | 37 ++++++++++ codex-rs/tui/src/updates.rs | 137 ++++++++++++++++++++++++++++++++++++ 5 files changed, 181 insertions(+) create mode 100644 codex-rs/tui/src/updates.rs diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index ae1fb9593c..df06dd36a7 100755 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -83,6 +83,7 @@ if (wantsNative && process.platform !== 'win32') { const child = spawn(binaryPath, process.argv.slice(2), { stdio: "inherit", + env: { ...process.env, CODEX_MANAGED_BY_NPM: "1" }, }); child.on("error", (err) => { diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 460a440e63..d71553cf4a 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -843,6 +843,7 @@ version = "0.0.0" dependencies = [ "anyhow", "base64 0.22.1", + "chrono", "clap", "codex-ansi-escape", "codex-arg0", @@ -861,6 +862,8 @@ dependencies = [ "ratatui", "ratatui-image", "regex-lite", + "reqwest", + "serde", "serde_json", "shlex", "strum 0.27.2", diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 468f2f3be6..09b537c6c3 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -17,6 +17,7 @@ workspace = true [dependencies] anyhow = "1" base64 = "0.22.1" +chrono = { version = "0.4", features = ["serde"] } clap = { version = "4", features = ["derive"] } codex-ansi-escape = { path = "../ansi-escape" } codex-arg0 = { path = "../arg0" } @@ -41,6 +42,8 @@ ratatui = { version = "0.29.0", features = [ ] } ratatui-image = "8.0.0" regex-lite = "0.1" +reqwest = { version = "0.12", features = ["json"] } +serde = { version = "1", features = ["derive"] } serde_json = { version = "1", features = ["preserve_order"] } shlex = "1.3.0" strum = "0.27.2" diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index f0a0e9d833..0ec9be6153 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -41,6 +41,11 @@ mod text_formatting; mod tui; mod user_approval_widget; +#[cfg(not(debug_assertions))] +mod updates; +#[cfg(not(debug_assertions))] +use color_eyre::owo_colors::OwoColorize; + pub use cli::Cli; pub async fn run_main( @@ -139,6 +144,38 @@ pub async fn run_main( .with(tui_layer) .try_init(); + #[allow(clippy::print_stderr)] + #[cfg(not(debug_assertions))] + if let Some(latest_version) = updates::get_upgrade_version(&config) { + let current_version = env!("CARGO_PKG_VERSION"); + let exe = std::env::current_exe()?; + let managed_by_npm = std::env::var_os("CODEX_MANAGED_BY_NPM").is_some(); + + eprintln!( + "{} {current_version} -> {latest_version}.", + "✨⬆️ Update available!".bold().cyan() + ); + + if managed_by_npm { + let npm_cmd = "npm install -g @openai/codex@latest"; + eprintln!("Run {} to update.", npm_cmd.cyan().on_black()); + } else if cfg!(target_os = "macos") + && (exe.starts_with("/opt/homebrew") || exe.starts_with("/usr/local")) + { + let brew_cmd = "brew upgrade codex"; + eprintln!("Run {} to update.", brew_cmd.cyan().on_black()); + } else { + eprintln!( + "See {} for the latest releases and installation options.", + "https://github.com/openai/codex/releases/latest" + .cyan() + .on_black() + ); + } + + eprintln!(""); + } + let show_login_screen = should_show_login_screen(&config); if show_login_screen { std::io::stdout() diff --git a/codex-rs/tui/src/updates.rs b/codex-rs/tui/src/updates.rs new file mode 100644 index 0000000000..c7f7afd2a5 --- /dev/null +++ b/codex-rs/tui/src/updates.rs @@ -0,0 +1,137 @@ +#![cfg(any(not(debug_assertions), test))] + +use chrono::DateTime; +use chrono::Duration; +use chrono::Utc; +use serde::Deserialize; +use serde::Serialize; +use std::path::Path; +use std::path::PathBuf; + +use codex_core::config::Config; + +pub fn get_upgrade_version(config: &Config) -> Option { + let version_file = version_filepath(config); + let info = read_version_info(&version_file).ok(); + + if match &info { + None => true, + Some(info) => info.last_checked_at < Utc::now() - Duration::hours(20), + } { + // Refresh the cached latest version in the background so TUI startup + // isn’t blocked by a network call. The UI reads the previously cached + // value (if any) for this run; the next run shows the banner if needed. + tokio::spawn(async move { + check_for_update(&version_file) + .await + .inspect_err(|e| tracing::error!("Failed to update version: {e}")) + }); + } + + info.and_then(|info| { + let current_version = env!("CARGO_PKG_VERSION"); + if is_newer(&info.latest_version, current_version).unwrap_or(false) { + Some(info.latest_version) + } else { + None + } + }) +} + +#[derive(Serialize, Deserialize, Debug, Clone)] +struct VersionInfo { + latest_version: String, + // ISO-8601 timestamp (RFC3339) + last_checked_at: DateTime, +} + +#[derive(Deserialize, Debug, Clone)] +struct ReleaseInfo { + tag_name: String, +} + +const VERSION_FILENAME: &str = "version.json"; +const LATEST_RELEASE_URL: &str = "https://api.github.com/repos/openai/codex/releases/latest"; + +fn version_filepath(config: &Config) -> PathBuf { + config.codex_home.join(VERSION_FILENAME) +} + +fn read_version_info(version_file: &Path) -> anyhow::Result { + let contents = std::fs::read_to_string(version_file)?; + Ok(serde_json::from_str(&contents)?) +} + +async fn check_for_update(version_file: &Path) -> anyhow::Result<()> { + let ReleaseInfo { + tag_name: latest_tag_name, + } = reqwest::Client::new() + .get(LATEST_RELEASE_URL) + .header( + "User-Agent", + format!( + "codex/{} (+https://github.com/openai/codex)", + env!("CARGO_PKG_VERSION") + ), + ) + .send() + .await? + .error_for_status()? + .json::() + .await?; + + let info = VersionInfo { + latest_version: latest_tag_name + .strip_prefix("rust-v") + .ok_or_else(|| anyhow::anyhow!("Failed to parse latest tag name '{latest_tag_name}'"))? + .into(), + last_checked_at: Utc::now(), + }; + + let json_line = format!("{}\n", serde_json::to_string(&info)?); + if let Some(parent) = version_file.parent() { + tokio::fs::create_dir_all(parent).await?; + } + tokio::fs::write(version_file, json_line).await?; + Ok(()) +} + +fn is_newer(latest: &str, current: &str) -> Option { + match (parse_version(latest), parse_version(current)) { + (Some(l), Some(c)) => Some(l > c), + _ => None, + } +} + +fn parse_version(v: &str) -> Option<(u64, u64, u64)> { + let mut iter = v.trim().split('.'); + let maj = iter.next()?.parse::().ok()?; + let min = iter.next()?.parse::().ok()?; + let pat = iter.next()?.parse::().ok()?; + Some((maj, min, pat)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn prerelease_version_is_not_considered_newer() { + assert_eq!(is_newer("0.11.0-beta.1", "0.11.0"), None); + assert_eq!(is_newer("1.0.0-rc.1", "1.0.0"), None); + } + + #[test] + fn plain_semver_comparisons_work() { + assert_eq!(is_newer("0.11.1", "0.11.0"), Some(true)); + assert_eq!(is_newer("0.11.0", "0.11.1"), Some(false)); + assert_eq!(is_newer("1.0.0", "0.9.9"), Some(true)); + assert_eq!(is_newer("0.9.9", "1.0.0"), Some(false)); + } + + #[test] + fn whitespace_is_ignored() { + assert_eq!(parse_version(" 1.2.3 \n"), Some((1, 2, 3))); + assert_eq!(is_newer(" 1.2.3 ", "1.2.2"), Some(true)); + } +} From 81bb1c9e264095708a01f6326bf8d527a6b2d47b Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Sat, 2 Aug 2025 12:05:06 -0700 Subject: [PATCH 22/22] Fix compact (#1798) We are not recording the summary in the history. --- codex-rs/core/src/codex.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 6e21642da1..e759acb4a2 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1373,7 +1373,7 @@ async fn run_compact_task( let mut retries = 0; loop { - let attempt_result = drain_to_completed(&sess, &prompt).await; + let attempt_result = drain_to_completed(&sess, &sub_id, &prompt).await; match attempt_result { Ok(()) => break, @@ -2001,7 +2001,7 @@ fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) -> Option CodexResult<()> { +async fn drain_to_completed(sess: &Session, sub_id: &str, prompt: &Prompt) -> CodexResult<()> { let mut stream = sess.client.clone().stream(prompt).await?; loop { let maybe_event = stream.next().await; @@ -2011,7 +2011,32 @@ async fn drain_to_completed(sess: &Session, prompt: &Prompt) -> CodexResult<()> )); }; match event { - Ok(ResponseEvent::Completed { .. }) => return Ok(()), + Ok(ResponseEvent::OutputItemDone(item)) => { + // Record only to in-memory conversation history; avoid state snapshot. + let mut state = sess.state.lock().unwrap(); + state.history.record_items(std::slice::from_ref(&item)); + } + Ok(ResponseEvent::Completed { + response_id: _, + token_usage, + }) => { + let token_usage = match token_usage { + Some(usage) => usage, + None => { + return Err(CodexErr::Stream( + "token_usage was None in ResponseEvent::Completed".into(), + )); + } + }; + sess.tx_event + .send(Event { + id: sub_id.to_string(), + msg: EventMsg::TokenCount(token_usage), + }) + .await + .ok(); + return Ok(()); + } Ok(_) => continue, Err(e) => return Err(e), }