From 4673090f733cb1e691181e2c63f28f3c24cee752 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Mon, 22 Dec 2025 15:12:23 -0800 Subject: [PATCH 1/5] feat: open prompt in configured external editor (#7606) Add `ctrl+g` shortcut to enable opening current prompt in configured editor (`$VISUAL` or `$EDITOR`). - Prompt is updated with editor's content upon editor close. - Paste placeholders are automatically expanded when opening the external editor, and are not "recompressed" on close - They could be preserved in the editor, but it would be hard to prevent the user from modifying the placeholder text directly, which would drop the mapping to the `pending_paste` value - Image placeholders stay as-is - `ctrl+g` explanation added to shortcuts menu, snapshot tests updated https://github.com/user-attachments/assets/4ee05c81-fa49-4e99-8b07-fc9eef0bbfce --- codex-rs/Cargo.lock | 10 + codex-rs/tui/Cargo.toml | 9 + codex-rs/tui/src/app.rs | 92 +++++++ codex-rs/tui/src/app_event.rs | 3 + codex-rs/tui/src/bottom_pane/chat_composer.rs | 239 +++++++++++++++++- codex-rs/tui/src/bottom_pane/footer.rs | 13 + codex-rs/tui/src/bottom_pane/mod.rs | 19 ++ ...__tests__footer_mode_shortcut_overlay.snap | 9 +- ...tests__footer_shortcuts_shift_and_esc.snap | 9 +- codex-rs/tui/src/chatwidget.rs | 38 ++- codex-rs/tui/src/chatwidget/tests.rs | 1 + codex-rs/tui/src/external_editor.rs | 171 +++++++++++++ codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/tui.rs | 127 +++++++++- 14 files changed, 723 insertions(+), 18 deletions(-) create mode 100644 codex-rs/tui/src/external_editor.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index fb63e758ef..42c8c557dc 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1756,6 +1756,7 @@ dependencies = [ "supports-color 3.0.2", "tempfile", "textwrap 0.16.2", + "thiserror 2.0.17", "tokio", "tokio-stream", "tokio-util", @@ -1770,6 +1771,9 @@ dependencies = [ "url", "uuid", "vt100", + "which", + "windows-sys 0.52.0", + "winsplit", ] [[package]] @@ -8379,6 +8383,12 @@ version = "0.0.19" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d135d17ab770252ad95e9a872d365cf3090e3be864a34ab46f48555993efc904" +[[package]] +name = "winsplit" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ab703352da6a72f35c39a533526393725640575bb211f61987a2748323ad956" + [[package]] name = "wiremock" version = "0.6.5" diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 06167c973a..3248c7377c 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -71,6 +71,7 @@ strum_macros = { workspace = true } supports-color = { workspace = true } tempfile = { workspace = true } textwrap = { workspace = true } +thiserror = { workspace = true } tokio = { workspace = true, features = [ "io-std", "macros", @@ -97,6 +98,14 @@ tokio-util = { workspace = true, features = ["time"] } [target.'cfg(unix)'.dependencies] libc = { workspace = true } +[target.'cfg(windows)'.dependencies] +which = { workspace = true } +windows-sys = { version = "0.52", features = [ + "Win32_Foundation", + "Win32_System_Console", +] } +winsplit = "0.1" + # Clipboard support via `arboard` is not available on Android/Termux. # Only include it for non-Android targets so the crate builds on Android. [target.'cfg(not(target_os = "android"))'.dependencies] diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 14b785ec95..c91eafcbe7 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -3,9 +3,12 @@ use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::ApprovalRequest; use crate::chatwidget::ChatWidget; +use crate::chatwidget::ExternalEditorState; use crate::diff_render::DiffSummary; use crate::exec_command::strip_bash_lc_and_escape; +use crate::external_editor; use crate::file_search::FileSearchManager; +use crate::history_cell; use crate::history_cell::HistoryCell; use crate::model_migration::ModelMigrationOutcome; use crate::model_migration::migration_copy_for_models; @@ -62,6 +65,8 @@ use tokio::sync::mpsc::unbounded_channel; #[cfg(not(debug_assertions))] use crate::history_cell::UpdateAvailableHistoryCell; +const EXTERNAL_EDITOR_HINT: &str = "Save and close external editor to continue."; + #[derive(Debug, Clone)] pub struct AppExitInfo { pub token_usage: TokenUsage, @@ -542,6 +547,11 @@ impl App { } }, )?; + if self.chat_widget.external_editor_state() == ExternalEditorState::Requested { + self.chat_widget + .set_external_editor_state(ExternalEditorState::Active); + self.app_event_tx.send(AppEvent::LaunchExternalEditor); + } } } } @@ -796,6 +806,11 @@ impl App { AppEvent::OpenFeedbackConsent { category } => { self.chat_widget.open_feedback_consent(category); } + AppEvent::LaunchExternalEditor => { + if self.chat_widget.external_editor_state() == ExternalEditorState::Active { + self.launch_external_editor(tui).await; + } + } AppEvent::OpenWindowsSandboxEnablePrompt { preset } => { self.chat_widget.open_windows_sandbox_enable_prompt(preset); } @@ -1148,6 +1163,68 @@ impl App { self.config.model_reasoning_effort = effort; } + async fn launch_external_editor(&mut self, tui: &mut tui::Tui) { + let editor_cmd = match external_editor::resolve_editor_command() { + Ok(cmd) => cmd, + Err(external_editor::EditorError::MissingEditor) => { + self.chat_widget + .add_to_history(history_cell::new_error_event( + "Cannot open external editor: set $VISUAL or $EDITOR".to_string(), + )); + self.reset_external_editor_state(tui); + return; + } + Err(err) => { + self.chat_widget + .add_to_history(history_cell::new_error_event(format!( + "Failed to open editor: {err}", + ))); + self.reset_external_editor_state(tui); + return; + } + }; + + let seed = self.chat_widget.composer_text_with_pending(); + let editor_result = tui + .with_restored(tui::RestoreMode::KeepRaw, || async { + external_editor::run_editor(&seed, &editor_cmd).await + }) + .await; + self.reset_external_editor_state(tui); + + match editor_result { + Ok(new_text) => { + // Trim trailing whitespace + let cleaned = new_text.trim_end().to_string(); + self.chat_widget.apply_external_edit(cleaned); + } + Err(err) => { + self.chat_widget + .add_to_history(history_cell::new_error_event(format!( + "Failed to open editor: {err}", + ))); + } + } + tui.frame_requester().schedule_frame(); + } + + fn request_external_editor_launch(&mut self, tui: &mut tui::Tui) { + self.chat_widget + .set_external_editor_state(ExternalEditorState::Requested); + self.chat_widget.set_footer_hint_override(Some(vec![( + EXTERNAL_EDITOR_HINT.to_string(), + String::new(), + )])); + tui.frame_requester().schedule_frame(); + } + + fn reset_external_editor_state(&mut self, tui: &mut tui::Tui) { + self.chat_widget + .set_external_editor_state(ExternalEditorState::Closed); + self.chat_widget.set_footer_hint_override(None); + tui.frame_requester().schedule_frame(); + } + async fn handle_key_event(&mut self, tui: &mut tui::Tui, key_event: KeyEvent) { match key_event { KeyEvent { @@ -1161,6 +1238,21 @@ impl App { self.overlay = Some(Overlay::new_transcript(self.transcript_cells.clone())); tui.frame_requester().schedule_frame(); } + KeyEvent { + code: KeyCode::Char('g'), + modifiers: crossterm::event::KeyModifiers::CONTROL, + kind: KeyEventKind::Press, + .. + } => { + // Only launch the external editor if there is no overlay and the bottom pane is not in use. + // Note that it can be launched while a task is running to enable editing while the previous turn is ongoing. + if self.overlay.is_none() + && self.chat_widget.can_launch_external_editor() + && self.chat_widget.external_editor_state() == ExternalEditorState::Closed + { + self.request_external_editor_launch(tui); + } + } // Esc primes/advances backtracking only in normal (not working) mode // with the composer focused and empty. In any other state, forward // Esc so the active UI (e.g. status indicator, modals, popups) diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 0be556de8b..1f99e372e9 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -181,6 +181,9 @@ pub(crate) enum AppEvent { OpenFeedbackConsent { category: FeedbackCategory, }, + + /// Launch the external editor after a normal draw has completed. + LaunchExternalEditor, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 17877e4bfa..d6d01045b4 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -282,6 +282,85 @@ impl ChatComposer { } } + /// Replace the composer content with text from an external editor. + /// Clears pending paste placeholders and keeps only attachments whose + /// placeholder labels still appear in the new text. Cursor is placed at + /// the end after rebuilding elements. + pub(crate) fn apply_external_edit(&mut self, text: String) { + self.pending_pastes.clear(); + + // Count placeholder occurrences in the new text. + let mut placeholder_counts: HashMap = HashMap::new(); + for placeholder in self.attached_images.iter().map(|img| &img.placeholder) { + if placeholder_counts.contains_key(placeholder) { + continue; + } + let count = text.match_indices(placeholder).count(); + if count > 0 { + placeholder_counts.insert(placeholder.clone(), count); + } + } + + // Keep attachments only while we have matching occurrences left. + let mut kept_images = Vec::new(); + for img in self.attached_images.drain(..) { + if let Some(count) = placeholder_counts.get_mut(&img.placeholder) + && *count > 0 + { + *count -= 1; + kept_images.push(img); + } + } + self.attached_images = kept_images; + + // Rebuild textarea so placeholders become elements again. + self.textarea.set_text(""); + let mut remaining: HashMap<&str, usize> = HashMap::new(); + for img in &self.attached_images { + *remaining.entry(img.placeholder.as_str()).or_insert(0) += 1; + } + + let mut occurrences: Vec<(usize, &str)> = Vec::new(); + for placeholder in remaining.keys() { + for (pos, _) in text.match_indices(placeholder) { + occurrences.push((pos, *placeholder)); + } + } + occurrences.sort_unstable_by_key(|(pos, _)| *pos); + + let mut idx = 0usize; + for (pos, ph) in occurrences { + let Some(count) = remaining.get_mut(ph) else { + continue; + }; + if *count == 0 { + continue; + } + if pos > idx { + self.textarea.insert_str(&text[idx..pos]); + } + self.textarea.insert_element(ph); + *count -= 1; + idx = pos + ph.len(); + } + if idx < text.len() { + self.textarea.insert_str(&text[idx..]); + } + + self.textarea.set_cursor(self.textarea.text().len()); + self.sync_popups(); + } + + pub(crate) fn current_text_with_pending(&self) -> String { + let mut text = self.textarea.text().to_string(); + for (placeholder, actual) in &self.pending_pastes { + if text.contains(placeholder) { + text = text.replace(placeholder, actual); + } + } + text + } + /// Override the footer hint items displayed beneath the composer. Passing /// `None` restores the default shortcut footer. pub(crate) fn set_footer_hint_override(&mut self, items: Option>) { @@ -321,7 +400,8 @@ impl ChatComposer { .file_name() .map(|name| name.to_string_lossy().into_owned()) .unwrap_or_else(|| "image".to_string()); - let placeholder = format!("[{file_label} {width}x{height}]"); + let base_placeholder = format!("{file_label} {width}x{height}"); + let placeholder = self.next_image_placeholder(&base_placeholder); // Insert as an element to match large paste placeholder behavior: // styled distinctly and treated atomically for cursor/mutations. self.textarea.insert_element(&placeholder); @@ -384,6 +464,22 @@ impl ChatComposer { } } + fn next_image_placeholder(&mut self, base: &str) -> String { + let text = self.textarea.text(); + let mut suffix = 1; + loop { + let placeholder = if suffix == 1 { + format!("[{base}]") + } else { + format!("[{base} #{suffix}]") + }; + if !text.contains(&placeholder) { + return placeholder; + } + suffix += 1; + } + } + pub(crate) fn insert_str(&mut self, text: &str) { self.textarea.insert_str(text); self.sync_popups(); @@ -3180,6 +3276,35 @@ mod tests { assert!(composer.attached_images.is_empty()); } + #[test] + fn duplicate_image_placeholders_get_suffix() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + let path = PathBuf::from("/tmp/image_dup.png"); + composer.attach_image(path.clone(), 10, 5, "PNG"); + composer.handle_paste(" ".into()); + composer.attach_image(path, 10, 5, "PNG"); + + let text = composer.textarea.text().to_string(); + assert!(text.contains("[image_dup.png 10x5]")); + assert!(text.contains("[image_dup.png 10x5 #2]")); + assert_eq!( + composer.attached_images[0].placeholder, + "[image_dup.png 10x5]" + ); + assert_eq!( + composer.attached_images[1].placeholder, + "[image_dup.png 10x5 #2]" + ); + } + #[test] fn image_placeholder_backspace_behaves_like_text_placeholder() { let (tx, _rx) = unbounded_channel::(); @@ -4029,4 +4154,116 @@ mod tests { "'/zzz' should not activate slash popup because it is not a prefix of any built-in command" ); } + + #[test] + fn apply_external_edit_rebuilds_text_and_attachments() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + let placeholder = "[image 10x10]".to_string(); + composer.textarea.insert_element(&placeholder); + composer.attached_images.push(AttachedImage { + placeholder: placeholder.clone(), + path: PathBuf::from("img.png"), + }); + composer + .pending_pastes + .push(("[Pasted]".to_string(), "data".to_string())); + + composer.apply_external_edit(format!("Edited {placeholder} text")); + + assert_eq!( + composer.current_text(), + format!("Edited {placeholder} text") + ); + assert!(composer.pending_pastes.is_empty()); + assert_eq!(composer.attached_images.len(), 1); + assert_eq!(composer.attached_images[0].placeholder, placeholder); + assert_eq!(composer.textarea.cursor(), composer.current_text().len()); + } + + #[test] + fn apply_external_edit_drops_missing_attachments() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + let placeholder = "[image 10x10]".to_string(); + composer.textarea.insert_element(&placeholder); + composer.attached_images.push(AttachedImage { + placeholder: placeholder.clone(), + path: PathBuf::from("img.png"), + }); + + composer.apply_external_edit("No images here".to_string()); + + assert_eq!(composer.current_text(), "No images here".to_string()); + assert!(composer.attached_images.is_empty()); + } + + #[test] + fn current_text_with_pending_expands_placeholders() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + let placeholder = "[Pasted Content 5 chars]".to_string(); + composer.textarea.insert_element(&placeholder); + composer + .pending_pastes + .push((placeholder.clone(), "hello".to_string())); + + assert_eq!( + composer.current_text_with_pending(), + "hello".to_string(), + "placeholder should expand to actual text" + ); + } + + #[test] + fn apply_external_edit_limits_duplicates_to_occurrences() { + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + let placeholder = "[image 10x10]".to_string(); + composer.textarea.insert_element(&placeholder); + composer.attached_images.push(AttachedImage { + placeholder: placeholder.clone(), + path: PathBuf::from("img.png"), + }); + + composer.apply_external_edit(format!("{placeholder} extra {placeholder}")); + + assert_eq!( + composer.current_text(), + format!("{placeholder} extra {placeholder}") + ); + assert_eq!(composer.attached_images.len(), 1); + } } diff --git a/codex-rs/tui/src/bottom_pane/footer.rs b/codex-rs/tui/src/bottom_pane/footer.rs index d47ffec98b..c3f2da0e35 100644 --- a/codex-rs/tui/src/bottom_pane/footer.rs +++ b/codex-rs/tui/src/bottom_pane/footer.rs @@ -162,6 +162,7 @@ fn shortcut_overlay_lines(state: ShortcutsState) -> Vec> { let mut newline = Line::from(""); let mut file_paths = Line::from(""); let mut paste_image = Line::from(""); + let mut external_editor = Line::from(""); let mut edit_previous = Line::from(""); let mut quit = Line::from(""); let mut show_transcript = Line::from(""); @@ -173,6 +174,7 @@ fn shortcut_overlay_lines(state: ShortcutsState) -> Vec> { ShortcutId::InsertNewline => newline = text, ShortcutId::FilePaths => file_paths = text, ShortcutId::PasteImage => paste_image = text, + ShortcutId::ExternalEditor => external_editor = text, ShortcutId::EditPrevious => edit_previous = text, ShortcutId::Quit => quit = text, ShortcutId::ShowTranscript => show_transcript = text, @@ -185,6 +187,7 @@ fn shortcut_overlay_lines(state: ShortcutsState) -> Vec> { newline, file_paths, paste_image, + external_editor, edit_previous, quit, Line::from(""), @@ -261,6 +264,7 @@ enum ShortcutId { InsertNewline, FilePaths, PasteImage, + ExternalEditor, EditPrevious, Quit, ShowTranscript, @@ -381,6 +385,15 @@ const SHORTCUTS: &[ShortcutDescriptor] = &[ prefix: "", label: " to paste images", }, + ShortcutDescriptor { + id: ShortcutId::ExternalEditor, + bindings: &[ShortcutBinding { + key: key_hint::ctrl(KeyCode::Char('g')), + condition: DisplayCondition::Always, + }], + prefix: "", + label: " to edit in external editor", + }, ShortcutDescriptor { id: ShortcutId::EditPrevious, bindings: &[ShortcutBinding { diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index ea599698b9..4ad117082c 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -274,6 +274,20 @@ impl BottomPane { self.composer.current_text() } + pub(crate) fn composer_text_with_pending(&self) -> String { + self.composer.current_text_with_pending() + } + + pub(crate) fn apply_external_edit(&mut self, text: String) { + self.composer.apply_external_edit(text); + self.request_redraw(); + } + + pub(crate) fn set_footer_hint_override(&mut self, items: Option>) { + self.composer.set_footer_hint_override(items); + self.request_redraw(); + } + /// Update the animated header shown to the left of the brackets in the /// status indicator (defaults to "Working"). No-ops if the status /// indicator is not active. @@ -428,6 +442,11 @@ impl BottomPane { !self.is_task_running && self.view_stack.is_empty() && !self.composer.popup_active() } + /// Return true when no popups or modal views are active, regardless of task state. + pub(crate) fn can_launch_external_editor(&self) -> bool { + self.view_stack.is_empty() && !self.composer.popup_active() + } + pub(crate) fn show_view(&mut self, view: Box) { self.push_view(view); } diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap index 3b6782d06d..7d05a92237 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shortcut_overlay.snap @@ -10,7 +10,8 @@ expression: terminal.backend() " " " " " " -" / for commands shift + enter for newline " -" @ for file paths ctrl + v to paste images " -" esc again to edit previous message ctrl + c to exit " -" ctrl + t to view transcript " +" / for commands shift + enter for newline " +" @ for file paths ctrl + v to paste images " +" ctrl + g to edit in external editor esc again to edit previous message " +" ctrl + c to exit " +" ctrl + t to view transcript " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap index 264515a6c2..445fa44484 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__footer__tests__footer_shortcuts_shift_and_esc.snap @@ -2,7 +2,8 @@ source: tui/src/bottom_pane/footer.rs expression: terminal.backend() --- -" / for commands shift + enter for newline " -" @ for file paths ctrl + v to paste images " -" esc again to edit previous message ctrl + c to exit " -" ctrl + t to view transcript " +" / for commands shift + enter for newline " +" @ for file paths ctrl + v to paste images " +" ctrl + g to edit in external editor esc again to edit previous message " +" ctrl + c to exit " +" ctrl + t to view transcript " diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index f0afed3bf2..aad4e44ad8 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -302,6 +302,14 @@ enum RateLimitSwitchPromptState { Shown, } +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) enum ExternalEditorState { + #[default] + Closed, + Requested, + Active, +} + pub(crate) struct ChatWidget { app_event_tx: AppEventSender, codex_op_tx: UnboundedSender, @@ -360,6 +368,7 @@ pub(crate) struct ChatWidget { feedback: codex_feedback::CodexFeedback, // Current session rollout path (if known) current_rollout_path: Option, + external_editor_state: ExternalEditorState, } struct UserMessage { @@ -1469,6 +1478,7 @@ impl ChatWidget { last_rendered_width: std::cell::Cell::new(None), feedback, current_rollout_path: None, + external_editor_state: ExternalEditorState::Closed, }; widget.prefetch_rate_limits(); @@ -1555,6 +1565,7 @@ impl ChatWidget { last_rendered_width: std::cell::Cell::new(None), feedback, current_rollout_path: None, + external_editor_state: ExternalEditorState::Closed, }; widget.prefetch_rate_limits(); @@ -1653,6 +1664,31 @@ impl ChatWidget { self.request_redraw(); } + pub(crate) fn composer_text_with_pending(&self) -> String { + self.bottom_pane.composer_text_with_pending() + } + + pub(crate) fn apply_external_edit(&mut self, text: String) { + self.bottom_pane.apply_external_edit(text); + self.request_redraw(); + } + + pub(crate) fn external_editor_state(&self) -> ExternalEditorState { + self.external_editor_state + } + + pub(crate) fn set_external_editor_state(&mut self, state: ExternalEditorState) { + self.external_editor_state = state; + } + + pub(crate) fn set_footer_hint_override(&mut self, items: Option>) { + self.bottom_pane.set_footer_hint_override(items); + } + + pub(crate) fn can_launch_external_editor(&self) -> bool { + self.bottom_pane.can_launch_external_editor() + } + fn dispatch_command(&mut self, cmd: SlashCommand) { if !cmd.available_during_task() && self.bottom_pane.is_task_running() { let message = format!( @@ -1854,7 +1890,7 @@ impl ChatWidget { .send(AppEvent::InsertHistoryCell(Box::new(cell))); } - fn add_to_history(&mut self, cell: impl HistoryCell + 'static) { + pub(crate) fn add_to_history(&mut self, cell: impl HistoryCell + 'static) { self.add_boxed_history(Box::new(cell)); } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 5e595d330f..5aa0e5d064 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -407,6 +407,7 @@ async fn make_chatwidget_manual( last_rendered_width: std::cell::Cell::new(None), feedback: codex_feedback::CodexFeedback::new(), current_rollout_path: None, + external_editor_state: ExternalEditorState::Closed, }; (widget, rx, op_rx) } diff --git a/codex-rs/tui/src/external_editor.rs b/codex-rs/tui/src/external_editor.rs new file mode 100644 index 0000000000..2818503d03 --- /dev/null +++ b/codex-rs/tui/src/external_editor.rs @@ -0,0 +1,171 @@ +use std::env; +use std::fs; +use std::process::Stdio; + +use color_eyre::eyre::Report; +use color_eyre::eyre::Result; +use tempfile::Builder; +use thiserror::Error; +use tokio::process::Command; + +#[derive(Debug, Error)] +pub(crate) enum EditorError { + #[error("neither VISUAL nor EDITOR is set")] + MissingEditor, + #[cfg(not(windows))] + #[error("failed to parse editor command")] + ParseFailed, + #[error("editor command is empty")] + EmptyCommand, +} + +/// Tries to resolve the full path to a Windows program, respecting PATH + PATHEXT. +/// Falls back to the original program name if resolution fails. +#[cfg(windows)] +fn resolve_windows_program(program: &str) -> std::path::PathBuf { + // On Windows, `Command::new("code")` will not resolve `code.cmd` shims on PATH. + // Use `which` so we respect PATH + PATHEXT (e.g., `code` -> `code.cmd`). + which::which(program).unwrap_or_else(|_| std::path::PathBuf::from(program)) +} + +/// Resolve the editor command from environment variables. +/// Prefers `VISUAL` over `EDITOR`. +pub(crate) fn resolve_editor_command() -> std::result::Result, EditorError> { + let raw = env::var("VISUAL") + .or_else(|_| env::var("EDITOR")) + .map_err(|_| EditorError::MissingEditor)?; + let parts = { + #[cfg(windows)] + { + winsplit::split(&raw) + } + #[cfg(not(windows))] + { + shlex::split(&raw).ok_or(EditorError::ParseFailed)? + } + }; + if parts.is_empty() { + return Err(EditorError::EmptyCommand); + } + Ok(parts) +} + +/// Write `seed` to a temp file, launch the editor command, and return the updated content. +pub(crate) async fn run_editor(seed: &str, editor_cmd: &[String]) -> Result { + if editor_cmd.is_empty() { + return Err(Report::msg("editor command is empty")); + } + + // Convert to TempPath immediately so no file handle stays open on Windows. + let temp_path = Builder::new().suffix(".md").tempfile()?.into_temp_path(); + fs::write(&temp_path, seed)?; + + let mut cmd = { + #[cfg(windows)] + { + // handles .cmd/.bat shims + Command::new(resolve_windows_program(&editor_cmd[0])) + } + #[cfg(not(windows))] + { + Command::new(&editor_cmd[0]) + } + }; + if editor_cmd.len() > 1 { + cmd.args(&editor_cmd[1..]); + } + let status = cmd + .arg(&temp_path) + .stdin(Stdio::inherit()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()) + .status() + .await?; + + if !status.success() { + return Err(Report::msg(format!("editor exited with status {status}"))); + } + + let contents = fs::read_to_string(&temp_path)?; + Ok(contents) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use serial_test::serial; + #[cfg(unix)] + use tempfile::tempdir; + + struct EnvGuard { + visual: Option, + editor: Option, + } + + impl EnvGuard { + fn new() -> Self { + Self { + visual: env::var("VISUAL").ok(), + editor: env::var("EDITOR").ok(), + } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + restore_env("VISUAL", self.visual.take()); + restore_env("EDITOR", self.editor.take()); + } + } + + fn restore_env(key: &str, value: Option) { + match value { + Some(val) => unsafe { env::set_var(key, val) }, + None => unsafe { env::remove_var(key) }, + } + } + + #[test] + #[serial] + fn resolve_editor_prefers_visual() { + let _guard = EnvGuard::new(); + unsafe { + env::set_var("VISUAL", "vis"); + env::set_var("EDITOR", "ed"); + } + let cmd = resolve_editor_command().unwrap(); + assert_eq!(cmd, vec!["vis".to_string()]); + } + + #[test] + #[serial] + fn resolve_editor_errors_when_unset() { + let _guard = EnvGuard::new(); + unsafe { + env::remove_var("VISUAL"); + env::remove_var("EDITOR"); + } + assert!(matches!( + resolve_editor_command(), + Err(EditorError::MissingEditor) + )); + } + + #[tokio::test] + #[cfg(unix)] + async fn run_editor_returns_updated_content() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempdir().unwrap(); + let script_path = dir.path().join("edit.sh"); + fs::write(&script_path, "#!/bin/sh\nprintf \"edited\" > \"$1\"\n").unwrap(); + let mut perms = fs::metadata(&script_path).unwrap().permissions(); + perms.set_mode(0o755); + fs::set_permissions(&script_path, perms).unwrap(); + + let cmd = vec![script_path.to_string_lossy().to_string()]; + let result = run_editor("seed", &cmd).await.unwrap(); + assert_eq!(result, "edited".to_string()); + } +} diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index b18754cab5..5f34c6f62f 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -47,6 +47,7 @@ pub mod custom_terminal; mod diff_render; mod exec_cell; mod exec_command; +mod external_editor; mod file_search; mod frames; mod get_git_diff; diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 0770b76648..e35e192a55 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -1,4 +1,5 @@ use std::fmt; +use std::future::Future; use std::io::IsTerminal; use std::io::Result; use std::io::Stdout; @@ -118,18 +119,86 @@ impl Command for DisableAlternateScroll { } } -/// Restore the terminal to its original state. -/// Inverse of `set_modes`. -pub fn restore() -> Result<()> { +fn restore_common(should_disable_raw_mode: bool) -> Result<()> { // Pop may fail on platforms that didn't support the push; ignore errors. let _ = execute!(stdout(), PopKeyboardEnhancementFlags); execute!(stdout(), DisableBracketedPaste)?; let _ = execute!(stdout(), DisableFocusChange); - disable_raw_mode()?; + if should_disable_raw_mode { + disable_raw_mode()?; + } let _ = execute!(stdout(), crossterm::cursor::Show); Ok(()) } +/// Restore the terminal to its original state. +/// Inverse of `set_modes`. +pub fn restore() -> Result<()> { + let should_disable_raw_mode = true; + restore_common(should_disable_raw_mode) +} + +/// Restore the terminal to its original state, but keep raw mode enabled. +pub fn restore_keep_raw() -> Result<()> { + let should_disable_raw_mode = false; + restore_common(should_disable_raw_mode) +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RestoreMode { + #[allow(dead_code)] + Full, // Fully restore the terminal (disables raw mode). + KeepRaw, // Restore the terminal but keep raw mode enabled. +} + +impl RestoreMode { + fn restore(self) -> Result<()> { + match self { + RestoreMode::Full => restore(), + RestoreMode::KeepRaw => restore_keep_raw(), + } + } +} + +/// Flush the underlying stdin buffer to clear any input that may be buffered at the terminal level. +/// For example, clears any user input that occurred while the crossterm EventStream was dropped. +#[cfg(unix)] +fn flush_terminal_input_buffer() { + // Safety: flushing the stdin queue is safe and does not move ownership. + let result = unsafe { libc::tcflush(libc::STDIN_FILENO, libc::TCIFLUSH) }; + if result != 0 { + let err = std::io::Error::last_os_error(); + tracing::warn!("failed to tcflush stdin: {err}"); + } +} + +/// Flush the underlying stdin buffer to clear any input that may be buffered at the terminal level. +/// For example, clears any user input that occurred while the crossterm EventStream was dropped. +#[cfg(windows)] +fn flush_terminal_input_buffer() { + use windows_sys::Win32::Foundation::GetLastError; + use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; + use windows_sys::Win32::System::Console::FlushConsoleInputBuffer; + use windows_sys::Win32::System::Console::GetStdHandle; + use windows_sys::Win32::System::Console::STD_INPUT_HANDLE; + + let handle = unsafe { GetStdHandle(STD_INPUT_HANDLE) }; + if handle == INVALID_HANDLE_VALUE || handle == 0 { + let err = unsafe { GetLastError() }; + tracing::warn!("failed to get stdin handle for flush: error {err}"); + return; + } + + let result = unsafe { FlushConsoleInputBuffer(handle) }; + if result == 0 { + let err = unsafe { GetLastError() }; + tracing::warn!("failed to flush stdin buffer: error {err}"); + } +} + +#[cfg(not(any(unix, windows)))] +pub(crate) fn flush_terminal_input_buffer() {} + /// Initialize the terminal (inline viewport; history stays in normal scrollback) pub fn init() -> Result { if !stdin().is_terminal() { @@ -215,18 +284,60 @@ impl Tui { self.enhanced_keys_supported } - // todo(sayan) unused for now; intend to use to enable opening external editors - #[allow(unused)] + pub fn is_alt_screen_active(&self) -> bool { + self.alt_screen_active.load(Ordering::Relaxed) + } + + // Drop crossterm EventStream to avoid stdin conflicts with other processes. pub fn pause_events(&mut self) { self.event_broker.pause_events(); } - // todo(sayan) unused for now; intend to use to enable opening external editors - #[allow(unused)] + // Resume crossterm EventStream to resume stdin polling. + // Inverse of `pause_events`. pub fn resume_events(&mut self) { self.event_broker.resume_events(); } + /// Temporarily restore terminal state to run an external interactive program `f`. + /// + /// This pauses crossterm's stdin polling by dropping the underlying event stream, restores + /// terminal modes (optionally keeping raw mode enabled), then re-applies Codex TUI modes and + /// flushes pending stdin input before resuming events. + pub async fn with_restored(&mut self, mode: RestoreMode, f: F) -> R + where + F: FnOnce() -> Fut, + Fut: Future, + { + // Pause crossterm events to avoid stdin conflicts with external program `f`. + self.pause_events(); + + // Leave alt screen if active to avoid conflicts with external program `f`. + let was_alt_screen = self.is_alt_screen_active(); + if was_alt_screen { + let _ = self.leave_alt_screen(); + } + + if let Err(err) = mode.restore() { + tracing::warn!("failed to restore terminal modes before external program: {err}"); + } + + let output = f().await; + + if let Err(err) = set_modes() { + tracing::warn!("failed to re-enable terminal modes after external program: {err}"); + } + // After the external program `f` finishes, reset terminal state and flush any buffered keypresses. + flush_terminal_input_buffer(); + + if was_alt_screen { + let _ = self.enter_alt_screen(); + } + + self.resume_events(); + output + } + /// Emit a desktop notification now if the terminal is unfocused. /// Returns true if a notification was posted. pub fn notify(&mut self, message: impl AsRef) -> bool { From 7d0c5c7bd5da912b7a0bd826f4ebd41361391344 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Mon, 22 Dec 2025 15:24:52 -0800 Subject: [PATCH 2/5] fix(tui2): copy transcript selection outside viewport (#8449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copy now operates on the full logical selection range (anchor..head), not just the visible viewport, so selections that include offscreen lines copy the expected text. Selection extraction is factored into `transcript_selection` to make the logic easier to test and reason about. It reconstructs the wrapped visual transcript, renders each wrapped line into a 1-row offscreen Buffer, and reads the selected cells. This keeps clipboard text aligned with what is rendered (gutter, indentation, wrapping). Additional behavior: - Skip continuation cells for wide glyphs (e.g. CJK) so copied text does not include spurious spaces like "コ X". - Avoid copying right-margin padding spaces. Manual tested performed: - "tell me a story" a few times - scroll up, select text, scroll down, copy text - confirm copied text is what you expect --- .../tui2/docs/tui_viewport_and_history.md | 23 +- codex-rs/tui2/src/app.rs | 213 ++------ codex-rs/tui2/src/lib.rs | 1 + codex-rs/tui2/src/transcript_selection.rs | 463 ++++++++++++++++++ 4 files changed, 525 insertions(+), 175 deletions(-) create mode 100644 codex-rs/tui2/src/transcript_selection.rs diff --git a/codex-rs/tui2/docs/tui_viewport_and_history.md b/codex-rs/tui2/docs/tui_viewport_and_history.md index cac56877db..d8cefedfe4 100644 --- a/codex-rs/tui2/docs/tui_viewport_and_history.md +++ b/codex-rs/tui2/docs/tui_viewport_and_history.md @@ -183,10 +183,12 @@ Mouse interaction is a first‑class part of the new design: that we use for bullets/prefixes. - **Copy.** - - When the user triggers copy, the TUI re‑renders just the transcript region off‑screen using the - same wrapping as the visible view. - - It then walks the selected lines and columns in that off‑screen buffer to reconstruct the exact - text region the user highlighted (including internal spaces and empty lines). + - When the user triggers copy, the TUI reconstructs the same wrapped transcript lines used for + on-screen rendering. + - It then walks the content-relative selection range (even if the selection extends outside the + current viewport) and re-renders each selected visual line into a 1-row offscreen buffer to + reconstruct the exact text region the user highlighted (including internal spaces and empty + lines, while skipping wide-glyph continuation cells and right-margin padding). - That text is sent to the system clipboard and a status footer indicates success or failure. Because scrolling, selection, and copy all operate on the same flattened transcript representation, @@ -392,8 +394,8 @@ The main app struct (`codex-rs/tui2/src/app.rs`) tracks the transcript and viewp - `transcript_cells: Vec>` – the logical history. - `transcript_scroll: TranscriptScroll` – whether the viewport is pinned to the bottom or anchored at a specific cell/line pair. -- `transcript_selection: TranscriptSelection` – a selection expressed in screen coordinates over - the flattened transcript region. +- `transcript_selection: TranscriptSelection` – a selection expressed in content-relative + coordinates over the flattened, wrapped transcript (line index + column). - `transcript_view_top` / `transcript_total_lines` – the current viewport’s top line index and total number of wrapped lines for the inline transcript area. @@ -410,8 +412,9 @@ Streaming wrapping details live in `codex-rs/tui2/docs/streaming_wrapping_design Mouse handling lives in `App::handle_mouse_event`, keyboard scrolling in `App::handle_key_event`, selection rendering in `App::apply_transcript_selection`, and copy in -`App::copy_transcript_selection` plus `codex-rs/tui2/src/clipboard_copy.rs`. Scroll/selection UI -state is forwarded through `ChatWidget::set_transcript_ui_state`, +`App::copy_transcript_selection` plus `codex-rs/tui2/src/transcript_selection.rs` and +`codex-rs/tui2/src/clipboard_copy.rs`. Scroll/selection UI state is forwarded through +`ChatWidget::set_transcript_ui_state`, `BottomPane::set_transcript_ui_state`, and `ChatComposer::footer_props`, with footer text assembled in `codex-rs/tui2/src/bottom_pane/footer.rs`. @@ -435,6 +438,8 @@ feedback are already implemented: probe data (see `codex-rs/tui2/docs/scroll_input_model.md`). - While a selection is active, streaming stops “follow latest output” so the selection remains stable, and follow mode resumes after the selection is cleared. +- Copy operates on the full selection range (including offscreen lines), using the same wrapping as + on-screen rendering. ### 10.2 Roadmap (prioritized) @@ -449,8 +454,6 @@ Vim) behavior as we can while still owning the viewport. input (avoid redraw/event-loop backlog that makes scrolling feel “janky”). - **Mouse event bounds.** Ignore mouse events outside the transcript region so clicks in the composer/footer don’t start or mutate transcript selection state. -- **Copy includes offscreen lines.** Make copy operate on the full selection range even when part (or - all) of the selection is outside the current viewport. - **Copy fidelity.** Preserve meaningful indentation (especially code blocks), treat soft-wrapped prose as a single logical line when copying, and copy markdown _source_ (including backticks and heading markers) even if we render it differently. diff --git a/codex-rs/tui2/src/app.rs b/codex-rs/tui2/src/app.rs index 9175b7d24a..a290433e07 100644 --- a/codex-rs/tui2/src/app.rs +++ b/codex-rs/tui2/src/app.rs @@ -17,6 +17,9 @@ use crate::pager_overlay::Overlay; use crate::render::highlight::highlight_bash_to_lines; use crate::render::renderable::Renderable; use crate::resume_picker::ResumeSelection; +use crate::transcript_selection::TRANSCRIPT_GUTTER_COLS; +use crate::transcript_selection::TranscriptSelection; +use crate::transcript_selection::TranscriptSelectionPoint; use crate::tui; use crate::tui::TuiEvent; use crate::tui::scrolling::MouseScrollState; @@ -359,28 +362,6 @@ pub(crate) struct App { skip_world_writable_scan_once: bool, } -/// Content-relative selection within the inline transcript viewport. -/// -/// Selection endpoints are expressed in terms of flattened, wrapped transcript -/// line indices and columns, so the highlight tracks logical conversation -/// content even when the viewport scrolls or the terminal is resized. -#[derive(Debug, Clone, Copy, Default)] -struct TranscriptSelection { - anchor: Option, - head: Option, -} - -/// A single endpoint of a transcript selection. -/// -/// `line_index` is an index into the flattened wrapped transcript lines, and -/// `column` is a zero-based column offset within that visual line, counted from -/// the first content column to the right of the transcript gutter. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -struct TranscriptSelectionPoint { - line_index: usize, - column: u16, -} - impl App { async fn shutdown_current_conversation(&mut self) { if let Some(conversation_id) = self.chat_widget.conversation_id() { @@ -889,7 +870,7 @@ impl App { width, height: transcript_height, }; - let base_x = transcript_area.x.saturating_add(2); + let base_x = transcript_area.x.saturating_add(TRANSCRIPT_GUTTER_COLS); let max_x = transcript_area.right().saturating_sub(1); // Treat the transcript as the only interactive region for transcript selection. @@ -952,8 +933,7 @@ impl App { clamped_x, clamped_y, ) { - self.transcript_selection.anchor = Some(point); - self.transcript_selection.head = Some(point); + self.transcript_selection = TranscriptSelection::new(point, point); tui.frame_requester().schedule_frame(); } } @@ -1281,16 +1261,10 @@ impl App { return; } - let base_x = area.x.saturating_add(2); + let base_x = area.x.saturating_add(TRANSCRIPT_GUTTER_COLS); let max_x = area.right().saturating_sub(1); - let mut start = anchor; - let mut end = head; - if (end.line_index < start.line_index) - || (end.line_index == start.line_index && end.column < start.column) - { - std::mem::swap(&mut start, &mut end); - } + let (start, end) = crate::transcript_selection::ordered_endpoints(anchor, head); let visible_start = self.transcript_view_top; let visible_end = self @@ -1365,15 +1339,13 @@ impl App { /// indices and columns, and this method reconstructs the same wrapped /// transcript used for on-screen rendering so the copied text closely /// matches the highlighted region. + /// + /// Important: copy operates on the selection's full content-relative range, + /// not just the current viewport. A selection can extend outside the visible + /// region (for example, by scrolling after selecting, or by selecting while + /// autoscrolling), and we still want the clipboard payload to reflect the + /// entire selected transcript. fn copy_transcript_selection(&mut self, tui: &tui::Tui) { - let (anchor, head) = match ( - self.transcript_selection.anchor, - self.transcript_selection.head, - ) { - (Some(a), Some(h)) if a != h => (a, h), - _ => return, - }; - let size = tui.terminal.last_known_screen_size; let width = size.width; let height = size.height; @@ -1391,132 +1363,13 @@ impl App { return; } - let transcript_area = Rect { - x: 0, - y: 0, - width, - height: transcript_height, + let (lines, _) = Self::build_transcript_lines(&self.transcript_cells, width); + let Some(text) = + crate::transcript_selection::selection_text(&lines, self.transcript_selection, width) + else { + return; }; - let cells = self.transcript_cells.clone(); - let (lines, _) = Self::build_transcript_lines(&cells, transcript_area.width); - if lines.is_empty() { - return; - } - - let wrapped = crate::wrapping::word_wrap_lines_borrowed( - &lines, - transcript_area.width.max(1) as usize, - ); - let total_lines = wrapped.len(); - if total_lines == 0 { - return; - } - - let max_visible = transcript_area.height as usize; - let visible_start = self - .transcript_view_top - .min(total_lines.saturating_sub(max_visible)); - let visible_end = std::cmp::min(visible_start + max_visible, total_lines); - - let mut buf = Buffer::empty(transcript_area); - Clear.render_ref(transcript_area, &mut buf); - - for (row_index, line_index) in (visible_start..visible_end).enumerate() { - let row_area = Rect { - x: transcript_area.x, - y: transcript_area.y + row_index as u16, - width: transcript_area.width, - height: 1, - }; - wrapped[line_index].render_ref(row_area, &mut buf); - } - - let base_x = transcript_area.x.saturating_add(2); - let max_x = transcript_area.right().saturating_sub(1); - - let mut start = anchor; - let mut end = head; - if (end.line_index < start.line_index) - || (end.line_index == start.line_index && end.column < start.column) - { - std::mem::swap(&mut start, &mut end); - } - - let mut lines_out: Vec = Vec::new(); - - for (row_index, line_index) in (visible_start..visible_end).enumerate() { - if line_index < start.line_index || line_index > end.line_index { - continue; - } - - let y = transcript_area.y + row_index as u16; - - let line_start_col = if line_index == start.line_index { - start.column - } else { - 0 - }; - let line_end_col = if line_index == end.line_index { - end.column - } else { - max_x.saturating_sub(base_x) - }; - - let row_sel_start = base_x.saturating_add(line_start_col); - let row_sel_end = base_x.saturating_add(line_end_col).min(max_x); - - if row_sel_start > row_sel_end { - continue; - } - - let mut first_text_x = None; - let mut last_text_x = None; - for x in base_x..=max_x { - let cell = &buf[(x, y)]; - if cell.symbol() != " " { - if first_text_x.is_none() { - first_text_x = Some(x); - } - last_text_x = Some(x); - } - } - - let (text_start, text_end) = match (first_text_x, last_text_x) { - // Treat indentation spaces as part of the copyable region by - // starting from the first content column to the right of the - // transcript gutter, but still clamp to the last non-space - // glyph so trailing padding is not included. - (Some(_), Some(e)) => (base_x, e), - _ => { - lines_out.push(String::new()); - continue; - } - }; - - let from_x = row_sel_start.max(text_start); - let to_x = row_sel_end.min(text_end); - if from_x > to_x { - continue; - } - - let mut line_text = String::new(); - for x in from_x..=to_x { - let cell = &buf[(x, y)]; - let symbol = cell.symbol(); - if !symbol.is_empty() { - line_text.push_str(symbol); - } - } - - lines_out.push(line_text); - } - - if lines_out.is_empty() { - return; - } - - let text = lines_out.join("\n"); if let Err(err) = clipboard_copy::copy_text(text) { tracing::error!(error = %err, "failed to copy selection to clipboard"); } @@ -2438,6 +2291,36 @@ mod tests { )); } + #[tokio::test] + async fn transcript_selection_copy_includes_offscreen_lines() { + let mut app = make_test_app().await; + app.transcript_cells = vec![Arc::new(AgentMessageCell::new( + vec![ + Line::from("one"), + Line::from("two"), + Line::from("three"), + Line::from("four"), + ], + true, + ))]; + + app.transcript_view_top = 2; + app.transcript_selection.anchor = Some(TranscriptSelectionPoint { + line_index: 0, + column: 0, + }); + app.transcript_selection.head = Some(TranscriptSelectionPoint { + line_index: 3, + column: u16::MAX, + }); + + let (lines, _) = App::build_transcript_lines(&app.transcript_cells, 40); + let text = + crate::transcript_selection::selection_text(&lines, app.transcript_selection, 40) + .unwrap(); + assert_eq!(text, "one\ntwo\nthree\nfour"); + } + #[tokio::test] async fn model_migration_prompt_respects_hide_flag_and_self_target() { let mut seen = BTreeMap::new(); diff --git a/codex-rs/tui2/src/lib.rs b/codex-rs/tui2/src/lib.rs index e31194b5b8..ce4f6d7b60 100644 --- a/codex-rs/tui2/src/lib.rs +++ b/codex-rs/tui2/src/lib.rs @@ -77,6 +77,7 @@ mod style; mod terminal_palette; mod text_formatting; mod tooltips; +mod transcript_selection; mod tui; mod ui_consts; pub mod update_action; diff --git a/codex-rs/tui2/src/transcript_selection.rs b/codex-rs/tui2/src/transcript_selection.rs new file mode 100644 index 0000000000..8967180b3d --- /dev/null +++ b/codex-rs/tui2/src/transcript_selection.rs @@ -0,0 +1,463 @@ +//! Transcript selection helpers. +//! +//! This module defines a content-relative selection model for the inline chat +//! transcript and utilities for extracting the selected region as plain text. +//! Selection endpoints are expressed in terms of flattened, wrapped transcript +//! line indices and columns so they remain stable across scrolling and +//! reflowing when the terminal is resized. +//! +//! Copy uses offscreen rendering into a 1-row `ratatui::Buffer` per visual line +//! to match on-screen glyph layout (including indentation/prefixes) while +//! skipping the transcript gutter. + +use itertools::Itertools as _; +use ratatui::prelude::*; +use ratatui::widgets::WidgetRef; +use unicode_width::UnicodeWidthStr; + +/// Number of columns reserved for the transcript gutter before the copyable +/// transcript text begins. +pub(crate) const TRANSCRIPT_GUTTER_COLS: u16 = 2; + +/// Content-relative selection within the inline transcript viewport. +/// +/// Selection endpoints are expressed in terms of flattened, wrapped transcript +/// line indices and columns, so the highlight tracks logical conversation +/// content even when the viewport scrolls or the terminal is resized. +#[derive(Debug, Clone, Copy, Default)] +pub(crate) struct TranscriptSelection { + /// The selection anchor (fixed start) in transcript coordinates. + pub(crate) anchor: Option, + /// The selection head (moving end) in transcript coordinates. + pub(crate) head: Option, +} + +impl TranscriptSelection { + /// Create an active selection with both endpoints set. + pub(crate) fn new( + anchor: impl Into, + head: impl Into, + ) -> Self { + Self { + anchor: Some(anchor.into()), + head: Some(head.into()), + } + } +} + +/// A single endpoint of a transcript selection. +/// +/// `line_index` is an index into the flattened wrapped transcript lines, and +/// `column` is a zero-based column offset within that visual line, counted from +/// the first content column to the right of the transcript gutter. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub(crate) struct TranscriptSelectionPoint { + /// Index into the flattened wrapped transcript lines. + pub(crate) line_index: usize, + /// Zero-based column offset within the wrapped line, relative to the first + /// content column to the right of the transcript gutter. + pub(crate) column: u16, +} + +impl TranscriptSelectionPoint { + /// Create a selection endpoint at a given wrapped line index and column. + pub(crate) const fn new(line_index: usize, column: u16) -> Self { + Self { line_index, column } + } +} + +impl From<(usize, u16)> for TranscriptSelectionPoint { + /// Convert from `(line_index, column)`. + fn from((line_index, column): (usize, u16)) -> Self { + Self::new(line_index, column) + } +} + +/// Extract the full transcript selection as plain text. +/// +/// This intentionally does *not* use viewport state. Instead it: +/// +/// - Applies the same word-wrapping used for on-screen rendering, producing +/// flattened "visual" lines. +/// - Renders each selected visual line into a 1-row offscreen `Buffer` and +/// extracts the selected character cells from that buffer. +/// +/// Using the rendered buffer (instead of slicing the source strings) keeps copy +/// semantics aligned with what the user sees on screen, including: +/// +/// - Prefixes / indentation introduced during rendering (e.g. list markers). +/// - The transcript gutter: selection columns are defined relative to the +/// first content column to the right of the gutter (`base_x = +/// TRANSCRIPT_GUTTER_COLS`). +/// - Multi-cell glyph rendering decisions made by the backend. +/// +/// Notes: +/// +/// - Trailing padding to the right margin is not included; we clamp each line +/// to the last non-space glyph to avoid copying a full-width block of spaces. +/// - `TranscriptSelectionPoint::column` can be arbitrarily large (e.g. +/// `u16::MAX` when dragging to the right edge); we clamp to the rendered line +/// width so "copy to end of line" behaves naturally. +pub(crate) fn selection_text( + lines: &[Line<'static>], + selection: TranscriptSelection, + width: u16, +) -> Option { + let (anchor, head) = selection.anchor.zip(selection.head)?; + if anchor == head { + return None; + } + + let (start, end) = ordered_endpoints(anchor, head); + let wrapped = wrap_transcript_lines(lines, width)?; + let ctx = RenderContext::new(width)?; + + let total_lines = wrapped.len(); + if start.line_index >= total_lines { + return None; + } + + // If the selection ends beyond the last wrapped line, clamp it so selection + // behaves like "copy through the end" rather than returning no text. + let (end_line_index, end_is_clamped) = clamp_end_line(end.line_index, total_lines)?; + + let mut buf = Buffer::empty(ctx.area); + let mut lines_out: Vec = Vec::new(); + + for (line_index, line) in wrapped + .iter() + .enumerate() + .take(end_line_index + 1) + .skip(start.line_index) + { + buf.reset(); + line.render_ref(ctx.area, &mut buf); + + let Some((row_sel_start, row_sel_end)) = + ctx.selection_bounds_for_line(line_index, start, end, end_is_clamped) + else { + // Preserve row count/newlines within the selection even if this + // particular visual line produces no selected cells. + lines_out.push(String::new()); + continue; + }; + + let Some(content_end_x) = ctx.content_end_x(&buf) else { + // Preserve explicit blank lines (e.g., spacer rows) in the selection. + lines_out.push(String::new()); + continue; + }; + + let from_x = row_sel_start.max(ctx.base_x); + let to_x = row_sel_end.min(content_end_x); + if from_x > to_x { + // Preserve row count/newlines even when selection falls beyond the + // rendered content for this visual line. + lines_out.push(String::new()); + continue; + } + + lines_out.push(ctx.extract_text(&buf, from_x, to_x)); + } + + Some(lines_out.join("\n")) +} + +/// Return `(start, end)` with `start <= end` in transcript order. +pub(crate) fn ordered_endpoints( + anchor: TranscriptSelectionPoint, + head: TranscriptSelectionPoint, +) -> (TranscriptSelectionPoint, TranscriptSelectionPoint) { + if anchor <= head { + (anchor, head) + } else { + (head, anchor) + } +} + +/// Wrap transcript lines using the same algorithm as on-screen rendering. +/// +/// Returns `None` for invalid widths or when wrapping produces no visual lines. +fn wrap_transcript_lines<'a>(lines: &'a [Line<'static>], width: u16) -> Option>> { + if width == 0 || lines.is_empty() { + return None; + } + + let wrapped = crate::wrapping::word_wrap_lines_borrowed(lines, width.max(1) as usize); + (!wrapped.is_empty()).then_some(wrapped) +} + +/// Context for rendering a single wrapped transcript line into a 1-row buffer and +/// extracting selected cells. +#[derive(Debug, Clone, Copy)] +struct RenderContext { + /// One-row region used for offscreen rendering. + area: Rect, + /// X coordinate where copyable transcript content begins (gutter skipped). + base_x: u16, + /// Maximum X coordinate inside the render area (inclusive). + max_x: u16, + /// Maximum content-relative column (0-based) within the render area. + max_content_col: u16, +} + +impl RenderContext { + /// Create a 1-row render context for a given terminal width. + /// + /// Returns `None` when the width is too small to hold any copyable content + /// (e.g. the gutter consumes the entire row). + fn new(width: u16) -> Option { + if width == 0 { + return None; + } + + let area = Rect::new(0, 0, width, 1); + let base_x = area.x.saturating_add(TRANSCRIPT_GUTTER_COLS); + let max_x = area.right().saturating_sub(1); + if base_x > max_x { + return None; + } + + Some(Self { + area, + base_x, + max_x, + max_content_col: max_x.saturating_sub(base_x), + }) + } + + /// Compute the inclusive selection X range for this visual line. + /// + /// `start`/`end` columns are content-relative (0 starts at the first column + /// to the right of the transcript gutter). For the terminal line containing + /// the selection endpoint, this clamps the selection to that endpoint; for + /// intermediate lines it selects the whole line. + /// + /// If the selection end was clamped to the last available line (meaning the + /// logical selection extended beyond the rendered transcript), the final + /// line is treated as selecting through the end of that line. + fn selection_bounds_for_line( + &self, + line_index: usize, + start: TranscriptSelectionPoint, + end: TranscriptSelectionPoint, + end_is_clamped: bool, + ) -> Option<(u16, u16)> { + let line_start_col = if line_index == start.line_index { + start.column + } else { + 0 + }; + let line_end_col = if !end_is_clamped && line_index == end.line_index { + end.column + } else { + self.max_content_col + }; + + let row_sel_start = self.base_x.saturating_add(line_start_col); + let row_sel_end = self.base_x.saturating_add(line_end_col).min(self.max_x); + + (row_sel_start <= row_sel_end).then_some((row_sel_start, row_sel_end)) + } + + /// Find the last non-space glyph in the rendered content area. + /// + /// This is used to avoid copying right-margin padding when the rendered row + /// is shorter than the terminal width. + fn content_end_x(&self, buf: &Buffer) -> Option { + (self.base_x..=self.max_x) + .rev() + .find(|&x| buf[(x, 0)].symbol() != " ") + } + + /// Extract rendered cell contents from an inclusive `[from_x, to_x]` range. + /// + /// Note: terminals represent wide glyphs (e.g. CJK characters) using multiple + /// cells, but only the first cell contains the glyph's symbol. The remaining + /// cells are "continuation" cells that should not be copied as separate + /// characters. Ratatui marks those continuation cells as a single space in + /// the buffer, so we must explicitly skip `width - 1` following cells after + /// reading each rendered symbol to avoid producing output like `"コ X"`. + fn extract_text(&self, buf: &Buffer, from_x: u16, to_x: u16) -> String { + (from_x..=to_x) + .batching(|xs| { + let x = xs.next()?; + let symbol = buf[(x, 0)].symbol(); + for _ in 0..symbol.width().saturating_sub(1) { + xs.next(); + } + (!symbol.is_empty()).then_some(symbol) + }) + .join("") + } +} + +/// Clamp `end_line_index` to the last available line and report if it was clamped. +/// +/// Returns `None` when there are no wrapped lines. +fn clamp_end_line(end_line_index: usize, total_lines: usize) -> Option<(usize, bool)> { + if total_lines == 0 { + return None; + } + + let clamped = end_line_index.min(total_lines.saturating_sub(1)); + Some((clamped, clamped != end_line_index)) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn selection_text_returns_none_when_missing_endpoints() { + let lines = vec![Line::from(vec!["• ".into(), "Hello".into()])]; + let selection = TranscriptSelection::default(); + + assert_eq!(selection_text(&lines, selection, 40), None); + } + + #[test] + fn selection_text_returns_none_when_endpoints_equal() { + let lines = vec![Line::from(vec!["• ".into(), "Hello".into()])]; + let selection = TranscriptSelection::new((0, 2), (0, 2)); + + assert_eq!(selection_text(&lines, selection, 40), None); + } + + #[test] + fn selection_text_returns_none_for_empty_lines() { + let selection = TranscriptSelection::new((0, 0), (0, 1)); + + assert_eq!(selection_text(&[], selection, 40), None); + } + + #[test] + fn selection_text_returns_none_for_zero_width() { + let lines = vec![Line::from(vec!["• ".into(), "Hello".into()])]; + let selection = TranscriptSelection::new((0, 0), (0, 1)); + + assert_eq!(selection_text(&lines, selection, 0), None); + } + + #[test] + fn selection_text_returns_none_when_width_smaller_than_gutter() { + let lines = vec![Line::from(vec!["• ".into(), "Hello".into()])]; + let selection = TranscriptSelection::new((0, 0), (0, 1)); + + assert_eq!(selection_text(&lines, selection, 2), None); + } + + #[test] + fn selection_text_skips_gutter_prefix() { + let lines = vec![Line::from(vec!["• ".into(), "Hello".into()])]; + let selection = TranscriptSelection::new((0, 0), (0, 4)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "Hello"); + } + + #[test] + fn selection_text_selects_substring_single_line() { + let lines = vec![Line::from(vec!["• ".into(), "Hello world".into()])]; + let selection = TranscriptSelection::new((0, 6), (0, 10)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "world"); + } + + #[test] + fn selection_text_preserves_interior_spaces() { + let lines = vec![Line::from(vec!["• ".into(), "a b".into()])]; + let selection = TranscriptSelection::new((0, 0), (0, 3)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "a b"); + } + + #[test] + fn selection_text_skips_hidden_wide_glyph_cells() { + let lines = vec![Line::from(vec!["• ".into(), "コX".into()])]; + let selection = TranscriptSelection::new((0, 0), (0, 2)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "コX"); + } + + #[test] + fn selection_text_orders_reversed_endpoints() { + let lines = vec![Line::from(vec!["• ".into(), "Hello world".into()])]; + let selection = TranscriptSelection::new((0, 10), (0, 6)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "world"); + } + + #[test] + fn selection_text_selects_multiple_lines_with_partial_endpoints() { + let lines = vec![ + Line::from(vec!["• ".into(), "abcde".into()]), + Line::from(vec!["• ".into(), "fghij".into()]), + Line::from(vec!["• ".into(), "klmno".into()]), + ]; + let selection = TranscriptSelection::new((0, 2), (2, 2)); + + assert_eq!( + selection_text(&lines, selection, 40).unwrap(), + "cde\nfghij\nklm" + ); + } + + #[test] + fn selection_text_selects_to_end_of_line_for_large_column() { + let lines = vec![Line::from(vec!["• ".into(), "one".into()])]; + let selection = TranscriptSelection::new((0, 0), (0, u16::MAX)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "one"); + } + + #[test] + fn selection_text_includes_indentation_spaces() { + let lines = vec![Line::from(vec!["• ".into(), " ind".into()])]; + let selection = TranscriptSelection::new((0, 0), (0, 4)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), " ind"); + } + + #[test] + fn selection_text_preserves_empty_lines() { + let lines = vec![ + Line::from(vec!["• ".into(), "one".into()]), + Line::from("• "), + Line::from(vec!["• ".into(), "two".into()]), + ]; + let selection = TranscriptSelection::new((0, 0), (2, 2)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "one\n\ntwo"); + } + + #[test] + fn selection_text_clamps_end_line_index() { + let lines = vec![ + Line::from(vec!["• ".into(), "one".into()]), + Line::from(vec!["• ".into(), "two".into()]), + ]; + let selection = TranscriptSelection::new((0, 0), (100, u16::MAX)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "one\ntwo"); + } + + #[test] + fn selection_text_clamps_end_line_index_ignoring_end_column() { + let lines = vec![ + Line::from(vec!["• ".into(), "one".into()]), + Line::from(vec!["• ".into(), "two".into()]), + ]; + let selection = TranscriptSelection::new((0, 0), (100, 0)); + + assert_eq!(selection_text(&lines, selection, 40).unwrap(), "one\ntwo"); + } + + #[test] + fn selection_text_returns_none_when_start_line_out_of_range() { + let lines = vec![Line::from(vec!["• ".into(), "one".into()])]; + let selection = TranscriptSelection::new((100, 0), (101, 0)); + + assert_eq!(selection_text(&lines, selection, 40), None); + } +} From f6275a51429c2a072cdc90889f2c2b0712617abd Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Mon, 22 Dec 2025 15:59:30 -0800 Subject: [PATCH 3/5] chore(tui): include tracing targets in file logs (#8418) with_target(true) is the default for tracing-subscriber, but we previously disabled it for file output. Keep it enabled so we can selectively enable specific targets/events at runtime via RUST_LOG=..., and then grep by target/module in the log file during troubleshooting. before and after: image --- codex-rs/tui/src/lib.rs | 5 ++++- codex-rs/tui2/src/lib.rs | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5f34c6f62f..bce9a350f4 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -273,7 +273,10 @@ pub async fn run_main( let file_layer = tracing_subscriber::fmt::layer() .with_writer(non_blocking) - .with_target(false) + // `with_target(true)` is the default, but we previously disabled it for file output. + // Keep it enabled so we can selectively enable targets via `RUST_LOG=...` and then + // grep for a specific module/target while troubleshooting. + .with_target(true) .with_ansi(false) .with_span_events(tracing_subscriber::fmt::format::FmtSpan::FULL) .with_filter(env_filter()); diff --git a/codex-rs/tui2/src/lib.rs b/codex-rs/tui2/src/lib.rs index ce4f6d7b60..0e3c805fbb 100644 --- a/codex-rs/tui2/src/lib.rs +++ b/codex-rs/tui2/src/lib.rs @@ -279,7 +279,10 @@ pub async fn run_main( let file_layer = tracing_subscriber::fmt::layer() .with_writer(non_blocking) - .with_target(false) + // `with_target(true)` is the default, but we previously disabled it for file output. + // Keep it enabled so we can selectively enable targets via `RUST_LOG=...` and then + // grep for a specific module/target while troubleshooting. + .with_target(true) .with_ansi(false) .with_span_events(tracing_subscriber::fmt::format::FmtSpan::CLOSE) .with_filter(env_filter()); From 14dbd0610a7a43fc716d394044c9f20ab9721cc6 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 22 Dec 2025 16:40:26 -0800 Subject: [PATCH 4/5] chore: include User layer in ConfigLayerStack even if config.toml is empty (#8456) This is necessary so that `$CODEX_HOME/skills` and `$CODEX_HOME/rules` still get loaded even if `$CODEX_HOME/config.toml` does not exist. See #8453. For now, it is possible to omit this layer when creating a dummy `ConfigLayerStack` in a test. We can revisit that later, if it turns out to be the right thing to do. --- .../app-server-protocol/src/protocol/v2.rs | 2 ++ codex-rs/core/src/config_loader/mod.rs | 20 +++++++++++------- codex-rs/core/src/config_loader/tests.rs | 21 ++++++++++++++++--- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 7ba7cc04b0..7a360cee20 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -237,6 +237,8 @@ pub enum ConfigLayerSource { #[serde(rename_all = "camelCase")] #[ts(rename_all = "camelCase")] User { + /// This is the path to the user's config.toml file, though it is not + /// guaranteed to exist. file: AbsolutePathBuf, }, diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 4bc9fb9c56..7d98db7084 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -101,7 +101,7 @@ pub async fn load_config_layers_state( // exists, but is malformed, then this error should be propagated to the // user. let user_file = AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home)?; - match tokio::fs::read_to_string(&user_file).await { + let user_layer = match tokio::fs::read_to_string(&user_file).await { Ok(contents) => { let user_config: TomlValue = toml::from_str(&contents).map_err(|e| { io::Error::new( @@ -123,13 +123,18 @@ pub async fn load_config_layers_state( })?; let user_config = resolve_relative_paths_in_config_toml(user_config, user_config_parent)?; - layers.push(ConfigLayerEntry::new( - ConfigLayerSource::User { file: user_file }, - user_config, - )); + ConfigLayerEntry::new(ConfigLayerSource::User { file: user_file }, user_config) } Err(e) => { - if e.kind() != io::ErrorKind::NotFound { + if e.kind() == io::ErrorKind::NotFound { + // If there is no config.toml file, record an empty entry + // for this user layer, as this may still have subfolders + // that are significant in the overall ConfigLayerStack. + ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + TomlValue::Table(toml::map::Map::new()), + ) + } else { return Err(io::Error::new( e.kind(), format!( @@ -139,7 +144,8 @@ pub async fn load_config_layers_state( )); } } - } + }; + layers.push(user_layer); if let Some(cwd) = cwd { let mut merged_so_far = TomlValue::Table(toml::map::Map::new()); diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index 7160f8c10a..4efeeaa98c 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -88,9 +88,24 @@ async fn returns_empty_when_all_layers_missing() { ) .await .expect("load layers"); - assert!( - layers.get_user_layer().is_none(), - "no user layer when CODEX_HOME/config.toml does not exist" + let user_layer = layers + .get_user_layer() + .expect("expected a user layer even when CODEX_HOME/config.toml does not exist"); + assert_eq!( + &ConfigLayerEntry { + name: super::ConfigLayerSource::User { + file: AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, tmp.path()) + .expect("resolve user config.toml path") + }, + config: TomlValue::Table(toml::map::Map::new()), + version: version_for_toml(&TomlValue::Table(toml::map::Map::new())), + }, + user_layer, + ); + assert_eq!( + user_layer.config, + TomlValue::Table(toml::map::Map::new()), + "expected empty config for user layer when config.toml does not exist" ); let binding = layers.effective_config(); From 896ec2f5fff87d8610e4d2a536a6d3813cfb9b84 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 22 Dec 2025 16:41:15 -0800 Subject: [PATCH 5/5] feat: load ExecPolicyManager from ConfigLayerStack --- codex-rs/core/src/codex.rs | 2 +- codex-rs/core/src/config_loader/mod.rs | 1 + codex-rs/core/src/config_loader/state.rs | 30 ++++- codex-rs/core/src/exec_policy.rs | 142 +++++++++++++++++++---- codex-rs/exec-server/src/posix.rs | 16 ++- codex-rs/utils/absolute-path/src/lib.rs | 7 ++ 6 files changed, 175 insertions(+), 23 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index cf4c30f184..5e039b13a0 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -240,7 +240,7 @@ impl Codex { ) .await; - let exec_policy = ExecPolicyManager::load(&config.features, &config.codex_home) + let exec_policy = ExecPolicyManager::load(&config.features, &config.config_layer_stack) .await .map_err(|err| CodexErr::Fatal(format!("failed to load execpolicy: {err}")))?; diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 7d98db7084..be336bd056 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -28,6 +28,7 @@ pub use config_requirements::ConfigRequirements; pub use merge::merge_toml_values; pub use state::ConfigLayerEntry; pub use state::ConfigLayerStack; +pub use state::ConfigLayerStackOrdering; pub use state::LoaderOverrides; /// On Unix systems, load requirements from this file path, if present. diff --git a/codex-rs/core/src/config_loader/state.rs b/codex-rs/core/src/config_loader/state.rs index 5afa30080e..bd00e77247 100644 --- a/codex-rs/core/src/config_loader/state.rs +++ b/codex-rs/core/src/config_loader/state.rs @@ -50,6 +50,25 @@ impl ConfigLayerEntry { config: serde_json::to_value(&self.config).unwrap_or(JsonValue::Null), } } + + // Get the `.codex/` folder associated with this config layer, if any. + pub fn config_folder(&self) -> Option { + match &self.name { + ConfigLayerSource::Mdm { .. } => None, + ConfigLayerSource::System { .. } => None, + ConfigLayerSource::User { file } => file.parent(), + ConfigLayerSource::Project { dot_codex_folder } => Some(dot_codex_folder.clone()), + ConfigLayerSource::SessionFlags => None, + ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } => None, + ConfigLayerSource::LegacyManagedConfigTomlFromMdm => None, + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ConfigLayerStackOrdering { + LowestPrecedenceFirst, + HighestPrecedenceFirst, } #[derive(Debug, Clone, Default, PartialEq)] @@ -156,7 +175,16 @@ impl ConfigLayerStack { /// Returns the highest-precedence to lowest-precedence layers, so /// `ConfigLayerSource::SessionFlags` would be first, if present. pub fn layers_high_to_low(&self) -> Vec<&ConfigLayerEntry> { - self.layers.iter().rev().collect() + self.get_layers(ConfigLayerStackOrdering::HighestPrecedenceFirst) + } + + /// Returns the highest-precedence to lowest-precedence layers, so + /// `ConfigLayerSource::SessionFlags` would be first, if present. + pub fn get_layers(&self, ordering: ConfigLayerStackOrdering) -> Vec<&ConfigLayerEntry> { + match ordering { + ConfigLayerStackOrdering::HighestPrecedenceFirst => self.layers.iter().rev().collect(), + ConfigLayerStackOrdering::LowestPrecedenceFirst => self.layers.iter().collect(), + } } } diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 6bbde29d3e..a5d9e6a4af 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -6,6 +6,8 @@ use std::sync::Arc; use arc_swap::ArcSwap; use crate::command_safety::is_dangerous_command::requires_initial_appoval; +use crate::config_loader::ConfigLayerStack; +use crate::config_loader::ConfigLayerStackOrdering; use codex_execpolicy::AmendError; use codex_execpolicy::Decision; use codex_execpolicy::Error as ExecPolicyRuleError; @@ -94,9 +96,9 @@ impl ExecPolicyManager { pub(crate) async fn load( features: &Features, - codex_home: &Path, + config_stack: &ConfigLayerStack, ) -> Result { - let policy = load_exec_policy_for_features(features, codex_home).await?; + let policy = load_exec_policy_for_features(features, config_stack).await?; Ok(Self::new(Arc::new(policy))) } @@ -194,18 +196,28 @@ impl Default for ExecPolicyManager { async fn load_exec_policy_for_features( features: &Features, - codex_home: &Path, + config_stack: &ConfigLayerStack, ) -> Result { if !features.enabled(Feature::ExecPolicy) { Ok(Policy::empty()) } else { - load_exec_policy(codex_home).await + load_exec_policy(config_stack).await } } -pub async fn load_exec_policy(codex_home: &Path) -> Result { - let policy_dir = codex_home.join(RULES_DIR_NAME); - let policy_paths = collect_policy_files(&policy_dir).await?; +pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result { + // Iterate the layers in increasing order of precedence, adding the *.rules + // from each layer, so that higher-precedence layers can override + // rules defined in lower-precedence ones. + let mut policy_paths = Vec::new(); + for layer in config_stack.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst) { + if let Some(config_folder) = layer.config_folder() { + #[expect(clippy::expect_used)] + let policy_dir = config_folder.join(RULES_DIR_NAME).expect("safe join"); + let layer_policy_paths = collect_policy_files(&policy_dir).await?; + policy_paths.extend(layer_policy_paths); + } + } let mut parser = PolicyParser::new(); for policy_path in &policy_paths { @@ -226,11 +238,7 @@ pub async fn load_exec_policy(codex_home: &Path) -> Result Option { }) } -async fn collect_policy_files(dir: &Path) -> Result, ExecPolicyError> { - let mut read_dir = match fs::read_dir(dir).await { +async fn collect_policy_files(dir: impl AsRef) -> Result, ExecPolicyError> { + let mut read_dir = match fs::read_dir(dir.as_ref()).await { Ok(read_dir) => read_dir, Err(err) if err.kind() == ErrorKind::NotFound => return Ok(Vec::new()), Err(source) => { return Err(ExecPolicyError::ReadDir { - dir: dir.to_path_buf(), + dir: dir.as_ref().to_path_buf(), source, }); } @@ -320,7 +328,7 @@ async fn collect_policy_files(dir: &Path) -> Result, ExecPolicyErro .next_entry() .await .map_err(|source| ExecPolicyError::ReadDir { - dir: dir.to_path_buf(), + dir: dir.as_ref().to_path_buf(), source, })? { @@ -329,7 +337,7 @@ async fn collect_policy_files(dir: &Path) -> Result, ExecPolicyErro .file_type() .await .map_err(|source| ExecPolicyError::ReadDir { - dir: dir.to_path_buf(), + dir: dir.as_ref().to_path_buf(), source, })?; @@ -345,28 +353,51 @@ async fn collect_policy_files(dir: &Path) -> Result, ExecPolicyErro policy_paths.sort(); + tracing::debug!( + "loaded {} .rules files in {}", + policy_paths.len(), + dir.as_ref().display() + ); Ok(policy_paths) } #[cfg(test)] mod tests { use super::*; + use crate::config_loader::ConfigLayerEntry; + use crate::config_loader::ConfigLayerStack; + use crate::config_loader::ConfigRequirements; use crate::features::Feature; use crate::features::Features; + use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; + use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::fs; + use std::path::Path; use std::sync::Arc; use tempfile::tempdir; + use toml::Value as TomlValue; + + fn config_stack_for_dot_codex_folder(dot_codex_folder: &Path) -> ConfigLayerStack { + let dot_codex_folder = AbsolutePathBuf::from_absolute_path(dot_codex_folder) + .expect("absolute dot_codex_folder"); + let layer = ConfigLayerEntry::new( + ConfigLayerSource::Project { dot_codex_folder }, + TomlValue::Table(Default::default()), + ); + ConfigLayerStack::new(vec![layer], ConfigRequirements::default()).expect("ConfigLayerStack") + } #[tokio::test] async fn returns_empty_policy_when_feature_disabled() { let mut features = Features::with_defaults(); features.disable(Feature::ExecPolicy); let temp_dir = tempdir().expect("create temp dir"); + let config_stack = config_stack_for_dot_codex_folder(temp_dir.path()); - let manager = ExecPolicyManager::load(&features, temp_dir.path()) + let manager = ExecPolicyManager::load(&features, &config_stack) .await .expect("manager result"); let policy = manager.current(); @@ -400,6 +431,7 @@ mod tests { #[tokio::test] async fn loads_policies_from_policy_subdirectory() { let temp_dir = tempdir().expect("create temp dir"); + let config_stack = config_stack_for_dot_codex_folder(temp_dir.path()); let policy_dir = temp_dir.path().join(RULES_DIR_NAME); fs::create_dir_all(&policy_dir).expect("create policy dir"); fs::write( @@ -408,7 +440,7 @@ mod tests { ) .expect("write policy file"); - let policy = load_exec_policy(temp_dir.path()) + let policy = load_exec_policy(&config_stack) .await .expect("policy result"); let command = [vec!["rm".to_string()]]; @@ -427,13 +459,14 @@ mod tests { #[tokio::test] async fn ignores_policies_outside_policy_dir() { let temp_dir = tempdir().expect("create temp dir"); + let config_stack = config_stack_for_dot_codex_folder(temp_dir.path()); fs::write( temp_dir.path().join("root.rules"), r#"prefix_rule(pattern=["ls"], decision="prompt")"#, ) .expect("write policy file"); - let policy = load_exec_policy(temp_dir.path()) + let policy = load_exec_policy(&config_stack) .await .expect("policy result"); let command = [vec!["ls".to_string()]]; @@ -449,6 +482,75 @@ mod tests { ); } + #[tokio::test] + async fn loads_policies_from_multiple_config_layers() { + let user_dir = tempdir().expect("create user dir"); + let project_dir = tempdir().expect("create project dir"); + + let user_policy_dir = user_dir.path().join(RULES_DIR_NAME); + fs::create_dir_all(&user_policy_dir).expect("create user policy dir"); + fs::write( + user_policy_dir.join("user.rules"), + r#"prefix_rule(pattern=["rm"], decision="forbidden")"#, + ) + .expect("write user policy file"); + + let project_policy_dir = project_dir.path().join(RULES_DIR_NAME); + fs::create_dir_all(&project_policy_dir).expect("create project policy dir"); + fs::write( + project_policy_dir.join("project.rules"), + r#"prefix_rule(pattern=["ls"], decision="prompt")"#, + ) + .expect("write project policy file"); + + let user_config_toml = + AbsolutePathBuf::from_absolute_path(user_dir.path().join("config.toml")) + .expect("absolute user config.toml"); + let project_dot_codex_folder = + AbsolutePathBuf::from_absolute_path(project_dir.path()).expect("absolute project dir"); + let layers = vec![ + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: user_config_toml, + }, + TomlValue::Table(Default::default()), + ), + ConfigLayerEntry::new( + ConfigLayerSource::Project { + dot_codex_folder: project_dot_codex_folder, + }, + TomlValue::Table(Default::default()), + ), + ]; + let config_stack = + ConfigLayerStack::new(layers, ConfigRequirements::default()).expect("ConfigLayerStack"); + + let policy = load_exec_policy(&config_stack) + .await + .expect("policy result"); + + assert_eq!( + Evaluation { + decision: Decision::Forbidden, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: vec!["rm".to_string()], + decision: Decision::Forbidden + }], + }, + policy.check_multiple([vec!["rm".to_string()]].iter(), &|_| Decision::Allow) + ); + assert_eq!( + Evaluation { + decision: Decision::Prompt, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: vec!["ls".to_string()], + decision: Decision::Prompt + }], + }, + policy.check_multiple([vec!["ls".to_string()]].iter(), &|_| Decision::Allow) + ); + } + #[tokio::test] async fn evaluates_bash_lc_inner_commands() { let policy_src = r#" diff --git a/codex-rs/exec-server/src/posix.rs b/codex-rs/exec-server/src/posix.rs index ba481264e2..12d0055cdd 100644 --- a/codex-rs/exec-server/src/posix.rs +++ b/codex-rs/exec-server/src/posix.rs @@ -230,7 +230,21 @@ fn format_program_name(path: &Path, preserve_program_paths: bool) -> Option anyhow::Result { let codex_home = find_codex_home().context("failed to resolve codex_home for execpolicy")?; - codex_core::load_exec_policy(&codex_home) + + // TODO(mbolin): At a minimum, `cwd` should be configurable via + // `codex/sandbox-state/update` or some other custom MCP call. + let cwd = None; + let cli_overrides = Vec::new(); + let overrides = codex_core::config_loader::LoaderOverrides::default(); + let config_layer_stack = codex_core::config_loader::load_config_layers_state( + &codex_home, + cwd, + &cli_overrides, + overrides, + ) + .await?; + + codex_core::load_exec_policy(&config_layer_stack) .await .map_err(anyhow::Error::from) } diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 808e031836..2eb7d48758 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -43,6 +43,13 @@ impl AbsolutePathBuf { Self::resolve_path_against_base(path, &self.0) } + pub fn parent(&self) -> Option { + self.0.parent().map(|p| { + #[expect(clippy::expect_used)] + Self::from_absolute_path(p).expect("parent of AbsolutePathBuf must be absolute") + }) + } + pub fn as_path(&self) -> &Path { &self.0 }