diff --git a/CHANGELOG.md b/CHANGELOG.md index f3ba6229..8ef31e36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ ### Fixed +- **A timed-out command-approval card can be dismissed again instead of getting stuck on screen forever.** When the agent raised a local approval with no gateway notifier registered, the raw pending entry was stored without an `approval_id`; the frontend needs that id to own the card, so a card that timed out (or otherwise went unanswered) could be neither approved nor dismissed and the poll re-served it indefinitely. Reconciliation now mints a stable `gwlocal-mirrorless:` id on the raw local entry itself — the same tokenization contract already used for gateway producers — so the id survives repeated polls and the client's dismiss marker sticks. Gateway/run-bearing entries are untouched, and the synthetic prefix can't be mistaken for gateway authority (routing keys on exact ids, `run_id`, and mirror tokens, never the prefix). Thanks @CharlesMcquade. (#7510) + - **Manual "Regenerate title" now works for conversations that open with several queued user messages.** For a session whose transcript begins with consecutive user turns (no assistant reply before the second user message), the first-exchange walker stopped at the second user row with no assistant text, so "Regenerate title" skipped the LLM call, silently persisted the local fallback (a truncated first message), and returned `200` with that same wrong title on every retry — leaving the session permanently stuck. Manual regeneration now scans past the opening user rows to the first complete user+assistant pair so it reaches the aux model. The automatic in-stream title path is deliberately unchanged. Thanks @Raylands. (#7545, closes #7543) - **Custom providers that Hermes auto-discovered now show their full live `/v1/models` catalog again instead of just the saved models.** When a provider entry carries `models_discovered: true` alongside a `models:` mapping of per-model metadata (vision, context length), that mapping is discovery metadata — not a hand-curated allowlist — but the WebUI treated it as one and suppressed the live catalog, collapsing the model picker to only the already-saved IDs. Such a provider now defers to its live `/v1/models` catalog (matching the Hermes Agent's own `_models_config_is_allowlist` semantics), while an explicit `discover_models: false` opt-out (including the Agent-compatible string forms `false`/`no`/`0`) still pins the configured list, and a transient empty live probe falls back to the configured models rather than dropping the provider from the picker. Thanks @evgenyponomarev. (#7409, closes #7404) diff --git a/api/route_approvals.py b/api/route_approvals.py index 84e43346..eb2761a0 100644 --- a/api/route_approvals.py +++ b/api/route_approvals.py @@ -265,6 +265,23 @@ def reconcile_gateway_pending_mirror_locked(session_key: str) -> tuple[dict | No live_local_tokens.add(live_entry_token) if not str(live_data.get("approval_id") or "").strip(): live_data["approval_id"] = f"gwlocal:{live_entry_token}" + + # A raw id-less LOCAL entry in `_pending` (agent-side _pending_result drops + # `{command, pattern_key, pattern_keys, description}` verbatim when no + # gateway notifier is registered) is unactionable by contract: the frontend + # owner-capture requires an approval_id, so its card can neither be + # approved nor dismissed — the poll re-serves it forever. Mint a stable id + # on the entry itself (the same contract the producer-tokenization loop + # above applies to _gateway_queues producers). Minting on the stored dict + # keeps the id stable across polls, so frontend dismiss markers persist. + for entry in queue_list: + if not isinstance(entry, dict) or _is_gateway_mirror_entry(entry): + continue + if str(entry.get("approval_id") or "").strip(): + continue + if str(entry.get("run_id") or "").strip(): + continue + entry["approval_id"] = f"gwlocal-mirrorless:{uuid.uuid4().hex}" live_token = ( _gateway_mirror_entry_token(live_head_entry) if live_head_entry and live_head_data diff --git a/tests/test_issue_raw_pending_approval_id.py b/tests/test_issue_raw_pending_approval_id.py new file mode 100644 index 00000000..77777d56 --- /dev/null +++ b/tests/test_issue_raw_pending_approval_id.py @@ -0,0 +1,249 @@ +"""Raw id-less local approval entries in ``_pending`` must be actionable. + +Regression for the Sep 2026 live incident. When a guarded tool needs approval +and no gateway notifier is registered for the session (``unregister_gateway_ +notify`` fires at turn end; a queued wakeup/continuation then hits a guard), +the agent-side fallback ``tools/approval.py::_pending_result`` writes a raw +dict — ``{command, description, pattern_key, pattern_keys}`` and NOTHING else, +no ``approval_id``, no ``request_id`` — directly into the shared +``tools.approval._pending[session_key]`` dict that the WebUI imports. + +Before the fix: +- ``GET /api/approval/pending`` served that raw dict verbatim (reconcile passes + non-mirror entries through untouched) with NO ``approval_id``, so the + frontend owner-capture (``_captureApprovalResponseOwner``) bailed and every + button on the approval card was a no-op; +- the frontend dismiss (X) only hid the card locally and the 1.5s poll + re-rendered it forever — the card could neither be approved nor dismissed; +- there is no waiter thread behind this shape (``_pending_result`` returns the + STOP message immediately), so draining the entry is purely UI hygiene — but + leaving it forever means the session shows a permanent phantom card. + +After the fix (mint at the reconcile chokepoint): +- id-less, run-less, non-mirror ``_pending`` entries get a stable minted + ``approval_id`` (persisted in the stored entry dict, so it is stable across + polls and dismissals persist); +- an exact-id response pops the entry and reports ``ok``; +- a stale/different explicit id still fails closed (#527 guard preserved); +- Skip-all (yolo release) drains the entry instead of dead-ending. + +The sibling shape — an orphaned ``_gateway_queues`` producer entry — is already +covered upstream (minting ``gwlocal:`` in the producer-tokenization loop, +commit 5826d76d); the tests in ``test_gateway_approval_legacy_path.py`` and this +file's history document it. This file covers the raw-``_pending`` shape, which +no existing test exercised. +""" + +from __future__ import annotations + +import json +import re +import uuid +from unittest.mock import patch + +import pytest + +from api import models +from api import route_approvals as ra +from api import routes + +try: + import tools.approval as ta + APPROVAL_AVAILABLE = True +except ImportError: + ta = 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, active_stream_id: str | None = None): + s = models.Session(session_id=sid, title="approval-raw-pending-orphan") + s.active_stream_id = active_stream_id + with models.LOCK: + models.SESSIONS[sid] = s + return s + + +def _seed_raw_pending(sid: str, command: str = "rm -rf /tmp/qa-probe-home", + key: str = "recursive delete") -> dict: + """Seed ``_pending[sid]`` with the exact ``_pending_result`` shape. + + Mirrors agent-side tools/approval.py::_pending_result when no gateway + notifier is registered: a raw dict with no approval_id, no request_id, + no run_id, written directly into the shared _pending dict. + """ + pending = { + "command": command, + "pattern_key": key, + "pattern_keys": [key], + "description": "recursive delete", + } + with ta._lock: + ta._pending[sid] = pending + ta._gateway_queues.pop(sid, None) + return pending + + +def _cleanup(sid: str): + with ta._lock: + ta._pending.pop(sid, None) + ta._gateway_queues.pop(sid, None) + with models.LOCK: + models.SESSIONS.pop(sid, None) + try: + ta.disable_session_yolo(sid) + except Exception: + pass + + +def _poll_pending(sid: str) -> dict: + h = _FakeHandler() + routes._handle_approval_pending(h, type("P", (), {"query": f"session_id={sid}"})()) + assert h.status == 200, f"pending poll failed: {h.status} {h.json()!r}" + return h.json() + + +def _respond(sid: str, body: dict): + h = _FakeHandler() + with patch("api.gateway_chat.webui_gateway_chat_enabled", return_value=True): + routes._handle_approval_respond(h, body) + return h + + +MINTED_ID_RE = re.compile(r"^gwlocal-mirrorless:[0-9a-f]{32}$") + + +def test_raw_pending_entry_surfaces_with_stable_actionable_id(): + """The id-less raw entry must be served with a minted id that is stable + across polls (dismiss persistence keys on sid+approval_id).""" + sid = f"raw-id-{uuid.uuid4().hex[:8]}" + entry = _seed_raw_pending(sid) + try: + data = _poll_pending(sid) + assert data["pending_count"] >= 1 + pending = data["pending"] + assert pending is not None + approval_id = pending.get("approval_id") + assert approval_id, ( + "pending payload served a raw id-less local entry with no " + "approval_id; the frontend cannot act on it (buttons no-op, " + "dismiss cannot stick)" + ) + assert MINTED_ID_RE.match(approval_id), approval_id + + # Stability: the second poll must return the SAME id — the mint must + # persist in the stored entry, not re-mint per poll. + data2 = _poll_pending(sid) + assert data2["pending"]["approval_id"] == approval_id + assert entry["approval_id"] == approval_id + finally: + _cleanup(sid) + + +def test_raw_pending_entry_respond_with_minted_id_pops_entry(): + """An exact-id response must pop the raw entry and report ok (no waiter + exists behind this shape — draining is the correct resolution).""" + sid = f"raw-resp-{uuid.uuid4().hex[:8]}" + entry = _seed_raw_pending(sid) + # The incident session had a LIVE run pointer; the stale stream-pointer + # guard must not 409 the click before the legacy resolver pops the entry. + _register_session(sid, active_stream_id="stream-raw-resp-live") + from api.gateway_chat import _STREAM_RUN_IDS + + _STREAM_RUN_IDS["stream-raw-resp-live"] = "run-live-1" + try: + approval_id = _poll_pending(sid)["pending"]["approval_id"] + assert approval_id + + h = _respond(sid, {"session_id": sid, "choice": "deny", + "approval_id": approval_id}) + body = h.json() + assert h.status == 200, f"respond failed: {h.status} {body!r}" + assert body.get("ok") is True, f"respond not accepted: {body!r}" + + # The entry must actually be gone — no more phantom card. + data = _poll_pending(sid) + assert data["pending"] is None + assert data["pending_count"] == 0 + finally: + _STREAM_RUN_IDS.pop("stream-raw-resp-live", None) + _cleanup(sid) + + +def test_raw_pending_entry_stale_id_fails_closed(): + """#527 guard preserved: a different explicit id must not pop the live + entry and must keep it surfaced with its stable id.""" + sid = f"raw-stale-{uuid.uuid4().hex[:8]}" + entry = _seed_raw_pending(sid) + _register_session(sid) + try: + approval_id = _poll_pending(sid)["pending"]["approval_id"] + assert approval_id + + h = _respond(sid, {"session_id": sid, "choice": "once", + "approval_id": "stale-different-id"}) + body = h.json() + assert body.get("ok") is not True, f"stale id must fail closed: {body!r}" + + # The live entry remains pending, unchanged, with its stable id. + data = _poll_pending(sid) + assert data["pending"] is not None + assert data["pending"]["approval_id"] == approval_id + assert entry["approval_id"] == approval_id + finally: + _cleanup(sid) + + +def test_raw_pending_entry_yolo_skip_all_drains(): + """Skip-all (yolo release) must drain the raw entry instead of leaving the + phantom card, and report the yolo state truthfully.""" + sid = f"raw-yolo-{uuid.uuid4().hex[:8]}" + entry = _seed_raw_pending(sid) + _register_session(sid) + try: + approval_id = _poll_pending(sid)["pending"]["approval_id"] + assert approval_id + + h = _respond(sid, {"session_id": sid, "choice": "once", + "approval_id": approval_id, "yolo": True}) + body = h.json() + assert h.status == 200, f"yolo respond failed: {h.status} {body!r}" + assert body.get("ok") is True, body + assert body.get("yolo_enabled") is True + assert _poll_pending(sid)["pending"] is None + finally: + # Restore: the yolo release enables session YOLO as a side effect. + _cleanup(sid)