diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 1da0e0ae6c..f6f48ce9ee 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -199,6 +199,7 @@ use toml::Value as TomlValue; use uuid::Uuid; mod agent_message_consolidation; mod agent_navigation; +mod agent_selection; mod agent_status_feed; mod app_server_event_targets; mod app_server_events; @@ -207,6 +208,7 @@ mod background_requests; mod config_persistence; mod conversation_events; mod conversation_panes; +mod conversation_retirement; mod event_dispatch; mod history_ui; mod input; @@ -229,6 +231,7 @@ mod thread_settings; use self::agent_navigation::AgentNavigationDirection; use self::agent_navigation::AgentNavigationState; +use self::agent_selection::PreparedAgentSelection; use self::app_server_requests::PendingAppServerRequests; use self::conversation_panes::ConversationPaneInit; use self::conversation_panes::ConversationPanes; @@ -566,6 +569,7 @@ pub(crate) struct App { thread_event_listener_tasks: HashMap>, agent_navigation: AgentNavigationState, side_threads: HashMap, + retired_thread_ids: VecDeque, primary_thread_id: Option, last_subagent_backfill_attempt: Option, primary_session_configured: Option, @@ -1057,6 +1061,7 @@ See the Codex keymap documentation for supported actions and examples." thread_event_listener_tasks: HashMap::new(), agent_navigation: AgentNavigationState::default(), side_threads: HashMap::new(), + retired_thread_ids: VecDeque::new(), primary_thread_id: None, last_subagent_backfill_attempt: None, primary_session_configured: None, diff --git a/codex-rs/tui/src/app/agent_selection.rs b/codex-rs/tui/src/app/agent_selection.rs new file mode 100644 index 0000000000..3006c4a18e --- /dev/null +++ b/codex-rs/tui/src/app/agent_selection.rs @@ -0,0 +1,87 @@ +//! Fallible preparation for switching the Parent pane to another agent thread. +//! +//! Preparing separately lets `/side` validate and attach the destination before it tears down the +//! Side pane. Applying the prepared selection stays in `session_lifecycle`, next to the existing +//! widget replacement and replay transition. + +use super::*; + +pub(super) struct PreparedAgentSelection { + pub(super) thread_id: ThreadId, + pub(super) is_replay_only: bool, + pub(super) attached_replay_only: bool, +} + +impl App { + pub(super) async fn prepare_agent_thread_selection( + &mut self, + app_server: &mut AppServerSession, + thread_id: ThreadId, + ) -> Option { + if self.chat_widget.active_thread_id == Some(thread_id) { + return Some(PreparedAgentSelection { + thread_id, + is_replay_only: false, + attached_replay_only: false, + }); + } + + if !self + .refresh_agent_picker_thread_liveness(app_server, thread_id) + .await + { + self.chat_widget + .add_error_message(format!("Agent thread {thread_id} is no longer available.")); + return None; + } + + let mut is_replay_only = self + .agent_navigation + .get(&thread_id) + .is_some_and(|entry| entry.is_closed); + let mut attached_replay_only = false; + if self.should_attach_live_thread_for_selection(thread_id) { + match self + .attach_live_thread_for_selection(app_server, thread_id) + .await + { + Ok(live_attached) => { + attached_replay_only = !live_attached; + if attached_replay_only { + is_replay_only = true; + } + } + Err(err) => { + self.chat_widget.add_error_message(format!( + "Failed to attach to agent thread {thread_id}: {err}" + )); + return None; + } + } + } else if !self.thread_event_channels.contains_key(&thread_id) && is_replay_only { + self.chat_widget + .add_error_message(format!("Agent thread {thread_id} is no longer available.")); + return None; + } + + let target_is_attached = self + .chat_widget + .by_thread_id(thread_id) + .is_some_and(|pane| pane.active_thread_id == Some(thread_id)); + let receiver_is_available = self + .thread_event_channels + .get(&thread_id) + .is_some_and(|channel| channel.receiver.is_some()); + if !target_is_attached && !receiver_is_available { + self.chat_widget + .add_error_message(format!("Agent thread {thread_id} is already active.")); + return None; + } + + Some(PreparedAgentSelection { + thread_id, + is_replay_only, + attached_replay_only, + }) + } +} diff --git a/codex-rs/tui/src/app/app_server_events.rs b/codex-rs/tui/src/app/app_server_events.rs index cda977096a..a000970ad7 100644 --- a/codex-rs/tui/src/app/app_server_events.rs +++ b/codex-rs/tui/src/app/app_server_events.rs @@ -154,6 +154,10 @@ impl App { match server_notification_thread_target(¬ification) { ServerNotificationThreadTarget::Thread(thread_id) => { + if self.is_thread_retired(&thread_id) { + tracing::debug!(%thread_id, "ignoring notification for retired conversation"); + return; + } let result = if self.primary_thread_id == Some(thread_id) || self.primary_thread_id.is_none() { @@ -193,6 +197,24 @@ impl App { app_server_client: &AppServerSession, request: ServerRequest, ) { + let thread_id = server_request_thread_id(&request); + if thread_id.is_some_and(|thread_id| self.is_thread_retired(&thread_id)) { + let request_id = request.id().clone(); + let reason = "Conversation closed before this request could be handled.".to_string(); + tracing::debug!( + ?request_id, + ?thread_id, + "rejecting request for retired conversation" + ); + if let Err(err) = self + .reject_app_server_request(app_server_client, request_id, reason) + .await + { + tracing::warn!("{err}"); + } + return; + } + if let Some(unsupported) = self .pending_app_server_requests .note_server_request(&request) @@ -217,7 +239,7 @@ impl App { return; } - let Some(thread_id) = server_request_thread_id(&request) else { + let Some(thread_id) = thread_id else { tracing::warn!("ignoring threadless app-server request"); return; }; diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 0fb97d5dee..6d41ac394f 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -88,6 +88,10 @@ impl PendingAppServerRequests { self.by_thread.clear(); } + pub(super) fn discard_thread(&mut self, thread_id: ThreadId) { + self.by_thread.remove(&thread_id); + } + pub(super) fn note_server_request( &mut self, request: &ServerRequest, diff --git a/codex-rs/tui/src/app/conversation_retirement.rs b/codex-rs/tui/src/app/conversation_retirement.rs new file mode 100644 index 0000000000..edf9dd28d2 --- /dev/null +++ b/codex-rs/tui/src/app/conversation_retirement.rs @@ -0,0 +1,33 @@ +//! Bounded retirement tombstones for conversations removed from the visible pane set. +//! +//! Tombstones reject late app-server work from a closed pane without permanently preventing an +//! authoritative resume of the same thread. + +use super::*; + +const RETIRED_THREAD_ID_CAPACITY: usize = 32; + +impl App { + pub(super) fn is_thread_retired(&self, thread_id: &ThreadId) -> bool { + self.retired_thread_ids.contains(thread_id) + } + + pub(super) fn retire_thread(&mut self, thread_id: ThreadId) { + if self.is_thread_retired(&thread_id) { + return; + } + if self.retired_thread_ids.len() >= RETIRED_THREAD_ID_CAPACITY { + self.retired_thread_ids.pop_front(); + } + self.retired_thread_ids.push_back(thread_id); + } + + pub(super) fn restore_thread(&mut self, thread_id: ThreadId) { + self.retired_thread_ids + .retain(|retired_thread_id| *retired_thread_id != thread_id); + } +} + +#[cfg(test)] +#[path = "conversation_retirement_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/app/conversation_retirement_tests.rs b/codex-rs/tui/src/app/conversation_retirement_tests.rs new file mode 100644 index 0000000000..cdb332e04b --- /dev/null +++ b/codex-rs/tui/src/app/conversation_retirement_tests.rs @@ -0,0 +1,14 @@ +use super::*; +use crate::app::test_support::make_test_app; +use pretty_assertions::assert_eq; + +#[tokio::test] +async fn retirement_tombstones_are_bounded() { + let mut app = make_test_app().await; + + for _ in 0..=RETIRED_THREAD_ID_CAPACITY { + app.retire_thread(ThreadId::new()); + } + + assert_eq!(app.retired_thread_ids.len(), RETIRED_THREAD_ID_CAPACITY); +} diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 57bec2b859..53afe15e1a 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -367,9 +367,12 @@ impl App { .by_slot(PaneSlot::Side) .and_then(|pane| pane.active_thread_id.or(pane.chat_widget.thread_id())) { - if self.discard_side_thread(app_server, side_thread_id).await { - self.surface_pending_inactive_thread_interactive_requests() - .await?; + if self.discard_side_thread(app_server, side_thread_id).await + && let Err(err) = self + .surface_pending_inactive_thread_interactive_requests() + .await + { + tracing::warn!(%err, "failed to surface pending requests after closing side pane"); } tui.frame_requester().schedule_frame(); return Ok(AppRunControl::Continue); @@ -483,6 +486,10 @@ impl App { self.append_message_history_entry(thread_id, text); } AppEvent::SyncThreadGitBranch { thread_id, branch } => { + if self.is_thread_retired(&thread_id) { + tracing::debug!(%thread_id, "ignoring branch sync for retired conversation"); + return Ok(AppRunControl::Continue); + } if let Err(err) = app_server .thread_metadata_update_branch(thread_id, branch) .await diff --git a/codex-rs/tui/src/app/session_lifecycle.rs b/codex-rs/tui/src/app/session_lifecycle.rs index e6c16470a4..999735ea5e 100644 --- a/codex-rs/tui/src/app/session_lifecycle.rs +++ b/codex-rs/tui/src/app/session_lifecycle.rs @@ -264,9 +264,15 @@ impl App { app_server: &mut AppServerSession, thread_id: ThreadId, ) -> Result { - if self.thread_event_channels.contains_key(&thread_id) { + if self.thread_event_channels.contains_key(&thread_id) + && !self.is_thread_retired(&thread_id) + { return Ok(true); } + if self.is_thread_retired(&thread_id) { + self.abort_thread_event_listener(thread_id); + self.thread_event_channels.remove(&thread_id); + } let (session, turns, live_attached) = match app_server .resume_thread(self.config.clone(), thread_id) @@ -309,6 +315,9 @@ impl App { (session, turns, false) } }; + if live_attached { + self.restore_thread(thread_id); + } let channel = self.ensure_thread_channel(thread_id); if !live_attached { channel.mark_replay_only(); @@ -358,45 +367,28 @@ impl App { app_server: &mut AppServerSession, thread_id: ThreadId, ) -> Result<()> { - if self.chat_widget.active_thread_id == Some(thread_id) { - return Ok(()); - } - - if !self - .refresh_agent_picker_thread_liveness(app_server, thread_id) + let Some(selection) = self + .prepare_agent_thread_selection(app_server, thread_id) .await - { - self.chat_widget - .add_error_message(format!("Agent thread {thread_id} is no longer available.")); + else { return Ok(()); - } + }; + self.apply_prepared_agent_thread_selection(tui, app_server, selection) + .await + } - let mut is_replay_only = self - .agent_navigation - .get(&thread_id) - .is_some_and(|entry| entry.is_closed); - let mut attached_replay_only = false; - if self.should_attach_live_thread_for_selection(thread_id) { - match self - .attach_live_thread_for_selection(app_server, thread_id) - .await - { - Ok(live_attached) => { - attached_replay_only = !live_attached; - if attached_replay_only { - is_replay_only = true; - } - } - Err(err) => { - self.chat_widget.add_error_message(format!( - "Failed to attach to agent thread {thread_id}: {err}" - )); - return Ok(()); - } - } - } else if !self.thread_event_channels.contains_key(&thread_id) && is_replay_only { - self.chat_widget - .add_error_message(format!("Agent thread {thread_id} is no longer available.")); + pub(super) async fn apply_prepared_agent_thread_selection( + &mut self, + tui: &mut tui::Tui, + app_server: &mut AppServerSession, + selection: PreparedAgentSelection, + ) -> Result<()> { + let PreparedAgentSelection { + thread_id, + is_replay_only, + attached_replay_only, + } = selection; + if self.chat_widget.active_thread_id == Some(thread_id) { return Ok(()); } @@ -450,7 +442,7 @@ impl App { } pub(super) fn should_attach_live_thread_for_selection(&self, thread_id: ThreadId) -> bool { - !self.thread_event_channels.contains_key(&thread_id) + (self.is_thread_retired(&thread_id) || !self.thread_event_channels.contains_key(&thread_id)) && self .agent_navigation .get(&thread_id) diff --git a/codex-rs/tui/src/app/side.rs b/codex-rs/tui/src/app/side.rs index bc66fa6836..7f1a0981dd 100644 --- a/codex-rs/tui/src/app/side.rs +++ b/codex-rs/tui/src/app/side.rs @@ -1,11 +1,7 @@ //! Transient side-conversation threads. //! -//! A side conversation is an ephemeral fork used for a quick /side question while keeping the -//! primary thread focused. This module owns the app-level lifecycle for those forks: switching into -//! them, returning to their parent, and discarding them when normal thread navigation moves -//! elsewhere. The fork receives hidden developer instructions that make inherited history reference -//! material only and steer the agent away from mutations unless the side conversation explicitly asks -//! for them. +//! A side conversation is an ephemeral fork for a quick `/side` question next to its parent. It +//! makes inherited history reference-only unless the side conversation explicitly requests mutation. use super::conversation_panes::ConversationPaneInit; use super::*; @@ -108,70 +104,8 @@ impl SideParentStatus { } #[cfg(test)] -mod tests { - use super::*; - use pretty_assertions::assert_eq; - - #[test] - fn side_boundary_prompt_marks_inherited_history_reference_only() { - let item = App::side_boundary_prompt_item(); - let ResponseItem::Message { role, content, .. } = item else { - panic!("expected hidden side boundary prompt to be a user message"); - }; - assert_eq!(role, "user"); - let [ContentItem::InputText { text }] = content.as_slice() else { - panic!("expected hidden side boundary prompt text"); - }; - assert!(text.contains("Side conversation boundary.")); - assert!(text.contains("Everything before this boundary is inherited history")); - assert!(text.contains("It is not your current task.")); - assert!(text.contains("Only messages submitted after this boundary are active")); - assert!(text.contains("Do not continue, execute, or complete")); - assert!(text.contains("separate from the main thread")); - assert!( - text.contains("External tools may be available according to this thread's current") - ); - assert!(text.contains("Any tool calls or outputs visible before this boundary happened")); - assert!(text.contains("Sub-agents are off-limits in this side conversation.")); - assert!(text.contains("Do not modify files")); - } - - #[test] - fn side_start_error_message_explains_missing_first_prompt() { - let err = color_eyre::eyre::eyre!( - "thread/fork failed during TUI bootstrap: thread/fork failed: no rollout found for thread id 019da1a1-bed9-7a43-88a2-b49d43915021" - ); - - assert_eq!( - App::side_start_error_message(&err), - "'/side' is unavailable until the current conversation has started. Send a message first, then try /side again." - ); - } - - #[test] - fn side_start_error_message_uses_generic_start_wording() { - let err = color_eyre::eyre::eyre!("transport disconnected"); - - assert_eq!( - App::side_start_error_message(&err), - "Failed to start side conversation: transport disconnected" - ); - } - - #[test] - fn side_developer_instructions_appends_existing_policy() { - let developer_instructions = - App::side_developer_instructions(Some("Existing developer policy.")); - - assert!(developer_instructions.contains("Existing developer policy.")); - assert!( - developer_instructions.contains("You are in a side conversation, not the main thread.") - ); - assert!( - developer_instructions.contains("Sub-agents are off-limits in this side conversation.") - ); - } -} +#[path = "side_tests.rs"] +mod tests; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(super) enum SideParentStatusChange { @@ -318,7 +252,7 @@ impl App { let Some(side_origin) = self .chat_widget .by_slot(PaneSlot::Side) - .and_then(|pane| pane.origin()) + .and_then(super::conversation_panes::ConversationPane::origin) else { return false; }; @@ -433,18 +367,6 @@ impl App { true } - pub(super) fn side_thread_to_discard_after_switch( - &self, - target_thread_id: ThreadId, - ) -> Option { - let side_thread_id = self.installed_side_thread_id()?; - if target_thread_id == side_thread_id { - return None; - } - - Some(side_thread_id) - } - pub(super) async fn discard_side_thread( &mut self, app_server: &mut AppServerSession, @@ -472,8 +394,12 @@ impl App { pub(super) async fn discard_thread_local_state(&mut self, thread_id: ThreadId) { let remove_side_pane = self.installed_side_thread_id() == Some(thread_id); + if remove_side_pane || self.side_threads.contains_key(&thread_id) { + self.retire_thread(thread_id); + } self.abort_thread_event_listener(thread_id); self.thread_event_channels.remove(&thread_id); + self.pending_app_server_requests.discard_thread(thread_id); self.side_threads.remove(&thread_id); self.agent_navigation.remove(thread_id); if remove_side_pane { @@ -536,8 +462,12 @@ impl App { user_message: Option, message: String, ) { - self.discard_side_thread_or_keep_visible(tui, app_server, thread_id) + let cleaned = self + .discard_side_thread_or_keep_visible(tui, app_server, thread_id) .await; + if !cleaned && self.installed_side_thread_id() != Some(thread_id) { + self.discard_thread_local_state(thread_id).await; + } self.restore_side_user_message(user_message); self.add_side_thread_error(thread_id, message); } @@ -635,14 +565,25 @@ impl App { return Ok(()); } - if let Some(side_thread_id) = self.side_thread_to_discard_after_switch(thread_id) { - if !self.discard_side_thread(app_server, side_thread_id).await { - self.keep_side_thread_visible_after_cleanup_failure(tui, side_thread_id); - return Ok(()); - } + let Some(selection) = self + .prepare_agent_thread_selection(app_server, thread_id) + .await + else { + self.chat_widget.focus(PaneSlot::Side); + tui.frame_requester().schedule_frame(); + return Ok(()); + }; + if let Some(side_thread_id) = self + .installed_side_thread_id() + .filter(|side_thread_id| *side_thread_id != thread_id) + && !self.discard_side_thread(app_server, side_thread_id).await + { + self.keep_side_thread_visible_after_cleanup_failure(tui, side_thread_id); + return Ok(()); } self.chat_widget.focus(PaneSlot::Parent); - self.select_agent_thread(tui, app_server, thread_id).await?; + self.apply_prepared_agent_thread_selection(tui, app_server, selection) + .await?; self.surface_pending_inactive_thread_interactive_requests() .await?; Ok(()) @@ -724,6 +665,11 @@ impl App { return Ok(AppRunControl::Continue); } + let Some(side) = self.chat_widget.by_slot_mut(PaneSlot::Side) else { + unreachable!("side pane was installed above"); + }; + side.attach_thread(child_thread_id, /*receiver*/ None); + let Some((receiver, snapshot)) = self.activate_thread_for_replay(child_thread_id).await else { diff --git a/codex-rs/tui/src/app/side_tests.rs b/codex-rs/tui/src/app/side_tests.rs new file mode 100644 index 0000000000..ca2d28cd13 --- /dev/null +++ b/codex-rs/tui/src/app/side_tests.rs @@ -0,0 +1,122 @@ +use super::*; +use crate::app::test_support::make_test_app_with_channels; +use pretty_assertions::assert_eq; + +#[test] +fn side_boundary_prompt_marks_inherited_history_reference_only() { + let item = App::side_boundary_prompt_item(); + let ResponseItem::Message { role, content, .. } = item else { + panic!("expected hidden side boundary prompt to be a user message"); + }; + assert_eq!(role, "user"); + let [ContentItem::InputText { text }] = content.as_slice() else { + panic!("expected hidden side boundary prompt text"); + }; + assert!(text.contains("Side conversation boundary.")); + assert!(text.contains("Everything before this boundary is inherited history")); + assert!(text.contains("It is not your current task.")); + assert!(text.contains("Only messages submitted after this boundary are active")); + assert!(text.contains("Do not continue, execute, or complete")); + assert!(text.contains("separate from the main thread")); + assert!(text.contains("External tools may be available according to this thread's current")); + assert!(text.contains("Any tool calls or outputs visible before this boundary happened")); + assert!(text.contains("Sub-agents are off-limits in this side conversation.")); + assert!(text.contains("Do not modify files")); +} + +#[test] +fn side_start_error_message_explains_missing_first_prompt() { + let err = color_eyre::eyre::eyre!( + "thread/fork failed during TUI bootstrap: thread/fork failed: no rollout found for thread id 019da1a1-bed9-7a43-88a2-b49d43915021" + ); + + assert_eq!( + App::side_start_error_message(&err), + "'/side' is unavailable until the current conversation has started. Send a message first, then try /side again." + ); +} + +#[test] +fn side_start_error_message_uses_generic_start_wording() { + let err = color_eyre::eyre::eyre!("transport disconnected"); + + assert_eq!( + App::side_start_error_message(&err), + "Failed to start side conversation: transport disconnected" + ); +} + +#[test] +fn side_developer_instructions_appends_existing_policy() { + let developer_instructions = + App::side_developer_instructions(Some("Existing developer policy.")); + + assert!(developer_instructions.contains("Existing developer policy.")); + assert!( + developer_instructions.contains("You are in a side conversation, not the main thread.") + ); + assert!( + developer_instructions.contains("Sub-agents are off-limits in this side conversation.") + ); +} + +#[tokio::test] +async fn failed_preinstall_cleanup_removes_invisible_side_state() -> Result<()> { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let parent_thread_id = ThreadId::new(); + let side_thread_id = ThreadId::new(); + app.side_threads + .insert(side_thread_id, SideThreadState::new(parent_thread_id)); + app.ensure_thread_channel(side_thread_id); + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + + app.fail_side_start( + &mut tui, + &mut app_server, + side_thread_id, + Some(crate::chatwidget::UserMessage::from("side question")), + "Failed to install side conversation.".to_string(), + ) + .await; + + assert!(!app.side_threads.contains_key(&side_thread_id)); + assert!(!app.thread_event_channels.contains_key(&side_thread_id)); + assert!(app.is_thread_retired(&side_thread_id)); + assert!(!app.chat_widget.has_side()); + assert_eq!( + app.chat_widget.composer_text_with_pending(), + "side question" + ); + let mut rendered_errors = Vec::new(); + while let Ok(event) = app_event_rx.try_recv() { + let cell = match event { + AppEvent::FromConversation { target, event } => { + assert_eq!(target.pane, PaneSlot::Parent); + match *event { + AppEvent::InsertHistoryCell(cell) => Some(cell), + _ => None, + } + } + AppEvent::InsertHistoryCell(cell) => Some(cell), + _ => None, + }; + if let Some(cell) = cell { + rendered_errors.push( + cell.display_lines(/*width*/ 80) + .iter() + .flat_map(|line| &line.spans) + .map(|span| span.content.as_ref()) + .collect::(), + ); + } + } + assert!( + rendered_errors + .iter() + .any(|error| error.contains("Failed to install side conversation.")), + "startup error should remain visible in Parent: {rendered_errors:?}" + ); + Ok(()) +} diff --git a/codex-rs/tui/src/app/test_support.rs b/codex-rs/tui/src/app/test_support.rs index cba00dcfce..aecebd8797 100644 --- a/codex-rs/tui/src/app/test_support.rs +++ b/codex-rs/tui/src/app/test_support.rs @@ -5,12 +5,17 @@ //! app-owned behavior. use super::*; +use crate::app_command::AppCommand; use crate::chatwidget::tests::make_chatwidget_manual_with_sender; use codex_models_manager::test_support::construct_model_info_offline_for_tests; use codex_models_manager::test_support::get_model_offline_for_tests; -pub(super) async fn make_test_app() -> App { - let (chat_widget, app_event_tx, _rx, _op_rx) = make_chatwidget_manual_with_sender().await; +pub(super) async fn make_test_app_with_channels() -> ( + App, + tokio::sync::mpsc::UnboundedReceiver, + tokio::sync::mpsc::UnboundedReceiver, +) { + let (chat_widget, app_event_tx, rx, op_rx) = make_chatwidget_manual_with_sender().await; let config = chat_widget.config_ref().clone(); let file_search = FileSearchManager::new(config.cwd.to_path_buf(), app_event_tx.clone()); let model = get_model_offline_for_tests(config.model.as_deref()); @@ -23,50 +28,60 @@ pub(super) async fn make_test_app() -> App { unreachable!("test chat widget must use the parent pane scope"); }; - App { - model_catalog: chat_widget.model_catalog(), - session_telemetry, - app_event_tx, - chat_widget, - workspace_command_runner: None, - config, - state_db: None, - cli_kv_overrides: Vec::new(), - harness_overrides: ConfigOverrides::default(), - loader_overrides: LoaderOverrides::without_managed_config_for_tests(), - cloud_config_bundle: CloudConfigBundleLoader::default(), - runtime_approval_policy_override: None, - runtime_permission_profile_override: None, - overlay: None, - deferred_history_lines: Vec::new(), - has_emitted_history_lines: false, - enhanced_keys_supported: false, - keymap: crate::keymap::RuntimeKeymap::defaults(), - status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)), - terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)), - skill_load_warnings: SkillLoadWarningState::default(), - backtrack: BacktrackState::default(), - backtrack_render_pending: false, - feedback: codex_feedback::CodexFeedback::new(), - feedback_audience: FeedbackAudience::External, - environment_manager: Arc::new(EnvironmentManager::default_for_tests()), - app_server_target: crate::AppServerTarget::Embedded, - pending_update_action: None, - pending_shutdown_exit_thread_id: None, - windows_sandbox: WindowsSandboxState::default(), - thread_event_channels: HashMap::new(), - thread_event_listener_tasks: HashMap::new(), - agent_navigation: AgentNavigationState::default(), - side_threads: HashMap::new(), - primary_thread_id: None, - last_subagent_backfill_attempt: None, - primary_session_configured: None, - pending_primary_events: VecDeque::new(), - pending_app_server_requests: PendingAppServerRequests::default(), - pending_startup_thread_start: false, - pending_plugin_enabled_writes: HashMap::new(), - pending_hook_enabled_writes: HashMap::new(), - } + ( + App { + model_catalog: chat_widget.model_catalog(), + session_telemetry, + app_event_tx, + chat_widget, + workspace_command_runner: None, + config, + state_db: None, + cli_kv_overrides: Vec::new(), + harness_overrides: ConfigOverrides::default(), + loader_overrides: LoaderOverrides::without_managed_config_for_tests(), + cloud_config_bundle: CloudConfigBundleLoader::default(), + runtime_approval_policy_override: None, + runtime_permission_profile_override: None, + overlay: None, + deferred_history_lines: Vec::new(), + has_emitted_history_lines: false, + enhanced_keys_supported: false, + keymap: crate::keymap::RuntimeKeymap::defaults(), + status_line_invalid_items_warned: Arc::new(AtomicBool::new(false)), + terminal_title_invalid_items_warned: Arc::new(AtomicBool::new(false)), + skill_load_warnings: SkillLoadWarningState::default(), + backtrack: BacktrackState::default(), + backtrack_render_pending: false, + feedback: codex_feedback::CodexFeedback::new(), + feedback_audience: FeedbackAudience::External, + environment_manager: Arc::new(EnvironmentManager::default_for_tests()), + app_server_target: crate::AppServerTarget::Embedded, + pending_update_action: None, + pending_shutdown_exit_thread_id: None, + windows_sandbox: WindowsSandboxState::default(), + thread_event_channels: HashMap::new(), + thread_event_listener_tasks: HashMap::new(), + agent_navigation: AgentNavigationState::default(), + side_threads: HashMap::new(), + retired_thread_ids: VecDeque::new(), + primary_thread_id: None, + last_subagent_backfill_attempt: None, + primary_session_configured: None, + pending_primary_events: VecDeque::new(), + pending_app_server_requests: PendingAppServerRequests::default(), + pending_startup_thread_start: false, + pending_plugin_enabled_writes: HashMap::new(), + pending_hook_enabled_writes: HashMap::new(), + }, + rx, + op_rx, + ) +} + +pub(super) async fn make_test_app() -> App { + let (app, _rx, _op_rx) = make_test_app_with_channels().await; + app } fn test_session_telemetry(config: &Config, model: &str) -> SessionTelemetry { diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 49598f8f59..a201447cda 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -3843,20 +3843,63 @@ async fn start_side_installs_live_child_without_replacing_parent() -> Result<()> } #[tokio::test] -async fn side_discard_selection_keeps_current_side_thread() { +async fn failed_agent_selection_keeps_side_conversation() -> Result<()> { let mut app = make_test_app().await; let parent_thread_id = ThreadId::new(); let side_thread_id = ThreadId::new(); install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; + let unavailable_thread_id = ThreadId::new(); + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; - assert_eq!( - app.side_thread_to_discard_after_switch(side_thread_id), - None + app.select_agent_thread_and_discard_side(&mut tui, &mut app_server, unavailable_thread_id) + .await?; + + assert!(app.chat_widget.has_side()); + assert_eq!(app.chat_widget.focused_slot(), PaneSlot::Side); + assert_eq!(app.chat_widget.active_thread_id, Some(side_thread_id)); + assert!(app.side_threads.contains_key(&side_thread_id)); + Ok(()) +} + +#[tokio::test] +async fn failed_side_cleanup_does_not_switch_parent() -> Result<()> { + let mut app = make_test_app().await; + let parent_thread_id = ThreadId::new(); + let side_thread_id = ThreadId::new(); + let target_thread_id = ThreadId::new(); + install_test_side_pane(&mut app, parent_thread_id, side_thread_id).await; + app.thread_event_channels.insert( + target_thread_id, + ThreadEventChannel::new_with_session( + /*capacity*/ 4, + test_thread_session(target_thread_id, test_path_buf("/tmp/target")), + Vec::new(), + ), ); + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + + app.select_agent_thread_and_discard_side(&mut tui, &mut app_server, target_thread_id) + .await?; + + assert!(app.chat_widget.has_side()); + assert_eq!(app.chat_widget.focused_slot(), PaneSlot::Side); + assert_eq!(app.chat_widget.active_thread_id, Some(side_thread_id)); assert_eq!( - app.side_thread_to_discard_after_switch(parent_thread_id), - Some(side_thread_id) + app.chat_widget + .by_slot(PaneSlot::Parent) + .and_then(|pane| pane.active_thread_id), + Some(parent_thread_id) ); + assert!( + app.thread_event_channels + .get(&target_thread_id) + .is_some_and(|channel| channel.receiver.is_some()) + ); + Ok(()) } #[tokio::test] @@ -3935,7 +3978,7 @@ async fn discard_side_thread_keeps_local_state_when_server_close_fails() -> Resu } #[tokio::test] -async fn discard_closed_side_thread_removes_local_state_without_server_rpc() { +async fn discard_closed_side_thread_ignores_late_server_events() -> Result<()> { let mut app = make_test_app().await; let parent_thread_id = ThreadId::new(); let side_thread_id = ThreadId::new(); @@ -3948,6 +3991,17 @@ async fn discard_closed_side_thread_removes_local_state_without_server_rpc() { Some("side".to_string()), /*is_closed*/ false, ); + let pending_request = exec_approval_request( + side_thread_id, + "turn-pending", + "call-pending", + /*approval_id*/ None, + ); + assert_eq!( + app.pending_app_server_requests + .note_server_request(&pending_request), + None + ); app.discard_closed_side_thread(side_thread_id).await; @@ -3956,6 +4010,92 @@ async fn discard_closed_side_thread_removes_local_state_without_server_rpc() { assert!(!app.side_threads.contains_key(&side_thread_id)); assert!(!app.thread_event_channels.contains_key(&side_thread_id)); assert_eq!(app.agent_navigation.get(&side_thread_id), None); + assert!( + !app.pending_app_server_requests + .contains_server_request(&pending_request) + ); + + app.primary_thread_id = None; + let app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + app.handle_app_server_event( + &app_server, + codex_app_server_client::AppServerEvent::ServerNotification(thread_closed_notification( + side_thread_id, + )), + ) + .await; + assert!(!app.thread_event_channels.contains_key(&side_thread_id)); + assert!(app.pending_primary_events.is_empty()); + + let late_request = exec_approval_request( + side_thread_id, + "turn-late", + "call-late", + /*approval_id*/ None, + ); + app.handle_app_server_event( + &app_server, + codex_app_server_client::AppServerEvent::ServerRequest(late_request.clone()), + ) + .await; + assert!(!app.thread_event_channels.contains_key(&side_thread_id)); + assert!( + !app.pending_app_server_requests + .contains_server_request(&late_request) + ); + + let unrelated_thread_id = ThreadId::new(); + app.enqueue_thread_notification( + unrelated_thread_id, + thread_closed_notification(unrelated_thread_id), + ) + .await + .expect("unrelated close should retain normal routing"); + assert!(app.thread_event_channels.contains_key(&unrelated_thread_id)); + + app.enqueue_primary_thread_session( + test_thread_session(side_thread_id, test_path_buf("/tmp/side")), + Vec::new(), + ) + .await?; + assert!(!app.is_thread_retired(&side_thread_id)); + Ok(()) +} + +#[tokio::test] +async fn retired_thread_ignores_late_git_branch_sync() -> Result<()> { + let mut app = make_test_app().await; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + let thread_id = app_server + .start_thread(app.chat_widget.config_ref()) + .await? + .session + .thread_id; + app.retire_thread(thread_id); + let mut tui = crate::tui::test_support::make_test_tui()?; + + let control = app + .handle_event( + &mut tui, + &mut app_server, + AppEvent::SyncThreadGitBranch { + thread_id, + branch: "retired-branch-must-not-write".to_string(), + }, + ) + .await?; + + assert!(matches!(control, AppRunControl::Continue)); + let thread = app_server + .thread_read(thread_id, /*include_turns*/ false) + .await?; + assert_ne!( + thread.git_info.and_then(|git_info| git_info.branch), + Some("retired-branch-must-not-write".to_string()) + ); + Ok(()) } #[tokio::test] @@ -4287,6 +4427,7 @@ async fn make_test_app() -> App { thread_event_listener_tasks: HashMap::new(), agent_navigation: AgentNavigationState::default(), side_threads: HashMap::new(), + retired_thread_ids: VecDeque::new(), primary_thread_id: None, last_subagent_backfill_attempt: None, primary_session_configured: None, @@ -4352,6 +4493,7 @@ async fn make_test_app_with_channels() -> ( thread_event_listener_tasks: HashMap::new(), agent_navigation: AgentNavigationState::default(), side_threads: HashMap::new(), + retired_thread_ids: VecDeque::new(), primary_thread_id: None, last_subagent_backfill_attempt: None, primary_session_configured: None, diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 3233590458..237864cebd 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -436,6 +436,10 @@ impl App { thread_id: ThreadId, op: AppCommand, ) -> Result<()> { + if self.is_thread_retired(&thread_id) { + tracing::debug!(%thread_id, "ignoring operation for retired conversation"); + return Ok(()); + } crate::session_log::log_outbound_op(&op); if self @@ -881,6 +885,10 @@ impl App { thread_id: ThreadId, notification: ServerNotification, ) -> Result<()> { + if self.is_thread_retired(&thread_id) { + tracing::debug!(%thread_id, "ignoring notification for retired conversation"); + return Ok(()); + } if matches!(notification, ServerNotification::ThreadSettingsUpdated(_)) && self.primary_thread_id.is_some() && self.primary_thread_id != Some(thread_id) @@ -1020,6 +1028,10 @@ impl App { thread_id: ThreadId, request: ServerRequest, ) -> Result<()> { + if self.is_thread_retired(&thread_id) { + tracing::debug!(%thread_id, "ignoring request for retired conversation"); + return Ok(()); + } let inactive_interactive_request = if !self.chat_widget.contains_thread(thread_id) { self.interactive_request_for_thread_request(thread_id, &request) .await? @@ -1114,6 +1126,7 @@ impl App { turns: Vec, ) -> Result<()> { let thread_id = session.thread_id; + self.restore_thread(thread_id); self.primary_thread_id = Some(thread_id); self.primary_session_configured = Some(session.clone()); self.upsert_agent_picker_thread(