openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 17:38:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(tui): harden side conversation switching

Browse Source
This commit is contained in:
Felipe Coury
2026-07-04 18:46:59 -03:00
parent b3edff66a9
commit 3ef12ab1d2
13 changed files with 587 additions and 185 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/app.rs
+5
View File
@@ -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<ThreadId, JoinHandle<()>>,
agent_navigation: AgentNavigationState,
side_threads: HashMap<ThreadId, SideThreadState>,
retired_thread_ids: VecDeque<ThreadId>,
primary_thread_id: Option<ThreadId>,
last_subagent_backfill_attempt: Option<ThreadId>,
primary_session_configured: Option<ThreadSessionState>,
@@ -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,
codex-rs/tui/src/app/agent_selection.rs
+87
View File
@@ -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<PreparedAgentSelection> {
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,
})
}
}
codex-rs/tui/src/app/app_server_events.rs
+23 -1
View File
@@ -154,6 +154,10 @@ impl App {
match server_notification_thread_target(&notification) {
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;
};
codex-rs/tui/src/app/app_server_requests.rs
+4
View File
@@ -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,
codex-rs/tui/src/app/conversation_retirement.rs
+33
View File
@@ -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;
codex-rs/tui/src/app/conversation_retirement_tests.rs
+14
View File
@@ -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);
}
codex-rs/tui/src/app/event_dispatch.rs
+10 -3
View File
@@ -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
codex-rs/tui/src/app/session_lifecycle.rs
+30 -38
View File
@@ -264,9 +264,15 @@ impl App {
app_server: &mut AppServerSession,
thread_id: ThreadId,
) -> Result<bool> {
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)
codex-rs/tui/src/app/side.rs
+36 -90
View File
@@ -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<ThreadId> {
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<crate::chatwidget::UserMessage>,
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 {
codex-rs/tui/src/app/side_tests.rs
+122
View File
@@ -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::<String>(),
);
}
}
assert!(
rendered_errors
.iter()
.any(|error| error.contains("Failed to install side conversation.")),
"startup error should remain visible in Parent: {rendered_errors:?}"
);
Ok(())
}
codex-rs/tui/src/app/test_support.rs
+61 -46
View File
@@ -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<AppEvent>,
tokio::sync::mpsc::UnboundedReceiver<AppCommand>,
) {
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 {
codex-rs/tui/src/app/tests.rs
+149 -7
View File
@@ -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,
codex-rs/tui/src/app/thread_routing.rs
+13
View File
@@ -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<Turn>,
) -> 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(