|
| 1 | +# ACP Integration: Critical Design Decisions |
| 2 | + |
| 3 | +## Core Principle: Minimal Footprint |
| 4 | + |
| 5 | +The ACP implementation is designed to **minimize changes to codex-core** to ease the burden of accepting upstream changes. This drives all other decisions. |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## Architecture Decisions |
| 10 | + |
| 11 | +### 1. Parallel Crate Structure (Not Trait Abstraction) |
| 12 | + |
| 13 | +**Decision:** `codex-acp` is a parallel crate to `codex-core`, NOT integrated via a shared trait. |
| 14 | + |
| 15 | +**Rejected Alternative:** An `AgentBackend` trait that would unify HTTP and ACP: |
| 16 | +```rust |
| 17 | +// REJECTED - pollutes core with ACP concerns |
| 18 | +trait AgentBackend { |
| 19 | + fn owns_tool_execution(&self) -> bool; // ACP-specific branching |
| 20 | + fn supports_session_resume(&self) -> bool; // ACP capability check |
| 21 | + fn permission_requests(&self) -> Receiver; // ACP protocol detail |
| 22 | +} |
| 23 | +``` |
| 24 | + |
| 25 | +**Why:** Every method except `execute_turn()` would exist to handle ACP's differences, causing: |
| 26 | +- Core logic branches on `owns_tool_execution()` |
| 27 | +- Testing must cover both paths |
| 28 | +- Upstream changes risk breaking ACP paths |
| 29 | + |
| 30 | +**Result:** Zero changes to codex-core. ACP is self-contained. |
| 31 | + |
| 32 | +--- |
| 33 | + |
| 34 | +### 2. Mode Selection: Config-Only at Startup |
| 35 | + |
| 36 | +**Decision:** ACP vs HTTP mode is determined at startup via configuration. No mid-session switching. |
| 37 | + |
| 38 | +**Implications:** |
| 39 | +- Model picker stays HTTP-only |
| 40 | +- No changes to `Op::OverrideTurnContext` |
| 41 | +- TUI branches once at startup, not per-turn |
| 42 | + |
| 43 | +**How core calling code changes:** |
| 44 | +```rust |
| 45 | +// In TUI/CLI main(): |
| 46 | +if config.acp_agent.is_some() { |
| 47 | + // ACP mode: use codex-acp directly |
| 48 | + let connection = AcpConnection::spawn(config).await; |
| 49 | + run_acp_event_loop(connection); |
| 50 | +} else { |
| 51 | + // HTTP mode: use codex-core (unchanged) |
| 52 | + let codex = Codex::spawn(config); |
| 53 | + run_event_loop(codex.events()); |
| 54 | +} |
| 55 | +``` |
| 56 | + |
| 57 | +--- |
| 58 | + |
| 59 | +### 3. History Ownership: ACP Owns It |
| 60 | + |
| 61 | +**Decision:** ACP agents fully manage their own history. Codex does not persist or convert ACP conversations. |
| 62 | + |
| 63 | +**Implications:** |
| 64 | +- Zero history conversion code in core |
| 65 | +- If user switches backends, history doesn't transfer (by design) |
| 66 | +- ACP agents may have proprietary context handling |
| 67 | + |
| 68 | +**Future Placeholders Added:** |
| 69 | +- `connection.rs:343-350` - Resume/fork integration point |
| 70 | +- `connection.rs:385-394` - Codex-format history persistence |
| 71 | +- `connection.rs:220-234` - History export for backend handoff |
| 72 | + |
| 73 | +--- |
| 74 | + |
| 75 | +### 4. Approval Bridging: Codex Intercedes via Event Translation |
| 76 | + |
| 77 | +**Decision:** ACP permission requests flow through Codex's approval UI via channel-based event translation. |
| 78 | + |
| 79 | +**Flow:** |
| 80 | +``` |
| 81 | +ACP Worker Thread Main Thread (TUI) |
| 82 | + │ │ |
| 83 | + │ ApprovalRequest │ |
| 84 | + │ ──────────────────────────────────►│ |
| 85 | + │ (ExecApprovalRequestEvent + │ Display approval UI |
| 86 | + │ options + oneshot::Sender) │ Get user decision |
| 87 | + │ │ |
| 88 | + │ ReviewDecision │ |
| 89 | + │ ◄──────────────────────────────────│ |
| 90 | + │ (via oneshot channel) │ |
| 91 | + │ │ |
| 92 | + ▼ ▼ |
| 93 | + Translate to ACP outcome Continue processing |
| 94 | +``` |
| 95 | + |
| 96 | +**Key Types:** |
| 97 | +```rust |
| 98 | +pub struct ApprovalRequest { |
| 99 | + pub event: ExecApprovalRequestEvent, // Codex format |
| 100 | + pub options: Vec<acp::PermissionOption>, // For response translation |
| 101 | + pub response_tx: oneshot::Sender<ReviewDecision>, |
| 102 | +} |
| 103 | +``` |
| 104 | + |
| 105 | +**How TUI integrates:** |
| 106 | +```rust |
| 107 | +let mut connection = AcpConnection::spawn(config).await?; |
| 108 | +let approval_rx = connection.take_approval_receiver(); |
| 109 | + |
| 110 | +// In event loop: |
| 111 | +tokio::select! { |
| 112 | + Some(approval) = approval_rx.recv() => { |
| 113 | + // Show approval UI |
| 114 | + let decision = show_approval_popup(approval.event).await; |
| 115 | + // Send decision back |
| 116 | + let _ = approval.response_tx.send(decision); |
| 117 | + } |
| 118 | + // ... other events |
| 119 | +} |
| 120 | +``` |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +### 5. Translation Layer: Bi-directional Type Conversion |
| 125 | + |
| 126 | +**Decision:** `translator.rs` handles all ACP ↔ Codex type conversions. |
| 127 | + |
| 128 | +**Functions:** |
| 129 | +| Function | Direction | |
| 130 | +|----------|-----------| |
| 131 | +| `permission_request_to_approval_event()` | ACP → Codex | |
| 132 | +| `review_decision_to_permission_outcome()` | Codex → ACP | |
| 133 | +| `response_items_to_content_blocks()` | Codex → ACP | |
| 134 | +| `translate_session_update()` | ACP → Codex | |
| 135 | + |
| 136 | +**Approval Translation Logic:** |
| 137 | +- `Approved`/`ApprovedForSession` → Find `AllowOnce`/`AllowAlways` option |
| 138 | +- `Denied`/`Abort` → Find `RejectOnce`/`RejectAlways` option |
| 139 | +- Fallback: text matching ("allow", "approve", "yes" vs "deny", "reject", "no") |
| 140 | +- Last resort: first option for approve, last for deny |
| 141 | + |
| 142 | +--- |
| 143 | + |
| 144 | +## How Core Module Calling Code Must Change |
| 145 | + |
| 146 | +### Current State (HTTP-only) |
| 147 | +```rust |
| 148 | +// codex-tui/src/main.rs (simplified) |
| 149 | +let codex = Codex::spawn(config); |
| 150 | +let events = codex.events(); |
| 151 | +run_event_loop(events); |
| 152 | +``` |
| 153 | + |
| 154 | +### Required Changes |
| 155 | + |
| 156 | +1. **Add ACP config detection:** |
| 157 | +```rust |
| 158 | +// Add to config parsing |
| 159 | +struct Config { |
| 160 | + // existing fields... |
| 161 | + acp_agent: Option<AcpAgentConfig>, // NEW |
| 162 | +} |
| 163 | +``` |
| 164 | + |
| 165 | +2. **Branch at startup:** |
| 166 | +```rust |
| 167 | +async fn main() { |
| 168 | + let config = load_config(); |
| 169 | + |
| 170 | + if let Some(acp_config) = &config.acp_agent { |
| 171 | + run_acp_mode(acp_config, &config).await; |
| 172 | + } else { |
| 173 | + run_http_mode(&config).await; // Existing code path |
| 174 | + } |
| 175 | +} |
| 176 | +``` |
| 177 | + |
| 178 | +3. **ACP mode implementation:** |
| 179 | +```rust |
| 180 | +async fn run_acp_mode(acp_config: &AcpAgentConfig, config: &Config) { |
| 181 | + // Spawn ACP connection |
| 182 | + let mut connection = AcpConnection::spawn(acp_config, &config.cwd).await?; |
| 183 | + |
| 184 | + // Take approval receiver for UI integration |
| 185 | + let approval_rx = connection.take_approval_receiver(); |
| 186 | + |
| 187 | + // Create session |
| 188 | + let session_id = connection.create_session(&config.cwd).await?; |
| 189 | + |
| 190 | + // Event loop with approval handling |
| 191 | + loop { |
| 192 | + tokio::select! { |
| 193 | + Some(approval) = approval_rx.recv() => { |
| 194 | + handle_approval_request(approval); |
| 195 | + } |
| 196 | + user_input = get_user_input() => { |
| 197 | + let (update_tx, mut update_rx) = mpsc::channel(32); |
| 198 | + connection.prompt(&session_id, prompt, update_tx).await?; |
| 199 | + |
| 200 | + while let Some(update) = update_rx.recv().await { |
| 201 | + let events = translator::translate_session_update(update); |
| 202 | + for event in events { |
| 203 | + display_event(event); |
| 204 | + } |
| 205 | + } |
| 206 | + } |
| 207 | + } |
| 208 | + } |
| 209 | +} |
| 210 | +``` |
| 211 | + |
| 212 | +4. **HTTP mode unchanged:** |
| 213 | +```rust |
| 214 | +async fn run_http_mode(config: &Config) { |
| 215 | + // Existing codex-core code path - UNCHANGED |
| 216 | + let codex = Codex::spawn(config); |
| 217 | + run_event_loop(codex.events()); |
| 218 | +} |
| 219 | +``` |
| 220 | + |
| 221 | +--- |
| 222 | + |
| 223 | +## Files Modified/Created |
| 224 | + |
| 225 | +| File | Change | |
| 226 | +|------|--------| |
| 227 | +| `codex-rs/acp/src/connection.rs` | Added `ApprovalRequest`, approval channel, placeholders | |
| 228 | +| `codex-rs/acp/src/translator.rs` | Added approval translation functions | |
| 229 | +| `codex-rs/acp/src/lib.rs` | Exported `ApprovalRequest` | |
| 230 | +| `codex-rs/acp/docs.md` | Updated documentation | |
| 231 | + |
| 232 | +## Files NOT Modified |
| 233 | + |
| 234 | +| File | Why | |
| 235 | +|------|-----| |
| 236 | +| `codex-rs/core/*` | **Zero changes** - minimal footprint principle | |
| 237 | +| `codex-rs/tui/*` | TUI integration is separate task | |
| 238 | + |
| 239 | +--- |
| 240 | + |
| 241 | +## Summary |
| 242 | + |
| 243 | +The ACP integration follows a **parallel architecture** where: |
| 244 | +- `codex-core` remains unchanged (upstream compatibility) |
| 245 | +- `codex-acp` is self-contained with its own session management |
| 246 | +- TUI/CLI chooses mode at startup based on config |
| 247 | +- Approval bridging is the single integration point, using channel-based event translation |
| 248 | +- History/resume features are stubbed with clear placeholder comments for future work |
0 commit comments