From 9646bf02950a614b9a5bba13135fa013b27a46a4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 9 Dec 2025 01:32:48 +0000 Subject: [PATCH 1/2] feat(acp): Add /agent command for ACP agent switching with pending selection Implements agent/model switching for ACP mode: - Add /agent slash command to open agent picker popup - Add agent_picker component in nori/ with SelectionViewParams - Track pending agent selection - switch happens on next prompt submission - Modify /model to show disabled options in ACP mode with redirect to /agent - Add list_available_agents() and AcpAgentInfo to ACP registry - Add AppEvent variants for SetPendingAgent, ClearPendingAgent, SubmitWithAgentSwitch - Add ACP file tracing initialization to TUI - Add E2E tests for agent switching behavior --- codex-rs/acp/src/lib.rs | 2 + codex-rs/acp/src/registry.rs | 43 +++ codex-rs/tui-pty-e2e/tests/agent_switching.rs | 333 ++++++++++++++++++ codex-rs/tui/src/app.rs | 87 +++++ codex-rs/tui/src/app_event.rs | 25 ++ codex-rs/tui/src/chatwidget.rs | 68 ++++ codex-rs/tui/src/chatwidget/tests.rs | 1 + codex-rs/tui/src/lib.rs | 8 + codex-rs/tui/src/nori/agent_picker.rs | 149 ++++++++ codex-rs/tui/src/nori/mod.rs | 1 + codex-rs/tui/src/slash_command.rs | 5 +- 11 files changed, 721 insertions(+), 1 deletion(-) create mode 100644 codex-rs/tui/src/nori/agent_picker.rs diff --git a/codex-rs/acp/src/lib.rs b/codex-rs/acp/src/lib.rs index 1818d7344..39413dbaf 100644 --- a/codex-rs/acp/src/lib.rs +++ b/codex-rs/acp/src/lib.rs @@ -14,8 +14,10 @@ pub use backend::AcpBackendConfig; pub use connection::AcpConnection; pub use connection::ApprovalRequest; pub use registry::AcpAgentConfig; +pub use registry::AcpAgentInfo; pub use registry::AcpProviderInfo; pub use registry::get_agent_config; +pub use registry::list_available_agents; pub use tracing_setup::init_file_tracing; pub use translator::TranslatedEvent; pub use translator::translate_session_update; diff --git a/codex-rs/acp/src/registry.rs b/codex-rs/acp/src/registry.rs index a5a58e459..e582380fe 100644 --- a/codex-rs/acp/src/registry.rs +++ b/codex-rs/acp/src/registry.rs @@ -6,6 +6,49 @@ use anyhow::Result; use std::time::Duration; +/// Information about an available ACP agent for display in the picker +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AcpAgentInfo { + /// Model name used to select this agent (e.g., "mock-model", "gemini-2.5-flash") + pub model_name: String, + /// Display name shown in the picker + pub display_name: String, + /// Description of the agent + pub description: String, + /// Provider slug for this agent + pub provider_slug: String, +} + +/// Get list of all available ACP agents for the agent picker +pub fn list_available_agents() -> Vec { + vec![ + AcpAgentInfo { + model_name: "mock-model".to_string(), + display_name: "Mock ACP".to_string(), + description: "Mock agent for testing".to_string(), + provider_slug: "mock-acp".to_string(), + }, + AcpAgentInfo { + model_name: "mock-model-alt".to_string(), + display_name: "Mock ACP Alt".to_string(), + description: "Alternate mock agent for testing".to_string(), + provider_slug: "mock-acp-alt".to_string(), + }, + AcpAgentInfo { + model_name: "gemini-2.5-flash".to_string(), + display_name: "Gemini 2.5 Flash".to_string(), + description: "Google Gemini via ACP".to_string(), + provider_slug: "gemini-acp".to_string(), + }, + AcpAgentInfo { + model_name: "claude-4.5".to_string(), + display_name: "Claude 4.5".to_string(), + description: "Anthropic Claude via ACP".to_string(), + provider_slug: "claude-acp".to_string(), + }, + ] +} + /// Default idle timeout for ACP streaming (5 minutes) const DEFAULT_STREAM_IDLE_TIMEOUT: Duration = Duration::from_secs(300); diff --git a/codex-rs/tui-pty-e2e/tests/agent_switching.rs b/codex-rs/tui-pty-e2e/tests/agent_switching.rs index 73d561f97..f04a82c71 100644 --- a/codex-rs/tui-pty-e2e/tests/agent_switching.rs +++ b/codex-rs/tui-pty-e2e/tests/agent_switching.rs @@ -374,3 +374,336 @@ fn test_acp_agent_switch_via_model_picker() { // If no new PID, the model picker might not trigger subprocess restart // This is acceptable behavior - document it } + +// ============================================================================ +// Test: /agent Slash Command - Shows Available Agents +// ============================================================================ + +/// Test that /agent command shows available ACP agents from the registry +#[test] +#[cfg(target_os = "linux")] +fn test_agent_command_shows_available_agents() { + let config = SessionConfig::new().with_model("mock-model".to_string()); + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + // Open agent picker with /agent command + session.send_str("/agent").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + + // Wait for agent picker to appear - it should show available agents + session + .wait_for( + |screen| { + // Should show available agents from the ACP registry + screen.contains("Select Agent") || screen.contains("mock-model") + }, + Duration::from_secs(3), + ) + .expect("Agent picker should appear"); + + // Verify both mock agents are visible + let screen = session.screen_contents(); + assert!( + screen.contains("mock-model") || screen.contains("Mock"), + "Agent picker should show mock-model agent, got: {}", + screen + ); +} + +// ============================================================================ +// Test: /agent Slash Command - Pending Selection +// ============================================================================ + +/// Test that selecting an agent in /agent tracks it as pending and doesn't +/// switch immediately +#[test] +#[cfg(target_os = "linux")] +fn test_agent_command_pending_selection() { + let config = SessionConfig::new().with_model("mock-model".to_string()); + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + let log_path = session.acp_log_path().expect("Should have log path"); + let initial_pids = extract_mock_agent_pids_from_log(&log_path); + assert!(!initial_pids.is_empty(), "Should have initial PID"); + let initial_pid = initial_pids[0]; + + // Open agent picker with /agent command + session.send_str("/agent").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + + // Wait for agent picker to appear + session + .wait_for( + |screen| screen.contains("Select Agent") || screen.contains("mock-model"), + Duration::from_secs(3), + ) + .expect("Agent picker should appear"); + + // Select a different agent (mock-model-alt) + session.send_key(Key::Down).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(Duration::from_millis(500)); + + // After selecting, the OLD agent should still be running (pending selection) + let pids_after_selection = extract_mock_agent_pids_from_log(&log_path); + assert_eq!( + pids_after_selection.len(), + initial_pids.len(), + "No new subprocess should be spawned yet - selection is pending until next prompt" + ); + + // The original process should still be alive + assert!( + process_exists_and_not_zombie(initial_pid), + "Original agent should still be running after pending selection" + ); +} + +// ============================================================================ +// Test: /agent Slash Command - Switch on Prompt Submission +// ============================================================================ + +/// Test that agent switch happens on next prompt submission +#[test] +#[cfg(target_os = "linux")] +fn test_agent_switch_on_prompt_submission() { + let config = SessionConfig::new().with_model("mock-model".to_string()); + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + let log_path = session.acp_log_path().expect("Should have log path"); + let initial_pids = extract_mock_agent_pids_from_log(&log_path); + assert!(!initial_pids.is_empty(), "Should have initial PID"); + let initial_pid = initial_pids[0]; + + // Open agent picker with /agent command + session.send_str("/agent").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + + // Wait for agent picker to appear + session + .wait_for( + |screen| screen.contains("Select Agent") || screen.contains("mock-model"), + Duration::from_secs(3), + ) + .expect("Agent picker should appear"); + + // Select a different agent (mock-model-alt) + session.send_key(Key::Down).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(Duration::from_millis(500)); + + // Now submit a prompt - this should trigger the agent switch + session.send_str("hello").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for the response to start + session + .wait_for_text("Working", Duration::from_secs(5)) + .ok(); // May or may not see this depending on response speed + std::thread::sleep(Duration::from_millis(1000)); + + // Check that a new agent was spawned + let post_prompt_pids = extract_mock_agent_pids_from_log(&log_path); + assert!( + post_prompt_pids.len() > initial_pids.len(), + "New subprocess should be spawned after prompt submission with pending agent: initial={:?}, after={:?}", + initial_pids, + post_prompt_pids + ); + + let new_pid = *post_prompt_pids.last().unwrap(); + assert_ne!( + initial_pid, new_pid, + "New agent should have different PID after prompt submission" + ); +} + +// ============================================================================ +// Test: /agent - No Switch During Active Prompt Turn +// ============================================================================ + +/// Test that navigating /agent picker during streaming doesn't kill the agent +#[test] +#[cfg(target_os = "linux")] +fn test_agent_picker_no_switch_during_streaming() { + let config = SessionConfig::new() + .with_model("mock-model".to_string()) + .with_stream_until_cancel(); // Agent streams until cancelled + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + let log_path = session.acp_log_path().expect("Should have log path"); + let initial_pids = extract_mock_agent_pids_from_log(&log_path); + assert!(!initial_pids.is_empty(), "Should have initial PID"); + let initial_pid = initial_pids[0]; + + // Start a streaming prompt + session.send_str("Start streaming").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for streaming to start + session + .wait_for_text("Working", Duration::from_secs(5)) + .expect("Streaming should start"); + + // While streaming, the agent should still be running + assert!( + process_exists_and_not_zombie(initial_pid), + "Agent should be running during streaming" + ); + + // Cancel streaming first so we can access the UI + session.send_key(Key::Escape).unwrap(); + std::thread::sleep(Duration::from_millis(500)); + + // The agent should still be the same + let pids_after = extract_mock_agent_pids_from_log(&log_path); + assert_eq!( + pids_after.len(), + initial_pids.len(), + "No new subprocess should be spawned during/after streaming cancel" + ); + assert!( + process_exists_and_not_zombie(initial_pid), + "Original agent should still be running after cancel" + ); +} + +// ============================================================================ +// Test: /model Slash Command - Shows Disabled in ACP Mode +// ============================================================================ + +/// Test that /model command shows disabled options in ACP mode +#[test] +#[cfg(target_os = "linux")] +fn test_model_command_shows_disabled_in_acp_mode() { + let config = SessionConfig::new().with_model("mock-model".to_string()); + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + // Open model picker with /model command + session.send_str("/model").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + + // Wait for model picker to appear + session + .wait_for( + |screen| screen.contains("Select Model") || screen.contains("Model"), + Duration::from_secs(3), + ) + .expect("Model picker should appear"); + + // In ACP mode, model options should show as disabled or indicate + // they're not available + let screen = session.screen_contents(); + assert!( + screen.contains("disabled") + || screen.contains("Not available") + || screen.contains("ACP") + || screen.contains("Use /agent"), + "Model picker should indicate options are disabled in ACP mode, got: {}", + screen + ); +} + +// ============================================================================ +// Test: /agent Slash Command - Cleanup After Switch +// ============================================================================ + +/// Test that old agent subprocess is cleaned up after switch on prompt +#[test] +#[cfg(target_os = "linux")] +fn test_agent_cleanup_after_switch_on_prompt() { + let config = SessionConfig::new().with_model("mock-model".to_string()); + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + let log_path = session.acp_log_path().expect("Should have log path"); + let initial_pids = extract_mock_agent_pids_from_log(&log_path); + assert!(!initial_pids.is_empty(), "Should have initial PID"); + let initial_pid = initial_pids[0]; + + // Verify initial process exists + assert!( + process_exists_and_not_zombie(initial_pid), + "Initial agent should exist" + ); + + // Open agent picker and select a different agent + session.send_str("/agent").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + + session + .wait_for( + |screen| screen.contains("Select Agent") || screen.contains("mock-model"), + Duration::from_secs(3), + ) + .expect("Agent picker should appear"); + + session.send_key(Key::Down).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(Duration::from_millis(500)); + + // Submit prompt to trigger switch + session.send_str("trigger switch").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for response + std::thread::sleep(Duration::from_millis(2000)); + + // Old process should be cleaned up + assert!( + !process_exists(initial_pid) || !process_exists_and_not_zombie(initial_pid), + "Old agent subprocess {} should be cleaned up after switch", + initial_pid + ); +} diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 5c636bd28..036576ac9 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -228,6 +228,19 @@ pub(crate) struct App { // One-shot suppression of the next world-writable scan after user confirmation. skip_world_writable_scan_once: bool, + + /// Pending agent selection. When set, the agent will switch on the next + /// prompt submission. This avoids disrupting active prompt turns. + pending_agent: Option, +} + +/// Information about a pending agent switch. +#[derive(Debug, Clone)] +pub(crate) struct PendingAgentSelection { + /// The model name of the selected agent (e.g., "mock-model", "gemini-2.5-flash") + pub model_name: String, + /// The display name for the status indicator + pub display_name: String, } impl App { @@ -351,6 +364,7 @@ impl App { pending_update_action: None, suppress_shutdown_complete: false, skip_world_writable_scan_once: false, + pending_agent: None, }; // On startup, if Agent mode (workspace-write) or ReadOnly is active, warn about world-writable dirs on Windows. @@ -883,6 +897,77 @@ impl App { )); } }, + AppEvent::SetPendingAgent { + model_name, + display_name, + } => { + // Store the pending agent selection in both App and ChatWidget + self.pending_agent = Some(PendingAgentSelection { + model_name: model_name.clone(), + display_name: display_name.clone(), + }); + // Also set on ChatWidget so it can trigger the switch on prompt submission + self.chat_widget + .set_pending_agent(model_name.clone(), display_name.clone()); + tracing::info!( + "Pending agent set: {} ({}). Will switch on next prompt.", + display_name, + model_name + ); + self.chat_widget.add_info_message( + format!( + "Agent '{}' selected. Will switch on next prompt submission.", + display_name + ), + None, + ); + } + AppEvent::ClearPendingAgent => { + if self.pending_agent.is_some() { + tracing::info!("Pending agent selection cleared"); + self.pending_agent = None; + self.chat_widget.clear_pending_agent(); + } + } + AppEvent::SubmitWithAgentSwitch { + model_name, + display_name, + message_text, + image_paths, + } => { + tracing::info!( + "Switching agent to {} ({}) and submitting message", + display_name, + model_name + ); + + // Clear the pending agent since we're applying it now + self.pending_agent = None; + + // Update the model in config + self.config.model = model_name.clone(); + + // Shutdown current conversation + self.shutdown_current_conversation().await; + + // Create the new chat widget with the new config and the message as initial prompt + let init = crate::chatwidget::ChatWidgetInit { + config: self.config.clone(), + frame_requester: tui.frame_requester(), + app_event_tx: self.app_event_tx.clone(), + initial_prompt: Some(message_text), + initial_images: image_paths, + enhanced_keys_supported: self.enhanced_keys_supported, + auth_manager: self.auth_manager.clone(), + feedback: self.feedback.clone(), + }; + self.chat_widget = ChatWidget::new(init, self.server.clone()); + + self.chat_widget.add_info_message( + format!("Switched to agent: {}", display_name), + None, + ); + } } Ok(true) } @@ -1072,6 +1157,7 @@ mod tests { pending_update_action: None, suppress_shutdown_complete: false, skip_world_writable_scan_once: false, + pending_agent: None, } } @@ -1109,6 +1195,7 @@ mod tests { pending_update_action: None, suppress_shutdown_complete: false, skip_world_writable_scan_once: false, + pending_agent: None, }, rx, op_rx, diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index cf494f57d..7c6ea93f2 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -166,6 +166,31 @@ pub(crate) enum AppEvent { OpenFeedbackConsent { category: FeedbackCategory, }, + + /// Set a pending agent selection. The agent switch will happen on the next + /// prompt submission to avoid disrupting active prompt turns. + SetPendingAgent { + /// The model name of the selected agent (e.g., "mock-model", "gemini-2.5-flash") + model_name: String, + /// The display name for the status indicator + display_name: String, + }, + + /// Clear the pending agent selection (e.g., user cancelled). + ClearPendingAgent, + + /// Submit a message with a pending agent switch. The agent will be switched + /// first, then the message will be submitted to the new agent. + SubmitWithAgentSwitch { + /// The model name of the agent to switch to + model_name: String, + /// The display name for the status indicator + display_name: String, + /// The user message text to submit after switching + message_text: String, + /// Optional image paths to include with the message + image_paths: Vec, + }, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e1d5f8626..77ea87a25 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -318,6 +318,15 @@ pub(crate) struct ChatWidget { feedback: codex_feedback::CodexFeedback, // Current session rollout path (if known) current_rollout_path: Option, + // Pending agent selection for next prompt submission + pending_agent: Option, +} + +/// Information about a pending agent switch in ChatWidget. +#[derive(Debug, Clone)] +pub(crate) struct PendingAgentInfo { + pub model_name: String, + pub display_name: String, } struct UserMessage { @@ -1260,6 +1269,7 @@ impl ChatWidget { last_rendered_width: std::cell::Cell::new(None), feedback, current_rollout_path: None, + pending_agent: None, }; widget.prefetch_rate_limits(); @@ -1337,6 +1347,7 @@ impl ChatWidget { last_rendered_width: std::cell::Cell::new(None), feedback, current_rollout_path: None, + pending_agent: None, }; widget.prefetch_rate_limits(); @@ -1344,6 +1355,24 @@ impl ChatWidget { widget } + /// Set a pending agent to switch to on the next prompt submission. + pub(crate) fn set_pending_agent(&mut self, model_name: String, display_name: String) { + self.pending_agent = Some(PendingAgentInfo { + model_name, + display_name, + }); + } + + /// Clear any pending agent selection. + pub(crate) fn clear_pending_agent(&mut self) { + self.pending_agent = None; + } + + /// Check if there's a pending agent selection. + pub(crate) fn has_pending_agent(&self) -> bool { + self.pending_agent.is_some() + } + pub(crate) fn handle_key_event(&mut self, key_event: KeyEvent) { match key_event { KeyEvent { @@ -1473,6 +1502,9 @@ impl ChatWidget { SlashCommand::Review => { self.open_review_popup(); } + SlashCommand::Agent => { + self.open_agent_popup(); + } SlashCommand::Model => { self.open_model_popup(); } @@ -1628,6 +1660,18 @@ impl ChatWidget { return; } + // Check if there's a pending agent switch - if so, send the message through + // the App to trigger the switch first + if let Some(pending) = self.pending_agent.take() { + self.app_event_tx.send(AppEvent::SubmitWithAgentSwitch { + model_name: pending.model_name, + display_name: pending.display_name, + message_text: text, + image_paths, + }); + return; + } + let mut items: Vec = Vec::new(); // Special-case: "!cmd" executes a local shell command instead of sending to the model. @@ -2084,10 +2128,34 @@ impl ChatWidget { }); } + /// Open the agent picker popup for ACP mode. + pub(crate) fn open_agent_popup(&mut self) { + let current_model = self.config.model.clone(); + let params = crate::nori::agent_picker::agent_picker_params( + ¤t_model, + self.app_event_tx.clone(), + ); + self.bottom_pane.show_selection_view(params); + } + /// Open a popup to choose the model (stage 1). After selecting a model, /// a second popup is shown to choose the reasoning effort. + /// + /// In ACP mode (when current model is an ACP agent), this shows a disabled + /// message directing users to use /agent instead. pub(crate) fn open_model_popup(&mut self) { let current_model = self.config.model.clone(); + + // Check if we're in ACP mode by checking if the current model is registered + // in the ACP agent registry + if codex_acp::get_agent_config(¤t_model).is_ok() { + // ACP mode - show disabled model picker + let params = crate::nori::agent_picker::acp_model_picker_params(); + self.bottom_pane.show_selection_view(params); + return; + } + + // Standard HTTP mode - show normal model picker let auth_mode = self.auth_manager.auth().map(|auth| auth.mode); let presets: Vec = builtin_model_presets(auth_mode); diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 362c589d6..f0c0f960e 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -377,6 +377,7 @@ fn make_chatwidget_manual() -> ( last_rendered_width: std::cell::Cell::new(None), feedback: codex_feedback::CodexFeedback::new(), current_rollout_path: None, + pending_agent: None, }; (widget, rx, op_rx) } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 784f6f3a4..e3d339dec 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -103,6 +103,14 @@ pub async fn run_main( mut cli: Cli, codex_linux_sandbox_exe: Option, ) -> std::io::Result { + // Initialize ACP file tracing for subprocess debugging + let acp_log_path = std::env::current_dir() + .unwrap_or_else(|_| PathBuf::from(".")) + .join(".codex-acp.log"); + if let Err(e) = codex_acp::init_file_tracing(&acp_log_path) { + tracing::warn!("Failed to initialize ACP file tracing: {e}"); + } + let (sandbox_mode, approval_policy) = if cli.full_auto { ( Some(SandboxMode::WorkspaceWrite), diff --git a/codex-rs/tui/src/nori/agent_picker.rs b/codex-rs/tui/src/nori/agent_picker.rs new file mode 100644 index 000000000..bf229c750 --- /dev/null +++ b/codex-rs/tui/src/nori/agent_picker.rs @@ -0,0 +1,149 @@ +//! Agent picker component for ACP mode. +//! +//! This module provides the UI for selecting between available ACP agents. +//! Agent selection is tracked as "pending" and the actual switch happens +//! on the next prompt submission to avoid disrupting active prompt turns. + +use codex_acp::list_available_agents; +use codex_acp::AcpAgentInfo; +use ratatui::text::Line; + +use crate::app_event::AppEvent; +use crate::app_event_sender::AppEventSender; +use crate::bottom_pane::SelectionAction; +use crate::bottom_pane::SelectionItem; +use crate::bottom_pane::SelectionViewParams; +use crate::bottom_pane::popup_consts::standard_popup_hint_line; + +/// Event for pending agent selection +#[derive(Debug, Clone)] +pub struct PendingAgentSelection { + /// The model name of the selected agent (e.g., "mock-model", "gemini-2.5-flash") + pub model_name: String, + /// The display name for the status indicator + pub display_name: String, +} + +/// Create selection view parameters for the agent picker. +/// +/// # Arguments +/// * `current_model` - The currently active model name +/// * `app_event_tx` - The app event sender for triggering selection events +pub fn agent_picker_params( + current_model: &str, + _app_event_tx: AppEventSender, +) -> SelectionViewParams { + let available_agents = list_available_agents(); + let current_normalized = current_model.to_lowercase(); + + let items: Vec = available_agents + .into_iter() + .map(|agent| { + let is_current = agent.model_name.to_lowercase() == current_normalized; + let model_name = agent.model_name.clone(); + let display_name = agent.display_name.clone(); + + // Create action that sends the pending agent selection event + let actions: Vec = vec![Box::new(move |tx| { + tx.send(AppEvent::SetPendingAgent { + model_name: model_name.clone(), + display_name: display_name.clone(), + }); + })]; + + SelectionItem { + name: agent.display_name, + description: Some(agent.description), + is_current, + actions, + dismiss_on_select: true, + ..Default::default() + } + }) + .collect(); + + SelectionViewParams { + title: Some("Select Agent".to_string()), + subtitle: Some("Agent will switch on next prompt submission".to_string()), + footer_hint: Some(standard_popup_hint_line()), + items, + ..Default::default() + } +} + +/// Create selection view parameters for the model picker in ACP mode. +/// Shows models as disabled with a message to use /agent instead. +pub fn acp_model_picker_params() -> SelectionViewParams { + // In ACP mode, we show a message that models are not directly selectable + // and users should use /agent instead + let items: Vec = vec![SelectionItem { + name: "Model switching disabled in ACP mode".to_string(), + description: Some("Use /agent to switch between ACP agents".to_string()), + is_current: false, + actions: vec![], + dismiss_on_select: true, + ..Default::default() + }]; + + SelectionViewParams { + title: Some("Select Model".to_string()), + subtitle: Some("Not available in ACP mode - use /agent instead".to_string()), + footer_hint: Some(Line::from("Press esc to dismiss, or use /agent to switch agents.")), + items, + ..Default::default() + } +} + +/// Get information about an agent by model name +pub fn get_agent_info(model_name: &str) -> Option { + let normalized = model_name.to_lowercase(); + list_available_agents() + .into_iter() + .find(|agent| agent.model_name.to_lowercase() == normalized) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::app_event::AppEvent; + use tokio::sync::mpsc::unbounded_channel; + + #[test] + fn test_agent_picker_params_lists_available_agents() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + + let params = agent_picker_params("mock-model", tx); + + assert!(params.title.is_some()); + assert!(params.title.unwrap().contains("Select Agent")); + assert!(!params.items.is_empty()); + + // Should have mock-model as current + let mock_agent = params.items.iter().find(|i| i.name == "Mock ACP"); + assert!(mock_agent.is_some()); + assert!(mock_agent.unwrap().is_current); + } + + #[test] + fn test_acp_model_picker_shows_disabled() { + let params = acp_model_picker_params(); + + assert!(params.title.is_some()); + assert!(params.subtitle.is_some()); + assert!(params.subtitle.unwrap().contains("Not available")); + } + + #[test] + fn test_get_agent_info() { + let info = get_agent_info("mock-model"); + assert!(info.is_some()); + assert_eq!(info.unwrap().display_name, "Mock ACP"); + + let info = get_agent_info("Mock-Model"); // Case insensitive + assert!(info.is_some()); + + let info = get_agent_info("unknown-agent"); + assert!(info.is_none()); + } +} diff --git a/codex-rs/tui/src/nori/mod.rs b/codex-rs/tui/src/nori/mod.rs index 7655fc613..a68bee7c3 100644 --- a/codex-rs/tui/src/nori/mod.rs +++ b/codex-rs/tui/src/nori/mod.rs @@ -3,4 +3,5 @@ //! This module contains Nori-branded components that replace or extend //! the default Codex TUI behavior. +pub(crate) mod agent_picker; pub(crate) mod session_header; diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index 969d279b0..2b45a6cd2 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -12,6 +12,7 @@ use strum_macros::IntoStaticStr; pub enum SlashCommand { // DO NOT ALPHA-SORT! Enum order is presentation order in the popup, so // more frequently used commands should be listed first. + Agent, Model, Approvals, Review, @@ -35,6 +36,7 @@ impl SlashCommand { /// User-visible description shown in the popup. pub fn description(self) -> &'static str { match self { + SlashCommand::Agent => "switch between available ACP agents", SlashCommand::Feedback => "send logs to maintainers", SlashCommand::New => "start a new chat during a conversation", SlashCommand::Init => "create an AGENTS.md file with instructions for Codex", @@ -63,7 +65,8 @@ impl SlashCommand { /// Whether this command can be run while a task is in progress. pub fn available_during_task(self) -> bool { match self { - SlashCommand::New + SlashCommand::Agent + | SlashCommand::New | SlashCommand::Init | SlashCommand::Compact | SlashCommand::Undo From 5dc1e975ee3f39278e81df315b98d3e644bb01cd Mon Sep 17 00:00:00 2001 From: Clifford Ressel Date: Tue, 9 Dec 2025 08:47:00 -0500 Subject: [PATCH 2/2] fix(acp): Prevent race condition during agent switching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When switching agents, events from the OLD agent could leak into the NEW widget before SessionConfigured was received, causing the UI to show responses from the wrong agent. Fix: - Add expected_model field to ChatWidgetInit to specify expected model - Add session_configured_received flag to ChatWidget - Filter events in handle_codex_event() until SessionConfigured arrives with matching model name Also: - Add E2E tests for agent switch message flow - Remove unused ClearPendingAgent event and has_pending_agent method - Fix clippy warnings for dead code 🤖 Generated with [Nori](https://nori.ai) Co-Authored-By: Nori --- codex-rs/tui-pty-e2e/docs.md | 11 +- codex-rs/tui-pty-e2e/tests/agent_switching.rs | 241 ++++++++++++++++++ codex-rs/tui/docs.md | 13 + codex-rs/tui/src/app.rs | 31 +-- codex-rs/tui/src/app_backtrack.rs | 1 + codex-rs/tui/src/app_event.rs | 3 - codex-rs/tui/src/chatwidget.rs | 71 +++++- codex-rs/tui/src/chatwidget/tests.rs | 1 + codex-rs/tui/src/nori/agent_picker.rs | 12 +- 9 files changed, 344 insertions(+), 40 deletions(-) diff --git a/codex-rs/tui-pty-e2e/docs.md b/codex-rs/tui-pty-e2e/docs.md index 829377e43..2e5de3771 100644 --- a/codex-rs/tui-pty-e2e/docs.md +++ b/codex-rs/tui-pty-e2e/docs.md @@ -145,7 +145,7 @@ This delay allows the PTY subprocess time to process input and update the displa | `@/codex-rs/tui-pty-e2e/tests/input_handling.rs` | Text editing, backspace, Ctrl-C clearing, arrow key navigation with snapshot testing | | `@/codex-rs/tui-pty-e2e/tests/streaming.rs` | Prompt submission with timing delays, agent response streaming | | `@/codex-rs/tui-pty-e2e/tests/acp_mode.rs` | ACP mode startup, response flow, and approval bridging - validates TUI works with ACP wire API and mock agent; includes test for permission request display | -| `@/codex-rs/tui-pty-e2e/tests/agent_switching.rs` | ACP agent subprocess lifecycle - verifies subprocess spawning, cleanup on session switch, and different agents use different processes (Linux only) | +| `@/codex-rs/tui-pty-e2e/tests/agent_switching.rs` | ACP agent subprocess lifecycle and event isolation - verifies subprocess spawning, cleanup on session switch, different agents use different processes, and event filtering prevents cross-agent contamination (Linux only) | | `@/codex-rs/tui-pty-e2e/tests/live_acp.rs` | Live authenticated ACP tests for Gemini and Claude with real API connections (opt-in, marked `#[ignore]`) | **Snapshot Files:** @@ -237,7 +237,9 @@ See `@/codex-rs/mock-acp-agent/docs.md` for full list of env vars. **Agent Subprocess Lifecycle Testing (`agent_switching.rs`):** -Linux-only tests that verify ACP subprocess lifecycle management by parsing the `.codex-acp.log` file for PID entries: +Linux-only tests that verify ACP subprocess lifecycle management and event isolation: + +*Subprocess Management Tests:* - `acp_log_path()` method on `TuiSession` returns the path to the ACP tracing log file - Tests extract PIDs from log lines matching `"ACP agent spawned (pid: Some(...))"` - Uses `/proc/{pid}` filesystem to verify process existence and zombie state @@ -248,6 +250,11 @@ Linux-only tests that verify ACP subprocess lifecycle management by parsing the - Cleanup happens when session switches, not when individual prompt turns end - Different models (`mock-model` vs `mock-model-alt`) spawn different subprocesses +*Event Isolation Tests:* +- `extract_agent_messages_from_log()` helper parses `Mock agent:` log entries from ACP log file +- `test_agent_switch_message_flow_mock_to_mock_alt` verifies that after switching agents, the NEW agent receives and responds to prompts (catches race conditions where OLD agent events could leak) +- `test_agent_switch_logs_correct_sequence` verifies the expected log sequence during agent switch: agent receives prompt, logs receipt, sends response + **Binary Discovery:** `codex_binary_path()` locates the compiled binary: diff --git a/codex-rs/tui-pty-e2e/tests/agent_switching.rs b/codex-rs/tui-pty-e2e/tests/agent_switching.rs index f04a82c71..b67d60ea7 100644 --- a/codex-rs/tui-pty-e2e/tests/agent_switching.rs +++ b/codex-rs/tui-pty-e2e/tests/agent_switching.rs @@ -707,3 +707,244 @@ fn test_agent_cleanup_after_switch_on_prompt() { initial_pid ); } + +// ============================================================================ +// Test: Agent Switch Message Flow - Verifies NEW agent receives and responds +// ============================================================================ + +/// Helper to extract agent messages from log file +/// Each mock agent logs to stderr which is captured in the ACP log +fn extract_agent_messages_from_log(log_path: &std::path::Path) -> Vec { + std::fs::read_to_string(log_path) + .unwrap_or_default() + .lines() + .filter(|line| { + line.contains("Mock agent:") + || line.contains("cancel") + || line.contains("shutdown") + || line.contains("prompt") + }) + .map(|s| s.to_string()) + .collect() +} + +/// Test that when switching agents via /agent command, the NEW agent +/// correctly receives and responds to the submitted prompt. +/// +/// This test explicitly verifies the message flow: +/// 1. OLD agent should receive a cancel/shutdown signal +/// 2. NEW agent should receive a new_session request +/// 3. NEW agent should receive the prompt and respond +/// 4. Response from NEW agent appears on screen +/// +/// This catches the race condition bug where events from the OLD agent +/// could leak into the NEW widget, causing the prompt to be lost. +#[test] +#[cfg(target_os = "linux")] +fn test_agent_switch_message_flow_mock_to_mock_alt() { + // Use default response (Test message 1/2) - both agents will use this + let config = SessionConfig::new().with_model("mock-model".to_string()); + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + let log_path = session.acp_log_path().expect("Should have log path"); + + // First, verify initial agent works - send a prompt + session.send_str("test initial").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for initial agent response (default response) + session + .wait_for_text("Test message", Duration::from_secs(5)) + .expect("Initial agent should respond"); + + // Log messages before switch + let msgs_before_switch = extract_agent_messages_from_log(&log_path); + eprintln!("Messages before switch: {:?}", msgs_before_switch); + + // Open agent picker with /agent command + session.send_str("/agent").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + + // Wait for agent picker to appear + session + .wait_for( + |screen| screen.contains("Select Agent") || screen.contains("mock-model"), + Duration::from_secs(3), + ) + .expect("Agent picker should appear"); + + // Select mock-model-alt (different agent) + session.send_key(Key::Down).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(Duration::from_millis(500)); + + // Messages after selection (but before prompt submission) + let msgs_after_selection = extract_agent_messages_from_log(&log_path); + eprintln!("Messages after selection: {:?}", msgs_after_selection); + + // Now submit a prompt - this should trigger the actual agent switch + // The NEW agent (mock-model-alt) should receive this prompt and respond + session.send_str("test after switch").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for the NEW agent's response to appear. + // The key verification: we should see TWO instances of "Test message" - + // one from the first prompt, and one from the second prompt after switch. + // If the switch fails, the second response won't appear. + std::thread::sleep(Duration::from_secs(3)); // Give time for response + + // Log messages after prompt submission + let msgs_after_prompt = extract_agent_messages_from_log(&log_path); + eprintln!("Messages after prompt submission: {:?}", msgs_after_prompt); + + // Verify we got two prompt calls (one before switch, one after) + let prompt_count = msgs_after_prompt + .iter() + .filter(|m| m.contains("Mock agent: prompt")) + .count(); + + if prompt_count < 2 { + let screen = session.screen_contents(); + panic!( + "Expected 2 prompt calls (before and after switch), got {}.\n\ + Screen contents: {}\n\ + Agent messages in log: {:?}", + prompt_count, screen, msgs_after_prompt + ); + } + + // Verify message flow in logs: + // 1. Should see "Mock agent: new_session" for the NEW agent + // 2. Should see "Mock agent: prompt" for the NEW agent + let has_new_session = msgs_after_prompt + .iter() + .filter(|m| m.contains("new_session")) + .count() + >= 2; // Initial + after switch + + assert!( + has_new_session, + "Should have new_session calls for both agents, messages: {:?}", + msgs_after_prompt + ); + assert!( + prompt_count >= 2, + "Should have prompt calls for both agents, messages: {:?}", + msgs_after_prompt + ); + + // Final verification: the screen should show response content + let screen = session.screen_contents(); + assert!( + screen.contains("Test message"), + "Screen should contain response text. Screen:\n{}", + screen + ); +} + +/// Test that verifies the expected sequence of operations when switching agents +/// This is a more focused test that checks specific message ordering +#[test] +#[cfg(target_os = "linux")] +fn test_agent_switch_logs_correct_sequence() { + let config = SessionConfig::new().with_model("mock-model".to_string()); + + let mut session = TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn TUI"); + + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + std::thread::sleep(TIMEOUT_INPUT); + + let log_path = session.acp_log_path().expect("Should have log path"); + let initial_pids = extract_mock_agent_pids_from_log(&log_path); + assert!(!initial_pids.is_empty(), "Should have initial PID"); + + // Select new agent via /agent + session.send_str("/agent").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + + session + .wait_for( + |screen| screen.contains("Select Agent"), + Duration::from_secs(3), + ) + .expect("Agent picker should appear"); + + session.send_key(Key::Down).unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + std::thread::sleep(Duration::from_millis(300)); + + // Submit prompt to trigger switch + session.send_str("trigger").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for response + session + .wait_for_text("Test message", Duration::from_secs(10)) + .expect("Should see response from new agent"); + + // Parse the log to verify sequence + let log_content = std::fs::read_to_string(&log_path).unwrap_or_default(); + + // Count agent spawns - should be 2 (initial + after switch) + let spawn_count = log_content + .lines() + .filter(|l| l.contains("ACP agent spawned")) + .count(); + + assert!( + spawn_count >= 2, + "Should spawn at least 2 agents (initial + after switch), got: {}. Log:\n{}", + spawn_count, + log_content + ); + + // Verify new_session and prompt sequence + let agent_messages: Vec<&str> = log_content + .lines() + .filter(|l| l.contains("Mock agent:")) + .collect(); + + eprintln!("Agent message sequence:"); + for (i, msg) in agent_messages.iter().enumerate() { + eprintln!(" {}: {}", i, msg); + } + + // Should have: initialize, new_session, prompt (first agent) + // Then: initialize, new_session, prompt (second agent) + let new_session_count = agent_messages + .iter() + .filter(|m| m.contains("new_session")) + .count(); + let prompt_count = agent_messages + .iter() + .filter(|m| m.contains("prompt")) + .count(); + + assert!( + new_session_count >= 2, + "Should have at least 2 new_session calls, got: {}", + new_session_count + ); + assert!( + prompt_count >= 1, + "Should have at least 1 prompt call, got: {}", + prompt_count + ); +} diff --git a/codex-rs/tui/docs.md b/codex-rs/tui/docs.md index b90495654..72bcbed2f 100644 --- a/codex-rs/tui/docs.md +++ b/codex-rs/tui/docs.md @@ -122,6 +122,19 @@ Most event types (exec begin/end, MCP calls, elicitation) are queued during acti - The `InterruptManager` still contains `ExecApproval` and `ApplyPatchApproval` variants for completeness, but these methods are marked `#[allow(dead_code)]` - `on_task_complete()` calls `flush_interrupt_queue()` for any remaining queued items +**Agent Switch Event Filtering:** + +When switching between ACP agents (e.g., via `/agent` command), `ChatWidget` uses an event filtering mechanism to prevent race conditions: + +- `expected_model: Option` in `ChatWidgetInit` specifies which model the widget expects +- `session_configured_received: bool` tracks whether `SessionConfigured` has arrived from the expected model +- When `expected_model` is set, `handle_codex_event()` filters events: + - All events are ignored until `SessionConfigured` arrives + - `SessionConfigured` is only accepted if `event.model` matches `expected_model` (case-insensitive) + - Once matching `SessionConfigured` arrives, `session_configured_received` is set to `true` and normal event processing resumes +- This prevents the OLD agent's final events (completion, shutdown) from being processed by the NEW agent's widget +- Fresh sessions, resumed sessions, and `/new` command use `expected_model: None` (no filtering) + **Color System:** The `color.rs` and `terminal_palette.rs` modules handle terminal color detection and theming. The app queries terminal colors at startup for theme adaptation. diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 036576ac9..2eaba17eb 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -10,6 +10,7 @@ use crate::history_cell::HistoryCell; use crate::model_migration::ModelMigrationOutcome; use crate::model_migration::migration_copy_for_config; use crate::model_migration::run_model_migration_prompt; +use crate::nori::agent_picker::PendingAgentSelection; use crate::pager_overlay::Overlay; use crate::render::highlight::highlight_bash_to_lines; use crate::render::renderable::Renderable; @@ -234,15 +235,6 @@ pub(crate) struct App { pending_agent: Option, } -/// Information about a pending agent switch. -#[derive(Debug, Clone)] -pub(crate) struct PendingAgentSelection { - /// The model name of the selected agent (e.g., "mock-model", "gemini-2.5-flash") - pub model_name: String, - /// The display name for the status indicator - pub display_name: String, -} - impl App { async fn shutdown_current_conversation(&mut self) { if let Some(conversation_id) = self.chat_widget.conversation_id() { @@ -307,6 +299,7 @@ impl App { enhanced_keys_supported, auth_manager: auth_manager.clone(), feedback: feedback.clone(), + expected_model: None, // No filtering for fresh sessions }; ChatWidget::new(init, conversation_manager.clone()) } @@ -330,6 +323,7 @@ impl App { enhanced_keys_supported, auth_manager: auth_manager.clone(), feedback: feedback.clone(), + expected_model: None, // No filtering for resumed sessions }; ChatWidget::new_from_existing( init, @@ -484,6 +478,7 @@ impl App { enhanced_keys_supported: self.enhanced_keys_supported, auth_manager: self.auth_manager.clone(), feedback: self.feedback.clone(), + expected_model: None, // No filtering for /new command }; self.chat_widget = ChatWidget::new(init, self.server.clone()); if let Some(summary) = summary { @@ -916,19 +911,11 @@ impl App { ); self.chat_widget.add_info_message( format!( - "Agent '{}' selected. Will switch on next prompt submission.", - display_name + "Agent '{display_name}' selected. Will switch on next prompt submission." ), None, ); } - AppEvent::ClearPendingAgent => { - if self.pending_agent.is_some() { - tracing::info!("Pending agent selection cleared"); - self.pending_agent = None; - self.chat_widget.clear_pending_agent(); - } - } AppEvent::SubmitWithAgentSwitch { model_name, display_name, @@ -951,6 +938,7 @@ impl App { self.shutdown_current_conversation().await; // Create the new chat widget with the new config and the message as initial prompt + // Set expected_model to filter events from the OLD agent until SessionConfigured let init = crate::chatwidget::ChatWidgetInit { config: self.config.clone(), frame_requester: tui.frame_requester(), @@ -960,13 +948,12 @@ impl App { enhanced_keys_supported: self.enhanced_keys_supported, auth_manager: self.auth_manager.clone(), feedback: self.feedback.clone(), + expected_model: Some(model_name.clone()), }; self.chat_widget = ChatWidget::new(init, self.server.clone()); - self.chat_widget.add_info_message( - format!("Switched to agent: {}", display_name), - None, - ); + self.chat_widget + .add_info_message(format!("Switched to agent: {display_name}"), None); } } Ok(true) diff --git a/codex-rs/tui/src/app_backtrack.rs b/codex-rs/tui/src/app_backtrack.rs index b161867e4..910b0bb8e 100644 --- a/codex-rs/tui/src/app_backtrack.rs +++ b/codex-rs/tui/src/app_backtrack.rs @@ -347,6 +347,7 @@ impl App { enhanced_keys_supported: self.enhanced_keys_supported, auth_manager: self.auth_manager.clone(), feedback: self.feedback.clone(), + expected_model: None, // No filtering for backtracked conversations }; self.chat_widget = crate::chatwidget::ChatWidget::new_from_existing(init, conv, session_configured); diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 7c6ea93f2..caec1a780 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -176,9 +176,6 @@ pub(crate) enum AppEvent { display_name: String, }, - /// Clear the pending agent selection (e.g., user cancelled). - ClearPendingAgent, - /// Submit a message with a pending agent switch. The agent will be switched /// first, then the message will be submitted to the new agent. SubmitWithAgentSwitch { diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 77ea87a25..2daafcc9e 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -254,6 +254,10 @@ pub(crate) struct ChatWidgetInit { pub(crate) enhanced_keys_supported: bool, pub(crate) auth_manager: Arc, pub(crate) feedback: codex_feedback::CodexFeedback, + /// Expected model name for this widget. When set, events from other models + /// (e.g., from a previous agent) are ignored until SessionConfigured arrives + /// with a matching model. This prevents race conditions when switching agents. + pub(crate) expected_model: Option, } #[derive(Default)] @@ -320,6 +324,12 @@ pub(crate) struct ChatWidget { current_rollout_path: Option, // Pending agent selection for next prompt submission pending_agent: Option, + // Expected model name for agent switch synchronization. + // When set, events are ignored until SessionConfigured arrives with this model. + expected_model: Option, + // Whether SessionConfigured has been received for this widget. + // Used with expected_model to filter events from previous agents. + session_configured_received: bool, } /// Information about a pending agent switch in ChatWidget. @@ -376,6 +386,10 @@ impl ChatWidget { // --- Small event handlers --- fn on_session_configured(&mut self, event: codex_core::protocol::SessionConfiguredEvent) { + // Mark that we've received SessionConfigured - this unlocks event processing + // when expected_model is set (during agent switching) + self.session_configured_received = true; + self.bottom_pane .set_history_metadata(event.history_log_id, event.history_entry_count); self.conversation_id = Some(event.session_id); @@ -1216,6 +1230,7 @@ impl ChatWidget { enhanced_keys_supported, auth_manager, feedback, + expected_model, } = common; let mut rng = rand::rng(); let placeholder = EXAMPLE_PROMPTS[rng.random_range(0..EXAMPLE_PROMPTS.len())].to_string(); @@ -1270,6 +1285,8 @@ impl ChatWidget { feedback, current_rollout_path: None, pending_agent: None, + expected_model, + session_configured_received: false, }; widget.prefetch_rate_limits(); @@ -1292,6 +1309,7 @@ impl ChatWidget { enhanced_keys_supported, auth_manager, feedback, + expected_model, } = common; let mut rng = rand::rng(); let placeholder = EXAMPLE_PROMPTS[rng.random_range(0..EXAMPLE_PROMPTS.len())].to_string(); @@ -1348,6 +1366,9 @@ impl ChatWidget { feedback, current_rollout_path: None, pending_agent: None, + expected_model, + // For existing conversations, we've already received SessionConfigured + session_configured_received: true, }; widget.prefetch_rate_limits(); @@ -1363,16 +1384,6 @@ impl ChatWidget { }); } - /// Clear any pending agent selection. - pub(crate) fn clear_pending_agent(&mut self) { - self.pending_agent = None; - } - - /// Check if there's a pending agent selection. - pub(crate) fn has_pending_agent(&self) -> bool { - self.pending_agent.is_some() - } - pub(crate) fn handle_key_event(&mut self, key_event: KeyEvent) { match key_event { KeyEvent { @@ -1739,6 +1750,46 @@ impl ChatWidget { pub(crate) fn handle_codex_event(&mut self, event: Event) { let Event { id, msg } = event; + + // When expected_model is set (during agent switching), we need to filter events + // to prevent events from the OLD agent from affecting the NEW widget. + if let Some(ref expected) = self.expected_model { + tracing::debug!( + "Event filtering active: expected_model={}, session_configured_received={}", + expected, + self.session_configured_received + ); + if !self.session_configured_received { + // Only process SessionConfigured events, and only if the model matches + match &msg { + EventMsg::SessionConfigured(e) => { + if e.model.to_lowercase() != expected.to_lowercase() { + tracing::debug!( + "Ignoring SessionConfigured from wrong model: expected={}, got={}", + expected, + e.model + ); + return; + } + tracing::debug!( + "SessionConfigured received with matching model: {}", + e.model + ); + // Model matches, proceed with processing + } + // Ignore all other events until SessionConfigured arrives + _ => { + tracing::debug!( + "Ignoring event before SessionConfigured: {:?} (waiting for model={})", + std::mem::discriminant(&msg), + expected + ); + return; + } + } + } + } + self.dispatch_event_msg(Some(id), msg, false); } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index f0c0f960e..8dc00f93f 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -314,6 +314,7 @@ async fn helpers_are_available_and_do_not_panic() { enhanced_keys_supported: false, auth_manager, feedback: codex_feedback::CodexFeedback::new(), + expected_model: None, }; let mut w = ChatWidget::new(init, conversation_manager); // Basic construction sanity. diff --git a/codex-rs/tui/src/nori/agent_picker.rs b/codex-rs/tui/src/nori/agent_picker.rs index bf229c750..07f1397a7 100644 --- a/codex-rs/tui/src/nori/agent_picker.rs +++ b/codex-rs/tui/src/nori/agent_picker.rs @@ -4,8 +4,8 @@ //! Agent selection is tracked as "pending" and the actual switch happens //! on the next prompt submission to avoid disrupting active prompt turns. -use codex_acp::list_available_agents; use codex_acp::AcpAgentInfo; +use codex_acp::list_available_agents; use ratatui::text::Line; use crate::app_event::AppEvent; @@ -15,8 +15,11 @@ use crate::bottom_pane::SelectionItem; use crate::bottom_pane::SelectionViewParams; use crate::bottom_pane::popup_consts::standard_popup_hint_line; -/// Event for pending agent selection +/// Information about a pending agent selection. +/// This struct is stored in the App to track which agent should be switched to +/// when the user submits their next prompt. #[derive(Debug, Clone)] +#[allow(dead_code)] pub struct PendingAgentSelection { /// The model name of the selected agent (e.g., "mock-model", "gemini-2.5-flash") pub model_name: String, @@ -88,13 +91,16 @@ pub fn acp_model_picker_params() -> SelectionViewParams { SelectionViewParams { title: Some("Select Model".to_string()), subtitle: Some("Not available in ACP mode - use /agent instead".to_string()), - footer_hint: Some(Line::from("Press esc to dismiss, or use /agent to switch agents.")), + footer_hint: Some(Line::from( + "Press esc to dismiss, or use /agent to switch agents.", + )), items, ..Default::default() } } /// Get information about an agent by model name +#[allow(dead_code)] pub fn get_agent_info(model_name: &str) -> Option { let normalized = model_name.to_lowercase(); list_available_agents()