From 343a8b50bfaceef6e62bb7483deda6e941bb7600 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sun, 28 Jun 2026 17:53:37 -0400 Subject: [PATCH] fix(#5139): distinguish offline gateway approval warnings --- api/config.py | 23 +++- api/gateway_chat.py | 11 +- static/messages.js | 4 + tests/test_gateway_approval_runs_api.py | 35 ++++++ .../test_issue4300_gateway_approval_notice.py | 6 +- ...sue5139_gateway_approval_offline_notice.py | 115 ++++++++++++++++++ 6 files changed, 186 insertions(+), 8 deletions(-) create mode 100644 tests/test_issue5139_gateway_approval_offline_notice.py diff --git a/api/config.py b/api/config.py index 8efafbb4d..8964e05c8 100644 --- a/api/config.py +++ b/api/config.py @@ -7443,7 +7443,13 @@ def get_gateway_caps(base_url: str, api_key: str = "") -> dict: cached = _GATEWAY_CAPS_CACHE.get(cache_key) if cached and now - cached.get("fetched_at", 0) < _GATEWAY_CAPS_TTL_S: return cached - caps = {"approval_events": False, "run_approval_response": False, "fetched_at": 0.0} + caps = { + "approval_events": False, + "run_approval_response": False, + "capabilities_reachable": False, + "probe_error": None, + "fetched_at": 0.0, + } try: headers = {} if api_key: @@ -7451,13 +7457,14 @@ def get_gateway_caps(base_url: str, api_key: str = "") -> dict: req = urllib.request.Request(f"{base_url}/v1/capabilities", headers=headers, method="GET") with urllib.request.urlopen(req, timeout=3) as resp: body = json.loads(resp.read(65536)) + caps["capabilities_reachable"] = True features = body.get("features") if isinstance(body, dict) else {} if not isinstance(features, dict): features = {} caps["approval_events"] = bool(features.get("approval_events")) caps["run_approval_response"] = bool(features.get("run_approval_response")) - except Exception: - pass + except Exception as exc: + caps["probe_error"] = f"{type(exc).__name__}: {exc}" with _GATEWAY_CAPS_LOCK: current = _GATEWAY_CAPS_CACHE.get(cache_key) if current and current.get("fetched_at", 0) > probe_started_at: @@ -7467,6 +7474,16 @@ def get_gateway_caps(base_url: str, api_key: str = "") -> dict: return caps +def gateway_approval_unavailable_reason(base_url: str, api_key: str = "") -> str | None: + """Return why approval support is unavailable, if it is unavailable.""" + caps = get_gateway_caps(base_url, api_key) + if bool(caps.get("approval_events") and caps.get("run_approval_response")): + return None + if not caps.get("capabilities_reachable"): + return "unreachable" + return "unsupported" + + def gateway_supports_approval(base_url: str, api_key: str = "") -> bool: """True only when the gateway advertises both approval_events and run_approval_response.""" caps = get_gateway_caps(base_url, api_key) diff --git a/api/gateway_chat.py b/api/gateway_chat.py index ebd202a7b..f234ecbe4 100644 --- a/api/gateway_chat.py +++ b/api/gateway_chat.py @@ -20,6 +20,7 @@ from api.config import ( STREAM_REASONING_TEXT, _get_session_agent_lock, coerce_reasoning_effort_for_model, + gateway_approval_unavailable_reason, gateway_supports_approval, register_active_run, unregister_active_run, @@ -639,12 +640,18 @@ def _run_gateway_chat_streaming( # Legacy gateway path: emit unsupported approval notice once per session, # but only when the gateway genuinely lacks approval capability. if not gateway_supports_approval(base_url, api_key): + approval_reason = gateway_approval_unavailable_reason(base_url, api_key) if not hasattr(s, "_approval_notice_emitted"): s._approval_notice_emitted = False if not s._approval_notice_emitted: + approval_message = "Approvals require a newer gateway. Upgrade the connected Hermes gateway to enable this." + approval_type = "approval_gateway_unsupported" + if approval_reason == "unreachable": + approval_type = "approval_gateway_offline" + approval_message = "Gateway connection failed. Check that the connected Hermes gateway is running and reachable." put_gateway_event("warning", { - "type": "approval_gateway_unsupported", - "message": "Approvals require a newer gateway. Upgrade the connected Hermes gateway to enable this.", + "type": approval_type, + "message": approval_message, }) s._approval_notice_emitted = True diff --git a/static/messages.js b/static/messages.js index 136dfd5e3..de17fa888 100644 --- a/static/messages.js +++ b/static/messages.js @@ -5447,6 +5447,10 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ if(typeof showToast==='function') showToast(typeof t==='function'?t('approval_gateway_unsupported_label'):'Approvals not supported',4000,'warning'); return; } + if(d.type==='approval_gateway_offline'){ + if(typeof showToast==='function') showToast(d.message||'Gateway offline',4000,'warning'); + return; + } // Show as a small inline notice, not a full error setComposerStatus(`${d.message||'Warning'}`); // If it's a fallback notice, show it briefly then clear diff --git a/tests/test_gateway_approval_runs_api.py b/tests/test_gateway_approval_runs_api.py index a5b57a616..018b7f3fb 100644 --- a/tests/test_gateway_approval_runs_api.py +++ b/tests/test_gateway_approval_runs_api.py @@ -15,7 +15,9 @@ from unittest.mock import MagicMock, patch def test_gateway_capability_detection(): """get_gateway_caps / gateway_supports_approval correctly parse /v1/capabilities.""" from api.config import ( + gateway_approval_unavailable_reason, gateway_supports_approval, + get_gateway_caps, invalidate_gateway_caps, ) @@ -38,6 +40,10 @@ def test_gateway_capability_detection(): return resp with patch("urllib.request.urlopen", side_effect=_fake_urlopen_capable): + caps = get_gateway_caps("http://fake:1234", "secret") + assert caps["capabilities_reachable"] is True + assert caps["probe_error"] is None + assert gateway_approval_unavailable_reason("http://fake:1234", "secret") is None assert gateway_supports_approval("http://fake:1234", "secret") is True invalidate_gateway_caps() @@ -52,11 +58,40 @@ def test_gateway_capability_detection(): return resp with patch("urllib.request.urlopen", side_effect=_fake_urlopen_incapable): + caps = get_gateway_caps("http://fake:5678") + assert caps["capabilities_reachable"] is True + assert caps["probe_error"] is None + assert gateway_approval_unavailable_reason("http://fake:5678") == "unsupported" assert gateway_supports_approval("http://fake:5678") is False invalidate_gateway_caps() +def test_gateway_capability_detection_marks_probe_failures_unreachable(): + """Probe failures stay non-fatal but remain distinguishable from unsupported gateways.""" + from api.config import ( + gateway_approval_unavailable_reason, + gateway_supports_approval, + get_gateway_caps, + invalidate_gateway_caps, + ) + + invalidate_gateway_caps() + + def _fake_urlopen_fail(req, *, timeout=None): + assert req.full_url == "http://fake:9999/v1/capabilities" + raise urllib.error.URLError("gateway unreachable") + + with patch("urllib.request.urlopen", side_effect=_fake_urlopen_fail): + caps = get_gateway_caps("http://fake:9999", "secret") + assert caps["capabilities_reachable"] is False + assert caps["probe_error"] + assert gateway_approval_unavailable_reason("http://fake:9999", "secret") == "unreachable" + assert gateway_supports_approval("http://fake:9999", "secret") is False + + invalidate_gateway_caps() + + def test_gateway_capability_cache_keeps_fresher_success_on_probe_race(): """A slower failed probe must not overwrite a fresher successful capability result.""" from api.config import gateway_supports_approval, invalidate_gateway_caps diff --git a/tests/test_issue4300_gateway_approval_notice.py b/tests/test_issue4300_gateway_approval_notice.py index 0ffc71dbe..555a6cd38 100644 --- a/tests/test_issue4300_gateway_approval_notice.py +++ b/tests/test_issue4300_gateway_approval_notice.py @@ -17,7 +17,7 @@ def test_gateway_chat_has_approval_notice_emitted_attribute_check(): def test_gateway_chat_emits_approval_gateway_unsupported_event(): """Verify put_gateway_event is called with approval_gateway_unsupported type on non-terminal channel.""" assert "put_gateway_event(\"warning\"" in GATEWAY_CHAT - assert "\"type\": \"approval_gateway_unsupported\"" in GATEWAY_CHAT + assert "approval_type = \"approval_gateway_unsupported\"" in GATEWAY_CHAT def test_gateway_chat_once_per_session_guard_pattern(): @@ -36,8 +36,8 @@ def test_gateway_chat_once_per_session_guard_pattern(): def test_gateway_chat_event_payload_contains_type_and_message(): """Verify the event payload has type and message fields.""" - assert "\"type\": \"approval_gateway_unsupported\"" in GATEWAY_CHAT - assert "\"message\": \"Approvals require a newer gateway. Upgrade the connected Hermes gateway to enable this.\"" in GATEWAY_CHAT + assert "approval_type = \"approval_gateway_unsupported\"" in GATEWAY_CHAT + assert "approval_message = \"Approvals require a newer gateway. Upgrade the connected Hermes gateway to enable this.\"" in GATEWAY_CHAT def test_messages_js_handles_approval_gateway_unsupported_event(): diff --git a/tests/test_issue5139_gateway_approval_offline_notice.py b/tests/test_issue5139_gateway_approval_offline_notice.py new file mode 100644 index 000000000..198b50dd4 --- /dev/null +++ b/tests/test_issue5139_gateway_approval_offline_notice.py @@ -0,0 +1,115 @@ +"""Regression coverage for #5139 gateway approval offline notice.""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import MagicMock, patch + +from api.config import STREAMS, STREAMS_LOCK +from api.gateway_chat import _run_gateway_chat_streaming + +REPO = Path(__file__).resolve().parents[1] +GATEWAY_CHAT = (REPO / "api" / "gateway_chat.py").read_text(encoding="utf-8") +MESSAGES_JS = (REPO / "static" / "messages.js").read_text(encoding="utf-8") + + +def _run_gateway_warning_case(unavailable_reason: str) -> list: + events = [] + q = MagicMock() + q.put_nowait = lambda item: events.append(item) + + stream_id = f"sid-{unavailable_reason}" + with STREAMS_LOCK: + STREAMS[stream_id] = q + + 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 + mock_session._approval_notice_emitted = False + + def fake_urlopen(req, *, timeout=None): + assert req.full_url == "http://127.0.0.1:8642/v1/chat/completions" + assert req.get_method() == "POST" + payload = req.data.decode("utf-8") + assert json.loads(payload)["messages"][-1]["content"] == "hi" + resp = MagicMock() + resp.__iter__ = lambda s: iter( + [ + b'data: {"choices":[{"delta":{"content":"Done"}}]}\n', + b"\n", + b"data: [DONE]\n", + ] + ) + resp.__enter__ = lambda s: s + resp.__exit__ = lambda s, *a: None + return resp + + try: + with patch.dict("os.environ", {"HERMES_WEBUI_CHAT_BACKEND": "gateway"}): + with patch("api.gateway_chat.gateway_supports_approval", return_value=False), \ + patch("api.gateway_chat.gateway_approval_unavailable_reason", return_value=unavailable_reason), \ + 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="sess-warning", + msg_text="hi", + model="test-model", + workspace="/tmp", + stream_id=stream_id, + ) + finally: + with STREAMS_LOCK: + STREAMS.pop(stream_id, None) + + return events + + +def test_gateway_chat_emits_offline_warning_for_unreachable_probe(): + events = _run_gateway_warning_case("unreachable") + warnings = [item for item in events if isinstance(item, tuple) and item[0] == "warning"] + assert warnings + assert warnings[0][1]["type"] == "approval_gateway_offline" + assert warnings[0][1]["message"] == "Gateway connection failed. Check that the connected Hermes gateway is running and reachable." + assert any(isinstance(item, tuple) and item[0] == "done" for item in events) + assert not any( + isinstance(item, tuple) and item[0] == "warning" and item[1].get("type") == "approval_gateway_unsupported" + for item in events + ) + + +def test_gateway_chat_keeps_unsupported_warning_for_reachable_older_gateway(): + events = _run_gateway_warning_case("unsupported") + warnings = [item for item in events if isinstance(item, tuple) and item[0] == "warning"] + assert warnings + assert warnings[0][1]["type"] == "approval_gateway_unsupported" + assert warnings[0][1]["message"] == "Approvals require a newer gateway. Upgrade the connected Hermes gateway to enable this." + assert any(isinstance(item, tuple) and item[0] == "done" for item in events) + assert not any( + isinstance(item, tuple) and item[0] == "warning" and item[1].get("type") == "approval_gateway_offline" + for item in events + ) + + +def test_messages_js_handles_offline_warning_without_touching_unsupported_branch(): + assert "d.type==='approval_gateway_offline'" in MESSAGES_JS + assert "Gateway offline" in MESSAGES_JS + assert "d.type==='approval_gateway_unsupported'" in MESSAGES_JS + assert "Approvals not supported" in MESSAGES_JS + assert "setComposerStatus(`${d.message||'Warning'}`);" in MESSAGES_JS + + +def test_gateway_chat_source_mentions_offline_warning_type(): + assert "approval_type = \"approval_gateway_offline\"" in GATEWAY_CHAT + assert "approval_type = \"approval_gateway_unsupported\"" in GATEWAY_CHAT + assert "approval_message = \"Gateway connection failed. Check that the connected Hermes gateway is running and reachable.\"" in GATEWAY_CHAT