From 528fa7239003950be1dccfd89b3a91c6e4ef733b Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Thu, 25 Jun 2026 07:33:37 -0400 Subject: [PATCH] fix(#4771): keep gateway approval failures actionable --- api/routes.py | 39 ++++ static/messages.js | 34 ++- tests/test_approval_unblock.py | 61 ++++++ tests/test_gateway_approval_legacy_path.py | 110 ++++++++++ ...t_issue4771_gateway_approval_failure_ui.py | 193 ++++++++++++++++++ 5 files changed, 431 insertions(+), 6 deletions(-) create mode 100644 tests/test_issue4771_gateway_approval_failure_ui.py diff --git a/api/routes.py b/api/routes.py index f7860ca6..00fc260b 100644 --- a/api/routes.py +++ b/api/routes.py @@ -6842,6 +6842,7 @@ from api.route_approvals import ( # noqa: F401 — re-exports for backward comp _approval_sse_unsubscribe, _approval_sse_notify_locked, _approval_sse_notify, + _GATEWAY_MIRROR_FLAG, reconcile_gateway_pending_mirror_locked, submit_gateway_pending_mirror, submit_pending, @@ -18645,6 +18646,32 @@ def _resolve_approval_legacy(sid: str, approval_id: str, choice: str) -> bool: return resolved +_GATEWAY_APPROVAL_RELAY_UNAVAILABLE = ( + "Gateway approval could not be relayed because the active run is unavailable. " + "Reopen the session or retry after it reconnects." +) + + +def _gateway_pending_approval_without_run_id(sid: str, approval_id: str) -> bool: + with _lock: + reconcile_gateway_pending_mirror_locked(sid) + queue = _pending.get(sid) + if isinstance(queue, list): + entries = queue + elif queue: + entries = [queue] + else: + entries = [] + if approval_id: + for entry in entries: + if isinstance(entry, dict) and entry.get("approval_id") == approval_id: + return bool(entry.get(_GATEWAY_MIRROR_FLAG)) + return False + if not entries or not isinstance(entries[0], dict): + return False + return bool(entries[0].get(_GATEWAY_MIRROR_FLAG)) + + def _handle_approval_respond(handler, body): sid = body.get("session_id", "") if not sid: @@ -18676,6 +18703,18 @@ def _handle_approval_respond(handler, body): except (RunnerClientError, ValueError) as exc: return j(handler, {"ok": False, "choice": choice, "relayed": True, "error": str(exc)}, status=502) return j(handler, {"ok": True, "choice": choice, "relayed": True}) + if _gateway_pending_approval_without_run_id(sid, approval_id): + return j( + handler, + { + "ok": False, + "choice": choice, + "relayed": False, + "code": "gateway_run_unavailable", + "error": _GATEWAY_APPROVAL_RELAY_UNAVAILABLE, + }, + status=409, + ) except Exception: pass # fall through to local approval path diff --git a/static/messages.js b/static/messages.js index cf14de91..69e28334 100644 --- a/static/messages.js +++ b/static/messages.js @@ -5578,16 +5578,38 @@ async function respondApproval(choice) { const b = $(id); if (b) { b.disabled = true; if (b.id === "approvalBtn" + choice.charAt(0).toUpperCase() + choice.slice(1)) b.classList.add("loading"); } }); - _approvalSessionId = null; - _approvalCurrentId = null; - _clearApprovalPendingForSession(sid); - hideApprovalCard(true); try { - await api("/api/approval/respond", { + const result = await api("/api/approval/respond", { method: "POST", body: JSON.stringify({ session_id: sid, choice, approval_id: approvalId }) }); - } catch(e) { setStatus(t("approval_responding") + " " + e.message); } + if (result && result.ok) { + const pendingEntry = _approvalPendingBySession.get(sid); + const samePending = !!(pendingEntry && pendingEntry.pending && (pendingEntry.pending.approval_id || null) === (approvalId || null)); + if (_approvalSessionId === sid && _approvalCurrentId === approvalId) { + _approvalSessionId = null; + _approvalCurrentId = null; + hideApprovalCard(true); + } + if (samePending) _clearApprovalPendingForSession(sid); + return; + } + const errMsg = (result && result.error) || "Approval response not accepted."; + ["approvalBtnOnce","approvalBtnSession","approvalBtnAlways","approvalBtnDeny"].forEach(id => { + const b = $(id); if (b) { b.disabled = false; b.classList.remove("loading"); } + }); + if (_approvalPromptBelongsToActiveSession(sid)) _renderPendingApprovalForActiveSession(); + if (typeof showToast === "function") showToast(errMsg, 5000); + if (typeof setStatus === "function") setStatus(errMsg); + } catch(e) { + const errMsg = (e && e.message) || (t("approval_responding") + " failed"); + ["approvalBtnOnce","approvalBtnSession","approvalBtnAlways","approvalBtnDeny"].forEach(id => { + const b = $(id); if (b) { b.disabled = false; b.classList.remove("loading"); } + }); + if (_approvalPromptBelongsToActiveSession(sid)) _renderPendingApprovalForActiveSession(); + if (typeof showToast === "function") showToast(errMsg, 5000); + if (typeof setStatus === "function") setStatus(errMsg); + } } function startApprovalPolling(sid) { diff --git a/tests/test_approval_unblock.py b/tests/test_approval_unblock.py index 3ef27adf..45d63642 100644 --- a/tests/test_approval_unblock.py +++ b/tests/test_approval_unblock.py @@ -12,6 +12,7 @@ import urllib.request import urllib.error import urllib.parse from pathlib import Path +from types import SimpleNamespace import pytest @@ -479,3 +480,63 @@ class TestApprovalHTTPEndpoints: r._pending.pop(sid, None) r._gateway_queues.pop(sid, None) pass # no external token state to clean + + def test_gateway_mirror_without_run_id_returns_explicit_conflict(self, monkeypatch): + """Gateway-mirrored approvals without relayable run state must stay actionable.""" + from api import routes as r + from api import route_approvals as ra + from api.gateway_chat import _STREAM_RUN_IDS + + sid = f"http-gateway-no-run-{uuid.uuid4().hex[:8]}" + stream_id = f"stream-no-run-{uuid.uuid4().hex[:8]}" + approval = { + "command": "rm -rf /tmp/no-run", + "pattern_key": "recursive delete", + "pattern_keys": ["recursive delete"], + "description": "recursive delete", + } + captured = {} + + def fake_j(handler, data, status=200, extra_headers=None): + captured["payload"] = data + captured["status"] = status + return data + + monkeypatch.setattr(r, "j", fake_j) + monkeypatch.setattr(r, "get_session", lambda _sid: SimpleNamespace(active_stream_id=stream_id)) + + with _lock: + r._pending.pop(sid, None) + r._gateway_queues.pop(sid, None) + _STREAM_RUN_IDS.pop(stream_id, None) + r._gateway_queues[sid] = [_ApprovalEntry(approval)] + try: + ra.submit_gateway_pending_mirror(sid, approval) + with _lock: + approval_id = r._pending[sid][0]["approval_id"] + + r._handle_approval_respond( + object(), + {"session_id": sid, "choice": "once", "approval_id": approval_id}, + ) + + assert captured["status"] == 409 + assert captured["payload"] == { + "ok": False, + "choice": "once", + "relayed": False, + "code": "gateway_run_unavailable", + "error": r._GATEWAY_APPROVAL_RELAY_UNAVAILABLE, + } + with _lock: + pending_queue = r._pending.get(sid) + assert isinstance(pending_queue, list) + assert len(pending_queue) == 1 + assert pending_queue[0]["approval_id"] == approval_id + assert sid in r._gateway_queues + assert not r._gateway_queues[sid][0].event.is_set() + finally: + with _lock: + r._pending.pop(sid, None) + r._gateway_queues.pop(sid, None) + _STREAM_RUN_IDS.pop(stream_id, None) diff --git a/tests/test_gateway_approval_legacy_path.py b/tests/test_gateway_approval_legacy_path.py index c8c86f16..b37f59bd 100644 --- a/tests/test_gateway_approval_legacy_path.py +++ b/tests/test_gateway_approval_legacy_path.py @@ -331,3 +331,113 @@ def test_legacy_approval_records_run_id_for_response_relay(): with STREAMS_LOCK: STREAMS.pop(stream_id, None) _STREAM_RUN_IDS.pop(stream_id, None) + +def test_legacy_approval_without_run_id_stays_actionable(): + """Legacy approvals without a run_id must fail explicitly and keep the mirror live.""" + from api import routes as r + from api import route_approvals as ra + from api.config import STREAMS, STREAMS_LOCK + from api.gateway_chat import _STREAM_RUN_IDS, _run_gateway_chat_streaming + from tools.approval import _ApprovalEntry + + stream_id = "sid-legacy-no-run" + session_id = "sess-legacy-no-run" + events = [] + q = MagicMock() + q.put_nowait = lambda item: events.append(item) + + with STREAMS_LOCK: + STREAMS[stream_id] = q + _STREAM_RUN_IDS.pop(stream_id, None) + + approval_payload = json.dumps({ + "command": "rm -rf /tmp/test", + "description": "Delete temporary files", + "pattern_key": "dangerous_command", + "pattern_keys": ["dangerous_command"], + "approval_id": "appr-legacy-no-run", + "choices": ["once", "session", "always", "deny"], + }) + sse_body = ( + f"event: approval.request\ndata: {approval_payload}\n\n" + 'data: {"choices":[{"delta":{"content":"Done"}}]}\n\n' + "data: [DONE]\n\n" + ).encode() + + mock_session = MagicMock() + mock_session.active_stream_id = stream_id + mock_session.workspace = "/tmp" + mock_session.model = "test" + mock_session.model_provider = None + mock_session.profile = None + mock_session.context_messages = [] + mock_session.messages = [] + mock_session.pending_user_message = None + mock_session.pending_attachments = None + mock_session.pending_started_at = None + + def fake_urlopen(req, *, timeout=None): + resp = MagicMock() + resp.__iter__ = lambda s: iter(sse_body.split(b"\n")) + resp.__enter__ = lambda s: s + resp.__exit__ = lambda s, *a: None + return resp + + captured = {} + + def fake_j(handler, data, status=200, extra_headers=None): + captured["payload"] = data + captured["status"] = status + return data + + try: + with patch.dict("os.environ", {"HERMES_WEBUI_CHAT_BACKEND": "gateway"}): + with patch("api.gateway_chat.gateway_supports_approval", return_value=False), \ + patch("urllib.request.urlopen", side_effect=fake_urlopen), \ + patch("api.gateway_chat.get_session", return_value=mock_session), \ + patch("api.gateway_chat._stream_writeback_is_current", return_value=True), \ + patch("api.gateway_chat.merge_session_messages_append_only", return_value=[]): + _run_gateway_chat_streaming( + session_id=session_id, + msg_text="do something risky", + model="test", + workspace="/tmp", + stream_id=stream_id, + ) + + assert _STREAM_RUN_IDS.get(stream_id) is None + approval_events = [ + item for item in events + if isinstance(item, tuple) and item[0] == "approval" + ] + assert approval_events + approval_data = approval_events[0][1] + with ra._lock: + r._gateway_queues[session_id] = [_ApprovalEntry(approval_data)] + ra.submit_gateway_pending_mirror(session_id, approval_data) + with ra._lock: + pending_queue = r._pending.get(session_id) + assert isinstance(pending_queue, list) + approval_id = pending_queue[0]["approval_id"] + + with patch("api.routes.get_session", return_value=mock_session), \ + patch("api.routes.j", new=fake_j): + r._handle_approval_respond( + object(), + {"session_id": session_id, "choice": "once", "approval_id": approval_id}, + ) + + assert captured["status"] == 409 + assert captured["payload"]["code"] == "gateway_run_unavailable" + assert captured["payload"]["error"] == r._GATEWAY_APPROVAL_RELAY_UNAVAILABLE + with ra._lock: + pending_queue = r._pending.get(session_id) + assert isinstance(pending_queue, list) + assert pending_queue[0]["approval_id"] == approval_id + finally: + with STREAMS_LOCK: + STREAMS.pop(stream_id, None) + with ra._lock: + r._pending.pop(session_id, None) + r._gateway_queues.pop(session_id, None) + _STREAM_RUN_IDS.pop(stream_id, None) diff --git a/tests/test_issue4771_gateway_approval_failure_ui.py b/tests/test_issue4771_gateway_approval_failure_ui.py new file mode 100644 index 00000000..abc4b6eb --- /dev/null +++ b/tests/test_issue4771_gateway_approval_failure_ui.py @@ -0,0 +1,193 @@ +"""Tests for #4771: failed gateway approval clicks must keep the card actionable.""" + +from __future__ import annotations + +import json +import shutil +import subprocess +import tempfile +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +MESSAGES_JS = (ROOT / "static" / "messages.js").read_text(encoding="utf-8") +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + + +def _extract_fn(src: str, name: str, prefix: str = "function ") -> str: + start = src.index(f"{prefix}{name}(") + brace = src.index("{", start) + depth = 0 + for i in range(brace, len(src)): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + if depth == 0: + return src[start:i + 1] + raise AssertionError(f"{name} body not closed") + + +def _run_node(script: str) -> dict: + with tempfile.NamedTemporaryFile("w", suffix=".js", delete=False, encoding="utf-8") as tf: + tf.write(script) + script_path = tf.name + try: + result = subprocess.run( + [NODE, script_path], + check=True, + capture_output=True, + text=True, + timeout=15, + ) + return json.loads(result.stdout) + finally: + Path(script_path).unlink(missing_ok=True) + + +def test_failed_gateway_approval_keeps_card_and_reenables_buttons(): + helpers = "\n".join( + [ + _extract_fn(MESSAGES_JS, "_approvalDismissKey"), + _extract_fn(MESSAGES_JS, "_getDismissedApprovals"), + _extract_fn(MESSAGES_JS, "_isApprovalDismissed"), + _extract_fn(MESSAGES_JS, "_unmarkApprovalDismissed"), + _extract_fn(MESSAGES_JS, "_promptActiveSessionId"), + _extract_fn(MESSAGES_JS, "_approvalPromptBelongsToActiveSession"), + _extract_fn(MESSAGES_JS, "_rememberApprovalPending"), + _extract_fn(MESSAGES_JS, "_clearApprovalPendingForSession"), + _extract_fn(MESSAGES_JS, "_renderPendingApprovalForActiveSession"), + _extract_fn(MESSAGES_JS, "showApprovalCard"), + _extract_fn(MESSAGES_JS, "respondApproval", prefix="async function "), + ] + ) + script = f""" +const output = {{}}; +const _store = {{}}; +const localStorage = {{ + getItem: key => Object.prototype.hasOwnProperty.call(_store, key) ? _store[key] : null, + setItem: (key, value) => {{ _store[key] = String(value); }}, +}}; +const S = {{ session: {{ session_id: 'sess-1' }} }}; +const _DISMISSED_APPROVALS_KEY = 'hermes_dismissed_approvals'; +let syncTopbarCalls = 0; +let showToastCalls = []; +let statusCalls = []; +let hideCalls = 0; +let renderCalls = 0; +let _approvalSessionId = 'sess-1'; +let _approvalCurrentId = 'appr-1'; +let _approvalPendingBySession = new Map(); +let _approvalSignature = ''; +let _approvalVisibleSince = 0; +let _approvalHideTimer = null; +const _clarifyPendingBySession = new Map(); +const buttons = new Map(); +function makeButton(id) {{ + return {{ + id, + disabled: false, + classList: {{ + values: new Set(), + add(value) {{ this.values.add(value); }}, + remove(value) {{ this.values.delete(value); }}, + contains(value) {{ return this.values.has(value); }}, + }}, + }}; +}} +['approvalBtnOnce','approvalBtnSession','approvalBtnAlways','approvalBtnDeny'].forEach(id => buttons.set(id, makeButton(id))); +const card = {{ + classList: {{ + values: new Set(['visible']), + add(value) {{ this.values.add(value); }}, + remove(value) {{ this.values.delete(value); }}, + contains(value) {{ return this.values.has(value); }}, + toggle(value, force) {{ + if (force === undefined) {{ + if (this.values.has(value)) this.values.delete(value); + else this.values.add(value); + }} else if (force) this.values.add(value); + else this.values.delete(value); + }}, + }}, + querySelector() {{ return null; }}, +}}; +const approvalDesc = {{ textContent: '' }}; +const approvalCmd = {{ textContent: '' }}; +const approvalCounter = {{ textContent: '', style: {{ display: '' }} }}; +const msg = {{}}; +const document = {{ activeElement: null }}; +const elements = {{ + approvalCard: card, + approvalDesc, + approvalCmd, + approvalCounter, + approvalBtnOnce: buttons.get('approvalBtnOnce'), + approvalBtnSession: buttons.get('approvalBtnSession'), + approvalBtnAlways: buttons.get('approvalBtnAlways'), + approvalBtnDeny: buttons.get('approvalBtnDeny'), + msg, +}}; +function $(id) {{ return elements[id] || null; }} +function syncTopbar() {{ syncTopbarCalls += 1; }} +function hideApprovalCard() {{ hideCalls += 1; card.classList.remove('visible'); }} +function _clearApprovalHideTimer() {{}} +function _syncApprovalCollapseButton() {{}} +function _syncApprovalTranscriptSpace() {{}} +function applyLocaleToDOM() {{}} +function setTimeout(fn) {{ return 1; }} +function showToast(msg) {{ showToastCalls.push(msg); }} +function setStatus(msg) {{ statusCalls.push(msg); }} +function t(key) {{ return key; }} +function api() {{ + const err = new Error('Gateway approval could not be relayed because the active run is unavailable. Reopen the session or retry after it reconnects.'); + err.status = 409; + return Promise.reject(err); +}} +{helpers} +const pending = {{ + approval_id: 'appr-1', + _session_id: 'sess-1', + command: 'rm -rf /tmp/test', + description: 'Dangerous command approval', + pattern_key: 'dangerous_command', + pattern_keys: ['dangerous_command'], +}}; +_approvalPendingBySession.set('sess-1', {{ pending, pendingCount: 1 }}); +showApprovalCard(pending, 1); +renderCalls = 0; +_renderPendingApprovalForActiveSession = function() {{ + renderCalls += 1; + const entry = _approvalPendingBySession.get('sess-1'); + if (entry) showApprovalCard(entry.pending, entry.pendingCount); +}}; +respondApproval('once').then(() => {{ + output.sessionId = _approvalSessionId; + output.approvalId = _approvalCurrentId; + output.pendingApprovalId = _approvalPendingBySession.get('sess-1').pending.approval_id; + output.hideCalls = hideCalls; + output.renderCalls = renderCalls; + output.toast = showToastCalls[0] || null; + output.status = statusCalls[0] || null; + output.onceDisabled = buttons.get('approvalBtnOnce').disabled; + output.onceLoading = buttons.get('approvalBtnOnce').classList.contains('loading'); + output.denyDisabled = buttons.get('approvalBtnDeny').disabled; + output.cardVisible = card.classList.contains('visible'); + process.stdout.write(JSON.stringify(output)); +}}); +""" + out = _run_node(script) + assert out["sessionId"] == "sess-1" + assert out["approvalId"] == "appr-1" + assert out["pendingApprovalId"] == "appr-1" + assert out["hideCalls"] == 0 + assert out["renderCalls"] == 1 + assert out["onceDisabled"] is False + assert out["onceLoading"] is False + assert out["denyDisabled"] is False + assert out["cardVisible"] is True + assert "active run is unavailable" in out["toast"] + assert "active run is unavailable" in out["status"]