diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index f7d24e1e0..d9d3b0c02 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -30,6 +30,9 @@ use codex_app_server_protocol::AdditionalPermissionProfile; use codex_app_server_protocol::AgentMessageDeltaNotification; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::ConfigWarningNotification; +use codex_app_server_protocol::FileChangeRequestApprovalParams; +use codex_app_server_protocol::FileUpdateChange; +use codex_app_server_protocol::ItemStartedNotification; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::McpServerStartupState; use codex_app_server_protocol::McpServerStatusUpdatedNotification; @@ -38,6 +41,7 @@ use codex_app_server_protocol::NetworkApprovalProtocol as AppServerNetworkApprov use codex_app_server_protocol::NetworkPolicyAmendment as AppServerNetworkPolicyAmendment; use codex_app_server_protocol::NetworkPolicyRuleAction as AppServerNetworkPolicyRuleAction; use codex_app_server_protocol::NonSteerableTurnKind as AppServerNonSteerableTurnKind; +use codex_app_server_protocol::PatchChangeKind; use codex_app_server_protocol::PermissionsRequestApprovalParams; use codex_app_server_protocol::RequestId as AppServerRequestId; use codex_app_server_protocol::ServerNotification; @@ -70,6 +74,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkApprovalContext; use codex_protocol::protocol::NetworkApprovalProtocol; use codex_protocol::protocol::RolloutItem; @@ -2522,6 +2527,75 @@ async fn inactive_thread_exec_approval_splits_shell_wrapped_command() { ); } +#[tokio::test] +async fn inactive_thread_file_change_approval_recovers_buffered_changes() { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let thread_id = ThreadId::new(); + app.enqueue_thread_notification( + thread_id, + ServerNotification::ItemStarted(ItemStartedNotification { + thread_id: thread_id.to_string(), + turn_id: "turn-approval".to_string(), + item: ThreadItem::FileChange { + id: "patch-approval".to_string(), + changes: vec![FileUpdateChange { + path: "README.md".to_string(), + kind: PatchChangeKind::Add, + diff: "hello\n".to_string(), + }], + status: codex_app_server_protocol::PatchApplyStatus::InProgress, + }, + }), + ) + .await + .expect("enqueue file change item"); + + let request = ServerRequest::FileChangeRequestApproval { + request_id: AppServerRequestId::Integer(9), + params: FileChangeRequestApprovalParams { + thread_id: thread_id.to_string(), + turn_id: "turn-approval".to_string(), + item_id: "patch-approval".to_string(), + reason: Some("command failed; retry without sandbox?".to_string()), + grant_root: None, + }, + }; + + let request = app + .interactive_request_for_thread_request(thread_id, &request) + .await + .expect("expected file change approval request"); + + let ThreadInteractiveRequest::Approval(ApprovalRequest::ApplyPatch { + changes, reason, .. + }) = &request + else { + panic!("expected apply-patch approval request"); + }; + assert_eq!( + changes, + &HashMap::from([( + PathBuf::from("README.md"), + FileChange::Add { + content: "hello\n".to_string(), + }, + )]) + ); + assert_eq!( + reason, + &Some("command failed; retry without sandbox?".to_string()) + ); + + app.push_thread_interactive_request(request); + let cell = match app_event_rx.try_recv() { + Ok(AppEvent::InsertHistoryCell(cell)) => cell, + other => panic!("expected patch preview history cell, saw {other:?}"), + }; + let rendered = lines_to_single_string(&cell.display_lines(/*width*/ 80)); + assert!(rendered.contains("• Added README.md (+1 -0)")); + assert!(rendered.contains("1 +hello")); +} + #[tokio::test] async fn inactive_thread_permissions_approval_preserves_file_system_permissions() { let app = make_test_app().await; diff --git a/codex-rs/tui/src/app/thread_events.rs b/codex-rs/tui/src/app/thread_events.rs index daf743b46..9bbb41fbd 100644 --- a/codex-rs/tui/src/app/thread_events.rs +++ b/codex-rs/tui/src/app/thread_events.rs @@ -157,6 +157,40 @@ impl ThreadEventStore { .collect() } + pub(super) fn file_change_changes( + &self, + turn_id: &str, + item_id: &str, + ) -> Option> { + self.buffer + .iter() + .rev() + .find_map(|event| match event { + ThreadBufferedEvent::Notification(ServerNotification::ItemStarted( + notification, + )) if turn_id_matches(turn_id, ¬ification.turn_id) => { + file_change_item_changes(¬ification.item, item_id) + } + ThreadBufferedEvent::Notification(ServerNotification::ItemCompleted( + notification, + )) if turn_id_matches(turn_id, ¬ification.turn_id) => { + file_change_item_changes(¬ification.item, item_id) + } + ThreadBufferedEvent::Request(_) + | ThreadBufferedEvent::Notification(_) + | ThreadBufferedEvent::HistoryEntryResponse(_) + | ThreadBufferedEvent::FeedbackSubmission(_) => None, + }) + .or_else(|| { + self.turns + .iter() + .rev() + .filter(|turn| turn_id_matches(turn_id, &turn.id)) + .flat_map(|turn| turn.items.iter().rev()) + .find_map(|item| file_change_item_changes(item, item_id)) + }) + } + pub(super) fn apply_thread_rollback(&mut self, response: &ThreadRollbackResponse) { self.turns = response.thread.turns.clone(); self.buffer.clear(); @@ -231,6 +265,20 @@ impl ThreadEventStore { } } +fn turn_id_matches(request_turn_id: &str, candidate_turn_id: &str) -> bool { + request_turn_id.is_empty() || request_turn_id == candidate_turn_id +} + +fn file_change_item_changes( + item: &ThreadItem, + item_id: &str, +) -> Option> { + match item { + ThreadItem::FileChange { id, changes, .. } if id == item_id => Some(changes.clone()), + _ => None, + } +} + #[derive(Debug)] pub(super) struct ThreadEventChannel { pub(super) sender: mpsc::Sender, diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 5f0f52c2c..345e560d4 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -194,6 +194,17 @@ impl App { store.session.as_ref().map(|session| session.cwd.clone()) } + async fn thread_file_change_changes( + &self, + thread_id: ThreadId, + turn_id: &str, + item_id: &str, + ) -> Option> { + let channel = self.thread_event_channels.get(&thread_id)?; + let store = channel.store.lock().await; + store.file_change_changes(turn_id, item_id) + } + pub(super) async fn interactive_request_for_thread_request( &self, thread_id: ThreadId, @@ -264,7 +275,11 @@ impl App { .thread_cwd(thread_id) .await .unwrap_or_else(|| self.config.cwd.clone()), - changes: HashMap::new(), + changes: self + .thread_file_change_changes(thread_id, ¶ms.turn_id, ¶ms.item_id) + .await + .map(crate::app_server_approval_conversions::file_update_changes_to_core) + .unwrap_or_default(), }), ), ServerRequest::McpServerElicitationRequest { request_id, params } => { @@ -311,6 +326,7 @@ impl App { pub(super) fn push_thread_interactive_request(&mut self, request: ThreadInteractiveRequest) { match request { ThreadInteractiveRequest::Approval(request) => { + self.render_inactive_patch_preview(&request); self.chat_widget.push_approval_request(request); } ThreadInteractiveRequest::McpServerElicitation(request) => { @@ -320,6 +336,23 @@ impl App { } } + fn render_inactive_patch_preview(&mut self, request: &ApprovalRequest) { + let ApprovalRequest::ApplyPatch { + thread_label, + cwd, + changes, + .. + } = request + else { + return; + }; + if thread_label.is_none() || changes.is_empty() { + return; + } + self.chat_widget + .add_to_history(history_cell::new_patch_event(changes.clone(), cwd)); + } + pub(super) async fn pending_inactive_thread_requests(&self) -> Vec<(ThreadId, ServerRequest)> { let channels: Vec<(ThreadId, Arc>)> = self .thread_event_channels diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs index a0d86db7d..894bd36ed 100644 --- a/codex-rs/tui/src/app_server_approval_conversions.rs +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -1,9 +1,14 @@ use codex_app_server_protocol::AdditionalNetworkPermissions; +use codex_app_server_protocol::FileUpdateChange; use codex_app_server_protocol::GrantedPermissionProfile; use codex_app_server_protocol::NetworkApprovalContext as AppServerNetworkApprovalContext; +use codex_app_server_protocol::PatchChangeKind; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkApprovalContext; use codex_protocol::protocol::NetworkApprovalProtocol; use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; +use std::collections::HashMap; +use std::path::PathBuf; pub(crate) fn network_approval_context_to_core( value: AppServerNetworkApprovalContext, @@ -38,21 +43,50 @@ pub(crate) fn granted_permission_profile_from_request( } } +pub(crate) fn file_update_changes_to_core( + changes: Vec, +) -> HashMap { + changes + .into_iter() + .map(|change| { + let path = PathBuf::from(change.path); + let file_change = match change.kind { + PatchChangeKind::Add => FileChange::Add { + content: change.diff, + }, + PatchChangeKind::Delete => FileChange::Delete { + content: change.diff, + }, + PatchChangeKind::Update { move_path } => FileChange::Update { + unified_diff: change.diff, + move_path, + }, + }; + (path, file_change) + }) + .collect() +} + #[cfg(test)] mod tests { + use super::file_update_changes_to_core; use super::granted_permission_profile_from_request; use super::network_approval_context_to_core; + use codex_app_server_protocol::FileUpdateChange; + use codex_app_server_protocol::PatchChangeKind; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::NetworkPermissions; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSpecialPath; + use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkApprovalContext; use codex_protocol::protocol::NetworkApprovalProtocol; use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; + use std::collections::HashMap; use std::path::PathBuf; fn absolute_path(path: &str) -> AbsolutePathBuf { @@ -73,6 +107,23 @@ mod tests { ); } + #[test] + fn converts_file_update_changes_to_core() { + assert_eq!( + file_update_changes_to_core(vec![FileUpdateChange { + path: "foo.txt".to_string(), + kind: PatchChangeKind::Add, + diff: "hello\n".to_string(), + }]), + HashMap::from([( + PathBuf::from("foo.txt"), + FileChange::Add { + content: "hello\n".to_string(), + }, + )]) + ); + } + #[test] fn converts_request_permissions_into_granted_permissions() { assert_eq!( diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 57d819e56..ce3b04b33 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -9,7 +9,6 @@ use crate::bottom_pane::CancellationEvent; use crate::bottom_pane::list_selection_view::ListSelectionView; use crate::bottom_pane::list_selection_view::SelectionItem; use crate::bottom_pane::list_selection_view::SelectionViewParams; -use crate::diff_render::DiffSummary; use crate::exec_command::strip_bash_lc_and_escape; use crate::history_cell; use crate::key_hint; @@ -633,8 +632,6 @@ fn build_header(request: &ApprovalRequest) -> Box { ApprovalRequest::ApplyPatch { thread_label, reason, - cwd, - changes, .. } => { let mut header: Vec> = Vec::new(); @@ -643,11 +640,13 @@ fn build_header(request: &ApprovalRequest) -> Box { "Thread: ".into(), thread_label.clone().bold(), ]))); - header.push(Box::new(Line::from(""))); } if let Some(reason) = reason && !reason.is_empty() { + if !header.is_empty() { + header.push(Box::new(Line::from(""))); + } header.push(Box::new( Paragraph::new(Line::from_iter([ "Reason: ".into(), @@ -655,9 +654,7 @@ fn build_header(request: &ApprovalRequest) -> Box { ])) .wrap(Wrap { trim: false }), )); - header.push(Box::new(Line::from(""))); } - header.push(DiffSummary::new(changes.clone(), cwd.clone()).into()); Box::new(ColumnRenderable::with(header)) } ApprovalRequest::McpElicitation { @@ -1556,6 +1553,32 @@ mod tests { ); } + #[test] + fn apply_patch_prompt_with_thread_label_omits_command_line() { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let mut changes = HashMap::new(); + changes.insert( + PathBuf::from("bug1.txt"), + FileChange::Add { + content: "one\ntwo\nthree\n".to_string(), + }, + ); + let request = ApprovalRequest::ApplyPatch { + thread_id: ThreadId::new(), + thread_label: Some("Banach [worker]".to_string()), + id: "test".to_string(), + reason: None, + cwd: absolute_path("/tmp"), + changes, + }; + let view = ApprovalOverlay::new(request, tx, Features::with_defaults()); + let rendered = render_overlay_lines(&view, /*width*/ 120); + assert!(rendered.contains("Thread: Banach [worker]")); + assert!(rendered.contains("o to open thread")); + assert!(!rendered.contains("$ apply_patch")); + } + #[test] fn network_exec_prompt_title_includes_host() { let (tx, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index f5454c2bf..8513fe881 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -47,6 +47,7 @@ use self::realtime::PendingSteerCompareKey; use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::app_command::AppCommand; use crate::app_event::RealtimeAudioDeviceKind; +use crate::app_server_approval_conversions::file_update_changes_to_core; use crate::app_server_approval_conversions::network_approval_context_to_core; use crate::app_server_session::ThreadSessionState; #[cfg(not(target_os = "linux"))] @@ -1774,36 +1775,6 @@ fn patch_approval_request_from_params( } } -fn app_server_patch_changes_to_core( - changes: Vec, -) -> HashMap { - changes - .into_iter() - .map(|change| { - let path = PathBuf::from(change.path); - let file_change = match change.kind { - codex_app_server_protocol::PatchChangeKind::Add => { - codex_protocol::protocol::FileChange::Add { - content: change.diff, - } - } - codex_app_server_protocol::PatchChangeKind::Delete => { - codex_protocol::protocol::FileChange::Delete { - content: change.diff, - } - } - codex_app_server_protocol::PatchChangeKind::Update { move_path } => { - codex_protocol::protocol::FileChange::Update { - unified_diff: change.diff, - move_path, - } - } - }; - (path, file_change) - }) - .collect() -} - fn app_server_collab_thread_id_to_core(thread_id: &str) -> Option { match ThreadId::from_string(thread_id) { Ok(thread_id) => Some(thread_id), @@ -6797,7 +6768,7 @@ impl ChatWidget { status, codex_app_server_protocol::PatchApplyStatus::Failed ), - changes: app_server_patch_changes_to_core(changes), + changes: file_update_changes_to_core(changes), status: match status { codex_app_server_protocol::PatchApplyStatus::Completed => { codex_protocol::protocol::PatchApplyStatus::Completed @@ -7356,7 +7327,7 @@ impl ChatWidget { call_id: id, turn_id: notification.turn_id, auto_approved: false, - changes: app_server_patch_changes_to_core(changes), + changes: file_update_changes_to_core(changes), }); } ThreadItem::McpToolCall { diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index e394605dc..8635b6668 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -1,18 +1,11 @@ --- -source: tui/src/chatwidget/tests.rs -expression: terminal.backend().vt100().screen().contents() +source: tui/src/chatwidget/tests/exec_flow.rs +expression: contents --- - - Would you like to make the following edits? Reason: The model wants to apply changes - README.md (+2 -0) - - 1 +hello - 2 +world - › 1. Yes, proceed (y) 2. Yes, and don't ask again for these files (a) 3. No, and tell Codex what to do differently (esc) diff --git a/codex-rs/tui/src/chatwidget/tests/app_server.rs b/codex-rs/tui/src/chatwidget/tests/app_server.rs index b9ec1d871..d4fa2df45 100644 --- a/codex-rs/tui/src/chatwidget/tests/app_server.rs +++ b/codex-rs/tui/src/chatwidget/tests/app_server.rs @@ -349,25 +349,6 @@ async fn live_app_server_command_execution_strips_shell_wrapper() { ); } -#[test] -fn app_server_patch_changes_to_core_preserves_diffs() { - let changes = app_server_patch_changes_to_core(vec![FileUpdateChange { - path: "foo.txt".to_string(), - kind: PatchChangeKind::Add, - diff: "hello\n".to_string(), - }]); - - assert_eq!( - changes, - HashMap::from([( - PathBuf::from("foo.txt"), - FileChange::Add { - content: "hello\n".to_string(), - }, - )]) - ); -} - #[tokio::test] async fn live_app_server_collab_wait_items_render_history() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index 84d7cd721..4d87914aa 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -1321,10 +1321,9 @@ async fn approval_modal_patch_snapshot() -> anyhow::Result<()> { terminal .draw(|f| chat.render(f.area(), f.buffer_mut())) .expect("draw patch approval modal"); - assert_chatwidget_snapshot!( - "approval_modal_patch", - terminal.backend().vt100().screen().contents() - ); + let contents = terminal.backend().vt100().screen().contents(); + assert!(!contents.contains("$ apply_patch")); + assert_chatwidget_snapshot!("approval_modal_patch", contents); Ok(()) } @@ -1469,28 +1468,11 @@ async fn apply_patch_events_emit_history_cells() { id: "s1".into(), msg: EventMsg::ApplyPatchApprovalRequest(ev), }); - let cells = drain_insert_history(&mut rx); assert!( - cells.is_empty(), + drain_insert_history(&mut rx).is_empty(), "expected approval request to surface via modal without emitting history cells" ); - let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80)); - let mut buf = ratatui::buffer::Buffer::empty(area); - chat.render(area, &mut buf); - let mut saw_summary = false; - for y in 0..area.height { - let mut row = String::new(); - for x in 0..area.width { - row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); - } - if row.contains("foo.txt (+1 -0)") { - saw_summary = true; - break; - } - } - assert!(saw_summary, "expected approval modal to show diff summary"); - // 2) Begin apply -> per-file apply block cell (no global header) let mut changes2 = HashMap::new(); changes2.insert( @@ -1820,7 +1802,7 @@ async fn apply_patch_untrusted_shows_approval_modal() -> anyhow::Result<()> { } #[tokio::test] -async fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> { +async fn apply_patch_request_omits_diff_summary_from_modal() -> anyhow::Result<()> { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; // Ensure we are in OnRequest so an approval is surfaced @@ -1849,43 +1831,24 @@ async fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> { }), }); - // No history entries yet; the modal should contain the diff summary - let cells = drain_insert_history(&mut rx); assert!( - cells.is_empty(), + drain_insert_history(&mut rx).is_empty(), "expected approval request to render via modal instead of history" ); let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80)); let mut buf = ratatui::buffer::Buffer::empty(area); chat.render(area, &mut buf); - - let mut saw_header = false; - let mut saw_line1 = false; - let mut saw_line2 = false; + let mut contents = String::new(); for y in 0..area.height { - let mut row = String::new(); for x in 0..area.width { - row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); - } - if row.contains("README.md (+2 -0)") { - saw_header = true; - } - if row.contains("+line one") { - saw_line1 = true; - } - if row.contains("+line two") { - saw_line2 = true; - } - if saw_header && saw_line1 && saw_line2 { - break; + contents.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); } + contents.push('\n'); } - assert!(saw_header, "expected modal to show diff header with totals"); - assert!( - saw_line1 && saw_line2, - "expected modal to show per-line diff summary" - ); + assert!(!contents.contains("README.md (+2 -0)")); + assert!(!contents.contains("+line one")); + assert!(!contents.contains("+line two")); Ok(()) }