diff --git a/.please/docs/references/semble.md b/.please/docs/references/semble.md index 632238a..11376dd 100644 --- a/.please/docs/references/semble.md +++ b/.please/docs/references/semble.md @@ -288,6 +288,14 @@ Ported faithfully (`LazyLock` for the static patterns, `RefCell` (Total saved, efficiency bar, By Period; By Call Type gated behind `--verbose`). `clear_savings`. - **Divergence**: fixed `~/.csp/savings.jsonl` (not the OS cache dir); no `flock` (sub-4KB appends are atomic on POSIX); header is "Csp". +- **Divergence** (issue #90): `file_chars` sizes come from `indexing::file_sizes::FileSizes` — + a local source root is read lazily per returned result with a memo (misses memoized too), where + upstream `_compute_file_sizes` runs eagerly over every indexed file in `SembleIndex.__init__`. + Git sources still capture eagerly at clone time (the temp checkout is gone by search time). + Because the read now happens inside a live search, it is bounded by `MAX_FILE_BYTES` (the same + ceiling the indexer applies) — upstream, running at construction time, has no such bound. + Decoding matches upstream `read_file_text` (`errors="replace"`) and the csp indexer + (`String::from_utf8_lossy`), so a non-UTF-8 file that got indexed still gets sized. ### 4.16 MCP — `csp/src/mcp.rs` (core) + `csp/src/bin/csp/mcp_server.rs` (rmcp transport) diff --git a/crates/csp/src/bin/csp/main.rs b/crates/csp/src/bin/csp/main.rs index 1934708..9e9eed7 100644 --- a/crates/csp/src/bin/csp/main.rs +++ b/crates/csp/src/bin/csp/main.rs @@ -616,8 +616,11 @@ mod tests { fn search_output_records_savings_when_stats_file_given() { let dir = build_index_dir(); let idx = CspIndex::from_path(dir.path(), &LoadOptions::default()).unwrap(); - // file_sizes is captured at build time from the source tree. - assert!(!idx.file_sizes.is_empty()); + // The source tree is still on disk, so sizes are read lazily per result. + // Look the size up under the path the chunks actually carry — that is + // the key `save_search_stats` will use. + let indexed_path = idx.chunks[0].file_path.clone(); + assert!(idx.file_sizes.get(&indexed_path).is_some()); let stats = tempdir().unwrap(); let stats_file = stats.path().join("savings.jsonl"); @@ -626,8 +629,12 @@ mod tests { let content = std::fs::read_to_string(&stats_file).unwrap(); let lines: Vec<&str> = content.lines().filter(|l| !l.is_empty()).collect(); assert_eq!(lines.len(), 1); - assert!(lines[0].contains("\"call\":\"search\"")); - assert!(lines[0].contains("file_chars")); + let record: serde_json::Value = serde_json::from_str(lines[0]).unwrap(); + assert_eq!(record["call"], "search"); + // A nonzero value, not just the key: a lazy lookup that resolved nothing + // would still serialize `"file_chars":0`. + assert!(record["file_chars"].as_u64().unwrap() > 0); + assert!(record["snippet_chars"].as_u64().unwrap() > 0); } #[test] diff --git a/crates/csp/src/indexing/file_sizes.rs b/crates/csp/src/indexing/file_sizes.rs new file mode 100644 index 0000000..9b64433 --- /dev/null +++ b/crates/csp/src/indexing/file_sizes.rs @@ -0,0 +1,286 @@ +//! Per-file character counts feeding the `file_chars` side of token-savings +//! telemetry (`crate::stats`). +//! +//! Deliberate divergence from upstream semble, which recomputes every indexed +//! file's size eagerly in `SembleIndex.__init__`: a local source root is read +//! lazily, per result, with a memo — only the handful of files a query actually +//! returns is touched. Git sources still capture eagerly, at clone time. + +use std::collections::HashMap; +use std::io::Read as _; +use std::path::{Path, PathBuf}; +use std::sync::Mutex; + +use crate::indexing::create::MAX_FILE_BYTES; + +/// UTF-16 character counts per repo-relative file path, resolved eagerly +/// (captured) or lazily (read from a local root on demand). +#[derive(Debug, Default)] +pub struct FileSizes { + /// Sizes captured while the source tree was on disk (git clones: the temp + /// checkout is gone by search time). + captured: HashMap, + /// Local source root read on demand for paths not in `captured`. + lazy_root: Option, + /// Memo of lazily resolved sizes, negatives included — a path that cannot + /// be read must not re-pay the syscalls (and, for a file the indexer + /// accepted, a full read) on every later query. `Mutex` because `CspIndex` + /// is shared as `Arc` across MCP calls. + memo: Mutex>>, +} + +impl FileSizes { + /// No sizes available — telemetry records `file_chars` as 0. + pub fn empty() -> Self { + Self::default() + } + + /// Sizes already read off a source tree that is no longer available. + pub fn captured(sizes: HashMap) -> Self { + Self { + captured: sizes, + ..Self::default() + } + } + + /// Sizes read on demand from a still-present local source root. The root + /// is canonicalized once here so each lookup's containment check is a plain + /// prefix comparison; a root that cannot be canonicalized is kept as-is and + /// every lookup then fails containment, which is the safe outcome. + pub fn lazy(root: PathBuf) -> Self { + let root = root.canonicalize().unwrap_or(root); + Self { + lazy_root: Some(root), + ..Self::default() + } + } + + /// Character count for `file_path`: captured → memo → read from the lazy + /// root. `None` when unavailable or unreadable; both outcomes are memoized, + /// so an unreadable path costs one read attempt per index, not one per + /// query. The memo lock is released across the read so concurrent lookups + /// of different files don't serialize. + pub fn get(&self, file_path: &str) -> Option { + if let Some(size) = self.captured.get(file_path) { + return Some(*size); + } + let root = self.lazy_root.as_deref()?; + if let Some(size) = self.lock_memo().get(file_path) { + return *size; + } + let size = read_file_chars(root, file_path); + self.lock_memo().insert(file_path.to_string(), size); + size + } + + /// `true` when sizes can be produced at all. Prefer this over inspecting + /// the map: a lazy root reports available before anything has been read. + pub fn is_available(&self) -> bool { + !self.captured.is_empty() || self.lazy_root.is_some() + } + + fn lock_memo(&self) -> std::sync::MutexGuard<'_, HashMap>> { + self.memo.lock().unwrap_or_else(|e| e.into_inner()) + } +} + +/// UTF-16 character count of the repo-relative `file_path` under `root`, or +/// `None` when it cannot be read. `root` must already be canonical (see +/// [`FileSizes::lazy`]); the containment check below is a prefix comparison +/// against it. UTF-16 keeps it consistent with `stats::save_search_stats`'s +/// snippet accounting. +/// +/// Chunk paths are repo-relative by construction; a path that is absolute or +/// escapes `root` via `..` can only come from a tampered on-disk index, so it is +/// skipped rather than resolved (path traversal guard — a deliberate addition +/// over upstream, which joins the path unchecked). Only regular files are read: +/// the file walker never follows symlinks, and a path that has since become a +/// symlink, FIFO, or device must not be able to redirect or stall the read. +/// +/// The read is bounded by [`MAX_FILE_BYTES`], the same ceiling +/// `create_index_from_path` applies — lazily, this runs inside a live search, +/// so a file that has grown past the indexing limit since it was chunked must +/// not be slurped whole on the query path. Decoding is lossy, matching the +/// indexer (`String::from_utf8_lossy`) and upstream `read_file_text`'s +/// `errors="replace"`: a file with invalid UTF-8 still gets indexed, so it must +/// still be sized instead of silently contributing 0 `file_chars`. +pub(crate) fn read_file_chars(root: &Path, file_path: &str) -> Option { + let rel = Path::new(file_path); + if !is_safe_relative_path(rel) { + return None; + } + let full = root.join(rel); + // Reject a symlink at the leaf (the walker never indexes one) and, via + // canonicalization, a symlinked intermediate directory that would resolve + // the read outside `root`. + if std::fs::symlink_metadata(&full).ok()?.is_symlink() { + return None; + } + let canonical = full.canonicalize().ok()?; + if !canonical.starts_with(root) { + return None; + } + // Reject a non-regular file *before* opening it: `open(2)` on a FIFO + // blocks until a writer shows up, which would stall the search path, and + // opening a device node can have side effects. The fstat below re-checks + // the opened handle so the regular-file and size checks apply to what is + // actually read, and the read itself is capped. + if !std::fs::symlink_metadata(&canonical).ok()?.is_file() { + return None; + } + // Residual race: `canonicalize` and `File::open` are separate path walks, + // so a writer swapping a parent directory for a symlink in between can make + // the open follow it to a regular file outside `root`. Closing that needs a + // descriptor-relative no-follow walk (`openat` + `O_NOFOLLOW` per component), + // which `std` does not expose portably. The exposure is a UTF-16 length of + // that file written to the user's own `savings.jsonl`, never its content, by + // a local writer who already controls the indexed tree. + let file = std::fs::File::open(&canonical).ok()?; + let meta = file.metadata().ok()?; + if !meta.is_file() || meta.len() > MAX_FILE_BYTES { + return None; + } + let mut bytes = Vec::with_capacity(meta.len() as usize); + file.take(MAX_FILE_BYTES + 1).read_to_end(&mut bytes).ok()?; + if bytes.len() as u64 > MAX_FILE_BYTES { + return None; + } + Some(String::from_utf8_lossy(&bytes).encode_utf16().count() as u64) +} + +/// `true` when `path` is relative and contains no `..` or root component, so +/// joining it onto an index root cannot resolve outside that root. +fn is_safe_relative_path(path: &Path) -> bool { + use std::path::Component; + !path.is_absolute() + && !path.components().any(|c| { + matches!( + c, + Component::ParentDir | Component::RootDir | Component::Prefix(_) + ) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[test] + fn lazy_reads_and_memoizes_regular_files() { + let root = tempdir().unwrap(); + std::fs::write(root.path().join("a.ts"), "abcd").unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert!(sizes.is_available()); + assert_eq!(sizes.get("a.ts"), Some(4)); + // Memoized: the value survives the file going away. + std::fs::remove_file(root.path().join("a.ts")).unwrap(); + assert_eq!(sizes.get("a.ts"), Some(4)); + } + + #[test] + fn lazy_returns_none_for_unreadable_paths() { + let outer = tempdir().unwrap(); + let root = outer.path().join("repo"); + std::fs::create_dir(&root).unwrap(); + std::fs::write(outer.path().join("secret.txt"), "top secret").unwrap(); + std::fs::write(root.join("real.ts"), "abcd").unwrap(); + std::fs::create_dir(root.join("dir.ts")).unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(outer.path().join("secret.txt"), root.join("link.ts")).unwrap(); + let abs = root.join("real.ts").to_string_lossy().into_owned(); + let sizes = FileSizes::lazy(root.clone()); + + assert_eq!(sizes.get("../secret.txt"), None); + assert_eq!(sizes.get(&abs), None); + assert_eq!(sizes.get("missing.ts"), None); + assert_eq!(sizes.get("dir.ts"), None); + #[cfg(unix)] + assert_eq!(sizes.get("link.ts"), None); + } + + #[cfg(unix)] + #[test] + fn lazy_rejects_symlinked_intermediate_directory() { + let outer = tempdir().unwrap(); + let root = outer.path().join("repo"); + std::fs::create_dir(&root).unwrap(); + let outside = outer.path().join("outside"); + std::fs::create_dir(&outside).unwrap(); + std::fs::write(outside.join("leak.ts"), "top secret").unwrap(); + // `repo/vendor` -> `../outside`: the leaf is a regular file, but the + // path only reaches it through a symlinked directory. + std::os::unix::fs::symlink(&outside, root.join("vendor")).unwrap(); + + let sizes = FileSizes::lazy(root); + assert_eq!(sizes.get("vendor/leak.ts"), None); + } + + #[cfg(unix)] + #[test] + fn lazy_rejects_fifo_without_blocking() { + let root = tempdir().unwrap(); + let fifo = root.path().join("pipe.ts"); + let status = std::process::Command::new("mkfifo") + .arg(&fifo) + .status() + .unwrap(); + assert!(status.success()); + + // A FIFO with no writer would block `File::open` forever; the + // pre-open regular-file check must skip it instead. + let sizes = FileSizes::lazy(root.path().to_path_buf()); + assert_eq!(sizes.get("pipe.ts"), None); + } + + #[test] + fn lazy_memoizes_misses_so_they_are_read_once() { + let root = tempdir().unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert_eq!(sizes.get("later.ts"), None); + // The miss is cached: a file appearing afterwards does not resurrect it, + // which is what proves no second read was attempted. + std::fs::write(root.path().join("later.ts"), "abcd").unwrap(); + assert_eq!(sizes.get("later.ts"), None); + } + + #[test] + fn lazy_sizes_non_utf8_files_lossily_like_the_indexer() { + let root = tempdir().unwrap(); + // Latin-1 byte: `create_index_from_path` decodes it lossily and indexes + // the file, so sizing must not reject it. + std::fs::write(root.path().join("legacy.js"), b"ab\xffcd").unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert_eq!(sizes.get("legacy.js"), Some(5)); + } + + #[test] + fn lazy_skips_files_larger_than_the_indexing_ceiling() { + let root = tempdir().unwrap(); + let big = vec![b'a'; MAX_FILE_BYTES as usize + 1]; + std::fs::write(root.path().join("grown.ts"), &big).unwrap(); + let sizes = FileSizes::lazy(root.path().to_path_buf()); + + assert_eq!(sizes.get("grown.ts"), None); + } + + #[test] + fn captured_serves_known_paths_only() { + let sizes = FileSizes::captured([("a.ts".to_string(), 7u64)].into_iter().collect()); + + assert!(sizes.is_available()); + assert_eq!(sizes.get("a.ts"), Some(7)); + assert_eq!(sizes.get("b.ts"), None); + } + + #[test] + fn empty_is_not_available() { + let sizes = FileSizes::empty(); + + assert!(!sizes.is_available()); + assert_eq!(sizes.get("a.ts"), None); + } +} diff --git a/crates/csp/src/indexing/index.rs b/crates/csp/src/indexing/index.rs index c3fa7e1..1026d41 100644 --- a/crates/csp/src/indexing/index.rs +++ b/crates/csp/src/indexing/index.rs @@ -12,6 +12,7 @@ use sha2::{Digest, Sha256}; use crate::chunking::source::DESIRED_CHUNK_LENGTH_CHARS; use crate::indexing::create::{create_index_from_path, CreateIndexOptions}; use crate::indexing::dense::{load_model, make_stub_model, Model, SelectableBasicBackend}; +use crate::indexing::file_sizes::{read_file_chars, FileSizes}; use crate::indexing::sparse::Bm25Index; use crate::search::{search as run_search, SearchOptions as RunSearchOptions, SearchResult}; use crate::types::{chunk_from_dict, chunk_to_dict, Chunk, ChunkDict, ContentType, IndexStats}; @@ -80,11 +81,12 @@ pub struct CspIndex { pub model_path: String, pub root: Option, pub content: Vec, - /// Per-file character counts (repo-relative path → UTF-16 length) captured - /// at build time from the source tree, for token-savings telemetry. Empty - /// when the source files aren't available (e.g. a git index loaded from - /// cache). Derived metadata, not part of [`CspIndexState`]. - pub file_sizes: HashMap, + /// Per-file character counts (repo-relative path → UTF-16 length) for + /// token-savings telemetry: read lazily from a still-present local source + /// root, or captured at build time when the source won't outlive the build + /// (a git clone's temp checkout). Derived metadata, not part of + /// [`CspIndexState`]. + pub file_sizes: FileSizes, } pub(crate) fn normalize_content(content: Option>) -> Vec { @@ -101,7 +103,7 @@ impl CspIndex { model_path: state.model_path, root: state.root, content: state.content, - file_sizes: HashMap::new(), + file_sizes: FileSizes::empty(), } } @@ -126,24 +128,20 @@ impl CspIndex { }, )?; + // Absolute, like upstream's `path.resolve()`, so an index built from + // `.` still finds its source tree when loaded from another cwd. + let root = std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()); let mut index = Self::new(CspIndexState { model, bm25_index: result.bm25_index, semantic_index: result.semantic_index, chunks: result.chunks, model_path, - // Absolute, like upstream's `path.resolve()`, so an index built from - // `.` still finds its source tree when loaded from another cwd. - root: Some( - std::path::absolute(path) - .unwrap_or_else(|_| path.to_path_buf()) - .to_string_lossy() - .into_owned(), - ), + root: Some(root.to_string_lossy().into_owned()), content, }); - // Capture file sizes now, while the source tree is on disk. - index.file_sizes = compute_file_sizes(path, &index.chunks); + // The source tree stays on disk, so sizes are read lazily per result. + index.file_sizes = FileSizes::lazy(root); Ok(index) } @@ -165,9 +163,9 @@ impl CspIndex { clone_shallow(url, dir.path(), git_ref)?; let index = Self::from_path(dir.path(), options)?; - // `from_path` already captured file sizes from the checkout; carry them - // over since the temp dir is removed when `dir` drops. - let file_sizes = index.file_sizes.clone(); + // Capture file sizes from the checkout now: the temp dir is removed when + // `dir` drops, so they can't be read lazily at search time. + let file_sizes = FileSizes::captured(compute_file_sizes(dir.path(), &index.chunks)); // Re-root at the URL so a persisted manifest records a stable sourceId // (the temp checkout is removed when `dir` drops). let mut rerooted = Self::new(CspIndexState { @@ -378,13 +376,14 @@ impl CspIndex { root: manifest.source_id, content: manifest.content, }); - // Recompute file sizes from the source when it's a still-present local - // directory (mirrors semble reading sizes off `root` on load). A git URL - // or a moved source leaves this empty → `file_chars` is simply 0. + // Read file sizes lazily from the source when it's a still-present local + // directory — a deliberate divergence from upstream semble, which + // recomputes them eagerly in `SembleIndex.__init__`. A git URL or a moved + // source leaves this unavailable → `file_chars` is simply 0. if let Some(root) = index.root.as_deref() { let root_path = Path::new(root); if root_path.is_dir() { - index.file_sizes = compute_file_sizes(root_path, &index.chunks); + index.file_sizes = FileSizes::lazy(root_path.to_path_buf()); } } Ok(index) @@ -393,50 +392,21 @@ impl CspIndex { /// Per-file UTF-16 character counts for the unique files referenced by `chunks`, /// read from `root`. Mirrors semble `_compute_file_sizes` (unreadable files are -/// skipped). Feeds the `file_chars` side of token-savings telemetry; UTF-16 keeps -/// it consistent with `stats::save_search_stats`'s snippet accounting. -/// -/// Chunk paths are repo-relative by construction; a path that is absolute or -/// escapes `root` via `..` can only come from a tampered on-disk index, so it is -/// skipped rather than resolved (path traversal guard — a deliberate addition -/// over upstream, which joins the path unchecked). Only regular files are read: -/// the file walker never follows symlinks, and a path that has since become a -/// symlink, FIFO, or device must not be able to redirect or stall the read. +/// skipped). Used for sources that won't outlive the build (a git clone's temp +/// checkout); local paths read lazily via [`FileSizes::lazy`] instead. fn compute_file_sizes(root: &Path, chunks: &[Chunk]) -> HashMap { - let mut sizes: HashMap = HashMap::new(); - for chunk in chunks { - if sizes.contains_key(&chunk.file_path) { - continue; - } - let rel = Path::new(&chunk.file_path); - if !is_safe_relative_path(rel) { - continue; - } - let full = root.join(rel); - let is_regular_file = std::fs::symlink_metadata(&full) - .map(|m| m.is_file()) - .unwrap_or(false); - if !is_regular_file { - continue; - } - if let Ok(text) = std::fs::read_to_string(&full) { - sizes.insert(chunk.file_path.clone(), text.encode_utf16().count() as u64); - } - } - sizes -} - -/// `true` when `path` is relative and contains no `..` or root component, so -/// joining it onto an index root cannot resolve outside that root. -fn is_safe_relative_path(path: &Path) -> bool { - use std::path::Component; - !path.is_absolute() - && !path.components().any(|c| { - matches!( - c, - Component::ParentDir | Component::RootDir | Component::Prefix(_) - ) - }) + // Canonicalize once (`read_file_chars` needs a canonical root) and dedup + // paths first so an unreadable file is attempted once, not once per chunk. + let Ok(root) = root.canonicalize() else { + return HashMap::new(); + }; + chunks + .iter() + .map(|c| &c.file_path) + .collect::>() + .into_iter() + .filter_map(|path| read_file_chars(&root, path).map(|chars| (path.clone(), chars))) + .collect() } /// Shallow-clone `url` into `dir`, non-interactively. Rejects a ref starting diff --git a/crates/csp/src/indexing/index/tests.rs b/crates/csp/src/indexing/index/tests.rs index 96f9959..4649b7d 100644 --- a/crates/csp/src/indexing/index/tests.rs +++ b/crates/csp/src/indexing/index/tests.rs @@ -355,3 +355,33 @@ fn compute_file_sizes_skips_symlinks_and_non_regular_files() { assert!(!sizes.contains_key("link.ts")); assert!(!sizes.contains_key("dir.ts")); } + +#[test] +fn from_path_reads_file_sizes_lazily() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("sample.ts"), "export const x = 1\n").unwrap(); + + let idx = CspIndex::from_path(dir.path(), &LoadOptions::default()).unwrap(); + + assert!(idx.file_sizes.is_available()); + // Look it up under the path the chunks carry, so a chunk-path/root drift + // (absolute paths, a stray prefix) fails here instead of silently zeroing + // `file_chars` at telemetry time. + assert_eq!(idx.chunks[0].file_path, "sample.ts"); + assert_eq!(idx.file_sizes.get(&idx.chunks[0].file_path), Some(19)); +} + +#[test] +fn load_from_disk_has_no_file_sizes_when_source_is_gone() { + let source = tempdir().unwrap(); + std::fs::write(source.path().join("sample.ts"), "export const x = 1\n").unwrap(); + let idx = CspIndex::from_path(source.path(), &LoadOptions::default()).unwrap(); + let cache = tempdir().unwrap(); + idx.save(cache.path(), None).unwrap(); + std::fs::remove_dir_all(source.path()).unwrap(); + + let loaded = CspIndex::load_from_disk(cache.path()).unwrap(); + + assert!(!loaded.file_sizes.is_available()); + assert_eq!(loaded.file_sizes.get("sample.ts"), None); +} diff --git a/crates/csp/src/indexing/mod.rs b/crates/csp/src/indexing/mod.rs index 771e6a4..1fd1664 100644 --- a/crates/csp/src/indexing/mod.rs +++ b/crates/csp/src/indexing/mod.rs @@ -8,6 +8,7 @@ pub mod cache; mod cache_orchestrator; pub mod create; pub mod dense; +pub mod file_sizes; pub mod file_walker; pub mod files; pub mod index; diff --git a/crates/csp/src/stats.rs b/crates/csp/src/stats.rs index 89d56ec..81db3ed 100644 --- a/crates/csp/src/stats.rs +++ b/crates/csp/src/stats.rs @@ -7,13 +7,14 @@ //! Time bucketing uses UTC `YYYY-MM-DD` (compared lexicographically, which is //! chronological); `now_secs` is injected so summaries/reports are testable. -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashSet}; use std::io::{IsTerminal, Write as _}; use std::path::{Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; +use crate::indexing::file_sizes::FileSizes; use crate::search::SearchResult; use crate::types::CallType; @@ -110,22 +111,19 @@ pub fn save_search_stats( stats_file: &Path, results: &[SearchResult], call_type: CallType, - file_sizes: &HashMap, + file_sizes: &FileSizes, max_snippet_lines: Option, ) { let snippet_chars: u64 = results .iter() .map(|r| delivered_chars(&r.chunk.content, max_snippet_lines)) .sum(); - let mut unique_paths: Vec<&str> = Vec::new(); - for r in results { - if !unique_paths.contains(&r.chunk.file_path.as_str()) { - unique_paths.push(r.chunk.file_path.as_str()); - } - } - let file_chars: u64 = unique_paths + // Each source file is counted once, however many of its chunks ranked. + let mut seen: HashSet<&str> = HashSet::new(); + let file_chars: u64 = results .iter() - .filter_map(|p| file_sizes.get(*p).copied()) + .filter(|r| seen.insert(r.chunk.file_path.as_str())) + .filter_map(|r| file_sizes.get(&r.chunk.file_path)) .sum(); let record = StatsRecord { @@ -469,8 +467,8 @@ mod tests { } } - fn sizes(pairs: &[(&str, u64)]) -> HashMap { - pairs.iter().map(|(p, s)| ((*p).to_string(), *s)).collect() + fn sizes(pairs: &[(&str, u64)]) -> FileSizes { + FileSizes::captured(pairs.iter().map(|(p, s)| ((*p).to_string(), *s)).collect()) } #[test] @@ -516,6 +514,32 @@ mod tests { assert_eq!(record.file_chars, 300); } + #[test] + fn save_reads_sizes_through_a_lazy_root() { + // The variant every local `from_path` / `load_from_disk` index uses: + // `save_search_stats` must resolve repo-relative result paths against + // the source root, not just a pre-seeded map. + let root = tempdir().unwrap(); + std::fs::write(root.path().join("a.ts"), "0123456789").unwrap(); + let dir = tempdir().unwrap(); + let file = dir.path().join("savings.jsonl"); + let results = vec![result("hello", "a.ts"), result("world", "gone.ts")]; + + save_search_stats( + &file, + &results, + CallType::Search, + &FileSizes::lazy(root.path().to_path_buf()), + None, + ); + + let content = std::fs::read_to_string(&file).unwrap(); + let record: StatsRecord = serde_json::from_str(content.lines().next().unwrap()).unwrap(); + // `a.ts` resolves off the root; the missing path simply contributes 0. + assert_eq!(record.file_chars, 10); + assert_eq!(record.snippet_chars, 10); + } + #[test] fn save_caps_snippet_chars_by_max_snippet_lines() { let dir = tempdir().unwrap();