fix: keep approval and clarify prompts session-owned

This commit is contained in:
Dennis Soong
2026-05-07 23:14:30 +08:00
committed by nesquena-hermes
parent e991d756e5
commit fbc023bb17
5 changed files with 227 additions and 30 deletions
+108 -21
View File
@@ -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)) {
+1
View File
@@ -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;
+106
View File
@@ -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
+3 -3
View File
@@ -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"
# ---------------------------------------------------------------------------
+9 -6
View File
@@ -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'