From 0b004f54bd3ed4c2d3fe6aa3eb7d778cf8dbb5bd Mon Sep 17 00:00:00 2001
From: Chris Coutinho <chris@coutinho.io>
Date: Fri, 8 May 2026 23:05:31 +0200
Subject: [PATCH] =?UTF-8?q?refactor(vector):=20address=20PR=20#775=20revie?=
 =?UTF-8?q?w=20round=203=20=E2=80=94=20fix=20unused=20var,=20harden=20boun?=
 =?UTF-8?q?dary=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) <noreply@anthropic.com>
---
 .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