chore(agents): phase roadmap from TODO.md, 3 phases
Converts the three open TODO items into executable phases (Protocol B append, numbered after the existing 149): - 150_drop_sync_result — retire span.sync-result; #sync-progress becomes the single sync surface and the sole aria-live region (TODO L5) - 151_pdf_image_extraction — dedupe PDF image occurrences (46 placements -> 15 unique in podman-systemd.pdf), composite /SMask so masked images are not blank, and skip near-white decorative underlays (TODO L4) - 152_sync_progress_image_pass — report the PDF vision-image position so the sync label stops freezing at the last chunk (TODO L3) Both reported bugs were reproduced against test_sources/podman-systemd.pdf before authoring; root causes are recorded in the phase overviews. The TODO file is cleared now that its items live in .agents/phases/. LLM-Generated: true
This commit is contained in:
@@ -0,0 +1,56 @@
|
||||
# Phase 151 — no more blank white image documents from PDFs (dedupe + soft masks + a blank gate)
|
||||
|
||||
**Source:** `TODO.md` L4 — "For the same podman pdf document, it extracted a series of blank white images for some reason. There don't appear to be any blank images in the document, is this a bug?"
|
||||
**Story:** n/a (TODO-derived phase, Protocol B append).
|
||||
**Context:** `app/rag/pdf.py` — `extract_pdf_images()` (the module's image extractor: `page.get_image_info(xrefs=True)` per page via `_page_image_entries`, then `doc.extract_image(xref)` per entry, one `PdfImage(page, index, width, height, data, mime)` per **occurrence**) and its consumer `app/rag/importer.py` `_index_pdf_embedded_images()` (L1924 — the per-PDF embedded-image pass: `_pdf_image_context_inputs` once, then `extract_pdf_images(full_path)` and one `_index_one_embedded_image()` (L2030) per image: mime→ext gate through `_MIME_TO_EXT` (L1340 — png/jpg/gif/webp/bmp/tiff; anything else maps to `.bin` and is skipped), sha256-digest skip, one vision describe call (`_describe_or_skip`, L2907), the `image_dir` copy, chunk + embed, one transaction per image). The `images` toggle (`BOR_IMAGES`, `settings.images`, default OFF) gates the whole pass (LOCKED A3). `ImportSummary` (L390–475) carries the log-only counters (`images_failed`, `pdfs_screenshotted`, `pdfs_pictorial`, `pdf_pictorial_pages`, `image_context_backfilled`) — the phase-133 stats log-only convention: counters land in the `import: summary …` line, never in the sync `detail` API shape.
|
||||
|
||||
**Reproduced on the owner's file (`test_sources/podman-systemd.pdf`, 50 pages) — three distinct defects:**
|
||||
1. **Duplicates:** `get_image_info` reports one entry per *placement*, so the file yields **46 occurrences of only 15 unique xrefs** (each image is placed 2–4 times). Every occurrence becomes its own document (`…#page=N#img=i.<ext>`), so the same image is vision-described and embedded up to 4×.
|
||||
2. **Soft masks ignored:** every one of the 15 images carries an `/SMask` (`extract_image` reports `smask=<xref>`, `get_image_info` reports `has-mask=True`). `extract_image()` returns the **base** sample stream only — the mask (which carries the visible shape) is dropped, so the stored bytes are not what a PDF viewer shows.
|
||||
3. **Decorative underlays indexed:** those images are table-shading underlays — `DeviceGray`, `mean=238/255`, **8 unique sample values**, **99.5% of samples within ±8 of the mean** (verified: composited with their mask they are still a near-white block; the table text a viewer shows is real PDF text drawn on top). They are indexed as documents, vision-described, and returned as image sources.
|
||||
|
||||
Measured comparison (the same probe, `fitz.Pixmap(...).samples`): the podman underlays `uniq=8`, near-uniform(±8) `0.95–0.995`; `test_sources/pdf_horse_test.pdf`'s photo `uniq=256`, `0.14`; `test_sources/players_handbook.pdf` page art `uniq≥188`, `0.10–0.59`. The gate separates them cleanly.
|
||||
|
||||
**Locked (owner decisions, chat):** blank/decorative images are **skipped entirely** (never indexed, never described — not stored-and-flagged); JPEG2000 (`jpx`) images stay **skipped** as they are today (the unsupported-extension gate keeps running on the RAW embedded format, so compositing to PNG must not newly admit the 586 `jpx` images in `players_handbook.pdf`).
|
||||
|
||||
## Objective
|
||||
A PDF's embedded-image pass yields one document per genuinely distinct, non-decorative image, with the bytes a viewer would show (soft mask applied) — no blank white documents, no triplicate images, and far fewer vision calls per PDF.
|
||||
|
||||
## Dependencies
|
||||
- `150_drop_sync_result` (todo) — pipeline order only (that phase touches the Sources-page sync UI; this phase touches `app/rag/pdf.py` + the importer's image pass). No shared files.
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
- **Dedupe (task 01):** in `extract_pdf_images`, keep the FIRST occurrence of each distinct image and skip the repeats — key = the `xref` when non-zero, else the sha256 of the final `data` (a digest set also catches byte-identical images stored under different xrefs). The kept `PdfImage` carries the first occurrence's `page`/`index` (the synthetic path `…#page=N#img=i.<ext>` is therefore stable for the first placement). One INFO line per PDF when duplicates were dropped (`pdf: deduped N duplicate image occurrence(s) in <path>`), DEBUG per image.
|
||||
- **Soft masks (task 02):** when the info/`extract_image` dict reports a non-zero `smask`, build the VISIBLE image instead of the raw base: `base = fitz.Pixmap(doc, xref)`, `mask = fitz.Pixmap(doc, smask_xref)`, `visible = fitz.Pixmap(base, mask)` (PyMuPDF's alpha-combining constructor) → `data = visible.tobytes("png")`, `mime = "image/png"`. Fail-soft: any exception (or a `Pixmap` with a colorspace PyMuPDF cannot combine) falls back to the raw `extract_image` bytes + the raw mime and logs a warning — the image is still indexed, exactly as today. **Ordering rule (owner-locked A4):** the raw-format gate runs FIRST — an image whose raw format is unsupported (`jpx` → `.bin`) is left exactly as today (raw bytes, raw mime, skipped downstream by the importer's ext gate); compositing happens only for images whose raw mime already maps through `_MIME_TO_EXT`. A `jpx` image with an smask therefore stays skipped.
|
||||
- **Blank gate (task 03):** a NEW pure predicate in `app/rag/pdf.py`, `is_blank_image(samples: bytes, *, mean_min: int = 235, tolerance: int = 12, fraction: float = 0.98) -> bool` — pure (no PDF, no I/O — the `is_usable_pdf_text` / `pictorial_pages` house style, so it is unit-testable with hand-built sample buffers): blank when `len(samples)` is non-empty AND `mean(samples) >= mean_min` AND at least `fraction` of the samples lie within `±tolerance` of the mean. Applied in `extract_pdf_images` to the samples of the image that will actually be stored (the composited pixmap when a mask was applied, else the base pixmap): a blank image is **skipped** (not returned), with one INFO line per skipped image (`pdf: skipped blank/decorative image page=%d index=%d of %s — mean=%.0f uniform=%.3f`) and one summary line per PDF. Fail-soft: a pixmap that cannot be built skips the gate (the image is kept — never lose a real image to a decode problem).
|
||||
- **The knob (task 04):** `Settings.pdf_image_skip_blank: bool = True` (`BOR_PDF_IMAGE_SKIP_BLANK`) — the kill switch, exactly the `pdf_pictorial` (L433) pattern: a `bool` with a `#:` docstring naming the env var, read by the importer and passed down, `False` restores the pre-phase behavior (every image kept, gate off). Dedupe and mask compositing are NOT knobed (they are correctness fixes). `.env.example` gains the documented entry next to the other PDF/image vars; `README.md` gains one line in the PDF/image settings section.
|
||||
- **Counters (task 03):** two log-only `ImportSummary` fields — `pdf_images_deduped` and `pdf_images_blank_skipped` — appended to `ImportSummary.log()`'s line (the phase-133 stats log-only convention: the sync `detail` API shape is untouched). They count occurrences dropped per run across all PDFs.
|
||||
- **ASSUMPTION A1 (owner-locked):** blank images are skipped entirely (no row, no copy, no vision call) rather than indexed-and-flagged.
|
||||
- **ASSUMPTION A2 (owner-locked):** thresholds `mean ≥ 235`, `≥98%` of samples within `±12` — pinned by unit tests against the three real PDFs in `test_sources/` (podman underlays skipped; the horse photo and the handbook page art kept).
|
||||
- **ASSUMPTION A3:** the dedupe changes the synthetic path for repeat occurrences, so image docs created pre-phase for a 2nd/3rd placement are pruned on the next sync and the first placement's doc is kept (idempotent from then on). No migration code.
|
||||
- **ASSUMPTION A4 (owner-locked):** `jpx` (JPEG2000) images stay unsupported/skipped; compositing never newly admits them.
|
||||
- **ASSUMPTION A5:** the gate lives in the parser (`extract_pdf_images`), not the importer, so every caller of the parser gets the fix and the parser stays the single place that decides what "an embedded image" is.
|
||||
|
||||
## Tasks
|
||||
1. `01_dedupe_image_occurrences.md` — one `PdfImage` per distinct image (xref, else digest); duplicate-drop log + counter.
|
||||
2. `02_composite_soft_masks.md` — `/SMask` composited to an alpha PNG (raw-format gate first, fail-soft to raw bytes).
|
||||
3. `03_blank_image_gate.md` — the pure `is_blank_image()` predicate + the skip in the extractor + the two log-only counters.
|
||||
4. `04_settings_knob.md` — `BOR_PDF_IMAGE_SKIP_BLANK` (default on) threaded parser → importer, `.env.example` + README.
|
||||
5. `05_unit_tests.md` — PyMuPDF-built fixtures for dedupe/mask/blank + the real-PDF regression pins.
|
||||
6. `06_e2e_pdf_image_extraction.md` — the dedicated Playwright suite (upload a PDF with a duplicate placement + a blank underlay + a real image → exactly one image doc, no blank docs).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit (`tests/unit/test_pdf.py` + a new `tests/unit/test_pdf_image_filter.py`): dedupe (N placements of one image → 1 result, first occurrence's page/index kept, counter), digest-level dedupe across xrefs, smask compositing (an RGBA image round-trips with alpha; the raw-format gate leaves `jpx`-style unsupported formats untouched; the fail-soft path returns raw bytes), `is_blank_image` as a pure function (uniform white → blank; noise → not blank; dark-uniform → NOT blank because the mean gate is about near-white underlays; empty samples → not blank), the knob off → every image kept.
|
||||
- Real-file pins (skip-if-missing, the `HANDBOOK`/`HORSE_PDF` pattern already in `tests/unit/test_pdf.py` L1734–1737): `podman-systemd.pdf` → 15 unique images in, **0 image docs out** (all are decorative underlays); `pdf_horse_test.pdf` → its photo kept; `players_handbook.pdf` → its images stay skipped by the ext gate (unchanged, `jpx`).
|
||||
- Integration: the importer's embedded-image pass indexes exactly the extractor's returned list (a blank/duplicate image never reaches `_index_one_embedded_image` — assert with a fake `Embedder` counting vision calls, the existing image-test idiom).
|
||||
- Coverage: **>90%** on new/modified code (`app/rag/pdf.py`, the importer's pass, `app/config.py`).
|
||||
- E2E: one dedicated Playwright file, run in isolation (`uv run pytest tests/e2e/test_pdf_image_extraction.py -v --no-cov`).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] syncing a PDF like `podman-systemd.pdf` creates **no** blank white image documents and **no** duplicate image documents (verified by the E2E and the real-file unit pins)
|
||||
- [ ] an image with an `/SMask` is stored as the composited image (alpha PNG), not the blank base; an unsupported raw format (`jpx`) is still skipped
|
||||
- [ ] `BOR_PDF_IMAGE_SKIP_BLANK=false` restores the pre-phase behavior (every extracted image indexed) — pinned by a unit test
|
||||
- [ ] `import: summary … pdf_images_deduped=N pdf_images_blank_skipped=M …` appears in the sync log; the sync `detail` API shape is unchanged
|
||||
- [ ] no behavior change in completed phases (the phase-137 document-context block, the phase-125 markdown enrichment, the digest skip, the `images` toggle semantics, the phase-133 fallback, the phase-140 pictorial pass)
|
||||
- [ ] `uv run pytest` green; the dedicated E2E green in isolation; `uv run ruff check . && uv run pyright` clean; `app/` coverage >90%
|
||||
- [ ] one atomic commit per AGENTS.md rule 8 (`--no-gpg-sign`)
|
||||
@@ -0,0 +1,29 @@
|
||||
# Task 01 — one `PdfImage` per distinct image (duplicate placements dropped)
|
||||
|
||||
**Phase:** `151_pdf_image_extraction` · **Source:** `TODO.md:4` — "For the same podman pdf document, it extracted a series of blank white images for some reason. There don't appear to be any blank images in the document, is this a bug?"
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
`extract_pdf_images()` returns one entry per DISTINCT image, so a PDF that places the same image 4× produces one document instead of four.
|
||||
|
||||
## Work
|
||||
1. `app/rag/pdf.py` — `extract_pdf_images()`: keep the per-page/occurrence walk (it is what gives `page`/`index`), but skip anything already seen:
|
||||
- maintain `seen_xrefs: set[int]` and `seen_digests: set[str]` (sha256 hex of the final `data`) outside the page loop;
|
||||
- after `extract_image` succeeds and `data` is non-empty: `if xref in seen_xrefs: dropped += 1; continue`, then `digest = hashlib.sha256(data).hexdigest()`; `if digest in seen_digests: dropped += 1; continue`; record both before appending (the digest set catches byte-identical images stored under different xrefs);
|
||||
- the FIRST occurrence wins: the appended `PdfImage` keeps that occurrence's `page` (1-based) and `index` (0-based on the page) — the importer's synthetic path `…#page=N#img=i.<ext>` therefore stays stable for the first placement (ASSUMPTION A3: repeat-placement docs created pre-phase are pruned on the next sync, no migration code);
|
||||
- when `dropped`: one INFO line after the page loop — `pdf: dropped %d duplicate image occurrence(s) from %s (%d unique)` — and a DEBUG line per drop (`pdf: duplicate image xref=%d page=%d index=%d of %s — skipped`);
|
||||
- `hashlib` is already imported in this module? If not, add `import hashlib` (check first — `app/rag/pdf.py` currently imports `logging`, `re`, `collections.abc`, `dataclasses`, `pathlib`, `typing`, `pymupdf`).
|
||||
2. Update the module docstring's `extract_pdf_images` bullet: "one :class:`PdfImage` per DISTINCT embedded image (page order, first placement wins) — repeated placements and byte-identical copies are deduplicated (phase 151 task 01)".
|
||||
3. `app/rag/importer.py` — `_index_pdf_embedded_images()` needs no logic change (it indexes exactly what the extractor returns), but its docstring's "each embedded image … in page order then 0-based index" sentence gains the dedupe note. Add the two `ImportSummary` counters' plumbing HERE only if task 03 has not landed yet — otherwise leave the counters to task 03 (avoid a duplicate field).
|
||||
4. ASSUMPTION: dedupe is per-PDF (the sets are local to one `extract_pdf_images` call), never across PDFs — two PDFs sharing a logo still each get their own image doc (the synthetic path is per-PDF, so cross-PDF dedupe would break the upsert key).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit (`tests/unit/test_pdf.py`, the PyMuPDF-built-fixture convention): a PDF with the SAME image inserted on 3 pages (or 3× on one page) → `len(extract_pdf_images(...)) == 1` and it carries the first occurrence's `page`/`index`; a PDF with two genuinely different images → 2; two different xrefs holding byte-identical bytes → 1 (digest path); the INFO line emitted (caplog) only when a duplicate was dropped.
|
||||
- Real-file pin (skip-if-missing): `test_sources/podman-systemd.pdf` → the extractor returns **15** entries (46 placements pre-phase).
|
||||
- Coverage: **>90%** on the new branches (both dedupe paths + the no-duplicate path).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `extract_pdf_images(Path("test_sources/podman-systemd.pdf"))` returns 15 entries (was 46)
|
||||
- [ ] the first occurrence's page/index are preserved; the digest path is covered by a unit test
|
||||
- [ ] the duplicate-drop INFO line appears once per PDF with duplicates
|
||||
- [ ] `uv run pytest tests/unit/test_pdf.py -q` green; `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,37 @@
|
||||
# Task 02 — apply the `/SMask`: store the image a viewer would show
|
||||
|
||||
**Phase:** `151_pdf_image_extraction` · **Source:** `TODO.md:4` — "For the same podman pdf document, it extracted a series of blank white images for some reason. There don't appear to be any blank images in the document, is this a bug?"
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
An image stored in the PDF with a soft mask is extracted as the composited (masked) image, not as the blank base sample stream `extract_image()` returns today.
|
||||
|
||||
## Work
|
||||
1. `app/rag/pdf.py` — `extract_pdf_images()`, inside the per-image block, AFTER the raw-format decision and BEFORE appending:
|
||||
- read the mask reference: `smask = int(extracted.get("smask") or 0)` (the `extract_image` dict reports it; `get_image_info`'s `has-mask` is the cross-check, not the source of truth);
|
||||
- **raw-format gate FIRST (owner-locked A4):** compute `mime` from the raw format exactly as today (`_mime_from_image_info({**info, "ext": extracted.get("ext") or ""})`). If that mime is `application/octet-stream` (i.e. a format the importer's `_MIME_TO_EXT` gate will reject — `jpx`/JPEG2000, the 586 images in `players_handbook.pdf`), DO NOT composite: keep the raw bytes + raw mime, so the behavior is byte-identical to pre-phase for unsupported formats (compositing must never newly admit them);
|
||||
- otherwise, when `smask` is non-zero: build the visible image
|
||||
```python
|
||||
base = fitz.Pixmap(doc, xref)
|
||||
mask = fitz.Pixmap(doc, smask)
|
||||
visible = fitz.Pixmap(base, mask) # base + alpha-from-mask
|
||||
data = visible.tobytes("png")
|
||||
mime = "image/png"
|
||||
```
|
||||
wrapped in `try/except Exception` → on failure log a warning (`pdf: could not composite smask=%d for image page=%d index=%d of %s — %s; using raw bytes`) and fall back to the raw `extracted["image"]` bytes + the raw mime (fail-soft: the image is still indexed, exactly as pre-phase);
|
||||
- keep `width`/`height` from `extracted` (unchanged — the composited pixmap has the same dimensions);
|
||||
- a DEBUG line when a mask was applied (`pdf: composited smask=%d for image page=%d index=%d of %s`).
|
||||
2. `app/rag/pdf.py` module docstring: extend the `extract_pdf_images` bullet — masked images are composited to an alpha PNG (phase 151 task 02), unsupported raw formats are left untouched so the importer's extension gate still decides.
|
||||
3. `app/rag/importer.py`: no code change — `_index_one_embedded_image()` already derives the copy extension from `img.mime` via `_MIME_TO_EXT`, and `image/png` → `.png` is in `settings.image_extension_set` by default. Verify the digest-skip consequence in a comment: a masked image's bytes change, so pre-phase rows for masked images re-describe ONCE on the first post-phase sync (the sha256 digest differs) — expected, self-limiting, no migration.
|
||||
4. ASSUMPTION: `fitz.Pixmap(base, mask)` is the correct combinator (verified against `podman-systemd.pdf` xrefs 202/744/1633 — it produces an alpha PNG of the same dimensions); a CMYK/Indexed base that PyMuPDF refuses to combine takes the fail-soft path.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit (`tests/unit/test_pdf.py`): build a PDF with an RGBA PNG inserted (`page.insert_image(rect, stream=png_bytes)` — PyMuPDF writes the `/SMask`) and assert (a) `extract_image`'s raw bytes ≠ the extractor's `data`, (b) the extractor's `mime == "image/png"`, (c) re-opening the extractor's bytes as a `fitz.Pixmap` reports `alpha == 1` (the mask survived), (d) a PDF whose image has no mask is returned byte-identical to pre-phase (no regression for the common case); the fail-soft path via a monkeypatched `fitz.Pixmap` that raises → raw bytes returned + the warning logged (caplog).
|
||||
- Real-file pin (skip-if-missing): every image extracted from `test_sources/podman-systemd.pdf` carries an smask pre-phase; post-phase the extractor's output for it is either composited PNG or (blank-gated away by task 03) absent — assert the composited bytes differ from the raw base for at least one xref when the gate is off.
|
||||
- Coverage: **>90%** on the new branches (mask applied / mask failed / unsupported format untouched / no mask).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] a masked image extracted through `extract_pdf_images` yields alpha-PNG bytes (`mime="image/png"`), not the blank base stream
|
||||
- [ ] an unsupported raw format (`jpx`) is untouched — raw bytes + raw mime, still skipped downstream
|
||||
- [ ] a compositing failure falls back to the raw bytes with a warning (no image lost, no exception raised)
|
||||
- [ ] `uv run pytest tests/unit/test_pdf.py -q` green; `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,42 @@
|
||||
# Task 03 — the blank/decorative gate: never index a near-white underlay
|
||||
|
||||
**Phase:** `151_pdf_image_extraction` · **Source:** `TODO.md:4` — "For the same podman pdf document, it extracted a series of blank white images for some reason. There don't appear to be any blank images in the document, is this a bug?"
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
A deterministic, pure predicate decides "this image carries no visible content" and the extractor drops those images — no row, no `image_dir` copy, no vision call.
|
||||
|
||||
## Work
|
||||
1. `app/rag/pdf.py` — add the pure predicate (the `is_usable_pdf_text` / `pictorial_pages` house style: no PDF, no I/O, no LLM):
|
||||
```python
|
||||
BLANK_IMAGE_MEAN = 235 #: near-white floor
|
||||
BLANK_IMAGE_TOLERANCE = 12 #: "same as the mean" band
|
||||
BLANK_IMAGE_FRACTION = 0.98 #: fraction of samples that must sit in the band
|
||||
|
||||
def is_blank_image(samples: bytes, *, mean_min: int = BLANK_IMAGE_MEAN,
|
||||
tolerance: int = BLANK_IMAGE_TOLERANCE,
|
||||
fraction: float = BLANK_IMAGE_FRACTION) -> bool:
|
||||
...
|
||||
```
|
||||
Blank when `samples` is non-empty AND `sum(samples)/len(samples) >= mean_min` AND `sum(1 for s in samples if abs(s - mean) <= tolerance) / len(samples) >= fraction`. Empty samples → `False` (never blank a thing we cannot measure). Document the measured basis in the docstring (podman underlays: mean 238, uniq 8, uniform(±8) 0.95–0.995 → blank; the horse photo: mean 123, uniq 256, uniform 0.14 → kept; handbook page art: mean 24–221, uniq ≥188, uniform 0.10–0.59 → kept). Note explicitly that a DARK uniform block is NOT blank by this rule (the gate targets near-white page underlays — a solid dark block is unusual and cheaper to keep than to explain away).
|
||||
2. `app/rag/pdf.py` — `extract_pdf_images()`: after the dedupe + mask steps, decide blank-ness on the samples of the image that would actually be stored:
|
||||
- build `gate_pixmap` = the composited pixmap when a mask was applied, else `fitz.Pixmap(doc, xref)`; when `gate_pixmap.alpha`, drop the alpha first (`fitz.Pixmap(gate_pixmap, 0)`) so the mean is over COLOR samples only (an alpha channel would drag the mean and produce false blanks);
|
||||
- `if settings-gate and is_blank_image(gate_pixmap.samples): blank_skipped += 1; continue` (the gate is skipped entirely when the caller passes `skip_blank=False` — task 04's knob);
|
||||
- one INFO line per skipped image: `pdf: skipped blank/decorative image page=%d index=%d of %s — mean=%.1f uniform=%.3f`, and one INFO per PDF when `blank_skipped` (`pdf: skipped %d blank/decorative image(s) in %s`);
|
||||
- fail-soft: if the pixmap cannot be built for the gate, DO NOT skip the image (keep it, DEBUG why) — a decode problem must never delete a real image;
|
||||
- the extractor signature gains `skip_blank: bool = True` (keyword, default on) so callers/tests can turn it off without settings; the dedupe and mask steps are NOT knobed.
|
||||
3. `app/rag/importer.py` — `ImportSummary` gains two log-only counters (the phase-133 stats log-only convention: the sync `detail` API shape is untouched), each with a `#:` docstring citing this phase: `pdf_images_deduped: int = 0` and `pdf_images_blank_skipped: int = 0`, appended to `ImportSummary.log()`'s format string (`… pdf_images_deduped=%d pdf_images_blank_skipped=%d formats=%s`). The extractor returns the counts alongside the images so the importer can record them — the minimal clean shape is a small result object or a 2-tuple; prefer keeping `extract_pdf_images` returning `list[PdfImage]` for its other callers and add the counters via an optional out-param-free route: give the function a sibling `extract_pdf_images_with_stats(pdf_path, *, skip_blank=True) -> tuple[list[PdfImage], PdfImageStats]` (a `@dataclass(frozen=True) PdfImageStats` with `duplicates`, `blank_skipped`, `total_found`) and make `extract_pdf_images` a thin wrapper returning `stats[0]`. `_index_pdf_embedded_images()` calls the `_with_stats` variant and adds to `summary.pdf_images_deduped` / `summary.pdf_images_blank_skipped`.
|
||||
4. ASSUMPTION: the counters are log-only (no API change) — the owner reads them in the sync log; the UI's sync result line is untouched.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit (`tests/unit/test_pdf_image_filter.py`, new file): `is_blank_image` as a pure function — all-250 bytes → blank; random noise → not blank; all-0 (dark) → NOT blank; empty → not blank; a 97%-uniform-with-a-few-dark-pixels buffer → not blank (just under the fraction); the boundary values pinned exactly at `mean_min` and `fraction`; the extractor-level behavior with PyMuPDF-built PDFs — a near-white 200×200 image inserted → 0 results + the INFO line + `stats.blank_skipped == 1`; a colorful noise image → 1 result; `skip_blank=False` → both returned.
|
||||
- Integration: the importer's pass with a fake `Embedder` (the existing image-test idiom) — a PDF with one blank + one real image produces exactly ONE image document and ONE vision call; `import: summary … pdf_images_blank_skipped=1` in the log (caplog).
|
||||
- Real-file pin (skip-if-missing): `test_sources/podman-systemd.pdf` → `extract_pdf_images` returns `[]` (all 15 unique images are decorative underlays) with `stats.duplicates == 31` and `stats.blank_skipped == 15`; `test_sources/pdf_horse_test.pdf` → its photo is returned.
|
||||
- Coverage: **>90%** on the new code.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `extract_pdf_images(Path("test_sources/podman-systemd.pdf"))` returns no images (pre-phase: 46 blank-white docs)
|
||||
- [ ] `extract_pdf_images(..., skip_blank=False)` still returns them (the kill switch at the function level)
|
||||
- [ ] `import: summary … pdf_images_deduped=… pdf_images_blank_skipped=…` appears in the sync log; the `/api/sync/status` `detail` shape is unchanged
|
||||
- [ ] a dark-uniform image and a real photo are both kept (unit-pinned)
|
||||
- [ ] `uv run pytest tests/unit/test_pdf_image_filter.py -q` green; `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,29 @@
|
||||
# Task 04 — `BOR_PDF_IMAGE_SKIP_BLANK`: the kill switch, wired end to end
|
||||
|
||||
**Phase:** `151_pdf_image_extraction` · **Source:** `TODO.md:4` — "For the same podman pdf document, it extracted a series of blank white images for some reason. There don't appear to be any blank images in the document, is this a bug?"
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
The blank gate is a documented setting, default ON, with a supported way to turn it off — the `pdf_pictorial` pattern.
|
||||
|
||||
## Work
|
||||
1. `app/config.py` — add to `Settings`, next to `pdf_pictorial` / `pdf_pictorial_min_chars` (L433–443):
|
||||
```python
|
||||
#: Skip blank/decorative PDF-embedded images (phase 151, task 04,
|
||||
#: ``BOR_PDF_IMAGE_SKIP_BLANK``; ``0``/``false`` = off) …
|
||||
pdf_image_skip_blank: bool = True
|
||||
```
|
||||
The docstring names the env var, states the default, and says what turning it off means (every extracted image is indexed again — the pre-phase behavior; dedupe and soft-mask compositing are unaffected, they are correctness fixes). No validator needed (a plain bool like `pdf_pictorial` / `images`).
|
||||
2. `app/rag/importer.py` — `_index_pdf_embedded_images()`: pass the setting into the extractor call (`extract_pdf_images_with_stats(full_path, skip_blank=settings.pdf_image_skip_blank)`), so the knob is read in exactly one place. Update the pass's docstring bullet list with the gate + the knob.
|
||||
3. `.env.example` — add the entry in the PDF/image block (next to `BOR_PDF_PICTORIAL` / `BOR_IMAGES`), commented in the file's existing voice: what it does, the default, and when to set it to `false` (a KB of scanned/decorative pages where a near-white image still matters).
|
||||
4. `README.md` — one line in the PDF/image settings list (the settings table/section the phase-126/140 entries live in): `BOR_PDF_IMAGE_SKIP_BLANK` (default `true`) — skip blank/decorative images embedded in PDFs.
|
||||
5. ASSUMPTION: no admin-UI toggle (the phase-91 Theme Tab precedent shows knobs live in `.env`; the owner asked for the bug fixed, not a settings page).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: `Settings` default is `True`; `BOR_PDF_IMAGE_SKIP_BLANK=false` (env override, the existing config-test idiom) yields `False`; an integration test with the fake `Embedder` proves the off path indexes a blank image again (the inverse of task 03's assertion) — the kill switch is proven, not assumed.
|
||||
- Coverage: **>90%** on the touched lines.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `BOR_PDF_IMAGE_SKIP_BLANK=false` restores indexing of blank PDF-embedded images (test-proven)
|
||||
- [ ] the setting is documented in `.env.example` and `README.md`
|
||||
- [ ] `uv run pytest tests/unit -q` green; `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,31 @@
|
||||
# Task 05 — unit + integration tests for dedupe, masks, and the blank gate
|
||||
|
||||
**Phase:** `151_pdf_image_extraction` · **Source:** `TODO.md:4` — "For the same podman pdf document, it extracted a series of blank white images for some reason. There don't appear to be any blank images in the document, is this a bug?"
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
Every new branch of the extractor is pinned without a browser: PyMuPDF-built fixtures for the synthetic cases, the three real PDFs in `test_sources/` for the regression proof.
|
||||
|
||||
## Work
|
||||
1. `tests/unit/test_pdf_image_filter.py` (new file; the `tests/unit/test_pdf.py` conventions — PyMuPDF-built temp PDFs, no binary fixtures committed, `tmp_path`):
|
||||
- **dedupe:** one image inserted on 3 pages → 1 result with the FIRST page/index; two distinct images → 2; byte-identical bytes under two xrefs → 1 (digest path); `stats.duplicates` counts; the INFO line via `caplog`;
|
||||
- **soft mask:** an RGBA PNG inserted (`page.insert_image(rect, stream=…)`) → the extractor's `mime == "image/png"`, its bytes ≠ `doc.extract_image(xref)["image"]`, and `fitz.Pixmap(stream=extracted_bytes).alpha == 1`; an unmasked image → byte-identical to pre-phase; a monkeypatched `fitz.Pixmap` that raises → raw bytes returned + the warning (fail-soft);
|
||||
- **blank gate:** `is_blank_image` as a pure function (uniform white → True; noise → False; uniform dark → False; empty → False; the exact `mean_min`/`fraction` boundaries); extractor-level — a near-white 200×200 image → 0 results + `stats.blank_skipped == 1` + the INFO line; a colorful noise image → 1 result; `skip_blank=False` → both returned;
|
||||
- **unsupported format untouched:** an image whose raw ext is unsupported (build one by monkeypatching `extract_image` to report `ext="jpx"` + a non-zero `smask`) → the extractor returns raw bytes + the unmapped mime (no compositing) — the owner-locked A4 pin.
|
||||
2. `tests/unit/test_pdf.py` — extend the existing extractor tests (they already cover `extract_pdf_images` and the fail-soft contract): the dedupe/mask steps must not break the "unreadable file → []", "no xref → skipped", "empty data → skipped" pins; add the real-file pins with the file's existing skip-if-missing idiom (see `HANDBOOK` / `HORSE_PDF`, L1734–1737):
|
||||
- `test_sources/podman-systemd.pdf` → `extract_pdf_images(...) == []`, `stats.duplicates == 31`, `stats.blank_skipped == 15` (the owner's exact file — the bug's tombstone test);
|
||||
- `test_sources/pdf_horse_test.pdf` → exactly 1 image, not blank;
|
||||
- `test_sources/players_handbook.pdf` → images are returned by the parser but their raw mime is unmapped (`jpx`) → the importer's ext gate still skips them (assert the mime mapping, not the importer).
|
||||
Each guarded by `pytest.mark.skipif(not path.exists(), …)` — `test_sources/*.pdf` are untracked fixtures.
|
||||
3. Integration (`tests/unit/test_image_documents.py` or a new `tests/integration/test_pdf_image_pass.py`, whichever matches the existing image-pass tests' home): with `tests/fakes.py::FakeEmbedder` (the `_VisionMockLLM` idiom — a vision-capable fake that counts `describe_image` calls), a PDF containing one blank underlay + one real image + a duplicate placement indexes EXACTLY ONE image document and makes exactly ONE vision describe call; `summary.pdf_images_deduped` / `summary.pdf_images_blank_skipped` carry the counts; the `import: summary …` line includes them (caplog).
|
||||
4. Run the whole suite after each addition: `uv run pytest -q`, then `uv run pytest --cov=app --cov-report=term-missing` (>90% on `app/rag/pdf.py`, `app/rag/importer.py`, `app/config.py`).
|
||||
|
||||
## Testing & Quality
|
||||
- Coverage: **>90%** on the new/modified code paths (dedupe ×2, mask applied/failed/unsupported, blank skip/keep/knob-off, the stats plumbing).
|
||||
- No network, no real LLM (the fakes only); the real-PDF pins are pure CPU (PyMuPDF) and must stay under a few seconds each.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_pdf_image_filter.py tests/unit/test_pdf.py -v` green
|
||||
- [ ] the podman-systemd pin asserts 0 images out of 46 placements (the reported bug, pinned)
|
||||
- [ ] the `skip_blank=False` and unsupported-format (A4) paths are both covered
|
||||
- [ ] `uv run pytest -q` fully green; `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,36 @@
|
||||
# Task 06 — the dedicated E2E: a PDF sync produces no blank, no duplicate image docs
|
||||
|
||||
**Phase:** `151_pdf_image_extraction` · **Source:** `TODO.md:4` — "For the same podman pdf document, it extracted a series of blank white images for some reason. There don't appear to be any blank images in the document, is this a bug?"
|
||||
**Story:** n/a
|
||||
|
||||
## Objective
|
||||
Through the real product path (upload a PDF → press Sync → inspect the KB), a PDF containing a decorative underlay, a duplicate placement, and one real image yields exactly one image document — the owner's complaint, proven fixed in the browser.
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_pdf_image_extraction.py` (new; the `tests/e2e/test_pdf_support.py` / `test_pdf_image_context.py` per-suite-app pattern — a module-scoped app with `BOR_IMAGES=true` + the `pdf` import token, the shared E2E Postgres, the autouse truncate, the mock LLM only):
|
||||
- **fixture PDF built in the suite with PyMuPDF** (the house convention — no binary committed): 2 text pages, page 1 carrying (a) a near-white 200×200 "decorative underlay" image, (b) the SAME real image inserted twice (the duplicate placement), (c) one genuinely distinct colorful image; page 2 carrying one more placement of the real image (a cross-page duplicate — the podman case);
|
||||
- **seed** it through the real admin upload flow + the RAG page's **Sync** button (the same user path the owner used), then wait for the sync to settle on the sync result surface (post-phase-150: `#sync-progress-text`);
|
||||
- **assertions:**
|
||||
* the catalog contains exactly ONE image document for the PDF (the distinct real image, first placement) — count the `is_image` docs whose path starts with the PDF's synthetic path;
|
||||
* NO document's stored image bytes are a blank white image (fetch the served copy / read the row's `image_path` and assert the file's decoded pixels are not near-uniform — the same predicate the app uses, imported from `app.rag.pdf.is_blank_image`, so the test cannot drift from the product);
|
||||
* the synthetic path of the kept doc is the FIRST placement's (`…#page=1#img=<i>.png`) — the dedupe's stability contract;
|
||||
* the mock's vision call count for image description is 1 (the duplicate and the blank never reached the model — the cost the owner paid in "several minutes");
|
||||
* a follow-up re-sync (nothing changed) reports `unchanged` and creates nothing new (idempotence after the fix).
|
||||
- **negative control:** a second module app (or a second seed source) with `BOR_PDF_IMAGE_SKIP_BLANK=false` — the same PDF now yields the blank doc too (the knob is proven live in the product, not just in unit tests). If a second app is too heavy for the file's structure, drive the knob through the same env-fallback mechanism the other suites use for per-suite settings.
|
||||
2. Run it in isolation until green: `uv run pytest tests/e2e/test_pdf_image_extraction.py -v --no-cov` (DB up: `podman compose up -d db`).
|
||||
3. Regression run: the pre-existing PDF/image suites must stay green unedited (they pin the behaviors this phase must not disturb): `tests/e2e/test_pdf_support.py`, `test_pdf_image_context.py`, `test_pictorial_pdf.py`, `test_image_documents.py`, `test_image_anchor_context.py`, `test_answer_images.py`.
|
||||
4. Commit the phase per AGENTS.md rule 8 — one atomic Conventional Commit with `--no-gpg-sign`, e.g.
|
||||
`fix(rag): dedupe PDF images, apply soft masks, skip blank/decorative images`
|
||||
(moves `.agents/phases/todo/151_pdf_image_extraction/` to `complete/`).
|
||||
|
||||
## Testing & Quality
|
||||
- The dedicated E2E green IN ISOLATION (the phase gate) and the six pre-existing PDF/image E2Es green.
|
||||
- `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` >90%; `uv run ruff check . && uv run pyright` clean.
|
||||
- Mock-only suite (no real vision calls), deterministic (no sleeps as barriers — wait on the sync result surface).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/e2e/test_pdf_image_extraction.py -v --no-cov` green in isolation
|
||||
- [ ] exactly one image document per distinct real image; zero blank-white documents; one vision call
|
||||
- [ ] `BOR_PDF_IMAGE_SKIP_BLANK=false` demonstrably restores the old behavior in the product
|
||||
- [ ] the six pre-existing PDF/image E2Es green unedited; full suite green; coverage >90%
|
||||
- [ ] the phase directory moved to `.agents/phases/complete/` and committed with `--no-gpg-sign`
|
||||
Reference in New Issue
Block a user