262 lines
13 KiB
Python
262 lines
13 KiB
Python
"""Regression coverage for #4504 — clarify card / composer stuck after expiry.
|
|
|
|
Two compounding gaps the user experienced:
|
|
|
|
1. Server-side ``clear_pending(sid)`` ran on silent timeout but emitted no
|
|
SSE notify, so the browser never knew the prompt was gone (the visible
|
|
card stayed up and the composer stayed locked).
|
|
|
|
2. The client's ``respondClarify`` catch-block treated every 409 (including
|
|
``stale: true``) as retryable, leaving the card + draft visible and the
|
|
controls re-enabled — but every retry returned 409, so the session was
|
|
permanently stuck. The user had zero affordance to dismiss the card.
|
|
|
|
This file pins:
|
|
|
|
- ``clear_pending`` notifies SSE subscribers (head=None, total=0) so the
|
|
silent-timeout path takes the card down via the existing pending=null
|
|
branch in ``_handleClarifyEvent`` if/when an SSE consumer is re-attached.
|
|
(As of v0.51.340 the WebUI clarify transport is HTTP-poll only — the SSE
|
|
notify is still ordering-correct and drives the sessions-list attention
|
|
badge via the pre-existing ``publish_session_list_changed`` call.)
|
|
- The client's ``respondClarify`` catch-block, on ``e.status === 409``:
|
|
* matches the success path's ``_clarifyId === clarifyId`` guard so a
|
|
late 409 for prompt A does not dismiss a rendered prompt B
|
|
(#2639-style regression),
|
|
* clears the loading/disabled state *before* ``hideClarifyCard`` so
|
|
``_stashClarifyDraft`` does not bail on the loading class (reviewer
|
|
P1 from the first review pass — see PR #4524),
|
|
* routes the same-id case through ``hideClarifyCard(true, 'expired')``
|
|
so the draft is rescued into the now-unlocked composer.
|
|
|
|
The tests intentionally mirror the static-analysis + unit pattern already in
|
|
``test_clarify_sse.py`` so they ride the existing clarify suite layout.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
import queue
|
|
|
|
import pytest
|
|
|
|
|
|
_CLARIFY = os.path.join(os.path.dirname(__file__), "..", "api", "clarify.py")
|
|
_MESSAGES = os.path.join(os.path.dirname(__file__), "..", "static", "messages.js")
|
|
|
|
|
|
def _read(path: str) -> str:
|
|
with open(path) as f:
|
|
return f.read()
|
|
|
|
|
|
# ═════════════════════════════════════════════════════════════════════════════
|
|
# 1. Server-side fix — clear_pending must emit an SSE notify so the silent
|
|
# timeout path actually wakes the browser. (Phase A in the issue.)
|
|
# ══════════════════════════════════════════════════════════════════════════════
|
|
@pytest.fixture()
|
|
def clarify_mod():
|
|
from api import clarify
|
|
return clarify
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _cleanup_subscribers(clarify_mod):
|
|
yield
|
|
clarify_mod._clarify_sse_subscribers.clear()
|
|
clarify_mod._gateway_queues.clear()
|
|
clarify_mod._pending.clear()
|
|
|
|
|
|
class TestClearPendingNotifiesSSE:
|
|
"""clear_pending must push (head=None, total=0) so any SSE subscriber
|
|
(or future SSE reattachment) gets a take-down event for the card."""
|
|
|
|
def test_clear_pending_pushes_none_head_to_subscriber(self, clarify_mod):
|
|
sid = "sess-4504-a"
|
|
# Pre-load a pending clarify entry the way submit_pending does.
|
|
entry = clarify_mod.submit_pending(sid, {"question": "y/n?"})
|
|
assert entry is not None
|
|
# Subscribe AFTER the submit so we don't have to drain its notify.
|
|
sub = clarify_mod.sse_subscribe(sid)
|
|
# Now expire it.
|
|
cleared = clarify_mod.clear_pending(sid)
|
|
assert cleared == 1
|
|
# We should receive a clear push (head=None, total=0).
|
|
msg = sub.get(timeout=1.0)
|
|
assert msg == {"pending": None, "pending_count": 0}, (
|
|
"clear_pending must emit a head=None / total=0 SSE notify so the "
|
|
"silent-timeout path tells any SSE subscriber to take the card "
|
|
"down (#4504)."
|
|
)
|
|
|
|
def test_clear_pending_no_op_does_not_notify(self, clarify_mod):
|
|
sid = "sess-4504-b"
|
|
sub = clarify_mod.sse_subscribe(sid)
|
|
# No pending entry → no clear → no spurious notify.
|
|
cleared = clarify_mod.clear_pending(sid)
|
|
assert cleared == 0
|
|
with pytest.raises(queue.Empty):
|
|
sub.get(timeout=0.1)
|
|
|
|
def test_clear_pending_unblocks_caller_event(self, clarify_mod):
|
|
"""The existing event.set() on the cleared entry stays in place."""
|
|
sid = "sess-4504-c"
|
|
entry = clarify_mod.submit_pending(sid, {"question": "ok?"})
|
|
assert not entry.event.is_set()
|
|
clarify_mod.clear_pending(sid)
|
|
assert entry.event.is_set(), (
|
|
"Clearing must still unblock the agent-side wait() so the "
|
|
"_clarify_callback_impl timeout branch returns its fallback string."
|
|
)
|
|
|
|
|
|
class TestClarifyClearPendingSourceMarkers:
|
|
"""Static-analysis pin for the Phase A fix (#4504)."""
|
|
|
|
def test_clear_pending_calls_notify(self):
|
|
src = _read(_CLARIFY)
|
|
# The fix adds _clarify_sse_notify(session_key, None, 0) inside
|
|
# clear_pending's _lock block.
|
|
assert "_clarify_sse_notify(session_key, None, 0)" in src, (
|
|
"clear_pending must invoke _clarify_sse_notify with None head so "
|
|
"the silent-timeout path notifies SSE subscribers (#4504)."
|
|
)
|
|
|
|
|
|
# ══════════════════════════════════════════════════════════════════════════════
|
|
# 2. Client-side fix — respondClarify catch must treat 409 as terminal
|
|
# *only* for the visible card, and rescue the typed draft into the
|
|
# composer in the same-id case. (Phase B in the issue, plus reviewer P1s.)
|
|
# ══════════════════════════════════════════════════════════════════════════════
|
|
class TestRespondClarify409Terminal:
|
|
"""The 409 branch must guard on id, clear loading first, and dismiss only
|
|
the visible card — never a newer prompt B that rendered while A's
|
|
response was in flight."""
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _load_js(self):
|
|
self.js = _read(_MESSAGES)
|
|
|
|
def _respond_clarify_body(self) -> str:
|
|
start = self.js.index("async function respondClarify(")
|
|
# The function ends at the next top-level "function " or "var ".
|
|
end_fn = self.js.index("\nfunction ", start + 1)
|
|
end_var = self.js.index("\nvar ", start + 1)
|
|
end = min(end_fn, end_var)
|
|
return self.js[start:end]
|
|
|
|
def _catch_409_branch(self, body: str) -> str:
|
|
"""Return the source of the 409 branch in respondClarify's catch."""
|
|
idx = body.index("e.status === 409")
|
|
# Pull a generous window covering both the same-id and different-id
|
|
# paths inside the 409 branch, before the network-error fallback.
|
|
return body[idx : idx + 1500]
|
|
|
|
def test_409_routes_to_hide_clarify_card_expired(self):
|
|
body = self._respond_clarify_body()
|
|
assert "e.status === 409" in body, (
|
|
"respondClarify catch should branch on e.status === 409 to handle "
|
|
"the stale/expired case distinctly from network errors (#4504)."
|
|
)
|
|
assert 'hideClarifyCard(true, "expired")' in body, (
|
|
"On 409 (same-id) the card must be dismissed via "
|
|
"hideClarifyCard(true, 'expired') so _stashClarifyDraft('expired') "
|
|
"routes the draft into the now-unlocked composer (#4504)."
|
|
)
|
|
|
|
def test_409_clears_loading_before_hide_so_draft_is_rescued(self):
|
|
"""Reviewer P1: _stashClarifyDraft bails when #clarifySubmit still
|
|
carries the ``loading`` class set by _clarifySetControlsDisabled(true,
|
|
true) at the top of respondClarify. The 409 same-id branch must
|
|
call ``_clarifySetControlsDisabled(false, false)`` *before*
|
|
``hideClarifyCard(true, "expired")`` — otherwise the typed answer is
|
|
silently dropped and no toast fires."""
|
|
body = self._respond_clarify_body()
|
|
branch = self._catch_409_branch(body)
|
|
# Both calls must be present.
|
|
assert "_clarifySetControlsDisabled(false, false)" in branch, (
|
|
"409 same-id branch must clear the loading/disabled state so the "
|
|
"_stashClarifyDraft loading-class guard does not bail (reviewer P1)."
|
|
)
|
|
assert 'hideClarifyCard(true, "expired")' in branch
|
|
# And the clear MUST precede the hide.
|
|
clear_idx = branch.index("_clarifySetControlsDisabled(false, false)")
|
|
hide_idx = branch.index('hideClarifyCard(true, "expired")')
|
|
assert clear_idx < hide_idx, (
|
|
"_clarifySetControlsDisabled(false, false) must run before "
|
|
"hideClarifyCard(true, 'expired') in the 409 same-id branch — "
|
|
"otherwise _stashClarifyDraft bails on the loading class and the "
|
|
"user's typed draft is silently dropped (PR #4524 reviewer P1)."
|
|
)
|
|
|
|
def test_409_is_guarded_by_clarify_id_match(self):
|
|
"""Reviewer P1: the success path is gated by ``_clarifyId === clarifyId``
|
|
so a parallel poll that already rendered the next queued prompt B
|
|
isn't clobbered (#2639). The 409 branch must follow the same
|
|
contract — otherwise A's late 409 will dismiss B."""
|
|
body = self._respond_clarify_body()
|
|
branch = self._catch_409_branch(body)
|
|
assert "_clarifyId === clarifyId" in branch, (
|
|
"409 branch must mirror the success path's _clarifyId === "
|
|
"clarifyId guard so a late 409 for prompt A does not dismiss a "
|
|
"newer prompt B that rendered while A's response was in flight "
|
|
"(#2639 regression flagged in PR #4524 review)."
|
|
)
|
|
|
|
def test_409_same_id_branch_clears_session_cache(self):
|
|
"""In the same-id case the stale per-session pending cache must be
|
|
cleared so the SSE/poll path can't re-render the just-dismissed card.
|
|
Mirrors the success path's _clearClarifyPendingForSession(sid) call."""
|
|
body = self._respond_clarify_body()
|
|
branch = self._catch_409_branch(body)
|
|
assert "_clearClarifyPendingForSession(sid)" in branch, (
|
|
"409 same-id branch must call _clearClarifyPendingForSession(sid) "
|
|
"so the cached pending entry for this session cannot re-render "
|
|
"the card we just dismissed (mirrors success-path contract)."
|
|
)
|
|
|
|
def test_409_different_id_branch_does_not_dismiss(self):
|
|
"""When _clarifyId != clarifyId at the time the 409 returns, a newer
|
|
prompt B is showing. The branch must re-enable controls and return
|
|
without touching the visible card (no hideClarifyCard, no
|
|
_clearClarifyPendingForSession)."""
|
|
body = self._respond_clarify_body()
|
|
branch = self._catch_409_branch(body)
|
|
# The 409 branch should dismiss the card in exactly one arm — the
|
|
# same-id arm — so the different-id arm leaves the visible newer
|
|
# prompt alone (#2639-style regression P1).
|
|
assert branch.count('hideClarifyCard(true, "expired")') == 1, (
|
|
"409 branch must dismiss the card in exactly one arm (the "
|
|
"same-id arm); the different-id arm must leave the visible "
|
|
"newer prompt alone (PR #4524 reviewer P1)."
|
|
)
|
|
# Both arms (same-id and different-id) must call
|
|
# _clarifySetControlsDisabled(false, false) so the user can keep
|
|
# interacting with whichever prompt is now visible.
|
|
assert branch.count("_clarifySetControlsDisabled(false, false)") >= 2, (
|
|
"Both 409 arms (same-id and different-id) must call "
|
|
"_clarifySetControlsDisabled(false, false) so the user can keep "
|
|
"interacting with whichever prompt is now visible."
|
|
)
|
|
|
|
def test_409_early_returns_before_network_fallback(self):
|
|
body = self._respond_clarify_body()
|
|
branch = self._catch_409_branch(body)
|
|
# The 409 branch must early-return so the network-error fallback
|
|
# below it does not double-touch state.
|
|
assert "return;" in branch, (
|
|
"The 409 branch must early-return so the network-error fallback "
|
|
"does not re-enable the controls of a card we just dismissed."
|
|
)
|
|
|
|
def test_non_409_still_keeps_card_visible(self):
|
|
"""Network/transient errors keep the existing retry-friendly behavior."""
|
|
body = self._respond_clarify_body()
|
|
# The non-409 branch still calls _clarifySetControlsDisabled(false, false)
|
|
# and re-focuses the input — that path must remain reachable.
|
|
assert "_clarifySetControlsDisabled(false, false)" in body, (
|
|
"Non-409 transient errors must still re-enable the controls so "
|
|
"the user can retry once connectivity returns."
|
|
)
|