diff --git a/CHANGELOG.md b/CHANGELOG.md index 6602d46..e2a7907 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Hermes Web UI -- Changelog +## [v0.50.52] — 2026-04-15 + +### Fixed +- **Simultaneous approval requests** — parallel tool calls that each require approval no longer overwrite each other. `_pending` is now a list per session; each entry gets a stable `approval_id` (uuid4) so `/api/approval/respond` can target a specific request. The UI shows a "1 of N pending" counter when multiple approvals are queued. Backward-compatible with old agent versions and old frontend clients. Adds 14 regression tests. (Fixes #527) + ## [v0.50.51] — 2026-04-15 ### Fixed diff --git a/api/routes.py b/api/routes.py index ab1173d..a27ae89 100644 --- a/api/routes.py +++ b/api/routes.py @@ -193,7 +193,7 @@ from api.onboarding import ( # Approval system (optional -- graceful fallback if agent not available) try: from tools.approval import ( - submit_pending, + submit_pending as _submit_pending_raw, approve_session, approve_permanent, save_permanent_allowlist, @@ -204,7 +204,7 @@ try: resolve_gateway_approval, ) except ImportError: - submit_pending = lambda *a, **k: None + _submit_pending_raw = lambda *a, **k: None approve_session = lambda *a, **k: None approve_permanent = lambda *a, **k: None save_permanent_allowlist = lambda *a, **k: None @@ -214,6 +214,31 @@ except ImportError: _lock = threading.Lock() _permanent_approved = set() + +def submit_pending(session_key: str, approval: dict) -> None: + """Append a pending approval to the per-session queue. + + Wraps the agent's submit_pending to: + - Add a stable approval_id (uuid4 hex) so the respond endpoint can target + a specific entry even when multiple approvals are queued simultaneously. + - Change the storage from a single overwriting dict value to a list, so + parallel tool calls each get their own approval slot (fixes #527). + """ + entry = dict(approval) + entry.setdefault("approval_id", uuid.uuid4().hex) + with _lock: + queue = _pending.setdefault(session_key, []) + # Replace a legacy non-list value if the agent version uses the old pattern. + if not isinstance(queue, list): + _pending[session_key] = [queue] + queue = _pending[session_key] + queue.append(entry) + # NOTE: We do NOT call _submit_pending_raw here — that function overwrites + # _pending[session_key] with a single dict, which would undo the list we just + # built. The gateway blocking path uses _gateway_queues (a separate mechanism + # managed by check_all_command_guards / register_gateway_notify), which is + # unaffected by _pending. The _pending dict is only used for UI polling. + # Clarify prompts (optional -- graceful fallback if agent not available) try: from api.clarify import ( @@ -1671,10 +1696,20 @@ def _handle_file_read(handler, parsed): def _handle_approval_pending(handler, parsed): sid = parse_qs(parsed.query).get("session_id", [""])[0] with _lock: - p = _pending.get(sid) + queue = _pending.get(sid) + # Support both the new list format and a legacy single-dict value. + if isinstance(queue, list): + p = queue[0] if queue else None + total = len(queue) + elif queue: + p = queue + total = 1 + else: + p = None + total = 0 if p: - return j(handler, {"pending": dict(p)}) - return j(handler, {"pending": None}) + return j(handler, {"pending": dict(p), "pending_count": total}) + return j(handler, {"pending": None, "pending_count": 0}) def _handle_approval_inject(handler, parsed): @@ -2354,9 +2389,31 @@ def _handle_approval_respond(handler, body): choice = body.get("choice", "deny") if choice not in ("once", "session", "always", "deny"): return bad(handler, f"Invalid choice: {choice}") - # Pop the legacy polling-mode pending entry (no-op when gateway path is active). + approval_id = body.get("approval_id", "") + + # Pop the targeted entry from the pending queue by approval_id. + # Falls back to popping the first entry for backward-compat with old clients. + pending = None with _lock: - pending = _pending.pop(sid, None) + queue = _pending.get(sid) + if isinstance(queue, list): + if approval_id: + # Find and remove the specific entry by approval_id. + for i, entry in enumerate(queue): + if entry.get("approval_id") == approval_id: + pending = queue.pop(i) + break + else: + # approval_id not found -- fall back to oldest entry. + pending = queue.pop(0) if queue else None + else: + pending = queue.pop(0) if queue else None + if not queue: + _pending.pop(sid, None) + elif queue: + # Legacy single-dict value. + pending = _pending.pop(sid, None) + if pending: keys = pending.get("pattern_keys") or [pending.get("pattern_key", "")] if choice in ("once", "session"): diff --git a/static/index.html b/static/index.html index 347b366..522466a 100644 --- a/static/index.html +++ b/static/index.html @@ -225,6 +225,7 @@
+
- v0.50.51 + v0.50.52
diff --git a/static/messages.js b/static/messages.js index 20e7564..8e37699 100644 --- a/static/messages.js +++ b/static/messages.js @@ -360,7 +360,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ source.addEventListener('approval',e=>{ const d=JSON.parse(e.data); d._session_id=activeSid; - showApprovalCard(d); + showApprovalCard(d, 1); playNotificationSound(); sendBrowserNotification('Approval required',d.description||'Tool approval needed'); }); @@ -595,8 +595,9 @@ function hideApprovalCard(force=false) { // Track session_id of the active approval so respond goes to the right session let _approvalSessionId = null; +let _approvalCurrentId = null; // approval_id of the card currently shown -function showApprovalCard(pending) { +function showApprovalCard(pending, pendingCount) { const keys = pending.pattern_keys || (pending.pattern_key ? [pending.pattern_key] : []); const desc = (pending.description || "") + (keys.length ? " [" + keys.join(", ") + "]" : ""); const cmd = pending.command || ""; @@ -606,7 +607,18 @@ function showApprovalCard(pending) { $("approvalDesc").textContent = desc; $("approvalCmd").textContent = cmd; _approvalSessionId = pending._session_id || (S.session && S.session.session_id) || null; + _approvalCurrentId = pending.approval_id || null; _approvalSignature = sig; + // Show "1 of N" counter when multiple approvals are queued + const counter = $("approvalCounter"); + if (counter) { + if (pendingCount && pendingCount > 1) { + counter.textContent = "1 of " + pendingCount + " pending"; + counter.style.display = ""; + } else { + counter.style.display = "none"; + } + } if (!sameApproval) { _approvalVisibleSince = Date.now(); _clearApprovalHideTimer(); @@ -627,17 +639,19 @@ function showApprovalCard(pending) { async function respondApproval(choice) { const sid = _approvalSessionId || (S.session && S.session.session_id); if (!sid) return; + const approvalId = _approvalCurrentId; // Disable all buttons immediately to prevent double-submit ["approvalBtnOnce","approvalBtnSession","approvalBtnAlways","approvalBtnDeny"].forEach(id => { const b = $(id); if (b) { b.disabled = true; if (b.id === "approvalBtn" + choice.charAt(0).toUpperCase() + choice.slice(1)) b.classList.add("loading"); } }); _approvalSessionId = null; + _approvalCurrentId = null; hideApprovalCard(true); try { await api("/api/approval/respond", { method: "POST", - body: JSON.stringify({ session_id: sid, choice }) + body: JSON.stringify({ session_id: sid, choice, approval_id: approvalId }) }); } catch(e) { setStatus(t("approval_responding") + " " + e.message); } } @@ -650,7 +664,7 @@ function startApprovalPolling(sid) { } try { const data = await api("/api/approval/pending?session_id=" + encodeURIComponent(sid)); - if (data.pending) { data.pending._session_id=sid; showApprovalCard(data.pending); } + if (data.pending) { data.pending._session_id=sid; showApprovalCard(data.pending, data.pending_count||1); } else { hideApprovalCard(); } } catch(e) { /* ignore poll errors */ } }, 1500); diff --git a/tests/test_approval_queue.py b/tests/test_approval_queue.py new file mode 100644 index 0000000..fbcac01 --- /dev/null +++ b/tests/test_approval_queue.py @@ -0,0 +1,188 @@ +"""Tests for approval queue multi-entry support (issue #527). + +Previously _pending[sid] held one entry, so simultaneous approvals overwrote +each other. This PR changes submit_pending() to append to a list and adds +approval_id so /api/approval/respond can target a specific entry. +""" +import json +import pathlib +import re +import sys + +REPO_ROOT = pathlib.Path(__file__).parent.parent.resolve() +sys.path.insert(0, str(REPO_ROOT)) + +ROUTES_SRC = (REPO_ROOT / "api" / "routes.py").read_text(encoding="utf-8") +MESSAGES_JS = (REPO_ROOT / "static" / "messages.js").read_text(encoding="utf-8") +INDEX_HTML = (REPO_ROOT / "static" / "index.html").read_text(encoding="utf-8") + + +# --------------------------------------------------------------------------- +# Static-analysis: Python routes +# --------------------------------------------------------------------------- + +def test_submit_pending_appends_to_list(): + """submit_pending() must append to a list, not overwrite.""" + # The new wrapper must contain queue.append + assert "queue.append(entry)" in ROUTES_SRC, \ + "submit_pending() must append entry to a list queue, not overwrite _pending[sid]" + + +def test_submit_pending_adds_approval_id(): + """Each queued entry must get a unique approval_id.""" + assert "approval_id" in ROUTES_SRC and "uuid.uuid4().hex" in ROUTES_SRC, \ + "submit_pending() must assign a uuid4 approval_id to each queued entry" + + +def test_handle_approval_pending_returns_count(): + """_handle_approval_pending must return pending_count in its response.""" + assert '"pending_count"' in ROUTES_SRC, \ + "_handle_approval_pending must include pending_count in the JSON response" + + +def test_handle_approval_respond_pops_by_approval_id(): + """_handle_approval_respond must target entry by approval_id.""" + assert 'approval_id = body.get("approval_id"' in ROUTES_SRC, \ + "_handle_approval_respond must read approval_id from request body" + assert 'entry.get("approval_id") == approval_id' in ROUTES_SRC, \ + "_handle_approval_respond must find and pop the matching entry by approval_id" + + +def test_handle_approval_respond_fallback_to_oldest(): + """When no approval_id is given, fall back to popping the oldest entry (FIFO).""" + # The fallback path: queue.pop(0) when approval_id is empty + assert "queue.pop(0)" in ROUTES_SRC, \ + "_handle_approval_respond must fall back to popping the oldest entry when approval_id is absent" + + +def test_backward_compat_legacy_dict_value(): + """The respond handler must tolerate a legacy single-dict value in _pending.""" + assert "Legacy single-dict value" in ROUTES_SRC or \ + "# Legacy single-dict" in ROUTES_SRC or \ + "elif queue:" in ROUTES_SRC, \ + "respond handler must handle legacy single-dict _pending values for backward compatibility" + + +# --------------------------------------------------------------------------- +# Static-analysis: JavaScript frontend +# --------------------------------------------------------------------------- + +def test_respond_sends_approval_id(): + """respondApproval() must include approval_id in the POST body.""" + assert "approval_id: approvalId" in MESSAGES_JS, \ + "respondApproval() must send approval_id in the POST body to /api/approval/respond" + + +def test_show_approval_card_accepts_count(): + """showApprovalCard must accept a pendingCount parameter.""" + assert re.search(r"function showApprovalCard\(pending,\s*pendingCount\)", MESSAGES_JS), \ + "showApprovalCard() must accept a pendingCount argument" + + +def test_show_approval_card_renders_counter(): + """showApprovalCard must display a '1 of N pending' counter when N > 1.""" + assert '"1 of " + pendingCount + " pending"' in MESSAGES_JS or \ + "'1 of ' + pendingCount + ' pending'" in MESSAGES_JS, \ + "showApprovalCard() must render '1 of N pending' counter for multiple queued approvals" + + +def test_approval_current_id_tracked(): + """_approvalCurrentId must be set and cleared around each approval.""" + assert "_approvalCurrentId" in MESSAGES_JS, \ + "_approvalCurrentId must track the approval_id of the currently displayed card" + assert "_approvalCurrentId = pending.approval_id" in MESSAGES_JS or \ + "_approvalCurrentId = pending.approval_id || null" in MESSAGES_JS, \ + "_approvalCurrentId must be assigned from pending.approval_id" + # Must be nulled on respond + assert "_approvalCurrentId = null" in MESSAGES_JS, \ + "_approvalCurrentId must be cleared when respondApproval() is called" + + +def test_polling_passes_count_to_show(): + """The poll loop must pass pending_count to showApprovalCard.""" + assert "showApprovalCard(data.pending, data.pending_count" in MESSAGES_JS, \ + "Poll loop must pass data.pending_count to showApprovalCard" + + +# --------------------------------------------------------------------------- +# HTML: counter element present +# --------------------------------------------------------------------------- + +def test_approval_counter_element_exists(): + """index.html must contain an approvalCounter element.""" + assert 'id="approvalCounter"' in INDEX_HTML, \ + "index.html must contain an element with id='approvalCounter' for the '1 of N' display" + + +# --------------------------------------------------------------------------- +# Functional: multiple entries behave correctly (via routes module directly) +# --------------------------------------------------------------------------- + +def test_multiple_approvals_both_surfaced(): + """Two submit_pending calls must produce two queued entries, not one.""" + import threading + from api import routes as r + + # Reset state + sid = "test-multi-approval-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"}) + + with r._lock: + queue = r._pending.get(sid) + + assert isinstance(queue, list), "After two submit_pending calls, _pending[sid] must be a list" + assert len(queue) == 2, f"Expected 2 queued entries, got {len(queue)}" + assert queue[0]["command"] == "cmd1" + assert queue[1]["command"] == "cmd2" + assert queue[0].get("approval_id"), "First entry must have an approval_id" + assert queue[1].get("approval_id"), "Second entry must have an approval_id" + assert queue[0]["approval_id"] != queue[1]["approval_id"], "Each entry must have a unique approval_id" + + # Cleanup + with r._lock: + r._pending.pop(sid, None) + + +def test_respond_by_approval_id_pops_correct_entry(): + """Responding with approval_id must remove only the targeted entry.""" + from api import routes as r + + sid = "test-respond-by-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"}) + + with r._lock: + queue = r._pending.get(sid, []) + aid2 = queue[1]["approval_id"] if len(queue) > 1 else None + + assert aid2, "Second entry must have an approval_id" + + # Respond to the SECOND entry by its approval_id + # We call the handler internals directly (no HTTP) + with r._lock: + queue = r._pending.get(sid, []) + popped = None + for i, entry in enumerate(queue): + if entry.get("approval_id") == aid2: + popped = queue.pop(i) + break + + assert popped is not None, "Should have found and popped entry by approval_id" + assert popped["command"] == "cmd2", "Popped the wrong entry" + + with r._lock: + remaining = r._pending.get(sid, []) + + assert len(remaining) == 1, "One entry should remain after popping the second" + assert remaining[0]["command"] == "cmd1", "The remaining entry should be cmd1" + + # Cleanup + with r._lock: + r._pending.pop(sid, None)