cora-code 0.17.2

/// Index-powered deterministic scanners — rules that require symbol graph intelligence.
///
/// Unlike regex-based scanners (secrets, security), these scanners query the
/// symbol index to detect issues impossible with pattern matching alone:
/// - 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.
///
/// 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 {
    !skip_patterns.is_empty() && PathMatcher::new(skip_patterns).is_match(file_path)
}

/// 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,
}

/// Shared preamble of the diff-based index scanners.
///
/// 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<F>(
    bridge: &IndexBridge,
    what: &str,
    chunks: &[FileChunk],
    max_findings: usize,
    skip_patterns: &[String],
    select: FileSelect,
    mut per_file: F,
) -> Vec<RuleFinding>
where
    F: FnMut(&Connection, i64, &str, &FileChunk, &mut Vec<RuleFinding>),
{
    let Some((conn, project_id)) = bridge.parts() else {
        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();

    for chunk in chunks {
        let file = chunk
            .new_path
            .as_deref()
            .or(chunk.old_path.as_deref())
            .unwrap_or("unknown");

        if select.skip_deleted && chunk.new_path.is_none() {
            continue;
        }
        if skip.is_match(file) {
            continue;
        }
        if select.require_additions
            && !chunk
                .chunks
                .iter()
                .any(|h| h.lines.iter().any(|l| l.line_type == DiffLineType::Add))
        {
            continue;
        }
        if select.dedupe && !seen_files.insert(file) {
            continue;
        }

        per_file(conn, project_id, file, chunk, &mut findings);

        if findings.len() >= max_findings {
            break;
        }
    }

    findings.truncate(max_findings);
    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<RuleFinding>` with severity `Minor` for each unused import.
pub fn scan_unused_imports(
    bridge: &IndexBridge,
    chunks: &[FileChunk],
    max_findings: usize,
    skip_patterns: &[String],
) -> Vec<RuleFinding> {
    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
/// project. Excludes well-known names (main, new, drop, etc.) and test functions.
///
/// Returns `Vec<RuleFinding>` with severity `Info` for each dead symbol.
pub fn scan_dead_code_in_review(
    bridge: &IndexBridge,
    chunks: &[FileChunk],
    max_findings: usize,
    skip_patterns: &[String],
) -> Vec<RuleFinding> {
    let select = FileSelect {
        skip_deleted: true,
        require_additions: false,
        dedupe: true,
    };
    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);
            }
        },
    )
}

/// 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. Unlike the other scanners
/// it also visits deleted files (that is where removals live) and does not
/// de-duplicate files.
///
/// Returns `Vec<RuleFinding>` with severity `Major` for each breaking change.
pub fn scan_breaking_changes(
    bridge: &IndexBridge,
    chunks: &[FileChunk],
    max_findings: usize,
    skip_patterns: &[String],
) -> Vec<RuleFinding> {
    // 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;
    // reporting it as "removal breaks N callers" against a possibly-stale
    // index is a false positive (#533).
    let added_defs = collect_added_definitions(chunks);

    // Patterns for public symbol removal across languages.
    // These are heuristic — not all removed lines match, but high-signal ones do.
    let removal_patterns: &[&str] = &[
        // Rust: pub fn/struct/enum/mod
        r"(?m)^(?:pub\s+)?(?:fn|struct|enum|trait|mod|type|const|static)\s+(\w+)",
        // TypeScript/JS: export function/const/class
        r"(?m)^export\s+(?:async\s+)?(?:function|const|class|interface|type)\s+(\w+)",
        // Go: func and type declarations
        r"(?m)^(?:func|type|var|const)\s+(\w+)",
        // Python: def and class
        r"(?m)^(?:async\s+)?(?:def|class)\s+(\w+)",
    ];

    let compiled: Vec<regex::Regex> = removal_patterns
        .iter()
        .filter_map(|p| regex::Regex::new(p).ok())
        .collect();

    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 {
                        let Some(caps) = re.captures(&line.content) else {
                            continue;
                        };
                        let symbol_name = &caps[1];

                        // Skip trivially short names
                        if symbol_name.len() < 3 {
                            continue;
                        }

                        // The diff itself redefines this symbol — not a removal.
                        if added_defs.contains(symbol_name) {
                            continue;
                        }

                        let line_no = line.old_line_no.unwrap_or(0);

                        // Check if this symbol has callers in the index
                        if let Ok(callers) = graph::find_callers(conn, project_id, symbol_name, 10)
                        {
                            if callers.is_empty() {
                                continue;
                            }
                            let caller_list = callers
                                .iter()
                                .take(3)
                                .map(|c| format!("{} ({}:{})", c.caller, c.file, c.line))
                                .collect::<Vec<_>>()
                                .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
                                ),
                            });
                        }
                    }
                }
            }
        },
    )
}

/// Names of symbol definitions appearing on added lines across the whole diff.
///
/// Same regex set as the removal scan, applied to `+` lines — the post-image
/// of the change. Diff-global (all chunks), so a definition moved between
/// files is still recognized as continuing to exist.
fn collect_added_definitions(chunks: &[FileChunk]) -> HashSet<String> {
    let patterns: &[&str] = &[
        r"(?m)^(?:pub\s+)?(?:fn|struct|enum|trait|mod|type|const|static)\s+(\w+)",
        r"(?m)^export\s+(?:async\s+)?(?:function|const|class|interface|type)\s+(\w+)",
        r"(?m)^(?:func|type|var|const)\s+(\w+)",
        r"(?m)^(?:async\s+)?(?:def|class)\s+(\w+)",
    ];
    let compiled: Vec<regex::Regex> = patterns
        .iter()
        .filter_map(|p| regex::Regex::new(p).ok())
        .collect();

    let mut names = HashSet::new();
    for chunk in chunks {
        for hunk in &chunk.chunks {
            for line in &hunk.lines {
                if line.line_type != DiffLineType::Add {
                    continue;
                }
                for re in &compiled {
                    if let Some(caps) = re.captures(&line.content) {
                        names.insert(caps[1].to_string());
                    }
                }
            }
        }
    }
    names
}

/// Scan a full project for index-based findings (unused imports + dead code).
/// Designed for `cora scan` which operates on file paths, not diffs.
/// Returns findings for any file in the project that has an index DB.
pub fn scan_project_index(
    bridge: &IndexBridge,
    files: &[crate::engine::scanner::FileEntry],
    max_findings: usize,
    skip_patterns: &[String],
) -> Vec<crate::engine::ReviewIssue> {
    use crate::engine::ReviewIssue;

    let mut findings = Vec::new();

    let Some((conn, project_id)) = bridge.parts() else {
        debug!("no project index available — skipping project index scan");
        return findings;
    };

    // 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 skip.is_match(&entry.path) {
            continue;
        }
        if seen_files.insert(entry.path.clone()) {
            match graph::find_unused_imports(conn, &entry.path, project_id) {
                Ok(unused) => {
                    for u in &unused {
                        findings.push(
                            ReviewIssue::new(
                                u.file.clone(),
                                Some(u.line),
                                crate::engine::Severity::Minor,
                                format!("[index-unused-import] Unused import: {}", u.target),
                            )
                            .with_type("index")
                            .with_body(format!(
                                "Import `{}` is never used in this file. \
                                 Consider removing it to keep imports clean.",
                                u.target
                            ))
                            .with_rule_id("index-unused-import"),
                        );
                    }
                }
                Err(e) => {
                    debug!("unused import scan failed for {}: {}", entry.path, e);
                }
            }
        }
        if findings.len() >= max_findings {
            break;
        }
    }

    if findings.len() >= max_findings {
        return findings;
    }

    // Scan for dead code in the indexed project
    let opts = graph::DeadCodeOptions::default();
    match graph::find_dead_code(conn, project_id, &opts) {
        Ok(dead) => {
            for func in dead.into_iter().take(max_findings - findings.len()) {
                findings.push(
                    ReviewIssue::new(
                        func.file.clone(),
                        Some(func.line),
                        crate::engine::Severity::Info,
                        format!("[index-dead-code] Potentially dead code: {}", func.name),
                    )
                    .with_type("index")
                    .with_body(format!(
                        "Function `{}` ({}) has no callers in the \
                         project. Verify it's not called via reflection, \
                         trait dispatch, or external entry points.",
                        func.name, func.kind
                    ))
                    .with_rule_id("index-dead-code"),
                );
            }
        }
        Err(e) => {
            debug!("dead code scan failed: {}", e);
        }
    }

    findings
}

#[cfg(test)]
mod tests {
    use super::*;

    /// Helper to create a minimal FileChunk for testing.
    fn make_chunk(new_path: &str, content: &str) -> FileChunk {
        FileChunk {
            old_path: Some(new_path.to_string()),
            new_path: Some(new_path.to_string()),
            language: "rust".to_string(),
            chunks: vec![crate::engine::diff_parser::DiffHunk {
                old_start: 1,
                old_count: 0,
                new_start: 1,
                new_count: 0,
                header: "@@ -1 +1 @@".to_string(),
                lines: content
                    .lines()
                    .map(|l| crate::engine::diff_parser::DiffLine {
                        content: l.to_string(),
                        line_type: if l.starts_with('+') {
                            DiffLineType::Add
                        } else if l.starts_with('-') {
                            DiffLineType::Remove
                        } else {
                            DiffLineType::Context
                        },
                        old_line_no: None,
                        new_line_no: None,
                    })
                    .collect(),
            }],
            is_binary: false,
            is_deleted: false,
            is_new: false,
        }
    }

    #[test]
    fn scan_unused_imports_no_index_graceful() {
        // No index available — should return empty, not panic
        let chunks = vec![make_chunk("src/main.rs", "+use std::collections::HashMap;")];
        let findings = scan_unused_imports(&IndexBridge::unavailable(), &chunks, 10, &[]);
        assert!(
            findings.is_empty(),
            "should gracefully return empty without index"
        );
    }

    #[test]
    fn scan_dead_code_no_index_graceful() {
        let chunks = vec![make_chunk("src/main.rs", "+fn foo() {}")];
        let findings = scan_dead_code_in_review(&IndexBridge::unavailable(), &chunks, 10, &[]);
        assert!(
            findings.is_empty(),
            "should gracefully return empty without index"
        );
    }

    #[test]
    fn scan_breaking_changes_no_index_graceful() {
        let chunks = vec![make_chunk("src/main.rs", "-pub fn important_api() {}")];
        let findings = scan_breaking_changes(&IndexBridge::unavailable(), &chunks, 10, &[]);
        assert!(
            findings.is_empty(),
            "should gracefully return empty without index"
        );
    }

    #[test]
    fn scan_breaking_changes_detects_removed_pub_fn() {
        let chunks = vec![make_chunk(
            "src/lib.rs",
            "-pub fn important_api() {}\n+pub fn new_api() {}",
        )];
        // No index, so no callers detected — but the pattern should still compile
        let findings = scan_breaking_changes(&IndexBridge::unavailable(), &chunks, 10, &[]);
        assert!(findings.is_empty(), "no index means no caller data");
    }

    // --- scan_breaking_changes: stale-index false-positive guard (#533) ---

    /// In-memory index with caller edges for a symbol, mirroring a populated
    /// global index that may be out of date relative to the diff.
    fn index_with_callers(callee: &str, callers: &[(&str, &str, i64)]) -> IndexBridge {
        index_at_root(std::path::Path::new("/fixture/proj"), callee, callers)
    }

    /// Same, with the project registered under `root` and the bridge opened from `start`.
    fn index_at_root(
        root: &std::path::Path,
        callee: &str,
        callers: &[(&str, &str, i64)],
    ) -> IndexBridge {
        let conn = rusqlite::Connection::open_in_memory().expect("in-memory db");
        crate::index::schema::run_migrations(&conn).expect("migrations");
        let project_id = crate::index::ensure_project(&conn, root).expect("project");
        for (caller, file, line) in callers {
            conn.execute(
                "INSERT INTO call_graph (caller, callee, file, line, project_id) \
                 VALUES (?1, ?2, ?3, ?4, ?5)",
                rusqlite::params![caller, callee, file, line, project_id],
            )
            .expect("insert call_graph");
        }
        IndexBridge::from_connection(conn, root).expect("bridge")
    }

    /// Build a chunk the way the real diff parser does: content WITHOUT the
    /// leading marker (the parser strips `+`/`-`/space before storing).
    fn chunk_lines(path: &str, lines: &[(&str, &str)]) -> FileChunk {
        FileChunk {
            old_path: Some(path.to_string()),
            new_path: Some(path.to_string()),
            language: "rust".to_string(),
            is_binary: false,
            is_deleted: false,
            is_new: false,
            chunks: vec![crate::engine::diff_parser::DiffHunk {
                old_start: 1,
                old_count: 0,
                new_start: 1,
                new_count: 0,
                header: "@@ -1 +1 @@".to_string(),
                lines: lines
                    .iter()
                    .map(|(marker, content)| crate::engine::diff_parser::DiffLine {
                        line_type: match *marker {
                            "+" => DiffLineType::Add,
                            "-" => DiffLineType::Remove,
                            _ => DiffLineType::Context,
                        },
                        content: content.to_string(),
                        old_line_no: None,
                        new_line_no: None,
                    })
                    .collect(),
            }],
        }
    }

    #[test]
    fn signature_drift_against_stale_index_is_not_a_removal() {
        // The reported FP (#533): only the signature line changed, so the old
        // definition shows up as a `-` line while the same symbol is re-added.
        let bridge = index_with_callers("build_review_prompt", &[("handler_a", "src/api.rs", 42)]);
        let chunks = vec![chunk_lines(
            "src/engine/llm.rs",
            &[
                ("-", "pub fn build_review_prompt(diff: &str) -> String {"),
                (
                    "+",
                    "pub fn build_review_prompt(diff: &str, scope: &str) -> String {",
                ),
            ],
        )];
        let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]);
        assert!(
            findings.is_empty(),
            "signature-only drift must not be reported as removal, got: {:?}",
            findings
        );
    }

    #[test]
    fn genuine_removal_with_callers_still_fires() {
        let bridge = index_with_callers("important_api", &[("caller_x", "src/app.rs", 7)]);
        let chunks = vec![chunk_lines(
            "src/lib.rs",
            &[("-", "pub fn important_api() {}")],
        )];
        let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]);
        assert_eq!(findings.len(), 1, "a true removal must still be reported");
        assert_eq!(findings[0].rule_id, "index-breaking-change");
        assert_eq!(findings[0].severity, Severity::Major);
    }

    #[test]
    fn rename_reports_only_the_old_name() {
        let bridge = index_with_callers("old_name", &[("caller_y", "src/app.rs", 3)]);
        let chunks = vec![chunk_lines(
            "src/lib.rs",
            &[("-", "pub fn old_name() {}"), ("+", "pub fn new_name() {}")],
        )];
        let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]);
        assert_eq!(findings.len(), 1, "rename is still breaking for old_name");
        assert!(findings[0].title.contains("old_name"));
    }

    #[test]
    fn cross_file_move_is_not_a_removal() {
        let bridge = index_with_callers("moved_fn", &[("caller_z", "src/main.rs", 11)]);
        let chunks = vec![
            chunk_lines("src/old_location.rs", &[("-", "pub fn moved_fn() {}")]),
            chunk_lines("src/new_location.rs", &[("+", "pub fn moved_fn() {}")]),
        ];
        let findings = scan_breaking_changes(&bridge, &chunks, 10, &[]);
        assert!(
            findings.is_empty(),
            "a definition moved between files still exists post-change, got: {:?}",
            findings
        );
    }

    #[test]
    fn added_definitions_are_diff_global() {
        let chunks = vec![
            chunk_lines(
                "a.rs",
                &[("-", "pub fn gone() {}"), ("+", "pub fn kept_one() {}")],
            ),
            chunk_lines("b.py", &[("+", "def kept_two():"), ("+", "    pass")]),
            chunk_lines("c.rs", &[(" ", "pub fn unchanged_context() {}")]),
        ];
        let names = collect_added_definitions(&chunks);
        assert!(names.contains("kept_one"));
        assert!(names.contains("kept_two"));
        assert!(
            !names.contains("gone"),
            "removed line is not part of post-image"
        );
        assert!(
            !names.contains("unchanged_context"),
            "context lines are not additions"
        );
    }

    // --- should_skip_file tests ---

    #[test]
    fn scanner_run_from_subdirectory_sees_the_indexed_project() {
        // Index registered under the workspace root; the review runs from a
        // member directory. Both must resolve the same project_id, otherwise
        // the scanner sees an empty project and reports nothing.
        let dir = tempfile::tempdir().unwrap();
        let root = dir.path().join("repo");
        let member = root.join("crates/member");
        std::fs::create_dir_all(&member).unwrap();
        std::fs::create_dir_all(root.join(".git")).unwrap();
        std::fs::write(
            root.join("Cargo.toml"),
            "[workspace]\nmembers = [\"crates/member\"]\n",
        )
        .unwrap();
        std::fs::write(member.join("Cargo.toml"), "[package]\nname = \"member\"\n").unwrap();

        let from_root = index_at_root(&root, "important_api", &[("caller_x", "src/app.rs", 7)]);
        let (conn, root_pid, _) = from_root.into_strict_parts().unwrap();
        // Re-open the same connection as if started from the subdirectory.
        let from_sub = IndexBridge::from_connection(conn, &member).unwrap();
        assert_eq!(from_sub.project_id(), Some(root_pid));

        let chunks = vec![chunk_lines(
            "src/lib.rs",
            &[("-", "pub fn important_api() {}")],
        )];
        let findings = scan_breaking_changes(&from_sub, &chunks, 10, &[]);
        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", &[]));
        assert!(!should_skip_file("vitest.config.ts", &[]));
    }

    #[test]
    fn skip_exact_match() {
        let patterns = vec!["src/main.ts".into(), "src/index.ts".into()];
        assert!(should_skip_file("src/main.ts", &patterns));
        assert!(!should_skip_file("src/app.ts", &patterns));
    }

    #[test]
    fn skip_wildcard_suffix() {
        let patterns = vec!["*.config.ts".into()];
        assert!(should_skip_file("vitest.config.ts", &patterns));
        assert!(should_skip_file("vite.config.ts", &patterns));
        assert!(should_skip_file("webpack.config.ts", &patterns));
        assert!(!should_skip_file("src/app.ts", &patterns));
        assert!(!should_skip_file("config.ts", &patterns)); // basename = "config.ts", doesn't end with ".config.ts"
    }

    #[test]
    fn skip_wildcard_prefix() {
        let patterns = vec!["vite.config.*".into()];
        assert!(should_skip_file("vite.config.ts", &patterns));
        assert!(should_skip_file("vite.config.js", &patterns));
        assert!(!should_skip_file("webpack.config.ts", &patterns));
    }

    #[test]
    fn skip_doublestar_prefix() {
        let patterns = vec!["**/main.ts".into()];
        assert!(should_skip_file("src/main.ts", &patterns));
        assert!(should_skip_file("main.ts", &patterns));
        assert!(!should_skip_file("src/app.ts", &patterns));
    }

    #[test]
    fn skip_basename_exact() {
        // "src/main.ts" pattern — basename match works too
        let patterns = vec!["main.ts".into()];
        assert!(should_skip_file("src/main.ts", &patterns));
        assert!(should_skip_file("main.ts", &patterns));
        assert!(!should_skip_file("src/app.ts", &patterns));
    }

    #[test]
    fn skip_default_list_blocks_common_entry_points() {
        use crate::engine::rules::types::default_index_skip_files;
        let defaults = default_index_skip_files();

        // These should all be skipped
        assert!(should_skip_file("vitest.config.ts", &defaults));
        assert!(should_skip_file("vite.config.js", &defaults));
        assert!(should_skip_file("webpack.config.ts", &defaults));
        assert!(should_skip_file("src/main.ts", &defaults));
        assert!(should_skip_file("src/index.tsx", &defaults));
        assert!(should_skip_file("src/app.tsx", &defaults));

        // These should NOT be skipped
        assert!(!should_skip_file("src/lib.rs", &defaults));
        assert!(!should_skip_file("src/utils.ts", &defaults));
        assert!(!should_skip_file("src/components/Button.tsx", &defaults));
    }

    // --- New glob pattern tests (#453) ---

    #[test]
    fn skip_doublestar_dir_doublestar() {
        // "**/phaser/**" → match any path containing "phaser" as a component
        let patterns = vec!["**/phaser/**".into()];
        assert!(should_skip_file(
            "src/scenes/phaser/GameScene.ts",
            &patterns
        ));
        assert!(should_skip_file("phaser/GameScene.ts", &patterns));
        assert!(should_skip_file("src/phaser/utils.ts", &patterns));
        assert!(!should_skip_file("src/utils/phaserHelper.ts", &patterns));
        assert!(!should_skip_file("src/app.ts", &patterns));
    }

    #[test]
    fn skip_prefix_dir_doublestar() {
        // "src/engine/**" → files under src/engine/
        let patterns = vec!["src/engine/**".into()];
        assert!(should_skip_file("src/engine/physics.rs", &patterns));
        assert!(should_skip_file("src/engine/core/mod.rs", &patterns));
        assert!(!should_skip_file("src/app/engine.rs", &patterns));
    }

    #[test]
    fn skip_doublestar_wildcard_ext() {
        // "**/*.test.ts" → any .test.ts file anywhere
        let patterns = vec!["**/*.test.ts".into()];
        assert!(should_skip_file("src/utils/helpers.test.ts", &patterns));
        assert!(should_skip_file("tests/integration.test.ts", &patterns));
        assert!(!should_skip_file("src/utils/helpers.ts", &patterns));
        assert!(!should_skip_file("src/utils/helpers.test.js", &patterns));
    }

    #[test]
    fn skip_nested_doublestar_dir() {
        // "**/node_modules/**" → match nested node_modules
        let patterns = vec!["**/node_modules/**".into()];
        assert!(should_skip_file("node_modules/lodash/index.js", &patterns));
        assert!(should_skip_file(
            "packages/app/node_modules/lodash/index.js",
            &patterns
        ));
        assert!(!should_skip_file("src/node_modulesHelper.ts", &patterns));
    }

    #[test]
    fn skip_multiple_patterns_any_match() {
        let patterns = vec![
            "**/phaser/**".into(),
            "*.config.ts".into(),
            "src/main.ts".into(),
        ];
        assert!(should_skip_file(
            "src/scenes/phaser/GameScene.ts",
            &patterns
        ));
        assert!(should_skip_file("vite.config.ts", &patterns));
        assert!(should_skip_file("src/main.ts", &patterns));
        assert!(!should_skip_file("src/lib.rs", &patterns));
    }
}