feat(index): incremental reindexing — reuse unchanged files' chunks, vectors, and BM25 postings - #91
Conversation
…vectors, and BM25 postings
Port upstream semble #225 (partial reindexing) to the Rust core. When the
cached index's whole-tree content hash is stale, `load_or_build_index` now
seeds the rebuild with the previous index instead of rebuilding from
scratch: files whose per-file content hash is unchanged keep their chunks,
vector rows, and BM25 postings; only changed files are re-chunked and
re-embedded, and deleted files' postings are dropped.
- `indexing/types.rs`: `FileManifestEntry {hash, start, count}`,
`PreviousIndex::try_new` (alignment checks), `make_chunk_id`.
- `sparse.rs`: `Bm25Index` becomes the id-keyed incremental index from
upstream `bm25.py` (`add_document` / `remove_document` /
`set_doc_order`); `bm25.json` v2 persists `{documents, docOrder}`.
- `create.rs`: `create_index_from_path(.., previous)` reuse path; rows
are moved (not copied) and reused rows are not re-normalised.
- `cache_orchestrator.rs`: `load_previous_for_incremental` (fails closed
on any structural inconsistency) + shared `manifest_compatible`.
- `index.rs`: `files` manifest in `IndexManifest`/`CspIndex`,
`from_path_with_previous`, `INDEX_SCHEMA_VERSION` 1 → 2, and
`load_from_disk` rejects component count mismatches.
- ADR-0005 records the per-file content hash (vs upstream `mtime_ns`)
decision; `semble.md` and both READMEs updated.
Refs #84
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 106 |
| Duplication | 8 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements incremental reindexing keyed on per-file content hashes (SHA-256) rather than modification times. It updates the BM25 index to support incremental document additions and removals, bumps the index schema version to 2, and introduces validation checks to ensure consistency across index components. Review feedback highlights a potential integer overflow in manifest validation that could cause a panic, and suggests replacing std::iter::repeat_n with a more widely supported construct to maintain a lower Minimum Supported Rust Version (MSRV).
- `PreviousIndex::try_new`: sort manifest entries by `(start, count)` so a zero-chunk file that ties with the following file no longer fails the tiling check (which silently disabled incremental reuse for that tree). Regression test `zero_chunk_file_does_not_break_manifest_tiling`. - `Bm25Index::load`: rebuild postings from the persisted term counts via `insert_document` instead of materialising `freq` copies of every term; sum lengths in u64 and reject out-of-range counts. Drop the duplicate `Doc.chunk_id`. - `create_index_from_path`: embed all changed files' chunks in one batched pass (`dense::embed_chunk_refs`) so a cold build keeps the tokenizer's batch parallelism. - `FileManifestEntry::end()`: saturating add so a corrupt manifest fails the range checks instead of overflowing. - `load_previous_for_incremental`: reject a seed whose vector rows do not match the live model's dimension, falling back to a full rebuild. - `parse_manifest`: read `files` through the `FileManifestEntry` serde derive that `save` writes with. - Docs: query-term de-duplication is a real ranking divergence from upstream's query-frequency weighting, not rank-neutral; record it as an open parity gap in ADR-0005 and `semble.md`. Refs #84
Greptile SummaryThis PR adds per-file incremental rebuilding for stale local indexes, preserving unchanged chunks, normalized vectors, and BM25 documents while rebuilding changed files and removing deleted ones.
Confidence Score: 4/5The PR should not merge until path-derived document IDs remain unique for distinct supported filesystem entries. Incremental indexing is structurally guarded and its component alignment is well validated, but converting Unix paths lossily before creating stable BM25 IDs can make a valid repository fail indexing when two filenames collapse to the same string. Files Needing Attention: crates/csp/src/indexing/create.rs, crates/csp/src/indexing/types.rs, crates/csp/src/indexing/sparse.rs
|
| Filename | Overview |
|---|---|
| crates/csp/src/indexing/create.rs | Implements per-file reuse and selective embedding, but lossy path conversion can cause distinct Unix filenames to share BM25 IDs and abort the build. |
| crates/csp/src/indexing/types.rs | Adds file-manifest types, stable chunk IDs, and alignment validation across chunks, vectors, and BM25 order. |
| crates/csp/src/indexing/sparse.rs | Replaces positional BM25 state with an incremental ID-keyed corpus and schema-v2 persistence. |
| crates/csp/src/indexing/cache_orchestrator.rs | Adds fail-closed loading of compatible previous indexes while retaining the existing validated cache-hit path. |
| crates/csp/src/indexing/index.rs | Integrates the file manifest into index construction and persistence and advances the disk schema to version 2. |
| crates/csp/src/indexing/dense/backend.rs | Adds construction from already-normalized rows with dimension validation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Local source fingerprint changes] --> B{Compatible previous index?}
B -- No --> C[Full chunk and embedding build]
B -- Yes --> D[Walk current files]
D --> E{Per-file hash unchanged?}
E -- Yes --> F[Move previous chunks and vectors]
E -- No --> G[Re-chunk, re-embed, replace BM25 documents]
D --> H[Remove documents for deleted files]
F --> I[Set new BM25 document order]
G --> I
H --> I
I --> J[Persist schema-v2 artifacts and file manifest]
Prompt To Fix All With AI
### Issue 1
crates/csp/src/indexing/create.rs:124-130
**Lossy paths collide**
On Unix, two distinct files whose names contain different invalid UTF-8 bytes can produce the same `indexed_path` because `to_string_lossy()` replaces those bytes with the same replacement character. Since BM25 document IDs are built directly from this path and the chunk slot, indexing the second file returns `chunk_id already indexed` and aborts the entire repository build. Please preserve an injective path representation or detect and reject the path collision explicitly.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(index): harden incremental reindex a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…xed file On Unix, file names that differ only in invalid UTF-8 bytes collapse to the same `to_string_lossy` path. The BM25 chunk ids derived from that path would then collide and abort the whole build with "chunk_id already indexed". Keep the first such file, skip the rest with a warning, and add a Linux-only regression test (APFS rejects non-UTF-8 names). Refs #84
|
/gemini review |
…y) into incremental reindexing
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Code Review
This pull request implements incremental reindexing keyed on a per-file content hash (SHA-256) rather than mtime_ns (ADR 0005), porting the incremental machinery from upstream. It introduces a per-file manifest, an id-keyed incremental Bm25Index (persisted as v2), and a PreviousIndex wrapper to reuse unchanged chunks, vectors, and BM25 postings. Feedback from the reviewer highlights a potential bug in content type comparison when duplicates are present, and identifies several style guide violations where files (create.rs, sparse.rs) exceed the 500 LOC limit and the create_index_from_path function exceeds the 50 LOC limit.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ors verbatim - Bm25Index::load rejects a zero term frequency (it would inflate the term's document frequency) so the cache falls back to a full rebuild. - SelectableBasicBackend::load no longer re-normalises rows that were normalised before save, keeping unchanged rows bit-identical across an incremental rebuild seeded from disk. Refs #84
…from_path helpers - create.rs / sparse.rs test modules move to create/tests.rs and sparse/tests.rs, matching the index/, dense/, cache_orchestrator/ layout. - create_index_from_path delegates to open_previous, display_path, take_previous_rows and embed_fresh_rows; behaviour unchanged. - load_previous_for_incremental compares the content selection as a set, so a duplicated request no longer matches a manifest that covers more. Refs #84
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements incremental reindexing keyed on a per-file content hash (sha256) rather than mtime_ns, aligning with ADR 0005. It refactors the BM25 index to support incremental document additions and removals, introduces a per-file manifest to track chunk ranges, and updates the index creation pipeline to reuse unchanged files' chunks, vectors, and postings. The review feedback points out a redundant O(N) vector dimension check in the cache orchestrator that can be optimized to check only the first vector, since the backend already guarantees consistent dimensions.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
1 participant
Summary
Ports upstream semble #225 (partial / incremental reindexing) to the Rust core.
When the cached index's whole-tree content hash is stale,
load_or_build_indexnow seeds the rebuild with the previous index instead of rebuilding from scratch: files whose per-file content hash is unchanged keep their chunks, vector rows, and BM25 postings; only changed files are re-chunked and re-embedded; deleted files' postings are dropped. The cache-hit path is unchanged.Changes
indexing/types.rs(new):FileManifestEntry {hash, start, count},FileManifest,PreviousIndex::try_new(alignment checks: counts agree, ranges tile the chunk list, chunk paths match, BM25 order == manifest-derived ids),make_chunk_id("{path}:{slot}").sparse.rs:Bm25Indexis now the id-keyed incremental index from upstreambm25.py—add_document/remove_document/set_doc_order; ids interned tou32slots (recycled), postingsterm → {slot → tf}.bm25.jsonv2 persists{documents, docOrder}; postings rebuilt on load, inconsistent order rejected.create.rs:create_index_from_path(.., previous)reuse path. Rows are moved out of the previous index (no copies) and reused rows are not re-normalised, so they stay bit-identical.filesmanifest returned (a valid file with no chunks getscount = 0).cache_orchestrator.rs:load_previous_for_incremental(fails closed on any structural inconsistency) + sharedmanifest_compatible(schema version, chunk size, model id, model kind) used bytry_reusetoo.index.rs:filesinIndexManifest/CspIndex/CspIndexState,from_path_with_previous,INDEX_SCHEMA_VERSION1 → 2 (existing caches rebuild once),load_from_diskrejects chunk/vector/BM25 count mismatches.dense/backend.rs:SelectableBasicBackend::from_normalized(skips the normalisation pass for already-normalised rows).semble.md(§3/§4.7–4.9/§4.14/§6.1/§6.3), README + README.ko cache paragraph.Intentional deviations from upstream (ADR-0005,
semble.md§6.1 items 9–10)mtime_ns, so the per-file decision uses the same oracle as csp's whole-treecontentHash(ADR-0002).(k1+1)numerator + de-duplicated query terms). Upstream's own class dropped(k1+1)and weights repeated query terms; both are per-term scale factors, so ranks — all that RRF consumes — are identical.Tests
Mirror upstream
tests/index/test_bm25.pyandtests/index/test_create.py:load_previous_for_incrementalhappy path + 9 fail-closed cases; end-to-endload_or_build_indexreuse on an incremental rebuild.Two existing tests were necessarily updated:
save_writes_manifest_fieldsexpectsschemaVersion2;save_writes_ts_compatible_json→save_writes_documents_and_doc_order(the positional v1bm25.jsonlayout cannot carry stable chunk ids, and the TS implementation it mirrored is gone).Gate:
cargo fmt --all && cargo clippy --all-targets --all-features -- -D warnings && cargo test --workspace— 280 lib + 20 CLI tests pass.Ordering
Per the issue, land after #80 / #82 (both touch
index.rs; expect small conflicts in theCspIndexState/from_gitconstructors on rebase).Related issue
Closes #84
Checklist
mise run test)mise run lint)BREAKING CHANGE:note is included — on-disk cache schema bump (v1 → v2) transparently rebuilds existing caches; no public API removed.Summary by cubic
Rebuilds a stale cache incrementally instead of from scratch. When the whole-tree content hash no longer matches,
load_or_build_indexseeds the rebuild with the previous index: unchanged files keep their chunks, vector rows, and BM25 postings; only changed files are re-chunked and re-embedded; deleted files' postings are dropped.New Features
Bm25Indexis now id-keyed and incremental (add_document/remove_document/set_doc_order);bm25.jsonv2 persists{documents, docOrder}.load_previous_for_incrementalfails closed on structural inconsistency or a backend dimension mismatch, falls back to a full rebuild, and compares the content selection as a set so a duplicate request no longer matches a broader manifest.Bm25Index::loadrejects zero term frequencies.--max-snippet-linesand savings telemetry come in via the main merge.Migration
INDEX_SCHEMA_VERSIONbumps 1 → 2; existing caches rebuild once.(k1+1)numerator; de-duplicating query terms is a known ranking divergence from upstream's query-frequency weighting.Closes #84.
Written for commit 4c5c1cf. Summary will update on new commits.