diff --git a/Cargo.lock b/Cargo.lock index 01cfc31..c25f7da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -767,9 +767,9 @@ dependencies = [ [[package]] name = "h2" -version = "0.4.12" +version = "0.4.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f3c0b69cfcb4e1b9f1bf2f53f95f766e4661169728ec61cd3fe5a0166f2d1386" +checksum = "839c0e8a181239723652be9062bb56ca5bf5f64011f73b623f6f4fc59086a228" dependencies = [ "atomic-waker", "bytes", diff --git a/src/incremental.rs b/src/incremental.rs new file mode 100644 index 0000000..5899af2 --- /dev/null +++ b/src/incremental.rs @@ -0,0 +1,492 @@ +//! Incremental scans: upload whole project, analyze only what changed. +//! +//! The server already does this, but it derives the diff through the project's +//! SCM integration, which leaves out every project that integration cannot +//! answer for: zip-only projects, unreachable self-hosted hosts, unpushed +//! commits. This module diffs in the clone the scan already reads from. +//! +//! Runs by default, so it must be safe on a repo never set up for it. Every +//! refusal falls through to the full scan that run would have done anyway. +//! `--disable-incremental` forces it. +//! +//! `base_sha` travels with the file list because the server carries findings +//! forward for every file the list omits. Copy from a different baseline than +//! the one diffed here and files changed between the two keep stale findings, +//! reported as current. The server copies from exactly this scan, or refuses. +//! +//! The archive is unchanged. Fusion reads unchanged files for cross-file +//! context, and a finding can only carry forward for a file the archive still +//! holds. Analysis shrinks, not the upload. + +use crate::config::Config; +use crate::scanners::blast::{classify_scan_status, ScanState}; +use crate::utils::api::{self, ScanResponse}; +use git2::Repository; +use std::collections::BTreeSet; + +/// How many of the project's scans to read at a time, newest first. +const SCAN_LOOKUP_PAGE_SIZE: u16 = 30; + +/// Backstop on pages walked looking for a baseline. +/// +/// The server filters out scans that cannot be a baseline, so the answer is +/// normally the first entry of page one and this never iterates. Kept for a +/// backend predating those filters: it ignores unknown parameters and returns +/// scans of every kind, so heavy pull-request traffic can fill a page with +/// nothing usable. +const SCAN_LOOKUP_MAX_PAGES: u16 = 3; + +/// Engine every blast scan carries. An uploaded third-party report describes +/// someone else's analysis and cannot be a baseline for ours. +const BLAST_ENGINE: &str = "corgea-blast"; + +/// Payload guard, not policy. The server applies the real ceiling +/// (`INCREMENTAL_SCAN_MAX_FILES`, 300) and falls back to a full scan above it. +/// This only avoids building a multi-megabyte form field to be refused. +const MAX_CHANGED_FILES: usize = 5_000; + +/// A diff the server can turn into an incremental scan. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct IncrementalPlan { + /// Commit this diff was measured from. The server carries its findings + /// forward for every file the diff does not name. + pub base_sha: String, + /// Repo-relative paths differing between `base_sha` and the commit being + /// scanned, including deletions and both sides of a rename. + pub changed_files: Vec, +} + +/// What an incremental scan of this commit would cover, or `None` to scan +/// everything. +pub fn resolve_incremental_plan( + config: &Config, + project_name: &str, + branch: Option<&str>, + head_sha: Option<&str>, + worktree_dirty: bool, +) -> Option { + // A commit-to-commit diff cannot see uncommitted edits, so a dirty tree + // leaves modified files off the list and their old findings copied forward + // as current. The server enforces this too; repeated here so the run says + // why before paying for the upload. + if worktree_dirty { + explain_full_scan( + "this worktree has uncommitted changes, and a commit-to-commit diff cannot see them", + ); + return None; + } + + // Nothing to diff from. Covers a non-git directory, a repo with no commit, + // a detached HEAD, and a scan started below the repo root — none of which + // report RepoInfo to the upload either. + let (Some(branch), Some(head_sha)) = (branch, head_sha) else { + explain_full_scan( + "no git branch and commit to diff from (not a git repository, no commit \ + yet, a detached HEAD, or a scan started below the repository root)", + ); + return None; + }; + + let Some(base_sha) = find_baseline_sha(config, project_name, branch) else { + explain_full_scan(&format!( + "no earlier completed scan of a clean worktree was found for project '{project_name}', \ + so there is nothing to diff against" + )); + return None; + }; + + let repo = match Repository::discover(".") { + Ok(repo) => repo, + Err(e) => { + explain_full_scan(&format!("this directory is not a git repository ({e})")); + return None; + } + }; + + let changed_files = match changed_files_between(&repo, &base_sha, head_sha) { + Ok(files) => files, + Err(reason) => { + explain_full_scan(&reason); + return None; + } + }; + + if changed_files.len() > MAX_CHANGED_FILES { + explain_full_scan(&format!( + "{} files changed since {}, which is more than an incremental scan is worth", + changed_files.len(), + short_sha(&base_sha) + )); + return None; + } + + match changed_files.len() { + 0 => println!( + "Incremental scan: nothing changed since commit {}. Corgea will carry every \ + finding forward.", + short_sha(&base_sha) + ), + count => println!( + "Incremental scan: {} file(s) changed since commit {}. Corgea will analyze those \ + and carry findings forward for the rest.", + count, + short_sha(&base_sha) + ), + } + + Some(IncrementalPlan { + base_sha, + changed_files, + }) +} + +/// Say why this run scans everything. Never fatal — a full scan is correct, +/// only slower, so the run continues. +fn explain_full_scan(reason: &str) { + println!("Scanning every file: {reason}."); +} + +/// Commit of the newest scan this project can be diffed against. +/// +/// Prefers the branch being scanned, falls back to the newest usable scan on +/// any branch, mirroring doghouse's own baseline order +/// (`ScanManager._try_incremental_scan`). That fallback is what makes a feature +/// branch's first scan incremental against trunk instead of full. +fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Option { + let url = config.get_url(); + let mut any_branch_fallback: Option = None; + + for page in 1..=SCAN_LOOKUP_MAX_PAGES { + let response = match api::query_baseline_scans( + &url, + project_name, + BLAST_ENGINE, + page, + SCAN_LOOKUP_PAGE_SIZE, + ) { + Ok(response) => response, + Err(e) => { + // A failed lookup proves nothing about the project's history, + // so it means full scan, not error. + crate::log::debug(&format!("Baseline scan lookup failed: {e}")); + return any_branch_fallback; + } + }; + + let scans = response.scans.unwrap_or_default(); + if scans.is_empty() { + break; + } + + // Newest first, so the first same-branch match is the best available + // and no later page can improve on it. + if let Some(scan) = usable_baselines(&scans) + .find(|scan| scan.branch.as_deref().is_some_and(|b| b == branch)) + { + return scan.git_sha.clone(); + } + if any_branch_fallback.is_none() { + any_branch_fallback = usable_baselines(&scans) + .next() + .and_then(|s| s.git_sha.clone()); + } + + if response + .total_pages + .is_some_and(|total| u32::from(page) >= total) + { + break; + } + } + + any_branch_fallback +} + +/// Scans on one page that can serve as a baseline, newest first. +fn usable_baselines(scans: &[ScanResponse]) -> impl Iterator { + scans.iter().filter(|scan| is_usable_baseline(scan)) +} + +/// Whether `scan` may be diffed against. +/// +/// Client-side half of the filter doghouse applies picking a baseline itself: a +/// completed blast scan of a whole, clean, non-pull-request commit. +/// `worktree_dirty` must be an explicit `false` — `None` means never reported, +/// and unknown scope is not clean, so the server rejects it as a baseline too. +/// +/// `query_baseline_scans` asks the server for exactly these, which keeps the +/// page walk from iterating. This stays because a backend predating those +/// parameters ignores them, and a dirty or pull-request scan's commit would +/// diff against the wrong tree. +fn is_usable_baseline(scan: &ScanResponse) -> bool { + classify_scan_status(&scan.status) == ScanState::Completed + && scan.engine.eq_ignore_ascii_case(BLAST_ENGINE) + && scan.pull_request_id.is_none() + && scan.worktree_dirty == Some(false) + && scan.git_sha.as_deref().is_some_and(|sha| !sha.is_empty()) +} + +/// Every repo-relative path differing between two commits. +/// +/// Both sides of every delta, no status filtered out, because the list decides +/// which findings are *not* carried forward. A deleted file left off keeps its +/// findings in a tree no longer holding it; a rename is a delete plus an add +/// whose old path needs the same. `--target`'s `git:diff=` selector wants the +/// opposite — paths still on disk, to archive — hence no reuse. +/// +/// Untracked files are not a gap: they make the worktree dirty, already +/// refused above. +/// +/// Submodules are the one thing this cannot describe. A committed pointer bump +/// is one gitlink delta naming the submodule directory, while packaging walks +/// into it and uploads the files inside, so those files would be missing from +/// the list and keep old findings. Diffing the two submodule commits means +/// opening a repo that may not be checked out, so this fails closed. +fn changed_files_between( + repo: &Repository, + base_sha: &str, + head_sha: &str, +) -> Result, String> { + let base_tree = commit_tree(repo, base_sha).map_err(|e| { + format!( + "commit {}, the one the last scan covered, is not in this clone ({e}). A shallow \ + clone cannot diff against it — fetch more history (for example `actions/checkout` \ + with `fetch-depth: 0`) to scan incrementally", + short_sha(base_sha) + ) + })?; + let head_tree = commit_tree(repo, head_sha) + .map_err(|e| format!("commit {} could not be read ({e})", short_sha(head_sha)))?; + + let diff = repo + .diff_tree_to_tree(Some(&base_tree), Some(&head_tree), None) + .map_err(|e| format!("the diff against {} failed ({e})", short_sha(base_sha)))?; + + // Sorted and deduplicated: a rename reports one path per side, and stable + // order keeps the uploaded list reproducible for the same two commits. + let mut files = BTreeSet::new(); + for delta in diff.deltas() { + if delta.old_file().mode() == git2::FileMode::Commit + || delta.new_file().mode() == git2::FileMode::Commit + { + let name = delta + .new_file() + .path() + .or_else(|| delta.old_file().path()) + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_else(|| "a submodule".to_string()); + return Err(format!( + "submodule {name} moved to a different commit, and the diff names only \ + the submodule itself rather than the files inside it that this scan \ + uploads" + )); + } + for file in [delta.old_file(), delta.new_file()] { + if let Some(path) = file.path() { + let path = path.to_string_lossy().replace('\\', "/"); + if !path.is_empty() { + files.insert(path); + } + } + } + } + Ok(files.into_iter().collect()) +} + +fn commit_tree<'repo>( + repo: &'repo Repository, + rev: &str, +) -> Result, git2::Error> { + repo.revparse_single(rev)?.peel_to_commit()?.tree() +} + +/// First 7 characters, by char boundary rather than byte index. The value comes +/// from the API, so a non-ASCII one must shorten, not panic mid-scan. +fn short_sha(sha: &str) -> &str { + match sha.char_indices().nth(7) { + Some((byte, _)) => &sha[..byte], + None => sha, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use std::path::Path; + + fn scan(branch: &str, sha: &str) -> ScanResponse { + ScanResponse { + id: format!("scan-{sha}"), + project: "proj".to_string(), + repo: None, + branch: Some(branch.to_string()), + status: "complete".to_string(), + engine: BLAST_ENGINE.to_string(), + created_at: "2026-01-01T00:00:00Z".to_string(), + git_sha: Some(sha.to_string()), + worktree_dirty: Some(false), + pull_request_id: None, + metadata: None, + failed_reason: None, + scan_errors: Vec::new(), + } + } + + #[test] + fn short_sha_shortens_a_non_ascii_value_instead_of_panicking() { + // The API supplies git_sha; a malformed one must not kill the scan. + assert_eq!(short_sha("0123456789abcdef"), "0123456"); + assert_eq!(short_sha("abc"), "abc"); + assert_eq!(short_sha(""), ""); + assert_eq!(short_sha("ααααααααα"), "ααααααα"); + } + + #[test] + fn a_completed_clean_blast_scan_is_a_baseline() { + assert!(is_usable_baseline(&scan("main", "abc"))); + } + + #[test] + fn scans_that_cannot_describe_a_whole_clean_commit_are_rejected() { + // The server refuses each of these too, so diffing against them narrows + // a scan the server then widens. + let mut running = scan("main", "abc"); + running.status = "processing".to_string(); + assert!(!is_usable_baseline(&running)); + + let mut third_party = scan("main", "abc"); + third_party.engine = "semgrep".to_string(); + assert!(!is_usable_baseline(&third_party)); + + let mut pr = scan("main", "abc"); + pr.pull_request_id = Some("42".to_string()); + assert!(!is_usable_baseline(&pr)); + + let mut dirty = scan("main", "abc"); + dirty.worktree_dirty = Some(true); + assert!(!is_usable_baseline(&dirty)); + + // Never reported is not known clean. + let mut unknown = scan("main", "abc"); + unknown.worktree_dirty = None; + assert!(!is_usable_baseline(&unknown)); + + let mut no_commit = scan("main", "abc"); + no_commit.git_sha = None; + assert!(!is_usable_baseline(&no_commit)); + } + + #[test] + fn the_newest_usable_scan_wins_within_a_page() { + let scans = vec![scan("main", "newest"), scan("main", "older")]; + assert_eq!( + usable_baselines(&scans).next().unwrap().git_sha.as_deref(), + Some("newest") + ); + } + + /// Two commits: three files, then one that adds, edits and deletes. + fn repo_with_history() -> (tempfile::TempDir, Repository, String, String) { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = Repository::init(dir.path()).expect("init"); + let sig = git2::Signature::now("t", "t@example.com").expect("sig"); + + let commit_all = + |repo: &Repository, message: &str, parent: Option| -> git2::Oid { + let mut index = repo.index().expect("index"); + index + .add_all(["*"], git2::IndexAddOption::DEFAULT, None) + .expect("add"); + index.write().expect("write index"); + let tree = repo + .find_tree(index.write_tree().expect("tree")) + .expect("find tree"); + let parents: Vec = parent + .map(|oid| vec![repo.find_commit(oid).expect("parent")]) + .unwrap_or_default(); + let parent_refs: Vec<&git2::Commit> = parents.iter().collect(); + repo.commit(Some("HEAD"), &sig, &sig, message, &tree, &parent_refs) + .expect("commit") + }; + + let write = |name: &str, body: &str| { + fs::write(dir.path().join(name), body).expect("write file"); + }; + + write("keep.txt", "same"); + write("edit.txt", "before"); + write("gone.txt", "doomed"); + let base = commit_all(&repo, "base", None); + + write("edit.txt", "after"); + write("added.txt", "new"); + fs::remove_file(dir.path().join("gone.txt")).expect("remove"); + // add_all does not stage a deletion on its own. + let mut index = repo.index().expect("index"); + index + .remove_path(Path::new("gone.txt")) + .expect("stage delete"); + index.write().expect("write index"); + let head = commit_all(&repo, "head", Some(base)); + + (dir, repo, base.to_string(), head.to_string()) + } + + #[test] + fn the_diff_names_added_edited_and_deleted_files_but_not_untouched_ones() { + let (_dir, repo, base, head) = repo_with_history(); + let files = changed_files_between(&repo, &base, &head).expect("diff"); + // Deleted file must be listed, else its findings carry into a tree that + // no longer holds it. + assert_eq!(files, vec!["added.txt", "edit.txt", "gone.txt"]); + } + + #[test] + fn a_commit_diffed_against_itself_reports_nothing_changed() { + let (_dir, repo, _base, head) = repo_with_history(); + assert!(changed_files_between(&repo, &head, &head) + .expect("diff") + .is_empty()); + } + + /// Commit whose tree carries a `vendor` gitlink pointing at `target`. + fn commit_with_gitlink(repo: &Repository, parent: git2::Oid, target: git2::Oid) -> git2::Oid { + let sig = git2::Signature::now("t", "t@example.com").expect("sig"); + let parent_commit = repo.find_commit(parent).expect("parent"); + let mut builder = repo + .treebuilder(Some(&parent_commit.tree().expect("parent tree"))) + .expect("treebuilder"); + builder + .insert("vendor", target, i32::from(git2::FileMode::Commit)) + .expect("insert gitlink"); + let tree_oid = builder.write().expect("write tree"); + let tree = repo.find_tree(tree_oid).expect("find tree"); + repo.commit(None, &sig, &sig, "gitlink", &tree, &[&parent_commit]) + .expect("commit") + } + + #[test] + fn a_moved_submodule_pointer_refuses_the_diff() { + // Packaging uploads the files inside the submodule, but the diff names + // only `vendor`, so those files would keep unexamined findings. + let (_dir, repo, base, head) = repo_with_history(); + let base_oid = git2::Oid::from_str(&base).expect("base oid"); + let head_oid = git2::Oid::from_str(&head).expect("head oid"); + let before = commit_with_gitlink(&repo, base_oid, base_oid); + let after = commit_with_gitlink(&repo, before, head_oid); + + let err = changed_files_between(&repo, &before.to_string(), &after.to_string()) + .expect_err("a moved submodule must refuse the diff"); + + assert!(err.contains("submodule vendor"), "{err}"); + } + + #[test] + fn a_base_commit_this_clone_does_not_have_is_reported_not_panicked() { + let (_dir, repo, _base, head) = repo_with_history(); + let err = changed_files_between(&repo, &"0".repeat(40), &head) + .expect_err("unknown base must fail"); + assert!(err.contains("shallow clone"), "{err}"); + } +} diff --git a/src/main.rs b/src/main.rs index e2920e6..1e934c6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,6 +2,7 @@ mod authorize; mod cicd; mod config; mod images; +mod incremental; mod inspect; mod list; mod log; @@ -89,6 +90,12 @@ enum Commands { #[arg(long, help = "Only scan uncommitted changes.")] only_uncommitted: bool, + #[arg( + long = "disable-incremental", + help = "Analyze every file, even when Corgea could have analyzed only what changed. Scans are incremental by default: the whole project is still uploaded, but only files that changed since this project's last scan are analyzed, and unchanged files keep their existing findings, so the result is a full picture either way. Use this to force a fresh analysis of every file — after changing scanner configuration outside corgea.yaml, for example. Incremental is skipped on its own, with a reason, when there is no git repository or commit to diff from, when the worktree is dirty, when no earlier scan of a clean worktree exists, or when the last scanned commit is missing from a shallow clone; and silently when --only-uncommitted, --target or --exclude already narrow the upload." + )] + disable_incremental: bool, + #[arg( long = "metadata", value_name = "KEY=VALUE", @@ -669,6 +676,7 @@ fn main() { fail, block_on, only_uncommitted, + disable_incremental, metadata, scan_type, policy, @@ -710,6 +718,11 @@ fn main() { std::process::exit(1); } + if *disable_incremental && *scanner != Scanner::Blast { + ::log::error!("--disable-incremental is only supported with blast scanner."); + std::process::exit(1); + } + if !metadata.is_empty() && *scanner != Scanner::Blast { ::log::error!("--metadata is only supported with the blast scanner."); std::process::exit(1); @@ -846,6 +859,7 @@ fn main() { fail, block_on, only_uncommitted, + disable_incremental, metadata_json, scan_type.clone(), policy.clone(), diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index bbc5dc3..254910f 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -53,6 +53,7 @@ pub fn run( fail: &bool, block_on: Option, only_uncommitted: &bool, + disable_incremental: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -109,6 +110,7 @@ pub fn run( config, &project_name, only_uncommitted, + disable_incremental, metadata, scan_type, policy, @@ -281,6 +283,7 @@ fn start_new_scan( config: &Config, project_name: &str, only_uncommitted: &bool, + disable_incremental: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -513,15 +516,39 @@ fn start_new_scan( info.dirty = true; } } + // Incremental is the default, so this asks what took it off the table. A + // narrowed archive is the silent case: carrying findings forward for files + // the archive no longer holds would be wrong, but those runs are not + // "scanning every file" either, so no message is honest. + let narrowed_archive = target_str.is_some() || exclude.is_some(); + let incremental_plan = if *disable_incremental || narrowed_archive { + None + } else { + // Reconciled repo info, so a tree that turned out dirty — or a HEAD + // that moved mid-packaging — refuses rather than diffing against a + // commit this upload is not a snapshot of. + crate::incremental::resolve_incremental_plan( + config, + project_name, + repo_info.as_ref().and_then(|info| info.branch.as_deref()), + repo_info.as_ref().and_then(|info| info.sha.as_deref()), + // Missing repo info is not dirtiness; it is the missing + // branch/commit the resolver reports next, by its real name. + repo_info.as_ref().is_some_and(|info| info.dirty), + ) + }; println!("\n\nSubmitting scan to Corgea:"); let upload_result = match utils::api::upload_zip( &zip_path, &config.get_url(), project_name, repo_info, - scan_type, - policy, - metadata, + utils::api::UploadOptions { + scan_type, + policy, + metadata, + incremental: incremental_plan, + }, ) { Ok(result) => result, Err(e) => { diff --git a/src/skip_scan.rs b/src/skip_scan.rs index 7ec5ccd..098ad01 100644 --- a/src/skip_scan.rs +++ b/src/skip_scan.rs @@ -456,8 +456,13 @@ fn format_age(age: Duration) -> String { format!("{}s", seconds) } +/// First 7 characters, by char boundary rather than byte index. The value comes +/// from the API, so a non-ASCII one must shorten, not panic mid-scan. fn short_sha(sha: &str) -> &str { - &sha[..sha.len().min(7)] + match sha.char_indices().nth(7) { + Some((byte, _)) => &sha[..byte], + None => sha, + } } #[cfg(test)] diff --git a/src/utils/api.rs b/src/utils/api.rs index 3f8d4f8..2c56cff 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -1,3 +1,4 @@ +use crate::incremental::IncrementalPlan; use crate::log::debug; use crate::utils; use corgea::vuln_api::{auth_header, source}; @@ -233,15 +234,30 @@ pub struct UploadZipResult { pub project_id: Option, } +/// Per-scan settings travelling with the archive without being part of it. +#[derive(Debug, Default)] +pub struct UploadOptions { + pub scan_type: Option, + pub policy: Option, + pub metadata: Option, + /// Set when this run resolved a diff for the server to analyze instead of + /// the whole project. + pub incremental: Option, +} + pub fn upload_zip( file_path: &str, url: &str, project_name: &str, repo_info: Option, - scan_type: Option, - policy: Option, - metadata: Option, + options: UploadOptions, ) -> Result> { + let UploadOptions { + scan_type, + policy, + metadata, + incremental, + } = options; let client = http_client(); let file_size = std::fs::metadata(file_path)?.len(); let file_name = Path::new(file_path).file_name().unwrap().to_str().unwrap(); @@ -370,6 +386,27 @@ pub fn upload_zip( if let Some(meta) = &metadata { form = form.part("metadata", multipart::Part::text(meta.clone())); } + // Both fields or neither: the list is only safe next to the commit it + // was measured from, and a server seeing one without the other would + // guess a baseline. A list that will not serialize drops both, leaving + // a full scan. + if let Some(plan) = &incremental { + match serde_json::to_string(&plan.changed_files) { + Ok(changed_files) => { + form = form.part( + "incremental_base_sha", + multipart::Part::text(plan.base_sha.clone()), + ); + form = form.part( + "incremental_changed_files", + multipart::Part::text(changed_files), + ); + } + Err(e) => debug(&format!( + "Could not serialize the incremental file list, scanning every file: {e}" + )), + } + } let response = match client .patch(format!("{}{}/start-scan/{}/", url, API_BASE, transfer_id)) @@ -819,6 +856,35 @@ pub fn query_scan_list( request_scan_list(url, query_params) } +/// One page of the project's scans that could be diffed against, newest first. +/// +/// Filters are server-side, so the answer is usually the first entry of page +/// one. A backend predating them ignores the unknown parameters and returns +/// scans of every kind, so the caller must still re-check each scan it acts on +/// — see `incremental::is_usable_baseline`. +pub fn query_baseline_scans( + url: &str, + project: &str, + engine: &str, + page: u16, + page_size: u16, +) -> Result> { + request_scan_list( + url, + vec![ + ("page", page.to_string()), + ("page_size", page_size.to_string()), + ("project", project.to_string()), + ("engine", engine.to_string()), + ("status", "complete".to_string()), + ("exclude_pull_requests", "true".to_string()), + // Explicitly clean only. A scan that never reported the flag is + // unknown scope, which the server rejects as a baseline. + ("worktree_dirty", "false".to_string()), + ], + ) +} + /// One page of the project's scans at exactly `sha`, newest first. /// /// The `sha` filter is server-side, but a backend that predates it ignores the diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index b475a28..2294b8c 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -310,6 +310,22 @@ pub(crate) fn assert_scan_list_request( assert_query(request, "project", project) } +/// The baseline lookup an incremental scan makes before uploading. +/// +/// Asserting the filters is the point: they keep this to one request instead of +/// a page walk, and a server dropping them silently returns pull-request and +/// dirty scans for the client to reject. +pub(crate) fn assert_baseline_lookup_request( + request: &CapturedRequest, + project: &str, +) -> Result<(), String> { + assert_scan_list_request(request, project)?; + assert_query(request, "engine", "corgea-blast")?; + assert_query(request, "status", "complete")?; + assert_query(request, "exclude_pull_requests", "true")?; + assert_query(request, "worktree_dirty", "false") +} + pub(crate) fn query_value(request: &CapturedRequest, key: &str) -> Result { let (_, query) = target_path_and_query(&request.target); query @@ -381,6 +397,23 @@ pub(crate) fn assert_multipart_text_field( } } +/// Proves a field was left off the form entirely. Some fields are only safe in +/// pairs, so "absent" is as much the contract as any value. +pub(crate) fn assert_no_multipart_field( + request: &CapturedRequest, + name: &str, +) -> Result<(), String> { + let needle = format!("name=\"{name}\""); + if request + .body + .windows(needle.len()) + .any(|window| window == needle.as_bytes()) + { + return Err(format!("unexpected multipart field {name}")); + } + Ok(()) +} + pub(crate) fn format_transcript(requests: &[CapturedRequest]) -> String { if requests.is_empty() { return "".to_string(); @@ -777,8 +810,19 @@ pub(crate) fn blast_upload_plan(sha: &str, dirty: bool, include_sca: bool) -> Ve let patch_path = "/api/v1/start-scan/transfer-123/".to_string(); let detail_path = "/api/v1/scan/blast-scan-123".to_string(); let issue_path = "/api/v1/scan/blast-scan-123/issues".to_string(); - let mut plan = vec![ - verify_request(), + let mut plan = vec![verify_request()]; + // Scans are incremental by default, so every clean-tree run looks for a + // baseline before uploading. Answering with no scans keeps this the + // full-scan contract: nothing to diff from, no incremental fields on the + // upload. A dirty tree never asks. + if !dirty { + plan.push(expected_request( + "look up a baseline scan to diff against", + |request| assert_baseline_lookup_request(request, "cloud-e2e"), + json_response(scans_response(Vec::new())), + )); + } + plan.extend([ expected_request( "start BLAST upload", |request| { @@ -835,7 +879,7 @@ pub(crate) fn blast_upload_plan(sha: &str, dirty: bool, include_sca: bool) -> Ve }, json_response(empty_issue_page()), ), - ]; + ]); if include_sca { let sca_path = "/api/v1/scan/blast-scan-123/issues/sca".to_string(); plan.push(expected_request( diff --git a/tests/cloud_commands_e2e/main.rs b/tests/cloud_commands_e2e/main.rs index eded107..439f10a 100644 --- a/tests/cloud_commands_e2e/main.rs +++ b/tests/cloud_commands_e2e/main.rs @@ -4,6 +4,7 @@ mod repo_common; mod block_on_report; mod common; mod inspect; +mod scan_incremental; mod scan_list; mod scan_skip; mod upload_wait; diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs new file mode 100644 index 0000000..57483bc --- /dev/null +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -0,0 +1,375 @@ +//! Incremental scans, attempted by default on every `corgea scan blast`: find +//! the project's last clean scan, diff this commit against it locally, send the +//! changed-file list with the archive. +//! +//! The stub asserts the exact wire contract because those two fields are what +//! the server acts on: `incremental_base_sha` picks whose findings carry +//! forward, `incremental_changed_files` picks which files are excluded from +//! that and analyzed instead. +//! +//! Being the default, the ways it declines matter as much as the way it works, +//! so each is a case here: stay correct, and do not even look for a baseline +//! when it already cannot be used. + +use crate::common::*; +use hyper::Method; +use serde_json::{json, Value}; + +const PROJECT: &str = "cloud-e2e"; +const BASELINE_SCAN: &str = "baseline-scan-123"; + +fn baseline_scan(sha: &str) -> Value { + json!({ + "id": BASELINE_SCAN, + "project": PROJECT, + "repo": null, + "branch": "e2e-main", + "status": "complete", + "engine": "corgea-blast", + "created_at": "2026-07-30T12:00:00Z", + "git_sha": sha, + "worktree_dirty": false + }) +} + +fn baseline_lookup(scans: Vec) -> ExpectedRequest { + expected_request( + "look up a baseline scan to diff against", + move |request| assert_baseline_lookup_request(request, PROJECT), + json_response(scans_response(scans)), + ) +} + +/// Everything after the archive upload, which incremental does not change. +fn scan_tail() -> Vec { + let detail_path = "/api/v1/scan/blast-scan-123".to_string(); + let issue_path = "/api/v1/scan/blast-scan-123/issues".to_string(); + vec![ + expected_request( + "read completed BLAST scan", + move |request| assert_authenticated_request(request, Method::GET, &detail_path), + json_response(scan_response("blast-scan-123", PROJECT, "complete")), + ), + expected_request( + "read regular BLAST issues", + move |request| { + assert_authenticated_request(request, Method::GET, &issue_path)?; + assert_query(request, "page", "1")?; + assert_query(request, "page_size", "30") + }, + json_response(empty_issue_page()), + ), + ] +} + +fn start_upload() -> ExpectedRequest { + expected_request( + "start BLAST upload", + |request| { + assert_authenticated_request(request, Method::POST, "/api/v1/start-scan")?; + assert_query(request, "scan_type", "blast") + }, + json_response(json!({"transfer_id": "transfer-123"})), + ) +} + +/// Adds a file and edits another, so the diff has more than one entry and a +/// file the baseline already contained. +fn second_commit(project: &GitProject) -> String { + std::fs::write(project.path().join("helper.py"), "print('helper')\n").expect("write helper"); + std::fs::write(project.path().join("main.py"), "print('edited')\n").expect("edit main"); + run_git(project.path(), &["add", "."]); + run_git(project.path(), &["commit", "-m", "second"]); + String::from_utf8(run_git(project.path(), &["rev-parse", "HEAD"]).stdout) + .expect("UTF-8 SHA") + .trim() + .to_string() +} + +#[test] +fn the_upload_carries_the_baseline_commit_and_the_files_that_changed_since_it() { + let project = git_project(); + let base_sha = project.sha.clone(); + let head_sha = second_commit(&project); + + let patch_sha = head_sha.clone(); + let expected_base = base_sha.clone(); + let mut plan = vec![ + verify_request(), + baseline_lookup(vec![baseline_scan(&base_sha)]), + start_upload(), + expected_request( + "upload BLAST archive with the diff", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field(request, "sha", &patch_sha)?; + assert_multipart_text_field(request, "dirty", "false")?; + assert_multipart_text_field(request, "incremental_base_sha", &expected_base)?; + // Both sides of the diff, sorted, as JSON — a path may contain a + // comma, so the list is never a delimited string. + assert_multipart_text_field( + request, + "incremental_changed_files", + r#"["helper.py","main.py"]"#, + ) + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains("Incremental scan: 2 file(s) changed since commit"), + "{context}" + ); +} + +/// No scan to diff against is a full scan, not an error. Every project's first +/// scan takes this path and must still produce a complete result. +#[test] +fn a_project_with_no_baseline_scan_uploads_without_a_diff() { + let project = git_project(); + let head_sha = second_commit(&project); + + let patch_sha = head_sha.clone(); + let mut plan = vec![ + verify_request(), + baseline_lookup(vec![]), + start_upload(), + expected_request( + "upload BLAST archive with no diff", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field(request, "sha", &patch_sha)?; + // Neither field alone, nor at all: a base commit without a + // list lets the server carry everything forward. + assert_no_multipart_field(request, "incremental_base_sha")?; + assert_no_multipart_field(request, "incremental_changed_files") + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains("Scanning every file: no earlier completed scan of a clean worktree"), + "{context}" + ); +} + +/// The opt-out is absolute: no baseline lookup, no fields, no message. Someone +/// reaching for it wants every file analyzed, usually because something outside +/// `corgea.yaml` changed that the server's baseline checks cannot see. +#[test] +fn disable_incremental_does_not_even_look_for_a_baseline() { + let project = git_project(); + let head_sha = second_commit(&project); + + let patch_sha = head_sha.clone(); + let mut plan = vec![ + verify_request(), + start_upload(), + expected_request( + "upload BLAST archive with no diff", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field(request, "sha", &patch_sha)?; + assert_multipart_text_field(request, "dirty", "false")?; + assert_no_multipart_field(request, "incremental_base_sha")?; + assert_no_multipart_field(request, "incremental_changed_files") + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args([ + "scan", + "blast", + "--disable-incremental", + "--project-name", + PROJECT, + ]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!(!stdout.contains("Scanning every file:"), "{context}"); + assert!(!stdout.contains("Incremental scan:"), "{context}"); +} + +/// `--target` already uploads a chosen subset. Carrying findings forward for +/// files the archive no longer holds would be wrong, so incremental is skipped +/// — silently, since "scanning every file" would be a lie here. +#[test] +fn a_narrowed_archive_skips_incremental_without_claiming_a_full_scan() { + let project = git_project(); + second_commit(&project); + + let mut plan = vec![ + verify_request(), + start_upload(), + expected_request( + "upload narrowed BLAST archive", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + // A partial archive is never an exact snapshot of the commit. + assert_multipart_text_field(request, "dirty", "true")?; + assert_no_multipart_field(request, "incremental_base_sha")?; + assert_no_multipart_field(request, "incremental_changed_files") + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args([ + "scan", + "blast", + "--target", + "main.py", + "--project-name", + PROJECT, + ]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!(!stdout.contains("Scanning every file:"), "{context}"); +} + +/// No git repository must not stall or fail: no commit to diff from, so skip +/// the lookup and scan everything. +#[test] +fn a_directory_that_is_not_a_git_repository_scans_everything() { + let project = tempfile::TempDir::new().expect("create project"); + std::fs::write(project.path().join("main.py"), "print('hi')\n").expect("write source"); + + let mut plan = vec![ + verify_request(), + start_upload(), + expected_request( + "upload BLAST archive with no repo metadata", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_no_multipart_field(request, "incremental_base_sha")?; + assert_no_multipart_field(request, "incremental_changed_files") + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains("Scanning every file: no git branch and commit to diff from"), + "{context}" + ); +} + +/// The server refuses a dirty tree too, and the refusal must come before the +/// baseline lookup: a commit-to-commit diff cannot see uncommitted edits, so no +/// baseline makes the list correct. +#[test] +fn a_dirty_worktree_skips_the_baseline_lookup_and_scans_everything() { + let project = git_project(); + let head_sha = second_commit(&project); + std::fs::write(project.path().join("main.py"), "print('uncommitted')\n") + .expect("dirty the tree"); + + let patch_sha = head_sha.clone(); + let mut plan = vec![ + verify_request(), + start_upload(), + expected_request( + "upload BLAST archive with no diff", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field(request, "sha", &patch_sha)?; + assert_multipart_text_field(request, "dirty", "true")?; + assert_no_multipart_field(request, "incremental_base_sha")?; + assert_no_multipart_field(request, "incremental_changed_files") + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains("Scanning every file: this worktree has uncommitted changes"), + "{context}" + ); +}