docs(phases): add phase 163 (additive PDF strip view, write-first atomic copies, fast-path self-heal)
Captures the 2026-10-07 live findings on Letumdei_ Heirs to the Imperium: a single sparse map page flipped a text PDF into the strip-only viewer, and the strip's page copies were lost from disk while page_count stayed stamped (no self-heal path). Owner decisions recorded: additive text+Pages toggle for the pictorial class, write-before-delete atomic persist, deterministic zero-LLM fast-path re-gate. LLM-Generated: true
This commit is contained in:
@@ -0,0 +1,73 @@
|
||||
# Phase 163 — PDF page strip: additive text+pages view, write-first atomic copies, deterministic fast-path re-gate (self-heal)
|
||||
|
||||
**Source:** owner live report, 2026-10-07 — brain.reeseapps.com, the document modal for `Letumdei_ Heirs to the Imperium/Letumdei_ Heirs to the Imperium.pdf` (doc id `b72f5515-dc40-4167-98ff-179a6503e6e5`). Every page rendered "Page N unavailable." and the document was unreadable in the viewer. **Pre-fix evidence (measured live, 2026-10-07):** the payload carries 36 `page_urls` (so `page_count = 36`); `GET /api/documents/<id>/pages/1…36` **all 404**; the content is genuine extracted text (114 389 chars, 36 `---PAGE-N---` markers, exactly ONE `[Page 36 illustration: …]` block — a map page under `pdf_pictorial_min_chars`); the same server serves the 10/3-indexed IKEA manual's pages 200 (the pages dir was NOT wiped — only this doc's copies are gone). Two stacked defects:
|
||||
|
||||
1. **Eligibility consequence (the "shouldn't be a pictorial pdf" complaint).** The phase-140 gate is per-page (`len(text.strip()) < pdf_pictorial_min_chars`, default 100); ONE sparse page in a text-heavy 36-page book set `pictorial_described = True` → the doc joined the strip class → the phase-148 viewer renders the strip **INSTEAD OF** the text content and the Summary panel. The owner's call (2026-10-07): **additive strip** — a usable-layer (pictorial-class) doc shows its TEXT (plus the Summary panel, parity with any text doc) with a **Text/Pages toggle** for the strip; the text is never hidden. The fallback class (unusable text layer — the phase-133 screenshots ARE the content) keeps today's strip-only view, DOM byte-identical.
|
||||
2. **Lost copies, stale `page_count`, no self-heal.** `_persist_pdf_pages` **deletes the old copies from disk first**, and the filesystem work is NOT transactional with the DB session: any later rollback of the file's transaction (the per-file `EmbeddingError` handler, a killed sync) — or an external wipe of `pdf_page_dir` — leaves the row claiming `page_count = 36` over ZERO files. The phase-147 fast path ("the copies persist") never re-renders, so the strip 404s forever. The owner's call: **write-before-delete + fast-path re-gate** — the changed path writes the new copies (atomically, per file) BEFORE any deletion; the unchanged paths deterministically re-gate the strip (no LLM/vision call) — re-stamp the class, re-render missing copies, or clear a strip that no longer qualifies.
|
||||
|
||||
**Story:** n/a (owner-directed bug fix; extends `148_pdf_pages_in_sources`, `147_sync_changed_only`, `140_pictorial_pdf_pages`, `133_pdf_screenshot_fallback`).
|
||||
|
||||
## Objective
|
||||
A pictorial-class PDF is a **readable text document with the pages one toggle away** (Text default; Pages shows the phase-148 strip); a fallback-class PDF keeps the strip-only view byte-identically; and a strip never dies silently — the next sync **re-gates and re-renders** missing copies (or clears a strip that no longer qualifies) with zero LLM calls. The live `Letumdei` doc is repaired by the next production Sync (no manual script): additive mode stamped, 36 copies re-rendered, the book's text back in the viewer.
|
||||
|
||||
## Dependencies
|
||||
- `148_pdf_pages_in_sources` (complete) — `page_count`, `pdf_page_dir`, `<doc-id>-p{N}.png`, the `/pages/{n}` route, the viewer's `docPagesStrip`, and the persist/regate seams this phase changes.
|
||||
- `147_sync_changed_only` (complete) — the `file_hash` fast path + shared `_unchanged_doc` (the re-gate hook site) whose "the copies persist" assumption this phase replaces.
|
||||
- `140_pictorial_pdf_pages` (complete) — the deterministic `pictorial_pages` gate + the kill switch `settings.pdf_pictorial` (the re-gate mirrors this gate exactly).
|
||||
- `133_pdf_screenshot_fallback` (complete) — `is_usable_pdf_text` (the fallback-class signal the re-gate mirrors).
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
|
||||
**D1 — the strip-mode wire signal (task 01, LOCKED A2/A3).** New nullable `documents.page_strip_mode` VARCHAR(16) (migration **0027**, tested downgrade, A13): `"primary"` = fallback class (strip replaces the text — today's behavior), `"additive"` = pictorial class (text + toggle). Set exactly when `page_count` is set (cleared with it). `DocContent` gains `page_strip_mode: str | None = None` with the `image_url` omission rule: **ABSENT (never null) when `page_count` is NULL** — every non-qualifying doc's payload stays byte-identical to pre-phase. For a qualifying doc the key is present; a **NULL-mode pre-phase row serializes ABSENT** and the viewer treats absent as `"primary"` (existing rows keep today's DOM until the re-gate stamps them). The `/pages/{n}` route is untouched.
|
||||
|
||||
**D2 — write-first atomic persist (task 02, LOCKED A4).** `_persist_pdf_pages` reorders: **render → write every page (atomic per file: write to `<name>.png.tmp`, `os.replace`) → only then delete stale copies** outside the written set (the shrunk-PDF tail; the current blanket `_delete_page_copies` BEFORE the render is removed — a rollback or crash can no longer leave the row over zero files). The partial-write failure path unlinks BOTH the new final files and the `.tmp` files before re-raising (no orphans; the existing fail-soft contract — caller sets `page_count` NULL + warning — unchanged). The function also stamps `doc.page_strip_mode`: `"primary"` when `fallback_fired`, `"additive"` when `pictorial_described` (the caller passes the CLASS, not just the boolean — `eligible=fallback_fired or pictorial_described` is replaced by `strip_class="fallback"|"pictorial"|None`); the ineligible arm clears mode with `page_count`. **The eligibility RULE itself is unchanged** (the phase-148 lock `fallback_fired or pictorial_described` stands — only its VIEW consequence changes, via D1/D4).
|
||||
|
||||
**D3 — the deterministic fast-path re-gate (task 03, LOCKED A5).** A new `_regate_pdf_strip(session, doc, *, full_path, llm, summary)` runs on BOTH unchanged paths — inside `_unchanged_doc` (an optional `full_path` kwarg, defaulting to `Path(doc.full_path)`), so it fires on the `file_hash` fast path AND the content-digest-hit path — guarded by `doc.page_count is not None` (only PDF rows ever carry it; every other file's fast path keeps its zero-extra-read contract). It mirrors the changed-path gate **deterministically from the current file bytes + current settings — ZERO vision/LLM calls** (`render_pdf_pages` is CPU-only, A21-safe):
|
||||
- `not is_usable_pdf_text(extract_pdf_text(full_path))` → class `fallback` (mode `primary`) — same doc as today;
|
||||
- elif `settings.pdf_pictorial` and `pictorial_pages(extract_pdf_page_texts(full_path), min_chars=settings.pdf_pictorial_min_chars)` is non-empty → class `pictorial` (mode `additive`) — **candidates are exactly the existing gate's** (text-carrying pages under `min_chars`; no-text pages are NOT candidates, mirroring `pictorial_pages` — the re-gate adds no new semantics);
|
||||
- else → **repair arm**: the strip no longer qualifies (e.g. the owner lowered `pdf_pictorial_min_chars`) → `page_count = None`, `page_strip_mode = None`, `_delete_page_copies(doc, page_dir)`.
|
||||
|
||||
For a re-gated-eligible doc, **verify the copies**: any of `page_dir/<doc.id>-p1..p{page_count}.png` missing → re-render ALL pages (`render_pdf_pages`), atomic write-first (D2's writer), and set `page_count` to the fresh render's length (a drift names the doc in a warning; bytes unchanged makes drift pathological). A fresh render producing NO pages (unopenable despite the just-computed hash — pathological) keeps the row as-is with a warning (fail-soft, never clears on a transient). One greppable INFO line whenever re-gate ACTS (never per-unchanged-doc): `import: PDF strip re-gate source=… path=… — healed N pages | cleared stale strip | mode <old>-><new>`. The re-gate mutates the row INSIDE the unchanged path's transaction (the commit already owned by `_unchanged_doc`'s caller flow); a copy-verify/render exception is caught, warned, and never fails the sync.
|
||||
|
||||
**D4 — the additive viewer (task 04, LOCKED A1/A6).** `renderDocument` (the ONE shared core — modal AND `/document.html`): the strip branch now keys on the mode —
|
||||
- `page_strip_mode` absent or `"primary"` (with `page_urls`): the EXACT current DOM (`.doc-pages` strip, no toggle, no Summary panel) — byte-identical, pinned;
|
||||
- `"additive"` (with `page_urls`): the text doc renders **exactly as a non-qualifying text doc** (Summary panel + content, the pre-148 branch reused), PLUS a `Text`/`Pages` toggle above the content: `<div class="doc-view-toggle" role="group" aria-label="Document view">` with two `<button type="button" aria-pressed>`; **default `Text`** (LOCKED A6 — no sparse-ratio default flip); switching to `Pages` appends/reveals the `docPagesStrip` (imgs `loading="lazy"` — hidden-until-shown never fetches) and hides the text block; switching back reverses. State is per-render (no persistence). createElement + properties/textContent only (the never-build-HTML rule); styles.css gains `.doc-view-toggle` per the house button idiom (focus-visible ring, ≥4.5:1, pressed state is text + style, never color alone — B5).
|
||||
- no `page_urls`: byte-identical today's text doc (untouched).
|
||||
|
||||
**D5 — the live repair (task 05, records; no code).** With D3 shipped, the next production Sync takes the `Letumdei` doc's unchanged fast path → re-gate: usable text layer + ≥1 sparse candidate (page 36) → mode `additive` stamped → all 36 copies missing → re-rendered → the viewer shows the book's text with the Pages toggle. The IKEA-class doc (bytes unchanged, copies present, mode NULL pre-phase) gets mode `additive` stamped with zero render work. The E2E fixture reproduces the exact live shape (one illustration block on the last page, copies deleted out from under a stamped `page_count`).
|
||||
|
||||
**Invariants (all tasks).** The changed-path eligibility rule (`fallback_fired or pictorial_described`), every vision pass, the `/pages/{n}` route, the fallback-class strip DOM, the non-qualifying payloads, all SSE frame shapes, the mock-LLM markers, and the prune (`_delete_page_copies` at prune time) are byte-identical. `PLAN.md` untouched (the §7 viewer line describing the strip is folded at the next redo; AGENTS.md's viewer bullets get the additive note in the final pass).
|
||||
|
||||
## Tasks
|
||||
1. `01_schema_wire_strip_mode.md` — the `page_strip_mode` column + migration 0027 + the `DocContent` field + the payload builder + unit pins (D1).
|
||||
2. `02_persist_write_first.md` — `_persist_pdf_pages` write-before-delete + atomic replace + the `strip_class` stamp (D2).
|
||||
3. `03_fast_path_regate.md` — `_regate_pdf_strip` on the unchanged paths: mirror gate, heal, repair, log line (D3).
|
||||
4. `04_viewer_additive_toggle.md` — `renderDocument` mode branch + Text/Pages toggle + styles + frontend unit pins (D4).
|
||||
5. `05_e2e_selfheal_pins.md` — the dedicated Playwright suite (additive view, primary byte-identity, heal, repair arm), the final gates, the docs pass (D5 + gates).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: `tests/unit/test_pdf_page_copies.py` (D2: write-first order, atomic replace, partial-write cleanup, the class stamp; D1: column defaults), `tests/unit/test_importer_unchanged_fast_path.py` (D3: every re-gate arm incl. kill-switch, heal, repair, fail-softs, the zero-extra-read guard for non-strip docs), `tests/unit/test_docs_api.py` or the phase-148 payload-pin file (D1: the key present/absent rule, the byte-identity of non-qualifying payloads), a frontend source-contract file per the house style (D4: the mode branch, the toggle semantics, the never-build-HTML rule, the primary-DOM byte-identity).
|
||||
- E2E: `tests/e2e/test_pdf_strip_additive_selfheal.py`, **in isolation** (`uv run pytest tests/e2e/test_pdf_strip_additive_selfheal.py -v --no-cov`); `test_pdf_pages_in_sources.py` / `test_pictorial_pdf.py` / `test_garbage_pdf.py` stay green (the re-cut pins included).
|
||||
- Coverage: **>90%** on `app/`; `uv run ruff check . && uv run pyright` clean.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] A pictorial-class PDF opens in the viewer showing its text + Summary panel with a Text/Pages toggle (Text pressed by default); Pages shows the strip and every page image loads (200); a fallback-class PDF's strip-only DOM is byte-identical to pre-phase (no toggle).
|
||||
- [ ] The changed path writes copies before deleting any (measured by the unit order pin); a mid-write failure leaves the prior copies intact and no orphans.
|
||||
- [ ] Deleting a strip's copies on disk + a re-sync (unchanged bytes) restores them and stamps the mode (the E2E heal test); a stale `page_count` on a now-ineligible doc is cleared on re-sync with the copies removed (the E2E repair test); zero vision/LLM calls on the fast path (asserted via the mock).
|
||||
- [ ] Non-qualifying-doc payloads and the primary-class DOM are byte-identical; the full `uv run pytest` suite is green; coverage >90%; `uv run ruff check . && uv run pyright` clean.
|
||||
- [ ] `README.md` + `AGENTS.md` reviewed in full in the final pass (the viewer description gains the additive-mode sentence; `PLAN.md` untouched per the invariants).
|
||||
- [ ] One `--no-gpg-sign` commit; the phase dir moves to `complete/` by the pipeline gate. Post-deploy (owner runtime, recorded here): the next production Sync self-heals `Letumdei` per D5 — no manual script.
|
||||
|
||||
## Locked decisions
|
||||
- **A1 — additive strip**: usable-layer (pictorial-class) PDFs show text + Summary + a Text/Pages toggle; the text is NEVER hidden for them; fallback-class keeps the strip-only view byte-identical (owner-confirmed 2026-10-07 — supersedes phase-148's "strip instead of the text content" for the pictorial class only).
|
||||
- **A2 — the wire signal**: `page_strip_mode: "primary"|"additive"` ABSENT (never null) when `page_count` is NULL; absent ⇒ viewer treats `"primary"`; non-qualifying payloads byte-identical (owner-accepted default, 2026-10-07).
|
||||
- **A3 — the column**: `documents.page_strip_mode` VARCHAR(16) nullable, migration 0027 with a tested downgrade (A13); stamped/cleared exactly with `page_count`.
|
||||
- **A4 — write-first atomic copies**: render → atomic per-file write (tmp + `os.replace`) → stale-tail delete; a rollback can never leave the row over zero files; the fail-soft contract unchanged (owner-confirmed 2026-10-07).
|
||||
- **A5 — the deterministic re-gate**: unchanged paths mirror the changed-path gate exactly (`is_usable_pdf_text` / `pictorial_pages` + the `pdf_pictorial` kill switch, current settings), re-render missing copies (CPU-only, no vision), or clear a stale strip; only rows with `page_count` pay the extra PDF parse; fail-soft on any re-gate exception (owner-confirmed 2026-10-07).
|
||||
- **A6 — default view = Text** for additive docs (no sparse-ratio default flip; the illustration descriptions already live in the text via phase 140) (owner-accepted default, 2026-10-07).
|
||||
- **A7 — the changed-path eligibility RULE is unchanged** (`fallback_fired or pictorial_described`, phase-148 lock intact); this phase changes only the persist ordering, the recovery, and the view consequence.
|
||||
|
||||
## Commit
|
||||
```bash
|
||||
git add app/ frontend/ tests/ alembic/ .agents/phases/ && git commit --no-gpg-sign -m "fix(pdf): additive text+pages viewer for pictorial PDFs, write-first atomic page copies, and a deterministic fast-path re-gate that self-heals lost strip images"
|
||||
```
|
||||
@@ -0,0 +1,26 @@
|
||||
# Task 01 — The `page_strip_mode` column + wire signal (phase-163 D1, LOCKED A2/A3)
|
||||
|
||||
**Phase:** `163_pdf_strip_additive_selfheal` · **Source:** phase overview D1 — the live modal showed the strip replacing the text of a genuinely textual book; the viewer needs an explicit class signal (fallback class = strip primary; pictorial class = additive) that never disturbs non-qualifying payloads.
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
`documents.page_strip_mode` (VARCHAR(16), nullable, migration 0027 with a tested downgrade) exists and `DocContent` carries `page_strip_mode` with the `image_url` omission rule — ABSENT (never null) when `page_count` is NULL, present for qualifying docs — so the frontend (task 04) can branch on the class, and every non-qualifying doc's payload stays byte-identical to pre-phase.
|
||||
|
||||
## Work
|
||||
1. `app/models.py` — `Document.page_strip_mode: Mapped[str | None]` (VARCHAR(16), nullable, default NULL), docstring right after `page_count` (phase 148's column): the strip CLASS — `"primary"` = the phase-133 fallback class (the strip IS the view — the screenshots are the content), `"additive"` = the phase-140 pictorial class (the text view + the strip behind a toggle); NULL = no strip. Stamped/cleared exactly with `page_count` (tasks 02/03 own the writes; this task only adds the column).
|
||||
2. `alembic/versions/0027_documents_page_strip_mode.py` — add the column; downgrade drops it. Follow `0026_documents_page_count.py` exactly (revision naming, `nullable=True`, no index). `uv run alembic upgrade head && uv run alembic downgrade -1 && uv run alembic upgrade head` round-trips.
|
||||
3. `app/schemas.py` — `DocContent` gains `page_strip_mode: str | None = None` beside `page_urls`, docstring: the phase-163 strip class; ABSENT (never null — the `image_url` omission rule, `exclude_none`-style serialization already in place for `image_url` — verify how `DocContent` drops `image_url=None` and apply the SAME mechanism) when there is no strip; `"primary"`/`"additive"` otherwise. A pre-phase row (column NULL while `page_count` is set) serializes ABSENT — the viewer treats absent as `"primary"` (task 04).
|
||||
4. `app/api/docs.py` — the content payload builder (the `page_urls=(…)` block, ~line 250): pass `page_strip_mode=doc.page_strip_mode` (the None-drops-the-key serialization handles the absent rule). The `/documents/{doc_id}/pages/{n}` route is NOT touched.
|
||||
5. Unit tests — extend the phase-148 payload-pin file (find where `page_urls` serialization is pinned — `tests/unit/test_docs_api.py` or `test_pdf_page_copies.py`):
|
||||
- a qualifying doc with mode set: the serialized payload carries `"page_strip_mode": "primary"` / `"additive"` next to `page_urls`;
|
||||
- a doc with `page_count` set and mode NULL (pre-phase row): the key is ABSENT (not null) and `page_urls` still present — the exact live `Letumdei` state;
|
||||
- a non-qualifying doc (`page_count` NULL): NO `page_strip_mode` and NO `page_urls` keys — pin the payload byte-identity against the current expected dict;
|
||||
- `page_strip_mode` never appears alone: with `page_count` NULL it is absent even if the column somehow holds a value (the builder keys the whole strip block off `page_count` — if the current builder does NOT, make it so).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the cases above; the existing `page_urls` pins stay green unmodified except where a test asserts an exact payload dict (then re-cut in this task). Coverage **>90%** on `app/`.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run alembic upgrade head` applies 0027; downgrade removes the column; upgrade again is clean.
|
||||
- [ ] The three payload shapes (mode set / mode NULL with strip / no strip) serialize exactly per D1, pinned by unit tests.
|
||||
- [ ] Full `uv run pytest` green; `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -0,0 +1,32 @@
|
||||
# Task 02 — Write-first atomic page-copy persist + the strip-class stamp (phase-163 D2, LOCKED A4/A7)
|
||||
|
||||
**Phase:** `163_pdf_strip_additive_selfheal` · **Source:** phase overview D2 — the live loss: the changed path deletes the old copies BEFORE rendering, outside the DB transaction, so any later per-file rollback (the `EmbeddingError` handler in the import loop) restores the stale `page_count` over ZERO files on disk (filesystem ops are not transactional). External wipes (`pdf_page_dir` cleaned) land the same state; the heal is task 03.
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
`_persist_pdf_pages` never removes a live copy before the replacement bytes are durably written: render → atomic per-file write (tmp + `os.replace`) → stale-tail delete, and the function stamps `page_strip_mode` from a `strip_class` keyword instead of the bare `eligible` boolean — the eligibility RULE (`fallback_fired or pictorial_described`, phase-148 lock) is unchanged at the call site.
|
||||
|
||||
## Work
|
||||
1. `app/rag/importer.py` — `_persist_pdf_pages(doc, *, full_path, page_dir, strip_class, source, rel)` (replaces the `eligible: bool` keyword):
|
||||
- `strip_class` is `"fallback" | "pictorial" | None` (the caller passes the CLASS — see 2).
|
||||
- `strip_class is None`: clear BOTH `doc.page_count = None` and `doc.page_strip_mode = None`, THEN `_delete_page_copies(doc, page_dir)` (the deletion-first order is correct HERE — there is no replacement set to protect; note the helper reads `doc.page_count`, so call it BEFORE nulling or pass the old count — check the current order and keep the deletion actually deleting: today the helper runs FIRST while the count is still set; preserve that behavior).
|
||||
- eligible: `render_pdf_pages(full_path)` → the no-pages guard unchanged (`page_count`/mode NULL, warning) → `page_dir.mkdir` → **write loop FIRST**: every non-empty page to `<doc-id>-p{n}.png.tmp` then `os.replace` onto the final name (atomic per file — a reader never sees a torn PNG); collect the written names → **then** delete the stale set: `for n in range(1, doc.page_count + 1)` beyond/present-but-not-written → unlink (the shrunk-PDF tail; reuse `_delete_page_copies` semantics by deleting the old extent files that are NOT in the written set — a file about to be overwritten is not deleted at all).
|
||||
- the failure path: on any exception in the render/write, unlink this call's `.tmp` AND final-name files it wrote (track both), then re-raise (the caller's fail-soft handler — `page_count` NULL + warning — is extended to also null `page_strip_mode`). The OLD copies are untouched by a failed call (they were never deleted up-front — the crash/rollback orphan state is structurally impossible now).
|
||||
- success tail: `doc.page_count = len(pages)` AND `doc.page_strip_mode = "primary"` if `strip_class == "fallback"` else `"additive"`.
|
||||
- docstring: the write-first rationale (the 2026-10-07 live loss: delete-first + a later rollback = the row over zero files) + the per-file atomicity note.
|
||||
2. `app/rag/importer.py` — the `_index_pdf_file` call site (~line 1671): `strip_class = "fallback" if fallback_fired else ("pictorial" if pictorial_described else None)` passed as the keyword; the `eligible=fallback_fired or pictorial_described` expression is GONE (the class carries the same truth table — pin it in the docstring: class non-None ⇔ eligible, the phase-148 rule verbatim). The `except Exception` fail-soft handler nulls mode too.
|
||||
3. `tests/unit/test_pdf_page_copies.py` — extend (the file's existing fixture pattern):
|
||||
- **the order pin**: patch `_delete_page_copies`-class file ops / use a real tmp dir — after a successful re-render of a CHANGED doc, the new files exist and the stale tail (old count > new count) is gone; the overwritten pages keep the NEW bytes;
|
||||
- **the failure pin**: a render that raises mid-write (the 2nd of 3 pages) → the FIRST render's old copies still on disk (the rollback-orphan state is unrepresentable), no `.tmp`/partial orphans from this call, the caller's fail-soft fires (`page_count`/mode NULL + the warning);
|
||||
- **the atomic pin**: the writer uses tmp + `os.replace` (spy or a failing-replace test — a reader opening the final path never sees truncated bytes; assert the tmp name pattern is used and `os.replace` is called);
|
||||
- **the class stamp**: fallback → `page_strip_mode == "primary"`; pictorial → `"additive"`; ineligible → both NULL and old copies deleted;
|
||||
- the existing phase-148 pins (gap = per-page `b""` leaves no file and counts in `page_count`; the shrunk tail) stay green, re-cut only where the deletion order changes expectations.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the pins above; coverage **>90%** on `app/`.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] A successful changed-path render: new copies durable BEFORE any old copy is removed (measured by the order pin); the shrunk tail still cannot leak.
|
||||
- [ ] A mid-write failure leaves the prior copies intact and zero orphans (files on disk exactly the prior set), `page_count`/`page_strip_mode` NULL, sync continues (the fail-soft unchanged).
|
||||
- [ ] `strip_class` carries the exact phase-148 eligibility truth table; no call site passes a bare `eligible` anymore.
|
||||
- [ ] `uv run pytest tests/unit/test_pdf_page_copies.py -v` green; full suite green; `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -0,0 +1,37 @@
|
||||
# Task 03 — The deterministic fast-path re-gate: heal & repair (phase-163 D3, LOCKED A5)
|
||||
|
||||
**Phase:** `163_pdf_strip_additive_selfheal` · **Source:** phase overview D3 — with the copies lost (external wipe, or the pre-phase-163 delete-first window), the phase-147 fast path assumes "the copies persist" and NEVER re-renders: the live `Letumdei` row (36 `page_urls`, zero files, unchanged bytes on every sync since) is the proof. The unchanged paths must re-gate the strip deterministically — no LLM/vision calls — heal missing copies, and clear strips that no longer qualify.
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
`_regate_pdf_strip(session, doc, *, full_path, llm, summary)` runs on BOTH unchanged paths (the `file_hash` fast path and the content-digest-hit path), guarded by `doc.page_count is not None`: it mirrors the changed-path gate from the current bytes + settings, re-stamps `page_strip_mode`, re-renders when any copy is missing, or clears a stale strip — one greppable INFO line only when it ACTS, fail-soft on every exception.
|
||||
|
||||
## Work
|
||||
1. `app/rag/importer.py` — new `_regate_pdf_strip(session, doc, *, full_path, llm, summary) -> None`:
|
||||
- **mirror the changed-path gate exactly** (the truth table in the phase overview D3):
|
||||
`text = extract_pdf_text(full_path)`; `is_usable = is_usable_pdf_text(text)`;
|
||||
- `not is_usable` → class `fallback` (mode `"primary"`) — the phase-133 signal verbatim;
|
||||
- elif `llm.settings.pdf_pictorial` and `pictorial_pages(extract_pdf_page_texts(full_path), min_chars=llm.settings.pdf_pictorial_min_chars)` non-empty → class `pictorial` (mode `"additive"`) — **the existing gate verbatim** (text-carrying pages under `min_chars` only; no-text pages are NOT candidates — `pictorial_pages`'s own contract; the re-gate adds no new eligibility semantics);
|
||||
- else → **repair arm**: `_delete_page_copies(doc, page_dir)` (while `page_count` is still set — the helper reads it), then `doc.page_count = None`, `doc.page_strip_mode = None`; INFO `import: PDF strip re-gate source=… path=… — cleared stale strip`.
|
||||
- **mode re-stamp** (eligible arms): a mode change (NULL→stamped, or the owner's kill-switch/class flip) writes `doc.page_strip_mode` and, when it changes an already-set mode, INFO `… mode <old>-><new>`; a NULL→stamp on existing rows is silent-beneficial (fold into the same line only when work follows).
|
||||
- **copy verify + heal** (eligible arms): `page_dir = Path(llm.settings.pdf_page_dir).expanduser()`; missing = any of `page_dir/f"{doc.id}-p{n}.png"` for `n in 1..doc.page_count` absent. Any missing → `render_pdf_pages(full_path)`: the no-pages result (unopenable — pathological right after the fast path hashed the file) keeps the row untouched + WARNING `import: PDF strip re-gate source=… path=… — re-render produced no pages (state kept)`; else WRITE EVERY page via task-02's atomic writer (tmp + `os.replace`) — reuse it (extract the shared writer helper from task 02 rather than duplicating), then delete the stale tail beyond the new count; `doc.page_count = len(pages)` — a drift from the old count adds a WARNING naming both counts; INFO `… healed N pages`.
|
||||
- **fail-soft**: the whole body in `try/except Exception` → `logger.warning` (the re-gate never fails a sync; the row keeps its pre-re-gate state — but note the repair arm's DB writes and the heal's row writes are in the caller's transaction, so an exception AFTER them rolls back naturally with the file anyway).
|
||||
- docstring: the 2026-10-07 live state as the rationale; the zero-LLM-call property (render is CPU — A21-safe); the guard means every non-strip file's fast path keeps its zero-extra-read contract.
|
||||
2. `app/rag/importer.py` — `_unchanged_doc(...)`: gains the optional keyword `full_path: Path | None = None` (defaults to `Path(doc.full_path)`); at its tail, `if doc.page_count is not None and str(doc.path).lower().endswith(".pdf"): _regate_pdf_strip(session, doc, full_path=…, llm=llm, summary=summary)`. Call sites: the `_index_file` fast path passes the live `full_path`; the branches' content-digest-hit calls pass their own `full_path` (every call site already has it). Docstring: the phase-147 "the copies persist" paragraph is replaced — "unchanged files re-gate the PDF strip deterministically (phase 163)".
|
||||
3. `tests/unit/test_importer_unchanged_fast_path.py` — extend (the file's fast-path fixture pattern; the mock embedder asserts ZERO chat/vision calls across every case):
|
||||
- **heal**: a stamped strip doc (`page_count=3`, mode NULL or set), copies deleted out from under it, unchanged bytes → sync → the 3 files exist (PNG bytes), mode stamped `"additive"` (usable layer + sparse candidate), INFO `healed 3 pages`, `page_count` unchanged (no drift);
|
||||
- **fallback heal**: an unusable-layer doc → mode `"primary"`, copies restored;
|
||||
- **no-op**: all copies present, mode set → zero file writes, zero INFO lines (pin "INFO never fires per unchanged doc" — assert the log records contain no re-gate line);
|
||||
- **repair**: a row with `page_count` set (copies present) on a PDF whose gate now says ineligible (clean text, zero sparse pages) → cleared (both columns NULL, copies deleted), INFO `cleared stale strip`;
|
||||
- **kill switch**: `pdf_pictorial=False` + pictorial-class doc → repair arm clears the strip (the changed-path semantics mirrored);
|
||||
- **fail-soft**: `render_pdf_pages` patched to raise → sync completes, row keeps its prior state, WARNING, no copies touched;
|
||||
- **guard**: a non-PDF unchanged file and a PDF with `page_count` NULL → `extract_pdf_text` NEVER called (spy) — the zero-extra-read contract intact.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the cases above; coverage **>90%** on `app/`.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] Unchanged re-sync restores deleted copies (files + row + route-serveable) and stamps the mode — the live-loss state cannot persist past one sync (unit-proven; the E2E pins it in task 05).
|
||||
- [ ] A stale strip on a now-ineligible doc (incl. via the kill switch) is cleared with its copies; the re-gate fires ZERO LLM/vision calls and no INFO when there is nothing to do.
|
||||
- [ ] Non-strip fast paths perform zero PDF parsing (spy-pinned); every re-gate exception is fail-soft.
|
||||
- [ ] `uv run pytest tests/unit/test_importer_unchanged_fast_path.py tests/unit/test_pdf_page_copies.py -v` green; full suite green; `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -0,0 +1,35 @@
|
||||
# Task 04 — The additive viewer: text + Summary + the Text/Pages toggle (phase-163 D4, LOCKED A1/A6)
|
||||
|
||||
**Phase:** `163_pdf_strip_additive_selfheal` · **Source:** phase overview D4 — the live complaint: "Notice how all the pages are unavailable… it shouldn't be a pictorial pdf." One short map page flipped a 114 KB-text book into the phase-148 strip-only view (the strip renders INSTEAD of the text content AND the Summary panel). The owner's call: additive for the pictorial class — the text is never hidden; the strip is a toggle away.
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
`renderDocument` (the ONE shared core — the Sources modal AND `/document.html`) branches on `doc.page_strip_mode`: absent/`"primary"` keeps the current strip-only DOM byte-identically; `"additive"` renders the doc exactly as a text doc (Summary panel + content) plus a `Text`/`Pages` toggle (default `Text`) revealing the existing `docPagesStrip`.
|
||||
|
||||
## Work
|
||||
1. `frontend/assets/document.js` — the `renderDocument` strip branch (~line 374, `const pageUrls = Array.isArray(doc.page_urls) ? doc.page_urls : null;`):
|
||||
- `pageUrls` truthy AND `doc.page_strip_mode === "additive"`: fall through to the EXISTING text-doc rendering (the pre-148 branch — Summary panel + content, reused unchanged), THEN append the view toggle + the strip container:
|
||||
- `<div class="doc-view-toggle" role="group" aria-label="Document view">` + two `<button type="button" class="doc-view-btn" aria-pressed>` — labels `Text` / `Pages`; initial state `Text` pressed (`"true"`), the strip NOT in the content flow;
|
||||
- `Pages` click: `aria-pressed` flips; the text block + Summary panel hide (a class toggle, e.g. a `.doc-views[data-view="pages"]` wrapper — CSS-driven, never re-render), the `docPagesStrip(doc, pageUrls)` (the EXISTING builder, unmodified — lazy imgs never fetch while hidden) is appended once and shown;
|
||||
- `Text` click reverses (the strip node stays mounted — no re-fetch on flip-flop);
|
||||
- structure: wrap the text view + the strip in one `.doc-views` container appended to `contentEl` (both surfaces get it via the shared core; the wrapper exists ONLY on additive docs — every other doc's DOM is untouched).
|
||||
- `pageUrls` truthy AND mode absent or `"primary"`: the CURRENT branch verbatim (strip only, no Summary panel, no toggle) — byte-identical DOM, pinned;
|
||||
- falsy `pageUrls`: untouched.
|
||||
- the never-build-HTML rule: createElement + properties/textContent only (the labels are fixed strings — no doc-derived text inside the toggle); the `strip_mode` read is `doc.page_strip_mode === "additive"` (anything else ⇒ primary — the tolerant-reading house rule).
|
||||
- the module docstring: the phase-163 paragraph (the additive class contract, LOCKED A1/A6) replacing the phase-148 "INSTEAD of the text content AND the .doc-summary panel" sentence's scope (it now reads: primary class only).
|
||||
2. `frontend/assets/styles.css` — `.doc-view-toggle` (house button idiom beside the existing segmented/pill controls — check `.format-badge`/the modal action buttons for the closest idiom): the two buttons adjacent, pressed state via `aria-pressed="true"` styling (weight/background pair — **text + style, never color alone**, B5), `:focus-visible` 3px ring, ≥4.5:1 text contrast on both states, ≥44px touch targets on mobile (§7.1); `.doc-views[data-view="pages"]` hides the text block + Summary panel and shows the strip; default (`data-view="text"`) hides the strip container entirely.
|
||||
3. Unit (frontend source-contract — the house pattern; extend the file that already pins the phase-148 strip DOM — locate it via `docPagesStrip` in `tests/unit/`, likely `test_pdf_page_copies.py` / a viewer-contract file):
|
||||
- the additive branch builds the `.doc-views` wrapper + the toggle with the exact labels/roles/`aria-pressed` initial state (source-regex or the file's established DOM-assertion style);
|
||||
- the primary/absent branch contains NO `doc-view-toggle` string and the strip-only structure is byte-identical to the pre-phase pins;
|
||||
- the toggle handler mutates only `aria-pressed` + the `data-view` attribute (no innerHTML anywhere in the new code path — the never-build-HTML pin);
|
||||
- `loading = "lazy"` remains on the strip imgs (the existing pin re-runs green).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the contract pins above; coverage **>90%** on `app/` (no `app/` change in this task — the gate must simply stay green).
|
||||
- The phase's E2E (task 05) exercises the real DOM; `test_pdf_pages_in_sources.py` (the phase-148 E2E) is re-cut ONLY where a modal-DOM assertion meets the additive fixture (the fixture's mode stamp decides; prefer fixture docs that keep primary semantics for the existing pins).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] An additive doc shows Summary + text with `Text` pressed by default; one click shows the strip (every page img loads); clicking back shows the text with zero refetch (the strip node persists — E2E-pinned in task 05).
|
||||
- [ ] A primary/absent-mode doc's viewer DOM is byte-identical to the pre-phase structure (unit pin green); a text doc and every non-PDF are untouched.
|
||||
- [ ] The toggle passes the WCAG basics: labeled group, `aria-pressed` state as text + style, focus-visible ring (checked against §7.2; the E2E asserts the attributes).
|
||||
- [ ] Full `uv run pytest` green; `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -0,0 +1,33 @@
|
||||
# Task 05 — The dedicated E2E: additive view, primary byte-identity, heal & repair; the final gates (phase-163 D5)
|
||||
|
||||
**Phase:** `163_pdf_strip_additive_selfheal` · **Source:** phase overview D5 + the AGENTS.md per-phase E2E rule (A16).
|
||||
**Story:** n/a
|
||||
|
||||
**Pre-fix evidence (measured on the live server, 2026-10-07 — recorded here per house practice):** `Letumdei_ Heirs to the Imperium.pdf` — payload carries 36 `page_urls`; `GET /api/documents/b72f5515-dc40-4167-98ff-179a6503e6e5/pages/1,2,18,36` → **404/404/404/404** (page 37 → 404, the out-of-range arm); the content is 114 389 chars of genuine extracted text with 36 `---PAGE-N---` markers and exactly ONE `[Page 36 illustration: …]` block; the same server serves `besta-frame-white__AA-1272068-11-100.pdf` page 1 → **200** (the dir is intact; only the 10/1-indexed doc's copies were lost). The viewer showed "Page N unavailable." on every page with the text hidden.
|
||||
|
||||
## Objective
|
||||
One Playwright suite proves the whole phase end to end — the additive view with the toggle, the fallback class's byte-identical strip-only DOM, the disk-level heal, and the repair arm — and the phase's final pass runs every gate plus the full documentation consistency review.
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_pdf_strip_additive_selfheal.py` — new suite (run in isolation; read `tests/e2e/test_pdf_pages_in_sources.py` and `tests/e2e/test_pictorial_pdf.py` first and reuse their synthetic-PDF builder + fixture-KB pattern; deterministic mock LLM, the default):
|
||||
- **fixture**: source `strip_fixture` with (a) `manual.pdf` — usable text layer, 3 text pages + 1 sparse `<100-char` map-style page (the pictorial class — the Letumdei shape in miniature); (b) `scanned.pdf` — the garbage/unusable-layer fixture (the phase-133 pattern) → fallback class; (c) `plain.pdf` — clean text, zero sparse pages → no strip.
|
||||
- **assert 1 (additive view)**: index → the modal for `manual.pdf`: mode stamped `additive` (via the content API), the text + Summary panel visible by default, the `Text`/`Pages` toggle present with `Text` `aria-pressed="true"`; click `Pages` → the strip renders, EVERY page `<img>` has `naturalWidth > 0` (routes serve — no "unavailable" note anywhere); click `Text` → text visible, strip hidden, the strip node still mounted (no refetch: assert the img count does not reset).
|
||||
- **assert 2 (primary byte-identity)**: the modal for `scanned.pdf`: the strip-only DOM (no `.doc-view-toggle`, no Summary panel) and the "Page N" figures match the phase-148 structure (the existing suite's DOM assertions adapted); `plain.pdf` renders the plain text doc with no strip artifacts.
|
||||
- **assert 3 (heal)**: for `manual.pdf`, delete ALL `pdf_page_dir/<id>-p*.png` (the fixture's page dir) + confirm a page route 404s → run a sync (unchanged bytes → the fast path) → the files exist, the page route 200s, the payload still carries `page_urls` + `additive`, the viewer's Pages view loads every img (the live-loss state, healed by the next sync — the exact D5 promise).
|
||||
- **assert 4 (repair)**: direct-DB seed — set `page_count` (and stale copies on disk) on `plain.pdf`'s row (the stale-eligible state no current index produces) → sync → `page_count`/`page_strip_mode` NULL (read through the fixture session), the stale copies deleted, the modal shows the plain text (the strip gone).
|
||||
- **assert 5 (no vision)**: the whole suite's syncs ran against the deterministic mock — no chat/vision endpoint hits beyond the initial index's own passes (the heal burns zero LLM calls; assert via the mock's call counter or the suite's request log, the house pattern).
|
||||
2. Final pass (the phase pipeline runs this after the tasks): full-suite + gates:
|
||||
- `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` **>90%**;
|
||||
- `uv run pytest tests/e2e/test_pdf_strip_additive_selfheal.py -v --no-cov` green **in isolation**; `tests/e2e/test_pdf_pages_in_sources.py`, `test_pictorial_pdf.py`, `test_garbage_pdf.py`, `test_document_viewer*.py`, `test_image_documents.py` green;
|
||||
- `uv run ruff check . && uv run pyright` clean; the alembic 0027 up/down/up round-trip clean.
|
||||
3. Documentation & consistency (mandatory): review `README.md` and `AGENTS.md` IN FULL against the post-phase app — the viewer description gains the additive-mode sentence (text + Text/Pages toggle for pictorial-class PDFs; the fallback class keeps the strip), the importer notes (write-first copies, the fast-path re-gate + its INFO line) — and fix EVERY inconsistency found, including pre-existing ones not caused by this phase. `PLAN.md` is NOT touched (its §7 strip line folds at the next redo — recorded in the phase overview invariants).
|
||||
|
||||
## Testing & Quality
|
||||
- This task IS the E2E + gates task; its own new-code coverage rides the suite (page route + fixture helpers are test-side; `app/` coverage stays **>90%**).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] All five E2E asserts green in isolation; the four existing viewer/PDF suites green unmodified (except explicitly re-cut pins, each named in the commit body).
|
||||
- [ ] The heal assert proves: copies deleted externally → one unchanged-byte sync → files restored, routes serve, viewer complete (the live `Letumdei` failure mode is impossible after this phase).
|
||||
- [ ] `uv run pytest` fully green; coverage >90%; ruff + pyright clean; migration round-trip clean.
|
||||
- [ ] `README.md` + `AGENTS.md` reviewed in full, every inconsistency fixed (the additive-viewer sentence included); the phase dir moves to `complete/` and the single `--no-gpg-sign` commit (see the overview) is made by the pipeline gate.
|
||||
- [ ] Owner follow-up recorded (not code): after deploy, the next production Sync self-heals `Letumdei` (D5) — verify once via the Sources modal (text visible, Pages loads all 36) and `grep` the server log for the one `import: PDF strip re-gate … healed 36 pages` line.
|
||||
Reference in New Issue
Block a user