Skip to content

Commit 1fb6cfc

Browse files
committed
feat(acp): Add approval bridging infrastructure
Implements the minimal-footprint approval bridging between ACP permission requests and Codex approval system: - Add ApprovalRequest type bundling translated event with response channel - Translate ACP RequestPermissionRequest to Codex ExecApprovalRequestEvent - Translate Codex ReviewDecision back to ACP RequestPermissionOutcome - Expose approval receiver via AcpConnection::take_approval_receiver() - Fall back to auto-approve if channel closed, deny if response dropped - Add placeholder comments for future: history persistence, export, resume This enables the TUI to receive approval requests and respond to them without modifying codex-core.
1 parent 96cafb4 commit 1fb6cfc

4 files changed

Lines changed: 411 additions & 16 deletions

File tree

codex-rs/acp/docs.md

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,29 @@ The ACP library uses `LocalBoxFuture` which is `!Send`, preventing direct use in
9292

9393
- Implements `acp::Client` trait to handle agent requests
9494
- Routes session updates to registered `mpsc::Sender<SessionUpdate>` channels
95-
- Auto-approves permission requests (TODO: bridge to codex approval system)
95+
- Bridges permission requests to Codex approval system via `ApprovalRequest` channel
9696
- Implements file read (synchronous `std::fs::read_to_string`)
9797
- Terminal operations return `method_not_found` (not yet supported)
9898

