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/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 73d561f97..b67d60ea7 100644 --- a/codex-rs/tui-pty-e2e/tests/agent_switching.rs +++ b/codex-rs/tui-pty-e2e/tests/agent_switching.rs @@ -374,3 +374,577 @@ 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 + ); +} + +// ============================================================================ +// 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 5c636bd28..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; @@ -228,6 +229,10 @@ 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, } impl App { @@ -294,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()) } @@ -317,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, @@ -351,6 +358,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. @@ -470,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 { @@ -883,6 +892,69 @@ 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 '{display_name}' selected. Will switch on next prompt submission." + ), + None, + ); + } + 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 + // 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(), + 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(), + 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); + } } Ok(true) } @@ -1072,6 +1144,7 @@ mod tests { pending_update_action: None, suppress_shutdown_complete: false, skip_world_writable_scan_once: false, + pending_agent: None, } } @@ -1109,6 +1182,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_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 cf494f57d..caec1a780 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -166,6 +166,28 @@ 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, + }, + + /// 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..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)] @@ -318,6 +322,21 @@ 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, + // 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. +#[derive(Debug, Clone)] +pub(crate) struct PendingAgentInfo { + pub model_name: String, + pub display_name: String, } struct UserMessage { @@ -367,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); @@ -1207,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(); @@ -1260,6 +1284,9 @@ impl ChatWidget { last_rendered_width: std::cell::Cell::new(None), feedback, current_rollout_path: None, + pending_agent: None, + expected_model, + session_configured_received: false, }; widget.prefetch_rate_limits(); @@ -1282,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(); @@ -1337,6 +1365,10 @@ impl ChatWidget { last_rendered_width: std::cell::Cell::new(None), 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(); @@ -1344,6 +1376,14 @@ 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, + }); + } + pub(crate) fn handle_key_event(&mut self, key_event: KeyEvent) { match key_event { KeyEvent { @@ -1473,6 +1513,9 @@ impl ChatWidget { SlashCommand::Review => { self.open_review_popup(); } + SlashCommand::Agent => { + self.open_agent_popup(); + } SlashCommand::Model => { self.open_model_popup(); } @@ -1628,6 +1671,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. @@ -1695,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); } @@ -2084,10 +2179,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..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. @@ -377,6 +378,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..07f1397a7 --- /dev/null +++ b/codex-rs/tui/src/nori/agent_picker.rs @@ -0,0 +1,155 @@ +//! 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::AcpAgentInfo; +use codex_acp::list_available_agents; +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; + +/// 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, + /// 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 +#[allow(dead_code)] +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