From ee402ea00ed69468e706116908d7f72923f82c61 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 21:16:00 +0200 Subject: [PATCH 1/4] feat(vector): replace inline page-image payloads with chunk_bbox (Deck #76) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-chunk PDF page renders (~150–700 KB base64 PNG each) were the dominant disk consumer in production, repeatedly tripping `No space left on device: WAL buffer size exceeds available disk space` on welcomed-malamute Qdrant. Replace the inline highlighted_page_image / highlighted_page_number / highlight_count fields with a small `chunk_bbox` field: list[(x0, y0, x1, y1)] of normalized [0, 1] floats, ~32 bytes per chunk. Astrolabe (the only known consumer) renders the highlight client-side as a percentage-positioned overlay on top of the existing /api/v1/pdf-preview render-on-demand path (cbcoutinho/astrolabe#76). - pdf_highlighter: new compute_chunk_bboxes_batch() that reuses the existing _find_chunk_bbox text-search path, skipping all pixmap/PIL/PNG work. - processor: store chunk_bbox + chunk_bbox_page in the Qdrant payload, drop highlighted_page_image + friends, drop the base64 import. - visualization /api/v1/chunk-context and auth/viz_routes: read chunk_bbox instead of highlighted_page_image. - vector/__init__: stop eagerly re-exporting `processor`/`scanner` — fixes a pre-existing circular import (search.algorithms -> vector.placeholder -> vector/__init__ -> processor -> scanner -> server.semantic -> search.bm25_hybrid -> search.algorithms partial). Test suite that was broken on master (test_bm25_hybrid.py et al.) now collects and passes. - scripts/purge_page_images.py: ad-hoc, idempotent migration that delete_payload's the legacy keys from existing points. No reindex required; legacy chunks render the page with no overlay. Pairs with cbcoutinho/astrolabe#76. Frontend handles missing chunk_bbox gracefully, so this can land in either order. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 18 +-- nextcloud_mcp_server/app.py | 3 +- nextcloud_mcp_server/auth/viz_routes.py | 26 ++-- .../search/pdf_highlighter.py | 105 ++++++++++++++ nextcloud_mcp_server/vector/__init__.py | 17 +-- nextcloud_mcp_server/vector/processor.py | 63 +++----- scripts/purge_page_images.py | 136 ++++++++++++++++++ .../unit/search/test_pdf_highlighter_bbox.py | 132 +++++++++++++++++ 8 files changed, 428 insertions(+), 72 deletions(-) create mode 100755 scripts/purge_page_images.py create mode 100644 tests/unit/search/test_pdf_highlighter_bbox.py diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 242c7c13..3ce60d8e 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -553,9 +553,10 @@ async def get_chunk_context(request: Request) -> JSONResponse: status_code=404, ) - # For PDF files, also fetch the highlighted page image from Qdrant if available - # This is useful for clients that want to show a pre-rendered image - highlighted_page_image = None + # For PDF files, also fetch the chunk's bounding box from Qdrant if + # available so the client can overlay a highlight on top of a + # render-on-demand page image (Deck #76). + chunk_bbox = None page_number = chunk_context.page_number if doc_type == "file": @@ -563,7 +564,6 @@ async def get_chunk_context(request: Request) -> JSONResponse: settings = get_settings() qdrant_client = await get_qdrant_client() - # Query for this specific chunk's highlighted image points_response = await qdrant_client.scroll( collection_name=settings.get_collection_name(), scroll_filter=Filter( @@ -585,19 +585,19 @@ async def get_chunk_context(request: Request) -> JSONResponse: ), limit=1, with_vectors=False, - with_payload=["highlighted_page_image", "page_number"], + with_payload=["chunk_bbox", "page_number"], ) if points_response[0]: payload = points_response[0][0].payload if payload: - highlighted_page_image = payload.get("highlighted_page_image") + chunk_bbox = payload.get("chunk_bbox") # Trust Qdrant page number if available (might be more accurate than context expansion logic) if payload.get("page_number") is not None: page_number = payload.get("page_number") except Exception as e: - logger.warning(f"Failed to fetch highlighted image: {e}") + logger.warning(f"Failed to fetch chunk bbox: {e}") # Build response response_data = { @@ -612,8 +612,8 @@ async def get_chunk_context(request: Request) -> JSONResponse: "total_chunks": chunk_context.total_chunks, } - if highlighted_page_image: - response_data["highlighted_page_image"] = highlighted_page_image + if chunk_bbox: + response_data["chunk_bbox"] = chunk_bbox return JSONResponse(response_data) diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 3fb9194f..d2c1d1ab 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -125,12 +125,13 @@ from nextcloud_mcp_server.server import ( ) from nextcloud_mcp_server.server.auth_tools import register_auth_tools from nextcloud_mcp_server.server.oauth_tools import register_oauth_tools -from nextcloud_mcp_server.vector import processor_task, scanner_task from nextcloud_mcp_server.vector.oauth_sync import ( oauth_processor_task, user_manager_task, ) +from nextcloud_mcp_server.vector.processor import processor_task from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client +from nextcloud_mcp_server.vector.scanner import scanner_task from nextcloud_mcp_server.vector.webhook_receiver import handle_nextcloud_webhook logger = logging.getLogger(__name__) diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index d072c373..35f286f9 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -604,8 +604,10 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: f"after_len={len(chunk_context.after_context)}" ) - # For PDF files, also fetch the highlighted page image from Qdrant - highlighted_page_image = None + # For PDF files, also fetch the chunk bbox from Qdrant so the client + # can overlay a highlight on top of a render-on-demand page image + # (Deck #76). + chunk_bbox = None page_number = None if doc_type == "file": try: @@ -613,7 +615,6 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: qdrant_client = await get_qdrant_client() username = request.user.display_name - # Query for this specific chunk's highlighted image points_response = await qdrant_client.scroll( collection_name=settings.get_collection_name(), scroll_filter=Filter( @@ -635,22 +636,20 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: ), limit=1, with_vectors=False, - with_payload=["highlighted_page_image", "page_number"], + with_payload=["chunk_bbox", "page_number"], ) points = points_response[0] if points and points[0].payload: - highlighted_page_image = points[0].payload.get( - "highlighted_page_image" - ) + chunk_bbox = points[0].payload.get("chunk_bbox") page_number = points[0].payload.get("page_number") - if highlighted_page_image: + if chunk_bbox: logger.info( - f"Found highlighted image for chunk: " - f"page={page_number}, image_size={len(highlighted_page_image)}" + f"Found chunk bbox: page={page_number}, " + f"rects={len(chunk_bbox)}" ) except Exception as e: - logger.warning(f"Failed to fetch highlighted image: {e}") + logger.warning(f"Failed to fetch chunk bbox: {e}") # Return response compatible with frontend expectations response_data: dict = { @@ -662,9 +661,8 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: "has_more_after": chunk_context.has_after_truncation, } - # Add image data if available - if highlighted_page_image: - response_data["highlighted_page_image"] = highlighted_page_image + if chunk_bbox: + response_data["chunk_bbox"] = chunk_bbox response_data["page_number"] = page_number return JSONResponse(response_data) diff --git a/nextcloud_mcp_server/search/pdf_highlighter.py b/nextcloud_mcp_server/search/pdf_highlighter.py index 3c8212a3..6dbaea9a 100644 --- a/nextcloud_mcp_server/search/pdf_highlighter.py +++ b/nextcloud_mcp_server/search/pdf_highlighter.py @@ -691,6 +691,111 @@ class PDFHighlighter: f"Failed to delete temp directory {temp_pdf_path.parent}: {e}" ) + @staticmethod + def compute_chunk_bboxes_batch( + pdf_bytes: bytes, + chunks: list[tuple[int, int, int, int | None, str]], + page_boundaries: list[dict], + full_text: str, + ) -> dict[int, tuple[list[tuple[float, float, float, float]], int]]: + """Compute normalized bounding boxes for chunks without rendering. + + Lightweight alternative to highlight_chunks_batch — opens the PDF, + locates each chunk on its assigned page using the same text-search + path as the highlighter (`_find_chunk_bbox`), and returns + page-normalized rectangles. Skips the get_pixmap + PIL pipeline + entirely, so no PNG bytes are produced. + + Args: + pdf_bytes: PDF file bytes. + chunks: List of (chunk_index, start_offset, end_offset, + stored_page_number, chunk_text). chunk_index is the dict key. + page_boundaries: Pre-computed page boundaries from the document + processor; each entry is {"page", "start_offset", "end_offset"}. + full_text: Full document text (for cross-page chunk handling). + + Returns: + dict mapping chunk_index to (normalized_bboxes, page_number). + Each bbox is (x0, y0, x1, y1) in [0, 1] relative to page width + and height, top-left origin. Chunks whose bbox cannot be located + are omitted from the result. + """ + results: dict[int, tuple[list[tuple[float, float, float, float]], int]] = {} + + if not chunks: + return results + + temp_pdf_path = None + try: + temp_dir = Path(tempfile.mkdtemp(prefix="pdf_bbox_batch_")) + temp_pdf_path = temp_dir / "pdf.pdf" + temp_pdf_path.write_bytes(pdf_bytes) + + doc = pymupdf.open(temp_pdf_path) + + for ( + chunk_index, + start_offset, + end_offset, + stored_page_num, + chunk_text, + ) in chunks: + chunk_page_info = PDFHighlighter.find_chunk_page( + start_offset, end_offset, page_boundaries + ) + if not chunk_page_info: + logger.debug(f"Chunk {chunk_index}: not found on any page") + continue + + page_num = chunk_page_info["page_num"] + page_boundary = page_boundaries[page_num - 1] + page_text_length = ( + page_boundary["end_offset"] - page_boundary["start_offset"] + ) + + # Page-relative slice (handles chunks that span page boundaries) + chunk_start_on_page = max(start_offset, page_boundary["start_offset"]) + chunk_end_on_page = min(end_offset, page_boundary["end_offset"]) + page_relative_text = full_text[chunk_start_on_page:chunk_end_on_page] + + page = doc[page_num - 1] + bbox = PDFHighlighter._find_chunk_bbox( + page, + page_relative_text, + chunk_page_info["page_relative_start"], + chunk_page_info["page_relative_end"], + page_text_length, + ) + + if bbox is None: + continue + + page_rect = page.rect + w = page_rect.width or 1.0 + h = page_rect.height or 1.0 + normalized = ( + bbox[0] / w, + bbox[1] / h, + bbox[2] / w, + bbox[3] / h, + ) + results[chunk_index] = ([normalized], page_num) + + doc.close() + logger.info(f"Computed bboxes for {len(results)}/{len(chunks)} chunks") + return results + + except Exception as e: + logger.error(f"Error computing chunk bboxes: {e}", exc_info=True) + return results + + finally: + if temp_pdf_path and temp_pdf_path.parent.exists(): + try: + shutil.rmtree(temp_pdf_path.parent) + except Exception as e: + logger.warning(f"Failed to clean up temp dir: {e}") + @staticmethod def highlight_chunks_batch( pdf_bytes: bytes, diff --git a/nextcloud_mcp_server/vector/__init__.py b/nextcloud_mcp_server/vector/__init__.py index 00c11cbe..8f83ef6e 100644 --- a/nextcloud_mcp_server/vector/__init__.py +++ b/nextcloud_mcp_server/vector/__init__.py @@ -1,16 +1,17 @@ -"""Vector database and background sync package.""" +"""Vector database and background sync package. + +`processor` and `scanner` are intentionally NOT re-exported from this +package init: they transitively import `server.semantic` -> +`search.bm25_hybrid`, which forms an import cycle with +`search.algorithms` -> `vector.placeholder` -> `vector/__init__`. +Consumers that need those symbols import them from their submodules +directly (e.g. `from nextcloud_mcp_server.vector.processor import ...`). +""" from .document_chunker import DocumentChunker -from .processor import process_document, processor_task from .qdrant_client import get_qdrant_client -from .scanner import DocumentTask, scan_user_documents, scanner_task __all__ = [ "get_qdrant_client", "DocumentChunker", - "scanner_task", - "scan_user_documents", - "DocumentTask", - "processor_task", - "process_document", ] diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 848b8098..3d6542ba 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -3,7 +3,6 @@ Processes documents from stream: fetches content, generates embeddings, stores in Qdrant. """ -import base64 import logging import time import uuid @@ -538,7 +537,9 @@ async def _index_document( # Initialize results containers dense_embeddings: list = [] sparse_embeddings: list = [] - chunk_images: dict[int, dict] = {} + # chunk_index -> {"bbox": list[(x0,y0,x1,y1)], "page": int} + # Bboxes are normalized to [0, 1] relative to page width/height. + chunk_bboxes: dict[int, dict] = {} # Determine if we need PDF highlighting is_pdf = doc_task.doc_type == "file" and content_type == "application/pdf" @@ -570,8 +571,8 @@ async def _index_document( sparse_embeddings = await bm25_service.encode_batch(chunk_texts) async def generate_highlights(): - """Generate highlighted page images for PDF chunks (CPU-bound).""" - nonlocal chunk_images + """Compute chunk bounding boxes for PDF chunks (CPU-bound, no rendering).""" + nonlocal chunk_bboxes if not is_pdf: return @@ -585,58 +586,39 @@ async def _index_document( "vector_sync.pdf_size": len(content_bytes), }, ): - # Build chunk data for batch processing - # Format: (chunk_index, start_offset, end_offset, page_number, chunk_text) chunk_data: list[tuple[int, int, int, int | None, str]] = [ (i, chunk.start_offset, chunk.end_offset, chunk.page_number, chunk.text) for i, chunk in enumerate(chunks) if chunk.page_number is not None ] - # Get pre-computed page boundaries from document processor page_boundaries = file_metadata.get("page_boundaries") if not page_boundaries: - logger.warning("No page boundaries available, skipping highlighting") + logger.warning( + "No page boundaries available, skipping bbox computation" + ) return - # Type narrowing: page_boundaries is guaranteed to be list[dict] here page_boundaries_list = cast(list[dict[str, Any]], page_boundaries) - logger.info( - f"Batch generating highlighted page images for {len(chunk_data)} PDF chunks" - ) + logger.info(f"Computing chunk bboxes for {len(chunk_data)} PDF chunks") - # Run CPU-bound highlighting in thread pool - # Pass pre-computed page boundaries and full text to avoid re-processing the PDF batch_results = await anyio.to_thread.run_sync( # type: ignore[attr-defined] - lambda: PDFHighlighter.highlight_chunks_batch( + lambda: PDFHighlighter.compute_chunk_bboxes_batch( pdf_bytes=content_bytes, chunks=chunk_data, page_boundaries=page_boundaries_list, full_text=content, - color="yellow", - zoom=2.0, ) ) - # Convert results to storage format - for chunk_index, ( - png_bytes, - actual_page_num, - highlight_count, - ) in batch_results.items(): - image_base64 = base64.b64encode(png_bytes).decode("utf-8") - chunk_images[chunk_index] = { - "image": image_base64, + for chunk_index, (bboxes, actual_page_num) in batch_results.items(): + chunk_bboxes[chunk_index] = { + "bbox": bboxes, "page": actual_page_num, - "highlights": highlight_count, - "size": len(png_bytes), } - logger.info( - f"Generated {len(chunk_images)}/{len(chunks)} highlighted page images " - f"(avg {sum(img['size'] for img in chunk_images.values()) // max(len(chunk_images), 1):,} bytes)" - ) + logger.info(f"Computed bboxes for {len(chunk_bboxes)}/{len(chunks)} chunks") # Run all embedding/highlighting operations in parallel # - Dense embeddings: I/O bound (API call) @@ -752,14 +734,15 @@ async def _index_document( if doc_task.doc_type == "deck_card" else {} ), - # Highlighted page image (PDF only) + # Chunk bbox (PDF only) — normalized rectangles in [0,1] + # relative to page width/height. Replaces the legacy + # `highlighted_page_image` (Deck #76). **( { - "highlighted_page_image": chunk_images[i]["image"], - "highlighted_page_number": chunk_images[i]["page"], - "highlight_count": chunk_images[i]["highlights"], + "chunk_bbox": chunk_bboxes[i]["bbox"], + "chunk_bbox_page": chunk_bboxes[i]["page"], } - if i in chunk_images + if i in chunk_bboxes else {} ), }, @@ -781,14 +764,14 @@ async def _index_document( ) # Upsert to Qdrant in batches to avoid timeout with large payloads - # Each batch is limited to avoid WriteTimeout when sending large image payloads - BATCH_SIZE = 10 # ~2MB per batch with images + # Batch size kept small for safety; payloads are now small (no inline images). + BATCH_SIZE = 10 with trace_operation( "vector_sync.qdrant_upsert", attributes={ "vector_sync.point_count": len(points), "vector_sync.collection": settings.get_collection_name(), - "vector_sync.images_count": len(chunk_images), + "vector_sync.bboxes_count": len(chunk_bboxes), "vector_sync.batch_size": BATCH_SIZE, }, ): diff --git a/scripts/purge_page_images.py b/scripts/purge_page_images.py new file mode 100755 index 00000000..2f4ce77b --- /dev/null +++ b/scripts/purge_page_images.py @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +"""Purge legacy `highlighted_page_image` payloads from Qdrant (Deck #76). + +Iterates all points in the configured Qdrant collection and deletes the +legacy payload keys `highlighted_page_image`, `highlighted_page_number`, +and `highlight_count`. This relieves disk pressure caused by inline +base64 PNGs that the new code path no longer writes. + +Idempotent: deleting non-existent keys is a no-op, so re-runs are safe. + +Usage: + uv run python scripts/purge_page_images.py [--dry-run] [--batch-size 256] + +Connection settings (Qdrant URL/API key, collection name) are read from +the same `Settings` object the server uses. +""" + +from __future__ import annotations + +import argparse +import asyncio +import logging +import sys + +from qdrant_client import AsyncQdrantClient + +from nextcloud_mcp_server.config import get_settings + +logger = logging.getLogger("purge_page_images") + +LEGACY_FIELDS = [ + "highlighted_page_image", + "highlighted_page_number", + "highlight_count", +] + + +def make_client() -> AsyncQdrantClient: + settings = get_settings() + if not settings.qdrant_url: + raise SystemExit( + "qdrant_url is not configured. Set QDRANT_URL (and QDRANT_API_KEY " + "if required) before running this script." + ) + return AsyncQdrantClient( + url=settings.qdrant_url, + api_key=settings.qdrant_api_key, + timeout=60, + ) + + +async def purge(dry_run: bool, batch_size: int) -> None: + settings = get_settings() + collection = settings.get_collection_name() + client = make_client() + + next_offset = None + total_seen = 0 + total_updated = 0 + + logger.info( + "Scanning collection %s; will delete keys %s%s", + collection, + LEGACY_FIELDS, + " (dry run)" if dry_run else "", + ) + + while True: + points, next_offset = await client.scroll( + collection_name=collection, + limit=batch_size, + offset=next_offset, + with_payload=False, + with_vectors=False, + ) + if not points: + break + + ids = [p.id for p in points] + total_seen += len(ids) + + if not dry_run: + await client.delete_payload( + collection_name=collection, + keys=LEGACY_FIELDS, + points=ids, + ) + total_updated += len(ids) + + logger.info( + "Batch: ids=%d total_seen=%d total_updated=%d", + len(ids), + total_seen, + total_updated, + ) + + if next_offset is None: + break + + logger.info( + "Done. total_seen=%d total_updated=%d%s", + total_seen, + total_updated, + " (dry run, no writes)" if dry_run else "", + ) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--dry-run", + action="store_true", + help="Scan only; do not write any changes.", + ) + parser.add_argument( + "--batch-size", + type=int, + default=256, + help="Points per scroll/update batch (default: 256).", + ) + parser.add_argument( + "-v", "--verbose", action="store_true", help="Enable debug logging." + ) + args = parser.parse_args() + + logging.basicConfig( + level=logging.DEBUG if args.verbose else logging.INFO, + format="%(asctime)s %(levelname)s %(name)s: %(message)s", + ) + + asyncio.run(purge(dry_run=args.dry_run, batch_size=args.batch_size)) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/unit/search/test_pdf_highlighter_bbox.py b/tests/unit/search/test_pdf_highlighter_bbox.py new file mode 100644 index 00000000..7bdba865 --- /dev/null +++ b/tests/unit/search/test_pdf_highlighter_bbox.py @@ -0,0 +1,132 @@ +"""Unit tests for PDFHighlighter.compute_chunk_bboxes_batch (Deck #76). + +Replaces the legacy `highlight_chunks_batch`-+-base64 pipeline that inflated +Qdrant payloads with per-chunk PNG screenshots. The new path returns +normalized bounding boxes only. +""" + +from __future__ import annotations + +import pymupdf +import pytest + +from nextcloud_mcp_server.search.pdf_highlighter import PDFHighlighter + + +def _make_pdf(pages: list[str]) -> bytes: + """Build an in-memory PDF whose pages contain the given text.""" + doc = pymupdf.open() + for body in pages: + page = doc.new_page(width=595, height=842) # A4 + page.insert_text((50, 50), body) + pdf_bytes = doc.tobytes() + doc.close() + return pdf_bytes + + +def _page_boundaries(pages: list[str]) -> tuple[list[dict], str]: + """Build (page_boundaries, full_text) compatible with the highlighter API.""" + boundaries: list[dict] = [] + cursor = 0 + parts: list[str] = [] + for i, body in enumerate(pages, start=1): + end = cursor + len(body) + boundaries.append({"page": i, "start_offset": cursor, "end_offset": end}) + parts.append(body) + cursor = end + return boundaries, "".join(parts) + + +def test_compute_chunk_bboxes_returns_normalized_rects(): + """Each returned bbox should be 4 floats in [0, 1] tagged with the page.""" + pages = [ + "Chapter 1: Introduction. Nextcloud is a self-hosted collaboration platform " + "covering installation, configuration and maintenance topics.", + "Chapter 2: Installation. Download the package, extract it to the web " + "server directory, and configure the database connection.", + ] + pdf_bytes = _make_pdf(pages) + boundaries, full_text = _page_boundaries(pages) + + chunks = [ + ( + 0, + 0, + len(pages[0]), + 1, + "Chapter 1: Introduction. Nextcloud is a self-hosted collaboration platform.", + ), + ( + 1, + len(pages[0]), + len(pages[0]) + len(pages[1]), + 2, + "Chapter 2: Installation. Download the package.", + ), + ] + + results = PDFHighlighter.compute_chunk_bboxes_batch( + pdf_bytes=pdf_bytes, + chunks=chunks, + page_boundaries=boundaries, + full_text=full_text, + ) + + assert set(results) == {0, 1} + + bboxes_p1, page_p1 = results[0] + bboxes_p2, page_p2 = results[1] + + assert page_p1 == 1 + assert page_p2 == 2 + + for rects in (bboxes_p1, bboxes_p2): + assert len(rects) >= 1 + for rect in rects: + assert len(rect) == 4 + x0, y0, x1, y1 = rect + assert 0.0 <= x0 < x1 <= 1.0 + assert 0.0 <= y0 < y1 <= 1.0 + + +def test_compute_chunk_bboxes_empty_input(): + assert ( + PDFHighlighter.compute_chunk_bboxes_batch( + pdf_bytes=b"", + chunks=[], + page_boundaries=[], + full_text="", + ) + == {} + ) + + +@pytest.mark.parametrize("page_index", [0, 1]) +def test_compute_chunk_bboxes_assigns_correct_page(page_index: int): + """Verify the page number returned matches the page the chunk lives on.""" + pages = [ + "Page one talks about apples and oranges in detail.", + "Page two discusses bananas and grapes thoroughly.", + ] + pdf_bytes = _make_pdf(pages) + boundaries, full_text = _page_boundaries(pages) + + if page_index == 0: + chunk_text = "apples and oranges" + offsets = (0, len(pages[0])) + else: + chunk_text = "bananas and grapes" + offsets = (len(pages[0]), len(pages[0]) + len(pages[1])) + + chunks = [(0, offsets[0], offsets[1], page_index + 1, chunk_text)] + + results = PDFHighlighter.compute_chunk_bboxes_batch( + pdf_bytes=pdf_bytes, + chunks=chunks, + page_boundaries=boundaries, + full_text=full_text, + ) + + assert results, "expected a bbox for the chunk" + _, page_num = results[0] + assert page_num == page_index + 1 From 80b27b1bc68401d917b55f08860e567844d249d3 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 21:32:12 +0200 Subject: [PATCH 2/4] =?UTF-8?q?refactor(vector):=20address=20PR=20#775=20r?= =?UTF-8?q?eview=20=E2=80=94=20drop=20unused=20payload=20key,=20fix=20reso?= =?UTF-8?q?urce=20leaks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop chunk_bbox_page from Qdrant payload — viz endpoints never read it (page_number is the canonical PDF page field). - Bump upsert BATCH_SIZE 10 → 100 now that payloads no longer carry PNGs. - compute_chunk_bboxes_batch: move doc.close() into finally, replace unused stored_page_num with _. - purge_page_images.py: switch to anyio.run() per project convention, and wrap AsyncQdrantClient in try/finally so the aiohttp session is always closed (the class doesn't implement async-context-manager). - Decorate new bbox unit tests with @pytest.mark.unit so they run under the fast-feedback selector. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../search/pdf_highlighter.py | 6 +- nextcloud_mcp_server/vector/processor.py | 15 ++- scripts/purge_page_images.py | 111 +++++++++--------- .../unit/search/test_pdf_highlighter_bbox.py | 3 + 4 files changed, 70 insertions(+), 65 deletions(-) diff --git a/nextcloud_mcp_server/search/pdf_highlighter.py b/nextcloud_mcp_server/search/pdf_highlighter.py index 6dbaea9a..fac8b09d 100644 --- a/nextcloud_mcp_server/search/pdf_highlighter.py +++ b/nextcloud_mcp_server/search/pdf_highlighter.py @@ -726,6 +726,7 @@ class PDFHighlighter: return results temp_pdf_path = None + doc = None try: temp_dir = Path(tempfile.mkdtemp(prefix="pdf_bbox_batch_")) temp_pdf_path = temp_dir / "pdf.pdf" @@ -737,7 +738,7 @@ class PDFHighlighter: chunk_index, start_offset, end_offset, - stored_page_num, + _, chunk_text, ) in chunks: chunk_page_info = PDFHighlighter.find_chunk_page( @@ -781,7 +782,6 @@ class PDFHighlighter: ) results[chunk_index] = ([normalized], page_num) - doc.close() logger.info(f"Computed bboxes for {len(results)}/{len(chunks)} chunks") return results @@ -790,6 +790,8 @@ class PDFHighlighter: return results finally: + if doc is not None: + doc.close() if temp_pdf_path and temp_pdf_path.parent.exists(): try: shutil.rmtree(temp_pdf_path.parent) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 3d6542ba..654c7a63 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -736,12 +736,10 @@ async def _index_document( ), # Chunk bbox (PDF only) — normalized rectangles in [0,1] # relative to page width/height. Replaces the legacy - # `highlighted_page_image` (Deck #76). + # `highlighted_page_image` (Deck #76). The page number + # comes from `page_number` (set above for PDF chunks). **( - { - "chunk_bbox": chunk_bboxes[i]["bbox"], - "chunk_bbox_page": chunk_bboxes[i]["page"], - } + {"chunk_bbox": chunk_bboxes[i]["bbox"]} if i in chunk_bboxes else {} ), @@ -763,9 +761,10 @@ async def _index_document( f"Failed to delete placeholder for {doc_task.doc_type}_{doc_task.doc_id}: {e}" ) - # Upsert to Qdrant in batches to avoid timeout with large payloads - # Batch size kept small for safety; payloads are now small (no inline images). - BATCH_SIZE = 10 + # Upsert to Qdrant in batches. Now that we no longer embed PNG payloads, + # per-point payloads are small (chunk text + small metadata), so we can + # safely use a larger batch size. + BATCH_SIZE = 100 with trace_operation( "vector_sync.qdrant_upsert", attributes={ diff --git a/scripts/purge_page_images.py b/scripts/purge_page_images.py index 2f4ce77b..350a792f 100755 --- a/scripts/purge_page_images.py +++ b/scripts/purge_page_images.py @@ -18,10 +18,11 @@ the same `Settings` object the server uses. from __future__ import annotations import argparse -import asyncio import logging import sys +from functools import partial +import anyio from qdrant_client import AsyncQdrantClient from nextcloud_mcp_server.config import get_settings @@ -35,74 +36,74 @@ LEGACY_FIELDS = [ ] -def make_client() -> AsyncQdrantClient: +async def purge(dry_run: bool, batch_size: int) -> None: settings = get_settings() if not settings.qdrant_url: raise SystemExit( "qdrant_url is not configured. Set QDRANT_URL (and QDRANT_API_KEY " "if required) before running this script." ) - return AsyncQdrantClient( + collection = settings.get_collection_name() + + # AsyncQdrantClient doesn't implement __aenter__/__aexit__, so use + # try/finally to guarantee the underlying aiohttp session is closed. + client = AsyncQdrantClient( url=settings.qdrant_url, api_key=settings.qdrant_api_key, timeout=60, ) - - -async def purge(dry_run: bool, batch_size: int) -> None: - settings = get_settings() - collection = settings.get_collection_name() - client = make_client() - - next_offset = None - total_seen = 0 - total_updated = 0 - - logger.info( - "Scanning collection %s; will delete keys %s%s", - collection, - LEGACY_FIELDS, - " (dry run)" if dry_run else "", - ) - - while True: - points, next_offset = await client.scroll( - collection_name=collection, - limit=batch_size, - offset=next_offset, - with_payload=False, - with_vectors=False, - ) - if not points: - break - - ids = [p.id for p in points] - total_seen += len(ids) - - if not dry_run: - await client.delete_payload( - collection_name=collection, - keys=LEGACY_FIELDS, - points=ids, - ) - total_updated += len(ids) + try: + next_offset = None + total_seen = 0 + total_updated = 0 logger.info( - "Batch: ids=%d total_seen=%d total_updated=%d", - len(ids), - total_seen, - total_updated, + "Scanning collection %s; will delete keys %s%s", + collection, + LEGACY_FIELDS, + " (dry run)" if dry_run else "", ) - if next_offset is None: - break + while True: + points, next_offset = await client.scroll( + collection_name=collection, + limit=batch_size, + offset=next_offset, + with_payload=False, + with_vectors=False, + ) + if not points: + break - logger.info( - "Done. total_seen=%d total_updated=%d%s", - total_seen, - total_updated, - " (dry run, no writes)" if dry_run else "", - ) + ids = [p.id for p in points] + total_seen += len(ids) + + if not dry_run: + await client.delete_payload( + collection_name=collection, + keys=LEGACY_FIELDS, + points=ids, + ) + total_updated += len(ids) + + logger.info( + "Batch: ids=%d total_seen=%d total_updated=%d", + len(ids), + total_seen, + total_updated, + ) + + if next_offset is None: + break + + logger.info( + "Done. total_seen=%d total_updated=%d%s", + total_seen, + total_updated, + " (dry run, no writes)" if dry_run else "", + ) + finally: + await client.close() def main() -> int: @@ -128,7 +129,7 @@ def main() -> int: format="%(asctime)s %(levelname)s %(name)s: %(message)s", ) - asyncio.run(purge(dry_run=args.dry_run, batch_size=args.batch_size)) + anyio.run(partial(purge, dry_run=args.dry_run, batch_size=args.batch_size)) return 0 diff --git a/tests/unit/search/test_pdf_highlighter_bbox.py b/tests/unit/search/test_pdf_highlighter_bbox.py index 7bdba865..dcf80b55 100644 --- a/tests/unit/search/test_pdf_highlighter_bbox.py +++ b/tests/unit/search/test_pdf_highlighter_bbox.py @@ -37,6 +37,7 @@ def _page_boundaries(pages: list[str]) -> tuple[list[dict], str]: return boundaries, "".join(parts) +@pytest.mark.unit def test_compute_chunk_bboxes_returns_normalized_rects(): """Each returned bbox should be 4 floats in [0, 1] tagged with the page.""" pages = [ @@ -89,6 +90,7 @@ def test_compute_chunk_bboxes_returns_normalized_rects(): assert 0.0 <= y0 < y1 <= 1.0 +@pytest.mark.unit def test_compute_chunk_bboxes_empty_input(): assert ( PDFHighlighter.compute_chunk_bboxes_batch( @@ -101,6 +103,7 @@ def test_compute_chunk_bboxes_empty_input(): ) +@pytest.mark.unit @pytest.mark.parametrize("page_index", [0, 1]) def test_compute_chunk_bboxes_assigns_correct_page(page_index: int): """Verify the page number returned matches the page the chunk lives on.""" From 8bc87ed37d3d1daab2a3b771a253fd9b545f6e0b Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 22:16:50 +0200 Subject: [PATCH 3/4] =?UTF-8?q?refactor(vector):=20address=20PR=20#775=20r?= =?UTF-8?q?eview=20round=202=20=E2=80=94=20drop=20dead=20page=20field,=20a?= =?UTF-8?q?dd=20omission=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - chunk_bboxes is now dict[int, list[tuple[...]]] holding the bbox list directly, not {"bbox": ..., "page": ...}. The page from text-search was stored but never read; page_number from offset-based assignment is authoritative for the Qdrant payload. - Add two unit tests for the documented omission contract: chunks whose offsets fall outside every page boundary, and chunks whose text cannot be located on the rendered page, are silently dropped from the result. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/vector/processor.py | 21 +++----- .../unit/search/test_pdf_highlighter_bbox.py | 52 +++++++++++++++++++ 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 654c7a63..7f943508 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -537,9 +537,11 @@ async def _index_document( # Initialize results containers dense_embeddings: list = [] sparse_embeddings: list = [] - # chunk_index -> {"bbox": list[(x0,y0,x1,y1)], "page": int} - # Bboxes are normalized to [0, 1] relative to page width/height. - chunk_bboxes: dict[int, dict] = {} + # chunk_index -> list[(x0, y0, x1, y1)] of normalized rectangles + # in [0, 1] relative to page width/height. The page is taken from + # `chunk.page_number` (offset-based) and stored as `page_number` + # in the Qdrant payload, so we don't carry an `actual_page_num` here. + chunk_bboxes: dict[int, list[tuple[float, float, float, float]]] = {} # Determine if we need PDF highlighting is_pdf = doc_task.doc_type == "file" and content_type == "application/pdf" @@ -612,11 +614,8 @@ async def _index_document( ) ) - for chunk_index, (bboxes, actual_page_num) in batch_results.items(): - chunk_bboxes[chunk_index] = { - "bbox": bboxes, - "page": actual_page_num, - } + for chunk_index, (bboxes, _) in batch_results.items(): + chunk_bboxes[chunk_index] = bboxes logger.info(f"Computed bboxes for {len(chunk_bboxes)}/{len(chunks)} chunks") @@ -738,11 +737,7 @@ async def _index_document( # relative to page width/height. Replaces the legacy # `highlighted_page_image` (Deck #76). The page number # comes from `page_number` (set above for PDF chunks). - **( - {"chunk_bbox": chunk_bboxes[i]["bbox"]} - if i in chunk_bboxes - else {} - ), + **({"chunk_bbox": chunk_bboxes[i]} if i in chunk_bboxes else {}), }, ) ) diff --git a/tests/unit/search/test_pdf_highlighter_bbox.py b/tests/unit/search/test_pdf_highlighter_bbox.py index dcf80b55..0c1c51bd 100644 --- a/tests/unit/search/test_pdf_highlighter_bbox.py +++ b/tests/unit/search/test_pdf_highlighter_bbox.py @@ -103,6 +103,58 @@ def test_compute_chunk_bboxes_empty_input(): ) +@pytest.mark.unit +def test_compute_chunk_bboxes_omits_when_offsets_out_of_range(): + """Chunks whose offsets fall outside every page boundary are omitted. + + Verifies the docstring contract: *"Chunks whose bbox cannot be located + are omitted from the result."* (path: ``find_chunk_page`` returns None). + """ + pages = ["Page one body text content here for the test."] + pdf_bytes = _make_pdf(pages) + boundaries, full_text = _page_boundaries(pages) + + # Offsets way beyond the document end — no page boundary matches. + out_of_range_start = len(full_text) + 1000 + out_of_range_end = out_of_range_start + 50 + chunks = [(0, out_of_range_start, out_of_range_end, 1, "irrelevant")] + + results = PDFHighlighter.compute_chunk_bboxes_batch( + pdf_bytes=pdf_bytes, + chunks=chunks, + page_boundaries=boundaries, + full_text=full_text, + ) + + assert results == {} + + +@pytest.mark.unit +def test_compute_chunk_bboxes_omits_when_text_not_in_pdf(): + """Chunks whose page-relative text isn't on the page are omitted. + + Verifies the second omission path: ``_find_chunk_bbox`` returns None + when the supplied text cannot be located on the rendered page. + """ + pages = ["Hello world."] + pdf_bytes = _make_pdf(pages) + # Build boundaries from the real text but pass a *different* full_text + # so the page-relative slice is content that does not exist in the PDF. + boundaries, _ = _page_boundaries(pages) + bogus_full_text = "Z" * len(pages[0]) + + chunks = [(0, 0, len(pages[0]), 1, "ignored")] + + results = PDFHighlighter.compute_chunk_bboxes_batch( + pdf_bytes=pdf_bytes, + chunks=chunks, + page_boundaries=boundaries, + full_text=bogus_full_text, + ) + + assert results == {} + + @pytest.mark.unit @pytest.mark.parametrize("page_index", [0, 1]) def test_compute_chunk_bboxes_assigns_correct_page(page_index: int): From 0b004f54bd3ed4c2d3fe6aa3eb7d778cf8dbb5bd Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 23:05:31 +0200 Subject: [PATCH 4/4] =?UTF-8?q?refactor(vector):=20address=20PR=20#775=20r?= =?UTF-8?q?eview=20round=203=20=E2=80=94=20fix=20unused=20var,=20harden=20?= =?UTF-8?q?boundary=20lookup,=20rename=20trace=20span?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pdf_highlighter.compute_chunk_bboxes_batch: drop unused chunk_text destructure (SonarQube finding), and replace positional page_boundaries[page_num - 1] with a key-based next() match so reordered or non-1-indexed boundaries can't silently shift the bbox. Convert touched f-string log to lazy %s formatting. - vector/processor: rename the trace_operation span from "vector_sync.generate_highlights" to "vector_sync.compute_chunk_bboxes" to match what the function actually does. - Add test_compute_chunk_bboxes_handles_unordered_page_boundaries — reverses the boundaries list and asserts identical results to the in-order case, guarding the boundary-lookup regression class. - Pin pre-push-review skill to sonnet model. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/skills/pre-push-review/SKILL.md | 1 + .../search/pdf_highlighter.py | 15 +++++-- nextcloud_mcp_server/vector/processor.py | 2 +- .../unit/search/test_pdf_highlighter_bbox.py | 45 +++++++++++++++++++ 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/.claude/skills/pre-push-review/SKILL.md b/.claude/skills/pre-push-review/SKILL.md index 26883d49..efc906b0 100644 --- a/.claude/skills/pre-push-review/SKILL.md +++ b/.claude/skills/pre-push-review/SKILL.md @@ -7,6 +7,7 @@ description: | in this repo's automated PR reviews. Use when the user is about to push, says "ready to push", "review my work", "check before PR", or invokes /pre-push-review. Report-only — does not modify code. +model: sonnet allowed-tools: - Bash - Read diff --git a/nextcloud_mcp_server/search/pdf_highlighter.py b/nextcloud_mcp_server/search/pdf_highlighter.py index fac8b09d..18ebafcd 100644 --- a/nextcloud_mcp_server/search/pdf_highlighter.py +++ b/nextcloud_mcp_server/search/pdf_highlighter.py @@ -739,17 +739,26 @@ class PDFHighlighter: start_offset, end_offset, _, - chunk_text, + _, ) in chunks: chunk_page_info = PDFHighlighter.find_chunk_page( start_offset, end_offset, page_boundaries ) if not chunk_page_info: - logger.debug(f"Chunk {chunk_index}: not found on any page") + logger.debug("Chunk %s: not found on any page", chunk_index) continue page_num = chunk_page_info["page_num"] - page_boundary = page_boundaries[page_num - 1] + page_boundary = next( + (b for b in page_boundaries if b["page"] == page_num), None + ) + if page_boundary is None: + logger.debug( + "Chunk %s: page %s not found in boundaries", + chunk_index, + page_num, + ) + continue page_text_length = ( page_boundary["end_offset"] - page_boundary["start_offset"] ) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 7f943508..f3acc8ff 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -582,7 +582,7 @@ async def _index_document( assert content_bytes is not None with trace_operation( - "vector_sync.generate_highlights", + "vector_sync.compute_chunk_bboxes", attributes={ "vector_sync.chunk_count": len(chunks), "vector_sync.pdf_size": len(content_bytes), diff --git a/tests/unit/search/test_pdf_highlighter_bbox.py b/tests/unit/search/test_pdf_highlighter_bbox.py index 0c1c51bd..60985916 100644 --- a/tests/unit/search/test_pdf_highlighter_bbox.py +++ b/tests/unit/search/test_pdf_highlighter_bbox.py @@ -185,3 +185,48 @@ def test_compute_chunk_bboxes_assigns_correct_page(page_index: int): assert results, "expected a bbox for the chunk" _, page_num = results[0] assert page_num == page_index + 1 + + +@pytest.mark.unit +def test_compute_chunk_bboxes_handles_unordered_page_boundaries(): + """Page lookup must match by ``page`` key, not by list position. + + Regression guard: an earlier implementation indexed + ``page_boundaries[page_num - 1]``, which silently produces a wrong + bbox if boundaries are passed out of order. Reverse the boundaries + and assert the result is identical to the in-order case. + """ + pages = [ + "Page one talks about apples and oranges in detail.", + "Page two discusses bananas and grapes thoroughly.", + ] + pdf_bytes = _make_pdf(pages) + boundaries, full_text = _page_boundaries(pages) + + chunks = [ + (0, 0, len(pages[0]), 1, "apples and oranges"), + ( + 1, + len(pages[0]), + len(pages[0]) + len(pages[1]), + 2, + "bananas and grapes", + ), + ] + + in_order = PDFHighlighter.compute_chunk_bboxes_batch( + pdf_bytes=pdf_bytes, + chunks=chunks, + page_boundaries=boundaries, + full_text=full_text, + ) + reversed_order = PDFHighlighter.compute_chunk_bboxes_batch( + pdf_bytes=pdf_bytes, + chunks=chunks, + page_boundaries=list(reversed(boundaries)), + full_text=full_text, + ) + + assert in_order == reversed_order + assert reversed_order[0][1] == 1 + assert reversed_order[1][1] == 2