phase: 183_sync_passive_delete_cascade
All gates green. Final state verified.
**Implemented / verified**
- Task 03's work was **never actually done** (its report was a stub, `tests/e2e/test_prune_cascade_cleanup.py` did not exist) → wrote it: 3 tests — 9-file local source synced from the Sources page (`9 added`), 6 files deleted upstream, re-sync (`0 added · 3 unchanged · 6 pruned`), documents vanished from the tree + `GET /api/documents/content` 404 + total 3, **0 orphan chunk rows** via the test's own SQL, survivor chunk total unchanged; partial-prune chat turn (grounded, non-deflected, `query_log` names the survivor and no pruned doc); total source removal (`DELETE /api/git-sources/{id}`) wipes every doc+chunk of the source with 0 orphans, sibling source untouched. Mock-only (`E2E_REAL_LLM=1` skipped), own port 8199, truncate before/after.
- Tasks 01/02 re-verified: `passive_deletes=True` + mapper pin, probe flipped to `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 0`, `_prune`/`delete_git_source` docstrings.
- **Drift guard proven non-tautological by my own one-off run**: scratch DB (`alembic upgrade head`) → FK `confdeltype='c'`, guard PASSES, CASCADE delete leaves 0 chunks; FK recreated `ON DELETE NO ACTION` → guard FAILS verbatim and a `passive_deletes` delete raises `ForeignKeyViolation` (never silent orphans). Scratch DB dropped; shared test DB untouched (`'c'`).
**Gates**
- `uv run pytest --cov=app --cov-report=term-missing` → **4488 passed**, `app/` TOTAL **99%** (>90%)
- `uv run pytest tests/integration/test_prune_cascade_statements.py -v` → 4 passed
- `uv run pytest tests/e2e/test_prune_cascade_cleanup.py -v --no-cov` → **3 passed in isolation**; `test_source_removal_cleanup.py` 6 passed, `test_force_push_resync.py` 2 passed (both isolated)
- `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors
**Completion criteria**: passive_deletes+pin ✅ · probe post-fix ✅ · drift guard green + proven-failing variant ✅ · full suite/coverage/lint ✅ · E2E in isolation ✅ · docs consistent ✅ · commit = harness-only (per pipeline rule I ran no git write commands).
**Docs**: fixed 4 pre-existing README inconsistencies — per-module E2E ports (claim of "one pair of fixed ports" predates phase 79), debugpy `launch.json` snippet vs the shipped file (`name`/`remoteRoot`), and the page-naming wobble (navbar labels `/sources.html` **RAG**, `/git-sources.html` **Sources** — the Sync button's home is now named correctly in 4 places); added the phase-183 E2E pointer. AGENTS.md verified accurate (rule 12 matches the delete-site audit).
**Deviation**: none from the locked decisions; the E2E is a behaviour pin (byte-identical owner-visible behaviour), the statement-count probe remains the memory teeth, as phase 183 D1/A4 specify.
**Next pending phase**: `184_sync_bounded_session`.
This commit is contained in:
@@ -0,0 +1,62 @@
|
||||
# Phase 188 — The vector search stops full-scanning 414,220 embeddings
|
||||
|
||||
**Source:** found while investigating the 2026-10-15 memory incident — `pg_indexes` for `chunks` lists `chunks_pkey`, `ix_chunks_document_id`, `ix_chunks_tsv` (GIN) and **no index on `embedding`**, so every chat turn runs `ORDER BY embedding <=> :vec LIMIT 100` (A7's cosine tier) as a sequential scan of a 3 GB relation: per-turn CPU on the interactive path, plus 3 GB of page-cache churn on a 3,904 MB box that the sync also needs.
|
||||
**Story:** n/a (incident-adjacent, owner directive 2026-10-15)
|
||||
**Depends on:** none functionally; LAST of the six incident phases — it is the only one that touches the schema and the retrieval quality contract, so it lands after the sync's memory work is in.
|
||||
|
||||
**Context:** `.agents/PLAN.md` §2 — **A3** (PostgreSQL 17 + pgvector, cosine `<=>`, one system), **A6** (dim **768**, fixed at table creation, a mismatch fails loudly), **A7** (hybrid: **cosine top-100** ∪ FTS top-30 → RRF k=60 → full text of the top-N=2 documents), **A8** (the honesty gate: deflect only when best cosine < `BOR_RELEVANCE_THRESHOLD` 0.62 AND (zero FTS hits OR best cosine < `BOR_LEXICAL_SUPPORT_FLOOR` 0.35)), **A13** (every migration ships a TESTED downgrade), **A16** (per-phase unit + integration + >90% + one dedicated E2E file), **A21** (LLM calls are free; the interactive path's latency is what matters); `app/models.py` — `Chunk.embedding: Mapped[list[float] | None] = mapped_column(Vector(EMBEDDING_DIM))` (**L241**); `app/rag/retriever.py` — **`_vector_candidates(db, question_embedding, limit)` (L431)** building `distance = Chunk.embedding.cosine_distance(question_embedding)` (**L440**) `.where(Chunk.embedding.is_not(None))` (**L444**) `.order_by(distance).limit(limit)` (**L446**), the fusion caller at **L690–711** (`vector = _vector_candidates(db, question_embedding, v_n)`), `_lexical_candidates` (L632); `app/config.py` (the `BOR_` `Settings` pattern, `relevance_threshold` / `lexical_support_floor` / `suggested_docs` as the retrieval-knob neighbours); `app/core/settings_catalog.py` — `GROUPS` (**L72**, the `"Retrieval & honesty"` group) + the `set(CATALOG) == set(Settings.model_fields)` completeness pin; `alembic/versions/` — head is **`0028`** (`uv run alembic heads`), so this phase adds **`0029`**; `db/Containerfile` — **`ARG PGVECTOR_VERSION=v0.8.0`** (HNSW + `halfvec` available; verified 0.8.0 on both the dev and the production instance); `scripts/eval_retrieval.py` (+ `tests/fixtures/retrieval_battery.txt`, the phase-119/162 regression battery — `uv run python -m scripts.eval_retrieval --from-file tests/fixtures/retrieval_battery.txt`, `AIPI_KEY` required); `benchmarks/model_benchmarks.csv` (the recorded-measurement home); `tests/e2e/test_retrieval_quality.py`, `tests/e2e/test_search_recall_small_sources.py`, `tests/e2e/test_chat_rag.py`, `tests/unit/test_retriever.py`, `tests/unit/test_retriever_recency.py`.
|
||||
|
||||
## Production numbers (read 2026-10-15, read-only)
|
||||
414,220 chunks, **all 414,220 with a non-null `embedding`**, `chunks` relation ≈ **3,000 MB**, database 3,147 MB, `shared_buffers=128MB`, `work_mem=4MB`, host 3,904 MB with **no swap**. pgvector **0.8.0**.
|
||||
|
||||
## Objective
|
||||
Add an HNSW cosine index on `chunks.embedding` through a reversible migration built `CONCURRENTLY` (A3's one-system rule respected — no new service, no new dependency), give the retrieval tier an explicit `hnsw.ef_search` knob that can never be set below the A7 top-100 candidate count, and prove with a deterministic recall pin plus the live retrieval battery that the approximate index does not move the top hits, the `top_score`, or the honesty-gate verdicts.
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
- **D1 — the index is on the existing `vector(768)` column, `vector_cosine_ops` (task 01).** `CREATE INDEX CONCURRENTLY … USING hnsw (embedding vector_cosine_ops) WITH (m = 16, ef_construction = 64)` — the pgvector defaults' neighbourhood, which keeps the graph buildable on a 4 GB box. `halfvec(768)` (half the index size) was considered and REJECTED for this phase (A2): it needs `embedding::halfvec(768)` in `_vector_candidates` AND a quantisation-recall study, i.e. two quality risks in one phase. The index is smaller-with-halfvec; if the prod build does not fit, that is a follow-up phase, not a silent compromise here.
|
||||
- **D2 — `CONCURRENTLY` needs autocommit, and an interrupted build leaves an INVALID index (task 01).** Migration `alembic/versions/0029_chunks_embedding_hnsw.py` runs its DDL inside `with op.get_context().autocommit_block():` and, before creating, drops any existing INVALID index of that name (`select indisvalid from pg_index join pg_class …` → `DROP INDEX CONCURRENTLY IF EXISTS ix_chunks_embedding_hnsw` when `indisvalid` is false), then re-checks `indisvalid` afterwards and raises if the build did not validate — because a `CONCURRENTLY` build that dies (a 3,904 MB box is exactly where it dies) leaves an index that costs insert time and buys nothing, silently. Upgrade and downgrade both run in the autocommit block; the downgrade is `DROP INDEX CONCURRENTLY IF EXISTS …` and is TESTED (A13) by `alembic downgrade -1` / `upgrade head` in the task's verification.
|
||||
- **D3 — `ef_search` is explicit and clamped (task 02).** `retrieval_ef_search: int = 100` (`BOR_RETRIEVAL_EF_SEARCH`, `"Retrieval & honesty"` catalogue group): `_vector_candidates` issues `SET LOCAL hnsw.ef_search = :ef` (per-transaction — no leakage between requests, no global GUC change; A12 keeps state where it belongs) immediately before the SELECT, and a value below the A7 candidate count is CLAMPED UP to it with one WARNING line (`retrieval: ef_search=%d raised to %d (A7 candidate count)`) rather than silently degrading recall. Negative values refuse startup the way the phase-182 `show_text_max_*` knobs do.
|
||||
- **D4 — recall is pinned twice: deterministic in CI, live against the battery (tasks 03 + 04).**
|
||||
- **CI (deterministic, no LLM):** seed ~5,000 chunks with fixed-seed 768-dim vectors, compute the EXACT cosine top-100 in Python (`numpy`-free is fine — a pure-Python dot product over the seeded corpus, or reuse the existing helper if `tests/` already has one), then assert the index-backed query's top-100 set overlaps the exact set by **≥ 0.99** at `ef_search=100` and that `1 - distance` (`top_score`) matches the exact value to 6 decimals for the returned rows. Plus the plan pin: `EXPLAIN (FORMAT JSON)` for the real `_vector_candidates` statement names `ix_chunks_embedding_hnsw` (and does NOT show `Seq Scan on chunks`).
|
||||
- **Live (A8's honesty contract):** the retrieval battery, index OFF vs ON, on the 1,000-document live-replica KB (the `restore-test-db` skill) with `AIPI_KEY` set — every question's `top_score`, top-5 document set and deflection verdict must be identical (a difference means RAISING `ef_search`, never accepting it). The before/after numbers are recorded in `benchmarks/` and the phase report; the build's wall time and the box's free RAM before/after are recorded too (they are the deploy-runbook numbers).
|
||||
- **D5 — the build is an operator step with a runbook (task 01 + README).** The migration is correct, but building HNSW over 414k × 768 on the production 3,904 MB box needs a documented pre-flight: free-RAM check, `maintenance_work_mem` set explicitly for the session (e.g. 512 MB, never left at the 4 MB `work_mem` default), the build run against the postgres container directly (`podman exec … psql`) if the app container is memory-limited (the owner's separate task), `autovacuum` left running afterwards, and the INVALID-index recovery path. `README.md` gains that runbook section; the phase's own CI build runs against the small test corpus, not 414k rows.
|
||||
- **NOT touched:** the embedding dim (A6), the `<=>` operator and the hybrid fusion (A7), the thresholds themselves (A8 — unchanged unless the battery forces an `ef_search` raise), the `tsv` GIN index, `chunks`/`documents` columns, the importer/sync (phases 183–186), the frontend, any new dependency or service (A3/A12).
|
||||
|
||||
## Dependencies
|
||||
— (none; deliberately the LAST of the six incident phases — schema + retrieval-quality risk after the sync's memory work)
|
||||
|
||||
## Tasks
|
||||
1. `01_hnsw_migration.md` — migration `0029` (autocommit block, `CONCURRENTLY`, the INVALID-index pre-drop + post-validate), the tested downgrade, the migration tests.
|
||||
2. `02_ef_search_setting.md` — `retrieval_ef_search` on `Settings` + the catalogue group + `.env.example`, `SET LOCAL` in `_vector_candidates`, the clamp-and-warn rule, the negative-refuses-startup pin.
|
||||
3. `03_plan_and_recall_pins.md` — the `EXPLAIN` plan pin (index used, no seq scan) and the deterministic ≥0.99 overlap / exact-`top_score` recall pin on a fixed-seed corpus.
|
||||
4. `04_recall_battery_gate.md` — the live battery index-off vs index-on on the 1,000-doc replica KB; the recorded numbers in `benchmarks/` + the phase report; the `ef_search` raise loop if anything moved.
|
||||
5. `05_e2e_index_chat_and_runbook.md` — `tests/e2e/test_vector_index_recall.py` (the phase's dedicated Playwright suite), the `README.md` build runbook, the full gates, the docs pass, the atomic commit.
|
||||
|
||||
## Testing & Quality
|
||||
- **Unit:** the `retrieval_ef_search` default/clamp/negative rules; the migration module's helpers (the INVALID-index decision function is a pure function over a query result, so it unit-tests without a DB).
|
||||
- **Integration:** `tests/integration/test_chunks_hnsw_index.py` — upgrade/downgrade round-trip on the test DB, `pg_index.indisvalid` asserted, the INVALID-index recovery path exercised by creating an INVALID index deliberately (`… WHERE false` won't produce INVALID; use the documented `UPDATE pg_index SET indisvalid=false` superuser trick or a deliberately interrupted build — pick the one the test DB allows and document it), the `EXPLAIN` plan pin, the deterministic recall pin. `tests/unit/test_retriever.py` + `test_retriever_recency.py` + `tests/integration/test_chat_db_sessions.py` green with `SET LOCAL` in play.
|
||||
- **Live gate:** the retrieval battery off/on, identical verdicts, recorded.
|
||||
- **E2E (AGENTS rule 4, task 05):** `tests/e2e/test_vector_index_recall.py`, run **in isolation**.
|
||||
- **Coverage:** **>90%** on `app/` (`uv run pytest --cov=app --cov-report=term-missing`).
|
||||
- **Lint/types:** `uv run ruff check . && uv run pyright`.
|
||||
|
||||
## Documentation & Consistency
|
||||
- Final pass reviews `README.md` and `AGENTS.md` in their entirety against the post-phase app and fixes every inconsistency found, pre-existing ones included.
|
||||
- `README.md`: the `0029` deploy section (the D5 build runbook: pre-flight free RAM, `maintenance_work_mem`, `CONCURRENTLY` semantics, the INVALID-index recovery, expected build time from task 04's measurement) and the `BOR_RETRIEVAL_EF_SEARCH` knob (default 100, the clamp, "raise it if recall matters"). `AGENTS.md`: the quick-reference gains nothing new unless a command changes — the retrieval battery line already exists; keep it accurate.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `alembic/versions/0029_…` creates `ix_chunks_embedding_hnsw` `CONCURRENTLY` inside an autocommit block, pre-drops an INVALID index of that name, post-validates `indisvalid`, and its downgrade is tested (`upgrade head` → `downgrade -1` → `upgrade head` green on the test DB, A13).
|
||||
- [ ] `retrieval_ef_search` (default 100) is applied with `SET LOCAL` in `_vector_candidates`, clamped up with a WARNING below the A7 candidate count, negative refuses startup, and it appears in `CATALOG` (`"Retrieval & honesty"`) + `.env.example` + `README.md`.
|
||||
- [ ] The `EXPLAIN` pin shows the index used and no seq scan; the deterministic recall pin shows ≥0.99 top-100 overlap and exact `top_score`s.
|
||||
- [ ] The live battery ran index-off and index-on over the 1,000-doc replica with IDENTICAL top scores, top-5 sets and deflection verdicts, and the numbers (plus build time + free-RAM readings) are recorded in `benchmarks/` and the phase report.
|
||||
- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean.
|
||||
- [ ] `uv run pytest tests/e2e/test_vector_index_recall.py -v --no-cov` green **in isolation**.
|
||||
- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (the runbook included).
|
||||
- [ ] One atomic `--no-gpg-sign` Conventional Commit (e.g. `perf(retrieval): index chunks.embedding with HNSW and pin recall so cosine stops full-scanning 414k vectors`); the phase dir moves to `.agents/phases/complete/`.
|
||||
|
||||
## Locked decisions (owner-confirmed 2026-10-15)
|
||||
- **A1 — HNSW on `vector(768)` with `vector_cosine_ops`, built `CONCURRENTLY`.** No query rewrite, no new dependency, `<=>` uses it directly (A3).
|
||||
- **A2 — `halfvec` quantisation is NOT part of this phase** (it needs a statement cast + its own recall study); revisit only if the production build does not fit in RAM.
|
||||
- **A3 — recall is a gate, not a hope.** The honesty gate (A8) reads `top_score`; an approximate index that changes a top hit can flip a deflect. The deterministic ≥0.99 pin AND the live battery must pass, and the remedy is a higher `ef_search`, never a relaxed assertion.
|
||||
- **A4 — `SET LOCAL`, per transaction.** No global GUC mutation, no connection-pool-wide state (A12: no aux services, restart clears state).
|
||||
- **A5 — the production build is an operator action with a runbook**, executed after the owner's container-memory work lands; the phase's CI never builds a 414k-row graph.
|
||||
@@ -0,0 +1,44 @@
|
||||
# Task 01 — Migration `0029`: the HNSW index, built concurrently, reversible
|
||||
|
||||
**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation (`pg_indexes` shows no index on `chunks.embedding`)
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
Add `ix_chunks_embedding_hnsw` on `chunks.embedding` with `vector_cosine_ops` through alembic `0029`, built `CONCURRENTLY` inside an autocommit block, self-healing an interrupted build's INVALID index, with a tested downgrade (A13).
|
||||
|
||||
## Work
|
||||
1. `alembic/versions/0029_chunks_embedding_hnsw.py` — `revision = "0029"`, `down_revision = "0028"` (verify with `uv run alembic heads` before writing), the house docstring shape (why / what / reversibility):
|
||||
- Pure helpers (unit-testable, no I/O): `INDEX_NAME = "ix_chunks_embedding_hnsw"`, and `def should_drop_invalid(indisvalid: bool | None) -> bool` (True when the row exists and `indisvalid is False`), plus `def is_valid(indisvalid: bool | None) -> bool`.
|
||||
- `upgrade()`:
|
||||
```python
|
||||
with op.get_context().autocommit_block():
|
||||
if should_drop_invalid(_lookup_index_validity(op.get_bind())):
|
||||
op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}")
|
||||
op.execute(
|
||||
f"CREATE INDEX CONCURRENTLY {INDEX_NAME} ON chunks "
|
||||
"USING hnsw (embedding vector_cosine_ops) WITH (m = 16, ef_construction = 64)"
|
||||
)
|
||||
if not is_valid(_lookup_index_validity(op.get_bind())):
|
||||
raise RuntimeError(f"{INDEX_NAME} did not validate — drop it and re-run")
|
||||
```
|
||||
`_lookup_index_validity(bind)` runs `select i.indisvalid from pg_index i join pg_class c on c.oid = i.indexrelid where c.relname = :name` and returns `None` when absent. The docstring names the failure mode this guards (a `CONCURRENTLY` build killed mid-way leaves an INVALID index that costs inserts and serves nothing — on a 3,904 MB production box that is the expected failure, not the exotic one).
|
||||
- `downgrade()`: the same autocommit block with `DROP INDEX CONCURRENTLY IF EXISTS ix_chunks_embedding_hnsw`.
|
||||
- No `chunk`/`document` column change, no data rewrite (so the migration is cheap on a healthy box and interruptible).
|
||||
2. `tests/integration/test_chunks_hnsw_index.py` — the migration family:
|
||||
- `test_upgrade_creates_a_valid_hnsw_index`: run the migration's `upgrade()` against the test DB (the house alembic-in-test pattern — find the existing migration test's approach first and reuse it), then assert `pg_indexes` has the index, `pg_index.indisvalid` is true, and `pg_get_indexdef` contains `USING hnsw` + `vector_cosine_ops` + `ef_construction=64`.
|
||||
- `test_downgrade_removes_it` then `test_upgrade_head_again` — the A13 round-trip.
|
||||
- `test_invalid_index_is_dropped_and_rebuilt`: force an INVALID state (either `UPDATE pg_index SET indisvalid = false WHERE indexrelid = '…'::regclass` as the test DB's superuser, or a documented alternative) → run `upgrade()` → the index is valid again (no `relation already exists` error, no leftover INVALID index).
|
||||
- `test_should_drop_invalid_matrix` (pure): `None` → False, `True` → False, `False` → True.
|
||||
- Skip-guard: these tests need the `vector` extension and DDL rights — follow the existing migration tests' skip/fixture conventions (do NOT invent a new one).
|
||||
3. Verify: `uv run alembic upgrade head && uv run alembic downgrade -1 && uv run alembic upgrade head` on the dev DB (record the output in the task note); `uv run pytest tests/integration/test_chunks_hnsw_index.py -v`; `uv run pytest`.
|
||||
|
||||
## Testing & Quality
|
||||
- Integration: the four migration pins above (create / downgrade / re-upgrade / INVALID self-heal).
|
||||
- Unit: the two pure helpers' matrices.
|
||||
- Coverage: **>90%** on `app/` (the migration module is covered by its own tests).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `alembic/versions/0029_chunks_embedding_hnsw.py` exists with the autocommit block, `CONCURRENTLY`, the INVALID self-heal, the post-validate raise, and the `CONCURRENTLY` downgrade.
|
||||
- [ ] The `upgrade head → downgrade -1 → upgrade head` round-trip is green on the dev DB (output recorded) and in the integration tests.
|
||||
- [ ] The forced-INVALID test is green and documents the trick it used.
|
||||
- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean.
|
||||
@@ -0,0 +1,37 @@
|
||||
# Task 02 — `BOR_RETRIEVAL_EF_SEARCH`: explicit, per-transaction, clamped
|
||||
|
||||
**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
Give the HNSW tier an operator-visible `ef_search` that defaults to the A7 candidate count and can never be set below it, applied per transaction with `SET LOCAL` so no request leaks state into another.
|
||||
|
||||
## Work
|
||||
1. `app/config.py` — `retrieval_ef_search: int = 100`, the `hybrid_vector_candidates` (L351) docstring style, `BOR_RETRIEVAL_EF_SEARCH` via the `env_prefix`. A negative value refuses startup exactly like the phase-182 `show_text_max_*` knobs (follow their validator, don't invent one); `0` is rejected the same way (there is no sensible `ef_search=0` — the clamp would silently rewrite it, which is exactly what a `0`-means-off operator would not expect).
|
||||
2. `app/core/settings_catalog.py` — `SettingMeta("retrieval_ef_search", "Retrieval & honesty", "Vector ef_search")`; `uv run pytest tests/unit/test_settings_catalog.py -v` proves the completeness pin.
|
||||
3. `app/rag/retriever.py` — `_vector_candidates` (**L430–446**): before the SELECT,
|
||||
```python
|
||||
ef = settings.retrieval_ef_search # settings passed in / read the way this module already reads them
|
||||
if ef < limit:
|
||||
logger.warning("retrieval: ef_search=%d raised to %d (A7 candidate count)", ef, limit)
|
||||
ef = limit
|
||||
db.execute(text(f"SET LOCAL hnsw.ef_search = {int(ef)}")) # int()-coerced: no user string reaches SQL
|
||||
```
|
||||
Notes the code must carry: `SET LOCAL` is transaction-scoped (A4 — the session's transaction commits/rolls back with the request; no global GUC mutation, nothing surviving a pool recycle); the value is `int()`-coerced so nothing but a number is interpolated; the clamp exists because `ef_search < LIMIT` returns FEWER candidates than the A7 tier promises, i.e. a silent recall regression.
|
||||
Threading: `_vector_candidates` currently takes `(db, question_embedding, limit)` — read `Settings` the same way the surrounding functions do (find the module's existing `get_settings()`/passed-settings convention and match it; no new plumbing if one exists).
|
||||
4. Pins:
|
||||
- `tests/unit/test_retriever.py`: the clamp (ef 10 with limit 100 → warning + 100 used), the pass-through (ef 200 → 200), the default (unset → 100), the negative-refusal at settings construction; assert the emitted statement text through a stub session that records `db.execute` calls (no DB needed).
|
||||
- `tests/integration/test_chunks_hnsw_index.py`: after a real `SET LOCAL`, `show hnsw.ef_search` in the same transaction reports the clamped value and a NEW transaction reports the server default again (the scoping pin).
|
||||
- `tests/integration/test_chat_db_sessions.py` + `tests/unit/test_retriever_recency.py` green (the extra statement in the turn's transaction breaks nothing).
|
||||
5. Verify: `uv run pytest tests/unit/test_retriever.py tests/unit/test_settings_catalog.py tests/integration/test_chunks_hnsw_index.py tests/integration/test_chat_db_sessions.py -v`; `uv run pytest`.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: clamp / pass-through / default / negative refusal / statement text.
|
||||
- Integration: `SET LOCAL` scoping (same-transaction vs new).
|
||||
- Coverage: **>90%** on `app/`.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `retrieval_ef_search` exists (default 100, negative refused), is catalogued under `"Retrieval & honesty"`, and is in `.env.example` + `README.md`.
|
||||
- [ ] `_vector_candidates` issues `SET LOCAL hnsw.ef_search` with the clamp-and-warn rule, int-coerced.
|
||||
- [ ] The scoping pin proves a new transaction sees the server default again.
|
||||
- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean.
|
||||
@@ -0,0 +1,27 @@
|
||||
# Task 03 — Prove the planner uses it and that recall did not move (deterministic)
|
||||
|
||||
**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
Two CI pins: the cosine tier's statement uses `ix_chunks_embedding_hnsw` (no sequential scan), and the index-backed top-100 matches the exact top-100 on a fixed-seed corpus, with `top_score` values exact to 6 decimals.
|
||||
|
||||
## Work
|
||||
1. `tests/integration/test_chunks_hnsw_recall.py` — a NEW file:
|
||||
- **The corpus fixture:** ~5,000 `Chunk` rows with 768-dim vectors from a FIXED seed (a documented `random.Random(20261015)` scheme: 40 cluster centroids + noise around them, so nearest neighbours are meaningful rather than uniformly random), all L2-normalised (cosine == dot product, so the exact answer is trivial to compute), committed, then the HNSW index built for this corpus via the `0029` migration's `upgrade()` (task 01) — never `CREATE INDEX` inline (the migration is the single definition).
|
||||
- `test_plan_uses_the_hnsw_index`: `EXPLAIN (FORMAT JSON)` for the exact statement `_vector_candidates` builds (the same SQLAlchemy statement compiled with literal binds, so the planner sees a real plan), assert the node names `ix_chunks_embedding_hnsw` / an index scan node and that no `Seq Scan on chunks` node appears. Assert the same statement's plan WITHOUT the index (a `DROP INDEX`/re-create around a sub-fixture, or a `SET enable_indexscan = off`-style comparison — pick one, document it) DOES seq-scan, so the pin cannot pass vacuously.
|
||||
- `test_top100_recall_at_ef_search_100`: for 30 fixed-seed query vectors, compute the EXACT cosine top-100 (pure Python over the fixture corpus) and compare against the index-backed `_vector_candidates(db, q, 100)` result: assert `|overlap| / 100 >= 0.99` for every query (mean reported in the docstring) and that each returned row's `1 - distance` equals the exact cosine within 1e-6. Print the per-query overlap in the failure message (so a future raise of `ef_search` has evidence).
|
||||
- `test_recall_never_below_the_pinned_floor_when_ef_search_raised`: the same assertion at `ef_search = 200` must be ≥ the `ef_search = 100` mean overlap (monotonicity sanity: raising `ef` cannot make recall worse).
|
||||
- Module docstring: the production numbers (414,220 chunks / 3,000 MB / no index found / pgvector 0.8.0), why 5,000 rows is enough for a plan pin but not a proxy for prod (the live battery in task 04 is), the measured overlaps, and the `ef_search` values used.
|
||||
2. Runtime guard: the fixture corpus build + index creation must stay fast (target < ~60 s); if it drifts, reduce the corpus (never the assertions) and record the new size — the phase-179 A4 discipline ("cut the fixture, never the guard").
|
||||
3. Verify: `uv run pytest tests/integration/test_chunks_hnsw_recall.py -v`; `uv run pytest`.
|
||||
|
||||
## Testing & Quality
|
||||
- Integration: the plan pin (with its vacuity guard) + the two recall pins.
|
||||
- Coverage: **>90%** on `app/`.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/integration/test_chunks_hnsw_recall.py -v` green: index used, no seq scan, mean top-100 overlap ≥ 0.99, `top_score`s exact to 1e-6, monotonic at `ef_search=200`.
|
||||
- [ ] The vacuity guard (seq scan when the index is unavailable) is present and green.
|
||||
- [ ] The docstring records the measured overlaps and the prod context numbers.
|
||||
- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean.
|
||||
@@ -0,0 +1,35 @@
|
||||
# Task 04 — The live gate: the retrieval battery, index off vs on
|
||||
|
||||
**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation; PLAN A8
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
Prove on the real embedding endpoint and the 1,000-document live-replica KB that the approximate index does not move a single verdict — same `top_score`, same top-5 documents, same deflection decisions — and record the numbers plus the production build cost.
|
||||
|
||||
## Why this is a gate and not a note
|
||||
PLAN **A8** derives the honesty decision from `best_cosine` against `BOR_RELEVANCE_THRESHOLD` (0.62) and `BOR_LEXICAL_SUPPORT_FLOOR` (0.35). An approximate index that swaps which chunk is nearest can flip a HIGH into a deflect, or cite a different document — the exact class of silent regression the phase-119/162 battery exists to catch.
|
||||
|
||||
## Prerequisites (document them in the phase report before starting)
|
||||
`AIPI_KEY` in the environment (the same convention as `scripts/llm_probe.py` / `scripts/eval_retrieval.py`), the 1,000-document live-replica KB restored (the `restore-test-db` skill), and `uv run alembic upgrade head` applied. If `AIPI_KEY` or the replica KB is unavailable, STOP and report it — do not commit the migration as phase-complete on the strength of task 03's synthetic pin alone.
|
||||
|
||||
## Work
|
||||
1. **Baseline (index OFF):** drop the index (`DROP INDEX CONCURRENTLY ix_chunks_embedding_hnsw` — the migration's downgrade), then
|
||||
`uv run python -m scripts.eval_retrieval --from-file tests/fixtures/retrieval_battery.txt > /tmp/battery_off.txt`.
|
||||
Capture per question: the effective/top score, the top-5 document list, the honesty-gate verdict column.
|
||||
2. **Candidate (index ON):** re-apply (`uv run alembic upgrade head`), same command to `/tmp/battery_on.txt`.
|
||||
3. **The diff:** compare question by question. Required: identical deflection verdicts, identical top-5 document sets, and `top_score` deltas ≤ 1e-4. Any difference → raise `BOR_RETRIEVAL_EF_SEARCH` (150 → 200 → 400) and re-run BOTH sides until the verdicts match; the `ef_search` that restores parity becomes the shipped default (update `Settings` + `.env.example` + `README.md` + the task-02 tests, and say so in the commit body). Never widen the tolerance.
|
||||
4. **Record** (the repo's measurement homes, not chat):
|
||||
- `benchmarks/` — a row/section per run: pgvector version, index definition (`m`, `ef_construction`), `ef_search`, corpus size, the battery's question count, verdict-agreement counts, the max `top_score` delta, the battery's wall time off vs on, and the per-question cosine-tier latency if the script reports it.
|
||||
- the phase report (`.agents/reports/`, house location) — the production BUILD numbers captured while building on a box of the deployment's size class where possible: build wall time, `maintenance_work_mem` used, free RAM before/after (`free -m`), index size (`select pg_size_pretty(pg_relation_size('ix_chunks_embedding_hnsw'))`), and the seq-scan → index-scan latency change on a real chat turn.
|
||||
5. **Timing sanity:** the interactive path got faster, not slower (A21's "latency binds the interactive path"). Record the before/after; if the index makes a turn slower at the shipped `ef_search`, that is a finding for the phase report, not a silent acceptance.
|
||||
6. Re-run the app's own retrieval regression expectations if the battery script's output is asserted anywhere in the suite (`grep -rn "retrieval_battery" tests/ scripts/`), and keep those green.
|
||||
|
||||
## Testing & Quality
|
||||
- Live gate: the off/on battery comparison with identical verdicts (the remedy is `ef_search`, never the tolerance).
|
||||
- Recorded artifacts: `benchmarks/` entry + the phase report numbers (both referenced by path in the commit body).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `/tmp/battery_off.txt` and `/tmp/battery_on.txt` (or their committed equivalents) exist, and the comparison shows identical deflection verdicts, identical top-5 sets, max score delta ≤ 1e-4.
|
||||
- [ ] The shipped `retrieval_ef_search` default is the value that achieved parity (documented whether it stayed 100 or was raised).
|
||||
- [ ] `benchmarks/` records the index definition, sizes, build time, free-RAM readings and the off/on timings; the phase report repeats the production build numbers.
|
||||
- [ ] Any test that reads the battery fixture stays green.
|
||||
@@ -0,0 +1,36 @@
|
||||
# Task 05 — E2E: chat over the indexed KB (the phase's dedicated Playwright suite)
|
||||
|
||||
**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
Prove through the browser that retrieval behaves identically over the indexed KB — the same document is cited, the answer is not deflected, and the turn completes — plus the `README.md` build runbook, the full gates, the docs pass, and the atomic commit.
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_vector_index_recall.py` — a NEW module, mock-only (`E2E_REAL_LLM=1` skipped; the E2E mock's deterministic L2-normalised token-overlap embeddings make the cosine tier reproducible, PLAN A16), the `tests/e2e/test_retrieval_quality.py` / `test_search_recall_small_sources.py` / `test_chat_rag.py` shapes (dedicated module app + port, mock LLM, dedicated `BOR_SOURCES_DIR`, shared E2E Postgres truncated before AND after each test, admin login via `tests/e2e/auth_helpers.py`).
|
||||
- Fixture: a small KB (8–12 docs) imported through the real importer so chunks + embeddings exist, with the `0029` migration's index present in the test DB (import-time inserts into an HNSW-indexed table are exercised by this fixture too — inserts are the phase's insert-cost surface, so keep them in the path rather than importing then indexing).
|
||||
- **Test 1 `test_answered_question_cites_the_same_document_indexed_or_not`:** the sentinel question is asked twice — once with the index dropped (test-local DDL, restored after) and once with it present — and the cited source chip / cited document is the SAME in both, with no deflection either way. This is the A7/A8 contract expressed in the UI, and it is a real before/after, not a tautology.
|
||||
- **Test 2 `test_index_exists_and_is_valid_after_import`:** after the fixture import, the test's own DB query asserts `pg_index.indisvalid` is true for `ix_chunks_embedding_hnsw` (an index that died during the fixture's inserts would be caught here — the silent-INVALID failure mode from task 01, at the app layer).
|
||||
- **Test 3 `test_near_miss_question_still_deflects`:** a question with no KB support is deflected with the index present (A8's LOW path is unchanged — the index must not manufacture a high enough `top_score` to flip a deflect).
|
||||
- **Test 4 `test_ef_search_setting_is_honoured`:** with `BOR_RETRIEVAL_EF_SEARCH=150` on the module app, the turn still answers normally (a knob change cannot break a chat turn), and the clamp warning appears when set to a value below the A7 tier (caplog or the status/log surface the test harness can read).
|
||||
- Module docstring: the incident/investigation reference, the index definition + `ef_search` default, the test→observable mapping, the run-in-isolation command, prerequisites (`podman compose up -d db`).
|
||||
2. **README runbook (D5):** a `Deploying migration 0029 (HNSW)` section — the pre-flight (`free -m`, `shared_buffers`, expected peak), building with an explicit `maintenance_work_mem` (never the 4 MB default), `CREATE INDEX CONCURRENTLY` semantics (no lock, longer build, may leave an INVALID index if killed), the recovery path (drop and re-run the migration), the measured numbers from task 04 (build time, index size), and the pointer that the owner's container-memory work should land before building on a 4 GB-class box.
|
||||
3. **Docs pass:** `README.md` + `AGENTS.md` reviewed in full against the post-phase app; every inconsistency fixed, pre-existing included; `AGENTS.md`'s quick-reference stays accurate (the retrieval-battery line already exists and is now load-bearing for this phase — confirm its command line is still correct).
|
||||
4. **The full gates:**
|
||||
- `uv run pytest tests/e2e/test_vector_index_recall.py -v --no-cov` — green **in isolation**.
|
||||
- `uv run pytest tests/e2e/test_retrieval_quality.py -v --no-cov`, `tests/e2e/test_search_recall_small_sources.py -v --no-cov`, `tests/e2e/test_chat_rag.py -v --no-cov` — green in isolation (the retrieval/chat families the index touches; run after the new suite).
|
||||
- `uv run pytest --cov=app --cov-report=term-missing` — full suite green, `app/` coverage **>90%**.
|
||||
- `uv run ruff check . && uv run pyright` — clean.
|
||||
- `uv run python -m scripts.eval_retrieval --from-file tests/fixtures/retrieval_battery.txt` — task 04's parity verdict still holds at the shipped `ef_search`.
|
||||
5. **Commit + move:** one atomic `--no-gpg-sign` Conventional Commit — e.g. `perf(retrieval): index chunks.embedding with HNSW and pin recall so cosine stops full-scanning 414k vectors` — covering `alembic/versions/0029_…`, `app/config.py`, `app/core/settings_catalog.py`, `app/rag/retriever.py`, the tests, `benchmarks/`, `README.md`, `.env.example` and `.agents/`. Move the phase dir to `.agents/phases/complete/188_chunks_hnsw_index/`.
|
||||
|
||||
## Testing & Quality
|
||||
- E2E: the dedicated suite + the three retrieval/chat E2E families in isolation.
|
||||
- Coverage: **>90%** on `app/`; ruff + pyright clean.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/e2e/test_vector_index_recall.py -v --no-cov` green **in isolation** (same citation indexed vs not, index valid after import, near-miss still deflects, `ef_search` change harmless).
|
||||
- [ ] The three regression E2E families green in isolation; the battery parity verdict still holds.
|
||||
- [ ] `uv run pytest --cov=app --cov-report=term-missing` green with `app/` coverage >90%; ruff + pyright clean.
|
||||
- [ ] `README.md` (runbook + knob) and `AGENTS.md` accurate and consistent with the whole project.
|
||||
- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moved to `.agents/phases/complete/`.
|
||||
Reference in New Issue
Block a user