From d0cb5f0ac5052a17ab9d833a40e43e2218c94dd1 Mon Sep 17 00:00:00 2001
From: Henry Park <henrypark133@gmail.com>
Date: Mon, 16 Mar 2026 20:06:15 -0700
Subject: [PATCH] test(e2e): fix approval waiting regression coverage (#1270)

* test(e2e): fix approval waiting regression coverage

* test(e2e): address Copilot review notes
---
 tests/e2e/CLAUDE.md                       |  4 +-
 tests/e2e/README.md                       |  6 ++-
 tests/e2e/mock_llm.py                     |  9 ++++
 tests/e2e/scenarios/test_tool_approval.py | 57 +++++++++++------------
 4 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/tests/e2e/CLAUDE.md b/tests/e2e/CLAUDE.md
index c977b6fd..0cf5e6dc 100644
--- a/tests/e2e/CLAUDE.md
+++ b/tests/e2e/CLAUDE.md
@@ -52,7 +52,7 @@ HEADED=1 pytest scenarios/
 | `test_html_injection.py` | XSS vectors injected directly via `page.evaluate("addMessage('assistant', ...)")` are sanitized by `renderMarkdown`; user messages are shown as escaped plain text |
 | `test_skills.py` | Skills tab UI visibility, ClawHub search (skipped if registry unreachable), install + remove lifecycle |
 | `test_sse_reconnect.py` | SSE reconnects after programmatic `eventSource.close()` + `connectSSE()`; history is reloaded after reconnect |
-| `test_tool_approval.py` | Approval card appears, buttons disable on approve/deny, parameters toggle; all triggered via `page.evaluate("showApproval(...)")` — no real tool call needed |
+| `test_tool_approval.py` | Approval card appears, buttons disable on approve/deny, parameters toggle via `page.evaluate("showApproval(...)")`; the waiting-approval regression uses a real HTTP tool call |
 
 ## `helpers.py`
 
@@ -164,7 +164,7 @@ async def test_my_ui_feature(page):
 - **`asyncio_default_fixture_loop_scope = "session"`** — all async fixtures share one event loop. Do not use `asyncio.run()` inside fixtures; use `await` directly.
 - **The `page` fixture navigates with `/?token=e2e-test-token` and waits for `#auth-screen` to be hidden.** Tests receive a page that is already past the auth screen and has SSE connected.
 - **`test_skills.py` makes real network calls to ClawHub.** Tests skip (not fail) if the registry is unreachable via `pytest.skip()`.
-- **`test_html_injection.py` and `test_tool_approval.py` inject state via `page.evaluate(...)`.** They test the browser-side rendering pipeline and do not depend on the LLM or backend tool execution.
+- **`test_html_injection.py` injects state via `page.evaluate(...)`, and most of `test_tool_approval.py` does too.** The waiting-approval regression in `test_tool_approval.py` intentionally uses a real tool approval flow so it can verify backend thread-state handling.
 - **Browser is Chromium only.** `conftest.py` uses `p.chromium.launch()`; there is no Firefox or WebKit variant.
 - **Default timeout is 120 seconds** (pyproject.toml). Individual `wait_for` calls inside tests use shorter timeouts (5–20s) for faster failure messages.
 - **The libsql database is a temp directory** created fresh per `pytest` invocation; tests do not share state across runs.
diff --git a/tests/e2e/README.md b/tests/e2e/README.md
index 5aac9613..17e1378b 100644
--- a/tests/e2e/README.md
+++ b/tests/e2e/README.md
@@ -164,5 +164,7 @@ await page.evaluate("""
 """)
 ```
 
-This is the pattern used in `test_tool_approval.py` and parts of
-`test_extensions.py` (auth card, configure modal).
+This is the pattern used in most of `test_tool_approval.py` and parts of
+`test_extensions.py` (auth card, configure modal). The waiting-approval
+regression in `test_tool_approval.py` uses a real tool call instead so it can
+exercise backend approval state.
diff --git a/tests/e2e/mock_llm.py b/tests/e2e/mock_llm.py
index b091fc17..c27f2762 100644
--- a/tests/e2e/mock_llm.py
+++ b/tests/e2e/mock_llm.py
@@ -25,6 +25,15 @@ DEFAULT_RESPONSE = "I understand your request."
 
 TOOL_CALL_PATTERNS = [
     (re.compile(r"echo (.+)", re.IGNORECASE), "echo", lambda m: {"message": m.group(1)}),
+    (
+        re.compile(r"make approval post (?P<label>[a-z0-9_-]+)", re.IGNORECASE),
+        "http",
+        lambda m: {
+            "method": "POST",
+            "url": f"https://example.com/{m.group('label')}",
+            "body": {"label": m.group("label")},
+        },
+    ),
     (re.compile(r"what time|current time", re.IGNORECASE), "time", lambda _: {"operation": "now"}),
     (
         re.compile(
diff --git a/tests/e2e/scenarios/test_tool_approval.py b/tests/e2e/scenarios/test_tool_approval.py
index 44418e46..f3144e36 100644
--- a/tests/e2e/scenarios/test_tool_approval.py
+++ b/tests/e2e/scenarios/test_tool_approval.py
@@ -135,42 +135,39 @@ async def test_approval_params_toggle(page):
 async def test_waiting_for_approval_message_no_error_prefix(page):
     """Verify that input submitted while awaiting approval shows non-error status with tool context.
 
-    Tests the real flow: show approval card, then attempt to send input while approval is pending.
-    Backend rejects with Pending result (not Error), and message includes tool context.
+    Trigger a real approval-needed tool call, then attempt to send another message while
+    approval is pending. The backend should reject the second input with a non-error
+    status that includes the pending tool context.
     """
-    # First, inject an approval card to simulate the thread being in AwaitingApproval state
-    await page.evaluate("""
-        showApproval({
-            request_id: 'test-req-waiting-approval',
-            thread_id: currentThreadId,
-            tool_name: 'shell',
-            description: 'Execute: echo hello',
-            parameters: '{"command": "echo hello"}'
-        })
-    """)
-
-    # Wait for approval card to be visible (thread is now in AwaitingApproval state)
-    card = page.locator('.approval-card[data-request-id="test-req-waiting-approval"]')
-    await card.wait_for(state="visible", timeout=5000)
-
-    # Record initial message count
-    initial_count = await page.locator(SEL["message_assistant"]).count()
-
-    # Now attempt to send input while approval is pending
-    # (the backend will reject this and return the "Waiting for approval" status message)
+    assistant_messages = page.locator(SEL["message_assistant"])
     chat_input = page.locator(SEL["chat_input"])
-    await chat_input.fill("Test input while awaiting approval")
+    await chat_input.wait_for(state="visible", timeout=5000)
+
+    # Trigger a real HTTP tool call that pauses for approval in the default E2E harness.
+    await chat_input.fill("make approval post approval-required")
+    await chat_input.press("Enter")
+
+    card = page.locator(SEL["approval_card"]).last
+    await card.wait_for(state="visible", timeout=10000)
+
+    tool_name = await card.locator(".approval-tool-name").text_content()
+    desc_text = await card.locator(".approval-description").text_content()
+    assert tool_name == "http"
+    assert desc_text is not None and "HTTP requests to external APIs" in desc_text
+
+    # With the thread now genuinely awaiting approval, the next message should be rejected
+    # as a non-error pending status.
+    initial_count = await assistant_messages.count()
+    await chat_input.fill("send another message now")
     await chat_input.press("Enter")
 
-    # Wait for the status message from the backend rejection
     await page.wait_for_function(
         f"() => document.querySelectorAll('{SEL['message_assistant']}').length > {initial_count}",
         timeout=10000,
     )
 
-    # Get the new status message
-    last_msg = page.locator(SEL["message_assistant"]).last
-    msg_text = await last_msg.text_content()
+    last_msg = assistant_messages.last.locator(".message-content")
+    msg_text = await last_msg.inner_text()
 
     # Verify no "Error:" prefix
     assert not msg_text.lower().startswith("error:"), (
@@ -183,9 +180,9 @@ async def test_waiting_for_approval_message_no_error_prefix(page):
     )
 
     # Verify it contains the tool name and description
-    assert "shell" in msg_text.lower(), (
-        f"Expected tool name 'shell' in message. Got: {msg_text!r}"
+    assert "http" in msg_text.lower(), (
+        f"Expected tool name 'http' in message. Got: {msg_text!r}"
     )
-    assert "echo hello" in msg_text, (
+    assert "HTTP requests to external APIs" in msg_text, (
         f"Expected tool description in message. Got: {msg_text!r}"
     )