From 137d5fa1a5117daecde9394c27032ee13b868c2a Mon Sep 17 00:00:00 2001 From: ducoterra Date: Wed, 2 Sep 2026 15:55:33 -0400 Subject: [PATCH] feat(sources): removing a source deletes its files and index entries behind a confirmation modal --- .../69_source_removal_cleanup/00_phase.md | 149 ++++ .../01_full_removal_backend.md | 115 +++ .../02_confirmation_modal.md | 122 +++ .../03_e2e_and_commit.md | 112 +++ ...al_cleanup__01_full_removal_backend.a1.err | 0 ...val_cleanup__01_full_removal_backend.a1.md | 19 + ...eanup__01_full_removal_backend.a1.validate | 74 ++ ...oval_cleanup__02_confirmation_modal.a1.err | 0 ...moval_cleanup__02_confirmation_modal.a1.md | 19 + ...cleanup__02_confirmation_modal.a1.validate | 74 ++ ..._removal_cleanup__03_e2e_and_commit.a1.err | 0 README.md | 23 +- app/api/git_sources.py | 139 +++- app/rag/source_removal.py | 138 ++++ frontend/assets/git-sources.js | 268 +++++-- frontend/assets/styles.css | 154 +++- frontend/git-sources.html | 66 +- tests/e2e/test_git_sources_admin.py | 37 +- tests/e2e/test_source_removal_cleanup.py | 738 ++++++++++++++++++ tests/integration/test_git_sources_api.py | 34 +- tests/integration/test_source_removal_api.py | 528 +++++++++++++ tests/unit/test_agent.py | 16 +- tests/unit/test_remove_confirm_modal.py | 472 +++++++++++ tests/unit/test_source_removal.py | 310 ++++++++ 24 files changed, 3489 insertions(+), 118 deletions(-) create mode 100644 .agent/phases/complete/69_source_removal_cleanup/00_phase.md create mode 100644 .agent/phases/complete/69_source_removal_cleanup/01_full_removal_backend.md create mode 100644 .agent/phases/complete/69_source_removal_cleanup/02_confirmation_modal.md create mode 100644 .agent/phases/complete/69_source_removal_cleanup/03_e2e_and_commit.md create mode 100644 .agent/reports/69_source_removal_cleanup/69_source_removal_cleanup__01_full_removal_backend.a1.err create mode 100644 .agent/reports/69_source_removal_cleanup/69_source_removal_cleanup__01_full_removal_backend.a1.md create mode 100644 .agent/reports/69_source_removal_cleanup/69_source_removal_cleanup__01_full_removal_backend.a1.validate create mode 100644 .agent/reports/69_source_removal_cleanup/69_source_removal_cleanup__02_confirmation_modal.a1.err create mode 100644 .agent/reports/69_source_removal_cleanup/69_source_removal_cleanup__02_confirmation_modal.a1.md create mode 100644 .agent/reports/69_source_removal_cleanup/69_source_removal_cleanup__02_confirmation_modal.a1.validate create mode 100644 .agent/reports/69_source_removal_cleanup/69_source_removal_cleanup__03_e2e_and_commit.a1.err create mode 100644 app/rag/source_removal.py create mode 100644 tests/e2e/test_source_removal_cleanup.py create mode 100644 tests/integration/test_source_removal_api.py create mode 100644 tests/unit/test_remove_confirm_modal.py create mode 100644 tests/unit/test_source_removal.py diff --git a/.agent/phases/complete/69_source_removal_cleanup/00_phase.md b/.agent/phases/complete/69_source_removal_cleanup/00_phase.md new file mode 100644 index 0000000..287a2e8 --- /dev/null +++ b/.agent/phases/complete/69_source_removal_cleanup/00_phase.md @@ -0,0 +1,149 @@ +# Phase 69 — Full Source Removal: Files, Index, and a Confirmation Modal + +**Source:** owner request (chat, 2026-09-02) — "Removing sources doesn't remove the +data from the filesystem or the rag… I need files cleaned up and the rag index +automatically synced. When I remove a source it should be totally removed. For that +reason, there should be a confirmation modal that pops up asking for confirmation if +the user clicks delete on a source." +**Story:** n/a (owner request from chat — full source removal, 2026-09-02) +**Context:** +- `app/api/git_sources.py` — `delete_git_source` is **row-only** today: its docstring + says "Removing does not touch the clones or the index — the next Sync + (`prune=True`) prunes the dropped repo (phase scope boundary)". The module's + "Scope boundary" paragraph repeats it. 404 (unknown id) / 422 (bad uuid) / 204 + pins. +- `app/rag/importer.py` — `_prune` deletes a source's `Document` rows whose files are + no longer walked; `Document.chunks` carries `cascade="all, delete-orphan"` + (`app/models.py:91`), so deleting a document drops every chunk row including its + pgvector embedding. Sync source naming: git rows index under + `repo_name(row.url)` (`scripts/import_docs.py`), local rows under + `Path(row.path or row.url).expanduser().name` — the exact expressions removal must + reuse to find the right documents. +- `app/api/sync.py::_run_sync` — the pipeline whose pieces removal reuses (not + re-implements): the `bump_sources_version` short-lived-session pattern + (`app/rag/sources_meta.py`, phase 53 saved-chat invalidation — any KB change, + including a pure prune, bumps exactly once) and the change-gated + `regenerate_overview` (`app/rag/overview.py`, phase 31 — best-effort by design: an + `LLMError` returns `False` with the previous row intact). +- On-disk layout: git checkouts live under `settings.sources_dir` + (`~/bor-sources//`), unpacked uploads under `settings.upload_dir` + (`~/bor-sources/uploads//`) — both **app-managed**. A `kind='local'` row may + also point at any owner directory; those are **not** app-managed and must never be + deleted (the page no longer offers the local-dir form — phase 49 — but the API + still accepts it, and such rows can exist). +- `frontend/assets/git-sources.js` — the remove flow calls `window.confirm` (the + **only** `window.confirm` in the frontend); module docstring bullets "remove" + (L69–76) and "Scope boundary" (L82–87) carry the stale "prunes on the next sync" + copy. `frontend/git-sources.html` — `#git-sources-hint` (L237–243) carries the same + stale copy. +- `tests/e2e/test_git_sources_admin.py` — test 4 pins the remove lifecycle through a + Playwright `page.on("dialog")` handler (accept → one DELETE; dismiss → none). +- `tests/integration/test_git_sources_api.py` — the existing DELETE pins + (`test_delete_removes_row_and_falls_back_to_env`, `test_delete_unknown_id_returns_404`, + `test_delete_invalid_id_returns_422`). `tests/integration/test_git_sources_upload.py` + is the house pattern for pointing the router at tmp dirs (the `_point_at` + monkeypatch) and faking the LLM (`FakeEmbedder`, spied `import_sources`). +- `README.md` — git-sources section (~L129–142: "Adding/removing does not clone…") + and local-sources section (~L404–410: removal semantics) carry the stale contract. + +## Objective +Removing a source (admin page or API) is a **total removal**: the stored row, every +indexed document of that source (chunks + embeddings), and — for app-managed sources +— the files on disk (the git checkout or the unpacked upload folder), all in one +action. The page confirms the removal first through an accessible, in-app modal +(replacing `window.confirm`) that spells out exactly what will be deleted. + +## Dependencies +- `68_search_tool` (complete; ordering by number — no functional dependency). +- Functional foundations, all complete: `28_git_based_sources` / phase 35 (sources + registry + CRUD), `38_local_directory_sources` (local rows), phase 49/`64_sync_upload_progress` + (archive uploads + upload dir), phase 32 (sync + `prune=True`), `53_stale_saved_chats` + (sources version), phase 31 (KB overview). + +## Tasks +1. `01_full_removal_backend.md` — the `app/rag/source_removal.py` helper (source-name + resolver, managed-dir mapping, sibling guard, disk removal) and the rewired + `DELETE /api/git-sources/{id}` (row + index + managed files + version bump + + best-effort overview). +2. `02_confirmation_modal.md` — the accessible confirmation modal on + `/git-sources.html` (replaces `window.confirm`), the updated hint-box + docstring + copy, frontend unit pins. +3. `03_e2e_and_commit.md` — the dedicated E2E suite (modal → API → disk + DB), + `test_git_sources_admin.py` updated to the modal, README copy, full gates, commit. + +## Testing & Quality +- Unit: `tests/unit/test_source_removal.py` — the resolver (git URL shapes incl. + `.git` suffix + scp-style `git@`, `~` expansion), the managed-dir mapping (git / + upload-under-root / foreign-local → `None`; the containment check so a sibling + named `uploads-foo` never counts as under `upload_dir`), disk removal (absent dir + no-op, present dir removed, `OSError` logged not fatal), the sibling-guard + decision. +- Unit (frontend): `tests/unit/test_remove_confirm_modal.py` — `window.confirm` gone + from `git-sources.js`; the dialog ids + `role="alertdialog"` + aria wiring + + Esc/Cancel/focus-return wiring present; the stale "stays indexed until the next + sync" copy gone from `git-sources.html`/`.js`; the new hint copy present. +- Integration: `tests/integration/test_source_removal_api.py` — the full-removal + matrix (task 01 step 3), tmp dirs + faked/spied LLM per the + `test_git_sources_upload.py` patterns; existing 404/422/204 pins stay green. +- E2E (mandatory, house rule): `tests/e2e/test_source_removal_cleanup.py`, run in + isolation — the modal flow end-to-end for uploaded, git, and local-directory + sources, incl. disk assertions (same-host `pathlib` against the settings-resolved + dirs) and the cancel/Esc paths. +- Coverage: **>90%** on `app/` (validate.sh gate). + +## Completion Criteria +- [ ] `DELETE /api/git-sources/{id}` removes the row **and** prunes the source's + documents (+chunks/embeddings) in one commit, deletes the app-managed on-disk + dir when present (git checkout / upload folder), bumps `sources_version` when + docs were pruned, and best-effort-regenerates the overview when pruned > 0. + 404/422/204 pins unchanged. Foreign local directories are never touched. + Sibling rows sharing a source name keep their documents and files. +- [ ] `/git-sources.html` removal is a page-local `role="alertdialog"` modal (no + `window.confirm` anywhere in `frontend/`): names the source, states the + removal policy, Cancel/Esc/backdrop cancel, "Removing…" lifecycle, in-modal + `role="alert"` error, focus return to the trigger, WCAG 2.1 AA basics + (labelled, focus-visible, contrast ≥4.5:1, ≥44px targets). +- [ ] `uv run pytest` green; `uv run pytest --cov=app` TOTAL **>90%**; + `uv run ruff check . && uv run pyright` clean. +- [ ] `uv run pytest tests/e2e/test_source_removal_cleanup.py -v --no-cov` green in + isolation (DB up); regression suites green in isolation: + `test_git_sources_admin.py`, `test_archive_upload_sources.py`. +- [ ] README removal-semantics copy updated (git-sources + local-sources sections). +- [ ] One `--no-gpg-sign` commit (message in the Commit block); phase dir moved to + `.agent/phases/complete/`. + +## Locked decisions +- **Owner (chat, 2026-09-02):** removal is a total removal — row + RAG index + + app-managed files, **immediately** (not deferred to the next sync); and a real + confirmation modal (not `window.confirm`) pops up when Remove is clicked, stating + what will be deleted before it happens. +- **DB first, disk second.** The row + document prune commit atomically first (the + RAG is always consistent with the registry — this is the owner's core ask); the + disk removal runs **after** the commit, and an `OSError` is logged + (`logger.exception`) but does not fail the 204. A leftover dir is inert (no row → + never imported) and self-heals on re-add (git re-clones — + `clone_or_pull` clones when `.git` is absent; a re-upload recreates the folder). + The reverse order is forbidden: a disk failure must never leave a row pointing at + deleted files. +- **App-managed files only.** Removal deletes `sources_dir//` for git rows and + the stored dir for local rows **when it is `upload_dir` itself or nested under it** + (containment via resolved paths + `parents`, so `…/uploads-foo` never counts). Any + other `kind='local'` path — the owner's own directory — is never touched on disk; + only its row + index entries are removed. +- **Sibling guard.** If another stored row resolves to the same source name (e.g. + `https://e.com/r` and `https://e.com/r.git` → both `r`), only the row is deleted — + the shared documents and files still belong to the sibling. Logged loudly. +- **Version bump + overview gates.** `sources_version` bumps exactly once when + pruned > 0 (the phase-53 saved-chat invalidation gate, same as sync). Overview + regeneration runs when pruned > 0 — deliberately broader than sync's + added+updated gate, because a whole-source removal changes the KB's face — and is + best-effort: an LLM failure logs and never fails the delete (the next + added/updated change refreshes it, as today). +- **Contract preserved.** `DELETE` keeps 204 with no body (the UI success path and + the 404/422 pins are unchanged); the per-operation INFO log line (PLAN §9 / + AGENTS.md rule 10) carries the counts. + +## Commit +```bash +git add -A .agent/ app/ tests/ frontend/ README.md && git commit --no-gpg-sign -m "feat(sources): removing a source deletes its files and index entries behind a confirmation modal" +``` diff --git a/.agent/phases/complete/69_source_removal_cleanup/01_full_removal_backend.md b/.agent/phases/complete/69_source_removal_cleanup/01_full_removal_backend.md new file mode 100644 index 0000000..410605f --- /dev/null +++ b/.agent/phases/complete/69_source_removal_cleanup/01_full_removal_backend.md @@ -0,0 +1,115 @@ +# Task 01 — Full-Removal Backend: Row + Index + App-Managed Files + +**Phase:** `69_source_removal_cleanup` · **Story:** n/a (owner request from chat, 2026-09-02) + +## Objective +`DELETE /api/git-sources/{id}` becomes a total removal: the stored row, the source's +indexed documents (chunks + embeddings), the app-managed on-disk directory, the +sources-version bump, and a best-effort overview refresh — via a new unit-testable +helper module, keeping the 204/404/422 contract. + +## Work +1. `app/rag/source_removal.py` (new module — stdlib + `app`/`scripts` imports only, + **no FastAPI**, so it unit-tests with plain objects and `tmp_path`): + - `resolve_source_name(row: GitSource) -> str` — exactly how sync/importer label + documents: `kind='git'` → `repo_name(row.url)` (import from + `scripts.import_docs`, the phase-28 helper); `kind='local'` → + `Path(row.path or row.url).expanduser().name` (the same expression + `app/api/sync.py::_run_sync` walks). + - `managed_dir_for(row: GitSource, sources_dir: Path, upload_dir: Path) -> Path | None` + — git → `sources_dir.expanduser() / repo_name(row.url)`; local → the expanded + stored path **only when** it equals `upload_dir.expanduser()` or is nested under + it (containment via `.resolve()` + `Path.parents` — a sibling named + `uploads-foo` must never count); any other local path → `None` (owner's own + directory, never touched). + - `remove_managed_dir(directory: Path | None) -> bool` — `None` or absent → + `False` (no-op, no filesystem write); present → `shutil.rmtree(directory)`; + `OSError` → `logger.exception` + `False`. **Never raises.** Returns `True` + only when a dir was actually removed. + - `has_sibling(db: Session, row: GitSource) -> bool` — another `git_sources` + row (different id) whose `resolve_source_name` equals the row's (the table is + tiny — resolve in Python, no SQL trickery). +2. `app/api/git_sources.py::delete_git_source` — rewire, in this locked order: + 1. `db.get(GitSource, source_id)` → 404 `git source not found` (unchanged). + 2. `name = resolve_source_name(row)`; if `has_sibling(db, row)` → `logger.warning` + (names the row + the shared source name), delete **only the row**, commit, + 204 (sibling guard — the shared documents + files stay). + 3. **DB first:** in the request transaction, `db.delete(doc)` for every + `Document` with `source == name` (the `all, delete-orphan` cascade drops all + chunks incl. embeddings — `app/models.py:91`), count them (`docs_pruned`), + `db.delete(row)`, `db.commit()`. A DB failure propagates as 500 **before any + disk work** (the 204 contract below never sees a half-removal). + 4. **Disk second:** `files_removed = remove_managed_dir(managed_dir_for(row, + Path(get_settings().sources_dir), Path(get_settings().upload_dir)))`. + 5. When `docs_pruned > 0`: make the endpoint `async def` (the + `update_document_summary` precedent in `app/api/docs.py` — async endpoint, + sync `get_db` session, `LLMClient`) and wrap + `await regenerate_overview(LLMClient())` in `try/except Exception` (log; an + LLM outage never fails the 204; `regenerate_overview` is already + `LLMError`-safe), then the phase-53 pattern: a short-lived `SessionLocal()` + (open → `bump_sources_version(db)` → `db.commit()` → `db.close()` in + `finally`, exactly as `_run_sync` does) — order: overview **then** bump, + mirroring sync (the bump lands even if the best-effort overview fails). + 6. One INFO log line (PLAN §9 / AGENTS.md rule 10): + `source removed: kind=%s name=%s docs_pruned=%d files_removed=%s + overview=%s total_ms=%d` (`files_removed` is `yes|no|skipped` — `skipped` + for the sibling guard and for foreign local dirs). + - Keep the route contract/status: 204 `Response`, `Depends(require_admin)`, + uuid 422 pin (the `def` → `async def` change is the only signature delta). Update the endpoint docstring (total-removal semantics + the + locked order) and the module docstring's "Scope boundary" paragraph (removal + now performs the cleanup; Sync's `prune=True` stays for **upstream file + churn**, not for row removal). +3. `tests/integration/test_source_removal_api.py` (new) — house patterns from + `tests/integration/test_git_sources_upload.py`: `_point_at`-style monkeypatch of + the settings the router reads so `sources_dir`/`upload_dir` point at fresh + `tmp_path` dirs per test; `FakeEmbedder` / spied `regenerate_overview` where the + LLM would be hit; the real-DB `db` fixture (skip when Postgres is down — shared + conftest behavior). Seed rows via `SessionLocal` (the `test_git_sources_admin.py` + pattern) and seed `Document`/`Chunk` rows for prune assertions. Cases: + - git row + tmp checkout dir with a marker file + seeded doc → DELETE 204; row + gone from `GET /api/git-sources`; **dir gone from disk** (marker included); + source absent from `GET /api/docs` (admin); `sources_version` bumped + (`app.rag.sources_meta.current_sources_version`). + - git row, checkout dir **absent** → 204, no error (the no-op path). + - local row under the tmp `upload_dir` + seeded doc → 204; row + doc gone; + **upload folder gone from disk**. + - local row at a `tmp_path` dir **outside** `upload_dir` with a marker file + + seeded doc → 204; row + doc gone; **dir + marker file still present**. + - containment edge: `upload_dir = tmp/"u"`, local row at `tmp/"u-evil"` → + `managed_dir_for` is `None`, dir untouched (also pinned in the unit test). + - sibling guard: URLs `https://example.com/reese/r` and + `https://example.com/reese/r.git` (same `repo_name`) + docs under source `r` + + the shared checkout dir → delete the first: 204, row gone, **docs + dir + remain**; delete the second: 204, docs + dir **gone**. + - overview spy: pruned > 0 → called exactly once; a row with **no** docs → not + called; spy raising `LLMError` → still 204 (best-effort). + - version bump: docs pruned → bumped exactly once (the `sources_meta` counter); + no docs → not bumped. +4. `tests/integration/test_git_sources_api.py` — harden + `test_delete_removes_row_and_falls_back_to_env`: monkeypatch the router's + `sources_dir`/`upload_dir` at `tmp_path` (the row's example.com checkout dir + never exists, but the test must not depend on the operator's real + `~/bor-sources` being clean — zero risk of an rmtree aimed at a real checkout). + The 404/422 pins are untouched. + +## Testing & Quality +- Unit (new `tests/unit/test_source_removal.py`): `resolve_source_name` (https + `.git`, https bare, scp-style `git@host:repo.git`, local with `~` and with + trailing-slash paths); `managed_dir_for` (git mapping; upload dir itself; nested + upload; foreign local → `None`; the `u-evil` containment edge); + `remove_managed_dir` (None → False; absent → False; present tree → False-free + removal + True; a read-only-ish failure path if cheaply simulatable — otherwise + the `logger.exception` branch via a monkeypatched `shutil.rmtree` raising + `OSError`); `has_sibling` (same-name sibling → True; different → False; + self-excluded). +- Coverage: **>90%** on `app/rag/source_removal.py` and the modified endpoint. + +## Completion Criteria +- [ ] `uv run pytest tests/unit/test_source_removal.py tests/integration/test_source_removal_api.py -v --no-cov` + green (DB up). +- [ ] `uv run pytest` green (existing 404/422/204 DELETE pins included); + `uv run pytest --cov=app` TOTAL >90%; `uv run ruff check . && uv run pyright` + clean. +- [ ] DELETE follows the locked order (row + prune committed before any disk + work; sibling guard; best-effort overview; one bump when pruned > 0). +- [ ] No behavior change in sync / upload / import (their suites green untouched). diff --git a/.agent/phases/complete/69_source_removal_cleanup/02_confirmation_modal.md b/.agent/phases/complete/69_source_removal_cleanup/02_confirmation_modal.md new file mode 100644 index 0000000..59ca532 --- /dev/null +++ b/.agent/phases/complete/69_source_removal_cleanup/02_confirmation_modal.md @@ -0,0 +1,122 @@ +# Task 02 — Confirmation Modal on the Git Sources Page + +**Phase:** `69_source_removal_cleanup` · **Story:** n/a (owner request from chat, 2026-09-02) + +## Objective +Replace the `window.confirm` remove flow on `/git-sources.html` with an accessible, +in-app confirmation modal that names the source and states the full-removal policy, +and update every stale "prunes on the next sync" copy the phase supersedes. + +## Work +1. `frontend/git-sources.html` — static modal markup inside `#git-sources-content` + (hidden by default; static markup so E2E gets stable selectors — the + `#git-sources-hint` / gate markup convention): + - `#remove-confirm-dialog` — `role="alertdialog"`, `aria-modal="true"`, + `aria-labelledby="remove-confirm-title"`, `aria-describedby="remove-confirm-copy"`, + `hidden` initially, containing: + - `#remove-confirm-title` (`

