From 73f2ffed832a7e6efa75b6123914d58815ee492c Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 7 Aug 2026 13:07:40 -0400 Subject: [PATCH] fix(buzz-acp): thread single nonce through sync denial paths; upgrade two tests to pipe-level proof Thread one nonce through each synchronous denial path so the acp_read and acp_write telemetry frames always carry the same nonce. Before this change, emit_permission_read_non_actionable generated its own nonce internally while the caller passed a different nonce to finish_permission_sync, producing one logical challenge/answer pair with two different nonces. Desktop's nonce-only correlation rule left the read card live because the write could never find it by nonce. Fix: accept nonce as a parameter in emit_permission_read_non_actionable (removing its internal new_permission_nonce() call) and drop the now-dead caller_will_emit_read parameter from handle_permission_request and emit_permission_read_with_nonce. Upgrade two tests from telemetry-proxy assertions to direct pipe proofs: - ask_permission_entry_deadline_equal_to_loop_hard_deadline_writes_denial_before_exit: replace observer telemetry assertion with a capture script that reads the denial line from child stdin NDJSON and parses the wire response. - cancel_first_write_fails_stops_immediately_no_second_write: add a write-attempt counter (Arc in write_ndjson_inner) to assert exactly ONE attempt was made and the loop stopped, not just that no successful writes occurred. Add Rust tests proving the nonce is shared: - sync_denial_malformed_options_read_and_write_carry_same_nonce - sync_denial_preflight_failure_read_and_write_carry_same_nonce Add TypeScript reducer tests: - buildTranscript_sync_denial_write_with_matching_nonce_retires_card - buildTranscript_sync_denial_write_with_mismatched_nonce_leaves_card_live Update NIP-AO schema prose and observer.rs field comment to explicitly document the permission_terminal exception to the authorization-only-on- acp_read/acp_write rule. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/acp.rs | 313 +++++++++++++----- crates/buzz-acp/src/observer.rs | 4 +- .../agents/ui/agentSessionTranscript.test.mjs | 73 ++++ docs/nips/NIP-AO.md | 6 +- 4 files changed, 315 insertions(+), 81 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 61fcf3c92..ebe1d6039 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -307,6 +307,11 @@ pub struct AcpClient { /// deltas. Both goose and buzz-agent emit this notification; goose gates /// on client capability advertisement, buzz-agent emits unconditionally. goose_usage: UsageTracker, + /// Test-only: count every write attempt (before the actual I/O). Incremented + /// at the top of `write_ndjson_inner` so callers can assert "exactly N attempts" + /// independently of whether the writes succeeded. + #[cfg(test)] + write_attempt_count: Option>, } /// Recursively merge `overlay` into `base`, with `overlay` winning on scalar/shape @@ -656,6 +661,8 @@ impl AcpClient { steering_supported: false, steer_rx: None, goose_usage: UsageTracker::default(), + #[cfg(test)] + write_attempt_count: None, }) } @@ -697,6 +704,19 @@ impl AcpClient { self.observer_context = context; } + /// Install a write-attempt counter for tests. + /// + /// When set, every call to `write_ndjson_inner` (regardless of success or failure) + /// atomically increments the counter before attempting the I/O. Tests can use this + /// to assert "exactly one attempt was made" even when the write fails. + #[cfg(test)] + pub fn set_write_attempt_count( + &mut self, + counter: std::sync::Arc, + ) { + self.write_attempt_count = Some(counter); + } + /// Return a clone of the observer handle, if attached. pub(crate) fn observer_handle(&self) -> Option { self.observer.clone() @@ -1301,6 +1321,10 @@ impl AcpClient { value: &serde_json::Value, emit_observe: bool, ) -> Result<(), AcpError> { + #[cfg(test)] + if let Some(counter) = &self.write_attempt_count { + counter.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } 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 { @@ -1650,12 +1674,12 @@ impl AcpClient { ); let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(PERMISSION_ASK_TIMEOUT_SECS); - let _ = self.handle_permission_request(&msg, true, deadline).await; + let _ = self.handle_permission_request(&msg, 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?; + self.handle_permission_request(&msg, deadline).await?; } } other => { @@ -2307,12 +2331,7 @@ impl AcpClient { self.handle_goose_usage_update(&msg); } "session/request_permission" => { - self.handle_permission_request( - &msg, - is_ask_permission_request, - hard_deadline, - ) - .await?; + self.handle_permission_request(&msg, hard_deadline).await?; } other => { // If the unknown message has an id, it's a request expecting a reply. @@ -2525,11 +2544,6 @@ impl AcpClient { pub(crate) async fn handle_permission_request( &mut self, msg: &serde_json::Value, - // When `true`, caller has NOT yet emitted acp_read for this message — - // this method emits it (enveloped) for permission frames under `ask`. - // When `false` (read_until_response, non-idle path), the caller already - // emitted it; we must not double-emit. - caller_will_emit_read: bool, // Hard deadline for the current turn. Used to bound per-request ask timeouts. hard_deadline: tokio::time::Instant, ) -> Result { @@ -2546,7 +2560,7 @@ impl AcpClient { let reason = "missing or non-array options field"; tracing::warn!(target: "acp::permission", "{reason}, id={id}"); let nonce = new_permission_nonce(); - self.emit_permission_read_non_actionable(&id, msg, reason, caller_will_emit_read); + self.emit_permission_read_non_actionable(&id, msg, &nonce, reason); let response = permission_denial_response(&id, &[])?; self.finish_permission_sync(&id, &nonce, "rejected", response) .await?; @@ -2587,7 +2601,7 @@ impl AcpClient { if let Err(reason) = preflight_result { tracing::warn!(target: "acp::permission", "preflight failed: {reason}, id={id}"); let nonce = new_permission_nonce(); - self.emit_permission_read_non_actionable(&id, msg, &reason, caller_will_emit_read); + self.emit_permission_read_non_actionable(&id, msg, &nonce, &reason); let response = permission_denial_response(&id, &options)?; self.finish_permission_sync(&id, &nonce, "rejected", response) .await?; @@ -2617,7 +2631,6 @@ impl AcpClient { &nonce, false, Some("policy=reject"), - caller_will_emit_read, ); let response = permission_denial_response(&id, &options)?; @@ -2646,7 +2659,6 @@ impl AcpClient { &nonce, false, Some("policy=allow; auto-approved"), - caller_will_emit_read, ); let response = permission_response_selected(&id, &option_id); self.finish_permission_sync(&id, &nonce, "allowed", response) @@ -2667,7 +2679,6 @@ impl AcpClient { &nonce, false, Some(&format!("policy=allow; fail closed: {reason}")), - caller_will_emit_read, ); let response = permission_denial_response(&id, &options)?; self.finish_permission_sync(&id, &nonce, "allow_failed_closed", response) @@ -2700,7 +2711,6 @@ impl AcpClient { &nonce, false, Some("policy=ask unavailable (no observer/owner); downgraded to reject"), - caller_will_emit_read, ); let response = permission_denial_response(&id, &options)?; self.finish_permission_sync(&id, &nonce, "rejected", response) @@ -2751,18 +2761,22 @@ impl AcpClient { } /// Emit a non-actionable `acp_read` authorization frame for a permission request. + /// + /// The caller is responsible for generating the nonce and passing the same + /// value to the corresponding `finish_permission_sync` call so that both the + /// `acp_read` and `acp_write` telemetry frames share one nonce — required for + /// Desktop's nonce-only correlation to retire the card. fn emit_permission_read_non_actionable( &self, id: &serde_json::Value, msg: &serde_json::Value, + nonce: &str, reason: &str, - _caller_will_emit_read: bool, ) { - let nonce = new_permission_nonce(); self.observe_authorized( "acp_read", AuthorizationEnvelope { - request_nonce: nonce, + request_nonce: nonce.to_string(), actionable: false, reason: Some(reason.to_string()), }, @@ -2772,10 +2786,6 @@ impl AcpClient { } /// Emit an `acp_read` with an authorization envelope. - /// - /// When `caller_will_emit_read` is `false` the caller already emitted the - /// raw `acp_read`; we emit only the enveloped version. When `true` we emit - /// the enveloped version (the caller suppresses its normal emit). fn emit_permission_read_with_nonce( &self, _id: &serde_json::Value, @@ -2783,7 +2793,6 @@ impl AcpClient { nonce: &str, actionable: bool, reason: Option<&str>, - _caller_will_emit_read: bool, ) { self.observe_authorized( "acp_read", @@ -5861,9 +5870,7 @@ mod tests { ); let msg = perm_request(1, default_opts()); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; // Must succeed (Ok) — denial was written and the call itself doesn't error. assert!( result.is_ok(), @@ -6073,9 +6080,7 @@ mod tests { // One more request with a new id → must be denied. let msg = perm_request(99, default_opts()); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; assert!( result.is_ok(), "map-at-cap must not propagate Err, got {result:?}" @@ -6196,9 +6201,7 @@ mod tests { let msg = perm_request(1, default_opts()); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; // Denial was written — Ok(true) means caller should suppress generic emit. assert!( result.is_ok(), @@ -6221,9 +6224,7 @@ mod tests { let msg = perm_request(2, default_opts()); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; assert!(result.is_ok()); assert!(client.pending_permissions.is_empty()); } @@ -6413,7 +6414,7 @@ mod tests { let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(300); let msg = perm_request(i, default_opts()); client - .handle_permission_request(&msg, true, hard_deadline) + .handle_permission_request(&msg, hard_deadline) .await .expect("ask registration must succeed"); // Capture the nonce that was bound to this entry. @@ -6562,7 +6563,7 @@ mod tests { let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(PERMISSION_ASK_TIMEOUT_SECS + 10); client - .handle_permission_request(&msg, true, hard_deadline) + .handle_permission_request(&msg, hard_deadline) .await .expect("ask registration must succeed"); assert_eq!(client.pending_permissions.len(), 1, "entry registered"); @@ -6643,7 +6644,7 @@ mod tests { // Use the same deadline for both the entry and the hard deadline. let msg = perm_request(1, default_opts()); client - .handle_permission_request(&msg, true, perm_deadline) + .handle_permission_request(&msg, perm_deadline) .await .expect("ask registration must succeed"); assert_eq!(client.pending_permissions.len(), 1, "entry registered"); @@ -6733,9 +6734,21 @@ mod tests { /// The pre-select check must NOT return `HardTimeout` before processing the /// expired entry — it must write the fail-closed denial first, THEN return /// `HardTimeout`. This test proves the fix: equal deadlines → denial written. + /// + /// Wire-level proof: the denial line is captured from child stdin NDJSON and + /// parsed to confirm it contains exactly one `timed_out` response for id=1 + /// before `HardTimeout` is returned. #[tokio::test(start_paused = true)] async fn ask_permission_entry_deadline_equal_to_loop_hard_deadline_writes_denial_before_exit() { - let mut client = spawn_script("sleep 600").await; + // Script: read one line from stdin (the timed-out denial), save it to a + // capture file, then sleep forever so the loop can advance to HardTimeout. + let capture_file = + std::env::temp_dir().join(format!("buzz-acp-eq-{}.ndjson", uuid::Uuid::new_v4())); + let script = format!( + "read -r line; printf '%s' \"$line\" > {capture}; sleep 600", + capture = capture_file.display(), + ); + let mut client = spawn_script(&script).await; let config = ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, None).unwrap(); client.set_permission_config(config); client.set_owner_pubkey_known(true); @@ -6751,7 +6764,7 @@ mod tests { let msg = perm_request(1, default_opts()); client - .handle_permission_request(&msg, true, shared_deadline) + .handle_permission_request(&msg, shared_deadline) .await .expect("ask registration must succeed"); assert_eq!(client.pending_permissions.len(), 1, "entry registered"); @@ -6786,27 +6799,47 @@ mod tests { }; } - // The entry must have been processed (removed) and a timed_out denial written. + // The entry must have been processed (removed). assert!( !client.pending_permissions.contains_key("1"), "entry must be removed after equality deadline fires" ); - let events = obs.snapshot(); - let timeout_writes: Vec<_> = events - .iter() - .filter(|e| { - e.kind == "acp_write" - && e.authorization - .as_ref() - .map(|a| a.reason.as_deref() == Some("timed_out")) - .unwrap_or(false) - }) - .collect(); + // Wire-level proof: read what the harness actually wrote on the pipe. + let wire_line = tokio::task::spawn_blocking({ + let capture_file = capture_file.clone(); + move || { + for _ in 0..40 { + if let Ok(s) = std::fs::read_to_string(&capture_file) { + if !s.is_empty() { + return s; + } + } + std::thread::sleep(std::time::Duration::from_millis(50)); + } + String::new() + } + }) + .await + .expect("spawn_blocking failed"); + + let _ = std::fs::remove_file(&capture_file); + + assert!( + !wire_line.is_empty(), + "harness must write a timed-out denial on the pipe before HardTimeout" + ); + let wire_json: serde_json::Value = + serde_json::from_str(&wire_line).expect("wire denial must be valid JSON"); assert_eq!( - timeout_writes.len(), - 1, - "exactly one timed_out denial must be written before HardTimeout return; got: {timeout_writes:?}" + wire_json["id"], + serde_json::json!(1), + "wire denial id must match the permission request id=1" + ); + // The response must be a valid permission result (non-null result field). + assert!( + !wire_json["result"].is_null(), + "wire denial must carry a result field; got {wire_json}" ); } @@ -6959,7 +6992,7 @@ mod tests { let hard = tokio::time::Instant::now() + std::time::Duration::from_secs(300); let msg = perm_request(i + 100, default_opts()); - let result = client.handle_permission_request(&msg, true, hard).await; + let result = client.handle_permission_request(&msg, hard).await; assert!( result.as_ref().is_ok_and(|v| *v), "request {i} must register successfully (capacity not exhausted), got: {result:?}" @@ -7109,9 +7142,7 @@ mod tests { let msg = perm_request(42, default_opts()); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; assert!( result.is_ok(), "ask must return Ok to suppress generic emit" @@ -7209,9 +7240,10 @@ mod tests { /// and the cancel loop must return `Err(PermissionPoisoned)` immediately — zero /// bytes are written for the second entry. /// - /// Observable: exactly ONE `permission_terminal` uncertain event is emitted - /// (for the first entry whose write failed) and ZERO `acp_write` events (no - /// successful cancel write for either entry). + /// Uses an instrumented write-attempt counter to assert exactly ONE attempt was + /// made (the first, which failed), not just that no successful writes occurred. + /// The counter distinguishes "stopped after first attempt" from "tried all and + /// all failed" — the latter would allow the loop to continue past the poison. #[tokio::test] async fn cancel_first_write_fails_stops_immediately_no_second_write() { // Script: exit immediately without reading stdin. @@ -7226,12 +7258,17 @@ mod tests { let (_tx, perm_rx) = tokio::sync::mpsc::channel::(8); client.install_permission_decision_rx(perm_rx); + // Install the write-attempt counter BEFORE registration so all writes + // (including the registration acks and the cancel responses) are counted. + let attempt_counter = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + client.set_write_attempt_count(attempt_counter.clone()); + // Register two Pending entries. let hard = tokio::time::Instant::now() + std::time::Duration::from_secs(300); for i in 0..2u64 { let msg = perm_request(i, default_opts()); client - .handle_permission_request(&msg, true, hard) + .handle_permission_request(&msg, hard) .await .expect("ask registration must succeed"); } @@ -7245,6 +7282,9 @@ mod tests { // Wait briefly for the script to exit and close its stdin read-end. tokio::time::sleep(std::time::Duration::from_millis(100)).await; + // Snapshot the attempt count before cancel so we can count only cancel writes. + let attempts_before_cancel = attempt_counter.load(std::sync::atomic::Ordering::Relaxed); + // Cancel: the first finish_permission() write must fail (BrokenPipe), // poison the process, and return Err(PermissionPoisoned) immediately. let err = client @@ -7260,7 +7300,18 @@ mod tests { "poisoned flag must be set after cancel write failure" ); - // No successful cancel writes — the first write failed. + // Exactly ONE write attempt during the cancel phase. + // If the loop stopped after the first failed attempt, count = 1. + // If it continued and tried the second entry, count = 2. + let attempts_during_cancel = + attempt_counter.load(std::sync::atomic::Ordering::Relaxed) - attempts_before_cancel; + assert_eq!( + attempts_during_cancel, 1, + "cancel must attempt exactly one write (for the first entry) then stop; \ + attempted {attempts_during_cancel} times" + ); + + // No successful cancel writes. let events = obs.snapshot(); let cancel_writes = events .iter() @@ -7277,8 +7328,7 @@ mod tests { "no successful cancel writes must be emitted when first write fails; got {cancel_writes}" ); - // At least one `permission_terminal` uncertain event must be emitted - // (for the failed entry). + // At least one `permission_terminal` uncertain event must be emitted. let uncertain_events = events .iter() .filter(|e| { @@ -7385,9 +7435,7 @@ mod tests { let msg = perm_request(7, default_opts()); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; // Reject is synchronous — no pending entry, Ok(true) to suppress generic emit. assert!(result.is_ok(), "reject must return Ok"); assert!(result.unwrap(), "reject must return Ok(true)"); @@ -7415,9 +7463,7 @@ mod tests { let msg = perm_request(8, default_opts()); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; assert!(result.is_ok(), "allow auto-select must return Ok"); assert!(result.unwrap(), "allow auto-select must return Ok(true)"); // No pending entries — handled synchronously. @@ -7432,9 +7478,7 @@ mod tests { // Only reject_once offered — allow policy must fail closed. let msg = perm_request(9, &[("opt-r", "reject_once", "Reject")]); let hard_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(30); - let result = client - .handle_permission_request(&msg, true, hard_deadline) - .await; + let result = client.handle_permission_request(&msg, hard_deadline).await; // Fail closed: denial written, Ok(true) returned. assert!(result.is_ok(), "fail-closed allow must return Ok"); assert!(result.unwrap(), "fail-closed allow must return Ok(true)"); @@ -7584,4 +7628,115 @@ mod tests { assert_eq!(PermissionMode::Auto.as_wire_str(), "auto"); assert!(!PermissionMode::Auto.is_default()); } + + /// Synchronous denial (missing options): `acp_read` and `acp_write` must share one nonce. + /// + /// Before the nonce-threading fix, `emit_permission_read_non_actionable` generated + /// its own nonce independently of the nonce passed to `finish_permission_sync`, so + /// the two telemetry frames carried different nonces. Desktop's nonce-only rule then + /// left the read card live because the write could never find it. + #[tokio::test] + async fn sync_denial_malformed_options_read_and_write_carry_same_nonce() { + let mut client = spawn_inert_client().await; + client.set_permission_config( + ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, None).unwrap(), + ); + client.set_owner_pubkey_known(true); + let obs = crate::observer::ObserverHandle::in_process(); + client.set_observer(Some(obs.clone()), 0); + + // Request with no options field — triggers the malformed path. + let msg = serde_json::json!({ + "jsonrpc": "2.0", + "id": 77, + "method": "session/request_permission", + "params": { + "sessionId": "sess", + "subject": "read a file" + // "options" deliberately omitted + } + }); + let hard = tokio::time::Instant::now() + std::time::Duration::from_secs(300); + client + .handle_permission_request(&msg, hard) + .await + .expect("malformed denial must not error"); + + let events = obs.snapshot(); + + let read_nonce = events + .iter() + .find(|e| e.kind == "acp_read" && e.authorization.is_some()) + .and_then(|e| e.authorization.as_ref()) + .map(|a| a.request_nonce.clone()) + .expect("acp_read with authorization must be emitted"); + + let write_nonce = events + .iter() + .find(|e| e.kind == "acp_write" && e.authorization.is_some()) + .and_then(|e| e.authorization.as_ref()) + .map(|a| a.request_nonce.clone()) + .expect("acp_write with authorization must be emitted"); + + assert_eq!( + read_nonce, write_nonce, + "acp_read and acp_write must carry the same nonce so Desktop can retire the card; \ + read={read_nonce}, write={write_nonce}" + ); + } + + /// Synchronous denial (preflight failure): `acp_read` and `acp_write` must share one nonce. + #[tokio::test] + async fn sync_denial_preflight_failure_read_and_write_carry_same_nonce() { + let mut client = spawn_inert_client().await; + client.set_permission_config( + ResolvedPermissionConfig::resolve(PermissionPolicy::Ask, None).unwrap(), + ); + client.set_owner_pubkey_known(true); + let obs = crate::observer::ObserverHandle::in_process(); + client.set_observer(Some(obs.clone()), 0); + + // Oversize subject triggers admission preflight failure. + let oversize_subject = "x".repeat(OBSERVER_MAX_PLAINTEXT_LEN + 1); + let msg = serde_json::json!({ + "jsonrpc": "2.0", + "id": 88, + "method": "session/request_permission", + "params": { + "sessionId": "sess", + "subject": oversize_subject, + "options": [ + {"optionId": "opt-allow", "kind": "allow_once", "name": "Allow"}, + {"optionId": "opt-deny", "kind": "reject_once", "name": "Deny"} + ] + } + }); + let hard = tokio::time::Instant::now() + std::time::Duration::from_secs(300); + client + .handle_permission_request(&msg, hard) + .await + .expect("preflight denial must not error"); + + let events = obs.snapshot(); + + let read_nonce = events + .iter() + .find(|e| e.kind == "acp_read" && e.authorization.is_some()) + .and_then(|e| e.authorization.as_ref()) + .map(|a| a.request_nonce.clone()) + .expect("acp_read with authorization must be emitted"); + + let write_nonce = events + .iter() + .find(|e| e.kind == "acp_write" && e.authorization.is_some()) + .and_then(|e| e.authorization.as_ref()) + .map(|a| a.request_nonce.clone()) + .expect("acp_write with authorization must be emitted"); + + assert_eq!( + read_nonce, write_nonce, + "acp_read and acp_write must carry the same nonce so Desktop can retire the card; \ + read={read_nonce}, write={write_nonce}" + ); + } } diff --git a/crates/buzz-acp/src/observer.rs b/crates/buzz-acp/src/observer.rs index 04ef08379..2f35e3e10 100644 --- a/crates/buzz-acp/src/observer.rs +++ b/crates/buzz-acp/src/observer.rs @@ -94,7 +94,9 @@ pub struct ObserverEvent { #[serde(skip_serializing_if = "Option::is_none")] pub started_at: Option, /// Authorization envelope — present only on permission `acp_read` / - /// `acp_write` frames. `None` on all other event kinds. + /// `acp_write` frames, and on the observer-only `permission_terminal` frame + /// (which carries `reason = "uncertain"` and is never sent on the ACP wire). + /// `None` on all other event kinds. #[serde(skip_serializing_if = "Option::is_none")] pub authorization: Option, /// Raw or semantic event payload. diff --git a/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs b/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs index 6fe71db58..70ddb3879 100644 --- a/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs +++ b/desktop/src/features/agents/ui/agentSessionTranscript.test.mjs @@ -2726,3 +2726,76 @@ test("buildTranscript_permission_terminal_in_archive_replay_retires_card", () => "nonce index must be clean after archive replay", ); }); + +// ─── Sync denial: acp_write with matching nonce retires the acp_read card ───── +// These tests verify the nonce-threading fix: before the fix, sync denial paths +// generated two different nonces (one for acp_read, a second for acp_write), +// so Desktop's nonce-only correlation could never find the read card. + +test("buildTranscript_sync_denial_write_with_matching_nonce_retires_card", () => { + // Non-actionable acp_read (sync denial — reject/preflight path) followed by + // acp_write carrying the SAME nonce. The write must retire the card and clear + // both indexes. + const nonce = "nonce-sync-deny"; + const events = [ + // Non-actionable read: card is created but not user-interactive. + makePermissionRequestWithAuth(1, "req-sd", nonce, { + actionable: false, + reason: "rejected", + }), + // Write with the same nonce — this is the fix under test. + makePermissionWriteWithNonce(2, "req-sd", nonce, "rejected", "reject_once"), + ]; + const state = buildTranscriptState(events); + + // Card must be retired (not actionable, outcome set). + const card = state.items.find( + (i) => i.renderClass === "permission" && i.requestNonce === nonce, + ); + assert.ok(card, "permission card must exist after sync denial"); + assert.equal( + card.actionable, + false, + "card must be non-actionable after matching-nonce acp_write", + ); + + // Both indexes must be cleared. + assert.ok( + !state.pendingPermissionsByNonce.has(nonce), + "nonce index must be cleared after matching-nonce acp_write", + ); + const legacyKey = `ch-1:session-1:turn-1:${JSON.stringify("req-sd")}`; + assert.ok( + !state.pendingPermissions.has(legacyKey), + "legacy index must be cleared after matching-nonce acp_write", + ); +}); + +test("buildTranscript_sync_denial_write_with_mismatched_nonce_leaves_card_live", () => { + // Regression guard: if the nonce on the acp_write does NOT match the acp_read, + // Desktop's nonce-only rule must drop the write — the read card stays live. + // (This is the broken-before-fix scenario the nonce-threading corrects.) + const readNonce = "nonce-read-mismatch"; + const writeNonce = "nonce-write-different"; // intentionally different + const events = [ + makePermissionRequestWithAuth(1, "req-mm", readNonce, { + actionable: false, + reason: "rejected", + }), + makePermissionWriteWithNonce( + 2, + "req-mm", + writeNonce, + "rejected", + "reject_once", + ), + ]; + const state = buildTranscriptState(events); + + // The write carried an unknown nonce → dropped per nonce-only rule. + // The read card remains in the nonce index. + assert.ok( + state.pendingPermissionsByNonce.has(readNonce), + "nonce index must still contain the read card when write nonce does not match", + ); +}); diff --git a/docs/nips/NIP-AO.md b/docs/nips/NIP-AO.md index 6885dd0f8..feb48fbf0 100644 --- a/docs/nips/NIP-AO.md +++ b/docs/nips/NIP-AO.md @@ -111,7 +111,11 @@ ignored. `authorization` is present only on `acp_read` and `acp_write` frames that correspond to `session/request_permission` calls (see [Authorization Envelope](#authorization-envelope) -below). It is omitted on all other frame kinds. +below). It is omitted on all other frame kinds — with one exception: the observer-only +`permission_terminal` kind also carries `authorization` (with `reason = "uncertain"`) to +signal an unconfirmed outcome. `permission_terminal` is never an ACP wire frame; it is +emitted by the harness solely for Desktop card retirement when no confirmed `acp_write` +response was possible. ### Frame Kinds