fix(chunk-context): propagate chunk_index=None through ChunkContext
Addresses the deferred half of PR #767 review issue 2: instead of just documenting the "0/N misreport" with a logger.warning, propagate the caller's None for chunk_index through the dataclass, position markers, and response builders so callers can distinguish "unknown position" from "actually chunk 0". Changes: - ChunkContext.chunk_index: int → int | None - _insert_position_markers: chunk_index parameter is int | None; when None, renders "Chunk ?/N" instead of "Chunk 1 of N" - get_chunk_with_context: drops the effective_chunk_index local entirely. Passes chunk_index (may be None) directly into both ChunkContext and _insert_position_markers, in both the Qdrant fast path and the doc-text fallback. - Fast path: when chunk_index is None and chunk_text was retrieved via the offset lookup (notes/cards), skip the adjacent-chunk fetch. Index arithmetic from a default 0 would query the chunks at positions -1 and 1 even when the actual chunk is, say, 5/20 — silently producing wrong "before"/"after" text. Mark both sides as truncated instead. - Drop the now-redundant logger.warning in the doc-text fallback (the response correctly communicates the unknown state via chunk_index=None). Both existing response builders (`api/visualization.py:657` and `auth/viz_routes.py:717`) already serialise `chunk_context.chunk_index` unconditionally; `None` becomes JSON `null`. No route changes needed. Adds 5 regression tests: - ChunkContext.chunk_index propagates as None in fast path - ChunkContext.chunk_index propagates as None in doc-text fallback - Fast path with chunk_index renders "Chunk N of M" correctly - _insert_position_markers renders "?/N" for None chunk_index - _insert_position_markers renders explicit index when supplied Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
51c1d42ea3
commit
45da876cf5
@@ -208,7 +208,10 @@ class ChunkContext:
|
|||||||
chunk_start_offset: Character position where chunk starts in document
|
chunk_start_offset: Character position where chunk starts in document
|
||||||
chunk_end_offset: Character position where chunk ends in document
|
chunk_end_offset: Character position where chunk ends in document
|
||||||
page_number: Page number for PDFs (None for other doc types)
|
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
|
total_chunks: Total number of chunks in document
|
||||||
marked_text: Full text with position markers around the chunk
|
marked_text: Full text with position markers around the chunk
|
||||||
has_before_truncation: True if before_context was truncated
|
has_before_truncation: True if before_context was truncated
|
||||||
@@ -221,7 +224,7 @@ class ChunkContext:
|
|||||||
chunk_start_offset: int
|
chunk_start_offset: int
|
||||||
chunk_end_offset: int
|
chunk_end_offset: int
|
||||||
page_number: int | None
|
page_number: int | None
|
||||||
chunk_index: int
|
chunk_index: int | None
|
||||||
total_chunks: int
|
total_chunks: int
|
||||||
marked_text: str
|
marked_text: str
|
||||||
has_before_truncation: bool
|
has_before_truncation: bool
|
||||||
@@ -271,16 +274,6 @@ async def get_chunk_with_context(
|
|||||||
else (doc_id if isinstance(doc_id, int) else None)
|
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).
|
# Try to get chunk from Qdrant (fast path).
|
||||||
# Prefer chunk_index lookup (always-indexed field) when caller supplied it;
|
# Prefer chunk_index lookup (always-indexed field) when caller supplied it;
|
||||||
# fall back to (chunk_start, chunk_end) lookup otherwise.
|
# fall back to (chunk_start, chunk_end) lookup otherwise.
|
||||||
@@ -319,54 +312,63 @@ async def get_chunk_with_context(
|
|||||||
has_before_truncation = False
|
has_before_truncation = False
|
||||||
has_after_truncation = False
|
has_after_truncation = False
|
||||||
|
|
||||||
# Fetch previous chunk if not first chunk
|
if chunk_index is not None:
|
||||||
if effective_chunk_index > 0:
|
# Fetch previous chunk if not first chunk
|
||||||
before_chunk = await _get_chunk_by_index_from_qdrant(
|
if chunk_index > 0:
|
||||||
user_id, doc_id_int, doc_type, effective_chunk_index - 1
|
before_chunk = await _get_chunk_by_index_from_qdrant(
|
||||||
)
|
user_id, doc_id_int, doc_type, 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 ""
|
|
||||||
)
|
)
|
||||||
# Truncate if requested context_chars < remaining length
|
if before_chunk:
|
||||||
if before_context and len(before_context) > context_chars:
|
# Remove overlap: the last chunk_overlap chars of previous chunk
|
||||||
before_context = before_context[-context_chars:]
|
# 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
|
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
|
# Fetch next chunk if not last chunk
|
||||||
if effective_chunk_index < total_chunks - 1:
|
if chunk_index < total_chunks - 1:
|
||||||
after_chunk = await _get_chunk_by_index_from_qdrant(
|
after_chunk = await _get_chunk_by_index_from_qdrant(
|
||||||
user_id, doc_id_int, doc_type, effective_chunk_index + 1
|
user_id, doc_id_int, doc_type, 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 ""
|
|
||||||
)
|
)
|
||||||
# Truncate if requested context_chars < remaining length
|
if after_chunk:
|
||||||
if after_context and len(after_context) > context_chars:
|
# Remove overlap: the first chunk_overlap chars of next chunk
|
||||||
after_context = after_context[:context_chars]
|
# 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
|
has_after_truncation = True
|
||||||
else:
|
else:
|
||||||
# Could not fetch next chunk, but we're not at end
|
# No chunk_index → can't fetch adjacent chunks via index arithmetic
|
||||||
has_after_truncation = True
|
# 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(
|
marked_text = _insert_position_markers(
|
||||||
before_context=before_context,
|
before_context=before_context,
|
||||||
chunk_text=chunk_text,
|
chunk_text=chunk_text,
|
||||||
after_context=after_context,
|
after_context=after_context,
|
||||||
page_number=page_number,
|
page_number=page_number,
|
||||||
chunk_index=effective_chunk_index,
|
chunk_index=chunk_index,
|
||||||
total_chunks=total_chunks,
|
total_chunks=total_chunks,
|
||||||
has_before_truncation=has_before_truncation,
|
has_before_truncation=has_before_truncation,
|
||||||
has_after_truncation=has_after_truncation,
|
has_after_truncation=has_after_truncation,
|
||||||
@@ -378,7 +380,7 @@ async def get_chunk_with_context(
|
|||||||
chunk_start_offset=chunk_start,
|
chunk_start_offset=chunk_start,
|
||||||
chunk_end_offset=chunk_end,
|
chunk_end_offset=chunk_end,
|
||||||
page_number=page_number,
|
page_number=page_number,
|
||||||
chunk_index=effective_chunk_index,
|
chunk_index=chunk_index,
|
||||||
total_chunks=total_chunks,
|
total_chunks=total_chunks,
|
||||||
marked_text=marked_text,
|
marked_text=marked_text,
|
||||||
has_before_truncation=has_before_truncation,
|
has_before_truncation=has_before_truncation,
|
||||||
@@ -403,16 +405,6 @@ async def get_chunk_with_context(
|
|||||||
f"(Qdrant cache miss, possibly legacy data)"
|
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.)
|
# Fetch full document text (notes, deck cards, news items, etc.)
|
||||||
full_text = await _fetch_document_text(nc_client, doc_id, doc_type, user_id)
|
full_text = await _fetch_document_text(nc_client, doc_id, doc_type, user_id)
|
||||||
if full_text is None:
|
if full_text is None:
|
||||||
@@ -451,7 +443,7 @@ async def get_chunk_with_context(
|
|||||||
chunk_text=chunk_text,
|
chunk_text=chunk_text,
|
||||||
after_context=after_context,
|
after_context=after_context,
|
||||||
page_number=page_number,
|
page_number=page_number,
|
||||||
chunk_index=effective_chunk_index,
|
chunk_index=chunk_index,
|
||||||
total_chunks=total_chunks,
|
total_chunks=total_chunks,
|
||||||
has_before_truncation=has_before_truncation,
|
has_before_truncation=has_before_truncation,
|
||||||
has_after_truncation=has_after_truncation,
|
has_after_truncation=has_after_truncation,
|
||||||
@@ -464,7 +456,7 @@ async def get_chunk_with_context(
|
|||||||
chunk_start_offset=chunk_start,
|
chunk_start_offset=chunk_start,
|
||||||
chunk_end_offset=chunk_end,
|
chunk_end_offset=chunk_end,
|
||||||
page_number=page_number,
|
page_number=page_number,
|
||||||
chunk_index=effective_chunk_index,
|
chunk_index=chunk_index,
|
||||||
total_chunks=total_chunks,
|
total_chunks=total_chunks,
|
||||||
marked_text=marked_text,
|
marked_text=marked_text,
|
||||||
has_before_truncation=has_before_truncation,
|
has_before_truncation=has_before_truncation,
|
||||||
@@ -644,7 +636,7 @@ def _insert_position_markers(
|
|||||||
chunk_text: str,
|
chunk_text: str,
|
||||||
after_context: str,
|
after_context: str,
|
||||||
page_number: int | None,
|
page_number: int | None,
|
||||||
chunk_index: int,
|
chunk_index: int | None,
|
||||||
total_chunks: int,
|
total_chunks: int,
|
||||||
has_before_truncation: bool,
|
has_before_truncation: bool,
|
||||||
has_after_truncation: bool,
|
has_after_truncation: bool,
|
||||||
@@ -659,7 +651,8 @@ def _insert_position_markers(
|
|||||||
chunk_text: The matched chunk
|
chunk_text: The matched chunk
|
||||||
after_context: Text after chunk
|
after_context: Text after chunk
|
||||||
page_number: Optional page number
|
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
|
total_chunks: Total chunks in document
|
||||||
has_before_truncation: Whether before_context is truncated
|
has_before_truncation: Whether before_context is truncated
|
||||||
has_after_truncation: Whether after_context is truncated
|
has_after_truncation: Whether after_context is truncated
|
||||||
@@ -671,7 +664,10 @@ def _insert_position_markers(
|
|||||||
position_parts = []
|
position_parts = []
|
||||||
if page_number is not None:
|
if page_number is not None:
|
||||||
position_parts.append(f"Page {page_number}")
|
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)
|
position_metadata = ", ".join(position_parts)
|
||||||
|
|
||||||
# Build marked text
|
# Build marked text
|
||||||
|
|||||||
@@ -135,3 +135,168 @@ class TestOffsetFallbackGate:
|
|||||||
|
|
||||||
mock_indexed.assert_not_awaited()
|
mock_indexed.assert_not_awaited()
|
||||||
mock_offset.assert_awaited_once()
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user