From 77f74405e859ad6e665cbfad87a450e8263a10bd Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 28 Aug 2026 16:10:37 -0400 Subject: [PATCH] fix(desktop): propagate control_result status; failure re-enables card for retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit retransmitPermissionDecision settled "acked" on any matching control_result frame without reading frame.status. The four failure statuses (no_active_turn, channel_full, channel_closed, no_channel) mean the harness received the frame but could not route the decision — retransmitting the same nonce cannot change that. The loop now resolves "failed" for those statuses and "acked" only for "sent" / "already_decided", so the card re-enables for owner retry on failure. Thread card (permission-request-card.tsx): .then checks the outcome and clears submitted on "failed" so the owner can retry. LifecycleActivity fast-path does the same before the reducer deliveryFailed path fires. Comments in both callers updated to reflect the non-throwing-but-tri-outcome contract. A11y: status text nodes ("Decision sent" / "Timed out" in thread card) now carry role="status" / aria-live="polite" so AT hears the async confirmation. NIP-AO: adds already_decided to the permission_decision status set with one line of meaning; updates the failure-status sentence to name the retry semantics. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/lib/permissionDecisionDelivery.ts | 9 ++- .../lib/retransmitPermissionDecision.test.mjs | 74 +++++++++++++++++++ .../lib/retransmitPermissionDecision.ts | 31 +++++--- .../LifecycleActivity.tsx | 22 ++++-- .../src/shared/ui/permission-request-card.tsx | 38 ++++++++-- docs/nips/NIP-AO.md | 10 ++- 6 files changed, 152 insertions(+), 32 deletions(-) diff --git a/desktop/src/features/agents/lib/permissionDecisionDelivery.ts b/desktop/src/features/agents/lib/permissionDecisionDelivery.ts index dedd9665fa..1fe6517b8b 100644 --- a/desktop/src/features/agents/lib/permissionDecisionDelivery.ts +++ b/desktop/src/features/agents/lib/permissionDecisionDelivery.ts @@ -41,9 +41,10 @@ export function resolveDecisionDeadlineSecs( * the pure {@link retransmitPermissionDecision} orchestrator. * * Fire-and-forget from the caller's view: the returned promise resolves when - * the harness acknowledges the nonce or the card's deadline passes. The card's - * UI reaction (resolve / retry) is driven separately by the `control_result` - * reducer path, so callers need not await this. + * the harness acknowledges the nonce, the harness returns an authoritative + * failure (the card re-enables for retry), or the card's deadline passes. + * The card's UI reaction (resolve / retry) is driven separately by the + * `control_result` reducer path, so callers need not await this. */ export function startPermissionDecisionDelivery({ agentPubkey, @@ -57,7 +58,7 @@ export function startPermissionDecisionDelivery({ requestNonce: string; optionId: string; deadlineSecs: number; -}): Promise<"acked" | "expired"> { +}): Promise<"acked" | "expired" | "failed"> { return retransmitPermissionDecision({ requestNonce, send: () => diff --git a/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs b/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs index b901b9d49f..c155457ed2 100644 --- a/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs +++ b/desktop/src/features/agents/lib/retransmitPermissionDecision.test.mjs @@ -204,3 +204,77 @@ test("retransmitPermissionDecision expires cleanly when every send rejects", asy } assert.deepEqual(unhandled, [], "rejected sends must not surface unhandled"); }); + +test("retransmitPermissionDecision resolves failed on a negative control_result status", async () => { + // Carl's regression: a failure status (no_active_turn / channel_full / + // channel_closed / no_channel) means the harness answered authoritatively but + // could not route the decision. The loop must stop retransmitting (re-sending + // the same nonce cannot change the routing refusal) and resolve "failed" so + // the card can re-enable for owner retry. + for (const status of [ + "no_active_turn", + "channel_full", + "channel_closed", + "no_channel", + ]) { + const h = harness(); + await drainMicrotasks(); + assert.equal(h.sendCalls, 1, `first send fired (${status})`); + + h.push(frame({ status })); + assert.equal( + await h.outcome, + "failed", + `negative status "${status}" must resolve "failed"`, + ); + assert.equal(h.unsubscribeCalls, 1, `listener torn down on "${status}"`); + assert.equal( + h.cancelRetransmitCalls, + 1, + `scheduler torn down on "${status}"`, + ); + + // A tick after failure must NOT resend — the loop has settled. + h.tick(); + await drainMicrotasks(); + assert.equal( + h.sendCalls, + 1, + `no resend after failure settle on "${status}"`, + ); + } +}); + +test("retransmitPermissionDecision: negative result then retry delivers acked", async () => { + // Carl's exact regression: a failure reply leaves the card actionable, the + // owner retries (new harness() = fresh loop), and the second attempt succeeds. + const first = harness(); + await drainMicrotasks(); + first.push(frame({ status: "no_active_turn" })); + assert.equal(await first.outcome, "failed"); + + // Owner retries — fresh orchestrator instance. + const second = harness(); + await drainMicrotasks(); + second.push(frame({ status: "sent" })); + assert.equal(await second.outcome, "acked"); + assert.equal(second.unsubscribeCalls, 1); + assert.equal(second.cancelRetransmitCalls, 1); +}); + +test("retransmitPermissionDecision: frame after failure settlement is inert", async () => { + // A late duplicate `control_result` for the same nonce arriving after the + // loop has already settled (via a failure status) must not re-resolve. + const h = harness(); + await drainMicrotasks(); + h.push(frame({ status: "no_active_turn" })); + assert.equal(await h.outcome, "failed"); + + // The unsubscribe detaches the listener, so a late push is dropped. + // Verify the loop doesn't try to double-resolve or re-enable a second loop. + h.push(frame({ status: "sent" })); // arrives after settle — inert + h.tick(); // tick after settle must not resend + await drainMicrotasks(); + assert.equal(h.sendCalls, 1, "no resend after settled failure"); + assert.equal(h.unsubscribeCalls, 1, "listener detached exactly once"); +}); diff --git a/desktop/src/features/agents/lib/retransmitPermissionDecision.ts b/desktop/src/features/agents/lib/retransmitPermissionDecision.ts index 9192adb1f7..68740e9731 100644 --- a/desktop/src/features/agents/lib/retransmitPermissionDecision.ts +++ b/desktop/src/features/agents/lib/retransmitPermissionDecision.ts @@ -13,13 +13,16 @@ import type { ControlResultFrame } from "@/shared/api/types"; * fail-closed timeout even though the owner decided promptly. * * This orchestrator resends the decision on a fixed cadence until it observes a - * `control_result` for THIS nonce, then stops. Any matching frame settles the - * loop — the harness has an authoritative answer (`sent` / `already_decided` = - * applied; a failure status is handled by the card's retry path), and resending - * cannot change it. If no reply arrives before the card's own `expiresAt` - * deadline the loop resolves `"expired"`: the card times out on its own and a - * decision applied past expiry would be rejected anyway, so retransmitting past - * it is pointless. + * `control_result` for THIS nonce, then stops. The outcome depends on the frame's + * status: `sent` and `already_decided` mean the harness routed or already applied + * the decision — the loop resolves `"acked"`. The four failure statuses + * (`no_active_turn`, `channel_full`, `channel_closed`, `no_channel`) mean the + * harness received the frame but could not route it — the loop resolves + * `"failed"`, stopping retransmission (re-sending the same nonce cannot change an + * authoritative routing refusal), and the card returns to the actionable state for + * owner retry. If no reply arrives before the card's own `expiresAt` deadline the + * loop resolves `"expired"`: the card times out on its own and a decision applied + * past expiry would be rejected anyway, so retransmitting past it is pointless. * * A nonce guard scopes the settling frame to this exact decision: a replayed or * concurrent `control_result` for a different card carries a different nonce and @@ -60,11 +63,11 @@ export function retransmitPermissionDecision({ * retransmit so the loop never resends past expiry. */ deadlineReached: () => boolean; -}): Promise<"acked" | "expired"> { - return new Promise<"acked" | "expired">((resolve) => { +}): Promise<"acked" | "expired" | "failed"> { + return new Promise<"acked" | "expired" | "failed">((resolve) => { let unsubscribe = () => {}; let cancelRetransmit = () => {}; - const finish = (outcome: "acked" | "expired") => { + const finish = (outcome: "acked" | "expired" | "failed") => { cancelRetransmit(); unsubscribe(); resolve(outcome); @@ -90,7 +93,13 @@ export function retransmitPermissionDecision({ ) { return; } - finish("acked"); + // `sent` and `already_decided` are both success: the harness routed or + // has already applied the decision. The four failure statuses indicate the + // harness received the frame but could not route it — retransmitting the + // same nonce cannot change that, so stop and let the card retry. + const success = + frame.status === "sent" || frame.status === "already_decided"; + finish(success ? "acked" : "failed"); }); cancelRetransmit = scheduleRetransmit(transmit); diff --git a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx index 278b36efb4..75ed99e29e 100644 --- a/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx +++ b/desktop/src/features/agents/ui/activityRenderClasses/LifecycleActivity.tsx @@ -152,12 +152,22 @@ function PermissionDecisionButtons({ requestNonce, optionId, deadlineSecs, - }).catch(() => { - // The delivery loop never rejects on a failed send — it retries - // until ack or the card's 300 s fail-closed expiry. This only - // fires on an unexpected error; re-enable so the user can retry. - setPending(null); - }); + }) + .then((outcome) => { + // `"failed"` means the harness received the frame but could + // not route it — re-enable so the user can retry. The reducer + // `deliveryFailed` path also re-enables via the `control_result` + // frame; this fast path handles the case before the reducer + // fires. `"acked"` / `"expired"` are terminal; the transcript + // item updates via the observer relay and no retry is needed. + if (outcome === "failed") setPending(null); + }) + .catch(() => { + // The delivery loop never rejects — it resolves one of + // "acked" | "expired" | "failed". This branch guards against + // any unexpected error and re-enables for safety. + setPending(null); + }); }} > {pending === optionId ? "…" : displayLabel} diff --git a/desktop/src/shared/ui/permission-request-card.tsx b/desktop/src/shared/ui/permission-request-card.tsx index 2283b48f7c..1d3dc64dd4 100644 --- a/desktop/src/shared/ui/permission-request-card.tsx +++ b/desktop/src/shared/ui/permission-request-card.tsx @@ -112,13 +112,25 @@ function PermissionButtons({ if (expired) { return ( -
Timed out
+
+ Timed out +
); } if (submitted !== null) { return ( -
Decision sent
+
+ Decision sent +
); } @@ -144,12 +156,22 @@ function PermissionButtons({ requestNonce: request.requestNonce, optionId, deadlineSecs: request.expiresAt, - }).catch(() => { - // The delivery loop never rejects on a failed send — it - // retries until ack or expiry. This only fires on an - // unexpected error; re-enable so the user can retry. - setSubmitted(null); - }); + }) + .then((outcome) => { + // `"failed"` means the harness received the frame but could + // not route it (no_active_turn / channel_full / etc.) — + // re-enable so the owner can retry. `"acked"` and `"expired"` + // are terminal: the harness applied the decision or the card + // timed out; the card transitions away via the kind-40003 edit + // or expiry countdown and no retry is needed. + if (outcome === "failed") setSubmitted(null); + }) + .catch(() => { + // The delivery loop never rejects — it resolves one of + // "acked" | "expired" | "failed". This branch guards against + // any unexpected error and re-enables for safety. + setSubmitted(null); + }); }} > {label} diff --git a/docs/nips/NIP-AO.md b/docs/nips/NIP-AO.md index 329459d5fb..2545d6e014 100644 --- a/docs/nips/NIP-AO.md +++ b/docs/nips/NIP-AO.md @@ -284,15 +284,19 @@ event to confirm receipt. This is an `acp_read`-style telemetry frame (kind = ```json { "type": "permission_decision", - "status": "sent" | "no_active_turn" | "channel_full" | "channel_closed" | "no_channel", + "status": "sent" | "already_decided" | "no_active_turn" | "channel_full" | "channel_closed" | "no_channel", "requestNonce": "", "optionId": "" } ``` `status: "sent"` means the decision was delivered to the in-flight read loop. -Other statuses indicate delivery failure; the per-request timeout will fail the -entry closed. +`status: "already_decided"` means the nonce was already applied by a prior delivery +(a retransmit reached the harness after the first copy was accepted); treat it as +success. The four remaining statuses (`no_active_turn`, `channel_full`, +`channel_closed`, `no_channel`) indicate delivery failure — the harness received +the frame but could not route the decision; the desktop should re-enable the card +so the owner can retry. ## Ephemerality Contract