fix(fingerprint): address review findings on corpus mode

Four issues raised by CodeRabbit on #70, three of them real defects:

1. O(n^2) building the shape -> groups map. `Vec::contains` was a linear
   membership scan repeated per indexed entry, and it degraded exactly on
   the path this feature exists for — one shape carried by many groups.
   CLAUDE.md forbids O(n^2) algorithms outright. Now a BTreeSet, which also
   removes the separate sort since it iterates in order. Verified to produce
   byte-identical output to the previous implementation on a 955-document
   corpus, so this is purely a complexity fix.

2. An unreadable subdirectory aborted the whole scan. `read_dir(...)?`
   propagated out of collect_corpus_files, so one permission-denied
   directory failed the entire command after thousands of files had already
   been walked — inconsistent with the per-file error model build_index
   already used. Directory and entry failures are now collected into the
   same `errors` list as parse failures, and the walk continues.

3. Entry-level failures were silently dropped. `let Ok(entry) = ... else
   continue` discarded unreadable entries with no counter and no
   diagnostic, so files_scanned could undercount without saying so. Each
   now produces an error entry.

4. No coverage for --format text in corpus mode, and the text output
   labelled cluster size as "document(s)" when `size` counts groups. The
   label directly contradicted the design rationale — a cluster of 3
   packages can span dozens of files. Corrected to "group(s)" and pinned by
   a test that also asserts the old wording is absent.

collect_corpus_files now returns a `Walk` struct rather than a tuple, since
it carries three things.

Tests: +2 integration tests (text output reports counts and labels groups;
an unreadable directory is reported and non-fatal, unix-gated).

Author: copyleftdev

vajra-cli/src/corpus.rs

@@ -13,7 +13,7 @@
 //! - **clusters**: documents linked transitively through *any* shared shape,
 //!   because related documents typically share several files rather than one.
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, BTreeSet};
 use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result};
@@ -120,45 +120,85 @@ pub struct CorpusError {
     pub error: String,
 }
 
+/// What a corpus walk found.
+pub struct Walk {
+    /// Files the selector accepted, sorted.
+    pub selected: Vec<PathBuf>,
+    /// Every file encountered, whether selected or not.
+    pub scanned: usize,
+    /// Directories and entries that could not be read — reported rather than
+    /// dropped silently or made fatal.
+    pub errors: Vec<CorpusError>,
+}
+
 /// Recursively collect files under `dir`, partitioned by `accept`.
 ///
 /// Unlike `batch` and `cluster`, this walk **recurses**: a corpus is normally a
 /// tree of extracted packages or checkouts, so the interesting files are nested.
 ///
+/// An unreadable subdirectory or entry is recorded in [`Walk::errors`] and the
+/// walk continues. Aborting a scan of thousands of files because one directory
+/// denied permission is the wrong trade, and would be inconsistent with the
+/// per-file error model `build_index` already uses.
+///
 /// # Errors
 ///
