From 74cee777388ff3833842c63492022f1134524fa2 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 26 Jun 2026 00:48:02 +0000 Subject: [PATCH 1/2] fix(approval): stale local approval card clears instead of dead-ending (#4948 local variant, #4771 follow-up) On the default local in-process backend, an approval card whose stream ended while still pending (cancel/fork/provider-error/completion) left the agent queue entry dropped and the _pending mirror reconciled away. Clicking Approve/Deny then sent an approval_id that matched nothing, so the handler returned a bare {ok:false}; since #4771 the frontend surfaces that as 'Approval response not accepted.' with a stuck card (reported by @santastabber on v0.51.666 and b3nw). Distinguish a genuinely stale card (no pending approval for the session -> benign {ok:true, stale_cleared:true} so the UI clears the orphan) from a stale-id click made while a DIFFERENT approval is live (still ok:false so it can never resolve the wrong command -- #527 guard preserved). Frontend clears the orphan card on stale_cleared even if the displayed id drifted. Co-authored-by: b3nw --- api/routes.py | 43 ++++ static/messages.js | 27 ++- tests/test_issue4948_local_stale_approval.py | 226 +++++++++++++++++++ 3 files changed, 294 insertions(+), 2 deletions(-) create mode 100644 tests/test_issue4948_local_stale_approval.py diff --git a/api/routes.py b/api/routes.py index bf16d9569c4..6a051c9c4fd 100644 --- a/api/routes.py +++ b/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}) diff --git a/static/messages.js b/static/messages.js index 431ca13de17..20d1d58e7f9 100644 --- a/static/messages.js +++ b/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."; diff --git a/tests/test_issue4948_local_stale_approval.py b/tests/test_issue4948_local_stale_approval.py new file mode 100644 index 00000000000..b60e012efef --- /dev/null +++ b/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) From 1ae73b3949bb98d517d2a97f9f7d854f60f1bd94 Mon Sep 17 00:00:00 2001 From: Nathan Esquenazi Date: Thu, 25 Jun 2026 23:57:11 -0700 Subject: [PATCH 2/2] docs(changelog): add [Unreleased] entry for #4948 local stale-approval fix AGENTS.md requires a CHANGELOG entry for user-visible behavior changes, and this is one (a stale approval card went from a stuck "Approval response not accepted." dead-end to clearing cleanly). The PR shipped the routes.js + messages.js fix without a changelog note; this adds the [Unreleased] -> Fixed entry crediting santastabber / b3nw, noting the #527 guard is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d730e87351..25c1710a600 100644 --- a/CHANGELOG.md +++ b/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