DasPoschi/hermes-webui
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(clarify): require stable clarify_id and wait for backend ack so stale responses are rejected

Browse Source 
The WebUI clarification popup had a response-delivery failure: users
submitted answers in the popup, but the agent still fell through to the
timeout fallback message.  Three bugs conspired:

1. No stable clarify_id — _ClarifyEntry had no unique identifier, so
   the frontend could not reference a specific pending prompt.  The
   backend used FIFO resolution which silently failed for stale/late
   responses.

2. Frontend hid the card before confirmation — respondClarify() called
   hideClarifyCard(true, 'sent') BEFORE the API call completed.  If the
   backend rejected the response, the card was already gone and the
   user's draft was discarded.

3. Backend lied about success — _resolve_clarify_legacy() returned
   bool(resolved) or not bool(clarify_id).  Since the frontend never
   sent clarify_id, the backend always reported ok:true even when
   nothing was resolved.

Changes:

api/clarify.py:
- _ClarifyEntry now auto-generates a stable clarify_id (uuid4.hex[:12])
- submit_pending() injects clarify_id into the data dict visible to the
  frontend via SSE and polling
- New resolve_clarify_by_id() for O(1) lookup by id instead of FIFO pop

api/routes.py:
- _resolve_clarify_legacy() uses resolve_clarify_by_id when clarify_id
  is provided; returns actual bool result (no more unconditional True)
- _handle_clarify_respond() returns HTTP 409 + {ok:false, stale:true}
  when resolution fails

static/messages.js:
- respondClarify() now sends clarify_id in the POST body
- Waits for a positive backend acknowledgement before hiding the card
- Saves a draft copy before POST and restores it on failure
- On 409/network error: re-enables controls, shows error toast
- Guards against parallel-SSE race where clearing the cache after a
  successful response could erase a newly queued next prompt (codex P1)

tests:
- Updated test_sprint30.py for new ack-before-hide behaviour
- Updated test_clarify_unblock.py for 409 on stale responses

Closes #2639.
This commit is contained in:
Arsh Kumar Singh
2026-05-20 16:35:15 +00:00
parent 9c983e693a
commit d385db69d5
5 changed files with 109 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
api/clarify.py
View File

@@ -9,6 +9,7 @@ from __future__ import annotations
import queue
import threading
import time
import uuid
from typing import Optional
@@ -25,12 +26,13 @@ _clarify_sse_subscribers: dict[str, list[queue.Queue]] = {}
class _ClarifyEntry:
"""One pending clarify request inside a session."""
__slots__ = ("event", "data", "result")
__slots__ = ("event", "data", "result", "clarify_id")
def __init__(self, data: dict):
self.event = threading.Event()
self.data = data
self.result: Optional[str] = None
self.clarify_id: str = data.get("clarify_id", "") or uuid.uuid4().hex[:12]
def register_gateway_notify(session_key: str, cb) -> None:
@@ -131,6 +133,8 @@ def submit_pending(session_key: str, data: dict) -> _ClarifyEntry:
return entry
entry = _ClarifyEntry(data)
# Ensure clarify_id is present in the serialised data the frontend receives.
entry.data["clarify_id"] = entry.clarify_id
gw_queue.append(entry)
_pending[session_key] = gw_queue[0].data
cb = _gateway_notify_cbs.get(session_key)
@@ -179,3 +183,28 @@ def resolve_clarify(session_key: str, response: str, resolve_all: bool = False)
entry.event.set()
count += 1
return count
def resolve_clarify_by_id(session_key: str, clarify_id: str, response: str) -> bool:
"""Resolve a specific pending clarify request by its stable id.
Returns True if the id was found and resolved, False otherwise.
"""
with _lock:
q = _gateway_queues.get(session_key)
if not q:
_pending.pop(session_key, None)
return False
for i, entry in enumerate(q):
if entry.clarify_id == clarify_id:
q.pop(i)
if q:
_pending[session_key] = q[0].data
_clarify_sse_notify(session_key, dict(q[0].data), len(q))
else:
_clear_queue_locked(session_key)
_clarify_sse_notify(session_key, None, 0)
entry.result = response
entry.event.set()
return True
return False

28
api/routes.py
View File

@@ -2386,6 +2386,7 @@ try:
submit_pending as submit_clarify_pending,
get_pending as get_clarify_pending,
resolve_clarify,
resolve_clarify_by_id,
sse_subscribe as clarify_sse_subscribe,
sse_unsubscribe as clarify_sse_unsubscribe,
)
@@ -2394,6 +2395,7 @@ except ImportError:
get_clarify_pending = lambda *a, **k: None
clarify_sse_subscribe = None
resolve_clarify = lambda *a, **k: 0
resolve_clarify_by_id = lambda *a, **k: False
# ── Login page locale strings ─────────────────────────────────────────────────
@@ -9043,14 +9045,16 @@ def _handle_approval_respond(handler, body):
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.
# When a stable clarify_id is provided, match the specific entry so stale
# or late responses from the frontend are reliably rejected (issue #2639).
if clarify_id:
from api.clarify import resolve_clarify_by_id
return resolve_clarify_by_id(sid, clarify_id, response)
# Legacy path: resolve the oldest pending entry. Return the REAL result
# instead of the old unconditional True so the frontend can detect when
# there is no pending prompt to resolve.
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)
return bool(resolved)
def _handle_clarify_respond(handler, body):
@@ -9074,7 +9078,15 @@ def _handle_clarify_respond(handler, body):
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})
if not ok:
return j(handler, {
"ok": False,
"error": "Clarification prompt expired or not found. The agent may have already proceeded.",
"stale": True,
}, status=409)
return j(handler, {"ok": True, "response": response})
class _ManualCompressionMemoryHandler:

