fix: address critical issues from code review

This commit addresses the critical and high-priority issues identified
in the comprehensive code review.

## Critical Fixes

### 1. Remove panics from library code (types.rs)
- Changed `codeowners_entry_to_matcher()` to return `Result<CodeownersEntryMatcher, PatternError>`
- Added new `PatternError` type with proper error message formatting
- Updated all callers to handle the Result properly

### 2. Fix unwrap() calls in cache.rs
- Replaced `unwrap()` with proper error handling using `match` and `filter_map`
- Added graceful fallback for files that fail ownership resolution
- Changed stdout flush to use `let _ =` to ignore errors

### 3. Fix broken hash calculation (common.rs)
- Added exclusion patterns for `.codeowners.cache` files
- Implemented double-check in diff callback to skip cache files
- Fixed the `and_then` closure returning wrong type

### 4. Add CLI integration tests
- Added 22 comprehensive integration tests covering:
  - Basic commands (help, version, config)
  - All codeowners subcommands (parse, list-files, list-owners, etc.)
  - Shell completion generation (bash, zsh, fish)
  - Various CLI flags and options
  - Error handling for invalid inputs

## High Priority Fixes

### 5. Improve error Display to include source context
- Modified `Display` impl to include source error message in output

### 6. Remove empty start() function (core/mod.rs)
- Removed unused `start()` function that did nothing

### 7. Add recursion depth limit to find_codeowners_files
- Added MAX_RECURSION_DEPTH constant (100)
- Implemented depth tracking to prevent stack overflow on deep directory structures

## Files Changed
- types.rs: Return Result from codeowners_entry_to_matcher, add PatternError
- cache.rs: Graceful error handling, filter invalid patterns with warnings
- infer_owners.rs: Handle Result from matcher conversion
- common.rs: Fix hash calculation, add recursion limit
- error.rs: Include source in Display
- mod.rs: Remove empty function
- resolver_bench.rs: Handle Result in benchmarks
- test_cli.rs: Add 22 integration tests

Author: claude

ci/tests/test_cli.rs

@@ -27,7 +27,7 @@ fn create_test_codeowners_entry_matcher(
         owners,
         tags,
     };
-    codeowners_entry_to_matcher(&entry)
+    codeowners_entry_to_matcher(&entry).expect("Failed to create matcher in benchmark")
 }
 
 fn bench_find_owners_and_tags_simple_pattern(c: &mut Criterion) {

codeinput/src/benches/resolver_bench.rs

@@ -23,9 +23,24 @@ pub fn build_cache(
     let mut owners_map = std::collections::HashMap::new();
     let mut tags_map = std::collections::HashMap::new();
 
+    // Build matchers, filtering out invalid patterns with warnings
     let matched_entries: Vec<CodeownersEntryMatcher> = entries
         .iter()
-        .map(|entry| codeowners_entry_to_matcher(entry))
+        .filter_map(|entry| {
+            match codeowners_entry_to_matcher(entry) {
+                Ok(matcher) => Some(matcher),
+                Err(e) => {
+                    log::warn!(
+                        "Skipping invalid CODEOWNERS pattern '{}' in {} line {}: {}",
+                        entry.pattern,
+                        entry.source_file.display(),
+                        entry.line_number,
+                        e.message
+                    );
+                    None
+                }
+            }
+        })
         .collect();
 
     // Process each file to find owners and tags
@@ -53,16 +68,22 @@ pub fn build_cache(
                         "\r\x1b[K📁 Processing [{}/{}] {}",
                         current, total_files, truncated_file
                     );
-                    std::io::stdout().flush().unwrap();
-
-                    let (owners, tags) =
-                        find_owners_and_tags_for_file(file_path, &matched_entries).unwrap();
+                    let _ = std::io::stdout().flush(); // Ignore flush errors
+
+                    // Handle errors gracefully - skip files that can't be processed
+                    let (owners, tags) = match find_owners_and_tags_for_file(file_path, &matched_entries) {
+                        Ok(result) => result,
+                        Err(e) => {
+                            log::warn!("Failed to resolve ownership for {}: {}", file_path.display(), e);
+                            (vec![], vec![])
+                        }
+                    };
 
                     // Build file entry
                     FileEntry {
                         path: file_path.clone(),
-                        owners: owners.clone(),
-                        tags: tags.clone(),
+                        owners,
+                        tags,
                     }
                 })
                 .collect::<Vec<FileEntry>>()

codeinput/src/core/cache.rs

@@ -132,7 +132,11 @@ fn filter_unowned_files(
     };
 
     let mut unowned_files = Vec::new();
