Stage 382: PR #2496

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.

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:

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)

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")