From 6a6653d9d861c84b8a86e13c420b55115e7c739e Mon Sep 17 00:00:00 2001 From: ajianaz Date: Wed, 7 Oct 2026 16:20:03 +0700 Subject: [PATCH] refactor(review): split deterministic analysis from the LLM call Extract engine::deterministic::run (rules, secrets, security, index scans, claim flags) returning a structured DeterministicReport with context() and merge_into(), testable without an LLM. The three index scanners share one scan_changed_files preamble. All ignore/skip/include/exclude matching now goes through engine::path_match (index, review scanners, scan, watch). Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- src/commands/watch.rs | 6 +- src/engine/deterministic.rs | 296 +++++++++++++++++++ src/engine/index_scanner.rs | 568 ++++++++++++++++++++---------------- src/engine/mod.rs | 2 + src/engine/path_match.rs | 188 ++++++++++++ src/engine/review.rs | 172 ++--------- src/engine/scanner.rs | 18 +- src/index/mod.rs | 5 +- src/index/session.rs | 13 +- 9 files changed, 836 insertions(+), 432 deletions(-) create mode 100644 src/engine/deterministic.rs create mode 100644 src/engine/path_match.rs diff --git a/src/commands/watch.rs b/src/commands/watch.rs index c08c8bbc..f49108fc 100644 --- a/src/commands/watch.rs +++ b/src/commands/watch.rs @@ -42,7 +42,7 @@ pub fn run_watch( // Compile glob filter if provided let glob_matcher = filter.map(|p| { - glob::Pattern::new(p).unwrap_or_else(|e| { + crate::engine::path_match::PathPattern::new(p).unwrap_or_else(|e| { eprintln!("{} Invalid glob pattern '{p}': {e}", "⚠ ".yellow()); std::process::exit(1); }) @@ -114,7 +114,7 @@ pub fn run_watch( fn detect_changes( project_root: &Path, git_files: &Option>, - glob_matcher: Option<&glob::Pattern>, + glob_matcher: Option<&crate::engine::path_match::PathPattern>, ) -> Result> { let mut changed = Vec::new(); let extensions: &[&str] = &[ @@ -150,7 +150,7 @@ fn detect_changes( // Apply glob filter if let Some(pattern) = glob_matcher { - if !pattern.matches_path(rel) { + if !pattern.matches(&rel.to_string_lossy().replace('\\', "/")) { return; } } diff --git a/src/engine/deterministic.rs b/src/engine/deterministic.rs new file mode 100644 index 00000000..0f57fcfe --- /dev/null +++ b/src/engine/deterministic.rs @@ -0,0 +1,296 @@ +//! The deterministic half of a review: every check that needs no LLM. +//! +//! [`run`] takes one input (parsed diff chunks, the config, the index bridge) +//! and returns a [`DeterministicReport`] with the findings of each scanner +//! family plus the helpers the orchestrator needs: the context text for the +//! LLM prompt ([`DeterministicReport::context`]) and the merge of the findings +//! into a list of issues ([`DeterministicReport::merge_into`]). +//! +//! It always operates on the ORIGINAL (unsanitized) diff chunks — only the LLM +//! sees sanitized text (ALIBI defense, arXiv:2607.24964). It never calls the +//! LLM and never touches the network, so it can be tested end to end with an +//! in-memory [`IndexBridge`]. +//! +//! Family order is part of the output contract (context text and merge order): +//! rules, secrets, security, index unused imports, index dead code, index +//! breaking changes. + +use crate::config::schema::Config; +use crate::engine::ReviewIssue; +use crate::engine::comment_sanitizer::{self, SanitizeReport}; +use crate::engine::diff_parser::FileChunk; +use crate::engine::index_bridge::IndexBridge; +use crate::engine::rules::{self, types::RuleFinding}; +use crate::engine::{index_scanner, secrets_scanner, security_scanner}; + +/// Findings of every deterministic scanner family, in contract order. +#[derive(Debug, Default)] +pub struct DeterministicReport { + pub rules: Vec, + pub secrets: Vec, + pub security: Vec, + pub index_unused: Vec, + pub index_dead: Vec, + pub index_breaking: Vec, + /// Unverified-claim flags found in added comments (not findings). + pub claims: SanitizeReport, +} + +/// Exclusion patterns for review-time index scanners: the exact set the +/// indexer uses (`ignore.files` + `index_skip_files`), so review and index +/// never disagree about which files are out of scope. +pub fn skip_patterns(config: &Config) -> Vec { + crate::index::skip_patterns_from_config(Some(config)).unwrap_or_default() +} + +/// Run every deterministic check on a parsed diff. +pub fn run(chunks: &[FileChunk], config: &Config, bridge: &IndexBridge) -> DeterministicReport { + let max = config.rules_config.max_findings; + let skip = skip_patterns(config); + + DeterministicReport { + rules: rules::run_rules(chunks, &config.rules_config), + secrets: secrets_scanner::scan_secrets(chunks, max), + security: security_scanner::scan_security(chunks, max), + index_unused: index_scanner::scan_unused_imports(bridge, chunks, max, &skip), + index_dead: index_scanner::scan_dead_code_in_review(bridge, chunks, max, &skip), + index_breaking: index_scanner::scan_breaking_changes(bridge, chunks, max, &skip), + claims: comment_sanitizer::flag_claims(chunks), + } +} + +impl DeterministicReport { + fn families(&self) -> [&Vec; 6] { + [ + &self.rules, + &self.secrets, + &self.security, + &self.index_unused, + &self.index_dead, + &self.index_breaking, + ] + } + + /// Total number of findings across all families. + pub fn len(&self) -> usize { + self.families().iter().map(|f| f.len()).sum() + } + + /// True when no scanner produced a finding. + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Context text for the LLM prompt, or `None` when there is nothing to say. + /// + /// Sections, in order: optional static-analysis output, the unverified + /// claim warning, then one formatted block per non-empty family; joined + /// with a blank line. + pub fn context(&self, static_context: Option<&str>) -> Option { + let mut parts: Vec = Vec::new(); + if let Some(sa) = static_context { + parts.push(sa.to_string()); + } + if let Some(warning) = comment_sanitizer::format_claim_warning(&self.claims) { + parts.push(warning); + } + for family in self.families() { + let text = rules::format_rule_context(family); + if !text.is_empty() { + parts.push(text); + } + } + if parts.is_empty() { + None + } else { + Some(parts.join("\n\n")) + } + } + + /// Merge every finding into `issues` (family order), skipping findings at a + /// file:line the existing issues already cover. + pub fn merge_into(self, issues: Vec) -> Vec { + let mut merged = issues; + for family in [ + self.rules, + self.secrets, + self.security, + self.index_unused, + self.index_dead, + self.index_breaking, + ] { + if !family.is_empty() { + merged = rules::merge_rule_findings(merged, family); + } + } + merged + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::engine::diff_parser::parse_diff; + + const ROOT: &str = "/fixture/proj"; + + /// In-memory index: an unused import and an uncalled function in + /// `src/app.js`, plus a caller of `important_api`. + fn fixture_bridge() -> IndexBridge { + let root = std::path::Path::new(ROOT); + let conn = rusqlite::Connection::open_in_memory().expect("db"); + crate::index::schema::run_migrations(&conn).expect("migrations"); + let pid = crate::index::ensure_project(&conn, root).expect("project"); + conn.execute( + "INSERT INTO edges (source, kind, target, file, line, project_id) \ + VALUES ('src/app.js', 'IMPORTS', 'leftpad', 'src/app.js', 1, ?1)", + [pid], + ) + .unwrap(); + conn.execute( + "INSERT INTO symbols (name, kind, file, line, signature, language, project_id) \ + VALUES ('orphan_helper', 'function', 'src/app.js', 7, 'function orphan_helper()', 'javascript', ?1)", + [pid], + ) + .unwrap(); + conn.execute( + "INSERT INTO call_graph (caller, callee, file, line, project_id) \ + VALUES ('main_caller', 'important_api', 'src/main.js', 3, ?1)", + [pid], + ) + .unwrap(); + IndexBridge::from_connection(conn, root).expect("bridge") + } + + const DIFF: &str = "\ +diff --git a/src/app.js b/src/app.js +--- a/src/app.js ++++ b/src/app.js +@@ -1,2 +1,6 @@ + import leftpad from 'leftpad'; ++const key = 'AKIAIOSFODNN7EXAMPLE'; ++const digest = hashlib.md5(data); ++// TODO revisit ++function orphan_helper() {} +diff --git a/src/api.js b/src/api.js +--- a/src/api.js ++++ b/src/api.js +@@ -1,2 +1 @@ +-export function important_api() {} + export const kept = 1; +"; + + #[test] + fn runs_every_family_without_an_llm() { + let chunks = parse_diff(DIFF); + let report = run(&chunks, &Config::default(), &fixture_bridge()); + + assert!(!report.secrets.is_empty(), "secrets scanner"); + assert!(!report.security.is_empty(), "security scanner"); + assert!(!report.rules.is_empty(), "rule engine"); + assert_eq!(report.index_unused.len(), 1); + assert_eq!(report.index_unused[0].rule_id, "index-unused-import"); + assert_eq!(report.index_dead.len(), 1); + assert!(report.index_dead[0].title.contains("orphan_helper")); + assert_eq!(report.index_breaking.len(), 1); + assert!(report.index_breaking[0].title.contains("important_api")); + assert_eq!(report.index_breaking[0].file, "src/api.js"); + assert!(!report.is_empty()); + assert_eq!( + report.len(), + report.rules.len() + + report.secrets.len() + + report.security.len() + + report.index_unused.len() + + report.index_dead.len() + + report.index_breaking.len() + ); + } + + #[test] + fn context_is_ordered_and_optional() { + let chunks = parse_diff(DIFF); + let report = run(&chunks, &Config::default(), &fixture_bridge()); + + let ctx = report.context(Some("STATIC")).expect("context"); + assert!(ctx.starts_with("STATIC\n\n")); + let pos = |needle: &str| { + ctx.find(needle) + .unwrap_or_else(|| panic!("missing {needle}")) + }; + assert!(pos("index-unused-import") < pos("index-dead-code")); + assert!(pos("index-dead-code") < pos("index-breaking-change")); + assert_eq!( + ctx, + [ + "STATIC".to_string(), + rules::format_rule_context(&report.rules), + rules::format_rule_context(&report.secrets), + rules::format_rule_context(&report.security), + rules::format_rule_context(&report.index_unused), + rules::format_rule_context(&report.index_dead), + rules::format_rule_context(&report.index_breaking), + ] + .into_iter() + .filter(|s| !s.is_empty()) + .collect::>() + .join("\n\n") + ); + + let empty = run( + &parse_diff(""), + &Config::default(), + &IndexBridge::unavailable(), + ); + assert!(empty.is_empty()); + assert_eq!(empty.context(None), None); + assert_eq!( + empty.context(Some("only static")).as_deref(), + Some("only static") + ); + } + + #[test] + fn no_index_degrades_to_pattern_scanners_only() { + let chunks = parse_diff(DIFF); + let report = run(&chunks, &Config::default(), &IndexBridge::unavailable()); + assert!(!report.secrets.is_empty()); + assert!(report.index_unused.is_empty()); + assert!(report.index_dead.is_empty()); + assert!(report.index_breaking.is_empty()); + } + + #[test] + fn config_ignore_files_apply_to_index_scans() { + let chunks = parse_diff(DIFF); + let mut config = Config::default(); + config.ignore.files.push("src/**".to_string()); + let report = run(&chunks, &config, &fixture_bridge()); + assert!(report.index_unused.is_empty()); + assert!(report.index_dead.is_empty()); + assert!(report.index_breaking.is_empty()); + // Pattern scanners still see the diff: ignore.files only scopes the index. + assert!(!report.secrets.is_empty()); + } + + #[test] + fn merge_into_keeps_issues_first_and_skips_covered_locations() { + let chunks = parse_diff(DIFF); + let report = run(&chunks, &Config::default(), &fixture_bridge()); + let total = report.len(); + let first = report.secrets[0].clone(); + let llm = vec![ReviewIssue { + file: first.file.clone(), + line: Some(first.line), + severity: crate::engine::Severity::Major, + issue_type: None, + title: "llm".into(), + body: String::new(), + suggested_fix: None, + }]; + let merged = report.merge_into(llm); + assert_eq!(merged[0].title, "llm"); + assert!(merged.len() < total + 1, "covered location is skipped"); + assert!(merged.len() > 1); + } +} diff --git a/src/engine/index_scanner.rs b/src/engine/index_scanner.rs index 1059c41a..afad516d 100644 --- a/src/engine/index_scanner.rs +++ b/src/engine/index_scanner.rs @@ -5,126 +5,70 @@ /// - Unused imports (needs cross-reference between imports and usages) /// - Dead code in changed files (needs caller graph) /// - Breaking changes (needs cross-file caller resolution) +/// +/// The three diff scanners share one preamble, [`scan_changed_files`]: open the +/// index, walk the diff chunks, drop deleted/skipped files and de-duplicate by +/// file. Only the per-file query differs. +use rusqlite::Connection; use tracing::debug; use crate::engine::Severity; use crate::engine::diff_parser::{DiffLineType, FileChunk}; use crate::engine::index_bridge::IndexBridge; +use crate::engine::path_match::PathMatcher; use crate::engine::rules::types::RuleFinding; use crate::index::graph; use std::collections::HashSet; /// Check if a file path matches any of the skip patterns. -/// Supports simple glob patterns: -/// - Exact: `"src/main.ts"` → full path match -/// - Suffix: `"*.config.ts"` → basename ends with `.config.ts` -/// - Prefix: `"vite.config.*"` → basename starts with `vite.config.` -/// - Any-dir name: `"**/something"` → basename or path suffix match -/// - Any-dir wildcard: `"**/phaser/**"` → any path component equals `phaser` -/// - Prefix-dir: `"src/engine/**"` → file under `src/engine/` -/// - Double wildcard ext: `"**/*.test.ts"` → basename ends with `.test.ts` +/// +/// Thin wrapper over the one shared matcher, [`PathMatcher`]; see its module +/// docs for the exact semantics (exact/basename, `*`, `**/`, `dir/**`). +/// Prefer compiling a [`PathMatcher`] once when checking many paths. +#[cfg(test)] pub fn should_skip_file(file_path: &str, skip_patterns: &[String]) -> bool { - if skip_patterns.is_empty() { - return false; - } - - let basename = std::path::Path::new(file_path) - .file_name() - .map(|n| n.to_string_lossy().to_string()) - .unwrap_or_default(); - - for pattern in skip_patterns { - // Exact match (e.g. "src/main.ts") - if file_path == pattern { - return true; - } - // Basename exact match - if basename == *pattern { - return true; - } - if !pattern.contains('*') { - continue; - } - - // "**/*.ext" → suffix match on basename (any directory) - if let Some(rest) = pattern.strip_prefix("**/") { - if let Some(ext) = rest.strip_prefix("*.") { - if basename.ends_with(ext) { - return true; - } - } - } - - // "*.ext" → suffix match on basename - if pattern.starts_with("*.") { - let suffix = &pattern[1..]; // ".config.ts" - if basename.ends_with(suffix) { - return true; - } - } - - // "name.*" → prefix match on basename - if pattern.ends_with(".*") { - let prefix = &pattern[..pattern.len() - 2]; // "vite.config" - if basename.starts_with(prefix) { - return true; - } - } - - // "**/name/**" → any path component equals "name" - if let Some(dir) = pattern - .strip_prefix("**/") - .and_then(|s| s.strip_suffix("/**")) - { - let components: Vec<&str> = file_path.split('/').collect(); - if components.contains(&dir) { - return true; - } - } - - // "dir/**" → file under dir/ - if let Some(dir) = pattern.strip_suffix("/**") { - if file_path.starts_with(&format!("{dir}/")) || file_path == dir { - return true; - } - } - - // "**/name" → match basename or path suffix - if let Some(rest) = pattern.strip_prefix("**/") { - if rest.contains('*') { - continue; - } - if basename == rest || file_path.ends_with(&format!("/{rest}")) { - return true; - } - } - } + !skip_patterns.is_empty() && PathMatcher::new(skip_patterns).is_match(file_path) +} - false +/// Which diff chunks a scanner wants to see. +#[derive(Clone, Copy)] +struct FileSelect { + /// Ignore chunks without a `new_path` (deleted files). + skip_deleted: bool, + /// Ignore chunks with no added lines. + require_additions: bool, + /// Visit each file path at most once. + dedupe: bool, } -/// Scan for unused imports across all changed files using the symbol index. -/// -/// For each file with IMPORTS edges, checks if each imported symbol is actually -/// referenced in the file. Works only when a symbol index is available. +/// Shared preamble of the diff-based index scanners. /// -/// Returns `Vec` with severity `Minor` for each unused import. -pub fn scan_unused_imports( +/// Resolves the index (no index → no findings), walks `chunks` in order, +/// applies `select` and the skip patterns, calls `per_file` for each surviving +/// chunk, and stops once `max_findings` is reached. The result is capped at +/// `max_findings`. +fn scan_changed_files( bridge: &IndexBridge, + what: &str, chunks: &[FileChunk], max_findings: usize, skip_patterns: &[String], -) -> Vec { + select: FileSelect, + mut per_file: F, +) -> Vec +where + F: FnMut(&Connection, i64, &str, &FileChunk, &mut Vec), +{ let Some((conn, project_id)) = bridge.parts() else { - debug!("no project index available — skipping unused import scan"); + debug!("no project index available — skipping {what} scan"); return Vec::new(); }; + let skip = PathMatcher::new(skip_patterns); let mut findings = Vec::new(); + let mut seen_files: HashSet<&str> = HashSet::new(); - // Collect unique changed files - let mut seen_files = std::collections::HashSet::new(); for chunk in chunks { let file = chunk .new_path @@ -132,61 +76,86 @@ pub fn scan_unused_imports( .or(chunk.old_path.as_deref()) .unwrap_or("unknown"); - // Skip deleted files (no new_path) and unknown - if chunk.new_path.is_none() { + if select.skip_deleted && chunk.new_path.is_none() { continue; } - - // Skip files matching skip patterns - if should_skip_file(file, skip_patterns) { + if skip.is_match(file) { continue; } - - // Only check files with actual additions - let has_additions = chunk - .chunks - .iter() - .any(|h| h.lines.iter().any(|l| l.line_type == DiffLineType::Add)); - if !has_additions { + if select.require_additions + && !chunk + .chunks + .iter() + .any(|h| h.lines.iter().any(|l| l.line_type == DiffLineType::Add)) + { continue; } - - if seen_files.insert(file.to_string()) { - match graph::find_unused_imports(conn, file, project_id) { - Ok(unused) => { - for u in &unused { - findings.push(RuleFinding { - rule_id: "index-unused-import".to_string(), - file: u.file.clone(), - line: u.line, - severity: Severity::Minor, - title: format!("[index-unused-import] Unused import: {}", u.target), - body: format!( - "Import `{}` is never used in this file. \ - Consider removing it to keep imports clean.", - u.target - ), - }); - } - } - Err(e) => { - debug!("unused import scan failed for {}: {}", file, e); - } - } + if select.dedupe && !seen_files.insert(file) { + continue; } + per_file(conn, project_id, file, chunk, &mut findings); + if findings.len() >= max_findings { break; } } - // Cap findings findings.truncate(max_findings); - - debug!(count = findings.len(), "unused import scan complete"); + debug!(count = findings.len(), "{what} scan complete"); findings } +/// Scan for unused imports across all changed files using the symbol index. +/// +/// For each file with IMPORTS edges, checks if each imported symbol is actually +/// referenced in the file. Works only when a symbol index is available. +/// +/// Returns `Vec` with severity `Minor` for each unused import. +pub fn scan_unused_imports( + bridge: &IndexBridge, + chunks: &[FileChunk], + max_findings: usize, + skip_patterns: &[String], +) -> Vec { + let select = FileSelect { + skip_deleted: true, + require_additions: true, + dedupe: true, + }; + scan_changed_files( + bridge, + "unused import", + chunks, + max_findings, + skip_patterns, + select, + |conn, project_id, file, _chunk, findings| match graph::find_unused_imports( + conn, file, project_id, + ) { + Ok(unused) => { + for u in &unused { + findings.push(RuleFinding { + rule_id: "index-unused-import".to_string(), + file: u.file.clone(), + line: u.line, + severity: Severity::Minor, + title: format!("[index-unused-import] Unused import: {}", u.target), + body: format!( + "Import `{}` is never used in this file. \ + Consider removing it to keep imports clean.", + u.target + ), + }); + } + } + Err(e) => { + debug!("unused import scan failed for {}: {}", file, e); + } + }, + ) +} + /// Scan for dead code (unreachable symbols) in changed files using the symbol index. /// /// For each changed file, finds functions/methods with zero callers in the @@ -199,70 +168,52 @@ pub fn scan_dead_code_in_review( max_findings: usize, skip_patterns: &[String], ) -> Vec { - let Some((conn, project_id)) = bridge.parts() else { - debug!("no project index available — skipping dead code scan"); - return Vec::new(); + let select = FileSelect { + skip_deleted: true, + require_additions: false, + dedupe: true, }; - - let mut findings = Vec::new(); - - let mut seen_files = std::collections::HashSet::new(); - for chunk in chunks { - let file = chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or("unknown"); - - if chunk.new_path.is_none() { - continue; - } - - if should_skip_file(file, skip_patterns) { - continue; - } - - if seen_files.insert(file.to_string()) { - match graph::find_dead_code_in_file(conn, file, project_id, false) { - Ok(dead) => { - for d in &dead { - findings.push(RuleFinding { - rule_id: "index-dead-code".to_string(), - file: d.file.clone(), - line: d.line, - severity: Severity::Info, - title: format!("[index-dead-code] Potentially dead code: {}", d.name), - body: format!( - "Function `{}` ({}) has no callers in the \ + scan_changed_files( + bridge, + "dead code", + chunks, + max_findings, + skip_patterns, + select, + |conn, project_id, file, _chunk, findings| match graph::find_dead_code_in_file( + conn, file, project_id, false, + ) { + Ok(dead) => { + for d in &dead { + findings.push(RuleFinding { + rule_id: "index-dead-code".to_string(), + file: d.file.clone(), + line: d.line, + severity: Severity::Info, + title: format!("[index-dead-code] Potentially dead code: {}", d.name), + body: format!( + "Function `{}` ({}) has no callers in the \ project. Verify it's not called via reflection, \ trait dispatch, or external entry points.", - d.name, d.kind - ), - }); - } - } - Err(e) => { - debug!("dead code scan failed for {}: {}", file, e); + d.name, d.kind + ), + }); } } - } - - if findings.len() >= max_findings { - break; - } - } - - findings.truncate(max_findings); - - debug!(count = findings.len(), "dead code scan complete"); - findings + Err(e) => { + debug!("dead code scan failed for {}: {}", file, e); + } + }, + ) } /// Scan for potential breaking changes — removed or modified public symbols /// that have existing callers in the project. /// /// Analyzes the diff for removed lines containing public symbol definitions, -/// then cross-references the index to find callers. +/// then cross-references the index to find callers. Unlike the other scanners +/// it also visits deleted files (that is where removals live) and does not +/// de-duplicate files. /// /// Returns `Vec` with severity `Major` for each breaking change. pub fn scan_breaking_changes( @@ -271,11 +222,6 @@ pub fn scan_breaking_changes( max_findings: usize, skip_patterns: &[String], ) -> Vec { - let Some((conn, project_id)) = bridge.parts() else { - debug!("no project index available — skipping breaking change scan"); - return Vec::new(); - }; - // Symbol names (re)defined by this very diff — the post-image of the change. // A removal candidate whose name still exists post-change is signature // drift, a move, or a wording tweak of the definition line, not a removal; @@ -283,8 +229,6 @@ pub fn scan_breaking_changes( // index is a false positive (#533). let added_defs = collect_added_definitions(chunks); - let mut findings = Vec::new(); - // Patterns for public symbol removal across languages. // These are heuristic — not all removed lines match, but high-signal ones do. let removal_patterns: &[&str] = &[ @@ -298,33 +242,36 @@ pub fn scan_breaking_changes( r"(?m)^(?:async\s+)?(?:def|class)\s+(\w+)", ]; - let compiled: Vec> = removal_patterns + let compiled: Vec = removal_patterns .iter() .filter_map(|p| regex::Regex::new(p).ok()) - .map(std::sync::Arc::new) .collect(); - for chunk in chunks { - let file = chunk - .new_path - .as_deref() - .or(chunk.old_path.as_deref()) - .unwrap_or("unknown"); - - if should_skip_file(file, skip_patterns) { - continue; - } - - for hunk in &chunk.chunks { - for line in &hunk.lines { - // Only look at removed lines (old code being deleted) - if line.line_type != DiffLineType::Remove { - continue; - } + let select = FileSelect { + skip_deleted: false, + require_additions: false, + dedupe: false, + }; + scan_changed_files( + bridge, + "breaking change", + chunks, + max_findings, + skip_patterns, + select, + |conn, project_id, file, chunk, findings| { + for hunk in &chunk.chunks { + for line in &hunk.lines { + // Only look at removed lines (old code being deleted) + if line.line_type != DiffLineType::Remove { + continue; + } - // Try to match a public symbol definition being removed - for re in &compiled { - if let Some(caps) = re.captures(&line.content) { + // Try to match a public symbol definition being removed + for re in &compiled { + let Some(caps) = re.captures(&line.content) else { + continue; + }; let symbol_name = &caps[1]; // Skip trivially short names @@ -340,51 +287,43 @@ pub fn scan_breaking_changes( let line_no = line.old_line_no.unwrap_or(0); // Check if this symbol has callers in the index - match graph::find_callers(conn, project_id, symbol_name, 10) { - Ok(callers) if !callers.is_empty() => { - let caller_list = callers - .iter() - .take(3) - .map(|c| format!("{} ({}:{})", c.caller, c.file, c.line)) - .collect::>() - .join(", "); - - findings.push(RuleFinding { - rule_id: "index-breaking-change".to_string(), - file: file.to_string(), - line: line_no, - severity: Severity::Major, - title: format!( - "[index-breaking-change] Removing `{}` \ - breaks {} caller(s)", - symbol_name, - callers.len() - ), - body: format!( - "Symbol `{}` is being removed but has {} \ - caller(s): {}. This is a breaking change.", - symbol_name, - callers.len(), - caller_list - ), - }); + if let Ok(callers) = graph::find_callers(conn, project_id, symbol_name, 10) + { + if callers.is_empty() { + continue; } - _ => continue, + let caller_list = callers + .iter() + .take(3) + .map(|c| format!("{} ({}:{})", c.caller, c.file, c.line)) + .collect::>() + .join(", "); + + findings.push(RuleFinding { + rule_id: "index-breaking-change".to_string(), + file: file.to_string(), + line: line_no, + severity: Severity::Major, + title: format!( + "[index-breaking-change] Removing `{}` \ + breaks {} caller(s)", + symbol_name, + callers.len() + ), + body: format!( + "Symbol `{}` is being removed but has {} \ + caller(s): {}. This is a breaking change.", + symbol_name, + callers.len(), + caller_list + ), + }); } } } } - } - - if findings.len() >= max_findings { - break; - } - } - - findings.truncate(max_findings); - - debug!(count = findings.len(), "breaking change scan complete"); - findings + }, + ) } /// Names of symbol definitions appearing on added lines across the whole diff. @@ -441,9 +380,10 @@ pub fn scan_project_index( }; // Scan for unused imports across all files in the scan set + let skip = PathMatcher::new(skip_patterns); let mut seen_files = std::collections::HashSet::new(); for entry in files { - if should_skip_file(&entry.path, skip_patterns) { + if skip.is_match(&entry.path) { continue; } if seen_files.insert(entry.path.clone()) { @@ -768,6 +708,124 @@ mod tests { assert_eq!(findings.len(), 1); } + // --- shared chunk-iteration preamble (scan_changed_files) --- + + /// Index with an uncalled function `fn_name` in each of `files`. + fn index_with_dead_fn(fn_name: &str, files: &[&str]) -> IndexBridge { + let root = std::path::Path::new("/fixture/proj"); + let conn = rusqlite::Connection::open_in_memory().expect("db"); + crate::index::schema::run_migrations(&conn).expect("migrations"); + let pid = crate::index::ensure_project(&conn, root).expect("project"); + for file in files { + conn.execute( + "INSERT INTO symbols (name, kind, file, line, signature, language, project_id) \ + VALUES (?1, 'function', ?2, 3, 'sig', 'rust', ?3)", + rusqlite::params![fn_name, file, pid], + ) + .unwrap(); + } + IndexBridge::from_connection(conn, root).expect("bridge") + } + + fn deleted_chunk(path: &str) -> FileChunk { + let mut c = chunk_lines(path, &[("-", "fn orphan() {}")]); + c.new_path = None; + c.is_deleted = true; + c + } + + #[test] + fn preamble_skips_deleted_files() { + let bridge = index_with_dead_fn("orphan", &["src/gone.rs", "src/live.rs"]); + let chunks = vec![ + deleted_chunk("src/gone.rs"), + chunk_lines("src/live.rs", &[("+", "fn orphan() {}")]), + ]; + let findings = scan_dead_code_in_review(&bridge, &chunks, 10, &[]); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].file, "src/live.rs"); + } + + #[test] + fn preamble_respects_skip_patterns() { + let bridge = index_with_dead_fn("orphan", &["src/a.rs", "gen/b.rs", "src/c.test.rs"]); + let chunks = vec![ + chunk_lines("src/a.rs", &[("+", "x")]), + chunk_lines("gen/b.rs", &[("+", "x")]), + chunk_lines("src/c.test.rs", &[("+", "x")]), + ]; + let skip = vec!["gen/**".to_string(), "*.test.rs".to_string()]; + let findings = scan_dead_code_in_review(&bridge, &chunks, 10, &skip); + let files: Vec<_> = findings.iter().map(|f| f.file.as_str()).collect(); + assert_eq!(files, ["src/a.rs"]); + } + + #[test] + fn preamble_dedupes_by_file() { + let bridge = index_with_dead_fn("orphan", &["src/a.rs"]); + let chunks = vec![ + chunk_lines("src/a.rs", &[("+", "x")]), + chunk_lines("src/a.rs", &[("+", "y")]), + ]; + assert_eq!(scan_dead_code_in_review(&bridge, &chunks, 10, &[]).len(), 1); + } + + #[test] + fn preamble_caps_findings_at_max() { + let bridge = index_with_dead_fn("orphan", &["src/a.rs", "src/b.rs", "src/c.rs"]); + let chunks = vec![ + chunk_lines("src/a.rs", &[("+", "x")]), + chunk_lines("src/b.rs", &[("+", "x")]), + chunk_lines("src/c.rs", &[("+", "x")]), + ]; + assert_eq!(scan_dead_code_in_review(&bridge, &chunks, 2, &[]).len(), 2); + } + + #[test] + fn unused_imports_require_additions() { + let root = std::path::Path::new("/fixture/proj"); + let conn = rusqlite::Connection::open_in_memory().expect("db"); + crate::index::schema::run_migrations(&conn).expect("migrations"); + let pid = crate::index::ensure_project(&conn, root).expect("project"); + for file in ["src/a.rs", "src/b.rs"] { + conn.execute( + "INSERT INTO edges (source, kind, target, file, line, project_id) \ + VALUES (?1, 'IMPORTS', 'lodash', ?1, 1, ?2)", + rusqlite::params![file, pid], + ) + .unwrap(); + } + let bridge = IndexBridge::from_connection(conn, root).expect("bridge"); + let chunks = vec![ + chunk_lines("src/a.rs", &[(" ", "context"), ("-", "removed")]), + chunk_lines("src/b.rs", &[("+", "added")]), + ]; + let findings = scan_unused_imports(&bridge, &chunks, 10, &[]); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].file, "src/b.rs"); + } + + #[test] + fn breaking_changes_visit_deleted_files_and_skip_patterns() { + let bridge = index_with_callers("important_api", &[("c", "src/app.rs", 1)]); + let mut deleted = chunk_lines("src/lib.rs", &[("-", "pub fn important_api() {}")]); + deleted.new_path = None; + deleted.is_deleted = true; + let findings = scan_breaking_changes(&bridge, &[deleted.clone()], 10, &[]); + assert_eq!(findings.len(), 1, "removals live in deleted files"); + let skip = vec!["src/**".to_string()]; + assert!(scan_breaking_changes(&bridge, &[deleted], 10, &skip).is_empty()); + } + + #[test] + fn should_skip_file_wrapper_uses_the_shared_matcher() { + let patterns = vec!["**/phaser/**".to_string(), "*.config.ts".to_string()]; + assert!(should_skip_file("a/phaser/b.ts", &patterns)); + assert!(should_skip_file("x/vite.config.ts", &patterns)); + assert!(!should_skip_file("src/lib.rs", &patterns)); + assert!(!should_skip_file("src/lib.rs", &[])); + } + #[test] fn skip_empty_patterns() { assert!(!should_skip_file("src/main.ts", &[])); diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 9c6d5f7b..2912e2b1 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -5,6 +5,7 @@ pub mod comment_sanitizer; pub mod context; pub mod db_writer; pub mod debt_tracker; +pub mod deterministic; pub mod diff_parser; pub mod enclosing; pub mod index_bridge; @@ -13,6 +14,7 @@ pub mod language_analyzer; pub mod llm; pub mod markdown; pub mod memory; +pub mod path_match; pub mod profiles; pub mod quality_gate; pub mod review; diff --git a/src/engine/path_match.rs b/src/engine/path_match.rs new file mode 100644 index 00000000..3f36d1aa --- /dev/null +++ b/src/engine/path_match.rs @@ -0,0 +1,188 @@ +//! The single path-pattern matcher for ignore/skip/include/exclude lists. +//! +//! Before this module existed two matchers disagreed on the same config: +//! a hand-rolled `should_skip_file` (index time, review-time index scanners) +//! and `glob::Pattern` (`cora scan` include/exclude, `cora watch --filter`). +//! +//! Semantics (a pattern matches a `/`-separated, project-relative path when +//! ANY of the following holds): +//! +//! 1. the pattern equals the whole path or the basename (`src/main.ts`, +//! `main.ts`); +//! 2. the pattern, as a glob, matches the whole path. `*` also crosses `/` +//! (the `glob` crate default, which `scan`/`watch` already relied on) and +//! `**/` matches zero or more directories; +//! 3. the pattern, after dropping any leading `**/`, has no `/` and, as a glob, +//! matches the basename (`*.config.ts`, `vite.config.*`, `**/*.test.ts`); +//! 4. the pattern ends in `/**` and the part before it matches the path itself +//! (`src/engine/**` also skips a file literally named `src/engine`, +//! `**/phaser/**` also matches `a/phaser`). +//! +//! Known, intentional differences from the retired hand-rolled matcher: +//! `**/*.test.ts` no longer matches `footest.ts` (it compared against the +//! extension without its dot), `vite.config.*` no longer matches +//! `vite.configx`, and wildcard patterns such as `src/*.rs` now match instead +//! of being silently ignored. Patterns that fail to compile as globs fall +//! back to rule 1 only. The differences from plain `glob::Pattern` are rules 1, +//! 3 and 4: slash-free patterns also match by basename, so +//! `cora scan --exclude 'vite.config.*'` now also excludes nested configs. + +use glob::Pattern; + +/// One compiled pattern. +#[derive(Debug, Clone)] +pub struct PathPattern { + raw: String, + full: Option, + /// Basename-only glob (rule 3), present for slash-free patterns. + base: Option, + /// Glob for the directory itself (rule 4), present for `dir/**` patterns. + dir: Option, +} + +impl PathPattern { + /// Compile a pattern. Fails only when the pattern is not a valid glob. + pub fn new(pattern: &str) -> Result { + let full = Pattern::new(pattern)?; + let mut core = pattern; + while let Some(rest) = core.strip_prefix("**/") { + core = rest; + } + let base = if core.contains('/') { + None + } else { + Pattern::new(core).ok() + }; + let dir = pattern + .strip_suffix("/**") + .filter(|d| !d.is_empty()) + .and_then(|d| Pattern::new(d).ok()); + Ok(Self { + raw: pattern.to_string(), + full: Some(full), + base, + dir, + }) + } + + /// Compile leniently: an invalid glob still matches literally (rule 1). + pub fn lenient(pattern: &str) -> Self { + Self::new(pattern).unwrap_or_else(|_| Self { + raw: pattern.to_string(), + full: None, + base: None, + dir: None, + }) + } + + pub fn matches(&self, path: &str) -> bool { + if self.raw.is_empty() { + return false; + } + let basename = path.rsplit('/').next().unwrap_or(path); + if path == self.raw || basename == self.raw { + return true; + } + if self.full.as_ref().is_some_and(|g| g.matches(path)) { + return true; + } + if self.base.as_ref().is_some_and(|g| g.matches(basename)) { + return true; + } + self.dir.as_ref().is_some_and(|g| g.matches(path)) + } +} + +/// A set of patterns; a path matches when any pattern does. +#[derive(Debug, Clone, Default)] +pub struct PathMatcher { + patterns: Vec, +} + +impl PathMatcher { + /// Compile every pattern (invalid globs degrade to literal matching). + pub fn new(patterns: &[String]) -> Self { + Self { + patterns: patterns.iter().map(|p| PathPattern::lenient(p)).collect(), + } + } + + pub fn is_empty(&self) -> bool { + self.patterns.is_empty() + } + + pub fn is_match(&self, path: &str) -> bool { + self.patterns.iter().any(|p| p.matches(path)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn m(pats: &[&str]) -> PathMatcher { + PathMatcher::new(&pats.iter().map(|s| (*s).to_string()).collect::>()) + } + + #[test] + fn empty_matcher_matches_nothing() { + assert!(!m(&[]).is_match("src/main.rs")); + assert!(m(&[]).is_empty()); + assert!(!m(&[""]).is_match("src/main.rs")); + } + + #[test] + fn exact_and_basename() { + let x = m(&["src/main.ts", "Makefile"]); + assert!(x.is_match("src/main.ts")); + assert!(x.is_match("a/b/Makefile")); + assert!(!x.is_match("src/main.tsx")); + } + + #[test] + fn double_star_variants() { + let x = m(&["**/*.test.ts"]); + assert!(x.is_match("a.test.ts")); + assert!(x.is_match("a/b/c.test.ts")); + assert!(!x.is_match("footest.ts")); + assert!(!x.is_match("a/c.test.js")); + + let d = m(&["**/phaser/**"]); + assert!(d.is_match("phaser/a.ts")); + assert!(d.is_match("src/phaser/a/b.ts")); + assert!(d.is_match("a/phaser")); + assert!(!d.is_match("src/phaserHelper.ts")); + + let s = m(&["**/something"]); + assert!(s.is_match("something")); + assert!(s.is_match("a/b/something")); + assert!(!s.is_match("a/b/something-else")); + } + + #[test] + fn dir_prefix() { + let x = m(&["target/**", "src/engine/**"]); + assert!(x.is_match("target/debug/x.rs")); + assert!(x.is_match("src/engine/core/mod.rs")); + assert!(x.is_match("src/engine")); + assert!(!x.is_match("src/app/engine.rs")); + assert!(!x.is_match("crates/a/target/x.rs")); + } + + #[test] + fn wildcards_and_edge_cases() { + assert!(m(&["vite.config.*"]).is_match("apps/web/vite.config.ts")); + assert!(!m(&["vite.config.*"]).is_match("vite.configx")); + assert!(m(&["src/*.rs"]).is_match("src/lib.rs")); + assert!(m(&["*.gen.*"]).is_match("a/b/x.gen.go")); + assert!(!m(&["*.config.ts"]).is_match("config.ts")); + } + + #[test] + fn invalid_glob_degrades_to_literal() { + let x = m(&["a[b"]); + assert!(x.is_match("a[b")); + assert!(!x.is_match("ab")); + assert!(PathPattern::new("a[b").is_err()); + } +} diff --git a/src/engine/review.rs b/src/engine/review.rs index ed31b171..74599995 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -2,7 +2,6 @@ use crate::error::CoraError; use tracing::{debug, instrument}; use crate::config::schema::Config; -use crate::engine::comment_sanitizer; use crate::engine::llm; use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, Severity}; @@ -49,13 +48,6 @@ pub fn resolve_system_prompt(inline: Option<&str>, file_path: Option<&str>) -> O } } -/// Exclusion patterns for review-time index scanners: the exact set the -/// indexer uses (`ignore.files` + `index_skip_files`), so review and index -/// never disagree about which files are out of scope. -pub fn index_skip_patterns(config: &Config) -> Vec { - crate::index::skip_patterns_from_config(Some(config)).unwrap_or_default() -} - /// Run a code review on the given diff string with optional streaming and cache control. /// /// When `stream` is true, LLM tokens are printed to stdout in real-time. @@ -139,7 +131,6 @@ async fn review_diff_inner( // security) always operate on the ORIGINAL unsanitized diff — only the // LLM sees sanitized text (ALIBI defense, arXiv:2607.24964). let diff_chunks = crate::engine::diff_parser::parse_diff(diff); - let sanitize_report = crate::engine::comment_sanitizer::flag_claims(&diff_chunks); let review_diff_text: std::borrow::Cow<'_, str> = if config.sanitize_comments { let mut sanitized_chunks = crate::engine::diff_parser::parse_diff(diff); let full_report = crate::engine::comment_sanitizer::sanitize_chunks(&mut sanitized_chunks); @@ -155,99 +146,30 @@ async fn review_diff_inner( std::borrow::Cow::Owned(rendered) } } else { - if !sanitize_report.suspicious_claims.is_empty() { - debug!( - claims = sanitize_report.suspicious_claims.len(), - "Untrusted verification claims flagged in added comments" - ); - } std::borrow::Cow::Borrowed(diff) }; - let rule_findings = crate::engine::rules::run_rules(&diff_chunks, &config.rules_config); - - // Run deterministic secrets pre-scan - let secrets_findings = crate::engine::secrets_scanner::scan_secrets( - &diff_chunks, - config.rules_config.max_findings, - ); - - // Run deterministic security pattern scan (weak crypto, injection, etc.) - let security_findings = crate::engine::security_scanner::scan_security( - &diff_chunks, - config.rules_config.max_findings, - ); - - // Run index-powered scans (requires symbol graph — graceful no-op without index) - // One bridge, rooted via resolve_project_root, shared by every index-backed - // step so a run from a subdirectory agrees with `cora index` (#566). + // Index bridge: one project handle, rooted via resolve_project_root, shared + // by every index-backed step so a run from a subdirectory agrees with + // `cora index` (#566). let index_bridge = crate::engine::index_bridge::IndexBridge::open_cwd(); let project_root = if index_bridge.root().as_os_str().is_empty() { std::env::current_dir().unwrap_or_default() } else { index_bridge.root().to_path_buf() }; - // Same exclusion set the indexer uses (ignore.files + index_skip_files). - let skip_patterns = &index_skip_patterns(config); - let index_unused_findings = crate::engine::index_scanner::scan_unused_imports( - &index_bridge, - &diff_chunks, - config.rules_config.max_findings, - skip_patterns, - ); - let index_dead_findings = crate::engine::index_scanner::scan_dead_code_in_review( - &index_bridge, - &diff_chunks, - config.rules_config.max_findings, - skip_patterns, - ); - let index_breaking_findings = crate::engine::index_scanner::scan_breaking_changes( - &index_bridge, - &diff_chunks, - config.rules_config.max_findings, - skip_patterns, - ); - let rule_context = crate::engine::rules::format_rule_context(&rule_findings); - let secrets_context = crate::engine::rules::format_rule_context(&secrets_findings); - let security_context = crate::engine::rules::format_rule_context(&security_findings); - let index_unused_context = crate::engine::rules::format_rule_context(&index_unused_findings); - let index_dead_context = crate::engine::rules::format_rule_context(&index_dead_findings); - let index_breaking_context = - crate::engine::rules::format_rule_context(&index_breaking_findings); - // Keep a clone for merging after LLM (rule_findings may be consumed in error fallback) - let rule_findings_clone = rule_findings.clone(); - let secrets_findings_clone = secrets_findings.clone(); - let security_findings_clone = security_findings.clone(); - let index_unused_findings_clone = index_unused_findings.clone(); - let index_dead_findings_clone = index_dead_findings.clone(); - let index_breaking_findings_clone = index_breaking_findings.clone(); - - // Combine all context sections for LLM prompt (static analysis + all scanner findings) - let mut context_parts: Vec = Vec::new(); - if let Some(sa) = static_context.as_deref() { - context_parts.push(sa.to_string()); - } - if let Some(warning) = comment_sanitizer::format_claim_warning(&sanitize_report) { - context_parts.push(warning); - } - for ctx in [ - rule_context.as_str(), - secrets_context.as_str(), - security_context.as_str(), - index_unused_context.as_str(), - index_dead_context.as_str(), - index_breaking_context.as_str(), - ] { - if !ctx.is_empty() { - context_parts.push(ctx.to_string()); - } + // All deterministic checks (rules, secrets, security, index scans) on the + // ORIGINAL diff — no LLM involved. Context = static analysis + claim + // warning + one block per scanner family. + let deterministic = crate::engine::deterministic::run(&diff_chunks, config, &index_bridge); + if !deterministic.claims.suspicious_claims.is_empty() && !config.sanitize_comments { + debug!( + claims = deterministic.claims.suspicious_claims.len(), + "Untrusted verification claims flagged in added comments" + ); } - let combined_context = if context_parts.is_empty() { - None - } else { - Some(context_parts.join("\n\n")) - }; + let combined_context = deterministic.context(static_context.as_deref()); // Build context chain (cross-file dependency extraction) // NOTE: pass ignore.files (e.g. target/**, node_modules/**) so the resolver @@ -358,19 +280,13 @@ async fn review_diff_inner( Ok(resp) => resp, Err(e) => { // LLM failed — return deterministic findings only (don't silently swallow them) - if !rule_findings.is_empty() - || !secrets_findings.is_empty() - || !security_findings.is_empty() - || !index_unused_findings.is_empty() - || !index_dead_findings.is_empty() - || !index_breaking_findings.is_empty() - { - let n_rules = rule_findings.len(); - let n_secrets = secrets_findings.len(); - let n_security = security_findings.len(); - let n_index_unused = index_unused_findings.len(); - let n_index_dead = index_dead_findings.len(); - let n_index_breaking = index_breaking_findings.len(); + if !deterministic.is_empty() { + let n_rules = deterministic.rules.len(); + let n_secrets = deterministic.secrets.len(); + let n_security = deterministic.security.len(); + let n_index_unused = deterministic.index_unused.len(); + let n_index_dead = deterministic.index_dead.len(); + let n_index_breaking = deterministic.index_breaking.len(); debug!( error = %e, rule_findings = n_rules, @@ -381,24 +297,7 @@ async fn review_diff_inner( index_breaking = n_index_breaking, "LLM call failed, returning deterministic findings only" ); - let mut all_deterministic = - crate::engine::rules::merge_rule_findings(vec![], rule_findings); - all_deterministic = - crate::engine::rules::merge_rule_findings(all_deterministic, secrets_findings); - all_deterministic = - crate::engine::rules::merge_rule_findings(all_deterministic, security_findings); - all_deterministic = crate::engine::rules::merge_rule_findings( - all_deterministic, - index_unused_findings, - ); - all_deterministic = crate::engine::rules::merge_rule_findings( - all_deterministic, - index_dead_findings, - ); - all_deterministic = crate::engine::rules::merge_rule_findings( - all_deterministic, - index_breaking_findings, - ); + let all_deterministic = deterministic.merge_into(vec![]); let mut fallback = ReviewResponse { issues: all_deterministic, summary: format!( @@ -420,33 +319,8 @@ async fn review_diff_inner( } }; - // Merge rule findings + secrets findings + security findings + index findings with LLM issues - if !rule_findings_clone.is_empty() { - response.issues = - crate::engine::rules::merge_rule_findings(response.issues, rule_findings_clone); - } - if !secrets_findings_clone.is_empty() { - response.issues = - crate::engine::rules::merge_rule_findings(response.issues, secrets_findings_clone); - } - if !security_findings_clone.is_empty() { - response.issues = - crate::engine::rules::merge_rule_findings(response.issues, security_findings_clone); - } - if !index_unused_findings_clone.is_empty() { - response.issues = - crate::engine::rules::merge_rule_findings(response.issues, index_unused_findings_clone); - } - if !index_dead_findings_clone.is_empty() { - response.issues = - crate::engine::rules::merge_rule_findings(response.issues, index_dead_findings_clone); - } - if !index_breaking_findings_clone.is_empty() { - response.issues = crate::engine::rules::merge_rule_findings( - response.issues, - index_breaking_findings_clone, - ); - } + // Merge deterministic findings (rules, secrets, security, index) with LLM issues + response.issues = deterministic.merge_into(response.issues); // Filter out issues with invalid file paths (hallucination guard) if !valid_files.is_empty() { diff --git a/src/engine/scanner.rs b/src/engine/scanner.rs index 72833f46..60a515e3 100644 --- a/src/engine/scanner.rs +++ b/src/engine/scanner.rs @@ -2,8 +2,8 @@ use std::collections::BTreeSet; use std::io::IsTerminal; use std::path::Path; +use crate::engine::path_match::PathMatcher; use crate::error::CoraError; -use glob::Pattern; use ignore::WalkBuilder; use indicatif::{ProgressBar, ProgressDrawTarget, ProgressStyle}; use tracing::debug; @@ -52,15 +52,8 @@ pub fn walk_project( extensions.insert(ext.trim_start_matches('.').to_lowercase()); } - let include_globs: Vec = include_patterns - .iter() - .filter_map(|p| Pattern::new(p).ok()) - .collect(); - - let exclude_globs: Vec = exclude_patterns - .iter() - .filter_map(|p| Pattern::new(p).ok()) - .collect(); + let include_globs = PathMatcher::new(include_patterns); + let exclude_globs = PathMatcher::new(exclude_patterns); let mut entries = Vec::new(); @@ -110,13 +103,12 @@ pub fn walk_project( .to_string(); // Check exclude patterns - if exclude_globs.iter().any(|g| g.matches(&relative)) { + if exclude_globs.is_match(&relative) { continue; } // Check include patterns (if any specified) - let has_include = !include_globs.is_empty(); - if has_include && !include_globs.iter().any(|g| g.matches(&relative)) { + if !include_globs.is_empty() && !include_globs.is_match(&relative) { continue; } diff --git a/src/index/mod.rs b/src/index/mod.rs index c8a8436b..0e459d16 100644 --- a/src/index/mod.rs +++ b/src/index/mod.rs @@ -378,6 +378,7 @@ fn index_project_with_id( skip_patterns: Option<&[String]>, ) -> anyhow::Result { let mut stats = IndexStats::default(); + let skip_matcher = skip_patterns.map(crate::engine::path_match::PathMatcher::new); // Every indexable file seen on disk this run (post language + skip // filters). Anything stored for the project but absent here is stale. let mut walked: std::collections::HashSet = std::collections::HashSet::new(); @@ -412,9 +413,7 @@ fn index_project_with_id( // Config-driven exclusion (#521): honor ignore.files / // index_skip_files so dead-code, review index scanners, and brain // never see these files. - if skip_patterns.is_some_and(|patterns| { - crate::engine::index_scanner::should_skip_file(&rel_str, patterns) - }) { + if skip_matcher.as_ref().is_some_and(|m| m.is_match(&rel_str)) { stats.files_excluded += 1; continue; } diff --git a/src/index/session.rs b/src/index/session.rs index 07ffc136..f4a35713 100644 --- a/src/index/session.rs +++ b/src/index/session.rs @@ -274,15 +274,10 @@ mod tests { let (_d, root) = project(YAML); let config = load_project_only(&root).unwrap(); let session = IndexSession::from_bridge(memory_bridge(&root), Some(&config)).unwrap(); - let review_patterns = crate::engine::review::index_skip_patterns(&config); + let review_patterns = crate::engine::deterministic::skip_patterns(&config); assert_eq!(Some(review_patterns.as_slice()), session.skip_patterns()); - assert!(crate::engine::index_scanner::should_skip_file( - "gen/b.rs", - &review_patterns - )); - assert!(crate::engine::index_scanner::should_skip_file( - "vendor/c.rs", - &review_patterns - )); + let matcher = crate::engine::path_match::PathMatcher::new(&review_patterns); + assert!(matcher.is_match("gen/b.rs")); + assert!(matcher.is_match("vendor/c.rs")); } }