49
static/messages.js
View File

@@ -2434,6 +2434,7 @@ let _clarifyHideTimer = null;
let _clarifyVisibleSince = 0;
let _clarifySignature = '';
let _clarifySessionId = null;
let _clarifyId = null;
let _clarifyMissingEndpointWarned = false;
let _clarifyCountdownTimer = null;
let _clarifyExpiresAt = 0;
@@ -2598,6 +2599,7 @@ function _resetClarifyCardState() {
_clearClarifyCountdownTimer();
_clarifyVisibleSince = 0;
_clarifySignature = '';
_clarifyId = null;
}
function hideClarifyCard(force=false, reason="dismissed") {
@@ -2673,6 +2675,7 @@ function showClarifyCard(pending) {
const input = $("clarifyInput");
const sameClarify = card.classList.contains("visible") && _clarifySignature === sig;
_clarifySessionId = sid;
_clarifyId = pending.clarify_id || null;
_clarifySignature = sig;
_startClarifyCountdown(pending);
if (!sameClarify) {
@@ -2752,16 +2755,50 @@ async function respondClarify(response) {
if (input) input.focus();
return;
}
_clarifySessionId = null;
_clearClarifyPendingForSession(sid);
const clarifyId = _clarifyId;
// Keep a draft copy so we can restore the input on failure (issue #2639).
const draft = value;
_clarifySetControlsDisabled(true, true);
hideClarifyCard(true, 'sent');
try {
await api("/api/clarify/respond", {
const result = await api("/api/clarify/respond", {
method: "POST",
body: JSON.stringify({ session_id: sid, response: value })
body: JSON.stringify({ session_id: sid, response: value, clarify_id: clarifyId || "" })
});
} catch(e) { setStatus(t("clarify_responding") + " " + e.message); }
if (result && result.ok) {
// Only clear/hide if the visible prompt still matches what was just
// submitted. If a parallel SSE event already loaded the next queued
// prompt, erasing the session cache would leave the agent waiting
// until timeout (codex review P1, issue #2639).
if (_clarifyId === clarifyId) {
_clarifySessionId = null;
_clarifyId = null;
_clearClarifyPendingForSession(sid);
hideClarifyCard(true, 'sent');
}
} else {
// Stale / expired / wrong session — keep the card and draft visible.
_clarifySetControlsDisabled(false, false);
if (input) {
input.value = draft;
input.focus();
}
const errMsg = (result && result.error) || "Clarification response not accepted — the agent may have already proceeded.";
if (typeof showToast === "function") showToast(errMsg, 5000);
if (typeof setStatus === "function") setStatus(errMsg);
}
} catch(e) {
// Stale (409) or network error — keep the card and draft visible so the user can retry.
_clarifySetControlsDisabled(false, false);
if (input) {
input.value = draft;
input.focus();
}
const errMsg = (e && e.status === 409)
? (e.message || "Clarification prompt expired or not found.")
: ((e && e.message) || "Failed to deliver clarification response.");
if (typeof setStatus === "function") setStatus("Clarify: " + errMsg);
if (typeof showToast === "function") showToast(errMsg, 5000);
}
}
var _clarifyEventSource = null;

8
tests/test_clarify_unblock.py
View File

@@ -137,14 +137,16 @@ class TestClarifyModuleExports:
class TestClarifyHTTPEndpoints:
"""Regression tests for /api/clarify/respond against the live test server."""
def test_respond_returns_ok_no_pending(self):
def test_respond_returns_stale_when_no_pending(self):
"""When no clarify prompt is pending, respond returns 409 (issue #2639)."""
sid = f"http-no-pending-{uuid.uuid4().hex[:8]}"
result, status = post("/api/clarify/respond", {
"session_id": sid,
"response": "Use option A",
})
assert status == 200
assert result["ok"] is True
assert status == 409
assert result["ok"] is False
assert result.get("stale") is True
def test_respond_requires_session_id(self):
result, status = post("/api/clarify/respond", {"response": "Hello"})

13
tests/test_sprint30.py
View File

@@ -617,17 +617,26 @@ class TestClarifyCardTimerLogic:
assert any(prop in body for prop in ('box-shadow', 'outline', 'border', 'text-decoration')), \
'urgent countdown styling must include a non-color visual cue'
def test_respond_clarify_calls_hide_with_force(self):
def test_respond_clarify_sends_clarify_id_and_waits_for_ack(self):
src = self._get_js().read_text()
import re
m = re.search(r'async function respondClarify.*?(?=\nasync function|\nfunction |\Z)',
src, re.DOTALL)
assert m, 'respondClarify function not found'
body = m.group(0)
assert 'clarify_id' in body, \
'respondClarify must send clarify_id to the backend for stable matching (issue #2639)'
assert 'hideClarifyCard(true' in body, \
'respondClarify must call hideClarifyCard(true) so card hides immediately after user clicks'
'respondClarify must still hide the card on successful acknowledgement'
assert "'sent'" in body, \
'respondClarify must mark user-submitted hides so drafts are not re-stashed'
assert 'result && result.ok' in body, \
'respondClarify must check ok before hiding the clarify card (issue #2639)'
# The card must NOT be hidden before the API call — it should wait for the response.
hide_idx = body.index('hideClarifyCard(true')
api_idx = body.index('/api/clarify/respond')
assert hide_idx > api_idx, \
'respondClarify must wait for the API response before calling hideClarifyCard (issue #2639)'
def test_clarify_poll_loop_uses_no_force(self):
src = self._get_js().read_text()