feat(sources): removing a source deletes its files and index entries behind a confirmation modal
This commit is contained in:
@@ -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/<repo>/`), unpacked uploads under `settings.upload_dir`
|
||||
(`~/bor-sources/uploads/<name>/`) — 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/<repo>/` 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"
|
||||
```
|
||||
@@ -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).
|
||||
@@ -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` (`<h2>`): "Remove this source?"
|
||||
- `#remove-confirm-source` — a `<code>` 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 `<p>` with the locked policy text (below).
|
||||
- `#remove-confirm-error` — a `<p role="alert">`, `hidden` initially.
|
||||
- `#remove-confirm-cancel` (`<button type="button">`, "Cancel") and
|
||||
`#remove-confirm-remove` (`<button type="button">`, destructive style class,
|
||||
"Remove source").
|
||||
- Update `#git-sources-hint` (currently L237–243: "…removing a source prunes its
|
||||
documents from the index on the next sync") to the new contract: removing a
|
||||
source immediately removes its entry, its indexed documents, and — for git
|
||||
clones and uploaded archives — its files from disk (a confirmation modal spells
|
||||
this out); the Sync button still mirrors the remaining sources (upstream churn
|
||||
still prunes on that run).
|
||||
2. `frontend/assets/git-sources.js` — rework the remove flow (the per-row Remove
|
||||
button now opens the modal instead of calling `window.confirm`):
|
||||
- `openRemoveConfirm(s, triggerBtn)` — populate `#remove-confirm-source`
|
||||
(`textContent` = the row value, `s.path ?? s.url` for local rows, `s.url` for
|
||||
git — the same `value` expression `makeRow` uses), clear
|
||||
`#remove-confirm-error`, unhide the dialog, and **focus
|
||||
`#remove-confirm-cancel`** (the safe default for a destructive action); record
|
||||
`triggerBtn` for focus return.
|
||||
- While open: `Escape` (keydown on the dialog/document) and the Cancel button
|
||||
and a click on the dim backdrop all close it as **cancel** — hide the dialog,
|
||||
clear the error line, return focus to `triggerBtn`, and send no request.
|
||||
- `#remove-confirm-remove` click — the §7.4 never-stale lifecycle, inside the
|
||||
modal: clear the error line; disable **both** buttons (Escape/backdrop
|
||||
cancel are no-ops while the request is out) + relabel the confirm button
|
||||
"Removing…". The in-flight state covers the whole server-side cleanup
|
||||
(DB prune → file removal → best-effort overview refresh) — the same
|
||||
"wait for the terminal state" pattern as the Sync/Upload processing
|
||||
states, so a slow LLM refresh is expected, not a stuck button. Note in the
|
||||
module docstring: navigating away mid-removal is not recommended — the row
|
||||
+ index commit first, so the KB stays consistent; a rare interrupted file
|
||||
step leaves an inert orphan dir (no row → never imported again).
|
||||
`fetch DELETE /api/git-sources/{id}` →
|
||||
- ok (204): close the dialog (focus return),
|
||||
`announce("Source removed — its files and index entries were cleaned up.")`
|
||||
(the existing `#git-sources-announcer`), `loadSources()`.
|
||||
- non-2xx: `#remove-confirm-error` = `await apiDetail(r, "Could not remove the
|
||||
source — try again.")`, re-enable both buttons + relabel ("Remove source"),
|
||||
dialog stays open (the fix is one retry, not a re-search for the row).
|
||||
- network failure: fixed line "Could not remove the source — is the app
|
||||
reachable?" + both buttons re-enabled.
|
||||
- Update the module docstring: the "remove" bullet (L69–76) and the "Scope
|
||||
boundary" paragraph (L82–87) — removal now performs the full cleanup
|
||||
server-side (row + index + app-managed files); the modal states that; the
|
||||
page's hint box matches.
|
||||
3. `frontend/assets/styles.css` — modal styling using existing design tokens only
|
||||
(no CDN — AGENTS.md rule 6): a fixed full-viewport dim backdrop + a centered
|
||||
dialog card (max-width ~46rem chat-column width or narrower); destructive button
|
||||
aligned with the existing error-token pairs (`.tuning-ne` / `.steering-ne`
|
||||
convention — text contrast ≥4.5:1); the house 3px `:focus-visible` outline on
|
||||
both buttons and the dialog (focus lands visibly on Cancel at open); both
|
||||
buttons ≥44px tall; no new animation (reduced-motion safe by construction).
|
||||
4. `tests/unit/test_remove_confirm_modal.py` (new — the source-parsing house pattern
|
||||
of `test_stale_ui_copy.py` / `test_summary_edit_ui.py`):
|
||||
- `window.confirm` is **absent** from `frontend/assets/git-sources.js` (and
|
||||
nowhere else in `frontend/`).
|
||||
- `git-sources.html` carries `#remove-confirm-dialog` with
|
||||
`role="alertdialog"`, `aria-modal`, `aria-labelledby`, `aria-describedby`, and
|
||||
the `#remove-confirm-title` / `#remove-confirm-source` / `#remove-confirm-copy`
|
||||
/ `#remove-confirm-error` (`role="alert"`) / `#remove-confirm-cancel` /
|
||||
`#remove-confirm-remove` ids.
|
||||
- `git-sources.js` wires the lifecycle: focus-on-open (Cancel), Escape handling,
|
||||
backdrop cancel, the "Removing…" in-flight label, the success announce string,
|
||||
focus return to the trigger, and `textContent` population of
|
||||
`#remove-confirm-source` (no `innerHTML` on that node).
|
||||
- Stale-copy pins: "stays indexed until the next sync" and "prunes … on the next
|
||||
sync" absent from `git-sources.js` + `git-sources.html`; the new hint copy
|
||||
present in `git-sources.html` (README is pinned in task 03).
|
||||
|
||||
## Locked decisions
|
||||
- **Modal copy** (the `#remove-confirm-copy` text, verbatim):
|
||||
"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." One fixed paragraph for both kinds (the UI does not know whether a
|
||||
local row is an upload or the owner's own directory — the fixed policy text is
|
||||
accurate for both; naming the source above it makes the target unambiguous).
|
||||
- Cancel is the safe default: focus lands on **Cancel** at open; Escape and
|
||||
backdrop clicks cancel; only the explicit "Remove source" button sends the
|
||||
request. While the DELETE is in flight both buttons disable (no second
|
||||
request, no half-cancel of an in-progress server-side removal). Focus returns
|
||||
to the row's Remove button on any close (WCAG 2.1).
|
||||
- The modal is page-local (no shared component extracted — one caller; extraction
|
||||
is a later-phase concern if a second destructive flow appears).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: `tests/unit/test_remove_confirm_modal.py` (pins above); the a11y
|
||||
interaction assertions (focus, Escape, ≥44px, focus-visible) are E2E-pinned in
|
||||
task 03.
|
||||
- Coverage: no `app/` code changes in this task — the `>90%` gate stays green as
|
||||
part of the full suite run.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `rg window.confirm frontend/` → no matches.
|
||||
- [ ] Open (focus on Cancel) → confirm → "Removing…" → success path closes the
|
||||
modal, announces, and reloads the list; failure path shows the in-modal
|
||||
`role="alert"` line and re-enables the button; Cancel/Esc/backdrop close
|
||||
without a request and return focus to the trigger.
|
||||
- [ ] `uv run pytest tests/unit/test_remove_confirm_modal.py -v --no-cov` green;
|
||||
`uv run pytest` green; `uv run ruff check . && uv run pyright` clean.
|
||||
- [ ] `#git-sources-hint` + module docstring carry the new contract; no stale
|
||||
"next sync" removal copy in `git-sources.html` / `git-sources.js`.
|
||||
@@ -0,0 +1,112 @@
|
||||
# Task 03 — E2E Suite, Regression Update, README Copy, Commit
|
||||
|
||||
**Phase:** `69_source_removal_cleanup` · **Story:** n/a (owner request from chat, 2026-09-02)
|
||||
|
||||
## Objective
|
||||
Prove the full removal end-to-end (modal → API → disk + DB) in a dedicated
|
||||
isolated Playwright suite, update the existing admin E2E to the modal, fix the
|
||||
README's removal copy, and pass the full phase gates with one commit.
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_source_removal_cleanup.py` (new — house docstring header:
|
||||
"Phase 69 story E2E … run in isolation (DB must be up): `uv run pytest
|
||||
tests/e2e/test_source_removal_cleanup.py -v --no-cov`", test → mapping list):
|
||||
- Setup patterns from the existing suites: session app + mock LLM via
|
||||
`tests/e2e/conftest.py`; `login(page, app_url, next=GIT_SOURCES_URL)`
|
||||
(the `test_archive_upload_sources.py` form-login helper); in-test archive
|
||||
built with `tarfile` (that suite's L190 pattern); rows + `Document` rows
|
||||
seeded through `SessionLocal` (`test_git_sources_admin.py` pattern);
|
||||
**disk paths resolved exactly like the app**: `Path(get_settings().sources_dir).expanduser()`
|
||||
/ `…(upload_dir)` — the E2E conftest does not override those vars and pytest
|
||||
runs with `cwd=REPO`, so the test process and the app process resolve the
|
||||
same `.env` (the test process also sets the same forced env the conftest
|
||||
sets for the app where it matters — sources/upload dirs are NOT among them).
|
||||
- The suite triggers **no sync** (git rows are `example.com` URLs, never
|
||||
cloned; the only real artifact is the API-driven upload of a small archive
|
||||
with a unique name `phase69-<8-hex-chars>.tar.gz` containing one `.md` file).
|
||||
- Mapped tests:
|
||||
1. `test_uploaded_source_removal_cleans_index_and_disk` — upload the
|
||||
uniquely named archive via the logged-in `page.request` POST
|
||||
(`/api/git-sources/upload`, 202), poll `GET /api/git-sources/upload/status`
|
||||
to `success`; assert the folder
|
||||
`<upload_dir>/<name>/` exists on disk (pathlib) and the source's document
|
||||
appears in `GET /api/docs` (admin); then in the UI: that row's Remove →
|
||||
modal visible (`role="alertdialog"`, `#remove-confirm-source` text = the
|
||||
upload path, focus on `#remove-confirm-cancel`) → click "Remove source" →
|
||||
"Removing…" state → row gone; `GET /api/docs` shows no document for the
|
||||
source; **the folder is gone from disk**; the announcer carries the
|
||||
success line.
|
||||
2. `test_git_source_removal_removes_checkout_and_index` — seed a git row
|
||||
(`SessionLocal`, deterministic `https://example.com/reese/phase69-gone.git`
|
||||
URL), create the checkout dir
|
||||
`<sources_dir>/<repo_name(url)>/` with a marker file, seed a `Document`
|
||||
row for source `repo_name(url)`; UI delete via the modal → row gone,
|
||||
**checkout dir gone from disk** (marker included), document gone from
|
||||
`GET /api/docs`.
|
||||
3. `test_git_source_removal_without_checkout_succeeds` — seed the row, no
|
||||
checkout dir (never synced) → UI delete → row gone (the absent-dir
|
||||
no-op path; no error state anywhere on the page).
|
||||
4. `test_local_directory_source_files_never_deleted` — create a `tmp_path`
|
||||
user dir with a marker file, seed a `kind='local'` row
|
||||
(`path=str(dir)`) + a `Document` row for the dir's basename source; UI
|
||||
delete → row gone, document pruned from `GET /api/docs`, **dir + marker
|
||||
file still present** (pathlib).
|
||||
5. `test_remove_modal_cancel_and_esc_keep_everything` — (a) Remove → modal →
|
||||
click Cancel → row stays, **zero DELETE requests** (`page.on("request")`
|
||||
tracker, the `test_git_sources_admin.py` pattern), document remains;
|
||||
(b) re-open → `page.keyboard.press("Escape")` → dialog hidden, focus back
|
||||
on the trigger button, still zero DELETEs.
|
||||
6. `test_remove_modal_a11y_and_no_cdn` — dialog aria attributes
|
||||
(`aria-modal`, `aria-labelledby`, `aria-describedby`); `:focus-visible`
|
||||
3px outline computable on both buttons; both buttons ≥44px tall; after a
|
||||
successful removal the `#git-sources-announcer` (role=status) carries the
|
||||
success line; the page loads only same-origin resources (AGENTS.md
|
||||
rule 6 — the no-CDN pin pattern from `test_git_sources_admin.py`).
|
||||
2. `tests/e2e/test_git_sources_admin.py` — update test 4's remove section and the
|
||||
module docstring: replace the `page.on("dialog")` handler with modal
|
||||
interactions — accept = click `#remove-confirm-remove`; cancel = click
|
||||
`#remove-confirm-cancel`. Keep the request-tracker assertions (exactly one
|
||||
DELETE on accept; none on cancel) and the "seed row survived" checks. Remove
|
||||
the `Dialog` import if now unused. Everything else in the suite is untouched.
|
||||
3. `README.md` — update the stale removal contract (the unit copy-pins cover the
|
||||
frontend; the README is checked here):
|
||||
- git-sources section (~L129–142: "Adding/removing does not clone …" and the
|
||||
"a removed source's documents leave the index" line) and local-sources
|
||||
section (~L404–410: "Removing the row on the page stops the directory being
|
||||
a source; its … [documents leave the index on the next sync]") → the new
|
||||
contract: removing a source (a confirmation modal states it first)
|
||||
immediately removes its entry, its indexed documents, and — for git clones
|
||||
and uploaded archives — its files from disk; files in the owner's own local
|
||||
directories are never touched; Sync still prunes upstream file churn.
|
||||
4. Gates + commit (in order):
|
||||
- `uv run pytest --cov=app --cov-report=term-missing` — green, 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: `podman compose up -d db`).
|
||||
- Regression suites, each in isolation:
|
||||
`uv run pytest tests/e2e/test_git_sources_admin.py -v --no-cov`,
|
||||
`uv run pytest tests/e2e/test_archive_upload_sources.py -v --no-cov`.
|
||||
- Move the phase dir: `mv .agent/phases/todo/69_source_removal_cleanup
|
||||
.agent/phases/complete/`.
|
||||
- Commit (AGENTS.md rule 8):
|
||||
`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"`
|
||||
|
||||
## Testing & Quality
|
||||
- The dedicated E2E file (6 mapped tests) **is** this phase's Playwright suite —
|
||||
it runs in isolation per AGENTS.md rules 4/9 and covers the backend cleanup
|
||||
(disk + DB), the modal UX, the cancel/Esc paths, and the a11y/no-CDN basics.
|
||||
- Coverage: **>90%** on `app/` (the full-suite gate above).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/e2e/test_source_removal_cleanup.py -v --no-cov` green in
|
||||
isolation; all six mapped behaviors hold, incl. the three disk assertions
|
||||
(upload folder removed, checkout removed, foreign local dir untouched).
|
||||
- [ ] `test_git_sources_admin.py` green in isolation with the modal (no Playwright
|
||||
`dialog` handler left in it); `test_archive_upload_sources.py` green in
|
||||
isolation.
|
||||
- [ ] `uv run pytest` green; `uv run pytest --cov=app` TOTAL >90%;
|
||||
`uv run ruff check . && uv run pyright` clean.
|
||||
- [ ] README removal copy matches the implemented contract (no "next sync"
|
||||
removal semantics left in the two sections above).
|
||||
- [ ] One `--no-gpg-sign` commit with the phase dir in
|
||||
`.agent/phases/complete/69_source_removal_cleanup/`.
|
||||
+19
@@ -0,0 +1,19 @@
|
||||
All gates green. Final state verified:
|
||||
|
||||
**Report — Task 01: Full-Removal Backend (phase 69)**
|
||||
|
||||
**Implemented**
|
||||
- `app/rag/source_removal.py` (new, no FastAPI): `resolve_source_name` (reuses `repo_name` / sync's local-path expression), `managed_dir_for` (git → `sources_dir/<repo>`, local → only `upload_dir` itself or nested under it via resolved-path containment — prefix-siblings and escaping symlinks → `None`), `remove_managed_dir` (never raises; `OSError` → `logger.exception` + `False`), `has_sibling` (Python-side name comparison, self-excluded)
|
||||
- `app/api/git_sources.py::delete_git_source` rewired (now `async def`): sibling guard → row-only; DB-first (docs + row, one commit, cascade drops chunks/embeddings); disk second (app-managed dir only; foreign local dirs skipped); when pruned > 0 → best-effort `regenerate_overview` then exactly one `sources_version` bump in a short-lived session; one `source removed: …` INFO line; 204/404/422 contract unchanged; module + endpoint docstrings updated (Sync `prune=True` = upstream churn only)
|
||||
- New tests: `tests/unit/test_source_removal.py` (27), `tests/integration/test_source_removal_api.py` (8 — full matrix incl. sibling guard, containment edge, LLM-outage best-effort, bump gates, log-line pins); hardened `test_git_sources_api.py::test_delete_removes_row_and_falls_back_to_env` (tmp `sources_dir`/`upload_dir`, KB-table ownership — no rmtree aimed at real `~/bor-sources`)
|
||||
- Fixed pre-existing defect (phase 68 commit `189aa92`): `tests/unit/test_agent.py` still pinned the old tool-description text → 4 pins updated to the intended shortened wording
|
||||
|
||||
**Gates** (all run this session)
|
||||
- `uv run pytest tests/unit/test_source_removal.py tests/integration/test_source_removal_api.py -v --no-cov` → 35 passed
|
||||
- `uv run pytest --cov=app --cov-report=term-missing` → 1352 passed, TOTAL **99%** (source_removal.py 100%, git_sources.py 100%)
|
||||
- `uv run ruff check . && uv run pyright` → clean / 0 errors
|
||||
- Regression: `tests/e2e/test_git_sources_admin.py` 6 passed; `tests/e2e/test_archive_upload_sources.py` 5 passed (in isolation)
|
||||
|
||||
**Decisions**: no commit (task 03 owns the phase commit + phase-dir move); `has_sibling` unit-tested via a duck-typed session + `cast` (house pattern, no SQLite for pgvector/JSONB models).
|
||||
|
||||
**Next pending task**: `.agent/phases/todo/69_source_removal_cleanup/02_confirmation_modal.md`
|
||||
+74
@@ -0,0 +1,74 @@
|
||||
........................................................................ [ 5%]
|
||||
........................................................................ [ 10%]
|
||||
........................................................................ [ 15%]
|
||||
........................................................................ [ 21%]
|
||||
........................................................................ [ 26%]
|
||||
........................................................................ [ 31%]
|
||||
........................................................................ [ 37%]
|
||||
........................................................................ [ 42%]
|
||||
........................................................................ [ 47%]
|
||||
........................................................................ [ 53%]
|
||||
........................................................................ [ 58%]
|
||||
........................................................................ [ 63%]
|
||||
........................................................................ [ 69%]
|
||||
........................................................................ [ 74%]
|
||||
........................................................................ [ 79%]
|
||||
........................................................................ [ 85%]
|
||||
........................................................................ [ 90%]
|
||||
........................................................................ [ 95%]
|
||||
........................................................ [100%]
|
||||
=============================== warnings summary ===============================
|
||||
.venv/lib/python3.13/site-packages/fastapi/testclient.py:1
|
||||
/var/home/ducoterra/Projects/Personal/brain_of_reese/.venv/lib/python3.13/site-packages/fastapi/testclient.py:1: StarletteDeprecationWarning: Using `httpx` with `starlette.testclient` is deprecated; install `httpx2` instead.
|
||||
from starlette.testclient import TestClient as TestClient # noqa
|
||||
|
||||
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
|
||||
================================ tests coverage ================================
|
||||
_______________ coverage: platform linux, python 3.13.13-final-0 _______________
|
||||
|
||||
Name Stmts Miss Cover
|
||||
-----------------------------------------------
|
||||
app/__init__.py 1 0 100%
|
||||
app/api/__init__.py 0 0 100%
|
||||
app/api/auth.py 22 0 100%
|
||||
app/api/chat.py 153 0 100%
|
||||
app/api/chats.py 110 0 100%
|
||||
app/api/config.py 7 0 100%
|
||||
app/api/doc_drafts.py 93 0 100%
|
||||
app/api/docs.py 50 0 100%
|
||||
app/api/git_sources.py 212 0 100%
|
||||
app/api/health.py 10 0 100%
|
||||
app/api/steering.py 42 0 100%
|
||||
app/api/suggestions.py 8 0 100%
|
||||
app/api/sync.py 101 0 100%
|
||||
app/config.py 126 0 100%
|
||||
app/core/__init__.py 0 0 100%
|
||||
app/core/auth.py 20 0 100%
|
||||
app/core/caching.py 108 0 100%
|
||||
app/core/debugging.py 29 2 93%
|
||||
app/core/docs_push.py 39 0 100%
|
||||
app/core/logging.py 13 0 100%
|
||||
app/db.py 21 0 100%
|
||||
app/main.py 52 0 100%
|
||||
app/models.py 86 0 100%
|
||||
app/rag/__init__.py 0 0 100%
|
||||
app/rag/agent.py 120 0 100%
|
||||
app/rag/archive_upload.py 128 0 100%
|
||||
app/rag/chunker.py 206 4 98%
|
||||
app/rag/git_sources.py 14 0 100%
|
||||
app/rag/importer.py 180 3 98%
|
||||
app/rag/llm.py 213 0 100%
|
||||
app/rag/overview.py 71 0 100%
|
||||
app/rag/prompts.py 65 0 100%
|
||||
app/rag/retriever.py 94 3 97%
|
||||
app/rag/source_removal.py 41 0 100%
|
||||
app/rag/sources_meta.py 16 0 100%
|
||||
app/rag/suggestions.py 27 0 100%
|
||||
app/rag/summarizer.py 24 0 100%
|
||||
app/schemas.py 205 0 100%
|
||||
-----------------------------------------------
|
||||
TOTAL 2707 12 99%
|
||||
coverage gate: app/ 99% (>90%) OK
|
||||
All checks passed!
|
||||
0 errors, 0 warnings, 0 informations
|
||||
validation OK
|
||||
+19
@@ -0,0 +1,19 @@
|
||||
Everything is green and verified. Final report:
|
||||
|
||||
## Task 02 complete — confirmation modal on the Git sources page
|
||||
|
||||
**Implemented**
|
||||
- `frontend/git-sources.html`: static `#remove-confirm-dialog` (`role="alertdialog"`, `aria-modal`, labelled/describedby, hidden) inside `#git-sources-content` — locked title/copy verbatim, `#remove-confirm-source` `<code>`, `role="alert"` error line, Cancel + "Remove source" `type="button"`; `#git-sources-hint` rewritten to the total-removal contract
|
||||
- `frontend/assets/git-sources.js`: `window.confirm` flow replaced — `openRemoveConfirm` (textContent-only source, makeRow's `value` expr, focus on Cancel, trigger recorded), Escape/Cancel/backdrop cancel (no request, focus return, in-flight no-op), `confirmRemove` §7.4 lifecycle ("Removing…", both buttons disabled, in-modal error on non-2xx, fixed reachable? line, re-enable in finally, success → close/announce/`loadSources()`); module docstring "remove" bullet + scope boundary updated; dead per-row error span retired
|
||||
- `frontend/assets/styles.css`: `.remove-confirm-*` modal on phase-08 tokens only (46rem cap, no blur, ≥44px buttons, err-token destructive pair 9.3:1/5.2:1, global 3px focus-visible); dead `.git-source-row-error` CSS removed
|
||||
- `tests/unit/test_remove_confirm_modal.py` (new, 12 tests, house source-parsing pattern): no `window.confirm` anywhere in `frontend/`, dialog markup/aria pins, full JS lifecycle order pins, stale "next sync" copy gone, new hint + docstring copy present, CSS token/contrast pins
|
||||
|
||||
**Results**
|
||||
- `uv run pytest tests/unit/test_remove_confirm_modal.py -v --no-cov` → 12 passed
|
||||
- `uv run pytest` (unit+integration) → green (exit 0); `uv run pytest --cov=app` TOTAL **99%** (>90%)
|
||||
- `uv run ruff check . && uv run pyright` → clean; `rg window.confirm frontend/` → no matches
|
||||
- Bonus: real-browser Playwright smoke (temp DB, since deleted; owner's DB restored) verified Escape/backdrop/confirm paths, focus contract, 204 → row + on-disk checkout gone
|
||||
|
||||
**Decisions**: backdrop implemented as its own `aria-hidden` element with a direct cancel listener (doc-modal overlay contract); small Tab/Shift+Tab cycle added so `aria-modal` holds for keyboard users; task's `.tuning-ne`/`.steering-ne` refs map to the actual `.tuning-delete`/`.steering-delete` err-pair convention.
|
||||
|
||||
**Next pending task**: `.agent/phases/todo/69_source_removal_cleanup/03_e2e_and_commit.md` (note: `tests/e2e/test_git_sources_admin.py` test 4 still pins the old dialog flow — red by design until task 03 rewrites it).
|
||||
+74
@@ -0,0 +1,74 @@
|
||||
........................................................................ [ 5%]
|
||||
........................................................................ [ 10%]
|
||||
........................................................................ [ 15%]
|
||||
........................................................................ [ 21%]
|
||||
........................................................................ [ 26%]
|
||||
........................................................................ [ 31%]
|
||||
........................................................................ [ 36%]
|
||||
........................................................................ [ 42%]
|
||||
........................................................................ [ 47%]
|
||||
........................................................................ [ 52%]
|
||||
........................................................................ [ 58%]
|
||||
........................................................................ [ 63%]
|
||||
........................................................................ [ 68%]
|
||||
........................................................................ [ 73%]
|
||||
........................................................................ [ 79%]
|
||||
........................................................................ [ 84%]
|
||||
........................................................................ [ 89%]
|
||||
........................................................................ [ 95%]
|
||||
.................................................................... [100%]
|
||||
=============================== warnings summary ===============================
|
||||
.venv/lib/python3.13/site-packages/fastapi/testclient.py:1
|
||||
/var/home/ducoterra/Projects/Personal/brain_of_reese/.venv/lib/python3.13/site-packages/fastapi/testclient.py:1: StarletteDeprecationWarning: Using `httpx` with `starlette.testclient` is deprecated; install `httpx2` instead.
|
||||
from starlette.testclient import TestClient as TestClient # noqa
|
||||
|
||||
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
|
||||
================================ tests coverage ================================
|
||||
_______________ coverage: platform linux, python 3.13.13-final-0 _______________
|
||||
|
||||
Name Stmts Miss Cover
|
||||
-----------------------------------------------
|
||||
app/__init__.py 1 0 100%
|
||||
app/api/__init__.py 0 0 100%
|
||||
app/api/auth.py 22 0 100%
|
||||
app/api/chat.py 153 0 100%
|
||||
app/api/chats.py 110 0 100%
|
||||
app/api/config.py 7 0 100%
|
||||
app/api/doc_drafts.py 93 0 100%
|
||||
app/api/docs.py 50 0 100%
|
||||
app/api/git_sources.py 212 0 100%
|
||||
app/api/health.py 10 0 100%
|
||||
app/api/steering.py 42 0 100%
|
||||
app/api/suggestions.py 8 0 100%
|
||||
app/api/sync.py 101 0 100%
|
||||
app/config.py 126 0 100%
|
||||
app/core/__init__.py 0 0 100%
|
||||
app/core/auth.py 20 0 100%
|
||||
app/core/caching.py 108 0 100%
|
||||
app/core/debugging.py 29 2 93%
|
||||
app/core/docs_push.py 39 0 100%
|
||||
app/core/logging.py 13 0 100%
|
||||
app/db.py 21 0 100%
|
||||
app/main.py 52 0 100%
|
||||
app/models.py 86 0 100%
|
||||
app/rag/__init__.py 0 0 100%
|
||||
app/rag/agent.py 120 0 100%
|
||||
app/rag/archive_upload.py 128 0 100%
|
||||
app/rag/chunker.py 206 4 98%
|
||||
app/rag/git_sources.py 14 0 100%
|
||||
app/rag/importer.py 180 3 98%
|
||||
app/rag/llm.py 213 0 100%
|
||||
app/rag/overview.py 71 0 100%
|
||||
app/rag/prompts.py 65 0 100%
|
||||
app/rag/retriever.py 94 3 97%
|
||||
app/rag/source_removal.py 41 0 100%
|
||||
app/rag/sources_meta.py 16 0 100%
|
||||
app/rag/suggestions.py 27 0 100%
|
||||
app/rag/summarizer.py 24 0 100%
|
||||
app/schemas.py 205 0 100%
|
||||
-----------------------------------------------
|
||||
TOTAL 2707 12 99%
|
||||
coverage gate: app/ 99% (>90%) OK
|
||||
All checks passed!
|
||||
0 errors, 0 warnings, 0 informations
|
||||
validation OK
|
||||
@@ -137,10 +137,16 @@ uv run uvicorn app.main:app --reload
|
||||
missing/relative path is rejected, naming the path; so are duplicates)
|
||||
— since phase 49 this is an API-only operation (`POST /api/git-sources`
|
||||
with `kind=local`; the page's form was replaced by the upload form). List
|
||||
rows carry a **Git** or **Local** badge. Adding/removing does not clone
|
||||
or prune on its own: the Sync button performs that (git + local together,
|
||||
one run, prune over the union), and a removed source's documents leave
|
||||
the index on the next sync.
|
||||
rows carry a **Git** or **Local** badge. Adding does not clone: the
|
||||
Sync button performs that (git + local together, one run, prune over
|
||||
the union) and still prunes **upstream file churn** — a file deleted
|
||||
in a repo or dropped from a local directory leaves the index on that
|
||||
run. **Removing a source is a total removal, done immediately** — a
|
||||
confirmation modal states it first, then the source's entry, all of
|
||||
its indexed documents, and — for git clones and uploaded archives —
|
||||
its files on disk (the checkout under `BOR_SOURCES_DIR` or the
|
||||
unpacked folder under `BOR_UPLOAD_DIR`) are gone in one action; files
|
||||
in the owner's own local directories are never touched.
|
||||
|
||||
## Thinking
|
||||
|
||||
@@ -406,9 +412,12 @@ table** (the `git_sources` registry with a `kind` discriminator: `git` |
|
||||
as a failing git clone).
|
||||
- **Pruning is over the union** — git checkouts and local directories are
|
||||
imported together with `prune=True`, so a file removed from a local
|
||||
directory, a repo, or a removed source leaves the index on that run.
|
||||
Removing the row on the page stops the directory being a source; its
|
||||
documents leave the index on the next sync (exactly like git sources).
|
||||
directory or a repo leaves the index on that run (upstream file
|
||||
churn). Removing the source on the page is a **total removal, done
|
||||
immediately** (a confirmation modal states it first): its entry and
|
||||
its indexed documents leave at once — but the directory itself is the
|
||||
owner's own and is **never touched on disk** (only git checkouts and
|
||||
uploaded archives get their files deleted).
|
||||
- **`import_docs`** (no `--source`) resolves the stored git **and** local
|
||||
rows — git cloned/pulled as above, local walked directly — in one run;
|
||||
`--source` still wins over everything; while the table is empty,
|
||||
|
||||
+127
-12
@@ -28,8 +28,10 @@ task** — see :func:`upload_archive` and :func:`_run_upload`),
|
||||
state of that run — incl. the phase-64 ``current_file`` /
|
||||
``files_done`` / ``files_total`` progress fields; navigating away from
|
||||
the page mid-scan no longer aborts anything), ``DELETE /{source_id}``
|
||||
(204). The whole router sits behind :func:`app.core.auth.require_admin`
|
||||
— anonymous callers get 403 on every route.
|
||||
(204 — total removal, phase 69: row + the source's documents (chunks +
|
||||
embeddings) committed first, then the app-managed on-disk dir). The
|
||||
whole router sits behind :func:`app.core.auth.require_admin` —
|
||||
anonymous callers get 403 on every route.
|
||||
|
||||
No credential-echo path: git URLs may embed ``user:pass@`` (phase 32's
|
||||
masking discipline), so every git 409/422 detail is a fixed generic
|
||||
@@ -38,9 +40,20 @@ secrets — the local 422/409 details name the (expanded) path so the
|
||||
owner sees exactly which directory failed.
|
||||
|
||||
Scope boundary (phase locked decisions): the CRUD routes do NOT
|
||||
clone, import, or prune anything — the existing Sync button performs
|
||||
that (a removal prunes on the next sync, ``prune=True``). The upload
|
||||
route is the exception (phase 64, task 03): after the 202 receive
|
||||
clone or import anything — the existing Sync button performs that, and
|
||||
its ``prune=True`` stays for **upstream file churn** (files deleted
|
||||
upstream or dropped from a local dir), not for row removal: ``DELETE``
|
||||
is a total removal in itself (phase 69) — the row + the source's
|
||||
documents (chunks + embeddings via the ``all, delete-orphan`` cascade)
|
||||
commit atomically **first** (the RAG is always consistent with the
|
||||
registry), then the app-managed on-disk dir (git checkout / unpacked
|
||||
upload folder) is deleted; an ``OSError`` there is logged, never fatal.
|
||||
Foreign local dirs (the owner's own) are never touched on disk, a
|
||||
sibling row sharing the source name keeps the shared documents +
|
||||
files (only the row goes), and a pruned KB bumps ``sources_version``
|
||||
exactly once (the phase-53 saved-chat invalidation) with a
|
||||
best-effort overview refresh. The upload route is the other exception
|
||||
(phase 64, task 03): after the 202 receive
|
||||
answer, its background task unpacks the archive, swaps it in, upserts
|
||||
the row, probes the models, scans the single source
|
||||
(``import_sources`` with ``prune=True`` + the change-gated overview
|
||||
@@ -69,7 +82,7 @@ from app.api.sync import _sanitize_error
|
||||
from app.config import get_settings
|
||||
from app.core.auth import require_admin
|
||||
from app.db import SessionLocal, get_db
|
||||
from app.models import GitSource
|
||||
from app.models import Document, GitSource
|
||||
from app.rag.archive_upload import (
|
||||
ARCHIVE_SUFFIXES,
|
||||
ArchiveUploadError,
|
||||
@@ -80,6 +93,13 @@ from app.rag.archive_upload import (
|
||||
from app.rag.importer import import_sources
|
||||
from app.rag.llm import LLMClient, check_models
|
||||
from app.rag.overview import regenerate_overview
|
||||
from app.rag.source_removal import (
|
||||
has_sibling,
|
||||
managed_dir_for,
|
||||
remove_managed_dir,
|
||||
resolve_source_name,
|
||||
)
|
||||
from app.rag.sources_meta import bump_sources_version
|
||||
from app.schemas import (
|
||||
GitSourceIn,
|
||||
GitSourceList,
|
||||
@@ -564,18 +584,113 @@ async def _run_upload(
|
||||
|
||||
|
||||
@router.delete("/{source_id}", status_code=204)
|
||||
def delete_git_source(
|
||||
async def delete_git_source(
|
||||
source_id: uuid.UUID,
|
||||
db: Session = Depends(get_db), # noqa: B008
|
||||
) -> Response:
|
||||
"""Remove a stored row; 404 when the id is unknown.
|
||||
"""Remove a stored source — **totally** (phase 69); 404 unknown id.
|
||||
|
||||
Removing does not touch the clones or the index — the next Sync
|
||||
(``prune=True``) prunes the dropped repo (phase scope boundary).
|
||||
Total removal, one action: the row, every indexed document of the
|
||||
source (chunks + embeddings via the ``all, delete-orphan`` cascade
|
||||
— ``app.models``), and, for app-managed sources, the on-disk dir
|
||||
(the git checkout or the unpacked upload folder). Foreign local
|
||||
directories (the owner's own) are never touched on disk — their row
|
||||
+ index entries still go.
|
||||
|
||||
Locked order (phase 69):
|
||||
|
||||
1. **Sibling guard** — another stored row resolves to the same
|
||||
source name (e.g. ``…/r`` and ``…/r.git``): it still owns the
|
||||
shared documents + files, so only this row is deleted (logged
|
||||
loudly); no index/disk work.
|
||||
2. **DB first** — in the request transaction: the source's
|
||||
documents are deleted, then the row, one ``commit``. A DB
|
||||
failure propagates as 500 **before any disk work** — the 204
|
||||
contract below never sees a half-removal.
|
||||
3. **Disk second** — the app-managed dir is removed after the
|
||||
commit; an ``OSError`` is logged, never fatal (a leftover dir
|
||||
is inert and self-heals on re-add; the reverse order is
|
||||
forbidden — a disk failure must never leave a row pointing at
|
||||
deleted files). Foreign local dir → skipped.
|
||||
4. **When documents were pruned** — the best-effort overview
|
||||
refresh (an LLM failure logs, never fails the 204; the next
|
||||
added/updated change refreshes it, as today) and then exactly
|
||||
one ``sources_version`` bump (phase 53 — the bump lands even if
|
||||
the overview failed, mirroring ``_run_sync``'s order). No docs
|
||||
pruned → no overview, no bump.
|
||||
|
||||
The route contract is unchanged: 204 with no body; the per-
|
||||
operation INFO line (PLAN §9) carries the counts —
|
||||
``files_removed`` is ``yes|no|skipped`` (``skipped`` for the
|
||||
sibling guard and for foreign local dirs).
|
||||
"""
|
||||
started = time.monotonic()
|
||||
row = db.get(GitSource, source_id)
|
||||
if row is None:
|
||||
raise HTTPException(status_code=404, detail="git source not found")
|
||||
db.delete(row)
|
||||
db.commit()
|
||||
|
||||
name = resolve_source_name(row)
|
||||
settings = get_settings()
|
||||
managed = managed_dir_for(row, Path(settings.sources_dir), Path(settings.upload_dir))
|
||||
|
||||
if has_sibling(db, row):
|
||||
# Sibling guard: the shared documents + files still belong to
|
||||
# the sibling row — only this row goes (phase 69 locked
|
||||
# decision), so no index prune and no disk work.
|
||||
logger.warning(
|
||||
"source removal: row %s (url=%s) shares source name %s with "
|
||||
"another stored row — deleting only the row; the shared "
|
||||
"documents and files stay",
|
||||
row.id,
|
||||
row.url,
|
||||
name,
|
||||
)
|
||||
db.delete(row)
|
||||
db.commit()
|
||||
docs_pruned, files_removed, overview = 0, "skipped", False
|
||||
else:
|
||||
# DB first: prune every document of the source (the cascade
|
||||
# drops all chunks incl. embeddings) and the row itself in one
|
||||
# commit — the RAG is always consistent with the registry.
|
||||
docs = db.scalars(select(Document).where(Document.source == name)).all()
|
||||
for doc in docs:
|
||||
db.delete(doc)
|
||||
db.delete(row)
|
||||
db.commit()
|
||||
docs_pruned = len(docs)
|
||||
# Disk second: only the app-managed dir (git checkout / upload
|
||||
# folder) — ``managed is None`` is the foreign local dir (the
|
||||
# owner's own), never touched on disk.
|
||||
if managed is None:
|
||||
files_removed = "skipped"
|
||||
else:
|
||||
files_removed = "yes" if remove_managed_dir(managed) else "no"
|
||||
# KB changed (documents pruned) → best-effort overview refresh,
|
||||
# then exactly one sources_version bump — the bump in its own
|
||||
# short-lived session (the ``_run_sync`` pattern) so it lands
|
||||
# even if the best-effort overview failed.
|
||||
overview = False
|
||||
if docs_pruned > 0:
|
||||
try:
|
||||
overview = await regenerate_overview(LLMClient())
|
||||
except Exception: # noqa: BLE001 — best-effort: never fail the 204
|
||||
logger.exception("source removal: overview regeneration failed (best-effort)")
|
||||
overview = False
|
||||
bump_db = SessionLocal()
|
||||
try:
|
||||
bump_sources_version(bump_db)
|
||||
bump_db.commit()
|
||||
finally:
|
||||
bump_db.close()
|
||||
|
||||
logger.info(
|
||||
"source removed: kind=%s name=%s docs_pruned=%d files_removed=%s "
|
||||
"overview=%s total_ms=%d",
|
||||
row.kind,
|
||||
name,
|
||||
docs_pruned,
|
||||
files_removed,
|
||||
overview,
|
||||
round((time.monotonic() - started) * 1000),
|
||||
)
|
||||
return Response(status_code=204)
|
||||
|
||||
@@ -0,0 +1,138 @@
|
||||
"""Total source removal helpers (phase 69, task 01).
|
||||
|
||||
Removing a source is a **total removal** (owner request 2026-09-02):
|
||||
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. These
|
||||
helpers carry the row-agnostic pieces (the source-name resolution, the
|
||||
managed-dir mapping, the disk removal, the sibling guard) so the
|
||||
``DELETE /api/git-sources/{id}`` endpoint stays thin.
|
||||
|
||||
Locked order (phase 69, ``00_phase.md``):
|
||||
|
||||
* **DB first, disk second.** The row + document prune commit atomically
|
||||
first (the RAG is always consistent with the registry — the owner's
|
||||
core ask); the disk removal runs **after** the commit, and an
|
||||
``OSError`` is logged but never fails the 204 (a leftover dir is
|
||||
inert — no row → never imported — and self-heals on re-add). The
|
||||
reverse order is forbidden: a disk failure must never leave a row
|
||||
pointing at deleted files.
|
||||
* **App-managed files only.** Git checkouts live under
|
||||
``sources_dir/<repo>/`` and unpacked uploads under
|
||||
``upload_dir/<name>/`` — both app-managed. A ``kind='local'`` row
|
||||
pointing at any other directory (the owner's own) is never touched
|
||||
on disk — only its row + index entries are removed.
|
||||
* **Sibling guard.** Two rows that resolve to the same source name
|
||||
(e.g. ``https://e.com/r`` and ``https://e.com/r.git`` → both ``r``)
|
||||
share documents and files — removing one of them deletes only the
|
||||
row; the shared documents and files stay.
|
||||
|
||||
The module is stdlib + SQLAlchemy + ``app``/``scripts`` imports only
|
||||
(**no FastAPI**), so every helper unit-tests with plain objects and
|
||||
``tmp_path``.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import shutil
|
||||
from pathlib import Path
|
||||
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app.models import GitSource
|
||||
from scripts.import_docs import repo_name
|
||||
|
||||
logger = logging.getLogger("app.rag.source_removal")
|
||||
|
||||
|
||||
def resolve_source_name(row: GitSource) -> str:
|
||||
"""The label under which the sync/importer index this row's documents.
|
||||
|
||||
Exactly the expressions the import pipeline walks — reuse them here
|
||||
or removal would prune the wrong documents: ``kind='git'`` →
|
||||
:func:`scripts.import_docs.repo_name` (the phase-28 helper — strips
|
||||
the trailing ``.git`` and handles scp-style ``git@host:repo``);
|
||||
``kind='local'`` → ``Path(row.path or row.url).expanduser().name``
|
||||
(the same expression ``app.api.sync._run_sync`` walks; phase 38
|
||||
mirrors the expanded path in the NOT-NULL ``url`` column, the
|
||||
``or`` keeps the type checker honest).
|
||||
"""
|
||||
if row.kind == "git":
|
||||
return repo_name(row.url)
|
||||
return Path(row.path or row.url).expanduser().name
|
||||
|
||||
|
||||
def managed_dir_for(row: GitSource, sources_dir: Path, upload_dir: Path) -> Path | None:
|
||||
"""The app-managed on-disk directory whose files belong to ``row``.
|
||||
|
||||
* ``kind='git'`` → ``sources_dir/<repo-name>/`` — the checkout
|
||||
location (the ``clone_or_pull`` target of the sync/import).
|
||||
* ``kind='local'`` → the stored directory **only when** it equals
|
||||
``upload_dir`` or is nested under it (the unpacked uploads,
|
||||
phase 49). Containment is checked on **resolved** paths with
|
||||
``parents``, so a sibling directory whose name merely shares a
|
||||
prefix (``…/uploads-foo`` next to ``…/uploads``) never counts —
|
||||
and a symlink that escapes the upload dir never counts either
|
||||
(``resolve()`` follows it to its target).
|
||||
* any other ``kind='local'`` path → ``None``: the owner's own
|
||||
directory, **never** touched on disk (row + index entries only).
|
||||
|
||||
``None`` as well when containment cannot be established (an
|
||||
unresolvable path) — never delete when in doubt.
|
||||
"""
|
||||
if row.kind == "git":
|
||||
return Path(sources_dir).expanduser() / repo_name(row.url)
|
||||
stored = Path(row.path or row.url).expanduser()
|
||||
upload = Path(upload_dir).expanduser()
|
||||
try:
|
||||
stored_resolved = stored.resolve()
|
||||
upload_resolved = upload.resolve()
|
||||
except OSError:
|
||||
return None
|
||||
if stored_resolved == upload_resolved or upload_resolved in stored_resolved.parents:
|
||||
return stored
|
||||
return None
|
||||
|
||||
|
||||
def remove_managed_dir(directory: Path | None) -> bool:
|
||||
"""Delete the app-managed directory (recursively), best-effort.
|
||||
|
||||
``None`` or an absent directory → ``False`` (a no-op, no
|
||||
filesystem write — the row may simply have no checkout yet).
|
||||
Present → ``shutil.rmtree`` → ``True`` (only a dir that was
|
||||
actually removed). An ``OSError`` (permissions, a busy file, a
|
||||
symlink-to-dir refusal, …) is logged with ``logger.exception`` and
|
||||
returns ``False`` — this function **never raises**: the row + index
|
||||
are already committed at call time, and a leftover dir is inert
|
||||
(no row → never imported) and self-heals on re-add (git re-clones,
|
||||
a re-upload recreates the folder).
|
||||
"""
|
||||
if directory is None or not directory.exists():
|
||||
return False
|
||||
try:
|
||||
shutil.rmtree(directory)
|
||||
except OSError:
|
||||
logger.exception(
|
||||
"source removal: could not remove the on-disk directory %s", directory
|
||||
)
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def has_sibling(db: Session, row: GitSource) -> bool:
|
||||
"""Whether another stored row resolves to the same source name.
|
||||
|
||||
The registry is tiny — every row is resolved in Python (no SQL
|
||||
trickery): ``True`` when a different-id row's
|
||||
:func:`resolve_source_name` equals this row's (e.g.
|
||||
``https://e.com/r`` and ``https://e.com/r.git`` → both ``r``). Such
|
||||
a sibling still owns the shared documents and files, so the caller
|
||||
must delete **only the row** and skip the index/disk work (the
|
||||
phase-69 sibling guard).
|
||||
"""
|
||||
name = resolve_source_name(row)
|
||||
for other in db.scalars(select(GitSource)).all():
|
||||
if other.id != row.id and resolve_source_name(other) == name:
|
||||
return True
|
||||
return False
|
||||
+223
-45
@@ -66,24 +66,59 @@
|
||||
* branch): a running scan re-enters the processing state + poll
|
||||
* (a reload mid-scan re-attaches — no second upload), a terminal
|
||||
* run re-renders its result line / error banner.
|
||||
* • remove — a row's Remove button asks window.confirm first
|
||||
* (removal prunes the documents only on the NEXT sync — the
|
||||
* confirm says so). Cancel → nothing; ok → the row button
|
||||
* disables, DELETE /api/git-sources/{id}, loadSources(). A
|
||||
* failure shows a per-row role="alert" error and re-enables the
|
||||
* button. Env-fallback rows (id null — the list comes from
|
||||
* BOR_GIT_SOURCES, not the table) carry no Remove: nothing is
|
||||
* stored to remove — they show a "from .env" tag instead.
|
||||
* • remove — a row's Remove button opens the page-local
|
||||
* confirmation modal (#remove-confirm-dialog, a real
|
||||
* role="alertdialog" — the native confirm() retired, phase 69):
|
||||
* it
|
||||
* names the source (#remove-confirm-source, textContent ONLY —
|
||||
* URLs may embed user:pass@ credentials, phase 32) and states
|
||||
* the full-removal policy — the row, the source's indexed
|
||||
* documents (chunks + embeddings), and, for git clones and
|
||||
* uploaded archives, the files on the server's disk, all removed
|
||||
* immediately by the server-side DELETE. Cancel is the safe
|
||||
* default: focus lands on Cancel at open; Escape, the Cancel
|
||||
* button, and the dim backdrop all close as CANCEL (no request —
|
||||
* focus returns to the row's Remove button). "Remove source" runs
|
||||
* the §7.4 in-flight lifecycle IN the modal: both buttons
|
||||
* disable + the confirm relabels "Removing…" while the DELETE is
|
||||
* out — the in-flight window covers the whole server-side
|
||||
* cleanup (DB prune → file removal → best-effort overview
|
||||
* refresh; a slow LLM refresh is expected, not a stuck button —
|
||||
* the same "wait for the terminal state" pattern as the
|
||||
* Sync/Upload processing states). Navigating away mid-removal is
|
||||
* not recommended: the row + index commit first, so the KB stays
|
||||
* consistent; a rare interrupted file step leaves an inert orphan
|
||||
* dir (no row → never imported again). 204 → close (focus
|
||||
* return), loadSources(), then announce — the removal
|
||||
* confirmation is the LAST announcement, so the reload's "N
|
||||
* sources listed." cannot overwrite it (the announcer is the
|
||||
* screen-reader confirmation for the destructive action);
|
||||
* non-2xx → the in-modal role="alert" line (the server detail,
|
||||
* apiDetail) + both
|
||||
* buttons re-enabled + the confirm relabeled "Remove source" (the
|
||||
* dialog stays open — the fix is one retry, not a re-search for
|
||||
* the row); network failure → the fixed "is the app reachable?"
|
||||
* line, same restore. Env-fallback rows (id null — the list
|
||||
* comes from BOR_GIT_SOURCES, not the table) carry no Remove:
|
||||
* nothing is stored to remove — they show a "from .env" tag
|
||||
* instead.
|
||||
* • announce(msg) — #git-sources-announcer (role=status,
|
||||
* aria-live=polite): the screen-reader confirmation for loads,
|
||||
* adds, and removals.
|
||||
*
|
||||
* Scope boundary (phase locked decisions): adding a git repo or
|
||||
* removing a source does NOT clone, import, or prune — the sync
|
||||
* service (server-side) performs that; the page's hint box says so.
|
||||
* The phase-49 upload is the exception: it unpacks and scans the
|
||||
* single source in place (the phase-64 background task — 202 +
|
||||
* status endpoint), and its counts render as the result line.
|
||||
* Scope boundary (phase locked decisions): adding a git repo does
|
||||
* NOT clone — the sync service (server-side) does that. Removing a
|
||||
* source, however, performs the FULL cleanup server-side (phase 69):
|
||||
* the row, the source's indexed documents (chunks + embeddings), and
|
||||
* — for git clones and uploaded archives — the app-managed files on
|
||||
* disk (foreign local directories are never touched), all in one
|
||||
* action; the confirmation modal states exactly that, and the
|
||||
* page's hint box matches. The Sync button still mirrors the
|
||||
* remaining sources (upstream file churn is pruned on that run).
|
||||
* The phase-49 upload is the other in-place exception: it unpacks
|
||||
* and scans the single source in place (the phase-64 background task
|
||||
* — 202 + status endpoint), and its counts render as the result
|
||||
* line.
|
||||
*
|
||||
* The shared header module loads through this script's own relative
|
||||
* import ("./header.js") — a hoisted import evaluated before this body
|
||||
@@ -116,6 +151,15 @@ const tbody = document.querySelector("#git-sources-tbody");
|
||||
const emptyEl = document.querySelector("#git-sources-empty");
|
||||
const envNote = document.querySelector("#git-sources-env-note");
|
||||
const announcer = document.querySelector("#git-sources-announcer");
|
||||
/* Phase 69: the remove confirmation modal (the native confirm()
|
||||
retired) — static markup in git-sources.html; this module owns the
|
||||
open / cancel / confirm lifecycle. */
|
||||
const removeDialog = document.querySelector("#remove-confirm-dialog");
|
||||
const removeBackdrop = document.querySelector(".remove-confirm-backdrop");
|
||||
const removeSourceEl = document.querySelector("#remove-confirm-source");
|
||||
const removeError = document.querySelector("#remove-confirm-error");
|
||||
const removeCancelBtn = document.querySelector("#remove-confirm-cancel");
|
||||
const removeRemoveBtn = document.querySelector("#remove-confirm-remove");
|
||||
|
||||
/* Polite live region: the screen-reader confirmation for loads, adds,
|
||||
and removals (the phase-15 announcer pattern). */
|
||||
@@ -248,12 +292,12 @@ function makeRow(s) {
|
||||
btn.className = "git-source-remove";
|
||||
btn.setAttribute("aria-label", `Remove ${kindLabel} source: ${value}`);
|
||||
btn.innerHTML = REMOVE_ICON + "<span>Remove</span>";
|
||||
const rowError = document.createElement("span");
|
||||
rowError.className = "git-source-row-error";
|
||||
rowError.setAttribute("role", "alert");
|
||||
rowError.hidden = true;
|
||||
btn.addEventListener("click", () => removeSource(s, btn, rowError, kindLabel));
|
||||
actTd.append(btn, rowError);
|
||||
// Phase 69: opens the confirmation modal (the native confirm()
|
||||
// retired) — the modal names the source and states the
|
||||
// full-removal policy; the per-row error span is retired (the
|
||||
// modal carries the in-flight error line).
|
||||
btn.addEventListener("click", () => openRemoveConfirm(s, btn));
|
||||
actTd.appendChild(btn);
|
||||
} else {
|
||||
const tag = document.createElement("span");
|
||||
tag.className = "git-source-env-tag";
|
||||
@@ -264,35 +308,169 @@ function makeRow(s) {
|
||||
return tr;
|
||||
}
|
||||
|
||||
/* ---------- remove (DELETE /api/git-sources/{id}) ----------
|
||||
* Removal does not prune anything immediately — the next sync does
|
||||
* (phase scope boundary), so the confirm says exactly that. Cancel →
|
||||
* nothing; a failed delete → per-row role="alert" error + re-enabled
|
||||
* button (never a stuck row); success → the list reloads. */
|
||||
async function removeSource(s, btn, rowError, kindLabel) {
|
||||
const ok = window.confirm(
|
||||
`Remove this ${kindLabel} source from the list? Its documents stay indexed until the next sync prunes them.`,
|
||||
);
|
||||
if (!ok) return;
|
||||
btn.disabled = true; // one delete per click
|
||||
rowError.hidden = true;
|
||||
try {
|
||||
const r = await fetch(`/api/git-sources/${encodeURIComponent(s.id)}`, { method: "DELETE" });
|
||||
if (!r.ok) {
|
||||
rowError.textContent = await apiDetail(r, `Could not remove the ${kindLabel} source — try again.`);
|
||||
rowError.hidden = false;
|
||||
btn.disabled = false;
|
||||
return;
|
||||
/* ---------- remove (DELETE /api/git-sources/{id}) — the confirmation modal ----------
|
||||
* A row's Remove button opens the page-local alertdialog
|
||||
* (#remove-confirm-dialog — the native confirm() retired, phase 69)
|
||||
* via openRemoveConfirm(s, triggerBtn): #remove-confirm-source shows the
|
||||
* row's value (textContent ONLY — the same `value` expression
|
||||
* makeRow uses: s.path ?? s.url for local rows, s.url for git — URLs
|
||||
* may embed user:pass@ credentials, phase 32), the error line clears,
|
||||
* and focus lands on Cancel (the safe default for a destructive
|
||||
* action). Escape / Cancel / the dim backdrop close as CANCEL: no
|
||||
* request, focus returns to the row's Remove button.
|
||||
*
|
||||
* "Remove source" (confirmRemove) runs the §7.4 never-stale
|
||||
* lifecycle IN the modal: both buttons disable and the confirm
|
||||
* relabels "Removing…" while the DELETE is out — the in-flight
|
||||
* window covers the whole server-side cleanup (DB prune → file
|
||||
* removal → best-effort overview refresh), so a slow LLM refresh is
|
||||
* expected, not a stuck button. Navigating away mid-removal is not
|
||||
* recommended: the row + index commit first, so the KB stays
|
||||
* consistent; a rare interrupted file step leaves an inert orphan
|
||||
* dir (no row → never imported again). 204 → close (focus return),
|
||||
* loadSources(), then announce — the removal confirmation is the
|
||||
* LAST announcement (the reload's "N sources listed." must not
|
||||
* overwrite it — the announcer is the screen-reader confirmation for
|
||||
* the destructive action); non-2xx → the in-modal role="alert" line
|
||||
* (the server detail, apiDetail 422-shape-aware) + both buttons
|
||||
* re-enabled + the confirm relabeled "Remove source" (the dialog
|
||||
* stays open — the fix is one retry, not a re-search for the row);
|
||||
* network failure → the fixed reachable? line, same restore. */
|
||||
let removeTriggerBtn = null; // the row's Remove button — focus returns here on close
|
||||
let removeInFlight = false; // §7.4: a DELETE is out (both buttons disabled)
|
||||
let removingId = null; // the row id of the open/in-flight removal
|
||||
|
||||
function openRemoveConfirm(s, triggerBtn) {
|
||||
if (!removeDialog || !removeSourceEl) return; // defensive — the markup ships with the page
|
||||
if (removeInFlight) return; // one removal at a time
|
||||
const isLocal = s.kind === "local";
|
||||
// The same `value` expression makeRow uses — textContent ONLY.
|
||||
removeSourceEl.textContent = isLocal ? s.path ?? s.url : s.url;
|
||||
if (removeError) {
|
||||
removeError.textContent = "";
|
||||
removeError.hidden = true; // a new attempt starts clean
|
||||
}
|
||||
removingId = s.id;
|
||||
removeInFlight = false;
|
||||
removeTriggerBtn = triggerBtn; // recorded for the focus return on close
|
||||
removeDialog.hidden = false;
|
||||
document.addEventListener("keydown", onRemoveDialogKeydown);
|
||||
// Cancel is the safe default for a destructive action — focus
|
||||
// lands on it (visibly: the global 3px :focus-visible outline).
|
||||
if (removeCancelBtn) removeCancelBtn.focus();
|
||||
}
|
||||
|
||||
/* Any close (cancel, success): hide the dialog, clear the error
|
||||
line, reset the buttons, detach the keydown handling, and return
|
||||
focus to the row's Remove button (WCAG 2.1). */
|
||||
function closeRemoveConfirm() {
|
||||
if (!removeDialog) return;
|
||||
removeDialog.hidden = true;
|
||||
removeInFlight = false;
|
||||
removingId = null;
|
||||
if (removeError) {
|
||||
removeError.textContent = "";
|
||||
removeError.hidden = true;
|
||||
}
|
||||
if (removeCancelBtn) removeCancelBtn.disabled = false;
|
||||
if (removeRemoveBtn) {
|
||||
removeRemoveBtn.disabled = false;
|
||||
removeRemoveBtn.textContent = "Remove source";
|
||||
}
|
||||
document.removeEventListener("keydown", onRemoveDialogKeydown);
|
||||
const trigger = removeTriggerBtn;
|
||||
removeTriggerBtn = null;
|
||||
if (trigger) trigger.focus(); // focus returns to the row's Remove button
|
||||
}
|
||||
|
||||
/* Escape / Cancel / backdrop all close as cancel — NO request. A
|
||||
cancel is a no-op while a DELETE is in flight (no half-cancel of
|
||||
an in-progress server-side removal; the buttons are disabled
|
||||
anyway, the Escape/backdrop paths need this guard). */
|
||||
function cancelRemoveConfirm() {
|
||||
if (removeInFlight) return;
|
||||
closeRemoveConfirm();
|
||||
}
|
||||
|
||||
/* While open (attached in openRemoveConfirm, detached in
|
||||
closeRemoveConfirm): Escape cancels; Tab/Shift+Tab cycle between
|
||||
the modal's two buttons (the only focusable elements — aria-modal
|
||||
is honored for keyboard users, not just screen readers). */
|
||||
function onRemoveDialogKeydown(e) {
|
||||
if (e.key === "Escape") {
|
||||
e.preventDefault();
|
||||
cancelRemoveConfirm();
|
||||
return;
|
||||
}
|
||||
if (e.key === "Tab" && removeCancelBtn && removeRemoveBtn) {
|
||||
const leaving = e.shiftKey ? removeCancelBtn : removeRemoveBtn;
|
||||
const wrapTo = e.shiftKey ? removeRemoveBtn : removeCancelBtn;
|
||||
if (document.activeElement === leaving) {
|
||||
e.preventDefault();
|
||||
wrapTo.focus();
|
||||
}
|
||||
announce("Source removed.");
|
||||
await loadSources(); // 204: the server confirmed — the list re-renders
|
||||
} catch {
|
||||
rowError.textContent = `Could not remove the ${kindLabel} source — is the app reachable?`;
|
||||
rowError.hidden = false;
|
||||
btn.disabled = false;
|
||||
}
|
||||
}
|
||||
|
||||
async function confirmRemove() {
|
||||
if (!removingId || removeInFlight) return; // one request at a time
|
||||
removeInFlight = true;
|
||||
if (removeError) {
|
||||
removeError.textContent = "";
|
||||
removeError.hidden = true;
|
||||
}
|
||||
if (removeCancelBtn) removeCancelBtn.disabled = true;
|
||||
if (removeRemoveBtn) {
|
||||
removeRemoveBtn.disabled = true;
|
||||
removeRemoveBtn.textContent = "Removing…"; // §7.4 in-flight label
|
||||
}
|
||||
try {
|
||||
const r = await fetch(`/api/git-sources/${encodeURIComponent(removingId)}`, {
|
||||
method: "DELETE",
|
||||
});
|
||||
if (r.ok) {
|
||||
// 204: the server confirmed the total removal (row + index +
|
||||
// app-managed files).
|
||||
closeRemoveConfirm(); // focus returns to the row's Remove button
|
||||
await loadSources(); // the row leaves the table
|
||||
// The removal confirmation is the LAST announcement: the
|
||||
// reload's "N sources listed." must not overwrite it (the
|
||||
// announcer is the screen-reader confirmation for the
|
||||
// destructive action — phase 69 task 03's E2E pins the success
|
||||
// line on the announcer after a real removal).
|
||||
announce("Source removed — its files and index entries were cleaned up.");
|
||||
return;
|
||||
}
|
||||
// non-2xx: the in-modal role="alert" line (the server detail) —
|
||||
// the dialog STAYS open: the fix is one retry, not a re-search
|
||||
// for the row.
|
||||
if (removeError) {
|
||||
removeError.textContent = await apiDetail(r, "Could not remove the source — try again.");
|
||||
removeError.hidden = false;
|
||||
}
|
||||
} catch {
|
||||
if (removeError) {
|
||||
removeError.textContent = "Could not remove the source — is the app reachable?";
|
||||
removeError.hidden = false;
|
||||
}
|
||||
} finally {
|
||||
// Never stale (PLAN §7.4): the failure paths re-enable BOTH
|
||||
// buttons + relabel the confirm; the success path already closed
|
||||
// the dialog (which resets them) — the restore is a no-op there.
|
||||
removeInFlight = false;
|
||||
if (removeCancelBtn) removeCancelBtn.disabled = false;
|
||||
if (removeRemoveBtn) {
|
||||
removeRemoveBtn.disabled = false;
|
||||
removeRemoveBtn.textContent = "Remove source";
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/* The modal's own buttons (static markup — wired once). */
|
||||
if (removeCancelBtn) removeCancelBtn.addEventListener("click", cancelRemoveConfirm);
|
||||
if (removeBackdrop) removeBackdrop.addEventListener("click", cancelRemoveConfirm);
|
||||
if (removeRemoveBtn) removeRemoveBtn.addEventListener("click", confirmRemove);
|
||||
|
||||
/* ---------- add (POST /api/git-sources) — the git form ----------
|
||||
* wireAddForm gives the form the §7.4 never-stale lifecycle: while
|
||||
* the request is out the button disables + relabels "Adding…" and
|
||||
|
||||
+141
-13
@@ -1915,6 +1915,147 @@ details.thinking .thinking-text ul { margin: 0 0 0.5rem; }
|
||||
font-size: 0.88rem;
|
||||
}
|
||||
|
||||
/* ---------- Remove confirmation modal (phase 69, task 02) ----------
|
||||
/git-sources.html: the in-app confirmation that replaces the
|
||||
native confirm() — the total-removal warning (the row, the
|
||||
source's indexed documents, and — for git clones and uploaded
|
||||
archives — the files on the server's disk). The .doc-modal overlay contract:
|
||||
a fixed full-viewport dim backdrop + a centered panel (z-index
|
||||
1000, above the sticky header (20) + skip-link (100); NO blur —
|
||||
the phase-08 no-blur perf anchor), scaled to a compact dialog:
|
||||
the 46rem chat-column width or the viewport, whichever is
|
||||
narrower. Phase-08 tokens only; system fonts; no CDN.
|
||||
|
||||
AA pairs: title/copy are --ink on --surface (13.8:1); the source
|
||||
value is --ink on --bg (16.7:1); the error line is the err pair
|
||||
(err-ink on err-bg 9.3:1, the err-line border); the destructive
|
||||
button rides the err token family — the .tuning-delete /
|
||||
.steering-delete hover convention used as the resting state
|
||||
(err-ink on err-bg 9.3:1; the hover inverts to dark --bg on
|
||||
--err-line, 5.2:1 — both AA); Cancel is the ghost ink-soft family
|
||||
(5.1:1 on --surface). :focus-visible via the global 3px outline
|
||||
rule (no local suppression — focus lands on Cancel at open and is
|
||||
visible); both buttons >=44px; no animation (reduced-motion safe
|
||||
by construction). */
|
||||
.remove-confirm {
|
||||
position: fixed;
|
||||
inset: 0;
|
||||
z-index: 1000;
|
||||
display: flex; /* the panel is the only in-flow child — margin: auto centers it */
|
||||
}
|
||||
/* Explicit (the global [hidden] rule already wins — the documented,
|
||||
testable contract for the skeleton). */
|
||||
.remove-confirm[hidden] { display: none; }
|
||||
|
||||
.remove-confirm-backdrop {
|
||||
position: fixed;
|
||||
inset: 0;
|
||||
/* --bg at 82% — the doc-modal dim, no backdrop-filter (no-blur). */
|
||||
background: rgba(15, 10, 10, 0.82);
|
||||
}
|
||||
|
||||
.remove-confirm-panel {
|
||||
/* position:relative lifts the panel above the fixed backdrop
|
||||
(positioned elements paint over in-flow siblings otherwise). */
|
||||
position: relative;
|
||||
z-index: 1;
|
||||
margin: auto;
|
||||
width: min(46rem, calc(100vw - 2rem));
|
||||
padding: 1.5rem;
|
||||
background: var(--surface);
|
||||
border: 1px solid var(--line);
|
||||
border-radius: var(--radius);
|
||||
box-shadow: var(--shadow-lg);
|
||||
}
|
||||
|
||||
.remove-confirm-title {
|
||||
margin: 0 0 0.75rem;
|
||||
font-size: 1.25rem;
|
||||
line-height: 1.3;
|
||||
color: var(--ink);
|
||||
}
|
||||
|
||||
/* The source's value (git URL or local path) — mono, wrapped (a long
|
||||
URL must not overflow the panel), --ink on --bg (16.7:1). */
|
||||
.remove-confirm-source {
|
||||
display: block;
|
||||
margin: 0 0 0.75rem;
|
||||
padding: 0.5rem 0.65rem;
|
||||
font-family: var(--mono);
|
||||
font-size: 0.85rem;
|
||||
line-height: 1.5;
|
||||
color: var(--ink);
|
||||
background: var(--bg);
|
||||
border: 1px solid var(--line);
|
||||
border-radius: var(--radius-sm);
|
||||
overflow-wrap: anywhere;
|
||||
}
|
||||
|
||||
.remove-confirm-copy {
|
||||
margin: 0;
|
||||
color: var(--ink); /* 13.8:1 on --surface */
|
||||
}
|
||||
|
||||
/* The in-modal failure line (role=alert): the err pair (err-ink on
|
||||
err-bg 9.3:1, the err-line border) — the .git-source-error banner
|
||||
language, boxed. */
|
||||
.remove-confirm-error {
|
||||
margin: 0.75rem 0 0;
|
||||
padding: 0.5rem 0.65rem;
|
||||
color: var(--err-ink);
|
||||
background: var(--err-bg);
|
||||
border: 1px solid var(--err-line);
|
||||
border-radius: var(--radius-sm);
|
||||
}
|
||||
|
||||
.remove-confirm-actions {
|
||||
display: flex;
|
||||
justify-content: flex-end;
|
||||
gap: 0.6rem;
|
||||
margin-top: 1.1rem;
|
||||
}
|
||||
|
||||
/* The two modal buttons: >=44px targets, the house 3px :focus-visible
|
||||
via the global outline rule (no local override). */
|
||||
.remove-confirm-btn {
|
||||
min-height: 44px;
|
||||
min-width: 44px;
|
||||
padding: 0.55rem 1.1rem;
|
||||
border-radius: var(--radius-sm);
|
||||
font: inherit;
|
||||
font-weight: 600;
|
||||
font-size: 0.9rem;
|
||||
white-space: nowrap;
|
||||
cursor: pointer;
|
||||
}
|
||||
.remove-confirm-btn:disabled { opacity: 0.5; cursor: wait; }
|
||||
|
||||
/* Cancel — the ghost ink-soft family (5.1:1 on --surface), like the
|
||||
row's Remove / .tuning-edit; the brand pair on hover (6.9:1). */
|
||||
.remove-confirm-cancel {
|
||||
border: 1px solid var(--line);
|
||||
background: transparent;
|
||||
color: var(--ink-soft);
|
||||
}
|
||||
.remove-confirm-cancel:hover:not(:disabled) {
|
||||
background: var(--brand-soft);
|
||||
color: var(--brand-ink);
|
||||
}
|
||||
|
||||
/* "Remove source" — the destructive button on the err token family
|
||||
(the .tuning-delete / .steering-delete convention): err-ink on
|
||||
err-bg 9.3:1, the err-line border; the hover inverts to dark --bg
|
||||
on --err-line (5.2:1 — AA). */
|
||||
.remove-confirm-remove {
|
||||
border: 1px solid var(--err-line);
|
||||
background: var(--err-bg);
|
||||
color: var(--err-ink);
|
||||
}
|
||||
.remove-confirm-remove:hover:not(:disabled) {
|
||||
background: var(--err-line);
|
||||
color: var(--bg);
|
||||
}
|
||||
|
||||
/* The list: the Sources page's table pattern — full width in the
|
||||
72rem frame, surface card, horizontally scrollable wrapper (the
|
||||
URL column never wraps or ellipsizes: long URLs, credentials
|
||||
@@ -1998,19 +2139,6 @@ details.thinking .thinking-text ul { margin: 0 0 0.5rem; }
|
||||
.git-source-remove:hover:not(:disabled) { background: var(--err-bg); color: var(--err-ink); border-color: var(--err-line); }
|
||||
.git-source-remove:disabled { opacity: 0.5; cursor: wait; }
|
||||
|
||||
/* Per-row delete failure (role=alert): the err pair, inline after the
|
||||
(re-enabled) button. */
|
||||
.git-source-row-error {
|
||||
margin-left: 0.6rem;
|
||||
background: var(--err-bg);
|
||||
color: var(--err-ink);
|
||||
border: 1px solid var(--err-line);
|
||||
border-radius: var(--radius-sm);
|
||||
padding: 0.3rem 0.6rem;
|
||||
font-size: 0.8rem;
|
||||
font-weight: 600;
|
||||
}
|
||||
|
||||
/* Env-fallback rows carry no Remove (nothing is stored to remove) —
|
||||
the tag says where the row comes from (brand pair, 6.9:1). */
|
||||
.git-source-env-tag {
|
||||
|
||||
@@ -229,19 +229,67 @@
|
||||
active list comes from. -->
|
||||
<p class="git-sources-empty" id="git-sources-empty" hidden>No sources stored yet.</p>
|
||||
|
||||
<!-- Scope boundary (phase locked decision): adding/removing a
|
||||
source does NOT clone or prune — the Sync button performs
|
||||
that; the phase-49 upload is the exception (it unpacks and
|
||||
scans in place, and a same-name re-upload replaces the
|
||||
source in place). -->
|
||||
<!-- Phase 69 (owner request 2026-09-02): removal is a TOTAL
|
||||
removal — the row, the source's indexed documents, and —
|
||||
for git clones and uploaded archives — the files on the
|
||||
server's disk, all immediately (the confirmation modal
|
||||
below spells it out; foreign local directories are never
|
||||
touched). Adding still does not clone — the Sync button
|
||||
mirrors the remaining sources (upstream file churn is
|
||||
pruned on that run); the phase-49 upload is the
|
||||
in-place exception (it unpacks and scans, and a
|
||||
same-name re-upload replaces the source in place). -->
|
||||
<p class="git-source-hint" id="git-sources-hint" role="note">
|
||||
Removing a source is a total removal, done immediately: its
|
||||
entry, its indexed documents, and — for git clones and
|
||||
uploaded archives — its files on the server's disk (the
|
||||
confirmation modal spells out exactly what will be deleted;
|
||||
files in your own local directories are never touched).
|
||||
Uploads unpack and scan immediately — re-uploading the same
|
||||
filename replaces that source in place (no new folder, no
|
||||
duplicate row). The Sync button still imports the git
|
||||
checkouts and local directories together (files removed from
|
||||
a source are pruned) — removing a source prunes its documents
|
||||
from the index on the next sync.
|
||||
duplicate row). The Sync button still mirrors the remaining
|
||||
sources (files removed upstream are pruned on that run).
|
||||
</p>
|
||||
|
||||
<!-- Phase 69 (owner request 2026-09-02): the remove
|
||||
confirmation — a real in-app alertdialog (the native
|
||||
confirm() retired): a row's Remove button opens it
|
||||
(git-sources.js).
|
||||
It names the source (#remove-confirm-source — ALWAYS
|
||||
populated via textContent: URLs may embed user:pass@
|
||||
credentials, the phase-32 masking discipline) and states
|
||||
the full-removal policy. Focus lands on Cancel (the safe
|
||||
default for a destructive action); Escape, the Cancel
|
||||
button, and the dim backdrop all close as cancel (no
|
||||
request — focus returns to the row's Remove button); only
|
||||
"Remove source" sends the DELETE, in the §7.4 "Removing…"
|
||||
in-flight state. The .doc-modal overlay contract: a fixed
|
||||
full-viewport dim backdrop + a centered panel (no blur).
|
||||
Static markup so the E2E suite gets stable selectors (the
|
||||
#git-sources-hint / gate convention). -->
|
||||
<div class="remove-confirm" id="remove-confirm-dialog" role="alertdialog"
|
||||
aria-modal="true" aria-labelledby="remove-confirm-title"
|
||||
aria-describedby="remove-confirm-copy" hidden>
|
||||
<div class="remove-confirm-backdrop" aria-hidden="true"></div>
|
||||
<div class="remove-confirm-panel">
|
||||
<h2 class="remove-confirm-title" id="remove-confirm-title">Remove this source?</h2>
|
||||
<code class="remove-confirm-source" id="remove-confirm-source"></code>
|
||||
<p class="remove-confirm-copy" id="remove-confirm-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.
|
||||
</p>
|
||||
<p class="remove-confirm-error" id="remove-confirm-error" role="alert" hidden></p>
|
||||
<div class="remove-confirm-actions">
|
||||
<button type="button" class="remove-confirm-btn remove-confirm-cancel"
|
||||
id="remove-confirm-cancel">Cancel</button>
|
||||
<button type="button" class="remove-confirm-btn remove-confirm-remove"
|
||||
id="remove-confirm-remove">Remove source</button>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
<!-- Polite live region: the screen-reader confirmation for list
|
||||
loads, adds, and removals (git-sources.js owns the text). -->
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -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}"
|
||||
)
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
@@ -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."
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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 <tag … id=…> element's full markup (a balanced-tag walk —
|
||||
the dialog nests the backdrop / panel / actions divs; the hint is
|
||||
a <p>)."""
|
||||
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[^>]*>|</{tag}>", html[open_i :]):
|
||||
if m.group(0).startswith(f"</{tag}>"):
|
||||
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"<h2[^>]*id=\"remove-confirm-title\"[^>]*>(.*?)</h2>", frag, re.S)
|
||||
assert title and _norm(title.group(1)) == "Remove this source?"
|
||||
err = re.search(r"<p[^>]*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"<button[^>]*id=\"{btn}\"[^>]*>", frag)
|
||||
assert m and 'type="button"' in m.group(0), f"#{btn} is a real type=button"
|
||||
cancel = re.search(r'<button[^>]*id="remove-confirm-cancel"[^>]*>(.*?)</button>', frag, re.S)
|
||||
remove = re.search(r'<button[^>]*id="remove-confirm-remove"[^>]*>(.*?)</button>', 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 <code> (mono) — textContent-only in JS.
|
||||
src = re.search(r"<code[^>]*id=\"remove-confirm-source\"[^>]*>", frag)
|
||||
assert src, "#remove-confirm-source is a <code>"
|
||||
|
||||
|
||||
# ---------- 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)"
|
||||
@@ -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/<repo>/``; 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
|
||||
Reference in New Issue
Block a user