fix(chat): keep the in-flight answer when navigating away mid-turn — partial answer restored on return
This commit is contained in:
+30
-3
@@ -18,6 +18,10 @@ Implements just enough of the aipi surface:
|
||||
- user message containing ``think out loud`` -> the answer is preceded by
|
||||
~800 chars of deterministic ``reasoning_content`` chunks (the
|
||||
thinking-display story, phase 17).
|
||||
- user message containing ``think out loud then hesitate`` -> the
|
||||
``think out loud`` stream, then a 4s pause before the first content
|
||||
frame (the sources-midstream story, phase 20 — a deterministic
|
||||
"leave during pure thinking" navigation window).
|
||||
- system prompt containing ``<tuning>`` (phase 15, steering notes) ->
|
||||
the composed answer ends with `` (tuning: <first note line>)`` —
|
||||
makes prompt injection observable in the UI deterministically.
|
||||
@@ -89,6 +93,16 @@ LONG_ANSWER_END = "LONG-ANSWER-END"
|
||||
#: contain the substring, so every other suite is unaffected.
|
||||
THINKING_TRIGGER = "think out loud"
|
||||
|
||||
#: Phase 20 (sources-midstream bug): a user message containing this
|
||||
#: substring (case-insensitive) gets the phase-17 thinking stream followed
|
||||
#: by a multi-second pause before the FIRST content frame — the
|
||||
#: navigation window for the "leave during pure thinking" scenario
|
||||
#: (owner-confirmed A1.2: nothing brain-side may be persisted then).
|
||||
#: Strictly longer than ``THINKING_TRIGGER``, so the phase-17 suite's
|
||||
#: questions are unaffected.
|
||||
SLOW_PRETOKEN_TRIGGER = "think out loud then hesitate"
|
||||
PRE_CONTENT_PAUSE_S = 4.0
|
||||
|
||||
|
||||
def long_answer() -> str:
|
||||
"""~900-word deterministic walkthrough (phase 11): numbered steps plus
|
||||
@@ -222,7 +236,9 @@ def embeddings(body: dict[str, Any]) -> dict[str, Any]:
|
||||
}
|
||||
|
||||
|
||||
def _sse_stream(answer: str, delay: float, thinking: str = "") -> Any:
|
||||
def _sse_stream(
|
||||
answer: str, delay: float, thinking: str = "", pre_content_delay: float = 0.0
|
||||
) -> Any:
|
||||
"""SSE frames for one chat completion (phase 17: + reasoning).
|
||||
|
||||
When ``thinking`` is non-empty its 12-char slices go out FIRST as
|
||||
@@ -230,6 +246,11 @@ def _sse_stream(answer: str, delay: float, thinking: str = "") -> Any:
|
||||
as the content frames, the aipi wire convention (reasoning before
|
||||
content). Without ``thinking`` the output is byte-identical to the
|
||||
content-only stream, so the other story suites are unaffected.
|
||||
|
||||
``pre_content_delay`` (phase 20) inserts a silence gap between the end
|
||||
of the thinking stream and the first content frame — the client stays
|
||||
in its pre-token "thinking" state the whole time (0.02s cadence and
|
||||
frame shapes are unchanged, so 0.0 is byte-identical to before).
|
||||
"""
|
||||
model = "turbo"
|
||||
chunk_id = f"chatcmpl-{uuid.uuid4()}"
|
||||
@@ -247,6 +268,8 @@ def _sse_stream(answer: str, delay: float, thinking: str = "") -> Any:
|
||||
}
|
||||
yield f"data: {json_dumps(payload)}\n\n"
|
||||
time.sleep(0.02)
|
||||
if pre_content_delay:
|
||||
time.sleep(pre_content_delay)
|
||||
for piece in re.findall(r".{1,12}", answer, re.S):
|
||||
payload = {
|
||||
"id": chunk_id,
|
||||
@@ -295,7 +318,11 @@ def _apply_max_tokens(answer: str, max_tokens: Any) -> str:
|
||||
def chat_completions(body: dict[str, Any]) -> Any:
|
||||
answer = _apply_max_tokens(compose_answer(body), body.get("max_tokens"))
|
||||
delay = 3.0 if "pretend to think slowly" in _user(body) else 0.0
|
||||
thinking = compose_thinking(body) if THINKING_TRIGGER in _user(body).lower() else ""
|
||||
user_lower = _user(body).lower()
|
||||
thinking = compose_thinking(body) if THINKING_TRIGGER in user_lower else ""
|
||||
pre_content = (
|
||||
PRE_CONTENT_PAUSE_S if SLOW_PRETOKEN_TRIGGER in user_lower else 0.0
|
||||
)
|
||||
|
||||
if not body.get("stream"):
|
||||
message: dict[str, Any] = {"role": "assistant", "content": answer}
|
||||
@@ -315,7 +342,7 @@ def chat_completions(body: dict[str, Any]) -> Any:
|
||||
}
|
||||
|
||||
return StreamingResponse(
|
||||
_sse_stream(answer, delay, thinking=thinking),
|
||||
_sse_stream(answer, delay, thinking=thinking, pre_content_delay=pre_content),
|
||||
media_type="text/event-stream",
|
||||
headers={"Cache-Control": "no-cache", "X-Accel-Buffering": "no"},
|
||||
)
|
||||
|
||||
@@ -0,0 +1,359 @@
|
||||
"""Phase 20 E2E (Playwright): navigating away mid-turn keeps the answer.
|
||||
|
||||
Story: ``.agent/user_stories/sources-midstream.md``
|
||||
Bug report (TODO.md L3): *"Clicking "sources" while chat is generating
|
||||
clears chat and result will never show up."*
|
||||
Run in isolation (DB must be up: ``podman compose up -d db``):
|
||||
|
||||
uv run pytest tests/e2e/test_sources_midstream_bug.py -v --no-cov
|
||||
|
||||
The bug: the brain message persisted only on ``done``, so leaving the
|
||||
chat page while a turn was in flight aborted the stream and dropped
|
||||
whatever had already streamed — the user came back to their own question
|
||||
with no result, ever. The fix (phase 20, owner-confirmed A1): a single
|
||||
``pagehide`` save point in app.js persists the partial raw answer (via
|
||||
the existing ``rememberBrainTurn`` helper) when navigation hits a turn
|
||||
that is in flight and has already streamed text.
|
||||
|
||||
Timing is deterministic by construction:
|
||||
|
||||
* scenario 1 keys off the mock's ``write a long answer`` trigger — a
|
||||
~5400-char / ~450-frame / ~9s content stream, so the navigation lands
|
||||
mid-stream with a wide margin;
|
||||
* scenario 2 keys off the mock's ``think out loud then hesitate``
|
||||
trigger — the phase-17 thinking stream followed by a 4s silence before
|
||||
the first content frame, so the navigation lands inside pure thinking;
|
||||
* scenarios 3 and 4 settle the turn fully (send button re-enabled)
|
||||
before any navigation.
|
||||
|
||||
Test → story mapping (Playwright Mapping Rule):
|
||||
1. ``test_partial_answer_survives_sources_nav_midstream``
|
||||
2. ``test_no_orphan_brain_message_when_navigated_before_first_token``
|
||||
3. ``test_completed_turn_unaffected``
|
||||
4. ``test_new_chat_still_clears_conversation``
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import json
|
||||
import re
|
||||
from collections.abc import Iterator
|
||||
from pathlib import Path
|
||||
from threading import Thread
|
||||
from typing import Any
|
||||
|
||||
import pytest
|
||||
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
|
||||
from e2e.mock_llm import long_answer
|
||||
|
||||
REPO = Path(__file__).resolve().parents[2]
|
||||
FIXTURES = REPO / "tests" / "fixtures" / "docs"
|
||||
QUESTION = "How is my Kubernetes cluster set up?"
|
||||
MOCK_ANSWER_MARKER = "Deterministic mock answer for E2E"
|
||||
STORAGE_KEY = "bor.chat.v1"
|
||||
|
||||
# --- scenario 1: a turn that is guaranteed to still be in flight --------
|
||||
#: The mock's long-answer trigger (~9s content stream at 0.02s/frame).
|
||||
LONG_QUESTION = "write a long answer about how my kubernetes cluster is set up"
|
||||
FULL_LONG = long_answer()
|
||||
#: The mock's first 12-char content slice (the same cut ``_sse_stream``
|
||||
#: makes) — the stored partial must START with it (raw text, pre-render).
|
||||
FIRST_CHUNK_RAW = re.findall(r".{1,12}", FULL_LONG, re.S)[0]
|
||||
#: The rendered form of those first frames: the shared escape-first
|
||||
#: markdown renderer converts the "1. " numbered line into a list item,
|
||||
#: dropping the marker (pinned by test_long_answers).
|
||||
FIRST_LINE_DOM = "Step 1: configure node-1"
|
||||
|
||||
# --- scenario 2: navigation during pure thinking (no answer tokens) -----
|
||||
HESITATE_QUESTION = (
|
||||
"think out loud then hesitate — how is my kubernetes cluster set up?"
|
||||
)
|
||||
#: Tail of the mock's deterministic scratchpad (mock_llm.compose_thinking)
|
||||
#: — when it is rendered, the thinking stream has just ended and the 4s
|
||||
#: pre-content pause (SLOW_PRETOKEN_TRIGGER) is running.
|
||||
THINKING_TAIL = "nothing is invented"
|
||||
|
||||
#: Phase-10 viewer URL + phase-13 back=/ (byte-identical to the chip the
|
||||
#: persistence suite pins — grounded-turn sources are unchanged by 20).
|
||||
CHIP_HREF = "/document.html?source=docs&path=homelab%2Fkubernetes.md&back=%2F"
|
||||
|
||||
|
||||
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.
|
||||
"""
|
||||
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 (and query log), then optionally re-import fixtures."""
|
||||
with SessionLocal() as db:
|
||||
db.execute(text("TRUNCATE chunks, documents, query_log"))
|
||||
db.commit()
|
||||
if not seed:
|
||||
return None
|
||||
return _run_in_thread(_import_fixtures(mock_port))
|
||||
|
||||
|
||||
def _stored(page: Page) -> str | None:
|
||||
"""Raw localStorage payload for the chat (None when the key is absent)."""
|
||||
return page.evaluate(f"() => localStorage.getItem('{STORAGE_KEY}')")
|
||||
|
||||
|
||||
def _stored_parsed(page: Page) -> dict[str, Any]:
|
||||
raw = _stored(page)
|
||||
assert raw is not None, "the conversation key must exist in localStorage"
|
||||
return json.loads(raw)
|
||||
|
||||
|
||||
def _ask(page: Page, question: str) -> None:
|
||||
"""Send one turn and wait until the grounded answer has fully landed."""
|
||||
page.fill("#message-input", question)
|
||||
page.click("#send-btn")
|
||||
expect(page.locator(".msg.user .bubble").last).to_contain_text(question)
|
||||
expect(page.locator(".msg.brain .bubble").last).to_contain_text(
|
||||
MOCK_ANSWER_MARKER, timeout=30_000
|
||||
)
|
||||
expect(page.locator("#send-btn")).to_be_enabled()
|
||||
expect(page.locator("#send-label")).to_have_text("Send")
|
||||
|
||||
|
||||
def _no_error_banner(page: Page) -> None:
|
||||
"""The never-stale contract: a restored/partial state must never
|
||||
present an error banner (role=alert) — the turn is simply partial."""
|
||||
expect(page.locator('[role="alert"]')).to_have_count(0)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def seeded_kb(mock_llm: int, db_ready: None) -> Iterator[None]:
|
||||
"""A fresh KB seeded from ``tests/fixtures/docs`` (8 docs, A9 formats),
|
||||
truncated again on teardown. ``db_ready`` (conftest) skips with clear
|
||||
instructions when Postgres is down."""
|
||||
summary = _reset_db(mock_llm, seed=True)
|
||||
assert summary is not None and summary.added == 8
|
||||
yield
|
||||
_reset_db(mock_llm, seed=False)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 1. Mid-stream navigation via the Sources nav link: the partial answer
|
||||
# that had already streamed is persisted and restored
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_partial_answer_survives_sources_nav_midstream(
|
||||
page: Page, app_url: str, seeded_kb: None
|
||||
) -> None:
|
||||
page.set_default_timeout(30_000)
|
||||
# Admin (phase 16/19): only the admin sees the #nav-sources link the
|
||||
# bug report clicks.
|
||||
login(page, app_url, next="/")
|
||||
expect(page.locator("#nav-sources")).to_be_visible()
|
||||
|
||||
# Start the ~9s long answer.
|
||||
page.fill("#message-input", LONG_QUESTION)
|
||||
page.click("#send-btn")
|
||||
expect(page.locator(".msg.user .bubble").last).to_contain_text(LONG_QUESTION)
|
||||
|
||||
# Wait until the first streamed frames have rendered (the first line,
|
||||
# in its list-rendered form) — the turn is now provably mid-stream.
|
||||
bubble = page.locator(".msg.brain .bubble").last
|
||||
expect(bubble).to_contain_text(FIRST_LINE_DOM, timeout=30_000)
|
||||
# The turn is still in flight (the stream runs ~9s; navigation takes
|
||||
# well under that).
|
||||
expect(page.locator("#send-btn")).to_be_disabled()
|
||||
|
||||
# THE BUG REPORT, VERBATIM: click "Sources" while chat is generating.
|
||||
page.click("#nav-sources")
|
||||
expect(page).to_have_url(app_url + "/sources.html")
|
||||
# The navigation really landed on the admin catalog (mid-stream state
|
||||
# of the stream itself does not matter to the page — the fetch is
|
||||
# aborted by the unload, which is the point).
|
||||
expect(page.locator("#docs-tbody tr").first).to_be_visible(timeout=15_000)
|
||||
|
||||
# Return to the chat.
|
||||
page.goto(app_url + "/")
|
||||
|
||||
# The question AND the already-streamed partial answer are both
|
||||
# rendered — no empty state, no error banner.
|
||||
expect(page.locator("#empty-state")).to_be_hidden()
|
||||
expect(page.locator(".msg.user .bubble")).to_have_count(1)
|
||||
expect(page.locator(".msg.user .bubble").first).to_contain_text(LONG_QUESTION)
|
||||
restored = page.locator(".msg.brain .bubble")
|
||||
expect(restored).to_have_count(1)
|
||||
expect(restored.first).to_contain_text(FIRST_LINE_DOM)
|
||||
_no_error_banner(page)
|
||||
|
||||
# Storage holds the partial as a plain brain message: raw text starting
|
||||
# with the first streamed chunk — and SHORTER than the full answer
|
||||
# (navigation landed mid-stream), with no done metadata (no
|
||||
# sources/deflected/suggestions/thinking: this turn had none and a
|
||||
# partial never carries the done fields).
|
||||
msgs = _stored_parsed(page)["messages"]
|
||||
assert [m["who"] for m in msgs] == ["user", "brain"]
|
||||
assert msgs[0]["text"] == LONG_QUESTION
|
||||
brain = msgs[1]
|
||||
assert brain["text"].startswith(FIRST_CHUNK_RAW)
|
||||
assert len(brain["text"]) < len(FULL_LONG), "the stored answer must be partial"
|
||||
assert brain["text"] != FULL_LONG
|
||||
assert "sources" not in brain
|
||||
assert "deflected" not in brain
|
||||
assert "suggestions" not in brain
|
||||
assert "thinking" not in brain
|
||||
|
||||
# The partial renders like any brain message (the existing
|
||||
# bubble contract — no new surface) and is tunable like one.
|
||||
expect(page.locator("details.thinking")).to_have_count(0)
|
||||
expect(page.locator(".msg.brain .source-chip")).to_have_count(0)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 2. Navigation BEFORE the first answer token (pure thinking): nothing
|
||||
# brain-side is persisted — the question comes back alone
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_no_orphan_brain_message_when_navigated_before_first_token(
|
||||
page: Page, app_url: str, seeded_kb: None
|
||||
) -> None:
|
||||
page.set_default_timeout(30_000)
|
||||
page.goto(app_url)
|
||||
page.fill("#message-input", HESITATE_QUESTION)
|
||||
page.click("#send-btn")
|
||||
expect(page.locator(".msg.user .bubble").last).to_contain_text(HESITATE_QUESTION)
|
||||
|
||||
# The mock streamed the ~800-char scratchpad (phase-17 thinking
|
||||
# frames); wait until its tail is rendered — the 4s pre-content pause
|
||||
# (SLOW_PRETOKEN_TRIGGER) is now running, so the navigation below
|
||||
# lands inside pure thinking with a wide margin.
|
||||
thinking = page.locator(".msg.brain").last.locator("details.thinking")
|
||||
thinking.wait_for(state="attached", timeout=10_000)
|
||||
expect(thinking.locator(".thinking-text")).to_contain_text(THINKING_TAIL)
|
||||
# Still pre-token: the button is busy with the Thinking state.
|
||||
expect(page.locator("#send-btn")).to_be_disabled()
|
||||
expect(page.locator("#send-label")).to_have_text("Thinking…")
|
||||
|
||||
# Leave during the pause (no answer token has streamed — acc is empty,
|
||||
# so the pagehide save point must persist nothing brain-side).
|
||||
page.goto(app_url + "/sources.html")
|
||||
|
||||
# Return to the chat.
|
||||
page.goto(app_url + "/")
|
||||
|
||||
# The question is restored — with NO brain message behind it: no empty
|
||||
# bubble, no partial, no thinking block (owner-confirmed A1.2).
|
||||
expect(page.locator("#empty-state")).to_be_hidden()
|
||||
expect(page.locator(".msg.user .bubble")).to_have_count(1)
|
||||
expect(page.locator(".msg.user .bubble").first).to_contain_text(HESITATE_QUESTION)
|
||||
expect(page.locator(".msg")).to_have_count(1)
|
||||
expect(page.locator(".msg.brain")).to_have_count(0)
|
||||
expect(page.locator("details.thinking")).to_have_count(0)
|
||||
_no_error_banner(page)
|
||||
|
||||
# Storage agrees: exactly the user message, nothing brain-side.
|
||||
msgs = _stored_parsed(page)["messages"]
|
||||
assert len(msgs) == 1
|
||||
assert msgs[0] == {"who": "user", "text": HESITATE_QUESTION}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 3. Completed turn: the done save point is byte-identical to before —
|
||||
# the new pagehide save point must not duplicate or alter it
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_completed_turn_unaffected(
|
||||
page: Page, app_url: str, seeded_kb: None
|
||||
) -> None:
|
||||
page.set_default_timeout(30_000)
|
||||
page.goto(app_url)
|
||||
_ask(page, QUESTION)
|
||||
|
||||
# The done save point: full answer + metadata, exactly as phase 14.
|
||||
before = _stored_parsed(page)
|
||||
assert [m["who"] for m in before["messages"]] == ["user", "brain"]
|
||||
brain = before["messages"][1]
|
||||
assert MOCK_ANSWER_MARKER in brain["text"]
|
||||
assert brain["deflected"] is False
|
||||
assert any(s["path"] == "homelab/kubernetes.md" for s in brain["sources"])
|
||||
|
||||
# A trip to Sources and back (the turn finished long ago — uiState is
|
||||
# idle, so the pagehide save point must be a no-op).
|
||||
page.goto(app_url + "/sources.html")
|
||||
page.goto(app_url + "/")
|
||||
|
||||
# Full answer + source chip rendered; no error banner.
|
||||
expect(page.locator("#empty-state")).to_be_hidden()
|
||||
expect(page.locator(".msg.user .bubble")).to_contain_text(QUESTION)
|
||||
bubble = page.locator(".msg.brain .bubble")
|
||||
expect(bubble).to_have_count(1)
|
||||
expect(bubble.first).to_contain_text(MOCK_ANSWER_MARKER)
|
||||
chip = page.locator(".msg.brain .source-chip", has_text="kubernetes.md")
|
||||
expect(chip).to_have_count(1)
|
||||
expect(chip.first).to_have_attribute("href", CHIP_HREF)
|
||||
_no_error_banner(page)
|
||||
|
||||
# Storage is byte-identical to the pre-navigation payload — the
|
||||
# completed turn persisted exactly as before (one brain message, done
|
||||
# metadata intact; no duplicate from the pagehide path).
|
||||
assert _stored_parsed(page) == before
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 4. The DELIBERATE clear is untouched: New Chat from the sources page
|
||||
# still clears the conversation (phase 14/19 contract)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_new_chat_still_clears_conversation(
|
||||
page: Page, app_url: str, seeded_kb: None
|
||||
) -> None:
|
||||
page.set_default_timeout(30_000)
|
||||
page.goto(app_url)
|
||||
_ask(page, QUESTION)
|
||||
assert _stored(page) is not None
|
||||
|
||||
# To the sources page (anonymous is fine — the shared bar carries the
|
||||
# New Chat control regardless of auth, phase 19).
|
||||
page.goto(app_url + "/sources.html")
|
||||
new_chat = page.locator("#new-chat-btn")
|
||||
expect(new_chat).to_be_visible()
|
||||
new_chat.click()
|
||||
|
||||
# "New chat" on a non-chat page means "go to the chat, fresh": the
|
||||
# land page shows the empty state and the conversation key is GONE —
|
||||
# the deliberate clearChatStorage() is unaffected by the phase-20
|
||||
# pagehide save point.
|
||||
expect(page).to_have_url(app_url + "/")
|
||||
expect(page.locator("#empty-state")).to_be_visible()
|
||||
expect(page.locator(".msg")).to_have_count(0)
|
||||
assert _stored(page) is None, "New Chat must clear the localStorage key"
|
||||
_no_error_banner(page)
|
||||
Reference in New Issue
Block a user