From 1d741175c56a6742beaa102691b8f4e036262a94 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 10:18:11 +0000 Subject: [PATCH 01/11] Add --incremental: diff against the last scan locally and send the file list Corgea already runs incremental scans, but doghouse works the diff out server-side through the project's SCM integration, so projects without one -- zip-only projects, unreachable self-hosted hosts, unpushed commits -- pay for a full analysis every run. At one enterprise customer that is 86% of full scans. --incremental resolves the project's newest completed scan of a clean worktree, diffs this commit against it with git2, and sends both the base commit and the changed-file list with the upload. The archive is unchanged: Fusion reads unchanged files for cross-file context, and the server can only carry a finding forward for a file the archive still contains. The base commit travels with the list because the server carries findings forward for every file the list omits; diffing against one baseline while the server copies from another would report stale findings as current. Both sides of every delta are collected and no status is filtered out, so a deleted file lands in the list and does not keep its findings in a tree that no longer contains it. Every refusal -- dirty tree, no baseline, a base commit missing from a shallow clone -- scans everything and says why. Co-authored-by: ibrahim --- src/incremental.rs | 428 +++++++++++++++++++ src/main.rs | 15 + src/scanners/blast.rs | 30 +- src/utils/api.rs | 43 +- tests/cloud_commands_e2e/common/mod.rs | 17 + tests/cloud_commands_e2e/main.rs | 1 + tests/cloud_commands_e2e/scan_incremental.rs | 230 ++++++++++ 7 files changed, 758 insertions(+), 6 deletions(-) create mode 100644 src/incremental.rs create mode 100644 tests/cloud_commands_e2e/scan_incremental.rs diff --git a/src/incremental.rs b/src/incremental.rs new file mode 100644 index 0000000..1ce8be6 --- /dev/null +++ b/src/incremental.rs @@ -0,0 +1,428 @@ +//! `--incremental`: upload the whole project, analyze only what changed. +//! +//! Corgea already runs incremental scans, but it works the diff out server-side +//! by asking the project's SCM integration to compare two commits. That leaves +//! out every project the integration cannot answer for: zip-only projects with +//! no integration at all, self-hosted hosts Corgea cannot reach, and commits +//! that were never pushed. Those projects pay for a full analysis on every run +//! no matter how little moved. This module closes that gap by diffing in the +//! clone the scan is already reading from. +//! +//! Two values travel together and must stay together: the changed-file list and +//! the commit it was measured from. The server carries findings forward for +//! every file *absent* from the list, so if it were to pick a different +//! baseline than the one diffed here, findings in the files that changed +//! between the two baselines would be carried forward stale — reported as +//! current when nobody looked at them. Sending `base_sha` alongside the list +//! lets the server copy from exactly the scan this diff describes, or refuse +//! and scan everything. +//! +//! The archive is unchanged: a full scan still uploads the full project. Fusion +//! reads unchanged files for cross-file context even when it only analyzes the +//! diff, and the server can only carry a finding forward for a file the archive +//! still contains. What shrinks is the analysis, not the upload. +//! +//! Every refusal below scans everything instead. That is the expensive answer, +//! and it is always the correct one, so anything this module cannot prove — +//! a dirty tree, a missing baseline, a base commit this clone does not have — +//! lands there rather than narrowing a scan on a guess. + +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 newest usable scan is +/// almost always on the first page; this bounds a project whose recent history +/// is all pull-request or dirty-worktree scans. +const SCAN_LOOKUP_MAX_PAGES: u16 = 3; + +/// The engine every blast scan carries, whoever started it. An uploaded +/// third-party report describes someone else's analysis and cannot be the +/// baseline for one of ours. +const BLAST_ENGINE: &str = "corgea-blast"; + +/// Payload guard, not policy. The server applies the real ceiling +/// (`INCREMENTAL_SCAN_MAX_FILES`, 300 by default) and falls back to a full scan +/// above it; this only keeps the CLI from building a multi-megabyte form field +/// for a diff that is obviously going 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 { + /// The commit this diff was measured from: the scan whose findings the + /// server carries forward for every file the diff does not name. + pub base_sha: String, + /// Repo-relative paths that differ 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 edits that were never committed, so a + // dirty tree would leave modified files out of the list and their old + // findings copied forward as if current. The server enforces this too; it + // is 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; + } + + let (Some(branch), Some(head_sha)) = (branch, head_sha) else { + explain_full_scan( + "this run could not resolve a git branch and commit for the project \ + (a scan started outside the repository root reports neither)", + ); + 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 is scanning everything. Never fatal: a full scan is the +/// correct answer, just a slower one, so the run continues. +fn explain_full_scan(reason: &str) { + println!("Scanning every file: {reason}."); +} + +/// The commit of the newest scan this project can be diffed against. +/// +/// Prefers the branch being scanned and falls back to the newest usable scan on +/// any branch, mirroring how doghouse orders its own baseline lookup +/// (`ScanManager._try_incremental_scan`). The fallback is what makes the first +/// scan of a feature branch incremental against the 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_scan_list( + &url, + Some(project_name), + Some(page), + Some(SCAN_LOOKUP_PAGE_SIZE), + ) { + Ok(response) => response, + Err(e) => { + // A lookup that fails proves nothing about the project's + // history, so this is a full scan, not an 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; + } + + // The list is newest first, so the first same-branch match is the best + // baseline 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 +} + +/// The 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. +/// +/// These are the client-side half of the filter doghouse applies when it picks +/// a baseline itself: a completed blast scan of a whole, clean commit that is +/// not a pull request. `worktree_dirty` must be an explicit `false` — `None` +/// means the scan never reported it, and unknown scope is not a clean tree, so +/// the server would reject it as a baseline anyway. +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 that differs between two commits. +/// +/// Both sides of every delta are collected, and no status is filtered out, +/// because the list decides which findings are *not* carried forward. A deleted +/// file left off the list keeps its old findings in a tree where the file no +/// longer exists, and a rename is a delete plus an add whose old path needs the +/// same treatment. `--target`'s `git:diff=` selector deliberately does the +/// opposite — it wants paths that still exist on disk to put in an archive — +/// which is why this does not reuse it. +/// +/// Files git does not track are not a gap here: an untracked file makes the +/// worktree dirty, and a dirty tree has already refused the incremental scan +/// above. +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 from each side, and a + // stable order keeps the uploaded list reproducible for the same two commits. + let mut files = BTreeSet::new(); + for delta in diff.deltas() { + 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() +} + +fn short_sha(sha: &str) -> &str { + &sha[..sha.len().min(7)] +} + +#[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 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() { + // Each of these would make the server refuse the baseline too, so + // diffing against them would narrow 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 the same as 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") + ); + } + + /// A repo with two commits: `first.txt`, then a commit that adds, edits and + /// deletes. Returns `(tempdir, base_sha, head_sha)`. + 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"); + // A deleted file must be listed: leaving it out would carry its old + // findings into a scan of a tree that no longer contains 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()); + } + + #[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..a6154d9 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,13 @@ enum Commands { #[arg(long, help = "Only scan uncommitted changes.")] only_uncommitted: bool, + #[arg( + long = "incremental", + conflicts_with_all = ["only_uncommitted", "target", "exclude"], + help = "Analyze only the files that changed since this project's last scan. The whole project is still uploaded — Corgea reads unchanged files for context and carries their existing findings forward — so the result is a full picture of the project, just cheaper to produce. Requires a git repository with a commit; the run scans everything instead (and says why) 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. Cannot be combined with the flags that upload a partial archive." + )] + incremental: bool, + #[arg( long = "metadata", value_name = "KEY=VALUE", @@ -669,6 +677,7 @@ fn main() { fail, block_on, only_uncommitted, + incremental, metadata, scan_type, policy, @@ -710,6 +719,11 @@ fn main() { std::process::exit(1); } + if *incremental && *scanner != Scanner::Blast { + ::log::error!("--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 +860,7 @@ fn main() { fail, block_on, only_uncommitted, + incremental, metadata_json, scan_type.clone(), policy.clone(), diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index bbc5dc3..ad74c8e 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, + incremental: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -109,6 +110,7 @@ pub fn run( config, &project_name, only_uncommitted, + incremental, metadata, scan_type, policy, @@ -281,6 +283,7 @@ fn start_new_scan( config: &Config, project_name: &str, only_uncommitted: &bool, + incremental: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -513,15 +516,36 @@ fn start_new_scan( info.dirty = true; } } + // Resolved from the reconciled repo info, so a tree that turned out dirty — + // or a HEAD that moved while the archive was being built — refuses the + // incremental scan rather than diffing against a commit this upload is not + // a snapshot of. + let incremental_plan = (*incremental) + .then(|| { + 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()), + // No repo info at all is not dirtiness; it is the missing + // branch/commit the resolver reports next, with a message that + // names the real problem. + repo_info.as_ref().is_some_and(|info| info.dirty), + ) + }) + .flatten(); 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/utils/api.rs b/src/utils/api.rs index 3f8d4f8..f8f5c0c 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 that travel 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 file list is only safe to act on next to + // the commit it was measured from, and a server that saw one without + // the other would have to guess a baseline. A list that will not + // serialize drops both and leaves this 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)) diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index b475a28..17729dd 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -381,6 +381,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 a part of 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(); 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..00669d9 --- /dev/null +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -0,0 +1,230 @@ +//! `--incremental`: the CLI finds the project's last clean scan, diffs this +//! commit against it locally, and sends the changed-file list with the archive. +//! +//! The stub asserts the exact wire contract, because the two fields are what +//! the server acts on: `incremental_base_sha` decides which scan's findings are +//! carried forward, and `incremental_changed_files` decides which files are +//! excluded from that carry-forward and analyzed instead. + +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_scan_list_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 on top of the fixture's first commit, 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", "--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("Incremental scan: 2 file(s) changed since commit"), + "{context}" + ); +} + +/// A project with no scan to diff against is a full scan, not an error: the +/// first `--incremental` run of any project 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 may appear alone or at all: a base commit + // without a list would let 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", "--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: no earlier completed scan of a clean worktree"), + "{context}" + ); +} + +/// A dirty tree is the one refusal the server would also make, and it must +/// happen before the baseline lookup: a commit-to-commit diff cannot see +/// uncommitted edits, so no baseline could make 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", "--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: this worktree has uncommitted changes"), + "{context}" + ); +} From b4748f6e70067364419bdb46743a1833924248ba Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 10:51:17 +0000 Subject: [PATCH 02/11] Fail closed to a full scan when a submodule pointer moves A committed submodule bump is a single gitlink delta naming the submodule directory, but packaging walks into that directory and uploads the files inside it. The parent worktree is clean, so the incremental path stayed enabled and those files -- absent from the changed-file list -- kept findings nothing had re-examined. Diffing the two submodule commits would mean opening a repository that may not be checked out, so a gitlink on either side of a delta refuses the diff and scans everything. Co-authored-by: ibrahim --- src/incremental.rs | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/incremental.rs b/src/incremental.rs index 1ce8be6..c9904bd 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -238,6 +238,13 @@ fn is_usable_baseline(scan: &ScanResponse) -> bool { /// Files git does not track are not a gap here: an untracked file makes the /// worktree dirty, and a dirty tree has already refused the incremental scan /// above. +/// +/// A submodule is the one thing this cannot describe. A committed pointer bump +/// is a single gitlink delta naming the submodule directory, while packaging +/// walks into that directory and uploads the files inside it — so the files +/// that actually changed would be missing from the list and keep their old +/// findings. Diffing the two submodule commits would mean opening a repository +/// that may not even be checked out, so this fails closed to a full scan. fn changed_files_between( repo: &Repository, base_sha: &str, @@ -262,6 +269,21 @@ fn changed_files_between( // 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('\\', "/"); @@ -418,6 +440,39 @@ mod tests { .is_empty()); } + /// A 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 walks into the submodule and uploads the files inside it, + // but the diff names only `vendor`, so those files would keep findings + // nothing re-examined. + 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(); From 6c37fd0a3cfaa6a136bf73e9a7a7da9bf44d7358 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 10:51:17 +0000 Subject: [PATCH 03/11] Bump h2 to 0.4.18 for RUSTSEC-2026-0258 cargo audit fails the CI gate on h2 0.4.12 (unbounded empty DATA frames, advisory published 2026-08-17). Lockfile-only bump; the advisory predates this branch and affects main identically. Co-authored-by: ibrahim --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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", From 9cc3ca79c1b879239bd928da6dbd083336f033dc Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 11:12:53 +0000 Subject: [PATCH 04/11] Make incremental the default and add --disable-incremental An optimization nobody opts into does not run. The projects this exists for -- zip-only, self-hosted, unpushed commits -- are also the ones least likely to hear that a new flag exists, so it now runs on every blast scan and --disable-incremental forces every file to be analyzed. Being the default means it has to be safe on a repository that was never set up for it, so every way it declines is a fall-through to the full scan that run would have done anyway: no git repository or commit, a dirty worktree, no earlier clean scan, or a base commit missing from a shallow clone. Each says why. --only-uncommitted, --target and --exclude no longer conflict with it; they disable it silently instead. Those runs upload a narrowed archive, so carrying findings forward for files the archive no longer contains would be wrong -- but they are not scanning every file either, so there is no honest reason to print. blast_upload_plan now expects the baseline lookup for clean-tree scans and answers with no scans, which is what makes it the full-scan contract. Co-authored-by: ibrahim --- src/incremental.rs | 16 +- src/main.rs | 15 +- src/scanners/blast.rs | 48 +++--- tests/cloud_commands_e2e/common/mod.rs | 17 +- tests/cloud_commands_e2e/scan_incremental.rs | 156 ++++++++++++++++++- 5 files changed, 212 insertions(+), 40 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index c9904bd..9f89669 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -1,4 +1,4 @@ -//! `--incremental`: upload the whole project, analyze only what changed. +//! Incremental scans: upload the whole project, analyze only what changed. //! //! Corgea already runs incremental scans, but it works the diff out server-side //! by asking the project's SCM integration to compare two commits. That leaves @@ -8,6 +8,12 @@ //! no matter how little moved. This module closes that gap by diffing in the //! clone the scan is already reading from. //! +//! This runs by default, so it has to be safe on a repository that was never +//! set up for it. Nothing here is required to succeed: the first scan of a +//! project, a directory that is not a git repository at all, and a CI job with +//! a shallow clone all fall through to a full scan, which is what those runs +//! would have done anyway. `--disable-incremental` forces that path. +//! //! Two values travel together and must stay together: the changed-file list and //! the commit it was measured from. The server carries findings forward for //! every file *absent* from the list, so if it were to pick a different @@ -83,10 +89,14 @@ pub fn resolve_incremental_plan( return None; } + // No branch and commit means there is nothing to diff from. That covers a + // directory that is not a git repository, a repository with no commit yet, + // a detached HEAD, and a scan started below the repository root — all of + // which report no RepoInfo to the upload either. let (Some(branch), Some(head_sha)) = (branch, head_sha) else { explain_full_scan( - "this run could not resolve a git branch and commit for the project \ - (a scan started outside the repository root reports neither)", + "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; }; diff --git a/src/main.rs b/src/main.rs index a6154d9..1e934c6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -91,11 +91,10 @@ enum Commands { only_uncommitted: bool, #[arg( - long = "incremental", - conflicts_with_all = ["only_uncommitted", "target", "exclude"], - help = "Analyze only the files that changed since this project's last scan. The whole project is still uploaded — Corgea reads unchanged files for context and carries their existing findings forward — so the result is a full picture of the project, just cheaper to produce. Requires a git repository with a commit; the run scans everything instead (and says why) 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. Cannot be combined with the flags that upload a partial archive." + 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." )] - incremental: bool, + disable_incremental: bool, #[arg( long = "metadata", @@ -677,7 +676,7 @@ fn main() { fail, block_on, only_uncommitted, - incremental, + disable_incremental, metadata, scan_type, policy, @@ -719,8 +718,8 @@ fn main() { std::process::exit(1); } - if *incremental && *scanner != Scanner::Blast { - ::log::error!("--incremental is only supported with blast scanner."); + if *disable_incremental && *scanner != Scanner::Blast { + ::log::error!("--disable-incremental is only supported with blast scanner."); std::process::exit(1); } @@ -860,7 +859,7 @@ fn main() { fail, block_on, only_uncommitted, - incremental, + disable_incremental, metadata_json, scan_type.clone(), policy.clone(), diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index ad74c8e..c8e885a 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -53,7 +53,7 @@ pub fn run( fail: &bool, block_on: Option, only_uncommitted: &bool, - incremental: &bool, + disable_incremental: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -110,7 +110,7 @@ pub fn run( config, &project_name, only_uncommitted, - incremental, + disable_incremental, metadata, scan_type, policy, @@ -283,7 +283,7 @@ fn start_new_scan( config: &Config, project_name: &str, only_uncommitted: &bool, - incremental: &bool, + disable_incremental: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -516,24 +516,30 @@ fn start_new_scan( info.dirty = true; } } - // Resolved from the reconciled repo info, so a tree that turned out dirty — - // or a HEAD that moved while the archive was being built — refuses the - // incremental scan rather than diffing against a commit this upload is not - // a snapshot of. - let incremental_plan = (*incremental) - .then(|| { - 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()), - // No repo info at all is not dirtiness; it is the missing - // branch/commit the resolver reports next, with a message that - // names the real problem. - repo_info.as_ref().is_some_and(|info| info.dirty), - ) - }) - .flatten(); + // Incremental is the default, so this asks whether anything has taken it off + // the table. A narrowed archive is the silent case: those runs are already + // scanning a subset the user chose, and carrying findings forward for files + // the archive no longer contains would be wrong — but they are also not + // "scanning every file", so there is no honest message to print. + let narrowed_archive = target_str.is_some() || exclude.is_some(); + let incremental_plan = if *disable_incremental || narrowed_archive { + None + } else { + // Resolved from the reconciled repo info, so a tree that turned out + // dirty — or a HEAD that moved while the archive was being built — + // refuses the incremental scan 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()), + // No repo info at all is not dirtiness; it is the missing + // branch/commit the resolver reports next, with a message that + // names the real problem. + 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, diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index 17729dd..57a408e 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -794,8 +794,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 to diff against before uploading. Answering with no scans is what + // keeps this the full-scan contract: with nothing to diff from, the upload + // carries no incremental fields. A dirty tree never asks. + if !dirty { + plan.push(expected_request( + "look up a baseline scan to diff against", + |request| assert_scan_list_request(request, "cloud-e2e"), + json_response(scans_response(Vec::new())), + )); + } + plan.extend([ expected_request( "start BLAST upload", |request| { @@ -852,7 +863,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/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 00669d9..1db6f50 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -1,10 +1,15 @@ -//! `--incremental`: the CLI finds the project's last clean scan, diffs this -//! commit against it locally, and sends the changed-file list with the archive. +//! Incremental scans, which every `corgea scan blast` run attempts by default: +//! the CLI finds the project's last clean scan, diffs this commit against it +//! locally, and sends the changed-file list with the archive. //! //! The stub asserts the exact wire contract, because the two fields are what //! the server acts on: `incremental_base_sha` decides which scan's findings are //! carried forward, and `incremental_changed_files` decides which files are //! excluded from that carry-forward and analyzed instead. +//! +//! Being the default means the ways it declines matter as much as the way it +//! works, so each of those is a case here: the run must stay correct and must +//! not even look for a baseline when it already knows it cannot use one. use crate::common::*; use hyper::Method; @@ -119,7 +124,7 @@ fn the_upload_carries_the_baseline_commit_and_the_files_that_changed_since_it() let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path()); - command.args(["scan", "blast", "--incremental", "--project-name", PROJECT]); + command.args(["scan", "blast", "--project-name", PROJECT]); let output = run_with_timeout(command, &api); let transcript = api.assert_finished(); @@ -167,7 +172,7 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path()); - command.args(["scan", "blast", "--incremental", "--project-name", PROJECT]); + command.args(["scan", "blast", "--project-name", PROJECT]); let output = run_with_timeout(command, &api); let transcript = api.assert_finished(); @@ -181,6 +186,147 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { ); } +/// The opt-out has to be 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 subset the user chose. Carrying findings forward +/// for files the archive no longer contains would be wrong, so incremental is +/// skipped — silently, because "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}"); +} + +/// A directory with no git repository must not stall or fail: it has no commit +/// to diff from, so it skips the lookup and scans 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}" + ); +} + /// A dirty tree is the one refusal the server would also make, and it must /// happen before the baseline lookup: a commit-to-commit diff cannot see /// uncommitted edits, so no baseline could make the list correct. @@ -215,7 +361,7 @@ fn a_dirty_worktree_skips_the_baseline_lookup_and_scans_everything() { let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path()); - command.args(["scan", "blast", "--incremental", "--project-name", PROJECT]); + command.args(["scan", "blast", "--project-name", PROJECT]); let output = run_with_timeout(command, &api); let transcript = api.assert_finished(); From 685260ff3b41925622b55ab5116c3989c17c0c6c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 11:24:31 +0000 Subject: [PATCH 05/11] Ask the API for baseline scans instead of filtering a page walk The lookup wanted the newest completed blast scan of a clean, non-pull-request commit, and none of that was expressible as a query, so it read the project's history 30 scans at a time and sorted it out locally. With enough pull-request traffic a page holds nothing usable, and once the page budget runs out the run gives up and scans everything -- so a project could be permanently unable to find a baseline it actually has. query_baseline_scans sends those four filters, so the answer is normally the first entry of the first page. is_usable_baseline and the page walk stay. A backend that predates the filters ignores unknown parameters and answers with scans of every kind, and diffing against a dirty or pull-request scan's commit would compare against the wrong tree -- the same reason query_scans_for_commit re-checks git_sha. Co-authored-by: ibrahim --- src/incremental.rs | 24 +++++++++++----- src/utils/api.rs | 29 ++++++++++++++++++++ tests/cloud_commands_e2e/common/mod.rs | 18 +++++++++++- tests/cloud_commands_e2e/scan_incremental.rs | 2 +- 4 files changed, 64 insertions(+), 9 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 9f89669..7f80812 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -42,9 +42,13 @@ 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 newest usable scan is -/// almost always on the first page; this bounds a project whose recent history -/// is all pull-request or dirty-worktree scans. +/// Backstop on pages walked looking for a baseline. +/// +/// The server filters out the scans that cannot be a baseline, so the answer is +/// normally the first entry of the first page and this never iterates. It is +/// here for a backend that predates those filters: it ignores the unknown +/// parameters and returns scans of every kind, which for a project with heavy +/// pull-request traffic can fill a page with nothing usable. const SCAN_LOOKUP_MAX_PAGES: u16 = 3; /// The engine every blast scan carries, whoever started it. An uploaded @@ -171,11 +175,12 @@ fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Optio let mut any_branch_fallback: Option = None; for page in 1..=SCAN_LOOKUP_MAX_PAGES { - let response = match api::query_scan_list( + let response = match api::query_baseline_scans( &url, - Some(project_name), - Some(page), - Some(SCAN_LOOKUP_PAGE_SIZE), + project_name, + BLAST_ENGINE, + page, + SCAN_LOOKUP_PAGE_SIZE, ) { Ok(response) => response, Err(e) => { @@ -227,6 +232,11 @@ fn usable_baselines(scans: &[ScanResponse]) -> impl Iterator bool { classify_scan_status(&scan.status) == ScanState::Completed && scan.engine.eq_ignore_ascii_case(BLAST_ENGINE) diff --git a/src/utils/api.rs b/src/utils/api.rs index f8f5c0c..95af9c5 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -856,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. +/// +/// The filters are server-side so the answer is usually the first entry of the +/// first page. A backend that predates them ignores the unknown parameters and +/// answers with the project's scans of every kind, so the caller still has to +/// 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, and the server will not accept it 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 57a408e..c99cebd 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 are what keeps this to one request +/// instead of a page walk, and a server that drops them would silently return +/// 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 @@ -802,7 +818,7 @@ pub(crate) fn blast_upload_plan(sha: &str, dirty: bool, include_sca: bool) -> Ve if !dirty { plan.push(expected_request( "look up a baseline scan to diff against", - |request| assert_scan_list_request(request, "cloud-e2e"), + |request| assert_baseline_lookup_request(request, "cloud-e2e"), json_response(scans_response(Vec::new())), )); } diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 1db6f50..255bb4c 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -35,7 +35,7 @@ fn baseline_scan(sha: &str) -> Value { fn baseline_lookup(scans: Vec) -> ExpectedRequest { expected_request( "look up a baseline scan to diff against", - move |request| assert_scan_list_request(request, PROJECT), + move |request| assert_baseline_lookup_request(request, PROJECT), json_response(scans_response(scans)), ) } From 02e9870e683d83d498a8fc10f03882d409ec768d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 11:38:09 +0000 Subject: [PATCH 06/11] Tighten comments added by this branch Same content, fewer words. Also drops two stale --incremental references in test docs left by the flip to opt-out. Co-authored-by: ibrahim --- src/incremental.rs | 181 ++++++++----------- src/scanners/blast.rs | 21 +-- src/utils/api.rs | 20 +- tests/cloud_commands_e2e/common/mod.rs | 14 +- tests/cloud_commands_e2e/scan_incremental.rs | 57 +++--- 5 files changed, 132 insertions(+), 161 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 7f80812..0a8dfa0 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -1,37 +1,22 @@ -//! Incremental scans: upload the whole project, analyze only what changed. +//! Incremental scans: upload whole project, analyze only what changed. //! -//! Corgea already runs incremental scans, but it works the diff out server-side -//! by asking the project's SCM integration to compare two commits. That leaves -//! out every project the integration cannot answer for: zip-only projects with -//! no integration at all, self-hosted hosts Corgea cannot reach, and commits -//! that were never pushed. Those projects pay for a full analysis on every run -//! no matter how little moved. This module closes that gap by diffing in the -//! clone the scan is already reading from. +//! 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. //! -//! This runs by default, so it has to be safe on a repository that was never -//! set up for it. Nothing here is required to succeed: the first scan of a -//! project, a directory that is not a git repository at all, and a CI job with -//! a shallow clone all fall through to a full scan, which is what those runs -//! would have done anyway. `--disable-incremental` forces that path. +//! 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. //! -//! Two values travel together and must stay together: the changed-file list and -//! the commit it was measured from. The server carries findings forward for -//! every file *absent* from the list, so if it were to pick a different -//! baseline than the one diffed here, findings in the files that changed -//! between the two baselines would be carried forward stale — reported as -//! current when nobody looked at them. Sending `base_sha` alongside the list -//! lets the server copy from exactly the scan this diff describes, or refuse -//! and scan everything. +//! `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: a full scan still uploads the full project. Fusion -//! reads unchanged files for cross-file context even when it only analyzes the -//! diff, and the server can only carry a finding forward for a file the archive -//! still contains. What shrinks is the analysis, not the upload. -//! -//! Every refusal below scans everything instead. That is the expensive answer, -//! and it is always the correct one, so anything this module cannot prove — -//! a dirty tree, a missing baseline, a base commit this clone does not have — -//! lands there rather than narrowing a scan on a guess. +//! 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}; @@ -44,31 +29,29 @@ const SCAN_LOOKUP_PAGE_SIZE: u16 = 30; /// Backstop on pages walked looking for a baseline. /// -/// The server filters out the scans that cannot be a baseline, so the answer is -/// normally the first entry of the first page and this never iterates. It is -/// here for a backend that predates those filters: it ignores the unknown -/// parameters and returns scans of every kind, which for a project with heavy -/// pull-request traffic can fill a page with nothing usable. +/// 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; -/// The engine every blast scan carries, whoever started it. An uploaded -/// third-party report describes someone else's analysis and cannot be the -/// baseline for one of ours. +/// 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 by default) and falls back to a full scan -/// above it; this only keeps the CLI from building a multi-megabyte form field -/// for a diff that is obviously going to be refused. +/// (`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 { - /// The commit this diff was measured from: the scan whose findings the - /// server carries forward for every file the diff does not name. + /// 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 that differ between `base_sha` and the commit being + /// Repo-relative paths differing between `base_sha` and the commit being /// scanned, including deletions and both sides of a rename. pub changed_files: Vec, } @@ -82,10 +65,10 @@ pub fn resolve_incremental_plan( head_sha: Option<&str>, worktree_dirty: bool, ) -> Option { - // A commit-to-commit diff cannot see edits that were never committed, so a - // dirty tree would leave modified files out of the list and their old - // findings copied forward as if current. The server enforces this too; it - // is repeated here so the run says why before paying for the upload. + // 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", @@ -93,10 +76,9 @@ pub fn resolve_incremental_plan( return None; } - // No branch and commit means there is nothing to diff from. That covers a - // directory that is not a git repository, a repository with no commit yet, - // a detached HEAD, and a scan started below the repository root — all of - // which report no RepoInfo to the upload either. + // 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 \ @@ -158,18 +140,18 @@ pub fn resolve_incremental_plan( }) } -/// Say why this run is scanning everything. Never fatal: a full scan is the -/// correct answer, just a slower one, so the run continues. +/// 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}."); } -/// The commit of the newest scan this project can be diffed against. +/// Commit of the newest scan this project can be diffed against. /// -/// Prefers the branch being scanned and falls back to the newest usable scan on -/// any branch, mirroring how doghouse orders its own baseline lookup -/// (`ScanManager._try_incremental_scan`). The fallback is what makes the first -/// scan of a feature branch incremental against the trunk instead of full. +/// 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; @@ -184,8 +166,8 @@ fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Optio ) { Ok(response) => response, Err(e) => { - // A lookup that fails proves nothing about the project's - // history, so this is a full scan, not an error. + // 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; } @@ -196,8 +178,8 @@ fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Optio break; } - // The list is newest first, so the first same-branch match is the best - // baseline available and no later page can improve on it. + // 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)) { @@ -220,23 +202,22 @@ fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Optio any_branch_fallback } -/// The scans on one page that can serve as a baseline, newest first. +/// 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. /// -/// These are the client-side half of the filter doghouse applies when it picks -/// a baseline itself: a completed blast scan of a whole, clean commit that is -/// not a pull request. `worktree_dirty` must be an explicit `false` — `None` -/// means the scan never reported it, and unknown scope is not a clean tree, so -/// the server would reject it as a baseline anyway. +/// 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 is what -/// keeps the page walk from iterating. This stays because a backend that -/// predates those parameters ignores them, and acting on a dirty or -/// pull-request scan's commit would diff against the wrong tree. +/// `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) @@ -245,26 +226,22 @@ fn is_usable_baseline(scan: &ScanResponse) -> bool { && scan.git_sha.as_deref().is_some_and(|sha| !sha.is_empty()) } -/// Every repo-relative path that differs between two commits. +/// Every repo-relative path differing between two commits. /// -/// Both sides of every delta are collected, and no status is filtered out, -/// because the list decides which findings are *not* carried forward. A deleted -/// file left off the list keeps its old findings in a tree where the file no -/// longer exists, and a rename is a delete plus an add whose old path needs the -/// same treatment. `--target`'s `git:diff=` selector deliberately does the -/// opposite — it wants paths that still exist on disk to put in an archive — -/// which is why this does not reuse it. +/// 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. /// -/// Files git does not track are not a gap here: an untracked file makes the -/// worktree dirty, and a dirty tree has already refused the incremental scan -/// above. +/// Untracked files are not a gap: they make the worktree dirty, already +/// refused above. /// -/// A submodule is the one thing this cannot describe. A committed pointer bump -/// is a single gitlink delta naming the submodule directory, while packaging -/// walks into that directory and uploads the files inside it — so the files -/// that actually changed would be missing from the list and keep their old -/// findings. Diffing the two submodule commits would mean opening a repository -/// that may not even be checked out, so this fails closed to a full scan. +/// 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, @@ -285,8 +262,8 @@ fn changed_files_between( .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 from each side, and a - // stable order keeps the uploaded list reproducible for the same two commits. + // 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 @@ -358,8 +335,8 @@ mod tests { #[test] fn scans_that_cannot_describe_a_whole_clean_commit_are_rejected() { - // Each of these would make the server refuse the baseline too, so - // diffing against them would narrow a scan the server then widens. + // 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)); @@ -376,7 +353,7 @@ mod tests { dirty.worktree_dirty = Some(true); assert!(!is_usable_baseline(&dirty)); - // Never reported is not the same as known clean. + // Never reported is not known clean. let mut unknown = scan("main", "abc"); unknown.worktree_dirty = None; assert!(!is_usable_baseline(&unknown)); @@ -395,8 +372,7 @@ mod tests { ); } - /// A repo with two commits: `first.txt`, then a commit that adds, edits and - /// deletes. Returns `(tempdir, base_sha, head_sha)`. + /// 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"); @@ -447,8 +423,8 @@ mod tests { 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"); - // A deleted file must be listed: leaving it out would carry its old - // findings into a scan of a tree that no longer contains it. + // 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"]); } @@ -460,7 +436,7 @@ mod tests { .is_empty()); } - /// A commit whose tree carries a `vendor` gitlink pointing at `target`. + /// 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"); @@ -478,9 +454,8 @@ mod tests { #[test] fn a_moved_submodule_pointer_refuses_the_diff() { - // Packaging walks into the submodule and uploads the files inside it, - // but the diff names only `vendor`, so those files would keep findings - // nothing re-examined. + // 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"); diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index c8e885a..254910f 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -516,27 +516,24 @@ fn start_new_scan( info.dirty = true; } } - // Incremental is the default, so this asks whether anything has taken it off - // the table. A narrowed archive is the silent case: those runs are already - // scanning a subset the user chose, and carrying findings forward for files - // the archive no longer contains would be wrong — but they are also not - // "scanning every file", so there is no honest message to print. + // 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 { - // Resolved from the reconciled repo info, so a tree that turned out - // dirty — or a HEAD that moved while the archive was being built — - // refuses the incremental scan rather than diffing against a commit - // this upload is not a snapshot of. + // 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()), - // No repo info at all is not dirtiness; it is the missing - // branch/commit the resolver reports next, with a message that - // names the real problem. + // 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), ) }; diff --git a/src/utils/api.rs b/src/utils/api.rs index 95af9c5..2c56cff 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -234,7 +234,7 @@ pub struct UploadZipResult { pub project_id: Option, } -/// Per-scan settings that travel with the archive without being part of it. +/// Per-scan settings travelling with the archive without being part of it. #[derive(Debug, Default)] pub struct UploadOptions { pub scan_type: Option, @@ -386,10 +386,10 @@ pub fn upload_zip( if let Some(meta) = &metadata { form = form.part("metadata", multipart::Part::text(meta.clone())); } - // Both fields or neither: the file list is only safe to act on next to - // the commit it was measured from, and a server that saw one without - // the other would have to guess a baseline. A list that will not - // serialize drops both and leaves this a full scan. + // 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) => { @@ -858,10 +858,10 @@ pub fn query_scan_list( /// One page of the project's scans that could be diffed against, newest first. /// -/// The filters are server-side so the answer is usually the first entry of the -/// first page. A backend that predates them ignores the unknown parameters and -/// answers with the project's scans of every kind, so the caller still has to -/// re-check each scan it acts on — see `incremental::is_usable_baseline`. +/// 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, @@ -879,7 +879,7 @@ pub fn query_baseline_scans( ("status", "complete".to_string()), ("exclude_pull_requests", "true".to_string()), // Explicitly clean only. A scan that never reported the flag is - // unknown scope, and the server will not accept it as a baseline. + // unknown scope, which the server rejects as a baseline. ("worktree_dirty", "false".to_string()), ], ) diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index c99cebd..2294b8c 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -312,9 +312,9 @@ pub(crate) fn assert_scan_list_request( /// The baseline lookup an incremental scan makes before uploading. /// -/// Asserting the filters is the point: they are what keeps this to one request -/// instead of a page walk, and a server that drops them would silently return -/// pull-request and dirty scans for the client to reject. +/// 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, @@ -398,7 +398,7 @@ 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 a part of the contract as any value. +/// pairs, so "absent" is as much the contract as any value. pub(crate) fn assert_no_multipart_field( request: &CapturedRequest, name: &str, @@ -812,9 +812,9 @@ pub(crate) fn blast_upload_plan(sha: &str, dirty: bool, include_sca: bool) -> Ve let issue_path = "/api/v1/scan/blast-scan-123/issues".to_string(); let mut plan = vec![verify_request()]; // Scans are incremental by default, so every clean-tree run looks for a - // baseline to diff against before uploading. Answering with no scans is what - // keeps this the full-scan contract: with nothing to diff from, the upload - // carries no incremental fields. A dirty tree never asks. + // 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", diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 255bb4c..57483bc 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -1,15 +1,15 @@ -//! Incremental scans, which every `corgea scan blast` run attempts by default: -//! the CLI finds the project's last clean scan, diffs this commit against it -//! locally, and sends the changed-file list with the archive. +//! 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 the two fields are what -//! the server acts on: `incremental_base_sha` decides which scan's findings are -//! carried forward, and `incremental_changed_files` decides which files are -//! excluded from that carry-forward and analyzed instead. +//! 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 means the ways it declines matter as much as the way it -//! works, so each of those is a case here: the run must stay correct and must -//! not even look for a baseline when it already knows it cannot use one. +//! 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; @@ -40,7 +40,7 @@ fn baseline_lookup(scans: Vec) -> ExpectedRequest { ) } -/// Everything after the archive upload, which `--incremental` does not change. +/// 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(); @@ -73,8 +73,8 @@ fn start_upload() -> ExpectedRequest { ) } -/// Adds a file and edits another on top of the fixture's first commit, so the -/// diff has more than one entry and a file the baseline already contained. +/// 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"); @@ -138,9 +138,8 @@ fn the_upload_carries_the_baseline_commit_and_the_files_that_changed_since_it() ); } -/// A project with no scan to diff against is a full scan, not an error: the -/// first `--incremental` run of any project takes this path and must still -/// produce a complete result. +/// 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(); @@ -160,8 +159,8 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { "/api/v1/start-scan/transfer-123/", )?; assert_multipart_text_field(request, "sha", &patch_sha)?; - // Neither field may appear alone or at all: a base commit - // without a list would let the server carry everything forward. + // 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") }, @@ -186,9 +185,9 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { ); } -/// The opt-out has to be 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. +/// 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(); @@ -236,9 +235,9 @@ fn disable_incremental_does_not_even_look_for_a_baseline() { assert!(!stdout.contains("Incremental scan:"), "{context}"); } -/// `--target` already uploads a subset the user chose. Carrying findings forward -/// for files the archive no longer contains would be wrong, so incremental is -/// skipped — silently, because "scanning every file" would be a lie here. +/// `--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(); @@ -285,8 +284,8 @@ fn a_narrowed_archive_skips_incremental_without_claiming_a_full_scan() { assert!(!stdout.contains("Scanning every file:"), "{context}"); } -/// A directory with no git repository must not stall or fail: it has no commit -/// to diff from, so it skips the lookup and scans everything. +/// 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"); @@ -327,9 +326,9 @@ fn a_directory_that_is_not_a_git_repository_scans_everything() { ); } -/// A dirty tree is the one refusal the server would also make, and it must -/// happen before the baseline lookup: a commit-to-commit diff cannot see -/// uncommitted edits, so no baseline could make the list correct. +/// 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(); From 1384ef3f6ef7d51f7bca0430adb12a2216df0a04 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 12:04:59 +0000 Subject: [PATCH 07/11] Shorten a sha by char boundary, not byte index short_sha sliced &sha[..7]. git_sha comes from the API, so a non-ASCII value would split a UTF-8 char and panic mid-scan -- for incremental, on a code path every scan now runs. Same one-liner in skip_scan.rs, same input. Co-authored-by: ibrahim --- src/incremental.rs | 16 +++++++++++++++- src/skip_scan.rs | 7 ++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 0a8dfa0..5899af2 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -300,8 +300,13 @@ fn commit_tree<'repo>( 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 { - &sha[..sha.len().min(7)] + match sha.char_indices().nth(7) { + Some((byte, _)) => &sha[..byte], + None => sha, + } } #[cfg(test)] @@ -328,6 +333,15 @@ mod tests { } } + #[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"))); 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)] From 711172f461acb01e333cac481539abd751d219c9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 26 Aug 2026 14:53:06 +0000 Subject: [PATCH 08/11] Keep git paths byte-for-byte, name lookup failures, refuse a dead flag pair Three review findings. Backslash translation corrupted legal POSIX filenames. Git stores '/' as its separator on every platform, so a backslash in a diff path is part of the name; rewriting it named a file that did not change. A failed baseline lookup was reported as 'no earlier completed scan', which tells someone with years of history that they have none -- and now that incremental is the default, any network blip said it. find_baseline_sha returns Found/NotFound/LookupFailed, and an earlier page that did answer still counts as Found. --disable-incremental did nothing alongside --skip-if-commit-scanned-recently: the reuse path returns before start_new_scan, so a run asking for every file could silently reuse a scan that was itself incremental. That flag already refuses every other scope flag for the same reason. Tests: branch preference, cross-branch fallback, unusable scans skipped, a page with nothing usable; and end to end, a baseline found on page two and a lookup failure that does not claim the project has no history. Co-authored-by: ibrahim --- src/incremental.rs | 118 +++++++++++++++---- src/main.rs | 4 +- tests/cloud_commands_e2e/scan_incremental.rs | 114 +++++++++++++++++- 3 files changed, 213 insertions(+), 23 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 5899af2..fae7f3f 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -87,12 +87,22 @@ pub fn resolve_incremental_plan( 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 base_sha = match find_baseline_sha(config, project_name, branch) { + Baseline::Found(sha) => sha, + Baseline::NotFound => { + 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; + } + Baseline::LookupFailed => { + explain_full_scan(&format!( + "the earlier scans of project '{project_name}' could not be looked up, \ + so there is nothing to diff against. Run with --verbose for the error" + )); + return None; + } }; let repo = match Repository::discover(".") { @@ -146,13 +156,25 @@ fn explain_full_scan(reason: &str) { println!("Scanning every file: {reason}."); } +/// Outcome of looking for a scan to diff against. +/// +/// `NotFound` and `LookupFailed` both mean a full scan, but they are different +/// things to tell someone: one says this project has no scan history to build +/// on, the other says we could not read the history it may well have. +#[derive(Debug, PartialEq, Eq)] +enum Baseline { + Found(String), + NotFound, + LookupFailed, +} + /// 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 { +fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Baseline { let url = config.get_url(); let mut any_branch_fallback: Option = None; @@ -167,9 +189,13 @@ fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Optio Ok(response) => response, Err(e) => { // A failed lookup proves nothing about the project's history, - // so it means full scan, not error. + // so it means full scan, not error. An earlier page that did + // answer still counts: that scan is a real baseline. crate::log::debug(&format!("Baseline scan lookup failed: {e}")); - return any_branch_fallback; + return match any_branch_fallback { + Some(sha) => Baseline::Found(sha), + None => Baseline::LookupFailed, + }; } }; @@ -180,15 +206,11 @@ fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Optio // 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 let Some(sha) = same_branch_baseline(&scans, branch) { + return Baseline::Found(sha); } if any_branch_fallback.is_none() { - any_branch_fallback = usable_baselines(&scans) - .next() - .and_then(|s| s.git_sha.clone()); + any_branch_fallback = any_branch_baseline(&scans); } if response @@ -199,7 +221,24 @@ fn find_baseline_sha(config: &Config, project_name: &str, branch: &str) -> Optio } } - any_branch_fallback + match any_branch_fallback { + Some(sha) => Baseline::Found(sha), + None => Baseline::NotFound, + } +} + +/// Newest usable scan of `branch` on this page. +fn same_branch_baseline(scans: &[ScanResponse], branch: &str) -> Option { + usable_baselines(scans) + .find(|scan| scan.branch.as_deref() == Some(branch)) + .and_then(|scan| scan.git_sha.clone()) +} + +/// Newest usable scan on this page, whatever branch it ran on. +fn any_branch_baseline(scans: &[ScanResponse]) -> Option { + usable_baselines(scans) + .next() + .and_then(|scan| scan.git_sha.clone()) } /// Scans on one page that can serve as a baseline, newest first. @@ -283,7 +322,10 @@ fn changed_files_between( } for file in [delta.old_file(), delta.new_file()] { if let Some(path) = file.path() { - let path = path.to_string_lossy().replace('\\', "/"); + // Byte-for-byte. Git stores `/` as its separator on every + // platform, so a backslash here is part of the filename, and + // translating it would name a file that did not change. + let path = path.to_string_lossy().into_owned(); if !path.is_empty() { files.insert(path); } @@ -380,12 +422,48 @@ mod tests { #[test] fn the_newest_usable_scan_wins_within_a_page() { let scans = vec![scan("main", "newest"), scan("main", "older")]; + assert_eq!(any_branch_baseline(&scans).as_deref(), Some("newest")); + } + + #[test] + fn the_branch_being_scanned_is_preferred_over_a_newer_one_elsewhere() { + let scans = vec![scan("main", "newer-on-main"), scan("feature", "on-feature")]; assert_eq!( - usable_baselines(&scans).next().unwrap().git_sha.as_deref(), - Some("newest") + same_branch_baseline(&scans, "feature").as_deref(), + Some("on-feature") ); } + #[test] + fn a_branch_with_no_scan_of_its_own_falls_back_to_any_branch() { + // A feature branch's first scan diffs against trunk rather than going + // full, which is the whole point of the fallback. + let scans = vec![scan("main", "on-main")]; + assert_eq!(same_branch_baseline(&scans, "feature"), None); + assert_eq!(any_branch_baseline(&scans).as_deref(), Some("on-main")); + } + + #[test] + fn unusable_scans_are_skipped_when_picking_a_fallback() { + let mut dirty = scan("main", "dirty"); + dirty.worktree_dirty = Some(true); + let scans = vec![dirty, scan("main", "clean")]; + assert_eq!(any_branch_baseline(&scans).as_deref(), Some("clean")); + assert_eq!( + same_branch_baseline(&scans, "main").as_deref(), + Some("clean") + ); + } + + #[test] + fn a_page_of_nothing_usable_yields_no_baseline() { + let mut pr = scan("main", "pr"); + pr.pull_request_id = Some("42".to_string()); + let scans = vec![pr]; + assert_eq!(same_branch_baseline(&scans, "main"), None); + assert_eq!(any_branch_baseline(&scans), None); + } + /// 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"); diff --git a/src/main.rs b/src/main.rs index 1e934c6..c945af6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -180,8 +180,8 @@ enum Commands { #[arg( long = "skip-if-commit-scanned-recently", - conflicts_with_all = ["only_uncommitted", "target", "scan_type", "policy", "include_image"], - help = "Do not start a new scan when this commit already has a recent completed scan in the project. That scan then drives the rest of the command — results table, --block-on gate, --out-file report — so the pipeline behaves the same either way. Prints CORGEA_SCAN_SKIPPED=true/false so a pipeline can tell the two apart, and fails if no git commit can be resolved. What can be reused is a scan of the whole commit, and no API tells this run how a past scan was scoped or configured, so the flag is refused with --only-uncommitted, --target, --scan-type, --policy and --include-image; with --exclude it warns instead, since a reused scan covers files this run would have skipped." + conflicts_with_all = ["only_uncommitted", "target", "scan_type", "policy", "include_image", "disable_incremental"], + help = "Do not start a new scan when this commit already has a recent completed scan in the project. That scan then drives the rest of the command — results table, --block-on gate, --out-file report — so the pipeline behaves the same either way. Prints CORGEA_SCAN_SKIPPED=true/false so a pipeline can tell the two apart, and fails if no git commit can be resolved. What can be reused is a scan of the whole commit, and no API tells this run how a past scan was scoped or configured, so the flag is refused with --only-uncommitted, --target, --scan-type, --policy, --include-image and --disable-incremental; with --exclude it warns instead, since a reused scan covers files this run would have skipped." )] skip_if_commit_scanned_recently: bool, diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 57483bc..b07eb21 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -12,7 +12,7 @@ //! when it already cannot be used. use crate::common::*; -use hyper::Method; +use hyper::{Method, StatusCode}; use serde_json::{json, Value}; const PROJECT: &str = "cloud-e2e"; @@ -40,6 +40,25 @@ fn baseline_lookup(scans: Vec) -> ExpectedRequest { ) } +/// One page of the baseline lookup, for the walk an old backend forces. +fn baseline_lookup_page(page: u16, total_pages: u32, scans: Vec) -> ExpectedRequest { + expected_request( + "look up a baseline scan to diff against", + move |request| { + assert_authenticated_request(request, Method::GET, "/api/v1/scans")?; + assert_query(request, "project", PROJECT)?; + assert_query(request, "page", &page.to_string())?; + assert_query(request, "engine", "corgea-blast") + }, + json_response(json!({ + "status": "ok", + "page": page, + "total_pages": total_pages, + "scans": 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(); @@ -185,6 +204,99 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { ); } +/// A backend predating the server-side filters returns scans of every kind, so +/// a page can hold nothing usable. The walk is what stops that project from +/// being permanently unable to find a baseline it has. +#[test] +fn a_baseline_on_a_later_page_is_still_found() { + let project = git_project(); + let base_sha = project.sha.clone(); + let head_sha = second_commit(&project); + + let mut unusable = baseline_scan(&head_sha); + unusable["worktree_dirty"] = json!(true); + let expected_base = base_sha.clone(); + + let mut plan = vec![ + verify_request(), + baseline_lookup_page(1, 2, vec![unusable]), + baseline_lookup_page(2, 2, 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, "incremental_base_sha", &expected_base) + }, + 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); + + assert_eq!(output.status.code(), Some(0), "{context}"); +} + +/// A lookup that failed says so. Reporting it as "no earlier scan" tells someone +/// with years of scan history that they have none, and now that incremental is +/// the default, any network blip would say it. +#[test] +fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { + let project = git_project(); + second_commit(&project); + + let mut plan = vec![ + verify_request(), + expected_request( + "fail the baseline lookup", + |request| assert_baseline_lookup_request(request, PROJECT), + json_response_with_status(StatusCode::INTERNAL_SERVER_ERROR, json!({"error": "boom"})), + ), + 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_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("could not be looked up"), "{context}"); + assert!( + !stdout.contains("no earlier completed scan"), + "a lookup failure must not claim the project has no scan history\n{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. From ba2569d77fdb1d7da21035306826ae48421a5cba Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 26 Aug 2026 15:20:04 +0000 Subject: [PATCH 09/11] Take baselines only from trunk Any completed clean scan is a correct thing to diff against, but not a stable one. Falling back to the newest scan on any branch could pick someone else's feature branch, and the findings copied forward would be that branch's rather than this project's. Candidates are now origin/HEAD, then main, then master, and nothing else. A run whose project has no trunk baseline scans everything and names the branches it looked on. origin/HEAD is what the remote advertised as its default when the clone was made -- correct when present, but absent from single-branch and actions/checkout clones and never refreshed after a rename, so main and master follow it rather than replace it. One query per branch, since the branch filter is server-side: a project with heavy feature-branch traffic can push trunk's newest scan past any page limit, and asking for trunk directly cannot miss it that way. The page budget is shared across candidates, and a failed lookup ends the search rather than retrying the same endpoint per branch. Co-authored-by: ibrahim --- src/incremental.rs | 245 +++++++++++-------- src/utils/api.rs | 2 + tests/cloud_commands_e2e/common/mod.rs | 32 ++- tests/cloud_commands_e2e/scan_incremental.rs | 43 ++-- 4 files changed, 199 insertions(+), 123 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index fae7f3f..deede89 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -79,7 +79,7 @@ pub fn resolve_incremental_plan( // 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 { + 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)", @@ -87,12 +87,22 @@ pub fn resolve_incremental_plan( return None; }; - let base_sha = match find_baseline_sha(config, project_name, branch) { + 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 trunks = baseline_branches(&repo); + let base_sha = match find_baseline_sha(config, project_name, &trunks) { Baseline::Found(sha) => sha, Baseline::NotFound => { 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" + "project '{project_name}' has no completed scan of a clean worktree on \ + {}, so there is nothing stable to diff against", + join_or(&trunks) )); return None; } @@ -105,14 +115,6 @@ pub fn resolve_incremental_plan( } }; - 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) => { @@ -168,79 +170,112 @@ enum Baseline { LookupFailed, } -/// Commit of the newest scan this project can be diffed against. +/// The branches a baseline may come from, best first. /// -/// 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) -> Baseline { - 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. An earlier page that did - // answer still counts: that scan is a real baseline. - crate::log::debug(&format!("Baseline scan lookup failed: {e}")); - return match any_branch_fallback { - Some(sha) => Baseline::Found(sha), - None => Baseline::LookupFailed, - }; - } - }; - - let scans = response.scans.unwrap_or_default(); - if scans.is_empty() { - break; +/// Only trunk qualifies. Any completed clean scan is a *correct* thing to diff +/// against, but not a *stable* one: a scan of someone else's feature branch is +/// a baseline whose contents nobody can predict, and the findings copied +/// forward from it would be that branch's, not this project's. Trunk is the +/// line every branch descends from, so it is the only shared reference point. +/// +/// `origin/HEAD` records what the remote advertised as its default when this +/// clone was made. It is absent from single-branch and `actions/checkout` +/// checkouts and is never refreshed after a rename, so `main` and `master` +/// follow it rather than replace it. +fn baseline_branches(repo: &Repository) -> Vec { + let mut branches: Vec = default_branch(repo).into_iter().collect(); + for fallback in ["main", "master"] { + if !branches.iter().any(|branch| branch == fallback) { + branches.push(fallback.to_string()); } + } + branches +} - // Newest first, so the first same-branch match is the best available - // and no later page can improve on it. - if let Some(sha) = same_branch_baseline(&scans, branch) { - return Baseline::Found(sha); - } - if any_branch_fallback.is_none() { - any_branch_fallback = any_branch_baseline(&scans); - } +/// Default branch this clone recorded, or None when it recorded none. +fn default_branch(repo: &Repository) -> Option { + let name = repo + .find_reference("refs/remotes/origin/HEAD") + .ok()? + .symbolic_target()? + .strip_prefix("refs/remotes/origin/")? + .to_string(); + (!name.is_empty() && name != "HEAD").then_some(name) +} - if response - .total_pages - .is_some_and(|total| u32::from(page) >= total) - { - break; - } +fn join_or(branches: &[String]) -> String { + match branches.split_last() { + Some((last, [])) => last.clone(), + Some((last, rest)) => format!("{} or {last}", rest.join(", ")), + None => "any branch".to_string(), } +} + +/// Commit of the newest scan on the first trunk branch that has one. +/// +/// One query per branch, because the branch filter is server-side: a project +/// with heavy feature-branch traffic can push trunk's newest scan far past any +/// page limit, and asking for trunk directly cannot miss it that way. The page +/// budget is shared across branches so the worst case stays bounded. +fn find_baseline_sha(config: &Config, project_name: &str, branches: &[String]) -> Baseline { + let url = config.get_url(); + let mut budget = SCAN_LOOKUP_MAX_PAGES; + + for branch in branches { + let mut page = 1; + while budget > 0 { + budget -= 1; + let response = match api::query_baseline_scans( + &url, + project_name, + BLAST_ENGINE, + branch, + page, + SCAN_LOOKUP_PAGE_SIZE, + ) { + Ok(response) => response, + Err(e) => { + // Proves nothing about this project's history, so it is a + // full scan rather than an error -- but it is a different + // answer from "trunk has no scan", so say which it was. + // Whatever failed is the endpoint, not the branch, so the + // remaining candidates would fail the same way. + crate::log::debug(&format!("Baseline scan lookup failed: {e}")); + return Baseline::LookupFailed; + } + }; - match any_branch_fallback { - Some(sha) => Baseline::Found(sha), - None => Baseline::NotFound, + let scans = response.scans.unwrap_or_default(); + if scans.is_empty() { + break; + } + // Newest first, so the first match on this branch is the best + // available and no later page can improve on it. Matched + // client-side too: a backend that ignored the branch filter would + // otherwise hand back another branch's scan. + if let Some(sha) = branch_baseline(&scans, branch) { + return Baseline::Found(sha); + } + if response + .total_pages + .is_some_and(|total| u32::from(page) >= total) + { + break; + } + page += 1; + } } + + Baseline::NotFound } /// Newest usable scan of `branch` on this page. -fn same_branch_baseline(scans: &[ScanResponse], branch: &str) -> Option { +fn branch_baseline(scans: &[ScanResponse], branch: &str) -> Option { usable_baselines(scans) .find(|scan| scan.branch.as_deref() == Some(branch)) .and_then(|scan| scan.git_sha.clone()) } -/// Newest usable scan on this page, whatever branch it ran on. -fn any_branch_baseline(scans: &[ScanResponse]) -> Option { - usable_baselines(scans) - .next() - .and_then(|scan| scan.git_sha.clone()) -} - /// 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)) @@ -420,48 +455,66 @@ mod tests { } #[test] - fn the_newest_usable_scan_wins_within_a_page() { + fn the_newest_usable_scan_on_the_branch_wins() { let scans = vec![scan("main", "newest"), scan("main", "older")]; - assert_eq!(any_branch_baseline(&scans).as_deref(), Some("newest")); - } - - #[test] - fn the_branch_being_scanned_is_preferred_over_a_newer_one_elsewhere() { - let scans = vec![scan("main", "newer-on-main"), scan("feature", "on-feature")]; - assert_eq!( - same_branch_baseline(&scans, "feature").as_deref(), - Some("on-feature") - ); + assert_eq!(branch_baseline(&scans, "main").as_deref(), Some("newest")); } #[test] - fn a_branch_with_no_scan_of_its_own_falls_back_to_any_branch() { - // A feature branch's first scan diffs against trunk rather than going - // full, which is the whole point of the fallback. - let scans = vec![scan("main", "on-main")]; - assert_eq!(same_branch_baseline(&scans, "feature"), None); - assert_eq!(any_branch_baseline(&scans).as_deref(), Some("on-main")); + fn a_scan_on_another_branch_is_never_the_baseline() { + // A backend that ignored the branch filter would otherwise hand back a + // feature branch's scan as trunk's. + let scans = vec![scan("feature", "on-feature")]; + assert_eq!(branch_baseline(&scans, "main"), None); } #[test] - fn unusable_scans_are_skipped_when_picking_a_fallback() { + fn unusable_scans_on_the_branch_are_skipped() { let mut dirty = scan("main", "dirty"); dirty.worktree_dirty = Some(true); let scans = vec![dirty, scan("main", "clean")]; - assert_eq!(any_branch_baseline(&scans).as_deref(), Some("clean")); - assert_eq!( - same_branch_baseline(&scans, "main").as_deref(), - Some("clean") - ); + assert_eq!(branch_baseline(&scans, "main").as_deref(), Some("clean")); } #[test] fn a_page_of_nothing_usable_yields_no_baseline() { let mut pr = scan("main", "pr"); pr.pull_request_id = Some("42".to_string()); - let scans = vec![pr]; - assert_eq!(same_branch_baseline(&scans, "main"), None); - assert_eq!(any_branch_baseline(&scans), None); + assert_eq!(branch_baseline(&[pr], "main"), None); + } + + #[test] + fn trunk_candidates_fall_back_to_main_then_master() { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = Repository::init(dir.path()).expect("init"); + // No origin/HEAD: single-branch and actions/checkout clones have none. + assert_eq!(default_branch(&repo), None); + assert_eq!(baseline_branches(&repo), vec!["main", "master"]); + } + + #[test] + fn a_recorded_default_branch_leads_and_is_not_repeated() { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = Repository::init(dir.path()).expect("init"); + repo.reference_symbolic( + "refs/remotes/origin/HEAD", + "refs/remotes/origin/trunk", + true, + "test", + ) + .expect("set origin/HEAD"); + + assert_eq!(default_branch(&repo).as_deref(), Some("trunk")); + assert_eq!(baseline_branches(&repo), vec!["trunk", "main", "master"]); + + repo.reference_symbolic( + "refs/remotes/origin/HEAD", + "refs/remotes/origin/main", + true, + "test", + ) + .expect("set origin/HEAD"); + assert_eq!(baseline_branches(&repo), vec!["main", "master"]); } /// Two commits: three files, then one that adds, edits and deletes. diff --git a/src/utils/api.rs b/src/utils/api.rs index 2c56cff..c3dad70 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -866,6 +866,7 @@ pub fn query_baseline_scans( url: &str, project: &str, engine: &str, + branch: &str, page: u16, page_size: u16, ) -> Result> { @@ -876,6 +877,7 @@ pub fn query_baseline_scans( ("page_size", page_size.to_string()), ("project", project.to_string()), ("engine", engine.to_string()), + ("branch", branch.to_string()), ("status", "complete".to_string()), ("exclude_pull_requests", "true".to_string()), // Explicitly clean only. A scan that never reported the flag is diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index 2294b8c..76c5f55 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -310,20 +310,23 @@ pub(crate) fn assert_scan_list_request( assert_query(request, "project", project) } -/// The baseline lookup an incremental scan makes before uploading. +/// One 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. +/// Asserting the filters is the point: they keep this to one request per trunk +/// branch instead of a page walk, and a server dropping them silently returns +/// pull-request and dirty scans for the client to reject. `branch` is asserted +/// because a baseline may only come from trunk. pub(crate) fn assert_baseline_lookup_request( request: &CapturedRequest, project: &str, + branch: &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") + assert_query(request, "worktree_dirty", "false")?; + assert_query(request, "branch", branch) } pub(crate) fn query_value(request: &CapturedRequest, key: &str) -> Result { @@ -812,15 +815,18 @@ pub(crate) fn blast_upload_plan(sha: &str, dirty: bool, include_sca: bool) -> Ve let issue_path = "/api/v1/scan/blast-scan-123/issues".to_string(); 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. + // baseline before uploading -- once per trunk branch, since the fixture + // records no origin/HEAD. 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())), - )); + for branch in ["main", "master"] { + plan.push(expected_request( + "look up a baseline scan to diff against", + move |request| assert_baseline_lookup_request(request, "cloud-e2e", branch), + json_response(scans_response(Vec::new())), + )); + } } plan.extend([ expected_request( diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index b07eb21..2979532 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -23,7 +23,7 @@ fn baseline_scan(sha: &str) -> Value { "id": BASELINE_SCAN, "project": PROJECT, "repo": null, - "branch": "e2e-main", + "branch": "main", "status": "complete", "engine": "corgea-blast", "created_at": "2026-07-30T12:00:00Z", @@ -32,21 +32,36 @@ fn baseline_scan(sha: &str) -> Value { }) } -fn baseline_lookup(scans: Vec) -> ExpectedRequest { +fn baseline_lookup(branch: &'static str, scans: Vec) -> ExpectedRequest { expected_request( "look up a baseline scan to diff against", - move |request| assert_baseline_lookup_request(request, PROJECT), + move |request| assert_baseline_lookup_request(request, PROJECT, branch), json_response(scans_response(scans)), ) } +/// The lookups a fixture repo makes when no trunk branch has a baseline. It +/// records no origin/HEAD, so the candidates are `main` then `master`. +fn baseline_lookups_finding_nothing() -> Vec { + vec![ + baseline_lookup("main", vec![]), + baseline_lookup("master", vec![]), + ] +} + /// One page of the baseline lookup, for the walk an old backend forces. -fn baseline_lookup_page(page: u16, total_pages: u32, scans: Vec) -> ExpectedRequest { +fn baseline_lookup_page( + branch: &'static str, + page: u16, + total_pages: u32, + scans: Vec, +) -> ExpectedRequest { expected_request( "look up a baseline scan to diff against", move |request| { assert_authenticated_request(request, Method::GET, "/api/v1/scans")?; assert_query(request, "project", PROJECT)?; + assert_query(request, "branch", branch)?; assert_query(request, "page", &page.to_string())?; assert_query(request, "engine", "corgea-blast") }, @@ -115,7 +130,7 @@ fn the_upload_carries_the_baseline_commit_and_the_files_that_changed_since_it() let expected_base = base_sha.clone(); let mut plan = vec![ verify_request(), - baseline_lookup(vec![baseline_scan(&base_sha)]), + baseline_lookup("main", vec![baseline_scan(&base_sha)]), start_upload(), expected_request( "upload BLAST archive with the diff", @@ -165,9 +180,9 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { let head_sha = second_commit(&project); let patch_sha = head_sha.clone(); - let mut plan = vec![ - verify_request(), - baseline_lookup(vec![]), + let mut plan = vec![verify_request()]; + plan.extend(baseline_lookups_finding_nothing()); + plan.extend([ start_upload(), expected_request( "upload BLAST archive with no diff", @@ -185,7 +200,7 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { }, json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), ), - ]; + ]); plan.extend(scan_tail()); let api = ApiStub::start(plan); @@ -199,7 +214,7 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { assert_eq!(output.status.code(), Some(0), "{context}"); assert!( - stdout.contains("Scanning every file: no earlier completed scan of a clean worktree"), + stdout.contains("has no completed scan of a clean worktree on main or master"), "{context}" ); } @@ -219,8 +234,8 @@ fn a_baseline_on_a_later_page_is_still_found() { let mut plan = vec![ verify_request(), - baseline_lookup_page(1, 2, vec![unusable]), - baseline_lookup_page(2, 2, vec![baseline_scan(&base_sha)]), + baseline_lookup_page("main", 1, 2, vec![unusable]), + baseline_lookup_page("main", 2, 2, vec![baseline_scan(&base_sha)]), start_upload(), expected_request( "upload BLAST archive with the diff", @@ -260,7 +275,7 @@ fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { verify_request(), expected_request( "fail the baseline lookup", - |request| assert_baseline_lookup_request(request, PROJECT), + |request| assert_baseline_lookup_request(request, PROJECT, "main"), json_response_with_status(StatusCode::INTERNAL_SERVER_ERROR, json!({"error": "boom"})), ), start_upload(), @@ -292,7 +307,7 @@ fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { assert_eq!(output.status.code(), Some(0), "{context}"); assert!(stdout.contains("could not be looked up"), "{context}"); assert!( - !stdout.contains("no earlier completed scan"), + !stdout.contains("no completed scan of a clean worktree"), "a lookup failure must not claim the project has no scan history\n{context}" ); } From 9efadb517a53e55aebf64fbeda195d90a5499765 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 26 Aug 2026 15:45:37 +0000 Subject: [PATCH 10/11] Make --ignore-dirty-worktree cover incremental scans too The flag governed reuse only, so a dirty tree always scanned every file even when the run had explicitly accepted working-tree state. It does not pretend the tree is clean, which would be the dangerous reading: a commit-range diff omits an uncommitted edit, so that file would keep findings taken from content nothing analyzed -- worse than reuse, because the scan looks fresh and is wrong for exactly the files being worked on. Instead the flag moves the far side of the diff to the index and working tree, so edited and untracked files are named and rescanned like any other change. The upload still reports the real dirty status, so the scan can never become a baseline itself. The flag no longer requires --skip-if-commit-scanned-recently; it now means something on its own. Co-authored-by: ibrahim --- src/incremental.rs | 119 ++++++++++++++----- src/main.rs | 4 +- src/scanners/blast.rs | 4 + src/utils/api.rs | 7 ++ tests/cloud_commands_e2e/scan_incremental.rs | 58 +++++++++ tests/cloud_commands_e2e/scan_skip.rs | 41 +++++-- 6 files changed, 189 insertions(+), 44 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index b955f93..44f3ef2 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -51,9 +51,13 @@ 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. + /// Repo-relative paths differing from `base_sha`, including deletions and + /// both sides of a rename. pub changed_files: Vec, + /// Whether the diff measured the working tree rather than a commit. The + /// server refuses a dirty upload otherwise, because a commit-to-commit diff + /// cannot describe one. + pub covers_worktree: bool, } /// What an incremental scan of this commit would cover, or `None` to scan @@ -64,14 +68,18 @@ pub fn resolve_incremental_plan( branch: Option<&str>, head_sha: Option<&str>, worktree_dirty: bool, + ignore_dirty_worktree: 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 { + // A commit-to-commit diff cannot see uncommitted edits, so on a dirty tree + // it leaves modified files off the list and their old findings are copied + // forward as current. --ignore-dirty-worktree does not paper over that; it + // switches the diff to measure the working tree, so those files are named + // and rescanned like any other change. + let covers_worktree = worktree_dirty; + if worktree_dirty && !ignore_dirty_worktree { explain_full_scan( - "this worktree has uncommitted changes, and a commit-to-commit diff cannot see them", + "this worktree has uncommitted changes, and a commit-to-commit diff cannot \ + see them. Pass --ignore-dirty-worktree to diff the working tree instead", ); return None; } @@ -115,7 +123,7 @@ pub fn resolve_incremental_plan( } }; - let changed_files = match changed_files_between(&repo, &base_sha, head_sha) { + let changed_files = match changed_files_since(&repo, &base_sha, head_sha, covers_worktree) { Ok(files) => files, Err(reason) => { explain_full_scan(&reason); @@ -132,23 +140,26 @@ pub fn resolve_incremental_plan( return None; } - match changed_files.len() { - 0 => println!( - "Incremental scan: nothing changed since commit {}. Corgea will carry every \ - finding forward.", + let since = if covers_worktree { + format!( + "commit {} and your uncommitted changes", short_sha(&base_sha) - ), + ) + } else { + format!("commit {}", short_sha(&base_sha)) + }; + match changed_files.len() { + 0 => println!("Incremental scan: nothing changed since {since}. Corgea will carry every finding forward."), 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) + "Incremental scan: {count} file(s) changed since {since}. Corgea will analyze \ + those and carry findings forward for the rest." ), } Some(IncrementalPlan { base_sha, changed_files, + covers_worktree, }) } @@ -300,7 +311,15 @@ fn is_usable_baseline(scan: &ScanResponse) -> bool { && scan.git_sha.as_deref().is_some_and(|sha| !sha.is_empty()) } -/// Every repo-relative path differing between two commits. +/// Every repo-relative path differing from the baseline commit. +/// +/// `include_worktree` decides what the far side of the diff is. False compares +/// two commits, which is exact when the tree is clean. True compares the +/// baseline against the index and working tree, which is what makes a dirty +/// tree scannable: a file edited but not committed differs from the baseline +/// and has to be named, or its old findings would be carried forward over +/// content nothing analyzed. Untracked files count for the same reason — the +/// archive contains them. /// /// 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 @@ -316,10 +335,11 @@ fn is_usable_baseline(scan: &ScanResponse) -> bool { /// 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( +fn changed_files_since( repo: &Repository, base_sha: &str, head_sha: &str, + include_worktree: bool, ) -> Result, String> { let base_tree = commit_tree(repo, base_sha).map_err(|e| { format!( @@ -329,12 +349,17 @@ fn changed_files_between( 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)))?; + let diff = if include_worktree { + let mut options = git2::DiffOptions::new(); + options.include_untracked(true).recurse_untracked_dirs(true); + repo.diff_tree_to_workdir_with_index(Some(&base_tree), Some(&mut options)) + } else { + let head_tree = commit_tree(repo, head_sha) + .map_err(|e| format!("commit {} could not be read ({e})", short_sha(head_sha)))?; + 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. @@ -567,7 +592,7 @@ mod tests { #[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"); + let files = changed_files_since(&repo, &base, &head, false).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"]); @@ -576,11 +601,47 @@ mod tests { #[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) + assert!(changed_files_since(&repo, &head, &head, false) .expect("diff") .is_empty()); } + #[test] + fn a_commit_range_diff_cannot_see_uncommitted_work() { + // Why a dirty tree may not use one: keep.txt differs from what will be + // uploaded, yet the diff does not name it, so its findings would be + // carried forward over content nothing analyzed. + let (dir, repo, _base, head) = repo_with_history(); + fs::write(dir.path().join("keep.txt"), "edited").expect("edit"); + fs::write(dir.path().join("brand-new.txt"), "new").expect("add"); + + let committed = changed_files_since(&repo, &head, &head, false).expect("diff"); + assert!(committed.is_empty()); + } + + #[test] + fn a_worktree_diff_names_edited_and_untracked_files() { + let (dir, repo, _base, head) = repo_with_history(); + fs::write(dir.path().join("keep.txt"), "edited").expect("edit"); + fs::write(dir.path().join("brand-new.txt"), "new").expect("add"); + + let files = changed_files_since(&repo, &head, &head, true).expect("diff"); + + assert_eq!(files, vec!["brand-new.txt", "keep.txt"]); + } + + #[test] + fn a_worktree_diff_still_spans_the_commits_behind_it() { + // The baseline is a commit, so committed changes since it count too -- + // the working tree is the far side of the diff, not the whole of it. + let (dir, repo, base, _head) = repo_with_history(); + fs::write(dir.path().join("keep.txt"), "edited").expect("edit"); + + let files = changed_files_since(&repo, &base, "unused", true).expect("diff"); + + assert_eq!(files, vec!["added.txt", "edit.txt", "gone.txt", "keep.txt"]); + } + /// 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"); @@ -607,7 +668,7 @@ mod tests { 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()) + let err = changed_files_since(&repo, &before.to_string(), &after.to_string(), false) .expect_err("a moved submodule must refuse the diff"); assert!(err.contains("submodule vendor"), "{err}"); @@ -616,7 +677,7 @@ mod tests { #[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) + let err = changed_files_since(&repo, &"0".repeat(40), &head, false) .expect_err("unknown base must fail"); assert!(err.contains("shallow clone"), "{err}"); } diff --git a/src/main.rs b/src/main.rs index c945af6..5e9cd0f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -195,8 +195,7 @@ enum Commands { #[arg( long = "ignore-dirty-worktree", - requires = "skip_if_commit_scanned_recently", - help = "With --skip-if-commit-scanned-recently, reuse a recent scan of this commit even if this worktree is dirty or the prior scan recorded worktree_dirty. A new scan still reports the real dirty status." + help = "Do not let uncommitted changes stop this run from taking a shortcut. For an incremental scan, the diff is measured against the working tree instead of the last commit, so edited and untracked files are analyzed rather than skipped. With --skip-if-commit-scanned-recently, a recent scan of this commit may be reused even though this worktree is dirty or the prior scan recorded worktree_dirty. A new scan still reports the real dirty status." )] ignore_dirty_worktree: bool, }, @@ -860,6 +859,7 @@ fn main() { block_on, only_uncommitted, disable_incremental, + ignore_dirty_worktree, metadata_json, scan_type.clone(), policy.clone(), diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index e94ece0..3c368fa 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -54,6 +54,7 @@ pub fn run( block_on: Option, only_uncommitted: &bool, disable_incremental: &bool, + ignore_dirty_worktree_for_run: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -111,6 +112,7 @@ pub fn run( &project_name, only_uncommitted, disable_incremental, + ignore_dirty_worktree_for_run, metadata, scan_type, policy, @@ -284,6 +286,7 @@ fn start_new_scan( project_name: &str, only_uncommitted: &bool, disable_incremental: &bool, + ignore_dirty_worktree: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -536,6 +539,7 @@ fn start_new_scan( // 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), + *ignore_dirty_worktree, ) }; println!("\n\nSubmitting scan to Corgea:"); diff --git a/src/utils/api.rs b/src/utils/api.rs index c3dad70..29d3272 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -401,6 +401,13 @@ pub fn upload_zip( "incremental_changed_files", multipart::Part::text(changed_files), ); + // Tells the server the list describes the working tree, not + // just a commit range, which is the only way it can accept a + // diff from a dirty upload. + if plan.covers_worktree { + form = + form.part("incremental_covers_worktree", multipart::Part::text("true")); + } } Err(e) => debug(&format!( "Could not serialize the incremental file list, scanning every file: {e}" diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 2979532..d382a67 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -453,6 +453,64 @@ fn a_directory_that_is_not_a_git_repository_scans_everything() { ); } +/// `--ignore-dirty-worktree` does not pretend the tree is clean: it moves the +/// far side of the diff to the working tree, so the edited file is named and +/// rescanned rather than keeping findings nothing analyzed. +#[test] +fn ignore_dirty_worktree_diffs_the_working_tree_instead_of_refusing() { + let project = git_project(); + let base_sha = project.sha.clone(); + std::fs::write(project.path().join("main.py"), "print('uncommitted')\n") + .expect("dirty the tree"); + + let expected_base = base_sha.clone(); + let mut plan = vec![ + verify_request(), + baseline_lookup("main", vec![baseline_scan(&base_sha)]), + start_upload(), + expected_request( + "upload BLAST archive with a worktree diff", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + // Still reported dirty: the upload is not a snapshot of the + // commit, and the scan must never become a baseline itself. + assert_multipart_text_field(request, "dirty", "true")?; + assert_multipart_text_field(request, "incremental_base_sha", &expected_base)?; + assert_multipart_text_field( + request, + "incremental_changed_files", + r#"["main.py"]"#, + )?; + assert_multipart_text_field(request, "incremental_covers_worktree", "true") + }, + 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", + "--ignore-dirty-worktree", + "--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("and your uncommitted changes"), "{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. diff --git a/tests/cloud_commands_e2e/scan_skip.rs b/tests/cloud_commands_e2e/scan_skip.rs index f02183b..6b073b1 100644 --- a/tests/cloud_commands_e2e/scan_skip.rs +++ b/tests/cloud_commands_e2e/scan_skip.rs @@ -32,6 +32,16 @@ fn prior_scan(sha: &str, created_at: &str) -> Value { }) } +/// The trunk baseline lookup a dirty run makes once --ignore-dirty-worktree +/// puts incremental back on the table. +fn baseline_lookup_for_branch(branch: &'static str, scans: Vec) -> ExpectedRequest { + expected_request( + "look up a baseline scan to diff against", + move |request| assert_baseline_lookup_request(request, PROJECT, branch), + json_response(scans_response(scans)), + ) +} + fn commit_lookup(sha: &str, scans: Vec) -> ExpectedRequest { let sha = sha.to_string(); expected_request( @@ -428,14 +438,17 @@ fn ignore_dirty_worktree_reuses_a_prior_dirty_scan() { assert!(!stdout.contains("Scanning with BLAST"), "{context}"); } -/// The override is only a reuse rule. When nothing can be reused, the new -/// scan still sends the real dirty status. +/// When nothing can be reused, the new scan still sends the real dirty status. +/// The override does not launder it -- it only lets the diff measure the +/// working tree, which is why a baseline lookup follows the reuse lookup here. #[test] fn ignore_dirty_worktree_still_uploads_dirty_when_nothing_is_reused() { let project = git_project(); std::fs::write(project.path().join("main.py"), "print('dirty')\n") .expect("modify tracked file"); let mut plan = blast_upload_plan(&project.sha, true, false); + plan.insert(1, baseline_lookup_for_branch("master", vec![])); + plan.insert(1, baseline_lookup_for_branch("main", vec![])); plan.insert(1, commit_lookup(&project.sha, vec![])); let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path()); @@ -661,25 +674,27 @@ fn the_window_cannot_be_set_without_the_skip_flag() { ); } -/// `--ignore-dirty-worktree` only changes reuse; it is meaningless without -/// `--skip-if-commit-scanned-recently`. +/// `--ignore-dirty-worktree` stands alone now: it governs the incremental diff +/// as well as reuse, so a run may pass it without the reuse flag. Covered end +/// to end in `scan_incremental`; this only asserts clap accepts it. #[test] -fn ignore_dirty_worktree_cannot_be_set_without_the_skip_flag() { - let api = ApiStub::start(Vec::new()); +fn ignore_dirty_worktree_may_be_set_without_the_skip_flag() { let project = git_project(); + let api = ApiStub::start(blast_upload_plan(&project.sha, false, false)); let (mut command, _home) = cloud_command(&api, project.path()); - command.args(["scan", "blast", "--ignore-dirty-worktree"]); + command.args([ + "scan", + "blast", + "--ignore-dirty-worktree", + "--project-name", + PROJECT, + ]); let output = run_with_timeout(command, &api); let transcript = api.assert_finished(); let context = output_context(&output, &transcript); - let stderr = String::from_utf8_lossy(&output.stderr); - assert_eq!(output.status.code(), Some(2), "{context}"); - assert!( - stderr.contains("--skip-if-commit-scanned-recently"), - "{context}" - ); + assert_eq!(output.status.code(), Some(0), "{context}"); } /// A backend that predates the `sha` filter answers with the project's scans From 6c76a5b331e20ef700c0c1042ba38976b1da3995 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 27 Aug 2026 08:38:54 +0000 Subject: [PATCH 11/11] Trim the incremental scan notice Drops the explanation of what Corgea does with the diff -- the line is printed on every scan and only the count and commit change. Also fixes the '1 file(s)' pluralization. Co-authored-by: ibrahim --- src/incremental.rs | 8 +++----- tests/cloud_commands_e2e/scan_incremental.rs | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 44f3ef2..8a8523b 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -149,11 +149,9 @@ pub fn resolve_incremental_plan( format!("commit {}", short_sha(&base_sha)) }; match changed_files.len() { - 0 => println!("Incremental scan: nothing changed since {since}. Corgea will carry every finding forward."), - count => println!( - "Incremental scan: {count} file(s) changed since {since}. Corgea will analyze \ - those and carry findings forward for the rest." - ), + 0 => println!("Incremental scan: nothing changed since {since}."), + 1 => println!("Incremental scan: 1 file changed since {since}."), + count => println!("Incremental scan: {count} files changed since {since}."), } Some(IncrementalPlan { diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index d382a67..c9385ba 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -167,7 +167,7 @@ fn the_upload_carries_the_baseline_commit_and_the_files_that_changed_since_it() assert_eq!(output.status.code(), Some(0), "{context}"); assert!( - stdout.contains("Incremental scan: 2 file(s) changed since commit"), + stdout.contains("Incremental scan: 2 files changed since commit"), "{context}" ); }