Repository navigation
fix(documents): no minimum-size merge across a PDF section boundary - #16
Merged
Merged
Conversation
A small PDF passage merged into the next sibling section, but PDF sections have no heading line to prefix, so the sibling's text ended up labelled, embedded and cited under the previous section's path. The Chunker now refuses a merge across a section without a heading line (PDF only in practice); such a passage stays under MinTokens instead. Golden: only pdf-blocks-small.txt changes. Specs 007 and 010 record the exception. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
What changed
Chunker.TryMergenow refuses a minimum-size merge that crosses into a section without a heading line. Only PDF sections have none in practice: plain text is a single section, and Markdown's heading-less section (text before the first heading) is always first.Why
A passage under
MinTokensthat cannot merge backward merges into the next sibling section (SameParent). Markdown puts the sibling's heading line (# Two) in front of the merged part, so the boundary stays visible.PdfSourceExtractor.ToDocumentgives sectionsHeadingLine = null, so section Two's text ended up in a passage labelledOne, with no boundary marker. It was then embedded under "One" and cited as "One".Two fixes were considered. The chosen one is to not merge. The rejected alternative was a synthetic heading line (the last path element) for PDF sections: the passage would still carry the wrong
SectionPath, and heading text would enter PDF passage text, against spec 010 AC-11.Trade-off: such a PDF passage can now stay under
MinTokens.Golden diff
Only
Golden/pdf-blocks-small.txtchanges. All other golden files are byte-identical.#13 path=One pages=4-6held "Back in chapter one after the detail." plus Two's "Tiny." and "Also tiny.".#13 path=One pages=4-4holds the first sentence alone (10 tokens).#14 path=Two pages=5-6holds "Tiny.\n\nAlso tiny.". The gamma passage is renumbered refactor(documents): one extractor per format, Chunker takes a SourceDocument #15, with the same text.Reviewer notes
ChunkerPageTests.Chunk_smallPassageBeforeASiblingSection_isNotMergedIntoItfailed before the fix (1 passage instead of 2).SourceSection.HeadingLinedoc comment record the exception.RAGASSISTANT_UPDATE_GOLDEN=1rewrites every golden file with LF. Withcore.autocrlf=true, the unchanged files then show as modified with no content diff. Restore them withgit checkout --before committing.dotnet build,dotnet format --verify-no-changesanddotnet test --solution RagAssistant.slnxpass (1177/1177). The branch was rebased onto main after refactor(documents): one extractor per format, Chunker takes a SourceDocument #15 merged; the tree is identical to the tested commit.🤖 Generated with Claude Code