From 5444890098b5a7b383fe3c03971311db0c469da0 Mon Sep 17 00:00:00 2001 From: Amol Kapoor Date: Sun, 18 Jan 2026 18:53:50 -0500 Subject: [PATCH] fix(acp): Enable dynamic approval policy updates via /approvals command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /approvals command was not working in ACP mode because: - run_approval_handler captured approval_policy by value at spawn time - Op::OverrideTurnContext was ignored in the ACP submit() method This fix uses a tokio::sync::watch channel to broadcast policy changes from AcpBackend::submit() to the long-running approval handler task, enabling mid-session policy changes (e.g., switching to full access mode). Changes: - Add approval_policy_tx watch::Sender field to AcpBackend - Create watch channel in spawn() and pass receiver to handler - Handle Op::OverrideTurnContext to update policy via watch channel - Update run_approval_handler to read current policy on each request 🤖 Generated with [Nori](https://nori.ai) Co-Authored-By: Nori --- codex-rs/acp/docs.md | 20 +++++ codex-rs/acp/src/backend.rs | 145 ++++++++++++++++++++++++++++++++++-- 2 files changed, 159 insertions(+), 6 deletions(-) diff --git a/codex-rs/acp/docs.md b/codex-rs/acp/docs.md index e1ff7f7a..93659a7d 100644 --- a/codex-rs/acp/docs.md +++ b/codex-rs/acp/docs.md @@ -391,6 +391,25 @@ When `approval_policy == AskForApproval::Never` (set via `--yolo` or `--dangerou CLI --yolo flag → AskForApproval::Never → AcpBackendConfig → run_approval_handler() → auto-approve ``` +**Dynamic Approval Policy Updates:** + +The approval policy can be changed mid-session via the `/approvals` command (which sends `Op::OverrideTurnContext`). The ACP backend uses a `tokio::sync::watch` channel to broadcast policy changes to the long-running approval handler: + +``` +┌─────────────────────────┐ ┌─────────────────────────┐ +│ AcpBackend::submit() │ watch::Sender │ run_approval_handler │ +│ │ ─────────────────► │ │ +│ Op::OverrideTurnContext │ watch::Receiver │ +│ { approval_policy } │ │ *rx.borrow() on each │ +│ │ │ request │ +└─────────────────────────┘ └─────────────────────────┘ +``` + +- `approval_policy_tx: watch::Sender` field in `AcpBackend` broadcasts updates +- `approval_policy_rx: watch::Receiver` passed to `run_approval_handler()` at spawn +- Handler reads `*approval_policy_rx.borrow()` on each incoming request to get the current policy +- Pattern enables immediate policy changes without restarting the approval handler task + For all other policies, approval requests are handled **immediately** (not deferred) to avoid deadlocks: ``` @@ -465,6 +484,7 @@ The `AcpBackend` provides a TUI-compatible interface that wraps `AcpConnection`: - `Op::ExecApproval`/`PatchApproval` → Resolves pending approval - `Op::AddToHistory` → Appends to history file (async background task) - `Op::GetHistoryEntryRequest` → Looks up history entry and sends response event + - `Op::OverrideTurnContext` → Updates approval policy via watch channel (enables `/approvals` command) - Unsupported ops → Error event sent to TUI - `AcpBackend::model_state()`: Returns current model state (available models and current selection) - `AcpBackend::set_model()` [unstable]: Delegates to `AcpConnection::set_model()` for model switching diff --git a/codex-rs/acp/src/backend.rs b/codex-rs/acp/src/backend.rs index 74d12ade..09c4e2be 100644 --- a/codex-rs/acp/src/backend.rs +++ b/codex-rs/acp/src/backend.rs @@ -26,6 +26,7 @@ use codex_protocol::protocol::TurnAbortedEvent; use codex_protocol::user_input::UserInput; use tokio::sync::Mutex; use tokio::sync::mpsc; +use tokio::sync::watch; use tracing::debug; use tracing::warn; @@ -174,6 +175,8 @@ pub struct AcpBackend { history_persistence: crate::config::HistoryPersistence, /// Conversation ID for this session (used for history entries) conversation_id: ConversationId, + /// Sender for broadcasting approval policy updates to the handler + approval_policy_tx: watch::Sender, } impl AcpBackend { @@ -258,6 +261,9 @@ impl AcpBackend { let idle_timer_abort = Arc::new(Mutex::new(None)); + // Create watch channel for dynamic approval policy updates + let (approval_policy_tx, approval_policy_rx) = watch::channel(config.approval_policy); + // Create conversation ID for this session let conversation_id = ConversationId::new(); @@ -276,6 +282,7 @@ impl AcpBackend { nori_home: config.nori_home.clone(), history_persistence: config.history_persistence, conversation_id, + approval_policy_tx, }; // Send synthetic SessionConfigured event @@ -308,7 +315,7 @@ impl AcpBackend { Arc::clone(&pending_approvals), Arc::clone(&user_notifier), cwd.clone(), - config.approval_policy, + approval_policy_rx, )); Ok(backend) @@ -437,10 +444,18 @@ impl AcpBackend { )) .await; } + Op::OverrideTurnContext { + approval_policy, .. + } => { + // Update approval policy if provided + if let Some(policy) = approval_policy { + debug!("Updating approval policy to {policy:?} in ACP mode"); + // Send the new policy to the approval handler via watch channel + let _ = self.approval_policy_tx.send(policy); + } + } // These ops are internal/context-related, silently ignore - Op::UserTurn { .. } - | Op::OverrideTurnContext { .. } - | Op::ResolveElicitation { .. } => { + Op::UserTurn { .. } | Op::ResolveElicitation { .. } => { debug!("Ignoring internal Op in ACP mode: {}", get_op_name(&op)); } // Catch any new Op variants we haven't handled - only show error in debug builds @@ -729,11 +744,14 @@ impl AcpBackend { pending_approvals: Arc>>, user_notifier: Arc, cwd: PathBuf, - approval_policy: AskForApproval, + approval_policy_rx: watch::Receiver, ) { while let Some(request) = approval_rx.recv().await { + // Check current approval policy (may have changed via OverrideTurnContext) + let current_policy = *approval_policy_rx.borrow(); + // If approval_policy is Never (yolo mode), auto-approve immediately - if approval_policy == AskForApproval::Never { + if current_policy == AskForApproval::Never { debug!( target: "acp_event_flow", call_id = %request.event.call_id(), @@ -2312,4 +2330,119 @@ mod tests { "Error message should mention authentication or provide login instructions, got: {error_message}" ); } + + /// Test that updating the approval policy via watch channel dynamically changes + /// the approval handler's behavior. This verifies that `/approvals` command + /// selecting "full access" makes it equivalent to `--yolo`. + #[tokio::test] + async fn test_approval_policy_dynamic_update() { + use codex_protocol::approvals::ExecApprovalRequestEvent; + use tokio::sync::oneshot; + use tokio::sync::watch; + + // Create channels for the test + let (approval_tx, approval_rx) = mpsc::channel::(16); + let (event_tx, mut event_rx) = mpsc::channel::(16); + let pending_approvals = Arc::new(Mutex::new(Vec::::new())); + let user_notifier = Arc::new(codex_core::UserNotifier::new(None, false)); + let cwd = PathBuf::from("/tmp/test"); + + // Create watch channel starting with OnRequest policy (requires approval) + let (policy_tx, policy_rx) = watch::channel(AskForApproval::OnRequest); + + // Spawn the approval handler with the watch receiver + tokio::spawn(AcpBackend::run_approval_handler( + approval_rx, + event_tx.clone(), + Arc::clone(&pending_approvals), + Arc::clone(&user_notifier), + cwd.clone(), + policy_rx, + )); + + // Create a mock approval request + let (response_tx1, mut response_rx1) = oneshot::channel(); + let request1 = ApprovalRequest { + event: ApprovalEventType::Exec(ExecApprovalRequestEvent { + call_id: "call-1".to_string(), + turn_id: String::new(), + command: vec!["ls".to_string()], + cwd: cwd.clone(), + reason: None, + risk: None, + parsed_cmd: vec![], + }), + options: vec![], + response_tx: response_tx1, + }; + + // Send first request - should be forwarded to TUI (not auto-approved) + approval_tx.send(request1).await.unwrap(); + + // Give the handler time to process + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + + // Should have received an approval request event in the TUI + let event = event_rx.try_recv(); + assert!( + event.is_ok(), + "Should have received approval request event for OnRequest policy" + ); + if let Ok(Event { + msg: EventMsg::ExecApprovalRequest(req), + .. + }) = event + { + assert_eq!(req.call_id, "call-1"); + } else { + panic!("Expected ExecApprovalRequest event"); + } + + // The request should be pending (not auto-approved) + assert!( + response_rx1.try_recv().is_err(), + "Request should not be auto-approved with OnRequest policy" + ); + + // Now update the policy to Never (yolo mode) + policy_tx.send(AskForApproval::Never).unwrap(); + + // Give the handler time to see the policy change + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + + // Send second request - should be auto-approved + let (response_tx2, mut response_rx2) = oneshot::channel(); + let request2 = ApprovalRequest { + event: ApprovalEventType::Exec(ExecApprovalRequestEvent { + call_id: "call-2".to_string(), + turn_id: String::new(), + command: vec!["echo".to_string(), "hello".to_string()], + cwd: cwd.clone(), + reason: None, + risk: None, + parsed_cmd: vec![], + }), + options: vec![], + response_tx: response_tx2, + }; + + approval_tx.send(request2).await.unwrap(); + + // Give the handler time to process + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + + // Should NOT have received another approval request event (auto-approved) + let event2 = event_rx.try_recv(); + assert!( + event2.is_err(), + "Should NOT receive approval request event when policy is Never (yolo mode)" + ); + + // The request should have been auto-approved + let decision = response_rx2.try_recv(); + assert!( + matches!(decision, Ok(ReviewDecision::Approved)), + "Request should be auto-approved with Never policy, got: {decision:?}" + ); + } }