fix(tui-e2e): Make E2E snapshot tests more resilient to scroll timing

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"

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

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;
 

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<String>,
-        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<u32>,
-}
-
-/// 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<AcpToolCallContent>,
-    pub locations: Vec<AcpToolCallLocation>,
-    pub raw_input: Option<serde_json::Value>,
-    pub raw_output: Option<serde_json::Value>,
-}
-
-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<String>,
-    pub kind: Option<AcpToolKind>,
-    pub status: Option<AcpToolStatus>,
-    pub content: Option<Vec<AcpToolCallContent>>,
-    pub locations: Option<Vec<AcpToolCallLocation>>,
-    pub raw_input: Option<serde_json::Value>,
-    pub raw_output: Option<serde_json::Value>,
-}
-
-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<AcpToolCallContent> {
-    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<TranslatedEve
                 }
             }
         }
-        acp::SessionUpdate::ToolCall(tool_call) => {
-            // 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,21 +180,18 @@ fn extract_command_from_tool_call(tool_call: &acp::ToolCallUpdate) -> Vec<String
 
     // Add stringified raw_input if present
     if let Some(input) = &tool_call.fields.raw_input
-        && let Ok(args_str) = serde_json::to_string(input) {
-            cmd.push(args_str);
-        }
+        && let Ok(args_str) = serde_json::to_string(input)
+    {
+        cmd.push(args_str);
+    }
 
     cmd
 }
 
 /// Extract a human-readable reason from the tool call.
 fn extract_reason_from_tool_call(tool_call: &acp::ToolCallUpdate) -> Option<String> {
     // 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}"))
 }
 

codex-rs/core/Cargo.toml

@@ -12,15 +12,13 @@ path = "src/lib.rs"
 workspace = true
 
 [dependencies]
-agent-client-protocol = "0.7"
 anyhow = { workspace = true }
 askama = { workspace = true }
 async-channel = { workspace = true }
 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 }