feat(chat): render markdown tables in answers, viewer, and thinking

GFM pipe tables in the shared renderer (TODO.md L6): a table-protection pass in frontend/assets/markdown.js (fences -> tables -> escape order) pulls each header+separator+body block out as a placeholder, renders cells escape-first with the same inline transforms, and reinserts a semantic <table class="md-table"> inside a horizontal-overflow .md-table-wrap — so a pipe table in a chat answer, the document viewer/modal, and the thinking block all render the same semantic table. Fences win over tables; lone pipes stay text. - styles.css: .md-table palette rules (PLAN §7.2 tokens, no motion); min-width: max-content so a WIDE table keeps its natural width and the wrapper is the real scroller (width:100% alone wrapped the wide table's cells — proven by the new E2E). - mock_llm.py: TABLE_TRIGGER ("show me a table") -> byte-stable TABLE_ANSWER (3-column table, <img onerror> XSS probe line, wide 5-column table), checked before DEFLECT_MODE like SUMMARY_MODE. - tests/fixtures/docs/homelab/tables.md: 3x3 pipe table + pipe-heavy fenced block (viewer/fence subject); the shared fixture set grows 8 -> 9 docs, so every suite pinning the count (added/formats/ stat-docs/EXPECTED_ROWS) is updated accordingly. - tests/e2e/test_markdown_tables.py (new, story suite): chat table shape + non-deflection, wide-table wrapper scroll (no page overflow), XSS probe inert, viewer modal table, fence-not-a-table, lone pipe stays text. - tests/e2e/test_agent_document_tools.py: fix a pre-existing flake — the "Calling tool…" label window is ~0.4 s at the mock's 0.1 s tool-frame pacing, and a polling expect could stride over it (failed 3 of 5 runs on the committed baseline). The pre-submit MutationObserver record is the deterministic source of truth; the racy to_have_text gate is gone. uv run pytest: 738 passed, app/ coverage 99% (TOTAL unchanged); ruff + pyright clean; story E2E 6/6 in isolation; regression E2E suites (chat_rag, document_viewer, document_summaries, smoke) green.
2026-08-28 03:35:50 -04:00
parent 27b7cb96d5
commit bc70ce36e0
30 changed files with 967 additions and 73 deletions
@@ -0,0 +1,350 @@
+"""Phase 44 E2E (Playwright): GFM pipe tables in the shared renderer.
+
+Story: ``.agent/user_stories/markdown-tables.md``
+Run in isolation (DB must be up: ``podman compose up -d db``):
+
+    uv run pytest tests/e2e/test_markdown_tables.py -v --no-cov
+
+Seeding reuses the real importer against ``tests/fixtures/docs/`` with
+the deterministic mock embeddings (same pattern as ``test_chat_rag.py``).
+The mock's ``TABLE_TRIGGER`` (``show me a table``, phase 44 task 02)
+returns the byte-stable table answer: a 3-column service table, an
+``<img onerror>`` XSS probe line, and a deliberately wide 5-column
+table. The phase-44 fixture ``homelab/tables.md`` (a 3×3 pipe table
+plus a pipe-heavy fenced block) is the viewer/fence subject — the
+document viewer is database-only, so the imported row is enough.
+
+Test → story mapping (Playwright Mapping Rule):
+1. ``test_chat_table_renders`` — the brain bubble carries
+   ``<div class="md-table-wrap"><table class="md-table">`` with a
+   ``<thead>`` of three ``<th scope="col">`` (Service/Port/Host), the
+   expected body cells, no raw ``|---|`` separator text, and the turn is
+   NOT deflected (the honesty-gate interplay is part of the contract).
+2. ``test_wide_table_scrolls`` — the wide table's wrapper has
+   ``scrollWidth > clientWidth`` and horizontal scroll moves it; the
+   page itself has no horizontal overflow (the 46rem column holds).
+3. ``test_table_xss_safe`` — the ``<img onerror>`` line renders as
+   visible, escaped text: zero injected ``<img>`` nodes, no dialog.
+4. ``test_viewer_table_renders`` — the fixture's pipe table opens from
+   the Sources table (admin) in the modal and renders the same
+   ``<table class="md-table">`` (shared renderer, story AC6).
+5. ``test_fence_not_a_table`` — the fixture's pipe-heavy fenced block
+   renders ``<pre><code>``; the only ``<table>`` in the document is the
+   real pipe table (fences win, story AC3).
+6. ``test_plain_pipe_stays_text`` — a grounded prose answer with a lone
+   ``|`` (the mock echoes the question) renders as text, no
+   ``<table>`` (story AC4).
+"""
+from __future__ import annotations
+
+import asyncio
+from pathlib import Path
+from threading import Thread
+from typing import Any
+
+from playwright.sync_api import Page, expect
+from sqlalchemy import text
+
+from app.config import Settings
+from app.db import SessionLocal
+from app.rag.importer import ImportSummary, import_sources
+from app.rag.llm import LLMClient
+from e2e.auth_helpers import login
+
+REPO = Path(__file__).resolve().parents[2]
+FIXTURES = REPO / "tests" / "fixtures" / "docs"
+
+#: Carries the mock's ``TABLE_TRIGGER`` ("show me a table") and is
+#: on-topic (the fixture set answers it — FTS-OR grounds it, so the
+#: turn is HIGH and the suite can assert non-deflection).
+QUESTION = "Show me a table of my homelab services?"
+#: Grounded kubernetes question with a single ``|`` in the prose — the
+#: mock's default branch echoes the question (first 80 chars), so the
+#: lone pipe lands in the rendered answer.
+PLAIN_QUESTION = "How is my Kubernetes cluster set up? A lone | in prose stays text."
+MOCK_ANSWER_MARKER = "Deterministic mock answer for E2E"
+TABLES_PATH = "homelab/tables.md"
+WIDE_HEADER = "A very long column header to force overflow"
+XSS_LINE = "<img src=x onerror=alert(1)>"
+
+EXPECTED_HEADER = ["Service", "Port", "Host"]
+EXPECTED_ROWS = [
+    ["Caddy", "80", "homelab-gw"],
+    ["GitLab", "8929", "homelab-git"],
+    ["ntfy", "2087", "homelab-ntfy"],
+]
+
+
+async def _import_fixtures(mock_port: int) -> ImportSummary:
+    kwargs: dict[str, Any] = {"_env_file": None, "llm_base_url": f"http://127.0.0.1:{mock_port}/v1"}
+    settings = Settings(**kwargs)  # pyright: ignore[reportCallIssue]
+    return await import_sources([FIXTURES], LLMClient(settings))
+
+
+def _run_in_thread(coro: Any) -> Any:
+    """Run a coroutine on a worker thread.
+
+    Playwright's sync API keeps an asyncio loop running on the test
+    thread, so ``asyncio.run`` cannot be called directly from a test
+    body (the established house helper).
+    """
+    box: dict[str, Any] = {}
+
+    def runner() -> None:
+        try:
+            box["value"] = asyncio.run(coro)
+        except BaseException as e:  # noqa: BLE001 — re-raised on the test thread
+            box["error"] = e
+
+    t = Thread(target=runner)
+    t.start()
+    t.join()
+    if "error" in box:
+        raise box["error"]
+    return box["value"]
+
+
+def _reset_db(mock_port: int, seed: bool) -> ImportSummary | None:
+    """Truncate the KB (+ the global prompt-state rows), then optionally
+    re-import the fixtures (9 docs since phase 44 added tables.md)."""
+    with SessionLocal() as db:
+        db.execute(
+            text("TRUNCATE chunks, documents, query_log, steering_notes, kb_overview")
+        )
+        db.commit()
+    if not seed:
+        return None
+    return _run_in_thread(_import_fixtures(mock_port))
+
+
+def _ask_table_answer(page: Page, app_url: str) -> Any:
+    """Drive the trigger question and return the brain bubble once the
+    whole byte-stable table answer has streamed in (the wide table's
+    last cell lands last)."""
+    page.goto(app_url)
+    page.fill("#message-input", QUESTION)
+    page.click("#send-btn")
+    bubble = page.locator(".msg.brain .bubble").first
+    bubble.wait_for(state="visible", timeout=30_000)
+    expect(bubble).to_contain_text("value-five", timeout=30_000)
+    # Non-deflection is part of the table contract (honesty gate interplay).
+    expect(page.locator(".msg.brain.is-deflected")).to_have_count(0)
+    return bubble
+
+
+def _open_tables_doc_modal(page: Page, app_url: str) -> None:
+    """Admin → Sources → the tables.md row → same-page document modal."""
+    login(page, app_url)  # phase 16: the Sources catalog is admin-only
+    row = page.locator("#docs-tbody tr", has_text=TABLES_PATH)
+    expect(row).to_have_count(1)
+    row.locator("td:nth-child(2) a.doc-link").click()
+    expect(page.locator(".doc-modal")).to_be_visible()
+    expect(page.locator("#doc-modal-title")).to_have_text("Service Port Table")
+
+
+# ---------------------------------------------------------------------------
+# 1. Chat: the pipe table renders as a semantic table
+# ---------------------------------------------------------------------------
+
+
+def test_chat_table_renders(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    summary = _reset_db(mock_llm, seed=True)
+    assert summary is not None and summary.added == 9  # phase 44: +tables.md
+    page.set_default_timeout(30_000)
+    bubble = _ask_table_answer(page, app_url)
+
+    # Both tables of the answer rendered: the 3-column service table and
+    # the wide one — each in its horizontal-overflow wrapper.
+    tables = bubble.locator("table.md-table")
+    expect(tables).to_have_count(2)
+    expect(bubble.locator(".md-table-wrap")).to_have_count(2)
+
+    # The 3×3 table: <thead> of three <th scope="col"> + the body cells
+    # (the whole answer has already streamed in — the DOM is settled).
+    first = tables.nth(0)
+    headers = first.locator("thead th[scope='col']")
+    expect(headers).to_have_count(3)
+    assert headers.all_inner_texts() == EXPECTED_HEADER
+    rows = first.locator("tbody tr")
+    expect(rows).to_have_count(3)
+    for i, cells in enumerate(EXPECTED_ROWS):
+        assert rows.nth(i).locator("td").all_inner_texts() == cells
+
+    # The raw markdown must not survive: no separator row, no raw header
+    # row as text anywhere in the bubble.
+    bubble_text = bubble.inner_text()
+    assert "|---|" not in bubble_text, "the |---| separator leaked into the bubble"
+    assert "| Service | Port | Host |" not in bubble_text, "the raw header row leaked"
+
+    # Grounded retrieval: the table fixture is the top source chip.
+    chip = page.locator(".msg.brain .source-chip", has_text="homelab/tables.md")
+    expect(chip).to_have_count(1)
+
+
+# ---------------------------------------------------------------------------
+# 2. Wide table: the wrapper scrolls, the page does not
+# ---------------------------------------------------------------------------
+
+
+def test_wide_table_scrolls(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    page.set_default_timeout(30_000)
+    bubble = _ask_table_answer(page, app_url)
+
+    # The wide table (5 columns, one deliberately long header) sits in
+    # ITS wrapper — the 3-column table's wrapper is not the scroller.
+    wrap = bubble.locator(".md-table-wrap", has=page.locator("th", has_text=WIDE_HEADER))
+    expect(wrap).to_have_count(1)
+    scroll_width, client_width = wrap.evaluate(
+        "el => [el.scrollWidth, el.clientWidth]"
+    )
+    assert scroll_width > client_width, (
+        f"the wide table must overflow its wrapper "
+        f"(scrollWidth {scroll_width} <= clientWidth {client_width})"
+    )
+
+    # Horizontal scrolling (scrollLeft) moves the wrapper's content.
+    before = wrap.evaluate("el => el.scrollLeft")
+    wrap.evaluate("el => { el.scrollLeft = 120; }")
+    after = wrap.evaluate("el => el.scrollLeft")
+    assert after > before, "the wrapper must scroll horizontally"
+
+    # The 46rem chat column must not break the page: no horizontal
+    # document overflow (PLAN §7.1).
+    page_scroll, page_client = page.evaluate(
+        "() => [document.documentElement.scrollWidth, document.documentElement.clientWidth]"
+    )
+    assert page_scroll <= page_client, (
+        f"the page overflowed horizontally ({page_scroll} > {page_client})"
+    )
+
+
+# ---------------------------------------------------------------------------
+# 3. XSS-safe: the <img onerror> probe renders inert text
+# ---------------------------------------------------------------------------
+
+
+def test_table_xss_safe(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    page.set_default_timeout(30_000)
+
+    dialogs: list[str] = []
+
+    def _catch(d) -> None:  # a fired dialog == the probe executed
+        dialogs.append(d.message)
+        d.dismiss()
+
+    page.on("dialog", _catch)
+    _ask_table_answer(page, app_url)
+
+    state = page.evaluate(
+        """() => {
+            const el = document.querySelector('.msg.brain .bubble');
+            return {
+                imgs: el.querySelectorAll('img').length,
+                onerror: el.querySelectorAll('[onerror]').length,
+                text: el.innerText,
+                html: el.innerHTML,
+            };
+        }"""
+    )
+    assert state["imgs"] == 0, "the XSS probe became a live <img> element"
+    assert state["onerror"] == 0, "an onerror attribute survived into the DOM"
+    # The escaped tag renders as VISIBLE text (the escape-first contract).
+    assert XSS_LINE in state["text"], "the probe line must be visible text"
+    assert "&lt;img src=x onerror=alert(1)&gt;" in state["html"]
+    assert dialogs == [], f"dialog fired — the probe executed: {dialogs}"
+
+
+# ---------------------------------------------------------------------------
+# 4. Shared renderer: the viewer/modal renders the fixture's table
+# ---------------------------------------------------------------------------
+
+
+def test_viewer_table_renders(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    page.set_default_timeout(30_000)
+    _open_tables_doc_modal(page, app_url)
+
+    # The same <table class="md-table"> shape the chat bubble gets — the
+    # shared renderer (story AC6) serves the viewer too.
+    table = page.locator("#doc-modal-content table.md-table")
+    expect(table).to_have_count(1)
+    headers = table.locator("thead th[scope='col']")
+    expect(headers).to_have_count(3)
+    assert headers.all_inner_texts() == EXPECTED_HEADER
+    rows = table.locator("tbody tr")
+    expect(rows).to_have_count(3)
+    for i, cells in enumerate(EXPECTED_ROWS):
+        assert rows.nth(i).locator("td").all_inner_texts() == cells
+    assert (
+        "|---|" not in page.locator("#doc-modal-content").inner_text()
+    ), "the separator row leaked into the viewer"
+
+
+# ---------------------------------------------------------------------------
+# 5. Fences win: the pipe-heavy fenced block is code, never a table
+# ---------------------------------------------------------------------------
+
+
+def test_fence_not_a_table(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    page.set_default_timeout(30_000)
+    _open_tables_doc_modal(page, app_url)
+
+    # The fixture's ``` block (pipe table inside) renders as code —
+    # fence protection runs before the table pass (story AC3).
+    pre = page.locator("#doc-modal-content pre code")
+    expect(pre).to_have_count(1)
+    expect(pre).to_contain_text("caddy", timeout=30_000)
+    code_text = pre.inner_text()
+    assert "| Service  | Port |" in code_text, "the fenced header line must stay raw"
+    assert "|----------|------|" in code_text, "the fenced separator must stay raw"
+    assert "| caddy    | 80   |" in code_text
+    assert "| gitlab   | 8929 |" in code_text
+
+    # Exactly ONE table in the whole document — the real pipe table. The
+    # fenced rows (lowercase "caddy"/"gitlab") must not become cells.
+    table = page.locator("#doc-modal-content table.md-table")
+    expect(table).to_have_count(1)
+    cells = table.locator("th, td").all_inner_texts()
+    assert "caddy" not in cells and "gitlab" not in cells, (
+        "the fenced pipe block was parsed as a table"
+    )
+
+
+# ---------------------------------------------------------------------------
+# 6. Non-tables stay put: a lone pipe in grounded prose renders as text
+# ---------------------------------------------------------------------------
+
+
+def test_plain_pipe_stays_text(
+    page: Page, app_url: str, mock_llm: int, db_ready: None
+) -> None:
+    _reset_db(mock_llm, seed=True)
+    page.set_default_timeout(30_000)
+    page.goto(app_url)
+    page.fill("#message-input", PLAIN_QUESTION)
+    page.click("#send-btn")
+
+    bubble = page.locator(".msg.brain .bubble").first
+    bubble.wait_for(state="visible", timeout=30_000)
+    expect(bubble).to_contain_text(MOCK_ANSWER_MARKER, timeout=30_000)
+    # Grounded (the kubernetes FTS hit), not deflected — this is the
+    # default-answer path, so the echoed question is what we assert on.
+    expect(page.locator(".msg.brain.is-deflected")).to_have_count(0)
+
+    # A single "|" in prose is not a table (no header + separator pair).
+    expect(bubble.locator("table")).to_have_count(0)
+    expect(bubble.locator(".md-table-wrap")).to_have_count(0)
+    assert "A lone | in prose stays text" in bubble.inner_text()