`): "Remove this source?" + - `#remove-confirm-source` — a `` for the source's value (git URL or local + path; **always set via `textContent` — the credential-masking discipline: + URLs may embed `user:pass@`**, phase 32). + - `#remove-confirm-copy` — a `

` with the locked policy text (below). + - `#remove-confirm-error` — a `

`, `hidden` initially. + - `#remove-confirm-cancel` (` + + + + diff --git a/tests/e2e/test_git_sources_admin.py b/tests/e2e/test_git_sources_admin.py index be0453a..f1ee160 100644 --- a/tests/e2e/test_git_sources_admin.py +++ b/tests/e2e/test_git_sources_admin.py @@ -35,8 +35,10 @@ Contract under test: render as a full-width table (mono URL, added date, per-row Remove) with the env note hidden while the DB has rows; add (201 → row, input cleared, button re-enabled), duplicate (inline role=alert, no new - row), invalid shape (inline 422, no new row), remove (confirm → gone; - cancel → stays); with the table truncated the env rows render with + row), invalid shape (inline 422, no new row), remove (the + confirmation modal: accept → gone; cancel → stays — the native + confirm() retired, phase 69); with the table truncated the env rows + render with "from .env" tags and ``#git-sources-env-note`` visible; * the page a11y / no-CDN basics (UI Structure Check, AGENTS.md rule 5): landmarks, labeled form control, full-width table, ≥44px targets, @@ -64,7 +66,7 @@ from pathlib import Path from typing import Any import pytest -from playwright.sync_api import Dialog, Page, expect +from playwright.sync_api import Page, expect from sqlalchemy import text from app.db import SessionLocal @@ -425,42 +427,43 @@ def test_admin_add_then_remove_lifecycle( expect(add_btn).to_be_enabled() expect(add_btn).to_have_text("Add source") - # --- remove: accept the confirm → the row disappears -------------- - dialog_action: dict[str, bool] = {"accept": True} + # --- remove: accept the confirmation modal → the row disappears --- + # (phase 69: the native confirm() is retired — a row's Remove opens + # the page-local alertdialog; "Remove source" accepts, Cancel + # cancels without a request.) deletes: list[str] = [] - def handle_dialog(dialog: Dialog) -> None: - if dialog_action["accept"]: - dialog.accept() - else: - dialog.dismiss() - def track_delete(r: Any) -> None: if r.method == "DELETE" and "/api/git-sources/" in r.url: deletes.append(r.url) - page.on("dialog", handle_dialog) page.on("request", track_delete) try: _row(page, NEW_REPO_URL).locator(".git-source-remove").click() + dialog = page.locator("#remove-confirm-dialog") + expect(dialog).to_be_visible(timeout=30_000) + expect(page.locator("#remove-confirm-source")).to_have_text(NEW_REPO_URL) + page.locator("#remove-confirm-remove").click() expect(page.locator("#git-sources-tbody tr")).to_have_count(1, timeout=30_000) expect(_row(page, NEW_REPO_URL)).to_have_count(0) + expect(dialog).to_be_hidden() # The seed row survived — and exactly one DELETE went out. expect(_row(page, SEED_URL)).to_have_count(1) assert len(deletes) == 1, f"expected one DELETE, saw: {deletes}" - # --- remove: cancel the confirm → the row stays, NO DELETE ---- - dialog_action["accept"] = False + # --- remove: cancel the modal → the row stays, NO DELETE ------ _row(page, SEED_URL).locator(".git-source-remove").click() - # Wait for the dialog round-trip to settle (dismiss → the JS - # returns without fetching) so the "no second DELETE" claim is + expect(page.locator("#remove-confirm-dialog")).to_be_visible(timeout=30_000) + page.locator("#remove-confirm-cancel").click() + expect(page.locator("#remove-confirm-dialog")).to_be_hidden() + # Wait for the cancel round-trip to settle (Cancel closes the + # dialog without fetching) so the "no second DELETE" claim is # made on a settled page. page.wait_for_timeout(500) assert len(deletes) == 1, f"canceled removal still deleted: {deletes}" expect(_row(page, SEED_URL)).to_have_count(1) expect(page.locator("#git-sources-tbody tr")).to_have_count(1) finally: - page.remove_listener("dialog", handle_dialog) page.remove_listener("request", track_delete) diff --git a/tests/e2e/test_source_removal_cleanup.py b/tests/e2e/test_source_removal_cleanup.py new file mode 100644 index 0000000..b680388 --- /dev/null +++ b/tests/e2e/test_source_removal_cleanup.py @@ -0,0 +1,738 @@ +"""Phase 69 story E2E (Playwright): total source removal — the +confirmation modal, the row + index prune, and the app-managed disk +cleanup (``/git-sources.html``). + +Story: n/a (owner request from chat, 2026-09-02) +Run in isolation (DB must be up: ``podman compose up -d db``): + + uv run pytest tests/e2e/test_source_removal_cleanup.py -v --no-cov + +The story gate for TOTAL source removal (phase 69): removing a source +removes, in one action, the stored row, every indexed document of that +source (chunks + embeddings), and — for app-managed sources — the files +on disk (the git checkout under ``BOR_SOURCES_DIR`` or the unpacked +upload folder under ``BOR_UPLOAD_DIR``), immediately (not deferred to +the next sync). The page confirms first through a page-local +``role="alertdialog"`` modal (the native ``confirm()`` is retired) that +names the source and states the removal policy; Cancel/Escape close +without a request, and only "Remove source" sends the DELETE. + +**Disk paths are resolved exactly like the app**: +``Path(get_settings().sources_dir).expanduser()`` / +``Path(get_settings().upload_dir).expanduser()`` — the E2E conftest +does NOT override those two vars and pytest runs with ``cwd=REPO``, so +the test process and the app subprocess resolve the same ``.env`` +(``BOR_SOURCES_DIR`` / ``BOR_UPLOAD_DIR``) and the same-host ``pathlib`` +assertions hit the very directories the DELETE handler cleans. + +The suite triggers **no sync** (the git rows are ``example.com`` URLs +that are never cloned); the only real artifact is the API-driven +upload of one small archive with a unique name (``phase69-<8-hex>.tar.gz``, +one ``.md`` file) — its background scan runs the mock-LLM pipeline +(no network beyond the app itself). Seeded ``Document`` rows +(``SessionLocal``, the ``test_git_sources_admin.py`` pattern) give the +prune assertions a deterministic KB. + +Per-module app env (the conftest pattern, module-scoped): the same env +shape as ``test_git_sources_admin.py`` with ``BOR_GIT_SOURCES`` forced +empty (a dev ``.env`` fallback URL must never render as an env row on +the table); **no** ``BOR_SOURCES_DIR`` / ``BOR_UPLOAD_DIR`` override +(deliberate — see the disk-path note above). + +Contract under test: + +* **upload → modal → total removal**: the uploaded folder exists on + disk and its document is in ``GET /api/docs``; the row's Remove → + the alertdialog opens (``#remove-confirm-source`` = the upload path, + focus on ``#remove-confirm-cancel``) → "Remove source" → the + "Removing…" in-flight state (both buttons disabled) → settled: the + row is gone, the document is pruned, **the folder is gone from + disk**, exactly one DELETE went out, and the announcer carries the + success line; +* **git checkout removal**: a seeded git row + a hand-made checkout + dir (marker file) + a seeded document → modal removal → the row is + gone, **the checkout dir is gone from disk** (marker included) and + the document is pruned; +* **no-checkout no-op**: a git row with NO checkout dir (never synced) + removes cleanly — the absent-dir path, no error state anywhere on + the page; +* **foreign local directories are never touched**: a seeded + ``kind='local'`` row pointing at a test-owned dir (marker file) + a + seeded document → modal removal → the row is gone and the document + is pruned, **but the dir + marker file are still present**; +* **cancel + Escape keep everything**: Cancel click → dialog hidden, + focus back on the trigger button, **zero DELETE requests**, row + + document remain; re-open → Escape → the same; +* **modal a11y + no CDN** (UI Structure Check, AGENTS.md rule 5 + + rule 6): the dialog's aria attributes, the 3px ``:focus-visible`` + outline computable on BOTH buttons (keyboard focus), both buttons + ≥44px tall, the success line on ``#git-sources-announcer`` + (role=status) after a real removal, and the page loads only + same-origin resources. + +Test → story mapping (Playwright Mapping Rule): +1. ``test_uploaded_source_removal_cleans_index_and_disk`` +2. ``test_git_source_removal_removes_checkout_and_index`` +3. ``test_git_source_removal_without_checkout_succeeds`` +4. ``test_local_directory_source_files_never_deleted`` +5. ``test_remove_modal_cancel_and_esc_keep_everything`` +6. ``test_remove_modal_a11y_and_no_cdn`` +""" +from __future__ import annotations + +import hashlib +import io +import os +import shutil +import subprocess +import sys +import tarfile +import time +import uuid +from collections.abc import Iterator +from pathlib import Path +from typing import Any + +import pytest +from playwright.sync_api import Page, Route, expect +from sqlalchemy import text + +from app.config import get_settings +from app.db import SessionLocal +from app.models import Document, GitSource +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}" + +GIT_SOURCES_URL = "/git-sources.html" + +#: The success line the page announces on a total removal (the +#: git-sources.js announce() string). +REMOVAL_ANNOUNCE = "Source removed — its files and index entries were cleaned up." + +#: Deterministic, never-cloned example.com URLs — the suite never +#: triggers a sync, so the only on-disk artifact a git row can have is +#: the checkout dir this suite itself creates (test 2). +GONE_URL = "https://example.com/reese/phase69-gone.git" +GONE_REPO = "phase69-gone" # repo_name(GONE_URL) +NO_CHECKOUT_URL = "https://example.com/reese/phase69-nodir.git" +KEPT_URL = "https://example.com/reese/phase69-kept.git" +KEPT_REPO = "phase69-kept" +A11Y_URL = "https://example.com/reese/phase69-a11y.git" +A11Y_REPO = "phase69-a11y" + +#: The upload sentinel — one markdown file in the unique archive. +UPLOAD_SENTINEL = "PHASE69-UPLOAD-9c2d" + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(scope="module") +def app_server(mock_llm: int) -> Iterator[str]: + """The real app under test — per-module env: ``BOR_GIT_SOURCES`` + forced empty (no env fallback rows on the table). Deliberately NO + ``BOR_SOURCES_DIR`` / ``BOR_UPLOAD_DIR`` override: the disk + assertions must resolve the dirs EXACTLY like the app (the module + docstring explains why the same ``.env`` resolves on both sides).""" + env = dict(os.environ) + env.pop("DEBUGPY", None) + env["BOR_ENVIRONMENT"] = "e2e" + env["BOR_STATIC_DIR"] = str(REPO / "frontend") + env["BOR_LLM_BASE_URL"] = ( + "https://aipi.reeseapps.com/v1" + if USE_REAL_LLM + else f"http://127.0.0.1:{mock_llm}/v1" + ) + # Mock-calibrated threshold (conftest pattern) — no chat turn is + # ever sent in this suite, but the app boots with the same env shape. + 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 + # No env fallback rows: the table state (seeded per test) is the + # only row source, so row-count assertions are deterministic. + env["BOR_GIT_SOURCES"] = "" + proc = subprocess.Popen( + [sys.executable, "-m", "uvicorn", "app.main:app", + "--host", "127.0.0.1", "--port", str(APP_PORT), "--log-level", "warning"], + cwd=REPO, + env=env, + ) + try: + _wait_http(f"{APP_URL}/api/health") + yield APP_URL + finally: + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + + +@pytest.fixture(scope="module") +def app_url(app_server: str) -> str: + return app_server + + +@pytest.fixture(scope="module") +def sources_dir() -> Path: + """The app's git-checkout root — resolved EXACTLY like the app + (same ``.env``, same ``cwd=REPO``; the conftest does not override + ``BOR_SOURCES_DIR``).""" + return Path(get_settings().sources_dir).expanduser() + + +@pytest.fixture(scope="module") +def upload_dir() -> Path: + """The app's upload root — resolved EXACTLY like the app (see + ``sources_dir``).""" + return Path(get_settings().upload_dir).expanduser() + + +def _truncate_all() -> None: + """Fresh registry + KB per test (the E2E isolation pattern, the + ``test_archive_upload_sources.py`` set): the row/doc assertions + must be this test's own doing — a leftover document under one of + the deterministic source names would survive the prune and flip an + assertion.""" + with SessionLocal() as db: + db.execute(text("TRUNCATE chunks, documents, query_log, kb_overview, git_sources")) + db.commit() + + +@pytest.fixture(autouse=True) +def _clean(db_ready: None) -> Iterator[None]: + _truncate_all() + yield + _truncate_all() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _seed_git_row(url: str) -> None: + """Store a git row directly (deterministic — the + ``test_git_sources_admin.py`` SessionLocal seeding pattern).""" + with SessionLocal() as db: + db.add(GitSource(url=url, kind="git")) + db.commit() + + +def _seed_local_row(path: str) -> None: + """Store a kind=local row (the phase-38 shape: the expanded + absolute path in BOTH ``path`` and the NOT-NULL ``url``).""" + with SessionLocal() as db: + db.add(GitSource(url=path, kind="local", path=path)) + db.commit() + + +def _seed_doc(source: str, path: str, full_path: str, title: str, content: str) -> None: + """One indexed document (the prune subject) — a full row so the + cascade + the /api/docs listing behave exactly like a scanned doc.""" + with SessionLocal() as db: + db.add( + Document( + source=source, + path=path, + full_path=full_path, + title=title, + content=content, + content_hash=hashlib.sha256(content.encode("utf-8")).hexdigest(), + ) + ) + db.commit() + + +def _admin_git_sources_page(page: Page, app_url: str) -> None: + """Real form login landing on the git sources page (admin settled: + Sign out visible, the manager revealed by the page module).""" + login(page, app_url, next=GIT_SOURCES_URL) + expect(page).to_have_url(app_url + GIT_SOURCES_URL, timeout=30_000) + expect(page.locator("#sign-out-btn")).to_be_visible(timeout=15_000) + expect(page.locator("#git-sources-gate")).to_be_hidden() + expect(page.locator("#git-sources-content")).to_be_visible() + + +def _row(page: Page, value: str) -> Any: + """The table row whose mono cell shows ``value`` (git URL or + local path — the row's rendered value).""" + return page.locator("#git-sources-tbody tr", has_text=value) + + +def _docs(page: Page, app_url: str) -> list[tuple[str, str]]: + """``GET /api/docs`` as the signed-in page → sorted (source, path) + pairs (the admin cookie rides the browser context).""" + r = page.request.get(f"{app_url}/api/docs") + assert r.status == 200, r.text + return sorted((d["source"], d["path"]) for d in r.json()["documents"]) + + +def _stored_sources(page: Page, app_url: str) -> list[dict[str, Any]]: + """``GET /api/git-sources`` as the signed-in page → the row list.""" + r = page.request.get(f"{app_url}/api/git-sources") + assert r.status == 200, r.text + return r.json()["sources"] + + +def _build_targz(path: Path, files: dict[str, str]) -> Path: + """A deterministic ``.tar.gz`` (mtime 0) over the given files (the + ``test_archive_upload_sources.py`` pattern).""" + with tarfile.open(path, "w:gz") as tf: + for rel, content in files.items(): + data = content.encode("utf-8") + info = tarfile.TarInfo(rel) + info.size = len(data) + info.mtime = 0 + tf.addfile(info, io.BytesIO(data)) + return path + + +def _upload_and_wait_success(page: Page, app_url: str, archive: Path) -> dict[str, Any]: + """POST the archive through the logged-in page's request context + (the admin cookie rides along) and poll the phase-64 status + endpoint to ``success`` — returns the terminal status body.""" + r = page.request.post( + f"{app_url}/api/git-sources/upload", + multipart={ + "file": { + "name": archive.name, + "mimeType": "application/gzip", + "buffer": archive.read_bytes(), + } + }, + ) + assert r.status == 202, f"upload POST failed: {r.status} {r.text}" + deadline = time.monotonic() + 60.0 + body: dict[str, Any] = {} + while time.monotonic() < deadline: + r = page.request.get(f"{app_url}/api/git-sources/upload/status") + assert r.status == 200, r.text + body = r.json() + if body["state"] == "success": + return body + if body["state"] == "failed": + raise AssertionError(f"the upload scan failed: {body}") + time.sleep(0.2) + raise AssertionError(f"the upload scan never settled: {body}") + + +def _open_remove_modal(page: Page, value: str) -> None: + """Click the row's Remove for ``value`` and assert the opened + alertdialog: it names the source and focus lands on Cancel (the + safe default).""" + _row(page, value).locator(".git-source-remove").click() + dialog = page.locator("#remove-confirm-dialog") + expect(dialog).to_be_visible(timeout=15_000) + assert dialog.get_attribute("role") == "alertdialog" + expect(page.locator("#remove-confirm-source")).to_have_text(value) + assert page.evaluate("() => document.activeElement.id") == "remove-confirm-cancel" + + +def _confirm_removal(page: Page) -> None: + """Click "Remove source" and wait for the settled removal: the + (single) row is gone and the dialog is closed.""" + page.locator("#remove-confirm-remove").click() + expect(page.locator("#git-sources-tbody tr")).to_have_count(0, timeout=30_000) + expect(page.locator("#remove-confirm-dialog")).to_be_hidden() + + +def _hold_delete_requests(page: Page, hold_s: float) -> None: + """Intercept DELETEs to the git-sources API and hold the REQUEST in + the browser for ``hold_s`` seconds (the + ``test_archive_upload_sources.py`` ``_hold_upload_request`` + pattern) — the modal's "Removing…" in-flight state (§7.4) becomes + deterministic instead of racing the (fast) mock-LLM cleanup. + Every other request (the list GETs, the upload status poll) passes + through untouched.""" + + def handle(route: Route) -> None: + if route.request.method == "DELETE": + time.sleep(hold_s) + route.continue_() + + page.route("**/api/git-sources/**", handle) + + +# --------------------------------------------------------------------------- +# 1. Upload → modal → total removal: index pruned AND the upload +# folder is gone from disk +# --------------------------------------------------------------------------- + + +def test_uploaded_source_removal_cleans_index_and_disk( + page: Page, app_url: str, db_ready: None, upload_dir: Path, tmp_path: Path +) -> None: + """A uniquely named archive uploaded through the API (202 → status + success): the folder exists on disk and its document is in the KB; + then the row's Remove → the alertdialog (the upload path named, + focus on Cancel) → "Remove source" → the "Removing…" in-flight + state → settled: row gone, document pruned, **the folder is gone + from disk**, one DELETE, the announcer's success line.""" + page.set_default_timeout(30_000) + # Unique per run — never collides with a real (or a crashed-run's) + # upload, so the disk assertions are safe on the shared dir. + name = f"phase69-{uuid.uuid4().hex[:8]}" + archive = _build_targz( + tmp_path / f"{name}.tar.gz", + { + "note.md": ( + "# Upload note\n" + "\n" + f"Phase 69 removal subject. Marker: {UPLOAD_SENTINEL}\n" + ) + }, + ) + + _admin_git_sources_page(page, app_url) + + # The API-driven upload (202) + the background scan (success). + status = _upload_and_wait_success(page, app_url, archive) + assert status["detail"]["source"] == name + assert status["detail"]["added"] == 1 + + # Preconditions — the artifact is real: the folder on disk (the + # app's resolved upload dir — this process resolved the same one), + # the document in the KB, the row in the registry. + folder = upload_dir / name + assert (folder / "note.md").is_file(), f"{folder}/note.md missing on disk" + assert _docs(page, app_url) == [(name, "note.md")] + r = page.request.get(f"{app_url}/api/git-sources") + assert r.status == 200, r.text + assert [(s["kind"], s["path"]) for s in r.json()["sources"]] == [ + ("local", str(folder)) + ] + + # The page's table still shows the pre-upload (empty) state — the + # upload went through the API, not the form (whose 202-success + # path would have called loadSources): a reload runs the page + # module's loadSources() and the new row lands. + page.reload() + expect(page.locator("#sign-out-btn")).to_be_visible(timeout=15_000) + expect(page.locator("#git-sources-content")).to_be_visible() + row = _row(page, str(folder)) + expect(row).to_have_count(1, timeout=15_000) + expect(row.locator("span.git-source-kind")).to_have_text("Local") + + # Track the DELETEs; hold the one the modal sends so the in-flight + # state is observable deterministically. + deletes: list[str] = [] + page.on( + "request", + lambda r: deletes.append(r.url) + if r.method == "DELETE" and "/api/git-sources/" in r.url + else None, + ) + _hold_delete_requests(page, hold_s=0.8) + + # The modal: the upload path is named (textContent — the row's + # value), focus on Cancel. + _open_remove_modal(page, str(folder)) + + # "Remove source" → the §7.4 in-flight state (both buttons + # disabled, the confirm relabeled) while the held DELETE is out. + page.locator("#remove-confirm-remove").click() + expect(page.locator("#remove-confirm-remove")).to_have_text("Removing…") + expect(page.locator("#remove-confirm-remove")).to_be_disabled() + expect(page.locator("#remove-confirm-cancel")).to_be_disabled() + expect(page.locator("#remove-confirm-error")).to_be_hidden() + + # Settled: the row is gone, the dialog closed and reset… + expect(page.locator("#git-sources-tbody tr")).to_have_count(0, timeout=30_000) + expect(page.locator("#remove-confirm-dialog")).to_be_hidden() + expect(page.locator("#remove-confirm-remove")).to_have_text("Remove source") + expect(page.locator("#remove-confirm-remove")).to_be_enabled() + # …the announcer carries the success line… + expect(page.locator("#git-sources-announcer")).to_have_text(REMOVAL_ANNOUNCE) + # …and exactly one DELETE went out. + assert len(deletes) == 1, f"expected one DELETE, saw: {deletes}" + + # The TOTAL removal, proven outside the UI: the registry is empty, + # the document is pruned from the KB, and the folder is GONE from + # the app's upload dir (same-host pathlib, the app's resolved dir). + assert _stored_sources(page, app_url) == [] + assert _docs(page, app_url) == [] + assert not folder.exists(), f"{folder} still on disk after removal" + + +# --------------------------------------------------------------------------- +# 2. Git row → the checkout dir is removed from disk + the index pruned +# --------------------------------------------------------------------------- + + +def test_git_source_removal_removes_checkout_and_index( + page: Page, app_url: str, db_ready: None, sources_dir: Path +) -> None: + """A seeded git row + a hand-made checkout dir (marker file, the + sync's clone target layout) + a seeded document: the modal removal + leaves the row gone, **the checkout dir gone from disk** (marker + included), and the document pruned from the KB.""" + page.set_default_timeout(30_000) + checkout = sources_dir / GONE_REPO + shutil.rmtree(checkout, ignore_errors=True) # defensive: no leftover + (checkout / "notes").mkdir(parents=True) + (checkout / "notes" / "readme.md").write_text( + f"# Readme\n\nCheckout marker for {GONE_REPO}.\n", encoding="utf-8" + ) + _seed_git_row(GONE_URL) + _seed_doc( + GONE_REPO, + "notes/readme.md", + str(checkout / "notes" / "readme.md"), + "Readme", + "Readme content for the phase69 checkout-removal test.", + ) + + _admin_git_sources_page(page, app_url) + expect(page.locator("#git-sources-tbody tr")).to_have_count(1) + assert _docs(page, app_url) == [(GONE_REPO, "notes/readme.md")] + + _open_remove_modal(page, GONE_URL) + _confirm_removal(page) + + # Row gone from the registry… + assert _stored_sources(page, app_url) == [] + # …the checkout dir is GONE from disk (the marker with it)… + assert not checkout.exists(), f"{checkout} still on disk after removal" + # …and the document is pruned from the KB. + assert _docs(page, app_url) == [] + + +# --------------------------------------------------------------------------- +# 3. Git row, no checkout dir (never synced) → clean no-op removal +# --------------------------------------------------------------------------- + + +def test_git_source_removal_without_checkout_succeeds( + page: Page, app_url: str, db_ready: None, sources_dir: Path +) -> None: + """The absent-dir path: a seeded git row whose checkout dir does + not exist (the repo was never cloned/synced) still removes + cleanly — row gone, and NO error state anywhere on the page.""" + page.set_default_timeout(30_000) + shutil.rmtree(sources_dir / "phase69-nodir", ignore_errors=True) # ensure absent + _seed_git_row(NO_CHECKOUT_URL) + + _admin_git_sources_page(page, app_url) + expect(page.locator("#git-sources-tbody tr")).to_have_count(1) + + _open_remove_modal(page, NO_CHECKOUT_URL) + _confirm_removal(page) + + assert _stored_sources(page, app_url) == [] + # No error state anywhere on the page (in-modal, load, or announcer + # — the announcer carries the success line, not an error). + expect(page.locator("#remove-confirm-error")).to_be_hidden() + expect(page.locator("#git-sources-load-error")).to_be_hidden() + expect(page.locator("#git-source-error")).to_be_hidden() + expect(page.locator("#git-sources-announcer")).to_have_text(REMOVAL_ANNOUNCE) + + +# --------------------------------------------------------------------------- +# 4. Foreign local directory → row + index go, the files NEVER do +# --------------------------------------------------------------------------- + + +def test_local_directory_source_files_never_deleted( + page: Page, app_url: str, db_ready: None, tmp_path: Path +) -> None: + """A seeded kind=local row pointing at the owner's OWN directory + (outside the app-managed roots) + a seeded document: the modal + removal deletes the row and prunes the document, **but the dir and + its marker file are still present** (locked decision: foreign + local directories are never touched on disk).""" + page.set_default_timeout(30_000) + user_dir = tmp_path / "my-notes" + user_dir.mkdir() + marker = user_dir / "keep-me.md" + marker.write_text("The owner's own file — removal must not touch it.\n", encoding="utf-8") + _seed_local_row(str(user_dir)) + _seed_doc( + user_dir.name, + "keep-me.md", + str(marker), + "Keep me", + "Document of the foreign local dir (the prune subject).", + ) + + _admin_git_sources_page(page, app_url) + expect(page.locator("#git-sources-tbody tr")).to_have_count(1) + assert _docs(page, app_url) == [(user_dir.name, "keep-me.md")] + + _open_remove_modal(page, str(user_dir)) + _confirm_removal(page) + + # Row gone, document pruned… + assert _stored_sources(page, app_url) == [] + assert _docs(page, app_url) == [] + # …but the foreign directory + its file are STILL on disk. + assert user_dir.is_dir(), f"{user_dir} was deleted — foreign dirs are never touched" + assert marker.is_file(), f"{marker} was deleted — foreign files are never touched" + + +# --------------------------------------------------------------------------- +# 5. Cancel + Escape → zero DELETEs, everything stays +# --------------------------------------------------------------------------- + + +def test_remove_modal_cancel_and_esc_keep_everything( + page: Page, app_url: str, db_ready: None +) -> None: + """(a) Remove → modal → Cancel: the dialog closes, focus returns + to the row's Remove button, **zero DELETE requests**, row + doc + remain. (b) Re-open → Escape: the same (the keyboard cancel path — + the request tracker stays empty through both).""" + page.set_default_timeout(30_000) + _seed_git_row(KEPT_URL) + _seed_doc( + KEPT_REPO, + "a.md", + f"/nonexistent-but-fine/{KEPT_REPO}/a.md", + "A", + "Document that must survive both cancels.", + ) + + deletes: list[str] = [] + page.on( + "request", + lambda r: deletes.append(r.url) + if r.method == "DELETE" and "/api/git-sources/" in r.url + else None, + ) + + _admin_git_sources_page(page, app_url) + expect(page.locator("#git-sources-tbody tr")).to_have_count(1) + assert _docs(page, app_url) == [(KEPT_REPO, "a.md")] + + # (a) Cancel click — no request, focus back on the trigger. + _open_remove_modal(page, KEPT_URL) + page.locator("#remove-confirm-cancel").click() + expect(page.locator("#remove-confirm-dialog")).to_be_hidden() + # Focus returned to the row's Remove button (the trigger)… + assert page.evaluate( + "() => document.activeElement.classList.contains('git-source-remove')" + ), "focus did not return to the row's Remove button" + # Let the page settle (a cancel sends nothing) before the + # "zero DELETEs" claim. + page.wait_for_timeout(500) + assert deletes == [], f"cancel sent a request: {deletes}" + expect(page.locator("#git-sources-tbody tr")).to_have_count(1) + assert _docs(page, app_url) == [(KEPT_REPO, "a.md")] + + # (b) Escape — the keyboard cancel path, same guarantees. + _open_remove_modal(page, KEPT_URL) + page.keyboard.press("Escape") + expect(page.locator("#remove-confirm-dialog")).to_be_hidden() + assert page.evaluate( + "() => document.activeElement.classList.contains('git-source-remove')" + ), "focus did not return to the row's Remove button after Escape" + page.wait_for_timeout(500) + assert deletes == [], f"Escape sent a request: {deletes}" + expect(page.locator("#git-sources-tbody tr")).to_have_count(1) + expect(_row(page, KEPT_URL)).to_have_count(1) + assert _docs(page, app_url) == [(KEPT_REPO, "a.md")] + + +# --------------------------------------------------------------------------- +# 6. Modal a11y (AGENTS.md rule 5) + no CDN (rule 6) + the success +# announcer after a real removal +# --------------------------------------------------------------------------- + + +def test_remove_modal_a11y_and_no_cdn(page: Page, app_url: str, db_ready: None) -> None: + """The dialog's aria wiring (alertdialog + labelled + described), + the 3px ``:focus-visible`` outline computable on BOTH buttons + (keyboard focus), both buttons ≥44px tall, the success line on + ``#git-sources-announcer`` (role=status) after a real removal, and + the page loading only same-origin resources (rule 6).""" + page.set_default_timeout(30_000) + _seed_git_row(A11Y_URL) + _seed_doc( + A11Y_REPO, + "a.md", + f"/nonexistent-but-fine/{A11Y_REPO}/a.md", + "A", + "Document removed by the a11y test's successful removal.", + ) + + _admin_git_sources_page(page, app_url) + expect(page.locator("#git-sources-tbody tr")).to_have_count(1) + + # The static dialog wiring (the markup ships with the page). + dialog = page.locator("#remove-confirm-dialog") + assert dialog.get_attribute("role") == "alertdialog" + assert dialog.get_attribute("aria-modal") == "true" + assert dialog.get_attribute("aria-labelledby") == "remove-confirm-title" + assert dialog.get_attribute("aria-describedby") == "remove-confirm-copy" + expect(page.locator("#remove-confirm-title")).to_have_text("Remove this source?") + assert page.locator("#remove-confirm-error").get_attribute("role") == "alert" + expect(page.locator("#remove-confirm-error")).to_be_hidden() + expect(page.locator("#remove-confirm-copy")).to_contain_text("permanently removes") + + # Open: focus lands on Cancel (the safe default)… + _row(page, A11Y_URL).locator(".git-source-remove").click() + expect(dialog).to_be_visible(timeout=15_000) + assert page.evaluate("() => document.activeElement.id") == "remove-confirm-cancel" + + # …and KEYBOARD focus moves between the two buttons, each drawing + # the house 3px :focus-visible outline (Tab Cancel→Remove, + # Shift+Tab back). + page.keyboard.press("Tab") + assert page.evaluate("() => document.activeElement.id") == "remove-confirm-remove" + outline = page.evaluate( + "() => getComputedStyle(document.activeElement).outlineWidth" + ) + assert outline == "3px", f"focus-visible outline missing on Remove: {outline!r}" + page.keyboard.press("Shift+Tab") + assert page.evaluate("() => document.activeElement.id") == "remove-confirm-cancel" + outline = page.evaluate( + "() => getComputedStyle(document.activeElement).outlineWidth" + ) + assert outline == "3px", f"focus-visible outline missing on Cancel: {outline!r}" + + # Touch targets ≥44px (both buttons). + for btn in ("#remove-confirm-cancel", "#remove-confirm-remove"): + box = page.locator(btn).bounding_box() + assert box is not None and box["height"] >= 44, f"target too small: {box}" + + # A real removal: the success line lands in the announcer + # (role=status, aria-live=polite). + page.locator("#remove-confirm-remove").click() + expect(page.locator("#git-sources-tbody tr")).to_have_count(0, timeout=30_000) + expect(dialog).to_be_hidden() + announcer = page.locator("#git-sources-announcer") + assert announcer.get_attribute("role") == "status" + assert announcer.get_attribute("aria-live") == "polite" + expect(announcer).to_have_text(REMOVAL_ANNOUNCE) + assert _stored_sources(page, app_url) == [] + assert _docs(page, app_url) == [] + + # No CDN (rule 6): no https:// asset tags; every script/link ref is + # same-origin or a data: URI (the test_git_sources_admin.py pin). + html = page.content() + assert 'src="https://' not in html and 'href="https://' not in html + refs = page.evaluate( + """() => [...document.querySelectorAll("script[src], link[href]")] + .map((el) => el.src || el.href)""" + ) + assert refs, "expected local asset references" + for ref in refs: + assert ref.startswith(app_url) or ref.startswith("data:"), ( + f"non-local asset reference: {ref}" + ) diff --git a/tests/integration/test_git_sources_api.py b/tests/integration/test_git_sources_api.py index bdb260f..87d84f9 100644 --- a/tests/integration/test_git_sources_api.py +++ b/tests/integration/test_git_sources_api.py @@ -60,10 +60,19 @@ def clean_git_sources(db: Session) -> Iterator[None]: db.commit() -def _settings(git_sources: str = "") -> Settings: - """Fresh settings with the ``.env`` file ignored; the explicit kwarg - beats any process env leaks (test_sync_api pattern).""" - return Settings(_env_file=None, git_sources=git_sources) # pyright: ignore[reportCallIssue] +def _settings(git_sources: str = "", **overrides: object) -> Settings: + """Fresh settings with the ``.env`` file ignored; the explicit + kwargs beat any process env leaks (test_sync_api pattern). + + ``**overrides`` carries the per-test dir pins (``sources_dir`` / + ``upload_dir`` — the phase-69 total-removal DELETE must never aim a + rmtree at the operator's real ``~/bor-sources``). + """ + return Settings( + _env_file=None, # pyright: ignore[reportCallIssue] + git_sources=git_sources, + **overrides, # pyright: ignore[reportCallIssue] + ) # --- anonymous ------------------------------------------------------------- @@ -493,12 +502,23 @@ def test_db_rows_win_over_env(admin_client: TestClient, db: Session, monkeypatch def test_delete_removes_row_and_falls_back_to_env( - admin_client: TestClient, monkeypatch: pytest.MonkeyPatch + admin_client: TestClient, db: Session, monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: + # Phase 69: the DELETE is a total removal — the (absent) checkout + # dir it would rmtree is pinned at fresh tmp dirs, so the test never + # depends on the operator's real ``~/bor-sources`` being clean; the + # document prune it performs owns the KB tables the same way the + # other suites do (no row's source name may shadow real docs). + db.execute(text("TRUNCATE chunks, documents")) + db.commit() monkeypatch.setattr( git_sources_api, "get_settings", - lambda: _settings("https://env.example.com/env.git"), + lambda: _settings( + "https://env.example.com/env.git", + sources_dir=str(tmp_path / "sources"), + upload_dir=str(tmp_path / "uploads"), + ), ) created = admin_client.post("/api/git-sources", json={"url": "https://new.example.com/x.git"}) assert created.status_code == 201 @@ -518,6 +538,8 @@ def test_delete_removes_row_and_falls_back_to_env( "added_at": None, } ] + db.execute(text("TRUNCATE chunks, documents")) + db.commit() def test_delete_unknown_id_returns_404(admin_client: TestClient) -> None: diff --git a/tests/integration/test_source_removal_api.py b/tests/integration/test_source_removal_api.py new file mode 100644 index 0000000..921d599 --- /dev/null +++ b/tests/integration/test_source_removal_api.py @@ -0,0 +1,528 @@ +"""Integration: total source removal (phase 69, task 01). + +Real Postgres (``podman compose up -d db``); the settings the router +reads are monkeypatched at fresh ``tmp_path`` dirs per test (the +``test_git_sources_upload.py`` ``_point_at`` pattern — the dev ``.env`` +never leaks in, and no rmtree can ever aim at a real checkout or +upload), and the LLM is never hit: the endpoint's +``regenerate_overview`` becomes a counting spy (faked/spied per the +house pattern — the best-effort branch gets a spy that raises +``LLMError``). + +Contract under test — ``DELETE /api/git-sources/{id}`` is a total +removal (owner request 2026-09-02), in the locked order: + +* row + the source's documents (chunks + embeddings via the + ``all, delete-orphan`` cascade) commit **first**; the app-managed + on-disk dir (git checkout / unpacked upload folder) is deleted + **after** the commit — absent dir is a no-op, foreign local dirs + (the owner's own) are never touched on disk; +* sibling guard — another stored row resolving to the same source name + (``https://e.com/r`` vs ``https://e.com/r.git``) keeps the shared + documents + files: only the row goes; +* ``sources_version`` bumps exactly once when documents were pruned + (the phase-53 saved-chat invalidation, same gate as sync) — no docs, + no bump; +* the overview refresh runs when pruned > 0 and is best-effort: a + failing spy still lands the 204 (and the bump — overview first, bump + second, mirroring ``_run_sync``); +* the 204 no-body contract and the 404/422 pins stay unchanged (the + 404/422 pins live in ``test_git_sources_api.py``); +* one per-operation INFO line (PLAN §9) — + ``source removed: kind=… name=… docs_pruned=… files_removed=yes|no| + skipped overview=… total_ms=…`` (``skipped`` for the sibling guard + and for foreign local dirs). + +``git_sources`` / ``documents`` / ``chunks`` / ``kb_overview`` are +global state: reset around every test; the single-row ``sources_meta`` +counter resets to 0 (the ``test_sync_api.py`` pattern). +""" +from __future__ import annotations + +import logging +import re +import uuid +from collections.abc import Iterator +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import text +from sqlalchemy.orm import Session + +from app.api import git_sources as git_sources_api +from app.config import Settings +from app.models import Chunk, Document, GitSource +from app.rag.llm import LLMError + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(autouse=True) +def clean_state(db: Session) -> Iterator[None]: + """The registry + KB tables are global state: reset around every + test; the seeded ``sources_meta`` row resets to version 0.""" + db.execute(text("TRUNCATE chunks, documents, kb_overview, git_sources")) + db.execute(text("UPDATE sources_meta SET version = 0 WHERE id = 1")) + db.commit() + yield + db.execute(text("TRUNCATE chunks, documents, kb_overview, git_sources")) + db.execute(text("UPDATE sources_meta SET version = 0 WHERE id = 1")) + db.commit() + + +class _OverviewSpy: + """The endpoint's ``regenerate_overview`` seam: counts calls and + (optionally) raises — the LLM outage the best-effort branch must + swallow.""" + + def __init__(self, fail: BaseException | None = None) -> None: + self.calls = 0 + self.fail = fail + + async def __call__(self, llm: object, session: object = None) -> bool: # noqa: ARG002 + self.calls += 1 + if self.fail is not None: + raise self.fail + return True + + +@pytest.fixture(autouse=True) +def overview_spy(monkeypatch: pytest.MonkeyPatch) -> _OverviewSpy: + """The LLM is never hit: by default the spy succeeds (the log line + pins ``overview=True``); the best-effort test swaps in a spy that + raises ``LLMError``.""" + spy = _OverviewSpy() + monkeypatch.setattr(git_sources_api, "regenerate_overview", spy) + return spy + + +def _point_at(monkeypatch: pytest.MonkeyPatch, sources_dir: Path, upload_dir: Path) -> None: + """Fresh settings on the router's module: the tmp sources dir and + upload dir — the dev ``.env`` never leaks in, and a rmtree can + never aim at a real checkout/upload.""" + monkeypatch.setattr( + git_sources_api, + "get_settings", + lambda: Settings( + _env_file=None, # pyright: ignore[reportCallIssue] + sources_dir=str(sources_dir), + upload_dir=str(upload_dir), + ), + ) + + +def _seed_doc(db: Session, source: str, path: str = "alpha.md") -> None: + """One indexed document (+ two chunks) under ``source`` — the prune + + cascade target. Seed directly (the task allows it): the import + pipeline is out of scope here.""" + doc = Document( + source=source, + path=path, + full_path=f"/srv/brain/{source}/{path}", + title="Alpha", + content="# Alpha\nsentinel content\n", + content_hash=uuid.uuid4().hex, + ) + db.add(doc) + db.flush() + db.add_all( + [ + Chunk(document_id=doc.id, position=0, content="chunk zero"), + Chunk(document_id=doc.id, position=1, content="chunk one"), + ] + ) + db.commit() + + +def _seed_git_row(db: Session, url: str) -> GitSource: + row = GitSource(url=url, kind="git") + db.add(row) + db.commit() + return row + + +def _seed_local_row(db: Session, path: Path) -> GitSource: + """A ``kind='local'`` row exactly as the create endpoint stores it + (phase 38: the expanded path mirrored in the NOT-NULL ``url``).""" + row = GitSource(url=str(path), kind="local", path=str(path)) + db.add(row) + db.commit() + return row + + +def _version(db: Session) -> int: + """The raw counter — a text query dodges the session identity map + (the bump commits in the endpoint's own short-lived session).""" + row = db.execute(text("SELECT version FROM sources_meta WHERE id = 1")).first() + return row[0] if row is not None else 0 + + +def _docs(client: TestClient) -> list[tuple[str, str]]: + body = client.get("/api/docs").json() + return [(d["source"], d["path"]) for d in body["documents"]] + + +def _rows(client: TestClient) -> list[dict[str, object]]: + """The stored rows (the settings carry no env URLs, so the + empty-table fallback also reports ``[]`` — the assertions only ever + rely on the DB rows' ids/urls).""" + return client.get("/api/git-sources").json()["sources"] + + +def _removal_lines(caplog: pytest.LogCaptureFixture) -> list[str]: + return [ + rec.getMessage() + for rec in caplog.records + if rec.getMessage().startswith("source removed: ") + ] + + +# --------------------------------------------------------------------------- +# git row: row + docs + checkout dir, one 204 +# --------------------------------------------------------------------------- + + +def test_git_removal_deletes_row_docs_and_checkout_dir( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + overview_spy: _OverviewSpy, +) -> None: + sources = tmp_path / "sources" + _point_at(monkeypatch, sources, tmp_path / "uploads") + url = "https://gitlab.example.com/reese/homelab.git" + row = _seed_git_row(db, url) + checkout = sources / "homelab" + checkout.mkdir(parents=True) + marker = checkout / "marker.md" + marker.write_text("sentinel\n", encoding="utf-8") + _seed_doc(db, "homelab") + _seed_doc(db, "homelab", path="bravo.md") + version_before = _version(db) + + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + r = admin_client.delete(f"/api/git-sources/{row.id}") + assert r.status_code == 204 + assert r.content == b"" # the 204 no-body contract, unchanged + + # The row is gone (and the table is empty → the env fallback shape, + # no env URLs set here). + body = admin_client.get("/api/git-sources").json() + assert body["from_env"] is True + assert body["sources"] == [] + + # The app-managed checkout dir is gone from disk — marker included. + assert not checkout.exists() + assert not marker.exists() + + # The source's documents are gone from the index (admin /api/docs)… + assert _docs(admin_client) == [] + # …and every chunk row too (the ``all, delete-orphan`` cascade). + assert db.execute(text("SELECT count(*) FROM chunks")).scalar_one() == 0 + assert db.execute(text("SELECT count(*) FROM documents")).scalar_one() == 0 + + # The KB changed → exactly one version bump (phase 53) and exactly + # one (spied) overview call. + assert _version(db) == version_before + 1 + assert overview_spy.calls == 1 + + # The per-operation INFO line (PLAN §9) with the counts. + lines = _removal_lines(caplog) + assert len(lines) == 1, lines + match = re.match( + r"^source removed: kind=git name=homelab docs_pruned=2 files_removed=yes " + r"overview=True total_ms=\d+$", + lines[0], + ) + assert match, lines[0] + + +def test_git_removal_without_checkout_dir_is_a_noop_on_disk( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + overview_spy: _OverviewSpy, +) -> None: + """No checkout yet (never cloned) → the disk step is a silent no-op; + the row + index removal still lands, 204.""" + sources = tmp_path / "sources" + _point_at(monkeypatch, sources, tmp_path / "uploads") + row = _seed_git_row(db, "https://gitlab.example.com/reese/ops.git") + _seed_doc(db, "ops") + version_before = _version(db) + + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row.id}").status_code == 204 + + assert not (sources / "ops").exists() + assert _docs(admin_client) == [] + assert db.execute(text("SELECT count(*) FROM git_sources")).scalar_one() == 0 + # Docs were pruned → the bump + the overview still run. + assert _version(db) == version_before + 1 + assert overview_spy.calls == 1 + line = _removal_lines(caplog)[0] + assert "files_removed=no" in line # the absent dir is the "no" case + + +# --------------------------------------------------------------------------- +# local rows: upload folder removed, foreign dir never touched +# --------------------------------------------------------------------------- + + +def test_local_upload_removal_deletes_row_docs_and_folder( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + upload = tmp_path / "uploads" + _point_at(monkeypatch, tmp_path / "sources", upload) + folder = upload / "my-notes" + folder.mkdir(parents=True) + marker = folder / "readme.md" + marker.write_text("sentinel\n", encoding="utf-8") + row = _seed_local_row(db, folder) + _seed_doc(db, "my-notes") + version_before = _version(db) + + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row.id}").status_code == 204 + + # Row + index entries gone… + assert _rows(admin_client) == [] + assert _docs(admin_client) == [] + # …and the app-managed upload folder is gone from disk. + assert not folder.exists() + assert not marker.exists() + assert _version(db) == version_before + 1 + line = _removal_lines(caplog)[0] + assert re.match( + r"^source removed: kind=local name=my-notes docs_pruned=1 files_removed=yes " + r"overview=True total_ms=\d+$", + line, + ), line + + +def test_local_foreign_dir_removal_never_touches_disk( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """A ``kind='local'`` row pointing at the owner's own directory: + row + index entries go, the directory NEVER does (the locked + decision) — and the KB change still bumps the version.""" + upload = tmp_path / "uploads" + _point_at(monkeypatch, tmp_path / "sources", upload) + foreign = tmp_path / "own" / "docs" + foreign.mkdir(parents=True) + marker = foreign / "precious.md" + marker.write_text("keep me\n", encoding="utf-8") + row = _seed_local_row(db, foreign) + _seed_doc(db, "docs") # the local source label = the dir's name + version_before = _version(db) + + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row.id}").status_code == 204 + + assert _rows(admin_client) == [] + assert _docs(admin_client) == [] + # The foreign directory — and its files — are exactly as they were. + assert foreign.is_dir() + assert marker.read_text(encoding="utf-8") == "keep me\n" + # Index change → the bump still lands; the disk step was skipped. + assert _version(db) == version_before + 1 + line = _removal_lines(caplog)[0] + assert "files_removed=skipped" in line, line + + +def test_local_prefix_sibling_dir_is_not_contained( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """The containment edge end-to-end: ``upload_dir = tmp/'u'`` and a + local row at ``tmp/'u-evil'`` — the prefix-sharing sibling is NOT + under the upload dir, so its files are never deleted (the unit test + pins ``managed_dir_for``; this pins the endpoint honoring it).""" + upload = tmp_path / "u" + upload.mkdir() + _point_at(monkeypatch, tmp_path / "sources", upload) + evil = tmp_path / "u-evil" + evil.mkdir() + marker = evil / "marker.md" + marker.write_text("not an upload\n", encoding="utf-8") + row = _seed_local_row(db, evil) + _seed_doc(db, "u-evil") + + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row.id}").status_code == 204 + + assert _rows(admin_client) == [] + assert _docs(admin_client) == [] + assert evil.is_dir() + assert marker.read_text(encoding="utf-8") == "not an upload\n" + assert "files_removed=skipped" in _removal_lines(caplog)[0] + + +# --------------------------------------------------------------------------- +# sibling guard +# --------------------------------------------------------------------------- + + +def test_sibling_guard_keeps_shared_docs_and_files_until_last_row( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + overview_spy: _OverviewSpy, +) -> None: + """``https://example.com/reese/r`` and ``https://example.com/reese/ + r.git`` both index under ``r`` — the first removal deletes ONLY the + row (docs + shared checkout stay, loudly logged, no bump/overview); + the second is the total removal.""" + sources = tmp_path / "sources" + _point_at(monkeypatch, sources, tmp_path / "uploads") + row_a = _seed_git_row(db, "https://example.com/reese/r") + row_b = _seed_git_row(db, "https://example.com/reese/r.git") + checkout = sources / "r" + checkout.mkdir(parents=True) + marker = checkout / "shared.md" + marker.write_text("belongs to both rows\n", encoding="utf-8") + _seed_doc(db, "r") + version_before = _version(db) + + # --- first row: sibling guard — only the row goes ----------------- + # INFO level so both the loudly-logged warning and the per-operation + # INFO line of this removal are captured. + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row_a.id}").status_code == 204 + + rows = _rows(admin_client) + assert [s["id"] for s in rows] == [str(row_b.id)] + assert rows[0]["url"] == "https://example.com/reese/r.git" + assert rows[0]["kind"] == "git" and rows[0]["path"] is None + # The shared documents + files still belong to the sibling… + assert _docs(admin_client) == [("r", "alpha.md")] + assert checkout.is_dir() + assert marker.read_text(encoding="utf-8") == "belongs to both rows\n" + # …and nothing KB-side happened: no prune, no bump, no overview. + assert _version(db) == version_before + assert overview_spy.calls == 0 + line = _removal_lines(caplog)[0] + assert re.match( + r"^source removed: kind=git name=r docs_pruned=0 files_removed=skipped " + r"overview=False total_ms=\d+$", + line, + ), line + # …and the guard is logged loudly (naming the row + the shared name). + warnings = [ + rec.getMessage() for rec in caplog.records if rec.levelno == logging.WARNING + ] + assert any( + "shares source name r" in w and str(row_a.id) in w for w in warnings + ), warnings + + # --- second row: the total removal, at last ------------------------ + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row_b.id}").status_code == 204 + + assert _rows(admin_client) == [] + assert _docs(admin_client) == [] + assert not checkout.exists() + assert _version(db) == version_before + 1 + assert overview_spy.calls == 1 + assert "files_removed=yes" in _removal_lines(caplog)[1] + + +# --------------------------------------------------------------------------- +# version bump + best-effort overview gates +# --------------------------------------------------------------------------- + + +def test_overview_failure_is_best_effort_still_204( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + overview_spy: _OverviewSpy, +) -> None: + """A failing overview (LLM outage) never fails the delete: the 204 + lands, the docs stay pruned, and the bump still lands — overview + first, bump second (mirroring ``_run_sync``).""" + sources = tmp_path / "sources" + _point_at(monkeypatch, sources, tmp_path / "uploads") + row = _seed_git_row(db, "https://gitlab.example.com/reese/notes.git") + checkout = sources / "notes" + checkout.mkdir(parents=True) + (checkout / "n.md").write_text("x\n", encoding="utf-8") + _seed_doc(db, "notes") + version_before = _version(db) + + failing = _OverviewSpy(fail=LLMError("simulated lite-model outage")) + monkeypatch.setattr(git_sources_api, "regenerate_overview", failing) + + # INFO level — the per-operation line (INFO) and the best-effort + # failure (ERROR) are both captured. + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row.id}").status_code == 204 + + assert failing.calls == 1 + assert _docs(admin_client) == [] + assert not checkout.exists() + # The bump lands even though the best-effort overview failed… + assert _version(db) == version_before + 1 + # …and the failure is logged, never fatal. + line = _removal_lines(caplog)[0] + assert "overview=False" in line, line + errors = [ + rec.getMessage() for rec in caplog.records if rec.levelno >= logging.ERROR + ] + assert any("overview regeneration failed" in e for e in errors), errors + + +def test_no_docs_pruned_means_no_overview_and_no_bump( + admin_client: TestClient, + db: Session, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + overview_spy: _OverviewSpy, +) -> None: + """A row with no indexed documents: the checkout dir is still + removed (files_removed=yes) but the KB is unchanged — no overview + call, no version bump (the gate is docs pruned, not files).""" + sources = tmp_path / "sources" + _point_at(monkeypatch, sources, tmp_path / "uploads") + row = _seed_git_row(db, "https://gitlab.example.com/reese/bare.git") + checkout = sources / "bare" + checkout.mkdir(parents=True) + (checkout / "b.md").write_text("never imported\n", encoding="utf-8") + version_before = _version(db) + + with caplog.at_level(logging.INFO, logger="app.api.git_sources"): + assert admin_client.delete(f"/api/git-sources/{row.id}").status_code == 204 + + assert not checkout.exists() + assert _rows(admin_client) == [] + assert _version(db) == version_before + assert overview_spy.calls == 0 + line = _removal_lines(caplog)[0] + assert re.match( + r"^source removed: kind=git name=bare docs_pruned=0 files_removed=yes " + r"overview=False total_ms=\d+$", + line, + ), line diff --git a/tests/unit/test_agent.py b/tests/unit/test_agent.py index 8979352..b3b4464 100644 --- a/tests/unit/test_agent.py +++ b/tests/unit/test_agent.py @@ -118,16 +118,16 @@ def test_agent_tools_names_and_parameters() -> None: "Add the full content of one more indexed document to your context" ) # Phase 63 (A2): the parameter descriptions point the LLM at the - # labeled `source:` / `path:` fields of the list_documents output. + # labeled `source:` / `path:` fields of the list_documents output + # (the example was dropped by the phase-68 description fix — the + # wording stays pinned, the model saw invented paths in calls). assert read_params["properties"]["source"]["description"] == ( "The document's source, as shown after 'source: ' in the " - "list_documents output (e.g. 'Homelab' from " - "'source: Homelab | path: homelab/aws-route53.md')." + "list_documents output." ) assert read_params["properties"]["path"]["description"] == ( "The document's path, as shown after 'path: ' in the " - "list_documents output (e.g. 'homelab/aws-route53.md' from " - "'source: Homelab | path: homelab/aws-route53.md')." + "list_documents output." ) # Phase 68: search_documents — the third tool, a locator (locked A5). search = by_name["search_documents"]["function"] @@ -149,13 +149,11 @@ def test_agent_tools_names_and_parameters() -> None: # Phase 63 labeled-field wording, same as read_document's parameters. assert search_params["properties"]["source"]["description"] == ( "The document's source, as shown after 'source: ' in the " - "list_documents output (e.g. 'Homelab' from " - "'source: Homelab | path: homelab/aws-route53.md')." + "list_documents output." ) assert search_params["properties"]["path"]["description"] == ( "The document's path, as shown after 'path: ' in the " - "list_documents output (e.g. 'homelab/aws-route53.md' from " - "'source: Homelab | path: homelab/aws-route53.md')." + "list_documents output." ) diff --git a/tests/unit/test_remove_confirm_modal.py b/tests/unit/test_remove_confirm_modal.py new file mode 100644 index 0000000..3ad6d63 --- /dev/null +++ b/tests/unit/test_remove_confirm_modal.py @@ -0,0 +1,472 @@ +"""Unit: the remove confirmation modal on /git-sources.html (phase 69, +task 02). + +Removal is a TOTAL removal (owner request 2026-09-02): the row, the +source's indexed documents (chunks + embeddings), and — for git clones +and uploaded archives — the files on the server's disk, all immediately +(task 01's ``DELETE /api/git-sources/{id}`` rewire). The page must +confirm that through a real in-app ``role="alertdialog"`` modal — +``window.confirm`` is retired — that names the source and states the +policy BEFORE the request goes out. + +The browser behavior itself is E2E-gated by the phase-69 story suite +(``tests/e2e/test_source_removal_cleanup.py`` — task 03: focus, +Escape, ≥44px targets, the a11y interaction layer); like the other +frontend-adjacent unit files (``test_stale_ui_copy.py`` / +``test_summary_edit_ui.py`` house pattern), this module pins the +source-level contract a silent regression would break: + +* ``window.confirm`` is GONE from the whole frontend (the only + native confirm the app ever shipped); +* the static ``#remove-confirm-dialog`` markup — ``role="alertdialog"``, + ``aria-modal``, ``aria-labelledby``/``aria-describedby``, hidden by + default, the six child ids, the ``role="alert"`` error line, real + ``type="button"`` buttons, the locked modal copy (verbatim); +* the JS lifecycle — ``openRemoveConfirm`` (textContent-only source + population with makeRow's ``value`` expression, error cleared, focus + on Cancel, the trigger recorded), cancel = Escape / Cancel button / + backdrop (no request; focus returns to the trigger; a no-op while a + DELETE is in flight), ``confirmRemove`` (§7.4 in-flight state: both + buttons disable + "Removing…", success → close/reload/announce — + the removal confirmation is the LAST announcement, non-2xx → the + in-modal alert line + dialog stays open, network → + the fixed reachable? line, re-enable in the finally); +* the stale "prunes on the next sync" removal copy is GONE from + ``git-sources.js`` + ``git-sources.html``; the new hint copy is + PRESENT (the README pins are task 03's); +* styles.css — the modal classes on the house dark-tech palette + (phase-08 tokens only, no CDN, no blur), ≥44px buttons, the + ``[hidden]`` override, the err-token destructive pair. +""" +from __future__ import annotations + +import re +from pathlib import Path + +FRONTEND = Path(__file__).resolve().parents[2] / "frontend" +HTML = FRONTEND / "git-sources.html" +JS = FRONTEND / "assets" / "git-sources.js" +CSS = FRONTEND / "assets" / "styles.css" + +#: The locked modal copy (phase 69, 00_phase.md) — one fixed paragraph +#: for both kinds (the UI cannot tell an upload from the owner's own +#: directory; naming the source above it makes the target +#: unambiguous). Pinned verbatim against the normalized text. +MODAL_COPY = ( + "This permanently removes the source entry, all of its indexed " + "documents from the knowledge base, and — for git clones and " + "uploaded archives — the files on the server's disk. Files in " + "your own local directories are never touched. This cannot be undone." +) + +#: The new hint-box contract (phase 69): removal is immediate and +#: total; the modal spells it out; the Sync button mirrors the +#: remaining sources (upstream churn is still pruned on that run). +HINT_TOTAL_REMOVAL = "Removing a source is a total removal, done immediately" +HINT_MODAL_SPELLS_OUT = ( + "the confirmation modal spells out exactly what will be deleted" +) +HINT_FOREVER_SAFE = "files in your own local directories are never touched" + +#: The success announce (the existing #git-sources-announcer live +#: region). +ANNOUNCE_OK = "Source removed — its files and index entries were cleaned up." + +#: The retired confirm copy (task 02 stale-copy pins). +OLD_STAY_INDEXED = "stays indexed until the next sync" +# "prunes/pruned … on the next sync" in any shape (the old hint + +# confirm message). +OLD_NEXT_SYNC = re.compile(r"prun\w+[^.]{0,120}?next sync") + + +def _text(path: Path) -> str: + return path.read_text(encoding="utf-8") + + +def _norm(text: str) -> str: + """Collapse whitespace runs — the markup wraps long lines, so the + locked copy is pinned against the normalized text.""" + return re.sub(r"\s+", " ", text).strip() + + +def _js() -> str: + return _text(JS) + + +def _css() -> str: + return _text(CSS) + + +def _fn(js: str, name: str) -> str: + """The source of a (possibly async) top-level function via + balanced-brace counting (the test_summary_edit_ui.py helper).""" + for prefix in ("async function ", "function "): + start = js.find(f"{prefix}{name}(") + if start != -1: + depth = 0 + for i in range(js.find("{", start), len(js)): + if js[i] == "{": + depth += 1 + elif js[i] == "}": + depth -= 1 + if depth == 0: + return js[start : i + 1] + raise AssertionError(f"unbalanced braces in {name}()") + raise AssertionError(f"{name}() must exist in git-sources.js") + + +def _element_block(html: str, id_attr: str, tag: str = "div") -> str: + """The element's full markup (a balanced-tag walk — + the dialog nests the backdrop / panel / actions divs; the hint is + a

).""" + marker = f'id="{id_attr}"' + i = html.find(marker) + assert i != -1, f"missing id={id_attr} in git-sources.html" + opens = [m.start() for m in re.finditer(rf"<{tag}\b", html[:i])] + assert opens, f"no <{tag}> owns id={id_attr}" + open_i = opens[-1] + depth = 0 + for m in re.finditer(rf"<{tag}\b[^>]*>|", html[open_i :]): + if m.group(0).startswith(f""): + depth -= 1 + else: + depth += 1 + if depth == 0: + return html[open_i : open_i + m.end()] + raise AssertionError(f"unbalanced <{tag}> for id={id_attr}") + + +# ---------- window.confirm is gone (the whole frontend) ---------- + + +def test_window_confirm_is_gone_from_the_whole_frontend() -> None: + """``window.confirm`` — the only native confirm the app ever + shipped — is absent from EVERY frontend file (markup, scripts, + styles — comments included: the retirement note must not keep the + literal).""" + for path in sorted(FRONTEND.rglob("*")): + if path.is_file(): + assert "window.confirm" not in path.read_text(encoding="utf-8"), ( + f"window.confirm survived in {path.relative_to(FRONTEND)}" + ) + + +# ---------- the static dialog markup ---------- + + +def test_dialog_markup_is_the_locked_alertdialog() -> None: + """#remove-confirm-dialog: role="alertdialog" + aria-modal + the + labelled/describedby pair, hidden by default, inside the manager + (#git-sources-content — the static-markup convention, stable E2E + selectors); all six child ids present; the error line is + role="alert"; both buttons are real type="button"; the title is + the locked h2; the modal copy is the locked paragraph verbatim.""" + html = _text(HTML) + frag = _element_block(html, "remove-confirm-dialog") + open_tag = frag[: frag.find(">") + 1] + assert 'role="alertdialog"' in open_tag + assert 'aria-modal="true"' in open_tag + assert 'aria-labelledby="remove-confirm-title"' in open_tag + assert 'aria-describedby="remove-confirm-copy"' in open_tag + assert "hidden" in open_tag, "the dialog ships hidden" + # Static markup INSIDE the manager (the #git-sources-hint / gate + # convention) — E2E can wait for the content to be revealed. + assert html.find('id="git-sources-content"') < html.find("remove-confirm-dialog") + for child in ( + "remove-confirm-title", + "remove-confirm-source", + "remove-confirm-copy", + "remove-confirm-error", + "remove-confirm-cancel", + "remove-confirm-remove", + ): + assert f'id="{child}"' in frag, f"missing #{child} in the dialog" + title = re.search(r"]*id=\"remove-confirm-title\"[^>]*>(.*?)

