phase: 140_pictorial_pdf_pages
Everything is verified green. Final report: ## Phase 140 final verification pass — report - Verified all 4 tasks already complete (code in working tree, task files in `complete/`): `pictorial_pages` gate + 2 settings (`BOR_PDF_PICTORIAL`, `BOR_PDF_PICTORIAL_MIN_CHARS` + validator), `PICTORIAL_PAGE_MODE`/`describe_pdf_page` seam + `render_pdf_pages` page selector + mock branch, `_pictorial_pdf_pass` in `_index_pdf_file` with log-only counters, dedicated E2E. No code changes needed. - Test results (exact commands): - `uv run pytest --cov=app --cov-report=term-missing` → **3320 passed, 11 skipped, 0 failed; TOTAL 99%** (>90% gate) - `uv run pytest tests/e2e/test_pictorial_pdf.py -v --no-cov` → **3 passed in isolation** (Playwright, mock-only) - Pre-existing suites unedited: `test_pdf_support.py` 6 passed; `test_garbage_pdf.py` 2 passed + 1 skipped (owner fixture); `test_pdf_image_context.py` 4 passed - `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors - Completion criteria: 1. IKEA tar import repro — pinned by 2 integration tests; **skipped loudly here** (`test_sources/ikea_instructions.tar` absent on this machine — verified repo-wide; same house convention as completed phase-133 HNB tests; runs real on the owner's machine). Equivalent logic fully covered by unit pins on synthetic IKEA-class (vector-drawn, zero-raster) PDFs. 2. Chat grounded on described page — **green** (E2E test b: phrase-only-in-illustration question, PDF in `done.sources`, answer echoes the block) 3. Clean-text no-op (zero vision calls) + garbage precedence pin — **green** (`test_zero_candidates_is_a_noop` unit; `test_garbage_pdf_with_pictorial_on_takes_the_fallback_not_the_pass` integration, verified alone) 4. `BOR_PDF_PICTORIAL=0` byte-identical — **green** (unit pin on synthetic PDF; IKEA variant runs on owner's machine) 5. Full gate + E2E isolation + pre-existing suites — **green** (above) 6. ruff + pyright — **clean** 7. Commit/phase-move — delegated to harness per run rules (left in working tree: 16 modified + 8 new files, uncommitted) - Deviations: none beyond the expected owner-fixture skips (11, all pre-existing fixture-missing skips, including 2 IKEA). - Next pending phase: **none** — `todo/` holds only 140; 139 is already complete.
This commit is contained in:
@@ -0,0 +1,450 @@
|
||||
"""Phase 140, task 02 — the ``PICTORIAL_PAGE_MODE`` vision seam + the
|
||||
``render_pdf_pages`` page selector (unit).
|
||||
|
||||
Pins the task-02 contracts:
|
||||
|
||||
* ``PICTORIAL_PAGE_MODE`` / ``PICTORIAL_INSTRUCTION`` /
|
||||
``PICTORIAL_PROMPT`` — the marker convention next to
|
||||
``IMAGE_DESCRIPTION_MODE`` (marker first, the deterministic E2E mock
|
||||
LLM keys on it, PLAN §6) with the LOCKED wording (phase 140 design:
|
||||
describe in detail what the page shows — the assembly steps, part
|
||||
names, arrows, tool icons, before/after states — ESPECIALLY the
|
||||
information the extracted text does not contain).
|
||||
* ``DESCRIBE_PROMPT`` byte-identity (regression pin — the phase 122/126/
|
||||
137 suites pass unedited; the pictorial pass has its OWN marker, it
|
||||
never re-wraps the image prompt).
|
||||
* :func:`describe_pdf_page` — the same one-chat-call multimodal
|
||||
contract as :func:`describe_image`: a capturing fake LLM pins the
|
||||
message shape (the text part starts with ``PICTORIAL_PROMPT``, then
|
||||
the page block — the page's own text as context, the
|
||||
``"(none)`` case; the ``image_url`` part is the data URL from the
|
||||
PNG bytes + mime), the CHAT (vision) model (never ``lite``), the
|
||||
phase-30 cap, and ``None`` on an empty reply or a client error
|
||||
(fail-soft — one dead page never stalls a sync).
|
||||
* :func:`render_pdf_pages`' optional 1-based *pages* selector — the
|
||||
subset's count + ascending order + PNG magic bytes (a 3-page
|
||||
fixture, the phase-126/133 recipe), ``None`` = every page (the
|
||||
phase-133 caller stays byte-identical), an unreadable/out-of-range
|
||||
named page → the zero-byte placeholder at its position, an
|
||||
unopenable file → ``[]``.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import base64
|
||||
import logging
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from typing import Any
|
||||
|
||||
import pymupdf as fitz
|
||||
import pytest
|
||||
|
||||
from app.config import Settings
|
||||
from app.rag import pdf as pdf_mod
|
||||
from app.rag.llm import LLMError
|
||||
from app.rag.pdf import render_pdf_pages
|
||||
from app.rag.summarizer import (
|
||||
DESCRIBE_PROMPT,
|
||||
IMAGE_DESCRIPTION_MODE,
|
||||
PICTORIAL_INSTRUCTION,
|
||||
PICTORIAL_PAGE_MODE,
|
||||
PICTORIAL_PROMPT,
|
||||
describe_pdf_page,
|
||||
)
|
||||
|
||||
PNG_MAGIC = b"\x89PNG"
|
||||
|
||||
#: A tiny non-empty PNG-ish payload for the describe-call's ``data``
|
||||
#: (the seam is content-agnostic — it never parses the image; a
|
||||
#: well-formed magic keeps the data-URL assertion honest).
|
||||
PNG_1X1 = base64.b64decode(
|
||||
"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVR4nGP4DwQACfsD/fteaysAAAAASUVORK5CYII="
|
||||
)
|
||||
|
||||
|
||||
def _settings(**kwargs: Any) -> Settings:
|
||||
"""Build Settings without reading a .env file (deterministic tests)."""
|
||||
kwargs.setdefault("_env_file", None)
|
||||
return Settings(**kwargs) # pyright: ignore[reportCallIssue] (kwarg exists at runtime)
|
||||
|
||||
|
||||
class _VisionMock:
|
||||
"""Duck-typed chat-model mock for :func:`describe_pdf_page` (task
|
||||
02): records the message + model and returns a canned reply (or
|
||||
raises it). Satisfies the summarizer's ``SummaryLLM`` protocol.
|
||||
"""
|
||||
|
||||
def __init__(
|
||||
self, reply: str | Exception = "Two parts joined with an arrow."
|
||||
) -> None:
|
||||
self.settings = _settings()
|
||||
self.reply = reply
|
||||
self.messages: list[Any] = []
|
||||
self.model: str | None = None
|
||||
|
||||
async def chat(
|
||||
self, messages: list[dict[str, Any]], model: str | None = None
|
||||
) -> str:
|
||||
self.messages = list(messages)
|
||||
self.model = model
|
||||
if isinstance(self.reply, Exception):
|
||||
raise self.reply
|
||||
return self.reply
|
||||
|
||||
|
||||
def _message(mock: _VisionMock) -> list[dict[str, Any]]:
|
||||
"""The one user message's multimodal ``content`` list (asserted to
|
||||
be a list of two parts by the shape test)."""
|
||||
assert len(mock.messages) == 1
|
||||
message = mock.messages[0]
|
||||
assert message["role"] == "user"
|
||||
content = message["content"]
|
||||
assert isinstance(content, list)
|
||||
return content
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The marker + the locked instruction (the mock's key, PLAN §6)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_pictorial_prompt_carries_the_mode_marker_and_locked_wording() -> None:
|
||||
"""The ``PICTORIAL_PAGE_MODE`` marker heads the pictorial prompt
|
||||
(the ``SUMMARY_MODE``/``IMAGE_DESCRIPTION_MODE`` convention — the
|
||||
deterministic E2E mock LLM keys on it), and the LOCKED wording's
|
||||
load-bearing fragments are in the instruction: a re-wrap of the
|
||||
prompt cannot silently re-route the mock, and the "describe what
|
||||
the text does NOT contain" mandate stays in the constant."""
|
||||
assert PICTORIAL_PAGE_MODE == "PICTORIAL_PAGE_MODE"
|
||||
assert PICTORIAL_PROMPT.startswith(PICTORIAL_PAGE_MODE + ": ")
|
||||
assert (
|
||||
PICTORIAL_PROMPT == PICTORIAL_PAGE_MODE + ": " + PICTORIAL_INSTRUCTION
|
||||
)
|
||||
for fragment in (
|
||||
"assembly steps",
|
||||
"part names",
|
||||
"arrows",
|
||||
"tool icons",
|
||||
"before/after states",
|
||||
"ESPECIALLY",
|
||||
"extracted text",
|
||||
"plain text",
|
||||
):
|
||||
assert fragment in PICTORIAL_PROMPT
|
||||
# The markers are disjoint fixed app constants (neither contains
|
||||
# the other) — the mock's two vision branches cannot shadow each
|
||||
# other regardless of branch order.
|
||||
assert IMAGE_DESCRIPTION_MODE not in PICTORIAL_PROMPT
|
||||
assert PICTORIAL_PAGE_MODE not in DESCRIBE_PROMPT
|
||||
|
||||
|
||||
def test_describe_prompt_byte_identity_regression_pin() -> None:
|
||||
"""REGRESSION PIN: the phase-122 ``DESCRIBE_PROMPT`` is untouched
|
||||
by the pictorial seam (the phase 122/126/137 suites pass unedited —
|
||||
the image marker's mock branch serves the exact pre-phase-140
|
||||
prompt). The full value is pinned, not just its shape: any
|
||||
re-wrap of the locked instruction fails here, not in the story
|
||||
suites."""
|
||||
assert DESCRIBE_PROMPT == (
|
||||
"IMAGE_DESCRIPTION_MODE: Describe this image faithfully, in "
|
||||
"plain text, for a search index. State what is depicted, "
|
||||
"transcribe any visible text, labels, or titles, describe the "
|
||||
"structure of any diagram, table, or layout, and call out the "
|
||||
"most salient details. Write 2-4 sentences of substance. Do not "
|
||||
"use markdown. Do not invent anything that is not visible in "
|
||||
"the image. Your description is the ONLY text that will ever be "
|
||||
"retrieved for this image — it must carry the image's full "
|
||||
"meaning."
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# describe_pdf_page — message shape, model, cap, fail-soft
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_describe_pdf_page_message_shape_multimodal_and_chat_model() -> None:
|
||||
"""ONE chat-model call (LOCKED A2 — the vision ``llm_chat_model``,
|
||||
NOT the ``lite`` summary model) with the OpenAI-compatible
|
||||
multimodal user message: the text part is exactly
|
||||
``PICTORIAL_PROMPT + "\\n\\nPage {page} of the PDF. Text extracted
|
||||
from this page:\\n" + page_text.strip()`` (the page's OWN text as
|
||||
context, so the model describes what the text does NOT carry) and
|
||||
the ``image_url`` part is the data URL ``data:<mime>;base64,<png
|
||||
bytes>`` — no system prompt, no tools."""
|
||||
mock = _VisionMock("Two parts joined with an arrow.")
|
||||
result = asyncio.run(
|
||||
describe_pdf_page(
|
||||
mock,
|
||||
data=PNG_1X1,
|
||||
mime="image/png",
|
||||
page=7,
|
||||
page_text="Illustration 7",
|
||||
)
|
||||
)
|
||||
assert result == "Two parts joined with an arrow."
|
||||
content = _message(mock)
|
||||
assert len(content) == 2
|
||||
assert content[0] == {
|
||||
"type": "text",
|
||||
"text": (
|
||||
PICTORIAL_PROMPT
|
||||
+ "\n\nPage 7 of the PDF. Text extracted from this page:\n"
|
||||
+ "Illustration 7"
|
||||
),
|
||||
}
|
||||
b64 = base64.b64encode(PNG_1X1).decode("ascii")
|
||||
assert content[1] == {
|
||||
"type": "image_url",
|
||||
"image_url": {"url": f"data:image/png;base64,{b64}"},
|
||||
}
|
||||
# LOCKED A2: the CHAT (vision) model — never the ``lite`` summary
|
||||
# model (it is not assumed vision-capable).
|
||||
assert mock.model == mock.settings.llm_chat_model
|
||||
assert mock.model != mock.settings.llm_summary_model
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("page_text", "suffix"),
|
||||
[
|
||||
(None, "(none)"),
|
||||
("", "(none)"),
|
||||
(" \t\n ", "(none)"),
|
||||
(" Illustration 7 ", "Illustration 7"),
|
||||
],
|
||||
)
|
||||
def test_describe_pdf_page_page_block_none_and_strip_cases(
|
||||
page_text: str | None, suffix: str,
|
||||
) -> None:
|
||||
"""The page block's tail is ``page_text.strip() or "(none)"`` — a
|
||||
page with no extracted text (``None``/empty/blank) sends the
|
||||
explicit ``(none)`` sentinel (the vision model knows there is
|
||||
nothing to restate), and a text-carrying page sends its STRIPPED
|
||||
text (the leading/trailing whitespace of the extraction never
|
||||
reaches the model)."""
|
||||
mock = _VisionMock("ok")
|
||||
asyncio.run(
|
||||
describe_pdf_page(
|
||||
mock, data=PNG_1X1, mime="image/png", page=3, page_text=page_text
|
||||
)
|
||||
)
|
||||
content = _message(mock)
|
||||
assert content[0]["text"] == (
|
||||
PICTORIAL_PROMPT
|
||||
+ f"\n\nPage 3 of the PDF. Text extracted from this page:\n{suffix}"
|
||||
)
|
||||
|
||||
|
||||
def test_describe_pdf_page_strips_and_caps_at_summary_max_chars() -> None:
|
||||
"""The reply is whitespace-stripped and cut exactly at
|
||||
``summary_max_chars`` (the phase-30 cap — the same uniform ceiling
|
||||
as :func:`describe_image`). The explicit ``settings`` parameter
|
||||
wins; ``settings=None`` falls back to the LLM's own settings."""
|
||||
long_reply = "x" * 500
|
||||
mock = _VisionMock(long_reply)
|
||||
capped = asyncio.run(
|
||||
describe_pdf_page(
|
||||
mock,
|
||||
data=PNG_1X1,
|
||||
mime="image/png",
|
||||
page=2,
|
||||
page_text=None,
|
||||
settings=_settings(summary_max_chars=100),
|
||||
)
|
||||
)
|
||||
assert capped == "x" * 100
|
||||
mock2 = _VisionMock(" " + "y" * 50 + "\n")
|
||||
mock2.settings = _settings(summary_max_chars=42)
|
||||
result = asyncio.run(
|
||||
describe_pdf_page(mock2, data=PNG_1X1, mime="image/png", page=2, page_text=None)
|
||||
)
|
||||
assert result == ("y" * 42)
|
||||
|
||||
|
||||
def test_describe_pdf_page_returns_none_on_failure_classes(
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""Fail-soft (LOCKED A2 — one dead page never stalls a sync):
|
||||
``None`` on a client error (with a warning log naming the page —
|
||||
the house one-shot style) and on an empty or whitespace-only reply
|
||||
(the house one-shot empty-content policy)."""
|
||||
failing = _VisionMock(LLMError("simulated vision-model failure (test sentinel)"))
|
||||
with caplog.at_level(logging.WARNING, logger="app.summarizer"):
|
||||
assert (
|
||||
asyncio.run(
|
||||
describe_pdf_page(
|
||||
failing, data=PNG_1X1, mime="image/png", page=9, page_text=None
|
||||
)
|
||||
)
|
||||
is None
|
||||
)
|
||||
warnings = [
|
||||
r
|
||||
for r in caplog.records
|
||||
if r.levelno == logging.WARNING and "pictorial page description" in r.getMessage()
|
||||
]
|
||||
assert len(warnings) == 1 and "page=9" in warnings[0].getMessage()
|
||||
for empty in ("", " \n"):
|
||||
blank = _VisionMock(empty)
|
||||
assert (
|
||||
asyncio.run(
|
||||
describe_pdf_page(
|
||||
blank, data=PNG_1X1, mime="image/png", page=9, page_text=None
|
||||
)
|
||||
)
|
||||
is None
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# render_pdf_pages — the optional 1-based page selector
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _three_page_text_pdf(path: Path) -> Path:
|
||||
"""A 3-page temp PDF with EXPLICIT letter pages (612x792pt —
|
||||
deterministic render sizes) and page-specific text (the render must
|
||||
be page-specific, so the pages' PNG bytes must differ — the
|
||||
phase-126/133 recipe, extended to three pages for the selector's
|
||||
count + order assertions)."""
|
||||
doc = fitz.open()
|
||||
for marker in ("Alpha page text.", "Beta page text.", "Gamma page text."):
|
||||
doc.new_page(width=612, height=792).insert_text((72, 72), marker)
|
||||
doc.save(path)
|
||||
doc.close()
|
||||
return path
|
||||
|
||||
|
||||
def test_render_selector_none_renders_every_page(tmp_path: Path) -> None:
|
||||
"""``pages=None`` (the default) = every page in page order — the
|
||||
phase-133 caller's result, byte-identical: a 3-page PDF → 3 PNGs,
|
||||
each a genuine decodable PNG, page-specific (the bytes differ)."""
|
||||
path = _three_page_text_pdf(tmp_path / "three.pdf")
|
||||
pages = render_pdf_pages(path)
|
||||
assert len(pages) == 3
|
||||
for png in pages:
|
||||
assert png.startswith(PNG_MAGIC)
|
||||
pix = fitz.Pixmap(png)
|
||||
assert (pix.width, pix.height) == (1275, 1650) # 150-DPI letter
|
||||
assert pages[0] != pages[1] != pages[2] and pages[0] != pages[2]
|
||||
|
||||
|
||||
def test_render_selector_subset_count_ascending_order_and_pngs(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""A named subset renders ONLY the named pages (3-page PDF,
|
||||
``pages=[3, 1]`` → 2 PNGs, not 3), in ASCENDING order regardless of
|
||||
the input order (list index *i* is the *i*-th smallest NAMED page —
|
||||
index 0 is page 1, index 1 is page 3), each a genuine PNG whose
|
||||
bytes equal the corresponding page's render in the full
|
||||
(``pages=None``) pass — the selector changes the set, never the
|
||||
per-page output."""
|
||||
path = _three_page_text_pdf(tmp_path / "three.pdf")
|
||||
full = render_pdf_pages(path)
|
||||
selected = render_pdf_pages(path, pages=[3, 1])
|
||||
assert len(selected) == 2
|
||||
for png in selected:
|
||||
assert png.startswith(PNG_MAGIC)
|
||||
pix = fitz.Pixmap(png)
|
||||
assert (pix.width, pix.height) == (1275, 1650) # 150-DPI letter
|
||||
assert selected[0] == full[0] # the smallest named page (1) first
|
||||
assert selected[1] == full[2] # page 3 second, page 2 rendered by no one
|
||||
single = render_pdf_pages(path, pages=[2])
|
||||
assert single == [full[1]]
|
||||
|
||||
|
||||
def test_render_selector_out_of_range_named_page_is_zero_bytes(
|
||||
tmp_path: Path, caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""A named page outside the document's page range yields ``b""`` at
|
||||
ITS position (the caller's placeholder — the result's order never
|
||||
shifts, so it stays aligned with the requested pages) + a warning;
|
||||
the in-range page keeps its PNG."""
|
||||
path = _three_page_text_pdf(tmp_path / "three.pdf")
|
||||
full = render_pdf_pages(path)
|
||||
with caplog.at_level(logging.WARNING, logger="app.rag.pdf"):
|
||||
selected = render_pdf_pages(path, pages=[2, 99])
|
||||
assert selected == [full[1], b""]
|
||||
warnings = [
|
||||
r
|
||||
for r in caplog.records
|
||||
if r.levelno == logging.WARNING and "could not render page 99" in r.getMessage()
|
||||
]
|
||||
assert len(warnings) == 1
|
||||
|
||||
|
||||
def test_render_selector_empty_list_renders_nothing(tmp_path: Path) -> None:
|
||||
"""``pages=[]`` (no candidates) renders nothing — the zero-
|
||||
candidates no-op the importer relies on (a clean-text PDF's path
|
||||
never burns a render or a vision call)."""
|
||||
path = _three_page_text_pdf(tmp_path / "three.pdf")
|
||||
assert render_pdf_pages(path, pages=[]) == []
|
||||
|
||||
|
||||
def test_render_selector_unopenable_file_returns_empty(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""A file that cannot be OPENED yields ``[]`` even with a selector
|
||||
(the caller's existing empty-content path — unchanged by the
|
||||
phase-140 parameter)."""
|
||||
bad = tmp_path / "bad.pdf"
|
||||
bad.write_bytes(b"this is not a pdf at all")
|
||||
assert render_pdf_pages(bad, pages=[1, 2]) == []
|
||||
assert render_pdf_pages(tmp_path / "missing.pdf", pages=[1]) == []
|
||||
|
||||
|
||||
def test_render_selector_broken_named_page_is_zero_bytes(
|
||||
monkeypatch: pytest.MonkeyPatch, tmp_path: Path,
|
||||
) -> None:
|
||||
"""A named page whose ``get_pixmap`` raises → ``b""`` at ITS
|
||||
position (the caller's placeholder slot — the page index NEVER
|
||||
shifts, so the result stays aligned with the requested pages); the
|
||||
good named pages keep their PNGs, and the failure is logged (the
|
||||
:func:`extract_pdf_text` fail-soft style)."""
|
||||
path = tmp_path / "fake.pdf"
|
||||
path.write_bytes(b"%PDF-1.4 fake")
|
||||
|
||||
class _RenderPage:
|
||||
def __init__(self, data: bytes | None) -> None:
|
||||
self._data = data
|
||||
|
||||
def get_pixmap(self, dpi: int) -> Any:
|
||||
if self._data is None:
|
||||
raise RuntimeError("boom: page render failed")
|
||||
return SimpleNamespace(tobytes=lambda _fmt: self._data)
|
||||
|
||||
class _FakeDoc:
|
||||
def __init__(self, pages: list[Any]) -> None:
|
||||
self._pages = pages
|
||||
|
||||
def __len__(self) -> int:
|
||||
return len(self._pages)
|
||||
|
||||
def __getitem__(self, index: int) -> Any:
|
||||
return self._pages[index]
|
||||
|
||||
def __enter__(self) -> _FakeDoc: # noqa: F821 (nested class, future annotations)
|
||||
return self
|
||||
|
||||
def __exit__(self, *args: object) -> bool:
|
||||
return False
|
||||
|
||||
doc = _FakeDoc(
|
||||
[
|
||||
_RenderPage(b"\x89PNG-fake-page-1"),
|
||||
_RenderPage(None),
|
||||
_RenderPage(b"\x89PNG-fake-page-3"),
|
||||
]
|
||||
)
|
||||
monkeypatch.setattr(pdf_mod.fitz, "open", lambda _p: doc)
|
||||
# pages=[3, 1, 2] renders ascending 1, 2, 3 — the broken page 2
|
||||
# holds ITS position (index 1), not the tail.
|
||||
assert render_pdf_pages(path, pages=[3, 1, 2]) == [
|
||||
b"\x89PNG-fake-page-1",
|
||||
b"",
|
||||
b"\x89PNG-fake-page-3",
|
||||
]
|
||||
# A repeated page number renders ONCE (the selector is a set of
|
||||
# pages, ascending).
|
||||
assert render_pdf_pages(path, pages=[1, 1]) == [b"\x89PNG-fake-page-1"]
|
||||
Reference in New Issue
Block a user