From b481dfea13fdc7e3eefd512f6afa6a6fea945b8b Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sun, 19 Jul 2026 12:41:24 -0400 Subject: [PATCH] fix(#6008): keep retained approvals visible in session attention --- api/routes.py | 30 +-------- tests/test_gateway_approval_runs_api.py | 82 +++++++++++++++++++++++++ tests/test_session_attention_badges.py | 67 ++++++++++++++++---- 3 files changed, 138 insertions(+), 41 deletions(-) diff --git a/api/routes.py b/api/routes.py index 76dfa066..b893d7e9 100644 --- a/api/routes.py +++ b/api/routes.py @@ -9747,40 +9747,12 @@ except ImportError: def _session_attention_summary(session_id: str) -> dict | None: """Return sidebar attention metadata for pending approval/clarify work.""" - active_run_id = "" - try: - from api.gateway_chat import _STREAM_RUN_IDS - - session = get_session(session_id) - active_stream_id = getattr(session, "active_stream_id", None) if session is not None else None - if active_stream_id: - active_run_id = str(_STREAM_RUN_IDS.get(active_stream_id) or "").strip() - except Exception: - active_run_id = "" - approval_count = 0 with _lock: reconcile_gateway_pending_mirror_locked(session_id) queue_list = _pending.get(session_id) if isinstance(queue_list, list): - gateway_queue = _gateway_queues.get(session_id) or [] - live_gateway_run_ids = { - str((getattr(entry, "data", None) or {}).get("run_id") or "").strip() - for entry in gateway_queue - if str((getattr(entry, "data", None) or {}).get("run_id") or "").strip() - } - approval_count = sum( - 1 - for entry in queue_list - if not ( - isinstance(entry, dict) - and entry.get(_GATEWAY_MIRROR_FLAG) - and str(entry.get("run_id") or "").strip() - and not str(entry.get(_GATEWAY_MIRROR_TOKEN) or "").strip() - and str(entry.get("run_id") or "").strip() not in live_gateway_run_ids - and str(entry.get("run_id") or "").strip() != active_run_id - ) - ) + approval_count = len(queue_list) elif queue_list: approval_count = 1 if approval_count > 0: diff --git a/tests/test_gateway_approval_runs_api.py b/tests/test_gateway_approval_runs_api.py index 3ba15e9a..3b06d611 100644 --- a/tests/test_gateway_approval_runs_api.py +++ b/tests/test_gateway_approval_runs_api.py @@ -421,6 +421,88 @@ def test_gateway_runs_api_streaming_parses_real_run_events(): assert approvals.gateway_pending_mirror("sess1", run_id="run-abc") is None +def test_gateway_runs_api_streaming_same_run_fifo_emits_head_and_promotes_successor(): + """Runs API approval events publish the reconciled FIFO head and count.""" + from api.config import STREAM_PARTIAL_TEXT, STREAM_REASONING_TEXT + from api.gateway_chat import _STREAM_RUN_IDS, _run_gateway_runs_api_streaming + from api import routes + import api.route_approvals as approvals + + sid = "sess-same-run-producer" + stream_id = "stream-same-run-producer" + events = [] + STREAM_PARTIAL_TEXT[stream_id] = "" + STREAM_REASONING_TEXT[stream_id] = "" + + class _JsonResponse: + def read(self, _limit=None): + return b'{"run_id":"run-fifo"}' + + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + class _SseResponse: + def __iter__(self): + return iter([ + b'data: {"event":"approval.request","command":"first","description":"First","run_id":"run-fifo","approval_id":"approval-first"}\n', + b'\n', + b'data: {"event":"approval.request","command":"second","description":"Second","run_id":"run-fifo","approval_id":"approval-second"}\n', + b'\n', + b'data: [DONE]\n', + b'\n', + ]) + + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def fake_urlopen(req, *, timeout=None): + return _JsonResponse() if req.full_url.endswith("/v1/runs") else _SseResponse() + + try: + with patch("urllib.request.urlopen", side_effect=fake_urlopen): + _run_gateway_runs_api_streaming( + sid, "hi", "test", "/tmp", stream_id, "http://gw:8642", "", [], {}, + put_gateway_event=lambda event, data: events.append((event, data)), + cancel_event=threading.Event(), + ) + + approval_events = [data for event, data in events if event == "approval"] + assert [(data["approval_id"], data["pending_count"]) for data in approval_events] == [ + ("approval-first", 1), + ("approval-first", 2), + ] + + handler = MagicMock() + handler.wfile = io.BytesIO() + with patch("api.routes.get_session", return_value=SimpleNamespace(active_stream_id=stream_id)), \ + patch("api.runner_client.HttpRunnerClient.respond_approval") as respond_approval: + routes._handle_approval_respond(handler, { + "session_id": sid, + "choice": "deny", + "approval_id": "approval-first", + }) + + handler.send_response.assert_called_with(200) + respond_approval.assert_called_once_with("run-fifo", "approval-first", "deny") + promoted = approvals.gateway_pending_mirror(sid, run_id="run-fifo") + assert promoted is not None + assert promoted["approval_id"] == "approval-second" + with approvals._lock: + assert len(approvals._pending[sid]) == 1 + finally: + STREAM_PARTIAL_TEXT.pop(stream_id, None) + STREAM_REASONING_TEXT.pop(stream_id, None) + _STREAM_RUN_IDS.pop(stream_id, None) + approvals._pending.pop(sid, None) + approvals._gateway_queues.pop(sid, None) + + def test_empty_id_runs_approval_reaches_real_response_lifecycle(): """Runs API empty IDs are carried and resolved through the live mirror.""" from api.gateway_chat import _run_gateway_runs_api_streaming diff --git a/tests/test_session_attention_badges.py b/tests/test_session_attention_badges.py index 91bf7c13..e67f2328 100644 --- a/tests/test_session_attention_badges.py +++ b/tests/test_session_attention_badges.py @@ -115,22 +115,65 @@ def test_attention_summary_prefers_pending_approvals_over_clarify_questions(): _clear_attention_state(sid) -def test_attention_summary_ignores_stale_gateway_mirror(): - sid = "attention-stale-gateway-session" +def test_attention_summary_keeps_direct_runs_mirror_after_stream_pointer_clears(): + from types import SimpleNamespace + from unittest.mock import MagicMock, patch + + import api.route_approvals as approvals + + sid = "attention-retained-gateway-session" _clear_attention_state(sid) try: - with routes._lock: - routes._pending[sid] = [{ - "_gateway_mirror": True, - "approval_id": "appr-stale-gateway", - "run_id": "run-stale-gateway", - "command": "rm -rf /tmp/stale", - "description": "Stale approval", - }] - routes._gateway_queues[sid] = [] + approvals.submit_gateway_pending_mirror(sid, { + "_gateway_mirror": True, + "approval_id": "approval-retained", + "run_id": "run-retained", + "command": "rm -rf /tmp/retained", + "description": "Retained approval", + }) + handler = MagicMock() + handler.wfile = io.BytesIO() - assert routes._session_attention_summary(sid) is None + with patch("api.routes.get_session", return_value=SimpleNamespace(active_stream_id=None)), \ + patch("api.runner_client.HttpRunnerClient.respond_approval") as respond_approval: + assert routes._session_attention_summary(sid) == { + "kind": "approval", + "count": 1, + "severity": "critical", + } + routes._handle_approval_respond(handler, { + "session_id": sid, + "choice": "deny", + "approval_id": "approval-retained", + }) + + handler.send_response.assert_called_with(200) + respond_approval.assert_called_once_with("run-retained", "approval-retained", "deny") + assert json.loads(handler.wfile.getvalue().decode("utf-8"))["ok"] is True finally: + approvals._pending.pop(sid, None) + _clear_attention_state(sid) + + +def test_attention_summary_hides_retired_direct_runs_mirror(): + import api.route_approvals as approvals + + sid = "attention-retired-gateway-session" + _clear_attention_state(sid) + try: + approvals.submit_gateway_pending_mirror(sid, { + "_gateway_mirror": True, + "approval_id": "approval-retired", + "run_id": "run-retired", + "command": "rm -rf /tmp/retired", + "description": "Retired approval", + }) + + assert approvals.retire_gateway_pending_mirror(sid, run_id="run-retired") is True + assert routes._session_attention_summary(sid) is None + assert approvals.gateway_pending_mirror(sid, run_id="run-retired") is None + finally: + approvals._pending.pop(sid, None) _clear_attention_state(sid)