mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
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 <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
+386
-201
@@ -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<serde_json::Value>,
|
||||
/// 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::<serde_json::Value>(&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::<PermissionDecision>(PERMISSION_MAP_CAP);
|
||||
let (perm_tx, perm_rx) = tokio::sync::mpsc::channel::<PermissionDecision>(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::<PermissionDecision>(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]
|
||||
|
||||
@@ -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<PermissionMode>,
|
||||
@@ -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),
|
||||
|
||||
@@ -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<crate::managed_agents::permission_policy::PermissionPolicy>,
|
||||
) -> 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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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([
|
||||
|
||||
@@ -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 ?? ""}
|
||||
/>
|
||||
</div>
|
||||
{/* Fleet-wide permission policy default */}
|
||||
<div className="space-y-1.5">
|
||||
<label
|
||||
className="text-sm font-medium text-foreground"
|
||||
htmlFor="global-agent-default-permission-policy"
|
||||
>
|
||||
Default permission policy
|
||||
</label>
|
||||
<select
|
||||
className="w-full rounded-md border border-input bg-background px-3 py-1.5 text-sm shadow-sm focus:outline-none focus:ring-1 focus:ring-ring"
|
||||
data-testid="global-agent-default-permission-policy"
|
||||
id="global-agent-default-permission-policy"
|
||||
value={config.permission_policy ?? ""}
|
||||
onChange={(e) => {
|
||||
const val = e.target.value;
|
||||
handleConfigChange({
|
||||
...config,
|
||||
permission_policy:
|
||||
val === "" ? null : (val as PermissionPolicy),
|
||||
});
|
||||
}}
|
||||
>
|
||||
<option value="">Inherit built-in (ask)</option>
|
||||
<option value="ask">Ask — show Allow/Deny card</option>
|
||||
<option value="allow">
|
||||
Allow — auto-approve (explicit opt-in)
|
||||
</option>
|
||||
<option value="reject">Reject — auto-deny</option>
|
||||
</select>
|
||||
</div>
|
||||
{flatLayout ? (
|
||||
<AnimatePresence initial={false}>
|
||||
{configFields ? (
|
||||
|
||||
@@ -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<string | null>(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 (
|
||||
<div
|
||||
className="rounded-md border border-amber-500/20 bg-amber-500/5 px-2 py-1.5 text-left text-xs text-amber-700 dark:text-amber-400"
|
||||
@@ -144,8 +163,10 @@ export function LifecycleActivity(props: ActivityRenderClassItemProps) {
|
||||
{actionable && requestNonce && !outcome ? (
|
||||
<PermissionDecisionButtons
|
||||
agentPubkey={props.agentPubkey}
|
||||
channelId={props.item.channelId ?? ""}
|
||||
options={options}
|
||||
requestNonce={requestNonce}
|
||||
deliveryFailed={deliveryFailed}
|
||||
/>
|
||||
) : null}
|
||||
{/* Row 5: decision — only when outcome is resolved */}
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<void> {
|
||||
await sendAgentObserverControl(pubkey, {
|
||||
type: "permission_decision",
|
||||
channelId,
|
||||
requestNonce: nonce,
|
||||
optionId,
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
|
||||
/**
|
||||
|
||||
+16
-5
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user