Fix chunk overlap duplication, glob ingestion, and store reliability - #35
Merged
Merged
Conversation
Stored chunk text kept the full chunk-overlap window, so concatenated chunks duplicated ~chunk-overlap runes at every boundary in note displays and context windows. Chunks now carry an overlap-free display variant (stored) and an overlap-full embed variant (embedding only). chunkSchemaVersion is bumped to 2 so unchanged files re-ingest. Also remove unreachable store APIs (GetNoteHashes, chunk window query, link upsert helpers) and llmparse test option flagged by deadcode.
The embedded ChromaDB FFI has a 1 MiB response ceiling. BatchIngest cleanup fetches and the Connections BFS link fetches were issuing unpaginated Get calls that can blow that ceiling on large notes and fail the whole command. Route them through paginated helpers.
An empty chunk batch causes BatchIngest to delete the note's previously indexed chunks. For markdown that happens when every chunk falls below --min-chunk-words; for PDFs a transient empty LLM conversion result erases the index entry permanently (the content hash never changes). Skip such notes so their existing index is preserved.
The --has-code search filter emitted chroma.EqBool("has_code", true)
but the metadata was never written, so the filter silently matched
nothing. Persist the flag from the parser's Chunk.HasCode and bump
chunkSchemaVersion to force re-ingestion.
Adds TestBuildChunks_HasCodeFlag and TestBatchIngest_HasCodeMetadata.
Slugify stripped every non-ASCII character, so "Café" and "Cafe"
collided on the same slug and CJK note names became empty. Match
\p{L}\p{N} so Unicode letters and numerals are kept.
IsAttachmentLink treated any suffix that is not .md/.pdf as an
attachment, which misclassified dotted note names such as "Note 1.2.3"
as attachments. Switch to an allowlist of known attachment extensions.
ocrBackend was assigned during auto-detection but never read; no pipeline path performs OCR. Remove TesseractBackend, the OCRBackend interface, RenderPage (PDFBackend method and PDFiumBackend implementation), the ingest auto-detect block, the doctor tesseract check, and OCR references in the docs.
NewPDFiumBackend fixed MaxTotal at 1, so with --workers 4 every extraction after the first blocked on GetInstance(30s) and could fail with "could not get PDFium instance". Accept a pool size and create MinIdle/MaxIdle/MaxTotal equal to the worker count. Use GetInstanceWithContext so cancellation propagates instead of a fixed 30s timeout.
splitByTokenBudget measured pages with len() (bytes) instead of runes, so multi-byte text was charged up to 4x against the budget and the page separator was miscounted (9 vs 7). A page larger than the budget was sent whole, risking context overflow; split it on rune boundaries instead. Chunks now never exceed maxTokens*4 runes.
…ry-After
An env key (e.g. DEEPSEEK_API_KEY) silently beat an explicit
--llm-model naming another provider. A model with a backend prefix
("ollama/llama3", "deepseek-v4-flash") now selects that backend and
errors clearly when it is not configured, falling back to env
detection with a warning. OLLAMA_HOST is now honored even when
OLLAMA_API_KEY is set, and a mismatched model/backend logs a warning.
Retry-After from a 429 could stall ingestion for hours; cap it at
60s. Remove the unreachable budget clamp (New enforces the minimum
window) and the unreachable post-loop return.
… filter block SemanticSearch with limit<=0 produced an invalid WithNResults and a requested limit above the FFI-safe cap was silently truncated; clamp the inputs and warn on truncation. The single-query path set MatchedQueries unconditionally while the multi-query path applied relevance thresholds; route it through filterMatchedQueries and skip non-positive scores for consistency. linkWhereFilters re-derived title/basename candidates that buildLinkTargetResolver already provides, so drop that block and the now-unused ctx parameter.
…esolver Upsert wrote every tag as tag_0..tag_N while TagWhereClause only queries tag_0..tag_19, so exact tag search silently missed tags on notes with more than 20 tags. Cap the written tags (and keep tag_count consistent so decodeTags stays in sync). buildLinkTargetResolver overwrote alias keys in pagination order, so a title shared by two notes resolved to whichever note came last — non-deterministic across runs and between ingest and query. Sort metadata by slug and keep the first mapping (lowest slug wins).
- Embedder interface gains Model(); fileHash now covers it, so switching embedding models invalidates stored hashes (no stale-dimension vectors) - chunkSchemaVersion bumped to 4, forcing re-ingest of existing vaults - Remove dead WithQuiet option (never read) and its 4 call sites - Remove dead ProgressUpdate.Final, done counter, and early-exit cancel; channel close is the only shutdown signal - Drop unused io.Reader/io.Writer params from Pipeline.Run; simplify all test call sites - recordingEmbedder is now mutex-guarded (workers embed concurrently) - Docs: replace quiet-mode claims with reality (slog always writes to stderr, stdout stays clean in machine formats)
The FFI-safe semantic limit warning fired inside semanticSearch on the raw limit argument. HiddenConnectionsDeep multiplies the user limit internally (limit*2 then *2 again, up to 120 for --limit 30), so a 28-chunk deep search logged 28 identical warnings while delivering all results. - semanticSearch now clamps silently; internal fetch multipliers are capped at ffiSafeSemanticLimit - New clampSemanticLimit helper warns exactly once at the user-facing entry points: SemanticSearch, MultiSemanticSearch, HiddenConnections, HiddenConnectionsDeep, GraphBoostedSearch - Tests: one warning when the user's limit exceeds the cap; zero warnings for hidden --deep --limit 30
printResultsFormatted swallowed JSONPath evaluation errors (printed to stderr, returned nil), so 'search --jsonpath bad' exited 0 while 'stats --jsonpath bad' exited 1. Now printResultsFormatted returns the error and all 7 list commands propagate it; invalid JSONPath or an unknown key consistently exits 1 with no stdout output.
The fallback boundary (start+maxRunes) could land inside a \x00INLINE:n\x00 or \x00CODE:n:lang\x00 placeholder when the scan window contained no sentence punctuation or newlines, cutting the token in half. formatChunkText could then no longer resolve it and literal NUL garbage was stored in Text, RichText, and EmbedText. Snap the fallback boundary forward to the token end so the token stays whole in the next part. Sentence and newline breaks cannot land inside tokens, so only the fallback path needs the snap.
filepath.Match treats ** as two adjacent * that never cross a path separator, so globs like Projects/** or **/*.pdf silently missed files more than one level deep (and the match error was discarded, so malformed patterns ingested nothing without a warning). Switch to doublestar.Match for standard recursive glob semantics and surface invalid patterns as an error instead of silently matching zero files.
Satisfies gocritic's ifElseChain rule so the repository is lint-clean from scratch. Behavior is unchanged.
NewLocalEmbedder launched a goroutine and synchronously waited on its done channel, adding no concurrency or timeout. Call ort.NewDefaultEmbeddingFunction directly.
The progress counter only advanced on the success path, so percent never reached 100 when any file failed. Move the increment into a defer so every file is counted, keeping automated ingestion logs accurate even with partial failures.
Dummy 16-dim link embeddings were regenerated randomly on every batch ingest, churning the nb_links HNSW index even for unchanged notes. Seed the PRNG per (source, target) pair so re-ingesting produces identical vectors. Metadata encoding errors from NewDocumentMetadataFromMap were discarded, silently dropping search/backlink/tag metadata for affected records. Propagate them instead.
paginatedGetMetadatas and paginatedGetIDs dereferenced the Get response unconditionally, matching the nil guard Stats already had. A nil response now returns an empty result instead of panicking.
The store is concurrency-sensitive by design (single-writer mutex around ChromaDB) and workers process files in parallel. -race adds ~2-3x test time, still well under a minute on this suite.
nmdra
changed the title
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
1 participant
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.
Summary
Fixes the chunk overlap duplication bug (duplicate sentences at chunk boundaries in
notebrain get) and a batch of correctness and reliability issues found during a full code review of the indexing pipeline, store, parser, and CLI.Key highlights:
Text/RichTextwhile embeddings keep the overlap (EmbedText), eliminating duplicated sentences in reconstructed notes and search output.\x00...\x00placeholder tokens so split boundaries can never cut an inline-link/code token in half and store NUL garbage.**patterns (Projects/**,**/*.pdf) now match at any depth viadoublestar; malformed globs error instead of silently ingesting nothing.nb_linksdummy embeddings are seeded per (source, target) pair, removing HNSW index churn on every re-ingest of unchanged notes.NewDocumentMetadataFromMappropagate instead of silently dropping record metadata; paginated Gets are nil-guarded.Changes by area
Parsing (
internal/parser)cb31b02)777ca0c)[image]/[attachment]markers (schema v5) (147dc20,63daa60)666a24d)5848938)Ingestion (
internal/ingest)**glob support viadoublestar/v4(8f00cfe)77a893d)75b116f)13c97ac)Store & queries (
internal/store)39733c4)b23eb64)946fc8f)96b288d)35ecea0)5d31237)CLI & config (
cmd,internal/configfile)doctordetects corrupted ChromaDB (sqlite magic, segment checks, subprocess probe) (4c55d51)50f3bfe)76959de)3464271)PDF & LLM (
internal/pdfextract,internal/llmparse)263a078)f86b283)Retry-After(b133ba3)ff109b6,6d195b8)CI / tooling
b1eb8ec)Validation
go vet ./...clean;golangci-lint run ./...reports 0 issues (repo lint-clean from scratch)go test -short ./...suite passes; race detector passes on parser/ingest/storedoctorall checks green, full ingest re-run skips unchanged files in ~8 ms with 100% progress, DB stats unchanged (771 notes / 8,740 chunks / 1,731 links)