Feature/102 context aware chunking via llm#131
Conversation
-based on AGENT.md with some additional refactoring
- Extend to_parsed_chunk with has_context_block/has_overlap flags and their character ranges, defaulting to false/empty so chunk_text and chunk_code behavior is unchanged. - Add ingestion/context_aware_chunker.py with the prompt builder and LLM call that will back the new opt-in LLM-based chunking strategy (semantic boundary detection + selective contextualization).
…nd contextualization - Add new chunk_text_context_aware function with LLM integration and robust fallback - Enhance parse_text and parse_pdf to optionally use context-aware chunking - Update parse dispatcher to forward chunking flags and LLM client - Extend IngestRequest schema and ingest endpoint with chunking toggle flags - Update README and .env.example with new CONTEXT_AWARE_CHUNKING_MAX_CHARS setting and detailed context-aware chunking docs - Add comprehensive unit and integration tests covering all new features and edge cases Note: Batch ingest (/ingest/sync) not modified, uses legacy chunk_text only.
| semantic_boundaries: bool = Field( | ||
| default=True, | ||
| description=( | ||
| "Only affects text and PDF content. When true, an LLM chooses " | ||
| "chunk boundaries based on semantic coherence (topic shifts, " | ||
| "section boundaries) instead of the default character-length " | ||
| "accumulation. Falls back to the default chunker if the " | ||
| "content is too large for the LLM or the LLM output is " | ||
| "invalid. Independently toggleable from 'contextualize'." | ||
| ), | ||
| ) | ||
| contextualize: bool = Field( | ||
| default=True, | ||
| description=( | ||
| "Only affects text and PDF content. When true, an LLM flags " | ||
| "which chunks would benefit from a short situating context " | ||
| "block (Anthropic-style Contextual Retrieval) and prepends it " | ||
| "to their content; self-contained chunks are left untouched. " | ||
| "Falls back to the default chunker if the content is too " | ||
| "large for the LLM or the LLM output is invalid. " | ||
| "Independently toggleable from 'semantic_boundaries'." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Blocking: these flags should default to false.
The issue describes context-aware chunking as opt-in/additive and explicitly says existing ingestion behavior should remain unchanged when the feature is not enabled. With both flags defaulting to true, normal /ingest requests now invoke the LLM-based chunker by default, which changes chunk boundaries, latency, and fallback behavior for existing callers.
Suggestion:
semantic_boundaries: bool = Field(default=False, ...)
contextualize: bool = Field(default=False, ...)
Then callers can opt in per request when they want LLM-based semantic chunking and/or contextualization.
| def parse( | ||
| filename: str, | ||
| content: bytes, | ||
| llm: LLMClient | None = None, | ||
| semantic_boundaries: bool = True, | ||
| contextualize: bool = True, | ||
| ) -> list[ParsedChunk]: |
There was a problem hiding this comment.
Please make the parser-level defaults false as well.
Even if the API schema defaults are changed, the lower-level parser functions should also default to legacy behavior. Otherwise, internal callers/tests can accidentally enable LLM chunking just by passing an LLM client.
Suggested default:
semantic_boundaries: bool = False
contextualize: bool = False
| return chunk_text_context_aware( | ||
| filename, | ||
| text, | ||
| llm, | ||
| semantic_boundaries=semantic_boundaries, | ||
| contextualize=contextualize, | ||
| ) |
There was a problem hiding this comment.
Same opt-in concern here: parse_text should default to legacy chunking.
The context-aware path should only run when semantic_boundaries or contextualize is true and an LLM client is available. With both flags false, this should call the existing chunk_text path exactly as before.
| if llm is None | ||
| else chunk_text_context_aware( | ||
| filename, | ||
| text, | ||
| llm, | ||
| semantic_boundaries=semantic_boundaries, | ||
| contextualize=contextualize, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Same opt-in concern here: parse_pdf should default to legacy chunking.
The context-aware path should only run when semantic_boundaries or contextualize is true and an LLM client is available. With both flags false, this should call the existing chunk_text path exactly as before.
| parsed_chunks = parse( | ||
| body.filename, | ||
| content_bytes, | ||
| llm=llm, | ||
| semantic_boundaries=body.semantic_boundaries, | ||
| contextualize=body.contextualize, | ||
| ) |
There was a problem hiding this comment.
Consider only passing the LLM into parse() when one of the context-aware features is enabled.
This makes it much harder for default ingest to accidentally call the LLM and makes the “existing behavior unchanged” guarantee easier to test.
| parsed_chunks = parse( | |
| body.filename, | |
| content_bytes, | |
| llm=llm, | |
| semantic_boundaries=body.semantic_boundaries, | |
| contextualize=body.contextualize, | |
| ) | |
| #Suggested pattern: | |
| use_context_aware_chunking = body.semantic_boundaries or body.contextualize | |
| parsed_chunks = parse( | |
| body.filename, | |
| content_bytes, | |
| llm=llm if use_context_aware_chunking else None, | |
| semantic_boundaries=body.semantic_boundaries, | |
| contextualize=body.contextualize, | |
| ) |
| @@ -0,0 +1,445 @@ | |||
| """LLM-based context-aware chunking strategy for text content. | |||
|
|
|||
There was a problem hiding this comment.
Please hard-cap context block length before prepending it.
The acceptance criteria say chunk size constraints must still be respected and the prepended context block counts toward the budget. If the LLM returns a long context block, duplicating it during hard-splitting can exceed CHUNK_SIZE or leave very little room for source text.
Suggested rule:
- cap context blocks to a small maximum, e.g. min(300, chunk_size // 4)
- subtract the context length from the available source-text budget
- assert/fallback if the final chunk would exceed chunk_size
| def _trim_context_block(context_block: str, chunk_size: int) -> str: | |
| max_context_chars = min(300, max(0, chunk_size // 4)) | |
| return context_block.strip()[:max_context_chars].rstrip() |
| return results | ||
|
|
||
|
|
||
| def chunk_text_context_aware( |
There was a problem hiding this comment.
Please validate LLM-provided split points strictly before using them.
The LLM can return duplicate, unsorted, missing, or out-of-range paragraph indices. Those should not produce empty chunks or dropped text. If validation fails, we should fall back to chunk_text.
Suggested validation:
- boundaries are integers
- sorted ascending
- unique
- first boundary is 0
- final boundary reaches the end, or the implementation appends the end safely
- all boundaries are within paragraph range
- no empty chunk ranges
LinseCed
left a comment
LinseCed
left a comment
There was a problem hiding this comment.
Verification
Checked the branch out locally: full suite passes (266 passed; the 3 failures in tests/llm/test_ollama_client.py are pre-existing Ollama-connectivity tests untouched by this PR), ruff check + ruff format --check clean, pyright src/ reports 0 errors.
What's good
- The fallback design is solid: size ceiling, unreachable LLM, and malformed/invalid output all degrade to
chunk_text— no request can fail because of this feature, and each fallback is logged. - Prompting over small segment indices instead of character offsets is the right call, and the docstring explains why.
- The
chunk_textrefactor intosplit_into_paragraphs/group_paragraphs_into_chunksis clean and behavior-preserving. - Boundary validation (sorted, deduped, in-range, starts at 0) is thorough; good test coverage including the fallback paths, hard-split, and API flag handling; README and
.env.exampleupdated.
Should fix before merge
-
Unvalidated context-block length can cause a chunk explosion.
_Payload.context_blocksvalues are never length-checked. In_hard_split_with_context, iflen(context_block) >= chunk_size(default 512),body_budgetclamps to 1 andbody_overlapto 0 — the loop then emits one sub-chunk per character of body, each of which still exceedschunk_size. A verbose (or prompt-injected — the document content is attacker-influenceable input to this LLM call) response with a long context block on a ~24k-char text produces tens of thousands of chunks, each individually embedded and stored. Validate context block length in_request_chunking_plan(e.g. reject blocks over some fraction ofchunk_size→ContextAwareChunkingError→ fallback), or truncate. -
Empty
boundariesin semantic mode silently degrades below the fallback. The prompt requires boundaries to start at 0, but an empty list passes validation, andboundaries = payload.boundaries or [0]then turns the whole document into a single chunk — which, if oversized, gets hard-split at raw character offsets (mid-word, no paragraph awareness). That's strictly worse than thechunk_textfallback the module promises. Whensemantic_boundaries=True, treat emptyboundariesas invalid output and fall back.
Worth considering
-
"Opt-in" vs default-on. The PR text and module docstring call this opt-in, but both flags default to
true— every existing/api/v1/ingestcaller silently gains one LLMgeneratecall per text artifact and one per PDF page (a 100-page PDF = 100 sequential LLM calls in a sync route). With a paid backend (LLM_BACKEND=anthropic/openai) this is also a silent cost change. Worth an explicit team decision: either defaultfalse, or keeptrueand drop the "opt-in" wording. -
Oversized semantic chunks are hard-split at character level. When the LLM groups paragraphs into a chunk larger than
chunk_size, it goes straight to character-window splitting. Re-runninggroup_paragraphs_into_chunkson just that chunk's paragraphs would preserve paragraph boundaries and reuse existing logic. -
Metadata inconsistencies (minor): the legacy path's paragraph-overlap chunks still report
has_overlap=false(only_hard_split_with_contextsets it), andcontext_block_rangeincludes the trailing\n\nseparator. Both fine if intentional — worth a doc note. -
Scope creep:
AGENTS.md(94 lines of agent-tooling docs) and theopencode.jsongitignore entry are unrelated to this feature. Fine content, but they'd be cleaner as a separate chore PR.
Verdict
Solid, well-tested feature with genuinely robust fallback behavior. Items 1 and 2 are cheap validation additions in _request_chunking_plan that close real degradation paths; item 3 needs a deliberate decision before merge since it changes default behavior for existing clients.
3 participants
Context-Aware Chunking Feature
Overview
This PR introduces a new context-aware chunking strategy for text and PDF content ingestion in the SprintStart AI service. This strategy leverages a Large Language Model (LLM) to produce semantically coherent chunk boundaries and selectively prepend short situating context blocks to the resulting chunks. It serves as an opt-in, additive alternative to the existing paragraph-based character-limit chunking.
Key Changes
Chunk Metadata Enhancements
to_parsed_chunknow supports additional metadata flags:has_context_block: whether a short context block was prependedcontext_block_range: character range of the context block within chunk contenthas_overlap: whether a chunk includes overlap content from adjacent chunksoverlap_range: character range of the overlap within chunk contentNew Context-Aware Chunking Implementation
Implemented in
src/ingestion/context_aware_chunker.py, including:chunk_textwhen the LLM is unreachable or produces invalid responsesParsing Path Updates
parse_textandparse_pdfnow accept optional LLM client and flags for semantic boundaries and contextualization, defaulting to enabledparse()forwards these parameters to text- and PDF-specific parsersAPI Changes
IngestRequestschema extended with two new boolean flags:semantic_boundariesandcontextualize(both defaulttrue) to control this feature per request/api/v1/ingestendpoint updated to forward these flags and LLM client to the parser/api/v1/ingest/sync(batch ingest) intentionally left unchanged to avoid backend contract changes; it continues using legacy chunkingDocumentation Updates
CONTEXT_AWARE_CHUNKING_MAX_CHARSand detailed description of the new chunking behavior.env.exampleupdated to include this new environment variableTests
Important Notes / Considerations
Scope:
This feature currently applies only to the single artifact ingest API (
POST /api/v1/ingest). The batch ingest API/api/v1/ingest/synccontinues using the legacy chunking strategy to avoid changes in wire contracts with the backend. Enabling this behavior in batch ingest would require further backend coordination and is out of scope for this PR.Chunk Inspector CLI:
The existing offline
scripts/chunk_inspector_cli.pydoes NOT yet support the new context-aware chunking. Extending it to utilize the LLM-based chunker with toggles for flags and LLM client configuration would be a larger undertaking and is deferred. Manual API testing can be done instead.LLM Client Handling:
The chunking code uses the injected
LLMClientinterface to remain agnostic to backend providers (Ollama, OpenAI, Anthropic). The fallback logic is designed to be robust to network or parsing failures.Performance:
The LLM-based chunker performs an additional
generatecall per ingested text or PDF artifact (once per PDF page). The environment variableCONTEXT_AWARE_CHUNKING_MAX_CHARScaps input size to mitigate latency and failure risks.Suggested Review Focus Areas
src/ingestion/context_aware_chunker.py, particularly prompt formulation, JSON parsing, and chunk splitting behavior.env.exampledocumentation for clarity and completenessHow to Test Manually
/api/v1/ingestendpoint with text or PDF files, examining chunk counts and metadata to verify context blocks and semantic chunking.This PR delivers a substantial quality and relevance improvement to the chunking stage of ingestion with minimal disruption to existing workflows and optional feature activation per request.
Type of PR — pick one:
1 Process baseline — every box must be ticked, on every PR
TODO/FIXMEwithout a follow-up issue2 Outcome proof — fill the bullets that match your PR type
Functional / Mixed PR:
e.g. upload a markdown file, then assert the chat answer cites it
Non-functional / Mixed PR:
e.g. benchmark output for
chat p95 < 2 s, axe audit fora11y ≥ 90, scan report for0 critical CVEs3 Cross-cutting impact — tick the areas this PR touches
For each area below, ask: "does my PR touch this?"
-> If yes → tick the area and complete its sub-checks (they become mandatory).
-> If no → skip it.
-> If nothing applies → tick "None of the above".
UI touched
e.g. new button has a focus ring and is readable on a 360 px screen
Backend endpoint added or changed
e.g.
/uploadrejects files > 10 MB with a clear errorStores user data, ingested content, or LLM output
New env var or config
.env.exampleand mentioned inREADME.mde.g.
LLM_API_KEY=...— only the key name lives in.env.exampleDeployment / build changed
None of the above — purely internal change