feat: harden clarify dialog flow and refresh recovery

2026-04-15 13:10:50 +08:00
parent 45d3dc0f68
commit ccba2f5c01
11 changed files with 1066 additions and 9 deletions
--- a/tests/test_clarify_unblock.py
+++ b/tests/test_clarify_unblock.py
@@ -0,0 +1,165 @@
+"""Tests for clarify prompt unblocking and HTTP endpoints."""
+
+import json
+import threading
+import uuid
+import urllib.request
+import urllib.error
+import urllib.parse
+
+import pytest
+
+try:
+    from api.clarify import (
+        register_gateway_notify,
+        unregister_gateway_notify,
+        resolve_clarify,
+        clear_pending,
+        _gateway_queues,
+        _gateway_notify_cbs,
+        _lock,
+        _ClarifyEntry,
+        submit_pending,
+    )
+    CLARIFY_AVAILABLE = True
+except ImportError:
+    CLARIFY_AVAILABLE = False
+
+pytestmark = pytest.mark.skipif(
+    not CLARIFY_AVAILABLE,
+    reason="api.clarify not available in this environment",
+)
+
+from tests._pytest_port import BASE
+
+
+def get(path):
+    url = BASE + path
+    with urllib.request.urlopen(url, timeout=10) as r:
+        return json.loads(r.read())
+
+
+def post(path, body=None):
+    url = BASE + path
+    data = json.dumps(body or {}).encode()
+    req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"})
+    try:
+        with urllib.request.urlopen(req, timeout=10) as r:
+            return json.loads(r.read()), r.status
+    except urllib.error.HTTPError as e:
+        return json.loads(e.read()), e.code
+
+
+class TestClarifyUnblocking:
+    """Unit tests for clarify queue resolution."""
+
+    def test_resolve_clarify_sets_event(self):
+        sid = f"unit-clarify-{uuid.uuid4().hex[:8]}"
+        entry = _ClarifyEntry({"question": "Pick one", "choices_offered": ["a", "b"]})
+        with _lock:
+            _gateway_queues.setdefault(sid, []).append(entry)
+
+        resolved = resolve_clarify(sid, "a", resolve_all=False)
+        assert resolved == 1
+        assert entry.event.is_set()
+        assert entry.result == "a"
+
+    def test_register_and_fire_notify_cb(self):
+        sid = f"unit-notify-{uuid.uuid4().hex[:8]}"
+        fired = []
+        register_gateway_notify(sid, lambda d: fired.append(d))
+
+        with _lock:
+            cb = _gateway_notify_cbs.get(sid)
+        assert cb is not None
+
+        data = {"question": "What now?", "choices_offered": ["x", "y"]}
+        cb(data)
+        assert fired == [data]
+
+        unregister_gateway_notify(sid)
+
+    def test_clear_pending_unblocks_waiters(self):
+        sid = f"unit-clear-{uuid.uuid4().hex[:8]}"
+        entry = _ClarifyEntry({"question": "Wait", "choices_offered": []})
+        with _lock:
+            _gateway_queues.setdefault(sid, []).append(entry)
+
+        cleared = clear_pending(sid)
+        assert cleared == 1
+        assert entry.event.is_set()
+        with _lock:
+            assert sid not in _gateway_queues
+
+    def test_submit_pending_registers_entry(self):
+        sid = f"unit-submit-{uuid.uuid4().hex[:8]}"
+        data = {"question": "Pick", "choices_offered": ["one", "two"], "session_id": sid}
+        entry = submit_pending(sid, data)
+        assert entry.data == data
+        with _lock:
+            assert sid in _gateway_queues
+
+        clear_pending(sid)
+
+
+class TestClarifyModuleExports:
+    def test_register_gateway_notify_exported(self):
+        import api.clarify as ap
+        assert hasattr(ap, "register_gateway_notify")
+
+    def test_unregister_gateway_notify_exported(self):
+        import api.clarify as ap
+        assert hasattr(ap, "unregister_gateway_notify")
+
+    def test_resolve_clarify_exported(self):
+        import api.clarify as ap
+        assert hasattr(ap, "resolve_clarify")
+
+    def test_clarify_entry_exported(self):
+        import api.clarify as ap
+        assert hasattr(ap, "_ClarifyEntry")
+
+
+class TestClarifyHTTPEndpoints:
+    """Regression tests for /api/clarify/respond against the live test server."""
+
+    def test_respond_returns_ok_no_pending(self):
+        sid = f"http-no-pending-{uuid.uuid4().hex[:8]}"
+        result, status = post("/api/clarify/respond", {
+            "session_id": sid,
+            "response": "Use option A",
+        })
+        assert status == 200
+        assert result["ok"] is True
+
+    def test_respond_requires_session_id(self):
+        result, status = post("/api/clarify/respond", {"response": "Hello"})
+        assert status == 400
+
+    def test_respond_requires_response(self):
+        sid = f"http-no-response-{uuid.uuid4().hex[:8]}"
+        result, status = post("/api/clarify/respond", {"session_id": sid})
+        assert status == 400
+
+    def test_respond_clears_injected_pending(self):
+        sid = f"http-clear-{uuid.uuid4().hex[:8]}"
+        question = urllib.parse.quote("Pick the better option")
+        choices = urllib.parse.quote("A")
+        inject = get(
+            f"/api/clarify/inject_test?session_id={urllib.parse.quote(sid)}"
+            f"&question={question}&choices={choices}"
+        )
+        assert inject["ok"] is True
+
+        data = get(f"/api/clarify/pending?session_id={urllib.parse.quote(sid)}")
+        assert data["pending"] is not None
+
+        result, status = post("/api/clarify/respond", {
+            "session_id": sid,
+            "response": "B",
+        })
+        assert status == 200
+        assert result["ok"] is True
+
+        data2 = get(f"/api/clarify/pending?session_id={urllib.parse.quote(sid)}")
+        assert data2["pending"] is None
--- a/tests/test_sprint30.py
+++ b/tests/test_sprint30.py
@@ -91,6 +91,31 @@ class TestApprovalCardHTML:
            "approval card missing aria-labelledby"


