Merge pull request #5044 from nesquena/release/stage-5041-approval

Release: relay mirrored gateway approvals after stream loss (#5041, fixes #5000)

CHANGELOG.md

@@ -5,6 +5,8 @@
 
 ### Fixed
 
+- **Gateway approval cards can be answered even after the live stream pointer is lost.** When you approve/deny a tool-use request that came through the gateway, `/api/approval/respond` walked back through the session's `active_stream_id` to find the run — so if that pointer was gone (reconnect, background tab, stream ended), responding failed with `gateway_run_unavailable` even though the mirrored approval card still carried the originating gateway run info. The respond handler now relays a mirrored gateway approval using the approval's own carried origin (scoped to the same session + approval id), so the card stays actionable across stream loss. The normal stream-alive path is unchanged, auth/CSRF gates are untouched, and a genuinely origin-less approval still returns `gateway_run_unavailable`. Thanks @rodboev. (#5041, fixes #5000)
+
 - **Installing an extension now shows its post-install next steps.** Some extensions need a follow-up step after install (run a setup command, restart, configure a key). The Settings → Extensions gallery now renders that guidance from the extension's own registry metadata — generic `post_install` text and lifecycle requirements, with an optional docs link — right on the card, plus a "see the card for next steps" install toast. The guidance is registry-driven (no vendor special-casing), every value is HTML-escaped, any docs URL is validated to safe http(s) before it's linked, and the note degrades cleanly when an extension declares no post-install steps. Thanks @franksong2702. (#4964, fixes #4959)
 
 - **The Settings → Extensions diagnostics panel now shows per-extension sidecar runtime status.** When an extension's loopback sidecar health response includes an optional top-level `runtime` object, the diagnostics panel parses it and renders only allowlisted scalar fields (sidecar/native-host/bridge status, last-seen time, and the loopback origin) so you can see at a glance whether an extension's helper process is running, waiting, or stale — without WebUI depending on any one extension's private payload shape. All values are status-enum-allowlisted, HTML-escaped, timestamp-validated, and origin-limited to `http://127.0.0.1`/`localhost`; a missing or malformed `runtime` object degrades cleanly, and a failed diagnostics refresh now renders the error instead of staying stuck on "Loading…". Thanks @franksong2702. (#4979)

api/route_approvals.py

@@ -184,6 +184,31 @@ def reconcile_gateway_pending_mirror_locked(session_key: str) -> tuple[dict | No
     return head, total, changed
 
 
+def _gateway_mirrored_pending_run_id(session_key: str, approval_id: str) -> str | None:
+    """Return the mirrored gateway approval run_id for a matching pending card.
+
+    Reconciles the mirror first so a live gateway head still survives a lost
+    `active_stream_id` pointer.
+    """
+    approval_id = str(approval_id or "").strip()
+    if not approval_id:
+        return None
+    with _lock:
+        reconcile_gateway_pending_mirror_locked(session_key)
+        queue = _pending.get(session_key)
+        if isinstance(queue, list):
+            entries = queue
+        elif queue:
+            entries = [queue]
+        else:
+            return None
+        for entry in entries:
+            if isinstance(entry, dict) and entry.get("approval_id") == approval_id and entry.get(_GATEWAY_MIRROR_FLAG):
+                run_id = str(entry.get("run_id") or "").strip()
+                return run_id or None
+    return None
+
+
 def submit_gateway_pending_mirror(session_key: str, approval: dict) -> None:
     """Mirror the live gateway head into WebUI polling state under a typed tag."""
     del approval  # mirror from the live gateway head under `_lock`, not from callback input

api/routes.py

@@ -6673,6 +6673,7 @@ from api.route_approvals import (  # noqa: F401 — re-exports for backward comp
     _approval_sse_notify_locked,
     _approval_sse_notify,
     _GATEWAY_MIRROR_FLAG,
+    _gateway_mirrored_pending_run_id,
     reconcile_gateway_pending_mirror_locked,
     submit_gateway_pending_mirror,
     submit_pending,
@@ -18553,7 +18554,9 @@ def _handle_approval_respond(handler, body):
         return bad(handler, f"Invalid choice: {choice}")
     approval_id = body.get("approval_id", "")
 
-    # Gateway relay: forward choice to the runs API when session has an active run.
+    # Gateway relay: forward choice to the runs API when session has an active run,
+    # or recover the run_id from the mirrored gateway approval entry if the
+    # stream pointer has already been cleared.
     try:
         from api.gateway_chat import (
             _STREAM_RUN_IDS,
@@ -18568,6 +18571,8 @@ def _handle_approval_respond(handler, body):
             active_sid = getattr(s, "active_stream_id", None)
             if active_sid:
                 _run_id = _STREAM_RUN_IDS.get(active_sid)
+            if not _run_id and approval_id:
+                _run_id = _gateway_mirrored_pending_run_id(sid, approval_id)
         if _run_id:
             if not approval_id:
                 return bad(handler, "approval_id is required for gateway approvals")
@@ -18579,21 +18584,13 @@ def _handle_approval_respond(handler, body):
                 HttpRunnerClient(base_url=_base, api_key=_key).respond_approval(_run_id, approval_id, choice)
             except (RunnerClientError, ValueError) as exc:
                 return j(handler, {"ok": False, "choice": choice, "relayed": True, "error": str(exc)}, status=502)
+            # The outbound relay only resumes the remote run; the local mirror
+            # still needs the same cleanup path so the parked entry, mirrored
+            # card, and agent signal all settle here too.
+            _resolve_approval_legacy(sid, approval_id, choice)
             return j(handler, {"ok": True, "choice": choice, "relayed": True})
-        # #4771 surfaces an explicit relay-failure 409 when a gateway approval
-        # is pending but its run is gone (so the card stays actionable instead
-        # of silently failing). That signal is ONLY meaningful on a
-        # gateway-backed deployment. On the default local in-process backend,
-        # every guarded command parks an entry in tools.approval._gateway_queues
-        # (via _await_gateway_decision), which the WebUI mirrors into _pending
-        # with _GATEWAY_MIRROR_FLAG set — but there is no gateway run and no
-        # _STREAM_RUN_IDS entry by design. Without the backend-mode gate below,
-        # _gateway_pending_approval_without_run_id() returns True for that purely
-        # local approval and the handler 409s ("active run unavailable"),
-        # refusing to resolve an approval that resolves perfectly well locally.
-        # Gate on gateway mode so local approvals fall through to the local
-        # resolution path; gateway behaviour is unchanged. (#4771 regression;
-        # also reported as #4948)
+        # Only a still-mirrored gateway approval with a missing run should 409;
+        # stale or empty gateway clicks fall through to local resolution.
         if webui_gateway_chat_enabled(_get_config()) and _gateway_pending_approval_without_run_id(
             sid, approval_id
         ):

tests/test_gateway_approval_legacy_path.py

@@ -332,6 +332,151 @@ def test_legacy_approval_records_run_id_for_response_relay():
             STREAMS.pop(stream_id, None)
         _STREAM_RUN_IDS.pop(stream_id, None)
 
+
+def test_mirrored_run_id_survives_active_stream_loss():
+    """A mirrored gateway approval must still relay after active_stream_id is lost."""
+    import io
+    import threading
+    from types import SimpleNamespace
+    from api import route_approvals as ra
+    from api import routes
+
+    sid = "sess-legacy-stream-loss"
+    approval_id = "appr-legacy-stream-loss"
+    run_id = "run-legacy-stream-loss"
+
+    with ra._lock:
+        ra._gateway_queues.pop(sid, None)
+        ra._pending.pop(sid, None)
+
+    entry = SimpleNamespace(
+        data={
+            "command": "rm -rf /tmp/test",
+            "description": "Delete temporary files",
+            "pattern_key": "dangerous_command",
+            "pattern_keys": ["dangerous_command"],
+            "approval_id": approval_id,
+            "run_id": run_id,
+            "choices": ["once", "session", "always", "deny"],
+        },
+        event=threading.Event(),
+        result=None,
+    )
+    with ra._lock:
+        ra._gateway_queues.setdefault(sid, []).append(entry)
+    ra.submit_gateway_pending_mirror(sid, entry.data)
+
+    with ra._lock:
+        mirrored = ra._pending[sid][0]
+    assert mirrored["approval_id"] == approval_id
+    assert mirrored["run_id"] == run_id
+    assert mirrored.get(ra._GATEWAY_MIRROR_FLAG) is True
+
+    relay_session = MagicMock()
+    relay_session.active_stream_id = None
+    captured = {}
+
+    def fake_request_json(self, req):
+        captured["url"] = req.full_url
+        captured["body"] = json.loads(req.data)
+        return {"ok": True}
+
+    def fake_resolve_gateway_approval(session_key, choice, resolve_all=False):
+        del resolve_all
+        with ra._lock:
+            queue = ra._gateway_queues.get(session_key) or []
+            if not queue:
+                return 0
+            queued_entry = queue.pop(0)
+            queued_entry.result = choice
+            queued_entry.event.set()
+            if not queue:
+                ra._gateway_queues.pop(session_key, None)
+            return 1
+
+    handler = MagicMock()
+    handler.wfile = io.BytesIO()
+    body = {"session_id": sid, "choice": "once", "approval_id": approval_id}
+
+    try:
+        with patch("api.routes.get_session", return_value=relay_session), \
+             patch("api.gateway_chat.webui_gateway_chat_enabled", return_value=True), \
+             patch("api.gateway_chat._gateway_base_url", return_value="http://gw:8642"), \
+             patch("api.gateway_chat._gateway_api_key", return_value=""), \
+             patch("api.config.get_config", return_value={}), \
+             patch("api.routes.resolve_gateway_approval", new=fake_resolve_gateway_approval), \
+             patch("api.runner_client.HttpRunnerClient._request_json", new=fake_request_json):
+            routes._handle_approval_respond(handler, body)
+
+        assert captured.get("url", "") == f"http://gw:8642/v1/runs/{run_id}/approval", (
+            f"approval respond must relay to the mirrored gateway run; got {captured.get('url')!r}"
+        )
+        assert captured["body"] == {"choice": "once", "approval_id": approval_id}
+        handler.send_response.assert_called_with(200)
+        assert entry.event.is_set(), "mirrored gateway approval was not resolved"
+        assert entry.result == "once"
+        with ra._lock:
+            assert sid not in ra._pending, "mirrored pending card was not cleared"
+            assert sid not in ra._gateway_queues, "parked gateway entry was not drained"
+        assert handler.wfile.getvalue()
+        assert json.loads(handler.wfile.getvalue().decode("utf-8")) == {
+            "ok": True,
+            "choice": "once",
+            "relayed": True,
+        }
+    finally:
+        with ra._lock:
+            ra._gateway_queues.pop(sid, None)
+            ra._pending.pop(sid, None)
+
+
+def test_gateway_mode_no_pending_click_stays_non_409():
+    """Gateway mode must still fall through when nothing is pending."""
+    from api import route_approvals as ra
+    from api import routes
+
+    sid = "sess-legacy-no-pending"
+    approval_id = "appr-legacy-no-pending"
+
+    with ra._lock:
+        ra._gateway_queues.pop(sid, None)
+        ra._pending.pop(sid, None)
+
+    mock_session = MagicMock()
+    mock_session.active_stream_id = None
+    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
+
+    captured = {}
+
+    def fake_j(handler, data, status=200, extra_headers=None):
+        captured["payload"] = data
+        captured["status"] = status
+        return data
+
+    with patch.dict("os.environ", {"HERMES_WEBUI_CHAT_BACKEND": "gateway"}), \
+         patch("api.routes.get_session", return_value=mock_session), \
+         patch("api.routes.j", new=fake_j), \
+         patch("api.runtime_adapter.runtime_adapter_enabled", return_value=False):
+        routes._handle_approval_respond(
+            object(),
+            {"session_id": sid, "choice": "once", "approval_id": approval_id},
+        )
+
+    assert captured["status"] == 200
+    assert captured["payload"]["ok"] is True
+    assert captured["payload"]["choice"] == "once"
+    assert captured["payload"]["stale_cleared"] is True
+    assert captured["payload"].get("code") != "gateway_run_unavailable"
+
+
 def test_legacy_approval_without_run_id_stays_actionable():
     """Legacy approvals without a run_id must fail explicitly and keep the mirror live."""
     from types import SimpleNamespace