diff --git a/codex-rs/tui/src/app/app_server_events.rs b/codex-rs/tui/src/app/app_server_events.rs index 6b13bc0313..d1bd3ad6e8 100644 --- a/codex-rs/tui/src/app/app_server_events.rs +++ b/codex-rs/tui/src/app/app_server_events.rs @@ -116,16 +116,22 @@ impl App { } match ¬ification { ServerNotification::ServerRequestResolved(notification) => { + let notification_thread_id = + codex_protocol::ThreadId::from_string(¬ification.thread_id).ok(); self.pending_primary_events.retain(|event| { !matches!(event, ThreadBufferedEvent::Request(request) - if request.id() == ¬ification.request_id) + if request.id() == ¬ification.request_id + && server_request_thread_id(request) == notification_thread_id) }); - for requests in self.agents_overview.dispatched_requests.values_mut() { + if let Some(thread_id) = notification_thread_id + && let Some(requests) = + self.agents_overview.dispatched_requests.get_mut(&thread_id) + { requests.retain(|request| request.id() != ¬ification.request_id); } if let Some(request) = self .pending_app_server_requests - .resolve_notification(¬ification.request_id) + .resolve_notification(¬ification.thread_id, ¬ification.request_id) { self.chat_widget.dismiss_app_server_request(&request); if self.startup_pending_protected_request { diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 99d907e2f2..5e5d0b980b 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -50,12 +50,15 @@ pub(super) struct UnsupportedAppServerRequest { #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum ResolvedAppServerRequest { ExecApproval { + thread_id: String, id: String, }, FileChangeApproval { + thread_id: String, id: String, }, PermissionsApproval { + thread_id: String, id: String, }, UserInput { @@ -69,14 +72,20 @@ pub(crate) enum ResolvedAppServerRequest { #[derive(Debug, Default)] pub(super) struct PendingAppServerRequests { - exec_approvals: HashMap, - file_change_approvals: HashMap, - permissions_approvals: HashMap, + exec_approvals: HashMap<(String, String), AppServerRequestId>, + file_change_approvals: HashMap<(String, String), AppServerRequestId>, + permissions_approvals: HashMap<(String, String), AppServerRequestId>, user_inputs: HashMap>, mcp_requests: HashMap, } impl PendingAppServerRequests { + fn canonical_thread_id(thread_id: &str) -> String { + codex_protocol::ThreadId::from_string(thread_id) + .map(|thread_id| thread_id.to_string()) + .unwrap_or_else(|_| thread_id.to_string()) + } + pub(super) fn clear(&mut self) { self.exec_approvals.clear(); self.file_change_approvals.clear(); @@ -95,12 +104,20 @@ impl PendingAppServerRequests { .approval_id .clone() .unwrap_or_else(|| params.item_id.clone()); - self.exec_approvals.insert(approval_id, request_id.clone()); + self.exec_approvals.insert( + (Self::canonical_thread_id(¶ms.thread_id), approval_id), + request_id.clone(), + ); None } ServerRequest::FileChangeRequestApproval { request_id, params } => { - self.file_change_approvals - .insert(params.item_id.clone(), request_id.clone()); + self.file_change_approvals.insert( + ( + Self::canonical_thread_id(¶ms.thread_id), + params.item_id.clone(), + ), + request_id.clone(), + ); None } ServerRequest::PermissionsRequestApproval { request_id, params } => { @@ -116,8 +133,13 @@ impl PendingAppServerRequests { message: format!("failed to localize requested filesystem paths: {err}"), }); } - self.permissions_approvals - .insert(params.item_id.clone(), request_id.clone()); + self.permissions_approvals.insert( + ( + Self::canonical_thread_id(¶ms.thread_id), + params.item_id.clone(), + ), + request_id.clone(), + ); None } ServerRequest::ToolRequestUserInput { request_id, params } => { @@ -178,16 +200,18 @@ impl PendingAppServerRequests { pub(super) fn take_resolution( &mut self, + thread_id: &str, op: T, ) -> Result, String> where T: Into, { + let thread_id = Self::canonical_thread_id(thread_id); let op: AppCommand = op.into(); let resolution = match &op { AppCommand::ExecApproval { id, decision, .. } => self .exec_approvals - .remove(id) + .remove(&(thread_id, id.clone())) .map(|request_id| { Ok::(AppServerRequestResolution { request_id, @@ -204,7 +228,7 @@ impl PendingAppServerRequests { .transpose()?, AppCommand::PatchApproval { id, decision } => self .file_change_approvals - .remove(id) + .remove(&(thread_id, id.clone())) .map(|request_id| { Ok::(AppServerRequestResolution { request_id, @@ -219,7 +243,7 @@ impl PendingAppServerRequests { .transpose()?, AppCommand::RequestPermissionsResponse { id, response } => self .permissions_approvals - .remove(id) + .remove(&(thread_id, id.clone())) .map(|request_id| { Ok::(AppServerRequestResolution { request_id, @@ -280,33 +304,38 @@ impl PendingAppServerRequests { pub(super) fn resolve_notification( &mut self, + thread_id: &str, 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 }); + let thread_id = Self::canonical_thread_id(thread_id); + if let Some(key) = self.exec_approvals.iter().find_map(|(key, value)| { + (key.0 == thread_id && value == request_id).then(|| key.clone()) + }) { + self.exec_approvals.remove(&key); + return Some(ResolvedAppServerRequest::ExecApproval { + thread_id: key.0, + id: key.1, + }); } - 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(key) = self.file_change_approvals.iter().find_map(|(key, value)| { + (key.0 == thread_id && value == request_id).then(|| key.clone()) + }) { + self.file_change_approvals.remove(&key); + return Some(ResolvedAppServerRequest::FileChangeApproval { + thread_id: key.0, + id: key.1, + }); } - 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(key) = self.permissions_approvals.iter().find_map(|(key, value)| { + (key.0 == thread_id && value == request_id).then(|| key.clone()) + }) { + self.permissions_approvals.remove(&key); + return Some(ResolvedAppServerRequest::PermissionsApproval { + thread_id: key.0, + id: key.1, + }); } if let Some(pending) = self.remove_user_input_request(request_id) { @@ -475,11 +504,14 @@ mod tests { assert_eq!(pending.note_server_request(&request), None); let resolution = pending - .take_resolution(&Op::ExecApproval { - id: "approval-1".to_string(), - turn_id: None, - decision: CommandExecutionApprovalDecision::Accept, - }) + .take_resolution( + "thread-1", + &Op::ExecApproval { + id: "approval-1".to_string(), + turn_id: None, + decision: CommandExecutionApprovalDecision::Accept, + }, + ) .expect("resolution should serialize") .expect("request should be pending"); @@ -487,6 +519,83 @@ mod tests { assert_eq!(resolution.result, json!({ "decision": "accept" })); } + #[test] + fn colliding_approvals_resolve_only_on_their_own_thread() { + let approvals = [ + ( + "item/commandExecution/requestApproval", + Op::ExecApproval { + id: "shared-id".to_string(), + turn_id: None, + decision: CommandExecutionApprovalDecision::Accept, + }, + ), + ( + "item/fileChange/requestApproval", + Op::PatchApproval { + id: "shared-id".to_string(), + decision: FileChangeApprovalDecision::Accept, + }, + ), + ( + "item/permissions/requestApproval", + Op::RequestPermissionsResponse { + id: "shared-id".to_string(), + response: codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: codex_protocol::request_permissions::PermissionGrantScope::Turn, + strict_auto_review: false, + }, + }, + ), + ]; + + for (method, op) in approvals { + let mut pending = PendingAppServerRequests::default(); + let thread_ids = [ + codex_protocol::ThreadId::new().to_string(), + codex_protocol::ThreadId::new().to_string(), + ]; + for (index, thread_id) in thread_ids.iter().enumerate() { + let wire_thread_id = if index == 0 { + thread_id.to_ascii_uppercase() + } else { + thread_id.replace('-', "") + }; + let request: ServerRequest = serde_json::from_value(json!({ + "method": method, + "id": index + 1, + "params": { + "threadId": wire_thread_id, + "turnId": "turn-1", + "itemId": "shared-id", + "startedAtMs": 0, + "cwd": if cfg!(windows) { r"C:\tmp" } else { "/tmp" }, + "permissions": {}, + }, + })) + .expect("approval request should deserialize"); + assert_eq!(pending.note_server_request(&request), None); + assert!(pending.contains_server_request(&request)); + } + assert_eq!( + pending.resolve_notification(&thread_ids[1], &AppServerRequestId::Integer(1)), + None + ); + + for (thread_id, request_id) in thread_ids.iter().zip(1..=2) { + let resolution = pending + .take_resolution(thread_id, &op) + .expect("approval resolution should serialize") + .expect("approval should remain pending on its own thread"); + assert_eq!( + resolution.request_id, + AppServerRequestId::Integer(request_id) + ); + } + } + } + #[test] fn rejects_permissions_with_paths_that_cannot_be_localized() { let mut pending = PendingAppServerRequests::default(); @@ -587,22 +696,25 @@ mod tests { ); let permissions = pending - .take_resolution(&Op::RequestPermissionsResponse { - id: "perm-1".to_string(), - response: codex_protocol::request_permissions::RequestPermissionsResponse { - permissions: RequestPermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path(read_path)]), - Some(vec![absolute_path(write_path)]), - )), + .take_resolution( + "thread-1", + &Op::RequestPermissionsResponse { + id: "perm-1".to_string(), + response: codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path(read_path)]), + Some(vec![absolute_path(write_path)]), + )), + }, + scope: codex_protocol::request_permissions::PermissionGrantScope::Session, + strict_auto_review: false, }, - scope: codex_protocol::request_permissions::PermissionGrantScope::Session, - strict_auto_review: false, }, - }) + ) .expect("permissions response should serialize") .expect("permissions request should be pending"); assert_eq!(permissions.request_id, AppServerRequestId::Integer(7)); @@ -640,18 +752,21 @@ mod tests { ); let user_input = pending - .take_resolution(&Op::UserInputAnswer { - id: "turn-2".to_string(), - response: ToolRequestUserInputResponse { - answers: std::iter::once(( - "question".to_string(), - ToolRequestUserInputAnswer { - answers: vec!["yes".to_string()], - }, - )) - .collect(), + .take_resolution( + "thread-1", + &Op::UserInputAnswer { + id: "turn-2".to_string(), + response: ToolRequestUserInputResponse { + answers: std::iter::once(( + "question".to_string(), + ToolRequestUserInputAnswer { + answers: vec!["yes".to_string()], + }, + )) + .collect(), + }, }, - }) + ) .expect("user input response should serialize") .expect("user input request should be pending"); assert_eq!(user_input.request_id, AppServerRequestId::Integer(8)); @@ -697,13 +812,16 @@ mod tests { ); let resolution = pending - .take_resolution(&Op::ResolveElicitation { - server_name: "example".to_string(), - request_id: AppServerRequestId::Integer(12), - decision: McpServerElicitationAction::Accept, - content: Some(json!({ "answer": "yes" })), - meta: Some(json!({ "source": "tui" })), - }) + .take_resolution( + "thread-1", + &Op::ResolveElicitation { + server_name: "example".to_string(), + request_id: AppServerRequestId::Integer(12), + decision: McpServerElicitationAction::Accept, + content: Some(json!({ "answer": "yes" })), + meta: Some(json!({ "source": "tui" })), + }, + ) .expect("elicitation response should serialize") .expect("elicitation request should be pending"); @@ -779,10 +897,13 @@ mod tests { ); let resolution = pending - .take_resolution(&Op::PatchApproval { - id: "patch-1".to_string(), - decision: FileChangeApprovalDecision::Cancel, - }) + .take_resolution( + "thread-1", + &Op::PatchApproval { + id: "patch-1".to_string(), + decision: FileChangeApprovalDecision::Cancel, + }, + ) .expect("resolution should serialize") .expect("request should be pending"); @@ -793,11 +914,12 @@ mod tests { #[test] fn resolve_notification_returns_resolved_exec_request() { let mut pending = PendingAppServerRequests::default(); + let thread_id = codex_protocol::ThreadId::new().to_string(); assert_eq!( pending.note_server_request(&ServerRequest::CommandExecutionRequestApproval { request_id: AppServerRequestId::Integer(41), params: CommandExecutionRequestApprovalParams { - thread_id: "thread-1".to_string(), + thread_id: thread_id.to_ascii_uppercase(), turn_id: "turn-1".to_string(), item_id: "call-1".to_string(), started_at_ms: 0, @@ -818,13 +940,17 @@ mod tests { ); assert_eq!( - pending.resolve_notification(&AppServerRequestId::Integer(41)), + pending.resolve_notification( + &thread_id.replace('-', ""), + &AppServerRequestId::Integer(41) + ), Some(ResolvedAppServerRequest::ExecApproval { + thread_id: thread_id.clone(), id: "approval-1".to_string(), }) ); assert_eq!( - pending.resolve_notification(&AppServerRequestId::Integer(41)), + pending.resolve_notification(&thread_id, &AppServerRequestId::Integer(41)), None ); } @@ -855,7 +981,7 @@ mod tests { ); assert_eq!( - pending.resolve_notification(&AppServerRequestId::Integer(12)), + pending.resolve_notification("thread-1", &AppServerRequestId::Integer(12)), Some(ResolvedAppServerRequest::McpElicitation { server_name: "example".to_string(), request_id: AppServerRequestId::Integer(12), @@ -879,7 +1005,7 @@ mod tests { }); assert_eq!( - pending.resolve_notification(&AppServerRequestId::Integer(8)), + pending.resolve_notification("thread-1", &AppServerRequestId::Integer(8)), Some(ResolvedAppServerRequest::UserInput { call_id: "tool-1".to_string(), }) @@ -907,17 +1033,23 @@ mod tests { answers: HashMap::new(), }; let first_response = pending - .take_resolution(&Op::UserInputAnswer { - id: "turn-1".to_string(), - response: response.clone(), - }) + .take_resolution( + "thread-1", + &Op::UserInputAnswer { + id: "turn-1".to_string(), + response: response.clone(), + }, + ) .expect("user input response should serialize") .expect("first user input request should be pending"); let second_response = pending - .take_resolution(&Op::UserInputAnswer { - id: "turn-1".to_string(), - response, - }) + .take_resolution( + "thread-1", + &Op::UserInputAnswer { + id: "turn-1".to_string(), + response, + }, + ) .expect("user input response should serialize") .expect("second user input request should be pending"); diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index daf992db59..2a053bff0a 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -377,7 +377,30 @@ async fn enqueue_primary_thread_session_replays_buffered_approval_after_attach() .note_server_request(&approval_request), None ); - app.enqueue_primary_thread_request(approval_request).await?; + app.agents_overview + .dispatched_requests + .insert(thread_id, vec![approval_request.clone()]); + app.enqueue_primary_thread_request(approval_request.clone()) + .await?; + let (mut app_server, requests, proxy) = session_lifecycle_requests::start_recording_app_server( + &app.config, + /*blocked_thread_list*/ None, + /*failed_thread_name*/ None, + ) + .await?; + app.handle_app_server_event( + &app_server, + codex_app_server_client::AppServerEvent::ServerNotification(Box::new( + ServerNotification::ServerRequestResolved( + codex_app_server_protocol::ServerRequestResolvedNotification { + thread_id: ThreadId::new().to_string(), + request_id: AppServerRequestId::Integer(1), + }, + ), + )), + ) + .await; + assert_eq!(app.agents_overview.dispatched_requests[&thread_id].len(), 1); app.enqueue_primary_thread_session( test_thread_session(thread_id, test_path_buf("/tmp/project")), Vec::new(), @@ -404,21 +427,49 @@ async fn enqueue_primary_thread_session_replays_buffered_approval_after_attach() )); app.handle_thread_event_now(event); - app.chat_widget - .handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + let background_thread_id = ThreadId::new(); + let mut background_request = approval_request; + let ServerRequest::CommandExecutionRequestApproval { request_id, params } = + &mut background_request + else { + unreachable!("expected a command approval request"); + }; + *request_id = AppServerRequestId::Integer(2); + params.thread_id = background_thread_id.to_string(); + app.pending_app_server_requests + .note_server_request(&background_request); + app.enqueue_thread_request(background_thread_id, background_request) + .await?; - while let Ok(app_event) = app_event_rx.try_recv() { - if let AppEvent::SubmitThreadOp { - thread_id: op_thread_id, - .. - } = app_event - { - assert_eq!(op_thread_id, thread_id); - return Ok(()); - } + for expected_thread_id in [thread_id, background_thread_id] { + assert!(app.chat_widget.has_active_view()); + app.chat_widget + .handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + let (actual_thread_id, op) = std::iter::from_fn(|| app_event_rx.try_recv().ok()) + .find_map(|event| match event { + AppEvent::SubmitThreadOp { thread_id, op } => Some((thread_id, op)), + _ => None, + }) + .expect("approval should submit a thread-scoped operation"); + assert_eq!(actual_thread_id, expected_thread_id); + app.submit_thread_op(&mut app_server, expected_thread_id, op) + .await?; } + assert!(!app.chat_widget.has_active_view()); - panic!("expected approval action to submit a thread-scoped op"); + app_server.shutdown().await?; + proxy.await??; + assert_eq!( + requests + .lock() + .expect("request recorder lock") + .iter() + .filter(|request| request.method == "server/request/response") + .map(|request| request.id.clone()) + .collect::>(), + (1..=2).map(AppServerRequestId::Integer).collect::>() + ); + Ok(()) } #[tokio::test] @@ -445,7 +496,7 @@ async fn resolved_buffered_approval_does_not_become_actionable_after_drain() -> let resolved = app .pending_app_server_requests - .resolve_notification(&AppServerRequestId::Integer(1)) + .resolve_notification(&thread_id.to_string(), &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() {} @@ -4874,11 +4925,14 @@ async fn background_side_cleanup_removes_local_state_and_ignores_late_events() - .await; let resolution = app .pending_app_server_requests - .take_resolution(Op::ExecApproval { - id: "approval-1".to_string(), - turn_id: None, - decision: codex_app_server_protocol::CommandExecutionApprovalDecision::Accept, - }) + .take_resolution( + &side_thread_id.to_string(), + Op::ExecApproval { + id: "approval-1".to_string(), + turn_id: None, + decision: codex_app_server_protocol::CommandExecutionApprovalDecision::Accept, + }, + ) .expect("approval resolution should serialize"); assert_eq!(resolution, None); Ok(()) diff --git a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs index a1fdd0c91e..23c2fe7e46 100644 --- a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs +++ b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs @@ -64,7 +64,7 @@ fn take_backfill_counts(requests: &RecordedRequests) -> (usize, usize) { } /// Starts an embedded app server behind a loopback WebSocket proxy that records JSON-RPC methods. -async fn start_recording_app_server( +pub(super) async fn start_recording_app_server( config: &Config, blocked_thread_list: Option<(ThreadId, oneshot::Sender<()>, oneshot::Receiver<()>)>, failed_thread_name: Option<&'static str>, @@ -255,7 +255,18 @@ async fn start_recording_app_server_with_history( )?) .await?; } - JSONRPCMessage::Response(_) | JSONRPCMessage::Error(_) => {} + JSONRPCMessage::Response(response) => { + request_sink + .lock() + .expect("request recorder lock") + .push(JSONRPCRequest { + id: response.id, + method: "server/request/response".to_string(), + params: Some(response.result), + trace: None, + }); + } + JSONRPCMessage::Error(_) => {} } } embedded.shutdown().await?; diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 016b25cbdb..d38606c3e7 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -876,7 +876,7 @@ impl App { ) -> Result { let Some(resolution) = self .pending_app_server_requests - .take_resolution(op) + .take_resolution(&thread_id.to_string(), op) .map_err(|err| color_eyre::eyre::eyre!(err))? else { return Ok(false); diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index d1fa6cc48f..c561dde317 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -120,7 +120,7 @@ pub(crate) struct McpElicitationApprovalRequest { } impl ApprovalRequest { - fn thread_id(&self) -> ThreadId { + pub(super) fn thread_id(&self) -> ThreadId { match self { ApprovalRequest::Exec(request) => request.thread_id, ApprovalRequest::Permissions(request) => request.thread_id, @@ -142,16 +142,16 @@ impl ApprovalRequest { match (self, request) { ( ApprovalRequest::Exec(request), - ResolvedAppServerRequest::ExecApproval { id: resolved_id }, - ) => request.id == *resolved_id, + ResolvedAppServerRequest::ExecApproval { thread_id, id }, + ) => request.thread_id.to_string() == *thread_id && request.id == *id, ( ApprovalRequest::Permissions(request), - ResolvedAppServerRequest::PermissionsApproval { id }, - ) => request.call_id == *id, + ResolvedAppServerRequest::PermissionsApproval { thread_id, id }, + ) => request.thread_id.to_string() == *thread_id && request.call_id == *id, ( ApprovalRequest::ApplyPatch(request), - ResolvedAppServerRequest::FileChangeApproval { id: resolved_id }, - ) => request.id == *resolved_id, + ResolvedAppServerRequest::FileChangeApproval { thread_id, id }, + ) => request.thread_id.to_string() == *thread_id && request.id == *id, ( ApprovalRequest::McpElicitation(request), ResolvedAppServerRequest::McpElicitation { @@ -1475,10 +1475,20 @@ mod tests { fn resolved_request_dismisses_overlay_without_emitting_abort() { let (tx, mut rx) = unbounded_channel::(); let tx = AppEventSender::new(tx); - let mut view = make_overlay(make_exec_request(), tx, Features::with_defaults()); + let request = make_exec_request(); + let thread_id = request.thread_id(); + let mut view = make_overlay(request, tx, Features::with_defaults()); + assert!( + !view.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + thread_id: ThreadId::new().to_string(), + id: "test".to_string(), + }) + ); + assert!(!view.is_complete()); assert!( view.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + thread_id: thread_id.to_string(), id: "test".to_string(), }) ); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index d22cb900d3..69296480f9 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -2018,7 +2018,7 @@ mod tests { } fn dismiss_app_server_request(&mut self, request: &ResolvedAppServerRequest) -> bool { - let ResolvedAppServerRequest::ExecApproval { id } = request else { + let ResolvedAppServerRequest::ExecApproval { id, .. } = request else { return false; }; if self.dismiss_exec_id != Some(id.as_str()) { @@ -2262,10 +2262,13 @@ mod tests { let mut pane = test_pane(tx); let now = Instant::now(); pane.last_composer_activity_at = Some(now); - pane.push_approval_request(exec_request(), &features); + let request = exec_request(); + let thread_id = request.thread_id(); + pane.push_approval_request(request, &features); assert!( pane.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + thread_id: thread_id.to_string(), id: "1".to_string(), }) ); @@ -2294,6 +2297,7 @@ mod tests { assert!( pane.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + thread_id: "thread-1".to_string(), id: "request-1".to_string(), }) ); @@ -2323,6 +2327,7 @@ mod tests { assert!( !pane.dismiss_app_server_request(&ResolvedAppServerRequest::ExecApproval { + thread_id: "thread-1".to_string(), id: "request-1".to_string(), }) ); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 66c3970656..331b347bf3 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1032,7 +1032,19 @@ impl ChatWidget { 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 request_thread_id = match request { + ResolvedAppServerRequest::ExecApproval { thread_id, .. } + | ResolvedAppServerRequest::FileChangeApproval { thread_id, .. } + | ResolvedAppServerRequest::PermissionsApproval { thread_id, .. } => { + Some(thread_id.as_str()) + } + ResolvedAppServerRequest::UserInput { .. } + | ResolvedAppServerRequest::McpElicitation { .. } => None, + }; + let removed_deferred = request_thread_id.is_none_or(|request_thread_id| { + self.thread_id + .is_some_and(|thread_id| thread_id.to_string() == request_thread_id) + }) && 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(); diff --git a/codex-rs/tui/src/chatwidget/interrupts.rs b/codex-rs/tui/src/chatwidget/interrupts.rs index 2f664345a6..35a9cb56d9 100644 --- a/codex-rs/tui/src/chatwidget/interrupts.rs +++ b/codex-rs/tui/src/chatwidget/interrupts.rs @@ -126,11 +126,11 @@ impl QueuedInterrupt { fn matches_resolved_prompt(&self, request: &ResolvedAppServerRequest) -> bool { match self { QueuedInterrupt::ExecApproval(ev) => { - matches!(request, ResolvedAppServerRequest::ExecApproval { id } + matches!(request, ResolvedAppServerRequest::ExecApproval { id, .. } if ev.effective_approval_id() == id.as_str()) } QueuedInterrupt::ApplyPatchApproval(ev) => { - matches!(request, ResolvedAppServerRequest::FileChangeApproval { id } + matches!(request, ResolvedAppServerRequest::FileChangeApproval { id, .. } if ev.call_id == id.as_str()) } QueuedInterrupt::Elicitation { request_id, params } => { @@ -140,7 +140,7 @@ impl QueuedInterrupt { } if params.server_name == server_name.as_str() && request_id == resolved_request_id) } QueuedInterrupt::RequestPermissions(ev) => { - matches!(request, ResolvedAppServerRequest::PermissionsApproval { id } + matches!(request, ResolvedAppServerRequest::PermissionsApproval { id, .. } if ev.call_id == id.as_str()) } QueuedInterrupt::RequestUserInput(ev) => { @@ -242,6 +242,7 @@ mod tests { assert!( !manager.remove_resolved_prompt(&ResolvedAppServerRequest::ExecApproval { + thread_id: "thread-1".to_string(), id: "call".to_string(), }) ); @@ -249,6 +250,7 @@ mod tests { assert!( manager.remove_resolved_prompt(&ResolvedAppServerRequest::ExecApproval { + thread_id: "thread-1".to_string(), id: "approval".to_string(), }) ); @@ -263,6 +265,7 @@ mod tests { assert!( !manager.remove_resolved_prompt(&ResolvedAppServerRequest::ExecApproval { + thread_id: "thread-1".to_string(), id: "call".to_string(), }) );