From fbc023bb17fdf58dbe264b32b899e79db6049c76 Mon Sep 17 00:00:00 2001 From: Dennis Soong Date: Thu, 7 May 2026 23:14:30 +0800 Subject: [PATCH] fix: keep approval and clarify prompts session-owned --- static/messages.js | 129 +++++++++++++++++++++++----- static/sessions.js | 1 + tests/test_1694_prompt_ownership.py | 106 +++++++++++++++++++++++ tests/test_approval_queue.py | 6 +- tests/test_sprint30.py | 15 ++-- 5 files changed, 227 insertions(+), 30 deletions(-) create mode 100644 tests/test_1694_prompt_ownership.py diff --git a/static/messages.js b/static/messages.js index 7d2f5075..f19b7a2c 100644 --- a/static/messages.js +++ b/static/messages.js @@ -367,11 +367,13 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ return _clarifySessionId===activeSid||(!_clarifySessionId&&_isActiveSession()); } function _clearApprovalForOwner(){ + _clearApprovalPendingForSession(activeSid); if(!_approvalBelongsToOwner()) return; stopApprovalPolling(); hideApprovalCard(true); } function _clearClarifyForOwner(reason){ + _clearClarifyPendingForSession(activeSid); if(!_clarifyBelongsToOwner()) return; stopClarifyPolling(); hideClarifyCard(true, reason||'terminal'); @@ -806,16 +808,14 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ source.addEventListener('approval',e=>{ const d=JSON.parse(e.data); - d._session_id=activeSid; - showApprovalCard(d, 1); + showApprovalForSession(activeSid, d, 1); playNotificationSound(); sendBrowserNotification('Approval required',d.description||'Tool approval needed'); }); source.addEventListener('clarify',e=>{ const d=JSON.parse(e.data); - d._session_id=activeSid; - showClarifyCard(d); + showClarifyForSession(activeSid, d); playNotificationSound(); sendBrowserNotification('Clarification needed',d.question||'Tool clarification needed'); }); @@ -1383,8 +1383,50 @@ function hideApprovalCard(force=false) { // Track session_id of the active approval so respond goes to the right session let _approvalSessionId = null; let _approvalCurrentId = null; // approval_id of the card currently shown +let _approvalPendingBySession = new Map(); + +function _promptActiveSessionId() { + return (S.session && S.session.session_id) || null; +} + +function _approvalPromptBelongsToActiveSession(sid) { + return !!(sid && _promptActiveSessionId() === sid); +} + +function _rememberApprovalPending(pending, pendingCount) { + if (!pending) return null; + const sid = pending._session_id || _promptActiveSessionId(); + if (!sid) return null; + const nextPending = {...pending, _session_id: sid}; + _approvalPendingBySession.set(sid, {pending: nextPending, pendingCount: pendingCount || 1}); + return sid; +} + +function _clearApprovalPendingForSession(sid) { + if (sid) _approvalPendingBySession.delete(sid); +} + +function _hideApprovalCardIfOwner(sid, force=false) { + if (!sid || _approvalSessionId === sid) hideApprovalCard(force); +} + +function _renderPendingApprovalForActiveSession() { + const sid = _promptActiveSessionId(); + if (!sid) return; + if (_approvalSessionId && _approvalSessionId !== sid) hideApprovalCard(true); + const entry = _approvalPendingBySession.get(sid); + if (entry) showApprovalCard(entry.pending, entry.pendingCount); +} + +function showApprovalForSession(sid, pending, pendingCount) { + if (!pending) return; + pending._session_id = sid; + showApprovalCard(pending, pendingCount); +} function showApprovalCard(pending, pendingCount) { + const sid = _rememberApprovalPending(pending, pendingCount); + if (!_approvalPromptBelongsToActiveSession(sid)) return; const keys = pending.pattern_keys || (pending.pattern_key ? [pending.pattern_key] : []); const desc = (pending.description || "") + (keys.length ? " [" + keys.join(", ") + "]" : ""); const cmd = pending.command || ""; @@ -1393,7 +1435,7 @@ function showApprovalCard(pending, pendingCount) { const sameApproval = card.classList.contains("visible") && _approvalSignature === sig; $("approvalDesc").textContent = desc; $("approvalCmd").textContent = cmd; - _approvalSessionId = pending._session_id || (S.session && S.session.session_id) || null; + _approvalSessionId = sid; _approvalCurrentId = pending.approval_id || null; _approvalSignature = sig; // Show "1 of N" counter when multiple approvals are queued @@ -1431,6 +1473,7 @@ async function respondApproval(choice) { }); _approvalSessionId = null; _approvalCurrentId = null; + _clearApprovalPendingForSession(sid); hideApprovalCard(true); try { await api("/api/approval/respond", { @@ -1450,14 +1493,14 @@ function startApprovalPolling(sid) { es.addEventListener('initial', e => { const d = JSON.parse(e.data); - if (d.pending) { d.pending._session_id = sid; showApprovalCard(d.pending, d.pending_count || 1); } - else { hideApprovalCard(); } + if (d.pending) { showApprovalForSession(sid, d.pending, d.pending_count || 1); } + else { _clearApprovalPendingForSession(sid); _hideApprovalCardIfOwner(sid); } }); es.addEventListener('approval', e => { const d = JSON.parse(e.data); - if (d.pending) { d.pending._session_id = sid; showApprovalCard(d.pending, d.pending_count || 1); } - else { hideApprovalCard(); } + if (d.pending) { showApprovalForSession(sid, d.pending, d.pending_count || 1); } + else { _clearApprovalPendingForSession(sid); _hideApprovalCardIfOwner(sid); } }); es.onerror = () => { @@ -1472,7 +1515,7 @@ function startApprovalPolling(sid) { // We detect this via a periodic check (cheap — no network request). _approvalSSEHealthTimer = setInterval(() => { if (!S.busy || !S.session || S.session.session_id !== sid) { - stopApprovalPolling(); hideApprovalCard(true); + stopApprovalPolling(); _hideApprovalCardIfOwner(sid, true); } }, 5000); @@ -1490,12 +1533,12 @@ let _approvalPollingSessionId = null; function _startApprovalFallbackPoll(sid) { _approvalPollTimer = setInterval(async () => { if (!S.busy || !S.session || S.session.session_id !== sid) { - stopApprovalPolling(); hideApprovalCard(true); return; + stopApprovalPolling(); _hideApprovalCardIfOwner(sid, true); return; } try { const data = await api("/api/approval/pending?session_id=" + encodeURIComponent(sid)); - if (data.pending) { data.pending._session_id=sid; showApprovalCard(data.pending, data.pending_count||1); } - else { hideApprovalCard(); } + if (data.pending) { showApprovalForSession(sid, data.pending, data.pending_count||1); } + else { _clearApprovalPendingForSession(sid); _hideApprovalCardIfOwner(sid); } } catch(e) { /* ignore poll errors */ } }, 1500); // matches the v0.50.247 polling cadence so degraded-mode users see the same responsiveness } @@ -1521,8 +1564,49 @@ let _clarifySessionId = null; let _clarifyMissingEndpointWarned = false; let _clarifyCountdownTimer = null; let _clarifyExpiresAt = 0; +let _clarifyPendingBySession = new Map(); const CLARIFY_MIN_VISIBLE_MS = 30000; +function _clarifyPromptBelongsToActiveSession(sid) { + return !!(sid && _promptActiveSessionId() === sid); +} + +function _rememberClarifyPending(pending) { + if (!pending) return null; + const sid = pending._session_id || _promptActiveSessionId(); + if (!sid) return null; + const nextPending = {...pending, _session_id: sid}; + _clarifyPendingBySession.set(sid, {pending: nextPending}); + return sid; +} + +function _clearClarifyPendingForSession(sid) { + if (sid) _clarifyPendingBySession.delete(sid); +} + +function _hideClarifyCardIfOwner(sid, force=false, reason="dismissed") { + if (!sid || _clarifySessionId === sid) hideClarifyCard(force, reason); +} + +function _renderPendingClarifyForActiveSession() { + const sid = _promptActiveSessionId(); + if (!sid) return; + if (_clarifySessionId && _clarifySessionId !== sid) hideClarifyCard(true, 'session'); + const entry = _clarifyPendingBySession.get(sid); + if (entry) showClarifyCard(entry.pending); +} + +function showClarifyForSession(sid, pending) { + if (!pending) return; + pending._session_id = sid; + showClarifyCard(pending); +} + +function _renderPendingPromptsForActiveSession() { + _renderPendingApprovalForActiveSession(); + _renderPendingClarifyForActiveSession(); +} + function _ensureClarifyCardDom() { let card = $("clarifyCard"); if (card) return card; @@ -1698,6 +1782,8 @@ function _clarifySetControlsDisabled(disabled, loading=false) { } function showClarifyCard(pending) { + const sid = _rememberClarifyPending(pending); + if (!_clarifyPromptBelongsToActiveSession(sid)) return; const question = pending.question || pending.description || ''; const choices = Array.isArray(pending.choices_offered) ? pending.choices_offered @@ -1713,7 +1799,7 @@ function showClarifyCard(pending) { const choicesEl = $("clarifyChoices"); const input = $("clarifyInput"); const sameClarify = card.classList.contains("visible") && _clarifySignature === sig; - _clarifySessionId = pending._session_id || (S.session && S.session.session_id) || null; + _clarifySessionId = sid; _clarifySignature = sig; _startClarifyCountdown(pending); if (!sameClarify) { @@ -1794,6 +1880,7 @@ async function respondClarify(response) { return; } _clarifySessionId = null; + _clearClarifyPendingForSession(sid); _clarifySetControlsDisabled(true, true); hideClarifyCard(true, 'sent'); try { @@ -1825,16 +1912,16 @@ function startClarifyPolling(sid) { _clarifyEventSource.addEventListener('initial', function(ev) { try { var d = JSON.parse(ev.data); - if (d.pending) { d.pending._session_id = sid; showClarifyCard(d.pending); } - else { hideClarifyCard(false, 'expired'); } + if (d.pending) { showClarifyForSession(sid, d.pending); } + else { _clearClarifyPendingForSession(sid); _hideClarifyCardIfOwner(sid, false, 'expired'); } } catch(e) {} }); _clarifyEventSource.addEventListener('clarify', function(ev) { try { var d = JSON.parse(ev.data); - if (d.pending) { d.pending._session_id = sid; showClarifyCard(d.pending); } - else { hideClarifyCard(false, 'expired'); } + if (d.pending) { showClarifyForSession(sid, d.pending); } + else { _clearClarifyPendingForSession(sid); _hideClarifyCardIfOwner(sid, false, 'expired'); } } catch(e) {} }); @@ -1871,12 +1958,12 @@ function startClarifyPolling(sid) { function _startClarifyFallbackPoll(sid) { _clarifyFallbackTimer = setInterval(async () => { if (!S.session || S.session.session_id !== sid) { - stopClarifyPolling(); hideClarifyCard(true, 'session'); return; + stopClarifyPolling(); _hideClarifyCardIfOwner(sid, true, 'session'); return; } try { const data = await api("/api/clarify/pending?session_id=" + encodeURIComponent(sid)); - if (data.pending) { data.pending._session_id=sid; showClarifyCard(data.pending); } - else { hideClarifyCard(false, 'expired'); } + if (data.pending) { showClarifyForSession(sid, data.pending); } + else { _clearClarifyPendingForSession(sid); _hideClarifyCardIfOwner(sid, false, 'expired'); } } catch(e) { const msg = String((e && e.message) || ""); if (!_clarifyMissingEndpointWarned && /(^|\b)(404|not found)(\b|$)/i.test(msg)) { diff --git a/static/sessions.js b/static/sessions.js index 2bfaeaff..1ab89dc5 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -558,6 +558,7 @@ async function loadSession(sid){ threshold_tokens: _pick(u.threshold_tokens, _s.threshold_tokens), }); } + if(typeof _renderPendingPromptsForActiveSession==='function') _renderPendingPromptsForActiveSession(); _resolveSessionModelForDisplaySoon(sid); // Clear the in-flight session marker now that this load has completed (#1060). if (_loadingSessionId === sid) _loadingSessionId = null; diff --git a/tests/test_1694_prompt_ownership.py b/tests/test_1694_prompt_ownership.py new file mode 100644 index 00000000..833d4abf --- /dev/null +++ b/tests/test_1694_prompt_ownership.py @@ -0,0 +1,106 @@ +"""Regression tests for #1694 approval/clarify prompt ownership. + +Prompt state belongs to the session that owns the running stream. A background +session's approval/clarify event must not render over or hide the currently +active pane's card, but the pending prompt should remain available when the user +switches back to that session. +""" +from pathlib import Path + +REPO_ROOT = Path(__file__).parent.parent +MESSAGES_JS = (REPO_ROOT / "static" / "messages.js").read_text(encoding="utf-8") +SESSIONS_JS = (REPO_ROOT / "static" / "sessions.js").read_text(encoding="utf-8") + + +def _body_from_brace(src: str, brace: int, label: str) -> str: + assert brace >= 0, f"body opening brace not found for: {label}" + depth = 1 + i = brace + 1 + while i < len(src) and depth: + ch = src[i] + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + i += 1 + assert depth == 0, f"body did not close for: {label}" + return src[brace + 1 : i - 1] + + +def _brace_body_after(src: str, marker: str) -> str: + start = src.find(marker) + assert start >= 0, f"marker not found: {marker}" + brace = src.find("{", start) + return _body_from_brace(src, brace, marker) + + +def _function_body(src: str, name: str) -> str: + marker = f"function {name}(" + start = src.find(marker) + assert start >= 0, f"function not found: {name}" + signature_end = src.find(")", start) + assert signature_end >= 0, f"function signature not found: {name}" + brace = src.find("{", signature_end) + return _body_from_brace(src, brace, name) + + +def _event_body(event_name: str) -> str: + return _brace_body_after(MESSAGES_JS, f"source.addEventListener('{event_name}'") + + +def test_stream_prompt_events_use_session_owned_show_helpers(): + """Background stream prompts should be cached by owner before pane render.""" + approval_body = _event_body("approval") + clarify_body = _event_body("clarify") + assert "showApprovalForSession(activeSid" in approval_body + assert "showApprovalCard(d, 1)" not in approval_body + assert "showClarifyForSession(activeSid" in clarify_body + assert "showClarifyCard(d)" not in clarify_body + + +def test_approval_card_render_is_gated_to_active_session_and_cached(): + body = _function_body(MESSAGES_JS, "showApprovalCard") + assert "_rememberApprovalPending(" in body + assert "_approvalPromptBelongsToActiveSession(sid)" in body + assert "return;" in body + assert "let _approvalPendingBySession" in MESSAGES_JS + assert "function _renderPendingPromptsForActiveSession()" in MESSAGES_JS + + +def test_clarify_card_render_is_gated_to_active_session_and_cached(): + body = _function_body(MESSAGES_JS, "showClarifyCard") + assert "_rememberClarifyPending(" in body + assert "_clarifyPromptBelongsToActiveSession(sid)" in body + assert "return;" in body + assert "let _clarifyPendingBySession" in MESSAGES_JS + assert "function _renderPendingPromptsForActiveSession()" in MESSAGES_JS + + +def test_polling_empty_state_clears_only_the_owner_prompt(): + approval_poll = _function_body(MESSAGES_JS, "startApprovalPolling") + approval_fallback = _function_body(MESSAGES_JS, "_startApprovalFallbackPoll") + clarify_poll = _function_body(MESSAGES_JS, "startClarifyPolling") + clarify_fallback = _function_body(MESSAGES_JS, "_startClarifyFallbackPoll") + combined = "\n".join([approval_poll, approval_fallback, clarify_poll, clarify_fallback]) + assert "_clearApprovalPendingForSession(sid)" in combined + assert "_hideApprovalCardIfOwner(sid" in combined + assert "_clearClarifyPendingForSession(sid)" in combined + assert "_hideClarifyCardIfOwner(sid" in combined + assert "else { hideApprovalCard(); }" not in combined + assert "else { hideClarifyCard(false, 'expired'); }" not in combined + assert "stopApprovalPolling(); hideApprovalCard(true); return;" not in combined + assert "stopClarifyPolling(); hideClarifyCard(true, 'session'); return;" not in combined + + +def test_load_session_rerenders_cached_prompt_for_new_active_session(): + body = _function_body(SESSIONS_JS, "loadSession") + assert "_renderPendingPromptsForActiveSession();" in body + + +def test_prompt_rerender_hides_previous_session_cards_without_clearing_cache(): + approval_body = _function_body(MESSAGES_JS, "_renderPendingApprovalForActiveSession") + clarify_body = _function_body(MESSAGES_JS, "_renderPendingClarifyForActiveSession") + assert "_approvalSessionId && _approvalSessionId !== sid" in approval_body + assert "hideApprovalCard(true)" in approval_body + assert "_clarifySessionId && _clarifySessionId !== sid" in clarify_body + assert "hideClarifyCard(true, 'session')" in clarify_body diff --git a/tests/test_approval_queue.py b/tests/test_approval_queue.py index 5d94e828..28978329 100644 --- a/tests/test_approval_queue.py +++ b/tests/test_approval_queue.py @@ -99,9 +99,9 @@ def test_approval_current_id_tracked(): def test_polling_passes_count_to_show(): - """The poll loop must pass pending_count to showApprovalCard.""" - assert "showApprovalCard(data.pending, data.pending_count" in MESSAGES_JS, \ - "Poll loop must pass data.pending_count to showApprovalCard" + """The poll loop must pass pending_count to the owner-aware approval renderer.""" + assert "showApprovalForSession(sid, data.pending, data.pending_count" in MESSAGES_JS, \ + "Poll loop must pass data.pending_count through showApprovalForSession" # --------------------------------------------------------------------------- diff --git a/tests/test_sprint30.py b/tests/test_sprint30.py index 92e0e360..5040ecbd 100644 --- a/tests/test_sprint30.py +++ b/tests/test_sprint30.py @@ -482,14 +482,16 @@ class TestApprovalCardTimerLogic: f'After stopApprovalPolling(), hideApprovalCard called without force=true (got: {match!r})' def test_poll_loop_still_uses_no_force(self): - """Poll loop hideApprovalCard() (when pending gone) keeps no-force — correct behavior.""" + """Poll loop approval hides (when pending gone) keep no-force behavior.""" src = self._get_js().read_text() - # Line 446: else { hideApprovalCard(); } — this is the poll-loop path - # The 30s guard should protect this call (don't force from poll ticks) - assert 'else { hideApprovalCard(); }' in src or \ + # Poll/SSE empty-state hides should preserve the 30s visibility guard. + # Owner-scoped prompt cleanup now routes this through the helper, whose + # default force=false is behavior-equivalent to the old hideApprovalCard(). + assert '_hideApprovalCardIfOwner(sid);' in src or \ + 'else { hideApprovalCard(); }' in src or \ 'else {hideApprovalCard();}' in src or \ 'else { hideApprovalCard() }' in src, \ - 'Poll loop should still call hideApprovalCard() without force=true' + 'Poll loop should still hide approval prompts without force=true' def test_show_approval_card_signature_dedup(self): """showApprovalCard uses a signature to avoid resetting timer on repeat polls.""" @@ -629,7 +631,8 @@ class TestClarifyCardTimerLogic: def test_clarify_poll_loop_uses_no_force(self): src = self._get_js().read_text() - assert "else { hideClarifyCard(false, 'expired'); }" in src or \ + assert "_hideClarifyCardIfOwner(sid, false, 'expired');" in src or \ + "else { hideClarifyCard(false, 'expired'); }" in src or \ "else {hideClarifyCard(false,'expired');}" in src, \ 'Clarify poll loop should hide without force=true'