", frag, re.S) + assert title and _norm(title.group(1)) == "Remove this source?" + err = re.search(r"]*id=\"remove-confirm-error\"[^>]*>", frag) + assert err and 'role="alert"' in err.group(0), "the in-modal error is role=alert" + for btn in ("remove-confirm-cancel", "remove-confirm-remove"): + m = re.search(rf"]*id=\"{btn}\"[^>]*>", frag) + assert m and 'type="button"' in m.group(0), f"#{btn} is a real type=button" + cancel = re.search(r']*id="remove-confirm-cancel"[^>]*>(.*?)', frag, re.S) + remove = re.search(r']*id="remove-confirm-remove"[^>]*>(.*?)', frag, re.S) + assert cancel and _norm(cancel.group(1)) == "Cancel" + assert remove and _norm(remove.group(1)) == "Remove source" + assert _norm(MODAL_COPY) in _norm(frag), "the locked modal copy, verbatim" + # The source value is a (mono) — textContent-only in JS. + src = re.search(r"]*id=\"remove-confirm-source\"[^>]*>", frag) + assert src, "#remove-confirm-source is a " + + +# ---------- the JS lifecycle ---------- + + +def test_open_populates_source_via_text_content_and_focuses_cancel() -> None: + """openRemoveConfirm(s, triggerBtn): the source value is + textContent ONLY (never innerHTML — the credential-masking + discipline, phase 32) with makeRow's exact ``value`` expression + (``s.path ?? s.url`` for local rows, ``s.url`` for git); the error + line clears; the dialog unhides; the trigger is recorded; the + keydown handler attaches; and focus lands on Cancel — the safe + default for a destructive action (AFTER the unhide).""" + body = _fn(_js(), "openRemoveConfirm") + assert "s.kind === \"local\"" in body, "the kind-typed value branch" + assert ( + "removeSourceEl.textContent = isLocal ? s.path ?? s.url : s.url" in body + ), "the same `value` expression makeRow uses, via textContent" + assert "removeSourceEl.innerHTML" not in _js(), "XSS contract: textContent only" + assert "removeError.hidden = true" in body, "a new attempt starts clean" + assert "removeTriggerBtn = triggerBtn" in body, "the trigger is recorded" + trigger_i = body.find("removeTriggerBtn = triggerBtn") + unhide_i = body.find("removeDialog.hidden = false") + attach_i = body.find('document.addEventListener("keydown", onRemoveDialogKeydown)') + focus_i = body.find("removeCancelBtn.focus()") + assert -1 < trigger_i < unhide_i < attach_i < focus_i, ( + "record the trigger → unhide → attach keydown → focus Cancel" + ) + + +def test_row_button_opens_the_modal_and_the_row_error_span_is_retired() -> None: + """makeRow's per-row Remove button opens the modal + (openRemoveConfirm(s, btn)) — no confirm call, no per-row error + span (the modal carries the in-flight error line); the retired + .git-source-row-error class is gone from the JS AND the CSS.""" + js = _js() + make = _fn(js, "makeRow") + assert "openRemoveConfirm(s, btn)" in make, "the row's Remove opens the modal" + assert "removeSource(" not in js, "the window.confirm-era flow is gone" + assert "git-source-row-error" not in js + assert "git-source-row-error" not in _css(), "the dead per-row error CSS is retired" + + +def test_cancel_paths_close_without_a_request() -> None: + """Cancel (Cancel button / Escape / backdrop) closes as cancel: + the dialog hides, the error line clears, the buttons reset + ("Remove source"), the keydown handler detaches, and focus + RETURNS to the recorded trigger — and the cancel path never + fetches. A cancel while a DELETE is in flight is a no-op (no + half-cancel of an in-progress server-side removal).""" + js = _js() + cancel = _fn(js, "cancelRemoveConfirm") + guard_i = cancel.find("if (removeInFlight) return") + close_i = cancel.find("closeRemoveConfirm()") + assert -1 < guard_i < close_i, "the in-flight guard precedes the close" + assert "fetch" not in cancel, "cancel never sends a request" + close = _fn(js, "closeRemoveConfirm") + hide_i = close.find("removeDialog.hidden = true") + clear_i = close.find("removeError.hidden = true") + reset_i = close.find('removeRemoveBtn.textContent = "Remove source"') + detach_i = close.find('document.removeEventListener("keydown", onRemoveDialogKeydown)') + save_i = close.find("const trigger = removeTriggerBtn") + null_i = close.find("removeTriggerBtn = null") + focus_i = close.find("trigger.focus()") + assert -1 < hide_i < clear_i < reset_i < detach_i < save_i < null_i < focus_i, ( + "hide → clear → reset → detach → save trigger → focus return" + ) + assert "removeCancelBtn.disabled = false" in close + assert "removeRemoveBtn.disabled = false" in close + + +def test_escape_and_backdrop_and_cancel_button_all_cancel() -> None: + """While open (handler attached on document in openRemoveConfirm, + detached in closeRemoveConfirm): Escape → preventDefault + + cancelRemoveConfirm; the dim backdrop AND the Cancel button wire + to cancelRemoveConfirm (only "Remove source" wires to + confirmRemove). Tab/Shift+Tab stay inside the two-button modal + (aria-modal honored for keyboard users).""" + js = _js() + keydown = _fn(js, "onRemoveDialogKeydown") + esc_i = keydown.find('e.key === "Escape"') + prevent_i = keydown.find("e.preventDefault()", esc_i) + cancel_i = keydown.find("cancelRemoveConfirm()", esc_i) + assert -1 < esc_i < prevent_i < cancel_i, "Escape: prevent + cancel" + assert 'e.key === "Tab"' in keydown, "the two-button focus cycle" + assert 'removeCancelBtn.addEventListener("click", cancelRemoveConfirm)' in js + assert 'removeBackdrop.addEventListener("click", cancelRemoveConfirm)' in js + assert 'removeRemoveBtn.addEventListener("click", confirmRemove)' in js + + +def test_confirm_runs_the_inflight_never_stale_lifecycle() -> None: + """confirmRemove: the §7.4 in-flight state BEFORE the fetch — + both buttons disable + the confirm relabels "Removing…"; one + DELETE /api/git-sources/{id}. Success (204): close (focus return) + → loadSources → announce (the removal confirmation is the LAST + announcement — the reload's "N sources listed." must not + overwrite it). Non-2xx: the in-modal role=alert line + (apiDetail, 422 shape-aware) and the dialog STAYS open (no + closeRemoveConfirm in the failure slice). Network failure: the + fixed reachable? line. The finally re-enables BOTH buttons + + relabels "Remove source" — never stale on any outcome.""" + body = _fn(_js(), "confirmRemove") + guard_i = body.find("if (!removingId || removeInFlight) return") + inflight_i = body.find("removeInFlight = true") + clear_i = body.find("removeError.hidden = true") + dis_c = body.find("removeCancelBtn.disabled = true") + dis_r = body.find("removeRemoveBtn.disabled = true") + label_i = body.find('"Removing…"') + fetch_i = body.find("fetch(`/api/git-sources/${encodeURIComponent(removingId)}`") + method_i = body.find('method: "DELETE"', fetch_i) + assert -1 < guard_i < inflight_i < clear_i < dis_c < dis_r < label_i < fetch_i < method_i, ( + "guard → in-flight → clear error → disable both + label → DELETE" + ) + # Success: close → reload → announce (the exact order, the exact + # announce string) — the removal confirmation is the LAST + # announcement: the reload's "N sources listed." must not + # overwrite it (phase 69 task 03's E2E pins the success line on + # the announcer after a real removal). + ok_i = body.find("if (r.ok)") + close_i = body.find("closeRemoveConfirm()", ok_i) + reload_i = body.find("await loadSources()", close_i) + announce_i = body.find(f'announce("{ANNOUNCE_OK}")', reload_i) + assert -1 < ok_i < close_i < reload_i < announce_i + assert body.count("closeRemoveConfirm()") == 1, ( + "only the success path closes — failures stay open for one retry" + ) + # Non-2xx: the in-modal alert line, dialog stays open. + nonok_i = body.find("Could not remove the source — try again.") + catch_i = body.find("} catch {") + assert -1 < nonok_i < catch_i + assert "await apiDetail(r," in body[body.find("if (r.ok)"):catch_i], ( + "the server detail is apiDetail-extracted (422 shape-aware)" + ) + failure_slice = body[body.rfind("// non-2xx", 0, catch_i):catch_i] + assert "closeRemoveConfirm" not in failure_slice, "failure keeps the dialog open" + # Network: the fixed reachable? line. + net_i = body.find("Could not remove the source — is the app reachable?", catch_i) + assert -1 < net_i < body.find("finally"), "the network copy lands in the catch" + # Never stale: the finally re-enables BOTH buttons + relabels. + fin_i = body.find("finally") + fin = body[fin_i:] + assert "removeInFlight = false" in fin + assert "removeCancelBtn.disabled = false" in fin + assert "removeRemoveBtn.disabled = false" in fin + assert 'removeRemoveBtn.textContent = "Remove source"' in fin + + +# ---------- the stale copy is gone; the new hint is present ---------- + + +def test_stale_next_sync_removal_copy_is_gone() -> None: + """The phase-35 "prunes on the next sync" removal contract is + superseded: the retired confirm copy AND any 'prune(s/d) … next + sync' shape are absent from git-sources.js + git-sources.html + (code AND comments — the docstring copy moved with the flow). + The README pins are task 03's.""" + for path in (JS, HTML): + raw = _text(path) + norm = _norm(raw) + assert OLD_STAY_INDEXED not in raw, ( + f"retired confirm copy still in {path.name}" + ) + m = OLD_NEXT_SYNC.search(norm) + assert m is None, f"'next sync' removal copy survived in {path.name}: {m.group(0)!r}" + + +def test_new_hint_copy_is_present_in_the_html() -> None: + """The #git-sources-hint carries the new contract: removal is a + total removal, done immediately (the modal spells it out; foreign + local directories are never touched), and the Sync button still + mirrors the remaining sources (upstream churn is pruned on that + run — not 'on the next sync').""" + hint = _norm(_element_block(_text(HTML), "git-sources-hint", tag="p")) + for frag in (HINT_TOTAL_REMOVAL, HINT_MODAL_SPELLS_OUT, HINT_FOREVER_SAFE): + assert frag in hint, f"the new hint copy is missing: {frag!r}" + assert "pruned on that run" in hint, "the Sync-mirror clause (upstream churn)" + assert "next sync" not in hint, "no 'next sync' removal claim in the hint" + + +def test_module_docstring_carries_the_new_contract() -> None: + """The git-sources.js module docstring's remove bullet + scope + boundary moved with the flow: the modal names the source + states + the policy, and removal performs the FULL cleanup server-side + (row + index + app-managed files) — with the navigating-away + note (the row + index commit first; an interrupted file step + leaves an inert orphan dir).""" + doc = _js().split("*/", 2)[0] # the module docstring (first block) + for frag in ( + 'role="alertdialog"', + "textContent ONLY", + 'relabels "Removing…"', + "not recommended", + "inert orphan", + "FULL cleanup server-side", + "foreign local directories are never touched", + ): + assert frag in doc, f"the module docstring lost: {frag!r}" + + +# ---------- styles.css ---------- + + +def test_modal_css_classes_present_and_house_tokens_only() -> None: + """styles.css carries the modal class family on the house + dark-tech palette (phase-08 tokens): the overlay + backdrop + + panel, the title / source / copy / error / actions / two-button + chrome; the [hidden] override (the documented, testable + contract); the global 3px :focus-visible outline is NOT + suppressed; no CDN; no blur (the phase-08 no-blur perf anchor).""" + css = _css() + for cls in ( + ".remove-confirm", + ".remove-confirm-backdrop", + ".remove-confirm-panel", + ".remove-confirm-title", + ".remove-confirm-source", + ".remove-confirm-copy", + ".remove-confirm-error", + ".remove-confirm-actions", + ".remove-confirm-btn", + ".remove-confirm-cancel", + ".remove-confirm-remove", + ): + assert f"{cls} " in css or f"{cls}." in css or f"{cls}[" in css, ( + f"styles.css must style {cls}" + ) + hidden = css.find(".remove-confirm[hidden]") + assert hidden != -1 and "display: none" in css[hidden : hidden + 60], ( + "the hidden attr must beat the display rule" + ) + assert ":focus-visible {" in css and "outline: 3px solid var(--brand)" in css + # No blur (the phase-08 no-blur perf anchor) — the backdrop rule + # itself must not carry backdrop-filter. + backdrop = css[css.find(".remove-confirm-backdrop {") :] + backdrop = backdrop[: backdrop.find("\n}")] + # Comments stripped — the note "no backdrop-filter (no-blur)" must + # not trip the pin; only a real declaration may. + backdrop = re.sub(r"/\*.*?\*/", "", backdrop, flags=re.S) + assert "backdrop-filter" not in backdrop, "no blur (phase-08 perf anchor)" + assert "url(http" not in css and "@import url(" not in css, ( + "no CDN (AGENTS.md rule 6)" + ) + + +def test_modal_css_targets_and_contrast_pairs() -> None: + """The WCAG 2.1 AA basics in CSS: both buttons >=44px; the + destructive button rides the err token family (err-ink on err-bg + 9.3:1, the err-line border — the .tuning-delete / .steering-delete + convention; the hover inverts to --bg on --err-line, 5.2:1); + Cancel is the ghost ink-soft family (5.1:1 on --surface); the + error line is the err pair; the panel caps at the 46rem + chat-column width or the viewport.""" + css = _css() + btn = css[css.find(".remove-confirm-btn {"):] + btn = btn[: btn.find("\n}")] + assert "min-height: 44px" in btn and "min-width: 44px" in btn + remove = css[css.find(".remove-confirm-remove {"):] + remove = remove[: remove.find("\n}")] + for prop in ("var(--err-bg)", "var(--err-ink)", "var(--err-line)"): + assert prop in remove, f"the destructive pair must keep {prop}" + hover = css[css.find(".remove-confirm-remove:hover:not(:disabled) {"):] + hover = hover[: hover.find("\n}")] + assert "var(--err-line)" in hover and "var(--bg)" in hover, ( + "the hover inversion: dark --bg on --err-line (5.2:1)" + ) + cancel = css[css.find(".remove-confirm-cancel {"):] + cancel = cancel[: cancel.find("\n}")] + assert "var(--ink-soft)" in cancel and "transparent" in cancel + err = css[css.find(".remove-confirm-error {"):] + err = err[: err.find("\n}")] + assert "var(--err-ink)" in err and "var(--err-bg)" in err + panel = css[css.find(".remove-confirm-panel {"):] + panel = panel[: panel.find("\n}")] + assert "min(46rem" in panel, "the 46rem chat-column cap (or the viewport)" diff --git a/tests/unit/test_source_removal.py b/tests/unit/test_source_removal.py new file mode 100644 index 0000000..508515f --- /dev/null +++ b/tests/unit/test_source_removal.py @@ -0,0 +1,310 @@ +"""Unit: the total-removal helpers (phase 69, task 01). + +Covers ``app.rag.source_removal`` with plain objects and ``tmp_path`` +(no FastAPI, no database): + +* ``resolve_source_name`` — exactly the sync/importer document labels: + git URL shapes (https ``.git`` / bare, scp-style ``git@host:repo.git``, + ``ssh://``) and local paths (``~`` expansion, trailing slash, the + ``path or url`` fallback); +* ``managed_dir_for`` — git → ``sources_dir//``; local → the + stored dir only when it is ``upload_dir`` itself or nested under it + (the containment check, so a sibling named ``uploads-foo`` never + counts); any other local path → ``None`` (owner's own dir, never + touched); +* ``remove_managed_dir`` — ``None``/absent no-op (no filesystem write), + present tree removed → ``True``, ``OSError`` logged (``logger. + exception``) and returned as ``False`` — never raises; +* ``has_sibling`` — same-name sibling (git ``…/r`` vs ``…/r.git``, and + across kinds) → ``True``; different names → ``False``; self-excluded. +""" +from __future__ import annotations + +import logging +import uuid +from pathlib import Path +from typing import cast + +import pytest +from sqlalchemy.orm import Session + +from app.models import GitSource +from app.rag import source_removal +from app.rag.source_removal import ( + has_sibling, + managed_dir_for, + remove_managed_dir, + resolve_source_name, +) + + +def _git(url: str) -> GitSource: + return GitSource(id=uuid.uuid4(), url=url, kind="git") + + +def _local(path: str, path_column: str | None = None) -> GitSource: + # Phase 38 mirrors the expanded path in the NOT-NULL ``url`` column; + # ``path_column=None`` exercises the ``row.path or row.url`` fallback. + return GitSource(id=uuid.uuid4(), url=path, kind="local", path=path_column) + + +# --------------------------------------------------------------------------- +# resolve_source_name +# --------------------------------------------------------------------------- + + +def test_resolve_git_url_https_dot_git_suffix() -> None: + assert resolve_source_name(_git("https://example.com/reese/homelab.git")) == "homelab" + + +def test_resolve_git_url_https_bare() -> None: + assert resolve_source_name(_git("https://example.com/reese/homelab")) == "homelab" + + +def test_resolve_git_url_scp_style_git_at() -> None: + """``git@host:repo.git`` — the ``:`` basename split (the phase-28 + ``repo_name`` behavior, reused not re-implemented).""" + assert resolve_source_name(_git("git@github.com:reese/deployments.git")) == "deployments" + assert resolve_source_name(_git("git@github.com:reese/deployments")) == "deployments" + + +def test_resolve_git_url_ssh_scheme() -> None: + assert resolve_source_name(_git("ssh://git@example.com/reese/ops.git")) == "ops" + + +def test_resolve_git_url_strips_whitespace() -> None: + assert resolve_source_name(_git(" https://example.com/reese/x.git ")) == "x" + + +def test_resolve_local_expands_tilde(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.setenv("HOME", str(tmp_path / "home")) + (tmp_path / "home" / "notes").mkdir(parents=True) + assert resolve_source_name(_local("~/notes")) == "notes" + + +def test_resolve_local_trailing_slash_and_nested() -> None: + assert resolve_source_name(_local("/srv/docs/notes/")) == "notes" + assert resolve_source_name(_local("/srv/a/b/c")) == "c" + + +def test_resolve_local_falls_back_to_url_column_when_path_null() -> None: + """Phase 38 mirrors the path in ``url`` — the ``or`` fallback keeps a + NULL ``path`` row resolvable the same way.""" + assert resolve_source_name(_local("/srv/docs/notes", path_column=None)) == "notes" + + +# --------------------------------------------------------------------------- +# managed_dir_for +# --------------------------------------------------------------------------- + + +def test_managed_dir_git_maps_to_sources_dir_repo_name(tmp_path: Path) -> None: + sources = tmp_path / "sources" + row = _git("https://example.com/reese/homelab.git") + assert managed_dir_for(row, sources, tmp_path / "uploads") == sources / "homelab" + + +def test_managed_dir_git_expands_tilde_sources_dir( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.setenv("HOME", str(tmp_path / "home")) + (tmp_path / "home" / "bor-sources").mkdir(parents=True) + row = _git("git@github.com:reese/ops.git") + got = managed_dir_for(row, Path("~/bor-sources"), tmp_path / "uploads") + assert got == tmp_path / "home" / "bor-sources" / "ops" + + +def test_managed_dir_local_upload_dir_itself(tmp_path: Path) -> None: + upload = tmp_path / "uploads" + upload.mkdir() + row = _local(str(upload)) + assert managed_dir_for(row, tmp_path / "sources", upload) == upload + + +def test_managed_dir_local_nested_under_upload_dir(tmp_path: Path) -> None: + upload = tmp_path / "uploads" + folder = upload / "my-notes" + folder.mkdir(parents=True) + row = _local(str(folder)) + assert managed_dir_for(row, tmp_path / "sources", upload) == folder + + +def test_managed_dir_local_deeply_nested_under_upload_dir(tmp_path: Path) -> None: + upload = tmp_path / "uploads" + folder = upload / "a" / "b" + folder.mkdir(parents=True) + row = _local(str(folder)) + assert managed_dir_for(row, tmp_path / "sources", upload) == folder + + +def test_managed_dir_unresolvable_path_is_none( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Containment that cannot be established (an unresolvable path) is + ``None`` — never delete when in doubt (the defensive branch).""" + row = _local("/srv/docs/notes") + + def boom(self: Path, strict: bool = False) -> Path: # noqa: ARG001 + raise OSError("simulated resolve failure") + + monkeypatch.setattr(Path, "resolve", boom) + assert managed_dir_for(row, tmp_path / "sources", tmp_path / "uploads") is None + + +def test_managed_dir_local_foreign_path_is_none(tmp_path: Path) -> None: + """The owner's own directory — never app-managed, never touched.""" + foreign = tmp_path / "own" / "docs" + foreign.mkdir(parents=True) + row = _local(str(foreign)) + assert managed_dir_for(row, tmp_path / "sources", tmp_path / "uploads") is None + + +def test_managed_dir_local_prefix_sibling_never_counts(tmp_path: Path) -> None: + """The containment edge: ``…/u-evil`` is NOT under ``…/u`` — a + prefix-sharing sibling name must never map into the upload dir.""" + evil = tmp_path / "u-evil" + evil.mkdir() + row = _local(str(evil)) + assert managed_dir_for(row, tmp_path / "sources", tmp_path / "u") is None + + +def test_managed_dir_local_symlink_escaping_upload_dir_is_none(tmp_path: Path) -> None: + """A symlink stored under the upload dir that points at the owner's + dir resolves OUTSIDE — containment fails → ``None`` (never delete + through a link).""" + upload = tmp_path / "uploads" + foreign = tmp_path / "foreign" + upload.mkdir() + foreign.mkdir() + link = upload / "sneaky" + link.symlink_to(foreign) + row = _local(str(link)) + assert managed_dir_for(row, tmp_path / "sources", upload) is None + + +def test_managed_dir_local_under_upload_dir_symlink_still_maps(tmp_path: Path) -> None: + """Mirror image: a link that stays under the upload dir resolves + inside it — the stored path is still the app-managed dir.""" + upload = tmp_path / "uploads" + real = upload / "real" + upload.mkdir() + real.mkdir() + link = upload / "alias" + link.symlink_to(real) + row = _local(str(link)) + assert managed_dir_for(row, tmp_path / "sources", upload) == link + + +# --------------------------------------------------------------------------- +# remove_managed_dir +# --------------------------------------------------------------------------- + + +def test_remove_managed_dir_none_is_noop_false() -> None: + assert remove_managed_dir(None) is False + + +def test_remove_managed_dir_absent_is_noop_false(tmp_path: Path) -> None: + missing = tmp_path / "never-created" + assert remove_managed_dir(missing) is False + assert not missing.exists() # no filesystem write + + +def test_remove_managed_dir_present_tree_removed_true(tmp_path: Path) -> None: + tree = tmp_path / "homelab" + (tree / "sub").mkdir(parents=True) + (tree / "alpha.md").write_text("one") + (tree / "sub" / "bravo.md").write_text("two") + assert remove_managed_dir(tree) is True + assert not tree.exists() + + +def test_remove_managed_dir_oserror_logged_not_fatal( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + """A permission/busy-dir failure is ``logger.exception`` + ``False`` + — removal never raises (the DB removal is already committed).""" + stuck = tmp_path / "stuck" + stuck.mkdir() + (stuck / "alpha.md").write_text("one") + + def boom(directory: Path) -> None: + raise OSError(13, "Permission denied", str(directory)) + + monkeypatch.setattr(source_removal.shutil, "rmtree", boom) + with caplog.at_level(logging.ERROR, logger="app.rag.source_removal"): + assert remove_managed_dir(stuck) is False + # The exception was logged (with the traceback) and the dir is as it + # was (the rmtree never ran) — inert, self-heals on re-add. + assert stuck.is_dir() + errors = [r for r in caplog.records if r.levelno >= logging.ERROR] + assert len(errors) == 1 + assert "could not remove the on-disk directory" in errors[0].getMessage() + assert errors[0].exc_info is not None + + +# --------------------------------------------------------------------------- +# has_sibling +# --------------------------------------------------------------------------- + + +class _FakeScalars: + def __init__(self, rows: list[GitSource]) -> None: + self._rows = rows + + def all(self) -> list[GitSource]: + return list(self._rows) + + +class _FakeSession: + """The duck-typed seam: ``has_sibling`` only calls + ``db.scalars(select(GitSource))`` (the statement builds fine without + a connection) — the fake returns the registry rows it was given.""" + + def __init__(self, rows: list[GitSource]) -> None: + self._rows = rows + + def scalars(self, statement: object) -> _FakeScalars: # noqa: ARG002 + return _FakeScalars(self._rows) + + +def _sibling(rows: list[GitSource], row: GitSource) -> bool: + """``has_sibling`` against a fake registry session — the duck-typed + fake goes through the seam via ``cast`` (the ``test_agent.py`` + pattern).""" + return has_sibling(cast("Session", _FakeSession(rows)), row) + + +def test_has_sibling_same_name_git_dot_git_pair() -> None: + a = _git("https://example.com/reese/r") + b = _git("https://example.com/reese/r.git") + assert _sibling([a, b], a) is True + assert _sibling([a, b], b) is True + + +def test_has_sibling_different_names_is_false() -> None: + a = _git("https://example.com/reese/one.git") + b = _git("https://example.com/reese/two.git") + assert _sibling([a, b], a) is False + assert _sibling([a, b], b) is False + + +def test_has_sibling_self_excluded() -> None: + a = _git("https://example.com/reese/only.git") + assert _sibling([a], a) is False + + +def test_has_sibling_empty_registry_is_false() -> None: + a = _git("https://example.com/reese/only.git") + assert _sibling([], a) is False + + +def test_has_sibling_across_kinds_same_resolved_name() -> None: + """A git URL whose ``repo_name`` equals a local dir name is a + sibling too — both index under the same label.""" + git_row = _git("https://example.com/reese/notes.git") + local_row = _local("/srv/docs/notes") + assert _sibling([git_row, local_row], git_row) is True + assert _sibling([git_row, local_row], local_row) is True