phase: 103_suggestions_session_openers
Phase 103 final verification pass — all green.
**Verified (all 3 tasks already in `complete/`; no code changes needed):**
- `opening_questions` in `app/api/suggestions.py` — forward walk, one opener per chat (first non-blank user msg, A3), reads raw `messages` not `title` (A4), phase-80 order/dedup/cap/seed contracts; `last_questions` name gone from `app/`+`tests/`
- Docs updated: `app/config.py` seed docstring, `.env.example` `BOR_SUGGESTIONS`, `README.md` — "session openers" wording
- Diff scope correct: only the 6 expected files + phase-file moves; `app/rag/suggestions.py` and `frontend/` untouched
**Test / lint / coverage results:**
- `uv run pytest tests/integration/test_suggestions_api.py -v` → 12 passed
- `uv run pytest tests/e2e/test_suggestion_chips.py -v --no-cov` → 8 passed in isolation (opener-only core pin included)
- `test_responsive_polish.py` → 7 passed; `test_chat_persistence.py` → 4 passed (both isolated, no edits)
- `uv run pytest --cov=app --cov-report=term-missing` → 2086 passed, TOTAL 99% (>90%); `app/api/suggestions.py` 100%
- `uv run ruff check .` → clean; `uv run pyright` → 0 errors, 0 warnings
**Completion criteria:** all 7 ✅ (follow-ups-never-surface pin; cap-across-chats pin; seed/dedup/case/partial/A3/401 pins; E2E suites isolated; deflection chips unchanged; full suite + lint; commit + dir move left to harness per executor rules).
**Deviations:** none — no defects found; nothing changed in this pass.
**Next pending phase:** `98_sync_summary_visibility` (numeric order in `todo/`).
This commit is contained in:
@@ -1,42 +1,56 @@
|
||||
"""Phase 80 E2E (Playwright): onboarding chips = the last 3 questions asked.
|
||||
"""Phase 103 E2E (Playwright): onboarding chips = the session openers.
|
||||
|
||||
Story: ``.agents/user_stories/suggestion-chips.md`` (phase 05) —
|
||||
REWRITTEN in place for the phase-80 semantics (the phase-76 precedent:
|
||||
a semantic change rewrites the story suite in place). Source:
|
||||
``TODO.md`` L6.
|
||||
REWRITTEN in place for the phase-103 semantics (the phase-76/80
|
||||
precedent: a semantic change rewrites the story suite in place).
|
||||
Source: owner request 2026-09-12 — a suggested question must make
|
||||
sense on its own, and a follow-up never does.
|
||||
|
||||
The new contract (owner decision A6): the empty-state chip row is the
|
||||
3 most recent user questions across ALL saved chats — chats walked
|
||||
newest-``updated_at`` first, each chat's messages newest-first,
|
||||
exact (case-sensitive) de-duplicated, cap 3. A fresh deployment — zero
|
||||
saved questions — gets the SEED list instead (``BOR_SUGGESTIONS`` /
|
||||
the built-in default). 1–2 saved questions → exactly those chips (NO
|
||||
mixing with the seed). The row refetches when the empty state comes
|
||||
back (New chat), so it is never stale. The deflection "Maybe try"
|
||||
chips are a separate contract (``derive_suggestions``) — untouched.
|
||||
The new contract (owner decision A1): the chips are the **session
|
||||
openers** — each saved chat contributes AT MOST ONE chip: its FIRST
|
||||
non-blank user message, the question that OPENED the session. Follow-up
|
||||
questions can NEVER surface: a follow-up like "What about qwen 3.6
|
||||
35b?" (asked after "What are the correct arguments for qwen 3.8 27b on
|
||||
llama.cpp?") is meaningless as a conversation starter without the
|
||||
session behind it. Everything else is the phase-80 contract, unchanged:
|
||||
chats are walked newest-``updated_at`` first, openers are exact
|
||||
(case-sensitive) de-duplicated, cap 3 — the cap binds ACROSS chats. A
|
||||
fresh deployment — zero saved openers — gets the SEED list instead
|
||||
(``BOR_SUGGESTIONS`` / the built-in default). The row refetches when
|
||||
the empty state comes back (New chat), so it is never stale. The
|
||||
deflection "Maybe try" chips are a separate contract
|
||||
(``derive_suggestions``) — untouched.
|
||||
|
||||
The four states pinned here:
|
||||
The states pinned here:
|
||||
|
||||
* **seed** — fresh DB (no saved chats) → the chip texts equal the
|
||||
built-in default list EXACTLY (the ``SEED`` literal below is the
|
||||
pin for the exact seed list — ``tests/unit/test_config.py`` pins
|
||||
only the shape) — rendered as accessible buttons in the role=list
|
||||
group, exactly as the phase-05 component contract;
|
||||
* **last-3** — two saved chats with 5 user questions total (the older
|
||||
one saved FIRST — the API stamps ``updated_at``) → a fresh page
|
||||
load shows EXACTLY the 3 newest questions, newest-first;
|
||||
* **partial** — exactly 2 saved questions deployment-wide → exactly
|
||||
2 chips (no seed top-up — the A6 contract, visible in the UI);
|
||||
* **refetch** — boot with the seed chips, save a chat whose newest
|
||||
question is Q via the API, click New chat (``#new-chat-btn``) → the
|
||||
chips now are Q, and the request log shows a SECOND
|
||||
``GET /api/suggestions`` (the boot fetch was the first).
|
||||
* **seed** (unchanged) — fresh DB (no saved chats) → the chip texts
|
||||
equal the built-in default list EXACTLY (the ``SEED`` literal below
|
||||
is the pin for the exact seed list — ``tests/unit/test_config.py``
|
||||
pins only the shape) — rendered as accessible buttons in the
|
||||
role=list group, exactly as the phase-05 component contract;
|
||||
* **opener-only** (the NEW core state — the owner's exact scenario) —
|
||||
ONE saved chat with a 3-turn conversation (the opener Q1, the
|
||||
follow-up Q2, the follow-up Q3, brain replies between) → a fresh
|
||||
page load shows EXACTLY ONE chip: Q1 (the opener); Q2/Q3 are absent;
|
||||
* **three-openers** (replaces the old "last-3" state) — THREE saved
|
||||
chats, each multi-turn (opener + at least one follow-up), DISTINCT
|
||||
``updated_at`` (the API stamps them on save — the test saves
|
||||
oldest→newest) → exactly 3 chips = the three openers, newest
|
||||
``updated_at`` first; none of the chats' FOLLOW-UPS appears;
|
||||
* **partial** (kept, re-scoped) — exactly 2 saved (multi-turn) chats →
|
||||
exactly 2 chips (the two openers — NO seed top-up; the follow-ups in
|
||||
those chats do not pad the row);
|
||||
* **refetch** (kept) — boot with the seed chips → save a multi-turn
|
||||
chat (opener Q + a follow-up) via the API → click New chat
|
||||
(``#new-chat-btn``) → the chips now are exactly Q, and the request
|
||||
log shows a SECOND ``GET /api/suggestions`` (the boot fetch was the
|
||||
first).
|
||||
|
||||
Carried-over story behavior (unchanged semantics from the phase-05
|
||||
suite): one-tap submit (chip click → composer filled → submitted →
|
||||
the mock-LLM brain bubble), Tab+Enter keyboard reachability of the
|
||||
chips (the keyboard-walk assertion), and the mobile single
|
||||
horizontal-scroll row.
|
||||
Carried-over story behavior (unchanged semantics from the phase-05/80
|
||||
suites): one-tap submit (chip click → composer filled → submitted →
|
||||
the mock-LLM brain bubble with the ``MOCK_ANSWER_MARKER``), Tab+Enter
|
||||
keyboard reachability of the chips (the keyboard-walk assertion), and
|
||||
the mobile single horizontal-scroll row.
|
||||
|
||||
The endpoint is authed (phase 79, ``require_user``), so every test
|
||||
signs in as admin first (``auth_helpers.login``). ``saved_chats`` is
|
||||
@@ -55,7 +69,6 @@ import asyncio
|
||||
import json
|
||||
import time
|
||||
from collections.abc import Iterator
|
||||
from datetime import datetime
|
||||
from pathlib import Path
|
||||
from threading import Thread
|
||||
from typing import Any
|
||||
@@ -75,12 +88,12 @@ FIXTURES = REPO / "tests" / "fixtures" / "docs"
|
||||
MOCK_ANSWER_MARKER = "Deterministic mock answer for E2E"
|
||||
|
||||
#: The EXACT built-in onboarding SEED (phase 80, TODO.md L6): the chip
|
||||
#: row of a brand-new deployment, shown only before any question has
|
||||
#: ever been saved. This literal is the E2E pin for the exact seed
|
||||
#: list — ``tests/unit/test_config.py`` pins only the SHAPE (>=3
|
||||
#: non-blank distinct strings), and the e2e app under test is forced
|
||||
#: to the code default by conftest's leak guard — keep in sync with
|
||||
#: the ``Settings.suggestions`` default in ``app/config.py``.
|
||||
#: row of a brand-new deployment, shown only while no saved chat has
|
||||
#: ever opened with a question. This literal is the E2E pin for the
|
||||
#: exact seed list — ``tests/unit/test_config.py`` pins only the SHAPE
|
||||
#: (>=3 non-blank distinct strings), and the e2e app under test is
|
||||
#: forced to the code default by conftest's leak guard — keep in sync
|
||||
#: with the ``Settings.suggestions`` default in ``app/config.py``.
|
||||
SEED: list[str] = [
|
||||
"What documents are in the knowledge base?",
|
||||
"Which source does each answer come from?",
|
||||
@@ -91,10 +104,10 @@ SEED: list[str] = [
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def clean_chats(db_ready: None) -> Iterator[None]:
|
||||
"""``saved_chats`` is the state the phase-80 contract reads:
|
||||
"""``saved_chats`` is the state the phase-103 contract reads:
|
||||
truncate it before and after every test so each state test starts
|
||||
from (and leaves) an empty deployment. Unlike the KB tables, this
|
||||
reset is non-optional — the chips ARE these rows, and the
|
||||
reset is non-optional — the chips ARE these rows' openers, and the
|
||||
carried-over submit tests auto-save a row per turn, which would
|
||||
otherwise leak into the later state tests."""
|
||||
with SessionLocal() as db:
|
||||
@@ -223,92 +236,142 @@ def test_seed_state_chips_are_the_builtin_default(
|
||||
assert box is not None and box["height"] >= 44
|
||||
|
||||
|
||||
def test_last_three_questions_state(page: Page, app_url: str, db_ready: None) -> None:
|
||||
"""5 user questions across two saved chats (the older one saved
|
||||
FIRST — the API stamps ``updated_at`` at INSERT) → a fresh page
|
||||
load shows EXACTLY the 3 newest questions, newest-first: the
|
||||
newer chat is walked first, then the older chat newest-first."""
|
||||
def test_opener_only_state(page: Page, app_url: str, db_ready: None) -> None:
|
||||
"""The NEW core state — the owner's exact scenario: ONE saved chat
|
||||
with a 3-turn conversation (opener Q1, follow-up Q2, follow-up Q3)
|
||||
→ a fresh page load shows EXACTLY ONE chip: Q1 (the opener). Q2/Q3
|
||||
are absent — a follow-up like "What about …?" is meaningless as a
|
||||
conversation starter without the session behind it."""
|
||||
page.set_default_timeout(30_000)
|
||||
login(page, app_url, next="/")
|
||||
|
||||
old_q = [
|
||||
"How did I install the GitLab runner on the Proxmox node?",
|
||||
"Which disk holds the Borg backup archives?",
|
||||
"How is the nftables firewall rule set ordered?",
|
||||
]
|
||||
new_q = [
|
||||
"What TLS termination does Traefik do for homelab.local?",
|
||||
"Which provider is the primary DNS for reeseapps.com?",
|
||||
]
|
||||
opener = "What are the correct arguments for qwen 3.8 27b on llama.cpp?"
|
||||
follow_up_1 = "What about qwen 3.6 35b?"
|
||||
follow_up_2 = "And which of the three needs the most VRAM?"
|
||||
|
||||
# The OLDER chat first: the API stamps ``updated_at`` (server
|
||||
# now()), so save order IS walk order. The short pause keeps the
|
||||
# two stamps strictly apart (and the assert below pins that the
|
||||
# order the walk sees is the order the test intended).
|
||||
older = _save_chat(
|
||||
# ONE multi-turn chat (three user questions, brain replies between)
|
||||
# — exactly the owner's session shape.
|
||||
_save_chat(
|
||||
page,
|
||||
app_url,
|
||||
[
|
||||
_user(old_q[0]), _brain(),
|
||||
_user(old_q[1]), _brain(),
|
||||
_user(old_q[2]), _brain(),
|
||||
_user(opener), _brain(),
|
||||
_user(follow_up_1), _brain(),
|
||||
_user(follow_up_2), _brain(),
|
||||
],
|
||||
)
|
||||
time.sleep(0.05)
|
||||
newer = _save_chat(
|
||||
page,
|
||||
app_url,
|
||||
[
|
||||
_user(new_q[0]), _brain(),
|
||||
_user(new_q[1]), _brain(),
|
||||
],
|
||||
)
|
||||
assert datetime.fromisoformat(newer["updated_at"]) > datetime.fromisoformat(
|
||||
older["updated_at"]
|
||||
), "the two API-stamped updated_at values must be strictly ordered"
|
||||
|
||||
# A FRESH page load (a new boot fetch, not the pre-save boot):
|
||||
# the chips are exactly the 3 newest questions, newest first.
|
||||
# EXACTLY ONE chip — the chat's opener, and nothing else.
|
||||
page.goto(app_url + "/")
|
||||
chips = _chip_locator(page)
|
||||
expect(chips.first).to_be_visible(timeout=30_000)
|
||||
expected = [new_q[1], new_q[0], old_q[2]]
|
||||
assert _chip_texts(page) == expected
|
||||
assert _api_suggestions(page, app_url) == expected
|
||||
# The two older questions (and everything seed-shaped) are gone.
|
||||
assert old_q[0] not in _chip_texts(page)
|
||||
assert old_q[1] not in _chip_texts(page)
|
||||
assert chips.count() == 1, "a 3-turn chat yields EXACTLY its opener as the single chip"
|
||||
texts = _chip_texts(page)
|
||||
assert texts == [opener], "the single chip is the EXACT full opener text"
|
||||
assert follow_up_1 not in texts, "the 'What about …?' follow-up must never chip"
|
||||
assert follow_up_2 not in texts
|
||||
# ...and the endpoint itself holds the same contract (same row).
|
||||
assert _api_suggestions(page, app_url) == [opener]
|
||||
|
||||
|
||||
def test_partial_state_no_seed_topup(page: Page, app_url: str, db_ready: None) -> None:
|
||||
"""Exactly 2 saved questions deployment-wide → EXACTLY 2 chips
|
||||
(newest first) — NO mixing/top-up with the seed (the A6
|
||||
contract, visible in the UI)."""
|
||||
def test_three_openers_newest_first(page: Page, app_url: str, db_ready: None) -> None:
|
||||
"""THREE saved chats, each multi-turn (opener + at least one
|
||||
follow-up), DISTINCT ``updated_at`` (the API stamps them on save —
|
||||
the test saves oldest→newest) → a fresh page load shows EXACTLY
|
||||
the three openers, newest ``updated_at`` first; none of the chats'
|
||||
FOLLOW-UPS appears anywhere in the row."""
|
||||
page.set_default_timeout(30_000)
|
||||
login(page, app_url, next="/")
|
||||
|
||||
a = "How do I rotate the WireGuard keys on the VPN node?"
|
||||
b = "What cron schedule runs the restic prune?"
|
||||
_save_chat(page, app_url, [_user(a), _brain()])
|
||||
openers = [
|
||||
"How did I install the GitLab runner on the Proxmox node?",
|
||||
"What TLS termination does Traefik do for homelab.local?",
|
||||
"Which provider is the primary DNS for reeseapps.com?",
|
||||
]
|
||||
follow_ups = [
|
||||
"What about the runners' Docker socket access?",
|
||||
"And does it terminate mTLS for the internal services?",
|
||||
"What about the secondary DNS for the LAN?",
|
||||
]
|
||||
|
||||
# Save oldest→newest: the API stamps ``updated_at`` (server
|
||||
# now()), so save order IS walk order. The short pauses keep the
|
||||
# three stamps strictly apart (and the assert below pins that the
|
||||
# order the walk sees is the order the test intended).
|
||||
stamps: list[str] = []
|
||||
for opener, follow_up in zip(openers, follow_ups, strict=True):
|
||||
body = _save_chat(
|
||||
page,
|
||||
app_url,
|
||||
[
|
||||
_user(opener), _brain(),
|
||||
_user(follow_up), _brain(),
|
||||
],
|
||||
)
|
||||
stamps.append(body["updated_at"])
|
||||
time.sleep(0.05)
|
||||
assert stamps == sorted(stamps) and len(set(stamps)) == 3, (
|
||||
"the three API-stamped updated_at values must be strictly increasing"
|
||||
)
|
||||
|
||||
# A FRESH page load (a new boot fetch, not the pre-save boot):
|
||||
# exactly the three openers, newest first — the cap of 3 binds
|
||||
# ACROSS chats, and every chip is a session's OPENER.
|
||||
page.goto(app_url + "/")
|
||||
chips = _chip_locator(page)
|
||||
expect(chips.first).to_be_visible(timeout=30_000)
|
||||
expected = list(reversed(openers))
|
||||
texts = _chip_texts(page)
|
||||
assert chips.count() == 3
|
||||
assert texts == expected
|
||||
assert _api_suggestions(page, app_url) == expected
|
||||
for follow_up in follow_ups:
|
||||
assert follow_up not in texts, "a chat's follow-up must never chip"
|
||||
|
||||
|
||||
def test_partial_state_no_seed_topup(page: Page, app_url: str, db_ready: None) -> None:
|
||||
"""Exactly 2 saved (multi-turn) chats → EXACTLY 2 chips (the two
|
||||
openers, newest first) — NO mixing/top-up with the seed (the
|
||||
phase-80 A6 contract, visible in the UI), and the follow-ups in
|
||||
those chats do not pad the row."""
|
||||
page.set_default_timeout(30_000)
|
||||
login(page, app_url, next="/")
|
||||
|
||||
a_opener = "How do I rotate the WireGuard keys on the VPN node?"
|
||||
a_follow_up = "What about the peers' allowed-ips?"
|
||||
b_opener = "What cron schedule runs the restic prune?"
|
||||
b_follow_up = "And where do the restic lock files live?"
|
||||
_save_chat(
|
||||
page,
|
||||
app_url,
|
||||
[_user(a_opener), _brain(), _user(a_follow_up), _brain()],
|
||||
)
|
||||
time.sleep(0.05)
|
||||
_save_chat(page, app_url, [_user(b), _brain()])
|
||||
_save_chat(
|
||||
page,
|
||||
app_url,
|
||||
[_user(b_opener), _brain(), _user(b_follow_up), _brain()],
|
||||
)
|
||||
|
||||
page.goto(app_url + "/")
|
||||
chips = _chip_locator(page)
|
||||
expect(chips.first).to_be_visible(timeout=30_000)
|
||||
assert chips.count() == 2, "exactly 2 chips — the row is never padded toward 3"
|
||||
texts = _chip_texts(page)
|
||||
assert texts == [b, a]
|
||||
assert texts == [b_opener, a_opener]
|
||||
assert a_follow_up not in texts and b_follow_up not in texts
|
||||
assert not (set(texts) & set(SEED)), "no seed text may appear once a question is saved"
|
||||
|
||||
|
||||
def test_new_chat_refetches_the_chips(page: Page, app_url: str, db_ready: None) -> None:
|
||||
"""The row is never stale: boot with the seed chips → save a chat
|
||||
whose newest question is Q via the API → click New chat
|
||||
(``#new-chat-btn``) → the empty state comes back with the
|
||||
"""The row is never stale: boot with the seed chips → save a
|
||||
multi-turn chat (opener Q + a follow-up) via the API → click New
|
||||
chat (``#new-chat-btn``) → the empty state comes back with the
|
||||
REFETCHED row (exactly Q — the deployment now has one saved
|
||||
question), and the request log shows a SECOND
|
||||
``GET /api/suggestions`` (the boot fetch was the first)."""
|
||||
OPENER; the chat's follow-up is never a chip), and the request log
|
||||
shows a SECOND ``GET /api/suggestions`` (the boot fetch was the
|
||||
first)."""
|
||||
page.set_default_timeout(30_000)
|
||||
sugg_gets: list[float] = []
|
||||
|
||||
@@ -325,13 +388,21 @@ def test_new_chat_refetches_the_chips(page: Page, app_url: str, db_ready: None)
|
||||
assert len(sugg_gets) == 1, "exactly one GET /api/suggestions at boot"
|
||||
|
||||
q = "Which service fronts the Pi-hole DNS on the network?"
|
||||
_save_chat(page, app_url, [_user(q), _brain()])
|
||||
_save_chat(
|
||||
page,
|
||||
app_url,
|
||||
[
|
||||
_user(q), _brain(),
|
||||
_user("What about the Pi-hole's DNSSEC settings?"), _brain(),
|
||||
],
|
||||
)
|
||||
|
||||
clicked_at = time.monotonic()
|
||||
page.click("#new-chat-btn")
|
||||
|
||||
# The refetch re-renders #suggestions in place: the 4 seed chips
|
||||
# are replaced by exactly Q (the partial state, live).
|
||||
# are replaced by exactly Q (the partial state, live — the chat's
|
||||
# follow-up does not pad the row).
|
||||
expect(chips).to_have_count(1, timeout=15_000)
|
||||
expect(chips.first).to_have_text(q, timeout=15_000)
|
||||
assert len(sugg_gets) == 2, "New chat triggered the refetch"
|
||||
|
||||
Reference in New Issue
Block a user