diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1ed497ba4..5b2ed8a8a 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1128,7 +1128,6 @@ dependencies = [ name = "codex-core" version = "0.0.0" dependencies = [ - "agent-client-protocol", "anyhow", "askama", "assert_cmd", @@ -1138,7 +1137,6 @@ dependencies = [ "base64", "bytes", "chrono", - "codex-acp", "codex-app-server-protocol", "codex-apply-patch", "codex-arg0", diff --git a/codex-rs/acp/DESIGN_SUMMARY.md b/codex-rs/acp/DESIGN_SUMMARY.md index dc9c3ffc6..e031a039a 100644 --- a/codex-rs/acp/DESIGN_SUMMARY.md +++ b/codex-rs/acp/DESIGN_SUMMARY.md @@ -1,7 +1,7 @@ # ACP Integration Design Summary - `codex-acp` is a parallel crate to `codex-core`, not integrated via shared traits -- Zero modifications to `codex-core` to ease upstream merge burden +- Minimal modifications to `codex-core` to ease upstream merge burden - ACP vs HTTP mode is determined at startup via config, no mid-session switching - TUI/CLI branches once at startup: `if config.acp_agent.is_some() { run_acp_mode() } else { run_http_mode() }` - HTTP mode code path remains completely unchanged @@ -15,4 +15,3 @@ - `permission_request_to_approval_event()` converts ACP requests to Codex format - `review_decision_to_permission_outcome()` converts Codex decisions back to ACP format - Fallback behavior: auto-approve if approval channel closed, deny if response channel dropped -- Model picker stays HTTP-only; ACP agents are not treated as switchable "models" diff --git a/codex-rs/acp/docs.md b/codex-rs/acp/docs.md index d435e6a5d..6b0552228 100644 --- a/codex-rs/acp/docs.md +++ b/codex-rs/acp/docs.md @@ -10,16 +10,16 @@ Path: @/codex-rs/acp ### How it fits into the larger codebase -- Used by `@/codex-rs/core/src/client.rs` to communicate with ACP-compliant agents via `WireApi::Acp` variant +- Designed as a parallel crate to `codex-core`, not tightly integrated - Uses channel-based streaming pattern (mpsc) consistent with core's `ResponseStream` -- Provides structured error handling via library's typed error responses that core translates to user-facing messages +- Provides structured error handling via library's typed error responses - TUI and other clients can access captured stderr for displaying agent diagnostic output +- ACP vs HTTP mode is determined at startup via config, no mid-session switching ### Model Registry The ACP registry in `@/codex-rs/acp/src/registry.rs` is **model-centric** rather than provider-centric: - `get_agent_config()` accepts model names (e.g., "mock-model", "gemini-2.5-flash", "claude-acp") instead of provider names -- Called from `@/codex-rs/core/src/client.rs` at the start of `stream()` to check if model is an ACP agent - Returns `AcpAgentConfig` containing: - `provider_slug`: Identifies which agent subprocess to spawn (e.g., "mock-acp", "gemini-acp", "claude-acp") - `command`: Executable path or command name @@ -34,14 +34,13 @@ The ACP registry in `@/codex-rs/acp/src/registry.rs` is **model-centric** rather ### Embedded Provider Info ACP providers embed their configuration directly in `AcpAgentConfig` via `AcpProviderInfo`: -- Avoids circular dependency between `codex-acp` and `codex-core` (core depends on acp, not vice versa) +- `codex-core` does not depend on `codex-acp` - they are decoupled crates - ACP providers are NOT in `built_in_model_providers()` in core - they're self-contained in the registry - `AcpProviderInfo` contains: - `name`: Display name (e.g., "Gemini ACP") - `request_max_retries`: Max request retries (default: 1) - `stream_max_retries`: Max stream reconnection attempts (default: 1) - `stream_idle_timeout`: Idle timeout for streaming (default: 5 minutes) -- Core's `client.rs` checks the ACP registry first in `stream()`, using the embedded provider info for ACP models ### Stderr Capture Implementation diff --git a/codex-rs/acp/src/lib.rs b/codex-rs/acp/src/lib.rs index 5273b568e..8eb8869b3 100644 --- a/codex-rs/acp/src/lib.rs +++ b/codex-rs/acp/src/lib.rs @@ -14,12 +14,6 @@ pub use registry::AcpAgentConfig; pub use registry::AcpProviderInfo; pub use registry::get_agent_config; pub use tracing_setup::init_file_tracing; -pub use translator::AcpToolCallContent; -pub use translator::AcpToolCallEvent; -pub use translator::AcpToolCallLocation; -pub use translator::AcpToolCallUpdateEvent; -pub use translator::AcpToolKind; -pub use translator::AcpToolStatus; pub use translator::TranslatedEvent; pub use translator::translate_session_update; diff --git a/codex-rs/acp/src/translator.rs b/codex-rs/acp/src/translator.rs index 5c86259ee..e3dde14a5 100644 --- a/codex-rs/acp/src/translator.rs +++ b/codex-rs/acp/src/translator.rs @@ -6,200 +6,6 @@ use agent_client_protocol as acp; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; -use std::path::PathBuf; - -/// Tool kind categories for ACP tool calls. -/// Maps to agent_client_protocol::ToolKind but owned by codex. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum AcpToolKind { - Read, - Edit, - Delete, - Move, - Search, - Execute, - Think, - Fetch, - SwitchMode, - Other, -} - -impl From<&acp::ToolKind> for AcpToolKind { - fn from(kind: &acp::ToolKind) -> Self { - match kind { - acp::ToolKind::Read => AcpToolKind::Read, - acp::ToolKind::Edit => AcpToolKind::Edit, - acp::ToolKind::Delete => AcpToolKind::Delete, - acp::ToolKind::Move => AcpToolKind::Move, - acp::ToolKind::Search => AcpToolKind::Search, - acp::ToolKind::Execute => AcpToolKind::Execute, - acp::ToolKind::Think => AcpToolKind::Think, - acp::ToolKind::Fetch => AcpToolKind::Fetch, - acp::ToolKind::SwitchMode => AcpToolKind::SwitchMode, - acp::ToolKind::Other => AcpToolKind::Other, - } - } -} - -impl std::fmt::Display for AcpToolKind { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AcpToolKind::Read => write!(f, "read"), - AcpToolKind::Edit => write!(f, "edit"), - AcpToolKind::Delete => write!(f, "delete"), - AcpToolKind::Move => write!(f, "move"), - AcpToolKind::Search => write!(f, "search"), - AcpToolKind::Execute => write!(f, "execute"), - AcpToolKind::Think => write!(f, "think"), - AcpToolKind::Fetch => write!(f, "fetch"), - AcpToolKind::SwitchMode => write!(f, "switch_mode"), - AcpToolKind::Other => write!(f, "other"), - } - } -} - -/// Tool call execution status. -/// Maps to agent_client_protocol::ToolCallStatus but owned by codex. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum AcpToolStatus { - Pending, - InProgress, - Completed, - Failed, -} - -impl From<&acp::ToolCallStatus> for AcpToolStatus { - fn from(status: &acp::ToolCallStatus) -> Self { - match status { - acp::ToolCallStatus::Pending => AcpToolStatus::Pending, - acp::ToolCallStatus::InProgress => AcpToolStatus::InProgress, - acp::ToolCallStatus::Completed => AcpToolStatus::Completed, - acp::ToolCallStatus::Failed => AcpToolStatus::Failed, - } - } -} - -impl std::fmt::Display for AcpToolStatus { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - AcpToolStatus::Pending => write!(f, "pending"), - AcpToolStatus::InProgress => write!(f, "in_progress"), - AcpToolStatus::Completed => write!(f, "completed"), - AcpToolStatus::Failed => write!(f, "failed"), - } - } -} - -/// Content produced by a tool call. -#[derive(Debug, Clone)] -pub enum AcpToolCallContent { - /// Text content - Text(String), - /// File diff - Diff { - path: PathBuf, - old_text: Option, - new_text: String, - }, - /// Terminal reference - Terminal { terminal_id: String }, -} - -/// A file location affected by a tool call. -#[derive(Debug, Clone)] -pub struct AcpToolCallLocation { - pub path: PathBuf, - pub line: Option, -} - -/// An ACP tool call event with all relevant information. -#[derive(Debug, Clone)] -pub struct AcpToolCallEvent { - pub call_id: String, - pub title: String, - pub kind: AcpToolKind, - pub status: AcpToolStatus, - pub content: Vec, - pub locations: Vec, - pub raw_input: Option, - pub raw_output: Option, -} - -impl From<&acp::ToolCall> for AcpToolCallEvent { - fn from(tc: &acp::ToolCall) -> Self { - AcpToolCallEvent { - call_id: tc.id.0.to_string(), - title: tc.title.clone(), - kind: AcpToolKind::from(&tc.kind), - status: AcpToolStatus::from(&tc.status), - content: tc.content.iter().filter_map(convert_tool_content).collect(), - locations: tc.locations.iter().map(convert_tool_location).collect(), - raw_input: tc.raw_input.clone(), - raw_output: tc.raw_output.clone(), - } - } -} - -/// An update to an existing ACP tool call. -#[derive(Debug, Clone)] -pub struct AcpToolCallUpdateEvent { - pub call_id: String, - pub title: Option, - pub kind: Option, - pub status: Option, - pub content: Option>, - pub locations: Option>, - pub raw_input: Option, - pub raw_output: Option, -} - -impl From<&acp::ToolCallUpdate> for AcpToolCallUpdateEvent { - fn from(update: &acp::ToolCallUpdate) -> Self { - let fields = &update.fields; - AcpToolCallUpdateEvent { - call_id: update.id.0.to_string(), - title: fields.title.clone(), - kind: fields.kind.as_ref().map(AcpToolKind::from), - status: fields.status.as_ref().map(AcpToolStatus::from), - content: fields - .content - .as_ref() - .map(|c| c.iter().filter_map(convert_tool_content).collect()), - locations: fields - .locations - .as_ref() - .map(|l| l.iter().map(convert_tool_location).collect()), - raw_input: fields.raw_input.clone(), - raw_output: fields.raw_output.clone(), - } - } -} - -/// Convert ACP ToolCallContent to our internal representation. -fn convert_tool_content(content: &acp::ToolCallContent) -> Option { - match content { - acp::ToolCallContent::Content { content } => match content { - acp::ContentBlock::Text(text) => Some(AcpToolCallContent::Text(text.text.clone())), - _ => None, // Non-text content not yet supported - }, - acp::ToolCallContent::Diff { diff } => Some(AcpToolCallContent::Diff { - path: diff.path.clone(), - old_text: diff.old_text.clone(), - new_text: diff.new_text.clone(), - }), - acp::ToolCallContent::Terminal { terminal_id } => Some(AcpToolCallContent::Terminal { - terminal_id: terminal_id.0.to_string(), - }), - } -} - -/// Convert ACP ToolCallLocation to our internal representation. -fn convert_tool_location(loc: &acp::ToolCallLocation) -> AcpToolCallLocation { - AcpToolCallLocation { - path: loc.path.clone(), - line: loc.line, - } -} /// Convert codex ResponseItems to ACP ContentBlocks for prompting. /// @@ -252,16 +58,12 @@ pub fn text_to_content_block(text: &str) -> acp::ContentBlock { } /// Represents an event translated from an ACP SessionUpdate. -#[derive(Debug, Clone)] +#[derive(Debug)] pub enum TranslatedEvent { /// Text content from the agent TextDelta(String), /// Agent completed the message with a stop reason Completed(acp::StopReason), - /// A new tool call has been initiated by the ACP agent - ToolCall(AcpToolCallEvent), - /// An existing tool call has been updated - ToolCallUpdate(AcpToolCallUpdateEvent), } /// Translate an ACP SessionUpdate to a list of TranslatedEvents. @@ -301,17 +103,14 @@ pub fn translate_session_update(update: acp::SessionUpdate) -> Vec { - // Convert ACP ToolCall to our internal representation - vec![TranslatedEvent::ToolCall(AcpToolCallEvent::from( - &tool_call, - ))] + acp::SessionUpdate::ToolCall(_tool_call) => { + // Tool calls are complex - for now, we just note them + // The agent will send updates about tool execution via ToolCallUpdate + vec![] } - acp::SessionUpdate::ToolCallUpdate(update) => { - // Convert ACP ToolCallUpdate to our internal representation - vec![TranslatedEvent::ToolCallUpdate( - AcpToolCallUpdateEvent::from(&update), - )] + acp::SessionUpdate::ToolCallUpdate(_update) => { + // Tool call results - could be mapped to function call outputs + vec![] } acp::SessionUpdate::Plan(_plan) => { // Plans are agent-internal state @@ -381,9 +180,10 @@ fn extract_command_from_tool_call(tool_call: &acp::ToolCallUpdate) -> Vec Vec Option { // Use the title as a basic description, or fall back to ID - let name = tool_call - .fields - .title - .as_deref() - .unwrap_or("unknown tool"); + let name = tool_call.fields.title.as_deref().unwrap_or("unknown tool"); Some(format!("ACP agent requests permission to use: {name}")) } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index d6f6ba50a..4d8f43778 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -12,7 +12,6 @@ path = "src/lib.rs" workspace = true [dependencies] -agent-client-protocol = "0.7" anyhow = { workspace = true } askama = { workspace = true } async-channel = { workspace = true } @@ -20,7 +19,6 @@ async-trait = { workspace = true } base64 = { workspace = true } bytes = { workspace = true } chrono = { workspace = true, features = ["serde"] } -codex-acp = { path = "../acp" } codex-app-server-protocol = { workspace = true } codex-apply-patch = { workspace = true } codex-async-utils = { workspace = true } diff --git a/codex-rs/core/docs.md b/codex-rs/core/docs.md index 56a86291b..3152f1f1a 100644 --- a/codex-rs/core/docs.md +++ b/codex-rs/core/docs.md @@ -82,47 +82,13 @@ The `auth/` module manages: **Model Client Architecture:** -The `client.rs` defines `ModelClient` trait implemented by: -- Default client for OpenAI-compatible APIs -- ACP client for Agent Context Protocol agents +`client.rs` provides `ModelClient` for communicating with HTTP-based model providers. Response streaming uses `ResponseStream` of `ResponseEvent` items. -Response streaming uses `ResponseStream` of `ResponseEvent` items. +The `WireApi` enum defines two HTTP-based protocols: +- `WireApi::Responses`: OpenAI Responses API (used by some internal models) +- `WireApi::Chat`: OpenAI Chat Completions API (the default) -For ACP providers (`wire_api: WireApi::Acp`), the client looks up subprocess configuration via `codex_acp::get_agent_config(self.config.model)` from `@/codex-rs/acp/src/registry.rs`. The registry is **model-centric**: it maps model names (e.g., "mock-model", "gemini-2.5-flash", "claude-acp") to `AcpAgentConfig` structs containing provider identifier, command, and args. This differs from the provider-based approach used for HTTP APIs. ACP providers should not define `env_key` or `env_key_instructions` in their `ModelProviderInfo` entries, as they communicate via subprocess rather than HTTP APIs. Unit test `test_claude_acp_model_has_family()` in `@/codex-rs/core/src/client_acp_tests.rs` verifies that Claude ACP models resolve to a valid model family. - -**ACP Streaming Flow (`stream_acp` / `stream_acp_internal`):** - -When ACP provider is detected in `stream()`, control passes to `stream_acp()` which: - -``` -Client.stream() - │ - ├─► Check ACP registry for model - │ │ - │ ├─► Not found: Continue to HTTP providers - │ └─► Found: Call stream_acp(config, prompt) - │ - └─► stream_acp() - │ - ├─► Convert prompt to ACP ContentBlocks via translator - ├─► Spawn async task with stream_acp_internal() - └─► Return ResponseStream immediately - -stream_acp_internal() [in spawned task]: - │ - ├─► AcpConnection::spawn() - Create subprocess & worker thread - ├─► connection.create_session() - ├─► Send OutputItemAdded event (establishes active_item) - ├─► Spawn forward_task for update translation - ├─► connection.prompt() - Blocks until completion - ├─► Wait for forward_task - ├─► Send OutputItemDone with accumulated text - └─► Send Completed event -``` - -**Critical Invariant - OutputItemAdded First:** - -The codex-core event processing expects `OutputItemAdded` before any `OutputTextDelta` events to establish the "active_item" tracking in the TUI. The ACP integration sends an empty assistant message via `OutputItemAdded` at the start, then streams text deltas, then sends `OutputItemDone` with the complete accumulated text. +ACP (Agent Context Protocol) integration is handled separately in `@/codex-rs/acp`, not embedded in core's model client. This decoupled architecture means codex-core only handles HTTP-based providers. **Session Recording:** diff --git a/codex-rs/core/src/chat_completions.rs b/codex-rs/core/src/chat_completions.rs index 7200ff06e..785a4d4ce 100644 --- a/codex-rs/core/src/chat_completions.rs +++ b/codex-rs/core/src/chat_completions.rs @@ -931,11 +931,6 @@ where Poll::Ready(Some(Ok(ResponseEvent::OutputItemAdded(item)))) => { return Poll::Ready(Some(Ok(ResponseEvent::OutputItemAdded(item)))); } - Poll::Ready(Some(Ok(ResponseEvent::Acp(acp_event)))) => { - // ACP events should not appear in chat completions streams, - // but forward them if they somehow do. - return Poll::Ready(Some(Ok(ResponseEvent::Acp(acp_event)))); - } } } } diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index ef8398c1f..13c277a77 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -35,7 +35,6 @@ use crate::auth::CodexAuth; use crate::auth::RefreshTokenError; use crate::chat_completions::AggregateStreamExt; use crate::chat_completions::stream_chat_completions; -use crate::client_common::AcpResponseEvent; use crate::client_common::Prompt; use crate::client_common::Reasoning; use crate::client_common::ResponseEvent; @@ -155,21 +154,6 @@ impl ModelClient { } pub async fn stream(&self, prompt: &Prompt) -> Result { - // First check if this model is an ACP agent. If so, the agent config - // contains embedded provider info and we use the ACP wire protocol. - // Otherwise, use the configured HTTP-based provider. - if let Ok(acp_config) = codex_acp::get_agent_config(&self.get_model()) { - debug!( - "Resolved ACP agent: {}, provider: {}, command: {}", - &self.get_model(), - acp_config.provider_slug, - acp_config.command - ); - - return self.stream_acp(&acp_config, prompt).await; - } - - // Non-ACP path: use HTTP-based provider (Responses or Chat API) match self.provider.wire_api { WireApi::Responses => self.stream_responses(prompt).await, WireApi::Chat => { @@ -209,15 +193,6 @@ impl ModelClient { Ok(ResponseStream { rx_event: rx }) } - WireApi::Acp => { - // This branch should not be reached since ACP models are handled above. - // If we get here, it means someone manually configured wire_api: acp - // for a model that isn't in the ACP registry. - Err(CodexErr::Fatal(format!( - "Model '{}' has wire_api=acp but is not registered in the ACP registry", - &self.config.model - ))) - } } } @@ -329,44 +304,6 @@ impl ModelClient { unreachable!("stream_responses_attempt should always return"); } - /// Implementation for ACP (Agent Client Protocol) agents. - /// - /// This spawns a subprocess agent and communicates via JSON-RPC over stdio. - async fn stream_acp( - &self, - acp_config: &codex_acp::AcpAgentConfig, - prompt: &Prompt, - ) -> Result { - // Convert prompt to ACP content blocks - let content_blocks = codex_acp::translator::response_items_to_content_blocks(&prompt.input); - - // If there's no content, use the instructions as the prompt - let content_blocks = if content_blocks.is_empty() { - let instructions = prompt.get_full_instructions(&self.config.model_family); - vec![codex_acp::translator::text_to_content_block(&instructions)] - } else { - content_blocks - }; - - let cwd = self.config.cwd.clone(); - let acp_config = acp_config.clone(); - - // Create channel for ResponseEvent - let (tx, rx) = mpsc::channel::>(32); - - // Spawn a task to manage the ACP connection and prompt - tokio::spawn(async move { - let result = stream_acp_internal(&acp_config, &cwd, content_blocks, tx.clone()).await; - - if let Err(e) = result { - // Send error through channel - let _ = tx.send(Err(e)).await; - } - }); - - Ok(ResponseStream { rx_event: rx }) - } - /// Single attempt to start a streaming Responses API call. async fn attempt_stream_responses( &self, @@ -1095,150 +1032,6 @@ async fn stream_from_fixture( Ok(ResponseStream { rx_event }) } -/// Internal helper for ACP streaming. -/// -/// This spawns an ACP connection, creates a session, sends the prompt, -/// and translates updates to ResponseEvent. -async fn stream_acp_internal( - config: &codex_acp::AcpAgentConfig, - cwd: &Path, - content_blocks: Vec, - tx: mpsc::Sender>, -) -> Result<()> { - use agent_client_protocol::SessionUpdate; - use codex_acp::translator::TranslatedEvent; - use codex_acp::translator::translate_session_update; - use codex_protocol::models::ContentItem; - use codex_protocol::models::ResponseItem; - use std::sync::Arc; - use std::sync::atomic::AtomicBool; - use std::sync::atomic::Ordering; - - // Spawn ACP connection - let connection = codex_acp::AcpConnection::spawn(config, cwd) - .await - .map_err(|e| CodexErr::Fatal(format!("Failed to spawn ACP connection: {e}")))?; - - // Create session - let session_id = connection - .create_session(cwd) - .await - .map_err(|e| CodexErr::Fatal(format!("Failed to create ACP session: {e}")))?; - - // Generate a unique message ID for this response - let message_id = uuid::Uuid::new_v4().to_string(); - - // Track whether we've sent the OutputItemAdded event - let item_started = Arc::new(AtomicBool::new(false)); - - // Accumulate all text content for the final OutputItemDone event - let accumulated_text = Arc::new(tokio::sync::Mutex::new(String::new())); - - // Create channel for session updates - let (update_tx, mut update_rx) = mpsc::channel::(32); - - // First, send an OutputItemAdded event to establish the active item - // This creates an assistant message that will receive subsequent text deltas - let initial_item = ResponseItem::Message { - id: Some(message_id.clone()), - role: "assistant".to_string(), - content: vec![], // Initially empty; text will be accumulated via deltas - }; - let _ = tx - .send(Ok(ResponseEvent::OutputItemAdded(initial_item))) - .await; - item_started.store(true, Ordering::SeqCst); - - // Spawn a task to forward updates while the prompt is running - let tx_clone = tx.clone(); - let accumulated_text_clone = accumulated_text.clone(); - let forward_task = tokio::spawn(async move { - while let Some(update) = update_rx.recv().await { - let events = translate_session_update(update); - for event in events { - match event { - TranslatedEvent::TextDelta(text) => { - // Accumulate text for the final message - { - let mut acc = accumulated_text_clone.lock().await; - acc.push_str(&text); - } - - // Send text delta event - if tx_clone - .send(Ok(ResponseEvent::OutputTextDelta(text))) - .await - .is_err() - { - return; - } - } - TranslatedEvent::Completed(_) => { - // Completion is handled when the prompt returns - } - TranslatedEvent::ToolCall(tool_call) => { - // Forward tool call event to the client - if tx_clone - .send(Ok(ResponseEvent::Acp(AcpResponseEvent::ToolCall( - tool_call, - )))) - .await - .is_err() - { - return; - } - } - TranslatedEvent::ToolCallUpdate(update) => { - // Forward tool call update event to the client - if tx_clone - .send(Ok(ResponseEvent::Acp(AcpResponseEvent::ToolCallUpdate( - update, - )))) - .await - .is_err() - { - return; - } - } - } - } - } - }); - - // Send prompt and wait for completion - let stop_reason = connection - .prompt(session_id, content_blocks, update_tx) - .await - .map_err(|e| CodexErr::Fatal(format!("ACP prompt failed: {e}")))?; - - // Wait for all updates to be forwarded - forward_task.await.ok(); - - // Get the accumulated text for the final message - let final_text = { - let acc = accumulated_text.lock().await; - acc.clone() - }; - - // Send OutputItemDone with the completed message - let final_item = ResponseItem::Message { - id: Some(message_id), - role: "assistant".to_string(), - content: vec![ContentItem::OutputText { text: final_text }], - }; - let _ = tx.send(Ok(ResponseEvent::OutputItemDone(final_item))).await; - - // Send completion event - let _ = tx - .send(Ok(ResponseEvent::Completed { - response_id: format!("acp-{stop_reason:?}"), - token_usage: None, - })) - .await; - - Ok(()) -} - fn rate_limit_regex() -> &'static Regex { static RE: OnceLock = OnceLock::new(); diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index ad496bca5..09cc76922 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -3,8 +3,6 @@ use crate::error::Result; use crate::model_family::ModelFamily; use crate::protocol::RateLimitSnapshot; use crate::protocol::TokenUsage; -use codex_acp::AcpToolCallEvent; -use codex_acp::AcpToolCallUpdateEvent; use codex_apply_patch::APPLY_PATCH_TOOL_INSTRUCTIONS; use codex_protocol::config_types::ReasoningEffort as ReasoningEffortConfig; use codex_protocol::config_types::ReasoningSummary as ReasoningSummaryConfig; @@ -195,18 +193,6 @@ fn strip_total_output_header(output: &str) -> Option<&str> { Some(remainder) } -/// ACP-specific response events. -/// -/// This enum encapsulates all ACP-related events to minimize changes -/// to the parent `ResponseEvent` enum. -#[derive(Debug)] -pub enum AcpResponseEvent { - /// A new tool call has been initiated by the ACP agent - ToolCall(AcpToolCallEvent), - /// An existing tool call has been updated - ToolCallUpdate(AcpToolCallUpdateEvent), -} - #[derive(Debug)] pub enum ResponseEvent { Created, @@ -229,8 +215,6 @@ pub enum ResponseEvent { summary_index: i64, }, RateLimits(RateLimitSnapshot), - /// ACP-specific events (tool calls, updates, etc.) - Acp(AcpResponseEvent), } #[derive(Debug, Serialize)] diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index c156272e1..64d06d057 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -2235,7 +2235,7 @@ async fn try_run_turn( sess.send_event(&turn_context, EventMsg::AgentMessageContentDelta(event)) .await; } else { - error_or_panic("OutputTextDelta without active item".to_string()); + error_or_panic("ReasoningSummaryDelta without active item".to_string()); } } ResponseEvent::ReasoningSummaryDelta { @@ -2286,123 +2286,6 @@ async fn try_run_turn( error_or_panic("ReasoningRawContentDelta without active item".to_string()); } } - ResponseEvent::Acp(acp_event) => { - // Handle ACP-specific events (tool calls from ACP agents) - use crate::client_common::AcpResponseEvent; - use codex_protocol::protocol::AcpEventMsg; - use codex_protocol::protocol::AcpToolCallEventMsg; - use codex_protocol::protocol::AcpToolCallKind; - use codex_protocol::protocol::AcpToolCallStatus; - - match acp_event { - AcpResponseEvent::ToolCall(tool_call) => { - let event_msg = AcpToolCallEventMsg { - call_id: tool_call.call_id, - title: tool_call.title, - kind: match tool_call.kind { - codex_acp::AcpToolKind::Read => AcpToolCallKind::Read, - codex_acp::AcpToolKind::Edit => AcpToolCallKind::Edit, - codex_acp::AcpToolKind::Delete => AcpToolCallKind::Delete, - codex_acp::AcpToolKind::Move => AcpToolCallKind::Move, - codex_acp::AcpToolKind::Search => AcpToolCallKind::Search, - codex_acp::AcpToolKind::Execute => AcpToolCallKind::Execute, - codex_acp::AcpToolKind::Think => AcpToolCallKind::Think, - codex_acp::AcpToolKind::Fetch => AcpToolCallKind::Fetch, - codex_acp::AcpToolKind::SwitchMode => AcpToolCallKind::SwitchMode, - codex_acp::AcpToolKind::Other => AcpToolCallKind::Other, - }, - status: match tool_call.status { - codex_acp::AcpToolStatus::Pending => AcpToolCallStatus::Pending, - codex_acp::AcpToolStatus::InProgress => { - AcpToolCallStatus::InProgress - } - codex_acp::AcpToolStatus::Completed => AcpToolCallStatus::Completed, - codex_acp::AcpToolStatus::Failed => AcpToolCallStatus::Failed, - }, - }; - sess.send_event( - &turn_context, - EventMsg::Acp(AcpEventMsg::ToolCall(event_msg)), - ) - .await; - } - AcpResponseEvent::ToolCallUpdate(update) => { - // For updates, we emit the updated state - // The TUI is responsible for merging with previous state - let status = update.status.clone(); - if let (Some(status), Some(title)) = (status, update.title.clone()) { - let event_msg = AcpToolCallEventMsg { - call_id: update.call_id.clone(), - title, - kind: update.kind.map_or(AcpToolCallKind::Other, |k| match k { - codex_acp::AcpToolKind::Read => AcpToolCallKind::Read, - codex_acp::AcpToolKind::Edit => AcpToolCallKind::Edit, - codex_acp::AcpToolKind::Delete => AcpToolCallKind::Delete, - codex_acp::AcpToolKind::Move => AcpToolCallKind::Move, - codex_acp::AcpToolKind::Search => AcpToolCallKind::Search, - codex_acp::AcpToolKind::Execute => AcpToolCallKind::Execute, - codex_acp::AcpToolKind::Think => AcpToolCallKind::Think, - codex_acp::AcpToolKind::Fetch => AcpToolCallKind::Fetch, - codex_acp::AcpToolKind::SwitchMode => { - AcpToolCallKind::SwitchMode - } - codex_acp::AcpToolKind::Other => AcpToolCallKind::Other, - }), - status: match status { - codex_acp::AcpToolStatus::Pending => AcpToolCallStatus::Pending, - codex_acp::AcpToolStatus::InProgress => { - AcpToolCallStatus::InProgress - } - codex_acp::AcpToolStatus::Completed => { - AcpToolCallStatus::Completed - } - codex_acp::AcpToolStatus::Failed => AcpToolCallStatus::Failed, - }, - }; - sess.send_event( - &turn_context, - EventMsg::Acp(AcpEventMsg::ToolCall(event_msg)), - ) - .await; - } else if let Some(status) = update.status.clone() { - // Only status update - still emit it - let event_msg = AcpToolCallEventMsg { - call_id: update.call_id.clone(), - title: update.title.unwrap_or_default(), - kind: update.kind.map_or(AcpToolCallKind::Other, |k| match k { - codex_acp::AcpToolKind::Read => AcpToolCallKind::Read, - codex_acp::AcpToolKind::Edit => AcpToolCallKind::Edit, - codex_acp::AcpToolKind::Delete => AcpToolCallKind::Delete, - codex_acp::AcpToolKind::Move => AcpToolCallKind::Move, - codex_acp::AcpToolKind::Search => AcpToolCallKind::Search, - codex_acp::AcpToolKind::Execute => AcpToolCallKind::Execute, - codex_acp::AcpToolKind::Think => AcpToolCallKind::Think, - codex_acp::AcpToolKind::Fetch => AcpToolCallKind::Fetch, - codex_acp::AcpToolKind::SwitchMode => { - AcpToolCallKind::SwitchMode - } - codex_acp::AcpToolKind::Other => AcpToolCallKind::Other, - }), - status: match status { - codex_acp::AcpToolStatus::Pending => AcpToolCallStatus::Pending, - codex_acp::AcpToolStatus::InProgress => { - AcpToolCallStatus::InProgress - } - codex_acp::AcpToolStatus::Completed => { - AcpToolCallStatus::Completed - } - codex_acp::AcpToolStatus::Failed => AcpToolCallStatus::Failed, - }, - }; - sess.send_event( - &turn_context, - EventMsg::Acp(AcpEventMsg::ToolCall(event_msg)), - ) - .await; - } - } - } - } } } } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 515fe3ce9..5b57d4dc0 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -52,7 +52,6 @@ use std::collections::HashMap; use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; -use tracing; use crate::config::profile::ConfigProfile; use toml::Value as TomlValue; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index b57e002e9..0a2d839da 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -35,8 +35,6 @@ mod mcp_tool_call; mod message_history; mod model_provider_info; -#[cfg(test)] -mod client_acp_tests; pub mod parse_command; pub mod powershell; mod response_processing; diff --git a/codex-rs/core/src/model_provider_info.rs b/codex-rs/core/src/model_provider_info.rs index a2cd7deda..7c9dc7a23 100644 --- a/codex-rs/core/src/model_provider_info.rs +++ b/codex-rs/core/src/model_provider_info.rs @@ -34,13 +34,12 @@ const MAX_REQUEST_MAX_RETRIES: u64 = 100; #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum WireApi { + /// The OpenAI Responses API. This is used by some internal models. + Responses, + /// The OpenAI Chat Completions API. This is the default. #[default] Chat, - /// The OpenAI Responses API. This is used by some internal models. - Responses, - /// The Agent Context Protocol. This is used by ACP-compliant agent subprocesses. - Acp, } /// Serializable representation of a provider definition. @@ -203,7 +202,6 @@ impl ModelProviderInfo { match self.wire_api { WireApi::Responses => format!("{base_url}/responses{query_string}"), WireApi::Chat => format!("{base_url}/chat/completions{query_string}"), - WireApi::Acp => String::new(), // ACP uses subprocess, not HTTP } } diff --git a/codex-rs/core/src/rollout/policy.rs b/codex-rs/core/src/rollout/policy.rs index 4d01f94d6..9e0e30836 100644 --- a/codex-rs/core/src/rollout/policy.rs +++ b/codex-rs/core/src/rollout/policy.rs @@ -84,7 +84,6 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool { | EventMsg::ItemCompleted(_) | EventMsg::AgentMessageContentDelta(_) | EventMsg::ReasoningContentDelta(_) - | EventMsg::ReasoningRawContentDelta(_) - | EventMsg::Acp(_) => false, + | EventMsg::ReasoningRawContentDelta(_) => false, } } diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 06f89fd06..8c7bb6881 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -570,8 +570,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { | EventMsg::ReasoningContentDelta(_) | EventMsg::ReasoningRawContentDelta(_) | EventMsg::UndoCompleted(_) - | EventMsg::UndoStarted(_) - | EventMsg::Acp(_) => {} + | EventMsg::UndoStarted(_) => {} } CodexStatus::Running } diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 56c6f9407..93dc7764d 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -301,8 +301,7 @@ async fn run_codex_tool_session_inner( | EventMsg::UndoStarted(_) | EventMsg::UndoCompleted(_) | EventMsg::ExitedReviewMode(_) - | EventMsg::DeprecationNotice(_) - | EventMsg::Acp(_) => { + | EventMsg::DeprecationNotice(_) => { // For now, we do not do anything extra for these // events. Note that // send(codex_event_to_notification(&event)) above has diff --git a/codex-rs/protocol/docs.md b/codex-rs/protocol/docs.md index 250c7dccf..898f03241 100644 --- a/codex-rs/protocol/docs.md +++ b/codex-rs/protocol/docs.md @@ -23,7 +23,7 @@ This separation ensures consistent type definitions without circular dependencie | Module | Contents | |--------|----------| -| `protocol` | `Event`, `Op`, `EventMsg`, `AskForApproval`, session types | +| `protocol` | `Event`, `Op`, `EventMsg`, `AskForApproval`, session types, turn lifecycle | | `models` | `ResponseItem`, `ContentItem`, `LocalShellAction`, etc. | | `config_types` | `SandboxMode`, `TrustLevel`, model settings | | `user_input` | `UserInput` variants (text, image, file) | diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 627d94cf9..7f5e5228d 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -560,9 +560,6 @@ pub enum EventMsg { AgentMessageContentDelta(AgentMessageContentDeltaEvent), ReasoningContentDelta(ReasoningContentDeltaEvent), ReasoningRawContentDelta(ReasoningRawContentDeltaEvent), - - /// ACP-specific events (tool calls from ACP agents). - Acp(AcpEventMsg), } #[derive(Debug, Clone, Deserialize, Serialize, TS, JsonSchema)] @@ -1557,60 +1554,6 @@ pub enum TurnAbortReason { ReviewEnded, } -// ──────────────────────────────────────────────────────────────────────────────── -// ACP (Agent Client Protocol) Events -// ──────────────────────────────────────────────────────────────────────────────── - -/// Tool call status for ACP tool calls. -#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] -#[serde(rename_all = "snake_case")] -pub enum AcpToolCallStatus { - Pending, - InProgress, - Completed, - Failed, -} - -/// Tool kind for ACP tool calls. -#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] -#[serde(rename_all = "snake_case")] -pub enum AcpToolCallKind { - Read, - Edit, - Delete, - Move, - Search, - Execute, - Think, - Fetch, - SwitchMode, - Other, -} - -/// An ACP tool call event for display in the TUI. -#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] -pub struct AcpToolCallEventMsg { - /// Unique ID for this tool call. - pub call_id: String, - /// Human-readable title for the tool call. - pub title: String, - /// Category of the tool call. - pub kind: AcpToolCallKind, - /// Current status of the tool call. - pub status: AcpToolCallStatus, -} - -/// ACP-specific events encapsulated in a nested enum. -/// -/// This pattern minimizes changes to the parent EventMsg enum -/// while allowing for future ACP event types. -#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] -#[serde(tag = "acp_type", rename_all = "snake_case")] -pub enum AcpEventMsg { - /// A tool call has been initiated or updated by the ACP agent. - ToolCall(AcpToolCallEventMsg), -} - #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/tui-pty-e2e/docs.md b/codex-rs/tui-pty-e2e/docs.md index 6445870af..e162cb781 100644 --- a/codex-rs/tui-pty-e2e/docs.md +++ b/codex-rs/tui-pty-e2e/docs.md @@ -47,7 +47,9 @@ impl Drop for TuiSession { The crate exports helper functions for consistent test patterns: - `TIMEOUT: Duration` - Standard 5-second timeout constant for use across all tests +- `TIMEOUT_INPUT: Duration` - 300ms timeout for input stabilization before snapshots - `normalize_for_snapshot(contents: String) -> String` - Normalizes dynamic content for snapshot testing (see below) +- `normalize_for_input_snapshot(contents: String) -> String` - Extends normalization by stripping the startup header block (see below) **Automatic Test Isolation:** @@ -142,6 +144,7 @@ This delay allows the PTY subprocess time to process input and update the displa | `@/codex-rs/tui-pty-e2e/tests/prompt_flow.rs` | Prompt submission and agent responses | | `@/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 and response flow - validates TUI works with ACP wire API and mock agent; includes TDD marker test for approval bridging (`#[ignore]`) | | `@/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:** @@ -151,6 +154,7 @@ This delay allows the PTY subprocess time to process input and update the displa | `@/codex-rs/tui-pty-e2e/tests/snapshots/startup__*.snap` | Various startup screen scenarios (welcome, dimensions, temp directory, trust screen) | | `@/codex-rs/tui-pty-e2e/tests/snapshots/input_handling__*.snap` | Input handling scenarios (ctrl-c clear, typing/backspace, model changed) | | `@/codex-rs/tui-pty-e2e/tests/snapshots/streaming__submit_input.snap` | Prompt submission and streaming response | +| `@/codex-rs/tui-pty-e2e/tests/snapshots/acp_mode__*.snap` | ACP mode startup screen | **Snapshot Testing with Insta:** @@ -163,24 +167,27 @@ Snapshots stored in `@/codex-rs/tui-pty-e2e/tests/snapshots/*.snap` for regressi **Snapshot Normalization:** -The `normalize_for_snapshot()` helper function exported from `@/codex-rs/tui-pty-e2e/src/lib.rs` ensures stable snapshots across test runs by replacing dynamic content: +Two normalization helpers in `@/codex-rs/tui-pty-e2e/src/lib.rs` ensure stable snapshots: -Normalization rules: +| Function | Use Case | +|----------|----------| +| `normalize_for_snapshot()` | General snapshots that should include the startup header | +| `normalize_for_input_snapshot()` | Input-focused tests where header visibility varies with scroll timing | + +**`normalize_for_snapshot()`** - Base normalization rules: 1. Temp directory paths (`/tmp/.tmpXXXXXX`) → `[TMP_DIR]` placeholder 2. Random default prompts on lines starting with `› ` → `[DEFAULT_PROMPT]` placeholder - Detects specific default prompt patterns: "Find and fix a bug", "Explain this codebase", "Write tests for", etc. - Preserves user-entered prompts and UI text like "? for shortcuts" -Implementation in `@/codex-rs/tui-pty-e2e/src/lib.rs:456-488`: -```rust -pub fn normalize_for_snapshot(contents: String) -> String { - // Replace /tmp/.tmpXXXXXX with [TMP_DIR] - // Replace known default prompts with [DEFAULT_PROMPT] - // Preserves UI structure and user input -} -``` +**`normalize_for_input_snapshot()`** - Extends base normalization by stripping the startup header block: +- Detects the header block (lines containing `╭──` through the `/review` and `/model` command list) +- Removes the entire header section to prevent flaky snapshots +- Used by input handling tests in `@/codex-rs/tui-pty-e2e/tests/input_handling.rs` + +**Why Two Functions:** Terminal render timing can cause the startup header block to scroll partially in or out of the viewport before a snapshot is taken. For tests focused on input handling, the header presence is irrelevant - only the input area matters. By stripping the header, `normalize_for_input_snapshot()` produces deterministic snapshots regardless of scroll state. -This normalization allows snapshot assertions to focus on UI structure and static content rather than ephemeral runtime values. All tests import and use this function consistently: `use tui_pty_e2e::{normalize_for_snapshot, ...};` +This normalization allows snapshot assertions to focus on UI structure and static content rather than ephemeral runtime values. **PTY Implementation Details:** diff --git a/codex-rs/tui-pty-e2e/src/lib.rs b/codex-rs/tui-pty-e2e/src/lib.rs index db552e505..364587b30 100644 --- a/codex-rs/tui-pty-e2e/src/lib.rs +++ b/codex-rs/tui-pty-e2e/src/lib.rs @@ -588,3 +588,49 @@ pub fn normalize_for_snapshot(contents: String) -> String { lines.join("\n") } + +/// Normalize for input tests - strips header for consistent snapshot regardless of scroll state +pub fn normalize_for_input_snapshot(contents: String) -> String { + let normalized = normalize_for_snapshot(contents); + + // Strip startup header block if present (prevents flaky snapshots due to scroll timing) + // The header can appear in two forms: + // 1. Boxed header with "╭──" border + // 2. Plain text "To get started, describe a task..." + // Both end with a list of commands like /init, /status, /approvals, /model, /review + let lines: Vec<&str> = normalized.lines().collect(); + + // Detect if header is present (either boxed or plain text form) + let has_header = lines.iter().any(|l| { + l.contains("╭──") + || l.contains("To get started, describe a task") + || l.contains("Welcome to Codex") + }); + + if has_header { + // Find where the header ends (after the /review command line) + let mut skip_until = 0; + for (i, line) in lines.iter().enumerate() { + // The /review line marks the end of the command list + if line.contains("/review") { + skip_until = i + 1; + break; + } + } + // Skip empty lines after the header block + while skip_until < lines.len() && lines[skip_until].trim().is_empty() { + skip_until += 1; + } + if skip_until > 0 { + lines + .into_iter() + .skip(skip_until) + .collect::>() + .join("\n") + } else { + normalized + } + } else { + normalized + } +} diff --git a/codex-rs/tui-pty-e2e/tests/acp_mode.rs b/codex-rs/tui-pty-e2e/tests/acp_mode.rs new file mode 100644 index 000000000..03efa7d90 --- /dev/null +++ b/codex-rs/tui-pty-e2e/tests/acp_mode.rs @@ -0,0 +1,174 @@ +//! E2E tests for ACP mode startup and approval bridging +//! +//! These tests verify that: +//! 1. ACP mode starts correctly when configured via wire_api = "acp" +//! 2. The approval bridging infrastructure works correctly +//! 3. Permission requests from ACP agents are properly displayed in the TUI + +use std::time::Duration; +use tui_pty_e2e::normalize_for_input_snapshot; +use tui_pty_e2e::Key; +use tui_pty_e2e::SessionConfig; +use tui_pty_e2e::TuiSession; +use tui_pty_e2e::TIMEOUT; +use tui_pty_e2e::TIMEOUT_INPUT; + +/// Test that ACP mode starts successfully with mock-model +#[test] +fn test_acp_mode_startup_with_mock_agent() { + let config = SessionConfig::new().with_model("mock-model".to_owned()); + + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode"); + + // Wait for the main prompt to appear (indicated by the chevron character) + session + .wait_for_text("›", TIMEOUT) + .expect("ACP mode TUI should start successfully with mock agent"); + + std::thread::sleep(TIMEOUT_INPUT); + + let contents = session.screen_contents(); + + // Verify we're in the TUI and not stuck at an error screen + assert!( + contents.contains("›") && contents.contains("context left"), + "Should show main prompt with context indicator in ACP mode, got: {}", + contents + ); + + // Should NOT show any ACP-related errors + assert!( + !contents.contains("Error") && !contents.contains("error"), + "ACP mode should start without errors, got: {}", + contents + ); +} + +/// Test that ACP mode can send a prompt and receive a response from mock agent +#[test] +fn test_acp_mode_prompt_response_flow() { + let config = SessionConfig::new() + .with_model("mock-model".to_owned()) + .with_mock_response("Hello from ACP mock agent!"); + + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode"); + + // Wait for startup + session + .wait_for_text("›", TIMEOUT) + .expect("ACP mode should start"); + + std::thread::sleep(TIMEOUT_INPUT); + + // Send a test prompt + session.send_str("Test ACP prompt").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for the mock response + session + .wait_for_text("Hello from ACP mock agent!", TIMEOUT) + .expect("Should receive response from mock ACP agent"); +} + +/// Test that ACP approval requests are displayed in the TUI +/// +/// This test verifies the approval bridging infrastructure by: +/// 1. Configuring the mock agent to request permission +/// 2. Verifying the permission request appears in the TUI +/// 3. Verifying user can respond to the permission request +/// +/// ## Prerequisites for this test to pass: +/// 1. Mock agent must support MOCK_AGENT_REQUEST_PERMISSION env var +/// 2. TUI must listen to AcpConnection::take_approval_receiver() +/// 3. TUI must display ExecApprovalRequestEvent and send ReviewDecision back +/// +/// This test is marked #[ignore] until approval bridging is integrated. +/// Run with: cargo test -p tui-pty-e2e -- --ignored +#[test] +#[ignore] +fn test_acp_approval_request_displayed_in_tui() { + let config = SessionConfig::new() + .with_model("mock-model".to_owned()) + // Configure mock agent to request permission before responding + .with_agent_env("MOCK_AGENT_REQUEST_PERMISSION", "1"); + + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode"); + + // Wait for startup + session + .wait_for_text("›", TIMEOUT) + .expect("ACP mode should start"); + + std::thread::sleep(TIMEOUT_INPUT); + + // Send a prompt that triggers a permission request + session.send_str("Run a shell command").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for the approval request to appear + // The TUI should display something like "ACP agent requests permission" + // or show the approval popup from ExecApprovalRequestEvent + let approval_appeared = session.wait_for( + |screen| { + screen.contains("permission") + || screen.contains("approve") + || screen.contains("allow") + || screen.contains("deny") + || screen.contains("ACP agent requests") + }, + Duration::from_secs(10), + ); + + match approval_appeared { + Ok(()) => { + // Approval UI appeared - verify we can see the request + let contents = session.screen_contents(); + eprintln!("Approval request displayed:\n{}", contents); + + // The approval UI should show options to approve or deny + assert!( + contents.contains("allow") + || contents.contains("approve") + || contents.contains("deny") + || contents.contains("reject"), + "Approval UI should show allow/deny options, got: {}", + contents + ); + } + Err(e) => { + // Expected to fail until approval bridging is integrated + panic!( + "Approval request not displayed in TUI. \ + This is expected until approval bridging is integrated. \ + Error: {}. Screen contents:\n{}", + e, + session.screen_contents() + ); + } + } +} + +/// Test snapshot of ACP mode startup screen +#[test] +fn test_acp_mode_startup_snapshot() { + let config = SessionConfig::new().with_model("mock-model".to_owned()); + + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode"); + + session + .wait_for_text("›", TIMEOUT) + .expect("ACP mode should start"); + + std::thread::sleep(TIMEOUT_INPUT); + + insta::assert_snapshot!( + "acp_mode_startup", + normalize_for_input_snapshot(session.screen_contents()) + ); +} diff --git a/codex-rs/tui-pty-e2e/tests/acp_tool_calls.rs b/codex-rs/tui-pty-e2e/tests/acp_tool_calls.rs index d71b5722b..7819d6060 100644 --- a/codex-rs/tui-pty-e2e/tests/acp_tool_calls.rs +++ b/codex-rs/tui-pty-e2e/tests/acp_tool_calls.rs @@ -1,83 +1,192 @@ -//! E2E tests for ACP tool call display in the TUI +//! E2E tests for ACP tool call rendering in the TUI //! -//! These tests verify that tool calls from ACP agents are properly displayed -//! in the TUI history cells. +//! These tests verify that tool calls from ACP agents are properly +//! rendered in the TUI using the McpToolCallCell component. +//! +//! ## Test Strategy +//! +//! The tests configure the mock-acp-agent to emit ToolCall/ToolCallUpdate +//! events, then verify the TUI displays them correctly. This validates +//! the entire ACP-to-TUI flow: +//! +//! 1. Mock agent sends `SessionUpdate::ToolCall` +//! 2. ACP translator converts to `EventMsg::McpToolCallBegin` +//! 3. TUI chatwidget renders via `McpToolCallCell` +//! +//! ## Expected TUI Output Format +//! +//! Active tool calls display as: +//! ```text +//! • Calling server.tool_name({"arg":"value"}) +//! ``` +//! +//! Completed tool calls display as: +//! ```text +//! ✓ Called server.tool_name({"arg":"value"}) (1.2s) +//! ``` -use insta::assert_snapshot; -use tui_pty_e2e::normalize_for_snapshot; +use std::time::Duration; +use tui_pty_e2e::normalize_for_input_snapshot; use tui_pty_e2e::Key; use tui_pty_e2e::SessionConfig; use tui_pty_e2e::TuiSession; use tui_pty_e2e::TIMEOUT; use tui_pty_e2e::TIMEOUT_INPUT; -/// Test that ACP tool calls are displayed in the TUI +/// Test that an ACP tool call is rendered in the TUI +/// +/// This test verifies the full ACP tool call rendering pipeline: +/// 1. Mock agent emits a ToolCall event +/// 2. Translator converts it to McpToolCallBegin +/// 3. TUI displays it using McpToolCallCell +/// +/// ## Prerequisites for this test to pass: +/// - Mock agent must support MOCK_AGENT_TOOL_CALL env var +/// - translator.rs must handle SessionUpdate::ToolCall +/// - core/client.rs must emit EventMsg::McpToolCallBegin +#[test] +fn test_acp_tool_call_rendered_in_tui() { + // Configure mock agent to send a tool call + let config = SessionConfig::new() + .with_model("mock-model".to_owned()) + // Configure mock agent to emit a tool call before responding + .with_agent_env("MOCK_AGENT_TOOL_CALL", "read_file") + .with_agent_env("MOCK_AGENT_TOOL_CALL_ARGS", r#"{"path":"test.txt"}"#) + .with_mock_response("Done with tool call."); + + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode"); + + // Wait for startup + session + .wait_for_text("›", TIMEOUT) + .expect("ACP mode should start"); + + std::thread::sleep(TIMEOUT_INPUT); + + // Send a prompt that triggers the tool call + session.send_str("Read a file for me").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(Key::Enter).unwrap(); + + // Wait for tool call to appear in TUI + // The tool call should render like: "• Calling acp.read_file(...)" + // or with the server name from ACP + let tool_call_appeared = session.wait_for( + |screen| { + // Look for signs of tool call rendering + screen.contains("Calling") || screen.contains("Called") || screen.contains("read_file") + }, + Duration::from_secs(10), + ); + + match tool_call_appeared { + Ok(()) => { + // Tool call UI appeared + let contents = session.screen_contents(); + + // The tool call should display with the tool name + assert!( + contents.contains("read_file") || contents.contains("Calling"), + "Tool call should show tool name or 'Calling' prefix, got:\n{}", + contents + ); + } + Err(e) => { + panic!( + "Tool call not rendered in TUI. Error: {}. Screen contents:\n{}", + e, + session.screen_contents() + ); + } + } +} + +/// Test that an ACP tool call completion is rendered /// -/// This test verifies that when an ACP agent sends a tool call sequence -/// (pending -> in_progress -> completed), the TUI displays information -/// about the tool call to the user. +/// This test verifies that when a tool call completes: +/// 1. The status changes from "Calling" to "Called" +/// 2. The duration is shown +/// 3. Any output is displayed #[test] -fn test_acp_tool_call_displayed() { - let config = SessionConfig::new().with_tool_call(); - let mut session = TuiSession::spawn_with_config(24, 80, config).unwrap(); +fn test_acp_tool_call_completion_rendered_in_tui() { + // Configure mock agent to send a tool call with completion + let config = SessionConfig::new() + .with_model("mock-model".to_owned()) + .with_agent_env("MOCK_AGENT_TOOL_CALL", "echo") + .with_agent_env("MOCK_AGENT_TOOL_CALL_ARGS", r#"{"message":"hello"}"#) + .with_agent_env("MOCK_AGENT_TOOL_CALL_RESULT", "hello") + .with_mock_response("Tool call completed."); - // Wait for prompt to appear + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode"); + + // Wait for startup session - .wait_for_text("?", TIMEOUT) - .expect("Prompt did not appear"); + .wait_for_text("›", TIMEOUT) + .expect("ACP mode should start"); + std::thread::sleep(TIMEOUT_INPUT); - // Submit a prompt to trigger the mock agent - session.send_str("test tool call").unwrap(); + // Send a prompt + session.send_str("Echo hello").unwrap(); std::thread::sleep(TIMEOUT_INPUT); session.send_key(Key::Enter).unwrap(); - // Wait for the response that comes after the tool call + // Wait for the mock response which means the tool call has completed session - .wait_for_text("Tool call completed successfully", TIMEOUT) - .expect("Tool call completion message not found"); + .wait_for_text("Tool call completed", Duration::from_secs(10)) + .expect("Should receive completion response"); + std::thread::sleep(TIMEOUT_INPUT); - // Verify that the tool call title is displayed in the TUI - // The mock agent sends a tool call with title "Reading configuration file" - assert_snapshot!( - "tool_call_title", - normalize_for_snapshot(session.screen_contents()) + let contents = session.screen_contents(); + + // After completion, should show "Called" with checkmark and duration + // The format is: "✓ Called server.tool_name(...) (Xs)" + assert!( + contents.contains("Called") || contents.contains("echo"), + "Completed tool call should show 'Called' or tool name, got:\n{}", + contents ); } -/// Test that tool call status transitions are reflected in the TUI +/// Snapshot test for ACP tool call rendering /// -/// This test verifies that as the tool call progresses through -/// pending -> in_progress -> completed, the UI updates accordingly. +/// This captures the exact visual rendering of an ACP tool call +/// to detect any regressions in the display format. #[test] -fn test_acp_tool_call_status_updates() { - let config = SessionConfig::new().with_tool_call(); - let mut session = TuiSession::spawn_with_config(24, 80, config).unwrap(); +fn test_acp_tool_call_snapshot() { + let config = SessionConfig::new() + .with_model("mock-model".to_owned()) + .with_agent_env("MOCK_AGENT_TOOL_CALL", "read_file") + .with_agent_env("MOCK_AGENT_TOOL_CALL_ARGS", r#"{"path":"test.txt"}"#) + .with_agent_env("MOCK_AGENT_TOOL_CALL_RESULT", "file content here") + .with_mock_response("Read complete."); + + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn codex in ACP mode"); - // Wait for prompt session - .wait_for_text("?", TIMEOUT) - .expect("Prompt did not appear"); + .wait_for_text("›", TIMEOUT) + .expect("ACP mode should start"); + std::thread::sleep(TIMEOUT_INPUT); - // Submit prompt - session.send_str("test").unwrap(); + // Send prompt to trigger tool call + session.send_str("Read test.txt").unwrap(); std::thread::sleep(TIMEOUT_INPUT); session.send_key(Key::Enter).unwrap(); - // Wait for completion + // Wait for the response session - .wait_for_text("Tool call completed successfully", TIMEOUT) - .expect("Tool call did not complete"); + .wait_for_text("Read complete", Duration::from_secs(10)) + .expect("Should receive response"); - // The screen should show some indication that a tool call occurred - // and completed (e.g., a checkmark, "completed" status, or similar) - let screen = session.screen_contents(); + std::thread::sleep(TIMEOUT_INPUT); - // At minimum, the tool call title should be visible - assert_snapshot!( - "tool_call_completion", - normalize_for_snapshot(session.screen_contents()) + insta::assert_snapshot!( + "acp_tool_call_rendered", + normalize_for_input_snapshot(session.screen_contents()) ); } diff --git a/codex-rs/tui-pty-e2e/tests/input_handling.rs b/codex-rs/tui-pty-e2e/tests/input_handling.rs index dd563c222..8b2671bbe 100644 --- a/codex-rs/tui-pty-e2e/tests/input_handling.rs +++ b/codex-rs/tui-pty-e2e/tests/input_handling.rs @@ -1,6 +1,6 @@ use insta::assert_snapshot; use std::time::Duration; -use tui_pty_e2e::normalize_for_snapshot; +use tui_pty_e2e::normalize_for_input_snapshot; use tui_pty_e2e::Key; use tui_pty_e2e::TuiSession; use tui_pty_e2e::TIMEOUT; @@ -28,7 +28,7 @@ fn test_ctrl_c_clears_input() { std::thread::sleep(TIMEOUT_INPUT); assert_snapshot!( "ctrl_c_clears", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -53,7 +53,7 @@ fn test_backspace() { std::thread::sleep(TIMEOUT_INPUT); assert_snapshot!( "typing_and_backspace", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -77,6 +77,6 @@ fn test_arrows() { std::thread::sleep(TIMEOUT_INPUT); assert_snapshot!( "model_changed", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } diff --git a/codex-rs/tui-pty-e2e/tests/prompt_flow.rs b/codex-rs/tui-pty-e2e/tests/prompt_flow.rs index 609014c04..27fdf6691 100644 --- a/codex-rs/tui-pty-e2e/tests/prompt_flow.rs +++ b/codex-rs/tui-pty-e2e/tests/prompt_flow.rs @@ -1,5 +1,5 @@ use insta::assert_snapshot; -use tui_pty_e2e::normalize_for_snapshot; +use tui_pty_e2e::normalize_for_input_snapshot; use tui_pty_e2e::Key; use tui_pty_e2e::SessionConfig; use tui_pty_e2e::TuiSession; @@ -33,7 +33,7 @@ fn test_submit_prompt_default_response() { std::thread::sleep(TIMEOUT_INPUT); assert_snapshot!( "prompt_submitted", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -67,7 +67,7 @@ fn test_submit_prompt_missing_model() { std::thread::sleep(TIMEOUT_INPUT); assert_snapshot!( "missing_model", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -92,7 +92,7 @@ fn test_submit_prompt_custom_response() { std::thread::sleep(TIMEOUT_INPUT); assert_snapshot!( "custom_response", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -112,6 +112,6 @@ fn test_multiline_input() { std::thread::sleep(TIMEOUT_INPUT); assert_snapshot!( "multiline_input", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/acp_mode__acp_mode_startup.snap b/codex-rs/tui-pty-e2e/tests/snapshots/acp_mode__acp_mode_startup.snap new file mode 100644 index 000000000..6cbec9a18 --- /dev/null +++ b/codex-rs/tui-pty-e2e/tests/snapshots/acp_mode__acp_mode_startup.snap @@ -0,0 +1,7 @@ +--- +source: tui-pty-e2e/tests/acp_mode.rs +expression: normalize_for_snapshot(session.screen_contents()) +--- +› [DEFAULT_PROMPT] + + 100% context left · ? for shortcuts diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_rendered.snap b/codex-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_rendered.snap new file mode 100644 index 000000000..9e04e0c0d --- /dev/null +++ b/codex-rs/tui-pty-e2e/tests/snapshots/acp_tool_calls__acp_tool_call_rendered.snap @@ -0,0 +1,13 @@ +--- +source: tui-pty-e2e/tests/acp_tool_calls.rs +expression: normalize_for_input_snapshot(session.screen_contents()) +--- +› Read test.txt + + +• Read complete. + + +› [DEFAULT_PROMPT] + + 100% context left · ? for shortcuts diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__custom_response.snap b/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__custom_response.snap index c262511d8..a5f3bd93c 100644 --- a/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__custom_response.snap +++ b/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__custom_response.snap @@ -1,16 +1,7 @@ --- source: tui-pty-e2e/tests/prompt_flow.rs -expression: normalize_for_snapshot(session.screen_contents()) +expression: normalize_for_input_snapshot(session.screen_contents()) --- - To get started, describe a task or try one of these commands: - - /init - create an AGENTS.md file with instructions for Codex - /status - show current session configuration - /approvals - choose what Codex can do without approval - /model - choose what model and reasoning effort to use - /review - review any changes and find issues - - › test prompt diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__prompt_submitted.snap b/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__prompt_submitted.snap index 397a7a0d8..bc6fd5d4d 100644 --- a/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__prompt_submitted.snap +++ b/codex-rs/tui-pty-e2e/tests/snapshots/prompt_flow__prompt_submitted.snap @@ -1,16 +1,7 @@ --- source: tui-pty-e2e/tests/prompt_flow.rs -expression: normalize_for_snapshot(session.screen_contents()) +expression: normalize_for_input_snapshot(session.screen_contents()) --- - To get started, describe a task or try one of these commands: - - /init - create an AGENTS.md file with instructions for Codex - /status - show current session configuration - /approvals - choose what Codex can do without approval - /model - choose what model and reasoning effort to use - /review - review any changes and find issues - - › Hello diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/startup__runs_in_temp_directory.snap b/codex-rs/tui-pty-e2e/tests/snapshots/startup__runs_in_temp_directory.snap index f420afa1a..91e68e127 100644 --- a/codex-rs/tui-pty-e2e/tests/snapshots/startup__runs_in_temp_directory.snap +++ b/codex-rs/tui-pty-e2e/tests/snapshots/startup__runs_in_temp_directory.snap @@ -1,23 +1,7 @@ --- source: tui-pty-e2e/tests/startup.rs -expression: normalize_for_snapshot(session.screen_contents()) +expression: normalize_for_input_snapshot(session.screen_contents()) --- -╭──────────────────────────────────────────╮ -│ >_ OpenAI Codex (v0.0.0) │ -│ │ -│ model: mock-model /model to change │ -│ directory: [TMP_DIR] │ -╰──────────────────────────────────────────╯ - - To get started, describe a task or try one of these commands: - - /init - create an AGENTS.md file with instructions for Codex - /status - show current session configuration - /approvals - choose what Codex can do without approval - /model - choose what model and reasoning effort to use - /review - review any changes and find issues - - › [DEFAULT_PROMPT] 100% context left · ? for shortcuts diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_shows_welcome.snap b/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_shows_welcome.snap index b34e572e3..08a9bfb2a 100644 --- a/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_shows_welcome.snap +++ b/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_shows_welcome.snap @@ -1,8 +1,7 @@ --- source: tui-pty-e2e/tests/startup.rs -expression: normalize_for_snapshot(session.screen_contents()) +expression: normalize_for_input_snapshot(session.screen_contents()) --- -                                                    _._:=++==+,_                       _=,/*\+/+\=||=_ _"+_                 ,|*|+**"^`   `"*`"~=~||+        diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_welcome_dimensions_40x120.snap b/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_welcome_dimensions_40x120.snap index f420afa1a..91e68e127 100644 --- a/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_welcome_dimensions_40x120.snap +++ b/codex-rs/tui-pty-e2e/tests/snapshots/startup__startup_welcome_dimensions_40x120.snap @@ -1,23 +1,7 @@ --- source: tui-pty-e2e/tests/startup.rs -expression: normalize_for_snapshot(session.screen_contents()) +expression: normalize_for_input_snapshot(session.screen_contents()) --- -╭──────────────────────────────────────────╮ -│ >_ OpenAI Codex (v0.0.0) │ -│ │ -│ model: mock-model /model to change │ -│ directory: [TMP_DIR] │ -╰──────────────────────────────────────────╯ - - To get started, describe a task or try one of these commands: - - /init - create an AGENTS.md file with instructions for Codex - /status - show current session configuration - /approvals - choose what Codex can do without approval - /model - choose what model and reasoning effort to use - /review - review any changes and find issues - - › [DEFAULT_PROMPT] 100% context left · ? for shortcuts diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/streaming__cancelled_stream.snap b/codex-rs/tui-pty-e2e/tests/snapshots/streaming__cancelled_stream.snap index 1854a6f2e..a085a429c 100644 --- a/codex-rs/tui-pty-e2e/tests/snapshots/streaming__cancelled_stream.snap +++ b/codex-rs/tui-pty-e2e/tests/snapshots/streaming__cancelled_stream.snap @@ -1,21 +1,7 @@ --- source: tui-pty-e2e/tests/streaming.rs -expression: normalize_for_snapshot(session.screen_contents()) +expression: normalize_for_input_snapshot(session.screen_contents()) --- -│ │ -│ model: mock-model /model to change │ -│ directory: [TMP_DIR] │ -╰──────────────────────────────────────────╯ - - To get started, describe a task or try one of these commands: - - /init - create an AGENTS.md file with instructions for Codex - /status - show current session configuration - /approvals - choose what Codex can do without approval - /model - choose what model and reasoning effort to use - /review - review any changes and find issues - - › testing!!! diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/streaming__submit_input.snap b/codex-rs/tui-pty-e2e/tests/snapshots/streaming__submit_input.snap index 1cbcbbab2..e2057d4d3 100644 --- a/codex-rs/tui-pty-e2e/tests/snapshots/streaming__submit_input.snap +++ b/codex-rs/tui-pty-e2e/tests/snapshots/streaming__submit_input.snap @@ -1,22 +1,7 @@ --- source: tui-pty-e2e/tests/streaming.rs -expression: normalize_for_snapshot(session.screen_contents()) +expression: normalize_for_input_snapshot(session.screen_contents()) --- -│ >_ OpenAI Codex (v0.0.0) │ -│ │ -│ model: mock-model /model to change │ -│ directory: [TMP_DIR] │ -╰──────────────────────────────────────────╯ - - To get started, describe a task or try one of these commands: - - /init - create an AGENTS.md file with instructions for Codex - /status - show current session configuration - /approvals - choose what Codex can do without approval - /model - choose what model and reasoning effort to use - /review - review any changes and find issues - - › testing!!! diff --git a/codex-rs/tui-pty-e2e/tests/startup.rs b/codex-rs/tui-pty-e2e/tests/startup.rs index 7db1b8e5f..c69145123 100644 --- a/codex-rs/tui-pty-e2e/tests/startup.rs +++ b/codex-rs/tui-pty-e2e/tests/startup.rs @@ -1,7 +1,7 @@ use insta::assert_snapshot; use std::time::Duration; use std::time::Instant; -use tui_pty_e2e::normalize_for_snapshot; +use tui_pty_e2e::normalize_for_input_snapshot; use tui_pty_e2e::SessionConfig; use tui_pty_e2e::TuiSession; use tui_pty_e2e::TIMEOUT; @@ -29,7 +29,7 @@ fn test_startup_shows_banner() { assert!(contents.contains("Welcome to Codex")); assert_snapshot!( "startup_shows_welcome", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -55,7 +55,7 @@ fn test_startup_welcome_with_dimensions() { assert!(contents.lines().count() <= 40); assert_snapshot!( "startup_welcome_dimensions_40x120", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -93,7 +93,7 @@ fn test_runs_in_temp_directory_by_default() { ); assert_snapshot!( "runs_in_temp_directory", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -124,7 +124,7 @@ fn test_trust_screen_is_skipped_with_default_config() { ); assert_snapshot!( "trust_screen_skipped", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } diff --git a/codex-rs/tui-pty-e2e/tests/streaming.rs b/codex-rs/tui-pty-e2e/tests/streaming.rs index b434f1bfd..e66f3cf48 100644 --- a/codex-rs/tui-pty-e2e/tests/streaming.rs +++ b/codex-rs/tui-pty-e2e/tests/streaming.rs @@ -1,5 +1,5 @@ use insta::assert_snapshot; -use tui_pty_e2e::normalize_for_snapshot; +use tui_pty_e2e::normalize_for_input_snapshot; use tui_pty_e2e::Key; use tui_pty_e2e::SessionConfig; use tui_pty_e2e::TuiSession; @@ -28,7 +28,7 @@ fn test_submit_text() { assert_snapshot!( "submit_input", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ); } @@ -75,6 +75,6 @@ fn test_escape_cancels_streaming() { assert_snapshot!( "cancelled_stream", - normalize_for_snapshot(session.screen_contents()) + normalize_for_input_snapshot(session.screen_contents()) ) } diff --git a/codex-rs/tui/config.toml b/codex-rs/tui/config.toml deleted file mode 100644 index f4ce84d1b..000000000 --- a/codex-rs/tui/config.toml +++ /dev/null @@ -1,5 +0,0 @@ -[projects."/home/clifford/Documents/source/codex"] -trust_level = "untrusted" - -[projects."/home/clifford/Documents/source/nori/cli"] -trust_level = "untrusted" diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 4bd4b4aaa..9f996007f 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -21,7 +21,6 @@ use codex_common::model_presets::ModelUpgrade; use codex_common::model_presets::all_model_presets; use codex_core::AuthManager; use codex_core::ConversationManager; -use codex_core::GEMINI_ACP_PROVIDER_ID; use codex_core::config::Config; use codex_core::config::edit::ConfigEditsBuilder; use codex_core::model_family::find_family_for_model; @@ -505,13 +504,6 @@ impl App { } AppEvent::UpdateModel(model) => { self.chat_widget.set_model(&model); - - if model.starts_with("gemini") { - self.config.model_provider_id = GEMINI_ACP_PROVIDER_ID.to_string(); - } else if self.config.model_provider_id == GEMINI_ACP_PROVIDER_ID { - self.config.model_provider_id = "openai".to_string(); - } - self.config.model = model.clone(); if let Some(family) = find_family_for_model(&model) { self.config.model_family = family; diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 558d543ee..cdcc78aba 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -774,24 +774,6 @@ impl ChatWidget { ))); } - fn on_acp_event(&mut self, ev: codex_core::protocol::AcpEventMsg) { - use codex_core::protocol::AcpEventMsg; - match ev { - AcpEventMsg::ToolCall(tool_call) => { - // Display on initial tool call (Pending) when we have the title. - // Updates may not include the title, so we show it early. - if matches!( - tool_call.status, - codex_core::protocol::AcpToolCallStatus::Pending - ) && !tool_call.title.is_empty() - { - self.flush_answer_stream_with_separator(); - self.add_to_history(history_cell::new_acp_tool_call(tool_call.title)); - } - } - } - } - fn on_get_history_entry_response( &mut self, event: codex_core::protocol::GetHistoryEntryResponseEvent, @@ -1700,7 +1682,6 @@ impl ChatWidget { self.on_entered_review_mode(review_request) } EventMsg::ExitedReviewMode(review) => self.on_exited_review_mode(review), - EventMsg::Acp(acp_event) => self.on_acp_event(acp_event), EventMsg::RawResponseItem(_) | EventMsg::ItemStarted(_) | EventMsg::ItemCompleted(_) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 1b2b45ba5..b026170f7 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -977,12 +977,6 @@ pub(crate) fn new_web_search_call(query: String) -> PlainHistoryCell { PlainHistoryCell { lines } } -/// Create a history cell for an ACP (Agent Client Protocol) tool call. -pub(crate) fn new_acp_tool_call(title: String) -> PlainHistoryCell { - let lines: Vec> = vec![Line::from(vec![padded_emoji("⚙").into(), title.into()])]; - PlainHistoryCell { lines } -} - /// If the first content is an image, return a new cell with the image. /// TODO(rgwood-dd): Handle images properly even if they're not the first result. fn try_new_completed_mcp_tool_call_with_image_output(