phase: 168_admin_settings_panel
**Phase 168 — Admin Settings panel — final verification: PASS** 1. **Implemented/verified:** `/settings.html` (admin-only) with the 75-field catalogue (`app/core/settings_catalog.py`), DB-over-env overrides in Postgres (`app_settings`, migration 0028 at head, `settings_overlay.py` resolver, `app/api/admin_settings.py` — 4 admin-only routes), live-apply on save with no restart + restart-required badges, Reset/Clear all, write-only secrets, `.env`-never-written, `BOR_SETTINGS_DB=0` kill switch. 2. **Phase E2E: 12/12** — run with `E2E_APP_PORT=8200` (ports 8123/8124 are held by an unrelated "undercooked" project; no process killed, no code change — the documented env override). 3. **Regression set: all 7 suites green in isolation** (theme tab, API tokens, configurable brand, global tuning, admin auth, cache busting, wide desktop column — 38 tests). 4. **Full suite: unit+integration exit 0 (4066 passed), app/ coverage 99%** (gate >90%); **ruff clean, pyright 0 errors / 0 warnings.** 5. **Log contract (rule 10) verified live:** per-turn + per-response byte-budget lines present; settings write lines carry key + action, **never the value**. 6. **Byte-budget audit: all endpoints under the 262,144 B budget**; settings catalogue 23,513 B (gzip 3,326). 7. **Defect fixed (one):** criterion 5 had no integration pin — added `test_bad_row_inserted_directly_never_bricks_boot` to `tests/integration/test_admin_settings_api.py` (malformed row inserted directly → fresh `create_app()` boots, row skipped with logged reason, rest of table applies, `/api/health` 200) — 12/12 green. 8. **Docs pass:** `README.md` and `AGENTS.md` reviewed in full — both consistent with the as-built feature (Settings page table, Settings section, Configuration-reference note, `.env.example` block); **no changes required**. 9. **Cleanup:** audit app killed (by PID); `app_settings` left empty (0 rows); all changes left in the working tree (no commit, per protocol). 10. **Completion criteria:** all 7 checkboxes verified green (criterion 7's commit is the harness's). 11. **Next pending phase:** `170_edit_bubble_sizing`.
This commit is contained in:
@@ -1,81 +0,0 @@
|
||||
# Phase 168 — Admin Settings panel: edit any `BOR_*` setting in the browser, DB over env, live on Save
|
||||
|
||||
**Source:** `TODO.md` L3 — "I want an admin panel that can edit anything in .env and save it to the database. The database value should override the env var. The change should take effect on clicking 'save' on that page."
|
||||
**Story:** n/a (owner-directed feature)
|
||||
**Context:** `app/config.py` (75 `Settings` fields, `env_prefix="BOR_"`, `env_file=".env"`, `extra="ignore"`, `get_settings()` `@lru_cache`d, ~40 call sites across `app/`), `app/db.py` (module-level `settings = get_settings()` + `create_engine`), `app/main.py` (`settings = get_settings()` at import, middleware wired at boot, `_shell_routes`), `app/core/theming.py::effective_settings` (the DB-over-env resolver precedent, B1), `app/models.py::UiSettings` (the single-row admin-persistence precedent), `app/api/ui_settings.py` + `app/api/tokens.py` (router-wide `require_admin`), `app/core/auth.py::require_admin`, `frontend/index.html` (`#view-*` shell views; `#view-tokens` L1077, `#view-theme` L1269), `frontend/assets/router.js` (`VIEW` / `VIEW_PATH` / `VIEW_MODULES`), `frontend/assets/header.js` (~L197-230, the ship-hidden / reveal-for-admin nav contract), `frontend/assets/styles.css` (`.table-wrap` L2825 + `.table-wrap table thead th` sticky L2841 — the phase-156 bounded-scroll precedent), `app/core/caching.py::HTML_PAGES` (L169), `alembic/versions/` (next free: **0028**), `tests/e2e/conftest.py` (the `os.environ["BOR_PDF_PICTORIAL"] = "0"` global-pin precedent, ~L52).
|
||||
|
||||
## Objective
|
||||
|
||||
An admin opens **Settings** in the navbar, edits any setting that today lives only in `.env` (75 fields, grouped, type-correct inputs), clicks **Save**, and the new value is (a) stored in Postgres, (b) preferred over the env var from that instant on with **no restart**, and (c) visible in the running UI. Per-row **Reset** deletes the DB row so the env value wins again. The `.env` file is never written by the app.
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
|
||||
**D1 — Storage: one `app_settings` table (task 01).** `key` = the **bare `Settings` field name** (`relevance_threshold`), `value` = the **env-native string form** (`"0.55"`, `"true"`, `"md,txt"`, a JSON array for the one `list[str]` field), `updated_at`. Storing bare strings (not typed columns, not a wide row) is what makes "anything in .env" work for all 75 fields with zero per-field DDL, and keeps the coercion in exactly one place (the pydantic validators). The `BOR_` prefix stays a *presentation* concern (`env_var = "BOR_" + key.upper()`), never stored — a stored prefix is a double-prefix typo waiting to happen.
|
||||
|
||||
**D2 — The overlay lives INSIDE `get_settings()` (task 02).** The ~40 existing call sites (`app/rag/retriever.py`, `app/api/chat.py`, middlewares, `scripts/`) must not change — that is the only way "anything" becomes live rather than "the three fields someone remembered to thread through". `get_settings()` keeps its `@lru_cache` and its zero-arg signature (`tests/{integration,e2e}/*.py` call `get_settings.cache_clear()` in 6 files — that surface must survive), and resolves: `Settings()` (env/`.env`, exactly today) → overlay the DB rows → validate → return. `invalidate_settings()` clears the cache and runs the reset hooks (D3). Hot-path cost after the first call: one `lru_cache` dict hit.
|
||||
|
||||
**D3 — "Takes effect on Save" = cache + the two in-process singletons (task 03).** Everything except two objects is read per use, so clearing the cache is enough for ~70 fields. The exceptions are `app/api/chat.py::_llm` (an `LLMClient` built once from `get_settings()` — holds `llm_base_url` / `effective_api_key` / `llm_chat_model` / `llm_embed_model` / timeouts / retries, L318-326) and `_chat_semaphore` (built once from `chat_max_concurrent`, L302-315). The overlay exposes a hook registry; `app/main.py::create_app()` registers the chat reset (`_llm = None`, `_chat_semaphore = None`) so `app/core/` never imports `app/api/` (dependency direction, PLAN §3). Settings wired at boot cannot be applied in-process (engine pools, `SessionMiddleware`, the static mount, the gzip middleware, logging) — they stay **editable** but carry a `restart_required` badge (LOCKED A4).
|
||||
|
||||
**D4 — The catalog is code, derived-checked, and complete (task 02).** `app/core/settings_catalog.py` declares, per field: `group` (mirrors `app/config.py`'s own section comments: App & branding, HTTP & logging, Database, LLM, Chat history, RAG & retrieval, Agent tools, Import & sources, Images, PDF, Docs push, Admin & security), `label`, `secret`, `restart_required`, `overridable`. A unit test asserts the catalog covers **exactly** `Settings.model_fields` (a new setting added without catalog metadata fails the test → it can never silently disappear from the panel).
|
||||
|
||||
**D5 — Fail loud on write, fail soft on read (tasks 02/04, LOCKED A5).** A write validates the FULL candidate set — every stored override plus the new one, layered over the env base — through the real `Settings` validators, so a cross-field typo (`lexical_support_floor` above a DB-overridden `relevance_threshold`) is a 422 that names the field and writes nothing. A read that finds an unusable row (hand-edited, or a validator changed in a later release) skips THAT key, logs `settings override ignored key=… reason=…` and applies the rest: a DB value must never brick boot, because a bricked app cannot be un-bricked from the panel.
|
||||
|
||||
**D6 — The kill switch, and the E2E poison guard (task 02).** `BOR_SETTINGS_DB=0` (env-only, never itself DB-overridable, read straight from `os.environ`) disables the overlay entirely — the pre-phase behavior, the house `agent_max_rounds`/`compress_min_bytes` convention. `tests/e2e/conftest.py` pins it to `0` for **every** E2E app the same way it already pins `BOR_PDF_PICTORIAL`: a row left behind by one suite would otherwise silently retune every later suite's app subprocess. The phase-168 suite opts back in on its own app fixture (the `test_pictorial_pdf.py` pattern) and `TRUNCATE app_settings` before AND after (the `test_admin_theme_tab.py:286` precedent).
|
||||
|
||||
**D7 — The API (task 04).** `app/api/admin_settings.py`, prefix `/admin/settings`, router-wide `dependencies=[Depends(require_admin)]` (the `ui_settings`/`tokens` pattern: anonymous AND token users 403): `GET` the catalog + effective values with a `source` badge (`db` | `env` | `default`), `PUT /{key}` set one value, `DELETE /{key}` reset to env, `DELETE` reset all (the panel's "Clear all overrides"). Secrets are **write-only** (LOCKED A4): `GET` reports `is_set`, never the plaintext, and the write log line carries the key and the action, never the value.
|
||||
|
||||
**D8 — The view (task 05).** The shell's eighth view: `#view-settings` in `frontend/index.html` + `frontend/assets/settings.js`, `/settings.html` added to `main.py::_shell_routes` and `caching.HTML_PAGES`, `VIEW`/`VIEW_PATH`/`VIEW_MODULES` in `router.js`, and `#nav-settings` (ship-hidden) in `index.html` / `document.html` / `login.html` / `shared.html` revealed by `header.js` — the exact phase-91 Theme-view wiring. Layout per PLAN §7 + AGENTS rule 5: the 72rem `.container`, one `.table-wrap` card (bounded `max-height: min(70vh, 64rem)`, sticky `thead`) with a `<tbody>` per group, type-correct controls (`<input type=number>` for int/float, a `true/false` `<select>` for bool, `<textarea>` for the CSV/JSON and long strings, a password input for secrets), a text source badge (B5: never color alone), a client-side `type=search` filter (the phase-101 tokens pattern), a page-level **Save settings** (dirty rows only), per-row **Reset**, and a `role="status"` / `role="alert"` live region (§7.4).
|
||||
|
||||
## Dependencies
|
||||
- `91_admin_theme_tab` + `92_theme_save_and_coverage` + `93_theme_semantic_completion` (complete) — the DB-over-default resolver (B1), the admin-only shell view + gate pattern this view copies, and the nav reveal contract.
|
||||
- `79_api_tokens` (complete) — router-wide `require_admin` + the bounded, searchable admin catalogue table with per-row actions.
|
||||
- `156_table_ux_polish` (complete) — `.table-wrap` bounded scroll + sticky header, the `cell-clip` ellipsis contract for long action labels.
|
||||
- `167_api_payload_diet` (complete) — the response byte budget: the new `GET /api/admin/settings` (~75 rows ≈ 20 KB) must stay under `BOR_LOG_LARGE_RESPONSE_BYTES` and the audit script's 256 KB bar; never a 413.
|
||||
- `82_security_headers` + `81_login_rate_limit` (complete) — the CSP/`default-src 'self'` posture the new JS must live inside (no CDN, rule 6).
|
||||
|
||||
## Tasks
|
||||
1. `01_app_settings_table.md` — `AppSetting` model + reversible migration `0028_app_settings.py` + its migration test.
|
||||
2. `02_settings_overlay_resolver.md` — `app/core/settings_catalog.py` + `app/core/settings_overlay.py`, `get_settings()` becomes the cached env+DB resolver, `invalidate_settings()`, the `BOR_SETTINGS_DB` kill switch + the E2E conftest pin, the catalog-completeness unit pins.
|
||||
3. `03_live_apply_on_save.md` — the reset-hook registry, the chat `_llm` / `_chat_semaphore` reset registered from `create_app()`, and the integration proof that a saved change bites **without a restart**.
|
||||
4. `04_admin_settings_api.md` — `/api/admin/settings` GET / PUT `/{key}` / DELETE `/{key}` / DELETE all, the schemas, the full-candidate validation, the masked secrets, the value-free log line, the README API note.
|
||||
5. `05_settings_view_ui.md` — the Settings shell view (`#view-settings`, `settings.js`, router + shell route + `HTML_PAGES` + nav reveal, `.table-wrap` catalogue, Save/Reset + live region).
|
||||
6. `06_e2e_settings_panel.md` — `tests/e2e/test_admin_settings_panel.py`, the regression set, the full gates, the atomic commit.
|
||||
|
||||
## Testing & Quality
|
||||
- **Unit:** `tests/unit/test_settings_catalog.py` (catalog ⇄ `Settings.model_fields` equality; the masked set is exactly `llm_api_key` + `admin_password` + `session_secret` + `database_url` (the last one is read-only but its URL carries credentials); the non-overridable set is exactly `database_url` + `embedding_dim`), `tests/unit/test_settings_overlay.py` (merge/coercion/validation-skip/`BOR_SETTINGS_DB=0`), `tests/unit/test_settings_frontend_view.py` (static source pins in the house style: the view section exists, `router.js` maps `/settings.html`, no CDN, `settings.js` builds DOM with `createElement`/`textContent`).
|
||||
- **Integration:** `tests/integration/test_migration_0028.py` (up/down, A13), `tests/integration/test_admin_settings_api.py` (auth matrix anonymous/token-user/admin, 404 unknown key, 422 validator failures naming the field, masked secrets in the response body, the DB-over-env flip visible through `GET /api/config`, reset-to-env, `get_settings()` reflecting a write with no restart), plus the existing `get_settings.cache_clear()` suites staying green untouched.
|
||||
- **Coverage:** **>90%** on `app/` — `uv run pytest --cov=app --cov-report=term-missing`.
|
||||
- **E2E:** `uv run pytest tests/e2e/test_admin_settings_panel.py -v --no-cov` green **in isolation**; regression set green in isolation: `test_admin_theme_tab.py`, `test_api_tokens.py`, `test_configurable_brand.py`, `test_global_tuning.py`, `test_admin_auth.py`, `test_cache_busting.py`.
|
||||
- **Lint/types:** `uv run ruff check . && uv run pyright`.
|
||||
- **Log contract (AGENTS rule 10):** every settings write logs one line (`settings action=set|clear|clear_all key=… source=db|env`) with no value; `GET /api/admin/settings` appears in the `grep ' :: api '` byte audit and under the 256 KB bar (`uv run python -m scripts.api_payload_audit`).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `/settings.html` is an admin-only shell view: anonymous sees the sign-in gate, a token user never sees the nav link, and all four API routes 403 for both.
|
||||
- [ ] Editing a value + **Save settings** stores it in `app_settings`, flips the row's badge from `env`/`default` to `DB`, and the effect is visible in the running app with **no restart** (pinned E2E: `BOR_INPUT_PLACEHOLDER` shows up in the composer; `BOR_RELEVANCE_THRESHOLD` / `BOR_LLM_CHAT_MODEL` are read from the overlay by a live request).
|
||||
- [ ] **Reset** deletes the row and the env value wins again; **Clear all overrides** empties the table; `.env` is never opened for writing by any code path (unit-pinned: no `open(` on the env file in `app/`).
|
||||
- [ ] Secrets never leave the server (`GET` body has no plaintext, `page.content()` has no secret), no write log line contains a value, and a bogus value is a 422 that names the field and stores nothing.
|
||||
- [ ] A stored-but-invalid row (inserted directly, bypassing the API) is ignored at read with a logged reason — the app still boots and `/api/health` is 200.
|
||||
- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean, the phase E2E + the regression set green in isolation.
|
||||
- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moves to `.agents/phases/complete/`.
|
||||
|
||||
## Locked decisions (owner-confirmed 2026-10-09 during phase authoring)
|
||||
- **A1 — DB-only persistence:** the panel writes `app_settings` and **never** touches `.env`; the env var stays the base and the DB row wins.
|
||||
- **A2 — the overlay sits inside `get_settings()`** (cached, `cache_clear()`-compatible) so all ~40 consumers go DB-over-env at once, behind the `BOR_SETTINGS_DB=0` kill switch.
|
||||
- **A3 — `database_url` and `embedding_dim` are NOT DB-overridable** (read-only rows, reason shown: the engine is built from `database_url`, so an override is a chicken-and-egg brick; A6 fixes `chunks.embedding` at 768, so a dim override would corrupt retrieval).
|
||||
- **A4 — boot-wired settings stay editable with a "restart required" badge** (pools, session middleware, static mount, gzip, log level); **secrets are masked and write-only** and never appear in a response body or a log line.
|
||||
- **A5 — validate the full candidate set on write (422, nothing stored), fail soft on read** (skip the bad key, log it, boot anyway).
|
||||
- **A6 — one story, one phase, one Playwright file** (AGENTS rule 4) — the backend tasks are covered by unit + integration, the live-effect contract by `tests/e2e/test_admin_settings_panel.py`.
|
||||
|
||||
## Assumptions (the TODO left these open; the executor does not guess)
|
||||
- **K1:** the key is the bare field name; `BOR_UPPER` is derived for display/API presentation only.
|
||||
- **K2:** the one `list[str]` field (`suggestions`) is edited as a JSON array in a `<textarea>` and stored in that same JSON form (pydantic parses it exactly like the env var); every other field is a scalar or CSV string.
|
||||
- **K3:** a missing `app_settings` table (fresh deployment, migrations not applied) means "no overrides" and logs at DEBUG once — only an unusable *value* logs at ERROR.
|
||||
- **K4:** "Save settings" sends only the dirty rows; the first failing row aborts the batch, shows its 422 detail, and keeps the other rows' edits in the form for correction.
|
||||
- **K5:** history/audit beyond `updated_at` (who changed what, when, old value) is out of scope — single-admin homelab (PLAN §1 "out of scope").
|
||||
- **K6:** the overlay is single-process (PLAN A12: in-process state, restart-clearing accepted, no Valkey); with more than one uvicorn worker a save is visible in the worker that served it and in every worker after its next resolution — the deployment runs one app process, and the panel notes nothing about it.
|
||||
- **K7:** `.env` keys that are not `Settings` fields (`extra="ignore"`, e.g. `AIPI_KEY`, `DEBUGPY*`) are not editable — they have no `app/` consumer to retune. `BOR_LLM_API_KEY` remains the way to set the key (it wins over `$AIPI_KEY` via `effective_api_key`).
|
||||
|
||||
## Commit
|
||||
```bash
|
||||
git add app/ alembic/ frontend/ tests/ .env.example README.md .agents/phases/ && git commit --no-gpg-sign -m "feat(settings): admin settings panel with DB-over-env overrides applied live on save"
|
||||
```
|
||||
@@ -1,60 +0,0 @@
|
||||
# Task 01 — `app_settings`: the table that holds DB-side setting overrides
|
||||
|
||||
**Phase:** `168_admin_settings_panel` · **Source:** `TODO.md:3` — "I want an admin panel that can edit anything in .env and save it to the database."
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
One narrow key/value table stores the admin's overrides, keyed by the bare `Settings` field name, with the value kept in its **env-native string form** — the storage half of phase-168 D1, with zero per-field DDL for the 75 settings.
|
||||
|
||||
## Work
|
||||
1. `app/models.py` — append the `AppSetting` class (after `FolderSummary`, the file's tail) in the house docstring style, naming the phase and the TODO line:
|
||||
```python
|
||||
class AppSetting(Base):
|
||||
"""One admin setting override (phase 168, task 01).
|
||||
|
||||
``TODO.md`` L3: the admin Settings panel persists what used to be
|
||||
``.env``-only knobs here, and the row WINS over the environment
|
||||
(resolved by ``app.core.settings_overlay``, consumed by
|
||||
``app.config.get_settings``). …
|
||||
"""
|
||||
|
||||
__tablename__ = "app_settings"
|
||||
|
||||
#: The BARE ``Settings`` field name (``relevance_threshold``) — never
|
||||
#: the ``BOR_…`` form: the prefix is presentation
|
||||
#: (``"BOR_" + key.upper()``), so a stored prefix could double up.
|
||||
key: Mapped[str] = mapped_column(String(100), primary_key=True)
|
||||
#: The value in the SAME textual form the env var would carry
|
||||
#: (``"0.55"``, ``"true"``, ``"md,txt"``, a JSON array for the one
|
||||
#: ``list[str]`` field) — coercion and validation belong to
|
||||
#: pydantic, in exactly one place (phase 168 D1/A5).
|
||||
value: Mapped[str] = mapped_column(Text, nullable=False)
|
||||
#: When the admin last saved it (the ``folder_summaries`` /
|
||||
#: ``kb_overview.updated_at`` house style: server-side now()).
|
||||
updated_at: Mapped[datetime] = mapped_column(
|
||||
DateTime(timezone=True), server_default=func.now(), nullable=False
|
||||
)
|
||||
```
|
||||
Import `func` from `sqlalchemy` if the module does not already (it imports `DateTime`, `String`, `Text`, `Integer`, `select`-adjacent names — check the header first, do not duplicate an import).
|
||||
No index: the whole table is read by primary key order in one `SELECT` per settings resolution (the `ui_settings` "display fact, never a query surface" note, 0027).
|
||||
2. `alembic/versions/0028_app_settings.py` — `revision = "0028"`, `down_revision = "0027"`, docstring in the 0027 style (what the table is for, why the value is TEXT, why no index, why the downgrade is a clean drop). `upgrade()` = `op.create_table("app_settings", sa.Column("key", sa.String(length=100), primary_key=True), sa.Column("value", sa.Text(), nullable=False), sa.Column("updated_at", sa.DateTime(timezone=True), server_default=sa.func.now(), nullable=False))`; `downgrade()` = `op.drop_table("app_settings")` with the A13 comment (dropping it leaves the 0027 schema byte-identical; the values are re-enterable from the panel).
|
||||
3. `tests/integration/test_migration_0028.py` (new) — copy the `test_migration_0017.py` harness verbatim (the `alembic` fixture with the `db_available()` skip + the teardown `upgrade(cfg, "head")`, `_version`, the `information_schema` assertion helpers) and pin:
|
||||
- 0027 → 0028 creates `app_settings` with `key` VARCHAR(100) NOT NULL and PK, `value` TEXT NOT NULL, `updated_at` TIMESTAMPTZ NOT NULL with the `now()` server default, and `alembic_version = '0028'`;
|
||||
- the 0027 schema survives (`documents.page_strip_mode` still present);
|
||||
- rows round-trip (insert `relevance_threshold`/`"0.55"` and `import_extensions`/`"md,txt"`, read them back);
|
||||
- a duplicate `key` raises `IntegrityError` (the PK is the uniqueness guard — two rows for one setting would make "which one wins" undefined);
|
||||
- `NULL` value is rejected (NOT NULL — "clear the override" is a DELETE, never an empty value, so the resolver never has to distinguish them);
|
||||
- downgrade to 0027 → the table is GONE, the rest survives; re-upgrade → the table is back (A13 round-trip).
|
||||
4. `.env.example` — add a short comment block at the top (under the header, before the first `BOR_` line) stating the precedence rule and the kill switch: the app reads `BOR_*` from the environment, an admin override saved in the database wins per key, the panel never writes this file, and `BOR_SETTINGS_DB=0` disables DB overrides (set it in the container env, not here — a `0` committed here is a silent "my change did nothing"). Task 02 implements what this comment describes; write it so it stays true.
|
||||
ASSUMPTION (K3): `.env.example` is documentation only — nothing parses it at runtime except pydantic's `env_file=".env"`, so a comment cannot change behavior.
|
||||
|
||||
## Testing & Quality
|
||||
- Integration: `uv run pytest tests/integration/test_migration_0028.py -v` (needs `podman compose up -d db`).
|
||||
- Unit: no new module yet; `uv run pytest tests/unit -q` stays green (adding a model cannot change behavior — if something moved, it is a bug in this task).
|
||||
- Coverage: **>90%** on `app/` — the added model is exercised by the migration test's insert/read plus task 02's resolver tests.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run alembic upgrade head` applies 0028 and `uv run alembic downgrade 0027` removes `app_settings` cleanly.
|
||||
- [ ] `tests/integration/test_migration_0028.py` green (including the duplicate-key and NOT-NULL pins).
|
||||
- [ ] `uv run pytest` green; `uv run ruff check . && uv run pyright` clean.
|
||||
- [ ] No behavior change anywhere: `get_settings()` still reads env only (that is task 02), and no route exists yet.
|
||||
@@ -1,76 +0,0 @@
|
||||
# Task 02 — The overlay: `get_settings()` resolves DB-over-env, behind a kill switch
|
||||
|
||||
**Phase:** `168_admin_settings_panel` · **Source:** `TODO.md:3` — "…save it to the database. The database value should override the env var."
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
A cached resolver that layers the `app_settings` rows over the environment and hands every existing consumer a valid `Settings` — no consumer changes, no restart, and a `BOR_SETTINGS_DB=0` switch that puts the pre-phase behavior back exactly.
|
||||
|
||||
## Work
|
||||
1. `app/core/settings_catalog.py` (new) — the presentation + capability metadata, code-owned and complete:
|
||||
- `@dataclass(frozen=True, slots=True) class SettingMeta`: `key: str`, `group: str`, `label: str`, `secret: bool = False`, `restart_required: bool = False`, `overridable: bool = True`.
|
||||
- `CATALOG: dict[str, SettingMeta]` covering **every one** of `Settings.model_fields` (75 today). Groups, in this display order, with these members (they mirror `app/config.py`'s own section comments — if the executor disagrees with one placement, keep the count and note it in the commit body):
|
||||
| Group | Fields |
|
||||
|---|---|
|
||||
| App & branding | `app_name` `app_version` `environment` `input_placeholder` `footer_text` `suggestions` |
|
||||
| HTTP & logging | `log_level`↻ `log_large_response_bytes` `compress_min_bytes`↻ `static_dir`↻ |
|
||||
| Database | `database_url`⛔ `db_pool_size`↻ `db_pool_max_overflow`↻ `db_pool_recycle`↻ `chat_max_concurrent` |
|
||||
| LLM (aipi) | `llm_base_url` `llm_api_key`🔒 `llm_chat_model` `llm_embed_model` `llm_summary_model` `stream_thinking` `llm_retries` `llm_retry_delay` `llm_timeout` |
|
||||
| Chat history | `history_max_turns` `history_max_chars` |
|
||||
| Retrieval & honesty | `embedding_dim`⛔ `top_n_docs` `relevance_threshold` `lexical_support_floor` `source_usefulness_floor` `related_max_docs` `suggested_docs` `suggestion_preview_chars` `hybrid_vector_candidates` `hybrid_lexical_candidates` `rrf_k` `recency_boost` `recency_half_life_days` `name_hit_bonus` |
|
||||
| Chunking & embedding | `chunk_target_chars` `chunk_overlap_chars` `embed_batch_size` `embed_question_max_chars` |
|
||||
| Prompt budgets | `max_output_tokens` `steering_max_chars` `summary_max_chars` `kb_overview_max_chars` `overview_input_max_chars` `folder_summary_input_max_chars` |
|
||||
| Agent tools | `agent_max_rounds` `read_max_chars` |
|
||||
| Import & sources | `import_extensions` `git_sources` `sources_dir` `upload_dir` `upload_max_mb` |
|
||||
| Images | `images` `image_extensions` `image_dir` `chat_image_dir` `chat_image_max_mb` `answer_images` |
|
||||
| PDF | `pdf_pictorial` `pdf_pictorial_min_chars` `pdf_image_skip_blank` `pdf_page_dir` |
|
||||
| Docs push | `docs_repo` `docs_branch` `docs_base_branch` `docs_work_dir` |
|
||||
| Admin & security | `admin_password`🔒 `session_secret`🔒↻ `session_max_age`↻ `session_cookie`↻ |
|
||||
`↻` = `restart_required=True` (wired once at boot: logging, the gzip middleware in `app/main.py`, the static mount, `create_engine` in `app/db.py`, `SessionMiddleware`); `⛔` = `overridable=False` (LOCKED A3 — `database_url` is what builds the engine that would read the override, `embedding_dim` is fixed in the `chunks` table by A6); `🔒` = `secret=True`, plus `database_url` as a fourth masked field (its URL carries credentials) even though it is read-only.
|
||||
- `ENV_PREFIX = "BOR_"`, `def env_var(key: str) -> str` → `"BOR_" + key.upper()`; `GROUPS: tuple[str, ...]` (the table order above); `SECRETS`, `RESTART_REQUIRED`, `NON_OVERRIDABLE` derived from `CATALOG` (never re-listed by hand).
|
||||
- Module docstring: the phase, the TODO line, why the catalog exists (the panel is generated, not hand-written) and the completeness pin.
|
||||
2. `app/core/settings_overlay.py` (new) — the mechanism:
|
||||
- `resolve_settings() -> Settings` — the whole overlay: `base = Settings()`; `if not enabled(): return base`; `rows = load_overrides()`; `return merge(rows)[0]`. **Never call `app.config.get_settings()` here** — `Settings()` is the env-only entry point (recursion guard, pinned by test).
|
||||
- `enabled() -> bool` — `os.environ.get("BOR_SETTINGS_DB", "1")` is not one of `0/false/no/off` (the house bool parsing), read **per call** from `os.environ` so tests can flip it; the flag itself is not a `Settings` field, so it can never be turned off from the database (the escape hatch cannot disable itself).
|
||||
- `load_overrides() -> dict[str, str]` — `SELECT key, value FROM app_settings ORDER BY key` through a module-level lazily-created engine: `create_engine(Settings().database_url, poolclass=NullPool, pool_pre_ping=True)` (NullPool: one connection per resolution, none held — the app's real pool belongs to `app/db.py` and must not be used here: importing `app.db` from the overlay would be a cycle, `app/db.py` imports `app.config`, and the pool whose size this table can change must not be the one that reads the table). Fail-soft: `ProgrammingError`/undefined-table → `{}` + one DEBUG line per process (K3 — a pre-migration deployment); any other `SQLAlchemyError`/`OSError` → `{}` + one WARNING per process naming the exception class ("settings overrides unavailable — env values in effect"). Both "once per process" guards are module-level flags, reset by `reset_state_for_tests()`.
|
||||
- `merge(rows: Mapping[str, str]) -> tuple[Settings, list[tuple[str, str]]]` — returns `(settings, skipped)` where `skipped` is `(key, reason)` pairs. Build the kwargs from the rows: drop keys not in `Settings.model_fields` (an orphan row from a removed setting) and keys with `overridable=False` (a hand-inserted `database_url` row must never hijack the engine — LOCKED A3), coerce each value (`_coerce(field, raw)`: `json.loads` for the one `list[str]` field `suggestions`, the raw string for everything else — pydantic-settings lax-coerces `str`→`int`/`float`/`bool`, and init kwargs rank ABOVE env in pydantic-settings, which is precisely "the database value overrides the env var"). Then `Settings(**kwargs)`; on `ValidationError` drop the named field (`error["loc"][0]`), append it to `skipped` with the validator's message, and retry — so one bad row costs one setting, not the whole table (LOCKED A5). Log each skipped key once at ERROR: `settings override ignored key=… reason=…`.
|
||||
- `register_invalidator(fn: Callable[[], None]) -> None` + `run_invalidators()` — the hook list task 03 uses; a hook that raises is logged, never propagated (a failed reset must not undo a successful save).
|
||||
- `reset_state_for_tests() -> None` — clears the module flags/engine for the unit suites.
|
||||
3. `app/config.py` — keep the `Settings` class 100% untouched and swap the resolver tail:
|
||||
```python
|
||||
@lru_cache
|
||||
def get_settings() -> Settings:
|
||||
"""Effective settings: env/.env with the admin's DB overrides layered
|
||||
on top (phase 168, TODO L3). … cached exactly as before —
|
||||
``get_settings.cache_clear()`` still works — and
|
||||
``invalidate_settings()`` is the save-side wrapper that also runs the
|
||||
runtime reset hooks (task 03)."""
|
||||
from app.core.settings_overlay import resolve_settings # lazy: app.core imports app.config
|
||||
|
||||
return resolve_settings()
|
||||
|
||||
|
||||
def invalidate_settings() -> None:
|
||||
get_settings.cache_clear()
|
||||
from app.core.settings_overlay import run_invalidators
|
||||
|
||||
run_invalidators()
|
||||
```
|
||||
The lazy imports are load-bearing (documented in the comment): `app.core.*` imports `app.config` at module scope, and `app/db.py` → `app.config` runs at import time; a module-scope `app.config` → `app.core` import would deadlock the import graph. The 6 existing `get_settings.cache_clear()` test call sites keep working unchanged (they only retune env, so skipping the hooks there is correct).
|
||||
4. `app/main.py` — inside `create_app()`, immediately after `ensure_admin_configured(settings)`: `invalidate_settings()` then `effective = get_settings()` and ONE boot log line, `settings overrides loaded count=N` (or `source=env-only disabled` when the kill switch is off) — the prime that turns a boot-time DB miss into a retry rather than a permanently cached env-only view. Keep the module-level `settings = get_settings()` as is (it is the pre-boot value the middleware wiring uses today).
|
||||
5. `tests/conftest.py` — next to the existing `os.environ.setdefault("BOR_ADMIN_PASSWORD", …)` block, add `os.environ.setdefault("BOR_SETTINGS_DB", "0")` with a comment: the shared test DB must never retune the in-process suites (the same poison-guard reasoning as the phase-140 `BOR_PDF_PICTORIAL` pin in `tests/e2e/conftest.py`), and tests that WANT the overlay set the var + `get_settings.cache_clear()` themselves.
|
||||
6. `tests/e2e/conftest.py` — same pin beside the `os.environ["BOR_PDF_PICTORIAL"] = "0"` line (~L52): `os.environ["BOR_SETTINGS_DB"] = "0"`, comment it as the cross-suite poison guard (one suite's saved row would silently change every later suite's app subprocess). The phase-168 E2E suite overrides it to `"1"` in its own module app fixture (the `test_pictorial_pdf.py:339` `env["BOR_PDF_PICTORIAL"] = "1"` pattern) — task 06 does that.
|
||||
7. `tests/unit/test_settings_catalog.py` (new) — `set(CATALOG) == set(Settings.model_fields)` (both directions, the diff in the failure message — this is the pin that stops a new setting from being invisible in the panel); `every group in GROUPS exists in CATALOG and every CATALOG group is in GROUPS`; `SECRETS == {llm_api_key, admin_password, session_secret, database_url}`; `NON_OVERRIDABLE == {database_url, embedding_dim}`; `env_var("relevance_threshold") == "BOR_RELEVANCE_THRESHOLD"`; no label is empty and no label duplicates another.
|
||||
8. `tests/unit/test_settings_overlay.py` (new) — pure/unit level, no DB: `enabled()` matrix (`"0"/"false"/"no"/"off"` off; unset/`"1"/"true"` on); `_coerce` for int/float/bool/str and `suggestions` JSON (a malformed JSON value is skipped, not fatal); `merge` happy path over the real env (`{"relevance_threshold": "0.55"}` → float 0.55, everything else byte-equal to `Settings()`), `merge` skips an unknown key and a `NON_OVERRIDABLE` key with a reason, `merge` drops one invalid value and keeps the rest, a cross-field violation (`lexical_support_floor` above a DB-overridden `relevance_threshold`) lands in `skipped` instead of raising, bool spellings (`"false"`, `"0"`) behave like the env, `load_overrides()` returns `{}` + one WARNING when the engine raises, `register_invalidator`/`run_invalidators` runs hooks in order and survives a raising hook, and a pin that `app/core/settings_overlay.py` contains no `get_settings(` call (the recursion guard) and that no file this phase owns writes `.env` (LOCKED A1 — assert the absence of an `open(` / `.env` write in `app/core/settings_overlay.py`, `app/config.py` and — once it exists — `app/api/admin_settings.py`).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the two new files; then `uv run pytest tests/unit -q` (the `get_settings` surface is unchanged for every existing pin) and `uv run pytest tests/integration/test_api.py -q` (it calls `get_settings.cache_clear()` — it must pass untouched).
|
||||
- Integration: `get_settings()` with `BOR_SETTINGS_DB=0` is byte-identical to `Settings()` — pin it in `tests/integration/test_admin_settings_api.py` (task 04) or here if the DB is up.
|
||||
- Coverage: **>90%** on the new modules — the fail-soft branches (undefined table, generic error, invalid value, non-overridable row) each need a test.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run python -c "from app.config import Settings, get_settings; print(get_settings().relevance_threshold)"` prints the env value, and `BOR_RELEVANCE_THRESHOLD=0.11 …` still prints 0.11 (the env path is untouched).
|
||||
- [ ] With a live DB and one hand-inserted `app_settings` row + `BOR_SETTINGS_DB=1` + `invalidate_settings()`, `get_settings()` reports the DB value; with `BOR_SETTINGS_DB=0` it reports the env value.
|
||||
- [ ] A hand-inserted row for `database_url` or `embedding_dim` changes nothing and logs the skip.
|
||||
- [ ] `tests/{conftest,e2e/conftest}.py` pin the switch off; the whole `uv run pytest` suite passes with a *populated* `app_settings` table present (the poison guard proven by running the suite twice: once with the table empty, once with a garbage row in it that must change nothing).
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -1,68 +0,0 @@
|
||||
# Task 03 — Live apply: Save re-points the two in-process singletons
|
||||
|
||||
**Phase:** `168_admin_settings_panel` · **Source:** `TODO.md:3` — "The change should take effect on clicking \"save\" on that page."
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
After `invalidate_settings()`, the next request behaves under the new value with **no restart**: the ~70 per-use settings are covered by the cache clear alone, and the two objects that captured a `Settings` snapshot — the shared `LLMClient` and the chat concurrency semaphore — are rebuilt.
|
||||
|
||||
## Why only two objects (traced 2026-10-09, do not re-derive)
|
||||
Every other setting is read through `get_settings()` at the point of use (`app/rag/retriever.py:408,700,807,860`, `app/core/response_budget.py:172`, `app/api/sync.py:528`, the `LLMClient()` constructions in `app/api/sync.py:532`, `app/api/docs.py:480`, `app/api/git_sources.py:944`, the path fields resolved per operation), so clearing the cache is the whole job for them. `app/core/caching.py`'s `functools.cache` `asset_version()` is the deploy token (A19), not a setting. The genuine snapshots are:
|
||||
|
||||
- `app/api/chat.py:318-326` — `_llm: LLMClient | None`, built once by `get_llm()`; `LLMClient.__init__` (`app/rag/llm.py:246-254`) holds `self.settings` and bakes `llm_base_url`, `effective_api_key` and `llm_timeout` into the `AsyncOpenAI` client, while `llm_chat_model` / `llm_embed_model` / `llm_summary_model` are read off that captured snapshot on every call.
|
||||
- `app/api/chat.py:301-315` — `_chat_semaphore: asyncio.Semaphore | None`, built once from `chat_max_concurrent`.
|
||||
|
||||
Boot-wired values (`db_pool_*`, `session_*`, `static_dir`, `log_level`, `compress_min_bytes`, `database_url`) are LOCKED as `restart_required` / non-overridable — this task does not attempt them (A3/A4).
|
||||
|
||||
## Work
|
||||
1. `app/api/chat.py` — add, directly below `get_llm()` (and documented as the phase-168 counterpart of the lazy-init comments above it):
|
||||
```python
|
||||
def reset_runtime_caches() -> None:
|
||||
"""Drop the settings-derived singletons (phase 168, task 03).
|
||||
|
||||
Called by ``invalidate_settings()`` after the admin saves. The next
|
||||
turn rebuilds the client from the CURRENT ``get_settings()`` — an
|
||||
in-flight turn keeps the client it started with (its ``AsyncOpenAI``
|
||||
transport is mid-request; swapping it would kill the stream), which
|
||||
is the same accepted-semantics note as A12's in-process state.
|
||||
``_chat_active`` is deliberately untouched: it counts live turns, and
|
||||
the new ``chat_max_concurrent`` applies as permits are taken.
|
||||
"""
|
||||
global _llm, _chat_semaphore
|
||||
_llm = None
|
||||
_chat_semaphore = None
|
||||
```
|
||||
2. `app/main.py` — in `create_app()`, right after the task-02 prime line: register the hook and log it once.
|
||||
```python
|
||||
# Phase 168 (task 03): the settings panel's Save must retune the running
|
||||
# process, and app/core must never import app/api (PLAN §3's direction) —
|
||||
# so create_app(), the composition root, wires the reset hook.
|
||||
register_invalidator(reset_runtime_caches)
|
||||
```
|
||||
with `from app.api.chat import reset_runtime_caches` added to the existing `app.api.chat` import block and `from app.core.settings_overlay import register_invalidator` alongside the other `app.core` imports. Importing `app.api.chat` here is already done (the router import at the top of the file), so this adds no new import cost or cycle.
|
||||
3. `tests/unit/test_settings_live_apply.py` (new) — source/behaviour pins in the house style:
|
||||
- `reset_runtime_caches()` nulls both globals: set `app.api.chat._llm = object()` and `app.api.chat._chat_semaphore = object()`, call it, assert both are `None`; assert `_chat_active` is untouched by the call;
|
||||
- `get_llm()` rebuilds after a reset and returns the SAME instance on two calls with no reset (the singleton contract still holds — this pin stops a future "always construct" shortcut that would break connection pooling);
|
||||
- `_get_chat_semaphore()` rebuilds after a reset and reflects a changed `chat_max_concurrent` (drive it with `monkeypatch.setenv("BOR_CHAT_MAX_CONCURRENT", "3")` + `get_settings.cache_clear()`, assert `_chat_semaphore._value == 3`), then restore;
|
||||
- `app/main.py`'s source contains `register_invalidator(reset_runtime_caches)` inside `create_app` (the static pin that stops the wiring being "cleaned up" — without it a Save silently stops retuning the LLM client);
|
||||
- `invalidate_settings()` runs registered hooks and clears the cache (call it after registering a counter, assert the counter moved and that `get_settings()` afterwards re-resolves);
|
||||
- a hook that raises does not propagate out of `invalidate_settings()` (a failed reset must not turn a committed save into a 500).
|
||||
4. `tests/integration/test_admin_settings_live.py` (new; DB required — use the existing `db` fixture + `pytest.skip` idiom from `tests/integration/test_ui_settings_api.py`) — the end-to-end proof of "no restart", in-process:
|
||||
- start from an empty `app_settings` and `monkeypatch.setenv("BOR_SETTINGS_DB", "1")` + `invalidate_settings()`, teardown `DELETE FROM app_settings` + `invalidate_settings()` + `get_settings.cache_clear()` (never leave a row behind for the suites that run after);
|
||||
- `relevance_threshold`: read `get_settings().relevance_threshold` (the env/test value), `INSERT` the row `('relevance_threshold','0.5')` directly, `invalidate_settings()`, assert `get_settings().relevance_threshold == 0.5` and that a retrieval call that consults it (`app/rag/retriever.py`'s gate reads it at `:408`) now behaves under 0.5 — assert through the function the way `tests/integration/test_recency_boost.py` does, no chat turn needed;
|
||||
- `llm_chat_model`: with the row `('llm_chat_model','gpt-test')`, `invalidate_settings()`, then `app.api.chat.get_llm().settings.llm_chat_model == "gpt-test"` **and** the rebuilt client's `AsyncOpenAI` base_url/timeout come from the new snapshot (set `('llm_timeout','7')` in the same batch and assert `client._client.timeout.connect == 7.0`, the way `app/rag/llm.py` builds it) — this is the pin that a Save actually swaps the client;
|
||||
- `chat_max_concurrent`: same shape, assert the rebuilt semaphore's `_value` (via `_get_chat_semaphore()`) follows the saved number;
|
||||
- the kill-switch counterpart: with rows present but `BOR_SETTINGS_DB=0`, `invalidate_settings()` → the env values are back (the switch really is the whole feature's kill switch);
|
||||
- `import_extensions` (the `scripts/` consumer): after the save, `get_settings().import_extension_set` reflects the DB value — the importer and Sync read the same overlay (no separate code path).
|
||||
5. `.env.example` — extend task 01's comment block with the one sentence that matters operationally: values marked "restart required" in the panel (DB pool sizes, session cookie wiring, log level, compression, the static dir) apply on the next `podman compose restart app`, everything else applies immediately.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: `uv run pytest tests/unit/test_settings_live_apply.py -v`, then `uv run pytest tests/unit -q`; the existing chat pins (`tests/unit/test_chat_*`, `tests/integration/test_chat_api.py`, `test_chat_concurrency.py`) pass untouched — the reset only runs when `invalidate_settings()` is called, which nothing in them calls.
|
||||
- Integration: `uv run pytest tests/integration/test_admin_settings_live.py -v` with the DB up; then `uv run pytest tests/integration -q` to prove the teardown left nothing behind.
|
||||
- Coverage: **>90%** on `app/api/chat.py`'s new function and `app/core/settings_overlay.py`'s hook paths.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] Saving a value (row insert + `invalidate_settings()`) changes what the NEXT request does — pinned for a retrieval setting, the LLM client (model + timeout), and the chat concurrency cap, with no process restart.
|
||||
- [ ] An in-flight turn is never swapped mid-stream (the docstring's contract; the unit test proves `get_llm()` hands out the same object until a reset).
|
||||
- [ ] `register_invalidator(reset_runtime_caches)` is wired in `create_app()` and unit-pinned; `app/core/` imports no `app/api/` symbol (grep-clean).
|
||||
- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean.
|
||||
@@ -1,51 +0,0 @@
|
||||
# Task 04 — `/api/admin/settings`: the admin-only read/write surface
|
||||
|
||||
**Phase:** `168_admin_settings_panel` · **Source:** `TODO.md:3` — "…can edit anything in .env and save it to the database."
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
One admin-only router that serves the whole catalogue (every setting, its effective value, and where that value came from) and writes single values back to `app_settings` — validated by the real `Settings` validators before anything is stored, masked where the value is a secret.
|
||||
|
||||
## Work
|
||||
1. `app/schemas.py` — append the four models (docstrings in the `UiSettingsIn`/`UiSettingsOut` style, naming the phase and the route):
|
||||
- `AdminSettingItem`: `key: str`, `env_var: str`, `group: str`, `label: str`, `type: Literal["int","float","bool","str","json"]`, `secret: bool`, `restart_required: bool`, `overridable: bool`, `source: Literal["db","env","default"]`, `value: str | None` (the effective value in env-native string form; **`None` when masked**), `masked: bool`, `has_value: bool` (for a masked field: is a non-empty value in effect at all), `has_override: bool` (a row exists), `db_value: str | None` (the stored string when it is a stored non-secret override, else `None`), `default: str` (the code default stringified — for a masked field `""`).
|
||||
- `AdminSettingsOut`: `items: list[AdminSettingItem]`, `totals: {"count": int, "overridden": int}`, `overlay_enabled: bool` (the `BOR_SETTINGS_DB` state — when `false` the panel shows a banner instead of pretending a save took effect).
|
||||
- `AdminSettingIn`: `value: str` — always a string, coercion belongs to pydantic (the same reason D1 stores strings).
|
||||
- `AdminSettingsCleared`: `cleared: int`.
|
||||
Stringify helpers live in the router (`_display(field, value)`), not in the schemas: `bool` → `"true"/"false"`, `list[str]` → `json.dumps(...)`, everything else `str(value)`.
|
||||
2. `app/api/admin_settings.py` (new) — `router = APIRouter(prefix="/admin/settings", tags=["admin-settings"], dependencies=[Depends(require_admin)])` (the `app/api/ui_settings.py` pattern: anonymous AND token users 403 on every route). Module docstring lists the routes and the three invariants (validate-before-store, masked secrets, never write `.env`).
|
||||
- `GET ""` → `AdminSettingsOut`: read the rows once (`SELECT key, value FROM app_settings`), resolve the effective `Settings` through `get_settings()`, then emit one `AdminSettingItem` per catalog entry **in `GROUPS` order, catalog order within the group** (a stable order the E2E pins against). `source`: `db` when an *allowed* row is in effect, `env` when the process env has the `BOR_*` var set (check `os.environ`), else `default`. A masked field emits `masked=True`, `value=None`, `db_value=None` and `has_value=bool(effective)`.
|
||||
- `PUT "/{key}"` (`AdminSettingIn` → `AdminSettingItem`, 200):
|
||||
- unknown key → 404 `f"Unknown setting {key}"`;
|
||||
- `overridable=False` → 422 `f"{env_var} is not settable from the database"` + the reason in the docstring (A3: the engine is built from `database_url`; `chunks.embedding` is fixed at `embedding_dim` by A6);
|
||||
- `secret=True` and the value is empty/whitespace → 422 `f"{env_var} must not be blank — use Reset to fall back to the environment"` (an empty API key or session secret is a self-lockout; `DELETE` is the explicit clear);
|
||||
- **candidate-set validation** (A5/D5): the stored rows for the allowed keys + this key/value, run through the same `app.core.settings_overlay.merge()` path — a `ValidationError` (bad shape OR a cross-field rule like `lexical_support_floor` above a DB-overridden `relevance_threshold`) is a 422 whose detail names the field, and **nothing is written**;
|
||||
- write: `INSERT … ON CONFLICT (key) DO UPDATE SET value = excluded.value, updated_at = now()` (`sqlalchemy.dialects.postgresql.insert`), commit, then `invalidate_settings()` (task 03 makes it live) and return the refreshed item.
|
||||
- `DELETE "/{key}"` → `AdminSettingItem`: delete the row (idempotent — deleting an absent key is a success, not a 404), `invalidate_settings()`, return the item as `source="env"|"default"`.
|
||||
- `DELETE ""` → `AdminSettingsCleared`: `DELETE FROM app_settings`, `invalidate_settings()`, `{"cleared": rowcount}` (the panel's "Clear all overrides").
|
||||
- **Log one line per write, never the value** (AGENTS rule 10, A4): `settings action=set key=… status=200`, `settings action=clear key=…`, `settings action=clear_all count=N` on the module logger `app.api.admin_settings`. GETs log nothing here — the phase-167 `ResponseBudgetMiddleware` already writes the `api method=… bytes=…` line, and the whole catalogue must stay under `BOR_LOG_LARGE_RESPONSE_BYTES`' INFO bar (75 items ≈ 20 KB; verified in task 06 with the audit script).
|
||||
- Never opens `.env`, never shells out, never `eval`s a value — the module contains no file writes (unit-pinned).
|
||||
3. `app/main.py` — `from app.api.admin_settings import router as admin_settings_router` in the router-import block and `app.include_router(admin_settings_router, prefix="/api")` right after the `ui_settings_router` line, with the one-line comment the neighbours carry ("admin-only (router-wide require_admin; a token user stays 403)").
|
||||
4. `tests/integration/test_admin_settings_api.py` (new; the `db`-fixture + skip idiom of `tests/integration/test_ui_settings_api.py`, admin client per that file's convention). Set `BOR_SETTINGS_DB=1` + `invalidate_settings()` in the fixture, and in teardown `DELETE FROM app_settings` + `invalidate_settings()` + `get_settings.cache_clear()` — **no row may survive this module** (the same discipline `test_admin_theme_tab.py:286` applies to `ui_settings`). Pin:
|
||||
- **auth matrix:** anonymous `403`, token user `403` (issue a token the way `test_tokens_api.py` does), admin `200` — on all four routes;
|
||||
- **shape:** `items` length == `len(Settings.model_fields)`, groups in `GROUPS` order, `totals.count`/`overridden` correct, `overlay_enabled` true; on an empty table a field whose `BOR_*` var the test env sets reports `source="env"`, an untouched one `source="default"`, and none reports `"db"`;
|
||||
- **write → live:** `PUT {"value": "0.5"}` for `relevance_threshold` → 200 with `source="db"`, `get_settings().relevance_threshold == 0.5`, `totals.overridden == 1`, and the row present with a non-null `updated_at`;
|
||||
- **visible-through-the-app proof:** `PUT input_placeholder` then `GET /api/config` (no auth) shows the DB value — the DB-over-env rule reaches the public config endpoint through the same overlay;
|
||||
- **errors:** unknown key → 404; `database_url` / `embedding_dim` → 422 naming the env var; `"abc"` for `relevance_threshold` → 422 naming `relevance_threshold` and the table stays empty; a cross-field pair that only breaks in combination (store `relevance_threshold=0.2`, then `lexical_support_floor=0.9`) → the second `PUT` 422s and the first row is unchanged;
|
||||
- **masking:** store `llm_api_key` directly, `GET` reports `masked=True`, `value is None`, `has_value=True`, and the plaintext appears NOWHERE in `response.text` (assert on the raw text, not the parsed model); blank `PUT` for it → 422; `DELETE` clears it;
|
||||
- **reset:** `DELETE /{key}` → the item is `source="env"` (with the env var set in the fixture) and `get_settings()` is back to the env value; `DELETE ""` with 3 rows → `{"cleared": 3}` and `overridden == 0`;
|
||||
- **kill switch:** with `BOR_SETTINGS_DB=0` (monkeypatch + `invalidate_settings()`) `overlay_enabled` is `false` while a `PUT` still stores the row and `get_settings()` still reports the env value — the switch disables the mechanism, not the panel;
|
||||
- **byte budget:** `len(response.content)` for the full `GET` is under 64 KB (the phase-167 INFO bar) — so the catalogue never floods the log.
|
||||
5. `README.md` — under **Configuration reference** (L1071): a short lead paragraph stating the precedence order (`app_settings` row → `BOR_*` environment → code default), that the panel never writes `.env`, that `BOR_SETTINGS_DB=0` disables DB overrides, and that the values marked "restart required" apply on the next `podman compose restart app`.
|
||||
|
||||
## Testing & Quality
|
||||
- Integration: `uv run pytest tests/integration/test_admin_settings_api.py -v` (DB up), then `uv run pytest tests/integration -q` twice with a `app_settings` row left in on purpose ONCE inside the run only — the module's teardown must clean it (proven by a second clean run).
|
||||
- Unit: the module's masking + `_display` helpers (bool/JSON/scalar stringification), the `.env`-never-written and no-shell static pins.
|
||||
- Coverage: **>90%** on `app/api/admin_settings.py` (every 404/422 branch + both `DELETE`s + the masked branch).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `GET /api/admin/settings` returns all 75 settings grouped, each with its effective value and a `db`/`env`/`default` badge; secrets report `masked`.
|
||||
- [ ] `PUT` stores, validates first (nothing written on failure, 422 names the field) and makes the value live through `get_settings()` in the same process.
|
||||
- [ ] `DELETE /{key}` restores the env value; `DELETE ""` clears everything; both log a value-free line (`grep " :: app.api.admin_settings " <logs>`).
|
||||
- [ ] Anonymous and token-user callers get 403 on every route; the byte audit keeps this endpoint under the 256 KB budget and, pinned, under 64 KB.
|
||||
- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean.
|
||||
@@ -1,53 +0,0 @@
|
||||
# Task 05 — The Settings view: the shell's eighth view, `#view-settings`
|
||||
|
||||
**Phase:** `168_admin_settings_panel` · **Source:** `TODO.md:3` — "I want an admin panel that can edit anything in .env … The change should take effect on clicking 'save' on that page."
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
An admin opens **Settings**, sees every setting grouped with the value in effect and where it came from, edits a few, presses **Save settings**, and sees the badges flip to `DB` — while the anonymous visitor gets the sign-in gate and a token user never sees the nav link. Same wiring as the phase-91 Theme view, same table discipline as the phase-156 catalogue tables.
|
||||
|
||||
## Work
|
||||
1. `frontend/index.html`
|
||||
- nav: after `#nav-theme` (L62) add `<a href="/settings.html" class="nav-link" id="nav-settings" hidden>Settings</a>` (ship-hidden, revealed for admin only — the phase-34 one-bar contract; the same one line goes into `document.html` (~L74), `login.html` (~L67) and `shared.html` (~L68) after their Theme links).
|
||||
- view: after `#view-theme` add `<section class="view" id="view-settings" hidden inert aria-label="Settings" tabindex="-1">` following the `#view-tokens` skeleton exactly:
|
||||
- `<div class="container settings-shell">` (PLAN §7.1's 72rem centered column — no new width token),
|
||||
- `.page-head` with `<h1>Settings</h1>` and a `.page-sub` that states the rule in the owner's terms: values saved here are stored in the database and win over the server's environment variables; clearing one hands control back to the environment,
|
||||
- `<section class="sources-gate" id="settings-gate" aria-labelledby="settings-gate-title" hidden>` — the `#tokens-gate` shape, heading "Sign in to change settings", sub-copy naming what is admin-only, `<a class="sources-gate-link" href="/login.html?next=/settings.html">Sign in</a>`,
|
||||
- `<div id="settings-content" hidden>` holding, in order:
|
||||
- `<div class="kb-banner is-error" id="settings-disabled" role="alert" hidden>` — shown when `overlay_enabled` is `false` ("DB overrides are switched off on the server (`BOR_SETTINGS_DB=0`) — saving stores the value but it will not take effect."),
|
||||
- `<span class="settings-status" id="settings-status" role="status" aria-live="polite"></span>` (the ONE status region, the `#tokens-status` precedent) and `<p class="settings-error" id="settings-error" role="alert" hidden></p>` (the `#theme-error` precedent),
|
||||
- the search row: `<label for="settings-filter">Search settings</label>` + `<input id="settings-filter" type="search" autocomplete="off">` (client-side filter, no fetch — the phase-101 D4 contract),
|
||||
- the scroll card: `<div class="table-wrap"><table class="settings-table"><caption class="visually-hidden">Application settings — the value in effect and where it comes from</caption><thead><tr><th scope="col">Setting</th><th scope="col">Value</th><th scope="col">Source</th><th scope="col"><span class="visually-hidden">Actions</span></th></tr></thead></table></div>` — the `tbody` elements are created by `settings.js` (one per group, each opening with a `<tr class="settings-group-row"><th colspan="4" scope="colgroup">Group</th></tr>`), and the `.table-wrap` + sticky-`th` rules at `styles.css:2825/2841` are already the bounded-scroll + sticky-header contract (AGENTS rule 5) — `.visually-hidden` inside the header needs the `.table-wrap` `position: relative` that phase 86 added,
|
||||
- the action row: `<button type="button" class="btn primary" id="settings-save" disabled>Save settings</button>` + `<button type="button" class="btn ghost" id="settings-clear-all">Clear all overrides</button>` + `<span class="settings-dirty" id="settings-dirty" aria-hidden="true"></span>` (an "N unsaved changes" hint, `textContent` only).
|
||||
- No `<script>` tag for the new module: the router lazy-imports it (rule 6 — everything local).
|
||||
2. `frontend/assets/router.js` — three entries in the phase-91 style: `"/settings.html": "settings"` in `VIEW`, `settings: "/settings.html"` in `VIEW_PATH`, `settings: () => import("./settings.js")` in `VIEW_MODULES` (a STATIC specifier so the Containerfile's `esbuild --bundle` of `router.js` inlines it — `settings.js` needs no new esbuild line because no page references it directly; `tests/integration/test_containerfile_assets.py` proves it).
|
||||
3. `app/main.py` — add `"/settings.html"` to the `_shell_routes(...)` tuple (after `"/theme.html"`) with the usual comment.
|
||||
4. `app/core/caching.py` — add `"/settings.html"` to `HTML_PAGES` (L169 block) with the `# phase 168: the admin settings view (shell route)` comment, so it gets the `no-cache` + `?v=` contract (A19).
|
||||
5. `frontend/assets/header.js` — after the Theme reveal block (~L225) add the null-safe `#nav-settings` block, admin-only, with the comment naming the contract it copies (a token user never sees it).
|
||||
6. `frontend/assets/settings.js` (new) — `export async function mount(root)`, the `theme.js` shape (gate first via `fetchIsAdmin()`/`fetchWhoami()` from `header.js`; anonymous shows `#settings-gate` and fetches nothing; then unhide `#settings-content`, load, bind; listen for `bor:view-refresh` to re-load on a re-show):
|
||||
- `GET /api/admin/settings` → render the groups in the server's order (`createElement` + `textContent` only, never HTML strings — PLAN §7 frontend rule). One row per setting:
|
||||
- **Setting cell:** the `label` (`.cell-clip` ellipsis when it truncates, the phase-156 contract with the full text in the control's `aria-label`), the `BOR_*` name in a `<code class="settings-env">`, and — when `restart_required` — a `.settings-flag` with the literal text `restart required` (text, never color alone, B5) plus `title`/`aria-describedby` copy explaining when it applies;
|
||||
- **Value cell:** the control chosen by `type` — `int`/`float` → `<input type="number">` (`step="any"` for float), `bool` → a two-option `<select>` (`true`/`false`), `json` → `<textarea spellcheck="false">`, `str` → `<input type="text">` (a `<textarea rows="2">` for the CSV/path fields whose value exceeds 40 chars), `secret` → `<input type="password" autocomplete="new-password" placeholder="•••••• (stored)">` with `aria-label` naming the env var. Every control gets an `id` (`set-<key>`) and a `<label for>` (visually the Setting cell's label, the house pattern for table-hosted inputs). For a masked field an empty input means "leave unchanged" (never an accidental wipe);
|
||||
- **Source cell:** `<span class="settings-src is-db|is-env|is-default">DB</span>` / `env` / `default` — literal text;
|
||||
- **Actions cell:** a `Reset` button, `disabled` when `!has_override` (nothing to reset) or `!overridable` (with `title` naming the reason).
|
||||
- dirty tracking in module state (`Map<key, value>`): an `input`/`change` marks the row (`.is-dirty` + the `N unsaved changes` hint) and enables `#settings-save`; a row that returns to its served value un-marks.
|
||||
- `#settings-save` → for each dirty row in DOM order `PUT /api/admin/settings/<key>` with `{ value }`; **stop at the first failure** (K4), show its server detail in `#settings-error` (role=alert), keep the remaining edits in the form, re-`GET` so the badges show the truth, keep focus on the offending input. All good → one `#settings-status` line: `Saved 3 settings.` (then `role=status`, not alert) + a re-`GET` so every badge/value is the settled server state. While in flight the button is `disabled` with `aria-busy="true"` (§7.4: every control state defined; a failure path always re-enables).
|
||||
- `Reset` → `DELETE /api/admin/settings/<key>` → status line `BOR_X is back to the environment value.` + re-`GET`. `#settings-clear-all` → the house two-step inline confirm (the `history-confirm-*` classes, focus moves to the confirm button, Escape/`No` cancels) → `DELETE /api/admin/settings` → status line `Cleared N overrides.` + re-`GET`.
|
||||
- `#settings-filter` → case-insensitive substring over the label + `BOR_*` text cells, applied on `input` with no fetch, re-applied after every re-`GET` (the query survives re-shows, phase-77); a group with no visible rows hides its group row; zero matches with a non-empty query → one no-match row `No settings match "<query>"` (`textContent`).
|
||||
- A `restart_required` row that saves successfully adds "— applies on the next restart" to the status line (so the owner is never told "saved" and then sees nothing change).
|
||||
7. `frontend/assets/styles.css` — a `---------- Settings view (phase 168) ----------` block: `.settings-shell` (no new container), `.settings-table { width: 100%; }` with `.git-sources-table`-style column proportions (never a skinny list — PLAN §7.1), `.settings-group-row th` (a `--surface` band, `--ink` text ≥4.5:1), `.settings-env` (mono, `--ink-soft`), `.settings-src` + the three `is-*` variants (solid `--brand-soft` / `--surface` / `--line` backgrounds with the existing ink tokens — no new palette variable, the phase-156 B3 rule; the badge is TEXT, the color is decoration), `.settings-flag` (`--accent-bg`/`--accent-ink`, the state-family pair), `.settings-row.is-dirty` (a `--brand` left border on the row + the `N unsaved changes` hint, so "unsaved" survives grayscale), `.settings-status` (the `.tokens-status` mirror), `.settings-error` (the `.theme-error` mirror), `input`/`select`/`textarea` sizing with `:focus-visible` inherited from the global rule, and ≥44px touch targets for the buttons/inputs at `≤640px` (§7.1 mobile). Nothing animates, so `prefers-reduced-motion` needs no new rule.
|
||||
8. `tests/unit/test_settings_frontend_view.py` (new — the house static-source-assertion style, cf. `tests/unit/test_thinking_scroll.py` / `test_frontend_router.py`): `index.html` contains the `#view-settings` section, the `#settings-gate`, `#settings-content`, `#settings-status role="status"`, `#settings-error role="alert"`, `#settings-save`, `#settings-clear-all`, and `#nav-settings` with `hidden`; `router.js` maps `/settings.html` in all three tables; `main.py`'s shell tuple and `caching.HTML_PAGES` both list `/settings.html`; `header.js` reveals `#nav-settings` and keys it off `admin` (not `signedIn`); `settings.js` contains no `innerHTML =` with a template literal and no `http(s)://` reference (rule 6 + the no-HTML-strings rule); `styles.css` declares `.settings-table` and the three `.settings-src.is-*` variants and does NOT add a `:root` variable (B3); the four pages that carry the Theme link now carry the Settings link too.
|
||||
9. `README.md` — a **Settings (`/settings.html`)** subsection under "Pages at a glance" (after "Access tokens", L205): what it is, the precedence rule, "restart required", and that it is admin-only.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the new file, plus `uv run pytest tests/unit/test_frontend_router.py tests/unit/test_caching.py -v` (both enumerate the views/pages and must be *extended*, not broken) and `uv run pytest tests/integration/test_containerfile_assets.py -v` (the new asset is reachable through the bundle).
|
||||
- Integration: `uv run pytest tests/integration/test_caching_revalidation.py -v` — `/settings.html` must serve `no-cache` with rewritten `?v=` asset refs.
|
||||
- The E2E for this view is task 06's file; here the view must be reachable by hand (`uv run uvicorn app.main:app --reload` → sign in → Settings).
|
||||
- Coverage: **>90%** on the `app/` diff of this task (the shell route + `HTML_PAGES`), untouched elsewhere.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `/settings.html` deep-links to the Settings view (the shell document, `router.js` picks the view), anonymous sees the gate and no data, a token user never sees the nav link.
|
||||
- [ ] The table lists every group with a bounded scroll region and a sticky header at 72rem and at 375px; long labels clip with the ellipsis and stay reachable through the control's `aria-label`.
|
||||
- [ ] Editing marks the row dirty, **Save settings** reports one status line and every badge/value re-reads from the server; a rejected value shows the server's detail in the `role="alert"` region and keeps the other edits.
|
||||
- [ ] Keyboard-only operation works end to end (filter → edit → Save → Reset → two-step Clear-all), `:focus-visible` visible on every control, and the page introduces no CDN reference and no HTML-string DOM building.
|
||||
- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean.
|
||||
@@ -1,57 +0,0 @@
|
||||
# Task 06 — `tests/e2e/test_admin_settings_panel.py`: the live-on-Save contract, proven in a browser
|
||||
|
||||
**Phase:** `168_admin_settings_panel` · **Source:** `TODO.md:3` — "…The change should take effect on clicking 'save' on that page."
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
The phase's dedicated Playwright suite (AGENTS rule 4/9): a real admin signs in, changes a setting through the panel, presses **Save**, and the running app behaves differently — no restart, no `.env` edit — plus the gate, the masking, the rejection path, and the reset. Then the full gates and the one atomic commit.
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_admin_settings_panel.py` (new) — copy the module-scoped app fixture from `tests/e2e/test_admin_theme_tab.py:205-270` verbatim (mock LLM, `BOR_ENVIRONMENT=e2e`, `BOR_STATIC_DIR`, the E2E admin password/secret, the docs-repo/suggestions/app-name leak guards) and change exactly these lines:
|
||||
- `env["BOR_SETTINGS_DB"] = "1"` — the module opts back in over `tests/e2e/conftest.py`'s global `0` pin (task 02), the `test_pictorial_pdf.py:339` opt-in pattern;
|
||||
- `env["BOR_INPUT_PLACEHOLDER"] = "Env placeholder for panel E2E"` and `env["BOR_FOOTER_TEXT"] = "Env footer for panel E2E"` — a distinctive env base, so "the DB row now wins" is a visible DOM change and not a no-op against the default;
|
||||
- `env["BOR_RELEVANCE_THRESHOLD"] = "0.30"` (the E2E calibration, so a test that saves `0.31` proves the retrieval path read the new number, not a broken gate).
|
||||
Add the module docstring in the house style: what the suite proves, the DB-isolation rule, and the Source line (`TODO.md:3`).
|
||||
2. **DB isolation** (module autouse fixture, the `test_admin_theme_tab.py:286` `TRUNCATE ui_settings` idiom): `TRUNCATE app_settings` before AND after every test, plus `invalidate_settings()`-equivalent on the server side is unnecessary (the app runs in its own subprocess and re-reads on the save's own invalidation). Teardown must leave the shared dev E2E DB with an empty `app_settings` — a leftover row is the cross-suite poison this phase's D6 exists to prevent.
|
||||
3. Tests (names pinned; helpers for `login(page, app_url)` from `tests/e2e/auth_helpers.py` and the `httpx` + cookies pattern those suites already use):
|
||||
- `test_anonymous_sees_gate_and_no_data` — `/settings.html` serves the shell with `#view-settings` visible, `#settings-gate` visible, `#settings-content` hidden, `#nav-settings` hidden; `GET /api/admin/settings` unauthenticated is 403; no setting label or value appears in `page.content()`.
|
||||
- `test_token_user_never_sees_the_panel` — a token user (issue one as admin via the API the way `test_api_tokens.py` does, then drive the token gate with `login_with_token`) sees `#nav-settings` hidden and gets 403 from the API.
|
||||
- `test_admin_sees_groups_and_source_badges` — after `login()`, `#nav-settings` reveals, the view mounts, rows render, `table.settings-table` is inside `.table-wrap` (bounded scroll, sticky `thead` computed style), at least the `Retrieval & honesty` and `LLM (aipi)` group rows appear, `input_placeholder`'s row shows the served env value and an `env` badge, an untouched knob (e.g. `top_n_docs`) shows its code default with a `default` badge, `llm_api_key` renders a password input whose `value` is `""` and whose placeholder is the "stored" hint, and `database_url` / `embedding_dim` render read-only with no Reset and no input.
|
||||
- `test_save_takes_effect_without_restart` — **the TODO's own acceptance test.** Filter to "placeholder", set the value to `DB placeholder — panel saved this`, press **Save settings**, expect: the `#settings-status` line (`Saved 1 setting`), the badge flips to `DB`, then `goto("/")` (a fresh page load, chat view) and expect the composer's `placeholder` attribute to be `DB placeholder — panel saved this` — the same browser, the same server process, no `.env` touched. Assert `footer_text` is still the env value (only one row was saved) and that the DB value survives a reload of `/settings.html`.
|
||||
- `test_saved_value_reaches_a_real_turn` — set `BOR_RELEVANCE_THRESHOLD` from the panel to `0.31`, POST `/api/chat` (the `test_admin_theme_tab.py`/`test_honest_deflection.py` HTTP helper style) and assert the turn's honesty classification follows the new bar for a fixture question whose cosine sits between `0.30` and `0.31` (if the fixture corpus has no such question — verify with `uv run python -m scripts.eval_retrieval "<question>"` — pin the strongest available proxy: `query_log.top_score` is unchanged while the deflected/grounded decision flips when the saved bar moves above/below it, which proves the live read). Then **Reset** it and assert the pre-save classification returns.
|
||||
- `test_invalid_value_is_refused_and_nothing_changes` — set `relevance_threshold` to `abc`, Save → the `#settings-error` (`role="alert"`) region is visible and names `relevance_threshold`, the badge is still `env`, the row keeps its typed value (K4), `GET /api/admin/settings` still reports `source=env`, and the composer placeholder (a second dirty row that was NOT saved because the batch stopped) is unchanged server-side.
|
||||
- `test_cross_field_pair_is_refused` — save `relevance_threshold = 0.2`, then try `lexical_support_floor = 0.9` → the alert names the field and the stored pair never becomes effective (`GET /api/admin/settings` still shows the first saved value only).
|
||||
- `test_secret_never_reaches_the_browser` — set `llm_api_key` to a sentinel (`sk-e2e-secret-…`) via the API (not through the DOM), reload `/settings.html`, and assert the sentinel is absent from `page.content()` while the row's password input shows the "stored" placeholder and `has_override` renders a live Reset button; then set it from the panel's password input, Save, and assert the sentinel still never appears in the served HTML or the response body.
|
||||
- `test_reset_hands_control_back_to_the_env` — after a DB override on `footer_text`, press its Reset → status line, badge back to `env`, and the footer on `/` reads the env value again.
|
||||
- `test_clear_all_overrides` — save three overrides, `#settings-clear-all` → the two-step confirm (Escape cancels and the overrides survive), then confirm → `Cleared 3 overrides`, every badge back to `env`/`default`.
|
||||
- `test_disabled_switch_is_announced` (subprocess-level, its own module fixture or an `httpx`-only variant): a second app subprocess with `BOR_SETTINGS_DB=0` → the panel shows `#settings-disabled` (`role="alert"`) and a save still stores without changing the app. If a second app fixture is too heavy for the file, pin the `overlay_enabled=false` render through `page.route` interception of `GET /api/admin/settings` instead — state which was done in the commit body.
|
||||
- `test_view_layout_and_accessibility` — no horizontal page overflow at 375px and at 1440px (§7.1: the table is ≥80% of the container, not a skinny list), keyboard tab order reaches filter → first input → Save → Reset (a `page.keyboard` walk asserting focus lands on the real controls), the `.settings-flag` text `restart required` present on `db_pool_size`'s row, and zero external requests during the whole test (`page.on("request")` — rule 6).
|
||||
4. **Regression set, all in isolation** (`--no-cov`): `test_admin_theme_tab.py` (the Theme view's gate/save contract that this view copies), `test_api_tokens.py`, `test_configurable_brand.py` (`/api/config`'s strings — now DB-over-env-capable), `test_global_tuning.py`, `test_admin_auth.py`, `test_cache_busting.py`, `test_wide_desktop_column.py` (the 72rem container contract, new view included).
|
||||
5. **Full gates** (AGENTS rule 9, all green before the commit):
|
||||
```bash
|
||||
uv run pytest
|
||||
uv run pytest --cov=app --cov-report=term-missing # >90% on app/
|
||||
uv run pytest tests/e2e/test_admin_settings_panel.py -v --no-cov
|
||||
uv run ruff check . && uv run pyright
|
||||
```
|
||||
Plus the operational proof of AGENTS rule 10: with the app running, save a setting, then `grep ' :: app.api.admin_settings ' <app log>` shows the key-only line and `grep ' :: api ' <app log>` shows the `GET /api/admin/settings` byte line; `uv run python -m scripts.api_payload_audit` lists the endpoint under the 256 KB budget.
|
||||
6. **Docs & close-out:** confirm `README.md` carries both additions from tasks 04/05 (the Settings page section + the Configuration-reference precedence note); move the phase directory:
|
||||
then move the phase directory and commit (AGENTS rule 8, `--no-gpg-sign`):
|
||||
```bash
|
||||
git mv .agents/phases/todo/168_admin_settings_panel .agents/phases/complete/168_admin_settings_panel
|
||||
```
|
||||
```bash
|
||||
git add app/ alembic/ frontend/ tests/ .env.example README.md .agents/phases/ && git commit --no-gpg-sign -m "feat(settings): admin settings panel with DB-over-env overrides applied live on save"
|
||||
```
|
||||
|
||||
## Testing & Quality
|
||||
- The suite passes **in isolation** (`-v --no-cov`) and the app it drives writes no `.env` change (`git status --porcelain .env` empty after the run — `.env` is gitignored, so assert by `stat` mtime instead if the file is untracked).
|
||||
- Unit + integration already exist from tasks 02/03/04 — this file adds the browser contract only; if it needs a behavior the API lacks, fix the API in this task and say so in the commit body.
|
||||
- Coverage: **>90%** on `app/` after everything (`--cov` runs exclude E2E by the repo's config; the gate is the unit+integration number).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `test_save_takes_effect_without_restart` is green: a value saved through the panel shows up in the running app in the same browser session with no restart.
|
||||
- [ ] Gate, masking, invalid-value, cross-field, reset, clear-all and mobile/keyboard coverage all green in isolation.
|
||||
- [ ] `app_settings` is empty after the suite (`SELECT count(*) FROM app_settings` → 0) and the rest of the E2E suite still passes with that pin in place.
|
||||
- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean, the byte audit clean.
|
||||
- [ ] Phase directory moved to `.agents/phases/complete/168_admin_settings_panel/` and one atomic `--no-gpg-sign` Conventional Commit exists (verify with `git log -1 --stat`).
|
||||
@@ -1,121 +0,0 @@
|
||||
# Phase 169 — Embedding generations: stamp every vector, measure comparability, re-embed in place
|
||||
|
||||
**Source:** owner request 2026-10-09 — "I'm about to upgrade the embedding model from embedding gemma to embedding gemma 2. The dimensions are the same, but the embeddings may not be comparable. What do I need to do on brain of reese's side?"
|
||||
**Story:** n/a (owner-directed infrastructure work)
|
||||
**Context:** `app/models.py:232-249` (`Chunk`, `embedding: Vector(EMBEDDING_DIM)` at L241 — the ONLY vector column in the schema), `app/config.py:102` (`llm_embed_model`), `:142` (`embedding_dim=768`), `:151` (`relevance_threshold=0.62`), `:160` (`lexical_support_floor=0.35`), `:179` (`source_usefulness_floor=0.35`), `:225` (`embed_question_max_chars`), `:548-578` (the two floor ⇄ threshold validators), `app/rag/llm.py:314-322` (`EmbeddingDimensionError` — the ONLY guard, dim-based), `:213-221` (`_TOKENS_PER_CHAR` / `_REQUEST_TOKEN_BUDGET`, written against the current endpoint's ~1024-token cap), `:346` (`embed()`), `:400` (`embed_one()`), `app/rag/importer.py:1008-1023` (content-chunk write), `:1587-1589` (the `is_summary` `position = -1` chunk), `:51-57` (the phase-118 "content-unchanged work is NOT a KB change" convention), `app/rag/retriever.py:433-455` (`_vector_list`, the brute-force cosine scan), `app/api/docs.py:488-491` (the manual-summary write), `app/api/chat.py:740` (the question embed), `app/api/sync.py:844` (`sync: done`), `app/core/auth.py:83` (`require_admin`), `app/main.py:146-172` (router includes), `app/core/settings_overlay.py` + `app/core/settings_catalog.py` (phase 168: the DB-over-env overlay — a stored override BEATS `.env`), `alembic/versions/0028_app_settings.py` (head — next free: **0029**), `scripts/regen_folder_summaries.py` (the one-shot entrypoint idiom this phase's script mirrors), `scripts/eval_retrieval.py`, `scripts/test_embed_model.py`, `scripts/llm_probe.py`, `tests/fixtures/retrieval_battery.txt` (the 43-line gated battery), `benchmarks/model_benchmarks.csv`, `tests/e2e/mock_llm.py` (`POST /v1/embeddings` → deterministic 768-dim bag-of-words), `tests/e2e/conftest.py:53,64` (the global env pins) and `:105-186` (the `app_server` env contract), `README.md:1408-1410` (the "Embedding dimension mismatch" troubleshooting entry).
|
||||
|
||||
## Objective
|
||||
|
||||
An embedding-model change that KEEPS the dimension (768 → 768) is today invisible and therefore silently destructive: nothing records which model produced a vector, `EmbeddingDimensionError` cannot fire, and cosine scores from two different geometries get compared on one list. This phase makes a vector's generation a fact in the database (`chunks.embedding_model`), makes a mismatch loud in the admin UI and the sync log, ships the two tools that answer "are the old vectors still usable?" (`--compare`, measured in cosine agreement) and "make them usable" (`--apply`, a resumable in-place re-embed over the text already stored), and writes the operator runbook the live upgrade follows.
|
||||
|
||||
## The problem, precisely (the executor reads this, not the chat)
|
||||
|
||||
- **No generation is recorded.** `chunks.embedding` is a bare `Vector(768)`; `grep -rn "embedding_model" app/ alembic/` is empty. A row does not know what made it.
|
||||
- **No guard can fire.** The only loud failure is `EmbeddingDimensionError` (`app/rag/llm.py:314-322`), which compares dimension against `BOR_EMBEDDING_DIM`. Same dims → no error, ever.
|
||||
- **A re-sync re-embeds NOTHING.** `documents.file_hash` matches → the importer returns through `_unchanged_doc` with "ZERO embed" (`app/rag/importer.py:70-84`, `:1391-1403`). Flipping `BOR_LLM_EMBED_MODEL` and pressing Sync leaves 100 % of the corpus in the old space while every QUESTION embeds in the new one (`app/api/chat.py:740`) — retrieval degrades with no error anywhere.
|
||||
- **The thresholds are model-calibrated, not absolute.** `relevance_threshold=0.62` / `lexical_support_floor=0.35` / `source_usefulness_floor=0.35` gate honesty (A8) and citation tiering, and `app/config.py:147-150` records that they were hand-re-tuned on 2026-08-21 *for the current model's* cosine distribution ("the default never discriminated"). A new model moves the whole distribution.
|
||||
- **No ANN index to rebuild** — `pg_indexes` for `chunks` is `chunks_pkey`, `ix_chunks_document_id`, `ix_chunks_tsv` only; `_vector_list` is a brute-force scan. (If a hnsw/ivfflat index is ever added, it must be built AFTER a re-embed.)
|
||||
- **Scale:** the live KB (and `data/bor_live_kb_replica.dump.sql`) is **1,000 documents / 8,866 chunks**, ~1,000 of them the `is_summary` `position = -1` row. At the benchmarked 93.8 vec/s a full re-embed is minutes — so a quiesced in-place run beats a blue/green column.
|
||||
|
||||
## Design (shared by all tasks)
|
||||
|
||||
**D1 — One nullable `chunks.embedding_model` text column (task 01, migration `0029`).** The value is the `llm_embed_model` string that was in effect when the vector was written. Nullable and **never backfilled by the migration**: for pre-phase rows the true producing model is genuinely unknown, and writing the current setting into them would be a lie that survives the very migration this phase exists to make detectable. `NULL` therefore means *unknown* and is reported as its own bucket. No index: the column is read by `GROUP BY` audits and by the re-embed's `WHERE` over ≤ 10⁴ rows, where a seq scan beats maintaining an index on a low-cardinality column on every chunk write.
|
||||
|
||||
**D2 — One write helper, so a fourth path cannot forget (task 02).** `app/core/embedding_generation.py::stamp_embedding(chunk, vector, model)` sets `embedding` and `embedding_model` together. Every path that writes a vector goes through it: the content chunks (`app/rag/importer.py:1008-1023`), the summary chunk (`:1587-1589`), and the manual-summary edit (`app/api/docs.py:488-491`). A unit pin asserts no `Chunk` vector write in `app/` bypasses it (source-scan style, the house static-pin idiom).
|
||||
|
||||
**D3 — Comparability is MEASURED, not assumed (task 04).** `scripts/reembed_embeddings.py --compare` takes N chunks (`--sample`, default 25, `ORDER BY id` — deterministic, no RNG), embeds their **stored** `content` with the endpoint/env it is given, and reports the cosine distribution (mean / median / p10 / min) against the **stored** vectors. Bands (LOCKED A4): mean ≥ **0.995** → `COMPATIBLE` (same geometry — stamp, don't re-embed); mean < **0.90** → `INCOMPATIBLE` (re-embed); between → `DRIFT` (re-embed + flag). The 0.995 bar exists because pgvector stores `float4` while the app writes Python floats: an unchanged model reproduces a vector to float4 round-trip precision (~0.9999), never to exactly 1.0. This is the tool that answers the owner's actual question instead of guessing, and it doubles as the post-migration smoke test (it must read ≈ 1.0 afterwards).
|
||||
|
||||
**D4 — Three modes, one script (task 04), mirroring `scripts/regen_folder_summaries.py`'s entrypoint idiom** (`get_settings()` + `configure_logging` + `configure_debugging()` + `ensure_admin_configured`, one event loop, one `LLMClient`, `DEBUGPY` honored and never imported by default — AGENTS rule 7):
|
||||
- `--compare` (default when no mode flag): read-only, prints the verdict, exits 0 on COMPATIBLE and non-zero otherwise so the runbook can branch on it. **It never writes.** Because it is its own process it can be run on prod with `BOR_LLM_EMBED_MODEL=<new> uv run python -m scripts.reembed_embeddings --compare` while the app keeps serving the OLD model — the measurement does not touch the running app.
|
||||
- `--stamp-only`: set `embedding_model` to the configured model on every chunk that has a vector, touching no vector. Valid **only** on a COMPATIBLE verdict — the runbook's step for "the endpoint changed name but not geometry".
|
||||
- `--apply`: re-embed in place keyed on `chunk.id`, selecting `WHERE embedding_model IS DISTINCT FROM :configured`, committing per batch, resumable by construction (re-running finishes what is left, so a mid-run outage is never a dead end). Batching is delegated to `LLMClient.embed()` — no second batching implementation. Per-batch progress plus one PLAN §9 summary line: `reembed: model=<stamp> mode=… chunks=… batches=… remaining=… duration_ms=…`.
|
||||
- Both writers **fail loud**: an `EmbeddingError` on a batch logs, stops, exits non-zero and prints the resume command. A chunk is NEVER skipped into `embedding = NULL` — a NULL embedding silently disappears from `_vector_list` (`app/rag/retriever.py:433-455`), which is the exact silent-degradation class this phase kills.
|
||||
- Both writers **never** touch `content`, `position`, `tsv`, `documents.*`, or `sources_meta.version` (the phase-118 convention, `app/rag/importer.py:51-57`: content-unchanged work is not a KB change, so saved chats must not go stale because the vectors were refreshed).
|
||||
|
||||
**D5 — A mismatch is loud in three places (tasks 03, 05, 06), and never silent:**
|
||||
- `app/core/embedding_generation.py::generation_report(session, expected_model)` → a pure, unit-tested counts object: `total`, `with_vector`, `null_vectors`, `by_model` (with a `None`/unknown bucket), `mismatched`, `unknown`, `stale`.
|
||||
- **After every sync** (`app/api/sync.py`, right after the `sync: done` line at `:844`): one INFO line `embeddings model=<configured> chunks=N mismatched=M unknown=K`. A **WARNING** only when `mismatched > 0`; `unknown > 0` alone stays INFO, because every pre-phase KB is 100 % unknown and would scream on every sync forever.
|
||||
- **Admin endpoint** `GET /api/admin/embeddings` (router-wide `Depends(require_admin)`, the `tokens`/`ui_settings` pattern) returning that report as the JSON the card and the runbook both read. Payload is a handful of numbers — it must stay under the phase-167 byte budget and appears in the `grep ' :: api '` audit.
|
||||
- **Settings view card** (phase 168's `#view-settings`, task 06): a read-only "Embedding generation" card above the settings table — `role="status"` when clean, `role="alert"` naming the offending stamp, the chunk counts, and the remediation command (`uv run python -m scripts.reembed_embeddings --apply`). Text states the state; never color alone (B5).
|
||||
|
||||
**D6 — Retrieval stays unfiltered, deliberately (LOCKED A3).** No `WHERE embedding_model = :model` in `_vector_list`: during a backfill it would silently return nothing and deflect every turn (a worse failure than the one being fixed), it costs a predicate on the hot path of every turn, and the migration window is minutes. The column is for DETECTION; the script is for REPAIR. This rejection is recorded in the code docstring so the next reader does not "fix" it.
|
||||
|
||||
**D7 — Threshold re-tuning is operator work, backed by tooling (task 07 docs).** The phase ships the measurement + the runbook; it does NOT change `relevance_threshold` / `lexical_support_floor` / `source_usefulness_floor` defaults, because the right numbers depend on the model the owner actually points at. The runbook makes the order explicit: re-embed FIRST, then re-tune (retuning against a mixed corpus tunes to noise), then verify with `tests/fixtures/retrieval_battery.txt` and the deflection rate. It also names the phase-168 trap: those keys may now be `app_settings` rows, and **a stored override beats `.env`** (`app/core/settings_overlay.py`) — so `select key, value from app_settings where key in ('relevance_threshold','lexical_support_floor','source_usefulness_floor','llm_embed_model');` is step one of any value change.
|
||||
|
||||
## Dependencies
|
||||
|
||||
- `168_admin_settings_panel` (**todo — must complete first**, this phase runs after it in the pipeline) — the overlay that makes `app_settings` beat `.env` (D7's trap), the catalog-completeness pin, and the `#view-settings` view + `/api/admin/settings` + `tests/e2e/test_admin_settings_panel.py` that this phase's card and E2E extend.
|
||||
- `97_kb_tree_catalog` + `156_table_ux_polish` (complete) — the admin catalogue/bounded-panel and `.cell-clip` contracts the card must not violate (AGENTS rule 5).
|
||||
- `167_api_payload_diet` (complete) — the 256 KB response budget + `scripts.api_payload_audit` the new endpoint is audited against.
|
||||
- `82_security_headers` + `78_static_background` + `54_asset_cache_bust_revalidation` (complete) — the CSP/`default-src 'self'` and locally-served-assets posture the card's JS lives inside (AGENTS rule 6, the No CDN Rule), and the `?v=` busting the E2E cache contract keeps.
|
||||
- `119_name_signal_read_chips` + `162_search_recall_small_sources` (complete) — own `tests/fixtures/retrieval_battery.txt`, which is this phase's measuring stick; the phase READS it and never edits it (the gated items are the retuning ruler).
|
||||
- `118_summary_seed_context` (complete) — the "content-unchanged work is not a KB change" convention (`app/rag/importer.py:51-57`) that D4 inherits for `sources_meta`.
|
||||
- `122_image_documents` + `137_pdf_image_context` (complete) — image docs store the vision description as `chunks.content`, which is what makes K3 (a re-embed never re-calls vision) true.
|
||||
|
||||
## Tasks
|
||||
|
||||
1. `01_embedding_model_column.md` — `Chunk.embedding_model` + reversible migration `0029_chunks_embedding_model.py` + its up/down test.
|
||||
2. `02_stamp_write_paths.md` — `stamp_embedding()` + the three vector-write call sites routed through it + the no-bypass source pin.
|
||||
3. `03_generation_report_and_logs.md` — `generation_report()` + the post-sync INFO/WARNING lines + the greppable log contract.
|
||||
4. `04_reembed_script.md` — `scripts/reembed_embeddings.py` with `--compare` / `--stamp-only` / `--apply`, the pure cosine helpers, resumability, the §9 summary line, unit + integration coverage.
|
||||
5. `05_admin_embeddings_endpoint.md` — `GET /api/admin/embeddings` (admin-only), the schema, the auth matrix, the byte-budget pin.
|
||||
6. `06_settings_status_card.md` — the read-only "Embedding generation" card in `#view-settings` (`role=status`/`role=alert`, text-first).
|
||||
7. `07_e2e_generation_and_docs.md` — `tests/e2e/test_embedding_generation.py`, the README runbook + AGENTS quick-reference updates, the gates, the atomic commit.
|
||||
|
||||
## Testing & Quality
|
||||
|
||||
- **Unit:** `tests/unit/test_embedding_generation.py` (`stamp_embedding` sets both fields; `cosine` on orthogonal / identical / zero-norm input; `generation_report` buckets — clean, mismatched, NULL-vector rows, unknown stamps, empty table), `tests/unit/test_reembed_embeddings.py` (the `scripts.reembed_embeddings` import idiom of `tests/unit/test_regen_folder_summaries.py:34`: mode parsing, COMPATIBLE/DRIFT/INCOMPATIBLE band edges at exactly 0.995 and 0.90, the deterministic `ORDER BY id` selection, `--apply` failing loud without writing a NULL, `--stamp-only` not touching `embedding`, no `sources_meta` bump), `tests/unit/test_embedding_write_paths.py` (the no-bypass source pin: no `.embedding =` / `embedding=` vector write in `app/` outside `stamp_embedding`), `tests/unit/test_settings_frontend_view.py` extension or a sibling (the card exists, no CDN, DOM built with `createElement`/`textContent`).
|
||||
- **Integration:** `tests/integration/test_migration_0029.py` (up AND down, A13), `tests/integration/test_admin_embeddings_api.py` (auth matrix anonymous / token-user / admin; counts correct against a seeded corpus with mixed stamps and one NULL-embedding chunk; the response body carries no API key), `tests/integration/test_sync_embedding_log.py` (the post-sync INFO line and the WARNING only on `mismatched > 0`), and an importer pin that a synced doc's content chunk AND its `is_summary` chunk both carry the configured stamp.
|
||||
- **Coverage:** **>90%** on `app/` — `uv run pytest --cov=app --cov-report=term-missing`.
|
||||
- **E2E (AGENTS rule 4):** `uv run pytest tests/e2e/test_embedding_generation.py -v --no-cov` green **in isolation**; regression set green in isolation: `test_admin_settings_panel.py`, `test_admin_auth.py`, `test_configurable_brand.py`, `test_global_tuning.py`, `test_llm_retry.py`, `test_sync_button.py`, `test_cache_busting.py`.
|
||||
- **Lint/types:** `uv run ruff check . && uv run pyright`.
|
||||
- **Log contract (AGENTS rule 10):** the two new greppable lines — `embeddings model=… chunks=… mismatched=… unknown=…` (post-sync) and `reembed: model=… mode=… chunks=… batches=… remaining=… duration_ms=…` — plus `GET /api/admin/embeddings` present in the byte audit and under the bar (`uv run python -m scripts.api_payload_audit`).
|
||||
|
||||
## Documentation & Consistency
|
||||
|
||||
- The final pass reviews `README.md` and `AGENTS.md` in full against the project as it will stand after this phase — fixing every inconsistency found, **including pre-existing ones not caused by this phase**.
|
||||
- Task 07 rewrites `README.md`'s "Embedding dimension mismatch" troubleshooting entry (`:1408-1410`) — it currently says the fix is "drop + recreate the chunks table", which is right for a DIM change and dangerously incomplete for a same-dim swap — and adds a "Switching the embedding model" runbook section; `AGENTS.md`'s quick reference gains the compare/apply commands.
|
||||
|
||||
## Completion Criteria
|
||||
|
||||
- [ ] `chunks.embedding_model` exists (migration `0029`, reversible), and EVERY vector write in `app/` stamps it — proven by the no-bypass pin and by an importer integration pin covering both a content chunk and the `position = -1` summary chunk.
|
||||
- [ ] `uv run python -m scripts.reembed_embeddings --compare` prints a mean/median/p10/min cosine agreement and a `COMPATIBLE` / `DRIFT` / `INCOMPATIBLE` verdict, writes nothing, and exits non-zero unless COMPATIBLE — proven against a mock endpoint on a seeded corpus (identical vectors → COMPATIBLE; different vectors → INCOMPATIBLE).
|
||||
- [ ] `--apply` re-embeds only the chunks whose stamp differs, commits per batch, is resumable (a second run reports `remaining=0`), never leaves an embedding NULL, and does not bump `sources_meta.version` or touch `documents`/`tsv`.
|
||||
- [ ] A mixed-generation KB is LOUD: `GET /api/admin/embeddings` reports the counts, `#view-settings` shows a `role="alert"` card naming the stamp and the repair command, and the post-sync log WARNs. An all-`unknown` (pre-phase) KB WARNs nothing and shows a `role="status"` card.
|
||||
- [ ] Retrieval is UNCHANGED by the new column — a mixed-stamp corpus still returns its hits (the D6 pin: detection, not filtering).
|
||||
- [ ] `README.md` has the runbook (measure → decide → back up → migrate → re-embed → verify → retune → smoke) including the phase-168 `app_settings`-beats-`.env` warning; `AGENTS.md`'s quick reference lists both script modes.
|
||||
- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean, the phase E2E + the regression set green in isolation.
|
||||
- [ ] README.md and AGENTS.md are accurate and consistent with the whole project (verified in the final pass).
|
||||
- [ ] no behavior change in completed phases (retrieval order, the A8 gate, sync counts, saved-chat staleness all untouched).
|
||||
- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moves to `.agents/phases/complete/`.
|
||||
|
||||
## Locked decisions (owner-confirmed 2026-10-09 during phase authoring)
|
||||
|
||||
- **A1 — one nullable `chunks.embedding_model` text column**, stamped with the `llm_embed_model` in effect at write time; `NULL` means *unknown* and the migration NEVER backfills a guess.
|
||||
- **A2 — every vector write goes through one helper** (`stamp_embedding`) so no path can desynchronize the pair; a source-scan test enforces it.
|
||||
- **A3 — detection only.** Retrieval never filters on the stamp — a mixed corpus still ranks. (Rejected: a `WHERE embedding_model = …` predicate in `_vector_list`, which silently deflects every turn during a backfill and taxes the hot path; the reason is in the code docstring.)
|
||||
- **A4 — comparability is measured in cosine agreement**, bands `COMPATIBLE ≥ 0.995 > DRIFT ≥ 0.90 > INCOMPATIBLE`, with the float4-round-trip rationale for the 0.995 bar; `--stamp-only` is legal only on COMPATIBLE.
|
||||
- **A5 — per-batch commits + a resumable `WHERE embedding_model IS DISTINCT FROM :configured` selection**, fail-loud on any batch error, never a NULL embedding, and NEVER a `sources_meta.version` bump (a re-embed is not a KB change — phase 118's convention).
|
||||
- **A6 — NO new `Settings` field.** Every knob is a CLI flag, so `tests/unit/test_settings_catalog.py`'s `set(CATALOG) == set(Settings.model_fields)` pin is untouched and the phase-168 panel stays display-only for this feature.
|
||||
- **A7 — one story, one phase, one Playwright file** (AGENTS rule 4): the backend is covered by unit + integration, the visible contract by `tests/e2e/test_embedding_generation.py`.
|
||||
- **A8 — the three A8/A13 tuning defaults are NOT changed by this phase** — they are operator-side, post-re-embed, battery-driven (D7). If the owner's post-migration battery says a default is now wrong, that is a separate, deliberate change.
|
||||
|
||||
## Assumptions (the request left these open; the executor does not guess)
|
||||
|
||||
- **K1:** the stamp is the model NAME. An operator who re-points the same alias server-side (aipi keeps serving `embed` but now runs gemma-2) is caught by `--compare`, not by the column — the column cannot see a name that did not change, and faking it with a vector fingerprint per row costs an endpoint call per write. This boundary is stated in the column docstring and the README runbook.
|
||||
- **K2:** the corpus is ≤ ~10⁴ chunks (live and replica: 8,866), so one quiesced in-process pass is minutes; no parallelism, no job queue, no progress table (PLAN A12: in-process state, no Valkey).
|
||||
- **K3:** every embedded text is already in `chunks.content` — image docs store the VISION DESCRIPTION as their content — so a re-embed never re-calls the vision model and never re-reads a PDF.
|
||||
- **K4:** `--compare`'s sample is `ORDER BY id LIMIT :n` (deterministic across runs; no RNG, no `RANDOM()`), and N defaults to 25 with `--sample` for more.
|
||||
- **K5:** the card is read-only and admin-only (no controls, no "fix it now" button — a re-embed is a maintenance-window operation, not a click).
|
||||
- **K6:** no new fixtures and no dump files are committed (`data/` stays gitignored); tests seed small corpora in the house way.
|
||||
- **K7:** quiescing = "do not run a sync while `--apply` runs" (single app process). The script does not take a lock; the README runbook states the order.
|
||||
- **K8:** `embedding_dim` stays 768 and stays non-overridable (phase 168 A3); a real DIM change remains the drop-and-recreate path in the README — this phase is for the same-dim swap that path does not cover.
|
||||
|
||||
## Commit
|
||||
|
||||
```bash
|
||||
git add app/ alembic/ scripts/ frontend/ tests/ README.md AGENTS.md .agents/phases/ && \
|
||||
git commit --no-gpg-sign \
|
||||
-m "feat(rag): stamp the embedding generation on every chunk and ship the in-place re-embed tooling" \
|
||||
-m "A same-dimension embedding-model swap was invisible: nothing recorded which model made a vector, EmbeddingDimensionError cannot fire at 768 -> 768, and a re-sync re-embeds nothing because the file_hash fast path returns with zero embeds. chunks.embedding_model (migration 0029, nullable, never backfilled) now records it, scripts/reembed_embeddings.py measures comparability in cosine agreement (--compare) and repairs in place (--stamp-only / --apply, per-batch commits, resumable, never a NULL embedding, never a sources_meta bump), and a mixed generation is loud in GET /api/admin/embeddings, the Settings card and the post-sync log. Retrieval is deliberately unfiltered: detection, not repair."
|
||||
```
|
||||
@@ -1,33 +0,0 @@
|
||||
# Task 01 — `chunks.embedding_model`: the nullable generation stamp (phase 169 D1, LOCKED A1)
|
||||
|
||||
**Phase:** `169_embedding_generation_migration` · **Source:** owner request 2026-10-09 — an embedding gemma → embedding gemma 2 upgrade keeps 768 dims, so nothing detects the swap
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
|
||||
`chunks` gains one nullable `embedding_model` text column recording which embedding model produced each vector, with a tested downgrade — the schema fact every later task (stamping, the report, the script, the card) reads.
|
||||
|
||||
## Work
|
||||
|
||||
1. `app/models.py` — in `class Chunk` (L232), directly after `embedding: Mapped[list[float] | None] = mapped_column(Vector(EMBEDDING_DIM))` (L241), add:
|
||||
- `embedding_model: Mapped[str | None] = mapped_column(Text, nullable=True)`
|
||||
- a comment in the file's established voice stating: the value is the `llm_embed_model` in effect when the vector was written; **`NULL` means UNKNOWN (a pre-phase-169 row), never "no model"**; the dimension cannot distinguish generations because `chunks.embedding` is fixed at 768 by A6 (`EMBEDDING_DIM`, L106), so this column is the ONLY thing that distinguishes two geometries of the same width; and a name change is what is recorded — a server-side re-point that KEEPS the name is detected by `scripts/reembed_embeddings.py --compare`, not here (K1).
|
||||
2. New `alembic/versions/0029_chunks_embedding_model.py` — `revision = "0029"`, `down_revision = "0028"` (verify with `uv run alembic heads` first: `0028_app_settings` is head).
|
||||
- `upgrade()`: `op.add_column("chunks", sa.Column("embedding_model", sa.Text(), nullable=True))`
|
||||
- `downgrade()`: `op.drop_column("chunks", "embedding_model")` (A13 — every migration ships a tested downgrade).
|
||||
- NO index, NO `UPDATE`: an index on a low-cardinality column costs every chunk write for an audit `GROUP BY` over ≤ 10⁴ rows (D1), and a backfill would stamp pre-phase rows with a model name nobody verified (A1 — the migration must never guess).
|
||||
- Module docstring: the 2026-10-09 embedding-gemma-2 question, why 768 → 768 is invisible to `EmbeddingDimensionError` (`app/rag/llm.py:314-322`), why a re-sync fixes nothing (the `file_hash` fast path, `app/rag/importer.py:70-84`), and that `NULL` = unknown by design.
|
||||
3. New `tests/integration/test_migration_0029.py` — copy the structure of `tests/integration/test_migration_0028.py` (the up/down harness, the temp-database idiom it uses): upgrade `head` puts `embedding_model` on `chunks` (assert through `sa.inspect(...).get_columns("chunks")`), the column is nullable and text-typed, a chunk row can be inserted with it `NULL` and with a value, and downgrade removes it cleanly with the surviving rows intact.
|
||||
4. `README.md` — in the schema section (`grep -n "chunks" README.md` to locate the table listing), add the new column to the `chunks` description with the `NULL` = unknown note; leave the troubleshooting rewrite for task 07.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit/integration: `tests/integration/test_migration_0029.py` (up, down, nullable both ways, rows survive the downgrade). The rest of the model surface is unchanged, so the existing model/migration suites must stay green untouched.
|
||||
- Coverage: **>90%** on this task's new/modified code (the migration module is exercised by the up/down test; `app/models.py` is already covered).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run alembic upgrade head` reports `0029` and `\d chunks` (via `podman exec brain-of-reese_db_1 psql -U reese -d brain_of_reese -c '\d chunks'`) shows `embedding_model | text`.
|
||||
- [ ] `uv run alembic downgrade -1` then `upgrade head` both succeed (A13), pinned by `tests/integration/test_migration_0029.py`.
|
||||
- [ ] No `UPDATE`, no `CREATE INDEX` in the migration; no existing chunk row gains a non-NULL stamp.
|
||||
- [ ] `uv run pytest` green; ruff + pyright clean.
|
||||
- [ ] README.md/AGENTS.md updated for anything user-visible this task changes (the phase's final pass re-checks both project-wide).
|
||||
- [ ] no behavior change in completed work (retrieval, sync counts, the A8 gate untouched).
|
||||
@@ -1,32 +0,0 @@
|
||||
# Task 02 — Stamp every vector write: `stamp_embedding()` + the three call sites (phase 169 D2, LOCKED A2)
|
||||
|
||||
**Phase:** `169_embedding_generation_migration` · **Story:** n/a
|
||||
|
||||
## Objective
|
||||
|
||||
Every code path that writes `chunks.embedding` also writes `chunks.embedding_model` from the same settings read, so the pair can never desynchronize — enforced by a source-scan test so a future fourth path fails the build rather than shipping a silent lie.
|
||||
|
||||
## Work
|
||||
|
||||
1. New `app/core/embedding_generation.py` — the phase's home for the generation logic (pure, no HTTP, no `app.db` import at module level; task 03 extends this same module):
|
||||
- `def stamp_embedding(chunk: Chunk, vector: list[float], model: str | None) -> None` — sets `chunk.embedding = list(vector)` and `chunk.embedding_model = model`. Nothing else: no flush, no commit, no validation beyond what SQLAlchemy does (the caller owns the transaction, exactly like the current code).
|
||||
- `def current_embedding_model(settings: Settings | None = None) -> str` — returns `settings.llm_embed_model` (defaults to `get_settings()`), the ONE place the stamp value is derived, so a later move to a fingerprinted stamp changes one function (K1).
|
||||
- Module docstring: why the pair exists (A6 fixes the column at 768 so the dimension cannot distinguish two geometries), the "write the pair together" rule, and the K1 boundary (a name that did not change is invisible here — `scripts/reembed_embeddings.py --compare` is the tool for that).
|
||||
2. `app/rag/importer.py` — the content-chunk write (L1008-1023): inside `for row, vec in zip(doc.chunks, vectors, strict=True):` replace `row.embedding = vec` with `stamp_embedding(row, vec, current_embedding_model(settings))`. Read `settings` the way the surrounding function already does (it takes/uses `settings` — do NOT add a second `get_settings()` call inside the loop).
|
||||
3. `app/rag/importer.py` — the summary-chunk write in `_store_summary` (L1587-1589): replace `chunk.embedding = vector` with the helper. This is the `is_summary` `position = -1` row — the one an operator would otherwise discover months later still carries the old geometry.
|
||||
4. `app/api/docs.py` — the manual-summary edit (L488-491): replace `summary_chunk.embedding = vector` with the helper (the file already constructs `LLMClient()` there; the stamp still comes from `current_embedding_model()`, NOT from the client).
|
||||
5. Greppable invariant comment at each of the three sites (`# phase 169: the vector and its generation are written together (LOCKED A2)`) — short, one line each.
|
||||
6. New `tests/unit/test_embedding_generation.py` — `stamp_embedding` sets BOTH attributes; a `Chunk` built through it carries the model; calling it twice overwrites both; `current_embedding_model()` reads `llm_embed_model` from a passed `Settings` and from `get_settings()`.
|
||||
7. New `tests/unit/test_embedding_write_paths.py` — the no-bypass pin, in the house static-source-scan style (`grep` the `app/` tree from the test, like the existing frontend/CDN pins do): every assignment matching `\.embedding\s*=` or a `Chunk(...)` construction passing `embedding=` inside `app/` lives in `app/core/embedding_generation.py`. Fail with an assertion message naming the offending file:line and the rule ("write vectors through `stamp_embedding` so the generation is stamped with them"). Also pin the reverse: the three call sites still exist (so the pin cannot be satisfied by deleting the calls).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: as above.
|
||||
- Integration (extend the existing importer/sync suites — locate them with `grep -rln "is_summary" tests/integration`): a synced markdown doc's content chunks AND its `is_summary` chunk all carry `embedding_model == "embed"` (the E2E/fixture model name); a doc edited through the summary API (`app/api/docs.py`) re-stamps its summary chunk; a doc RE-imported with changed bytes re-stamps every content chunk it re-embeds. Run the touched suite, then the whole suite.
|
||||
- Coverage: **>90%** on `app/core/embedding_generation.py` and the modified lines.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `grep -rn "\.embedding\s*=" app/ --include=*.py | grep -v __pycache__` shows assignments ONLY inside `app/core/embedding_generation.py`.
|
||||
- [ ] A sync against the mock endpoint leaves `select distinct embedding_model from chunks` = one value, the configured `BOR_LLM_EMBED_MODEL`, across content and summary chunks (integration-pinned).
|
||||
- [ ] `uv run pytest tests/unit/test_embedding_write_paths.py -v` passes, and adding a bypassing `row.embedding = [...]` anywhere in `app/` makes it fail (verified once by hand, then reverted).
|
||||
- [ ] `uv run pytest` green; coverage on the new module >90%; ruff + pyright clean.
|
||||
- [ ] no behavior change in completed work — chunk counts, sync summary numbers, `sources_meta` bumps, retrieval order and the A8 gate are all byte-identical to pre-task.
|
||||
-33
@@ -1,33 +0,0 @@
|
||||
# Task 03 — `generation_report()` + the post-sync loud lines (phase 169 D5, LOCKED A3)
|
||||
|
||||
**Phase:** `169_embedding_generation_migration` · **Story:** n/a
|
||||
|
||||
## Objective
|
||||
|
||||
One pure, unit-tested function turns `chunks.embedding_model` into counts-per-generation plus a `stale` verdict, and the sync run reports it on every success — INFO always, WARNING only when chunks actually carry a different generation than the model the app embeds questions with.
|
||||
|
||||
## Work
|
||||
|
||||
1. `app/core/embedding_generation.py` (from task 02) — add:
|
||||
- `@dataclass(frozen=True) class GenerationReport` with `expected_model: str`, `embedding_dim: int`, `total_chunks: int`, `with_vector: int`, `null_vector: int`, `by_model: tuple[ModelCount, ...]`, `mismatched: int`, `unknown: int`, `stale: bool`. `ModelCount` is a small frozen dataclass `(model: str | None, chunks: int, embedded: int)` — `model is None` is the UNKNOWN bucket.
|
||||
- `def generation_report(db: Session, expected_model: str | None = None, embedding_dim: int | None = None) -> GenerationReport` — ONE aggregate query (`select(Chunk.embedding_model, func.count(), func.count(Chunk.embedding)).group_by(Chunk.embedding_model)`), then pure Python bucketing: `mismatched` = chunks whose stamp is non-NULL and `!= expected_model`; `unknown` = chunks whose stamp IS NULL; `stale = mismatched > 0`. NULL-vector rows stay inside their model's `chunks` count and show up in `embedded < chunks` — a chunk that lost its vector is a separate defect and must remain visible, not folded into the generation story.
|
||||
- **D6 in writing:** the docstring states that nothing here filters retrieval (`app/rag/retriever.py:433-455` keeps its unfiltered cosine scan) and why — a `WHERE embedding_model = …` predicate would silently return nothing mid-backfill and deflect every turn, which is worse than the mixed ranking it prevents, and taxes the hot path of every turn for a migration window measured in minutes (LOCKED A3, rejected alternative).
|
||||
2. `app/api/sync.py` — immediately after `logger.info("sync: done detail=%s", _status.detail)` (L844, the SUCCESS terminal only — the cancelled and failed paths log nothing new):
|
||||
- open a short-lived `SessionLocal()` (the file's own convention, L538/L805), call `generation_report(db, settings.llm_embed_model, settings.embedding_dim)`, close it;
|
||||
- always one INFO line, greppable and cron-safe (PLAN §9): `embeddings model=<expected> chunks=<total> mismatched=<n> unknown=<n> null_vector=<n>`;
|
||||
- escalate to **WARNING** only when `mismatched > 0`, appending the repair hint: `— chunks were embedded by another model, run: uv run python -m scripts.reembed_embeddings --apply`. `unknown > 0` alone NEVER warns: a pre-phase-169 KB is 100 % unknown, and a permanent warning is noise that trains the owner to ignore the one signal that matters (D5).
|
||||
- the whole block is wrapped in `except Exception` (broad, with a `# noqa: BLE001` and a comment, matching this file's background-task style): a reporting hiccup logs `embeddings report unavailable: <type>` at WARNING and **never** changes the sync's terminal state, counts, or `_status.detail`.
|
||||
3. New `tests/unit/test_embedding_generation_report.py` — clean corpus (all stamps equal → `stale=False`, `mismatched=0`), a mixed corpus (two named stamps → the counts split, `stale=True`), the UNKNOWN bucket (NULL stamps counted, `unknown=N`, NOT counted as mismatched), NULL-vector rows (`with_vector`/`null_vector` correct, `embedded < chunks`), an empty table (all zeros, `stale=False`), and the `expected_model`/`embedding_dim` defaults resolving from `get_settings()`.
|
||||
4. New `tests/integration/test_sync_embedding_log.py` — seed a tiny corpus, run the sync the way the existing sync integration suites do (`grep -rln "sync: done\|/api/sync" tests/integration` to find the harness and copy it), and assert with `caplog`: the INFO line on success with the right numbers; the WARNING + repair hint appears after `UPDATE chunks SET embedding_model='older-model'` on a subset; NO warning when the only oddity is `embedding_model IS NULL`; no new line on a cancelled run; a broken report (monkeypatch `generation_report` to raise) leaves the sync terminal `done` and its counts untouched.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit + integration as above.
|
||||
- Coverage: **>90%** on `app/core/embedding_generation.py` (every bucket branch) and the new sync block (success / warning / unknown-only / raising paths).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `generation_report()` returns correct counts for the five pinned corpus shapes, and `stale` is `True` ONLY for a non-NULL stamp mismatch.
|
||||
- [ ] A successful sync logs exactly one `embeddings model=… chunks=… mismatched=… unknown=… null_vector=…` line: `grep`-able on stdout in dev (`uv run uvicorn app.main:app --reload` + a sync) and pinned via `caplog`.
|
||||
- [ ] The WARNING fires on a mismatch and does NOT fire on an all-unknown corpus; the repair hint names `scripts.reembed_embeddings --apply`.
|
||||
- [ ] A raising `generation_report` cannot change a sync's terminal state or counts.
|
||||
- [ ] `uv run pytest` green (the existing sync suites untouched), coverage >90% on the new code, ruff + pyright clean.
|
||||
- [ ] no behavior change in completed work (the sync's `detail` payload, `sources_meta` bumping and the saved-chat staleness gate are byte-identical).
|
||||
@@ -1,42 +0,0 @@
|
||||
# Task 04 — `scripts/reembed_embeddings.py`: measure comparability, then repair in place (phase 169 D3/D4, LOCKED A4/A5)
|
||||
|
||||
**Phase:** `169_embedding_generation_migration` · **Story:** n/a
|
||||
|
||||
## Objective
|
||||
|
||||
One script answers the owner's real question — *"are the stored vectors still usable against this endpoint?"* — by measuring cosine agreement (`--compare`), and repairs the corpus when they are not (`--apply`), with `--stamp-only` for the "endpoint changed name, not geometry" case. This is the tool a same-dimension embedding-model swap has never had.
|
||||
|
||||
## Work
|
||||
|
||||
1. `app/core/embedding_generation.py` — the pure math + selection, so coverage and unit tests stay clean and the script stays thin glue:
|
||||
- `cosine(a: Sequence[float], b: Sequence[float]) -> float` — pure stdlib (`math.fsum`), `0.0` for a zero-norm operand (never `NaN`/`ZeroDivisionError`), `ValueError` naming both lengths when `len(a) != len(b)` (a length difference is a bug, not a score).
|
||||
- Band constants, documented where they are declared: `COMPATIBLE_MEAN = 0.995` and `INCOMPATIBLE_MEAN = 0.90`, plus `VERDICT_COMPATIBLE / VERDICT_DRIFT / VERDICT_INCOMPATIBLE`. The 0.995 rationale goes in the docstring **verbatim**: pgvector stores `float4` while the app writes Python floats, so an unchanged model reproduces a vector to float4 round-trip precision (~0.9999), never to exactly 1.0 — anything meaningfully below that is a different geometry, and "expect exactly 1.0" would make every healthy corpus read as DRIFT.
|
||||
- `@dataclass(frozen=True) class AgreementStats` — `n`, `mean`, `median`, `p10`, `min`, `worst: tuple[tuple[int, float], ...]` (the ≤ 3 lowest-agreement chunk ids), `verdict: str`; `agreement(samples: Iterable[tuple[int, Sequence[float], Sequence[float]]]) -> AgreementStats` computes it (`(chunk_id, stored, fresh)` triples; empty input → `n=0`, `verdict=VERDICT_INCOMPATIBLE` — "nothing measured" is never "compatible").
|
||||
- Selection helpers (all `ORDER BY Chunk.id`, deterministic, no RNG — K4): `sample_chunks(db, limit)` → `id, content, embedding` rows `WHERE embedding IS NOT NULL`; `pending_chunks(db, model, limit=None)` → `id, content` rows `WHERE embedding IS NULL OR embedding_model IS DISTINCT FROM :model`; `count_pending(db, model) -> int`; `count_missing_vector(db) -> int`.
|
||||
- `stamp_rows(db, model, ids) -> int` — the `--stamp-only` UPDATE, **restricted to rows that actually have a vector** (`embedding IS NOT NULL`) so a vector-less row is never stamped as if it did.
|
||||
2. New `scripts/reembed_embeddings.py` — the entrypoint mirrors `scripts/regen_folder_summaries.py` EXACTLY (the house one-shot idiom): `get_settings()` → `configure_logging(settings.log_level)` → `configure_debugging()` (debugpy only under `DEBUGPY=1`, never imported otherwise — AGENTS rule 7) → `ensure_admin_configured(settings)`, a boot failure prints `reembed_embeddings: boot failed: <e>` to stderr and returns 1; one `LLMClient()`; one `asyncio.run(...)`; a short-lived `SessionLocal()` per unit of work (closed in `finally`); one cron-safe PLAN §9 summary line.
|
||||
- **Modes** (`argparse mutually exclusive group`, default `--compare`): `--compare` | `--stamp-only` | `--apply`; plus `--sample N` (default 25, compare only), `--limit N` (apply/stamp-only: cap the rows this run touches — the debug/smoke knob), `--dry-run` (apply/stamp-only: report `count_pending` and write nothing).
|
||||
- **Exit codes as module constants**, documented in the parser epilog: `EXIT_OK = 0`, `EXIT_FAILED = 1` (boot/endpoint/DB failure), `EXIT_INCOMPATIBLE = 3` (the measurement answered "no") — so the runbook branches on "the answer is no" without confusing it with "the tool broke".
|
||||
- **First log line of every run**: `reembed: starting mode=<mode> model=<llm_embed_model> dim=<embedding_dim> base=<sanitized>` with the base URL through `app.core.errors.sanitize_error` (the phase-84 SEC-13 rule: a `user:pass@` in `BOR_LLM_BASE_URL` must never reach a log). This line is also the operator's proof the run used the env the app will run with — the rule that keeps the stamp honest (K1).
|
||||
- `--compare`: `sample_chunks` → `await llm.embed([content …])` → `agreement(...)` per pair → print a human table (per-chunk id + cosine, and the aggregate), then `reembed: compare model=… n=… mean=… median=… p10=… min=… verdict=…` and, for a non-COMPATIBLE verdict, one more line stating the repair command (`--apply`) and the caveat that `--stamp-only` is only legal on COMPATIBLE. **It writes nothing, ever** — no session writes, no commit (pin it).
|
||||
- `--apply`: loop `pending_chunks(db, model, limit)` in id-ordered pages (default page 200 rows), per page `vectors = await llm.embed([content …])`, `zip(rows, vectors, strict=True)`, `stamp_embedding(row, vec, model)` (task 02's helper — the pair is written together), `session.commit()` per page. Progress INFO per page: `reembed: progress done=%d pending_total=%d`. **Fail loud:** any `EmbeddingError` logs `reembed: FAILED after %d rows — resume with: uv run python -m scripts.reembed_embeddings --apply` and returns `EXIT_FAILED`; no row is ever written with `embedding=None`, and no row is silently skipped (a NULL embedding vanishes from `_vector_list`, `app/rag/retriever.py:433-455` — exactly the silent class this phase kills).
|
||||
- `--stamp-only`: `stamp_rows(...)`, loud INFO line naming the count, and a WARNING that repeats what must be true for this to be correct ("only valid on a COMPATIBLE --compare verdict against the SAME endpoint").
|
||||
- **Never touched:** `content`, `position`, `tsv`, `documents.*`, `sources_meta.version` (the phase-118 convention, `app/rag/importer.py:51-57` — a re-embed is not a KB change, so saved chats must not go stale). No `chunk_document` call, no PDF read, no vision call (K3 — the embedded text already lives in `chunks.content`, image docs included).
|
||||
- Final summary line (all modes): `reembed: model=<stamp> mode=<mode> chunks=<processed|sampled> batches=<llm.embed_batches> remaining=<count_pending after> duration_ms=<…>`. `remaining=0` after a clean `--apply` is the resumability proof; a second run prints `pending=0` and does nothing.
|
||||
- No whitespace special-case for `content`: the chunker never yields empty chunks, and an endpoint rejection on a pathological row fails loud on the batch rule above rather than being quietly skipped.
|
||||
- Module docstring: the embedding-gemma → gemma-2 question this exists for, the three modes, the bands + float4 rationale, the resumability contract, the "run it with the app's own env" rule, and the K1 boundary (a same-name server-side re-point is invisible to the stamp and is exactly what `--compare` catches).
|
||||
3. New `tests/unit/test_reembed_embeddings.py` — the `from scripts import reembed_embeddings as reembed` idiom of `tests/unit/test_regen_folder_summaries.py:34`: mode mutual-exclusion and defaults, `--sample`/`--limit`/`--dry-run` parsing, the exit-code constants, the band edges (mean exactly 0.995 → COMPATIBLE, 0.9949 → not, exactly 0.90 → INCOMPATIBLE boundary handled per the declared rule, empty sample → INCOMPATIBLE), `cosine` (identical → 1.0, orthogonal → 0.0, zero-norm → 0.0 not NaN, length mismatch → `ValueError` naming both lengths), the deterministic sample/pending ordering, and that the base URL in the starting line is sanitized (an `https://user:pass@host` env yields no `user:pass` in the log).
|
||||
4. New `tests/integration/test_reembed_apply.py` — seed a small corpus (a handful of docs + chunks) with `embedding_model='old-model'`; with a stubbed `LLMClient` returning known vectors: `--dry-run` writes nothing (`count_pending` unchanged, no UPDATE issued); `--apply` stamps every row, sets the new vectors, leaves `content`/`position`/`tsv`/`documents` and `sources_meta.version` untouched, and a SECOND `--apply` reports `pending=0` / `remaining=0` (the resumability pin); a partial failure (the stub raises `EmbeddingError` on the second page) leaves page 1 committed, exits `EXIT_FAILED`, logs the resume line, and NO row ends up with `embedding IS NULL`; `--stamp-only` changes only `embedding_model` (vector bytes bit-identical — compare against a snapshot) and skips vector-less rows; `--compare` against a stub that echoes the stored vectors → COMPATIBLE / exit 0, against different vectors → INCOMPATIBLE / exit 3, and the chunks table byte-identical afterwards (the read-only pin).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit + integration as above; the existing `scripts` suites (`tests/unit/test_regen_folder_summaries.py`, `tests/unit/test_llm_probe.py`, `tests/integration/test_import_docs_*.py`) stay green untouched.
|
||||
- Coverage: **>90%** on `scripts/reembed_embeddings.py` and on `app/core/embedding_generation.py` (every mode, every band edge, every failure path).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run python -m scripts.reembed_embeddings --help` shows the three modes, `--sample`, `--limit`, `--dry-run` and the exit-code epilog.
|
||||
- [ ] Against the restored 1,000-doc replica locally (`restore-test-db` skill) the three modes do exactly what the docstring says — `--compare` prints a verdict and leaves the dump's fingerprint byte-identical; `--apply` on a `UPDATE chunks SET embedding_model='old-model'` corpus re-embeds all 8,866 chunks and reports `remaining=0`; a second run is a no-op.
|
||||
- [ ] No code path can write `embedding = NULL`; no path bumps `sources_meta.version`; `--compare` issues no UPDATE (pinned).
|
||||
- [ ] `--stamp-only` on a COMPATIBLE corpus leaves every `embedding` value bit-identical and stamps all vector rows.
|
||||
- [ ] `uv run pytest` green, coverage >90% on the script and module, ruff + pyright clean.
|
||||
- [ ] README.md/AGENTS.md updated for anything user-visible this task changes (the full runbook lands in task 07; the phase's final pass re-checks project-wide).
|
||||
- [ ] no behavior change in completed work (the app, the sync, and retrieval are untouched by this task — only the new script + helpers).
|
||||
-30
@@ -1,30 +0,0 @@
|
||||
# Task 05 — `GET /api/admin/embeddings`: the generation report as an admin endpoint (phase 169 D5)
|
||||
|
||||
**Phase:** `169_embedding_generation_migration` · **Story:** n/a
|
||||
|
||||
## Objective
|
||||
|
||||
A read-only, admin-only endpoint exposing task 03's `GenerationReport` as JSON — the single source the Settings card (task 06) and the operator runbook (task 07 + the `migrate-embed-model` skill) both read to answer "is this KB on one embedding generation?".
|
||||
|
||||
## Work
|
||||
|
||||
1. `app/schemas.py` — two response models beside the other read models (the `HealthResponse` style, no `response_model_exclude` tricks):
|
||||
- `EmbeddingModelCount(BaseModel)`: `model: str | None`, `chunks: int`, `embedded: int` — `model=None` is the UNKNOWN bucket, and `embedded < chunks` names the vector-less rows inside that stamp.
|
||||
- `EmbeddingGeneration(BaseModel)`: `configured_model: str`, `embedding_dim: int`, `total_chunks: int`, `with_vector: int`, `null_vector: int`, `by_model: list[EmbeddingModelCount]`, `mismatched: int`, `unknown: int`, `stale: bool`.
|
||||
- Docstring notes: numbers only, no text from the corpus; and NO tuning values here — `relevance_threshold` / the two floors and their `db`/`env`/`default` source badges are phase 168's `GET /api/admin/settings` job, never duplicated (two sources of truth for a tuning value is how a runbook lies).
|
||||
2. New `app/api/admin_embeddings.py` — `router = APIRouter(prefix="/admin/embeddings", tags=["embeddings"], dependencies=[Depends(require_admin)])` (the `tokens.py`/`ui_settings.py` router-wide pattern: anonymous AND token-user both 403), one route `GET ""` → `EmbeddingGeneration`, `get_db` session, `generation_report(db, settings.llm_embed_model, settings.embedding_dim)` (task 03) mapped to the schema. Module docstring: what "stale" means (a chunk whose stamped model ≠ the model the app embeds questions with, i.e. two geometries on one cosine list), that detection is not filtering (LOCKED A3), and the repair pointer to `scripts.reembed_embeddings`.
|
||||
3. `app/main.py` — register it with the other admin routers (`app.include_router(admin_embeddings_router, prefix="/api")`, next to the `ui_settings`/`tokens` includes at L146-172).
|
||||
4. New `tests/integration/test_admin_embeddings_api.py` — the auth matrix copied from the existing admin-surface suite (`grep -rln "require_admin" tests/integration`): anonymous → 403, token user → 403, admin session → 200; against seeded corpora: clean (one stamp → `stale=false`, `mismatched=0`), mixed (two stamps → counts split, `stale=true`, `mismatched` exact), unknown (NULL stamps counted in `unknown` and NOT in `mismatched`), vector-less rows (`null_vector` and the per-stamp `embedded < chunks`), empty DB (all zeros, `stale=false`); the response body contains no `api_key` / `password` / `secret` substring; the payload is tiny (< 4 KB — the phase-167 byte budget is a 256 KB bar, and a handful of counters must never approach it).
|
||||
5. Byte-audit check (AGENTS rule 10): with the app running, `uv run python -m scripts.api_payload_audit` lists `/api/admin/embeddings` under the bar, and `grep ' :: api '` shows the route's line.
|
||||
|
||||
## Testing & Quality
|
||||
- Integration as above; unit coverage comes free from task 03's report tests (the route is a mapping, so pin it here instead of duplicating the bucket tests).
|
||||
- Coverage: **>90%** on `app/api/admin_embeddings.py` and the new schemas.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `GET /api/admin/embeddings` returns 200 for the admin and 403 for anonymous and token users (all pinned).
|
||||
- [ ] The JSON matches the four seeded corpus shapes exactly, `stale` is true only for a non-NULL mismatch, and `unknown` never counts toward `mismatched`.
|
||||
- [ ] No secret substring in the body; the payload is a few hundred bytes and appears under the bar in `scripts.api_payload_audit`.
|
||||
- [ ] `uv run pytest` green, coverage >90% on the new route, ruff + pyright clean.
|
||||
- [ ] README.md updated for the new endpoint in its API listing (task 07 finishes the runbook; the final pass re-checks project-wide).
|
||||
- [ ] no behavior change in completed work.
|
||||
@@ -1,34 +0,0 @@
|
||||
# Task 06 — the "Embedding generation" card in the Settings view (phase 169 D5, AGENTS rule 5)
|
||||
|
||||
**Phase:** `169_embedding_generation_migration` · **Story:** n/a
|
||||
|
||||
## Objective
|
||||
|
||||
The admin sees, unprompted and in the browser, whether the corpus and the configured embedding model agree — one read-only card at the top of phase 168's `#view-settings` that is quiet-clean on a healthy KB and unmistakably loud when chunks were embedded by another model.
|
||||
|
||||
## Work
|
||||
|
||||
1. `frontend/index.html` — inside `#view-settings` (the phase-168 view), ABOVE the settings `.table-wrap` card, a new card in the file's existing card idiom:
|
||||
- `<section class="card" id="embedding-status" aria-labelledby="embedding-status-title">` with `<h2 id="embedding-status-title">Embedding generation</h2>`, a `<p id="embedding-status-line" role="status" aria-live="polite">` for the state line, and a `<p id="embedding-status-fix" hidden>` wrapping the repair command in a `<code>` element.
|
||||
- No new view, no nav entry, no router change — this is one card inside a route that already exists (`VIEW` / `VIEW_PATH` / `VIEW_MODULES` in `frontend/assets/router.js` are untouched), which is what keeps the phase's blast radius small.
|
||||
2. `frontend/assets/settings.js` (the phase-168 module — extend it, do not fork it) — on view activation, `fetch("/api/admin/embeddings")` once and render through ONE function:
|
||||
- **clean** (`stale=false`, `unknown=0`): the line reads `All N chunks are on "<model>" (dim D).` in `role="status"`; the fix paragraph stays `hidden`.
|
||||
- **unknown only** (`stale=false`, `unknown>0`): `role="status"` and the sentence names it as pre-phase rows, not an error — `N of M chunks predate the stamp (unknown model); K have no vector.` plus the informational `--stamp-only`-or-`--apply` hint in the `<code>` line. This never paints as an error (task 03's INFO-not-WARNING rule, mirrored in the UI).
|
||||
- **stale** (`stale=true`): the SAME line element flips to `role="alert"` and names the offending stamp, the count, the configured model, and the command: `8,612 of 8,866 chunks were embedded by "embed" — the app is set to "embed2". Run: uv run python -m scripts.reembed_embeddings --apply`. Numbers via `toLocaleString()`; the model name via `textContent` (never string-built HTML).
|
||||
- **fetch failure**: `role="alert"`, `Embedding generation status unavailable.` — never a blank card and never a stale previous value left on screen (PLAN §7.4 "never stale").
|
||||
- All DOM built with `createElement` / `textContent` (the house rule — no `innerHTML` with fetched data, no CDN, AGENTS rule 6); a `null`-model bucket label is the literal text `unknown`.
|
||||
3. `frontend/assets/styles.css` — the card needs no new layout primitives: reuse `.card`, and add `overflow-wrap: anywhere;` to the command `<code>` so the repair command wraps instead of clipping (an operator must be able to SELECT and copy it — an ellipsis would break the one action the card offers). Contrast: the stale state uses the existing warning/danger text token on its background (≥ 4.5:1, the theme tokens phase 92/93 established), and the state is carried by the WORDS and the `role`, never by color alone (B5).
|
||||
4. New `tests/unit/test_settings_embedding_card.py` — the static source pins in the house style (read the two files, assert): the section + `aria-labelledby` + `role="status"` + `aria-live="polite"` exist; the fix `<code>` exists and starts `hidden`; `settings.js` references `/api/admin/embeddings`, sets `role` to `alert` on the stale path, uses `textContent` for the model name, contains no `innerHTML` assignment with fetched data, and no `https://` src/href (the No-CDN pin).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the static pins above.
|
||||
- Integration: none needed beyond task 05's endpoint suite (the card is client-side; its behavior is E2E-pinned in task 07). If phase 168's view has an integration render pin, extend it with the card's presence for an admin and its absence for a token user.
|
||||
- Coverage: **>90%** on the changed JS surface as far as the static pins reach (the runtime paths are covered by task 07's E2E).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] An admin opening `/settings.html` sees the card above the settings table, inside the 72rem container, with the correct state for the seeded data (checked in task 07's E2E).
|
||||
- [ ] A stale corpus shows `role="alert"` naming the old stamp, the counts, the configured model, and a fully visible, copyable `scripts.reembed_embeddings --apply` command; a clean corpus shows `role="status"` and no command.
|
||||
- [ ] An all-unknown corpus is a `role="status"` card, never an alert.
|
||||
- [ ] The state is legible with color removed (words + `role` carry it), focus styles are untouched, and no `innerHTML` / CDN appears in the changed JS.
|
||||
- [ ] `uv run pytest` green, ruff + pyright clean, `test_cache_busting.py` and `test_admin_theme_tab.py` still green in isolation (the asset-busting and admin-view contracts survive).
|
||||
- [ ] no behavior change in completed work (the settings table, its Save/Reset behavior, and `/api/admin/settings` are untouched).
|
||||
@@ -1,50 +0,0 @@
|
||||
# Task 07 — E2E for the generation card, the README runbook, the gates, the commit (phase 169, AGENTS rules 4/8/9)
|
||||
|
||||
**Phase:** `169_embedding_generation_migration` · **Story:** n/a
|
||||
|
||||
## Objective
|
||||
|
||||
Prove in a real browser that a mixed embedding generation is visible and that detection does not filter retrieval, write the operator runbook the live upgrade follows, run every gate, and land the phase as one atomic commit.
|
||||
|
||||
## Work
|
||||
|
||||
1. New `tests/e2e/test_embedding_generation.py` — the phase's dedicated Playwright file (AGENTS rule 4), built on the shared fixtures (`from e2e.auth_helpers import login, login_with_token`, the `page` / `app_url` / `db_ready` fixtures, `from app.db import SessionLocal`, and the `TRUNCATE chunks, documents, query_log, kb_overview, git_sources` cleanup `tests/e2e/test_sync_button.py:177` uses — TRUNCATE before AND after). Seed the corpus the way that suite seeds it, so the mock embeddings endpoint (`tests/e2e/mock_llm.py`: deterministic 768-dim bag-of-words) stamps chunks with `BOR_LLM_EMBED_MODEL`. Tests:
|
||||
- **clean corpus** — `login(page, app_url, next="/settings.html")`, open the view, `#embedding-status-line` has `role="status"`, its text contains the configured model name and the chunk count, and `#embedding-status-fix` is hidden.
|
||||
- **mixed corpus is loud** — `UPDATE chunks SET embedding_model = 'older-model'` for a known subset, reload, assert `#embedding-status-line` has `role="alert"` and that its text names `older-model`, the mismatched count, the configured model, and the literal `scripts.reembed_embeddings --apply`; the command element is visible and its full text is present in the DOM (the AGENTS rule 5 clip contract — nothing ellipsised away).
|
||||
- **detection, never filtering (LOCKED A3)** — with the SAME mixed corpus, ask a chat question whose answer is pinned by the seeded corpus and assert the turn still grounds on the same source as it did before the stamps were changed (the retrieval list is unchanged by the column — this is the pin that stops a future task from "fixing" the mixed state by hiding chunks).
|
||||
- **all-unknown is calm** — `UPDATE chunks SET embedding_model = NULL`, assert `role="status"` (NOT `alert`) and that the text says `unknown` rather than naming an error.
|
||||
- **walled surface** — `page`-less API requests (`APIRequestContext`): anonymous and `login_with_token` users get 403 from `GET /api/admin/embeddings`, the admin gets 200 (mirror phase 168's `test_anonymous_and_token_user_are_walled`).
|
||||
- **the script repairs what the card reports** — run the real script in-process against the E2E database (`BOR_LLM_BASE_URL` points at the mock endpoint, so `--apply` is deterministic and free): set all stamps to `older-model`, `reembed.main(["--apply"])` returns 0, the DB reports one stamp again, and reloading `/settings.html` returns to the clean `role="status"` card. (If `LLMClient()` construction needs the env the subprocess-style fixture provides, drive it with the same env the app_server fixture uses — do not add a new env contract.)
|
||||
- Module docstring: the embedding-gemma → gemma-2 story, the four states, and why the A3 pin matters.
|
||||
2. `README.md` — two edits, both substantive:
|
||||
- **New section "Switching the embedding model (same dimension)"** — the operator runbook, in order: (1) baseline the battery and the cosine percentiles; (2) probe the new model (`scripts.llm_probe`, `scripts.test_embed_model`); (3) measure comparability with `uv run python -m scripts.reembed_embeddings --compare` run as its OWN process with `BOR_LLM_EMBED_MODEL=<new>` — which does not disturb the running app — and read the verdict bands (COMPATIBLE ≥ 0.995 / DRIFT / INCOMPATIBLE, with the float4-round-trip reason for the bar); (4) back up (`pg_dump -Fc`, copied off-box, verified); (5) `uv run alembic upgrade head`, then `--stamp-only` on COMPATIBLE or `--apply` otherwise; (6) verify — `--compare` now ≈ 1.0, `GET /api/admin/embeddings` `stale=false`, the Settings card clean, `remaining=0`; (7) ONLY NOW re-tune `BOR_RELEVANCE_THRESHOLD` / `BOR_LEXICAL_SUPPORT_FLOOR` / `BOR_SOURCE_USEFULNESS_FLOOR` against the battery, remembering the two validators cap the floors at the threshold and that **a value stored via the Settings panel (`app_settings`) beats `.env`** (phase 168), so check `select key, value from app_settings where key in ('llm_embed_model','relevance_threshold','lexical_support_floor','source_usefulness_floor');` before and after any change; (8) smoke: the battery's gated items, an out-of-KB question still deflects, and the next N turns' deflection rate in `query_log` looks like the pre-migration one. State the maintenance-window rule (do not run a sync while `--apply` runs) and the K1 boundary (a server-side alias re-point that keeps the model NAME is invisible to the stamp — `--compare` is the detector).
|
||||
- **Rewrite the troubleshooting entry at `:1408-1410`** — "Embedding dimension mismatch" keeps its drop-and-recreate advice (a real DIM change, A6) and gains the pointer that a SAME-dim swap is the invisible one: run `--compare`, then `--apply`; and that the Settings card / `GET /api/admin/embeddings` / the post-sync `embeddings model=… mismatched=…` line are where a mismatch announces itself.
|
||||
3. `AGENTS.md` — the quick-reference block gains the two commands (with a one-line "why" in the surrounding style):
|
||||
```
|
||||
uv run python -m scripts.reembed_embeddings --compare # are the stored vectors still usable? (cosine agreement verdict)
|
||||
uv run python -m scripts.reembed_embeddings --apply # re-embed the chunks whose generation != BOR_LLM_EMBED_MODEL (resumable)
|
||||
```
|
||||
and rule 10's log list gains the two new greppable lines (`embeddings model=… chunks=… mismatched=… unknown=…`, `reembed: model=… mode=… … remaining=… duration_ms=…`).
|
||||
4. Full gates (AGENTS rule 9, all of them, in this order):
|
||||
- `uv run pytest`
|
||||
- `uv run pytest --cov=app --cov-report=term-missing` → `app/` coverage **>90%**
|
||||
- `uv run pytest tests/e2e/test_embedding_generation.py -v --no-cov` → green **in isolation**
|
||||
- regression set, each **in isolation**: `tests/e2e/test_admin_settings_panel.py`, `test_admin_auth.py`, `test_configurable_brand.py`, `test_global_tuning.py`, `test_llm_retry.py`, `test_sync_button.py`, `test_cache_busting.py`, `test_admin_theme_tab.py`
|
||||
- `uv run ruff check . && uv run pyright`
|
||||
- `uv run python -m scripts.eval_retrieval --from-file tests/fixtures/retrieval_battery.txt` on the restored replica (locally, via the `restore-test-db` skill) — the battery's gate items keep passing with the new column present (nothing here should move them; a movement means something in this phase changed ranking, which is a regression)
|
||||
- `uv run python -m scripts.api_payload_audit` (app running) — `/api/admin/embeddings` present and under the bar
|
||||
5. The Documentation & Consistency final pass — read `README.md` and `AGENTS.md` end to end against the post-phase project, fix every inconsistency found **including pre-existing ones this phase did not cause** (e.g. a stale command, a wrong default, a section count), and record what was fixed in the commit body.
|
||||
6. Move `.agents/phases/todo/169_embedding_generation_migration/` → `.agents/phases/complete/` and commit exactly as the overview's **Commit** block specifies (`--no-gpg-sign`, Conventional Commit, plus the `LLM-Generated: true` trailer).
|
||||
|
||||
## Testing & Quality
|
||||
- This task OWNS the phase's whole-suite gate: 100 % passing, `app/` coverage >90%, the phase E2E plus the eight-file regression set green in isolation.
|
||||
- Coverage: **>90%** on `app/` (the phase's new modules included).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `tests/e2e/test_embedding_generation.py` passes in isolation and covers all six behaviors above, including the A3 "the retrieval list is unchanged by the stamp" pin.
|
||||
- [ ] The README runbook exists and is self-sufficient (a competent operator who has never seen this chat can execute a production embedding swap from it alone, including the `app_settings`-beats-`.env` warning and the retune-after-re-embed ordering); the `:1408` troubleshooting entry no longer tells anyone to drop the table for a same-dim swap.
|
||||
- [ ] `AGENTS.md`'s quick reference and rule 10 name both script modes and both log lines.
|
||||
- [ ] Every gate in step 4 is green, with the coverage number recorded in the commit body.
|
||||
- [ ] `README.md` and `AGENTS.md` are accurate and consistent with the whole project (verified in the final pass, fixes listed in the commit body).
|
||||
- [ ] no behavior change in completed phases: the battery's ranks, the A8 gate verdicts, sync counts, `sources_meta` behavior and saved-chat staleness are unchanged.
|
||||
- [ ] One atomic `--no-gpg-sign` Conventional Commit; `.agents/phases/complete/169_embedding_generation_migration/` exists and the todo copy is gone.
|
||||
Reference in New Issue
Block a user