+class TestClarifyCardHTML:
+
+    def test_clarify_card_markup_present(self):
+        html = read(REPO / "static/index.html")
+        assert 'id="clarifyCard"' in html, "clarify card missing from index.html"
+        assert 'id="clarifyHeading"' in html, "clarify heading missing"
+        assert 'id="clarifyQuestion"' in html, "clarify question text missing"
+        assert 'id="clarifyChoices"' in html, "clarify choices container missing"
+        assert 'id="clarifyInput"' in html, "clarify input missing"
+        assert 'id="clarifySubmit"' in html, "clarify submit button missing"
+
+    def test_clarify_card_has_data_i18n(self):
+        html = read(REPO / "static/index.html")
+        assert 'data-i18n="clarify_heading"' in html
+        assert 'data-i18n="clarify_send"' in html
+        assert 'data-i18n-placeholder="clarify_input_placeholder"' in html
+
+    def test_clarify_card_has_aria_roles(self):
+        html = read(REPO / "static/index.html")
+        assert 'role="dialog"' in html, \
+            "clarify card missing role=dialog for accessibility"
+        assert 'aria-labelledby="clarifyHeading"' in html, \
+            "clarify card missing aria-labelledby"
+
+
 # ── CSS ──────────────────────────────────────────────────────────────────────

 class TestApprovalCardCSS:
@@ -130,6 +155,37 @@ class TestApprovalCardCSS:
            assert cls in css, f"CSS class '{cls}' missing"


