diff --git a/api/routes.py b/api/routes.py index e7b6ce6..0ed4093 100644 --- a/api/routes.py +++ b/api/routes.py @@ -58,6 +58,7 @@ try: has_pending, pop_pending, submit_pending, approve_session, approve_permanent, save_permanent_allowlist, is_approved, _pending, _lock, _permanent_approved, + resolve_gateway_approval, ) except ImportError: has_pending = lambda *a, **k: False @@ -67,6 +68,7 @@ except ImportError: approve_permanent = lambda *a, **k: None save_permanent_allowlist = lambda *a, **k: None is_approved = lambda *a, **k: True + resolve_gateway_approval = lambda *a, **k: 0 _pending = {} _lock = threading.Lock() _permanent_approved = set() @@ -1353,6 +1355,7 @@ def _handle_approval_respond(handler, body): choice = body.get('choice', 'deny') if choice not in ('once', 'session', 'always', 'deny'): return bad(handler, f'Invalid choice: {choice}') + # Pop the legacy polling-mode pending entry (no-op when gateway path is active). with _lock: pending = _pending.pop(sid, None) if pending: @@ -1363,6 +1366,10 @@ def _handle_approval_respond(handler, body): for k in keys: approve_session(sid, k); approve_permanent(k) save_permanent_allowlist(_permanent_approved) + # Unblock the agent thread waiting in the gateway approval queue. + # This is the primary signal when streaming is active — the agent + # thread is parked in entry.event.wait() and needs to be woken up. + resolve_gateway_approval(sid, choice, resolve_all=False) return j(handler, {'ok': True, 'choice': choice}) diff --git a/api/streaming.py b/api/streaming.py index 1f1f6f6..9906f6d 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -121,6 +121,26 @@ def _run_agent_streaming(session_id, msg_text, model, workspace, stream_id, atta if _profile_home: os.environ['HERMES_HOME'] = _profile_home # Lock released — agent runs without holding it + # Register a gateway-style notify callback so the approval system can + # push the `approval` SSE event the moment a dangerous command is + # detected, without waiting for the next on_tool() poll cycle. + # Without this, the agent thread blocks inside the terminal tool + # waiting for approval that the UI never knew to ask for, leaving + # the chat stuck in "Thinking…" forever. + _approval_registered = False + _unreg_notify = None + try: + from tools.approval import ( + register_gateway_notify as _reg_notify, + unregister_gateway_notify as _unreg_notify, + ) + def _approval_notify_cb(approval_data): + put('approval', approval_data) + _reg_notify(session_id, _approval_notify_cb) + _approval_registered = True + except ImportError: + pass # approval module not available — fall back to polling + try: def on_token(text): if text is None: @@ -133,13 +153,17 @@ def _run_agent_streaming(session_id, msg_text, model, workspace, stream_id, atta for k, v in list(args.items())[:4]: s2 = str(v); args_snap[k] = s2[:120]+('...' if len(s2)>120 else '') put('tool', {'name': name, 'preview': preview, 'args': args_snap}) - # also check for pending approval and surface it immediately - from tools.approval import has_pending as _has_pending, _pending, _lock - if _has_pending(session_id): - with _lock: - p = dict(_pending.get(session_id, {})) - if p: - put('approval', p) + # Fallback: poll for pending approval in case notify_cb wasn't + # registered (e.g. older approval module without gateway support). + try: + from tools.approval import has_pending as _has_pending, _pending, _lock + if _has_pending(session_id): + with _lock: + p = dict(_pending.get(session_id, {})) + if p: + put('approval', p) + except ImportError: + pass if AIAgent is None: raise ImportError("AIAgent not available -- check that hermes-agent is on sys.path") @@ -382,6 +406,13 @@ def _run_agent_streaming(session_id, msg_text, model, workspace, stream_id, atta usage['last_prompt_tokens'] = getattr(_cc, 'last_prompt_tokens', 0) or 0 put('done', {'session': s.compact() | {'messages': s.messages, 'tool_calls': tool_calls}, 'usage': usage}) finally: + # Unregister the gateway approval callback and unblock any threads + # still waiting on approval (e.g. stream cancelled mid-approval). + if _approval_registered and _unreg_notify is not None: + try: + _unreg_notify(session_id) + except Exception: + pass with _ENV_LOCK: if old_cwd is None: os.environ.pop('TERMINAL_CWD', None) else: os.environ['TERMINAL_CWD'] = old_cwd diff --git a/static/boot.js b/static/boot.js index 3597e5c..1fd2b45 100644 --- a/static/boot.js +++ b/static/boot.js @@ -219,6 +219,17 @@ $('msg').addEventListener('keydown',e=>{ }); // B14: Cmd/Ctrl+K creates a new chat from anywhere document.addEventListener('keydown',async e=>{ + // Enter on approval card = Allow once (when a button inside the card is focused or + // card is visible and focus is not on an input/textarea/select) + if(e.key==='Enter'&&!e.metaKey&&!e.ctrlKey&&!e.shiftKey){ + const card=$('approvalCard'); + const tag=(document.activeElement||{}).tagName||''; + if(card&&card.classList.contains('visible')&&tag!=='TEXTAREA'&&tag!=='INPUT'&&tag!=='SELECT'){ + e.preventDefault(); + if(typeof respondApproval==='function') respondApproval('once'); + return; + } + } if((e.metaKey||e.ctrlKey)&&e.key==='k'){ e.preventDefault(); if(!S.busy){await newSession();await renderSessionList();$('msg').focus();} diff --git a/static/i18n.js b/static/i18n.js index 1fb666b..4f69703 100644 --- a/static/i18n.js +++ b/static/i18n.js @@ -32,6 +32,18 @@ const LOCALES = { regen_failed: 'Regenerate failed: ', reconnect_active: 'A response is still being generated. Reload when ready?', reconnect_finished: 'A response was in progress when you last left. Messages may have updated.', + // approval card + approval_heading: 'Approval required', + approval_desc_prefix: 'Dangerous command detected', + approval_btn_once: 'Allow once', + approval_btn_once_title: 'Allow this one command (Enter)', + approval_btn_session: 'Allow session', + approval_btn_session_title: 'Allow for this conversation session', + approval_btn_always: 'Always allow', + approval_btn_always_title: 'Always allow this command pattern', + approval_btn_deny: 'Deny', + approval_btn_deny_title: 'Deny — do not run this command', + approval_responding: 'Responding\u2026', untitled: 'Untitled', n_messages: (n) => `${n} messages`, model_unavailable: ' (unavailable)', @@ -154,6 +166,18 @@ const LOCALES = { regen_failed: '\u91cd\u65b0\u751f\u6210\u5931\u8d25\uff1a', reconnect_active: '\u56de\u590d\u4ecd\u5728\u751f\u6210\u4e2d\uff0c\u51c6\u5907\u597d\u540e\u8981\u91cd\u65b0\u52a0\u8f7d\u5417\uff1f', reconnect_finished: '\u4f60\u79bb\u5f00\u65f6\u6709\u56de\u590d\u6b63\u5728\u751f\u6210\uff0c\u6d88\u606f\u5185\u5bb9\u53ef\u80fd\u5df2\u7ecf\u66f4\u65b0\u3002', + // approval card + approval_heading: '需要审批', + approval_desc_prefix: '检测到危险命令', + approval_btn_once: '允许一次', + approval_btn_once_title: '允许执行此命令一次(Enter)', + approval_btn_session: '本次允许', + approval_btn_session_title: '本次会话期间允许', + approval_btn_always: '始终允许', + approval_btn_always_title: '始终允许此命令模式', + approval_btn_deny: '拒绝', + approval_btn_deny_title: '拒绝 — 不执行此命令', + approval_responding: '处理中…', untitled: '\u672a\u547d\u540d', n_messages: (n) => `${n} \u6761\u6d88\u606f`, model_unavailable: '\uff08\u4e0d\u53ef\u7528\uff09', diff --git a/static/index.html b/static/index.html index 0e24dff..877ace6 100644 --- a/static/index.html +++ b/static/index.html @@ -217,19 +217,32 @@ -
+
- Dangerous command — approval required + Approval required
- - - - + + + +
diff --git a/static/messages.js b/static/messages.js index 2d4acd6..f13cb8d 100644 --- a/static/messages.js +++ b/static/messages.js @@ -355,25 +355,40 @@ function hideApprovalCard() { let _approvalSessionId = null; function showApprovalCard(pending) { - $("approvalDesc").textContent = pending.description || ""; - $("approvalCmd").textContent = pending.command || ""; const keys = pending.pattern_keys || (pending.pattern_key ? [pending.pattern_key] : []); - $("approvalDesc").textContent = (pending.description || "") + (keys.length ? " [" + keys.join(", ") + "]" : ""); + const desc = (pending.description || "") + (keys.length ? " [" + keys.join(", ") + "]" : ""); + $("approvalDesc").textContent = desc; + $("approvalCmd").textContent = pending.command || ""; _approvalSessionId = pending._session_id || (S.session && S.session.session_id) || null; - $("approvalCard").classList.add("visible"); + // Re-enable buttons in case a previous approval disabled them + ["approvalBtnOnce","approvalBtnSession","approvalBtnAlways","approvalBtnDeny"].forEach(id => { + const b = $(id); if (b) { b.disabled = false; b.classList.remove("loading"); } + }); + const card = $("approvalCard"); + card.classList.add("visible"); + // Apply current locale to data-i18n elements inside the card + if (typeof applyLocaleToDOM === "function") applyLocaleToDOM(); + // Focus Allow once button so Enter works immediately + const onceBtn = $("approvalBtnOnce"); + if (onceBtn) setTimeout(() => onceBtn.focus(), 50); } async function respondApproval(choice) { const sid = _approvalSessionId || (S.session && S.session.session_id); if (!sid) return; - hideApprovalCard(); + // Disable all buttons immediately to prevent double-submit + ["approvalBtnOnce","approvalBtnSession","approvalBtnAlways","approvalBtnDeny"].forEach(id => { + const b = $(id); + if (b) { b.disabled = true; if (b.id === "approvalBtn" + choice.charAt(0).toUpperCase() + choice.slice(1)) b.classList.add("loading"); } + }); _approvalSessionId = null; + hideApprovalCard(); try { await api("/api/approval/respond", { method: "POST", body: JSON.stringify({ session_id: sid, choice }) }); - } catch(e) { setStatus("Approval error: " + e.message); } + } catch(e) { setStatus(t("approval_responding") + " " + e.message); } } function startApprovalPolling(sid) { diff --git a/static/style.css b/static/style.css index 0675e32..85d58f6 100644 --- a/static/style.css +++ b/static/style.css @@ -164,19 +164,27 @@ /* ── Approval card ── */ .approval-card{display:none;max-width:780px;margin:0 auto 0;padding:0 20px 12px;} .approval-card.visible{display:block;} - .approval-inner{background:var(--surface);backdrop-filter:blur(8px);border:1px solid rgba(233,69,96,0.35);border-radius:14px;padding:14px 16px;} + .approval-inner{background:var(--surface);backdrop-filter:blur(8px);border:1px solid rgba(233,69,96,0.35);border-radius:14px;padding:16px 18px;} .approval-header{display:flex;align-items:center;gap:8px;margin-bottom:10px;font-size:13px;font-weight:600;color:#e94560;} - .approval-desc{font-size:12px;color:var(--muted);margin-bottom:8px;} - .approval-cmd{background:var(--code-bg);border:1px solid var(--border);border-radius:8px;padding:8px 12px;font-family:"SF Mono",ui-monospace,monospace;font-size:12px;color:var(--pre-text);white-space:pre-wrap;word-break:break-all;margin-bottom:12px;max-height:120px;overflow-y:auto;} - .approval-btns{display:flex;gap:8px;flex-wrap:wrap;} - .approval-btn{padding:6px 14px;border-radius:8px;font-size:12px;font-weight:600;border:1px solid var(--border2);background:var(--hover-bg);color:var(--text);cursor:pointer;transition:all .15s;} - .approval-btn:hover{background:rgba(255,255,255,0.12);} - .approval-btn.once{border-color:rgba(124,185,255,0.5);color:var(--blue);} - .approval-btn.once:hover{background:rgba(124,185,255,0.15);} - .approval-btn.session{border-color:rgba(124,185,255,0.3);color:var(--blue);} + .approval-desc{font-size:12px;color:var(--muted);margin-bottom:8px;line-height:1.5;} + .approval-cmd{background:var(--code-bg);border:1px solid var(--border);border-radius:8px;padding:8px 12px;font-family:"SF Mono",ui-monospace,monospace;font-size:12px;color:var(--pre-text);white-space:pre-wrap;word-break:break-all;margin-bottom:14px;max-height:120px;overflow-y:auto;} + .approval-btns{display:flex;gap:8px;flex-wrap:wrap;align-items:center;} + .approval-btn{display:inline-flex;align-items:center;gap:6px;padding:7px 15px;border-radius:8px;font-size:12px;font-weight:600;border:1px solid var(--border2);background:var(--hover-bg);color:var(--text);cursor:pointer;transition:all .15s;white-space:nowrap;} + .approval-btn:hover{background:rgba(255,255,255,0.12);transform:translateY(-1px);box-shadow:0 2px 8px rgba(0,0,0,0.2);} + .approval-btn:active{transform:translateY(0);box-shadow:none;} + .approval-btn:disabled{opacity:.5;cursor:not-allowed;transform:none;} + .approval-btn-icon{font-size:13px;line-height:1;} + .approval-btn-label{line-height:1;} + .approval-kbd{display:inline-flex;align-items:center;justify-content:center;padding:1px 5px;border-radius:4px;font-size:10px;font-family:inherit;background:rgba(255,255,255,.08);border:1px solid rgba(255,255,255,.15);color:var(--muted);line-height:1.4;margin-left:2px;} + .approval-btn.once{border-color:rgba(124,185,255,0.6);color:var(--blue);background:rgba(124,185,255,0.08);} + .approval-btn.once:hover{background:rgba(124,185,255,0.18);border-color:rgba(124,185,255,0.8);} + .approval-btn.session{border-color:rgba(124,185,255,0.35);color:var(--blue);} + .approval-btn.session:hover{background:rgba(124,185,255,0.12);border-color:rgba(124,185,255,0.55);} .approval-btn.always{border-color:rgba(201,168,76,0.5);color:var(--gold);} + .approval-btn.always:hover{background:rgba(201,168,76,0.12);border-color:rgba(201,168,76,0.7);} .approval-btn.deny{border-color:rgba(233,69,96,0.5);color:var(--accent);} - .approval-btn.deny:hover{background:rgba(233,69,96,0.12);} + .approval-btn.deny:hover{background:rgba(233,69,96,0.12);border-color:rgba(233,69,96,0.7);} + .approval-btn.loading{opacity:.7;cursor:wait;} /* Sidebar navigation tabs */ .sidebar-nav{display:flex;border-bottom:1px solid var(--border);flex-shrink:0;padding:6px 8px 0;gap:2px;} .nav-tab{flex:1;padding:10px 4px 8px;font-size:20px;text-align:center;cursor:pointer;color:var(--muted);border:none;background:none;transition:color .15s;border-bottom:2px solid transparent;white-space:nowrap;overflow:hidden;position:relative;} @@ -462,6 +470,7 @@ .approval-card{padding:0 10px 8px;} .approval-btns{gap:6px;} .approval-btn{padding:8px 12px;font-size:12px;min-height:44px;} + .approval-kbd{display:none;} /* Tool cards */ .tool-card{margin-left:0!important;font-size:12px;} /* Settings modal */ diff --git a/tests/test_approval_unblock.py b/tests/test_approval_unblock.py new file mode 100644 index 0000000..975ab15 --- /dev/null +++ b/tests/test_approval_unblock.py @@ -0,0 +1,286 @@ +""" +Tests for fix/approval-stuck-thinking: +Verify that /api/approval/respond correctly unblocks gateway approval queues +and that the approval module exports the symbols streaming.py and routes.py +need to prevent the UI getting stuck in "Thinking…" during dangerous commands. +""" + +import json +import threading +import uuid +import urllib.request +import urllib.error +import urllib.parse + +import pytest + +# Import approval internals — shared module-level state within this process. +# The HTTP tests use the test server (port 8788, separate process). +# The unit tests operate directly on the module. +try: + from tools.approval import ( + register_gateway_notify, + unregister_gateway_notify, + resolve_gateway_approval, + _gateway_queues, + _gateway_notify_cbs, + _lock, + _ApprovalEntry, + submit_pending, + has_pending, + pop_pending, + ) + APPROVAL_AVAILABLE = True +except ImportError: + APPROVAL_AVAILABLE = False + +pytestmark = pytest.mark.skipif( + not APPROVAL_AVAILABLE, + reason="tools.approval not available in this environment" +) + +BASE = "http://127.0.0.1:8788" + + +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 + + +# ── Unit tests (in-process, no HTTP server needed) ────────────────────────── + +class TestGatewayApprovalUnblocking: + """Unit tests for the gateway queue unblocking mechanism.""" + + def test_resolve_gateway_approval_sets_event(self): + """resolve_gateway_approval() must set the entry's event and store the result.""" + sid = f"unit-resolve-{uuid.uuid4().hex[:8]}" + data = {"command": "rm -rf /tmp/x", "description": "recursive delete"} + entry = _ApprovalEntry(data) + with _lock: + _gateway_queues.setdefault(sid, []).append(entry) + + resolved = resolve_gateway_approval(sid, "once", resolve_all=False) + assert resolved == 1 + assert entry.event.is_set() + assert entry.result == "once" + + # Queue should be cleaned up + with _lock: + assert sid not in _gateway_queues + + def test_resolve_gateway_approval_deny(self): + """Deny choice is propagated correctly.""" + sid = f"unit-deny-{uuid.uuid4().hex[:8]}" + entry = _ApprovalEntry({"command": "pkill -9 x", "description": "force kill"}) + with _lock: + _gateway_queues.setdefault(sid, []).append(entry) + + resolve_gateway_approval(sid, "deny") + assert entry.result == "deny" + + def test_resolve_gateway_approval_no_queue_is_harmless(self): + """resolve_gateway_approval with no queue entry returns 0, no crash.""" + sid = f"unit-no-queue-{uuid.uuid4().hex[:8]}" + result = resolve_gateway_approval(sid, "once") + assert result == 0 + + def test_resolve_all_unblocks_multiple_entries(self): + """resolve_all=True unblocks every pending entry in the queue.""" + sid = f"unit-resolve-all-{uuid.uuid4().hex[:8]}" + entries = [_ApprovalEntry({"command": f"cmd{i}"}) for i in range(3)] + with _lock: + _gateway_queues[sid] = list(entries) + + resolved = resolve_gateway_approval(sid, "session", resolve_all=True) + assert resolved == 3 + for e in entries: + assert e.event.is_set() + assert e.result == "session" + + def test_register_and_fire_notify_cb(self): + """register_gateway_notify stores the cb; calling it delivers approval data.""" + 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 = {"command": "test", "description": "test"} + cb(data) + assert fired == [data] + + unregister_gateway_notify(sid) + + def test_unregister_clears_cb_and_signals_entries(self): + """unregister_gateway_notify removes cb and unblocks any queued entries.""" + sid = f"unit-unreg-{uuid.uuid4().hex[:8]}" + register_gateway_notify(sid, lambda d: None) + + entry = _ApprovalEntry({"command": "x"}) + with _lock: + _gateway_queues.setdefault(sid, []).append(entry) + + unregister_gateway_notify(sid) + + assert entry.event.is_set(), "unregister should signal blocked entries" + with _lock: + assert sid not in _gateway_notify_cbs + assert sid not in _gateway_queues + + def test_streaming_approval_integration(self): + """ + End-to-end unit simulation of the streaming.py fix: + 1. streaming.py registers notify_cb + 2. check_all_command_guards fires notify_cb (pushing approval SSE) + 3. User responds — resolve_gateway_approval unblocks agent thread + 4. Agent thread sees choice and continues + """ + sid = f"unit-e2e-{uuid.uuid4().hex[:8]}" + approval_events_sent = [] + + # Step 1: streaming.py registers the notify callback + def _approval_notify_cb(approval_data): + approval_events_sent.append(approval_data) # would be put('approval', ...) + register_gateway_notify(sid, _approval_notify_cb) + + # Step 2: check_all_command_guards fires the callback and queues an entry + approval_data = { + "command": "rm -rf /tmp/test", + "pattern_key": "recursive delete", + "pattern_keys": ["recursive delete"], + "description": "recursive delete", + } + entry = _ApprovalEntry(approval_data) + with _lock: + _gateway_queues.setdefault(sid, []).append(entry) + # notify_cb fires synchronously (gateway notifies user) + with _lock: + cb = _gateway_notify_cbs.get(sid) + cb(approval_data) + + assert len(approval_events_sent) == 1, "approval SSE event should have been queued" + + # Step 3: user responds via /api/approval/respond → resolve_gateway_approval + resolved = resolve_gateway_approval(sid, "once") + assert resolved == 1 + + # Step 4: agent thread is unblocked with the correct choice + assert entry.event.is_set() + assert entry.result == "once" + + # Cleanup + unregister_gateway_notify(sid) + + +# ── Symbol existence tests ─────────────────────────────────────────────────── + +class TestApprovalModuleExports: + """Verify the module exports all symbols that streaming.py and routes.py need.""" + + def test_register_gateway_notify_exported(self): + import tools.approval as ap + assert hasattr(ap, "register_gateway_notify"), \ + "tools.approval must export register_gateway_notify" + + def test_unregister_gateway_notify_exported(self): + import tools.approval as ap + assert hasattr(ap, "unregister_gateway_notify"), \ + "tools.approval must export unregister_gateway_notify" + + def test_resolve_gateway_approval_exported(self): + import tools.approval as ap + assert hasattr(ap, "resolve_gateway_approval"), \ + "tools.approval must export resolve_gateway_approval" + + def test_approval_entry_exported(self): + import tools.approval as ap + assert hasattr(ap, "_ApprovalEntry"), \ + "tools.approval must export _ApprovalEntry" + + +# ── HTTP regression tests (test server, port 8788) ─────────────────────────── + +class TestApprovalHTTPEndpoints: + """ + Regression tests for /api/approval/respond against the live test server. + These verify that the HTTP layer behaves correctly — they don't rely on + in-process module state shared with the server subprocess. + """ + + def test_respond_returns_ok_no_pending(self): + """respond with no pending entry returns ok (no crash, no 500).""" + sid = f"http-no-pending-{uuid.uuid4().hex[:8]}" + result, status = post("/api/approval/respond", { + "session_id": sid, + "choice": "deny", + }) + assert status == 200 + assert result["ok"] is True + + def test_respond_clears_injected_pending(self): + """Inject a pending entry, respond, verify it's cleared.""" + sid = f"http-clear-{uuid.uuid4().hex[:8]}" + cmd = "rm -rf /tmp/testdir" + + inject = get(f"/api/approval/inject_test?session_id={urllib.parse.quote(sid)}" + f"&pattern_key=recursive+delete&command={urllib.parse.quote(cmd)}") + assert inject["ok"] is True + + data = get(f"/api/approval/pending?session_id={urllib.parse.quote(sid)}") + assert data["pending"] is not None + + result, status = post("/api/approval/respond", { + "session_id": sid, + "choice": "deny", + }) + assert status == 200 + assert result["ok"] is True + + data2 = get(f"/api/approval/pending?session_id={urllib.parse.quote(sid)}") + assert data2["pending"] is None, "pending should be cleared after respond" + + def test_respond_rejects_invalid_choice(self): + """respond with an unknown choice returns 400.""" + result, status = post("/api/approval/respond", { + "session_id": "some-session", + "choice": "INVALID", + }) + assert status == 400 + + def test_respond_requires_session_id(self): + """respond without session_id returns 400.""" + result, status = post("/api/approval/respond", {"choice": "deny"}) + assert status == 400 + + def test_respond_session_choice_clears_pending(self): + """Inject pending, respond with 'session', verify cleared.""" + sid = f"http-session-{uuid.uuid4().hex[:8]}" + inject = get(f"/api/approval/inject_test?session_id={urllib.parse.quote(sid)}" + f"&pattern_key=force+kill+processes&command=pkill+-9+something") + assert inject["ok"] is True + + result, status = post("/api/approval/respond", { + "session_id": sid, + "choice": "session", + }) + assert status == 200 + assert result["choice"] == "session" + + data = get(f"/api/approval/pending?session_id={urllib.parse.quote(sid)}") + assert data["pending"] is None diff --git a/tests/test_sprint30.py b/tests/test_sprint30.py new file mode 100644 index 0000000..6bc6a92 --- /dev/null +++ b/tests/test_sprint30.py @@ -0,0 +1,282 @@ +""" +Sprint 30: Approval card UI, i18n coverage, and approval flow polish. + +Tests for: +- Approval card HTML structure (all 4 buttons, IDs, data-i18n attrs) +- Keyboard shortcut handler presence in boot.js +- i18n keys for approval card in both locales +- CSS for approval-btn states (loading, disabled, kbd badge) +- respondApproval loading/disable pattern in messages.js +- streaming.py scoping fix (_unreg_notify=None initialisation) +- Approval respond HTTP endpoint (existing + new behaviour) +""" + +import json +import re +import urllib.request +import urllib.error +import urllib.parse + +import pytest + +BASE = "http://127.0.0.1:8788" + + +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 + + +def read(path): + with open(path, encoding="utf-8") as f: + return f.read() + + +import pathlib +REPO = pathlib.Path(__file__).parent.parent + + +# ── HTML structure ─────────────────────────────────────────────────────────── + +class TestApprovalCardHTML: + + def test_approval_card_has_four_buttons(self): + html = read(REPO / "static/index.html") + for choice in ("once", "session", "always", "deny"): + assert f"respondApproval('{choice}')" in html, \ + f"approval button for '{choice}' missing from index.html" + + def test_approval_buttons_have_ids(self): + html = read(REPO / "static/index.html") + for btn_id in ("approvalBtnOnce", "approvalBtnSession", + "approvalBtnAlways", "approvalBtnDeny"): + assert f'id="{btn_id}"' in html, \ + f"button id '{btn_id}' missing from approval card" + + def test_approval_heading_has_data_i18n(self): + html = read(REPO / "static/index.html") + assert 'data-i18n="approval_heading"' in html, \ + "approval heading missing data-i18n attribute" + + def test_approval_buttons_have_data_i18n_labels(self): + html = read(REPO / "static/index.html") + for key in ("approval_btn_once", "approval_btn_session", + "approval_btn_always", "approval_btn_deny"): + assert f'data-i18n="{key}"' in html, \ + f"button label data-i18n='{key}' missing" + + def test_approval_once_button_has_kbd_badge(self): + html = read(REPO / "static/index.html") + assert '' in html, \ + "kbd badge missing from Allow once button" + + def test_approval_card_has_aria_roles(self): + html = read(REPO / "static/index.html") + assert 'role="alertdialog"' in html, \ + "approval card missing role=alertdialog for accessibility" + assert 'aria-labelledby="approvalHeading"' in html, \ + "approval card missing aria-labelledby" + + +# ── CSS ────────────────────────────────────────────────────────────────────── + +class TestApprovalCardCSS: + + def test_btn_disabled_style_present(self): + css = read(REPO / "static/style.css") + assert ".approval-btn:disabled" in css, \ + "disabled state style missing for approval buttons" + + def test_btn_loading_class_present(self): + css = read(REPO / "static/style.css") + assert ".approval-btn.loading" in css, \ + "loading class style missing for approval buttons" + + def test_approval_kbd_style_present(self): + css = read(REPO / "static/style.css") + assert ".approval-kbd" in css, \ + ".approval-kbd style missing from style.css" + + def test_approval_kbd_hidden_on_mobile(self): + css = read(REPO / "static/style.css") + # Should be display:none inside the mobile media query + assert ".approval-kbd{display:none;}" in css or \ + ".approval-kbd { display: none; }" in css or \ + re.search(r'\.approval-kbd\s*\{[^}]*display\s*:\s*none', css), \ + ".approval-kbd should be hidden on mobile" + + def test_btn_transform_on_hover(self): + css = read(REPO / "static/style.css") + assert "translateY(-1px)" in css, \ + "hover lift effect missing from approval buttons" + + def test_four_choice_styles_present(self): + css = read(REPO / "static/style.css") + for cls in (".approval-btn.once", ".approval-btn.session", + ".approval-btn.always", ".approval-btn.deny"): + assert cls in css, f"CSS class '{cls}' missing" + + +# ── i18n keys ──────────────────────────────────────────────────────────────── + +class TestApprovalI18nKeys: + + REQUIRED_KEYS = [ + "approval_heading", + "approval_btn_once", + "approval_btn_session", + "approval_btn_always", + "approval_btn_deny", + "approval_responding", + ] + + def test_english_locale_has_all_approval_keys(self): + src = read(REPO / "static/i18n.js") + # Find en locale block (before the first closing };) + 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_approval_keys(self): + src = read(REPO / "static/i18n.js") + # Find zh locale block (from ` zh: {` to the closing ` },` before `};`) + 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_approval_heading_english_value(self): + src = read(REPO / "static/i18n.js") + assert "approval_heading: 'Approval required'" in src, \ + "English approval_heading value incorrect" + + def test_approval_btn_once_english_value(self): + src = read(REPO / "static/i18n.js") + assert "approval_btn_once: 'Allow once'" in src, \ + "English approval_btn_once value incorrect" + + def test_approval_btn_deny_english_value(self): + src = read(REPO / "static/i18n.js") + assert "approval_btn_deny: 'Deny'" in src, \ + "English approval_btn_deny value incorrect" + + +# ── messages.js behaviour ──────────────────────────────────────────────────── + +class TestApprovalMessagesJS: + + def test_show_approval_card_re_enables_buttons(self): + src = read(REPO / "static/messages.js") + assert "b.disabled = false" in src and "loading" in src, \ + "showApprovalCard should re-enable buttons on each show" + + def test_respond_disables_buttons_immediately(self): + src = read(REPO / "static/messages.js") + assert "b.disabled = true" in src, \ + "respondApproval should disable buttons immediately to prevent double-submit" + + def test_respond_uses_i18n_for_error(self): + src = read(REPO / "static/messages.js") + # Should use t('approval_responding') not a hardcoded string + assert "t(\"approval_responding\")" in src or "t('approval_responding')" in src, \ + "respondApproval error message should use t('approval_responding')" + + def test_show_card_applies_locale_to_dom(self): + src = read(REPO / "static/messages.js") + assert "applyLocaleToDOM" in src, \ + "showApprovalCard should call applyLocaleToDOM to translate data-i18n labels" + + def test_show_card_focuses_once_button(self): + src = read(REPO / "static/messages.js") + assert "approvalBtnOnce" in src and "focus()" in src, \ + "showApprovalCard should focus the Allow once button" + + +# ── boot.js keyboard shortcut ──────────────────────────────────────────────── + +class TestApprovalKeyboardShortcut: + + def test_enter_shortcut_present_in_boot_js(self): + src = read(REPO / "static/boot.js") + assert "respondApproval('once')" in src or 'respondApproval("once")' in src, \ + "Enter shortcut calling respondApproval('once') missing from boot.js" + + def test_enter_shortcut_checks_card_visible(self): + src = read(REPO / "static/boot.js") + assert "approvalCard" in src and "visible" in src, \ + "Enter shortcut should check if approval card is visible" + + def test_enter_shortcut_guards_input_elements(self): + src = read(REPO / "static/boot.js") + assert "TEXTAREA" in src and "INPUT" in src, \ + "Enter shortcut should not fire when focus is on TEXTAREA or INPUT" + + +# ── streaming.py scoping fix ───────────────────────────────────────────────── + +class TestStreamingApprovalScoping: + + def test_unreg_notify_initialised_to_none(self): + src = read(REPO / "api/streaming.py") + assert "_unreg_notify = None" in src, \ + "_unreg_notify must be initialised to None before the try block" + + def test_finally_checks_unreg_notify_not_none(self): + src = read(REPO / "api/streaming.py") + assert "_unreg_notify is not None" in src, \ + "finally block must check '_unreg_notify is not None' before calling it" + + def test_approval_registered_flag_present(self): + src = read(REPO / "api/streaming.py") + assert "_approval_registered = False" in src, \ + "_approval_registered flag must be initialised to False" + + +# ── HTTP regression: approval respond ──────────────────────────────────────── + +class TestApprovalRespondHTTP: + + def test_respond_ok_with_all_choices(self): + for choice in ("once", "session", "always", "deny"): + import uuid + sid = f"sprint30-{uuid.uuid4().hex[:8]}" + result, status = post("/api/approval/respond", + {"session_id": sid, "choice": choice}) + assert status == 200, f"choice={choice} should return 200" + assert result["ok"] is True + assert result["choice"] == choice + + def test_respond_rejects_bad_choice(self): + result, status = post("/api/approval/respond", + {"session_id": "x", "choice": "HACKED"}) + assert status == 400 + + def test_respond_requires_session_id(self): + result, status = post("/api/approval/respond", {"choice": "deny"}) + assert status == 400 + + def test_respond_returns_choice_field(self): + import uuid + sid = f"sprint30-choice-{uuid.uuid4().hex[:8]}" + result, status = post("/api/approval/respond", + {"session_id": sid, "choice": "always"}) + assert status == 200 + assert "choice" in result + assert result["choice"] == "always"