-/// Returns an error if `dir` is not a directory or cannot be read.
-pub fn collect_corpus_files(
-    dir: &Path,
-    accept: &dyn Fn(&Path) -> bool,
-) -> Result<(Vec<PathBuf>, usize)> {
+/// Returns an error only if `dir` itself is not a directory.
+pub fn collect_corpus_files(dir: &Path, accept: &dyn Fn(&Path) -> bool) -> Result<Walk> {
     if !dir.is_dir() {
         anyhow::bail!("{} is not a directory", dir.display());
     }
-    let mut selected = Vec::new();
-    let mut scanned = 0usize;
+    let mut walk = Walk {
+        selected: Vec::new(),
+        scanned: 0,
+        errors: Vec::new(),
+    };
     let mut stack = vec![dir.to_path_buf()];
 
     while let Some(current) = stack.pop() {
-        let entries = std::fs::read_dir(&current)
-            .with_context(|| format!("failed to read directory {}", current.display()))?;
+        let entries = match std::fs::read_dir(&current) {
+            Ok(entries) => entries,
+            Err(e) => {
+                walk.errors.push(CorpusError {
+                    file: current.display().to_string(),
+                    error: format!("failed to read directory: {e}"),
+                });
+                continue;
+            }
+        };
         let mut dirs = Vec::new();
         for entry in entries {
-            let Ok(entry) = entry else { continue };
+            let entry = match entry {
+                Ok(entry) => entry,
+                Err(e) => {
+                    walk.errors.push(CorpusError {
+                        file: current.display().to_string(),
+                        error: format!("failed to read directory entry: {e}"),
+                    });
+                    continue;
+                }
+            };
             let path = entry.path();
             // Do not follow symlinks: a cycle would hang the walk.
-            let Ok(meta) = entry.file_type() else {
-                continue;
+            let meta = match entry.file_type() {
+                Ok(meta) => meta,
+                Err(e) => {
+                    walk.errors.push(CorpusError {
+                        file: path.display().to_string(),
+                        error: format!("failed to stat: {e}"),
+                    });
+                    continue;
+                }
             };
             if meta.is_symlink() {
                 continue;
             }
             if meta.is_dir() {
                 dirs.push(path);
             } else if meta.is_file() {
-                scanned += 1;
+                walk.scanned += 1;
                 if accept(&path) {
-                    selected.push(path);
+                    walk.selected.push(path);
                 }
             }
         }
@@ -168,8 +208,9 @@ pub fn collect_corpus_files(
         stack.extend(dirs);
     }
 
-    selected.sort();
-    Ok((selected, scanned))
+    walk.selected.sort();
+    walk.errors.sort_by(|a, b| a.file.cmp(&b.file));
+    Ok(walk)
 }
 
 /// Build the shape-reuse index over `files`.
@@ -181,13 +222,13 @@ pub fn collect_corpus_files(
 /// indexing it.
 pub fn build_index(
     root: &Path,
-    files: &[PathBuf],
-    scanned: usize,
+    walk: &Walk,
     min_nodes: u64,
     group_depth: usize,
     load: &(dyn Fn(&Path) -> Result<Document> + Send + Sync),
     shape_of: &(dyn Fn(&Document) -> Result<String> + Send + Sync),
 ) -> CorpusIndex {
+    let files = &walk.selected;
     let outcomes: Vec<(PathBuf, Result<Option<Indexed>>)> = files
         .par_iter()
         .map(|path| {
@@ -211,7 +252,16 @@ pub fn build_index(
         .collect();
 
     let mut indexed = Vec::new();
-    let mut errors = Vec::new();
+    // Walk-level failures (unreadable directories) belong in the same list as
+    // parse failures: both are files the caller asked about and did not get.
+    let mut errors: Vec<CorpusError> = walk
+        .errors
+        .iter()
+        .map(|e| CorpusError {
+            file: e.file.clone(),
+            error: e.error.clone(),
+        })
+        .collect();
     let mut suppressed = 0usize;
     for (path, outcome) in outcomes {
         match outcome {
@@ -253,21 +303,24 @@ pub fn build_index(
 
     // Clustering links *groups*, not files: shape -> the distinct groups
     // carrying it. A shape confined to one group links nothing.
-    let mut shape_to_groups: BTreeMap<&str, Vec<&str>> = BTreeMap::new();
+    //
+    // A BTreeSet rather than Vec + `contains`: the whole point of this feature
+    // is shapes carried by many groups, so a linear membership scan per entry
+    // would degrade toward O(n^2) exactly on the path that matters.
+    let mut shape_to_groups: BTreeMap<&str, BTreeSet<&str>> = BTreeMap::new();
     for entry in &indexed {
-        let slot = shape_to_groups.entry(&entry.shape).or_default();
-        if !slot.contains(&entry.group.as_str()) {
-            slot.push(&entry.group);
-        }
+        shape_to_groups
+            .entry(&entry.shape)
+            .or_default()
+            .insert(&entry.group);
     }
     let cross_group: Vec<(&str, Vec<&str>, u64)> = shape_to_groups
         .iter()
         .filter(|(_, groups)| groups.len() > 1)
         .map(|(shape, groups)| {
             let nodes = by_shape.get(*shape).map_or(0, |(n, _)| *n);
-            let mut g = groups.clone();
-            g.sort_unstable();
-            (*shape, g, nodes)
+            // BTreeSet iterates in sorted order already.
+            (*shape, groups.iter().copied().collect(), nodes)
         })
         .collect();
 
@@ -280,10 +333,10 @@ pub fn build_index(
     let clusters = build_clusters(&cross_group);
 
     CorpusIndex {
-        files_scanned: scanned,
+        files_scanned: walk.scanned,
         documents_indexed: indexed.len(),
         groups_indexed,
-        skipped: scanned.saturating_sub(files.len()),
+        skipped: walk.scanned.saturating_sub(files.len()),
         suppressed,
         distinct_shapes,
         shapes_in_multiple_documents,
@@ -473,10 +526,11 @@ mod tests {
         std::fs::write(dir.path().join("readme.txt"), "x")?;
 
         let json_only = |p: &Path| p.extension().is_some_and(|e| e == "json");
-        let (files, scanned) = collect_corpus_files(dir.path(), &json_only)?;
-        assert_eq!(files.len(), 3, "walk must recurse");
-        assert_eq!(scanned, 4, "scanned counts every file seen");
-        assert!(files.windows(2).all(|w| w[0] <= w[1]), "sorted");
+        let walk = collect_corpus_files(dir.path(), &json_only)?;
+        assert_eq!(walk.selected.len(), 3, "walk must recurse");
+        assert_eq!(walk.scanned, 4, "scanned counts every file seen");
+        assert!(walk.errors.is_empty());
+        assert!(walk.selected.windows(2).all(|w| w[0] <= w[1]), "sorted");
         Ok(())
     }
 

vajra-cli/src/main.rs

@@ -1651,23 +1651,27 @@ fn cmd_fingerprint_corpus(
     cli: &Cli,
 ) -> Result<()> {
     let dir = Path::new(directory);
-    let (files, scanned) = corpus::collect_corpus_files(dir, &|p| is_selectable_file(p, cli))?;
+    let walk = corpus::collect_corpus_files(dir, &|p| is_selectable_file(p, cli))?;
 
-    if files.is_empty() {
+    if walk.selected.is_empty() {
         anyhow::bail!(
-            "no analysable files found under {directory} ({scanned} file(s) scanned). \
-             Use --input-format source to index source files."
+            "no analysable files found under {directory} ({} file(s) scanned). \
+             Use --input-format source to index source files.",
+            walk.scanned
         );
     }
 
     if !cli.quiet {
-        eprintln!("Indexing {} of {scanned} file(s)...", files.len());
+        eprintln!(
+            "Indexing {} of {} file(s)...",
+            walk.selected.len(),
+            walk.scanned
+        );
     }
 
     let index = corpus::build_index(
         dir,
-        &files,
-        scanned,
+        &walk,
         min_nodes,
         group_depth,
         &|p| load_document_path(p, cli),
@@ -1721,7 +1725,7 @@ fn cmd_fingerprint_corpus(
             } else {
                 for c in &index.clusters {
                     println!(
-                        "  {} document(s), {} shared shape(s), min nodes {}",
+                        "  {} group(s), {} shared shape(s), min nodes {}",
                         c.size, c.shared_shapes, c.min_node_count
                     );
                     for m in &c.members {

vajra-cli/tests/fingerprint_corpus.rs

@@ -296,6 +296,85 @@ fn index_is_deterministic() -> Result<()> {
     Ok(())
 }
 
+/// Text mode must state the counts and label clusters as *groups*, not
+/// documents — `size` counts clustering units, and a cluster of 3 packages can
+/// span dozens of files.
+#[test]
+fn text_output_reports_counts_and_labels_groups() -> Result<()> {
+    let dir = three_package_corpus()?;
+    let out = Command::new(vajra_bin())
+        .arg("fingerprint")
+        .arg(as_str(dir.path())?)
+        .arg("--corpus")
+        .args(JS)
+        .arg("--quiet")
+        .output()?;
+
+    assert!(
+        out.status.success(),
+        "text mode failed: {}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+    let stdout = String::from_utf8_lossy(&out.stdout);
+    for expected in [
+        "Corpus Shape Index",
+        "Files scanned:",
+        "Documents indexed:",
+        "Distinct shapes:",
+        "Reuse Groups",
+        "Clusters",
+    ] {
+        assert!(stdout.contains(expected), "missing {expected:?}:\n{stdout}");
+    }
+    assert!(
+        stdout.contains("group(s)"),
+        "clusters must be labelled as groups, not documents:\n{stdout}"
+    );
+    assert!(
+        !stdout.contains("document(s), "),
+        "cluster size must not be called documents:\n{stdout}"
+    );
+    Ok(())
+}
+
+/// An unreadable subdirectory must be reported and the walk must continue —
+/// failing a scan of thousands of files because one directory denied permission
+/// would be the wrong trade.
+#[cfg(unix)]
+#[test]
+fn unreadable_directory_is_reported_not_fatal() -> Result<()> {
+    use std::os::unix::fs::PermissionsExt;
+
+    let dir = TempDir::new()?;
+    write(dir.path(), "pkg-a/lib/index.js", TEMPLATE_A)?;
+    write(dir.path(), "pkg-b/lib/index.js", TEMPLATE_B)?;
+    let locked = dir.path().join("locked");
+    std::fs::create_dir(&locked)?;
+    write(dir.path(), "locked/hidden.js", OTHER)?;
+    std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000))?;
+
+    let result = corpus(dir.path(), JS);
+
+    // Restore permissions before any assertion so the TempDir can clean up.
+    let _ = std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o755));
+    let json = result?;
+
+    assert_eq!(
+        json["documents_indexed"], 2,
+        "readable packages still indexed"
+    );
+    let errors = json["errors"]
+        .as_array()
+        .ok_or_else(|| anyhow!("errors missing"))?;
+    assert!(
+        errors
+            .iter()
+            .any(|e| e["file"].as_str().is_some_and(|f| f.contains("locked"))),
+        "the unreadable directory must be reported: {errors:?}"
+    );
+    Ok(())
+}
+
 #[test]
 fn parse_failures_are_reported_not_dropped() -> Result<()> {
     let dir = TempDir::new()?;