fix(vector): address PR review round 11 — broaden offset-skip gate, clarify ordering

- search/context.py: drop the doc_type=='file' guard on skip_offset_lookup
  so notes / deck cards / news items also bypass the unindexed offset
  fallback when chunk_index is available. Legacy chunk_index=None data
  still uses the offset path.
- vector/qdrant_client.py: clarify the backfill/_ensure_payload_indexes
  ordering invariant (backfill rewrites payload values only, never schema
  or indexes). Acknowledge OSS-vs-Cloud uncertainty in the 400-branch
  comment and the new-collection call-site comment.
- vector/scanner.py: hoist qdrant_client to function scope so the
  file-scroll block doesn't depend on a name bound inside the
  notes-scroll block.
- tests/unit/test_chunk_context_offset_gate.py: flip the note-with-
  chunk_index test to assert the offset fallback is skipped.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/search/context.py

@@ -373,12 +373,15 @@ async def get_chunk_with_context(
         chunk_text = await _get_chunk_by_index_from_qdrant(
             user_id, doc_id, doc_type, chunk_index
         )
-    # Skip the offset fallback for files when the indexed chunk_index
-    # lookup already ran: chunk_start/end_offset aren't indexed in Qdrant
-    # Cloud strict mode, so the call returns 400 and surfaces a misleading
-    # logger.error. The file fast-fail below correctly handles the miss
-    # without it.
-    skip_offset_lookup = chunk_index is not None and doc_type == "file"
+    # When chunk_index is available, treat the indexed lookup as canonical
+    # for every doc_type. A miss means the chunk is genuinely absent, not
+    # "fall back to the unindexed slow path". chunk_start/end_offset aren't
+    # in _PAYLOAD_INDEX_FIELDS, so the offset filter 400s in Qdrant Cloud
+    # strict mode and surfaces a misleading logger.error. Legacy data
+    # without chunk_index (pre-cbcoutinho/astrolabe#75) still hits the
+    # offset path and degrades to a None chunk with a WARNING; that's the
+    # same behavior get_chunk_bbox_and_page_from_qdrant already documents.
+    skip_offset_lookup = chunk_index is not None
     if chunk_text is None and not skip_offset_lookup:
         chunk_text = await _get_chunk_from_qdrant(
             user_id, doc_id, doc_type, chunk_start, chunk_end

nextcloud_mcp_server/vector/qdrant_client.py

@@ -132,9 +132,14 @@ async def _ensure_payload_indexes(
             body = getattr(e, "content", b"") or b""
             body_text = body.decode("utf-8", errors="replace")
             # 400 is the expected schema-conflict path (index already exists
-            # with a different type). 5xx is unexpected — keep the loop going
-            # so the remaining fields still get attempted, but log at error
-            # so operators see it.
+            # with a different type). Verified for Qdrant OSS, where an
+            # idempotent re-create against a matching schema returns 200; if
+            # Qdrant Cloud diverges and returns 400 for benign re-creates,
+            # the WARNING below will fire on every restart against an
+            # already-indexed collection — read the response body before
+            # treating that as a real schema conflict. 5xx is unexpected —
+            # keep the loop going so the remaining fields still get
+            # attempted, but log at error so operators see it.
             if e.status_code == 400:
                 logger.warning(
                     "Schema conflict on payload index '%s': %s", field, body_text
@@ -534,7 +539,10 @@ async def get_qdrant_client() -> AsyncQdrantClient:
                 # payload-index work. Backfill before creating the index so the
                 # index covers every point. Pass the already-fetched
                 # collection_info.payload_schema through to avoid a redundant
-                # get_collection round-trip on every restart.
+                # get_collection round-trip on every restart — safe because
+                # _backfill_doc_id_to_string only rewrites payload *values*,
+                # never schema or indexes, so the snapshot remains accurate
+                # across the backfill call.
                 await _backfill_doc_id_to_string(
                     _qdrant_client, collection_name, expected_dimension
                 )
@@ -575,8 +583,16 @@ async def get_qdrant_client() -> AsyncQdrantClient:
                     f"  Distance: COSINE\n"
                     f"Background sync will index all documents with dense + sparse vectors."
                 )
-                # Freshly created collection has no payload schema yet; pass {}
-                # explicitly to skip the otherwise-redundant get_collection call.
+                # Freshly created collection has no payload schema yet; pass
+                # {} explicitly to skip the otherwise-redundant
+                # get_collection call. Every field in _PAYLOAD_INDEX_FIELDS
+                # then goes through create_payload_index; on a brand-new
+                # collection none of them exist yet, so the WARNING in the
+                # 400-handler should *never* fire on this path. If it does
+                # on Qdrant Cloud first-start, that points at a
+                # deployment-level issue (race with a concurrent creator,
+                # implicit auto-indexes, etc.) worth investigating before
+                # suppressing.
                 await _ensure_payload_indexes(
                     _qdrant_client, collection_name, existing_schema={}
                 )

nextcloud_mcp_server/vector/scanner.py

@@ -210,10 +210,16 @@ async def scan_user_documents(
             )
 
         # For deletion tracking, get all doc_ids in Qdrant (for incremental sync)
-        # Note: We no longer bulk-query indexed_at, instead check per-document
+        # Note: We no longer bulk-query indexed_at, instead check per-document.
+        # Hoisted to function scope so the file-scroll block below doesn't
+        # depend on a name bound inside the notes-scroll block; future
+        # refactors that add an early return between the two blocks would
+        # otherwise hit an UnboundLocalError. get_qdrant_client is a
+        # singleton call, so the cost is identical.
+        qdrant_client = await get_qdrant_client() if not initial_sync else None
         indexed_doc_ids = set()
         if not initial_sync:
-            qdrant_client = await get_qdrant_client()
+            assert qdrant_client is not None  # narrow for the type checker
             scroll_result = await qdrant_client.scroll(
                 collection_name=get_settings().get_collection_name(),
                 scroll_filter=Filter(
@@ -387,6 +393,7 @@ async def scan_user_documents(
         # Get indexed file IDs from Qdrant (for deletion tracking)
         indexed_file_ids = set()
         if not initial_sync:
+            assert qdrant_client is not None  # narrow for the type checker
             file_scroll_result = await qdrant_client.scroll(
                 collection_name=settings.get_collection_name(),
                 scroll_filter=Filter(

tests/unit/test_chunk_context_offset_gate.py

@@ -28,8 +28,12 @@ def mock_nc_client() -> MagicMock:
 
 
 class TestOffsetFallbackGate:
-    """When chunk_index is provided AND doc_type=='file', the offset fallback
-    must be skipped — see PR #767 review (🟡 spurious Qdrant error log).
+    """When chunk_index is provided, the offset fallback must be skipped for
+    every doc_type. The original gate was file-only (PR #767 review, 🟡
+    spurious Qdrant error log); PR #773 round 11 broadened it to all
+    doc_types because chunk_start/end_offset aren't in
+    ``_PAYLOAD_INDEX_FIELDS`` and 400 in Qdrant Cloud strict mode for
+    notes / deck cards / news items the same way they do for files.
     """
 
     async def test_file_with_chunk_index_skips_offset_fallback_on_miss(
@@ -64,11 +68,15 @@ class TestOffsetFallbackGate:
         mock_indexed.assert_awaited_once()
         mock_offset.assert_not_awaited()
 
-    async def test_note_with_chunk_index_still_uses_offset_fallback(
+    async def test_note_with_chunk_index_skips_offset_fallback_on_miss(
         self, mock_nc_client
     ):
-        """Notes/deck cards keep the offset fallback (cheap, useful for legacy
-        data): the gate is file-specific.
+        """Notes (and other non-file doc_types) trust the indexed
+        chunk_index lookup as canonical too. A miss means absent — don't
+        hit the unindexed offset path that 400s in Qdrant Cloud strict
+        mode. Legacy data without chunk_index still uses the offset path
+        via the chunk_index=None branch (see
+        test_file_without_chunk_index_uses_offset_fallback).
         """
         with (
             patch.object(
@@ -81,7 +89,7 @@ class TestOffsetFallbackGate:
                 context_module,
                 "_get_chunk_from_qdrant",
                 new_callable=AsyncMock,
-                return_value=None,
+                return_value="should-not-be-returned",
             ) as mock_offset,
             patch.object(
                 context_module,
@@ -102,7 +110,7 @@ class TestOffsetFallbackGate:
             )
 
         mock_indexed.assert_awaited_once()
-        mock_offset.assert_awaited_once()
+        mock_offset.assert_not_awaited()
 
     async def test_file_without_chunk_index_uses_offset_fallback(self, mock_nc_client):
         """Files with no chunk_index supplied still use the offset path —