+class TestClarifyCardCSS:
+
+    def test_clarify_styles_present(self):
+        css = read(REPO / "static/style.css")
+        for cls in (
+            ".clarify-card",
+            ".clarify-card.visible",
+            ".clarify-inner",
+            ".clarify-header",
+            ".clarify-question",
+            ".clarify-choices",
+            ".clarify-choice",
+            ".clarify-response",
+            ".clarify-input",
+            ".clarify-submit",
+            ".clarify-hint",
+        ):
+            assert cls in css, f"CSS class '{cls}' missing"
+
+    def test_clarify_mobile_styles_present(self):
+        css = read(REPO / "static/style.css")
+        assert ".clarify-card{padding:0 10px 8px;}" in css or \
+               ".clarify-card { padding:0 10px 8px; }" in css or \
+               "clarify-card" in css, "clarify mobile styles missing"
+
+    def test_clarify_focus_styles_present(self):
+        css = read(REPO / "static/style.css")
+        assert ".clarify-choice:focus" in css and ".clarify-submit:focus" in css, \
+            "clarify focus styles missing"
+
+
 # ── i18n keys ────────────────────────────────────────────────────────────────

 class TestApprovalI18nKeys:
@@ -178,6 +234,38 @@ class TestApprovalI18nKeys:
            "English approval_btn_deny value incorrect"


+class TestClarifyI18nKeys:
+
+    REQUIRED_KEYS = [
+        "clarify_heading",
+        "clarify_hint",
+        "clarify_other",
+        "clarify_send",
+        "clarify_input_placeholder",
+        "clarify_responding",
+    ]
+
+    def test_english_locale_has_all_clarify_keys(self):
+        src = read(REPO / "static/i18n.js")
+        en_block_end = src.find("\n};")
+        en_block = src[:en_block_end]
+        for key in self.REQUIRED_KEYS:
+            assert f"{key}:" in en_block, f"English locale missing i18n key: {key}"
+
+    def test_chinese_locale_has_all_clarify_keys(self):
+        src = read(REPO / "static/i18n.js")
+        zh_start = src.find("\n  zh: {")
+        assert zh_start != -1, "zh locale block not found in i18n.js"
+        zh_block = src[zh_start:]
+        for key in self.REQUIRED_KEYS:
+            assert f"{key}:" in zh_block, f"Chinese locale missing i18n key: {key}"
+
+    def test_clarify_heading_english_value(self):
+        src = read(REPO / "static/i18n.js")
+        assert "clarify_heading: 'Clarification needed'" in src, \
+            "English clarify_heading value incorrect"
+
+
 # ── messages.js behaviour ────────────────────────────────────────────────────

 class TestApprovalMessagesJS:
@@ -209,6 +297,30 @@ class TestApprovalMessagesJS:
            "showApprovalCard should focus the Allow once button"


+class TestClarifyMessagesJS:
+
+    def test_clarify_event_listener_present(self):
+        src = read(REPO / "static/messages.js")
+        assert "addEventListener('clarify'" in src, \
+            "clarify SSE listener missing from messages.js"
+
+    def test_show_clarify_card_present(self):
+        src = read(REPO / "static/messages.js")
+        assert "function showClarifyCard" in src, "showClarifyCard missing"
+        assert "clarifyChoices" in src and "clarifyInput" in src, \
+            "showClarifyCard should manage clarify DOM elements"
+
+    def test_respond_clarify_uses_api_endpoint(self):
+        src = read(REPO / "static/messages.js")
+        assert '/api/clarify/respond' in src, \
+            "respondClarify should POST to /api/clarify/respond"
+
+    def test_clarify_polling_helpers_present(self):
+        src = read(REPO / "static/messages.js")
+        for token in ("startClarifyPolling", "stopClarifyPolling", "hideClarifyCard", "_clarifySessionId"):
+            assert token in src, f"{token} missing from messages.js"
+
+
 # ── boot.js keyboard shortcut ────────────────────────────────────────────────

 class TestApprovalKeyboardShortcut:
@@ -248,6 +360,21 @@ class TestStreamingApprovalScoping:
        assert "_approval_registered = False" in src, \
            "_approval_registered flag must be initialised to False"

