fix(approval): stale local approval card clears instead of dead-ending on "Approval response not accepted." (#4948 local variant, #4771 follow-up)

CHANGELOG.md

@@ -3,6 +3,10 @@
 
 ## [Unreleased]
 
+### Fixed
+
+- **A stale command-approval card now clears instead of dead-ending on "Approval response not accepted." (#4948 local variant, #4771 follow-up).** On the default local backend, if an approval card was still on screen when its turn ended (cancel, fork, provider error, or normal completion while pending), clicking Approve/Deny sent an id that no longer matched anything and the card got stuck behind that error toast. The server now distinguishes a genuinely stale card (nothing pending for the session → clears the orphan card) from a stale-id click made while a *different* approval is still live (still rejected, so it can never resolve the wrong command — #527 preserved). Reported by santastabber and b3nw.
+
 ## [v0.51.672] — 2026-06-26 — Release YB (faster cron sidebar rebuild on stores missing the message index)
 
 ### Fixed

api/routes.py

@@ -18695,6 +18695,27 @@ def _gateway_pending_approval_without_run_id(sid: str, approval_id: str) -> bool
         return bool(entries[0].get(_GATEWAY_MIRROR_FLAG))
 
 
+def _session_has_pending_approval(sid: str) -> bool:
+    """True when the session still has any live pending approval to act on.
+
+    Used to tell a benign STALE-CARD click (the card's approval already
+    resolved or its stream ended, so nothing is pending) apart from a stale
+    explicit-id click made WHILE a different approval is still live (which must
+    stay unresolved so it can't accidentally approve the wrong command — #527).
+    Reconciles the gateway mirror first so a purged orphan is not counted.
+    """
+    with _lock:
+        reconcile_gateway_pending_mirror_locked(sid)
+        queue = _pending.get(sid)
+        if isinstance(queue, list):
+            if queue:
+                return True
+        elif queue:
+            return True
+        gw_queue = _gateway_queues.get(sid)
+        return bool(gw_queue)
+
+
 def _handle_approval_respond(handler, body):
     sid = body.get("session_id", "")
     if not sid:
@@ -18769,6 +18790,28 @@ def _handle_approval_respond(handler, body):
         ok = adapter.respond_approval(sid, approval_id, choice).accepted
     else:
         ok = _resolve_approval_legacy(sid, approval_id, choice)
+    if not ok and not _session_has_pending_approval(sid):
+        # The local resolution path returns False when an explicit approval_id
+        # was sent but no matching pending entry exists. There are two distinct
+        # causes, and only one is an error:
+        #   (a) a STALE CARD — the approval the card was rendered from already
+        #       resolved or its stream ended (cancel / fork / provider error /
+        #       completion while pending), so the agent's gateway entry was
+        #       dropped and reconcile purged the mirror. Nothing is pending for
+        #       this session anymore. Before #4771 the frontend was
+        #       fire-and-forget and this silently cleared the card; #4771 began
+        #       surfacing the bare {ok:false} as "Approval response not
+        #       accepted." with a STUCK card (reported by Jamie on .666 / b3nw;
+        #       the local-backend variant of #4948).
+        #   (b) a STALE EXPLICIT ID while a DIFFERENT approval IS live — that
+        #       MUST stay ok:false so a stale click on resolved approval A can
+        #       never resolve the unrelated live approval B (#527 guard).
+        # Distinguish them: when the session has NO pending approval at all,
+        # the click is benign — report it resolved so the UI clears the orphan
+        # card instead of dead-ending. When something IS still pending, keep
+        # the protective ok:false. `stale_cleared` lets the frontend log/branch
+        # without showing an error toast.
+        return j(handler, {"ok": True, "choice": choice, "stale_cleared": True})
     return j(handler, {"ok": ok, "choice": choice})
 
 

static/messages.js

@@ -5625,12 +5625,35 @@ async function respondApproval(choice) {
       _approvalResponding = null;
       const pendingEntry = _approvalPendingBySession.get(sid);
       const samePending = !!(pendingEntry && pendingEntry.pending && (pendingEntry.pending.approval_id || null) === (approvalId || null));
-      if (_approvalSessionId === sid && _approvalCurrentId === approvalId) {
+      // `stale_cleared` means the server found nothing pending for this session
+      // (the approval already resolved or its stream ended while the card was
+      // up). The orphan card must be cleared unconditionally so it can never
+      // get stuck — even if the displayed id has since drifted. (#4948 local
+      // variant: previously surfaced as a stuck "Approval response not
+      // accepted." toast.)
+      if (result.stale_cleared || (_approvalSessionId === sid && _approvalCurrentId === approvalId)) {
         _approvalSessionId = null;
         _approvalCurrentId = null;
         hideApprovalCard(true);
       }
-      if (samePending) _clearApprovalPendingForSession(sid);
+      if (samePending || result.stale_cleared) _clearApprovalPendingForSession(sid);
+      // Hardening for the narrow stale-clear race: a brand-new approval could
+      // have been parked server-side after the server's empty-check but before
+      // we processed this stale response. The unconditional clear above would
+      // hide that fresh card. Re-query the authoritative server pending state
+      // (same endpoint the fallback poll uses) so any approval that arrived in
+      // the window re-surfaces immediately instead of waiting for the next
+      // SSE/poll tick. Best-effort; poll/SSE remain the backstop. (Opus review
+      // nit on the #4948 fix.)
+      if (result.stale_cleared) {
+        api("/api/approval/pending?session_id=" + encodeURIComponent(sid), {timeoutToast: false})
+          .then(data => {
+            if (data && data.pending && _approvalPromptBelongsToActiveSession(sid)) {
+              showApprovalForSession(sid, data.pending, data.pending_count || 1);
+            }
+          })
+          .catch(() => {});
+      }
       return;
     }
     const errMsg = (result && result.error) || "Approval response not accepted.";

tests/test_issue4948_local_stale_approval.py

@@ -0,0 +1,226 @@
+"""Regression tests for the #4948 LOCAL-backend variant (Jamie/.666 report):
+a STALE approval card click must clear gracefully, not dead-end on
+"Approval response not accepted." with a stuck card.
+
+Background
+----------
+#4771 changed the frontend `respondApproval` from fire-and-forget to checking
+`result.ok` and surfacing a toast on failure. That exposed a pre-existing
+local-backend edge: when a guarded command's approval card is still on screen
+but its stream has ended (user cancel / fork / provider error / completion
+while pending), the agent's gateway entry is dropped and reconcile purges the
+mirrored `_pending` entry. A click then sends the held `approval_id`, which
+`_resolve_approval_legacy` cannot match -> returns False -> the handler
+returned a bare `{ok: false}` (no `error`) -> the frontend showed
+"Approval response not accepted." with a STUCK card. (Reported by Jamie on
+v0.51.666 and b3nw on Discord; the local-backend sibling of the gateway-side
+#4948.)
+
+The fix: when the local resolution returns False AND the session has no live
+pending approval at all, treat the click as a benign stale-card clear and
+return `{ok: True, stale_cleared: True}` so the UI clears the orphan card.
+The #527 protective guard is preserved: a stale explicit-id click made WHILE a
+DIFFERENT approval is still live must stay `ok: false` so it can never resolve
+the wrong command.
+"""
+from __future__ import annotations
+
+import json
+import threading
+import uuid
+from unittest.mock import patch
+
+import pytest
+
+from api import routes
+from api import models
+
+try:
+    import tools.approval as ta
+    from api import route_approvals as ra
+    APPROVAL_AVAILABLE = True
+except ImportError:
+    ta = None
+    ra = None
+    APPROVAL_AVAILABLE = False
+
+pytestmark = pytest.mark.skipif(
+    not APPROVAL_AVAILABLE,
+    reason="tools.approval not available in this environment",
+)
+
+
+class _FakeHandler:
+    def __init__(self):
+        self.status = None
+        self._body = b""
+        self.client_address = ("127.0.0.1", 0)
+        self.headers = {}
+
+        class _W:
+            def __init__(self, outer):
+                self.outer = outer
+
+            def write(self, b):
+                self.outer._body += b
+
+        self.wfile = _W(self)
+
+    def send_response(self, code):
+        self.status = code
+
+    def send_header(self, k, v):
+        pass
+
+    def end_headers(self):
+        pass
+
+    def json(self):
+        return json.loads(self._body.decode("utf-8"))
+
+
+def _register_session(sid: str):
+    s = models.Session(session_id=sid, title="approval-4948-local")
+    s.active_stream_id = None
+    with models.LOCK:
+        models.SESSIONS[sid] = s
+    return s
+
+
+def _park_local_approval(sid: str, command: str = "rm -rf /tmp/x", key: str = "dangerous_command"):
+    """Wire the notify callback exactly like api/streaming.py and park a
+    guarded command on a background thread (it blocks awaiting approval).
+
+    Returns the rendered approval_id (what the frontend card holds), taken from
+    /api/approval/pending — the same surface the real UI reads.
+    """
+    with ta._lock:
+        ta._gateway_queues.pop(sid, None)
+        ta._pending.pop(sid, None)
+
+    def _cb(approval_data):
+        ra.submit_gateway_pending_mirror(sid, approval_data)
+
+    ta.register_gateway_notify(sid, _cb)
+
+    def _agent():
+        ad = {
+            "command": command,
+            "pattern_key": key,
+            "pattern_keys": [key],
+            "description": "Dangerous command",
+        }
+        ta._await_gateway_decision(sid, ta._gateway_notify_cbs.get(sid), ad, surface="gateway")
+
+    th = threading.Thread(target=_agent, daemon=True)
+    th.start()
+    th.join(timeout=1.5)  # still blocked; the queue is seeded
+
+    h = _FakeHandler()
+    routes._handle_approval_pending(h, type("P", (), {"query": f"session_id={sid}"})())
+    return (h.json().get("pending") or {}).get("approval_id")
+
+
+def _end_stream_drop_entry(sid: str):
+    """Simulate the stream ending while the card is pending: the agent's
+    gateway entry is dropped (cancel/fork/error/completion) and reconcile
+    purges the orphaned _pending mirror — exactly what _drop_entry +
+    _cleanup_gateway_pending_mirror do."""
+    with ta._lock:
+        ta._gateway_queues.pop(sid, None)
+    with ra._lock:
+        ra.reconcile_gateway_pending_mirror_locked(sid)
+
+
+def _respond(sid: str, approval_id: str, choice: str = "once"):
+    h = _FakeHandler()
+    with patch("api.gateway_chat.webui_gateway_chat_enabled", return_value=False):
+        routes._handle_approval_respond(
+            h, {"session_id": sid, "choice": choice, "approval_id": approval_id}
+        )
+    return h
+
+
+def _cleanup(sid: str):
+    with ta._lock:
+        ta._gateway_queues.pop(sid, None)
+        ta._pending.pop(sid, None)
+    with models.LOCK:
+        models.SESSIONS.pop(sid, None)
+
+
+def test_stale_card_click_clears_not_dead_ends():
+    """The Jamie/.666 bug: clicking a card whose approval is gone returns a
+    benign cleared result, NOT a bare ok:false (which the UI rendered as
+    'Approval response not accepted.' with a stuck card)."""
+    sid = f"stale-local-{uuid.uuid4().hex[:8]}"
+    _register_session(sid)
+    try:
+        card_id = _park_local_approval(sid)
+        assert card_id, "precondition: a pending approval card should be rendered"
+        _end_stream_drop_entry(sid)
+        # Nothing is pending now.
+        assert routes._session_has_pending_approval(sid) is False
+        resp = _respond(sid, card_id)
+        body = resp.json()
+        assert resp.status == 200, f"expected 200, got {resp.status}: {body}"
+        assert body.get("ok") is True, f"stale card must clear gracefully: {body}"
+        assert body.get("stale_cleared") is True
+        # Crucially: no bare ok:false-without-error (the stuck-card symptom).
+    finally:
+        _cleanup(sid)
+
+
+def test_fresh_local_approval_still_resolves():
+    """A normal, live local approval still resolves (200/ok) and is NOT
+    mislabeled stale_cleared."""
+    sid = f"fresh-local-{uuid.uuid4().hex[:8]}"
+    _register_session(sid)
+    try:
+        card_id = _park_local_approval(sid)
+        assert card_id
+        assert routes._session_has_pending_approval(sid) is True
+        resp = _respond(sid, card_id, choice="once")
+        body = resp.json()
+        assert resp.status == 200, f"{body}"
+        assert body.get("ok") is True
+        assert not body.get("stale_cleared"), "a live approval must not be tagged stale_cleared"
+    finally:
+        _cleanup(sid)
+
+
+def test_stale_id_while_different_approval_live_still_blocked():
+    """#527 guard preserved: a stale explicit-id click made WHILE a different
+    approval is live must NOT resolve the live one (stays ok:false) and must
+    leave the live approval pending."""
+    sid = f"guard-local-{uuid.uuid4().hex[:8]}"
+    _register_session(sid)
+    try:
+        live_id = _park_local_approval(sid, command="rm -rf /tmp/B", key="dangerous_B")
+        assert live_id
+        stale_id = uuid.uuid4().hex  # a long-gone approval A
+        resp = _respond(sid, stale_id, choice="once")
+        body = resp.json()
+        assert body.get("ok") is False, f"stale id must not resolve while B is live: {body}"
+        assert not body.get("stale_cleared")
+        # B is still pending and unchanged.
+        assert routes._session_has_pending_approval(sid) is True
+        h = _FakeHandler()
+        routes._handle_approval_pending(h, type("P", (), {"query": f"session_id={sid}"})())
+        assert (h.json().get("pending") or {}).get("approval_id") == live_id
+    finally:
+        _cleanup(sid)
+
+
+def test_session_has_pending_approval_predicate():
+    """Unit: the predicate the benign-clear hinges on reports live vs empty."""
+    sid = f"pred-{uuid.uuid4().hex[:8]}"
+    _register_session(sid)
+    try:
+        assert routes._session_has_pending_approval(sid) is False
+        _park_local_approval(sid)
+        assert routes._session_has_pending_approval(sid) is True
+        _end_stream_drop_entry(sid)
+        assert routes._session_has_pending_approval(sid) is False
+    finally:
+        _cleanup(sid)