diff --git a/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs b/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs index b5531377526e..f8eaf799da0b 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_interrupt.rs @@ -127,10 +127,17 @@ async fn turn_interrupt_aborts_running_turn() -> Result<()> { #[tokio::test] async fn turn_interrupt_resolves_pending_command_approval_request() -> Result<()> { + #[cfg(target_os = "windows")] + let shell_command = vec![ + "powershell".to_string(), + "-Command".to_string(), + "Start-Sleep -Seconds 10".to_string(), + ]; + #[cfg(not(target_os = "windows"))] let shell_command = vec![ "python3".to_string(), "-c".to_string(), - "print(42)".to_string(), + "import time; time.sleep(10)".to_string(), ]; let tmp = TempDir::new()?; @@ -143,7 +150,7 @@ async fn turn_interrupt_resolves_pending_command_approval_request() -> Result<() shell_command.clone(), Some(&working_directory), Some(10_000), - "call_python_approval", + "call_sleep_approval", )?]) .await; create_config_toml(&codex_home, &server.uri(), "untrusted", "read-only")?; @@ -172,6 +179,7 @@ async fn turn_interrupt_resolves_pending_command_approval_request() -> Result<() text_elements: Vec::new(), }], cwd: Some(working_directory), + approval_policy: Some(codex_app_server_protocol::AskForApproval::UnlessTrusted), ..Default::default() }) .await?; @@ -190,7 +198,7 @@ async fn turn_interrupt_resolves_pending_command_approval_request() -> Result<() let ServerRequest::CommandExecutionRequestApproval { request_id, params } = request else { panic!("expected CommandExecutionRequestApproval request"); }; - assert_eq!(params.item_id, "call_python_approval"); + assert_eq!(params.item_id, "call_sleep_approval"); assert_eq!(params.thread_id, thread.id); assert_eq!(params.turn_id, turn.id); @@ -251,6 +259,7 @@ fn create_config_toml( r#" model = "mock-model" approval_policy = "{approval_policy}" +approvals_reviewer = "user" sandbox_mode = "{sandbox_mode}" model_provider = "mock_provider" diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 026bd05d7c19..61f955519360 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -164,7 +164,7 @@ use toml::Value as TomlValue; use uuid::Uuid; mod agent_navigation; mod app_server_adapter; -mod app_server_requests; +pub(crate) mod app_server_requests; mod loaded_threads; mod pending_interactive_replay; @@ -5895,8 +5895,13 @@ impl App { .handle_server_notification(notification, /*replay_kind*/ None); } ThreadBufferedEvent::Request(request) => { - self.chat_widget - .handle_server_request(request, /*replay_kind*/ None); + if self + .pending_app_server_requests + .contains_server_request(&request) + { + self.chat_widget + .handle_server_request(request, /*replay_kind*/ None); + } } ThreadBufferedEvent::HistoryEntryResponse(event) => { self.chat_widget.handle_history_entry_response(event); @@ -6838,6 +6843,11 @@ mod tests { let approval_request = exec_approval_request(thread_id, "turn-1", "call-1", /*approval_id*/ None); + assert_eq!( + app.pending_app_server_requests + .note_server_request(&approval_request), + None + ); app.enqueue_primary_thread_request(approval_request).await?; app.enqueue_primary_thread_session( test_thread_session(thread_id, test_path_buf("/tmp/project")), @@ -6880,6 +6890,66 @@ mod tests { panic!("expected approval action to submit a thread-scoped op"); } + #[tokio::test] + async fn resolved_buffered_approval_does_not_become_actionable_after_drain() -> Result<()> { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let thread_id = ThreadId::new(); + let approval_request = + exec_approval_request(thread_id, "turn-1", "call-1", /*approval_id*/ None); + + app.enqueue_primary_thread_session( + test_thread_session(thread_id, test_path_buf("/tmp/project")), + Vec::new(), + ) + .await?; + while app_event_rx.try_recv().is_ok() {} + + assert_eq!( + app.pending_app_server_requests + .note_server_request(&approval_request), + None + ); + app.enqueue_thread_request(thread_id, approval_request) + .await?; + + let resolved = app + .pending_app_server_requests + .resolve_notification(&AppServerRequestId::Integer(1)) + .expect("matching app-server request should resolve"); + app.chat_widget.dismiss_app_server_request(&resolved); + while app_event_rx.try_recv().is_ok() {} + + let rx = app + .active_thread_rx + .as_mut() + .expect("primary thread receiver should be active"); + let event = time::timeout(Duration::from_millis(50), rx.recv()) + .await + .expect("timed out waiting for buffered approval event") + .expect("channel closed unexpectedly"); + + assert!(matches!( + &event, + ThreadBufferedEvent::Request(ServerRequest::CommandExecutionRequestApproval { + params, + .. + }) if params.turn_id == "turn-1" + )); + + app.handle_thread_event_now(event); + app.chat_widget + .handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + + while let Ok(app_event) = app_event_rx.try_recv() { + assert!( + !matches!(app_event, AppEvent::SubmitThreadOp { .. }), + "resolved buffered approval should not become actionable" + ); + } + + Ok(()) + } + #[tokio::test] async fn enqueue_primary_thread_session_replays_turns_before_initial_prompt_submit() -> Result<()> { diff --git a/codex-rs/tui/src/app/app_server_adapter.rs b/codex-rs/tui/src/app/app_server_adapter.rs index 09b8ce06cdf8..f2490ca6a2f7 100644 --- a/codex-rs/tui/src/app/app_server_adapter.rs +++ b/codex-rs/tui/src/app/app_server_adapter.rs @@ -158,8 +158,12 @@ impl App { ) { match ¬ification { ServerNotification::ServerRequestResolved(notification) => { - self.pending_app_server_requests - .resolve_notification(¬ification.request_id); + if let Some(request) = self + .pending_app_server_requests + .resolve_notification(¬ification.request_id) + { + self.chat_widget.dismiss_app_server_request(&request); + } } ServerNotification::McpServerStatusUpdated(_) => { self.refresh_mcp_startup_expected_servers_from_config(); diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index b1393e95e08d..e2483eb25df1 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::collections::VecDeque; use crate::app_command::AppCommand; use crate::app_command::AppCommandView; @@ -27,12 +28,32 @@ pub(super) struct UnsupportedAppServerRequest { pub(super) message: String, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ResolvedAppServerRequest { + ExecApproval { + id: String, + }, + FileChangeApproval { + id: String, + }, + PermissionsApproval { + id: String, + }, + UserInput { + call_id: String, + }, + McpElicitation { + server_name: String, + request_id: McpRequestId, + }, +} + #[derive(Debug, Default)] pub(super) struct PendingAppServerRequests { exec_approvals: HashMap, file_change_approvals: HashMap, permissions_approvals: HashMap, - user_inputs: HashMap, + user_inputs: HashMap>, mcp_requests: HashMap, } @@ -70,7 +91,12 @@ impl PendingAppServerRequests { } ServerRequest::ToolRequestUserInput { request_id, params } => { self.user_inputs - .insert(params.turn_id.clone(), request_id.clone()); + .entry(params.turn_id.clone()) + .or_default() + .push_back(PendingUserInputRequest { + item_id: params.item_id.clone(), + request_id: request_id.clone(), + }); None } ServerRequest::McpServerElicitationRequest { request_id, params } => { @@ -165,11 +191,10 @@ impl PendingAppServerRequests { }) .transpose()?, AppCommandView::UserInputAnswer { id, response } => self - .user_inputs - .remove(id) - .map(|request_id| { + .pop_user_input_request_for_turn(id) + .map(|pending| { Ok::(AppServerRequestResolution { - request_id, + request_id: pending.request_id, result: serde_json::to_value( serde_json::from_value::( serde_json::to_value(response).map_err(|err| { @@ -229,17 +254,133 @@ impl PendingAppServerRequests { Ok(resolution) } - pub(super) fn resolve_notification(&mut self, request_id: &AppServerRequestId) { - self.exec_approvals.retain(|_, value| value != request_id); - self.file_change_approvals - .retain(|_, value| value != request_id); - self.permissions_approvals - .retain(|_, value| value != request_id); - self.user_inputs.retain(|_, value| value != request_id); - self.mcp_requests.retain(|_, value| value != request_id); + pub(super) fn resolve_notification( + &mut self, + request_id: &AppServerRequestId, + ) -> Option { + if let Some(id) = self + .exec_approvals + .iter() + .find_map(|(id, value)| (value == request_id).then(|| id.clone())) + { + self.exec_approvals.remove(&id); + return Some(ResolvedAppServerRequest::ExecApproval { id }); + } + + if let Some(id) = self + .file_change_approvals + .iter() + .find_map(|(id, value)| (value == request_id).then(|| id.clone())) + { + self.file_change_approvals.remove(&id); + return Some(ResolvedAppServerRequest::FileChangeApproval { id }); + } + + if let Some(id) = self + .permissions_approvals + .iter() + .find_map(|(id, value)| (value == request_id).then(|| id.clone())) + { + self.permissions_approvals.remove(&id); + return Some(ResolvedAppServerRequest::PermissionsApproval { id }); + } + + if let Some(pending) = self.remove_user_input_request(request_id) { + return Some(ResolvedAppServerRequest::UserInput { + call_id: pending.item_id, + }); + } + + if let Some(key) = self + .mcp_requests + .iter() + .find_map(|(key, value)| (value == request_id).then(|| key.clone())) + { + self.mcp_requests.remove(&key); + return Some(ResolvedAppServerRequest::McpElicitation { + server_name: key.server_name, + request_id: key.request_id, + }); + } + + None + } + + pub(super) fn contains_server_request(&self, request: &ServerRequest) -> bool { + match request { + ServerRequest::CommandExecutionRequestApproval { request_id, .. } => self + .exec_approvals + .values() + .any(|pending_request_id| pending_request_id == request_id), + ServerRequest::FileChangeRequestApproval { request_id, .. } => self + .file_change_approvals + .values() + .any(|pending_request_id| pending_request_id == request_id), + ServerRequest::PermissionsRequestApproval { request_id, .. } => self + .permissions_approvals + .values() + .any(|pending_request_id| pending_request_id == request_id), + ServerRequest::ToolRequestUserInput { request_id, .. } => { + self.user_inputs.values().any(|queue| { + queue + .iter() + .any(|pending| &pending.request_id == request_id) + }) + } + ServerRequest::McpServerElicitationRequest { request_id, .. } => self + .mcp_requests + .values() + .any(|pending_request_id| pending_request_id == request_id), + ServerRequest::DynamicToolCall { .. } + | ServerRequest::ChatgptAuthTokensRefresh { .. } + | ServerRequest::ApplyPatchApproval { .. } + | ServerRequest::ExecCommandApproval { .. } => true, + } + } + + fn pop_user_input_request_for_turn( + &mut self, + turn_id: &str, + ) -> Option { + let pending = self + .user_inputs + .get_mut(turn_id) + .and_then(VecDeque::pop_front); + if self + .user_inputs + .get(turn_id) + .is_some_and(VecDeque::is_empty) + { + self.user_inputs.remove(turn_id); + } + pending + } + + fn remove_user_input_request( + &mut self, + request_id: &AppServerRequestId, + ) -> Option { + let (turn_id, index) = self.user_inputs.iter().find_map(|(turn_id, queue)| { + queue + .iter() + .position(|pending| &pending.request_id == request_id) + .map(|index| (turn_id.clone(), index)) + })?; + let queue = self.user_inputs.get_mut(&turn_id)?; + let removed = queue.remove(index); + if queue.is_empty() { + self.user_inputs.remove(&turn_id); + } + removed } } +#[derive(Debug)] +struct PendingUserInputRequest { + item_id: String, + request_id: AppServerRequestId, +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct McpLegacyRequestKey { server_name: String, @@ -272,6 +413,7 @@ fn file_change_decision(decision: &ReviewDecision) -> Result bool { self.complete } + + fn dismiss_app_server_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + let ResolvedAppServerRequest::McpElicitation { + server_name, + request_id, + } = request + else { + return false; + }; + let Some(target) = self.elicitation_target.as_ref() else { + return false; + }; + if target.server_name != *server_name || target.request_id != *request_id { + return false; + } + + self.complete = true; + true + } } impl crate::render::renderable::Renderable for AppLinkView { @@ -546,9 +566,11 @@ impl crate::render::renderable::Renderable for AppLinkView { #[cfg(test)] mod tests { use super::*; + use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::app_event::AppEvent; use crate::render::renderable::Renderable; use insta::assert_snapshot; + use pretty_assertions::assert_eq; use tokio::sync::mpsc::unbounded_channel; fn suggestion_target() -> AppLinkElicitationTarget { @@ -889,6 +911,64 @@ mod tests { assert!(view.is_complete()); } + #[test] + fn resolved_tool_suggestion_dismisses_matching_view() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut view = AppLinkView::new( + AppLinkViewParams { + app_id: "connector_google_calendar".to_string(), + title: "Google Calendar".to_string(), + description: Some("Plan events and schedules.".to_string()), + instructions: "Enable this app to use it for the current request.".to_string(), + url: "https://example.test/google-calendar".to_string(), + is_installed: true, + is_enabled: false, + suggest_reason: Some("Plan and reference events from your calendar".to_string()), + suggestion_type: Some(AppLinkSuggestionType::Enable), + elicitation_target: Some(suggestion_target()), + }, + tx, + ); + + assert!( + view.dismiss_app_server_request(&ResolvedAppServerRequest::McpElicitation { + server_name: "codex_apps".to_string(), + request_id: McpRequestId::String("request-1".to_string()), + }) + ); + assert!(view.is_complete()); + } + + #[test] + fn resolved_tool_suggestion_ignores_non_matching_request() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut view = AppLinkView::new( + AppLinkViewParams { + app_id: "connector_google_calendar".to_string(), + title: "Google Calendar".to_string(), + description: Some("Plan events and schedules.".to_string()), + instructions: "Enable this app to use it for the current request.".to_string(), + url: "https://example.test/google-calendar".to_string(), + is_installed: true, + is_enabled: false, + suggest_reason: Some("Plan and reference events from your calendar".to_string()), + suggestion_type: Some(AppLinkSuggestionType::Enable), + elicitation_target: Some(suggestion_target()), + }, + tx, + ); + + assert!( + !view.dismiss_app_server_request(&ResolvedAppServerRequest::McpElicitation { + server_name: "other_server".to_string(), + request_id: McpRequestId::String("request-1".to_string()), + }) + ); + assert!(!view.is_complete()); + } + #[test] fn install_suggestion_with_reason_snapshot() { let (tx_raw, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 656e812d669a..0e2bb2b2e374 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::path::PathBuf; +use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::BottomPaneView; @@ -97,6 +98,35 @@ impl ApprovalRequest { | ApprovalRequest::McpElicitation { thread_label, .. } => thread_label.as_deref(), } } + + fn matches_resolved_request(&self, request: &ResolvedAppServerRequest) -> bool { + match (self, request) { + ( + ApprovalRequest::Exec { id, .. }, + ResolvedAppServerRequest::ExecApproval { id: resolved_id }, + ) => id == resolved_id, + ( + ApprovalRequest::Permissions { call_id, .. }, + ResolvedAppServerRequest::PermissionsApproval { id }, + ) => call_id == id, + ( + ApprovalRequest::ApplyPatch { id, .. }, + ResolvedAppServerRequest::FileChangeApproval { id: resolved_id }, + ) => id == resolved_id, + ( + ApprovalRequest::McpElicitation { + server_name, + request_id, + .. + }, + ResolvedAppServerRequest::McpElicitation { + server_name: resolved_server_name, + request_id: resolved_request_id, + }, + ) => server_name == resolved_server_name && request_id == resolved_request_id, + _ => false, + } + } } /// Modal overlay asking the user to approve or deny one or more requests. @@ -131,6 +161,23 @@ impl ApprovalOverlay { self.queue.push(req); } + fn dismiss_resolved_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + let queue_len = self.queue.len(); + self.queue + .retain(|queued_request| !queued_request.matches_resolved_request(request)); + if self + .current_request + .as_ref() + .is_some_and(|current_request| current_request.matches_resolved_request(request)) + { + self.current_complete = true; + self.advance_queue(); + return true; + } + + self.queue.len() != queue_len + } + fn set_current(&mut self, request: ApprovalRequest) { self.current_complete = false; let header = build_header(&request); @@ -465,6 +512,10 @@ impl BottomPaneView for ApprovalOverlay { self.enqueue_request(request); None } + + fn dismiss_app_server_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + self.dismiss_resolved_request(request) + } } impl Renderable for ApprovalOverlay { @@ -951,6 +1002,27 @@ mod tests { assert!(saw_op, "expected approval decision to emit an op"); } + #[test] + fn resolved_request_dismisses_overlay_without_emitting_abort() { + let (tx, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let mut view = ApprovalOverlay::new(make_exec_request(), tx, Features::with_defaults()); + + assert!( + view.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + id: "test".to_string(), + }) + ); + assert!( + view.is_complete(), + "resolved request should close the overlay" + ); + assert!( + rx.try_recv().is_err(), + "dismissing a stale request should not emit an approval op" + ); + } + #[test] fn o_opens_source_thread_for_cross_thread_approval() { let (tx, mut rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs index 35165db491e6..6fa946a5af2b 100644 --- a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs +++ b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs @@ -1,3 +1,4 @@ +use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::bottom_pane::ApprovalRequest; use crate::bottom_pane::McpServerElicitationFormRequest; use crate::render::renderable::Renderable; @@ -87,4 +88,11 @@ pub(crate) trait BottomPaneView: Renderable { ) -> Option { Some(request) } + + /// Dismiss a request that was resolved by another client. + /// + /// Returns `true` when the view changed state. + fn dismiss_app_server_request(&mut self, _request: &ResolvedAppServerRequest) -> bool { + false + } } diff --git a/codex-rs/tui/src/bottom_pane/mcp_server_elicitation.rs b/codex-rs/tui/src/bottom_pane/mcp_server_elicitation.rs index 1e3ad6591244..c9844ff9151e 100644 --- a/codex-rs/tui/src/bottom_pane/mcp_server_elicitation.rs +++ b/codex-rs/tui/src/bottom_pane/mcp_server_elicitation.rs @@ -28,6 +28,7 @@ use ratatui::widgets::Widget; use serde_json::Value; use unicode_width::UnicodeWidthStr; +use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::CancellationEvent; use crate::bottom_pane::ChatComposer; @@ -1145,6 +1146,16 @@ impl McpServerElicitationOverlay { ); } + fn advance_queue_or_complete(&mut self) { + if let Some(next) = self.queue.pop_front() { + self.request = next; + self.reset_for_request(); + self.restore_current_draft(); + } else { + self.done = true; + } + } + fn submit_answers(&mut self) { self.save_current_draft(); if let Some(idx) = self.first_required_unanswered_index() { @@ -1181,13 +1192,7 @@ impl McpServerElicitationOverlay { /*content*/ None, meta, ); - if let Some(next) = self.queue.pop_front() { - self.request = next; - self.reset_for_request(); - self.restore_current_draft(); - } else { - self.done = true; - } + self.advance_queue_or_complete(); return; } let content = self @@ -1205,13 +1210,28 @@ impl McpServerElicitationOverlay { Some(Value::Object(content)), /*meta*/ None, ); - if let Some(next) = self.queue.pop_front() { - self.request = next; - self.reset_for_request(); - self.restore_current_draft(); - } else { - self.done = true; + self.advance_queue_or_complete(); + } + + fn dismiss_resolved_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + let ResolvedAppServerRequest::McpElicitation { + server_name, + request_id, + } = request + else { + return false; + }; + + let queue_len = self.queue.len(); + self.queue.retain(|queued_request| { + queued_request.server_name != *server_name || queued_request.request_id != *request_id + }); + if self.request.server_name == *server_name && self.request.request_id == *request_id { + self.advance_queue_or_complete(); + return true; } + + self.queue.len() != queue_len } fn go_next_or_submit(&mut self) { @@ -1642,6 +1662,10 @@ impl BottomPaneView for McpServerElicitationOverlay { self.queue.push_back(request); None } + + fn dismiss_app_server_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + self.dismiss_resolved_request(request) + } } fn wrap_footer_tips(width: u16, tips: Vec) -> Vec> { @@ -2362,6 +2386,72 @@ mod tests { assert_eq!(overlay.request.message, "Third"); } + #[test] + fn resolved_request_dismisses_overlay_without_emitting_events() { + let (tx, mut rx) = test_sender(); + let thread_id = ThreadId::default(); + let supported_form_schema = serde_json::json!({ + "type": "object", + "properties": { + "confirmed": { + "type": "boolean", + "title": "Confirm", + } + }, + }); + let mut overlay = McpServerElicitationOverlay::new( + McpServerElicitationFormRequest::from_event( + thread_id, + form_request("First", supported_form_schema.clone(), /*meta*/ None), + ) + .expect("expected supported form"), + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + ); + overlay.try_consume_mcp_server_elicitation_request( + McpServerElicitationFormRequest::from_event( + thread_id, + ElicitationRequestEvent { + turn_id: Some("turn-2".to_string()), + server_name: "server-1".to_string(), + id: McpRequestId::String("request-2".to_string()), + request: ElicitationRequest::Form { + meta: None, + message: "Second".to_string(), + requested_schema: supported_form_schema, + }, + }, + ) + .expect("expected supported form"), + ); + + assert!( + overlay.dismiss_app_server_request(&ResolvedAppServerRequest::McpElicitation { + server_name: "server-1".to_string(), + request_id: McpRequestId::String("request-1".to_string()), + }) + ); + assert_eq!(overlay.request.message, "Second"); + assert!(matches!( + rx.try_recv(), + Err(tokio::sync::mpsc::error::TryRecvError::Empty) + )); + + assert!( + overlay.dismiss_app_server_request(&ResolvedAppServerRequest::McpElicitation { + server_name: "server-1".to_string(), + request_id: McpRequestId::String("request-2".to_string()), + }) + ); + assert!(overlay.is_complete()); + assert!(matches!( + rx.try_recv(), + Err(tokio::sync::mpsc::error::TryRecvError::Empty) + )); + } + #[test] fn boolean_form_snapshot() { let (tx, _rx) = test_sender(); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 85020d71374c..91c8bd48eba6 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -15,6 +15,7 @@ //! hint. The pane schedules redraws so those hints can expire even when the UI is otherwise idle. use std::path::PathBuf; +use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::app_event::ConnectorsSnapshot; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::pending_input_preview::PendingInputPreview; @@ -379,6 +380,18 @@ impl BottomPane { self.request_redraw(); } + fn pop_active_view(&mut self) { + if self.view_stack.pop().is_some() { + self.on_view_stack_depth_decreased(); + } + } + + fn on_view_stack_depth_decreased(&mut self) { + if self.view_stack.is_empty() { + self.on_active_view_complete(); + } + } + /// Forward a key event to the active view or the composer. pub fn handle_key_event(&mut self, key_event: KeyEvent) -> InputResult { // If a modal/view is active, handle it here; otherwise forward to composer. @@ -408,16 +421,14 @@ impl BottomPane { }; if ctrl_c_completed { - self.view_stack.pop(); - self.on_active_view_complete(); + self.pop_active_view(); if let Some(next_view) = self.view_stack.last() && next_view.is_in_paste_burst() { self.request_redraw_in(ChatComposer::recommended_paste_flush_delay()); } } else if view_complete { - self.view_stack.clear(); - self.on_active_view_complete(); + self.pop_active_view(); } else if view_in_paste_burst { self.request_redraw_in(ChatComposer::recommended_paste_flush_delay()); } @@ -469,10 +480,10 @@ impl BottomPane { pub(crate) fn on_ctrl_c(&mut self) -> CancellationEvent { if let Some(view) = self.view_stack.last_mut() { let event = view.on_ctrl_c(); + let view_complete = view.is_complete(); if matches!(event, CancellationEvent::Handled) { - if view.is_complete() { - self.view_stack.pop(); - self.on_active_view_complete(); + if view_complete { + self.pop_active_view(); } self.show_quit_shortcut_hint(key_hint::ctrl(KeyCode::Char('c'))); self.request_redraw(); @@ -493,17 +504,14 @@ impl BottomPane { } pub fn handle_paste(&mut self, pasted: String) { - if !self.view_stack.is_empty() { - let (needs_redraw, view_complete) = { - let last_index = self.view_stack.len() - 1; - let view = &mut self.view_stack[last_index]; - (view.handle_paste(pasted), view.is_complete()) - }; + if let Some(view) = self.view_stack.last_mut() { + let needs_redraw = view.handle_paste(pasted); + let view_complete = view.is_complete(); if view_complete { self.view_stack.clear(); self.on_active_view_complete(); } - if needs_redraw { + if needs_redraw || view_complete { self.request_redraw(); } } else { @@ -1032,6 +1040,37 @@ impl BottomPane { self.push_view(Box::new(modal)); } + pub(crate) fn dismiss_app_server_request( + &mut self, + request: &ResolvedAppServerRequest, + ) -> bool { + if self.view_stack.is_empty() { + return false; + } + + let mut changed = false; + let mut completed_indices = Vec::new(); + for index in (0..self.view_stack.len()).rev() { + let view = &mut self.view_stack[index]; + if !view.dismiss_app_server_request(request) { + continue; + } + changed = true; + if view.is_complete() { + completed_indices.push(index); + } + } + if !changed { + return false; + } + for index in completed_indices { + self.view_stack.remove(index); + } + self.on_view_stack_depth_decreased(); + self.request_redraw(); + true + } + fn on_active_view_complete(&mut self) { self.resume_status_timer_after_modal(); self.set_composer_input_enabled(/*enabled*/ true, /*placeholder*/ None); @@ -1241,6 +1280,7 @@ impl Renderable for BottomPane { #[cfg(test)] mod tests { use super::*; + use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::app_event::AppEvent; use crate::status_indicator_widget::STATUS_DETAILS_DEFAULT_MAX_LINES; use crate::status_indicator_widget::StatusDetailsCapitalization; @@ -1248,9 +1288,12 @@ mod tests { use crate::test_support::test_path_buf; use codex_protocol::protocol::Op; use codex_protocol::protocol::SkillScope; + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; use crossterm::event::KeyModifiers; use insta::assert_snapshot; + use pretty_assertions::assert_eq; use ratatui::buffer::Buffer; use ratatui::layout::Rect; use std::cell::Cell; @@ -1275,6 +1318,19 @@ mod tests { snapshot_buffer(&buf) } + fn test_pane(app_event_tx: AppEventSender) -> BottomPane { + BottomPane::new(BottomPaneParams { + app_event_tx, + frame_requester: FrameRequester::test_dummy(), + has_input_focus: true, + enhanced_keys_supported: false, + placeholder_text: "Ask Codex to do anything".to_string(), + disable_paste_burst: false, + animations_enabled: true, + skills: Some(Vec::new()), + }) + } + fn exec_request() -> ApprovalRequest { ApprovalRequest::Exec { thread_id: codex_protocol::ThreadId::new(), @@ -1291,6 +1347,73 @@ mod tests { } } + #[derive(Default)] + struct DismissibleView { + id: Option<&'static str>, + dismiss_exec_id: Option<&'static str>, + complete: bool, + } + + impl Renderable for DismissibleView { + fn render(&self, _area: Rect, _buf: &mut Buffer) {} + + fn desired_height(&self, _width: u16) -> u16 { + 0 + } + } + + impl BottomPaneView for DismissibleView { + fn is_complete(&self) -> bool { + self.complete + } + + fn view_id(&self) -> Option<&'static str> { + self.id + } + + fn dismiss_app_server_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + let ResolvedAppServerRequest::ExecApproval { id } = request else { + return false; + }; + if self.dismiss_exec_id != Some(id.as_str()) { + return false; + } + + self.complete = true; + true + } + } + + #[derive(Default)] + struct CompletingView { + id: Option<&'static str>, + complete: bool, + } + + impl Renderable for CompletingView { + fn render(&self, _area: Rect, _buf: &mut Buffer) {} + + fn desired_height(&self, _width: u16) -> u16 { + 0 + } + } + + impl BottomPaneView for CompletingView { + fn handle_key_event(&mut self, key_event: KeyEvent) { + if key_event.code == KeyCode::Enter { + self.complete = true; + } + } + + fn is_complete(&self) -> bool { + self.complete + } + + fn view_id(&self) -> Option<&'static str> { + self.id + } + } + #[test] fn ctrl_c_on_modal_consumes_without_showing_quit_hint() { let (tx_raw, _rx) = unbounded_channel::(); @@ -1373,6 +1496,89 @@ mod tests { ); } + #[test] + fn dismiss_app_server_request_removes_matching_buried_view() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut pane = test_pane(tx); + + pane.push_view(Box::new(DismissibleView { + id: Some("buried"), + dismiss_exec_id: Some("request-1"), + complete: false, + })); + pane.push_view(Box::new(DismissibleView { + id: Some("top"), + dismiss_exec_id: None, + complete: false, + })); + + assert!( + pane.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + id: "request-1".to_string(), + }) + ); + assert_eq!(pane.view_stack.len(), 1); + assert_eq!( + pane.view_stack.last().and_then(|view| view.view_id()), + Some("top") + ); + } + + #[test] + fn dismiss_app_server_request_returns_false_when_no_view_matches() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut pane = test_pane(tx); + + pane.push_view(Box::new(DismissibleView { + id: Some("first"), + dismiss_exec_id: Some("other-request"), + complete: false, + })); + pane.push_view(Box::new(DismissibleView { + id: Some("second"), + dismiss_exec_id: None, + complete: false, + })); + + assert!( + !pane.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + id: "request-1".to_string(), + }) + ); + assert_eq!(pane.view_stack.len(), 2); + assert_eq!( + pane.view_stack.last().and_then(|view| view.view_id()), + Some("second") + ); + } + + #[test] + fn completing_top_view_preserves_underlying_view() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut pane = test_pane(tx); + + pane.push_view(Box::new(DismissibleView { + id: Some("underlying"), + dismiss_exec_id: None, + complete: false, + })); + pane.push_view(Box::new(CompletingView { + id: Some("top"), + complete: false, + })); + + pane.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!(pane.view_stack.len(), 1); + assert_eq!( + pane.view_stack.last().and_then(|view| view.view_id()), + Some("underlying") + ); + } + #[test] fn composer_shown_after_denied_while_task_running() { let (tx_raw, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs index f7db7c65dd9a..bc07c06e0ba2 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs +++ b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; use std::collections::VecDeque; use std::path::PathBuf; +use crate::app::app_server_requests::ResolvedAppServerRequest; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; @@ -198,6 +199,17 @@ impl RequestUserInputOverlay { self.request.questions.len() } + fn advance_queue_or_complete(&mut self) { + if let Some(next) = self.queue.pop_front() { + self.request = next; + self.reset_for_request(); + self.ensure_focus_available(); + self.restore_current_draft(); + } else { + self.done = true; + } + } + fn has_options(&self) -> bool { self.current_question() .and_then(|question| question.options.as_ref()) @@ -759,14 +771,23 @@ impl RequestUserInputOverlay { interrupted: false, }, ))); - if let Some(next) = self.queue.pop_front() { - self.request = next; - self.reset_for_request(); - self.ensure_focus_available(); - self.restore_current_draft(); - } else { - self.done = true; + self.advance_queue_or_complete(); + } + + fn dismiss_resolved_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + let ResolvedAppServerRequest::UserInput { call_id } = request else { + return false; + }; + + let queue_len = self.queue.len(); + self.queue + .retain(|queued_request| queued_request.call_id != *call_id); + if self.request.call_id == *call_id { + self.advance_queue_or_complete(); + return true; } + + self.queue.len() != queue_len } fn open_unanswered_confirmation(&mut self) { @@ -1273,6 +1294,10 @@ impl BottomPaneView for RequestUserInputOverlay { self.queue.push_back(request); None } + + fn dismiss_app_server_request(&mut self, request: &ResolvedAppServerRequest) -> bool { + self.dismiss_resolved_request(request) + } } #[cfg(test)] @@ -1532,6 +1557,118 @@ mod tests { expect_interrupt_only(&mut rx); } + #[test] + fn resolved_request_dismisses_overlay_without_emitting_events() { + let (tx, mut rx) = test_sender(); + let mut overlay = RequestUserInputOverlay::new( + RequestUserInputEvent { + call_id: "call-1".to_string(), + turn_id: "turn-1".to_string(), + questions: vec![question_with_options("q1", "First")], + }, + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + ); + + assert!( + overlay.dismiss_app_server_request(&ResolvedAppServerRequest::UserInput { + call_id: "call-1".to_string(), + }) + ); + assert!(overlay.done, "resolved request should close the overlay"); + assert!( + rx.try_recv().is_err(), + "dismissing a stale request should not emit an interrupt or answer" + ); + } + + #[test] + fn resolved_current_request_advances_to_next_same_turn_prompt() { + let (tx, mut rx) = test_sender(); + let mut overlay = RequestUserInputOverlay::new( + RequestUserInputEvent { + call_id: "call-1".to_string(), + turn_id: "turn-1".to_string(), + questions: vec![question_with_options("q1", "First")], + }, + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + ); + overlay.try_consume_user_input_request(RequestUserInputEvent { + call_id: "call-2".to_string(), + turn_id: "turn-1".to_string(), + questions: vec![question_with_options("q2", "Second")], + }); + + assert!( + overlay.dismiss_app_server_request(&ResolvedAppServerRequest::UserInput { + call_id: "call-1".to_string(), + }) + ); + + assert!(!overlay.done, "newer same-turn prompt should stay pending"); + assert_eq!(overlay.request.call_id, "call-2"); + assert_eq!(overlay.request.turn_id, "turn-1"); + assert_eq!(overlay.request.questions[0].id, "q2"); + assert!( + rx.try_recv().is_err(), + "dismissing a stale request should not emit an interrupt or answer" + ); + } + + #[test] + fn resolved_queued_request_removes_only_that_prompt() { + let (tx, mut rx) = test_sender(); + let mut overlay = RequestUserInputOverlay::new( + RequestUserInputEvent { + call_id: "call-1".to_string(), + turn_id: "turn-1".to_string(), + questions: vec![question_with_options("q1", "First")], + }, + tx, + /*has_input_focus*/ true, + /*enhanced_keys_supported*/ false, + /*disable_paste_burst*/ false, + ); + overlay.try_consume_user_input_request(RequestUserInputEvent { + call_id: "call-2".to_string(), + turn_id: "turn-1".to_string(), + questions: vec![question_with_options("q2", "Second")], + }); + overlay.try_consume_user_input_request(RequestUserInputEvent { + call_id: "call-3".to_string(), + turn_id: "turn-1".to_string(), + questions: vec![question_with_options("q3", "Third")], + }); + + assert!( + overlay.dismiss_app_server_request(&ResolvedAppServerRequest::UserInput { + call_id: "call-2".to_string(), + }) + ); + + assert_eq!(overlay.request.call_id, "call-1"); + assert!( + rx.try_recv().is_err(), + "dismissing a stale queued request should not emit an event" + ); + overlay.submit_answers(); + assert_eq!(overlay.request.call_id, "call-3"); + assert_eq!(overlay.request.questions[0].id, "q3"); + assert!( + rx.try_recv().is_ok(), + "submitting the still-current prompt should emit an answer" + ); + assert!( + rx.try_recv().is_ok(), + "submitting the still-current prompt should emit a history cell" + ); + } + #[test] fn options_can_submit_empty_when_unanswered() { let (tx, mut rx) = test_sender(); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e9f56a0ef07c..599655b3640b 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -43,6 +43,7 @@ use std::time::Duration; use std::time::Instant; 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::network_approval_context_to_core; @@ -2163,6 +2164,16 @@ impl ChatWidget { self.request_redraw(); } + pub(crate) fn dismiss_app_server_request(&mut self, request: &ResolvedAppServerRequest) { + // A remotely resolved request must not remain user-actionable. It may be + // materialized in the bottom pane or still deferred behind active streaming. + let removed_deferred = self.interrupts.remove_resolved_prompt(request); + let removed_visible = self.bottom_pane.dismiss_app_server_request(request); + if removed_deferred || removed_visible { + self.request_redraw(); + } + } + pub(crate) fn open_feedback_consent(&mut self, category: crate::app_event::FeedbackCategory) { let snapshot = self.feedback.snapshot(self.thread_id); let params = crate::bottom_pane::feedback_upload_consent_params( diff --git a/codex-rs/tui/src/chatwidget/interrupts.rs b/codex-rs/tui/src/chatwidget/interrupts.rs index 0a3fc800168a..f41e3b3a47b4 100644 --- a/codex-rs/tui/src/chatwidget/interrupts.rs +++ b/codex-rs/tui/src/chatwidget/interrupts.rs @@ -1,5 +1,6 @@ use std::collections::VecDeque; +use crate::app::app_server_requests::ResolvedAppServerRequest; use codex_protocol::approvals::ElicitationRequestEvent; use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; use codex_protocol::protocol::ExecApprovalRequestEvent; @@ -86,6 +87,13 @@ impl InterruptManager { self.queue.push_back(QueuedInterrupt::PatchEnd(ev)); } + pub(crate) fn remove_resolved_prompt(&mut self, request: &ResolvedAppServerRequest) -> bool { + let original_len = self.queue.len(); + self.queue + .retain(|queued| !queued.matches_resolved_prompt(request)); + self.queue.len() != original_len + } + pub(crate) fn flush_all(&mut self, chat: &mut ChatWidget) { while let Some(q) = self.queue.pop_front() { match q { @@ -103,3 +111,144 @@ impl InterruptManager { } } } + +impl QueuedInterrupt { + fn matches_resolved_prompt(&self, request: &ResolvedAppServerRequest) -> bool { + match self { + QueuedInterrupt::ExecApproval(ev) => { + matches!(request, ResolvedAppServerRequest::ExecApproval { id } + if ev.effective_approval_id() == id.as_str()) + } + QueuedInterrupt::ApplyPatchApproval(ev) => { + matches!(request, ResolvedAppServerRequest::FileChangeApproval { id } + if ev.call_id == id.as_str()) + } + QueuedInterrupt::Elicitation(ev) => { + matches!(request, ResolvedAppServerRequest::McpElicitation { + server_name, + request_id, + } if ev.server_name == server_name.as_str() && &ev.id == request_id) + } + QueuedInterrupt::RequestPermissions(ev) => { + matches!(request, ResolvedAppServerRequest::PermissionsApproval { id } + if ev.call_id == id.as_str()) + } + QueuedInterrupt::RequestUserInput(ev) => { + matches!(request, ResolvedAppServerRequest::UserInput { call_id } + if ev.call_id == call_id.as_str()) + } + QueuedInterrupt::ExecBegin(_) + | QueuedInterrupt::ExecEnd(_) + | QueuedInterrupt::McpBegin(_) + | QueuedInterrupt::McpEnd(_) + | QueuedInterrupt::PatchEnd(_) => false, + } + } +} + +#[cfg(test)] +mod tests { + use codex_protocol::approvals::ExecApprovalRequestEvent; + use codex_protocol::protocol::ExecCommandBeginEvent; + use codex_protocol::protocol::ExecCommandSource; + use codex_protocol::request_user_input::RequestUserInputEvent; + use codex_utils_absolute_path::AbsolutePathBuf; + use pretty_assertions::assert_eq; + + use super::*; + + fn user_input(call_id: &str, turn_id: &str) -> RequestUserInputEvent { + RequestUserInputEvent { + call_id: call_id.to_string(), + turn_id: turn_id.to_string(), + questions: Vec::new(), + } + } + + fn exec_approval(call_id: &str, approval_id: Option<&str>) -> ExecApprovalRequestEvent { + ExecApprovalRequestEvent { + call_id: call_id.to_string(), + approval_id: approval_id.map(str::to_string), + turn_id: "turn".to_string(), + command: vec!["true".to_string()], + cwd: AbsolutePathBuf::current_dir().expect("current dir"), + reason: None, + network_approval_context: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + additional_permissions: None, + available_decisions: None, + parsed_cmd: Vec::new(), + } + } + + fn exec_begin(call_id: &str) -> ExecCommandBeginEvent { + ExecCommandBeginEvent { + call_id: call_id.to_string(), + process_id: None, + turn_id: "turn".to_string(), + command: vec!["true".to_string()], + cwd: AbsolutePathBuf::current_dir().expect("current dir"), + parsed_cmd: Vec::new(), + source: ExecCommandSource::Agent, + interaction_input: None, + } + } + + #[test] + fn remove_resolved_prompt_removes_matching_user_input_only() { + let mut manager = InterruptManager::new(); + manager.push_user_input(user_input("call-a", "turn")); + manager.push_user_input(user_input("call-b", "turn")); + + assert!( + manager.remove_resolved_prompt(&ResolvedAppServerRequest::UserInput { + call_id: "call-b".to_string(), + }) + ); + + assert_eq!(manager.queue.len(), 1); + let Some(QueuedInterrupt::RequestUserInput(remaining)) = manager.queue.front() else { + panic!("expected remaining queued user input"); + }; + assert_eq!(remaining.call_id, "call-a"); + } + + #[test] + fn remove_resolved_prompt_matches_exec_approval_id() { + let mut manager = InterruptManager::new(); + manager.push_exec_approval(exec_approval("call", Some("approval"))); + + assert!( + !manager.remove_resolved_prompt(&ResolvedAppServerRequest::ExecApproval { + id: "call".to_string(), + }) + ); + assert_eq!(manager.queue.len(), 1); + + assert!( + manager.remove_resolved_prompt(&ResolvedAppServerRequest::ExecApproval { + id: "approval".to_string(), + }) + ); + assert!(manager.queue.is_empty()); + } + + #[test] + fn remove_resolved_prompt_keeps_lifecycle_events() { + let mut manager = InterruptManager::new(); + manager.push_exec_begin(exec_begin("call")); + + assert!( + !manager.remove_resolved_prompt(&ResolvedAppServerRequest::ExecApproval { + id: "call".to_string(), + }) + ); + + assert_eq!(manager.queue.len(), 1); + assert!(matches!( + manager.queue.front(), + Some(QueuedInterrupt::ExecBegin(_)) + )); + } +}