Skip to content

bug(kb): single-sentence chunks skip subclass metadata — _create_single_sentence_chunk bypasses _enrich_chunks_with_metadata #14467

Closed

Description

@mrveiss

Summary

SemanticChunkerBase.chunk_text() enriches chunks with _extra_chunk_metadata() on the normal path, but the zero/one-sentence fallback returns via _create_single_sentence_chunk (autobot-backend/utils/semantic_chunker_base.py:259-269, reached from chunk_text() at :351), which bypasses _enrich_chunks_with_metadata/_build_chunk_metadata entirely.

So for short input, subclass metadata — optimization_version, and anything else a subclass contributes — is absent, while the same call on longer input carries it.

How it surfaced

Found while reviewing PR #14456 (#14215), which removed OptimizedSemanticChunker after proving GPUSemanticChunker already stamps optimization_version via _extra_chunk_metadata().

That equivalence holds for the multi-sentence path and not for this one. The deleted chunk_text_optimized() looped over the returned chunks after the fact, so it stamped unconditionally regardless of which path produced them.

The removal is still correct — no production consumer ever read that metadata, verified by grepping the value string across the whole tree. But the covering claim is narrower than "every chunk from chunk_text()", and that distinction should live somewhere other than a closed PR's review.

Why it is worth fixing rather than just documenting

The inconsistency is invisible in the shape that matters: a caller reading chunk.metadata["optimization_version"] gets a value for most inputs and a KeyError or None for short ones. Nothing fails; the field is simply missing sometimes. That is the kind of conditional-shape defect that surfaces as a confusing bug report much later, and it would be a genuine one the moment any consumer starts keying on subclass metadata.

It is also a subclass-extension seam: _extra_chunk_metadata() is the documented way for a subclass to contribute metadata, and it silently does not apply on one of the two return paths. Any future subclass inherits the same surprise.

What is needed

Have _create_single_sentence_chunk apply _extra_chunk_metadata() too — or route it through _build_chunk_metadata so there is one place that decides a chunk's metadata rather than two.

Check the empty-input path as well while there; the fallback covers zero and one sentence, and they may not behave identically.

Acceptance criteria

  • A chunk produced from single-sentence input carries the same subclass metadata keys as one from multi-sentence input
  • Asserted by a test that calls the real chunk_text() with one sentence and with several, comparing the metadata key sets rather than checking one literal
  • The empty-input path is covered by the same assertion or explicitly excluded with a reason
  • Metadata assembly happens in one place, so a future subclass cannot inherit a path that skips it

Refs #14215, #14456

Activity

  1. mrveiss commented on Aug 18, 2026

    @mrveiss
    OwnerAuthor

    Closed by PR #14482, squash-merged as 812d6fd5afbb and verified in origin/Dev_new_gui.

    chunk_text() now converges both success branches on a single enrichment call site, so zero- and one-sentence input carries the same subclass metadata as longer input.

    That structural point was the review item, and it matters more than the original bug: the first fix added an enrichment call to the path that was missing one, leaving two sites that had to agree. "Two paths, each independently responsible for remembering the same step" is exactly what produced #14467. Now the branches only assign chunks and one trailing call enriches — a future path added to that function cannot reintroduce the defect by omission. The existing tests were untouched by that collapse (verified: zero diff), confirming it was structural rather than behavioural.

    The test drives the real GPUSemanticChunker.chunk_text() with multi-, single- and empty-sentence input rather than calling the private method, and it discriminates: against the pre-fix shape it fails with missing subclass metadata keys: {'chunking_method', 'optimization_version', 'embedding_model', 'source_metadata', 'chunk_index', 'total_chunks'}.

    No stored data is affected, and that was traced rather than assumed. knowledge/pipeline/loaders/chromadb_loader.py:91's **chunk.metadata only ever receives ProcessedChunk objects from the pipeline's own extractor, which imports none of these classes. Of the two production callers of SemanticChunkerBase.chunk_text(), knowledge_sync_incremental.py reads only content/semantic_score/sentences and builds its own metadata dict, and advanced_rag_optimizer.py never calls chunk_text() at all — it reads the chunker as a truthy flag. chunk_document(), the one method surfacing chunk.metadata verbatim, has zero production callers. So these fields were never persisted on either the buggy or the fixed path, and no migration is owed.

    Two follow-ups filed rather than folded in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions