Fix/149 structural chunker drops documents with noheader - #883
Open
flyingtony1424 wants to merge 7 commits into
Open
Fix/149 structural chunker drops documents with noheader#883flyingtony1424 wants to merge 7 commits into
flyingtony1424 wants to merge 7 commits into
Conversation
… docs Add failing reproduction tests showing that StructuralChunker.chunk() returns an empty list for any document without markdown headings, so heading-less READMEs are silently excluded from the RAG index via the source_type=readme ingestion path. Also documents a related loss: preamble text before the first heading is discarded. Repro: .venv/Scripts/python -m pytest tests/unit/test_issue_149_reproduction.py -v (3 failed as expected; fix planned in PLAN.md for Week 9) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
StructuralChunker._extract_sections() only collected content lines after a heading had been seen, and only saved the final section when the heading stack was non-empty. A document with no markdown headings therefore yielded zero sections, so chunk() returned [] and the doc was silently excluded from the RAG index. The same logic also dropped any preamble text before a document's first heading. Collect content lines unconditionally and always flush the trailing section (skipping ones that are empty/whitespace-only, so adjacent headings with no body don't produce blank chunks). Heading-less sections get an empty heading_path/level 0, consistent with how downstream code already treats top-level sections. Fixes ascherj#149
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Closes #149
Changes
ingestion/chunking/structural_chunker.py:_extract_sections()now appends content lines regardless of whether a heading has been seen yet, and always attempts to flush the trailing section (guarded by a non-empty-after-strip check so blank sections aren't emitted).tests/unit/test_issue_149_reproduction.py: reframed from "expected to fail" reproduction tests to permanent regression tests (docstrings/comments updated; no assertions changed) now that the underlying bug is fixed.JOURNAL.md: added Week 9 check-ins documenting progress.Testing
make test-unit)make test-integration) — not applicable, no integration surface touchedmake lint) — clean on the files changed in this PR; see note below on pre-existing repo-wide failuresmake typecheck) — clean on the files changed in this PR; see note belowHow to manually verify:
.venv/Scripts/python -m pytest tests/unit/test_issue_149_reproduction.py tests/unit/test_structural_chunker.py -v— all 18 should pass, includingtest_document_with_no_headingsand the three issue-149 tests.[]:.venv/Scripts/python -m pytest tests/unit -m unit -qand diffing against a run onmain— the same 52 pre-existing failures should appear in both (see Notes below), with no new failures introduced by this branch.Pre-existing failures observed (unrelated to this change):
Before making any changes, the local venv was missing several declared dev dependencies (
redis,structlog,pypdf,python-jose, etc.); after runningpip install -e ".[dev]", the fulltests/unitsuite has 52 pre-existing failures across unrelated modules (test_bias_detector.py,test_pii_scrubber.py,test_resume_parser.py,test_review_service.py,test_skill_extractor.py,test_tech_detector.py, etc.). I confirmed viagit stashthat these failures exist identically before and after this branch's changes — none touchstructural_chunkeror issue #149. Separately,ruff checkhas one pre-existing unused-variable warning instructural_chunker.py(current_level, present before this PR) that I left untouched as out of scope, andmypyfails repo-wide due to a numpy/Python 3.14 typestub incompatibility (Type statement is only supported in Python 3.12 and greater) unrelated to this change.Screenshots / Demo
N/A — backend chunking logic change, no UI surface.
Notes for Reviewers
PLAN.md): heading-less sections getheading_path=""(via" > ".join([])) andheading_level=0. I greppedrag/andapi/forheading_pathconsumers and found none outside the chunking module itself, so an empty string should be safe — but flagging in case there's a downstream consumer I missed.chunk_indexvalues for previously-ingested documents; may be worth a follow-up re-ingestion pass for existing indexed docs, but I didn't see an existing re-ingestion script wired to run automatically, so I left that out of scope for this fix.