From 6cf1df9bf2c850d2399f459e42059887c78b3ef0 Mon Sep 17 00:00:00 2001 From: ducoterra Date: Thu, 27 Aug 2026 23:44:35 -0400 Subject: [PATCH] feat(sync): fail fast with a modal when a model is unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TODO.md L4: with a dead model endpoint the sync discovered it only mid-import, after slow clones — and a tooltip on the button is not a readable error. - app/rag/llm.py: ModelUnavailableError + check_models(llm) — a tiny pre-sync probe (one short embedding + one 1-token-scale completion) that fails naming the unavailable model (embed first, then the summary model); the sync sanitizer still masks credentials. - app/api/sync.py: the probe is step 1 of _run_sync — before source resolution and before any clone_or_pull; a model failure is just another 'failed' state (no new endpoint, A10/A12 untouched). - frontend/assets/header.js: applySyncFailure now also opens the module-owned error modal (every page carrying #sync-btn, zero page-markup changes): lazily built backdrop + role=alertdialog panel, error text via textContent, close via button / Esc / backdrop, focus in-and-out to #sync-btn (with a body→#sync-btn fallback — the run's disabled button drops focus to ). - frontend/assets/styles.css: the modal on the phase-08 error palette (z-index above the header, .is-open open/close, reduced-motion stilling, 44px close target). - Tests: probe unit tests (both up / embed down / summary down / custom model names), sync integration (fail-fast before any clone, probe-before-effective_sources ordering, credential masking, healthy regression), the phase-41 source pins, and the story E2E (two module apps on distinct ports — dead endpoint on a closed loopback port vs session mock: ≤10 s fail-fast + modal contract, all three dismissal paths with focus out to #sync-btn, button title/.is-error + Sources banner untouched, healthy phase-32 lifecycle regression to 'Synced HH:MM'). E2E (isolation): test_sync_model_down.py 4/4, test_sync_button.py 3/3, test_git_sources_admin.py 6/6, test_local_directory_sources.py 3/3; unit+integration 721 passed, app/ coverage 99%; ruff + pyright clean. --- app/api/sync.py | 20 +- app/rag/llm.py | 39 +++ frontend/assets/header.js | 96 +++++- frontend/assets/styles.css | 78 +++++ tests/e2e/test_sync_model_down.py | 447 ++++++++++++++++++++++++++++ tests/fakes.py | 6 + tests/integration/test_sync_api.py | 174 ++++++++++- tests/unit/test_sync_button.py | 156 ++++++++++ tests/unit/test_sync_model_probe.py | 199 +++++++++++++ 9 files changed, 1203 insertions(+), 12 deletions(-) create mode 100644 tests/e2e/test_sync_model_down.py create mode 100644 tests/unit/test_sync_model_probe.py diff --git a/app/api/sync.py b/app/api/sync.py index 4b6ef9e..73fb5ec 100644 --- a/app/api/sync.py +++ b/app/api/sync.py @@ -15,25 +15,29 @@ stale button state (§7.4 adaptation, phase locked decisions). Pipeline (the canonical "mirror the sources" action — phase locked decisions): -1. resolve the effective sources — the ``git_sources`` DB rows (git +1. verify ``embed`` + summary model availability — fail fast before + any clone (:func:`app.rag.llm.check_models`, phase 41): a dead + model endpoint aborts the run naming the unavailable model, before + source resolution or any ``clone_or_pull``; +2. resolve the effective sources — the ``git_sources`` DB rows (git **and** local, phase 38), else the ``BOR_GIT_SOURCES`` fallback (git-only) (:func:`app.rag.git_sources.effective_sources`, shared with the CLI) — empty on both origins (no git rows, no local rows, no env URLs) fails loudly (``no sources configured (git or local)``) instead of silently importing the legacy local directories; -2. per resolved row: ``kind=git`` → :func:`scripts.git_sync.clone_or_pull` +3. per resolved row: ``kind=git`` → :func:`scripts.git_sync.clone_or_pull` into ``BOR_SOURCES_DIR//`` (phase 28 — reused, not re-implemented); ``kind=local`` → the stored directory, re-verified ``.is_dir()`` **at sync time** (it may have moved/deleted since add-time) — a missing directory raises ``local source missing: ``; a failing clone or a missing local dir aborts before any import; -3. ``import_sources(..., prune=True)`` over the single combined list +4. ``import_sources(..., prune=True)`` over the single combined list (git checkouts + local dirs) — prune so files deleted upstream or out of a local dir leave the index (pruning covers the union; the CLI's no-prune default is unchanged); -4. when the import changed the KB (added + updated > 0), +5. when the import changed the KB (added + updated > 0), ``regenerate_overview`` refreshes the single ``kb_overview`` row (phase 31 trigger, best-effort inside). @@ -57,7 +61,7 @@ from app.core.auth import require_admin from app.db import SessionLocal from app.rag.git_sources import effective_sources from app.rag.importer import ImportSummary, import_sources -from app.rag.llm import LLMClient +from app.rag.llm import LLMClient, check_models from app.rag.overview import regenerate_overview from scripts.git_sync import GitSyncError, clone_or_pull from scripts.import_docs import repo_name @@ -153,6 +157,11 @@ async def _run_sync() -> None: _status.error = None try: settings = get_settings() + # Step 1 (phase 41): fail fast — verify both models the sync + # needs (embed + summary) before source resolution or any + # clone. The client is reused for the import + overview below. + llm = LLMClient() + await check_models(llm) # The background task has no request session: open a short-lived # one around the shared phase-35/38 resolver (DB rows of both # kinds win; the BOR_GIT_SOURCES git list is a fallback while @@ -189,7 +198,6 @@ async def _run_sync() -> None: if not path.is_dir(): raise GitSyncError(f"local source missing: {path}") sources.append(path) - llm = LLMClient() summary: ImportSummary = await import_sources(sources, llm, prune=True) overview = False if summary.added + summary.updated > 0: diff --git a/app/rag/llm.py b/app/rag/llm.py index 2718ac6..dd112c1 100644 --- a/app/rag/llm.py +++ b/app/rag/llm.py @@ -44,6 +44,15 @@ class LLMError(RuntimeError): """The chat-completions endpoint failed (network, HTTP, or mid-stream).""" +class ModelUnavailableError(LLMError): + """One of the models a sync needs is unreachable (phase 41 probe). + + Raised by :func:`check_models` when the pre-sync probe finds the + embedding or summary model down; the message names the model so the + admin can fix the right thing. + """ + + @dataclass(frozen=True) class StreamPiece: """One piece of a streamed chat turn (phase 17, PLAN §4 extension). @@ -408,3 +417,33 @@ class LLMClient: raise except Exception as e: # noqa: BLE001 — wrap transport-level failures raise LLMError(f"chat stream from {self.settings.llm_base_url} failed: {e}") from e + + +async def check_models(llm: LLMClient) -> None: + """Verify the models a sync needs (embed + summary) before any + expensive work; raise ModelUnavailableError naming the model. + + The probe is deliberately tiny — one short embedding + (``sync model check``) and one 1-token-scale completion (``ping``) + — so a dead endpoint is discovered cheaper than a single git + clone. The sync sanitizer downstream (``app.api.sync + ._sanitize_error``) still masks any credentials embedded in the + wrapped error text, so the raw endpoint URL in the original + exception is safe to include. + """ + embed_model = llm.settings.llm_embed_model + try: + await llm.embed_one("sync model check") + except Exception as e: # noqa: BLE001 — wrap EmbeddingError + transport failures + raise ModelUnavailableError( + f"The embedding model ('{embed_model}') is not available — " + f"check the model endpoint and retry. ({e})" + ) from e + summary_model = llm.settings.llm_summary_model + try: + await llm.chat([{"role": "user", "content": "ping"}]) + except Exception as e: # noqa: BLE001 — wrap LLMError + transport failures + raise ModelUnavailableError( + f"The summary model ('{summary_model}') is not available — " + f"check the model endpoint and retry. ({e})" + ) from e diff --git a/frontend/assets/header.js b/frontend/assets/header.js index 52381bd..d39d6b4 100644 --- a/frontend/assets/header.js +++ b/frontend/assets/header.js @@ -44,8 +44,14 @@ * timeout — the server state is authoritative). Every state change * dispatches window "bor:sync-status" (detail = the status object) * so the Sources page renders its #sync-result line + - * #sync-error-banner off the event; on non-Sources pages the - * failed state is visible in the button's title + aria-label; + * #sync-error-banner off the event; the button title/aria are the + * secondary failure surfaces — and a failed run ALSO opens the + * module-owned error modal (phase 41, 2026-08-27, TODO.md L4, the + * primary readable failure surface): lazily built by this module, + * appended to , error text textContent-rendered, closable via + * its button / Esc / backdrop, focus in-and-out to #sync-btn — + * every page carrying #sync-btn gets it with zero page-markup + * changes; * • the SINGLE New chat binding (phase 34 task 02 — it was * duplicated across app.js / sources.js / tuning.js / document.js): * on the chat page (#messages exists) the module dispatches @@ -336,8 +342,10 @@ if (newChatBtn) { * at a time, one poll loop at a time); * success → "Synced HH:MM"; failed → retry-ready "Sync sources" * + the sanitized error in the button's title + - * aria-label (on non-Sources pages that IS where the - * failure is visible; the Sources banner is the event). + * aria-label + the module-owned error modal (phase 41, + * 2026-08-27, TODO.md L4 — the primary readable failure + * surface on every page; the button affordance and the + * Sources banner stay the secondary surfaces). * * Boot (admin only — non-admins never poll, the status endpoint is * admin-only): one GET /api/sync/status on the SAME cached whoami — @@ -460,6 +468,86 @@ function applySyncFailure(status) { syncBtn.classList.add("is-error"); } emitSyncStatus(status); + // Phase 41 (2026-08-27, TODO.md L4): the module-owned error modal — + // the primary readable failure surface (the button title/aria and the + // Sources banner above stay as the secondary surfaces). + showSyncModal(error); +} + +/* ---------- sync failure modal (phase 41, 2026-08-27, TODO.md L4) ---------- + * + * A tooltip on the button is not a readable error — a failed sync ALSO + * opens a modal dialog. It is built by THIS module (the owner of the + * sync state machine), so every page carrying #sync-btn gets it with + * zero page-markup changes: created lazily ONCE (module-level + * `syncModal`) and appended to — a .sync-modal-backdrop (fixed, + * full-viewport dim) holding the .sync-modal panel + * (role="alertdialog", aria-modal, labelled + described). The error + * text is ALWAYS set via textContent (XSS-safe — never innerHTML with + * user data); a second failure while open updates the text IN PLACE + * (no stacking). Closes via the close button, Esc (ONE document + * keydown binding, acting only while open), or a click on the backdrop + * itself (never the panel); focus moves to the close button on open + * and back to the remembered element (#sync-btn — the control that + * started the run) on close. Null-safe: no #sync-btn (or no ) → + * no modal, exactly like the rest of this module. + */ +let syncModal = null; // the backdrop element — created once, lazily +let syncModalReturnFocus = null; // the element to refocus on close + +function createSyncModal() { + const backdrop = document.createElement("div"); + backdrop.className = "sync-modal-backdrop"; + // Static skeleton — no user data anywhere in it; the error text is + // filled via textContent in showSyncModal, never interpolated here. + backdrop.innerHTML = + '
' + + '

Sync failed

' + + '

' + + '' + + "
"; + document.body.appendChild(backdrop); + // Close path 1: the close button (×). + backdrop.querySelector(".sync-modal-close").addEventListener("click", closeSyncModal); + // Close path 2: a click on the backdrop element itself — never one + // that bubbles up from the panel (event.target check). + backdrop.addEventListener("click", (e) => { + if (e.target === backdrop) closeSyncModal(); + }); + // Close path 3: Esc — ONE document-level keydown binding for the + // life of the page, acting only while the modal is open. + document.addEventListener("keydown", (e) => { + if (e.key === "Escape" && backdrop.classList.contains("is-open")) closeSyncModal(); + }); + return backdrop; +} + +function showSyncModal(error) { + if (!syncBtn || !document.body) return; // null-safe: pages without the button + if (!syncModal) syncModal = createSyncModal(); + // The sanitized error as TEXT (XSS-safe) — a second failure while + // open updates the text in place (no stacking, no focus jump). + syncModal.querySelector("#sync-modal-error").textContent = error; + if (syncModal.classList.contains("is-open")) return; + // First open: remember the focused element and move focus into the + // dialog (the close button). While the run was in flight the button + // was disabled (focus had fallen to ), so a body-level active + // element means "no meaningful focus target" — remember #sync-btn, + // the control that started the run, so the close returns focus there. + const active = document.activeElement; + syncModalReturnFocus = active && active !== document.body ? active : syncBtn; + syncModal.classList.add("is-open"); + syncModal.querySelector(".sync-modal-close").focus(); +} + +function closeSyncModal() { + if (!syncModal || !syncModal.classList.contains("is-open")) return; + syncModal.classList.remove("is-open"); + // Focus returns to the remembered element — #sync-btn when present. + const target = syncModalReturnFocus; + syncModalReturnFocus = null; + if (target && document.contains(target)) target.focus(); } /* A run can only vanish with a server restart mid-sync (status resets diff --git a/frontend/assets/styles.css b/frontend/assets/styles.css index 4a0616b..8939f32 100644 --- a/frontend/assets/styles.css +++ b/frontend/assets/styles.css @@ -381,6 +381,84 @@ html::after { white-space: nowrap; } +/* Phase 41 (2026-08-27, TODO.md L4): the module-owned sync failure + modal — lazily built by header.js and appended to , so every + page carrying #sync-btn gets it with zero page-markup changes. + Phase-08 error palette (PLAN §7.2): the panel sits on the error + surface --err-bg with a 1px --err-line border (the amber + --accent-line stays deflection-only), its title in --ink (≈14.5:1 + on --err-bg) and the error text in --err-ink (9.1:1 on --err-bg). + The backdrop dims the page with --bg at 82% — no blur (the phase-08 + no-blur anchor). Stacking: z-index 1000, the same overlay contract + as the doc-modal — above the sticky header (20) and the skip-link + (100). Open/close via .is-open (visibility/opacity — the closed + modal is unfocusable and inert); the close button keeps the global + 3px :focus-visible outline and the 44px touch floor. */ +.sync-modal-backdrop { + position: fixed; + inset: 0; + z-index: 1000; + display: flex; + align-items: center; + justify-content: center; + padding: 1rem; + background: rgba(10, 14, 23, 0.82); + visibility: hidden; + opacity: 0; + transition: opacity 120ms ease; +} +.sync-modal-backdrop.is-open { + visibility: visible; + opacity: 1; +} +.sync-modal { + position: relative; + width: 100%; + max-width: 28rem; + background: var(--err-bg); + border: 1px solid var(--err-line); + border-radius: var(--radius); + box-shadow: var(--shadow-lg); + /* right padding clears the absolutely-positioned close button */ + padding: 1.1rem 3rem 1.25rem 1.25rem; +} +#sync-modal-title { + margin: 0 0 0.6rem; + font-size: 1.1rem; + color: var(--ink); +} +#sync-modal-error { + margin: 0 0 1rem; + font-family: var(--mono); + font-size: 0.9rem; + color: var(--err-ink); + overflow-wrap: anywhere; +} +.sync-modal-close { + position: absolute; + top: 0.3rem; + right: 0.3rem; + display: inline-flex; + align-items: center; + justify-content: center; + min-width: 44px; + min-height: 44px; + padding: 0; + border-radius: 999px; + border: 1px solid var(--err-line); + background: transparent; + color: var(--err-ink); + font-size: 1.2rem; + line-height: 1; + cursor: pointer; +} +.sync-modal-close:hover { background: rgb(239 68 68 / 0.15); } +/* No motion under reduced motion (same opt-out pattern as the + phase-25 background layers and the doc-modal backdrop). */ +@media (prefers-reduced-motion: reduce) { + .sync-modal-backdrop { transition: none; } +} + /* "Tuning" toggle (phase 15): ghost pill like New chat + a mono count badge (brand-ink on brand-soft ≈6.9:1). The label is visually-hidden (not removed) below 640px so the accessible name keeps the word. diff --git a/tests/e2e/test_sync_model_down.py b/tests/e2e/test_sync_model_down.py new file mode 100644 index 0000000..7c491f8 --- /dev/null +++ b/tests/e2e/test_sync_model_down.py @@ -0,0 +1,447 @@ +"""Phase 41 E2E (Playwright): model-down sync fails fast with a modal. + +Story: ``.agent/user_stories/sync-model-fail-fast.md`` +Run in isolation (DB must be up: ``podman compose up -d db``; git on +PATH — a documented environment prerequisite, phase 28): + + uv run pytest tests/e2e/test_sync_model_down.py -v --no-cov + +The story gate proves the whole story in the browser against a +**dead model endpoint**: + +* the pre-sync probe (``app.rag.llm.check_models``) fails the run + **before source resolution and before any clone** — a click against + the dead endpoint settles retry-ready with a model-naming modal in + a short wall-clock budget (≤ ~10 s, vs the phase-32 healthy run's + generous 60 s budget); +* the modal contract (AC 2): ``role="alertdialog"`` + + ``aria-modal="true"`` + the "Sync failed" title + the sanitized, + model-naming error text, dismissed via the × button, ``Esc``, or a + backdrop click — focus in-and-out to ``#sync-btn``; +* the existing surfaces stay (AC 4): the button's failed-state + ``title`` / ``aria-label`` / ``.is-error`` affordance and the + Sources page's ``#sync-error-banner`` (rendered off + ``bor:sync-status``); +* the healthy pipeline is untouched (AC 5): a **second** module app + (session mock, real ``file://`` clone → import → overview) still + runs the full phase-32 lifecycle to "Synced HH:MM" — the probe did + not break the happy path. + +Two module-scoped apps on distinct ports (the ``test_sync_button.py`` +module-app pattern): the dead-model app on ``APP_PORT + 41`` with +``BOR_LLM_BASE_URL=http://127.0.0.1:9/v1`` (a closed port — an +instant connection refused on loopback) pointed at a **real** +``file://`` fixture repo, so a regressed, non-fail-fast run would +spend real time cloning before failing; and the healthy app on +``APP_PORT`` (the session mock — the conftest session app never boots +in this isolated run, so no port clash). + +Test → story mapping (Playwright Mapping Rule): +1. ``test_model_down_fails_fast_with_modal`` → AC 1 + 2 (fast fail + before any clone; the modal with the model-naming error) +2. ``test_modal_dismissal`` → AC 2 (× / Esc / backdrop, one fresh + failure per path, focus back to ``#sync-btn`` each time) +3. ``test_sync_error_surfaces_unaffected`` → AC 4 (button + title/``.is-error``; the Sources banner off ``bor:sync-status``) +4. ``test_healthy_sync_still_succeeds`` → AC 5 (phase 32/35/38 + regression: counts + the idempotent second run) +""" +from __future__ import annotations + +import os +import re +import subprocess +import sys +import time +from collections.abc import Iterator +from pathlib import Path + +import pytest +from playwright.sync_api import Page, expect +from sqlalchemy import text + +from app.db import SessionLocal +from app.models import KbOverview +from e2e.auth_helpers import login +from e2e.conftest import ( + ADMIN_PASSWORD, + APP_PORT, + SESSION_SECRET, + USE_REAL_LLM, + _wait_http, +) + +REPO = Path(__file__).resolve().parents[2] +APP_URL = f"http://127.0.0.1:{APP_PORT}" +#: The dead-model app's port — distinct so the isolated run never +#: clashes with the session app (``APP_PORT``) or the mock (8901). +DEAD_PORT = APP_PORT + 41 +DEAD_URL = f"http://127.0.0.1:{DEAD_PORT}" +#: A closed port on loopback — an instant connection refused, so the +#: probe (and therefore the whole run) dies within milliseconds. +DEAD_LLM_URL = "http://127.0.0.1:9/v1" + +#: The fixture doc, same shape as ``test_sync_button.py``'s (a +#: regressed, non-fail-fast run would clone + import it — then still +#: fail, but only after spending real clone time). +FIXTURE_DOC = "notes/sync-fixture.md" +SENTINEL = "RESE-MDOWN-SENTINEL-41a7" + +#: "Synced HH:MM" — the local-time last-result label (header.js's +#: fmtSyncTime), any hour/minute. +SYNCED_LABEL = re.compile(r"Synced \d{1,2}:\d{2}") + +#: The fail-fast budget: probe dies on a connection refused, the 2 s +#: poll observes the ``failed`` state — seconds, not the healthy run's +#: 60 s (a regressed run that cloned first would blow this budget). +FAIL_TIMEOUT_MS = 10_000 +#: Real git clone + embed against the mock LLM (phase-32 budget). +SYNC_TIMEOUT_MS = 60_000 + + +def _git(cwd: Path, *args: str) -> None: + """Run git in *cwd*; a non-zero exit fails the fixture loudly.""" + proc = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) + if proc.returncode != 0: + raise AssertionError(f"git {' '.join(args)} failed: {proc.stderr.strip()}") + + +def _build_fixture_repo(root: Path) -> Path: + """A real one-commit git repo the sync would clone (built under + *root* with real ``git`` subprocess calls — module-safe, no + network).""" + repo = root / "homelab-notes" + (repo / "notes").mkdir(parents=True) + (repo / "notes" / "sync-fixture.md").write_text( + "# Model-down sync fixture note\n" + "\n" + "One small note that exists only to prove the phase-41 fail-fast\n" + "story end to end: with the model endpoint dead the sync must\n" + "fail before this repo is ever cloned.\n" + "\n" + f"Marker: {SENTINEL}\n", + encoding="utf-8", + ) + _git(repo, "init", "-q") + _git(repo, "add", "-A") + _git( + repo, + "-c", "user.email=e@x", "-c", "user.name=t", + "-c", "commit.gpgsign=false", # the fixture commit never signs + "commit", "-qm", "one", + ) + assert (repo / ".git").is_dir() + return repo + + +def _app_env(port: int, llm_base_url: str, repo: Path) -> dict[str, str]: + """Per-module app env (the conftest pattern): admin auth, static + frontend, mock-calibrated threshold, its own checkout dir, and the + story's subject — one ``file://`` fixture repo as the source.""" + env = dict(os.environ) + env.pop("DEBUGPY", None) + env["BOR_ENVIRONMENT"] = "e2e" + env["BOR_STATIC_DIR"] = str(REPO / "frontend") + env["BOR_LLM_BASE_URL"] = llm_base_url + # Mock-calibrated threshold (conftest pattern) — keeps the healthy + # app's deterministic gate behavior; production default stays 0.62. + env["BOR_RELEVANCE_THRESHOLD"] = "0.30" + env.setdefault( + "BOR_DATABASE_URL", + "postgresql+psycopg://reese:reese@localhost:5432/brain_of_reese", + ) + # Phase 16: admin auth must be set or create_app() refuses to boot. + env["BOR_ADMIN_PASSWORD"] = ADMIN_PASSWORD + env["BOR_SESSION_SECRET"] = SESSION_SECRET + env["BOR_GIT_SOURCES"] = f"file://{repo}" + env["BOR_SOURCES_DIR"] = str(repo.parent / "checkouts") + return env + + +def _boot_app(env: dict[str, str], port: int, health_url: str) -> subprocess.Popen: + """Boot the app under test and block until its health endpoint is up.""" + proc = subprocess.Popen( + [sys.executable, "-m", "uvicorn", "app.main:app", + "--host", "127.0.0.1", "--port", str(port), "--log-level", "warning"], + cwd=REPO, + env=env, + ) + _wait_http(health_url) + return proc + + +@pytest.fixture(scope="module") +def dead_git_repo(tmp_path_factory: pytest.TempPathFactory) -> Path: + """The dead app's configured source — a real repo it must NEVER + clone (the probe fails first). Its own tmp root keeps the checkout + dir apart from the healthy app's.""" + return _build_fixture_repo(tmp_path_factory.mktemp("mdown_dead")) + + +@pytest.fixture(scope="module") +def healthy_git_repo(tmp_path_factory: pytest.TempPathFactory) -> Path: + """The healthy app's configured source — the probe passes, the real + pipeline runs: clone → import → overview (phase 32/35/38 shape).""" + return _build_fixture_repo(tmp_path_factory.mktemp("mdown_healthy")) + + +@pytest.fixture(scope="module") +def dead_app_server(dead_git_repo: Path) -> Iterator[str]: + """The dead-model app: ``BOR_LLM_BASE_URL`` points at a closed + loopback port, so ``check_models`` dies on a connection refused — + before source resolution, before any clone.""" + proc = _boot_app( + _app_env(DEAD_PORT, DEAD_LLM_URL, dead_git_repo), + DEAD_PORT, + f"{DEAD_URL}/api/health", + ) + try: + yield DEAD_URL + finally: + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + + +@pytest.fixture(scope="module") +def healthy_app_server(mock_llm: int, healthy_git_repo: Path) -> Iterator[str]: + """The healthy app: same pipeline, live session mock — proves the + phase-41 probe did not break the happy path.""" + llm = "https://aipi.reeseapps.com/v1" if USE_REAL_LLM else f"http://127.0.0.1:{mock_llm}/v1" + proc = _boot_app(_app_env(APP_PORT, llm, healthy_git_repo), APP_PORT, f"{APP_URL}/api/health") + try: + yield APP_URL + finally: + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + + +@pytest.fixture(scope="module") +def dead_app_url(dead_app_server: str) -> str: + return dead_app_server + + +@pytest.fixture(scope="module") +def healthy_app_url(healthy_app_server: str) -> str: + return healthy_app_server + + +@pytest.fixture(scope="module") +def app_url(healthy_app_url: str) -> str: + """Shadow the session fixture (the conftest session app must never + boot in this isolated run — it would clash on ``APP_PORT``); + ``db_ready`` health-checks the healthy module app instead.""" + return healthy_app_url + + +def _truncate_kb() -> None: + """Fresh KB per test (the E2E isolation pattern): any counts and the + ``kb_overview`` row the healthy run produces are its own doing.""" + with SessionLocal() as db: + db.execute(text("TRUNCATE chunks, documents, query_log, kb_overview")) + db.commit() + + +@pytest.fixture(autouse=True) +def _clean_kb(db_ready: None) -> Iterator[None]: + _truncate_kb() + yield + + +def _overview_row() -> KbOverview | None: + with SessionLocal() as db: + return db.get(KbOverview, 1) + + +def _dismiss_reattached_modal(page: Page) -> None: + """Close the modal the page load RE-ATTACHED to (phase 32/41 + behavior): the dead app's status is still ``failed`` from an + earlier test in this module, so a fresh page load re-enters the + failed state — button affordances restored, modal open, focus in + the dialog. Dismiss it (Esc) for a clean trigger state.""" + modal = page.locator(".sync-modal") + expect(modal).to_be_visible(timeout=5_000) + expect(page.locator(".sync-modal-close")).to_be_focused() + page.keyboard.press("Escape") + expect(modal).to_be_hidden() + + +def _trigger_dead_failure(page: Page, app_url: str) -> None: + """One fresh fail-fast failure against the dead-model app: click + ``#sync-btn`` → running → the probe dies → the run settles + retry-ready with the modal open (≤ the 10 s fail-fast budget).""" + btn = page.locator("#sync-btn") + btn.click() + expect(page.locator("#sync-label")).to_have_text("Syncing…", timeout=5_000) + expect(page.locator(".sync-modal")).to_be_visible(timeout=FAIL_TIMEOUT_MS) + expect(btn).to_be_enabled(timeout=FAIL_TIMEOUT_MS) + expect(page.locator("#sync-label")).to_have_text("Sync sources") + + +# --- 1. Fail fast + the model-naming modal --------------------------------- + + +def test_model_down_fails_fast_with_modal( + page: Page, dead_app_url: str, db_ready: None +) -> None: + """AC 1 + 2 — with a dead model endpoint the click settles + retry-ready **and** the modal is visible within a short wall-clock + budget (≤ ~10 s, vs the healthy run's 60 s): the run failed before + any clone. The error names the embedding model; the dialog carries + the alertdialog/aria-modal contract.""" + login(page, dead_app_url) + btn = page.locator("#sync-btn") + expect(btn).to_be_visible() + + start = time.monotonic() + _trigger_dead_failure(page, dead_app_url) + elapsed = time.monotonic() - start + # Fail-fast: seconds, not a clone-then-import budget. + assert elapsed < 10.0, ( + f"the dead-model sync took {elapsed:.1f}s — a non-fail-fast run " + "(clones before probing) would blow this budget" + ) + + # The server state agrees: failed, model-naming error. + body = page.request.get(f"{dead_app_url}/api/sync/status").json() + assert body["state"] == "failed", body + assert "embedding model" in body["error"], body["error"] + assert "'embed'" in body["error"], body["error"] + assert "not available" in body["error"], body["error"] + + # The modal contract (AC 2). + modal = page.get_by_role("alertdialog") + expect(modal).to_be_visible() + expect(modal).to_have_attribute("aria-modal", "true") + expect(page.locator("#sync-modal-title")).to_have_text("Sync failed") + error = page.locator("#sync-modal-error") + expect(error).to_contain_text("embedding model") + expect(error).to_contain_text("'embed'") + expect(error).to_contain_text("not available") + # The close control is present and focused (focus moved IN). + expect(page.locator(".sync-modal-close")).to_be_focused() + + +# --- 2. Dismissal: × / Esc / backdrop, focus out to #sync-btn --------------- + + +def test_modal_dismissal(page: Page, dead_app_url: str, db_ready: None) -> None: + """AC 2 — one fresh failure per dismissal path: the × button, + ``Esc``, and a backdrop click (never the panel). Each open focuses + the close button; each close returns focus to ``#sync-btn`` — the + control that started the run.""" + login(page, dead_app_url) + _dismiss_reattached_modal(page) # the prior test's failure is still last + btn = page.locator("#sync-btn") + modal = page.locator(".sync-modal") + close_btn = page.locator(".sync-modal-close") + + # Path 1: the × button. + _trigger_dead_failure(page, dead_app_url) + expect(close_btn).to_be_focused() + close_btn.click() + expect(modal).to_be_hidden() + expect(btn).to_be_focused() # focus returned to #sync-btn + + # Path 2: Esc. + _trigger_dead_failure(page, dead_app_url) + expect(close_btn).to_be_focused() + page.keyboard.press("Escape") + expect(modal).to_be_hidden() + expect(btn).to_be_focused() + + # Path 3: a click on the backdrop itself (the dimmed area — the + # panel is centered, so a corner click can never land on it). + _trigger_dead_failure(page, dead_app_url) + expect(close_btn).to_be_focused() + page.locator(".sync-modal-backdrop").click(position={"x": 10, "y": 10}) + expect(modal).to_be_hidden() + expect(btn).to_be_focused() + + +# --- 3. The secondary surfaces are untouched -------------------------------- + + +def test_sync_error_surfaces_unaffected( + page: Page, dead_app_url: str, db_ready: None +) -> None: + """AC 4 — after a failure the button keeps its failed-state + ``title`` / ``aria-label`` / ``.is-error`` affordance (the + non-Sources visibility), and on ``/sources.html`` (same dead-model + app) the ``#sync-error-banner`` still renders the error off + ``bor:sync-status`` — the phase-32/34 contract the modal is + additive to.""" + # Trigger the failure on the chat page (the modal is module-owned — + # it follows the button onto every page). The re-attach modal from + # the prior test's last failure closes first (clean trigger state). + login(page, dead_app_url, next="/") + _dismiss_reattached_modal(page) + _trigger_dead_failure(page, dead_app_url) + + btn = page.locator("#sync-btn") + expect(btn).to_have_attribute("title", re.compile("embedding model")) + expect(btn).to_have_attribute("aria-label", re.compile("embedding model")) + expect(btn).to_have_class(re.compile("is-error")) + + # A clean navigation: the Sources page re-attaches to the failed + # run and renders the banner off the event. + page.goto(f"{dead_app_url}/sources.html") + banner = page.locator("#sync-error-banner") + expect(banner).to_be_visible() + expect(banner).to_have_attribute("role", "alert") + expect(page.locator("#sync-error-text")).to_contain_text("embedding model") + expect(page.locator("#sync-error-text")).to_contain_text("'embed'") + + +# --- 4. The healthy pipeline is untouched (phase 32 regression) ------------- + + +def test_healthy_sync_still_succeeds( + page: Page, healthy_app_url: str, db_ready: None +) -> None: + """AC 5 — with a live model the probe passes and the full + phase-32/35/38 pipeline runs unchanged: real ``file://`` clone → + import (prune) → KB overview regeneration → "Synced HH:MM" + the + counts, then the idempotent second run ("0 added · 1 unchanged").""" + login(page, healthy_app_url) # lands on /sources.html (the button's home) + + btn = page.locator("#sync-btn") + expect(btn).to_be_visible() + expect(page.locator("#sync-label")).to_have_text("Sync sources") + + # --- run 1: probe passes; clone + import + overview ------------------ + btn.click() + expect(btn).to_be_disabled() + expect(page.locator("#sync-label")).to_have_text("Syncing…") + expect(page.locator("#sync-label")).to_have_text(SYNCED_LABEL, timeout=SYNC_TIMEOUT_MS) + expect(btn).to_be_enabled() # never stale — re-enabled at the terminal state + expect(page.locator("#sync-result")).to_have_text("1 added") + + # The REAL clone was imported: the fixture path is in the Sources + # table (the sentinel lives inside it). + expect(page.locator("#docs-tbody tr", has_text=FIXTURE_DOC)).to_have_count(1) + + # Phase-31 regeneration ran (the import changed the KB): the single + # kb_overview row is fresh and non-empty (DB check — truncated + # before this test, so it is the sync's own doing). + row = _overview_row() + assert row is not None and row.content.strip(), ( + "kb_overview must be regenerated by a successful, KB-changing sync" + ) + + # --- run 2: idempotent pull + hash skip ------------------------------- + btn.click() + expect(btn).to_be_disabled() + expect(page.locator("#sync-label")).to_have_text("Syncing…") + expect(page.locator("#sync-label")).to_have_text(SYNCED_LABEL, timeout=SYNC_TIMEOUT_MS) + expect(btn).to_be_enabled() + expect(page.locator("#sync-result")).to_have_text("0 added · 1 unchanged") + expect(page.locator("#docs-tbody tr", has_text=FIXTURE_DOC)).to_have_count(1) + + # And no modal ever opened (no failure on the healthy app). + expect(page.locator(".sync-modal")).to_have_count(0) diff --git a/tests/fakes.py b/tests/fakes.py index 3acefb5..5883202 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -29,6 +29,12 @@ class FakeEmbedder: self.embed_batches += 1 return [[0.01 * (i % 97) for i in range(self.dim)] for _ in texts] + async def embed_one(self, text: str) -> list[float]: + """The retrieval/probe convenience path — delegates to :meth:`embed` + (satisfies the phase-41 pre-sync model probe).""" + (vec,) = await self.embed([text]) + return vec + async def chat( self, messages: list[dict[str, str]], model: str | None = None ) -> str: diff --git a/tests/integration/test_sync_api.py b/tests/integration/test_sync_api.py index 31c6c34..f86f784 100644 --- a/tests/integration/test_sync_api.py +++ b/tests/integration/test_sync_api.py @@ -29,6 +29,16 @@ test), so DB-over-env and the env fallback go through the actual indirection; the env list is driven by a fresh ``Settings`` on the resolver's module (the dev ``.env`` never leaks in). +Phase 41: the pre-sync model probe (``check_models``) — a dead model +endpoint fails the run **before any clone** (the model-naming error +lands in the ``failed`` state verbatim; the sanitizer is a no-op for +it); the probe runs before ``effective_sources``; a real probe against +a stubbed dead ``LLMClient`` names the embed model **and** masks the +credentials wrapped from the endpoint URL. Where the runner keeps a +real ``LLMClient`` the probe is stubbed (:func:`_stub_probe`) so no +test ever hits the network; the ``_real_llm`` tests get a passing +probe from ``FakeEmbedder.embed_one``/``chat``. + The git / import / overview layers are monkeypatched in ``app.api.sync`` (same fake style as ``test_import_docs_git.py``) — no real git, no LLM: the runner's state machine and HTTP surface are under test. @@ -47,6 +57,7 @@ import time from collections.abc import Iterator from datetime import UTC, datetime from pathlib import Path +from typing import Any import pytest from fastapi.testclient import TestClient @@ -59,7 +70,7 @@ from app.main import app as fastapi_app from app.models import GitSource from app.rag import git_sources as git_sources_resolver from app.rag.importer import ImportSummary -from app.rag.llm import EmbeddingError, LLMClient +from app.rag.llm import EmbeddingError, LLMClient, ModelUnavailableError from scripts.git_sync import GitSyncError from tests.conftest import ADMIN_PASSWORD from tests.fakes import FakeEmbedder @@ -142,10 +153,25 @@ def clean_documents(db: Session) -> Iterator[None]: def _real_llm(monkeypatch: pytest.MonkeyPatch) -> None: """The pipeline's ``LLMClient`` becomes the deterministic in-process - ``FakeEmbedder`` (real import, no network).""" + ``FakeEmbedder`` (real import, no network). ``FakeEmbedder`` + implements ``embed_one`` + ``chat``, so the phase-41 probe passes + against it without a stub.""" monkeypatch.setattr(sync_api, "LLMClient", lambda: FakeEmbedder()) +def _stub_probe(monkeypatch: pytest.MonkeyPatch) -> list[Any]: + """Stub the phase-41 model probe (a real ``LLMClient`` in the runner + would hit the network). Returns the clients the probe received, so + tests can assert the probe and the import share one client.""" + seen: list[Any] = [] + + async def fake_check_models(llm: LLMClient) -> None: + seen.append(llm) + + monkeypatch.setattr(sync_api, "check_models", fake_check_models) + return seen + + def _login(client: TestClient) -> None: r = client.post("/api/login", json={"password": ADMIN_PASSWORD}) assert r.status_code == 204, f"admin login failed: {r.status_code} {r.text}" @@ -250,6 +276,7 @@ def test_admin_sync_success_reports_full_detail( ) clone_calls, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + probe_seen = _stub_probe(monkeypatch) # real LLMClient — probe stubbed summary = ImportSummary( files=5, added=1, updated=2, unchanged=2, pruned=3, errors=0, chunks=11, embed_batches=4, summaries=1, summary_errors=0, @@ -294,6 +321,9 @@ def test_admin_sync_success_reports_full_detail( assert isinstance(fake_import.llms[0], LLMClient) # Overview: refreshed (added + updated > 0) with the same client. assert fake_overview.llms == [fake_import.llms[0]] + # Phase 41: the probe ran first and got the very client the import + # and the overview reuse. + assert probe_seen == [fake_import.llms[0]] def test_unchanged_kb_skips_overview_refresh( @@ -310,6 +340,7 @@ def test_unchanged_kb_skips_overview_refresh( ) _, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + _stub_probe(monkeypatch) summary = ImportSummary(files=7, added=0, updated=0, unchanged=7, pruned=0) fake_import = FakeImportSources(summary) monkeypatch.setattr(sync_api, "import_sources", fake_import) @@ -341,6 +372,7 @@ def test_double_trigger_while_running_returns_409( ) _, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + _stub_probe(monkeypatch) # The in-flight run takes a while (asyncio.sleep) so the second POST # lands while it is still running. fake_import = FakeImportSources(ImportSummary(files=1, added=1), delay=0.5) @@ -387,6 +419,7 @@ def test_git_failure_marks_failed_and_skips_import( ) monkeypatch.setattr(sync_api, "clone_or_pull", failing_clone) + _stub_probe(monkeypatch) fake_import = FakeImportSources(ImportSummary()) monkeypatch.setattr(sync_api, "import_sources", fake_import) fake_overview = FakeOverview(ok=True) @@ -423,6 +456,7 @@ def test_no_sources_configured_fails_loudly( ) clone_calls, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + _stub_probe(monkeypatch) fake_import = FakeImportSources(ImportSummary()) monkeypatch.setattr(sync_api, "import_sources", fake_import) @@ -454,6 +488,7 @@ def test_db_rows_win_over_env( ) clone_calls, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + _stub_probe(monkeypatch) fake_import = FakeImportSources(ImportSummary(files=1, added=1)) monkeypatch.setattr(sync_api, "import_sources", fake_import) monkeypatch.setattr(sync_api, "regenerate_overview", FakeOverview(ok=True)) @@ -486,6 +521,7 @@ def test_env_fallback_when_table_empty( ) clone_calls, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + _stub_probe(monkeypatch) fake_import = FakeImportSources(ImportSummary(files=1, added=1)) monkeypatch.setattr(sync_api, "import_sources", fake_import) monkeypatch.setattr(sync_api, "regenerate_overview", FakeOverview(ok=True)) @@ -618,6 +654,7 @@ def test_missing_local_dir_fails_loudly_and_imports_nothing( ) clone_calls, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + _stub_probe(monkeypatch) fake_import = FakeImportSources(ImportSummary()) monkeypatch.setattr(sync_api, "import_sources", fake_import) fake_overview = FakeOverview(ok=True) @@ -647,6 +684,7 @@ def test_import_error_is_reported_with_credentials_masked( ) _, fake_clone = _fake_clone() monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + _stub_probe(monkeypatch) async def failing_import( sources: list[Path], @@ -671,3 +709,135 @@ def test_import_error_is_reported_with_credentials_masked( assert "*****@aipi.reeseapps.com" in body["error"] # credentials masked assert "user:secret" not in body["error"] assert "connection refused" in body["error"] # the reason survives + + +# --- phase 41: model probe (fail fast before any clone) -------------------- + + +class _DeadEmbedder: + """Probe fake whose embedding call dies like a dead endpoint — with a + credential-bearing URL in the wrapped error (the sanitizer must + mask it). ``chat`` is recorded: it must never be reached.""" + + def __init__(self) -> None: + self.settings = Settings(_env_file=None) # pyright: ignore[reportCallIssue] + self.embed_batches = 0 + self.chat_calls: list[list[dict[str, str]]] = [] + + async def embed_one(self, text: str) -> list[float]: + raise EmbeddingError( + "embeddings request to https://user:secret@aipi.reeseapps.com/v1 " + "failed: connection refused" + ) + + async def chat(self, messages: list[dict[str, str]], model: str | None = None) -> str: + self.chat_calls.append(list(messages)) + return "pong" + + +def test_model_down_fails_fast_before_any_clone( + sync_client: TestClient, monkeypatch: pytest.MonkeyPatch, db: Session, tmp_path: Path +) -> None: + """A dead model endpoint fails the run **before any clone**: the + model-naming error lands in the ``failed`` state verbatim (the + sanitizer is a no-op for it), and clone + import never run.""" + repo_url = f"file://{tmp_path / 'repo.git'}" + _seed(db, repo_url) + _stub_env(monkeypatch) + monkeypatch.setattr( + sync_api, + "get_settings", + lambda: _settings(sources_dir=str(tmp_path / "bor")), + ) + clone_calls, fake_clone = _fake_clone() + monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + fake_import = FakeImportSources(ImportSummary()) + monkeypatch.setattr(sync_api, "import_sources", fake_import) + message = ( + "The embedding model ('embed') is not available — " + "check the model endpoint and retry." + ) + + async def dead_probe(llm: LLMClient) -> None: + raise ModelUnavailableError(message) + + monkeypatch.setattr(sync_api, "check_models", dead_probe) + + _login(sync_client) + assert sync_client.post("/api/sync").status_code == 202 + + body = _poll(sync_client, "failed") + assert body["error"] == message # the model is named, verbatim + # The sanitizer must leave the model-naming message untouched. + assert sync_api._sanitize_error(body["error"]) == body["error"] + assert body["detail"] == {} + assert clone_calls == [] # fail fast: before any clone + assert fake_import.sources == [] # and before any import + + +def test_probe_names_dead_embed_model_and_masks_credentials( + sync_client: TestClient, monkeypatch: pytest.MonkeyPatch, db: Session, tmp_path: Path +) -> None: + """The **real** probe against a stubbed dead client: the error names + the embedding model, and the credentials wrapped from the endpoint + URL are masked by the sync sanitizer (the reason survives).""" + repo_url = f"file://{tmp_path / 'repo.git'}" + _seed(db, repo_url) + _stub_env(monkeypatch) + monkeypatch.setattr( + sync_api, + "get_settings", + lambda: _settings(sources_dir=str(tmp_path / "bor")), + ) + dead = _DeadEmbedder() + monkeypatch.setattr(sync_api, "LLMClient", lambda: dead) + clone_calls, fake_clone = _fake_clone() + monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + fake_import = FakeImportSources(ImportSummary()) + monkeypatch.setattr(sync_api, "import_sources", fake_import) + + _login(sync_client) + assert sync_client.post("/api/sync").status_code == 202 + + body = _poll(sync_client, "failed") + assert "The embedding model ('embed') is not available" in body["error"] + assert "*****@aipi.reeseapps.com" in body["error"] # credentials masked + assert "user:secret" not in body["error"] + assert "connection refused" in body["error"] # the reason survives + assert dead.chat_calls == [] # embed died first — chat never probed + assert clone_calls == [] + assert fake_import.sources == [] + + +def test_probe_runs_before_source_resolution( + sync_client: TestClient, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Ordering: the probe is the **first** pipeline step — it runs + before ``effective_sources`` (and, transitively, before any + clone).""" + order: list[str] = [] + + async def spy_probe(llm: LLMClient) -> None: + order.append("probe") + + def spy_effective_sources(session: Session) -> tuple[list[GitSource], str]: + order.append("effective_sources") + raise GitSyncError("short-circuit after the ordering spy") + + monkeypatch.setattr(sync_api, "check_models", spy_probe) + monkeypatch.setattr(sync_api, "effective_sources", spy_effective_sources) + monkeypatch.setattr( + sync_api, + "get_settings", + lambda: _settings(sources_dir=str(tmp_path / "bor")), + ) + clone_calls, fake_clone = _fake_clone() + monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone) + + _login(sync_client) + assert sync_client.post("/api/sync").status_code == 202 + body = _poll(sync_client, "failed") + + assert order == ["probe", "effective_sources"] + assert clone_calls == [] + assert "short-circuit" in body["error"] # the spy aborted the run diff --git a/tests/unit/test_sync_button.py b/tests/unit/test_sync_button.py index 72422a0..5baa6b8 100644 --- a/tests/unit/test_sync_button.py +++ b/tests/unit/test_sync_button.py @@ -327,6 +327,104 @@ def test_header_js_emits_bor_sync_status_on_state_changes() -> None: assert "emitSyncStatus(status)" in _body(js, "initSyncButton") +# ---------- header.js: the sync failure modal (phase 41, TODO.md L4) ---------- + + +def test_header_js_owns_a_lazily_created_sync_modal() -> None: + """The modal is module-owned and created lazily ONCE (module-level + `let syncModal = null`): a .sync-modal-backdrop holding a .sync-modal + panel with role="alertdialog" + aria-modal + labelled/described ids + + the close button, appended to document.body — no page-markup + changes, so every page carrying #sync-btn gets it. The module + docstring's sync bullet records the modal (phase 41).""" + js = _text(HEADER_JS) + assert "let syncModal = null" in js, "module-level once-only modal ref" + assert 'role="alertdialog"' in js + assert 'aria-modal="true"' in js + assert 'aria-labelledby="sync-modal-title"' in js + assert 'aria-describedby="sync-modal-error"' in js + assert "module-owned error modal" in js, "the docstring sync bullet" + create = _body(js, "createSyncModal") + assert "sync-modal-backdrop" in create + assert 'document.body.appendChild(backdrop)' in create + assert "return backdrop" in create, "the module ref must hold the created element" + assert "Sync failed" in create, "the dialog title" + assert 'class="sync-modal-close"' in create + assert 'aria-label="Close error dialog"' in create + + +def test_sync_modal_error_is_rendered_via_text_content() -> None: + """The error text is ALWAYS set via textContent (XSS-safe — no + innerHTML with user data in the open path), and it is set BEFORE + the already-open check, so a second failure while open updates the + text IN PLACE (no stacking, no focus jump).""" + js = _text(HEADER_JS) + body = _body(js, "showSyncModal") + assert 'querySelector("#sync-modal-error").textContent' in body + assert "innerHTML" not in body, "the open path never touches innerHTML" + text = body.find("textContent") + open_check = body.find('contains("is-open")') + assert text != -1 and open_check != -1 and text < open_check, ( + "the in-place update happens while the modal is already open" + ) + + +def test_sync_modal_focus_goes_in_and_out_to_sync_btn() -> None: + """On open: document.activeElement is remembered and focus moves + to the close button — with a fallback to #sync-btn when the active + element is (the run's disabled button dropped focus there; + the close must still land on the control that started the run); + on close: focus returns to the remembered element (guarded by + document.contains — a detached target is a no-op).""" + js = _text(HEADER_JS) + open_body = _body(js, "showSyncModal") + assert "const active = document.activeElement" in open_body + assert "active !== document.body" in open_body, ( + "the disabled-button window leaves focus on — the fallback" + ) + assert "? active : syncBtn" in open_body, ("the fallback remembers #sync-btn") + assert 'querySelector(".sync-modal-close").focus()' in open_body + close_body = _body(js, "closeSyncModal") + assert "document.contains(target)" in close_body + assert "target.focus()" in close_body + assert 'contains("is-open")' in close_body, "closing a closed modal is a no-op" + + +def test_sync_modal_closes_via_button_esc_and_backdrop() -> None: + """All three dismissal paths call the SAME close function: the + close button, Esc (ONE document keydown binding, acting only while + the modal is open), and a click on the backdrop element itself — + the event.target check keeps clicks bubbling from the panel from + closing it.""" + js = _text(HEADER_JS) + create = _body(js, "createSyncModal") + assert 'addEventListener("click", closeSyncModal)' in create + assert 'e.key === "Escape"' in create + assert 'addEventListener("keydown"' in create + assert 'backdrop.classList.contains("is-open")' in create + assert "e.target === backdrop" in create + + +def test_apply_sync_failure_opens_the_modal_after_the_event() -> None: + """applySyncFailure opens the modal with the SANITIZED error, and + the call comes AFTER the existing button title/aria/.is-error + lines + emitSyncStatus (the bor:sync-status event the Sources + banner renders off — those lines stay byte-identical; the modal is + additive).""" + js = _text(HEADER_JS) + body = _body(js, "applySyncFailure") + call = body.find("showSyncModal(") + emit = body.find("emitSyncStatus(status)") + assert call != -1, "the modal must open from the failure path" + assert emit != -1 and call > emit, "the event still emits first (byte-identical)" + assert "showSyncModal(error)" in body, "the sanitized error goes to the modal" + # The pre-existing affordances stay (the Sources banner contract). + assert "syncBtn.title = error" in body + assert 'syncBtn.setAttribute("aria-label", error)' in body + assert 'syncBtn.classList.add("is-error")' in body + assert "emitSyncStatus(status)" in body + + # ---------- sources.js: the event-driven result line + banner ---------- @@ -415,3 +513,61 @@ def test_sync_result_is_styled() -> None: block = re.search(r"\.sync-result\s*\{([^}]*)\}", css) assert block, "styles.css must define .sync-result" assert "var(--ink-soft)" in block.group(1) + + +# ---------- styles.css: the sync failure modal (phase 41, TODO.md L4) ---------- + + +def test_sync_modal_css_error_palette_and_stacking() -> None: + """.sync-modal-backdrop: fixed, full-viewport, rgba dim, z-index + above the sticky header; .sync-modal: the centered ≈28rem panel on + the phase-08 error palette (panel on --err-bg, 1px --err-line + border, --err-ink error text, --ink title — all computed ≥4.5:1); + open/close via .is-open (visibility/opacity).""" + css = _text(STYLES_CSS) + backdrop = re.search(r"\.sync-modal-backdrop\s*\{([^}]*)\}", css) + assert backdrop, "styles.css must define .sync-modal-backdrop" + b = backdrop.group(1) + assert "position: fixed" in b + assert "inset: 0" in b + assert "z-index: 1000" in b, "above the sticky header (20) + skip-link (100)" + assert "rgba(" in b, "the dim over the page" + open_state = re.search(r"\.sync-modal-backdrop\.is-open\s*\{([^}]*)\}", css) + assert open_state, ".is-open must be the open state" + assert "visibility: visible" in open_state.group(1) + assert "opacity: 1" in open_state.group(1) + panel = re.search(r"\.sync-modal\s*\{([^}]*)\}", css) + assert panel, "styles.css must define .sync-modal" + p = panel.group(1) + assert "max-width: 28rem" in p + assert "var(--err-bg)" in p + assert "var(--err-line)" in p + title = re.search(r"#sync-modal-title\s*\{([^}]*)\}", css) + assert title, "the modal title must be styled" + assert "var(--ink)" in title.group(1) + error = re.search(r"#sync-modal-error\s*\{([^}]*)\}", css) + assert error, "the modal error line must be styled" + assert "var(--err-ink)" in error.group(1) + + +def test_sync_modal_close_button_touch_floor() -> None: + """The close button keeps the 44px touch floor at every width (the + global 3px :focus-visible outline applies — no per-button + override).""" + css = _text(STYLES_CSS) + block = re.search(r"\.sync-modal-close\s*\{([^}]*)\}", css) + assert block, "styles.css must define .sync-modal-close" + assert "min-width: 44px" in block.group(1) + assert "min-height: 44px" in block.group(1) + + +def test_sync_modal_respects_reduced_motion() -> None: + """The open/close fade is stilled under prefers-reduced-motion — + the phase-25 / doc-modal opt-out pattern.""" + css = _text(STYLES_CSS) + reduced = re.search( + r"@media \(prefers-reduced-motion: reduce\)\s*\{\s*\.sync-modal-backdrop\s*\{([^}]*)\}", + css, + ) + assert reduced, "the backdrop fade must opt out under prefers-reduced-motion" + assert "transition: none" in reduced.group(1) diff --git a/tests/unit/test_sync_model_probe.py b/tests/unit/test_sync_model_probe.py new file mode 100644 index 0000000..4599424 --- /dev/null +++ b/tests/unit/test_sync_model_probe.py @@ -0,0 +1,199 @@ +"""Unit tests: the phase-41 pre-sync model probe (``check_models``). + +The probe verifies both models a sync needs — ``embed`` (one short +embedding) and the summary model ``lite`` (one 1-token-scale +completion) — before any expensive work, and fails with a +: class:`ModelUnavailableError` that **names the dead model**. The fake +client is duck-typed (``embed_one`` / ``chat`` + ``settings``), same +shape as :class:`tests.fakes.FakeEmbedder` — no network, no httpx +transport. + +Covered paths (keep ``app/`` >90%): +- both models up → returns, both methods called exactly once; +- embedding probe raises → ``ModelUnavailableError`` naming the + **embedding model**, and the summary probe is **never** reached; +- summary probe raises → ``ModelUnavailableError`` naming the + **summary model**; +- message content: model name present, "not available" wording, the + original exception chained as ``__cause__``; +- the error is an ``LLMError`` (the sync's generic failure catch). +""" +from __future__ import annotations + +import asyncio +from typing import Any, cast + +import pytest + +from app.config import Settings +from app.rag.llm import ( + EmbeddingError, + LLMClient, + LLMError, + ModelUnavailableError, + check_models, +) + + +class _ProbeClient: + """Duck-typed :class:`app.rag.llm.LLMClient` stand-in for the probe.""" + + def __init__( + self, + embed_error: Exception | None = None, + chat_error: Exception | None = None, + embed_model: str = "embed", + summary_model: str = "lite", + ) -> None: + kwargs: dict[str, Any] = { + "_env_file": None, + "llm_embed_model": embed_model, + "llm_summary_model": summary_model, + } + self.settings = Settings(**kwargs) # pyright: ignore[reportCallIssue] + self.embed_error = embed_error + self.chat_error = chat_error + self.embed_texts: list[str] = [] + self.chat_messages: list[list[dict[str, str]]] = [] + + async def embed_one(self, text: str) -> list[float]: + self.embed_texts.append(text) + if self.embed_error is not None: + raise self.embed_error + return [0.0] * 768 + + async def chat(self, messages: list[dict[str, str]], model: str | None = None) -> str: + self.chat_messages.append(list(messages)) + if self.chat_error is not None: + raise self.chat_error + return "pong" + + +def _run_check(llm: _ProbeClient) -> Any: + # The probe is duck-typed at the ``embed_one`` / ``chat`` surface — the + # cast keeps pyright honest about the narrow stand-in. + return asyncio.run(check_models(cast("LLMClient", llm))) + + +# --- both models up -------------------------------------------------------- + + +def test_both_models_up_returns_and_calls_both() -> None: + llm = _ProbeClient() + + assert _run_check(llm) is None + assert llm.embed_texts == ["sync model check"] # the tiny probe input + assert llm.chat_messages == [[{"role": "user", "content": "ping"}]] + + +def test_chat_probe_defaults_to_summary_model_setting() -> None: + """The probe calls ``chat`` without a model override — the client's + ``llm_summary_model`` default (``lite``) is what gets pinged.""" + llm = _ProbeClient() + + _run_check(llm) + # ``chat`` received exactly the probe messages; the model default is + # resolved inside LLMClient.chat from settings.llm_summary_model. + assert llm.settings.llm_summary_model == "lite" + assert llm.chat_messages == [[{"role": "user", "content": "ping"}]] + + +# --- embedding model down -------------------------------------------------- + + +def test_embed_down_names_embed_model_and_never_reaches_chat() -> None: + boom = EmbeddingError( + "embeddings request to https://aipi.reeseapps.com/v1 failed: " + "connection refused" + ) + llm = _ProbeClient(embed_error=boom) + + with pytest.raises(ModelUnavailableError) as excinfo: + _run_check(llm) + + message = str(excinfo.value) + assert "'embed'" in message # the embedding model is named + assert "not available" in message + assert excinfo.value.__cause__ is boom # the original failure is chained + assert llm.chat_messages == [] # the summary probe is never reached + assert isinstance(excinfo.value, LLMError) # the sync's generic catch + + +def test_embed_down_wraps_any_exception_not_just_embedding_error() -> None: + boom = RuntimeError("transport exploded") + llm = _ProbeClient(embed_error=boom) + + with pytest.raises(ModelUnavailableError) as excinfo: + _run_check(llm) + + assert "The embedding model ('embed') is not available" in str(excinfo.value) + assert excinfo.value.__cause__ is boom + assert llm.chat_messages == [] + + +# --- summary model down ---------------------------------------------------- + + +def test_chat_down_names_summary_model() -> None: + boom = LLMError( + "chat completion from https://aipi.reeseapps.com/v1 failed: " + "connection refused" + ) + llm = _ProbeClient(chat_error=boom) + + with pytest.raises(ModelUnavailableError) as excinfo: + _run_check(llm) + + message = str(excinfo.value) + assert "'lite'" in message # the summary model is named + assert "not available" in message + assert excinfo.value.__cause__ is boom + assert llm.embed_texts == ["sync model check"] # the embed probe passed + assert isinstance(excinfo.value, LLMError) + + +def test_chat_down_wraps_any_exception() -> None: + llm = _ProbeClient(chat_error=ValueError("stream died")) + + with pytest.raises(ModelUnavailableError) as excinfo: + _run_check(llm) + + assert "The summary model ('lite') is not available" in str(excinfo.value) + + +# --- message content (modal-readable wording) ------------------------------- + + +@pytest.mark.parametrize( + ("client", "model"), + [ + (_ProbeClient(embed_error=EmbeddingError("down")), "embed"), + (_ProbeClient(chat_error=LLMError("down")), "lite"), + ], + ids=["embed-down", "summary-down"], +) +def test_message_content_mentions_model_and_availability( + client: _ProbeClient, model: str +) -> None: + with pytest.raises(ModelUnavailableError) as excinfo: + _run_check(client) + + message = str(excinfo.value) + assert f"('{model}')" in message # the model name, quoted + assert "not available" in message # the modal-readable wording + assert "check the model endpoint" in message # actionable hint + + +def test_custom_model_names_are_surfaced_verbatim() -> None: + """The probe names whatever the settings configure, not hardcoded + model strings.""" + llm = _ProbeClient( + embed_error=EmbeddingError("down"), + embed_model="bge-m3", + summary_model="hermes-lite", + ) + + with pytest.raises(ModelUnavailableError) as excinfo: + _run_check(llm) + + assert "'bge-m3'" in str(excinfo.value)