feat: Wave 1 — Lexical search with BM25F scoring and n-gram index

[leesharon]

Summary

Full lexical search layer for skim — BM25F scoring with n-gram indexing over AST-classified code fields.

• N-gram extraction with border weighting for better boundary-aware matching
• BM25F scoring with field-weighted classifiers (type definitions, function signatures, symbol names, etc.)
• Index builder with tree-sitter AST field classification + serde-based classification for JSON/YAML/TOML/Markdown
• Binary index format (.skidx + .skpost) with memory-mapped reader, delta segments, and tombstone tracking
• CLI integration — skim search with auto-build, --rebuild, --stats, snippet display, JSON output
• Performance — O(1) delta scans via FxHashMap, two-pass streaming writer (no in-memory buffer), reusable query buffers
• Hardened — size guards on all mmap'd files, checked arithmetic, capacity caps against crafted indexes

Architecture

rskim-search/src/lexical/
├── ngram.rs          — N-gram extraction + border weighting
├── scoring.rs        — BM25F scorer with field boosts
├── builder.rs        — Index builder (AST + serde field classification)
├── walker.rs         — AST cursor walker for field classification
├── query.rs          — SearchLayer impl — query engine
└── index_format/
    ├── writer.rs     — Two-pass atomic write (.skidx + .skpost)
    ├── reader.rs     — Mmap'd binary search reader
    ├── delta.rs      — Append-only delta segment
    ├── tombstones.rs — Deleted doc_id tracking
    └── entry.rs      — Binary IndexEntry format

Key metrics

• 55 files changed, ~10,580 lines added
• 319 tests (unit + integration + acceptance + robustness + adversarial)
• 0 tech debt items remaining (PF-001 through PF-009 all resolved)
• All clippy warnings clean

Test plan

• cargo test -p rskim-search — 248 tests pass (unit + integration)
• cargo test -p rskim — 1001 tests pass (1 flaky perf test excluded — process spawn overhead, unrelated)
• cargo clippy -p rskim-search -p rskim -- -D warnings — clean
• Index write/read roundtrip tests (empty, single, multiple, large posting lists)
• Delta writer/reader roundtrip + scan filtering
• Tombstone persistence and binary search
• Full search pipeline via SearchLayer::search (BM25F scoring, delta merge, tombstone exclusion)
• Concurrent search safety (Arc + thread::scope)
• Corrupted index detection (bad magic, wrong version, truncated files, out-of-bounds postings)
• Adversarial inputs (null bytes, very long queries, unicode, whitespace-only)

[leesharon]

🔴 BLOCKING: Resource Exhaustion via Uncapped Vec::with_capacity

The read_postings method calls Vec::with_capacity(count) where count comes from untrusted on-disk data. A crafted index file with posting_length = u32::MAX would attempt to allocate ~51GB, causing OOM or panic.

Fix: Cap the capacity hint before allocation:

let max_possible = self.post_mmap.len().saturating_sub(start) / POSTING_ENTRY_SIZE;
let cap = count.min(max_possible);
let mut result = Vec::with_capacity(cap);

This is a straightforward bounds check (3 lines) that prevents DoS via crafted index files.

[leesharon]

🔴 BLOCKING: Unchecked Truncation of node.start_byte() to u32

node.start_byte() as u32 silently truncates on files near 5MB (the file size limit), potentially corrupting snippet positions. While unlikely given the 5MB cap, this violates Rust safety discipline used elsewhere (see line 179 which uses try_from).

Fix: Use try_from matching the existing pattern:

position: u32::try_from(node.start_byte()).unwrap_or(u32::MAX),

The same issue exists at line 146 (range.start as u32). Both should be fixed together for consistency.

[leesharon]

🔴 BLOCKING: CLI Depends on Concrete Type, Not Trait

output.rs imports and depends on LexicalSearchLayer directly (lines 8, 16, 79, 97, 107), violating Dependency Inversion Principle. This makes it impossible to add Wave 2 temporal or AST layers without rewriting all output functions.

Fix: Change function signatures to accept &dyn SearchIndex:

pub(super) fn print_text_results(
    index: &dyn SearchIndex,  // Not &LexicalSearchLayer
    results: &[(FileId, f32)],
    query_text: &str,
    repo_root: &Path,
) -> anyhow::Result<()> {
    // Use index.file_table() instead of layer.file_table()
}

All callsites in search/mod.rs:135 already have LexicalSearchLayer which implements SearchIndex, so this is backward-compatible. The trait infrastructure is ready — just decouple the output layer.

[leesharon]

🔴 BLOCKING: Inconsistent Error Variant Naming

