From 6f593f80e88f9c9f3c8a314cc4e5362c03e101c8 Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Mon, 14 Sep 2026 18:21:54 -0500 Subject: [PATCH] fix(notifications): preserve prompt owners through lifecycle exits --- README.md | 1 + static/boot.js | 2 + static/messages.js | 72 ++++++----- ...test_issue4754_approval_dismiss_persist.py | 48 +------- tests/test_prompt_notification_lifecycle.py | 112 ++++++++++++++++++ tests/test_pwa_notification_controls.py | 67 +---------- 6 files changed, 163 insertions(+), 139 deletions(-) create mode 100644 tests/test_prompt_notification_lifecycle.py diff --git a/README.md b/README.md index 7e73b9f1..e75397fa 100644 --- a/README.md +++ b/README.md @@ -291,6 +291,7 @@ If an AI assistant is helping with install, reinstall, bootstrap, provider setup - Unsaved changes guard -- discard/save prompt when closing with unpersisted changes - Cron completion alerts -- toast notifications and unread badges scoped to the active profile on the Tasks tab and session sidebar - Background agent error alerts -- banner when a non-active session encounters an error +- Approval and clarification browser alerts notify once per pending owner when its session is not actively viewed. Local approval dismissal hides attention without resolving the prompt; resolution, replacement, and cancellation retire that owner's alert state. Denied or failed notification delivery can retry while the prompt remains pending. ### Slash commands - Type `/` in the composer for autocomplete dropdown diff --git a/static/boot.js b/static/boot.js index 721f2b13..755751cc 100644 --- a/static/boot.js +++ b/static/boot.js @@ -93,6 +93,8 @@ async function cancelSessionStream(session){ else setStatus(''); } if(typeof _clearPendingPromptsForSession==='function') _clearPendingPromptsForSession(sid); + if(typeof stopApprovalPollingForSession==='function') stopApprovalPollingForSession(sid); + if(typeof stopClarifyPollingForSession==='function') stopClarifyPollingForSession(sid); if(typeof _approvalSessionId!=='undefined' && _approvalSessionId===sid){ stopApprovalPolling(); hideApprovalCard(true); diff --git a/static/messages.js b/static/messages.js index f160fe17..58ee6ed6 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7421,13 +7421,12 @@ let _approvalDisplayedOwner = null; const _DISMISSED_APPROVALS_KEY = 'hermes_dismissed_approvals'; -// Dismissed approvals are namespaced by session so that two sessions carrying -// the SAME approval_id (e.g. a gateway/run source that reuses externally -// supplied IDs across sessions) can't have a dismissal in one session hide the -// other's still-pending approval. Stored value is "\u0000". +// Dismissal uses the same injective full-owner tuple as notifications. +// Legacy session/ID-only tombstones cannot safely identify a gateway run. function _approvalDismissKey(sid, approvalId) { if (!approvalId) return ''; - return String(sid || '') + '\u0000' + String(approvalId); + const pending = typeof approvalId === 'object' ? approvalId : {approval_id: approvalId}; + return _promptNotifyKey('approval', sid, pending); } function _getDismissedApprovals() { @@ -7469,7 +7468,8 @@ function _approvalPromptBelongsToActiveSession(sid) { function activeSessionHasPendingPromptAttention() { const sid = _promptActiveSessionId(); return !!(sid && ( - _approvalPendingBySession.has(sid) || + (_approvalPendingBySession.has(sid) && + !_isApprovalDismissed(sid, _approvalPendingBySession.get(sid).pending)) || _clarifyPendingBySession.has(sid) )); } @@ -7486,10 +7486,6 @@ function _rememberApprovalPending(pending, pendingCount) { if (!pending) return null; const sid = pending._session_id || _promptActiveSessionId(); if (!sid) return null; - if (pending.approval_id && _isApprovalDismissed(sid, pending.approval_id)) { - _clearApprovalPendingForSession(sid); - return sid; - } const prev = _approvalPendingBySession.get(sid); // A replacement pending entry DISPLACES the previous prompt for this // session: retire the displaced prompt notification-dedupe key so the same @@ -7499,6 +7495,7 @@ function _rememberApprovalPending(pending, pendingCount) { if (prev && prev.pending && _promptNotifyKey("approval", sid, prev.pending) !== _promptNotifyKey("approval", sid, pending)) { _retirePromptNotifyKey("approval", sid, prev.pending); + _unmarkApprovalDismissed(sid, prev.pending); _bumpApprovalPromptGeneration(sid); } else if (!prev) { _bumpApprovalPromptGeneration(sid); @@ -7512,7 +7509,8 @@ function _clearApprovalPendingForSession(sid) { if (sid) { const entry = _approvalPendingBySession.get(sid); _approvalPendingBySession.delete(sid); - if (entry) _bumpApprovalPromptGeneration(sid); + _bumpApprovalPromptGeneration(sid); + if (entry && entry.pending) _unmarkApprovalDismissed(sid, entry.pending); if (entry && entry.pending) _retirePromptNotifyKey('approval', sid, entry.pending); if (typeof syncTopbar === 'function') syncTopbar(); } @@ -7656,7 +7654,7 @@ function showApprovalForSession(sid, pending, pendingCount) { function showApprovalCard(pending, pendingCount) { const sid = _rememberApprovalPending(pending, pendingCount); - if (pending && pending.approval_id && _isApprovalDismissed(sid, pending.approval_id)) return; + if (pending && pending.approval_id && _isApprovalDismissed(sid, pending)) return; if(typeof _notifyPromptCard==='function') _notifyPromptCard('approval', sid, pending); if (!_approvalPromptBelongsToActiveSession(sid)) return; _approvalClearedOwner = null; @@ -7717,10 +7715,11 @@ function showApprovalCard(pending, pendingCount) { function dismissApprovalCard() { const sid = _approvalSessionId; - if (_approvalCurrentId) _markApprovalDismissed(sid, _approvalCurrentId); - // Dismissal clears local attention while the server remains authoritative. - // A later poll of the same dismissed approval stays suppressed below. - _clearApprovalPendingForSession(sid); + const entry = _approvalPendingBySession.get(sid); + if (entry && entry.pending) _markApprovalDismissed(sid, entry.pending); + // Keep the unresolved owner for authoritative resolution/replacement. + // Local dismissal hides attention, not the server-owned prompt. + if (typeof syncTopbar === 'function') syncTopbar(); hideApprovalCard(true); } @@ -8509,7 +8508,7 @@ function _clearClarifyPendingForSession(sid) { if (sid) { const entry = _clarifyPendingBySession.get(sid); _clarifyPendingBySession.delete(sid); - if (entry) _bumpClarifyPromptGeneration(sid); + _bumpClarifyPromptGeneration(sid); if (entry && entry.pending) _retirePromptNotifyKey('clarify', sid, entry.pending); if (typeof syncTopbar === 'function') syncTopbar(); } @@ -9332,16 +9331,23 @@ function _notifyPromptCard(kind, sid, pending){ if (typeof _isSessionActivelyViewed === 'function' && _isSessionActivelyViewed(sid)) return; if (typeof sendBrowserNotification !== 'function') return; if (typeof window !== 'undefined' && !window._notificationsEnabled) return; - _promptNotifySeen.set(key, Date.now()); + const attempt = {}; + _promptNotifySeen.set(key, attempt); // forceHidden: this caller already made the visibility decision (the // actively-viewed gate above). sendBrowserNotification's own live gate // ("notify only when document.hidden") would otherwise veto the // unfocused-but-visible case, which this feature explicitly notifies for. - if (kind === 'clarify') { - sendBrowserNotification('Clarification needed', p.question || p.description || 'Tool clarification needed', { sid, forceHidden: true }); - } else { - sendBrowserNotification('Approval required', p.description || p.command || 'Tool approval needed', { sid, forceHidden: true }); - } + const rollback = () => { + if (_promptNotifySeen.get(key) === attempt) _promptNotifySeen.delete(key); + }; + try { + const delivery = kind === 'clarify' + ? sendBrowserNotification('Clarification needed', p.question || p.description || 'Tool clarification needed', { sid, forceHidden: true }) + : sendBrowserNotification('Approval required', p.description || p.command || 'Tool approval needed', { sid, forceHidden: true }); + Promise.resolve(delivery).then(accepted => { + if (accepted !== true) rollback(); + }, rollback); + } catch (_) { rollback(); } } function sendBrowserNotification(title,body,options={}){ const force=!!(options&&options.force); @@ -9355,14 +9361,24 @@ function sendBrowserNotification(title,body,options={}){ if(!force&&!window._notificationsEnabled) return; if(!force&&!forceHidden&&!_isBackgroundedForBrowserNotification()) return; if(!('Notification' in window)) return; - if(Notification.permission==='granted'){ - _showPwaNotification(title,body,options).catch(()=>{try{new Notification(title||assistantDisplayName(),_notificationOptions(body,options));}catch(_err){}}); - }else if(Notification.permission==='denied'){ + const deliver = async () => { + try { + await _showPwaNotification(title,body,options); + return true; + } catch (_) { + try { + new Notification(title||assistantDisplayName(),_notificationOptions(body,options)); + return true; + } catch (_err) { return false; } + } + }; + if(Notification.permission==='granted') return deliver(); + if(Notification.permission==='denied'){ // Explicit "Send test" (force) deserves feedback instead of a silent no-op. if(force&&typeof showToast==='function') showToast(t('notifications_denied'),3500,'error'); - }else{ - requestNotificationPermission().then(p=>{if(p==='granted') _showPwaNotification(title,body,options).catch(()=>{try{new Notification(title||assistantDisplayName(),_notificationOptions(body,options));}catch(_err){}});}); + return false; } + return requestNotificationPermission().then(p => p==='granted' ? deliver() : false).catch(() => false); } // ── /btw ephemeral stream ──────────────────────────────────────────────────── diff --git a/tests/test_issue4754_approval_dismiss_persist.py b/tests/test_issue4754_approval_dismiss_persist.py index 2a97f379..878a56de 100644 --- a/tests/test_issue4754_approval_dismiss_persist.py +++ b/tests/test_issue4754_approval_dismiss_persist.py @@ -53,45 +53,12 @@ def test_dismissed_set_capped_at_100(): # Guard in showApprovalCard # --------------------------------------------------------------------------- -def test_guard_in_show_approval_card(): - compact = _compact(MESSAGES_JS) - # Guard must appear inside showApprovalCard, after _rememberApprovalPending - func_start = compact.find("functionshowApprovalCard(") - assert func_start != -1 - # Locate the guard after the function start (dismissals are namespaced by - # session, so the guard passes sid + approval_id). - guard = "_isApprovalDismissed(sid,pending.approval_id)" - guard_idx = compact.find(guard, func_start) - assert guard_idx != -1, "guard _isApprovalDismissed must appear in showApprovalCard" - # _rememberApprovalPending must appear before the guard - remember = "_rememberApprovalPending(" - remember_idx = compact.find(remember, func_start) - assert remember_idx != -1 - assert remember_idx < guard_idx, "guard must come after _rememberApprovalPending" -def test_guard_returns_early(): - # The guard must be a return statement - compact = _compact(MESSAGES_JS) - assert "if(pending&&pending.approval_id&&_isApprovalDismissed(sid,pending.approval_id))return;" in compact - - -# --------------------------------------------------------------------------- -# dismissApprovalCard function -# --------------------------------------------------------------------------- - def test_dismiss_approval_card_defined(): assert "function dismissApprovalCard(" in MESSAGES_JS -def test_dismiss_approval_card_marks_dismissed(): - compact = _compact(MESSAGES_JS) - func_start = compact.find("functiondismissApprovalCard(") - assert func_start != -1 - body_end = compact.find("}", func_start) - body = compact[func_start:body_end + 1] - assert "_markApprovalDismissed(sid,_approvalCurrentId)" in body - def test_dismiss_approval_card_hides_card(): compact = _compact(MESSAGES_JS) @@ -102,19 +69,6 @@ def test_dismiss_approval_card_hides_card(): assert "hideApprovalCard(true)" in body -def test_dismiss_approval_card_clears_pending_attention(): - # dismissApprovalCard must call _clearApprovalPendingForSession so the tab - # indicator stops blinking after dismiss. Without this, _rememberApprovalPending - # reinsertes the pending entry on every poll tick and the indicator stays lit. - compact = _compact(MESSAGES_JS) - func_start = compact.find("functiondismissApprovalCard(") - assert func_start != -1 - body_end = compact.find("}", func_start) - body = compact[func_start:body_end + 1] - assert "_clearApprovalPendingForSession(sid)" in body, ( - "dismissApprovalCard must call _clearApprovalPendingForSession(sid)" - ) - def test_dismiss_approval_card_captures_sid_before_hide(): # The session ID must be captured before hideApprovalCard(true) nulls out @@ -286,7 +240,7 @@ def test_same_approval_id_in_two_sessions_does_not_collide(): pytest.skip("node not available") helpers = "\n".join( _extract_fn(MESSAGES_JS, n) - for n in ("_approvalDismissKey", "_getDismissedApprovals", + for n in ("_promptNotifyKey", "_approvalDismissKey", "_getDismissedApprovals", "_isApprovalDismissed", "_markApprovalDismissed", "_unmarkApprovalDismissed") ) script = ( diff --git a/tests/test_prompt_notification_lifecycle.py b/tests/test_prompt_notification_lifecycle.py new file mode 100644 index 00000000..d76ef2fa --- /dev/null +++ b/tests/test_prompt_notification_lifecycle.py @@ -0,0 +1,112 @@ +"""Composed prompt-owner and real delivery regressions for #7493.""" +from tests.test_pwa_notification_controls import MESSAGES_JS, _extract_fn, _run_node + + +def run(body, extra=()): + names = ['_promptNotifyKey', '_retirePromptNotifyKey', '_notifyPromptCard', + '_approvalPromptGeneration', '_bumpApprovalPromptGeneration', + '_clarifyPromptGeneration', '_bumpClarifyPromptGeneration', + '_rememberApprovalPending', '_clearApprovalPendingForSession', + '_clearClarifyPendingForSession', '_approvalDismissKey', + '_getDismissedApprovals', '_isApprovalDismissed', '_markApprovalDismissed', + '_unmarkApprovalDismissed', 'dismissApprovalCard', + '_startApprovalFallbackPoll', 'showApprovalCard', + 'activeSessionHasPendingPromptAttention'] + source = '\n'.join(_extract_fn(MESSAGES_JS, n) for n in names + list(extra)) + return _run_node(''' +const _promptNotifySeen = new Map(); +const _approvalPendingBySession = new Map(), _clarifyPendingBySession = new Map(); +const _approvalPromptGenerationBySession = new Map(), _clarifyPromptGenerationBySession = new Map(); +const _DISMISSED_APPROVALS_KEY = 'dismissed'; +const store = new Map(); +const localStorage = {getItem:k=>store.get(k), setItem:(k,v)=>store.set(k,v)}; +const S = {busy:true,session:{session_id:'s'}}; +const _approvalPromptBelongsToActiveSession=()=>false; +let _approvalSessionId = 's', _approvalCurrentId = 'id'; +let _approvalFallbackPollInFlight=false, _approvalPollTimer; +let _clarifyPollingSessionId, _clarifyPollTimer, _clarifyFallbackPollInFlight=false; +const _promptActiveSessionId=()=> 's'; +const _approvalPollingSessionMissingOrMismatched=()=>false; +const _hideApprovalCardIfOwner=()=>{}; +const stopApprovalPolling=()=>{}, stopApprovalPollingForSession=()=>{}; +const hideApprovalCard=()=>{}; +const setInterval=()=>1; +const _isSessionActivelyViewed=()=>false; +const flush=()=>new Promise(r=>setImmediate(r)); +''' + source + '\n(async()=>{\n' + body + '\n})().catch(e=>{console.error(e);process.exit(1)});') + + +def test_dismiss_poll_resolution_and_new_run_owner(): + result = run(''' +const sent=[]; +global.sendBrowserNotification=()=>{sent.push(1);return true}; +const a={_session_id:'s',approval_id:'id',run_id:'r1',_gateway_mirror_token:'t1'}; +global.showApprovalForSession=(sid,p)=>showApprovalCard(p,1); +showApprovalForSession('s',a); +dismissApprovalCard(); +const dismissed=_isApprovalDismissed('s',a); +if(activeSessionHasPendingPromptAttention())throw Error('dismissed attention remains'); +showApprovalForSession('s',a); +const quiet=sent.length; +const b={...a,run_id:'r2',_gateway_mirror_token:'t2'}; +showApprovalForSession('s',b); +const replacement=sent.length; +dismissApprovalCard(); +global.api=async()=>({pending:null}); +_startApprovalFallbackPoll('s');await flush(); +showApprovalForSession('s',b); +console.log(JSON.stringify({dismissed,quiet,replacement,reused:sent.length})); +''') + assert result == dict(dismissed=True, quiet=1, replacement=2, reused=3) + + +def test_empty_map_terminal_clear_fences_inflight_poll(): + result = run(''' +let resolve;const shown=[]; +global.api=()=>new Promise(r=>resolve=r); +global.showApprovalForSession=(sid,p)=>shown.push(p); +_startApprovalFallbackPoll('s'); +_clearApprovalPendingForSession('s'); +_clearClarifyPendingForSession('s'); +resolve({pending:{approval_id:'stale'}});await flush(); +console.log(JSON.stringify({shown,clarifyGeneration:_clarifyPromptGeneration('s')})); +''') + assert result == dict(shown=[], clarifyGeneration=1) + + +def test_real_delivery_denied_failed_and_inflight_retry(): + result = run(''' +const window=global.window={_notificationsEnabled:true}; +let attempts=0,fail=false; +window.Notification=global.Notification=function(){attempts++;if(fail)throw Error('unavailable')}; +Notification.permission='denied'; +global.assistantDisplayName=()=> 'Hermes'; +global._notificationOptions=()=>({}); +Object.defineProperty(global,'navigator',{value:{serviceWorker:{getRegistration:async()=>({active:true,showNotification:async()=>{throw Error('SW failed')}})}}}); +const p={approval_id:'id'}; +_notifyPromptCard('approval','s',p);await flush(); +Notification.permission='granted';fail=true; +_notifyPromptCard('approval','s',p);await flush(); +const failedConsumed=_promptNotifySeen.size; +fail=false; +_notifyPromptCard('approval','s',p); +_notifyPromptCard('approval','s',p);await flush(); +_notifyPromptCard('approval','s',p);await flush(); +console.log(JSON.stringify({attempts,failedConsumed,consumed:_promptNotifySeen.size})); +''', ('sendBrowserNotification', '_showPwaNotification')) + assert result == dict(attempts=2, failedConsumed=0, consumed=1) + + +def test_empty_clarify_poll_cannot_resurrect_after_terminal_clear(): + result = run(''' +let _clarifyPollingSessionId, _clarifyPollTimer, _clarifyFallbackPollInFlight=false; +let resolve;const shown=[]; +global.api=()=>new Promise(r=>resolve=r); +global.showClarifyForSession=(sid,p)=>shown.push(p); +_startClarifyFallbackPoll('s'); +_clearClarifyPendingForSession('s'); +resolve({pending:{clarify_id:'stale'}});await flush(); +console.log(JSON.stringify({shown})); +''', ('_startClarifyFallbackPoll',)) + assert result == dict(shown=[]) + diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index beaad244..5b249660 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -43,73 +43,11 @@ def test_notification_payload_uses_completion_session_when_provided(): assert "assistantText?assistantText.slice(0,100)" not in MESSAGES_JS -def test_prompt_notifications_fire_from_card_renderer_chokepoint(): - """Approval/clarify notifications are owned by the card renderers, so EVERY - surfacing path (live SSE, 1.5s fallback poll, post-respond 'next approval' - refresh, reload-while-pending) notifies — not just the active-session SSE - event. The SSE listeners no longer send notifications directly; the shared - _notifyPromptCard gate dedupes per prompt id so repeated poll ticks and - re-renders ping exactly once.""" - # The chokepoint helper exists and both card renderers call it BEFORE the - # belongs-to-active-session guard (so a non-active session's prompt still - # notifies). - assert "function _notifyPromptCard(kind, sid, pending){" in MESSAGES_JS - for fn_name, kind in (("showApprovalCard", "approval"), ("showClarifyCard", "clarify")): - start = MESSAGES_JS.index(f"function {fn_name}(") - body_start = MESSAGES_JS.index("{", start) - depth = 0 - for i in range(body_start, len(MESSAGES_JS)): - if MESSAGES_JS[i] == "{": - depth += 1 - elif MESSAGES_JS[i] == "}": - depth -= 1 - if depth == 0: - body = MESSAGES_JS[start : i + 1] - break - assert "_notifyPromptCard(" in body, f"{fn_name} must route notifications through _notifyPromptCard" - assert "_notifyPromptCard('approval', sid, pending);" in MESSAGES_JS - assert "_notifyPromptCard('clarify', sid, pending);" in MESSAGES_JS - # Dedupe gate: per logical owner - an INJECTIVE typed-tuple key - # (JSON.stringify of [kind, sid, prompt id, gateway run id, mirror token]), - # so delimiter-bearing producer strings cannot collide. - assert "function _promptNotifyKey(kind, sid, pending){" in MESSAGES_JS - assert "return JSON.stringify([kind, String(sid || ''), String(id)," in MESSAGES_JS - assert "const key = _promptNotifyKey(kind, sid, p);" in MESSAGES_JS - assert "if (_promptNotifySeen.has(key)) return;" in MESSAGES_JS - # Entries retire by prompt LIFECYCLE (the pending-clear chokepoints), never - # by wall-clock age - a prompt pending >10min must not re-notify. - assert "_PROMPT_NOTIFY_TTL_MS" not in MESSAGES_JS - assert "function _retirePromptNotifyKey(kind, sid, pending){" in MESSAGES_JS - assert "_retirePromptNotifyKey('approval', sid, entry.pending);" in MESSAGES_JS - assert "_retirePromptNotifyKey('clarify', sid, entry.pending);" in MESSAGES_JS - # Disabled notifications must not consume the notify, so enabling - # mid-prompt still re-notifies that owner. - assert "!window._notificationsEnabled) return;" in MESSAGES_JS - # Visibility gate: suppress only while the prompt's session is actively - # viewed (open in pane + tab visible + tab focused). All three of the - # user's notification cases fire: non-active session, hidden tab, - # unfocused window. - assert "_isSessionActivelyViewed(sid)) return;" in MESSAGES_JS - # SSE listeners delegate; they must not send directly (would bypass dedupe). - approval_listener = _source_between( - "source.addEventListener('approval',e=>{", - "source.addEventListener('clarify',e=>{", - ) - clarify_listener = _source_between( - "source.addEventListener('clarify',e=>{", - "source.addEventListener('state_saved',e=>{", - ) - assert "sendBrowserNotification(" not in approval_listener - assert "sendBrowserNotification(" not in clarify_listener - assert "_notifyPromptCard()" in approval_listener # ownership comment anchor - assert "_notifyPromptCard()" in clarify_listener - - def _extract_fn(src: str, name: str) -> str: marker = f"function {name}" start = src.find(marker) assert start >= 0, f"{name} not found" - brace = src.find("{", start) + brace = src.find("{", src.find(")", start)) depth = 0 for i in range(brace, len(src)): if src[i] == "{": @@ -389,7 +327,7 @@ const ownerKey = _promptNotifyKey('approval', 'sid-1', {{ approval_id: 'd1' }}); process.stdout.write(JSON.stringify({{ count: sent.length, bodies: sent.map(s => s.body), consumed: _promptNotifySeen.has(ownerKey) }})); """ result = _run_node(script) - assert result["count"] == 2 or result["count"] == 1, f"unexpected ping count (got {result})" + assert result["count"] == 1, f"unexpected ping count (got {result})" assert result["bodies"][0] == 'after enabling', f"enabling mid-prompt must re-notify the same owner (got {result})" assert result["consumed"] is True, "the enabled notify must be recorded (consumed) for the owner" @@ -414,6 +352,7 @@ def test_displaced_pending_prompt_key_is_retired_on_replacement(): script = f""" const _promptNotifySeen = new Map(); const _isApprovalDismissed = () => false; +const _unmarkApprovalDismissed = () => {{}}; const S = {{ session: {{ session_id: "sid-1" }} }}; {keyfn} {retire}