-    let matchers: Vec<_> = cache.entries.iter().map(codeowners_entry_to_matcher).collect();
+    let matchers: Vec<_> = cache
+        .entries
+        .iter()
+        .filter_map(|e| codeowners_entry_to_matcher(e).ok())
+        .collect();
     for file in files {
         let (owners, _tags) = find_owners_and_tags_for_file(&file, &matchers)?;
         if owners.is_empty() || owners.iter().all(|o| o.owner_type == OwnerType::Unowned) {
@@ -156,7 +160,11 @@ fn analyze_file_ownership(
     // Get existing owners from cache
     let existing_owners = match cache {
         Some(cache) => {
-            let matchers: Vec<_> = cache.entries.iter().map(codeowners_entry_to_matcher).collect();
+            let matchers: Vec<_> = cache
+                .entries
+                .iter()
+                .filter_map(|e| codeowners_entry_to_matcher(e).ok())
+                .collect();
             let (owners, _tags) = find_owners_and_tags_for_file(file_path, &matchers).unwrap_or_default();
             owners
         },

codeinput/src/core/commands/infer_owners.rs

@@ -6,8 +6,24 @@ use std::path::{Path, PathBuf};
 
 use super::types::{CodeownersEntry, Owner, Tag};
 
+/// Maximum recursion depth for finding CODEOWNERS files (prevents stack overflow on deep structures)
+const MAX_RECURSION_DEPTH: usize = 100;
+
 /// Find CODEOWNERS files recursively in the given directory and its subdirectories
 pub fn find_codeowners_files<P: AsRef<Path>>(base_path: P) -> Result<Vec<PathBuf>> {
+    find_codeowners_files_impl(base_path.as_ref(), 0)
+}
+
+fn find_codeowners_files_impl(base_path: &Path, depth: usize) -> Result<Vec<PathBuf>> {
+    if depth > MAX_RECURSION_DEPTH {
+        log::warn!(
+            "Maximum recursion depth ({}) reached while searching for CODEOWNERS files at {}",
+            MAX_RECURSION_DEPTH,
+            base_path.display()
+        );
+        return Ok(Vec::new());
+    }
+
     let mut result = Vec::new();
     if let Ok(entries) = std::fs::read_dir(base_path) {
         for entry in entries.flatten() {
@@ -21,7 +37,7 @@ pub fn find_codeowners_files<P: AsRef<Path>>(base_path: P) -> Result<Vec<PathBuf
             {
                 result.push(path);
             } else if path.is_dir() {
-                result.extend(find_codeowners_files(path)?);
+                result.extend(find_codeowners_files_impl(&path, depth + 1)?);
             }
         }
     }
@@ -67,6 +83,12 @@ pub fn collect_tags(entries: &[CodeownersEntry]) -> Vec<Tag> {
     tags.into_iter().collect()
 }
 
+/// Files to exclude from the repository hash calculation (these are generated files)
+const HASH_EXCLUDED_PATTERNS: &[&str] = &[
+    ".codeowners.cache",
+    "*.codeowners.cache",
+];
+
 pub fn get_repo_hash(repo_path: &Path) -> Result<[u8; 32]> {
     let repo = Repository::open(repo_path)
         .map_err(|e| Error::with_source("Failed to open repo", Box::new(e)))?;
@@ -75,7 +97,7 @@ pub fn get_repo_hash(repo_path: &Path) -> Result<[u8; 32]> {
     let head_oid = repo
         .head()
         .and_then(|r| r.resolve())
-        .and_then(|r| Ok(r.target()))
+        .map(|r| r.target())
         .unwrap_or(None);
 
     // 2. Get index/staging area tree hash
@@ -87,16 +109,35 @@ pub fn get_repo_hash(repo_path: &Path) -> Result<[u8; 32]> {
         .write_tree()
         .map_err(|e| Error::with_source("Failed to write index tree", Box::new(e)))?;
 
-    // 3. Calculate hash of unstaged changes
-    // TODO: this doesn't work and also we need to exclude .codeowners.cache file
-    // otherwise the hash will change every time we parse the repo
+    // 3. Calculate hash of unstaged changes, excluding cache files
     let unstaged_hash = {
+        let mut diff_opts = DiffOptions::new();
+        diff_opts.include_untracked(true);
+
+        // Add pathspec exclusions for cache files
+        for pattern in HASH_EXCLUDED_PATTERNS {
+            diff_opts.pathspec(format!(":(exclude){}", pattern));
+        }
+
         let diff = repo
-            .diff_index_to_workdir(None, Some(DiffOptions::new().include_untracked(true)))
+            .diff_index_to_workdir(None, Some(&mut diff_opts))
             .map_err(|e| Error::with_source("Failed to get diff", Box::new(e)))?;
 
         let mut hasher = Sha256::new();
-        diff.print(DiffFormat::Patch, |_, _, line| {
+        diff.print(DiffFormat::Patch, |delta, _, line| {
+            // Double-check: skip any cache files that might have slipped through
+            if let Some(path) = delta.new_file().path() {
+                let path_str = path.to_string_lossy();
+                if HASH_EXCLUDED_PATTERNS.iter().any(|p| {
+                    if p.starts_with('*') {
+                        path_str.ends_with(&p[1..])
+                    } else {
+                        path_str == *p
+                    }
+                }) {
+                    return true; // Skip this file
+                }
+            }
             hasher.update(line.content());
             true
         })

codeinput/src/core/common.rs

@@ -10,11 +10,3 @@ pub mod resolver;
 pub(crate) mod smart_iter;
 pub mod tag_resolver;
 pub mod types;
-
-use crate::utils::error::Result;
-
-pub fn start() -> Result<()> {
-    // does nothing
-
-    Ok(())
-}

codeinput/src/core/mod.rs

@@ -51,53 +51,74 @@ pub struct CodeownersEntryMatcher {
     pub override_matcher: Override,
 }
 
+/// Error type for pattern matching failures
+#[derive(Debug)]
+pub struct PatternError {
+    pub pattern: String,
+    pub source_file: PathBuf,
+    pub message: String,
+}
+
+impl std::fmt::Display for PatternError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "Invalid pattern '{}' in {}: {}",
+            self.pattern,
+            self.source_file.display(),
+            self.message
+        )
+    }
+}
+
+impl std::error::Error for PatternError {}
+
+/// Converts a CodeownersEntry to a CodeownersEntryMatcher with pattern compilation.
+///
+/// # Errors
+///
+/// Returns a `PatternError` if:
+/// - The entry's source file has no parent directory
+/// - The pattern is invalid and cannot be compiled
+/// - The override matcher fails to build
 #[cfg(feature = "ignore")]
-pub fn codeowners_entry_to_matcher(entry: &CodeownersEntry) -> CodeownersEntryMatcher {
-    let codeowners_dir = match entry.source_file.parent() {
-        Some(dir) => dir,
-        None => {
-            eprintln!(
-                "CODEOWNERS entry has no parent directory: {}",
-                entry.source_file.display()
-            );
-            panic!("Invalid CODEOWNERS entry without parent directory");
-        }
-    };
+pub fn codeowners_entry_to_matcher(
+    entry: &CodeownersEntry,
+) -> Result<CodeownersEntryMatcher, PatternError> {
+    let codeowners_dir = entry.source_file.parent().ok_or_else(|| PatternError {
+        pattern: entry.pattern.clone(),
+        source_file: entry.source_file.clone(),
+        message: "CODEOWNERS entry has no parent directory".to_string(),
+    })?;
 
     let mut builder = ignore::overrides::OverrideBuilder::new(codeowners_dir);
 
     // Transform directory patterns to match GitHub CODEOWNERS behavior
     let pattern = normalize_codeowners_pattern(&entry.pattern);
 
-    if let Err(e) = builder.add(&pattern) {
-        eprintln!(
-            "Invalid pattern '{}' (normalized from '{}') in {}: {}",
-            pattern,
-            entry.pattern,
-            entry.source_file.display(),
-            e
-        );
-        panic!("Invalid CODEOWNERS entry pattern");
-    }
-    let override_matcher: Override = match builder.build() {
-        Ok(o) => o,
-        Err(e) => {
-            eprintln!(
-                "Failed to build override for pattern '{}': {}",
-                entry.pattern, e
-            );
-            panic!("Failed to build CODEOWNERS entry matcher");
-        }
-    };
+    builder.add(&pattern).map_err(|e| PatternError {
+        pattern: entry.pattern.clone(),
+        source_file: entry.source_file.clone(),
+        message: format!(
+            "Invalid pattern '{}' (normalized from '{}'): {}",
+            pattern, entry.pattern, e
+        ),
+    })?;
+
+    let override_matcher = builder.build().map_err(|e| PatternError {
+        pattern: entry.pattern.clone(),
+        source_file: entry.source_file.clone(),
+        message: format!("Failed to build override matcher: {}", e),
+    })?;
 
-    CodeownersEntryMatcher {
+    Ok(CodeownersEntryMatcher {
         source_file: entry.source_file.clone(),
         line_number: entry.line_number,
         pattern: entry.pattern.clone(),
         owners: entry.owners.clone(),
         tags: entry.tags.clone(),
         override_matcher,
-    }
+    })
 }
 
 /// Detailed owner representation
@@ -355,7 +376,7 @@ mod tests {
             tags: vec![],
         };
 
-        let matcher = codeowners_entry_to_matcher(&entry);
+        let matcher = codeowners_entry_to_matcher(&entry).unwrap();
 
         // Test files that should match
         let test_files = vec![

codeinput/src/core/types.rs

@@ -16,7 +16,11 @@ pub struct Error {
 // Implement the Display trait for our Error type.
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{}", self.msg)
+        write!(f, "{}", self.msg)?;
+        if let Some(ref source) = self.source {
+            write!(f, ": {}", source)?;
+        }
+        Ok(())
     }
 }