phase: 148_pdf_pages_in_sources

**Phase 148 — final verification pass: complete, 100% green.** - **Verified (tasks 01–04 already implemented):** page render/persist at index (`_persist_page_copies`, fail-soft, stale-replace, qualifier-loss, prune hook via `_delete_page_copies`); public `GET /api/documents/{id}/pages/{n}` (uuid4-credential, 404-all-non-servable, `image/png` + cache header); `DocContent.page_urls` additive-omitted; shared `renderDocument` strip (lazy imgs, "Page N" captions, text + Summary suppressed, onerror note, createElement-only); migration 0026; `pdf_page_dir` setting; `.env.example` entry - **Completion criteria:** (1) pictorial + fallback classes get `p{N}.png` + `page_count`, text-heavy gets neither — pinned, pass; (2) unchanged re-sync renders zero (fast path) — pass; (3) changed re-index replaces copies, qualifier-loss/prune delete — pass; (4) route 404 matrix + payload key absent-never-null — pass; (5) modal AND /document.html strip with suppression, text-heavy byte-identical — E2E pass; (6) see below; (7) no completed-phase drift — full suite green - **Tests:** `uv run pytest --cov=app --cov-report=term-missing` → **3461 passed, 0 failed**, `app/` coverage **99%** (>90%); `uv run pytest tests/e2e/test_pdf_pages_in_sources.py -v --no-cov` → **4 passed** in isolation; `uv run ruff check .` clean; `uv run pyright` → 0 errors, 0 warnings - **Documentation:** README had **1 pre-existing inconsistency** — the Production Deployment section's standalone `podman run` example (and compose note) omitted the **required** `BOR_ADMIN_PASSWORD`/`BOR_SESSION_SECRET` (the fail-loud boot gate the README's own Admin section documents — the container would refuse to start); fixed with the two `-e` flags + a note for the compose path. All else verified consistent: 70/70 `.env.example` vars documented, all defaults match `config.py`, pages table matches routes, Containerfile/esbuild/uid-10001/healthcheck/entrypoint claims accurate, QA commands match pytest config. **AGENTS.md: verified consistent — no changes needed** - **Notable:** repo uses `Containerfile` (Podman) — README's `podman build` example is valid as-is; no live infrastructure touched - **Next pending phase:** `149_sync_progress_box`
2026-10-01 16:58:01 -04:00
parent 863519e980
commit b233c8d714
39 changed files with 3426 additions and 56 deletions
@@ -177,6 +177,29 @@ counters ``pdfs_pictorial`` / ``pdf_pictorial_pages`` are log-only
 (the sync ``detail`` API shape is untouched, the stats log-only
 convention).

