From 42c677b223bb0dd6732506d13aa58ec0638ee60b Mon Sep 17 00:00:00 2001 From: nesquena-hermes <[email protected]> Date: Mon, 18 May 2026 03:43:59 +0000 Subject: [PATCH] Stage 382: PR #2496 --- CHANGELOG.md | 4 ++ api/routes.py | 81 +++++++++++++++++++++++------- tests/test_approval_queue.py | 23 +++++++++ tests/test_runtime_adapter_seam.py | 56 +++++++++++++++++++++ 4 files changed, 147 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 105ccd3c..423a1820 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Changed + +- **PR #2496** by @Michaelyklam (refs #1925) — Route approval and clarify responses through the default-off `RuntimeAdapter.respond_approval(...)` / `respond_clarify(...)` seam when `HERMES_WEBUI_RUNTIME_ADAPTER=legacy-journal` is enabled. The default `legacy-direct` path still uses the existing callback helpers directly, legacy no-id responses keep their historical `ok: true` shape, and stale explicit approval ids are now bounded as not-active instead of falling back to the oldest queued command. No approval queue, clarify queue, callback registry, runner, sidecar, queue/goal migration, or cached-agent state is introduced. + ### Fixed - **PR #2499** by @franksong2702 — Keep server-idle session rows from inheriting stale local streaming fields during sidebar optimistic merging, so PWA/browser caches cannot keep a completed session's spinner alive after `/api/sessions` reports no active stream or pending user message. diff --git a/api/routes.py b/api/routes.py index 8b86c72e..0b956998 100644 --- a/api/routes.py +++ b/api/routes.py @@ -8613,18 +8613,16 @@ def _handle_workspace_reorder(handler, body): return j(handler, {"ok": True, "workspaces": reordered}) -def _handle_approval_respond(handler, body): - sid = body.get("session_id", "") - if not sid: - return bad(handler, "session_id is required") - choice = body.get("choice", "deny") - if choice not in ("once", "session", "always", "deny"): - return bad(handler, f"Invalid choice: {choice}") - approval_id = body.get("approval_id", "") +def _resolve_approval_legacy(sid: str, approval_id: str, choice: str) -> bool: + """Resolve an approval through the existing callback path. - # Pop the targeted entry from the pending queue by approval_id. - # Falls back to popping the first entry for backward-compat with old clients. + Slice 3b keeps the RuntimeAdapter as a protocol translator: it delegates to + this legacy helper rather than owning approval queues or callback state. + """ + # Pop the targeted entry from the pending queue by approval_id. Old clients + # that omit approval_id still resolve the oldest entry for compatibility. pending = None + found_target = False with _lock: queue = _pending.get(sid) if isinstance(queue, list): @@ -8633,17 +8631,23 @@ def _handle_approval_respond(handler, body): for i, entry in enumerate(queue): if entry.get("approval_id") == approval_id: pending = queue.pop(i) + found_target = True break else: - # approval_id not found -- fall back to oldest entry. - pending = queue.pop(0) if queue else None + # A stale explicit id must not accidentally approve the + # oldest queued command; duplicate/stale responses are + # bounded as not-active by the adapter route. + pending = None else: pending = queue.pop(0) if queue else None + found_target = pending is not None if not queue: _pending.pop(sid, None) elif queue: # Legacy single-dict value. - pending = _pending.pop(sid, None) + if not approval_id or queue.get("approval_id") == approval_id: + pending = _pending.pop(sid, None) + found_target = pending is not None # Notify SSE subscribers of the new head (or empty state) so the UI # surfaces any trailing approvals that were queued behind this one # without waiting for the next submit_pending. Without this, a parallel @@ -8668,8 +8672,43 @@ def _handle_approval_respond(handler, body): # Unblock the agent thread waiting in the gateway approval queue. # This is the primary signal when streaming is active — the agent # thread is parked in entry.event.wait() and needs to be woken up. - resolve_gateway_approval(sid, choice, resolve_all=False) - return j(handler, {"ok": True, "choice": choice}) + gateway_resolved = 0 + if found_target or not approval_id: + gateway_resolved = resolve_gateway_approval(sid, choice, resolve_all=False) or 0 + # Keep the historical no-id response path truthy for old clients/tests while + # making stale explicit ids bounded as not-active for Slice 3b. + return bool(pending) or bool(gateway_resolved) or not bool(approval_id) + + +def _handle_approval_respond(handler, body): + sid = body.get("session_id", "") + if not sid: + return bad(handler, "session_id is required") + choice = body.get("choice", "deny") + if choice not in ("once", "session", "always", "deny"): + return bad(handler, f"Invalid choice: {choice}") + approval_id = body.get("approval_id", "") + + from api.runtime_adapter import LegacyJournalRuntimeAdapter, runtime_adapter_enabled + + if runtime_adapter_enabled(): + adapter = LegacyJournalRuntimeAdapter(approval_delegate=_resolve_approval_legacy) + ok = adapter.respond_approval(sid, approval_id, choice).accepted + else: + ok = _resolve_approval_legacy(sid, approval_id, choice) + return j(handler, {"ok": ok, "choice": choice}) + + +def _resolve_clarify_legacy(sid: str, clarify_id: str, response: str) -> bool: + """Resolve clarify through the existing callback path without new state.""" + # The legacy clarify queue is FIFO and does not yet expose stable ids to the + # browser, so clarify_id is accepted by the adapter contract but not used to + # create a parallel callback registry in the WebUI process. + resolved = resolve_clarify(sid, response, resolve_all=False) + # Preserve the historical no-id response shape for old clients/tests: a + # plain /api/clarify/respond call returns ok even when no pending prompt is + # active. Explicit stale ids remain bounded as not-active under the adapter. + return bool(resolved) or not bool(clarify_id) def _handle_clarify_respond(handler, body): @@ -8684,8 +8723,16 @@ def _handle_clarify_respond(handler, body): response = str(response or "").strip() if not response: return bad(handler, "response is required") - resolve_clarify(sid, response, resolve_all=False) - return j(handler, {"ok": True, "response": response}) + clarify_id = body.get("clarify_id", "") + + from api.runtime_adapter import LegacyJournalRuntimeAdapter, runtime_adapter_enabled + + if runtime_adapter_enabled(): + adapter = LegacyJournalRuntimeAdapter(clarify_delegate=_resolve_clarify_legacy) + ok = adapter.respond_clarify(sid, clarify_id, response).accepted + else: + ok = _resolve_clarify_legacy(sid, clarify_id, response) + return j(handler, {"ok": ok, "response": response}) class _ManualCompressionMemoryHandler: diff --git a/tests/test_approval_queue.py b/tests/test_approval_queue.py index 28978329..d52a3421 100644 --- a/tests/test_approval_queue.py +++ b/tests/test_approval_queue.py @@ -186,3 +186,26 @@ def test_respond_by_approval_id_pops_correct_entry(): # Cleanup with r._lock: r._pending.pop(sid, None) + + +def test_stale_explicit_approval_id_does_not_pop_oldest_entry(): + """Duplicate/stale approval responses must not resolve a different command.""" + from api import routes as r + + sid = "test-stale-approval-id-sid" + with r._lock: + r._pending.pop(sid, None) + + r.submit_pending(sid, {"command": "cmd1", "pattern_key": "p1", "pattern_keys": ["p1"], "description": "d1"}) + r.submit_pending(sid, {"command": "cmd2", "pattern_key": "p2", "pattern_keys": ["p2"], "description": "d2"}) + + accepted = r._resolve_approval_legacy(sid, "missing-approval-id", "deny") + + assert accepted is False + with r._lock: + queue = r._pending.get(sid, []) + commands = [entry["command"] for entry in queue] + assert commands == ["cmd1", "cmd2"] + + with r._lock: + r._pending.pop(sid, None) diff --git a/tests/test_runtime_adapter_seam.py b/tests/test_runtime_adapter_seam.py index ac248822..18cb9d89 100644 --- a/tests/test_runtime_adapter_seam.py +++ b/tests/test_runtime_adapter_seam.py @@ -124,6 +124,27 @@ def test_legacy_journal_adapter_cancel_returns_bounded_not_active_status(): assert result.safe_message == "Legacy control did not accept the request." +def test_legacy_journal_adapter_approval_and_clarify_return_bounded_not_active_status(): + runtime = importlib.import_module("api.runtime_adapter") + calls = [] + adapter = runtime.LegacyJournalRuntimeAdapter( + approval_delegate=lambda run_id, approval_id, choice: calls.append(("approval", run_id, approval_id, choice)) or False, + clarify_delegate=lambda run_id, clarify_id, response: calls.append(("clarify", run_id, clarify_id, response)) or False, + ) + + approval = adapter.respond_approval("already-finished-run", "stale-approval", "deny") + clarify = adapter.respond_clarify("already-finished-run", "stale-clarify", "answer") + + assert calls == [ + ("approval", "already-finished-run", "stale-approval", "deny"), + ("clarify", "already-finished-run", "stale-clarify", "answer"), + ] + assert approval.accepted is False + assert approval.status == "not-active" + assert clarify.accepted is False + assert clarify.status == "not-active" + + def test_chat_cancel_route_uses_adapter_only_when_flag_enabled(): routes = importlib.import_module("api.routes") src = (routes.Path(__file__).parent.parent / "api" / "routes.py").read_text(encoding="utf-8") @@ -137,6 +158,41 @@ def test_chat_cancel_route_uses_adapter_only_when_flag_enabled(): assert "HERMES_WEBUI_RUNTIME_ADAPTER" not in cancel_body, "route should use runtime_adapter_enabled(), not inline env checks" +def test_approval_and_clarify_routes_use_adapter_only_when_flag_enabled(): + routes = importlib.import_module("api.routes") + src = (routes.Path(__file__).parent.parent / "api" / "routes.py").read_text(encoding="utf-8") + + approval_idx = src.index("def _handle_approval_respond") + approval_body = src[approval_idx:src.index("def _resolve_clarify_legacy", approval_idx)] + clarify_idx = src.index("def _handle_clarify_respond") + clarify_body = src[clarify_idx:src.index("class _ManualCompressionMemoryHandler", clarify_idx)] + + assert "runtime_adapter_enabled()" in approval_body + assert "LegacyJournalRuntimeAdapter(approval_delegate=_resolve_approval_legacy)" in approval_body + assert "adapter.respond_approval(sid, approval_id, choice).accepted" in approval_body + assert "else:\n ok = _resolve_approval_legacy(sid, approval_id, choice)" in approval_body + assert "HERMES_WEBUI_RUNTIME_ADAPTER" not in approval_body + + assert "runtime_adapter_enabled()" in clarify_body + assert "LegacyJournalRuntimeAdapter(clarify_delegate=_resolve_clarify_legacy)" in clarify_body + assert "adapter.respond_clarify(sid, clarify_id, response).accepted" in clarify_body + assert "else:\n ok = _resolve_clarify_legacy(sid, clarify_id, response)" in clarify_body + assert "HERMES_WEBUI_RUNTIME_ADAPTER" not in clarify_body + + +def test_approval_respond_does_not_fallback_to_oldest_when_explicit_id_is_stale(): + routes = importlib.import_module("api.routes") + src = (routes.Path(__file__).parent.parent / "api" / "routes.py").read_text(encoding="utf-8") + helper_idx = src.index("def _resolve_approval_legacy") + helper_body = src[helper_idx:src.index("def _handle_approval_respond", helper_idx)] + + assert "A stale explicit id must not accidentally approve" in helper_body + assert "if found_target or not approval_id:" in helper_body + stale_branch = helper_body[helper_body.index("else:", helper_body.index("for i, entry")):helper_body.index("else:\n pending = queue.pop(0)")] + assert "pending = None" in stale_branch + assert "queue.pop(0)" not in stale_branch + + def test_chat_start_route_selects_adapter_only_when_flag_enabled(): routes = importlib.import_module("api.routes") src = (routes.Path(__file__).parent.parent / "api" / "routes.py").read_text(encoding="utf-8")