diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 661c34ef22..3cf37ce624 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -4255,9 +4255,11 @@ fn build_kind40003_sentinel( /// /// `delivery_deadline` is computed at resolution time as /// `Instant::now() + RESOLVED_DELIVERY_WINDOW_SECS`, independent of the original -/// card/click deadline. This guarantees at least one publish attempt for every -/// terminal outcome, including ordinary timeouts (where the card deadline is -/// already past when `finish_permission` fires). +/// card/click deadline. The first publish attempt is **unconditional** — the +/// deadline is only consulted before each *retry* so that the relay always sees +/// at least one publication even when the caller supplies an already-expired +/// deadline (e.g. during ordinary timeouts where `entry_deadline` was already +/// past when `finish_permission` fired). /// /// Spawned detached so it never blocks the read loop. `event` is consumed and /// resent by clone each attempt so the signature and id are stable across retries. @@ -4266,8 +4268,13 @@ async fn retransmit_resolved_edit( event: nostr::Event, delivery_deadline: tokio::time::Instant, ) { + let mut first_attempt = true; loop { - if tokio::time::Instant::now() >= delivery_deadline { + // The first attempt is unconditional — an already-expired deadline must + // not prevent the single relay write that resolves the card. Subsequent + // retries (Uncertain outcome) are gated by the deadline so the loop + // terminates once the delivery window closes. + if !first_attempt && tokio::time::Instant::now() >= delivery_deadline { tracing::warn!( target: "acp::permission", "resolved edit {} not accepted before delivery window — giving up", @@ -4275,10 +4282,11 @@ async fn retransmit_resolved_edit( ); return; } + first_attempt = false; // Per-attempt ACK deadline: min(fixed publish timeout, delivery_deadline). // Capping each attempt at SENTINEL_PUBLISH_TIMEOUT sweeps a stuck waiter - // promptly so the same signed event is resent, while the loop-top check - // keeps the overall delivery window as the outer bound. + // promptly so the same signed event is resent, while the deadline check + // above keeps the overall delivery window as the outer bound. let attempt_deadline = (tokio::time::Instant::now() + std::time::Duration::from_secs(SENTINEL_PUBLISH_TIMEOUT_SECS)) .min(delivery_deadline); @@ -12280,69 +12288,116 @@ mod tests { let _ = std::fs::remove_file(&capture_file); } + // ── F2: unconditional first attempt with an already-expired deadline ────── + // + // The retransmit loop must publish the resolved edit at least once even when + // the delivery_deadline has already elapsed at call time. This covers the + // ordinary-timeout path where the card's entry_deadline expired before + // `finish_permission` called `retransmit_resolved_edit`. + // + // Mutation proof: reverting the `!first_attempt &&` guard (i.e. making the + // deadline check unconditional at loop-top) causes the loop to return + // immediately on an already-expired deadline without publishing — the + // assertion that a kind-40003 event was emitted goes red. + + #[tokio::test(start_paused = true)] + async fn retransmit_resolved_edit_unconditional_first_attempt_on_expired_deadline() { + let keys = Keys::generate(); + + let event = nostr::EventBuilder::new(nostr::Kind::from(40003), "resolved-expired") + .sign(&keys) + .unwrap(); + let event_id = event.id.to_hex(); + + // Use an accepting publisher — we only need to confirm the event is + // attempted once despite the expired deadline. + let (publisher, mut event_rx) = crate::relay::RelayEventPublisher::test_pair(); + + let collected: std::sync::Arc>> = + std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let collected_drain = collected.clone(); + tokio::spawn(async move { + while let Some(ev) = event_rx.recv().await { + if ev.kind.as_u16() == 40003 { + collected_drain.lock().unwrap().push(ev.id.to_hex()); + } + } + }); + + // Supply an already-expired deadline. + // Under start_paused, Instant::now() is fixed at epoch; subtract 1ns. + let already_expired = tokio::time::Instant::now() - std::time::Duration::from_nanos(1); + let handle = tokio::spawn(retransmit_resolved_edit(publisher, event, already_expired)); + + // Advance time past the per-attempt timeout so the spawned task drains. + tokio::time::advance(std::time::Duration::from_secs( + SENTINEL_PUBLISH_TIMEOUT_SECS + 1, + )) + .await; + tokio::task::yield_now().await; + let _ = tokio::time::timeout(std::time::Duration::from_millis(200), handle).await; + tokio::task::yield_now().await; + + let seen = collected.lock().unwrap().clone(); + assert!( + !seen.is_empty(), + "retransmit must publish even when delivery_deadline is already expired at call time; \ + mutation: unconditional loop-top deadline check → zero publishes → this goes red" + ); + assert_eq!( + seen[0], event_id, + "published event must carry the same stable signed id" + ); + } + // ── F2: always-Uncertain bounded-exit ───────────────────────────────────── // - // A retransmit loop that never receives `Accepted` must still terminate - // once the delivery window (300s) elapses. Under paused time we advance past - // the window and confirm the loop exits — i.e. the spawned task finishes - // without hanging forever. + // A retransmit loop that perpetually receives `Uncertain` (via test_pair_silent + // which drops ack_tx so every await resolves as RecvError → Uncertain) must + // still terminate once the delivery window elapses. Under paused tokio time + // we advance past the window and confirm the loop exits. // - // Same signed event ID across retries: the same `nostr::Event` struct - // (identical id + signature) is re-submitted on every `Uncertain` retry. - // The relay's idempotency guarantee only holds when the id is stable. - // - // Mutation proof: replacing `RESOLVED_DELIVERY_WINDOW_SECS` with a very - // large value (e.g. `u64::MAX / 2`) makes the loop effectively infinite - // under the test's time budget — the `JoinHandle` would never complete. + // Mutation proof: replacing `RESOLVED_DELIVERY_WINDOW_SECS` with a very large + // value (e.g. `u64::MAX / 2`) makes the loop effectively infinite under the + // test's time budget — the JoinHandle would never complete and the timeout + // assertion fails. #[tokio::test(start_paused = true)] async fn retransmit_resolved_edit_always_uncertain_bounded_exit() { - // Verify two properties of `retransmit_resolved_edit`: - // - // 1. **Bounded exit**: the loop terminates when the delivery window closes. - // Exercised by dropping the publisher's event receiver immediately so - // the first `register_publish_ack` call returns `Err` (channel closed) - // — the loop treats that as terminal and exits. - // - // 2. **Same signed event ID across retries**: the same `nostr::Event` - // struct (identical id + signature) must be re-submitted on every - // attempt. Verified by comparing `id.to_hex()` on the original and a - // clone — these must match. let keys = Keys::generate(); - let event = nostr::EventBuilder::new(nostr::Kind::from(40003), "resolved") + let event = nostr::EventBuilder::new(nostr::Kind::from(40003), "resolved-uncertain") .sign(&keys) .unwrap(); - // Drop the receiver immediately so the publisher channel is closed before - // the first attempt — the loop exits on the `Err(channel closed)` path. - let (publisher, event_rx) = crate::relay::RelayEventPublisher::test_pair(); - drop(event_rx); + // test_pair_silent drops ack_tx on each PublishEventAcked → every + // ack_rx.await yields Err(RecvError) → unwrapped as Uncertain. + let (publisher, _event_rx) = crate::relay::RelayEventPublisher::test_pair_silent(); - let delivery_deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(4); + // Short delivery window to advance past quickly. + let delivery_window = std::time::Duration::from_secs(4); + let delivery_deadline = tokio::time::Instant::now() + delivery_window; let handle = tokio::spawn(retransmit_resolved_edit( publisher, - event.clone(), + event, delivery_deadline, )); - // Advance time to let the spawned task run its first iteration. - tokio::time::advance(std::time::Duration::from_millis(100)).await; + // Each Uncertain attempt is followed by RESOLVED_RETRANSMIT_BACKOFF sleep. + // Under paused time the runtime auto-advances through parked timers. + // Advance well past the delivery window to drain all retry cycles. + tokio::time::advance( + delivery_window + std::time::Duration::from_secs(SENTINEL_PUBLISH_TIMEOUT_SECS * 3), + ) + .await; + tokio::task::yield_now().await; - // The loop must exit (channel closed = terminal). - let join_result = tokio::time::timeout(std::time::Duration::from_millis(500), handle).await; + let join_result = tokio::time::timeout(std::time::Duration::from_secs(1), handle).await; assert!( join_result.is_ok(), - "retransmit loop must exit when publisher channel closes (bounded-exit property)" - ); - - // Same event ID across clones — the relay idempotency guarantee requires - // the same signed event (same id) on every retry attempt. - let id_original = event.id.to_hex(); - let id_cloned = event.clone().id.to_hex(); - assert_eq!( - id_original, id_cloned, - "event ID must be stable across clones (same signed event retransmitted on retry)" + "retransmit loop must exit once the delivery window elapses even when every \ + attempt returns Uncertain; mutation: large RESOLVED_DELIVERY_WINDOW_SECS → \ + loop never completes within test budget → timeout fires" ); } diff --git a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.render.test.mjs b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.render.test.mjs index 91589a48ad..35c3490310 100644 --- a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.render.test.mjs +++ b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.render.test.mjs @@ -1,6 +1,7 @@ import assert from "node:assert/strict"; -import test from "node:test"; +import { after, afterEach, before, mock, test } from "node:test"; +import { JSDOM } from "jsdom"; import React from "react"; import { renderToStaticMarkup } from "react-dom/server"; @@ -43,9 +44,62 @@ function pendingPermissionItem(options) { } // --------------------------------------------------------------------------- -// allow_once — renders a green actionable Allow button +// jsdom + fake-timer setup (required for the interactive delivery-seam tests) +// The static renderToStaticMarkup tests do not use document/window but the +// setup is harmless for them: it only assigns globals they never read. // --------------------------------------------------------------------------- +const dom = new JSDOM("", { + url: "http://localhost", +}); + +// Deterministic wall-clock epoch — far from real time to avoid expiry surprises. +// expiresAt is set to FAKE_NOW_SECS + 9_999_999 in the interactive tests so +// the card never expires during the test. +const FAKE_NOW_MS = 1_000_000_000_000; + +before(() => { + mock.timers.enable({ apis: ["setInterval", "Date"], now: FAKE_NOW_MS }); + + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + MutationObserver: class { + observe() {} + disconnect() {} + takeRecords() { + return []; + } + }, + ResizeObserver: class { + observe() {} + unobserve() {} + disconnect() {} + }, + }); + dom.window.matchMedia = () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); + dom.window.MutationObserver = globalThis.MutationObserver; + dom.window.ResizeObserver = globalThis.ResizeObserver; +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); + mock.timers.reset(); + mock.timers.enable({ apis: ["setInterval", "Date"], now: FAKE_NOW_MS }); +}); + +after(() => { + mock.timers.reset(); + dom.window.close(); +}); + test("test_allow_once_renders_actionable_allow_button", () => { const html = renderToStaticMarkup( React.createElement(LifecycleActivity, { @@ -462,3 +516,175 @@ test("test_f3_cross_layer_four_options_acp_read_to_lifecycle_activity_two_button `cross-layer four-option card must render exactly 2 buttons; got ${buttonCount}`, ); }); + +// --------------------------------------------------------------------------- +// F3 interactive delivery-seam: acp_read → buildTranscript → LifecycleActivity +// click buttons → assert _deliveryFn called with ruled allow_once/reject_once IDs +// +// Mutation proof: removing `allow_once` from ACTIONABLE_KINDS → the allow_once +// button is not rendered → fireEvent.click finds no element → first delivery +// assertion fails. Removing `reject_once` → same for reject_once. +// Removing both → zero delivery calls → both assertions fail. +// --------------------------------------------------------------------------- + +test("test_f3_interactive_delivery_seam_allow_once_and_reject_once_fire_delivery", async () => { + const { createElement, act } = await import("react"); + const { render, fireEvent } = await import("@testing-library/react"); + + const FAKE_NOW_SECS = Math.floor(FAKE_NOW_MS / 1000); + const FUTURE_EXPIRY = FAKE_NOW_SECS + 9_999_999; + + // Record delivery calls: { optionId, requestNonce }[] + const deliveryCalls = []; + function mockDeliveryFn({ optionId, requestNonce }) { + deliveryCalls.push({ optionId, requestNonce }); + // Resolve as "acked" so the component doesn't re-enable the button. + return Promise.resolve("acked"); + } + + // Build the transcript card from a real acp_read event carrying all four + // option kinds — same wire shape as the static cross-layer test above. + const acpReadEvent = { + seq: 1, + timestamp: "2026-09-01T10:00:00.000Z", + kind: "acp_read", + agentIndex: 0, + channelId: "ch-interactive", + sessionId: "sess-interactive", + turnId: "turn-interactive", + payload: { + jsonrpc: "2.0", + id: "req-interactive", + method: "session/request_permission", + params: { + title: "Tool requires approval", + toolCallId: "tc-interactive", + options: [ + { + optionId: "opt-allow-once", + kind: "allow_once", + name: "Allow once", + }, + { optionId: "opt-reject-once", kind: "reject_once", name: "Deny" }, + { + optionId: "opt-allow-always", + kind: "allow_always", + name: "Always allow", + }, + { + optionId: "opt-reject-always", + kind: "reject_always", + name: "Always deny", + }, + ], + }, + }, + authorization: { + requestNonce: "nonce-interactive", + actionable: true, + expiresAt: FUTURE_EXPIRY, + }, + }; + + const transcript = buildTranscript([acpReadEvent]); + const card = transcript.find((item) => item.renderClass === "permission"); + assert.ok(card, "transcript must contain a permission card"); + assert.ok(card.actionable, "card must be actionable"); + + let container; + await act(async () => { + ({ container } = render( + createElement(LifecycleActivity, { + ...BASE_PROPS, + item: card, + _deliveryFn: mockDeliveryFn, + }), + )); + }); + + // ── Click allow_once — must call delivery with opt-allow-once ───────────── + const allowBtn = container.querySelector( + '[data-testid="permission-decision-opt-allow-once"]', + ); + assert.ok( + allowBtn !== null, + "allow_once button must be present (ACTIONABLE_KINDS must include allow_once)", + ); + await act(async () => { + fireEvent.click(allowBtn); + // Drain microtasks so the async delivery fn resolves. + await Promise.resolve(); + await Promise.resolve(); + }); + assert.equal( + deliveryCalls.length, + 1, + "exactly one delivery call after clicking allow_once; mutation: remove allow_once from ACTIONABLE_KINDS → zero calls", + ); + assert.equal( + deliveryCalls[0].optionId, + "opt-allow-once", + "delivery must be called with the allow_once optionId; mutation: wrong id → fails", + ); + assert.equal( + deliveryCalls[0].requestNonce, + "nonce-interactive", + "delivery must carry the card's requestNonce", + ); + + // ── allow_always must NOT have a button (not in ACTIONABLE_KINDS) ───────── + assert.equal( + container.querySelector( + '[data-testid="permission-decision-opt-allow-always"]', + ), + null, + "allow_always must not render a clickable button", + ); + + // ── reject_always must NOT have a button either ─────────────────────────── + assert.equal( + container.querySelector( + '[data-testid="permission-decision-opt-reject-always"]', + ), + null, + "reject_always must not render a clickable button", + ); + + // ── Render a fresh card and click reject_once ────────────────────────────── + // Use a separate render to avoid the pending-state from the allow_once click + // disabling the reject_once button. + deliveryCalls.length = 0; + let container2; + await act(async () => { + ({ container: container2 } = render( + createElement(LifecycleActivity, { + ...BASE_PROPS, + item: card, + _deliveryFn: mockDeliveryFn, + }), + )); + }); + + const rejectBtn = container2.querySelector( + '[data-testid="permission-decision-opt-reject-once"]', + ); + assert.ok( + rejectBtn !== null, + "reject_once button must be present (ACTIONABLE_KINDS must include reject_once)", + ); + await act(async () => { + fireEvent.click(rejectBtn); + await Promise.resolve(); + await Promise.resolve(); + }); + assert.equal( + deliveryCalls.length, + 1, + "exactly one delivery call after clicking reject_once; mutation: remove reject_once from ACTIONABLE_KINDS → zero calls", + ); + assert.equal( + deliveryCalls[0].optionId, + "opt-reject-once", + "delivery must be called with the reject_once optionId; mutation: wrong id → fails", + ); +}); diff --git a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx index 5e5f2653f3..3e74dcd735 100644 --- a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx +++ b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx @@ -82,6 +82,7 @@ function PermissionDecisionButtons({ requestNonce, deliveryFailed, deadlineSecs, + _deliveryFn, }: { agentPubkey: string; channelId: string; @@ -97,7 +98,14 @@ function PermissionDecisionButtons({ * Effective expiry deadline (unix seconds) bounding the retransmit loop. */ deadlineSecs: number; + /** + * Seam for testing — injects a mock delivery function without needing + * `mock.module`. Production callers omit this; the real + * `startPermissionDecisionDelivery` is used by default. + */ + _deliveryFn?: typeof startPermissionDecisionDelivery; }) { + const deliveryFn = _deliveryFn ?? startPermissionDecisionDelivery; const [pending, setPending] = React.useState(null); // Re-enable buttons when the reducer signals delivery failure (non-`sent` @@ -140,7 +148,7 @@ function PermissionDecisionButtons({ disabled={pending !== null} onClick={() => { setPending(optionId); - void startPermissionDecisionDelivery({ + void deliveryFn({ agentPubkey, channelId, requestNonce, @@ -172,7 +180,15 @@ function PermissionDecisionButtons({ ); } -export function LifecycleActivity(props: ActivityRenderClassItemProps) { +export function LifecycleActivity( + props: ActivityRenderClassItemProps & { + /** + * Seam for testing — injected mock delivery function threaded through to + * `PermissionDecisionButtons`. Production callers omit this prop. + */ + _deliveryFn?: typeof startPermissionDecisionDelivery; + }, +) { if (props.item.type === "tool") { return ; } @@ -230,6 +246,7 @@ export function LifecycleActivity(props: ActivityRenderClassItemProps) { props.item.timestamp, Date.now() / 1000, )} + _deliveryFn={props._deliveryFn} /> ) : null} {/* Row 5: decision — only when outcome is resolved */}