diff --git a/src/commands/scan.rs b/src/commands/scan.rs index 2aa4e04..66f4f73 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -118,22 +118,8 @@ pub async fn execute_scan( // 1b. Incremental: filter out unchanged files if opts.incremental { - let cache = ScanCache::load()?; let before_count = files.len(); - let root_abs = root.canonicalize().unwrap_or_else(|_| root.clone()); - files.retain(|f| { - let abs_path = root_abs.join(&f.path); - let Some(hash) = file_content_hash(&abs_path) else { - return true; // can't read file, rescan it - }; - match cache.get(&root_abs, &f.path) { - Some(cached_hash) if cached_hash == hash => { - debug!(file = %f.path, "skipping unchanged file (incremental)"); - false - } - _ => true, - } - }); + retain_changed(&root, &mut files)?; let skipped = before_count - files.len(); if skipped > 0 { eprintln!( @@ -276,16 +262,7 @@ pub async fn execute_scan( // 6. Save scan cache for incremental mode if opts.incremental { - let root_abs = root.canonicalize().unwrap_or_else(|_| root.clone()); - let mut cache = ScanCache::load().unwrap_or_default(); - for f in &files { - let abs_path = root_abs.join(&f.path); - let Some(hash) = file_content_hash(&abs_path) else { - continue; // can't read file, skip cache entry - }; - cache.set(&root_abs, &f.path, &hash); - } - cache.save()?; + record_scanned(&root, &files)?; debug!(cached = files.len(), "saved scan cache"); } @@ -435,6 +412,37 @@ pub(crate) fn finalize_issues( (report.issues, report.dropped) } +/// Incremental mode: drop files whose content hash matches the scan cache. +pub(crate) fn retain_changed(root: &std::path::Path, files: &mut Vec) -> Result<()> { + let cache = ScanCache::load()?; + let root_abs = root.canonicalize().unwrap_or_else(|_| root.to_path_buf()); + files.retain(|f| { + let Some(hash) = file_content_hash(&root_abs.join(&f.path)) else { + return true; // can't read file, rescan it + }; + match cache.get(&root_abs, &f.path) { + Some(cached_hash) if cached_hash == hash => { + debug!(file = %f.path, "skipping unchanged file (incremental)"); + false + } + _ => true, + } + }); + Ok(()) +} + +/// Incremental mode: remember the hashes of the files just scanned. +pub(crate) fn record_scanned(root: &std::path::Path, files: &[FileEntry]) -> Result<()> { + let root_abs = root.canonicalize().unwrap_or_else(|_| root.to_path_buf()); + let mut cache = ScanCache::load().unwrap_or_default(); + for f in files { + if let Some(hash) = file_content_hash(&root_abs.join(&f.path)) { + cache.set(&root_abs, &f.path, &hash); + } + } + cache.save() +} + /// Compute a short SHA256 hash of a file's content for incremental scanning. /// Returns None if the file cannot be read (caller should rescan it). #[allow(clippy::format_collect)] diff --git a/src/engine/scanner.rs b/src/engine/scanner.rs index 84b7ea6..b4e0404 100644 --- a/src/engine/scanner.rs +++ b/src/engine/scanner.rs @@ -163,6 +163,25 @@ pub fn walk_project( Ok(entries) } +/// Extensions that are program code (the rest of the defaults are docs/data). +const SOURCE_EXTENSIONS: &[&str] = &[ + "rs", "py", "js", "ts", "tsx", "jsx", "go", "java", "kt", "rb", "c", "cpp", "h", "hpp", "cs", + "php", "swift", "scala", "vue", "svelte", "sh", "bash", "zsh", "ps1", "sql", +]; + +/// Value-first order: source files before docs/data/lockfiles, then shallower +/// directories first, then by path so the result is deterministic. +pub fn order_source_first(files: &mut [FileEntry]) { + files.sort_by_cached_key(|f| { + let p = Path::new(&f.path); + let is_source = p + .extension() + .and_then(|e| e.to_str()) + .is_some_and(|e| SOURCE_EXTENSIONS.contains(&e.to_lowercase().as_str())); + (!is_source, p.components().count(), f.path.clone()) + }); +} + /// Split files into batches to avoid exceeding LLM token limits. pub fn batch_files(files: &[FileEntry], max_chars: usize, max_files: usize) -> Vec> { let mut batches = Vec::new(); diff --git a/src/mcp/agent_tools.rs b/src/mcp/agent_tools.rs index 33a3d14..68aef1c 100644 --- a/src/mcp/agent_tools.rs +++ b/src/mcp/agent_tools.rs @@ -27,7 +27,9 @@ use crate::engine::judge::{AgentJudge, admit}; use crate::engine::llm; use crate::engine::postprocess::Source; use crate::engine::review; -use crate::engine::scanner::{FileEntry, batch_files, walk_project}; +use crate::engine::scanner::{ + FileEntry, batch_files, format_batch_for_prompt, order_source_first, walk_project, +}; use crate::engine::types::{ReviewIssue, Severity}; use super::protocol::{Tool, ToolResult}; @@ -94,23 +96,35 @@ pub fn tools() -> Vec { }, Tool { name: "cora.scan_prepare".to_string(), - description: "Agent-as-reviewer scan, step 1. Prepare one batch of project files for you to review with your own model (no LLM in cora). Page through batches with `batch` (0-based) until `batch` reaches total_batches - 1; deterministic findings are returned with batch 0. Then call cora.scan_finalize once with all your findings. The path must be inside the current project.".to_string(), + description: "Agent-as-reviewer scan, step 1. Prepare one batch of project files for you to review with your own model (no LLM in cora). Page through batches with `batch` (0-based) until `batch` reaches total_batches - 1; deterministic findings are returned with batch 0. Optional include/exclude/extensions/incremental/changed_since select files; `order` defaults to source-first. Every response carries total_chars, estimated_tokens and per-batch sizes for planning. Then call cora.scan_finalize once with all your findings. The path must be inside the current project.".to_string(), input_schema: json!({ "type": "object", "properties": { "path": { "type": "string", "description": "Directory to scan, inside the current working directory (default: the working directory)" }, - "batch": { "type": "integer", "description": "0-based batch index (default 0)" } + "batch": { "type": "integer", "description": "0-based batch index (default 0)" }, + "include": { "type": "array", "items": { "type": "string" }, "description": "Only scan files matching these globs, relative to path (e.g. [\"crates/**/*.rs\"])" }, + "exclude": { "type": "array", "items": { "type": "string" }, "description": "Skip files matching these globs, in addition to the config's ignore.files" }, + "extensions": { "type": "array", "items": { "type": "string" }, "description": "Extra file extensions to scan, beyond the defaults" }, + "incremental": { "type": "boolean", "description": "Skip files unchanged since the last completed incremental scan (same cache as `cora scan --incremental`; scan_finalize records them)" }, + "changed_since": { "type": "string", "description": "Git revision: only scan files changed since it (`git diff ` plus untracked files)" }, + "order": { "type": "string", "enum": ["source-first", "path"], "description": "Batch order: source-first (default; code before docs/data, shallower first) or path (walk order)" } }, "required": [] }), }, Tool { name: "cora.scan_finalize".to_string(), - description: "Agent-as-reviewer scan, step 2. Merge all your findings from every scan_prepare batch with cora's deterministic findings, validate them (untrusted input), apply ignore rules and inline markers, and return the verdict. Use the same path as scan_prepare. No LLM call.".to_string(), + description: "Agent-as-reviewer scan, step 2. Merge all your findings from every scan_prepare batch with cora's deterministic findings, validate them (untrusted input), apply ignore rules and inline markers, and return the verdict. Use the same path and the same selection inputs (include, exclude, extensions, incremental, changed_since, order) as scan_prepare, or the call is refused via files_fingerprint. No LLM call.".to_string(), input_schema: json!({ "type": "object", "properties": { "path": { "type": "string", "description": "The same path given to cora.scan_prepare" }, + "include": { "type": "array", "items": { "type": "string" }, "description": "Only scan files matching these globs, relative to path (e.g. [\"crates/**/*.rs\"])" }, + "exclude": { "type": "array", "items": { "type": "string" }, "description": "Skip files matching these globs, in addition to the config's ignore.files" }, + "extensions": { "type": "array", "items": { "type": "string" }, "description": "Extra file extensions to scan, beyond the defaults" }, + "incremental": { "type": "boolean", "description": "Skip files unchanged since the last completed incremental scan (same cache as `cora scan --incremental`; scan_finalize records them)" }, + "changed_since": { "type": "string", "description": "Git revision: only scan files changed since it (`git diff ` plus untracked files)" }, + "order": { "type": "string", "enum": ["source-first", "path"], "description": "Batch order: source-first (default; code before docs/data, shallower first) or path (walk order)" }, "files_fingerprint": { "type": "string", "description": "files_fingerprint from cora.scan_prepare; if given and the project changed since, the call is refused" }, "batches_judged": { "type": "array", "items": { "type": "integer" }, "description": "0-based indexes of the scan_prepare batches you actually judged. Without it the result reports coverage as unverified; with fewer than total_batches it is partial and should_block is null" }, "issues": issues_prop @@ -399,12 +413,107 @@ fn files_fingerprint(files: &[FileEntry]) -> String { .collect() } -fn scan_files_for( +/// The CLI's selection inputs, shared by `scan_prepare` and `scan_finalize` so +/// both compute the same file set and batch layout. +struct ScanSelection { + include: Vec, + exclude: Vec, + extensions: Vec, + incremental: bool, + changed_since: Option, + source_first: bool, +} + +fn str_array(params: &Value, key: &str) -> Result, ToolResult> { + match params.get(key) { + None | Some(Value::Null) => Ok(Vec::new()), + Some(Value::Array(a)) => a + .iter() + .map(|v| v.as_str().map(str::to_string)) + .collect::>() + .ok_or_else(|| ToolResult::error(format!("{key} must be an array of strings"))), + Some(_) => Err(ToolResult::error(format!( + "{key} must be an array of strings" + ))), + } +} + +fn scan_selection( + params: &Value, config: &crate::config::schema::Config, - root: &Path, -) -> Result, ToolResult> { - walk_project(root, &[], &config.ignore.files, &[]) - .map_err(|e| ToolResult::error(format!("Scan failed: {e}"))) +) -> Result { + // Same merge as the CLI: config ignore.files + the caller's excludes. + let mut exclude = config.ignore.files.clone(); + exclude.extend(str_array(params, "exclude")?); + let changed_since = match params.get("changed_since") { + None | Some(Value::Null) => None, + // A leading '-' would be read by git as an option. + Some(Value::String(r)) if !r.is_empty() && !r.starts_with('-') => Some(r.clone()), + Some(_) => return Err(ToolResult::error("changed_since must be a git revision")), + }; + let source_first = match params.get("order").and_then(Value::as_str) { + None | Some("source-first") => true, + Some("path") => false, + Some(o) => { + return Err(ToolResult::error(format!( + "order must be \"source-first\" or \"path\", got \"{o}\"" + ))); + } + }; + Ok(ScanSelection { + include: str_array(params, "include")?, + exclude, + extensions: str_array(params, "extensions")?, + incremental: params + .get("incremental") + .and_then(Value::as_bool) + .unwrap_or(false), + changed_since, + source_first, + }) +} + +/// Paths (relative to `root`) changed since `rev`: tracked changes plus untracked files. +fn changed_paths(root: &Path, rev: &str) -> Result, ToolResult> { + let git = |args: &[&str]| -> Result { + let out = std::process::Command::new("git") + .args(args) + .current_dir(root) + .output() + .map_err(|e| ToolResult::error(format!("Cannot run git: {e}")))?; + if !out.status.success() { + return Err(ToolResult::error(format!( + "git {} failed: {}", + args[0], + String::from_utf8_lossy(&out.stderr).trim() + ))); + } + Ok(String::from_utf8_lossy(&out.stdout).into_owned()) + }; + let diff = git(&["diff", "--name-only", "--relative", rev, "--"])?; + let untracked = git(&["ls-files", "--others", "--exclude-standard"])?; + Ok(diff + .lines() + .chain(untracked.lines()) + .map(str::to_string) + .collect()) +} + +fn scan_files_for(sel: &ScanSelection, root: &Path) -> Result, ToolResult> { + let mut files = walk_project(root, &sel.include, &sel.exclude, &sel.extensions) + .map_err(|e| ToolResult::error(format!("Scan failed: {e}")))?; + if let Some(rev) = &sel.changed_since { + let changed = changed_paths(root, rev)?; + files.retain(|f| changed.contains(&f.path)); + } + if sel.incremental { + crate::commands::scan::retain_changed(root, &mut files) + .map_err(|e| ToolResult::error(format!("Incremental scan failed: {e}")))?; + } + if sel.source_first { + order_source_first(&mut files); + } + Ok(files) } pub fn scan_prepare(params: &Value) -> ToolResult { @@ -418,10 +527,19 @@ pub fn scan_prepare(params: &Value) -> ToolResult { }; // Scanned paths are relative to the scan root (#682). config.rules_config.project_root = Some(root.clone()); - let files = match scan_files_for(&config, &root) { + let sel = match scan_selection(params, &config) { + Ok(s) => s, + Err(e) => return e, + }; + let files = match scan_files_for(&sel, &root) { Ok(f) => f, Err(e) => return e, }; + let order = if sel.source_first { + "source-first" + } else { + "path" + }; let batches = batch_files( &files, crate::commands::scan::DEFAULT_BATCH_CHAR_BUDGET, @@ -432,6 +550,9 @@ pub fn scan_prepare(params: &Value) -> ToolResult { return json_result(&json!({ "mode": "agent", "total_batches": 0, + "total_chars": 0, + "estimated_tokens": 0, + "order": order, "note": "No files to scan.", })); } @@ -442,8 +563,16 @@ pub fn scan_prepare(params: &Value) -> ToolResult { )); }; + // Planning data: the formatted size of every batch, so the agent can decide + // how much to review before paying for it. + let batch_chars: Vec = batches + .iter() + .map(|b| format_batch_for_prompt(b).len()) + .collect(); + let total_chars: usize = batch_chars.iter().sum(); + let prompts = llm::scan_prompts( - &crate::engine::scanner::format_batch_for_prompt(batch), + &format_batch_for_prompt(batch), &config.focus, &config.rules, None, @@ -468,6 +597,14 @@ pub fn scan_prepare(params: &Value) -> ToolResult { "instructions": "Answer user_prompt following system_prompt with your own model. Return ONLY a JSON array shaped like response_schema (use [] if there are no issues), reporting only the files listed in `files`. Do this for every batch, collect all findings, then call cora.scan_finalize once with them as `issues`.", "batch": batch_idx, "total_batches": batches.len(), + "total_chars": total_chars, + "estimated_tokens": total_chars / 4, + "order": order, + "include": sel.include, + "exclude": sel.exclude, + "batches": batches.iter().zip(&batch_chars).enumerate().map(|(i, (b, c))| json!({ + "batch": i, "files": b.len(), "chars": c, + })).collect::>(), "files_fingerprint": files_fingerprint(&files), "files": batch.iter().map(|f| f.path.clone()).collect::>(), "system_prompt": prompts.system, @@ -544,7 +681,11 @@ pub fn scan_finalize(params: &Value) -> ToolResult { }; // Scanned paths are relative to the scan root (#682). config.rules_config.project_root = Some(root.clone()); - let files = match scan_files_for(&config, &root) { + let sel = match scan_selection(params, &config) { + Ok(s) => s, + Err(e) => return e, + }; + let files = match scan_files_for(&sel, &root) { Ok(f) => f, Err(e) => return e, }; @@ -578,6 +719,12 @@ pub fn scan_finalize(params: &Value) -> ToolResult { ci: false, }, ); + // Incremental: remember what a complete judgement covered, as the CLI does. + if sel.incremental && coverage.label == "complete" { + if let Err(e) = crate::commands::scan::record_scanned(&root, &files) { + tracing::debug!(error = %e, "could not save the incremental scan cache"); + } + } let total_lines: usize = files.iter().map(|f| f.lines).sum(); let summary = match &coverage.files_judged { Some(j) if coverage.label != "complete" => format!( @@ -999,4 +1146,171 @@ mod tests { .contains("max_findings") ); } + + // ─── #686: selection inputs, ordering, planning data ─── + + /// A scratch project inside the working directory (scan paths must be). + fn fixture(files: &[(&str, &str)]) -> tempfile::TempDir { + let dir = tempfile::Builder::new() + .prefix(".scanfx") + .tempdir_in(".") + .unwrap(); + for (path, content) in files { + let full = dir.path().join(path); + std::fs::create_dir_all(full.parent().unwrap()).unwrap(); + std::fs::write(full, content).unwrap(); + } + dir + } + + fn mixed() -> tempfile::TempDir { + fixture(&[ + ("README.md", "# readme\n"), + ("Cargo.toml", "[package]\n"), + ("data.json", "{\"a\": 1}\n"), + ("crates/a/src/lib.rs", "fn a() {}\n"), + ("crates/a/tests/t.rs", "fn t() {}\n"), + ("crates/b/deep/er/x.rs", "fn x() {}\n"), + ("main.py", "x = 1\n"), + ]) + } + + fn prep(dir: &tempfile::TempDir, extra: Value) -> Value { + let mut p = json!({ "path": dir.path().to_str().unwrap() }); + p.as_object_mut() + .unwrap() + .extend(extra.as_object().unwrap().clone()); + as_json(&scan_prepare(&p)) + } + + fn names(v: &Value) -> Vec { + v["files"] + .as_array() + .unwrap() + .iter() + .map(|f| f.as_str().unwrap().to_string()) + .collect() + } + + #[test] + fn include_exclude_filter_and_planning_data_is_reported() { + let dir = mixed(); + let out = prep( + &dir, + json!({ "include": ["crates/**/*.rs"], "exclude": ["**/tests/**"] }), + ); + assert_eq!( + names(&out), + ["crates/a/src/lib.rs", "crates/b/deep/er/x.rs"] + ); + assert_eq!(out["order"], "source-first"); + assert_eq!(out["include"], json!(["crates/**/*.rs"])); + assert!( + out["exclude"] + .as_array() + .unwrap() + .contains(&json!("**/tests/**")) + ); + let total = out["total_chars"].as_u64().unwrap(); + assert!(total > 0); + assert_eq!(out["estimated_tokens"].as_u64().unwrap(), total / 4); + let max = out["batches"] + .as_array() + .unwrap() + .iter() + .map(|b| b["chars"].as_u64().unwrap()) + .max() + .unwrap(); + assert!(total >= max); + assert_eq!(out["batches"][0]["files"], 2); + } + + #[test] + fn source_files_come_first_then_shallower_and_order_is_deterministic() { + let dir = mixed(); + let a = names(&prep(&dir, json!({}))); + assert_eq!( + a, + [ + "main.py", + "crates/a/src/lib.rs", + "crates/a/tests/t.rs", + "crates/b/deep/er/x.rs", + "Cargo.toml", + "README.md", + "data.json", + ] + ); + assert_eq!(a, names(&prep(&dir, json!({ "order": "source-first" })))); + let legacy = prep(&dir, json!({ "order": "path" })); + assert_eq!(legacy["order"], "path"); + assert!(scan_prepare(&json!({ "order": "bogus" })).is_error); + } + + #[test] + fn changed_since_returns_only_changed_files() { + let dir = mixed(); + let git = |args: &[&str]| { + let st = std::process::Command::new("git") + .args(["-c", "user.name=t", "-c", "user.email=t@t"]) + .args(args) + .current_dir(dir.path()) + .output() + .unwrap(); + assert!( + st.status.success(), + "{}", + String::from_utf8_lossy(&st.stderr) + ); + }; + git(&["init", "-q"]); + git(&["add", "."]); + git(&["commit", "-q", "-m", "init"]); + std::fs::write(dir.path().join("main.py"), "x = 2\n").unwrap(); + std::fs::write(dir.path().join("new.rs"), "fn n() {}\n").unwrap(); + let out = prep(&dir, json!({ "changed_since": "HEAD" })); + assert_eq!(names(&out), ["main.py", "new.rs"]); + assert!(scan_prepare(&json!({ "changed_since": "--output=x" })).is_error); + } + + #[test] + fn finalize_agrees_with_prepare_only_for_the_same_filters() { + let dir = mixed(); + let sel = json!({ "include": ["crates/**/*.rs"], "order": "path" }); + let out = prep(&dir, sel.clone()); + let fp = out["files_fingerprint"].as_str().unwrap(); + let path = dir.path().to_str().unwrap(); + let mut same = json!({ "path": path, "issues": [], "files_fingerprint": fp }); + same.as_object_mut() + .unwrap() + .extend(sel.as_object().unwrap().clone()); + assert!(!scan_finalize(&same).is_error); + let other = json!({ "path": path, "issues": [], "files_fingerprint": fp }); + let r = scan_finalize(&other); + assert!(r.is_error); + assert!(text(&r).contains("changed since")); + let reordered = json!({ "path": path, "issues": [], "files_fingerprint": fp, + "include": ["crates/**/*.rs"] }); + assert!(scan_finalize(&reordered).is_error); + } + + #[test] + fn schemas_list_the_selection_properties() { + for t in tools().iter().filter(|t| t.name.starts_with("cora.scan_")) { + for k in [ + "include", + "exclude", + "extensions", + "incremental", + "changed_since", + "order", + ] { + assert!( + t.input_schema["properties"].get(k).is_some(), + "{} lacks {k}", + t.name + ); + } + } + } }