+    def test_clarify_registered_flag_present(self):
+        src = read(REPO / "api/streaming.py")
+        assert "_clarify_registered = False" in src, \
+            "_clarify_registered flag must be initialised to False"
+
+    def test_clarify_unreg_notify_initialised_to_none(self):
+        src = read(REPO / "api/streaming.py")
+        assert "_unreg_clarify_notify = None" in src, \
+            "_unreg_clarify_notify must be initialised to None before the try block"
+
+    def test_finally_checks_clarify_unreg_notify_not_none(self):
+        src = read(REPO / "api/streaming.py")
+        assert "_unreg_clarify_notify is not None" in src, \
+            "finally block must check '_unreg_clarify_notify is not None' before calling it"
+

 # ── HTTP regression: approval respond ────────────────────────────────────────

@@ -384,3 +511,66 @@ class TestApprovalCardTimerLogic:
        src = self._get_js().read_text()
        assert '_clearApprovalHideTimer' in src, \
            '_clearApprovalHideTimer helper must exist to cancel deferred setTimeout'
+
+
+class TestClarifyCardTimerLogic:
+
+    def _get_js(self):
+        return pathlib.Path(__file__).parent.parent / 'static' / 'messages.js'
+
+    def test_clarify_min_visible_ms_constant_present(self):
+        src = self._get_js().read_text()
+        assert 'CLARIFY_MIN_VISIBLE_MS' in src
+        import re
+        m = re.search(r'CLARIFY_MIN_VISIBLE_MS\s*=\s*(\d+)', src)
+        assert m is not None, 'CLARIFY_MIN_VISIBLE_MS not assigned'
+        assert int(m.group(1)) == 30000, f'Expected 30000, got {m.group(1)}'
+
+    def test_hide_clarify_card_has_force_parameter(self):
+        src = self._get_js().read_text()
+        assert 'hideClarifyCard(force=false)' in src or \
+               'hideClarifyCard(force = false)' in src, \
+            'hideClarifyCard must have force=false default parameter'
+
+    def test_hide_clarify_card_checks_force_flag(self):
+        src = self._get_js().read_text()
+        assert '!force' in src, 'hideClarifyCard must check !force before deferred hide'
+
+    def test_clarify_hide_timer_variable_present(self):
+        src = self._get_js().read_text()
+        assert '_clarifyHideTimer' in src
+
+    def test_clarify_visible_since_variable_present(self):
+        src = self._get_js().read_text()
+        assert '_clarifyVisibleSince' in src
+
+    def test_clarify_signature_variable_present(self):
+        src = self._get_js().read_text()
+        assert '_clarifySignature' in src
+
+    def test_respond_clarify_calls_hide_with_force(self):
+        src = self._get_js().read_text()
+        import re
+        m = re.search(r'async function respondClarify.*?(?=\nasync function|\nfunction |\Z)',
+                      src, re.DOTALL)
+        assert m, 'respondClarify function not found'
+        body = m.group(0)
+        assert 'hideClarifyCard(true)' in body, \
+            'respondClarify must call hideClarifyCard(true) so card hides immediately after user clicks'
+
+    def test_clarify_poll_loop_uses_no_force(self):
+        src = self._get_js().read_text()
+        assert 'else { hideClarifyCard(); }' in src or \
+               'else {hideClarifyCard();}' in src or \
+               'else { hideClarifyCard() }' in src, \
+            'Clarify poll loop should hide without force=true'
+
+    def test_show_clarify_card_signature_dedup(self):
+        src = self._get_js().read_text()
+        import re
+        m = re.search(r'function showClarifyCard.*?(?=\nfunction |\nasync function |\Z)',
+                      src, re.DOTALL)
+        assert m, 'showClarifyCard function not found'
+        body = m.group(0)
+        assert 'JSON.stringify' in body, 'showClarifyCard must compute a signature via JSON.stringify'
+        assert '_clarifySignature' in body, 'showClarifyCard must check/set _clarifySignature'