diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index ef4f2fbe..5b5bdaa7 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -208,7 +208,10 @@ class ChunkContext: chunk_start_offset: Character position where chunk starts in document chunk_end_offset: Character position where chunk ends in document page_number: Page number for PDFs (None for other doc types) - chunk_index: Zero-based chunk index (N in "chunk N of M") + chunk_index: Zero-based chunk index (N in "chunk N of M"). None when + the caller didn't supply chunk_index and we couldn't determine it + from the lookup path — distinguishes "unknown position" from + "actually chunk 0". total_chunks: Total number of chunks in document marked_text: Full text with position markers around the chunk has_before_truncation: True if before_context was truncated @@ -221,7 +224,7 @@ class ChunkContext: chunk_start_offset: int chunk_end_offset: int page_number: int | None - chunk_index: int + chunk_index: int | None total_chunks: int marked_text: str has_before_truncation: bool @@ -271,16 +274,6 @@ async def get_chunk_with_context( else (doc_id if isinstance(doc_id, int) else None) ) - # Effective chunk_index for adjacent lookups, marker insertion, and the - # response payload — keep `chunk_index is not None` distinct from this so - # the gate at line ~280 still controls *whether* to take the indexed path. - # NOTE: when the caller doesn't supply chunk_index, this defaults to 0, so - # the doc-text fallback path will report the chunk as "0/N" in markers and - # the response payload regardless of its actual position. Callers that - # need accurate position metadata in the fallback path must pass - # chunk_index. See PR #767 review. - effective_chunk_index = chunk_index if chunk_index is not None else 0 - # Try to get chunk from Qdrant (fast path). # Prefer chunk_index lookup (always-indexed field) when caller supplied it; # fall back to (chunk_start, chunk_end) lookup otherwise. @@ -319,54 +312,63 @@ async def get_chunk_with_context( has_before_truncation = False has_after_truncation = False - # Fetch previous chunk if not first chunk - if effective_chunk_index > 0: - before_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, effective_chunk_index - 1 - ) - if before_chunk: - # Remove overlap: the last chunk_overlap chars of previous chunk - # overlap with the first chunk_overlap chars of current chunk - before_context = ( - before_chunk[:-chunk_overlap] - if len(before_chunk) > chunk_overlap - else "" + if chunk_index is not None: + # Fetch previous chunk if not first chunk + if chunk_index > 0: + before_chunk = await _get_chunk_by_index_from_qdrant( + user_id, doc_id_int, doc_type, chunk_index - 1 ) - # Truncate if requested context_chars < remaining length - if before_context and len(before_context) > context_chars: - before_context = before_context[-context_chars:] + if before_chunk: + # Remove overlap: the last chunk_overlap chars of previous chunk + # overlap with the first chunk_overlap chars of current chunk + before_context = ( + before_chunk[:-chunk_overlap] + if len(before_chunk) > chunk_overlap + else "" + ) + # Truncate if requested context_chars < remaining length + if before_context and len(before_context) > context_chars: + before_context = before_context[-context_chars:] + has_before_truncation = True + else: + # Could not fetch previous chunk, but we're not at start has_before_truncation = True - else: - # Could not fetch previous chunk, but we're not at start - has_before_truncation = True - # Fetch next chunk if not last chunk - if effective_chunk_index < total_chunks - 1: - after_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, effective_chunk_index + 1 - ) - if after_chunk: - # Remove overlap: the first chunk_overlap chars of next chunk - # overlap with the last chunk_overlap chars of current chunk - after_context = ( - after_chunk[chunk_overlap:] - if len(after_chunk) > chunk_overlap - else "" + # Fetch next chunk if not last chunk + if chunk_index < total_chunks - 1: + after_chunk = await _get_chunk_by_index_from_qdrant( + user_id, doc_id_int, doc_type, chunk_index + 1 ) - # Truncate if requested context_chars < remaining length - if after_context and len(after_context) > context_chars: - after_context = after_context[:context_chars] + if after_chunk: + # Remove overlap: the first chunk_overlap chars of next chunk + # overlap with the last chunk_overlap chars of current chunk + after_context = ( + after_chunk[chunk_overlap:] + if len(after_chunk) > chunk_overlap + else "" + ) + # Truncate if requested context_chars < remaining length + if after_context and len(after_context) > context_chars: + after_context = after_context[:context_chars] + has_after_truncation = True + else: + # Could not fetch next chunk, but we're not at end has_after_truncation = True - else: - # Could not fetch next chunk, but we're not at end - has_after_truncation = True + else: + # No chunk_index → can't fetch adjacent chunks via index arithmetic + # without risking wrong neighbours (a default of 0 would query the + # chunks at positions -1 and 1 even when the actual chunk is, say, + # 5/20). Mark both sides as truncated so the caller knows context + # wasn't expanded. + has_before_truncation = True + has_after_truncation = True marked_text = _insert_position_markers( before_context=before_context, chunk_text=chunk_text, after_context=after_context, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, has_before_truncation=has_before_truncation, has_after_truncation=has_after_truncation, @@ -378,7 +380,7 @@ async def get_chunk_with_context( chunk_start_offset=chunk_start, chunk_end_offset=chunk_end, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, marked_text=marked_text, has_before_truncation=has_before_truncation, @@ -403,16 +405,6 @@ async def get_chunk_with_context( f"(Qdrant cache miss, possibly legacy data)" ) - # When chunk_index isn't supplied, the response and markers will report - # this chunk as 0/N regardless of its actual position (effective_chunk_index - # defaulted to 0 above). Surface this so callers can detect the inaccuracy. - if chunk_index is None: - logger.warning( - f"chunk_index not supplied for {doc_type} {doc_id} doc-text " - f"fallback; position metadata in response will default to " - f"0/{total_chunks}" - ) - # Fetch full document text (notes, deck cards, news items, etc.) full_text = await _fetch_document_text(nc_client, doc_id, doc_type, user_id) if full_text is None: @@ -451,7 +443,7 @@ async def get_chunk_with_context( chunk_text=chunk_text, after_context=after_context, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, has_before_truncation=has_before_truncation, has_after_truncation=has_after_truncation, @@ -464,7 +456,7 @@ async def get_chunk_with_context( chunk_start_offset=chunk_start, chunk_end_offset=chunk_end, page_number=page_number, - chunk_index=effective_chunk_index, + chunk_index=chunk_index, total_chunks=total_chunks, marked_text=marked_text, has_before_truncation=has_before_truncation, @@ -644,7 +636,7 @@ def _insert_position_markers( chunk_text: str, after_context: str, page_number: int | None, - chunk_index: int, + chunk_index: int | None, total_chunks: int, has_before_truncation: bool, has_after_truncation: bool, @@ -659,7 +651,8 @@ def _insert_position_markers( chunk_text: The matched chunk after_context: Text after chunk page_number: Optional page number - chunk_index: Zero-based chunk index + chunk_index: Zero-based chunk index, or None when the caller didn't + supply it (rendered as "Chunk ?/N" instead of "Chunk 0/N"). total_chunks: Total chunks in document has_before_truncation: Whether before_context is truncated has_after_truncation: Whether after_context is truncated @@ -671,7 +664,10 @@ def _insert_position_markers( position_parts = [] if page_number is not None: position_parts.append(f"Page {page_number}") - position_parts.append(f"Chunk {chunk_index + 1} of {total_chunks}") + if chunk_index is None: + position_parts.append(f"Chunk ?/{total_chunks}") + else: + position_parts.append(f"Chunk {chunk_index + 1} of {total_chunks}") position_metadata = ", ".join(position_parts) # Build marked text diff --git a/tests/unit/test_chunk_context_offset_gate.py b/tests/unit/test_chunk_context_offset_gate.py index 3e735289..3f39d303 100644 --- a/tests/unit/test_chunk_context_offset_gate.py +++ b/tests/unit/test_chunk_context_offset_gate.py @@ -135,3 +135,168 @@ class TestOffsetFallbackGate: mock_indexed.assert_not_awaited() mock_offset.assert_awaited_once() + + +class TestNullableChunkIndexPropagation: + """When the caller doesn't supply chunk_index, it must propagate as None + through to ChunkContext and the position markers — distinguishing + "unknown position" from "actually chunk 0". See PR #767 review (🟡 issue 2). + """ + + async def test_fast_path_without_chunk_index_returns_none_in_response( + self, mock_nc_client + ): + """Note retrieved via offset fallback (chunk_index=None) → response + chunk_index is None, markers render '?/N', and adjacent fetch is + skipped (would otherwise produce wrong neighbours from index 0). + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ) as mock_indexed, + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value="matched chunk text", + ), + ): + result = await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=42, + doc_type="note", + chunk_start=100, + chunk_end=200, + chunk_index=None, + total_chunks=8, + ) + + assert result is not None + assert result.chunk_index is None, ( + "chunk_index must propagate as None, not default to 0" + ) + assert "Chunk ?/8" in result.marked_text + assert "Chunk 1 of 8" not in result.marked_text + # Adjacent fetch must be skipped — index arithmetic from 0 would + # query the wrong neighbours when actual position isn't 0. + mock_indexed.assert_not_awaited() + assert result.has_before_truncation is True + assert result.has_after_truncation is True + + async def test_fast_path_with_chunk_index_renders_position_correctly( + self, mock_nc_client + ): + """Counter-positive: when chunk_index is supplied, response carries + the value and markers render the explicit "Chunk N of M". + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + side_effect=[ + "current chunk text", # primary lookup + "previous chunk text", # adjacent before + "next chunk text", # adjacent after + ], + ), + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + ): + result = await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=42, + doc_type="note", + chunk_start=0, + chunk_end=10, + chunk_index=5, + total_chunks=20, + ) + + assert result is not None + assert result.chunk_index == 5 + assert "Chunk 6 of 20" in result.marked_text + + async def test_doc_text_fallback_without_chunk_index_returns_none( + self, mock_nc_client + ): + """Doc-text fallback (Qdrant miss → re-fetch document) must also + propagate chunk_index=None into the response so callers can tell + the position is unknown. + """ + with ( + patch.object( + context_module, + "_get_chunk_by_index_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + patch.object( + context_module, + "_get_chunk_from_qdrant", + new_callable=AsyncMock, + return_value=None, + ), + patch.object( + context_module, + "_fetch_document_text", + new_callable=AsyncMock, + return_value="x" * 500, + ), + ): + result = await get_chunk_with_context( + nc_client=mock_nc_client, + user_id="alice", + doc_id=42, + doc_type="note", + chunk_start=100, + chunk_end=200, + chunk_index=None, + total_chunks=10, + ) + + assert result is not None + assert result.chunk_index is None + assert "Chunk ?/10" in result.marked_text + + +class TestPositionMarkers: + """Direct tests for `_insert_position_markers` rendering when chunk_index + is None vs explicit. + """ + + def test_marker_renders_question_mark_when_chunk_index_is_none(self): + text = context_module._insert_position_markers( + before_context="", + chunk_text="x", + after_context="", + page_number=None, + chunk_index=None, + total_chunks=12, + has_before_truncation=False, + has_after_truncation=False, + ) + assert "Chunk ?/12" in text + + def test_marker_renders_explicit_index_when_supplied(self): + text = context_module._insert_position_markers( + before_context="", + chunk_text="x", + after_context="", + page_number=3, + chunk_index=4, + total_chunks=12, + has_before_truncation=False, + has_after_truncation=False, + ) + assert "Page 3" in text + assert "Chunk 5 of 12" in text