99+
**Approval Bridging:**
100+
101+
The ACP module bridges permission requests to Codex's approval UI:
102+
103+
```
104+
┌─────────────────────────┐ ApprovalRequest ┌─────────────────────────┐
105+
│ ACP Worker Thread │──────────────────────►│ Main Thread (TUI) │
106+
│ │ │ │
107+
│ ClientDelegate │ │ - Display approval UI │
108+
│ - request_permission()│◄──────────────────────│ - Get user decision │
109+
│ │ ReviewDecision │ - Send via oneshot │
110+
└─────────────────────────┘ (via oneshot) └─────────────────────────┘
111+
```
112+
113+
- `ApprovalRequest` bundles the translated `ExecApprovalRequestEvent`, original ACP options, and response channel
114+
- `AcpConnection::take_approval_receiver()` exposes the receiver for TUI consumption
115+
- Falls back to auto-approve if approval channel is closed (no UI listening)
116+
- Falls back to deny if response channel is dropped (UI didn't respond)
117+
99118
**Event Translation (`translator.rs`):**
100119

101120
Bridges between ACP types and codex-protocol types:
@@ -105,13 +124,24 @@ Bridges between ACP types and codex-protocol types:
105124
| `response_items_to_content_blocks()` | Converts codex `ResponseItem` to ACP `ContentBlock` for prompts |
106125
| `text_to_content_block()` | Simple text-to-ContentBlock conversion |
107126
| `translate_session_update()` | Translates ACP `SessionUpdate` to `TranslatedEvent` enum |
127+
| `permission_request_to_approval_event()` | Converts ACP `RequestPermissionRequest` to Codex `ExecApprovalRequestEvent` |
128+
| `review_decision_to_permission_outcome()` | Converts Codex `ReviewDecision` back to ACP `RequestPermissionOutcome` |
108129

109130
`TranslatedEvent` variants:
110131
- `TextDelta(String)` - Text content from `AgentMessageChunk` or `AgentThoughtChunk`
111132
- `Completed(StopReason)` - Session completion signal
112133

113134
Non-text content (images, audio, resources) and tool calls are currently dropped with empty vec.
114135

136+
**Approval Translation Details:**
137+
138+
The approval translation maps between Codex's binary approve/deny model and ACP's option-based model:
139+
140+
- `Approved`/`ApprovedForSession` → Finds option with `AllowOnce` or `AllowAlways` kind
141+
- `Denied`/`Abort` → Finds option with `RejectOnce` or `RejectAlways` kind
142+
- Falls back to text matching ("allow", "approve", "yes" vs "deny", "reject", "no") if kind-based matching fails
143+
- Last resort: first option for approve, last option for deny
144+
115145
### Things to Know
116146

117147
**Protocol Version Check:**
@@ -139,4 +169,23 @@ Client advertises these capabilities to agents:
139169
- `fs.write_text_file: true`
140170
- `terminal: false`
141171

172+
### Future Work
173+
174+
The following features are marked with TODO comments in the codebase:
175+
176+
**Resume/Fork Integration (connection.rs:343-350):**
177+
- Accept optional session_id parameter to resume existing sessions
178+
- Load persisted history from Codex rollout format
179+
- Send history to agent via session initialization
180+
181+
**Codex-format History Persistence (connection.rs:385-394):**
182+
- Collect all SessionUpdates during prompts
183+
- Convert to Codex ResponseItem format using translator functions
184+
- Write to rollout storage for session resume and history browsing
185+
186+
**History Export for Handoff (connection.rs:220-234):**
187+
- Export session history in Codex format
188+
- Enable switching from ACP mode to HTTP mode mid-session
189+
- Support replaying history through different backends
190+
142191
Created and maintained by Nori.

codex-rs/acp/src/connection.rs

Lines changed: 146 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ use std::thread;
1717
use agent_client_protocol as acp;
1818
use anyhow::Context;
1919
use anyhow::Result;
20+
use codex_protocol::approvals::ExecApprovalRequestEvent;
21+
use codex_protocol::protocol::ReviewDecision;
2022
use futures::AsyncBufReadExt;
2123
use futures::io::BufReader;
2224
use tokio::process::Child;
@@ -29,6 +31,22 @@ use tracing::debug;
2931
use tracing::warn;
3032

3133
use crate::registry::AcpAgentConfig;
34+
use crate::translator;
35+
36+
/// An approval request sent from the ACP layer to the UI layer.
37+
///
38+
/// When an ACP agent requests permission to perform an operation,
39+
/// this struct is sent to the UI layer which should display the request
40+
/// to the user and return their decision via the response channel.
41+
#[derive(Debug)]
42+
pub struct ApprovalRequest {
43+
/// The translated Codex approval event
44+
pub event: ExecApprovalRequestEvent,
45+
/// The original ACP permission options for translating the response
46+
pub options: Vec<acp::PermissionOption>,
47+
/// Channel to send the user's decision back
48+
pub response_tx: oneshot::Sender<ReviewDecision>,
49+
}
3250

3351
/// Minimum supported ACP protocol version
3452
const MINIMUM_SUPPORTED_VERSION: acp::ProtocolVersion = acp::V1;
@@ -59,6 +77,9 @@ enum AcpCommand {
5977
pub struct AcpConnection {
6078
command_tx: mpsc::Sender<AcpCommand>,
6179
agent_capabilities: acp::AgentCapabilities,
80+
/// Channel to receive approval requests from the agent.
81+
/// The UI layer should listen on this channel and respond via the oneshot sender.
82+
approval_rx: mpsc::Receiver<ApprovalRequest>,
6283
_worker_thread: thread::JoinHandle<()>,
6384
}
6485

@@ -82,6 +103,9 @@ impl AcpConnection {
82103
let (init_tx, init_rx) = oneshot::channel();
83104
let (command_tx, command_rx) = mpsc::channel::<AcpCommand>(32);
84105

106+
// Create approval channel - sender goes to worker, receiver stays here
107+
let (approval_tx, approval_rx) = mpsc::channel::<ApprovalRequest>(16);
108+
85109
// Spawn a dedicated thread with a single-threaded tokio runtime
86110
let worker_thread = thread::spawn(move || {
87111
#[expect(
@@ -97,7 +121,7 @@ impl AcpConnection {
97121
let local = tokio::task::LocalSet::new();
98122
local
99123
.run_until(async move {
100-
match spawn_connection_internal(&config, &cwd).await {
124+
match spawn_connection_internal(&config, &cwd, approval_tx).await {
101125
Ok((inner, capabilities)) => {
102126
let _ = init_tx.send(Ok(capabilities));
103127
run_command_loop(inner, command_rx).await;
@@ -119,6 +143,7 @@ impl AcpConnection {
119143
Ok(Self {
120144
command_tx,
121145
agent_capabilities: capabilities,
146+
approval_rx,
122147
_worker_thread: worker_thread,
123148
})
124149
}
@@ -176,6 +201,37 @@ impl AcpConnection {
176201
pub fn capabilities(&self) -> &acp::AgentCapabilities {
177202
&self.agent_capabilities
178203
}
204+
205+
/// Take ownership of the approval request receiver.
206+
///
207+
/// This should be called once by the UI layer to receive approval requests.
208+
/// When an ACP agent requests permission, an `ApprovalRequest` will be sent
209+
/// through this channel. The UI should:
210+
/// 1. Display the request to the user (using `ApprovalRequest::event`)
211+
/// 2. Get the user's decision
212+
/// 3. Send the decision back via `ApprovalRequest::response_tx`
213+
///
214+
/// # Panics
215+
/// This method can only be called once. Calling it again will panic.
216+
pub fn take_approval_receiver(&mut self) -> mpsc::Receiver<ApprovalRequest> {
217+
std::mem::replace(&mut self.approval_rx, mpsc::channel(1).1)
218+
}
219+
220+
// TODO: [Future] History Export for Handoff
221+
// Add a method to export session history in Codex format for handoff to HTTP mode:
222+
//
223+
// ```rust
224+
// pub async fn export_history(&self, session_id: &SessionId) -> Result<Vec<ResponseItem>> {
225+
// // 1. Retrieve accumulated history from ACP agent (if supported)
226+
// // 2. Convert ACP format to Codex ResponseItem format
227+
// // 3. Return for use in HTTP mode continuation
228+
// }
229+
// ```
230+
//
231+
// This would enable:
232+
// - Switching from ACP mode to HTTP mode mid-session
233+
// - Continuing a conversation started with one backend using another
234+
// - Debugging by replaying history through a different backend
179235
}
180236

181237
/// Internal connection state that lives on the worker thread.
@@ -194,6 +250,7 @@ struct AcpConnectionInner {
194250
async fn spawn_connection_internal(
195251
config: &AcpAgentConfig,
196252
cwd: &Path,
253+
approval_tx: mpsc::Sender<ApprovalRequest>,
197254
) -> Result<(AcpConnectionInner, acp::AgentCapabilities)> {
198255
debug!(
199256
"Spawning ACP agent: {} {:?} in {}",
@@ -231,7 +288,7 @@ async fn spawn_connection_internal(
231288
});
232289

233290
// Create client delegate for handling agent requests
234-
let client_delegate = Rc::new(ClientDelegate::new());
291+
let client_delegate = Rc::new(ClientDelegate::new(cwd.to_path_buf(), approval_tx));
235292

236293
// Establish JSON-RPC connection
237294
let (connection, io_task) = acp::ClientSideConnection::new(
@@ -300,6 +357,14 @@ async fn run_command_loop(inner: AcpConnectionInner, mut command_rx: mpsc::Recei
300357
while let Some(cmd) = command_rx.recv().await {
301358
match cmd {
302359
AcpCommand::CreateSession { cwd, response_tx } => {
360+
// TODO: [Future] Resume/Fork Integration
361+
// When creating a session, check if there's an existing session to resume.
362+
// This would require:
363+
// 1. Accepting an optional session_id parameter to resume
364+
// 2. Loading persisted history from Codex rollout format
365+
// 3. Sending history to the agent via the session initialization
366+
// See: codex-core/src/rollout.rs for the persistence format
367+
303368
let result = inner
304369
.connection
305370
.new_session(acp::NewSessionRequest {
@@ -333,6 +398,17 @@ async fn run_command_loop(inner: AcpConnectionInner, mut command_rx: mpsc::Recei
333398
.map(|r| r.stop_reason)
334399
.context("ACP prompt failed");
335400

401+
// TODO: [Future] Codex-format History Persistence
402+
// After a successful prompt, persist the conversation history in Codex's rollout
403+
// format. This would enable:
404+
// 1. Session resume after restart
405+
// 2. History browsing in the TUI
406+
// 3. Conversation forking
407+
// Implementation would involve:
408+
// - Collecting all SessionUpdates received during the prompt
409+
// - Converting them to Codex ResponseItem format using translator functions
410+
// - Writing to rollout storage (see codex-core/src/rollout.rs)
411+
336412
inner.client_delegate.unregister_session(&session_id);
337413
let _ = response_tx.send(result);
338414
}
@@ -363,12 +439,18 @@ async fn run_command_loop(inner: AcpConnectionInner, mut command_rx: mpsc::Recei
363439
/// - Terminal operations (stubbed)
364440
pub struct ClientDelegate {
365441
sessions: RefCell<HashMap<acp::SessionId, mpsc::Sender<acp::SessionUpdate>>>,
442+
/// Working directory for approval events
443+
cwd: PathBuf,
444+
/// Channel to send approval requests to the UI layer
445+
approval_tx: mpsc::Sender<ApprovalRequest>,
366446
}
367447

368448
impl ClientDelegate {
369-
fn new() -> Self {
449+
fn new(cwd: PathBuf, approval_tx: mpsc::Sender<ApprovalRequest>) -> Self {
370450
Self {
371451
sessions: RefCell::new(HashMap::new()),
452+
cwd,
453+
approval_tx,
372454
}
373455
}
374456

@@ -387,18 +469,67 @@ impl acp::Client for ClientDelegate {
387469
&self,
388470
arguments: acp::RequestPermissionRequest,
389471
) -> acp::Result<acp::RequestPermissionResponse> {
390-
// For now, auto-approve all requests by selecting the first option
391-
// TODO: Bridge to codex approval system
392-
let option_id = arguments
393-
.options
394-
.first()
395-
.map(|opt| opt.id.clone())
396-
.unwrap_or_else(|| acp::PermissionOptionId::from("allow".to_string()));
397-
398-
Ok(acp::RequestPermissionResponse {
399-
outcome: acp::RequestPermissionOutcome::Selected { option_id },
400-
meta: None,
401-
})
472+
// Translate ACP permission request to Codex approval event
473+
let event = translator::permission_request_to_approval_event(&arguments, &self.cwd);
474+
475+
// Create a response channel for the UI to send the decision
476+
let (response_tx, response_rx) = oneshot::channel();
477+
478+
// Send the approval request to the UI layer
479+
let approval_request = ApprovalRequest {
480+
event,
481+
options: arguments.options.clone(),
482+
response_tx,
483+
};
484+
485+
if self.approval_tx.send(approval_request).await.is_err() {
486+
// If the receiver is dropped (UI not listening), fall back to auto-approve
487+
warn!("Approval channel closed, auto-approving permission request");
488+
let option_id = arguments
489+
.options
490+
.first()
491+
.map(|opt| opt.id.clone())
492+
.unwrap_or_else(|| acp::PermissionOptionId::from("allow".to_string()));
493+
494+
return Ok(acp::RequestPermissionResponse {
495+
outcome: acp::RequestPermissionOutcome::Selected { option_id },
496+
meta: None,
497+
});
498+
}
499+
500+
// Wait for the UI's decision
501+
match response_rx.await {
502+
Ok(decision) => {
503+
// Translate the Codex ReviewDecision back to ACP outcome
504+
let outcome =
505+
translator::review_decision_to_permission_outcome(decision, &arguments.options);
506+
Ok(acp::RequestPermissionResponse {
507+
outcome,
508+
meta: None,
509+
})
510+
}
511+
Err(_) => {
512+
// Response channel was dropped (UI didn't respond), fall back to deny
513+
warn!("Approval response channel dropped, denying permission request");
514+
let option_id = arguments
515+
.options
516+
.iter()
517+
.find(|opt| {
518+
matches!(
519+
opt.kind,
520+
acp::PermissionOptionKind::RejectOnce
521+
| acp::PermissionOptionKind::RejectAlways
522+
)
523+
})
524+
.map(|opt| opt.id.clone())
525+
.unwrap_or_else(|| acp::PermissionOptionId::from("deny".to_string()));
526+
527+
Ok(acp::RequestPermissionResponse {
528+
outcome: acp::RequestPermissionOutcome::Selected { option_id },
529+
meta: None,
530+
})
531+
}
532+
}
402533
}
403534

404535
async fn write_text_file(

codex-rs/acp/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ pub mod tracing_setup;
99
pub mod translator;
1010

1111
pub use connection::AcpConnection;
12+
pub use connection::ApprovalRequest;
1213
pub use registry::AcpAgentConfig;
1314
pub use registry::AcpProviderInfo;
1415
pub use registry::get_agent_config;

0 commit comments

Comments
 (0)