From 1d741175c56a6742beaa102691b8f4e036262a94 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 10:18:11 +0000 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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)]