Repository navigation
refactor(index): migrate FlatBuffers from flatc IDL to kotatsu reflection - #486
16bit-ykiko wants to merge 4 commits into
Conversation
…tion Replace the flatc-generated serialization of TUIndex / ProjectIndex / MergedIndex with kotatsu's reflection-driven FlatBuffers codec. The in-memory types are now serialized directly — no IDL, no generated code, no hand-written builder/reader walks. - Wire layout is derived from the reflected structs themselves. Runtime-only state is marked kota::meta::skip<> (TUIndex::file_indices, IncludeGraph::file_table, Impl::canonical_ref_counts / occurrences_cache) and derived state is rebuilt after decoding. - Non-reflectable leaves are bridged by type-level codec adapters in index/serialization.h: RelationKind (u32), SymbolKind (u8), roaring bitmaps (bytes, non-portable format as before), std::chrono::milliseconds (i64) and the shard PathPool (string table, re-interned on decode). - Occurrence / Relation / LocalSourceRange gain defaulted operator<=>; the codec writes map entries sorted by key, which the zero-copy buffer paths binary-search. MergedIndex's lazy lookups (lookup / need_update / has_contribution / find_symbol / content / line_starts) now run on kotatsu's table_view proxy instead of flatc accessors. - Compaction of removed-masked state moves from serialize-time copying into an explicit Impl::compact() that vacuums in place; serialize() therefore takes a mutable self. - TUIndex::build() drains the FileID-keyed scratch map into the canonical path-id-keyed path_file_indices instead of copying, halving peak index memory in the worker; tests access per-file indexes via path ids now. - Decoding runs kotatsu's schema-driven deep verifier, so corrupt or truncated blobs are rejected instead of read out of bounds; TUIndex::from takes an explicit size for the same reason. - index_format_version bumps to 2. flatc-era blobs are additionally rejected by the kotatsu buffer identifier, so stale caches rebuild in the background as designed. Drops src/index/schema.fbs, the flatc codegen target and the direct flatbuffers/conda dependencies (flatbuffers now comes transitively via kota::codec::flatbuffers). Depends on the kotatsu branch fix/fbs-reflection-codec, pinned in cmake/package.cmake.
📝 WalkthroughWalkthroughThis PR replaces the manual FlatBuffers schema/codegen workflow with kota::codec-based serialization for index data, updates index containers and versioning, switches TU index lookups to path-keyed storage, and adjusts call sites and tests to the new serialized layout. ChangesFlatBuffers to kota::codec migration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TUIndex
participant kota_codec_fbs
participant MergedIndex
Caller->>TUIndex: from(data, size)
TUIndex->>kota_codec_fbs: from_flatbuffer(span)
kota_codec_fbs-->>TUIndex: decoded TUIndex or failure
TUIndex-->>Caller: TUIndex
Caller->>MergedIndex: load(path)
MergedIndex->>kota_codec_fbs: verify_flatbuffer<Impl>(bytes)
kota_codec_fbs-->>MergedIndex: verification result
MergedIndex->>kota_codec_fbs: table_view<Impl>(bytes)
kota_codec_fbs-->>MergedIndex: format_version and fields
MergedIndex-->>Caller: loaded index or empty
Caller->>MergedIndex: serialize(out)
MergedIndex->>MergedIndex: compact()
MergedIndex->>kota_codec_fbs: to_flatbuffer(*impl)
kota_codec_fbs-->>MergedIndex: encoded bytes
MergedIndex-->>Caller: write bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/index/project_index.cpp (1)
91-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent serialize failure persists an empty blob without any diagnostic.
On encode failure the
assertis a no-op in release builds and the function returns having written nothing toos, so the caller persists an empty/absent blob. It recovers on next load (rejected → rebuild), but the failure is invisible in production. Consider logging a warning so the failed persist is diagnosable rather than silently swallowed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index/project_index.cpp` around lines 91 - 96, The ProjectIndex serialization path silently drops failures when kota::codec::fbs::to_flatbuffer returns no buffer, since the assert is disabled in release builds and the function just returns from the persist flow. Update the ProjectIndex write/persist logic around encoded handling to emit a warning or error when serialization fails before returning, so the failure is visible in production while keeping the existing early-exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index/merged_index.cpp`:
- Around line 356-372: The rebuild of MergedIndex::canonical_ref_counts is
trusting the decoded max_canonical_id field, which can cause an excessive
allocation from a corrupted but structurally valid shard. In
MergedIndex::load_in_memory (or the nearby rebuild block), compute the needed
size from the actual canonical ids referenced in header_contexts and
compilation_contexts, then clamp or validate before calling resize; keep the
existing bounds-checked count_ref logic but avoid using index.max_canonical_id
as the direct allocation size. Use the surrounding load/verify flow in
MergedIndex to ensure the derived count array is sized only to what is
semantically required.
In `@src/index/serialization.h`:
- Around line 146-162: The PathPool deserialization in deserialize_visit<Vis,
clice::index::PathPool, Config>::visit is silently dropping empty strings, which
changes later path_id() assignments and can corrupt id-based references. Update
the loop so an empty entry is treated as a decode failure: detect it while
rebuilding out.paths/cache and return false instead of skipping it, keeping the
PathPool state consistent with the original serialized ordering.
---
Nitpick comments:
In `@src/index/project_index.cpp`:
- Around line 91-96: The ProjectIndex serialization path silently drops failures
when kota::codec::fbs::to_flatbuffer returns no buffer, since the assert is
disabled in release builds and the function just returns from the persist flow.
Update the ProjectIndex write/persist logic around encoded handling to emit a
warning or error when serialization fails before returning, so the failure is
visible in production while keeping the existing early-exit behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf58cd1a-bae3-4efa-9f83-3fdcb297961c
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
CMakeLists.txtcmake/package.cmakepixi.tomlsrc/index/include_graph.hsrc/index/merged_index.cppsrc/index/merged_index.hsrc/index/project_index.cppsrc/index/schema.fbssrc/index/serialization.hsrc/index/tu_index.cppsrc/index/tu_index.hsrc/semantic/relation_kind.hsrc/server/compiler/compiler.cppsrc/server/index/background_indexer.cppsrc/syntax/token.htests/unit/index/index_query_tests.cpptests/unit/index/merged_index_tests.cpptests/unit/index/tu_index_tests.cpp
💤 Files with no reviewable changes (2)
- src/index/schema.fbs
- pixi.toml
| // Rebuild the derived reference counts from the decoded contexts. Ids are | ||
| // bounds-checked: the verifier guarantees structure, not semantics, so a | ||
| // corrupted-but-well-formed shard must not index out of range. | ||
| index.canonical_ref_counts.resize(index.max_canonical_id, 0); | ||
|
|
||
| for(auto entry: *root->header_contexts()) { | ||
| HeaderContext context; | ||
| auto path = entry->path_id(); | ||
| context.version = entry->version(); | ||
| for(auto include: *entry->includes()) { | ||
| index.canonical_ref_counts[include->canonical_id()] += 1; | ||
| context.includes.emplace_back(*safe_cast<IncludeContext>(include)); | ||
| } | ||
| index.header_contexts.try_emplace(path, std::move(context)); | ||
| } | ||
|
|
||
| for(auto entry: *root->compilation_contexts()) { | ||
| CompilationContext context; | ||
| auto path = entry->path_id(); | ||
| context.version = entry->version(); | ||
| context.canonical_id = entry->canonical_id(); | ||
| context.build_at = entry->build_at(); | ||
| for(auto include: *entry->include_locations()) { | ||
| context.include_locations.emplace_back(*safe_cast<IncludeLocation>(include)); | ||
| } | ||
| if(entry->dep_hashes()) { | ||
| for(auto dep: *entry->dep_hashes()) { | ||
| context.dep_hashes.emplace_back(*safe_cast<DepHash>(dep)); | ||
| } | ||
| auto count_ref = [&](std::uint32_t canonical_id) { | ||
| if(canonical_id < index.canonical_ref_counts.size()) { | ||
| index.canonical_ref_counts[canonical_id] += 1; | ||
| } | ||
| index.compilation_contexts.try_emplace(path, std::move(context)); | ||
| } | ||
|
|
||
| // Count ref counts from compilation contexts. | ||
| for(auto entry: *root->compilation_contexts()) { | ||
| index.canonical_ref_counts[entry->canonical_id()] += 1; | ||
| } | ||
|
|
||
| // Deserialize removed bitmap. | ||
| if(root->removed() && root->removed()->size() > 0) { | ||
| index.removed = read_bitmap(root->removed()); | ||
| } | ||
|
|
||
| for(auto entry: *root->occurrences()) { | ||
| index.occurrences.try_emplace(*safe_cast<Occurrence>(entry->occurrence()), | ||
| read_bitmap(entry->context())); | ||
| } | ||
|
|
||
| for(auto entry: *root->relations()) { | ||
| auto& relations = index.relations[entry->symbol()]; | ||
| for(auto relation_entry: *entry->relations()) { | ||
| relations.try_emplace(*safe_cast<Relation>(relation_entry->relation()), | ||
| read_bitmap(relation_entry->context())); | ||
| }; | ||
| for(auto& [_, context]: index.header_contexts) { | ||
| for(auto& include: context.includes) { | ||
| count_ref(include.canonical_id); | ||
| } | ||
| } | ||
|
|
||
| if(root->content()) { | ||
| index.content = root->content()->str(); | ||
| for(auto& [_, context]: index.compilation_contexts) { | ||
| count_ref(context.canonical_id); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded canonical_ref_counts resize from an untrusted decoded field.
index.canonical_ref_counts.resize(index.max_canonical_id, 0) trusts max_canonical_id straight out of the decoded shard. verify_flatbuffer/from_flatbuffer only guarantee structural (offset/bounds) validity, not that this scalar is semantically reasonable. A corrupted or maliciously-crafted (but structurally valid) shard on disk with max_canonical_id near UINT32_MAX triggers a multi-GB allocation here, which is exactly the "must never crash the server" scenario the surrounding comments call out (MergedIndex::load's deep-verify docstring at Line 388-391). This path runs whenever a shard is mutated (merge/remove/merge_symbols → load_in_memory), not just on read.
Bound the resize by what's actually referenced in the decoded contexts instead of trusting the raw field directly.
🛡️ Proposed fix
- index.canonical_ref_counts.resize(index.max_canonical_id, 0);
- auto count_ref = [&](std::uint32_t canonical_id) {
- if(canonical_id < index.canonical_ref_counts.size()) {
- index.canonical_ref_counts[canonical_id] += 1;
- }
- };
- for(auto& [_, context]: index.header_contexts) {
- for(auto& include: context.includes) {
- count_ref(include.canonical_id);
- }
- }
- for(auto& [_, context]: index.compilation_contexts) {
- count_ref(context.canonical_id);
- }
+ // Bound the allocation by the maximum id actually referenced by the
+ // decoded contexts (clamped to max_canonical_id), rather than trusting
+ // max_canonical_id directly — a corrupted-but-structurally-valid shard
+ // must not be able to force an unbounded allocation here.
+ std::uint32_t max_ref = 0;
+ for(auto& [_, context]: index.header_contexts) {
+ for(auto& include: context.includes) {
+ max_ref = std::max(max_ref, include.canonical_id);
+ }
+ }
+ for(auto& [_, context]: index.compilation_contexts) {
+ max_ref = std::max(max_ref, context.canonical_id);
+ }
+ auto safe_size = std::min<std::uint64_t>(index.max_canonical_id,
+ static_cast<std::uint64_t>(max_ref) + 1);
+ index.canonical_ref_counts.resize(safe_size, 0);
+ auto count_ref = [&](std::uint32_t canonical_id) {
+ if(canonical_id < index.canonical_ref_counts.size()) {
+ index.canonical_ref_counts[canonical_id] += 1;
+ }
+ };
+ for(auto& [_, context]: index.header_contexts) {
+ for(auto& include: context.includes) {
+ count_ref(include.canonical_id);
+ }
+ }
+ for(auto& [_, context]: index.compilation_contexts) {
+ count_ref(context.canonical_id);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Rebuild the derived reference counts from the decoded contexts. Ids are | |
| // bounds-checked: the verifier guarantees structure, not semantics, so a | |
| // corrupted-but-well-formed shard must not index out of range. | |
| index.canonical_ref_counts.resize(index.max_canonical_id, 0); | |
| for(auto entry: *root->header_contexts()) { | |
| HeaderContext context; | |
| auto path = entry->path_id(); | |
| context.version = entry->version(); | |
| for(auto include: *entry->includes()) { | |
| index.canonical_ref_counts[include->canonical_id()] += 1; | |
| context.includes.emplace_back(*safe_cast<IncludeContext>(include)); | |
| } | |
| index.header_contexts.try_emplace(path, std::move(context)); | |
| } | |
| for(auto entry: *root->compilation_contexts()) { | |
| CompilationContext context; | |
| auto path = entry->path_id(); | |
| context.version = entry->version(); | |
| context.canonical_id = entry->canonical_id(); | |
| context.build_at = entry->build_at(); | |
| for(auto include: *entry->include_locations()) { | |
| context.include_locations.emplace_back(*safe_cast<IncludeLocation>(include)); | |
| } | |
| if(entry->dep_hashes()) { | |
| for(auto dep: *entry->dep_hashes()) { | |
| context.dep_hashes.emplace_back(*safe_cast<DepHash>(dep)); | |
| } | |
| auto count_ref = [&](std::uint32_t canonical_id) { | |
| if(canonical_id < index.canonical_ref_counts.size()) { | |
| index.canonical_ref_counts[canonical_id] += 1; | |
| } | |
| index.compilation_contexts.try_emplace(path, std::move(context)); | |
| } | |
| // Count ref counts from compilation contexts. | |
| for(auto entry: *root->compilation_contexts()) { | |
| index.canonical_ref_counts[entry->canonical_id()] += 1; | |
| } | |
| // Deserialize removed bitmap. | |
| if(root->removed() && root->removed()->size() > 0) { | |
| index.removed = read_bitmap(root->removed()); | |
| } | |
| for(auto entry: *root->occurrences()) { | |
| index.occurrences.try_emplace(*safe_cast<Occurrence>(entry->occurrence()), | |
| read_bitmap(entry->context())); | |
| } | |
| for(auto entry: *root->relations()) { | |
| auto& relations = index.relations[entry->symbol()]; | |
| for(auto relation_entry: *entry->relations()) { | |
| relations.try_emplace(*safe_cast<Relation>(relation_entry->relation()), | |
| read_bitmap(relation_entry->context())); | |
| }; | |
| for(auto& [_, context]: index.header_contexts) { | |
| for(auto& include: context.includes) { | |
| count_ref(include.canonical_id); | |
| } | |
| } | |
| if(root->content()) { | |
| index.content = root->content()->str(); | |
| for(auto& [_, context]: index.compilation_contexts) { | |
| count_ref(context.canonical_id); | |
| } | |
| // Rebuild the derived reference counts from the decoded contexts. Ids are | |
| // bounds-checked: the verifier guarantees structure, not semantics, so a | |
| // corrupted-but-well-formed shard must not index out of range. | |
| // Bound the allocation by the maximum id actually referenced by the | |
| // decoded contexts (clamped to max_canonical_id), rather than trusting | |
| // max_canonical_id directly — a corrupted-but-structurally-valid shard | |
| // must not be able to force an unbounded allocation here. | |
| std::uint32_t max_ref = 0; | |
| for(auto& [_, context]: index.header_contexts) { | |
| for(auto& include: context.includes) { | |
| max_ref = std::max(max_ref, include.canonical_id); | |
| } | |
| } | |
| for(auto& [_, context]: index.compilation_contexts) { | |
| max_ref = std::max(max_ref, context.canonical_id); | |
| } | |
| auto safe_size = std::min<std::uint64_t>(index.max_canonical_id, | |
| static_cast<std::uint64_t>(max_ref) + 1); | |
| index.canonical_ref_counts.resize(safe_size, 0); | |
| auto count_ref = [&](std::uint32_t canonical_id) { | |
| if(canonical_id < index.canonical_ref_counts.size()) { | |
| index.canonical_ref_counts[canonical_id] += 1; | |
| } | |
| }; | |
| for(auto& [_, context]: index.header_contexts) { | |
| for(auto& include: context.includes) { | |
| count_ref(include.canonical_id); | |
| } | |
| } | |
| for(auto& [_, context]: index.compilation_contexts) { | |
| count_ref(context.canonical_id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index/merged_index.cpp` around lines 356 - 372, The rebuild of
MergedIndex::canonical_ref_counts is trusting the decoded max_canonical_id
field, which can cause an excessive allocation from a corrupted but structurally
valid shard. In MergedIndex::load_in_memory (or the nearby rebuild block),
compute the needed size from the actual canonical ids referenced in
header_contexts and compilation_contexts, then clamp or validate before calling
resize; keep the existing bounds-checked count_ref logic but avoid using
index.max_canonical_id as the direct allocation size. Use the surrounding
load/verify flow in MergedIndex to ensure the derived count array is sized only
to what is semantically required.
| template <typename Vis, typename Config> | ||
| struct deserialize_visit<Vis, clice::index::PathPool, Config> { | ||
| static bool visit(Vis& vis, clice::index::PathPool& out) { | ||
| std::vector<std::string> paths; | ||
| if(!decode_value<Config>(vis, paths)) { | ||
| return false; | ||
| } | ||
| out.paths.clear(); | ||
| out.cache.clear(); | ||
| for(const auto& path: paths) { | ||
| if(!path.empty()) { | ||
| out.path_id(path); | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Skipping empty paths silently shifts subsequent path ids.
path_id() assigns ids by insertion order (paths.size() at call time), so skipping an empty entry mid-list shifts every later path's id down by one relative to how it was originally serialized. Any stored references that key off the original path id (e.g. path_file_indices, symbol reference sets) would then resolve to the wrong path after reload — a silent data-integrity issue rather than the "never misread" failure mode the rest of this migration aims for.
Since PathPool::path_id asserts !path.empty(), an empty entry can only appear here due to blob corruption; treat it as a decode failure instead of quietly dropping it.
🛡️ Proposed fix
out.paths.clear();
out.cache.clear();
for(const auto& path: paths) {
- if(!path.empty()) {
- out.path_id(path);
- }
+ if(path.empty()) {
+ return false;
+ }
+ out.path_id(path);
}
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <typename Vis, typename Config> | |
| struct deserialize_visit<Vis, clice::index::PathPool, Config> { | |
| static bool visit(Vis& vis, clice::index::PathPool& out) { | |
| std::vector<std::string> paths; | |
| if(!decode_value<Config>(vis, paths)) { | |
| return false; | |
| } | |
| out.paths.clear(); | |
| out.cache.clear(); | |
| for(const auto& path: paths) { | |
| if(!path.empty()) { | |
| out.path_id(path); | |
| } | |
| } | |
| return true; | |
| } | |
| }; | |
| template <typename Vis, typename Config> | |
| struct deserialize_visit<Vis, clice::index::PathPool, Config> { | |
| static bool visit(Vis& vis, clice::index::PathPool& out) { | |
| std::vector<std::string> paths; | |
| if(!decode_value<Config>(vis, paths)) { | |
| return false; | |
| } | |
| out.paths.clear(); | |
| out.cache.clear(); | |
| for(const auto& path: paths) { | |
| if(path.empty()) { | |
| return false; | |
| } | |
| out.path_id(path); | |
| } | |
| return true; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index/serialization.h` around lines 146 - 162, The PathPool
deserialization in deserialize_visit<Vis, clice::index::PathPool, Config>::visit
is silently dropping empty strings, which changes later path_id() assignments
and can corrupt id-based references. Update the loop so an empty entry is
treated as a decode failure: detect it while rebuilding out.paths/cache and
return false instead of skipping it, keeping the PathPool state consistent with
the original serialized ordering.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmake/package.cmake (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning to a full commit SHA instead of an abbreviated one.
GIT_TAG 5882279is a short hash rather than a tag or full 40-character SHA. SinceGIT_SHALLOWisn't set for this fetch, CMake will do a full clone and resolve it locally, so this should work, but a full SHA is unambiguous and more resilient to future object-name collisions or tooling that expects exact SHAs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/package.cmake` at line 33, The fetch in package.cmake uses an abbreviated GIT_TAG, so update the Git tag reference in the relevant FetchContent or ExternalProject declaration to use the full 40-character commit SHA instead of 5882279. Keep the same target revision, but make it unambiguous and stable by replacing the short hash in the GIT_TAG field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmake/package.cmake`:
- Line 33: The fetch in package.cmake uses an abbreviated GIT_TAG, so update the
Git tag reference in the relevant FetchContent or ExternalProject declaration to
use the full 40-character commit SHA instead of 5882279. Keep the same target
revision, but make it unambiguous and stable by replacing the short hash in the
GIT_TAG field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16ff3203-30ee-46c2-ab05-a574cbe28b66
📒 Files selected for processing (1)
cmake/package.cmake
… ProjectIndex - The Builder now writes each occurrence straight into path_file_indices (or main_file_index for the interested file) instead of a FileID-keyed scratch map that build() drained afterwards. TUIndex::file_indices is gone, and a path included under several FileIDs now merges its contributions instead of keeping an arbitrary last one. Tests drop their fid-based helpers and iterate path_file_indices like production code does. - IncludeGraph::first_include_of names the "first include location of a path" scan the background indexer already did inline; tests reuse it. - ProjectIndex is its own serialization root: format_version / paths / shards become real (runtime-empty) fields, serialize encodes a persisted twin of the same type, and from() decodes into self before rebasing bitmaps onto the running pool. The intermediate PersistedProjectIndex type is gone. index_format_version moves to index/shared.h so the header no longer needs the codec.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index/serialization.h (2)
67-98: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
readSafefor Roaring deserialization
roaring::Roaring::read(..., false)can read past the end ofbyteson malformed input. Switch toreadSafe(..., bytes.size(), false)so deserialization stays bounded by the buffer length.🛡️ Proposed fix
if(bytes.empty()) { out = roaring::Roaring(); } else { - out = roaring::Roaring::read(reinterpret_cast<const char*>(bytes.data()), false); + out = roaring::Roaring::readSafe(reinterpret_cast<const char*>(bytes.data()), + bytes.size(), + false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index/serialization.h` around lines 67 - 98, Roaring deserialization in deserialize_visit::visit currently uses roaring::Roaring::read(..., false), which is not bounded by the decoded buffer and can overread on malformed input. Update this path to use readSafe with the bytes buffer and its size, while keeping the empty-bytes case unchanged, so the deserialization of roaring::Roaring stays safely constrained to the input vector.
24-65: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDecoded enum values aren't range-checked.
deserialize_visit<..., clice::RelationKind, ...>casts any decodedstd::uint32_tstraight intoRelationKind::Kindwithout checking it falls within the validInvalid..Calleerange (0-16). The same pattern applies to theSymbolKindspecialization further down (decoding an arbitrarystd::uint8_t). A corrupted or truncated blob (schema verification only checks structural shape, not semantic enum ranges) will silently produce an enum holding a value that matches none of the named cases, which can then flow throughis_one_of/switch-style logic elsewhere with no error surfaced. Given the PR's stated goal of not misreading corrupted blobs (and the siblingPathPooldeserializer already treats bad data as a hard failure), consider bounds-checking here too.🛡️ Proposed fix for RelationKind
static bool visit(Vis& vis, clice::RelationKind& out) { std::uint32_t value = 0; if(!decode_value<Config>(vis, value)) { return false; } + if(value > clice::RelationKind::Callee) { + return false; + } out = clice::RelationKind(static_cast<clice::RelationKind::Kind>(value)); return true; }Please confirm
SymbolKind's valid range and whether an out-of-range value could ever be used for indexing (e.g. inIndexQuery::to_lsp_symbol_kindor similar lookup tables), which would raise the severity from a data-quality issue to an OOB read.#!/bin/bash # Inspect SymbolKind's enum definition and any array/table indexing by its value. fd -HI 'symbol_kind' rg -n -C5 'enum.*SymbolKind' rg -n -C5 'SymbolKind::' --type=cpp -g '*.cpp' | rg -n 'value\(\)|static_cast'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index/serialization.h` around lines 24 - 65, The enum deserializers for clice::RelationKind and clice::SymbolKind currently accept any decoded integer and cast it directly into the enum, so add explicit range validation in deserialize_visit for both specializations before assigning to out. Use the existing valid enum bounds for RelationKind::Kind and confirm SymbolKind::Kind’s allowed range from its definition, then return false on out-of-range values instead of constructing the enum. Keep the change localized to the deserialize_visit helpers in serialization.h so corrupted blobs fail hard before invalid enum values reach switch logic or any table indexing.
🧹 Nitpick comments (1)
tests/unit/index/merged_index_tests.cpp (1)
27-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
file_index_ofusesoperator[], which can silently insert a default entry.If
fid's path id isn't actually present inpath_file_indices(e.g. a test setup mistake), this quietly default-constructs an emptyFileIndexinstead of failing with a clear error, making failures harder to diagnose.Suggested fix using explicit lookup
index::FileIndex& file_index_of(index::TUIndex& index, clang::FileID fid) { if(fid == unit->interested_file()) { return index.main_file_index; } - return index.path_file_indices[index.graph.path_id(fid)]; + auto it = index.path_file_indices.find(index.graph.path_id(fid)); + ASSERT_TRUE(it != index.path_file_indices.end()); + return it->second; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/index/merged_index_tests.cpp` around lines 27 - 34, The issue is that file_index_of uses operator[] on index.path_file_indices, which can silently create a default FileIndex when the path id is missing. Update file_index_of to use an explicit lookup on path_file_indices and fail fast when index.graph.path_id(fid) is not present, so test setup mistakes are reported clearly instead of being masked by an empty entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/index/serialization.h`:
- Around line 67-98: Roaring deserialization in deserialize_visit::visit
currently uses roaring::Roaring::read(..., false), which is not bounded by the
decoded buffer and can overread on malformed input. Update this path to use
readSafe with the bytes buffer and its size, while keeping the empty-bytes case
unchanged, so the deserialization of roaring::Roaring stays safely constrained
to the input vector.
- Around line 24-65: The enum deserializers for clice::RelationKind and
clice::SymbolKind currently accept any decoded integer and cast it directly into
the enum, so add explicit range validation in deserialize_visit for both
specializations before assigning to out. Use the existing valid enum bounds for
RelationKind::Kind and confirm SymbolKind::Kind’s allowed range from its
definition, then return false on out-of-range values instead of constructing the
enum. Keep the change localized to the deserialize_visit helpers in
serialization.h so corrupted blobs fail hard before invalid enum values reach
switch logic or any table indexing.
---
Nitpick comments:
In `@tests/unit/index/merged_index_tests.cpp`:
- Around line 27-34: The issue is that file_index_of uses operator[] on
index.path_file_indices, which can silently create a default FileIndex when the
path id is missing. Update file_index_of to use an explicit lookup on
path_file_indices and fail fast when index.graph.path_id(fid) is not present, so
test setup mistakes are reported clearly instead of being masked by an empty
entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9a5b104-9af3-4a03-976d-38d500456510
📒 Files selected for processing (10)
src/index/include_graph.hsrc/index/project_index.cppsrc/index/project_index.hsrc/index/serialization.hsrc/index/shared.hsrc/index/tu_index.cppsrc/index/tu_index.hsrc/server/index/background_indexer.cpptests/unit/index/index_query_tests.cpptests/unit/index/merged_index_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index/tu_index.h
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f30f8a8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if(canonical_id < index.canonical_ref_counts.size()) { | ||
| index.canonical_ref_counts[canonical_id] += 1; | ||
| } |
There was a problem hiding this comment.
When a structurally valid shard has a canonical_id greater than or equal to max_canonical_id, this silently ignores it while leaving the bad header/compilation context in the index. The next reindex or removal of that context calls release_canonical with the same id and indexes canonical_ref_counts[canonical_id] out of bounds, so a corrupted current-format cache can still crash the server despite passing load() verification; reject the shard or drop the invalid context instead of just skipping the count.
Useful? React with 👍 / 👎.
| std::span<const std::uint8_t> bytes(static_cast<const std::uint8_t*>(data), size); | ||
|
|
||
| ProjectIndex loaded; | ||
| if(auto result = kota::codec::fbs::from_flatbuffer(bytes, loaded); !result) { |
There was a problem hiding this comment.
For a ProjectIndex blob that still has the kotatsu FlatBuffer identifier but carries an old format_version, this decodes the entire object — including Roaring bitmaps via Roaring::read — before the version check below can return nullopt. A stale or corrupted cache with malformed bitmap bytes can therefore abort during startup instead of being discarded as intended; read format_version from a verified table view first, like MergedIndex::load, and only then run from_flatbuffer.
Useful? React with 👍 / 👎.
| /// Ordering by raw kind value; lets Relation default its own operator<=> so | ||
| /// serialized relation maps have a deterministic, binary-searchable order. | ||
| constexpr auto operator<=>(RelationKind lhs, RelationKind rhs) { | ||
| return lhs.value() <=> rhs.value(); |
There was a problem hiding this comment.
When this header is included without another standard header that happens to pull in <compare> first, this <=> expression fails to compile because std::strong_ordering is not declared. Since relation_kind.h is directly included from several public/internal headers, an include-order change or a standalone include can break the build; add the required <compare> include here.
Useful? React with 👍 / 👎.
…ed helpers - Codec adapters collapse to one value-mode serialize_visit specialization per type (kotatsu's to_wire/from_wire protocol) instead of paired serialize/deserialize specializations. - write_flatbuffer/read_flatbuffer helpers replace the encode/write and span/decode boilerplate repeated across the three index types. - Builder::finalize sorts and dedups relations/occurrences with their defaulted orderings instead of hand-written tuple lambdas, and the sorted-occurrence walk shared by FileIndex::lookup and MergedIndex's in-memory path is extracted into lookup_occurrences. - Bump kotatsu pin for the value-mode protocol.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/package.cmake (1)
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning to full commit SHA.
5ff9bf1is an abbreviated hash; a full 40-character SHA is more collision-resistant and easier to audit for supply-chain provenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/package.cmake` around lines 30 - 34, The kotatsu dependency declaration is using an abbreviated GIT_TAG, which should be replaced with the full 40-character commit SHA for stronger supply-chain pinning. Update the FetchContent_Declare block for kotatsu to use the full immutable commit identifier instead of the short hash so the dependency is easier to audit and less collision-prone.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index/serialization.h`:
- Around line 108-113: The from_wire helper in serialization.h is using
Roaring::read() on raw bytes without bounds checking. Update the from_wire
function to decode with Roaring::readSafe(..., bytes.size(), false) instead,
preserving the existing false native-format behavior while ensuring the buffer
is length-bounded.
---
Nitpick comments:
In `@cmake/package.cmake`:
- Around line 30-34: The kotatsu dependency declaration is using an abbreviated
GIT_TAG, which should be replaced with the full 40-character commit SHA for
stronger supply-chain pinning. Update the FetchContent_Declare block for kotatsu
to use the full immutable commit identifier instead of the short hash so the
dependency is easier to audit and less collision-prone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4531207-539d-43f8-96a6-2082450813c4
📒 Files selected for processing (6)
cmake/package.cmakesrc/index/merged_index.cppsrc/index/project_index.cppsrc/index/serialization.hsrc/index/tu_index.cppsrc/index/tu_index.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index/tu_index.cpp
- src/index/merged_index.cpp
| static roaring::Roaring from_wire(const wire_type& bytes) { | ||
| if(bytes.empty()) { | ||
| return roaring::Roaring(); | ||
| } | ||
| return roaring::Roaring::read(reinterpret_cast<const char*>(bytes.data()), false); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
roaring Roaring readSafe vs read non-portable bounds checking maxbytes
💡 Result:
In CRoaring, the distinction between "safe" and standard ("unsafe") deserialization functions centers on bounds checking and memory safety when processing serialized input [1][2]. Safe Deserialization (readSafe /..._safe variants) The functions ending in _safe (e.g., roaring_bitmap_portable_deserialize_safe and the C++ wrapper Roaring::readSafe) accept a maxbytes parameter [3][4]. They are guaranteed to not read beyond this provided buffer limit, preventing potential buffer overflows [1][5][4]. If the data is truncated or too small for a valid bitmap, the function safely returns NULL [1][4]. Standard/Unsafe Deserialization Standard deserialization functions (e.g., roaring_bitmap_portable_deserialize or the base Roaring::read) do not take a length argument [1][2]. They assume the input buffer points to a complete, correctly formatted serialized bitmap [1]. If the input is invalid, malformed, or shorter than the actual bitmap, these functions may perform out-of-bounds reads, potentially leading to crashes or security vulnerabilities [1][2]. Key Considerations: - Compatibility: The "safe" variants are designed to be compatible with the standard Roaring format specification used by Java and Go implementations [1][4]. - Untrusted Input: Because "safe" functions only guarantee memory safety (not logic safety), they do not verify that the data itself is a valid bitmap [5][4]. If an attacker provides "garbage" data that passes the length check but violates the format specification (e.g., incorrect container ordering), the resulting bitmap may be in an invalid state [5][4]. For untrusted input, it is recommended to call the internal validation functions (e.g., roaring_bitmap_internal_validate) after deserialization to ensure the bitmap is safe to use [5][2]. - Non-portable Deserialization: While the query mentions "non-portable" in the context of bounds checking, note that the library also provides "portable" vs "non-portable" (or "native") serialization formats. The "safe" deserialization functions specifically support the portable format [1][4][2]. Using non-portable (native) formats is generally not recommended for cross-platform data exchange or long-term storage [1].
Citations:
- 1: https://github.com/RoaringBitmap/CRoaring/blob/master/include/roaring/roaring.h
- 2: https://github.com/lemire/CRoaring
- 3: https://github.com/RoaringBitmap/CRoaring/blob/master/README.md
- 4: https://github.com/RoaringBitmap/CRoaring/blob/master/cpp/roaring/roaring.hh
- 5: https://docs.rs/croaring-sys/latest/croaring_sys/fn.roaring_bitmap_portable_deserialize_safe.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline src/index/serialization.h --view expanded || true
printf '\n== Relevant lines ==\n'
sed -n '90,130p' src/index/serialization.h
printf '\n== Search for Roaring API references ==\n'
rg -n "readSafe|Roaring::read\\(|roaring::Roaring" src include . -g '!**/node_modules/**' || true
printf '\n== Search for roaring headers in repo ==\n'
fd -a 'roaring*.h' . || true
fd -a 'roaring*.hh' . || trueRepository: clice-io/clice
Length of output: 1951
🌐 Web query:
CRoaring C++ Roaring::readSafe signature non-portable portable deserialize bounds checking
💡 Result:
In the CRoaring C++ library, Roaring::readSafe is the recommended method for deserializing bitmaps from potentially untrusted or external sources, as it provides critical memory safety guarantees by preventing buffer overflows [1][2]. The function signature for Roaring::readSafe is defined as: static Roaring readSafe(const char* buf, size_t maxbytes, bool portable = true); Key aspects of its functionality include: 1. Bounds Checking: Unlike the standard read method, readSafe enforces memory safety by ensuring the deserialization process does not read beyond the specified maxbytes [1][3][2]. This protects the application against buffer overflow vulnerabilities when processing external data [4][3]. 2. Portable vs. Non-portable: The portable parameter defaults to true [2]. When set to true, the library uses a format compatible with other Roaring Bitmap implementations (e.g., Java and Go) [5][3]. Setting it to false enables a custom, non-portable format that can be more space-efficient, especially for very sparse bitmaps, but it is not intended for cross-language interoperability [5]. 3. Safety vs. Validity: While readSafe prevents buffer overflows (memory safety), it does not automatically guarantee that the resulting bitmap is logically valid or consistent [5][3]. It assumes the input data was once serialized from a valid bitmap according to the Roaring format specification [5][3]. If the input contains garbage or is malformed, the bitmap may be initialized in an invalid state [5][3]. To fully validate that the deserialized bitmap is safe for use from untrusted sources, users should call roaring_bitmap_internal_validate on the resulting structure [4][3]. Additionally, the library recommends using checksums at the application level to ensure data integrity, as CRoaring does not perform internal checksumming [1][4]. 4. Endianness: The portable format is endian-sensitive [3]. On big-endian systems, the serialized data will also be big-endian, which may cause incompatibility if data is transferred between systems with different endianness [3].
Citations:
- 1: https://github.com/RoaringBitmap/CRoaring/blob/master/README.md
- 2: https://github.com/RoaringBitmap/CRoaring/blob/58b34013c8bc81b38ede0c3aae5b11a40336d4b8/README.md
- 3: https://docs.rs/croaring-sys/latest/croaring_sys/fn.roaring_bitmap_portable_deserialize_safe.html
- 4: safety guarantees about deserialize_safe & internal_validate functions RoaringBitmap/CRoaring#675
- 5: https://github.com/RoaringBitmap/CRoaring/blob/master/cpp/roaring/roaring.hh
Use readSafe(..., bytes.size(), false) here
Roaring::read() leaves the buffer unchecked; switch to readSafe() to bound the decode, but keep false so this native wire format still round-trips.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index/serialization.h` around lines 108 - 113, The from_wire helper in
serialization.h is using Roaring::read() on raw bytes without bounds checking.
Update the from_wire function to decode with Roaring::readSafe(...,
bytes.size(), false) instead, preserving the existing false native-format
behavior while ensuring the buffer is length-bounded.
|
Superseded — kotatsu reflection has been adopted via #598. |
1 participant
Summary
Replace the flatc-generated serialization of
TUIndex/ProjectIndex/MergedIndexwith kotatsu's reflection-driven FlatBuffers codec. The in-memory types are serialized directly — no IDL, no generated code, no hand-written builder/reader walks. Supersedes #430, redone on top of current main (multi-level symbol table, dep-hash staleness) and the reworked kotatsu visitor codec.kota::meta::skip<>(TUIndex::file_indices,IncludeGraph::file_table,Impl::canonical_ref_counts/occurrences_cache); derived state is rebuilt after decoding.index/serialization.hbridge the non-reflectable leaves:RelationKind(u32),SymbolKind(u8), roaring bitmaps (bytes, same non-portable format as before),std::chrono::milliseconds(i64), and the shardPathPool(string table, re-interned on decode). They apply anywhere the type appears — fields, map keys/values, elements.Occurrence/Relation/LocalSourceRangegain defaultedoperator<=>; the codec writes map entries sorted by key, andMergedIndex's buffer-only lookups (lookup/need_update/has_contribution/find_symbol/content/line_starts) binary-search that order through kotatsu'stable_viewproxy instead of flatc accessors.find_symbolupgrades from a linear scan to binary search for free.Impl::compact()before encoding (previously interleaved with the hand-written builder walk), so the written bytes and the runtime object stay equivalent;serialize()takes a mutable self.TUIndex::build()drains the FileID-keyed scratch map into the canonical path-id-keyedpath_file_indicesinstead of copying (the refactor(index): migrate FlatBuffers from flatc IDL to kotatsu reflection #430 approach doubled peak index memory in the worker). Tests access per-file indexes via path ids.MergedIndex::load, so corrupt or truncated blobs are rejected instead of read out of bounds.TUIndex::fromtakes an explicit size for the same reason.index_format_versionbumps to 2. flatc-era blobs are additionally rejected by the kotatsu buffer identifier, so stale caches rebuild in the background as designed.Drops
src/index/schema.fbsand the flatc codegen targetkota::codec::flatbuffers)Net -317 LoC.
Dependencies
kotatsu is pinned to
b4620e9(clice-io/kotatsu#178, the codec fixes this migration relies on: typed map-key ordering, llvm container entry protocols, deep verification, inline-struct criterion). Repoint to main once that PR merges.Test plan
unit_tests— 812 passed / 12 skipped, including MergedIndex round-trips, lazy lookups on serialized shards, stale-version and garbage shard rejection, ProjectIndex path remapping across sessions🤖 Generated with Claude Code
Summary by CodeRabbit