SearchError has both IndexError(String) (line 14) and CorruptedIndex { path, reason } (lines 29-36). Both describe index-related failures but with inconsistent shapes:

• Builder (line 676): IndexError("file_id exceeds u32::MAX")
• Reader (line 92): CorruptedIndex { path, reason }

This creates confusion: which variant should be used? The structured variant with path context is more debuggable.

Fix: Consolidate into a single variant. Either:

1. Rename IndexError → IndexBuildError to clarify it's build-time only
2. Migrate all IndexError call sites to CorruptedIndex with structured fields

The structured version is better for error handling and debugging.

[leesharon]

Code Review Summary

Review Status: ⛔️ CHANGES_REQUESTED (4 blocking findings)

High-Confidence Blocking Issues (≥80% confidence)

Created inline comments for these critical findings:

1. Security: Uncapped Vec::with_capacity in read_postings - Resource exhaustion via crafted index (90% confidence) — reader.rs:248
2. Rust: Unchecked as u32 truncation of node.start_byte() - Position corruption on large files (90% confidence) — builder.rs:335
3. Architecture: CLI depends on concrete LexicalSearchLayer, not trait - Breaks Wave 2 extensibility (90% confidence) — output.rs:79
4. Consistency: IndexError vs CorruptedIndex overlap - Confusing error handling (85% confidence) — error.rs:29

Lower-Confidence Suggestions (60-79%)

These are important but less urgent than the 4 blocking issues above:

Security (80% confidence, medium priority)

• Missing size limit on delta file (200MB cap recommended) — delta.rs:100
• TOCTOU race in metadata size guard (read-first pattern) — query.rs:70
• Missing size limit on .skpost file (4GB cap recommended) — reader.rs:87
• --limit flag accepts zero and usize::MAX — Clamp to 1-10,000 — search/mod.rs:38

Performance (90% confidence, HIGH priority)

• Delta linear scan O(n) per query — Use binary search or in-memory hash index — delta.rs:111
• Vec allocation per posting lookup (32 allocations per query) — Return iterator or reuse buffer — reader.rs:242
• FxHashMap grows unbounded in builder — Pre-allocate with estimate — builder.rs:44
• Redundant n-gram extraction for nested nodes — Track classified byte ranges — builder.rs:282

Rust (82-90% confidence)

• Unchecked as u32 of range.start — Use try_from — builder.rs:146
• Unchecked as u32 of ngrams.len() — Use try_from — builder.rs:151, 219, 340
• IndexStats::clone() in accessor — Return reference instead — query.rs:216
• Missing #[must_use] on Result-returning functions — Add to all public fallible functions

Complexity (92% confidence, MEDIUM priority)

• serde_fields.rs exceeds 400 lines (477) — Extract tests to existing integration file
• builder.rs exceeds 400 lines (426) — Extract walker to lexical/walker.rs
• walk_and_classify_inner has 7 parameters — Encapsulate mutable state in struct
• Moderate cyclomatic complexity in search/mod.rs:run() — Extract unimplemented flag checks

Consistency (85-92% confidence)

• Inconsistent #[must_use] annotations — Standardize on reason strings
• Inconsistent test naming (test_ prefix vs descriptive) — Pick one convention
• Inconsistent section headers (// Unit Tests vs // Unit tests) — Standardize on capital T

Test Coverage (85-90% confidence)

• No test for CorruptedIndex error variant
• No test for IndexReader::validate() method
• No test for MAX_METADATA_BYTES guard
• No test for unimplemented CLI flags
• Scoring edge cases (doc_count == 0, df > doc_count) not tested
• Tombstone + search integration not end-to-end tested
• Delta reader + search integration not end-to-end tested

Deduplication

Multiple reviewers flagged identical issues — consolidated into single inline comments:

• as u32 truncation (security + rust reviewers)
• IndexError vs CorruptedIndex (consistency + rust reviewers)
• LexicalSearchLayer field visibility (architecture + rust reviewers)

Summary by Category

Finding	Blocking	Should-Fix	Lower-Confidence
Security	2	0	4
Architecture	1	2	1
Performance	2	2	0
Rust	1	4	4
Complexity	3	1	0
Consistency	2	2	3
Tests	3	2	0
Regression	0	0	1
Total	14	13	13

Recommendation

Do not merge until the 4 inline-comment blocking issues are resolved. These are straightforward fixes (3-10 lines each) that prevent resource exhaustion, position corruption, and Wave 2 extensibility issues. The lower-confidence findings are important for code quality but can be tracked as tech debt if the blocking issues are fixed.

See inline comments for specific fixes.