From 09e419d5ed023c407ca2e01117385852d8fe4f1a Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:15:49 -0500 Subject: [PATCH 01/13] fix(notifications): browser notification on every approval/clarify surfacing, not just the SSE event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approval/clarify prompts block the run until answered, but only the active-session SSE event ever sent a browser notification — and it was gated on document.hidden at delivery time, which background-tab SSE throttling makes unreliable (the same #4416 class fixed for completion notices). The 1.5s fallback poll, the post-respond 'next approval' refresh, and reload-while-pending showed the card silently. Notification ownership moves into the card renderers (showApprovalCard / showClarifyCard) via _notifyPromptCard: per-id dedupe (poll ticks re-render without re-pinging), suppressed only while the tab is visible AND focused (an unfocused-but-visible window still pings, since nothing progresses until the prompt is answered). --- static/messages.js | 42 ++++++- tests/test_pwa_notification_controls.py | 144 +++++++++++++++++++++++- 2 files changed, 182 insertions(+), 4 deletions(-) diff --git a/static/messages.js b/static/messages.js index 2d0c96d6..60be3cd6 100644 --- a/static/messages.js +++ b/static/messages.js @@ -5980,7 +5980,9 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ _applyToAnchor('approval',d,e); showApprovalForSession(activeSid, d, d.pending_count || 1); playAttentionSound(_attentionSoundKey(activeSid,'approval',1)); - sendBrowserNotification('Approval required',d.description||'Tool approval needed',{sid:activeSid}); + // Browser notification is owned by showApprovalCard()/_notifyPromptCard() + // so every surfacing path (SSE, fallback poll, post-respond refresh, + // reload) dedupes through the same per-id gate. }); source.addEventListener('clarify',e=>{ @@ -5988,7 +5990,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ _applyToAnchor('clarify',d,e); showClarifyForSession(activeSid, d); playAttentionSound(_attentionSoundKey(activeSid,'clarify',1)); - sendBrowserNotification('Clarification needed',d.question||'Tool clarification needed',{sid:activeSid}); + // Browser notification is owned by showClarifyCard()/_notifyPromptCard(). }); source.addEventListener('state_saved',e=>{ @@ -7625,6 +7627,7 @@ function showApprovalForSession(sid, pending, pendingCount) { function showApprovalCard(pending, pendingCount) { const sid = _rememberApprovalPending(pending, pendingCount); + if(typeof _notifyPromptCard==='function') _notifyPromptCard('approval', sid, pending); if (!_approvalPromptBelongsToActiveSession(sid)) return; if (pending && pending.approval_id && _isApprovalDismissed(sid, pending.approval_id)) return; _approvalClearedOwner = null; @@ -8743,6 +8746,7 @@ function _clarifySetControlsDisabled(disabled, loading=false) { function showClarifyCard(pending) { const sid = _rememberClarifyPending(pending); + if(typeof _notifyPromptCard==='function') _notifyPromptCard('clarify', sid, pending); if (!_clarifyPromptBelongsToActiveSession(sid)) return; const question = pending.question || pending.description || ''; const choices = Array.isArray(pending.choices_offered) @@ -9208,6 +9212,40 @@ function requestNotificationPermission(){ return p; }); } +const _PROMPT_NOTIFY_TTL_MS = 600000; +const _promptNotifySeen = new Map(); +// Prompt-card notifications: an approval or clarify card BLOCKS the run until +// it is answered, so every surfacing path must notify — the live SSE event, +// the 1.5s fallback poll, the post-respond "next approval" refresh, and a +// page reload while a prompt is still pending. The chokepoint is the card +// renderer itself (showApprovalCard / showClarifyCard); this helper dedupes +// per prompt id so repeated poll ticks, re-renders, and session switches +// ping exactly once per unique prompt. +function _notifyPromptCard(kind, sid, pending){ + const p = pending || {}; + const id = p.approval_id || p.clarify_id || ''; + if (!id) return; + const key = kind + ':' + id; + const now = Date.now(); + for (const [staleKey, seenAt] of _promptNotifySeen) { + if (now - Number(seenAt || 0) > _PROMPT_NOTIFY_TTL_MS) _promptNotifySeen.delete(staleKey); + } + if (_promptNotifySeen.has(key)) return; + // Unlike the completion notice (#4416, "was hidden at any point"), a prompt + // is suppressed only when the user is actively looking at the tab right + // now (visible AND focused). An unfocused-but-visible window — e.g. the + // WebUI on a second monitor — still gets the ping, because nothing will + // progress until the prompt is answered. Suppression is NOT recorded, so + // backgrounding while a prompt is pending yields one ping. + if (typeof _isDocumentVisibleAndFocused === 'function' && _isDocumentVisibleAndFocused()) return; + _promptNotifySeen.set(key, now); + if (typeof sendBrowserNotification !== 'function') return; + if (kind === 'clarify') { + sendBrowserNotification('Clarification needed', p.question || p.description || 'Tool clarification needed', { sid }); + } else { + sendBrowserNotification('Approval required', p.description || p.command || 'Tool approval needed', { sid }); + } +} function sendBrowserNotification(title,body,options={}){ const force=!!(options&&options.force); // #4416: `forceHidden` means the caller already determined the tab was hidden diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index 1784c7ff..5ecad0c1 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -41,8 +41,148 @@ def test_notification_payload_uses_completion_session_when_provided(): assert "_completionNotificationPreviewText(lastAsst," in MESSAGES_JS assert "sendBrowserNotification('Response complete',_completionPreview||'Task finished',{forceHidden:_wasEverBackgrounded,sid:activeSid})" in MESSAGES_JS assert "assistantText?assistantText.slice(0,100)" not in MESSAGES_JS - assert "sendBrowserNotification('Approval required',d.description||'Tool approval needed',{sid:activeSid})" in MESSAGES_JS - assert "sendBrowserNotification('Clarification needed',d.question||'Tool clarification needed',{sid:activeSid})" 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 prompt id, with TTL cleanup of stale keys. + assert "const key = kind + ':' + id;" in MESSAGES_JS + assert "if (_promptNotifySeen.has(key)) return;" in MESSAGES_JS + assert "_PROMPT_NOTIFY_TTL_MS" in MESSAGES_JS + # Visibility gate: suppress only while the tab is visible AND focused + # (unlike the completion notice, an unfocused-but-visible window still + # gets the ping because the run is blocked until the prompt is answered). + assert "_isDocumentVisibleAndFocused()) 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) + depth = 0 + for i in range(brace, len(src)): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + if depth == 0: + return src[start : i + 1] + raise AssertionError(f"{name} body did not close") + + +def _run_node(source: str) -> dict: + import json + import shutil + import subprocess + import tempfile + + node = shutil.which("node") + if node is None: + import pytest + + pytest.skip("node is required for the executed notification-gate test") + with tempfile.NamedTemporaryFile("w", suffix=".cjs", encoding="utf-8", dir=ROOT, delete=False) as script: + script.write(source) + script_path = Path(script.name) + try: + result = subprocess.run([node, str(script_path)], cwd=str(ROOT), capture_output=True, text=True, timeout=30) + finally: + script_path.unlink(missing_ok=True) + assert result.returncode == 0, result.stderr + return json.loads(result.stdout.strip()) + + +def test_notify_prompt_card_executed_gate_behavior(): + """Executed node-VM proof of _notifyPromptCard's runtime gates: + (1) dedupes per prompt id — a second call for the same id is a no-op; + (2) suppresses while the tab is visible+focused WITHOUT recording, so the + same id still notifies once the tab is hidden (blocking prompt semantics); + (3) notifies when hidden, with the right title/body and sid routing.""" + helper = _extract_fn(MESSAGES_JS, "_notifyPromptCard") + vis = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") + script = f""" +const document = {{ hidden: false, visibilityState: 'visible', hasFocus: () => true }}; +const _promptNotifySeen = new Map(); // module-level state closed over by the helper +const _PROMPT_NOTIFY_TTL_MS = 600000; // same, for the stale-key cleanup pass +{vis} +{helper} +const sent = []; +function sendBrowserNotification(title, body, options) {{ sent.push({{ title, body, options }}); }} +const hiddenPending = {{ approval_id: 'a1', description: 'run the thing' }}; +// Hidden tab: fires once... +document.hidden = true; document.visibilityState = 'hidden'; document.hasFocus = () => false; +_notifyPromptCard('approval', 'sid-1', hiddenPending); +_notifyPromptCard('approval', 'sid-1', hiddenPending); // dedupe: no second ping +// Same id after returning to a focused tab: dedupe still holds. +document.hidden = false; document.visibilityState = 'visible'; document.hasFocus = () => true; +_notifyPromptCard('approval', 'sid-1', hiddenPending); +// A DIFFERENT id while focused-visible: suppressed, NOT recorded... +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a2', description: 'second' }}); +const afterFocused = sent.length; // checkpoint: must still be 1 (no ping while focused) +// ...so hiding the tab again pings it exactly once. +document.hidden = true; document.visibilityState = 'hidden'; document.hasFocus = () => false; +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a2', description: 'second' }}); +const afterHidden = sent.length; // checkpoint: the suppressed id fires exactly once now +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a2', description: 'second' }}); // dedupe +// Clarify shape routes its own title/body. +_notifyPromptCard('clarify', 'sid-1', {{ clarify_id: 'c1', question: 'Which one?' }}); +process.stdout.write(JSON.stringify({{ sent, afterFocused, afterHidden }})); +""" + result = _run_node(script) + sent = result["sent"] + # Mid-sequence checkpoints (a final count alone cannot distinguish + # suppression from reordering — bite-check verified this exact gap). + assert result["afterFocused"] == 1, ( + f"no ping may fire while the tab is visible+focused (got {result['afterFocused']} pings at checkpoint)" + ) + assert result["afterHidden"] == 2, ( + f"the suppressed id must ping exactly once once the tab is hidden (got {result['afterHidden']} at checkpoint)" + ) + assert len(sent) == 3, f"expected exactly 3 pings, got {len(sent)}: {sent}" + assert sent[0]["title"] == "Approval required" + assert sent[0]["body"] == "run the thing" + assert sent[0]["options"] == {"sid": "sid-1"} + assert sent[1]["body"] == "second" + assert sent[2]["title"] == "Clarification needed" + assert sent[2]["body"] == "Which one?" def test_completion_notification_preview_uses_settled_message_not_live_prefix(): From ff53395fdead53799c064527b10fa1df3e906af6 Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:41:10 -0500 Subject: [PATCH 02/13] fix(notifications): make prompt-notification gate session-aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Case 1 was missed: an approval on a NON-active session was suppressed while the user was focused on a different session's tab — even though they had no idea the prompt existed. The gate now suppresses only when the prompt's own session is actively viewed (open in pane + tab visible + focused, via the existing _isSessionActivelyViewed). All three user cases now notify: non-active session, hidden tab, unfocused window. --- static/messages.js | 16 ++-- tests/test_pwa_notification_controls.py | 100 +++++++++++++++--------- 2 files changed, 70 insertions(+), 46 deletions(-) diff --git a/static/messages.js b/static/messages.js index 60be3cd6..e1daa2d2 100644 --- a/static/messages.js +++ b/static/messages.js @@ -9231,13 +9231,15 @@ function _notifyPromptCard(kind, sid, pending){ if (now - Number(seenAt || 0) > _PROMPT_NOTIFY_TTL_MS) _promptNotifySeen.delete(staleKey); } if (_promptNotifySeen.has(key)) return; - // Unlike the completion notice (#4416, "was hidden at any point"), a prompt - // is suppressed only when the user is actively looking at the tab right - // now (visible AND focused). An unfocused-but-visible window — e.g. the - // WebUI on a second monitor — still gets the ping, because nothing will - // progress until the prompt is answered. Suppression is NOT recorded, so - // backgrounding while a prompt is pending yields one ping. - if (typeof _isDocumentVisibleAndFocused === 'function' && _isDocumentVisibleAndFocused()) return; + // Suppress ONLY when the user is effectively looking at this prompt right + // now: it belongs to the session open in the pane AND the tab is visible + // AND focused. Every other combination pings: + // 1. prompt for a non-active session (even with the tab focused) + // 2. active session, but a different tab is selected + // 3. active session, tab active, but the Chrome window is not focused + // Suppression is NOT recorded, so the same prompt still pings exactly once + // the moment the user is no longer looking at it. + if (typeof _isSessionActivelyViewed === 'function' && _isSessionActivelyViewed(sid)) return; _promptNotifySeen.set(key, now); if (typeof sendBrowserNotification !== 'function') return; if (kind === 'clarify') { diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index 5ecad0c1..2e37df15 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -73,10 +73,11 @@ def test_prompt_notifications_fire_from_card_renderer_chokepoint(): assert "const key = kind + ':' + id;" in MESSAGES_JS assert "if (_promptNotifySeen.has(key)) return;" in MESSAGES_JS assert "_PROMPT_NOTIFY_TTL_MS" in MESSAGES_JS - # Visibility gate: suppress only while the tab is visible AND focused - # (unlike the completion notice, an unfocused-but-visible window still - # gets the ping because the run is blocked until the prompt is answered). - assert "_isDocumentVisibleAndFocused()) 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=>{", @@ -133,56 +134,77 @@ def _run_node(source: str) -> dict: def test_notify_prompt_card_executed_gate_behavior(): """Executed node-VM proof of _notifyPromptCard's runtime gates: (1) dedupes per prompt id — a second call for the same id is a no-op; - (2) suppresses while the tab is visible+focused WITHOUT recording, so the - same id still notifies once the tab is hidden (blocking prompt semantics); - (3) notifies when hidden, with the right title/body and sid routing.""" + (2) suppresses only when the prompt's session is ACTIVELY VIEWED (open in + the pane + tab visible + tab focused) WITHOUT recording, so the same id + still notifies the moment the user looks away (blocking prompt semantics); + (3) notifies for a non-active session EVEN with the tab focused (case 1); + (4) notifies when the tab is hidden or the window unfocused (cases 2-3), + with the right title/body and sid routing.""" helper = _extract_fn(MESSAGES_JS, "_notifyPromptCard") - vis = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") + viewed = _extract_fn(MESSAGES_JS, "_isSessionActivelyViewed") + current_pane = _extract_fn(MESSAGES_JS, "_isSessionCurrentPane") + visible = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") script = f""" const document = {{ hidden: false, visibilityState: 'visible', hasFocus: () => true }}; const _promptNotifySeen = new Map(); // module-level state closed over by the helper const _PROMPT_NOTIFY_TTL_MS = 600000; // same, for the stale-key cleanup pass -{vis} +let _loadingSessionId = null; +const S = {{ session: {{ session_id: 'sid-1' }} }}; +{current_pane} +{visible} +{viewed} {helper} const sent = []; function sendBrowserNotification(title, body, options) {{ sent.push({{ title, body, options }}); }} -const hiddenPending = {{ approval_id: 'a1', description: 'run the thing' }}; -// Hidden tab: fires once... -document.hidden = true; document.visibilityState = 'hidden'; document.hasFocus = () => false; -_notifyPromptCard('approval', 'sid-1', hiddenPending); -_notifyPromptCard('approval', 'sid-1', hiddenPending); // dedupe: no second ping -// Same id after returning to a focused tab: dedupe still holds. +const CASES = {{}}; +// Case 3: active session sid-1, tab visible, WINDOW unfocused → notifies. +document.hidden = false; document.visibilityState = 'visible'; document.hasFocus = () => false; +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a3', description: 'window unfocused' }}); +CASES.windowUnfocused = sent.length; // 1 +// Case 2: tab hidden (different tab selected) → notifies. +document.hidden = true; document.visibilityState = 'hidden'; +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a2', description: 'tab hidden' }}); +CASES.tabHidden = sent.length; // 2 document.hidden = false; document.visibilityState = 'visible'; document.hasFocus = () => true; -_notifyPromptCard('approval', 'sid-1', hiddenPending); -// A DIFFERENT id while focused-visible: suppressed, NOT recorded... -_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a2', description: 'second' }}); -const afterFocused = sent.length; // checkpoint: must still be 1 (no ping while focused) -// ...so hiding the tab again pings it exactly once. -document.hidden = true; document.visibilityState = 'hidden'; document.hasFocus = () => false; -_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a2', description: 'second' }}); -const afterHidden = sent.length; // checkpoint: the suppressed id fires exactly once now -_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a2', description: 'second' }}); // dedupe -// Clarify shape routes its own title/body. +// Case 1: prompt for a NON-active session while tab is fully focused → notifies. +_notifyPromptCard('approval', 'sid-2', {{ approval_id: 'a1', description: 'other session' }}); +CASES.otherSession = sent.length; // 3 +// Suppressed case: prompt for the ACTIVE session with tab visible+focused — +// the user is literally looking at the card. NOT recorded, so... +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a4', description: 'watching' }}); +CASES.activelyViewed = sent.length; // still 3 (no ping while watching) +// ...looking away (window blur) pings that same id exactly once. +document.hasFocus = () => false; +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a4', description: 'watching' }}); +CASES.afterBlur = sent.length; // 4 +document.hasFocus = () => true; +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a4', description: 'watching' }}); // dedupe +// Dedupe holds across focus changes for the first id too. +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a3', description: 'window unfocused' }}); +// Clarify shape routes its own title/body (window blurred again). +document.hasFocus = () => false; _notifyPromptCard('clarify', 'sid-1', {{ clarify_id: 'c1', question: 'Which one?' }}); -process.stdout.write(JSON.stringify({{ sent, afterFocused, afterHidden }})); +CASES.clarify = sent.length; // 5 +process.stdout.write(JSON.stringify({{ CASES, sent }})); """ result = _run_node(script) + cases = result["CASES"] sent = result["sent"] - # Mid-sequence checkpoints (a final count alone cannot distinguish - # suppression from reordering — bite-check verified this exact gap). - assert result["afterFocused"] == 1, ( - f"no ping may fire while the tab is visible+focused (got {result['afterFocused']} pings at checkpoint)" - ) - assert result["afterHidden"] == 2, ( - f"the suppressed id must ping exactly once once the tab is hidden (got {result['afterHidden']} at checkpoint)" - ) - assert len(sent) == 3, f"expected exactly 3 pings, got {len(sent)}: {sent}" + # The user's three notification cases must all fire. + assert cases["windowUnfocused"] == 1, f"case 3 (window unfocused) must notify (got {cases['windowUnfocused']})" + assert cases["tabHidden"] == 2, f"case 2 (tab hidden) must notify (got {cases['tabHidden']})" + assert cases["otherSession"] == 3, f"case 1 (non-active session, tab focused) must notify (got {cases['otherSession']})" + # Suppression only while actively viewing; the suppressed id fires after blur. + assert cases["activelyViewed"] == 3, f"no ping while actively viewing the card (got {cases['activelyViewed']})" + assert cases["afterBlur"] == 4, f"the suppressed id must ping exactly once after blur (got {cases['afterBlur']})" + assert cases["clarify"] == 5, f"clarify routes its own title/body (got {cases['clarify']})" + assert len(sent) == 5, f"expected exactly 5 pings, got {len(sent)}: {sent}" assert sent[0]["title"] == "Approval required" - assert sent[0]["body"] == "run the thing" assert sent[0]["options"] == {"sid": "sid-1"} - assert sent[1]["body"] == "second" - assert sent[2]["title"] == "Clarification needed" - assert sent[2]["body"] == "Which one?" + assert sent[2]["options"] == {"sid": "sid-2"} + assert sent[2]["body"] == "other session" + assert sent[4]["title"] == "Clarification needed" + assert sent[4]["body"] == "Which one?" def test_completion_notification_preview_uses_settled_message_not_live_prefix(): From 9281976b2ba75ec8bbfb1f6c06d4d16aefa0c9e5 Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:47:20 -0500 Subject: [PATCH 03/13] fix(notifications): pass forceHidden so prompt notifications survive the live visibility gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live testing on the PPC exposed the real reason unfocused-tab prompt notifications never appeared: _notifyPromptCard correctly decided to notify, but sendBrowserNotification's own gate (_isBackgroundedForBrowserNotification — notify only when document.hidden) vetoed the unfocused-but-visible case. The chokepoint has already made the visibility decision; pass forceHidden so its decision survives, while the notifications-enabled setting is still honored. --- static/messages.js | 8 ++++++-- tests/test_pwa_notification_controls.py | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/static/messages.js b/static/messages.js index e1daa2d2..9fc0a8c7 100644 --- a/static/messages.js +++ b/static/messages.js @@ -9242,10 +9242,14 @@ function _notifyPromptCard(kind, sid, pending){ if (typeof _isSessionActivelyViewed === 'function' && _isSessionActivelyViewed(sid)) return; _promptNotifySeen.set(key, now); if (typeof sendBrowserNotification !== 'function') return; + // 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 }); + 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 }); + sendBrowserNotification('Approval required', p.description || p.command || 'Tool approval needed', { sid, forceHidden: true }); } } function sendBrowserNotification(title,body,options={}){ diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index 2e37df15..4f2e1227 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -200,11 +200,15 @@ process.stdout.write(JSON.stringify({{ CASES, sent }})); assert cases["clarify"] == 5, f"clarify routes its own title/body (got {cases['clarify']})" assert len(sent) == 5, f"expected exactly 5 pings, got {len(sent)}: {sent}" assert sent[0]["title"] == "Approval required" - assert sent[0]["options"] == {"sid": "sid-1"} - assert sent[2]["options"] == {"sid": "sid-2"} + assert sent[0]["options"] == {"sid": "sid-1", "forceHidden": True} + assert sent[2]["options"] == {"sid": "sid-2", "forceHidden": True} assert sent[2]["body"] == "other session" assert sent[4]["title"] == "Clarification needed" assert sent[4]["body"] == "Which one?" + # forceHidden must be set: _notifyPromptCard already made the visibility + # decision, and sendBrowserNotification's live gate ("notify only when + # document.hidden") would otherwise veto the unfocused-but-visible case. + assert all(o["options"].get("forceHidden") for o in sent) def test_completion_notification_preview_uses_settled_message_not_live_prefix(): From ca364400eaf38fdd635e5856e6410a7f1c2749ec Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:56:55 -0500 Subject: [PATCH 04/13] fix(notifications): scope prompt-notification dedupe to the logical owner The dedupe key was kind + prompt id only. The approval lifecycle explicitly permits the same externally supplied approval_id in multiple sessions and in multiple gateway runs within one session (the same identity _notifyPromptCard's neighbors use via _approvalOwnerForPending), so the second logical owner's prompt was silently suppressed: no notification, card still blocking the run. Key is now kind + sid + prompt id + gateway owner (run_id + _gateway_mirror_token when both present), mirroring the approval-owner identity used elsewhere in this file. Repeated SSE/poll/render calls for the same full owner stay deduped. Regression coverage (node-VM executed): same approval_id across two sessions notifies both; same session + id under two distinct gateway run owners notifies twice; repeat calls for one full owner notify once. Fails on the pre-fix key (second session suppressed), passes with it. --- static/messages.js | 16 +++++++-- tests/test_pwa_notification_controls.py | 44 ++++++++++++++++++++----- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/static/messages.js b/static/messages.js index 9fc0a8c7..07f5c2af 100644 --- a/static/messages.js +++ b/static/messages.js @@ -9219,13 +9219,23 @@ const _promptNotifySeen = new Map(); // the 1.5s fallback poll, the post-respond "next approval" refresh, and a // page reload while a prompt is still pending. The chokepoint is the card // renderer itself (showApprovalCard / showClarifyCard); this helper dedupes -// per prompt id so repeated poll ticks, re-renders, and session switches -// ping exactly once per unique prompt. +// per logical OWNER - kind + session id + prompt id (+ gateway run id and +// mirror token when present) - so repeated poll ticks, re-renders, and +// session switches ping exactly once per owner. The gateway owner fields +// mirror the approval-owner identity used elsewhere in this file +// (_approvalOwnerForPending): the same externally supplied approval_id may +// legitimately be pending in two sessions, or in two gateway runs within one +// session, and each owner must notify on its own. function _notifyPromptCard(kind, sid, pending){ const p = pending || {}; const id = p.approval_id || p.clarify_id || ''; if (!id) return; - const key = kind + ':' + id; + const runId = String(p.run_id || '').trim(); + const mirrorToken = String(p._gateway_mirror_token || '').trim(); + const ownerFields = runId && mirrorToken + ? '' + runId + '' + mirrorToken + : ''; + const key = kind + ':' + String(sid || '') + ':' + String(id) + ownerFields; const now = Date.now(); for (const [staleKey, seenAt] of _promptNotifySeen) { if (now - Number(seenAt || 0) > _PROMPT_NOTIFY_TTL_MS) _promptNotifySeen.delete(staleKey); diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index 4f2e1227..4f361d10 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -69,8 +69,9 @@ def test_prompt_notifications_fire_from_card_renderer_chokepoint(): 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 prompt id, with TTL cleanup of stale keys. - assert "const key = kind + ':' + id;" in MESSAGES_JS + # Dedupe gate: per logical owner (kind + sid + prompt id + gateway owner + # fields), with TTL cleanup of stale keys. + assert "const key = kind + ':' + String(sid || '') + ':' + String(id) + ownerFields;" in MESSAGES_JS assert "if (_promptNotifySeen.has(key)) return;" in MESSAGES_JS assert "_PROMPT_NOTIFY_TTL_MS" in MESSAGES_JS # Visibility gate: suppress only while the prompt's session is actively @@ -133,7 +134,9 @@ def _run_node(source: str) -> dict: def test_notify_prompt_card_executed_gate_behavior(): """Executed node-VM proof of _notifyPromptCard's runtime gates: - (1) dedupes per prompt id — a second call for the same id is a no-op; + (1) dedupes per logical owner (kind + session id + prompt id + gateway + run owner) — a second call for the same owner is a no-op, while the same + prompt id under a different session or gateway run owner notifies again; (2) suppresses only when the prompt's session is ACTIVELY VIEWED (open in the pane + tab visible + tab focused) WITHOUT recording, so the same id still notifies the moment the user looks away (blocking prompt semantics); @@ -169,6 +172,25 @@ document.hidden = false; document.visibilityState = 'visible'; document.hasFocus // Case 1: prompt for a NON-active session while tab is fully focused → notifies. _notifyPromptCard('approval', 'sid-2', {{ approval_id: 'a1', description: 'other session' }}); CASES.otherSession = sent.length; // 3 +// Owner-scope regression: same approval_id in a DIFFERENT session must +// notify again — the dedupe key is owner-scoped (kind + sid + id), not +// prompt-id-only. +document.hidden = false; document.visibilityState = 'visible'; document.hasFocus = () => true; +_notifyPromptCard('approval', 'sid-2', {{ approval_id: 'a3', description: 'same id, other session' }}); +CASES.sameIdOtherSession = sent.length; // 4 (a3 was already sent for sid-1) +// Gateway owner-scope regression: same session + same approval_id but a +// distinct gateway run owner (run_id + _gateway_mirror_token) must notify +// again; repeating the SAME full owner stays deduped. Window blurred so the +// actively-viewed gate (which would suppress an ACTIVE-session prompt the +// user is looking at) does not interfere with the owner-scope assertions. +document.hasFocus = () => false; +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a3', run_id: 'run-A', _gateway_mirror_token: 'tok-A', description: 'gateway run A' }}); +CASES.gatewayRunA = sent.length; // 5 +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a3', run_id: 'run-B', _gateway_mirror_token: 'tok-B', description: 'gateway run B' }}); +CASES.gatewayRunB = sent.length; // 6 +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a3', run_id: 'run-A', _gateway_mirror_token: 'tok-A', description: 'gateway run A repeat' }}); +CASES.gatewayRunARepeat = sent.length; // still 6 (same full owner deduped) +document.hasFocus = () => true; // Suppressed case: prompt for the ACTIVE session with tab visible+focused — // the user is literally looking at the card. NOT recorded, so... _notifyPromptCard('approval', 'sid-1', {{ approval_id: 'a4', description: 'watching' }}); @@ -195,16 +217,20 @@ process.stdout.write(JSON.stringify({{ CASES, sent }})); assert cases["tabHidden"] == 2, f"case 2 (tab hidden) must notify (got {cases['tabHidden']})" assert cases["otherSession"] == 3, f"case 1 (non-active session, tab focused) must notify (got {cases['otherSession']})" # Suppression only while actively viewing; the suppressed id fires after blur. - assert cases["activelyViewed"] == 3, f"no ping while actively viewing the card (got {cases['activelyViewed']})" - assert cases["afterBlur"] == 4, f"the suppressed id must ping exactly once after blur (got {cases['afterBlur']})" - assert cases["clarify"] == 5, f"clarify routes its own title/body (got {cases['clarify']})" - assert len(sent) == 5, f"expected exactly 5 pings, got {len(sent)}: {sent}" + assert cases["activelyViewed"] == 6, f"no ping while actively viewing the card (got {cases['activelyViewed']})" + assert cases["afterBlur"] == 7, f"the suppressed id must ping exactly once after blur (got {cases['afterBlur']})" + assert cases["clarify"] == 8, f"clarify routes its own title/body (got {cases['clarify']})" + assert cases["sameIdOtherSession"] == 4, f"same approval_id in another session must notify again (got {cases['sameIdOtherSession']})" + assert cases["gatewayRunA"] == 5, f"distinct gateway run owner must notify again (got {cases['gatewayRunA']})" + assert cases["gatewayRunB"] == 6, f"second distinct gateway run owner must notify again (got {cases['gatewayRunB']})" + assert cases["gatewayRunARepeat"] == 6, f"repeated calls for the same full owner must stay deduped (got {cases['gatewayRunARepeat']})" + assert len(sent) == 8, f"expected exactly 8 pings, got {len(sent)}: {sent}" assert sent[0]["title"] == "Approval required" assert sent[0]["options"] == {"sid": "sid-1", "forceHidden": True} assert sent[2]["options"] == {"sid": "sid-2", "forceHidden": True} assert sent[2]["body"] == "other session" - assert sent[4]["title"] == "Clarification needed" - assert sent[4]["body"] == "Which one?" + assert sent[7]["title"] == "Clarification needed" + assert sent[7]["body"] == "Which one?" # forceHidden must be set: _notifyPromptCard already made the visibility # decision, and sendBrowserNotification's live gate ("notify only when # document.hidden") would otherwise veto the unfocused-but-visible case. From 2f74c2daac99b2192e97042243375ee18225aafc Mon Sep 17 00:00:00 2001 From: Charles McQuade Date: Fri, 11 Sep 2026 17:17:21 -0500 Subject: [PATCH 05/13] fix(notifications): injective owner key and lifecycle-based dedupe retirement Round-2 review fixes for the prompt-notification deduper: - The dedupe key is now the JSON.stringify of the typed owner tuple [kind, sid, prompt id, run owner id, mirror token]. Plain delimiter concatenation let approval_id='x\0r\0t' (no gateway owner) collide with approval_id='x' + run 'r' + token 't'; the gateway accepts approval_id as an unrestricted string, so the second distinct owner was suppressed. - Seen entries retire on prompt LIFECYCLE via the pending-clear chokepoints (_clearApprovalPendingForSession/_clearClarifyPendingForSession now call _retirePromptNotifyKey) instead of a 10-minute wall-clock TTL, so a prompt left pending for hours never re-notifies while a resolved prompt whose ID is reused legitimately notifies again. - Entries are no longer recorded when notifications are disabled, so enabling mid-prompt re-notifies that owner instead of the disabled call permanently consuming it. New regressions: key injectivity with delimiter-bearing approval_id, 24h-still-pending no-renotify, resolved-owner ID reuse, and disabled-to-enabled retry. Full file: 15 passed (11 pre-existing). --- static/messages.js | 56 +++++--- tests/test_pwa_notification_controls.py | 165 +++++++++++++++++++++++- 2 files changed, 198 insertions(+), 23 deletions(-) diff --git a/static/messages.js b/static/messages.js index 07f5c2af..931a59b6 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7484,7 +7484,9 @@ function _rememberApprovalPending(pending, pendingCount) { function _clearApprovalPendingForSession(sid) { if (sid) { + const entry = _approvalPendingBySession.get(sid); _approvalPendingBySession.delete(sid); + if (entry && entry.pending) _retirePromptNotifyKey('approval', sid, entry.pending); if (typeof syncTopbar === 'function') syncTopbar(); } } @@ -8454,7 +8456,9 @@ function _rememberClarifyPending(pending) { function _clearClarifyPendingForSession(sid) { if (sid) { + const entry = _clarifyPendingBySession.get(sid); _clarifyPendingBySession.delete(sid); + if (entry && entry.pending) _retirePromptNotifyKey('clarify', sid, entry.pending); if (typeof syncTopbar === 'function') syncTopbar(); } } @@ -9212,34 +9216,47 @@ function requestNotificationPermission(){ return p; }); } -const _PROMPT_NOTIFY_TTL_MS = 600000; const _promptNotifySeen = new Map(); // Prompt-card notifications: an approval or clarify card BLOCKS the run until // it is answered, so every surfacing path must notify — the live SSE event, // the 1.5s fallback poll, the post-respond "next approval" refresh, and a // page reload while a prompt is still pending. The chokepoint is the card // renderer itself (showApprovalCard / showClarifyCard); this helper dedupes -// per logical OWNER - kind + session id + prompt id (+ gateway run id and -// mirror token when present) - so repeated poll ticks, re-renders, and -// session switches ping exactly once per owner. The gateway owner fields -// mirror the approval-owner identity used elsewhere in this file -// (_approvalOwnerForPending): the same externally supplied approval_id may -// legitimately be pending in two sessions, or in two gateway runs within one -// session, and each owner must notify on its own. +// per logical OWNER - the typed tuple [kind, sid, prompt id, run owner id, +// mirror token] serialized with JSON.stringify so delimiter-bearing producer +// strings cannot collide (the gateway accepts approval_id as an unrestricted +// string) - so repeated poll ticks, re-renders, and session switches ping +// exactly once per owner. The run-owner fields mirror the approval-owner +// identity used elsewhere in this file (_approvalOwnerForPending): the same +// externally supplied approval_id may legitimately be pending in two +// sessions, or in two gateway runs within one session, and each owner must +// notify on its own. Seen entries are retired by PROMPT LIFECYCLE, not by +// wall-clock age: _clearApprovalPendingForSession() and +// _clearClarifyPendingForSession() (the resolution/dismissal/terminal +// chokepoints) call _retirePromptNotifyKey(), so a prompt left pending for +// hours never re-notifies, while a resolved prompt whose ID is later reused +// legitimately notifies again. Entries are also NOT recorded when +// notifications are disabled, so enabling mid-prompt re-notifies that owner. +function _promptNotifyKey(kind, sid, pending){ + const p = pending || {}; + const id = p.approval_id || p.clarify_id || ''; + const runId = String(p.run_id || '').trim(); + const mirrorToken = String(p._gateway_mirror_token || '').trim(); + return JSON.stringify([kind, String(sid || ''), String(id), + runId && mirrorToken ? runId : '', + runId && mirrorToken ? mirrorToken : '']); +} +function _retirePromptNotifyKey(kind, sid, pending){ + if (!pending) return; + const id = pending.approval_id || pending.clarify_id || ''; + if (!id) return; + _promptNotifySeen.delete(_promptNotifyKey(kind, sid, pending)); +} function _notifyPromptCard(kind, sid, pending){ const p = pending || {}; const id = p.approval_id || p.clarify_id || ''; if (!id) return; - const runId = String(p.run_id || '').trim(); - const mirrorToken = String(p._gateway_mirror_token || '').trim(); - const ownerFields = runId && mirrorToken - ? '' + runId + '' + mirrorToken - : ''; - const key = kind + ':' + String(sid || '') + ':' + String(id) + ownerFields; - const now = Date.now(); - for (const [staleKey, seenAt] of _promptNotifySeen) { - if (now - Number(seenAt || 0) > _PROMPT_NOTIFY_TTL_MS) _promptNotifySeen.delete(staleKey); - } + const key = _promptNotifyKey(kind, sid, p); if (_promptNotifySeen.has(key)) return; // Suppress ONLY when the user is effectively looking at this prompt right // now: it belongs to the session open in the pane AND the tab is visible @@ -9250,8 +9267,9 @@ function _notifyPromptCard(kind, sid, pending){ // Suppression is NOT recorded, so the same prompt still pings exactly once // the moment the user is no longer looking at it. if (typeof _isSessionActivelyViewed === 'function' && _isSessionActivelyViewed(sid)) return; - _promptNotifySeen.set(key, now); if (typeof sendBrowserNotification !== 'function') return; + if (typeof window !== 'undefined' && !window._notificationsEnabled) return; + _promptNotifySeen.set(key, Date.now()); // 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 diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index 4f361d10..0ab9d89d 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -69,11 +69,22 @@ def test_prompt_notifications_fire_from_card_renderer_chokepoint(): 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 (kind + sid + prompt id + gateway owner - # fields), with TTL cleanup of stale keys. - assert "const key = kind + ':' + String(sid || '') + ':' + String(id) + ownerFields;" 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 - assert "_PROMPT_NOTIFY_TTL_MS" 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, @@ -144,6 +155,7 @@ def test_notify_prompt_card_executed_gate_behavior(): (4) notifies when the tab is hidden or the window unfocused (cases 2-3), with the right title/body and sid routing.""" helper = _extract_fn(MESSAGES_JS, "_notifyPromptCard") + keyfn = _extract_fn(MESSAGES_JS, "_promptNotifyKey") viewed = _extract_fn(MESSAGES_JS, "_isSessionActivelyViewed") current_pane = _extract_fn(MESSAGES_JS, "_isSessionCurrentPane") visible = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") @@ -156,6 +168,7 @@ const S = {{ session: {{ session_id: 'sid-1' }} }}; {current_pane} {visible} {viewed} +{keyfn} {helper} const sent = []; function sendBrowserNotification(title, body, options) {{ sent.push({{ title, body, options }}); }} @@ -236,6 +249,150 @@ process.stdout.write(JSON.stringify({{ CASES, sent }})); # document.hidden") would otherwise veto the unfocused-but-visible case. assert all(o["options"].get("forceHidden") for o in sent) +# ── Reviewer reproduction regressions (PR #7493 round 2) ──────────────────── + +def test_owner_key_is_injective(): + """The dedupe key is built from a typed owner TUPLE serialized with + JSON.stringify, so delimiter-bearing producer strings (the gateway + accepts approval_id as an unrestricted string) cannot collide with a + different owner that merely contains the same delimiters.""" + helper = _extract_fn(MESSAGES_JS, "_notifyPromptCard") + keyfn = _extract_fn(MESSAGES_JS, "_promptNotifyKey") + viewed = _extract_fn(MESSAGES_JS, "_isSessionActivelyViewed") + current_pane = _extract_fn(MESSAGES_JS, "_isSessionCurrentPane") + visible = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") + script = f""" +const document = {{ hidden: true, visibilityState: 'hidden', hasFocus: () => false }}; +const _promptNotifySeen = new Map(); +let _loadingSessionId = null; +const S = {{ session: {{ session_id: 'sid-1' }} }}; +{current_pane} +{visible} +{viewed} +{keyfn} +{helper} +const sent = []; +function sendBrowserNotification(title, body, options) {{ sent.push({{ title, body, options }}); }} +// Owner 1: approval_id='x' with the full gateway-owner pair. +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'x', run_id: 'r', _gateway_mirror_token: 't', description: 'owner one' }}); +// Owner 2: approval_id CONTAINING the exact delimiter sequence of owner 1, +// with NO gateway-owner fields - a plain concatenation key would collide +// here and suppress this second, distinct owner. +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'x\u0000r\u0000t', description: 'owner two' }}); +process.stdout.write(JSON.stringify({{ count: sent.length, bodies: sent.map(s => s.body) }})); +""" + result = _run_node(script) + assert result["count"] == 2, f"two distinct owners must both notify (got {result})" + assert result["bodies"] == ['owner one', 'owner two'] + +def test_same_pending_owner_does_not_renotify_on_wall_clock(): + """A prompt left pending beyond the old 10-minute TTL must NOT re-notify: + seen entries retire on prompt lifecycle (resolution), not wall-clock age. + Simulated by backdating the seen entry far past the retired TTL window.""" + helper = _extract_fn(MESSAGES_JS, "_notifyPromptCard") + keyfn = _extract_fn(MESSAGES_JS, "_promptNotifyKey") + viewed = _extract_fn(MESSAGES_JS, "_isSessionActivelyViewed") + current_pane = _extract_fn(MESSAGES_JS, "_isSessionCurrentPane") + visible = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") + script = f""" +const document = {{ hidden: true, visibilityState: 'hidden', hasFocus: () => false }}; +const _promptNotifySeen = new Map(); +let _loadingSessionId = null; +const S = {{ session: {{ session_id: 'sid-1' }} }}; +{current_pane} +{visible} +{viewed} +{keyfn} +{helper} +const sent = []; +function sendBrowserNotification(title, body, options) {{ sent.push({{ title, body, options }}); }} +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'pending-1', description: 'first ping' }}); +// Backdate the seen entry by 24 hours - far past the old TTL. With +// lifecycle-based retirement this must NOT re-notify on the next poll tick. +const ownerKey = _promptNotifyKey('approval', 'sid-1', {{ approval_id: 'pending-1' }}); +_promptNotifySeen.set(ownerKey, Date.now() - 24 * 60 * 60 * 1000); +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'pending-1', description: 'poll tick 24h later' }}); +process.stdout.write(JSON.stringify({{ count: sent.length }})); +""" + result = _run_node(script) + assert result["count"] == 1, f"still-pending owner must not re-notify after 24h (got {result})" + +def test_resolved_owner_allows_legitimate_id_reuse(): + """When a prompt's lifecycle ENDS (resolution/dismissal clears pending + state via _clearApprovalPendingForSession), the seen entry is retired, so + a later prompt reusing the same ID notifies again.""" + helper = _extract_fn(MESSAGES_JS, "_notifyPromptCard") + keyfn = _extract_fn(MESSAGES_JS, "_promptNotifyKey") + retire = _extract_fn(MESSAGES_JS, "_retirePromptNotifyKey") + viewed = _extract_fn(MESSAGES_JS, "_isSessionActivelyViewed") + current_pane = _extract_fn(MESSAGES_JS, "_isSessionCurrentPane") + visible = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") + script = f""" +const document = {{ hidden: true, visibilityState: 'hidden', hasFocus: () => false }}; +const _promptNotifySeen = new Map(); +let _loadingSessionId = null; +const S = {{ session: {{ session_id: 'sid-1' }} }}; +{current_pane} +{visible} +{viewed} +{keyfn} +{retire} +{helper} +const sent = []; +function sendBrowserNotification(title, body, options) {{ sent.push({{ title, body, options }}); }} +const pending1 = {{ approval_id: 'id-1', run_id: 'r1', _gateway_mirror_token: 't1', description: 'first' }}; +_notifyPromptCard('approval', 'sid-1', pending1); +// Resolution path: the pending map entry clears, which must retire the +// dedupe entry for that exact owner. +_retirePromptNotifyKey('approval', 'sid-1', pending1); +// Same session + same ID + same run owner later: legitimate reuse, notifies. +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'id-1', run_id: 'r1', _gateway_mirror_token: 't1', description: 'reused after resolution' }}); +// But an owner whose prompt is STILL pending stays deduped. +_notifyPromptCard('approval', 'sid-1', pending1); +process.stdout.write(JSON.stringify({{ count: sent.length, bodies: sent.map(s => s.body) }})); +""" + result = _run_node(script) + assert result["count"] == 2, f"resolved owner ID reuse must notify again, still-pending owner must not (got {result})" + assert result["bodies"] == ['first', 'reused after resolution'] + +def test_disabled_notifications_do_not_consume_notify(): + """When notifications are disabled, no seen entry is recorded - so the + same owner still notifies once the user enables notifications mid-prompt. + Failed/denied delivery must not permanently consume the notification.""" + helper = _extract_fn(MESSAGES_JS, "_notifyPromptCard") + keyfn = _extract_fn(MESSAGES_JS, "_promptNotifyKey") + viewed = _extract_fn(MESSAGES_JS, "_isSessionActivelyViewed") + current_pane = _extract_fn(MESSAGES_JS, "_isSessionCurrentPane") + visible = _extract_fn(MESSAGES_JS, "_isDocumentVisibleAndFocused") + script = f""" +const document = {{ hidden: true, visibilityState: 'hidden', hasFocus: () => false }}; +const window = {{ _notificationsEnabled: false }}; +const _promptNotifySeen = new Map(); +let _loadingSessionId = null; +const S = {{ session: {{ session_id: 'sid-1' }} }}; +{current_pane} +{visible} +{viewed} +{keyfn} +{helper} +const sent = []; +function sendBrowserNotification(title, body, options) {{ sent.push({{ title, body, options }}); }} +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'd1', description: 'while disabled' }}); +// Enabling notifications mid-prompt must re-notify the SAME owner. +window._notificationsEnabled = true; +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'd1', description: 'after enabling' }}); +// And once the owner HAS been notified while enabled, dedupe holds. +_notifyPromptCard('approval', 'sid-1', {{ approval_id: 'd1', description: 'repeat while enabled' }}); +// The disabled first call recorded nothing; the enabled second call is now +// the consumed notify for this owner. +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["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" + def test_completion_notification_preview_uses_settled_message_not_live_prefix(): """Background completion preview must not slice the live-stream accumulator.""" From 78affab91cd14b0b559fc1da93b52937e26433dd Mon Sep 17 00:00:00 2001 From: Charles Inglis Date: Sat, 12 Sep 2026 18:46:56 -0500 Subject: [PATCH 06/13] fix(notifications): retire displaced prompt notify keys on pending replacement Greptile P1: when a new approval/clarify replaces the per-session pending entry, the displaced prompt's notification-dedupe key was never retired. Cleanup only removed the key of the currently stored prompt, and since dedupe entries no longer expire on a wall-clock TTL, a stale key persisted forever - so the same externally supplied prompt ID re-surfacing later was wrongly suppressed as already-notified. _rememberApprovalPending() and _rememberClarifyPending() now retire the displaced entry's notify key on owner change. A re-render of the SAME owner serializes to the same typed tuple key, so repeated poll ticks stay deduped. Regression: node-VM test drives displacement, same-owner re-render, and clear retirement for both approval and clarify paths (fails on the prior head, passes here). --- static/messages.js | 18 ++++++++ tests/test_pwa_notification_controls.py | 59 +++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/static/messages.js b/static/messages.js index 931a59b6..24744498 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7477,6 +7477,16 @@ function _rememberApprovalPending(pending, pendingCount) { if (!pending) return null; const sid = pending._session_id || _promptActiveSessionId(); if (!sid) return null; + 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 + // externally supplied ID can legitimately notify again later. A re-render + // of the SAME prompt serializes to the same owner key - skip retirement + // there so repeated poll ticks stay deduped. + if (prev && prev.pending + && _promptNotifyKey("approval", sid, prev.pending) !== _promptNotifyKey("approval", sid, pending)) { + _retirePromptNotifyKey("approval", sid, prev.pending); + } const nextPending = {...pending, _session_id: sid}; _approvalPendingBySession.set(sid, {pending: nextPending, pendingCount: pendingCount || 1}); return sid; @@ -8449,6 +8459,14 @@ function _rememberClarifyPending(pending) { if (!pending) return null; const sid = pending._session_id || _promptActiveSessionId(); if (!sid) return null; + const prev = _clarifyPendingBySession.get(sid); + // Mirror of the approval displacement retirement above: a NEW clarification + // replaces the session pending entry, so the displaced prompt dedupe key + // retires (same-owner re-renders keep their key). + if (prev && prev.pending + && _promptNotifyKey("clarify", sid, prev.pending) !== _promptNotifyKey("clarify", sid, pending)) { + _retirePromptNotifyKey("clarify", sid, prev.pending); + } const nextPending = {...pending, _session_id: sid}; _clarifyPendingBySession.set(sid, {pending: nextPending}); return sid; diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index 0ab9d89d..cd7257be 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -394,6 +394,65 @@ process.stdout.write(JSON.stringify({{ count: sent.length, bodies: sent.map(s => assert result["consumed"] is True, "the enabled notify must be recorded (consumed) for the owner" +def test_displaced_pending_prompt_key_is_retired_on_replacement(): + """Greptile round-2 regression: when a NEW approval/clarify replaces the + session pending entry, the DISPLACED prompt notification-dedupe key must + retire. Without retirement (and with TTL-based expiry gone) the stale key + persists forever, so the same externally supplied ID re-surfacing later is + wrongly suppressed as already-notified. A re-render of the SAME owner must + keep its key (still deduped).""" + remember_ap = _extract_fn(MESSAGES_JS, "_rememberApprovalPending") + remember_cl = _extract_fn(MESSAGES_JS, "_rememberClarifyPending") + clear_ap = _extract_fn(MESSAGES_JS, "_clearApprovalPendingForSession") + keyfn = _extract_fn(MESSAGES_JS, "_promptNotifyKey") + retire = _extract_fn(MESSAGES_JS, "_retirePromptNotifyKey") + active = _extract_fn(MESSAGES_JS, "_promptActiveSessionId") + script = f""" +const _promptNotifySeen = new Map(); +const S = {{ session: {{ session_id: "sid-1" }} }}; +{keyfn} +{retire} +{active} +{remember_ap} +{remember_cl} +{clear_ap} +const _approvalPendingBySession = new Map(); +const _clarifyPendingBySession = new Map(); +// Displaced approval owner: id-1, then a NEW different approval for sid-1. +_rememberApprovalPending({{ approval_id: "id-1", description: "first" }}, 1); +const key1 = _promptNotifyKey("approval", "sid-1", {{ approval_id: "id-1" }}); +_promptNotifySeen.set(key1, Date.now()); +_rememberApprovalPending({{ approval_id: "id-2", description: "replacement" }}, 1); +const displacedRetired = !_promptNotifySeen.has(key1); +// Same-owner re-render must NOT retire its own key (seed it as already +// notified, as a prior poll tick would have). +const key2 = _promptNotifyKey("approval", "sid-1", {{ approval_id: "id-2" }}); +_promptNotifySeen.set(key2, Date.now()); +_rememberApprovalPending({{ approval_id: "id-2", description: "re-render same owner" }}, 1); +const sameOwnerKept = _promptNotifySeen.has(key2); +// Clear path retires the CURRENT entry. +_clearApprovalPendingForSession("sid-1"); +const clearedRetired = !_promptNotifySeen.has(_promptNotifyKey("approval", "sid-1", {{ approval_id: "id-2" }})); +// Clarify mirror: displacement retires, same-owner re-render keeps. +_rememberClarifyPending({{ clarify_id: "c-1" }}); +const ckey1 = _promptNotifyKey("clarify", "sid-1", {{ clarify_id: "c-1" }}); +_promptNotifySeen.set(ckey1, Date.now()); +_rememberClarifyPending({{ clarify_id: "c-2" }}); +const clarifyDisplacedRetired = !_promptNotifySeen.has(ckey1); +const ckey2 = _promptNotifyKey("clarify", "sid-1", {{ clarify_id: "c-2" }}); +_promptNotifySeen.set(ckey2, Date.now()); +_rememberClarifyPending({{ clarify_id: "c-2" }}); +const clarifySameOwnerKept = _promptNotifySeen.has(ckey2); +process.stdout.write(JSON.stringify({{ displacedRetired, sameOwnerKept, clearedRetired, clarifyDisplacedRetired, clarifySameOwnerKept }})); +""" + result = _run_node(script) + assert result["displacedRetired"] is True, f"displaced approval key must retire on replacement (got {result})" + assert result["sameOwnerKept"] is True, f"same-owner re-render must keep its dedupe key (got {result})" + assert result["clearedRetired"] is True, f"clear path must retire the current entry key (got {result})" + assert result["clarifyDisplacedRetired"] is True, f"displaced clarify key must retire on replacement (got {result})" + assert result["clarifySameOwnerKept"] is True, f"clarify same-owner re-render must keep its key (got {result})" + + def test_completion_notification_preview_uses_settled_message_not_live_prefix(): """Background completion preview must not slice the live-stream accumulator.""" assert "function _completionNotificationPreviewText" in MESSAGES_JS From ac49d623f58f5a841d3e1d5eab094821d11ba48f Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Sun, 13 Sep 2026 18:56:39 -0500 Subject: [PATCH 07/13] fix(notifications): close prompt lifecycle races --- static/boot.js | 2 + static/messages.js | 55 ++++++++++++++++--- ...t_issue4771_gateway_approval_failure_ui.py | 12 ++++ tests/test_pwa_notification_controls.py | 10 ++++ 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/static/boot.js b/static/boot.js index 526caa24..721f2b13 100644 --- a/static/boot.js +++ b/static/boot.js @@ -59,6 +59,7 @@ async function cancelStream(reason){ else setStatus(''); // /api/chat/cancel only exposes `cancelled:bool`, so we cannot // distinguish reasons — keep the toast generic and short. + if(typeof _clearPendingPromptsForSession==='function') _clearPendingPromptsForSession(sid); if(typeof showToast==='function') showToast('Stream is no longer active',2000); } return respOk; @@ -91,6 +92,7 @@ async function cancelSessionStream(session){ if(typeof setComposerStatus==='function') setComposerStatus(''); else setStatus(''); } + if(typeof _clearPendingPromptsForSession==='function') _clearPendingPromptsForSession(sid); if(typeof _approvalSessionId!=='undefined' && _approvalSessionId===sid){ stopApprovalPolling(); hideApprovalCard(true); diff --git a/static/messages.js b/static/messages.js index 24744498..85261739 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7414,6 +7414,7 @@ function hideApprovalCard(force=false) { let _approvalSessionId = null; let _approvalCurrentId = null; // approval_id of the card currently shown let _approvalPendingBySession = new Map(); +const _approvalPromptGenerationBySession = new Map(); let _approvalResponding = null; let _approvalClearedOwner = null; let _approvalDisplayedOwner = null; @@ -7473,6 +7474,14 @@ function activeSessionHasPendingPromptAttention() { )); } +function _approvalPromptGeneration(sid) { + return Number(_approvalPromptGenerationBySession.get(sid) || 0); +} + +function _bumpApprovalPromptGeneration(sid) { + if (sid) _approvalPromptGenerationBySession.set(sid, _approvalPromptGeneration(sid) + 1); +} + function _rememberApprovalPending(pending, pendingCount) { if (!pending) return null; const sid = pending._session_id || _promptActiveSessionId(); @@ -7486,6 +7495,9 @@ function _rememberApprovalPending(pending, pendingCount) { if (prev && prev.pending && _promptNotifyKey("approval", sid, prev.pending) !== _promptNotifyKey("approval", sid, pending)) { _retirePromptNotifyKey("approval", sid, prev.pending); + _bumpApprovalPromptGeneration(sid); + } else if (!prev) { + _bumpApprovalPromptGeneration(sid); } const nextPending = {...pending, _session_id: sid}; _approvalPendingBySession.set(sid, {pending: nextPending, pendingCount: pendingCount || 1}); @@ -7496,6 +7508,7 @@ function _clearApprovalPendingForSession(sid) { if (sid) { const entry = _approvalPendingBySession.get(sid); _approvalPendingBySession.delete(sid); + if (entry) _bumpApprovalPromptGeneration(sid); if (entry && entry.pending) _retirePromptNotifyKey('approval', sid, entry.pending); if (typeof syncTopbar === 'function') syncTopbar(); } @@ -7639,9 +7652,9 @@ 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(typeof _notifyPromptCard==='function') _notifyPromptCard('approval', sid, pending); if (!_approvalPromptBelongsToActiveSession(sid)) return; - if (pending && pending.approval_id && _isApprovalDismissed(sid, pending.approval_id)) return; _approvalClearedOwner = null; const keys = pending.pattern_keys || (pending.pattern_key ? [pending.pattern_key] : []); const desc = (pending.description || "") + (keys.length ? " [" + keys.join(", ") + "]" : ""); @@ -7701,8 +7714,10 @@ function showApprovalCard(pending, pendingCount) { function dismissApprovalCard() { const sid = _approvalSessionId; if (_approvalCurrentId) _markApprovalDismissed(sid, _approvalCurrentId); + // Dismissal is local UI state, not authoritative prompt completion. Keep + // the pending owner and its notification-dedupe key until the server reports + // resolution or a terminal/cancel lifecycle clears it. hideApprovalCard(true); - if (sid) _clearApprovalPendingForSession(sid); } function _syncApprovalCollapseButton(card) { @@ -7894,7 +7909,9 @@ function _startApprovalFallbackPoll(sid) { if (_approvalFallbackPollInFlight) return; _approvalFallbackPollInFlight = true; try { + const generation = _approvalPromptGeneration(sid); const data = await api("/api/approval/pending?session_id=" + encodeURIComponent(sid),{timeoutToast:false}); + if (_approvalPollingSessionMissingOrMismatched(sid) || _approvalPromptGeneration(sid) !== generation) return; if (data.pending) { showApprovalForSession(sid, data.pending, data.pending_count||1); } else if (!_approvalPollingSessionMissingOrMismatched(sid)) { const _resolvedEntry = _approvalPendingBySession.get(sid); @@ -8449,12 +8466,21 @@ let _clarifyMissingEndpointWarned = false; let _clarifyCountdownTimer = null; let _clarifyExpiresAt = 0; let _clarifyPendingBySession = new Map(); +const _clarifyPromptGenerationBySession = new Map(); const CLARIFY_MIN_VISIBLE_MS = 30000; function _clarifyPromptBelongsToActiveSession(sid) { return !!(sid && _promptActiveSessionId() === sid); } +function _clarifyPromptGeneration(sid) { + return Number(_clarifyPromptGenerationBySession.get(sid) || 0); +} + +function _bumpClarifyPromptGeneration(sid) { + if (sid) _clarifyPromptGenerationBySession.set(sid, _clarifyPromptGeneration(sid) + 1); +} + function _rememberClarifyPending(pending) { if (!pending) return null; const sid = pending._session_id || _promptActiveSessionId(); @@ -8466,6 +8492,9 @@ function _rememberClarifyPending(pending) { if (prev && prev.pending && _promptNotifyKey("clarify", sid, prev.pending) !== _promptNotifyKey("clarify", sid, pending)) { _retirePromptNotifyKey("clarify", sid, prev.pending); + _bumpClarifyPromptGeneration(sid); + } else if (!prev) { + _bumpClarifyPromptGeneration(sid); } const nextPending = {...pending, _session_id: sid}; _clarifyPendingBySession.set(sid, {pending: nextPending}); @@ -8476,11 +8505,17 @@ function _clearClarifyPendingForSession(sid) { if (sid) { const entry = _clarifyPendingBySession.get(sid); _clarifyPendingBySession.delete(sid); + if (entry) _bumpClarifyPromptGeneration(sid); if (entry && entry.pending) _retirePromptNotifyKey('clarify', sid, entry.pending); if (typeof syncTopbar === 'function') syncTopbar(); } } +function _clearPendingPromptsForSession(sid) { + _clearApprovalPendingForSession(sid); + _clearClarifyPendingForSession(sid); +} + function _hideClarifyCardIfOwner(sid, force=false, reason="dismissed") { if (!sid || _clarifySessionId === sid) hideClarifyCard(force, reason); } @@ -9015,7 +9050,9 @@ function _startClarifyFallbackPoll(sid) { if (_clarifyFallbackPollInFlight) return; _clarifyFallbackPollInFlight = true; try { + const generation = _clarifyPromptGeneration(sid); const data = await api("/api/clarify/pending?session_id=" + encodeURIComponent(sid),{timeoutToast:false}); + if (!S.session || S.session.session_id !== sid || _clarifyPromptGeneration(sid) !== generation) return; if (data.pending) { showClarifyForSession(sid, data.pending); } else { _clearClarifyPendingForSession(sid); _hideClarifyCardIfOwner(sid, false, 'expired'); } } catch(e) { @@ -9257,7 +9294,11 @@ const _promptNotifySeen = new Map(); // notifications are disabled, so enabling mid-prompt re-notifies that owner. function _promptNotifyKey(kind, sid, pending){ const p = pending || {}; - const id = p.approval_id || p.clarify_id || ''; + // Older clarify producers can omit clarify_id. Their stable prompt payload is + // still a usable lifecycle identity and is retired with the pending entry. + const id = p.approval_id || p.clarify_id || (kind === 'clarify' + ? JSON.stringify([p.question || p.description || '', p.choices_offered || p.choices || [], p.requested_at || '']) + : ''); const runId = String(p.run_id || '').trim(); const mirrorToken = String(p._gateway_mirror_token || '').trim(); return JSON.stringify([kind, String(sid || ''), String(id), @@ -9266,13 +9307,13 @@ function _promptNotifyKey(kind, sid, pending){ } function _retirePromptNotifyKey(kind, sid, pending){ if (!pending) return; - const id = pending.approval_id || pending.clarify_id || ''; - if (!id) return; - _promptNotifySeen.delete(_promptNotifyKey(kind, sid, pending)); + const key = _promptNotifyKey(kind, sid, pending); + if (!key) return; + _promptNotifySeen.delete(key); } function _notifyPromptCard(kind, sid, pending){ const p = pending || {}; - const id = p.approval_id || p.clarify_id || ''; + const id = p.approval_id || p.clarify_id || (kind === 'clarify' ? p.question || p.description || '' : ''); if (!id) return; const key = _promptNotifyKey(kind, sid, p); if (_promptNotifySeen.has(key)) return; diff --git a/tests/test_issue4771_gateway_approval_failure_ui.py b/tests/test_issue4771_gateway_approval_failure_ui.py index 4825f418..d0b0928d 100644 --- a/tests/test_issue4771_gateway_approval_failure_ui.py +++ b/tests/test_issue4771_gateway_approval_failure_ui.py @@ -67,6 +67,10 @@ def _run_failure_case(api_js: str) -> dict: _extract_fn(MESSAGES_JS, "_getDismissedApprovals"), _extract_fn(MESSAGES_JS, "_isApprovalDismissed"), _extract_fn(MESSAGES_JS, "_unmarkApprovalDismissed"), + _extract_fn(MESSAGES_JS, "_promptNotifyKey"), + _extract_fn(MESSAGES_JS, "_retirePromptNotifyKey"), + _extract_fn(MESSAGES_JS, "_approvalPromptGeneration"), + _extract_fn(MESSAGES_JS, "_bumpApprovalPromptGeneration"), _extract_fn(MESSAGES_JS, "_promptActiveSessionId"), _extract_fn(MESSAGES_JS, "_approvalPromptBelongsToActiveSession"), _extract_fn(MESSAGES_JS, "_rememberApprovalPending"), @@ -103,6 +107,7 @@ let renderCalls = 0; let _approvalSessionId = 'sess-1'; let _approvalCurrentId = 'appr-1'; let _approvalPendingBySession = new Map(); +const _approvalPromptGenerationBySession = new Map(); let _loadSessionGeneration = 1; let _approvalResponding = null; let _approvalClearedOwner = null; @@ -111,6 +116,7 @@ let _approvalSignature = ''; let _approvalVisibleSince = 0; let _approvalHideTimer = null; const _clarifyPendingBySession = new Map(); +const _promptNotifySeen = new Map(); const buttons = new Map(); function makeButton(id) {{ return {{ @@ -258,6 +264,10 @@ def test_poll_rerender_keeps_inflight_buttons_disabled_and_blocks_duplicates(): _extract_fn(MESSAGES_JS, "_getDismissedApprovals"), _extract_fn(MESSAGES_JS, "_isApprovalDismissed"), _extract_fn(MESSAGES_JS, "_unmarkApprovalDismissed"), + _extract_fn(MESSAGES_JS, "_promptNotifyKey"), + _extract_fn(MESSAGES_JS, "_retirePromptNotifyKey"), + _extract_fn(MESSAGES_JS, "_approvalPromptGeneration"), + _extract_fn(MESSAGES_JS, "_bumpApprovalPromptGeneration"), _extract_fn(MESSAGES_JS, "_promptActiveSessionId"), _extract_fn(MESSAGES_JS, "_approvalPromptBelongsToActiveSession"), _extract_fn(MESSAGES_JS, "_rememberApprovalPending"), @@ -289,6 +299,7 @@ const _DISMISSED_APPROVALS_KEY = 'hermes_dismissed_approvals'; let _approvalSessionId = 'sess-1'; let _approvalCurrentId = 'appr-1'; let _approvalPendingBySession = new Map(); +const _approvalPromptGenerationBySession = new Map(); let _loadSessionGeneration = 1; let _approvalResponding = null; let _approvalClearedOwner = null; @@ -297,6 +308,7 @@ let _approvalSignature = ''; let _approvalVisibleSince = 0; let _approvalHideTimer = null; const _clarifyPendingBySession = new Map(); +const _promptNotifySeen = new Map(); let resolveApi; let apiCalls = 0; const buttons = new Map(); diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index cd7257be..48a72162 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -407,17 +407,27 @@ def test_displaced_pending_prompt_key_is_retired_on_replacement(): keyfn = _extract_fn(MESSAGES_JS, "_promptNotifyKey") retire = _extract_fn(MESSAGES_JS, "_retirePromptNotifyKey") active = _extract_fn(MESSAGES_JS, "_promptActiveSessionId") + approval_generation = _extract_fn(MESSAGES_JS, "_approvalPromptGeneration") + bump_approval_generation = _extract_fn(MESSAGES_JS, "_bumpApprovalPromptGeneration") + clarify_generation = _extract_fn(MESSAGES_JS, "_clarifyPromptGeneration") + bump_clarify_generation = _extract_fn(MESSAGES_JS, "_bumpClarifyPromptGeneration") script = f""" const _promptNotifySeen = new Map(); const S = {{ session: {{ session_id: "sid-1" }} }}; {keyfn} {retire} {active} +{approval_generation} +{bump_approval_generation} +{clarify_generation} +{bump_clarify_generation} {remember_ap} {remember_cl} {clear_ap} const _approvalPendingBySession = new Map(); const _clarifyPendingBySession = new Map(); +const _approvalPromptGenerationBySession = new Map(); +const _clarifyPromptGenerationBySession = new Map(); // Displaced approval owner: id-1, then a NEW different approval for sid-1. _rememberApprovalPending({{ approval_id: "id-1", description: "first" }}, 1); const key1 = _promptNotifyKey("approval", "sid-1", {{ approval_id: "id-1" }}); From 819d3661e13e19c543bf9a86661931ba471bf668 Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Sun, 13 Sep 2026 19:04:58 -0500 Subject: [PATCH 08/13] test(notifications): include prompt generation helpers --- tests/test_issue3668_prompt_card_reshow_on_switch.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_issue3668_prompt_card_reshow_on_switch.py b/tests/test_issue3668_prompt_card_reshow_on_switch.py index b13aabcf..e8aa5485 100644 --- a/tests/test_issue3668_prompt_card_reshow_on_switch.py +++ b/tests/test_issue3668_prompt_card_reshow_on_switch.py @@ -79,6 +79,8 @@ function extractFunc(name){ // ── module-scope state the extracted functions close over ── let _clarifyPendingBySession = new Map(); let _approvalPendingBySession = new Map(); +let _clarifyPromptGenerationBySession = new Map(); +let _approvalPromptGenerationBySession = new Map(); let _clarifySessionId = null; let _approvalSessionId = null; let S = { session: null }; @@ -109,6 +111,10 @@ function hideApprovalCard(force){ // ── real functions under test ── eval(extractFunc('_promptActiveSessionId')); +eval(extractFunc('_clarifyPromptGeneration')); +eval(extractFunc('_bumpClarifyPromptGeneration')); +eval(extractFunc('_approvalPromptGeneration')); +eval(extractFunc('_bumpApprovalPromptGeneration')); eval(extractFunc('_clarifyPromptBelongsToActiveSession')); eval(extractFunc('_rememberClarifyPending')); eval(extractFunc('_clearClarifyPendingForSession')); From fabf5e0b7845a5f51e2a9f81f81b63056445283d Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Sun, 13 Sep 2026 19:11:21 -0500 Subject: [PATCH 09/13] fix(notifications): clear dismissed approval attention --- static/messages.js | 10 +++++++--- tests/test_pwa_notification_controls.py | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/static/messages.js b/static/messages.js index 85261739..f160fe17 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7486,6 +7486,10 @@ 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 @@ -7714,9 +7718,9 @@ function showApprovalCard(pending, pendingCount) { function dismissApprovalCard() { const sid = _approvalSessionId; if (_approvalCurrentId) _markApprovalDismissed(sid, _approvalCurrentId); - // Dismissal is local UI state, not authoritative prompt completion. Keep - // the pending owner and its notification-dedupe key until the server reports - // resolution or a terminal/cancel lifecycle clears it. + // Dismissal clears local attention while the server remains authoritative. + // A later poll of the same dismissed approval stays suppressed below. + _clearApprovalPendingForSession(sid); hideApprovalCard(true); } diff --git a/tests/test_pwa_notification_controls.py b/tests/test_pwa_notification_controls.py index 48a72162..beaad244 100644 --- a/tests/test_pwa_notification_controls.py +++ b/tests/test_pwa_notification_controls.py @@ -413,6 +413,7 @@ def test_displaced_pending_prompt_key_is_retired_on_replacement(): bump_clarify_generation = _extract_fn(MESSAGES_JS, "_bumpClarifyPromptGeneration") script = f""" const _promptNotifySeen = new Map(); +const _isApprovalDismissed = () => false; const S = {{ session: {{ session_id: "sid-1" }} }}; {keyfn} {retire} 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 10/13] 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} From b3d66dd40db06803592c586165d6a29b8e6eea4c Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:15:37 -0500 Subject: [PATCH 11/13] fix(notifications): migrate legacy approval dismissals --- static/messages.js | 19 ++++++++++++++++++- tests/test_prompt_notification_lifecycle.py | 14 +++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/static/messages.js b/static/messages.js index 58ee6ed6..43105da1 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7429,6 +7429,12 @@ function _approvalDismissKey(sid, approvalId) { return _promptNotifyKey('approval', sid, pending); } +function _legacyApprovalDismissKey(sid, approvalId) { + const pending = typeof approvalId === 'object' ? approvalId : {approval_id: approvalId}; + const id = pending && pending.approval_id; + return sid && id ? String(sid) + '\0' + String(id) : ''; +} + function _getDismissedApprovals() { try { return JSON.parse(localStorage.getItem(_DISMISSED_APPROVALS_KEY) || '[]'); } catch (_) { return []; } @@ -7437,7 +7443,18 @@ function _getDismissedApprovals() { function _isApprovalDismissed(sid, approvalId) { const key = _approvalDismissKey(sid, approvalId); if (!key) return false; - return _getDismissedApprovals().includes(key); + const dismissed = _getDismissedApprovals(); + if (dismissed.includes(key)) return true; + const legacyKey = _legacyApprovalDismissKey(sid, approvalId); + if (!legacyKey || !dismissed.includes(legacyKey)) return false; + // Bind an old session/ID tombstone to the currently observed full owner. + // This keeps an unresolved dismissal quiet through upgrade without letting + // the ambiguous legacy key suppress every future gateway run forever. + const migrated = dismissed.filter(item => item !== legacyKey && item !== key); + migrated.push(key); + try { localStorage.setItem(_DISMISSED_APPROVALS_KEY, JSON.stringify(migrated.slice(-100))); } + catch (_) {} + return true; } function _markApprovalDismissed(sid, approvalId) { diff --git a/tests/test_prompt_notification_lifecycle.py b/tests/test_prompt_notification_lifecycle.py index d76ef2fa..af6fa46a 100644 --- a/tests/test_prompt_notification_lifecycle.py +++ b/tests/test_prompt_notification_lifecycle.py @@ -7,7 +7,7 @@ def run(body, extra=()): '_approvalPromptGeneration', '_bumpApprovalPromptGeneration', '_clarifyPromptGeneration', '_bumpClarifyPromptGeneration', '_rememberApprovalPending', '_clearApprovalPendingForSession', - '_clearClarifyPendingForSession', '_approvalDismissKey', + '_clearClarifyPendingForSession', '_approvalDismissKey', '_legacyApprovalDismissKey', '_getDismissedApprovals', '_isApprovalDismissed', '_markApprovalDismissed', '_unmarkApprovalDismissed', 'dismissApprovalCard', '_startApprovalFallbackPoll', 'showApprovalCard', @@ -36,6 +36,18 @@ const flush=()=>new Promise(r=>setImmediate(r)); ''' + source + '\n(async()=>{\n' + body + '\n})().catch(e=>{console.error(e);process.exit(1)});') +def test_legacy_dismissal_migrates_to_current_full_owner(): + result = run(r''' +const pending={approval_id:'id',run_id:'run-2',_gateway_mirror_token:'token-2'}; +const legacy='s'+'\0'+'id'; +store.set(_DISMISSED_APPROVALS_KEY,JSON.stringify([legacy])); +const dismissed=_isApprovalDismissed('s',pending); +const keys=JSON.parse(store.get(_DISMISSED_APPROVALS_KEY)); +console.log(JSON.stringify({dismissed,legacyPresent:keys.includes(legacy),fullPresent:keys.includes(_approvalDismissKey('s',pending))})); +''') + assert result == dict(dismissed=True, legacyPresent=False, fullPresent=True) + + def test_dismiss_poll_resolution_and_new_run_owner(): result = run(''' const sent=[]; From 056b3c268f199773018cd4fa738a3f51edfd7464 Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:35:25 -0500 Subject: [PATCH 12/13] fix(notifications): keep dismissal helpers self-contained --- static/messages.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/static/messages.js b/static/messages.js index 43105da1..8f956580 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7445,7 +7445,9 @@ function _isApprovalDismissed(sid, approvalId) { if (!key) return false; const dismissed = _getDismissedApprovals(); if (dismissed.includes(key)) return true; - const legacyKey = _legacyApprovalDismissKey(sid, approvalId); + const pending = typeof approvalId === "object" ? approvalId : {approval_id: approvalId}; + const legacyId = pending && pending.approval_id; + const legacyKey = sid && legacyId ? String(sid) + "\0" + String(legacyId) : ""; if (!legacyKey || !dismissed.includes(legacyKey)) return false; // Bind an old session/ID tombstone to the currently observed full owner. // This keeps an unresolved dismissal quiet through upgrade without letting From 1fb213380c9b229fab41239e4ffd41db1a7be6fd Mon Sep 17 00:00:00 2001 From: CharlesMcquade <6466275+CharlesMcquade@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:36:07 -0500 Subject: [PATCH 13/13] fix(notifications): isolate legacy dismissal ownership --- static/messages.js | 7 +++++-- tests/test_prompt_notification_lifecycle.py | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/static/messages.js b/static/messages.js index 8f956580..a3b186c3 100644 --- a/static/messages.js +++ b/static/messages.js @@ -7453,10 +7453,13 @@ function _isApprovalDismissed(sid, approvalId) { // This keeps an unresolved dismissal quiet through upgrade without letting // the ambiguous legacy key suppress every future gateway run forever. const migrated = dismissed.filter(item => item !== legacyKey && item !== key); - migrated.push(key); + const hasFullOwner = !!(pending && pending.run_id && pending._gateway_mirror_token); + // A legacy tombstone cannot identify a gateway run. Consume it rather than + // transferring its authority to an unrelated later owner that reused the ID. + if (!hasFullOwner) migrated.push(key); try { localStorage.setItem(_DISMISSED_APPROVALS_KEY, JSON.stringify(migrated.slice(-100))); } catch (_) {} - return true; + return !hasFullOwner; } function _markApprovalDismissed(sid, approvalId) { diff --git a/tests/test_prompt_notification_lifecycle.py b/tests/test_prompt_notification_lifecycle.py index af6fa46a..9c53b182 100644 --- a/tests/test_prompt_notification_lifecycle.py +++ b/tests/test_prompt_notification_lifecycle.py @@ -36,7 +36,7 @@ const flush=()=>new Promise(r=>setImmediate(r)); ''' + source + '\n(async()=>{\n' + body + '\n})().catch(e=>{console.error(e);process.exit(1)});') -def test_legacy_dismissal_migrates_to_current_full_owner(): +def test_legacy_dismissal_does_not_transfer_to_distinct_full_owner(): result = run(r''' const pending={approval_id:'id',run_id:'run-2',_gateway_mirror_token:'token-2'}; const legacy='s'+'\0'+'id'; @@ -45,9 +45,21 @@ const dismissed=_isApprovalDismissed('s',pending); const keys=JSON.parse(store.get(_DISMISSED_APPROVALS_KEY)); console.log(JSON.stringify({dismissed,legacyPresent:keys.includes(legacy),fullPresent:keys.includes(_approvalDismissKey('s',pending))})); ''') - assert result == dict(dismissed=True, legacyPresent=False, fullPresent=True) + assert result == dict(dismissed=False, legacyPresent=False, fullPresent=False) + +def test_legacy_dismissal_migrates_when_no_stronger_owner_exists(): + result = run(r""" +const pending={approval_id:"id"}; +const legacy="s"+"\0"+"id"; +store.set(_DISMISSED_APPROVALS_KEY,JSON.stringify([legacy])); +const dismissed=_isApprovalDismissed("s",pending); +const keys=JSON.parse(store.get(_DISMISSED_APPROVALS_KEY)); +console.log(JSON.stringify({dismissed,legacyPresent:keys.includes(legacy),fullPresent:keys.includes(_approvalDismissKey("s",pending))})); +""") + assert result == dict(dismissed=True, legacyPresent=False, fullPresent=True) + def test_dismiss_poll_resolution_and_new_run_owner(): result = run(''' const sent=[];