All gates green. Report: **Implemented / verified (phase 185)** - `_doc_row_stmt` is the ONE deferral helper for all four fast-path row loads; only `Document.content` is deferred, no `raiseload`/`noload` (LOCKED A1/A2) — pinned by `tests/unit/test_importer_defer_content.py`. - `tests/integration/test_import_fast_path_columns.py` (5), `tests/e2e/test_fast_path_deferred_content.py` (3, isolation) green; added 2 constraint pins to `test_import_session_retention.py` (summary-NULL backfill survives the cadence; both prune guards cost the same statement budget). - **Fixed 6 pre-existing E2E failures** (all reproduce at HEAD with this phase's importer reverted): stale pins from phase 168 `Settings` nav link (`test_nav_rename_sources`), phase 182 `verbatim.js` script count (`test_steering`), phase 175 `/api/chats?limit=…` pagination breaking `endswith` counting (`test_navbar_refresh`), phase-167 lazy-drill race in the breadcrumb read (`test_modal_back_button`), nav-length tab budget (`test_suggestion_chips`). - **Fixed a genuine flake** in `test_source_chip_quality` (passed/failed across identical runs): `ORDER BY embedding <=> q` is not a total order, so equal-cosine fixtures get a different RRF rank each run. Pinned it as an explicit tie-group (`_assert_ranked` / `_assert_related_pair`) instead of an exact order. **Tests / lint (all 100% green)** - `uv run pytest --cov=app --cov-report=term-missing` → **4519 passed**, `app/rag/importer.py` **99%**, `TOTAL` **99%** (>90%). - `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors. - Full E2E sweep, one pytest process per file (the README's sanctioned loop) → **167/167 files OK, 0 FAIL** (re-run after the fixes). **Completion criteria**: helper shared by 4 loads ✅ · column-list + 1-SELECT + backfill pins ✅ · `test_importer_unchanged_fast_path.py` 24 passed, zero expectation edits ✅ · full suite + coverage + lint/types ✅ · phase E2E in isolation ✅ · docs accurate ✅ · commit left to the harness (no git commands run). **Documentation**: README + AGENTS.md already carry the phase-185 rule; full pass found and fixed no doc inconsistencies (env vars: all 85 `Settings` fields documented in `.env.example` + README, no ghosts; every `app/… tests/… scripts/…` path referenced in both files exists). **Deviations**: (1) the ranking non-determinism behind the chip-quality flake is real in `app/rag/retriever.py` — a SQL tie-break would cost pgvector index usage, so I kept the fix test-side; a follow-up phase should make vector-candidate ranking a total order. (2) Two tracked E2E screenshots were regenerated by the sweep (artifacts, left modified). **Next pending phase**: `186_git_log_streaming_dates`.
259 lines
11 KiB
Python
259 lines
11 KiB
Python
"""Phase 185, task 02 — the ONE fast-path row statement (unit pins).
|
||
|
||
``app/rag/importer.py::_doc_row_stmt`` is the single place the importer's
|
||
unchanged fast path (phase 147) loads a ``Document`` row, and it defers
|
||
exactly ONE column: ``Document.content``. That shape is load-bearing twice
|
||
over — it is what keeps a full re-sync's memory bound to row METADATA
|
||
instead of to the corpus (the 2026-10-15 incident: 17,407 rows × avg
|
||
15,217 chars = 252.6 MB of markdown hydrated for a run that re-indexed
|
||
217 files), AND it is what must NOT grow into a lazy-load landmine six
|
||
phases from now.
|
||
|
||
Pinned here (the behaviour itself — the captured SQL, the one-SELECT-per-file
|
||
count and the backfill's single lazy read — lives in
|
||
``tests/integration/test_import_fast_path_columns.py``):
|
||
|
||
* the statement defers ``content`` and NOTHING ELSE: its projection names
|
||
every other mapped column of ``Document``, so a column a later phase adds
|
||
keeps today's whole-row semantics (LOCKED A1 — no ``load_only`` allowlist,
|
||
which would silently defer every future column and resurface as an
|
||
unexplained N+1);
|
||
* the statement is the ``(source, path)`` lookup the fast path has always
|
||
used — the deferral changed the projection, never the predicate;
|
||
* all FOUR fast-path row loads call the helper (``_index_file``,
|
||
``_index_pdf_file``, ``_index_one_embedded_image``,
|
||
``_index_image_file``) and none of them builds its own
|
||
``select(Document)`` — the four sites cannot drift;
|
||
* the importer contains no ``raiseload``/``noload`` (LOCKED A2): the
|
||
phase-118 summary-NULL backfill genuinely reads ``doc.content`` and must
|
||
pay one SELECT, not crash;
|
||
* reading the deferred column through a real session still yields the stored
|
||
text (the honest "plain ``defer`` keeps today's behaviour" claim).
|
||
"""
|
||
from __future__ import annotations
|
||
|
||
import ast
|
||
import inspect
|
||
import re
|
||
import uuid
|
||
from collections.abc import Iterator
|
||
|
||
import pytest
|
||
from sqlalchemy import inspect as sa_inspect
|
||
from sqlalchemy import text
|
||
from sqlalchemy.dialects import postgresql
|
||
from sqlalchemy.orm import Session
|
||
|
||
from app.models import Document
|
||
from app.rag import importer
|
||
from app.rag.importer import _doc_row_stmt
|
||
|
||
#: The four loads that share the phase-147 unchanged fast path (phase 185
|
||
#: D1) — the ONLY call sites of :func:`_doc_row_stmt`.
|
||
FAST_PATH_FUNCTIONS = (
|
||
"_index_file",
|
||
"_index_pdf_file",
|
||
"_index_one_embedded_image",
|
||
"_index_image_file",
|
||
)
|
||
|
||
#: The helper call as it appears at an importer call site (``session.scalar(
|
||
#: _doc_row_stmt(source, rel))``) — counted, not trusted.
|
||
_HELPER_CALL = "_doc_row_stmt("
|
||
|
||
#: ``documents.<column>`` inside a compiled projection (everything the
|
||
#: statement SELECTs before ``FROM``). The loader options are invisible in
|
||
#: the ORM statement's own column list — only the COMPILED projection shows
|
||
#: what actually travels to the database, which is what this phase is about.
|
||
_PROJECTION_COLUMN_RE = re.compile(r"documents\.([a-z_]+)")
|
||
|
||
|
||
def _projected_columns() -> set[str]:
|
||
"""The ``documents`` columns the fast-path statement actually SELECTs."""
|
||
compiled = str(_doc_row_stmt("s", "p").compile(dialect=postgresql.dialect()))
|
||
projection = compiled.split("\nFROM")[0].split(" FROM ")[0]
|
||
return set(_PROJECTION_COLUMN_RE.findall(projection))
|
||
|
||
|
||
def test_helper_defers_content_and_nothing_else() -> None:
|
||
"""``content`` is the ONLY column missing from the fast-path projection.
|
||
|
||
The compiled projection is compared against the mapper's FULL column set,
|
||
so a future ``Document`` column must appear there — if someone swaps the
|
||
``defer`` for a ``load_only`` allowlist, the new column goes missing and
|
||
this test fails (which is exactly the drift LOCKED A1 forbids).
|
||
"""
|
||
mapped = {c.name for c in sa_inspect(Document).columns}
|
||
projected = _projected_columns()
|
||
|
||
assert "content" in mapped, "Document.content vanished — this pin needs a new shape"
|
||
assert projected, "the fast-path statement projects no documents columns at all"
|
||
assert "content" not in projected, (
|
||
"the fast-path row statement no longer defers Document.content — a "
|
||
"full walk would hydrate the corpus markdown again (252.6 MB on the "
|
||
"incident box)"
|
||
)
|
||
assert projected == mapped - {"content"}, (
|
||
"the fast-path row statement defers more (or other) columns than "
|
||
f"content: missing={sorted(mapped - projected - {'content'})} "
|
||
f"unexpected={sorted(projected - mapped)} — only Document.content "
|
||
"may be deferred (LOCKED A1)"
|
||
)
|
||
|
||
|
||
def test_helper_keeps_the_fast_path_predicate() -> None:
|
||
"""The predicate stays the ``(source, path)`` lookup of phase 147."""
|
||
compiled = str(_doc_row_stmt("the-source", "the/path.md").compile()).lower()
|
||
|
||
assert "documents.source" in compiled
|
||
assert "documents.path" in compiled
|
||
assert "where" in compiled
|
||
# The bind names the integration probe attributes statements by.
|
||
assert "source_1" in compiled and "path_1" in compiled
|
||
|
||
|
||
def test_every_fast_path_load_calls_the_helper() -> None:
|
||
"""All four fast-path row loads go through ONE statement (phase 185 D1).
|
||
|
||
Source-level on purpose: the deferral must be impossible to bypass, and
|
||
a fifth whole-entity load would silently undo the phase for one document
|
||
class. The two per-source post-pass walks
|
||
(``_enrich_pending_images``, ``_backfill_image_contexts``) and the prune
|
||
(``load_only`` of six light columns, phase 184) are NOT fast-path loads
|
||
and are deliberately excluded — see the module docstring of
|
||
``tests/integration/test_import_fast_path_columns.py``.
|
||
"""
|
||
for name in FAST_PATH_FUNCTIONS:
|
||
fn = getattr(importer, name)
|
||
src = inspect.getsource(fn)
|
||
assert _HELPER_CALL in src, (
|
||
f"{name} no longer loads its row through importer.{_HELPER_CALL} — "
|
||
"the four fast-path row loads must share the ONE deferring "
|
||
"statement so they cannot drift"
|
||
)
|
||
assert "select(Document)" not in src, (
|
||
f"{name} builds its own select(Document) row again — a whole-entity "
|
||
"fast-path load re-hydrates Document.content (phase 185)"
|
||
)
|
||
|
||
|
||
def test_helper_has_exactly_the_four_fast_path_call_sites() -> None:
|
||
"""The importer calls the helper exactly four times (one per fast path).
|
||
|
||
A fifth ``session.scalar(_doc_row_stmt(…)`` would mean a new consumer was
|
||
wired to the fast-path statement (it is the ``(source, path)`` deferring
|
||
lookup, not a general-purpose loader), and a surviving whole-entity
|
||
``(source, path)`` ``select(Document)`` would mean a bypass.
|
||
"""
|
||
src = inspect.getsource(importer)
|
||
calls = src.count(f"session.scalar({_HELPER_CALL}")
|
||
assert calls == len(FAST_PATH_FUNCTIONS), (
|
||
f"importer.py calls session.scalar({_HELPER_CALL}…) {calls} times — "
|
||
f"expected exactly {len(FAST_PATH_FUNCTIONS)} (the four fast-path "
|
||
"row loads)"
|
||
)
|
||
# No fast-path-shaped whole-entity load survives next to the helper.
|
||
assert src.count("select(Document).where(Document.source ==") == 0, (
|
||
"a (source, path) whole-entity Document load is back — route it "
|
||
"through _doc_row_stmt or give it its own explicit projection"
|
||
)
|
||
|
||
|
||
def test_no_raiseload_or_noload_anywhere_in_the_importer() -> None:
|
||
"""LOCKED A2: the deferred column must QUERY, never raise or come back None.
|
||
|
||
``raiseload``/``noload`` would turn the phase-118 summary-NULL backfill
|
||
(``_unchanged_doc`` step 3, ``content=doc.content``) into a crash or an
|
||
empty summary — a behaviour regression on the one branch that genuinely
|
||
needs the stored markdown. The accepted cost is one SELECT for that row.
|
||
|
||
Checked through the AST, not the text: the module's own docstrings argue
|
||
against both options by NAME, so a substring scan would fail on the
|
||
prose. Only real call/import names count here.
|
||
"""
|
||
tree = ast.parse(inspect.getsource(importer))
|
||
called: set[str] = set()
|
||
imported: set[str] = set()
|
||
for node in ast.walk(tree):
|
||
if isinstance(node, ast.Call):
|
||
if isinstance(node.func, ast.Name):
|
||
called.add(node.func.id)
|
||
elif isinstance(node.func, ast.Attribute):
|
||
called.add(node.func.attr)
|
||
elif isinstance(node, ast.ImportFrom | ast.Import):
|
||
imported.update(alias.name for alias in node.names)
|
||
|
||
for forbidden in ("raiseload", "noload"):
|
||
assert forbidden not in called | imported, (
|
||
f"importer.py calls/imports {forbidden} — phase 185 LOCKED A2 "
|
||
"keeps plain defer() so the summary-NULL backfill reads the "
|
||
"stored content instead of raising"
|
||
)
|
||
assert "defer" in called, "the fast path no longer calls defer() at all"
|
||
# The importer's ONLY load_only is _prune's six-light-column row
|
||
# (phase 184) — it must never reach the fast path, whose shape is
|
||
# "everything except content" (LOCKED A1).
|
||
prune_src = inspect.getsource(importer._prune)
|
||
assert "load_only" in inspect.getsource(importer)
|
||
assert "defer" not in prune_src, (
|
||
"_prune picked up a defer() — its light row is an explicit "
|
||
"load_only allowlist by phase 184, not a deferral"
|
||
)
|
||
|
||
|
||
@pytest.fixture()
|
||
def probe_row(db: Session) -> Iterator[tuple[str, str]]:
|
||
"""One seeded row (raw SQL — this file pins a SELECT, not the importer)
|
||
removed again after the test."""
|
||
source = f"phase185-unit-{uuid.uuid4().hex[:8]}"
|
||
rel = "notes/deferred.md"
|
||
db.execute(
|
||
text(
|
||
"INSERT INTO documents (id, source, path, full_path, title, content, "
|
||
"content_hash, file_hash, indexed_at, created_at, created_at_manual, "
|
||
"summary, is_image) VALUES (:id, :source, :path, :full_path, :title, "
|
||
":content, :content_hash, :file_hash, now(), now(), false, :summary, "
|
||
"false)"
|
||
),
|
||
{
|
||
"id": uuid.uuid4(),
|
||
"source": source,
|
||
"path": rel,
|
||
"full_path": f"/tmp/{source}/{rel}",
|
||
"title": "Deferred",
|
||
"content": "the stored markdown body — phase 185",
|
||
"content_hash": "ch",
|
||
"file_hash": "fh",
|
||
"summary": None,
|
||
},
|
||
)
|
||
db.commit()
|
||
db.expunge_all()
|
||
try:
|
||
yield source, rel
|
||
finally:
|
||
db.execute(text("DELETE FROM documents WHERE source = :s"), {"s": source})
|
||
db.commit()
|
||
db.expunge_all()
|
||
|
||
|
||
def test_deferred_content_still_reads_back(db: Session, probe_row: tuple[str, str]) -> None:
|
||
"""A deferred column is LAZY, not gone: the row reads back whole.
|
||
|
||
The trade phase 185 accepted (D2): the metadata arrives with the row, and
|
||
the markdown arrives on demand for the one reader that asks for it. With
|
||
``raiseload`` this would raise; with ``noload`` it would be ``None`` —
|
||
both would break the phase-118 backfill, and both are pinned out by
|
||
:func:`test_no_raiseload_or_noload_anywhere_in_the_importer`.
|
||
"""
|
||
source, rel = probe_row
|
||
|
||
doc = db.scalar(_doc_row_stmt(source, rel))
|
||
|
||
assert doc is not None
|
||
assert doc.file_hash == "fh"
|
||
assert doc.summary is None
|
||
assert doc.content == "the stored markdown body — phase 185"
|
||
# And the second read is served from the identity map, not the database.
|
||
assert doc.content == "the stored markdown body — phase 185"
|