+Persistent page copies (phase 148, task 01): in the PDF branch's
+NEW/CHANGED path — after the final content is decided (the
+fallback/pictorial passes have run) and after the row's flush (the
+doc has its id) — a page-QUALIFYING PDF (the phase-133 fallback
+fired, or the phase-140 pictorial pass described ≥1 page — the
+sync's own decisions: text-heavy PDFs stay ineligible, and the
+``pdf_pictorial`` kill switch keeps its semantics) gets EVERY page
+rendered at index time (:func:`app.rag.pdf.render_pdf_pages` — 150
+dpi, every page) and persisted to ``settings.pdf_page_dir``
+(``BOR_PDF_PAGE_DIR``, the ``image_dir`` convention) as
+``<doc-id>-p{N}.png`` (N 1-based), with ``documents.page_count``
+(migration 0026) recording the page count — a per-page render
+failure leaves a GAP (no file — the route 404s that N, the viewer's
+note covers it). The existing copies are deleted before replacement
+(a shrunk PDF's old ``p{N}`` tail cannot leak), a document that
+STOPS qualifying is cleared (``page_count`` NULL + copies deleted),
+and the prune deletes the copies with the row
+(:func:`_delete_page_copies`). FAIL-SOFT: a render or write failure
+leaves ``page_count`` NULL and logs a warning — the document indexes
+EXACTLY as pre-phase (the render is a display concern, it never
+gates the indexing). The UNCHANGED fast path renders nothing (the
+copies persist — zero render work, zero orphans).
+
 Image-anchor context (phase 125, task 03, LOCKED A1/A2/A3): BEFORE the
 description, the same source's importable markdown files are scanned for
 ``![alt](path)`` references that name the image (the task-02
@@ -851,6 +874,7 @@ async def import_sources(
                summary.pruned = _prune(
                    session, source_names, seen, images=llm.settings.images,
                    summary=summary,
+                    page_dir=Path(llm.settings.pdf_page_dir).expanduser(),
                )
        summary.embed_batches = llm.embed_batches
        summary.log()
@@ -1413,7 +1437,19 @@ async def _index_pdf_file(
      (:func:`_unchanged_doc`) — no re-extract, no re-embed, no image
      pass, the backfill/manual-date/date-refresh semantics in one
      code site — and the helper stamps the new file hash (the next
-      sync takes the fast path).
+      sync takes the fast path). This path renders NO pages and
+      touches NO ``page_count`` (the copies persist — zero render
+      work).
+    * Phase 148 (task 01): in this NEW/CHANGED path, RIGHT AFTER the
+      row's flush and BEFORE the chunk pipeline, a page-QUALIFYING
+      PDF (the phase-133 fallback fired, or the phase-140 pictorial
+      pass described ≥1 page — the signals recorded above) gets its
+      every page rendered to ``settings.pdf_page_dir`` as
+      ``<doc-id>-p{N}.png`` and ``documents.page_count`` set
+      (:func:`_persist_pdf_pages` — stale copies deleted first, a
+      stopped-qualifier cleared, a per-page ``b""`` a gap). FAIL-SOFT
+      — a render/write exception leaves ``page_count`` NULL + a
+      warning; the document indexes exactly as pre-phase.
    * THEN the embedded images: :func:`_index_pdf_embedded_images`
      (A3 — the phase-122 pipeline, gated by the ``images`` toggle).

@@ -1435,12 +1471,25 @@ async def _index_pdf_file(
    # Extract the text (fail-soft: an unopenable PDF yields "" — the
    # document may still carry embedded images worth indexing).
    content = extract_pdf_text(full_path)
+    # Phase 148 (task 01): the page-strip ELIGIBILITY signals — the
+    # LOCKED rule, recorded where the sync makes the decision: a PDF
+    # gets rendered pages iff the sync ALREADY decided its text is not
+    # the useful view — the phase-133 fallback fired (unusable text
+    # layer — the screenshot class) OR the phase-140 pictorial pass
+    # described ≥1 page (the sparse-page class — the IKEA fixture).
+    # Text-heavy PDFs (usable layer, zero candidates) stay ineligible
+    # (the text view), and ``pdf_pictorial=0`` keeps its kill-switch
+    # semantics (no pictorial pass → the sparse class stays ineligible
+    # — the pre-phase-148 viewer).
+    fallback_fired = False
+    pictorial_described = False
    # Phase 133 (task 03): the garbage gate — BEFORE the digest/chunk/
    # summary pipeline, so it runs on the FINAL content exactly once.
    # A clean layer never reaches the fallback (the gate is the only
    # gate — no fallback vision calls on the happy path).
    if not is_usable_pdf_text(content):
        summary.pdfs_screenshotted += 1
+        fallback_fired = True
        content = await _fallback_page_screenshots(
            session, source=source, rel=rel, full_path=full_path, llm=llm
        )
@@ -1457,6 +1506,7 @@ async def _index_pdf_file(
            source=source, rel=rel, full_path=full_path, llm=llm, summary=summary
        )
        if passed is not None:
+            pictorial_described = True
            content = passed
    if not content:
        logger.warning(
@@ -1515,6 +1565,35 @@ async def _index_pdf_file(

    session.flush()  # guarantees doc.id even for brand-new rows

+    # Phase 148 (task 01): the PERSISTENT PAGE COPIES — a qualifying
+    # PDF (the eligibility signals above) gets every page rendered to
+    # ``settings.pdf_page_dir`` (``<doc-id>-p{N}.png``) and
+    # ``documents.page_count`` set. RIGHT AFTER the flush (the doc has
+    # its id — the copy names need it) and BEFORE the chunk pipeline
+    # (the count commits ATOMICALLY with the doc + chunks). Runs ONLY
+    # in this new/changed path: the phase-147 fast path returned
+    # before the branch (an unchanged PDF re-renders NOTHING — the
+    # copies persist) and the content-digest hit above took the shared
+    # unchanged path (no page work there either). FAIL-SOFT: the
+    # render is a DISPLAY concern — any exception leaves
+    # ``page_count`` NULL, the warning is the signal, and the document
+    # proceeds to chunk/embed/summary EXACTLY as pre-phase.
+    try:
+        _persist_pdf_pages(
+            doc,
+            full_path=full_path,
+            page_dir=Path(llm.settings.pdf_page_dir).expanduser(),
+            eligible=fallback_fired or pictorial_described,
+            source=source,
+            rel=rel,
+        )
+    except Exception as e:
+        doc.page_count = None
+        logger.warning(
+            "import: PDF page render failed source=%s path=%s — %s",
+            source, rel, e,
+        )
+
    # Phase 1+2 — the shared chunk/replace/embed pipeline (the page-
    # aware chunker dispatches on the ``.pdf`` suffix; empty content
    # clears the chunk rows and returns 0), committed atomically with
@@ -1757,6 +1836,91 @@ async def _pictorial_pdf_pass(
    return "\n\n".join(parts)


+def _persist_pdf_pages(
+    doc: Document,
+    *,
+    full_path: Path,
+    page_dir: Path,
+    eligible: bool,
+    source: str,
+    rel: str,
+) -> None:
+    """Phase 148 (task 01): the persistent page copies of one NEW/CHANGED
+    PDF — the display strip's bytes (task 02's public route serves them).
+
+    * ELIGIBLE (the phase-133 fallback fired, or the phase-140
+      pictorial pass described ≥1 page — the sync's own decisions, see
+      the caller's signals): EVERY page of the PDF is rendered (150 dpi,
+      :func:`render_pdf_pages`' default — the phase-133 constant; the
+      ``pages=None`` "list index *i* is page *i*+1" contract) and each
+      NON-EMPTY PNG is written to *page_dir* as
+      ``<doc-id>-p{N}.png`` (N 1-based), then ``doc.page_count`` is set
+      to the PDF's page count (``len(pages)`` — one slot per page, so
+      the list IS the page count for an openable file). A per-page
+      ``b""`` (an unreadable page) is a GAP, not an error: no file is
+      written for it (the route 404s that N, the viewer's note covers
+      it), and the count still covers the full range.
+    * the STALE-REPLACEMENT convention (phase 122's image copies): the
+      doc's EXISTING copies (1..``doc.page_count`` — the row knows its
+      own extent, read BEFORE any change) are deleted FIRST, so a
+      changed PDF's old page set (e.g. 30 pages → 24) cannot leak
+      ``p25..p30``; a fresh doc's NULL count is a no-op.
+    * NOT eligible, or an unopenable file (:func:`render_pdf_pages` →
+      ``[]``), or a render with ZERO non-empty pages: ``page_count`` is
+      None + the existing copies deleted — a document that STOPPED
+      qualifying loses its strip (e.g. a new version gained a real text
+      layer).
+    * FAIL-SOFT: a write failure removes the copies THIS CALL wrote
+      (no partial-write orphans) and re-raises — the caller
+      (:func:`_index_pdf_file`) catches, leaves ``page_count`` NULL,
+      logs the warning, and the document indexes exactly as pre-phase
+      (the render is a display concern — it never gates indexing).
+
+    *page_dir* is ``Path(settings.pdf_page_dir).expanduser()`` (the
+    ``image_dir`` convention — raw string, expanduser'd by the
+    importer); the directory is created on demand, only when a page is
+    actually written (an ineligible doc never creates it).
+    """
+    # 1 — the stale-replacement: this doc's EXISTING copies go first —
+    # a changed PDF's old page set must not leak, and a doc that
+    # stopped qualifying loses its strip (a fresh doc's NULL count is
+    # a no-op; missing files are a silent no-op).
+    _delete_page_copies(doc, page_dir)
+    if not eligible:
+        doc.page_count = None
+        return
+    pages = render_pdf_pages(full_path)
+    if not pages or not any(pages):
+        # Unopenable file (the render's ``[]``) or every page a
+        # placeholder: no strip — fail-soft, the index is untouched
+        # (the render helper already logged the per-page failures).
+        logger.warning(
+            "import: PDF page render produced no pages source=%s path=%s",
+            source, rel,
+        )
+        doc.page_count = None
+        return
+    page_dir.mkdir(parents=True, exist_ok=True)
+    written: list[Path] = []
+    try:
+        for n, png in enumerate(pages, start=1):
+            if not png:
+                # A per-page render failure: the GAP (no file — the
+                # route 404s that N, the viewer's note covers it).
+                continue
+            path = page_dir / f"{doc.id}-p{n}.png"
+            path.write_bytes(png)
+            written.append(path)
+    except Exception:
+        # A partial write leaves no orphan: the copies this call wrote
+        # go, then the re-raise hands the failure to the caller's
+        # fail-soft handler (page_count NULL + the warning).
+        for path in written:
+            path.unlink(missing_ok=True)
+        raise
+    doc.page_count = len(pages)
+
+
 async def _index_pdf_embedded_images(
    session: Session,
    *,
@@ -2800,6 +2964,28 @@ def _delete_image_copy(image_path: str | None) -> None:
        logger.warning("import: could not delete image copy %s — %s", image_path, e)


+def _delete_page_copies(doc: Document, page_dir: Path) -> None:
+    """Best-effort removal of one document's stale PDF page copies
+    (phase 148, task 01 — the :func:`_delete_image_copy` convention):
+    the ``<doc-id>-p{N}.png`` files for N in 1..``doc.page_count`` —
+    the row knows its own extent (a NULL/zero count is a no-op, which
+    covers fresh docs, non-qualifying docs, and every fail-soft miss).
+
+    A missing file is a silent no-op (already gone — e.g. the owner
+    cleaned the pages dir); an unreadable one is logged, never raised —
+    copy cleanup must not break the sync (the doc row's fate is decided
+    by the upsert/prune logic, not by filesystem hygiene).
+    """
+    if not doc.page_count:
+        return
+    for n in range(1, doc.page_count + 1):
+        path = page_dir / f"{doc.id}-p{n}.png"
+        try:
+            path.unlink(missing_ok=True)
+        except OSError as e:
+            logger.warning("import: could not delete PDF page copy %s — %s", path, e)
+
+
 async def _index_image_file(
    session: Session,
    *,
@@ -3020,6 +3206,7 @@ def _prune(
    seen: set[tuple[str, str]],
    images: bool = False,
    summary: ImportSummary | None = None,
+    page_dir: Path | None = None,
 ) -> int:
    """Delete documents of *source_names* whose file is no longer in *seen*.

@@ -3050,6 +3237,14 @@ def _prune(
    appended BEFORE the ``session.delete`` — so the sync's incremental
    folder-summary regeneration (phase 127) sees the pruned docs' folders
    too. Callers passing nothing keep the pre-phase count-only contract.
+
+    *page_dir* (phase 148, task 01): when given (the sync's
+    ``Path(settings.pdf_page_dir).expanduser()``), every pruned doc's
+    PDF page copies (1..``doc.page_count`` — a NULL count is a no-op,
+    covering non-qualifying docs) are deleted beside the row and the
+    image copy — no orphans when a page-qualifying PDF (its parent or
+    a standalone doc) leaves the source. Callers passing nothing keep
+    the pre-phase copy-cleanup contract.
    """
    if not source_names:
        return 0
@@ -3069,6 +3264,11 @@ def _prune(
                    # invisible to the walk, not deleted.
                    continue
            _delete_image_copy(doc.image_path)
+            # Phase 148 (task 01): the page-qualifying doc's page
+            # copies go with the row (a pruned parent PDF's copies too
+            # — the same site as its embedded-image copies' deletion).
+            if page_dir is not None:
+                _delete_page_copies(doc, page_dir)
            if summary is not None:
                summary.changed_paths.append((doc.source, doc.path))
            session.delete(doc)