diff --git a/api/clarify.py b/api/clarify.py index 4fbbfc35..c827ab0c 100644 --- a/api/clarify.py +++ b/api/clarify.py @@ -7,9 +7,11 @@ clarification string instead of an approval decision. from __future__ import annotations import threading +import time from typing import Optional +DEFAULT_TIMEOUT_SECONDS = 120 _lock = threading.Lock() _pending: dict[str, dict] = {} _gateway_queues: dict[str, list] = {} @@ -57,8 +59,20 @@ def clear_pending(session_key: str) -> int: return len(entries) +def _with_timeout_metadata(data: dict) -> dict: + item = dict(data or {}) + requested_at = float(item.get("requested_at") or time.time()) + timeout_seconds = int(item.get("timeout_seconds") or DEFAULT_TIMEOUT_SECONDS) + expires_at = float(item.get("expires_at") or requested_at + timeout_seconds) + item["requested_at"] = requested_at + item["timeout_seconds"] = timeout_seconds + item["expires_at"] = expires_at + return item + + def submit_pending(session_key: str, data: dict) -> _ClarifyEntry: """Queue a pending clarify request and notify the UI callback if registered.""" + data = _with_timeout_metadata(data) with _lock: queue = _gateway_queues.setdefault(session_key, []) # De-duplicate while unresolved: if the most recent pending clarify is diff --git a/static/index.html b/static/index.html index c28a35f5..295cdf5b 100644 --- a/static/index.html +++ b/static/index.html @@ -297,6 +297,7 @@
Clarification needed +
diff --git a/static/messages.js b/static/messages.js index 73518d05..7bb3f2d2 100644 --- a/static/messages.js +++ b/static/messages.js @@ -230,7 +230,7 @@ async function send(){ stopClarifyPolling(); // Only hide approval card if it belongs to the session that just finished if(!_approvalSessionId || _approvalSessionId===activeSid) hideApprovalCard(true);removeThinking(); - if(!_clarifySessionId || _clarifySessionId===activeSid) hideClarifyCard(true); + if(!_clarifySessionId || _clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); S.messages.push({role:'assistant',content:`**Error:** ${errMsg}`}); _queueDrainSid=activeSid;renderMessages();setBusy(false);setComposerStatus(`Error: ${errMsg}`); return; @@ -785,7 +785,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ stopApprovalPolling(); stopClarifyPolling(); if(!_approvalSessionId || _approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId || _clarifySessionId===activeSid) hideClarifyCard(true); + if(!_clarifySessionId || _clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); if(isActiveSession){ S.activeStreamId=null; } @@ -932,7 +932,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ source.close(); delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPolling();stopClarifyPolling(); if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true); + if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; clearLiveToolCards();if(!assistantText)removeThinking(); @@ -1010,7 +1010,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ source.close(); delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPolling();stopClarifyPolling(); if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true); + if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; } @@ -1050,7 +1050,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPolling();stopClarifyPolling(); _closeSource(); if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true); + if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); const isSessionViewed=_isSessionActivelyViewed(activeSid); const completedSid=session.session_id||activeSid; if(!isSessionViewed && typeof _markSessionCompletionUnread==='function'){ @@ -1091,7 +1091,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPolling();stopClarifyPolling(); _closeSource(); if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true); + if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; clearLiveToolCards();if(!assistantText)removeThinking(); @@ -1119,7 +1119,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ stopApprovalPolling(); stopClarifyPolling(); if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true); + if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; clearLiveToolCards(); @@ -1331,6 +1331,8 @@ let _clarifyVisibleSince = 0; let _clarifySignature = ''; let _clarifySessionId = null; let _clarifyMissingEndpointWarned = false; +let _clarifyCountdownTimer = null; +let _clarifyExpiresAt = 0; const CLARIFY_MIN_VISIBLE_MS = 30000; function _ensureClarifyCardDom() { @@ -1349,6 +1351,7 @@ function _ensureClarifyCardDom() {
Clarification needed +
@@ -1373,13 +1376,84 @@ function _clearClarifyHideTimer() { } } +function _clearClarifyCountdownTimer() { + if (_clarifyCountdownTimer) { + clearInterval(_clarifyCountdownTimer); + _clarifyCountdownTimer = null; + } + _clarifyExpiresAt = 0; + const countdown = $("clarifyCountdown"); + if (countdown) { + countdown.textContent = ""; + countdown.classList.remove("urgent"); + } +} + +function _clarifyExpiryMs(pending) { + const expiresAt = Number(pending && pending.expires_at); + if (Number.isFinite(expiresAt) && expiresAt > 0) return expiresAt * 1000; + const requestedAt = Number(pending && pending.requested_at); + const timeoutSeconds = Number(pending && pending.timeout_seconds); + if (Number.isFinite(requestedAt) && Number.isFinite(timeoutSeconds)) { + return (requestedAt + timeoutSeconds) * 1000; + } + return 0; +} + +function _updateClarifyCountdown() { + const countdown = $("clarifyCountdown"); + if (!countdown || !_clarifyExpiresAt) return; + const remaining = Math.max(0, Math.ceil((_clarifyExpiresAt - Date.now()) / 1000)); + countdown.textContent = `${remaining}s`; + countdown.classList.toggle("urgent", remaining <= 10); +} + +function _startClarifyCountdown(pending) { + _clearClarifyCountdownTimer(); + _clarifyExpiresAt = _clarifyExpiryMs(pending); + if (!_clarifyExpiresAt) return; + _updateClarifyCountdown(); + _clarifyCountdownTimer = setInterval(_updateClarifyCountdown, 1000); +} + +function _stashClarifyDraft(reason) { + if (reason !== "expired" && reason !== "terminal") return false; + const input = $("clarifyInput"); + const draft = String((input && input.value) || "").trim(); + if (!draft) return false; + const sid = _clarifySessionId || (S.session && S.session.session_id) || "unknown"; + const key = `hermes-clarify-draft-${sid}-${_clarifySignature || "unknown"}`; + try { + sessionStorage.setItem(key, JSON.stringify({ + draft, + reason, + saved_at: Date.now(), + })); + } catch (_) {} + const composer = $('msg'); + if (composer) { + const current = String(composer.value || ""); + composer.value = current.trim() ? `${current.replace(/\s+$/, "")}\n\n${draft}` : draft; + if (typeof autoResize === "function") autoResize(); + if (typeof updateSendBtn === "function") updateSendBtn(); + } + const notice = reason === "expired" + ? "Clarification timed out. Your draft was kept in the composer." + : "Clarification closed. Your draft was kept in the composer."; + if (typeof setComposerStatus === "function") setComposerStatus(notice); + else if (typeof setStatus === "function") setStatus(notice); + if (typeof showToast === "function") showToast(notice, 5000); + return true; +} + function _resetClarifyCardState() { _clearClarifyHideTimer(); + _clearClarifyCountdownTimer(); _clarifyVisibleSince = 0; _clarifySignature = ''; } -function hideClarifyCard(force=false) { +function hideClarifyCard(force=false, reason="dismissed") { const card = $("clarifyCard"); if (!card) { _clarifySessionId = null; @@ -1387,7 +1461,7 @@ function hideClarifyCard(force=false) { if (typeof unlockComposerForClarify === "function") unlockComposerForClarify(); return; } - if (!force && _clarifyVisibleSince) { + if (!force && reason !== "expired" && _clarifyVisibleSince) { const remaining = CLARIFY_MIN_VISIBLE_MS - (Date.now() - _clarifyVisibleSince); if (remaining > 0) { const scheduledSignature = _clarifySignature; @@ -1395,11 +1469,12 @@ function hideClarifyCard(force=false) { _clarifyHideTimer = setTimeout(() => { _clarifyHideTimer = null; if (_clarifySignature !== scheduledSignature) return; - hideClarifyCard(true); + hideClarifyCard(true, reason); }, remaining); return; } } + _stashClarifyDraft(reason); _clarifySessionId = null; _resetClarifyCardState(); card.classList.remove("visible"); @@ -1450,6 +1525,7 @@ function showClarifyCard(pending) { const sameClarify = card.classList.contains("visible") && _clarifySignature === sig; _clarifySessionId = pending._session_id || (S.session && S.session.session_id) || null; _clarifySignature = sig; + _startClarifyCountdown(pending); if (!sameClarify) { _clarifyVisibleSince = Date.now(); _clearClarifyHideTimer(); @@ -1529,7 +1605,7 @@ async function respondClarify(response) { } _clarifySessionId = null; _clarifySetControlsDisabled(true, true); - hideClarifyCard(true); + hideClarifyCard(true, 'sent'); try { await api("/api/clarify/respond", { method: "POST", @@ -1543,12 +1619,12 @@ function startClarifyPolling(sid) { _clarifyMissingEndpointWarned = false; _clarifyPollTimer = setInterval(async () => { if (!S.session || S.session.session_id !== sid) { - stopClarifyPolling(); hideClarifyCard(true); return; + stopClarifyPolling(); hideClarifyCard(true, 'session'); return; } try { const data = await api("/api/clarify/pending?session_id=" + encodeURIComponent(sid)); if (data.pending) { data.pending._session_id=sid; showClarifyCard(data.pending); } - else { hideClarifyCard(); } + else { hideClarifyCard(false, 'expired'); } } catch(e) { const msg = String((e && e.message) || ""); if (!_clarifyMissingEndpointWarned && /(^|\b)(404|not found)(\b|$)/i.test(msg)) { diff --git a/static/style.css b/static/style.css index 91762b33..ccda1f4a 100644 --- a/static/style.css +++ b/static/style.css @@ -472,6 +472,8 @@ .clarify-card.visible .clarify-inner{transform:translateY(0);opacity:1;} .clarify-inner{background:var(--surface);backdrop-filter:blur(8px);border:1px solid var(--accent-bg-strong);border-radius:12px;padding:12px 14px 36px;box-shadow:0 1px 0 rgba(255,255,255,.02) inset;} .clarify-header{display:flex;align-items:center;gap:8px;margin-bottom:10px;font-size:12px;font-weight:700;color:var(--blue);letter-spacing:.01em;} + .clarify-countdown{margin-left:auto;min-width:42px;text-align:right;color:var(--muted);font-variant-numeric:tabular-nums;font-weight:700;} + .clarify-countdown.urgent{color:var(--error);} .clarify-question{font-size:14px;color:var(--text);line-height:1.7;white-space:pre-wrap;margin-bottom:12px;} .clarify-choices{display:flex;flex-direction:column;gap:8px;margin-bottom:12px;} .clarify-choice{display:flex;align-items:flex-start;gap:10px;width:100%;padding:11px 14px;border-radius:12px;font-size:13px;font-weight:600;border:1px solid var(--accent-bg-strong);background:var(--accent-bg);color:var(--accent-text);cursor:pointer;transition:all .15s;white-space:normal;text-align:left;box-shadow:0 1px 0 rgba(255,255,255,.03) inset;} diff --git a/tests/test_clarify_unblock.py b/tests/test_clarify_unblock.py index 89fb7d21..1170f880 100644 --- a/tests/test_clarify_unblock.py +++ b/tests/test_clarify_unblock.py @@ -1,7 +1,6 @@ """Tests for clarify prompt unblocking and HTTP endpoints.""" import json -import threading import uuid import urllib.request import urllib.error @@ -9,6 +8,8 @@ import urllib.parse import pytest +from tests._pytest_port import BASE + try: from api.clarify import ( register_gateway_notify, @@ -30,8 +31,6 @@ pytestmark = pytest.mark.skipif( reason="api.clarify not available in this environment", ) -from tests._pytest_port import BASE - def get(path): url = BASE + path @@ -95,12 +94,27 @@ class TestClarifyUnblocking: sid = f"unit-submit-{uuid.uuid4().hex[:8]}" data = {"question": "Pick", "choices_offered": ["one", "two"], "session_id": sid} entry = submit_pending(sid, data) - assert entry.data == data + assert entry.data["question"] == data["question"] + assert entry.data["choices_offered"] == data["choices_offered"] + assert entry.data["session_id"] == data["session_id"] with _lock: assert sid in _gateway_queues clear_pending(sid) + def test_submit_pending_adds_timeout_metadata(self): + sid = f"unit-timeout-{uuid.uuid4().hex[:8]}" + entry = submit_pending(sid, {"question": "Wait", "choices_offered": []}) + + assert isinstance(entry.data["requested_at"], (int, float)) + assert entry.data["timeout_seconds"] == 120 + assert entry.data["expires_at"] == pytest.approx( + entry.data["requested_at"] + 120, + abs=0.1, + ) + + clear_pending(sid) + class TestClarifyModuleExports: def test_register_gateway_notify_exported(self): diff --git a/tests/test_sprint30.py b/tests/test_sprint30.py index 328d670b..ca84aec0 100644 --- a/tests/test_sprint30.py +++ b/tests/test_sprint30.py @@ -12,13 +12,12 @@ Tests for: """ import json +import pathlib import re import urllib.request import urllib.error import urllib.parse -import pytest - from tests._pytest_port import BASE @@ -44,8 +43,6 @@ def read(path): with open(path, encoding="utf-8") as f: return f.read() - -import pathlib REPO = pathlib.Path(__file__).parent.parent @@ -518,6 +515,9 @@ class TestClarifyCardTimerLogic: def _get_js(self): return pathlib.Path(__file__).parent.parent / 'static' / 'messages.js' + def _get_html(self): + return pathlib.Path(__file__).parent.parent / 'static' / 'index.html' + def test_clarify_min_visible_ms_constant_present(self): src = self._get_js().read_text() assert 'CLARIFY_MIN_VISIBLE_MS' in src @@ -529,6 +529,7 @@ class TestClarifyCardTimerLogic: def test_hide_clarify_card_has_force_parameter(self): src = self._get_js().read_text() assert 'hideClarifyCard(force=false)' in src or \ + 'hideClarifyCard(force=false, reason=' in src or \ 'hideClarifyCard(force = false)' in src, \ 'hideClarifyCard must have force=false default parameter' @@ -548,6 +549,25 @@ class TestClarifyCardTimerLogic: src = self._get_js().read_text() assert '_clarifySignature' in src + def test_clarify_countdown_element_present(self): + html = self._get_html().read_text() + assert 'id="clarifyCountdown"' in html, \ + 'clarify card must include a countdown element so users see timeout risk' + + def test_clarify_countdown_uses_pending_expiry(self): + src = self._get_js().read_text() + assert '_clarifyCountdownTimer' in src + assert 'function _startClarifyCountdown' in src + assert 'expires_at' in src, \ + 'clarify countdown must use expires_at from the pending payload' + + def test_hide_clarify_card_can_preserve_draft(self): + src = self._get_js().read_text() + assert 'function _stashClarifyDraft' in src + assert 'sessionStorage.setItem' in src + assert "$('msg')" in src, \ + 'clarify timeout should keep the typed draft visible in the composer' + def test_respond_clarify_calls_hide_with_force(self): src = self._get_js().read_text() import re @@ -555,14 +575,15 @@ class TestClarifyCardTimerLogic: src, re.DOTALL) assert m, 'respondClarify function not found' body = m.group(0) - assert 'hideClarifyCard(true)' in body, \ + assert 'hideClarifyCard(true' in body, \ 'respondClarify must call hideClarifyCard(true) so card hides immediately after user clicks' + assert "'sent'" in body, \ + 'respondClarify must mark user-submitted hides so drafts are not re-stashed' def test_clarify_poll_loop_uses_no_force(self): src = self._get_js().read_text() - assert 'else { hideClarifyCard(); }' in src or \ - 'else {hideClarifyCard();}' in src or \ - 'else { hideClarifyCard() }' in src, \ + assert "else { hideClarifyCard(false, 'expired'); }" in src or \ + "else {hideClarifyCard(false,'expired');}" in src, \ 'Clarify poll loop should hide without force=true' def test_show_clarify_card_signature_dedup(self):