From b3b1b3beac17be2b015d2268d5ef5a0b05a564f4 Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 6 Aug 2026 18:05:21 -0400 Subject: [PATCH] fix(acp): address Thufir pass-1 CRITICAL and IMPORTANT findings (#4938) Harness (crates/buzz-acp/): - Remove legacy single-slot (pending_permission_id/permission_responded) from ask path; map is sole source of truth; Writing state drops stored option id - write_ndjson_no_observe: prevent duplicate generic+authorized telemetry on permission response paths - Deadline logic: select min(earliest pending deadline, hard deadline) when any Pending entries exist; suspend idle while pending; drain map on turn exit and cancel completion to prevent capacity leak across reused sessions - Pre-turn ask requests: force reject in non-turn reader (session/new path) so map entries can never be registered without a decision arm to resolve them - Admission preflight: measure annotated ObserverEvent size (raw + envelope overhead constant) not just raw msg; add OBSERVER_EVENT_ENVELOPE_MAX = 512 - permission_denial_response: malformed reject_once (missing/empty optionId) falls back to cancelled instead of returning Protocol error - ask+auto: change to compatible-with-warning; keep reject+auto hard error; auto is a model classifier not bypass mode (per adapter source review) - Dead state: Writing(String) -> Writing; is_permission_poisoned() removed; PermissionMode::is_default #[cfg(test)] - Tests: decision loop success, bad optionId idle-timeout, annotated-size preflight, malformed reject_once fallback, updated cancelled behavior tests Desktop (desktop/): - Thread channelId through PermissionDecisionButtons and sendPermissionDecision() - Key permission cards by nonce; fallback to turn-based key for legacy paths - control_result non-sent: set deliveryFailed on card; buttons re-enable via useEffect; add deliveryFailed field to TranscriptItem lifecycle type - Fleet-wide permission_policy: add to TS GlobalAgentConfig, EMPTY_GLOBAL_CONFIG, and AgentDefaultsEditor fleet defaults select control - Remote deploy: pass caller-resolved policy to build_launch_block; resolver tests in permission_policy.rs; deploy tests for all three policy sources - Terminal outcomes: timed_out and uncertain (pinned copy) in describePermissionOutcome - Tests: nonce-keyed card, concurrent cards, auth envelope, fallback key, channelId threading, delivery-failed/sent control_result (9 new) NIP-AO (docs/nips/NIP-AO.md): - switch_model: describe actual behavior; fix control_result statuses - acp_write example: actionable=false; correct payload shape Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/acp.rs | 587 ++++++++++++------ crates/buzz-acp/src/config.rs | 34 +- .../src-tauri/src/commands/agents_deploy.rs | 118 +++- .../src-tauri/src/commands/agents_tests.rs | 1 + .../src/managed_agents/permission_policy.rs | 72 +++ .../src-tauri/src/managed_agents/runtime.rs | 11 +- .../src/managed_agents/types/tests.rs | 3 +- .../features/agents/ui/AgentConfigFields.tsx | 1 + .../agents/ui/AgentDefaultsEditor.tsx | 32 +- .../LifecycleActivity.tsx | 25 +- .../agents/ui/agentSessionTranscript.test.mjs | 257 ++++++++ .../agents/ui/agentSessionTranscript.ts | 49 +- .../features/agents/ui/agentSessionTypes.ts | 7 + desktop/src/shared/api/agentControl.ts | 12 +- desktop/src/shared/api/types.ts | 6 + docs/nips/NIP-AO.md | 21 +- 16 files changed, 972 insertions(+), 264 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 554674bdf..866cd6536 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -13,7 +13,7 @@ use tokio::io::AsyncWriteExt; use tokio::process::{Child, ChildStdin, ChildStdout}; use tokio_util::codec::{FramedRead, LinesCodec, LinesCodecError}; -use crate::config::{ModeSource, PermissionMode, PermissionPolicy, ResolvedPermissionConfig}; +use crate::config::{PermissionMode, PermissionPolicy, ResolvedPermissionConfig}; use crate::observer::{AuthorizationEnvelope, ObserverContext, ObserverHandle}; use crate::usage::{TurnUsage, UsageTracker}; use buzz_core::observer::OBSERVER_MAX_PLAINTEXT_LEN; @@ -36,6 +36,15 @@ const PERMISSION_OPTIONS_MAX: usize = 16; /// fails closed with the denial response. const PERMISSION_ASK_TIMEOUT_SECS: u64 = 300; +/// Conservative upper bound on the serialised `ObserverEvent` envelope fields +/// (seq, timestamp, kind, channelId, sessionId, turnId, startedAt, authorization +/// nonce + actionable + reason, plus all JSON structural bytes). +/// +/// Used in the admission preflight to estimate the full annotated event size +/// without constructing the event — ensuring payloads that fit raw will still +/// fit once wrapped. 512 bytes comfortably covers all envelope fields. +const OBSERVER_EVENT_ENVELOPE_MAX: usize = 512; + /// An MCP server configuration passed to `session/new`. /// /// Corresponds to the `McpServerStdio` variant in the ACP schema. @@ -176,10 +185,10 @@ enum PermissionEntryState { /// Registered and waiting for an owner decision. Pending, /// A decision arrived; we are in the process of writing the response. - /// Holds the chosen `optionId`. Cancel during this state → `PermissionPoisoned`. - Writing(String), - /// Fully resolved — write confirmed. Kept in map until next request - /// or turn end to guard against duplicate delivery. + /// Cancel during this state → `PermissionPoisoned`. + Writing, + /// Fully resolved — write confirmed. Kept in map until turn end to guard + /// against duplicate delivery. Resolved, } @@ -189,8 +198,6 @@ struct PermissionEntry { nonce: String, /// The exact options snapshot from the original request. options_snapshot: Vec, - /// The original `session/request_permission` message — retained for acp_write emit. - msg_snapshot: serde_json::Value, /// Current lifecycle state. state: PermissionEntryState, /// Per-request hard deadline: `min(registered_at + 300s, turn hard deadline)`. @@ -690,13 +697,6 @@ impl AcpClient { self.permission_decision_rx = Some(rx); } - /// Whether this process is poisoned due to a cancel-during-write. - /// - /// Pool lifecycle MUST NOT return a poisoned process to the pool. - pub fn is_permission_poisoned(&self) -> bool { - self.permission_poisoned - } - /// Update metadata that will be attached to subsequent raw wire events. pub fn set_observer_context(&mut self, context: ObserverContext) { self.observer_context = context; @@ -967,6 +967,10 @@ impl AcpClient { Ok(_) => { self.last_prompt_id = None; self.current_hard_deadline = None; + // Turn completed normally — drain resolved/expired permission entries. + // Pending entries are unexpected here (should be Resolved or expired), + // but drain unconditionally to guarantee the map never leaks across turns. + self.pending_permissions.clear(); } Err(AcpError::IdleTimeout(_) | AcpError::HardTimeout { .. }) => { // Leave last_prompt_id and current_hard_deadline set — @@ -975,6 +979,10 @@ impl AcpClient { Err(_) => { self.last_prompt_id = None; self.current_hard_deadline = None; + // Non-recoverable error — drain the map to prevent capacity leak + // if the pool reuses this process (poisoned processes are respawned, + // but clean error exits may be returned to the pool). + self.pending_permissions.clear(); } } self.parse_stop_reason(&result?) @@ -1185,7 +1193,7 @@ impl AcpClient { for req_id_str in ids_to_cancel { let entry = self.pending_permissions.remove(&req_id_str).unwrap(); match entry.state { - PermissionEntryState::Writing(_) => { + PermissionEntryState::Writing => { tracing::error!( target: "acp::cancel", "cancel during permission write for req_id={req_id_str} — poisoning process" @@ -1258,6 +1266,9 @@ impl AcpClient { remaining, ) .await?; + // Cancel completed — drain any remaining permission entries (they were + // answered with cancelled above, but drain Resolved ones to free capacity). + self.pending_permissions.clear(); self.parse_stop_reason(&result) } @@ -1265,7 +1276,26 @@ impl AcpClient { /// /// Bounded by a 30-second write timeout. If the agent stops reading stdin /// (e.g., it's stuck or dead), the write would otherwise block forever. + /// + /// Emits a generic `acp_write` observer event. For permission response paths + /// that emit their own authorized event, use `write_ndjson_no_observe`. async fn write_ndjson(&mut self, value: &serde_json::Value) -> Result<(), AcpError> { + self.write_ndjson_inner(value, true).await + } + + /// Write NDJSON without emitting a generic `acp_write` observer event. + /// + /// Used for permission response paths that emit a single authorized event + /// themselves — prevents duplicate generic+authorized telemetry. + async fn write_ndjson_no_observe(&mut self, value: &serde_json::Value) -> Result<(), AcpError> { + self.write_ndjson_inner(value, false).await + } + + async fn write_ndjson_inner( + &mut self, + value: &serde_json::Value, + emit_observe: bool, + ) -> Result<(), AcpError> { const WRITE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); let line = serde_json::to_string(value)?; tokio::time::timeout(WRITE_TIMEOUT, async { @@ -1277,7 +1307,9 @@ impl AcpClient { .await .map_err(|_| AcpError::WriteTimeout(WRITE_TIMEOUT))? .map_err(AcpError::Io)?; - self.observe("acp_write", value.clone()); + if emit_observe { + self.observe("acp_write", value.clone()); + } Ok(()) } @@ -1459,9 +1491,26 @@ impl AcpClient { self.handle_goose_usage_update(&msg); } "session/request_permission" => { - let deadline = tokio::time::Instant::now() - + std::time::Duration::from_secs(PERMISSION_ASK_TIMEOUT_SECS); - self.handle_permission_request(&msg, true, deadline).await?; + // Pre-turn (session/new) path: no decision arm installed. + // Force reject regardless of policy — ask requests would + // register map entries that can never be resolved without + // the turn reader's decision arm. + let saved_policy = self.permission_config.policy; + if matches!(saved_policy, PermissionPolicy::Ask) { + // Temporarily downgrade to reject for this request only. + let saved = std::mem::replace( + &mut self.permission_config.policy, + PermissionPolicy::Reject, + ); + let deadline = tokio::time::Instant::now() + + std::time::Duration::from_secs(PERMISSION_ASK_TIMEOUT_SECS); + let _ = self.handle_permission_request(&msg, true, deadline).await; + self.permission_config.policy = saved; + } else { + let deadline = tokio::time::Instant::now() + + std::time::Duration::from_secs(PERMISSION_ASK_TIMEOUT_SECS); + self.handle_permission_request(&msg, true, deadline).await?; + } } other => { // If the unknown message has an id, it's a request expecting a reply. @@ -1566,12 +1615,37 @@ impl AcpClient { // Determine which deadline fires first BEFORE sleeping — this is // the classification we'll use on timeout, immune to scheduler jitter. - let idle_fires_first = idle_deadline < hard_deadline; - let next_deadline = if idle_fires_first { - idle_deadline + // + // Deadline logic: + // - When any Pending permission entries exist, suspend the idle + // deadline (owner is deciding; agent silence is expected) and + // wake on the earliest permission deadline instead. + // - Otherwise wake on min(idle, hard) as normal. + let has_pending_permissions = self + .pending_permissions + .values() + .any(|e| matches!(e.state, PermissionEntryState::Pending)); + let next_deadline; + let idle_fires_first; + if has_pending_permissions { + // Suspend idle; find earliest permission deadline (capped by hard). + let earliest_perm = self + .pending_permissions + .values() + .filter(|e| matches!(e.state, PermissionEntryState::Pending)) + .map(|e| e.deadline) + .min() + .unwrap_or(hard_deadline); + next_deadline = earliest_perm.min(hard_deadline); + idle_fires_first = false; // hard deadline governs if we wake } else { - hard_deadline - }; + idle_fires_first = idle_deadline < hard_deadline; + next_deadline = if idle_fires_first { + idle_deadline + } else { + hard_deadline + }; + } // Pre-select deadline check — required by Max's review. Under // `biased`, a continuously-ready reader arm wins every poll and @@ -1581,20 +1655,26 @@ impl AcpClient { // exists). Check the classified deadline here so a steady- // stream agent is still bounded. if Instant::now() >= next_deadline { - if let Some((_, _, ack_tx)) = pending_steer.take() { - // Prompt is timing out — release the withheld event via - // PromptCompletedNeutral (no fallback signal: there is - // no in-flight turn to signal once we return, and - // normal dispatch handles redelivery). - let _ = ack_tx.send(crate::pool::SteerAck::PromptCompletedNeutral); - } - if idle_fires_first { - tracing::warn!("idle timeout ({idle_timeout:?}) — no agent activity"); - return Err(AcpError::IdleTimeout(idle_timeout)); - } else { - let silence = Instant::now().saturating_duration_since(last_activity_at); - tracing::warn!("hard turn timeout exceeded (silence {silence:?})"); - return Err(AcpError::HardTimeout { silence }); + // When we woke for a permission deadline (not the hard deadline), + // skip the error return — let the expiry block below process the + // timed-out entries, then continue the loop. + let is_permission_wake = has_pending_permissions && next_deadline != hard_deadline; + if !is_permission_wake { + if let Some((_, _, ack_tx)) = pending_steer.take() { + // Prompt is timing out — release the withheld event via + // PromptCompletedNeutral (no fallback signal: there is + // no in-flight turn to signal once we return, and + // normal dispatch handles redelivery). + let _ = ack_tx.send(crate::pool::SteerAck::PromptCompletedNeutral); + } + if idle_fires_first { + tracing::warn!("idle timeout ({idle_timeout:?}) — no agent activity"); + return Err(AcpError::IdleTimeout(idle_timeout)); + } else { + let silence = Instant::now().saturating_duration_since(last_activity_at); + tracing::warn!("hard turn timeout exceeded (silence {silence:?})"); + return Err(AcpError::HardTimeout { silence }); + } } } @@ -1689,13 +1769,12 @@ impl AcpClient { ); } else { // Transition Pending → Writing. - let (nonce, opts, msg_snap, id_val) = { + let (nonce, opts, id_val) = { let entry = self.pending_permissions.get_mut(&id_str).unwrap(); - entry.state = PermissionEntryState::Writing(decision.option_id.clone()); + entry.state = PermissionEntryState::Writing; ( entry.nonce.clone(), entry.options_snapshot.clone(), - entry.msg_snapshot.clone(), serde_json::from_str::(&id_str) .unwrap_or_else(|_| serde_json::Value::String(id_str.clone())), ) @@ -1706,7 +1785,7 @@ impl AcpClient { let write_deadline = (Instant::now() + std::time::Duration::from_secs(30)) .min(hard_deadline); - let write_result = tokio::time::timeout_at(write_deadline, self.write_ndjson(&response)).await; + let write_result = tokio::time::timeout_at(write_deadline, self.write_ndjson_no_observe(&response)).await; match write_result { Ok(Ok(())) => { @@ -1714,7 +1793,7 @@ impl AcpClient { if let Some(entry) = self.pending_permissions.get_mut(&id_str) { entry.state = PermissionEntryState::Resolved; } - // Emit enveloped acp_write after confirmed write. + // Emit single authorized acp_write after confirmed write. self.observe_authorized( "acp_write", AuthorizationEnvelope { @@ -1724,7 +1803,7 @@ impl AcpClient { }, response, ); - let _ = (opts, msg_snap); // used above for validation + let _ = opts; // used above for validation tracing::info!( target: "acp::permission", "permission id={id_val} answered: optionId={:?}", @@ -1862,16 +1941,24 @@ impl AcpClient { // would catch this anyway, but firing the deadline arm // here makes the wakeup immediate (no extra reader poll // round-trip when stdout is idle). - if let Some((_, _, ack_tx)) = pending_steer.take() { - let _ = ack_tx.send(crate::pool::SteerAck::PromptCompletedNeutral); - } - if idle_fires_first { - tracing::warn!("idle timeout ({idle_timeout:?}) — no agent activity"); - return Err(AcpError::IdleTimeout(idle_timeout)); + // For a permission-deadline wake, loop back to let the + // expiry block process timed-out entries. + let is_permission_wake = + has_pending_permissions && next_deadline != hard_deadline; + if is_permission_wake { + None // loop back; expiry block will fire } else { - let silence = Instant::now().saturating_duration_since(last_activity_at); - tracing::warn!("hard turn timeout exceeded (silence {silence:?})"); - return Err(AcpError::HardTimeout { silence }); + if let Some((_, _, ack_tx)) = pending_steer.take() { + let _ = ack_tx.send(crate::pool::SteerAck::PromptCompletedNeutral); + } + if idle_fires_first { + tracing::warn!("idle timeout ({idle_timeout:?}) — no agent activity"); + return Err(AcpError::IdleTimeout(idle_timeout)); + } else { + let silence = Instant::now().saturating_duration_since(last_activity_at); + tracing::warn!("hard turn timeout exceeded (silence {silence:?})"); + return Err(AcpError::HardTimeout { silence }); + } } } }; @@ -2506,17 +2593,14 @@ impl AcpClient { PermissionEntry { nonce, options_snapshot: options.clone(), - msg_snapshot: msg.clone(), state: PermissionEntryState::Pending, deadline: entry_deadline, }, ); - // Also track in the simple single-id field so cancel_with_cleanup - // can drain without touching the map (belt-and-suspenders, cleared - // by the map drain path in cancel_with_cleanup_until). - self.pending_permission_id = Some(id.clone()); - self.permission_responded = false; + // Do NOT set pending_permission_id for ask — the map is the + // sole source of truth. The legacy single-id slot is only used + // by reject/allow (synchronous paths). Ok(true) } } @@ -2684,9 +2768,16 @@ fn permission_denial_response( return Ok(permission_response_cancelled(id)); }; - let option_id = opt["optionId"] - .as_str() - .ok_or_else(|| AcpError::Protocol("reject_once option missing optionId".into()))?; + let Some(option_id) = opt["optionId"].as_str().filter(|s| !s.is_empty()) else { + // reject_once found but optionId is missing or empty — malformed request; + // fall back to `cancelled` rather than returning a Protocol error so the + // adapter still receives a valid JSON-RPC response. + tracing::warn!( + target: "acp::permission", + "reject_once option has missing or empty optionId for id={id}, cancelling" + ); + return Ok(permission_response_cancelled(id)); + }; tracing::info!( target: "acp::permission", "rejecting permission id={id} with reject_once optionId={option_id:?}" @@ -2825,13 +2916,22 @@ fn run_admission_preflight( )); } - // 8. full serialised msg fits within OBSERVER_MAX_PLAINTEXT_LEN - let serialised_len = serde_json::to_string(msg) + // 8. Full annotated `ObserverEvent` fits within `OBSERVER_MAX_PLAINTEXT_LEN`. + // + // The limit applies to the complete serialised event (seq, timestamp, kind, + // context fields, authorization envelope, payload), not just the raw `msg`. + // We conservatively add `OBSERVER_EVENT_ENVELOPE_MAX` to the raw payload + // size to account for all wrapper fields (seq, timestamp, kind, channelId, + // sessionId, turnId, startedAt, authorization nonce+actionable+reason, JSON + // punctuation). Any raw payload within the limit-minus-overhead is guaranteed + // to fit once wrapped; anything larger may overflow after wrapping. + let raw_len = serde_json::to_string(msg) .map(|s| s.len()) .unwrap_or(usize::MAX); - if serialised_len > OBSERVER_MAX_PLAINTEXT_LEN { + let annotated_len = raw_len.saturating_add(OBSERVER_EVENT_ENVELOPE_MAX); + if annotated_len > OBSERVER_MAX_PLAINTEXT_LEN { return Err(format!( - "permission request payload too large: {serialised_len} > {OBSERVER_MAX_PLAINTEXT_LEN}" + "permission request payload too large: annotated size ~{annotated_len} > {OBSERVER_MAX_PLAINTEXT_LEN}" )); } @@ -3047,6 +3147,7 @@ fn configure_no_window(cmd: &mut tokio::process::Command) { #[cfg(test)] mod tests { use super::*; + use crate::config::ModeSource; #[test] fn stop_reason_parses_all_known_values() { @@ -3157,17 +3258,21 @@ mod tests { assert_eq!(outcome(&response), Some("cancelled")); } - /// A `reject_once` option missing its `optionId` is a protocol violation. - /// Erroring propagates to the caller, which tears the turn down — still no - /// approval is ever sent. + /// A `reject_once` option missing its `optionId` falls back to a `cancelled` + /// response rather than propagating a Protocol error. This ensures the adapter + /// always receives a valid JSON-RPC response, even for malformed requests. #[test] - fn reject_once_without_option_id_is_a_protocol_error() { + fn reject_once_without_option_id_falls_back_to_cancelled() { let options = options(r#"[{"name": "Reject", "kind": "reject_once"}]"#); - let err = permission_denial_response(&serde_json::json!(1), &options) - .expect_err("missing optionId must error"); + let response = permission_denial_response(&serde_json::json!(1), &options) + .expect("malformed reject_once must not error"); - assert!(matches!(err, AcpError::Protocol(_)), "got {err:?}"); + assert_eq!( + response["result"]["outcome"]["outcome"].as_str(), + Some("cancelled"), + "malformed reject_once must produce cancelled, got: {response}" + ); } #[test] @@ -5580,7 +5685,6 @@ mod tests { PermissionEntry { nonce: "nonce-abc".to_string(), options_snapshot: vec![], - msg_snapshot: serde_json::json!({}), state: PermissionEntryState::Pending, deadline: tokio::time::Instant::now() + std::time::Duration::from_secs(300), }, @@ -5637,6 +5741,75 @@ mod tests { ); } + #[test] + fn admission_preflight_rejects_payload_fitting_raw_but_overflowing_after_envelope() { + // Construct a payload just *below* OBSERVER_MAX_PLAINTEXT_LEN in raw + // serialised size, but exceeding it after adding OBSERVER_EVENT_ENVELOPE_MAX. + // This is the exact case the annotation-aware check defends against: a + // request that would pass a raw-only gate but overflow after wrapping. + let id = serde_json::json!(42); + // Raw payload that is (OBSERVER_MAX_PLAINTEXT_LEN - 1) bytes when serialised. + // The string value is padded to make the total serialised msg length exactly + // OBSERVER_MAX_PLAINTEXT_LEN - 1; the envelope overhead then pushes it over. + // + // We embed a string of length L where the *total* serialised msg equals + // OBSERVER_MAX_PLAINTEXT_LEN - 1. Because we can't compute L analytically + // without knowing the surrounding JSON size, we binary-search by trying a + // small-enough payload and padding it. + // + // Simpler: just use a payload of size (OBSERVER_MAX_PLAINTEXT_LEN - OBSERVER_EVENT_ENVELOPE_MAX + 1). + // Raw size will be just above (cap - overhead), so annotated = raw + overhead > cap. + let pad_len = OBSERVER_MAX_PLAINTEXT_LEN.saturating_sub(OBSERVER_EVENT_ENVELOPE_MAX) + 1; + let subject = "y".repeat(pad_len); + let msg = serde_json::json!({ + "jsonrpc": "2.0", + "id": 42, + "method": "session/request_permission", + "params": { + "sessionId": "sess", + "subject": subject, + "options": [{"optionId":"opt","kind":"allow_once","name":"A"}] + } + }); + let opts = vec![serde_json::json!({"optionId":"opt","kind":"allow_once","name":"A"})]; + // Verify our payload is actually raw-size > (cap - overhead) — i.e., annotated size > cap. + let raw_len = serde_json::to_string(&msg).unwrap().len(); + assert!( + raw_len > OBSERVER_MAX_PLAINTEXT_LEN.saturating_sub(OBSERVER_EVENT_ENVELOPE_MAX), + "test setup: raw_len ({raw_len}) must exceed cap-minus-overhead to trigger the annotated check" + ); + let result = run_admission_preflight(&id, &opts, &msg, PermissionPolicy::Ask, false, false); + assert!( + result.is_err(), + "payload that overflows after envelope overhead must fail preflight (raw_len={raw_len})" + ); + let reason = result.unwrap_err(); + assert!( + reason.contains("too large") || reason.contains("payload"), + "reason should mention payload size, got: {reason}" + ); + } + + #[test] + fn denial_response_with_malformed_reject_once_falls_back_to_cancelled() { + // A reject_once option with a missing optionId must produce a `cancelled` + // response, not a Protocol error — the adapter must always receive a valid + // JSON-RPC response. + let id = serde_json::json!(7); + let opts = vec![ + serde_json::json!({"kind": "reject_once", "name": "Reject"}), // no optionId + ]; + let response = permission_denial_response(&id, &opts) + .expect("malformed reject_once must not return Err"); + // The response must be a cancelled frame (no optionId in result.outcome). + let outcome = &response["result"]["outcome"]; + assert_eq!( + outcome["outcome"].as_str(), + Some("cancelled"), + "malformed reject_once must produce cancelled response, got: {response}" + ); + } + // ── Pinned §5: map overflow ─────────────────────────────────────────────── #[tokio::test] @@ -5651,7 +5824,6 @@ mod tests { PermissionEntry { nonce: format!("nonce-{i}"), options_snapshot: vec![], - msg_snapshot: serde_json::json!({}), state: PermissionEntryState::Pending, deadline: tokio::time::Instant::now() + std::time::Duration::from_secs(300), }, @@ -5827,109 +5999,82 @@ mod tests { #[tokio::test] async fn ask_decision_consumed_writes_response_and_continues() { - // Script: emit the permission request, pause for the harness to process and write - // the decision response to stdin, then read the response from stdin and emit the - // final prompt response. - // - // The harness reads from the script's stdout; the script reads from the harness's - // write (stdin). The script waits to confirm the harness wrote a response before - // emitting the terminal prompt response. - let perm_req = serde_json::json!({ - "jsonrpc": "2.0", - "id": 1, - "method": "session/request_permission", - "params": { - "sessionId": "sess-ask", - "options": [ - {"optionId": "opt-allow", "kind": "allow_once", "name": "Allow once"}, - {"optionId": "opt-reject", "kind": "reject_once", "name": "Reject once"} - ] - } - }); - let perm_req_line = serde_json::to_string(&perm_req).unwrap(); + // Setup: ask policy, observer + owner active, permission_decision channel installed. + // A Pending entry is pre-planted with a known nonce so we can deliver a matching + // decision without needing access to nonce generation inside the loop. + // The script immediately emits the terminal id=999 response (simulating the + // adapter continuing after the permission response was written to its stdin). + let script = r#"echo '{"jsonrpc":"2.0","id":999,"result":{"done":true}}'"#; + let mut client = spawn_script(script).await; - // Script: - // 1. Emit the permission request immediately. - // 2. Wait for the harness to write the permission response (read one line from stdin). - // 3. Emit the terminal prompt response (id=999). - let script = format!( - "echo '{perm}'; read -t 5 _perm_response; echo '{{\"jsonrpc\":\"2.0\",\"id\":999,\"result\":{{\"done\":true}}}}'", - perm = perm_req_line.replace("'", "'\\''"), - ); - - let mut client = spawn_script(&script).await; - - // Configure ask policy + owner known so the ask path is available. let config = ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, None).unwrap(); client.set_permission_config(config); client.set_owner_pubkey_known(true); - - // Install observer so the ask arm doesn't downgrade. let obs = crate::observer::ObserverHandle::in_process(); client.set_observer(Some(obs.clone()), 0); - // Install the permission decision channel. - let (perm_tx, perm_rx) = - tokio::sync::mpsc::channel::(PERMISSION_MAP_CAP); + let (perm_tx, perm_rx) = tokio::sync::mpsc::channel::(8); client.install_permission_decision_rx(perm_rx); - // Spawn a task to deliver the decision after a short delay — simulates - // the desktop owner clicking the card. - let perm_tx_clone = perm_tx.clone(); - tokio::spawn(async move { - // Allow the read loop to register the pending entry first. - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - // We need the nonce from the registered entry, but for this test we - // deliver the decision by looking up the entry nonce after it's set. - // Instead, deliver via a separate channel + coordinate by sleeping. - let _ = perm_tx_clone; // dropped; the test re-sends via perm_tx below - }); + // Plant a Pending entry with a known nonce. + let known_nonce = "test-nonce-loop-success".to_string(); + let req_id_str = "42".to_string(); + client.pending_permissions.insert( + req_id_str.clone(), + PermissionEntry { + nonce: known_nonce.clone(), + options_snapshot: vec![ + serde_json::json!({"optionId":"opt-allow","kind":"allow_once","name":"Allow"}), + ], + state: PermissionEntryState::Pending, + deadline: tokio::time::Instant::now() + std::time::Duration::from_secs(300), + }, + ); - // Drive the read loop to register the entry, then inject the decision. - // We use a background task to drive the loop and inject via the sender. - let (result_tx, result_rx) = tokio::sync::oneshot::channel(); - let mut client_moved = client; - let perm_tx_deliver = perm_tx; - let driver_handle = tokio::spawn(async move { - // Read until a decision delivers the response, then the loop continues - // until it reads the id=999 response. - let idle = std::time::Duration::from_secs(5); - let max_dur = std::time::Duration::from_secs(10); - let hard_deadline = tokio::time::Instant::now() + max_dur; + // Deliver a matching decision (by nonce) with a valid optionId. + // The decision is already in the channel before the loop starts; the biased + // select! arm reads it on the first iteration. + perm_tx + .send(PermissionDecision { + request_nonce: known_nonce, + option_id: "opt-allow".to_string(), + }) + .await + .unwrap(); - // Deliver a decision slightly after the loop starts — give the loop - // time to register the pending entry and note the nonce. - // We spawn another task to do this delivery. - let deliver_task = tokio::spawn(async move { - // Short sleep so the read loop processes the permission request first. - tokio::time::sleep(std::time::Duration::from_millis(200)).await; - // The nonce is unknown here, but `decision_rx` finds the entry by nonce match. - // We peek the map from the same client — can't do that here since client is moved. - // Work around: inject a dummy nonce; the entry will NOT match, so this tests - // the nonce-mismatch path. Instead, the real test flow requires two-step: - // see the NOTE below. - let _ = perm_tx_deliver; // Let the channel close to avoid a block. - }); - let read_result = client_moved - .read_until_response_with_idle_timeout( - "sess-ask", - 999, - idle, - hard_deadline, - max_dur, - ) - .await; - deliver_task.await.ok(); - let _ = result_tx.send((read_result, client_moved)); - }); - driver_handle.await.expect("driver task did not panic"); - let (read_result, _client) = result_rx.await.expect("oneshot result"); - // The loop should exit via idle timeout (decision channel was dropped without - // delivering — the real success path requires same-task nonce access). - // This test validates the structure compiles and runs without panic. - // Full end-to-end ask success is exercised by integration tests. - let _ = result_rx; // silence unused warning - let _ = read_result; + // Drive the loop. It should: (1) find the pre-delivered decision, write the + // permission response, transition entry → Resolved; (2) continue and read the + // id=999 terminal response from the script. + let idle = std::time::Duration::from_secs(5); + let max_dur = std::time::Duration::from_secs(10); + let hard_deadline = tokio::time::Instant::now() + max_dur; + let result = client + .read_until_response_with_idle_timeout( + "sess-ask-success", + 999, + idle, + hard_deadline, + max_dur, + ) + .await; + + assert!( + result.is_ok(), + "loop must succeed after decision is consumed, got: {result:?}" + ); + + // The entry must have been transitioned to Resolved (decision was applied). + let entry = client.pending_permissions.get(&req_id_str); + match entry { + Some(e) => assert!( + matches!(e.state, PermissionEntryState::Resolved), + "entry must be Resolved after decision applied, got: {:?}", + e.state + ), + None => { + // Entry may have been drained at turn end — also acceptable. + } + } } // ── Pinned §1 (simpler): ask entry registered synchronously ────────────── @@ -6003,8 +6148,7 @@ mod tests { PermissionEntry { nonce: "n99".to_string(), options_snapshot: vec![], - msg_snapshot: serde_json::json!({}), - state: PermissionEntryState::Writing("opt-allow".to_string()), + state: PermissionEntryState::Writing, deadline: tokio::time::Instant::now() + std::time::Duration::from_secs(300), }, ); @@ -6021,7 +6165,7 @@ mod tests { "expected PermissionPoisoned, got {err:?}" ); assert!( - client.is_permission_poisoned(), + client.permission_poisoned, "poisoned flag must be set after cancel-during-write" ); }); @@ -6074,6 +6218,11 @@ mod tests { fn cancel_drains_pending_entries_with_cancelled_response() { // Under ask policy: cancel must drain all Pending entries and write // "cancelled" responses for each, then proceed to session/cancel. + // Verifies: + // - Map is empty after cancel (entries were drained). + // - Cancel result is NOT PermissionPoisoned (no Writing entries present). + // - Cancel exits normally (Ok or CancelDrainTimeout — sleep script never + // emits a response, so this exits via timeout, which is expected). // // We can verify that Pending entries are removed by checking the map post-cancel. // We don't verify the wire bytes here (that requires a live script) — we verify @@ -6095,7 +6244,6 @@ mod tests { options_snapshot: vec![ serde_json::json!({"optionId":"opt","kind":"reject_once","name":"R"}), ], - msg_snapshot: serde_json::json!({}), state: PermissionEntryState::Pending, deadline: tokio::time::Instant::now() + std::time::Duration::from_secs(300), }, @@ -6141,6 +6289,15 @@ mod tests { client.pending_permissions.is_empty(), "reject must not leave pending entries" ); + // Legacy single-id slot must also be cleared after the synchronous response. + assert!( + client.pending_permission_id.is_none(), + "pending_permission_id must be None after reject completes" + ); + assert!( + client.permission_responded, + "permission_responded must be true after reject completes" + ); } // ── Pinned §2: allow policy auto-selects allow_once ─────────────────────── @@ -6191,22 +6348,30 @@ mod tests { #[tokio::test] async fn decision_with_unknown_option_id_is_ignored() { // A decision carrying an optionId not in the snapshot must be ignored - // (no response written, entry stays Pending) — tested by verifying that - // the entry remains in Pending state after the decision is delivered. - let mut client = spawn_inert_client().await; + // (no response written, entry stays Pending) — the loop continues. + // After the bad decision is processed, the loop times out on idle (since the + // script produces no output after the initial response) and the entry is + // still Pending at that point. + // + // The script produces the terminal id=999 response only AFTER a short delay, + // giving the loop time to process the bad decision and leave the entry Pending. + // We verify the entry is still Pending by running the loop until idle timeout. + let script = "sleep 2; echo '{\"jsonrpc\":\"2.0\",\"id\":999,\"result\":{\"done\":true}}'"; + let mut client = spawn_script(script).await; + client.set_owner_pubkey_known(true); set_policy(&mut client, PermissionPolicy::Ask); let obs = crate::observer::ObserverHandle::in_process(); client.set_observer(Some(obs), 0); - let nonce = "test-nonce-abc".to_string(); + let nonce = "test-nonce-bad-opt".to_string(); + let req_id_str = "5".to_string(); client.pending_permissions.insert( - "5".to_string(), + req_id_str.clone(), PermissionEntry { nonce: nonce.clone(), options_snapshot: vec![ serde_json::json!({"optionId":"valid-opt","kind":"allow_once","name":"A"}), ], - msg_snapshot: serde_json::json!({}), state: PermissionEntryState::Pending, deadline: tokio::time::Instant::now() + std::time::Duration::from_secs(300), }, @@ -6218,26 +6383,39 @@ mod tests { option_id: "nonexistent-option".to_string(), }; - // Drive one read-loop iteration with the decision on the channel. - // We use a script that produces the terminal response quickly so the loop exits. let (tx, rx) = tokio::sync::mpsc::channel::(1); client.install_permission_decision_rx(rx); + // Send the bad decision; then close the sender so the channel is exhausted. tx.send(bad_decision).await.unwrap(); - drop(tx); // Close channel so the loop can exit. + drop(tx); - // Re-assign client to a fresh script that immediately emits the terminal response. - // We can't easily run the loop here because we've already moved the receiver. - // Instead, verify map state directly: the entry should still be Pending after - // the bad decision (the loop hasn't run, so it hasn't had a chance to ignore it). - // This is a structural unit test for the invariant. - let entry = client - .pending_permissions - .get("5") - .expect("entry must exist"); - assert!( - matches!(entry.state, PermissionEntryState::Pending), - "entry must still be Pending before the bad decision is processed" - ); + // Drive the loop with a short idle timeout — the bad decision is processed + // on the first iteration (entry stays Pending), then the loop idles. + let idle = std::time::Duration::from_millis(300); + let max_dur = std::time::Duration::from_secs(5); + let hard_deadline = tokio::time::Instant::now() + max_dur; + let result = client + .read_until_response_with_idle_timeout("sess-bad-opt", 5, idle, hard_deadline, max_dur) + .await; + + // The loop exits via idle timeout (script sleeps; bad decision was ignored, + // so no terminal response for id=5 was written, and idle fires). + // We accept either idle timeout OR id=999 match (if the script's sleep was short). + // The critical assertion is on the entry state. + let _ = result; // exit reason is not the focus + + // Entry must still be Pending — the bad decision did not mutate it. + let entry = client.pending_permissions.get(&req_id_str); + // The loop drains on non-recoverable errors; on idle timeout (recoverable) it + // does NOT drain — entry must still be there and Pending. + match entry { + Some(e) => assert!( + matches!(e.state, PermissionEntryState::Pending), + "entry must still be Pending after bad decision, got: {:?}", + e.state + ), + None => panic!("entry was removed — idle timeout should not drain the map"), + } } // ── Pinned §7 (wire transmission): transmit_mode drives set_config_option ─ @@ -6257,10 +6435,11 @@ mod tests { // ── Pinned amendment: PermissionMode::Auto matrix row ──────────────────── // - // `auto` = "fully autonomous execution" — the adapter self-approves all - // tool calls internally and never emits `session/request_permission`. + // `auto` = model-gated classifier — the adapter may self-approve most tool + // calls internally but can still forward residual permission requests to ACP. // - allow + auto → compatible (transmit as-is; both want unattended approval) - // - ask + auto → startup error (card never fires — ask becomes dead letter) + // - ask + auto → compatible with warning (residual escalations surface cards; + // internally-approved calls bypass ask silently) // - reject + auto → startup error (inverted security: policy says deny, adapter // auto-approves everything) @@ -6276,13 +6455,19 @@ mod tests { } #[test] - fn resolved_permission_config_ask_plus_explicit_auto_is_startup_error() { - // ask + auto: adapter self-approves internally, card never fires. + fn resolved_permission_config_ask_plus_explicit_auto_is_ok_with_warning() { + // ask + auto is compatible-with-warning: residual escalations still surface + // cards; internally-approved calls bypass the ask flow silently. + // `auto` is a model classifier, not a bypass — some requests still escalate. let result = ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, Some(PermissionMode::Auto)); - assert!(result.is_err(), "ask + auto must be a startup error"); - let msg = format!("{}", result.unwrap_err()); - assert!(msg.contains("auto"), "error must mention auto, got: {msg}"); + assert!( + result.is_ok(), + "ask + auto must succeed (warn only), got: {result:?}" + ); + let cfg = result.unwrap(); + assert_eq!(cfg.effective_mode, PermissionMode::Auto); + assert_eq!(cfg.mode_source, ModeSource::Explicit); } #[test] diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 34423f458..984ee9b24 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -162,6 +162,7 @@ impl PermissionMode { /// Returns `true` when the mode is the agent's built-in default and /// therefore doesn't need to be explicitly set. + #[cfg(test)] pub fn is_default(&self) -> bool { matches!(self, Self::Default) } @@ -258,10 +259,12 @@ impl ResolvedPermissionConfig { /// - `ask` + explicit `dontAsk` — harness would want the agent to /// escalate, but `dontAsk` makes the agent self-deny internally. /// - `allow` + explicit `dontAsk` — same contradiction. - /// - `ask` + explicit `auto` — adapter self-approves internally, so the - /// card never fires; the `ask` policy becomes a silent dead letter. /// - `reject` + explicit `auto` — inverted-security worst case: policy says /// "deny" but the adapter auto-approves everything internally. + /// + /// Emits a warning (not an error) for `ask + auto`: internally-approved tool + /// calls bypass the ask flow silently, but residual escalations still surface + /// cards — the combination works, with the caveat that not all requests are seen. pub fn resolve( policy: PermissionPolicy, explicit_mode: Option, @@ -275,20 +278,29 @@ impl ResolvedPermissionConfig { dontAsk makes the agent self-deny internally before Buzz can answer" ))); } - // Fail on ask/reject + auto: `auto` makes the adapter self-approve - // internally so `session/request_permission` never crosses ACP. - // Under `ask` the card never fires; under `reject` the policy is a dead - // letter while the adapter silently grants everything (inverted security). + // Fail on reject + auto: inverted-security worst case — policy says "deny" + // but the adapter auto-approves everything internally. + // `ask` + auto is a warning-only case: the adapter MAY still forward residual + // permission requests to ACP (auto is a model classifier, not bypass mode); + // warn and transmit rather than fail startup. // `allow` + auto is compatible: both policies want unattended approval. - if matches!(policy, PermissionPolicy::Ask | PermissionPolicy::Reject) - && explicit_mode == Some(PermissionMode::Auto) - { + if policy == PermissionPolicy::Reject && explicit_mode == Some(PermissionMode::Auto) { return Err(ConfigError::ConfigFile(format!( "permission_policy={policy} conflicts with permission_mode=auto: \ - auto makes the adapter self-approve internally before Buzz can answer \ - (ask: card never fires; reject: policy becomes a dead letter)" + auto makes the adapter self-approve internally, which bypasses the \ + reject policy — inverted-security worst case" ))); } + // Warn on ask + auto: residual permission requests may still reach ACP + // (auto is a model classifier, not bypass mode) so ask can still surface + // cards — but internally-approved calls will bypass the ask flow silently. + if policy == PermissionPolicy::Ask && explicit_mode == Some(PermissionMode::Auto) { + tracing::warn!( + "permission_policy=ask with permission_mode=auto: internally-approved \ + tool calls bypass Buzz ask flow; residual escalations will still \ + surface cards. Consider policy=allow if unattended approval is intended." + ); + } let (effective_mode, mode_source) = match explicit_mode { Some(m) => (m, ModeSource::Explicit), diff --git a/desktop/src-tauri/src/commands/agents_deploy.rs b/desktop/src-tauri/src/commands/agents_deploy.rs index 898653da3..8e425dd07 100644 --- a/desktop/src-tauri/src/commands/agents_deploy.rs +++ b/desktop/src-tauri/src/commands/agents_deploy.rs @@ -46,6 +46,10 @@ pub(crate) fn resolve_deploy_model_provider( /// `descriptor.env` is the authoritative six-layer environment. Policy values /// are deliberately separate because providers apply them below that layered /// environment, preserving the local spawn's power-user override semantics. +/// +/// `effective_permission_policy` is the already-resolved per-agent → global → +/// built-in policy. Pass it from the caller so that this function does not need +/// the global config; tests can pass `None` to get the built-in default. pub(super) fn build_launch_block( record: &ManagedAgentRecord, descriptor: &crate::managed_agents::readiness::EffectiveHarnessDescriptor, @@ -53,6 +57,7 @@ pub(super) fn build_launch_block( effective_prompt: Option<&str>, effective_model: Option<&str>, owner_pubkey: &str, + effective_permission_policy: Option, ) -> serde_json::Value { use crate::managed_agents::{ known_acp_runtime, resolve_session_title, DISPLAY_NAME_ENV_VAR, SESSION_TITLE_ENV_VAR, @@ -101,24 +106,17 @@ pub(super) fn build_launch_block( policy_env.insert("BUZZ_ACP_TEAM_INSTRUCTIONS".into(), value); } - // Permission policy: injected into policy_env so the remote process uses the - // same resolved value as a local spawn. Because deployed remote agents are - // read-only for this field (changing it requires shutdown + redeploy), the - // value here is always the record's own field falling back to the built-in - // (`ask`). The global config is intentionally not consulted for remote deploy — - // the global config is a desktop-local setting, not a per-record contract. + // Permission policy: use the caller-resolved value (per-agent → global → + // built-in), falling back to the built-in default if the caller did not + // provide one. Tests pass `None`; production callers pass the result of + // `resolve_effective_permission_policy(record, global_config)`. { - // For remote deploys we resolve directly from the record + built-in. - // The global config is not available here (it's a desktop-local fallback); - // an empty GlobalAgentConfig has no permission_policy so only the record - // and built-in are consulted — correct for a remote agent whose lifetime - // outlasts the spawning desktop session. - let remote_policy = record - .permission_policy - .unwrap_or_else(crate::managed_agents::permission_policy::PermissionPolicy::desktop_default); + let policy = effective_permission_policy.unwrap_or_else( + crate::managed_agents::permission_policy::PermissionPolicy::desktop_default, + ); policy_env.insert( "BUZZ_ACP_PERMISSION_POLICY".into(), - remote_policy.as_str().to_string(), + policy.as_str().to_string(), ); } @@ -170,6 +168,10 @@ pub(super) fn build_deploy_payload( crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) .map_err(|error| crate::managed_agents::user_facing_harness_error(&error))?; let owner_pubkey = super::workspace_owner_hex(state)?; + let (effective_policy, _) = + crate::managed_agents::permission_policy::resolve_effective_permission_policy( + record, &global, + ); let launch = build_launch_block( record, &descriptor, @@ -177,6 +179,7 @@ pub(super) fn build_deploy_payload( effective.system_prompt.value.as_deref(), effective.model.value.as_deref(), &owner_pubkey, + Some(effective_policy), ); let effective_parallelism = @@ -288,6 +291,7 @@ mod tests { Some("prompt"), Some("model"), "owner-hex", + None, ); assert_eq!(launch["command"], "goose"); @@ -326,7 +330,7 @@ mod tests { env: BTreeMap::new(), }; - let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex", None); assert_eq!( launch["policy_env"]["BUZZ_ACP_AGENTS"], @@ -348,7 +352,7 @@ mod tests { env: BTreeMap::new(), }; - let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex", None); assert_eq!( launch["policy_env"]["BUZZ_ACP_AGENTS"], "8", @@ -378,7 +382,7 @@ mod tests { }; let cap = crate::managed_agents::parallelism::OPENCLAW_MAX_PARALLELISM; - let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex", None); let effective_parallelism = crate::managed_agents::effective_parallelism(&descriptor.command, record.parallelism); let payload = deploy_payload_json( @@ -423,7 +427,7 @@ mod tests { env: BTreeMap::new(), }; - let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex", None); let effective_parallelism = crate::managed_agents::effective_parallelism(&descriptor.command, record.parallelism); let payload = deploy_payload_json( @@ -469,7 +473,7 @@ mod tests { }; let cap = crate::managed_agents::parallelism::OPENCLAW_MAX_PARALLELISM; - let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex"); + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex", None); let effective_parallelism = crate::managed_agents::effective_parallelism(&descriptor.command, record.parallelism); let payload = deploy_payload_json( @@ -496,4 +500,78 @@ mod tests { "legacy top-level parallelism must match launch.policy_env — both must be {cap}" ); } + + /// `build_launch_block` with an explicit `allow` policy injects `allow`. + #[test] + fn launch_block_explicit_allow_policy_injected() { + let record = record(); + let descriptor = EffectiveHarnessDescriptor { + command: "goose".into(), + args: vec![], + env: BTreeMap::new(), + }; + let launch = build_launch_block( + &record, + &descriptor, + &[], + None, + None, + "owner-hex", + Some(crate::managed_agents::permission_policy::PermissionPolicy::Allow), + ); + assert_eq!( + launch["policy_env"]["BUZZ_ACP_PERMISSION_POLICY"], "allow", + "explicit allow policy must be injected into policy_env" + ); + } + + /// `build_launch_block` with `None` (test callers / no global) falls back to + /// the built-in desktop default (`ask`). + #[test] + fn launch_block_none_policy_falls_back_to_built_in_ask() { + let record = record(); + let descriptor = EffectiveHarnessDescriptor { + command: "goose".into(), + args: vec![], + env: BTreeMap::new(), + }; + let launch = build_launch_block(&record, &descriptor, &[], None, None, "owner-hex", None); + assert_eq!( + launch["policy_env"]["BUZZ_ACP_PERMISSION_POLICY"], "ask", + "None effective_permission_policy must fall back to built-in ask" + ); + } + + /// Production deploy path: global `allow` override is respected when the + /// record has no per-agent policy, matching the local-spawn resolver. + #[test] + fn launch_block_global_allow_policy_used_when_record_has_none() { + let mut record = record(); + record.permission_policy = None; + let descriptor = EffectiveHarnessDescriptor { + command: "goose".into(), + args: vec![], + env: BTreeMap::new(), + }; + let mut global = crate::managed_agents::global_config::GlobalAgentConfig::default(); + global.permission_policy = + Some(crate::managed_agents::permission_policy::PermissionPolicy::Allow); + let (effective_policy, _) = + crate::managed_agents::permission_policy::resolve_effective_permission_policy( + &record, &global, + ); + let launch = build_launch_block( + &record, + &descriptor, + &[], + None, + None, + "owner-hex", + Some(effective_policy), + ); + assert_eq!( + launch["policy_env"]["BUZZ_ACP_PERMISSION_POLICY"], "allow", + "global allow policy must be injected when record has no per-agent policy" + ); + } } diff --git a/desktop/src-tauri/src/commands/agents_tests.rs b/desktop/src-tauri/src/commands/agents_tests.rs index 8863a62d1..ec52fc183 100644 --- a/desktop/src-tauri/src/commands/agents_tests.rs +++ b/desktop/src-tauri/src/commands/agents_tests.rs @@ -483,6 +483,7 @@ fn deploy_payload_matches_the_shared_full_launch_fixture() { None, Some("gpt-5"), "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + None, ); let agent = deploy_payload_json( &record, diff --git a/desktop/src-tauri/src/managed_agents/permission_policy.rs b/desktop/src-tauri/src/managed_agents/permission_policy.rs index bcacccb12..cc6fb92f0 100644 --- a/desktop/src-tauri/src/managed_agents/permission_policy.rs +++ b/desktop/src-tauri/src/managed_agents/permission_policy.rs @@ -80,3 +80,75 @@ pub fn resolve_effective_permission_policy( PermissionPolicySource::BuiltIn, ) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::managed_agents::global_config::GlobalAgentConfig; + + fn empty_record() -> ManagedAgentRecord { + serde_json::from_value(serde_json::json!({ + "pubkey": "abcd1234", + "name": "test", + "display_name": "Test", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://relay.example", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 300, + "idle_timeout_seconds": 900, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + })) + .expect("minimal ManagedAgentRecord") + } + + #[test] + fn test_per_agent_policy_beats_global_and_built_in() { + let mut record = empty_record(); + record.permission_policy = Some(PermissionPolicy::Allow); + let mut global = GlobalAgentConfig::default(); + global.permission_policy = Some(PermissionPolicy::Reject); + + let (policy, source) = resolve_effective_permission_policy(&record, &global); + assert_eq!(policy, PermissionPolicy::Allow); + assert_eq!(source, PermissionPolicySource::Agent); + } + + #[test] + fn test_global_policy_beats_built_in_when_no_per_agent() { + let mut record = empty_record(); + record.permission_policy = None; + let mut global = GlobalAgentConfig::default(); + global.permission_policy = Some(PermissionPolicy::Allow); + + let (policy, source) = resolve_effective_permission_policy(&record, &global); + assert_eq!(policy, PermissionPolicy::Allow); + assert_eq!(source, PermissionPolicySource::GlobalDefault); + } + + #[test] + fn test_built_in_used_when_neither_per_agent_nor_global_is_set() { + let mut record = empty_record(); + record.permission_policy = None; + let global = GlobalAgentConfig::default(); // permission_policy = None + + let (policy, source) = resolve_effective_permission_policy(&record, &global); + assert_eq!(policy, PermissionPolicy::Ask); // desktop_default + assert_eq!(source, PermissionPolicySource::BuiltIn); + } + + #[test] + fn test_per_agent_reject_beats_global_allow() { + let mut record = empty_record(); + record.permission_policy = Some(PermissionPolicy::Reject); + let mut global = GlobalAgentConfig::default(); + global.permission_policy = Some(PermissionPolicy::Allow); + + let (policy, source) = resolve_effective_permission_policy(&record, &global); + assert_eq!(policy, PermissionPolicy::Reject); + assert_eq!(source, PermissionPolicySource::Agent); + } +} diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index f1e47c091..adb232794 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -7,10 +7,10 @@ use super::agent_env::build_buzz_agent_provider_defaults; use crate::{ managed_agents::{ append_log_marker, known_acp_runtime, login_shell_path, managed_agent_log_path, - missing_command_message, normalize_agent_args, open_log_file, resolve_command, - spawn_key_refusal, KnownAcpRuntime, ManagedAgentPairRuntime, ManagedAgentRecord, - ManagedAgentRuntimeKey, ManagedAgentSummary, - permission_policy::resolve_effective_permission_policy, + missing_command_message, normalize_agent_args, open_log_file, + permission_policy::resolve_effective_permission_policy, resolve_command, spawn_key_refusal, + KnownAcpRuntime, ManagedAgentPairRuntime, ManagedAgentRecord, ManagedAgentRuntimeKey, + ManagedAgentSummary, }, util::now_iso, }; @@ -769,8 +769,7 @@ pub fn spawn_agent_child( // Inject BUZZ_ACP_PERMISSION_POLICY — resolved here so the running process // and the UI-visible setting are always in sync. - let (effective_permission_policy, _) = - resolve_effective_permission_policy(record, &global); + let (effective_permission_policy, _) = resolve_effective_permission_policy(record, &global); command.env( "BUZZ_ACP_PERMISSION_POLICY", effective_permission_policy.as_str(), diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index c3db5ae90..914568cf1 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -745,7 +745,8 @@ fn summary_fixture( respond_to: RespondTo::OwnerOnly, respond_to_allowlist: Vec::new(), permission_policy: crate::managed_agents::permission_policy::PermissionPolicy::Ask, - permission_policy_source: crate::managed_agents::permission_policy::PermissionPolicySource::BuiltIn, + permission_policy_source: + crate::managed_agents::permission_policy::PermissionPolicySource::BuiltIn, } } diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 11f68e8a5..67b5513bf 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -68,6 +68,7 @@ export const EMPTY_GLOBAL_CONFIG: GlobalAgentConfig = { provider: null, model: null, preferred_runtime: null, + permission_policy: null, }; const BAKED_STRUCTURED_KEYS = new Set([ diff --git a/desktop/src/features/agents/ui/AgentDefaultsEditor.tsx b/desktop/src/features/agents/ui/AgentDefaultsEditor.tsx index 69e8b3a5b..71b7b2544 100644 --- a/desktop/src/features/agents/ui/AgentDefaultsEditor.tsx +++ b/desktop/src/features/agents/ui/AgentDefaultsEditor.tsx @@ -17,7 +17,7 @@ import { getGlobalAgentConfig, setGlobalAgentConfig, } from "@/shared/api/tauriGlobalAgentConfig"; -import type { GlobalAgentConfig } from "@/shared/api/types"; +import type { GlobalAgentConfig, PermissionPolicy } from "@/shared/api/types"; import { getBakedBuildEnv, type BakedEnvEntry } from "@/shared/api/tauri"; import { globalAgentConfigQueryKey } from "@/features/agents/useGlobalAgentConfig"; import { @@ -294,6 +294,36 @@ export function AgentDefaultsEditor({ value={selectedRuntime?.id ?? ""} /> + {/* Fleet-wide permission policy default */} +
+ + +
{flatLayout ? ( {configFields ? ( diff --git a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx index 9e8c342f2..b71dbeb95 100644 --- a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx +++ b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx @@ -43,18 +43,35 @@ function permissionOutcomeTone(outcome: string): "approve" | "deny" | "cancel" { * Allow/Deny buttons for an actionable permission card. * Renders the agent's exact options as labeled buttons; a click sends the * `permission_decision` control event (fire-and-forget). + * + * On send failure (relay reject or non-`sent` delivery status), buttons are + * re-enabled so the user can retry. The harness's 300 s fail-closed timeout + * is the backstop for permanently lost frames. */ function PermissionDecisionButtons({ agentPubkey, + channelId, options, requestNonce, + deliveryFailed, }: { agentPubkey: string; + channelId: string; options: Array<{ optionId: string; kind: string; label?: string }>; requestNonce: string; + deliveryFailed?: boolean; }) { const [pending, setPending] = React.useState(null); + // Re-enable buttons when the reducer signals delivery failure (non-`sent` + // control_result status). The relay send succeeded but the harness couldn't + // route the click — the user should be able to retry. + React.useEffect(() => { + if (deliveryFailed) { + setPending(null); + } + }, [deliveryFailed]); + if (options.length === 0) { return null; } @@ -79,11 +96,12 @@ function PermissionDecisionButtons({ setPending(optionId); void sendPermissionDecision( agentPubkey, + channelId, requestNonce, optionId, ).catch(() => { - // Fire-and-forget: harness will time out if the frame is lost. - // Reset pending so the user can retry. + // Relay rejected the send. Re-enable so the user can retry; + // the harness's 300 s fail-closed timeout handles permanent loss. setPending(null); }); }} @@ -118,6 +136,7 @@ export function LifecycleActivity(props: ActivityRenderClassItemProps) { const requestNonce = props.item.requestNonce; const options = props.item.options ?? []; const authorizationReason = props.item.authorizationReason; + const deliveryFailed = props.item.deliveryFailed; return (
) : null} {/* Row 5: decision — only when outcome is resolved */} diff --git a/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs b/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs index b4a139eb0..476fd3326 100644 --- a/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs +++ b/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs @@ -2077,3 +2077,260 @@ test("buildTranscript session/new bare systemPrompt field takes precedence over "_meta.systemPrompt.append must not appear when bare field is present", ); }); + +// ── authorization envelope + nonce-keyed cards ──────────────────────────────── + +/** Build an acp_read permission event with a full authorization envelope. */ +function makePermissionRequestWithAuth( + seq, + requestId, + nonce, + { actionable = true, reason, turnId = "turn-1", channelId = "ch-1" } = {}, +) { + return { + seq, + timestamp: "2026-07-01T10:00:00.000Z", + kind: "acp_read", + agentIndex: 0, + channelId, + sessionId: "session-1", + turnId, + payload: { + jsonrpc: "2.0", + id: requestId, + method: "session/request_permission", + params: { + title: "Confirm push", + toolCallId: "tool-1", + options: [ + { optionId: "allow_once", kind: "allow_once", name: "Allow" }, + { optionId: "reject_once", kind: "reject_once", name: "Reject" }, + ], + }, + }, + authorization: { requestNonce: nonce, actionable, reason }, + }; +} + +test("buildTranscript_nonce_keyed_card_is_actionable_with_options", () => { + // An acp_read with an authorization envelope should produce one card + // keyed by nonce, with actionable=true and the parsed options attached. + const transcript = buildTranscript([ + makePermissionRequestWithAuth(1, "req-n1", "nonce-abc"), + ]); + + assert.equal(transcript.length, 1); + const item = transcript[0]; + assert.equal(item.type, "lifecycle"); + assert.equal(item.renderClass, "permission"); + assert.equal(item.requestNonce, "nonce-abc"); + assert.equal(item.actionable, true); + assert.equal(item.channelId, "ch-1"); + assert.ok(Array.isArray(item.options)); + assert.equal(item.options.length, 2); + assert.equal(item.options[0].optionId, "allow_once"); + // Card is keyed by nonce, not by turn. + assert.ok( + item.id.includes("nonce-abc"), + `expected nonce in id, got ${item.id}`, + ); +}); + +test("buildTranscript_actionable_false_envelope_produces_read_only_card", () => { + const transcript = buildTranscript([ + makePermissionRequestWithAuth(1, "req-n2", "nonce-readonly", { + actionable: false, + reason: "auto-rejected: reject policy", + }), + ]); + + assert.equal(transcript.length, 1); + const item = transcript[0]; + assert.equal(item.actionable, false); + assert.equal(item.authorizationReason, "auto-rejected: reject policy"); +}); + +test("buildTranscript_concurrent_requests_same_turn_produce_separate_cards", () => { + // Two permission requests in the same turn with different nonces must each + // get their own card — nonce is the unique key. + const transcript = buildTranscript([ + makePermissionRequestWithAuth(1, "req-c1", "nonce-c1", { + turnId: "turn-1", + }), + makePermissionRequestWithAuth(2, "req-c2", "nonce-c2", { + turnId: "turn-1", + }), + ]); + + // Two distinct cards. + const cards = transcript.filter((i) => i.renderClass === "permission"); + assert.equal(cards.length, 2, "expected two separate permission cards"); + const nonces = cards.map((c) => c.requestNonce).sort(); + assert.deepEqual(nonces, ["nonce-c1", "nonce-c2"]); + // Each card id is unique. + assert.notEqual(cards[0].id, cards[1].id); +}); + +test("buildTranscript_without_auth_envelope_falls_back_to_turn_keyed_card", () => { + // A permission request without an authorization envelope (legacy / reject + // policy path) still produces a card using the turn-based key. + const transcript = buildTranscript([ + { + seq: 1, + timestamp: "2026-07-01T10:00:00.000Z", + kind: "acp_read", + agentIndex: 0, + channelId: "ch-1", + sessionId: "session-1", + turnId: "turn-legacy", + payload: { + jsonrpc: "2.0", + id: "req-leg", + method: "session/request_permission", + params: { + title: "Confirm push", + toolCallId: "tool-1", + options: [ + { optionId: "allow_once", kind: "allow_once", name: "Allow" }, + ], + }, + }, + // No authorization field. + }, + ]); + + assert.equal(transcript.length, 1); + const item = transcript[0]; + assert.equal(item.renderClass, "permission"); + assert.equal(item.requestNonce, undefined); + assert.equal(item.actionable, undefined); + // Fall-back key uses turn id. + assert.ok( + item.id.includes("turn-legacy"), + `expected turn id in fallback key, got ${item.id}`, + ); +}); + +test("buildTranscript_uncertain_outcome_uses_pinned_copy", () => { + // The 'uncertain' terminal state must use the verbatim pinned copy, never + // "denied" or "failed closed". + const transcript = buildTranscript([ + makePermissionRequest(1, "req-unc"), + makePermissionResponse(2, "req-unc", "uncertain"), + ]); + + assert.equal(transcript.length, 1); + const item = transcript[0]; + assert.equal(item.renderClass, "permission"); + assert.match( + item.outcome ?? "", + /Approval outcome unknown.*agent process stopped/i, + "uncertain must use the pinned copy", + ); + // Must not use 'denied' or 'failed closed'. + assert.doesNotMatch(item.outcome ?? "", /denied/i); + assert.doesNotMatch(item.outcome ?? "", /failed closed/i); +}); + +test("buildTranscript_timed_out_outcome_renders_correctly", () => { + const transcript = buildTranscript([ + makePermissionRequest(1, "req-to"), + makePermissionResponse(2, "req-to", "timed_out"), + ]); + + const item = transcript[0]; + assert.equal(item.renderClass, "permission"); + assert.ok(item.outcome, "timed_out should produce an outcome string"); + assert.doesNotMatch(item.outcome ?? "", /Approved/i); +}); + +test("buildTranscript_nonce_card_channelId_is_threaded_from_event", () => { + // The channelId on the card must come from the event, not a hard-coded value, + // so PermissionDecisionButtons can pass it to sendPermissionDecision. + const transcript = buildTranscript([ + makePermissionRequestWithAuth(1, "req-ch", "nonce-ch", { + channelId: "specific-channel-id", + }), + ]); + + const item = transcript[0]; + assert.equal(item.channelId, "specific-channel-id"); +}); + +test("buildTranscript_control_result_non_sent_marks_card_delivery_failed", () => { + // A `control_result` with non-`sent` status must set deliveryFailed on the + // matching card so PermissionDecisionButtons can re-enable buttons for retry. + const nonce = "nonce-delivery-fail"; + const events = [ + // First: the permission request that creates the card. + makePermissionRequestWithAuth(1, "req-df", nonce), + // Second: a control_result with non-sent status. + { + seq: 2, + timestamp: "2026-07-01T10:00:01.000Z", + kind: "control_result", + agentIndex: 0, + channelId: "ch-1", + sessionId: "session-1", + turnId: "turn-1", + payload: { + type: "permission_decision", + status: "no_active_turn", + requestNonce: nonce, + optionId: "allow_once", + }, + }, + ]; + const transcript = buildTranscript(events); + + const card = transcript.find( + (i) => i.renderClass === "permission" && i.requestNonce === nonce, + ); + assert.ok(card, "permission card must exist"); + assert.equal( + card.deliveryFailed, + true, + "deliveryFailed must be set on non-sent control_result", + ); + // Card must still be actionable so the user can retry. + assert.equal( + card.actionable, + true, + "card must remain actionable after delivery failure", + ); +}); + +test("buildTranscript_control_result_sent_does_not_mark_delivery_failed", () => { + // A `control_result` with `sent` status must NOT set deliveryFailed — the + // click reached the harness successfully. + const nonce = "nonce-delivery-ok"; + const events = [ + makePermissionRequestWithAuth(1, "req-ok", nonce), + { + seq: 2, + timestamp: "2026-07-01T10:00:01.000Z", + kind: "control_result", + agentIndex: 0, + channelId: "ch-1", + sessionId: "session-1", + turnId: "turn-1", + payload: { + type: "permission_decision", + status: "sent", + requestNonce: nonce, + optionId: "allow_once", + }, + }, + ]; + const transcript = buildTranscript(events); + + const card = transcript.find( + (i) => i.renderClass === "permission" && i.requestNonce === nonce, + ); + assert.ok(card, "permission card must exist"); + assert.equal( + card.deliveryFailed, + undefined, + "deliveryFailed must not be set on sent control_result", + ); +}); diff --git a/desktop/src/features/agents/ui/agentSessionTranscript.ts b/desktop/src/features/agents/ui/agentSessionTranscript.ts index 1a352ed4e..f80306103 100644 --- a/desktop/src/features/agents/ui/agentSessionTranscript.ts +++ b/desktop/src/features/agents/ui/agentSessionTranscript.ts @@ -258,6 +258,13 @@ function describePermissionOutcome( if (outcome === "cancelled") { return "Cancelled"; } + if (outcome === "timed_out") { + return "Timed out"; + } + if (outcome === "uncertain") { + // Pinned verbatim copy — must never say "denied" or "failed closed". + return "Approval outcome unknown; agent process stopped before it could continue."; + } if (outcome === "selected" && optionId) { const kind = optionNames.get(optionId) ?? optionId; const isDenial = kind.startsWith("reject"); @@ -809,7 +816,13 @@ export function processTranscriptEvent( if (method === "session/request_permission") { const request = describePermissionRequest(payload); - const itemId = `permission:${ch}:${event.turnId ?? event.seq}`; + // Key by nonce when the authorization envelope is present — this gives + // each concurrent ACP request its own card. Fall back to the turn-based + // key for legacy/non-ask paths where no nonce is emitted. + const auth = event.authorization; + const itemId = auth?.requestNonce + ? `permission:${ch}:nonce:${auth.requestNonce}` + : `permission:${ch}:${event.turnId ?? event.seq}`; upsertLifecycleItem( d, itemId, @@ -825,7 +838,6 @@ export function processTranscriptEvent( // Attach authorization-envelope fields to the item. The `authorization` // object is on the ObserverEvent itself (not the payload — payloads are // raw ACP with no `_buzz` wrapper). - const auth = event.authorization; if (auth) { const existing = d.itemsById.get(itemId); if (existing?.type === "lifecycle") { @@ -837,7 +849,7 @@ export function processTranscriptEvent( options: request.options, }); } - // Also index by nonce so control_result frames can retire the card. + // Index by nonce so acp_write terminal frames can retire the card. d.pendingPermissionsByNonce = new Map(d.pendingPermissionsByNonce); d.pendingPermissionsByNonce.set(auth.requestNonce, itemId); } @@ -1181,20 +1193,31 @@ export function processTranscriptEvent( // not a terminal outcome. Status values are: sent | no_active_turn | // channel_full | channel_closed | no_channel. // - // A non-"sent" status means the click did not reach the harness — the card - // stays actionable so the user can retry. Terminal outcomes (applied, - // denied, timed_out, cancelled, uncertain) arrive as enveloped acp_write - // frames correlated by requestNonce (see the acp_write branch above). - // That path will be wired once Thufir's review of Duncan's contract lands. + // A non-"sent" status means the click did not reach the harness — mark the + // card with `deliveryFailed = true` so buttons re-enable for retry. Terminal + // outcomes (applied, denied, timed_out, cancelled, uncertain) arrive as + // enveloped acp_write frames correlated by requestNonce. const payload = asRecord(event.payload); const frameType = asString(payload.type); if (frameType === "permission_decision") { const deliveryStatus = asString(payload.status); - // If delivery failed, the PermissionDecisionButtons component handles - // button-level pending-state reset via its own catch handler. No card - // retirement here — the card stays actionable until a terminal acp_write - // frame confirms the outcome. - void deliveryStatus; // acknowledged; no card mutation on delivery results + if (deliveryStatus !== "sent") { + // Delivery failed — find the card by nonce and mark it retryable. + const nonce = asString(payload.requestNonce); + if (nonce) { + const itemId = d.pendingPermissionsByNonce.get(nonce); + if (itemId) { + const existing = d.itemsById.get(itemId); + if ( + existing?.type === "lifecycle" && + existing.renderClass === "permission" && + existing.actionable + ) { + replaceItem(d, itemId, { ...existing, deliveryFailed: true }); + } + } + } + } } } diff --git a/desktop/src/features/agents/ui/agentSessionTypes.ts b/desktop/src/features/agents/ui/agentSessionTypes.ts index 1ae5ec87a..ccdc32daa 100644 --- a/desktop/src/features/agents/ui/agentSessionTypes.ts +++ b/desktop/src/features/agents/ui/agentSessionTypes.ts @@ -146,6 +146,13 @@ export type TranscriptItem = * button rendering. */ options?: Array<{ optionId: string; kind: string; label?: string }>; + /** + * Set to `true` when a `control_result` frame indicates that the last + * `permission_decision` click was not delivered to the harness (status + * was non-`sent`). The `PermissionDecisionButtons` component uses this + * to re-enable buttons so the user can retry without reloading. + */ + deliveryFailed?: boolean; } & TranscriptItemIdentity) | ({ id: string; diff --git a/desktop/src/shared/api/agentControl.ts b/desktop/src/shared/api/agentControl.ts index 888fb537f..47db543d8 100644 --- a/desktop/src/shared/api/agentControl.ts +++ b/desktop/src/shared/api/agentControl.ts @@ -35,18 +35,22 @@ export async function switchManagedAgentModel( * is fire-and-forget: the harness receives it via the observer control channel * and updates the permission card asynchronously via a `control_result` frame. * - * @param pubkey - Agent's public key (hex or npub). - * @param nonce - `requestNonce` from the `authorization` envelope on the - * corresponding `acp_read` permission frame. - * @param optionId - The chosen option's `optionId` (e.g. `"allow_once"`). + * @param pubkey - Agent's public key (hex or npub). + * @param channelId - The channel from which the permission request was issued. + * The harness validates this before looking up the nonce. + * @param nonce - `requestNonce` from the `authorization` envelope on the + * corresponding `acp_read` permission frame. + * @param optionId - The chosen option's `optionId` (e.g. `"allow_once"`). */ export async function sendPermissionDecision( pubkey: string, + channelId: string, nonce: string, optionId: string, ): Promise { await sendAgentObserverControl(pubkey, { type: "permission_decision", + channelId, requestNonce: nonce, optionId, }); diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index 4dad0db01..3c2efd602 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -1050,6 +1050,12 @@ export type GlobalAgentConfig = { model: string | null; /** Preferred ACP runtime for agents without a persona-specific runtime. */ preferred_runtime: string | null; + /** + * Fleet-wide permission policy fallback. Null = no fleet default; agents + * without a per-agent policy use the built-in desktop default (`ask`). + * Mirrors `GlobalAgentConfig.permission_policy` in Rust. + */ + permission_policy: PermissionPolicy | null; }; /** diff --git a/docs/nips/NIP-AO.md b/docs/nips/NIP-AO.md index b31322233..f0bfa4917 100644 --- a/docs/nips/NIP-AO.md +++ b/docs/nips/NIP-AO.md @@ -175,8 +175,14 @@ Cancel the in-flight agent turn for the given channel. #### `switch_model` -Switch the active model for the agent session in the given channel. Takes effect on -the next turn; the current turn is unaffected. +Switch the active model for the agent session in the given channel. + +- **Busy turn:** delivers `ControlSignal::SwitchModel` over the per-turn oneshot, + which triggers the harness to cancel the current turn and requeue with the new model. + If the oneshot is already consumed (a prior cancel/interrupt is in flight), the + switch cannot land and the current turn is left to complete with the old model. +- **Idle session:** validates the model against the cached catalog and, if valid, + invalidates and reapplies the agent's model config immediately. ```json { @@ -229,7 +235,7 @@ event to confirm receipt. This is an `acp_read`-style telemetry frame (kind = **`switch_model`:** ```json -{ "type": "switch_model", "status": "queued" | "no_active_session" | ..., "modelId": "..." } +{ "type": "switch_model", "status": "sent" | "turn_ending" | "switched" | "unsupported_model" | "no_active_turn", "modelId": "..." } ``` **`permission_decision`:** @@ -497,16 +503,21 @@ of decrypted payloads and MUST NOT log them at INFO level or above. "turnId": "turn-xyz", "authorization": { "requestNonce": "a9f3b2c1d4e5...", - "actionable": true + "actionable": false, + "reason": "applied" }, "payload": { "jsonrpc": "2.0", "id": "req-17", - "result": { "optionId": "opt-allow" } + "result": { "outcome": { "outcome": "selected", "optionId": "opt-allow" } } } } ``` +Note: `actionable` is `false` on the `acp_write` telemetry frame — the decision has +been applied and the card is no longer actionable. `reason: "applied"` is the +standard terminal annotation for a successfully delivered decision. + ## Reference Implementation [block/buzz PR #4938](https://github.com/block/buzz/pull/4938)