From 638d2cf772c45be053aecacb9a708ba88ca2f082 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 24 Sep 2026 09:06:35 +0000 Subject: [PATCH 1/5] Find an incremental baseline by commit ancestry on a detached HEAD A detached CI checkout reports no branch, so the baseline lookup asked only about trunk and never found the pipeline's own earlier scans, which recorded no branch either. When the checkout names no branch, pick the recent scan whose commit is nearest behind HEAD (any branch, including none), then fall back to the trunk lookup. The git diff no longer requires a branch name, which it never used. Co-authored-by: ibrahim --- src/incremental.rs | 425 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 392 insertions(+), 33 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 18b8e42..2677a42 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -7,8 +7,11 @@ //! //! Finding what to diff *against* is the same in both directions: ask the //! server for the newest completed scan of this branch, then of trunk if this -//! branch has never been scanned. What changed since it can then be measured -//! two ways. +//! branch has never been scanned. When the checkout names no branch -- the +//! detached HEAD a CI checkout usually lands on, where earlier runs of the same +//! pipeline recorded no branch either -- the scan of the nearest commit in this +//! checkout's own history is tried first, whatever branch it was recorded +//! under. What changed since the baseline can then be measured two ways. //! //! The first is the file checksums that scan uploaded, fetched and subtracted //! from this run's. It needs no git history, so it is the only one that works @@ -47,7 +50,7 @@ use crate::manifest::{Manifest, MANIFEST_VERSION}; use crate::scanners::blast::{classify_scan_status, ScanState}; use crate::utils::api::{self, ScanResponse}; use git2::Repository; -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; /// How many of the project's scans to read at a time, newest first. const SCAN_LOOKUP_PAGE_SIZE: u16 = 30; @@ -139,10 +142,11 @@ impl IncrementalPlan { /// booleans and a caller listing them positionally can swap two without the /// compiler noticing — which would quietly change how the scan is scoped. pub struct DiffSources<'a> { - /// Branch the upload reports. Names where a baseline may come from, and - /// the git diff needs it to have one at all. + /// Branch the upload reports. Names where a baseline may come from; when + /// absent, a baseline is looked for in this commit's history first. pub branch: Option<&'a str>, - /// Commit the upload reports, the near side of a git diff. + /// Commit the upload reports: what a baseline found without a branch has + /// to be an ancestor of, and the near side of a git diff. pub head_sha: Option<&'a str>, /// Whether the worktree holds edits a commit-to-commit diff cannot see. pub worktree_dirty: bool, @@ -189,6 +193,7 @@ fn plan_diff( // Optional, because a checksum diff needs no repository. Only the git diff // below does, and it reports its absence by its real name. let repo = Repository::discover(".").ok(); + let checksums_usable = sources.manifest.is_some(); // Branch names are only meaningful where there is a clone to read them // from. Without one, take the project's newest usable scan whatever branch // it names -- including none, which is what a scan uploaded from a @@ -198,12 +203,40 @@ fn plan_diff( .as_ref() .map(|repo| baseline_branches(repo, sources.branch.filter(|name| !name.is_empty()))); - let lookup = find_baseline( - config, - project_name, - candidates.as_deref(), - sources.manifest.is_some(), - ); + // With no branch to name -- a detached HEAD, which is how CI checkouts + // usually land -- the name lookup can only ask about trunk, and misses the + // earlier runs of this same pipeline, which recorded no branch either. + // Ancestry finds them by commit instead. A named branch keeps the name + // lookup: its own last scan is the baseline it has always diffed against. + let branch_known = sources.branch.is_some_and(|name| !name.is_empty()); + let ancestry = match (repo.as_ref(), sources.head_sha) { + (Some(repo), Some(head)) if !branch_known => Ancestry::new(repo, head), + _ => None, + }; + let ancestry_tried = ancestry.is_some(); + let lookup = match ancestry { + Some(mut ancestry) => match find_ancestor_baseline( + config, + project_name, + &mut ancestry, + checksums_usable, + can_git_diff(sources), + ) { + BaselineLookup::NotFound => find_baseline( + config, + project_name, + candidates.as_deref(), + checksums_usable, + ), + found_or_failed => found_or_failed, + }, + None => find_baseline( + config, + project_name, + candidates.as_deref(), + checksums_usable, + ), + }; let baseline = match lookup { BaselineLookup::Found(scan) => scan, BaselineLookup::NotFound => { @@ -211,6 +244,10 @@ fn plan_diff( "project '{project_name}' has no completed scan {} that could be diffed \ against, so there is nothing to compare this one to", match &candidates { + Some(candidates) if ancestry_tried => format!( + "among its recent scans of this commit's history, nor on {}", + join_or(candidates) + ), Some(candidates) => format!("on {}", join_or(candidates)), None => "of its whole state".to_string(), } @@ -301,21 +338,22 @@ fn plan_git_diff( } // 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), Some(repo), Some(base_sha)) = ( - sources.branch, - sources.head_sha, - repo, - baseline.sha.as_deref(), - ) else { + // and a scan started below the repo root — none of which report RepoInfo + // to the upload either. A detached HEAD is not one of them: a diff between + // two commits needs no branch. + let (Some(head_sha), Some(repo)) = (sources.head_sha, repo) else { return Err( - "there is no git branch and commit to diff from either (not a git \ - repository, no commit yet, a detached HEAD, or a scan started below the \ - repository root)" + "there is no git commit to diff from either (not a git repository, no \ + commit yet, or a scan started below the repository root)" .to_string(), ); }; + let Some(base_sha) = baseline.sha.as_deref() else { + return Err(format!( + "the {} recorded no commit for a git diff to start from", + baseline.describe() + )); + }; let changed_files = changed_files_since(repo, base_sha, head_sha, covers_worktree)?; let since = if covers_worktree { @@ -401,11 +439,38 @@ struct BaselineScan { /// Digest of the file manifest it stored, `None` when it stored none. manifest_root: Option, manifest_version: Option, + /// Commits between its commit and this run's, when it was picked for + /// being in this run's history rather than for the branch it names. + commits_back: Option, } impl BaselineScan { + fn from_response(scan: &ScanResponse) -> Self { + BaselineScan { + id: scan.id.clone(), + sha: scan.git_sha.clone().filter(|sha| !sha.is_empty()), + branch: scan.branch.clone(), + manifest_root: scan + .file_manifest_root + .clone() + .filter(|root| !root.is_empty()), + manifest_version: scan.file_manifest_version.clone(), + commits_back: None, + } + } + /// How to refer to this scan in the one line the run prints. fn describe(&self) -> String { + if let (Some(commits_back), Some(sha)) = (self.commits_back, &self.sha) { + return match commits_back { + 0 => format!("scan of this commit ({})", short_sha(sha)), + 1 => format!("scan of ancestor commit {} (1 commit back)", short_sha(sha)), + n => format!( + "scan of ancestor commit {} ({n} commits back)", + short_sha(sha) + ), + }; + } let what = match &self.branch { Some(branch) => format!("last scan of {branch}"), None => "last scan of this project".to_string(), @@ -623,16 +688,144 @@ fn branch_baseline( } else { newest? }; - Some(BaselineScan { - id: scan.id.clone(), - sha: scan.git_sha.clone().filter(|sha| !sha.is_empty()), - branch: scan.branch.clone(), - manifest_root: scan - .file_manifest_root - .clone() - .filter(|root| !root.is_empty()), - manifest_version: scan.file_manifest_version.clone(), - }) + Some(BaselineScan::from_response(scan)) +} + +/// Whether this run could diff from a baseline's commit, as opposed to only +/// from its stored checksums. Mirrors the refusals in `plan_git_diff`. +fn can_git_diff(sources: &DiffSources<'_>) -> bool { + !sources.exclude_narrowed && (!sources.worktree_dirty || sources.ignore_dirty_worktree) +} + +/// Where scanned commits sit relative to this run's, read from this clone. +struct Ancestry<'repo> { + repo: &'repo Repository, + head: git2::Oid, + /// Per commit, since many scans share one and each answer is a graph walk. + known: HashMap>, +} + +impl<'repo> Ancestry<'repo> { + /// None when `head_sha` is not a commit this clone holds. + fn new(repo: &'repo Repository, head_sha: &str) -> Option { + let head = repo + .find_commit(git2::Oid::from_str(head_sha).ok()?) + .ok()? + .id(); + Some(Ancestry { + repo, + head, + known: HashMap::new(), + }) + } + + /// Commits reachable from HEAD but not from `sha`, or None when `sha` is + /// not an ancestor of HEAD (or HEAD itself). A commit this clone does not + /// hold -- beyond a shallow clone's depth, or on a branch never fetched -- + /// cannot be shown to be one, so it is None as well. + fn commits_back(&mut self, sha: &str) -> Option { + if let Some(known) = self.known.get(sha) { + return *known; + } + let answer = git2::Oid::from_str(sha) + .ok() + .and_then(|oid| self.repo.find_commit(oid).ok()) + .and_then(|base| self.repo.graph_ahead_behind(self.head, base.id()).ok()) + .and_then(|(ahead, behind)| (behind == 0).then_some(ahead)); + self.known.insert(sha.to_string(), answer); + answer + } +} + +/// The recent scan whose commit is nearest behind HEAD, on any branch. Asked +/// only when this checkout names no branch to look a baseline up by. +/// +/// Any branch, including none, because ancestry settles what the branch name +/// was standing in for. A scan of a commit HEAD descends from covered a tree +/// this history produced, so the diff from it is exactly what this history has +/// done since -- whether the scan was recorded under this branch, under trunk, +/// under the branch this one forked from, or, from a detached CI checkout, +/// under no branch at all. The last is what every earlier run of a pipeline +/// that checks out detached records, and a lookup keyed on names never finds +/// them. +/// +/// Nearest wins because it leaves the fewest files to rescan. Stops at the +/// first page holding any ancestor: scans arrive newest first, and an older +/// one being of a nearer commit is the unusual case not worth reading the +/// whole history for. +fn find_ancestor_baseline( + config: &Config, + project_name: &str, + ancestry: &mut Ancestry<'_>, + checksums_usable: bool, + git_diffable: bool, +) -> BaselineLookup { + let url = config.get_url(); + for page in 1..=SCAN_LOOKUP_MAX_PAGES { + let response = match api::query_baseline_scans( + &url, + project_name, + BLAST_ENGINE, + None, + // Only a checksum baseline may have been dirty, so without one + // the server can drop what could never qualify. + !checksums_usable, + page, + SCAN_LOOKUP_PAGE_SIZE, + ) { + Ok(response) => response, + Err(e) => { + crate::log::debug(&format!("Baseline scan lookup failed: {e}")); + return BaselineLookup::LookupFailed; + } + }; + let scans = response.scans.unwrap_or_default(); + if scans.is_empty() { + break; + } + if let Some(scan) = + nearest_ancestor_baseline(&scans, ancestry, checksums_usable, git_diffable) + { + return BaselineLookup::Found(scan); + } + if response + .total_pages + .is_some_and(|total| u32::from(page) >= total) + { + break; + } + } + BaselineLookup::NotFound +} + +/// The scan on this page nearest behind HEAD that this run can diff from. +/// +/// A scan only diffable by its commit is no use to a run that cannot git diff +/// (a dirty tree without `--ignore-dirty-worktree`, or an `--exclude`d +/// archive), so there it has to carry checksums. Between scans of the same +/// commit, checksums win, then recency. +fn nearest_ancestor_baseline( + scans: &[ScanResponse], + ancestry: &mut Ancestry<'_>, + checksums_usable: bool, + git_diffable: bool, +) -> Option { + scans + .iter() + .enumerate() + .filter(|(_, scan)| is_usable_baseline(scan, checksums_usable)) + .filter(|(_, scan)| git_diffable || (checksums_usable && has_readable_checksums(scan))) + .filter_map(|(position, scan)| { + let sha = scan.git_sha.as_deref().filter(|sha| !sha.is_empty())?; + let commits_back = ancestry.commits_back(sha)?; + let without_checksums = !(checksums_usable && has_readable_checksums(scan)); + Some(((commits_back, without_checksums, position), scan)) + }) + .min_by_key(|(rank, _)| *rank) + .map(|((commits_back, _, _), scan)| BaselineScan { + commits_back: Some(commits_back), + ..BaselineScan::from_response(scan) + }) } /// Whether `scan` may be diffed against, given what this run can diff with. @@ -1227,6 +1420,172 @@ mod tests { assert!(err.contains("submodule vendor"), "{err}"); } + /// Commit with `parents`, adding `name` to the first parent's tree. + /// Leaves HEAD alone, so a test can grow side branches off any commit. + fn commit_on(repo: &Repository, parents: &[git2::Oid], name: &str) -> git2::Oid { + let sig = git2::Signature::now("t", "t@example.com").expect("sig"); + let parents: Vec = parents + .iter() + .map(|oid| repo.find_commit(*oid).expect("parent")) + .collect(); + let parent_tree = parents.first().map(|c| c.tree().expect("parent tree")); + let mut builder = repo.treebuilder(parent_tree.as_ref()).expect("treebuilder"); + let blob = repo.blob(name.as_bytes()).expect("blob"); + builder.insert(name, blob, 0o100644).expect("insert"); + let tree = repo + .find_tree(builder.write().expect("write tree")) + .expect("find tree"); + let parent_refs: Vec<&git2::Commit> = parents.iter().collect(); + repo.commit(None, &sig, &sig, name, &tree, &parent_refs) + .expect("commit") + } + + /// root -> near -> head on one line, and `side` forked off root. + struct Lineage { + _dir: tempfile::TempDir, + repo: Repository, + root: String, + near: String, + head: String, + side: String, + } + + fn lineage() -> Lineage { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = Repository::init(dir.path()).expect("init"); + let root = commit_on(&repo, &[], "root.txt"); + let near = commit_on(&repo, &[root], "near.txt"); + let head = commit_on(&repo, &[near], "head.txt"); + let side = commit_on(&repo, &[root], "side.txt"); + Lineage { + _dir: dir, + repo, + root: root.to_string(), + near: near.to_string(), + head: head.to_string(), + side: side.to_string(), + } + } + + /// A scan uploaded from a detached CI checkout: a commit, no branch. + fn detached_scan(sha: &str) -> ScanResponse { + let mut scan = scan("unused", sha); + scan.branch = None; + scan + } + + #[test] + fn a_detached_head_finds_the_nearest_ancestor_scan_on_any_branch() { + let l = lineage(); + let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + // Newest first: a scan of a sibling branch, one of a commit this clone + // never fetched, then two ancestors -- the nearer recorded detached. + let scans = vec![ + scan("feature", &l.side), + scan("other", &"f".repeat(40)), + scan("main", &l.root), + detached_scan(&l.near), + ]; + + let picked = + nearest_ancestor_baseline(&scans, &mut ancestry, false, true).expect("a baseline"); + + assert_eq!(picked.sha.as_deref(), Some(l.near.as_str())); + assert_eq!(picked.commits_back, Some(1)); + assert_eq!( + picked.describe(), + format!("scan of ancestor commit {} (1 commit back)", &l.near[..7]) + ); + } + + #[test] + fn a_scan_of_head_itself_is_zero_commits_back() { + let l = lineage(); + let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let scans = vec![scan("main", &l.near), detached_scan(&l.head)]; + + let picked = + nearest_ancestor_baseline(&scans, &mut ancestry, false, true).expect("a baseline"); + + assert_eq!(picked.commits_back, Some(0)); + } + + #[test] + fn no_ancestor_on_the_page_means_no_ancestry_baseline() { + let l = lineage(); + let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let scans = vec![scan("feature", &l.side), scan_without_git("no-git")]; + + assert_eq!( + nearest_ancestor_baseline(&scans, &mut ancestry, true, true), + None + ); + } + + #[test] + fn between_scans_of_one_commit_the_one_with_checksums_wins() { + let l = lineage(); + let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let mut plain = detached_scan(&l.near); + plain.id = "plain".to_string(); + let mut hashed = with_checksums(detached_scan(&l.near)); + hashed.id = "hashed".to_string(); + + let picked = nearest_ancestor_baseline(&[plain, hashed], &mut ancestry, true, true) + .expect("a baseline"); + + assert_eq!(picked.id, "hashed"); + } + + #[test] + fn a_run_that_cannot_git_diff_needs_an_ancestor_with_checksums() { + // A dirty tree without --ignore-dirty-worktree, or an --exclude run: + // the nearer scan could only be diffed from its commit. + let l = lineage(); + let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let scans = vec![ + detached_scan(&l.near), + with_checksums(detached_scan(&l.root)), + ]; + + let picked = + nearest_ancestor_baseline(&scans, &mut ancestry, true, false).expect("a baseline"); + + assert_eq!(picked.sha.as_deref(), Some(l.root.as_str())); + assert_eq!(picked.commits_back, Some(2)); + } + + #[test] + fn a_head_this_clone_does_not_hold_offers_no_ancestry() { + let l = lineage(); + assert!(Ancestry::new(&l.repo, &"0".repeat(40)).is_none()); + assert!(Ancestry::new(&l.repo, "not-a-sha").is_none()); + } + + #[test] + fn a_git_diff_needs_no_branch() { + // A detached HEAD reports a commit and no branch; a diff between two + // commits needs nothing more. + let (_dir, repo, base, head) = repo_with_history(); + let baseline = BaselineScan::from_response(&detached_scan(&base)); + let sources = DiffSources { + branch: None, + head_sha: Some(&head), + worktree_dirty: false, + ignore_dirty_worktree: false, + exclude_narrowed: false, + manifest: None, + }; + + let (plan, _) = plan_git_diff(&baseline, Some(&repo), &sources).expect("a plan"); + + assert_eq!(plan.base, BaselineRef::Commit(base)); + assert_eq!( + plan.changed_files, + vec!["added.txt", "edit.txt", "gone.txt"] + ); + } + #[test] fn a_base_commit_this_clone_does_not_have_is_reported_not_panicked() { let (_dir, repo, _base, head) = repo_with_history(); From 40b3f1f32a350177bb83f97934b0b7cbe3e12599 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 29 Sep 2026 13:27:17 +0000 Subject: [PATCH 2/5] Look up an ancestor baseline by commit list, for named branches too Walk HEAD's nearest 50 commits locally (breadth-first over every parent) and ask the scan list for scans of any of them in one request, instead of paging the project's recent scans and testing each for ancestry. Matches are re-checked client-side for backends that ignore the list. Runs after the branch's own lookup and before trunk's, so the first scan of a branch whose trunk is not main or master diffs against the commit it forked from, and a detached HEAD finds its pipeline's earlier runs. Co-authored-by: ibrahim --- src/incremental.rs | 290 +++++++++++-------- src/utils/api.rs | 40 ++- tests/cloud_commands_e2e/common/mod.rs | 95 ++++-- tests/cloud_commands_e2e/scan_incremental.rs | 93 +++++- 4 files changed, 367 insertions(+), 151 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 2677a42..920256b 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -6,12 +6,12 @@ //! commits. This module works the diff out where the scan is run. //! //! Finding what to diff *against* is the same in both directions: ask the -//! server for the newest completed scan of this branch, then of trunk if this -//! branch has never been scanned. When the checkout names no branch -- the -//! detached HEAD a CI checkout usually lands on, where earlier runs of the same -//! pipeline recorded no branch either -- the scan of the nearest commit in this -//! checkout's own history is tried first, whatever branch it was recorded -//! under. What changed since the baseline can then be measured two ways. +//! server for the newest completed scan of this branch. If this branch has +//! never been scanned -- or the checkout names none, the detached HEAD a CI +//! checkout usually lands on -- take the scan of the nearest commit in this +//! checkout's recent history, whatever branch it was recorded under, and only +//! then the newest scan of trunk. What changed since the baseline can then be +//! measured two ways. //! //! The first is the file checksums that scan uploaded, fetched and subtracted //! from this run's. It needs no git history, so it is the only one that works @@ -50,11 +50,17 @@ use crate::manifest::{Manifest, MANIFEST_VERSION}; use crate::scanners::blast::{classify_scan_status, ScanState}; use crate::utils::api::{self, ScanResponse}; use git2::Repository; -use std::collections::{BTreeSet, HashMap}; +use std::collections::{BTreeSet, HashMap, VecDeque}; /// How many of the project's scans to read at a time, newest first. const SCAN_LOOKUP_PAGE_SIZE: u16 = 30; +/// How many of this checkout's nearest commits a baseline is looked for among. +/// The most the scan list's `sha` filter takes in one request, and the most +/// scans per page it returns. +const ANCESTOR_LOOKUP_COMMITS: usize = 50; +const ANCESTOR_LOOKUP_PAGE_SIZE: u16 = 50; + /// Backstop on pages walked looking for a baseline. /// /// The server filters out scans that cannot be a baseline, so the answer is @@ -142,11 +148,10 @@ impl IncrementalPlan { /// booleans and a caller listing them positionally can swap two without the /// compiler noticing — which would quietly change how the scan is scoped. pub struct DiffSources<'a> { - /// Branch the upload reports. Names where a baseline may come from; when - /// absent, a baseline is looked for in this commit's history first. + /// Branch the upload reports. Its own last scan is the preferred baseline. pub branch: Option<&'a str>, - /// Commit the upload reports: what a baseline found without a branch has - /// to be an ancestor of, and the near side of a git diff. + /// Commit the upload reports: what a baseline found by ancestry has to be + /// an ancestor of, and the near side of a git diff. pub head_sha: Option<&'a str>, /// Whether the worktree holds edits a commit-to-commit diff cannot see. pub worktree_dirty: bool, @@ -199,43 +204,40 @@ fn plan_diff( // it names -- including none, which is what a scan uploaded from a // directory with no git records, and so what the earlier runs of *this* // pipeline recorded. - let candidates = repo - .as_ref() - .map(|repo| baseline_branches(repo, sources.branch.filter(|name| !name.is_empty()))); - - // With no branch to name -- a detached HEAD, which is how CI checkouts - // usually land -- the name lookup can only ask about trunk, and misses the - // earlier runs of this same pipeline, which recorded no branch either. - // Ancestry finds them by commit instead. A named branch keeps the name - // lookup: its own last scan is the baseline it has always diffed against. - let branch_known = sources.branch.is_some_and(|name| !name.is_empty()); + let branch = sources.branch.filter(|name| !name.is_empty()); + let candidates = repo.as_ref().map(|repo| baseline_branches(repo, branch)); let ancestry = match (repo.as_ref(), sources.head_sha) { - (Some(repo), Some(head)) if !branch_known => Ancestry::new(repo, head), + (Some(repo), Some(head)) => Ancestry::walk(repo, head), _ => None, }; - let ancestry_tried = ancestry.is_some(); - let lookup = match ancestry { - Some(mut ancestry) => match find_ancestor_baseline( - config, - project_name, - &mut ancestry, - checksums_usable, - can_git_diff(sources), - ) { - BaselineLookup::NotFound => find_baseline( - config, - project_name, - candidates.as_deref(), - checksums_usable, - ), - found_or_failed => found_or_failed, - }, - None => find_baseline( - config, - project_name, - candidates.as_deref(), - checksums_usable, - ), + + let lookup = match &candidates { + Some(candidates) => { + // `baseline_branches` puts the branch being scanned first. + let (own, trunks) = candidates.split_at(usize::from(branch.is_some())); + let mut lookup = find_baseline(config, project_name, Some(own), checksums_usable); + // Between this branch's own history and trunk's newest scan, + // because the scan of a commit this one descends from differs by + // only what this history has done since. Trunk's newest also + // differs by whatever trunk did after this branch forked, and may + // not be this branch's trunk at all where that is not main or + // master. It also finds the earlier runs of a detached CI pipeline, + // which recorded no branch for a name to match. + if let (BaselineLookup::NotFound, Some(ancestry)) = (&lookup, &ancestry) { + lookup = find_ancestor_baseline( + config, + project_name, + ancestry, + checksums_usable, + can_git_diff(sources), + ); + } + if lookup == BaselineLookup::NotFound { + lookup = find_baseline(config, project_name, Some(trunks), checksums_usable); + } + lookup + } + None => find_baseline(config, project_name, None, checksums_usable), }; let baseline = match lookup { BaselineLookup::Found(scan) => scan, @@ -243,13 +245,15 @@ fn plan_diff( return Err(format!( "project '{project_name}' has no completed scan {} that could be diffed \ against, so there is nothing to compare this one to", - match &candidates { - Some(candidates) if ancestry_tried => format!( - "among its recent scans of this commit's history, nor on {}", - join_or(candidates) + match (&candidates, &ancestry) { + (Some(candidates), Some(ancestry)) => format!( + "on {}, nor of any of the {} most recent commits in this checkout's \ + history", + join_or(candidates), + ancestry.shas.len() ), - Some(candidates) => format!("on {}", join_or(candidates)), - None => "of its whole state".to_string(), + (Some(candidates), None) => format!("on {}", join_or(candidates)), + (None, _) => "of its whole state".to_string(), } )) } @@ -697,81 +701,81 @@ fn can_git_diff(sources: &DiffSources<'_>) -> bool { !sources.exclude_narrowed && (!sources.worktree_dirty || sources.ignore_dirty_worktree) } -/// Where scanned commits sit relative to this run's, read from this clone. -struct Ancestry<'repo> { - repo: &'repo Repository, - head: git2::Oid, - /// Per commit, since many scans share one and each answer is a graph walk. - known: HashMap>, +/// HEAD and the commits nearest behind it in this clone, with how far back +/// each sits. +struct Ancestry { + /// Nearest first, HEAD leading. + shas: Vec, + commits_back: HashMap, } -impl<'repo> Ancestry<'repo> { - /// None when `head_sha` is not a commit this clone holds. - fn new(repo: &'repo Repository, head_sha: &str) -> Option { - let head = repo - .find_commit(git2::Oid::from_str(head_sha).ok()?) - .ok()? - .id(); - Some(Ancestry { - repo, - head, - known: HashMap::new(), - }) - } - - /// Commits reachable from HEAD but not from `sha`, or None when `sha` is - /// not an ancestor of HEAD (or HEAD itself). A commit this clone does not - /// hold -- beyond a shallow clone's depth, or on a branch never fetched -- - /// cannot be shown to be one, so it is None as well. - fn commits_back(&mut self, sha: &str) -> Option { - if let Some(known) = self.known.get(sha) { - return *known; +impl Ancestry { + /// Breadth-first over every parent, so the far side of a merge counts as + /// history too, stopping at `ANCESTOR_LOOKUP_COMMITS`. None when + /// `head_sha` is not a commit this clone holds. A shallow clone's history + /// ends at its depth, and so does this. + fn walk(repo: &Repository, head_sha: &str) -> Option { + let head = repo.find_commit(git2::Oid::from_str(head_sha).ok()?).ok()?; + let mut ancestry = Ancestry { + shas: vec![head.id().to_string()], + commits_back: HashMap::from([(head.id().to_string(), 0)]), + }; + let mut queue = VecDeque::from([(head, 0)]); + while let Some((commit, depth)) = queue.pop_front() { + for parent_id in commit.parent_ids() { + if ancestry.shas.len() >= ANCESTOR_LOOKUP_COMMITS { + return Some(ancestry); + } + let sha = parent_id.to_string(); + if ancestry.commits_back.contains_key(&sha) { + continue; + } + let Ok(parent) = repo.find_commit(parent_id) else { + continue; + }; + ancestry.commits_back.insert(sha.clone(), depth + 1); + ancestry.shas.push(sha); + queue.push_back((parent, depth + 1)); + } } - let answer = git2::Oid::from_str(sha) - .ok() - .and_then(|oid| self.repo.find_commit(oid).ok()) - .and_then(|base| self.repo.graph_ahead_behind(self.head, base.id()).ok()) - .and_then(|(ahead, behind)| (behind == 0).then_some(ahead)); - self.known.insert(sha.to_string(), answer); - answer + Some(ancestry) } } -/// The recent scan whose commit is nearest behind HEAD, on any branch. Asked -/// only when this checkout names no branch to look a baseline up by. +/// The scan whose commit is nearest behind HEAD among this checkout's recent +/// commits, on any branch. /// /// Any branch, including none, because ancestry settles what the branch name /// was standing in for. A scan of a commit HEAD descends from covered a tree /// this history produced, so the diff from it is exactly what this history has -/// done since -- whether the scan was recorded under this branch, under trunk, -/// under the branch this one forked from, or, from a detached CI checkout, -/// under no branch at all. The last is what every earlier run of a pipeline -/// that checks out detached records, and a lookup keyed on names never finds -/// them. +/// done since -- whether the scan was recorded under trunk, under the branch +/// this one forked from, or, from a detached CI checkout, under no branch at +/// all. The last is what every earlier run of a pipeline that checks out +/// detached records, and a lookup keyed on names never finds them. /// -/// Nearest wins because it leaves the fewest files to rescan. Stops at the -/// first page holding any ancestor: scans arrive newest first, and an older -/// one being of a nearer commit is the unusual case not worth reading the -/// whole history for. +/// Nearest wins because it leaves the fewest files to rescan. The commits +/// travel in the request, so a page normally holds every candidate. Stops at +/// the first page holding any: the pages after it are for a backend that +/// ignored the filter and is listing the whole history. fn find_ancestor_baseline( config: &Config, project_name: &str, - ancestry: &mut Ancestry<'_>, + ancestry: &Ancestry, checksums_usable: bool, git_diffable: bool, ) -> BaselineLookup { let url = config.get_url(); for page in 1..=SCAN_LOOKUP_MAX_PAGES { - let response = match api::query_baseline_scans( + let response = match api::query_baseline_scans_at_commits( &url, project_name, BLAST_ENGINE, - None, + &ancestry.shas, // Only a checksum baseline may have been dirty, so without one // the server can drop what could never qualify. !checksums_usable, page, - SCAN_LOOKUP_PAGE_SIZE, + ANCESTOR_LOOKUP_PAGE_SIZE, ) { Ok(response) => response, Err(e) => { @@ -800,13 +804,15 @@ fn find_ancestor_baseline( /// The scan on this page nearest behind HEAD that this run can diff from. /// -/// A scan only diffable by its commit is no use to a run that cannot git diff -/// (a dirty tree without `--ignore-dirty-worktree`, or an `--exclude`d -/// archive), so there it has to carry checksums. Between scans of the same -/// commit, checksums win, then recency. +/// Matched against the walked commits client-side too: a backend that ignored +/// the filter would otherwise hand back a scan of any commit. A scan only +/// diffable by its commit is no use to a run that cannot git diff (a dirty +/// tree without `--ignore-dirty-worktree`, or an `--exclude`d archive), so +/// there it has to carry checksums. Between scans of the same commit, +/// checksums win, then recency. fn nearest_ancestor_baseline( scans: &[ScanResponse], - ancestry: &mut Ancestry<'_>, + ancestry: &Ancestry, checksums_usable: bool, git_diffable: bool, ) -> Option { @@ -816,8 +822,8 @@ fn nearest_ancestor_baseline( .filter(|(_, scan)| is_usable_baseline(scan, checksums_usable)) .filter(|(_, scan)| git_diffable || (checksums_usable && has_readable_checksums(scan))) .filter_map(|(position, scan)| { - let sha = scan.git_sha.as_deref().filter(|sha| !sha.is_empty())?; - let commits_back = ancestry.commits_back(sha)?; + let sha = scan.git_sha.as_deref()?; + let commits_back = *ancestry.commits_back.get(sha)?; let without_checksums = !(checksums_usable && has_readable_checksums(scan)); Some(((commits_back, without_checksums, position), scan)) }) @@ -1475,9 +1481,9 @@ mod tests { } #[test] - fn a_detached_head_finds_the_nearest_ancestor_scan_on_any_branch() { + fn the_nearest_ancestor_scan_wins_whatever_branch_it_names() { let l = lineage(); - let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); // Newest first: a scan of a sibling branch, one of a commit this clone // never fetched, then two ancestors -- the nearer recorded detached. let scans = vec![ @@ -1487,8 +1493,7 @@ mod tests { detached_scan(&l.near), ]; - let picked = - nearest_ancestor_baseline(&scans, &mut ancestry, false, true).expect("a baseline"); + let picked = nearest_ancestor_baseline(&scans, &ancestry, false, true).expect("a baseline"); assert_eq!(picked.sha.as_deref(), Some(l.near.as_str())); assert_eq!(picked.commits_back, Some(1)); @@ -1501,11 +1506,10 @@ mod tests { #[test] fn a_scan_of_head_itself_is_zero_commits_back() { let l = lineage(); - let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); let scans = vec![scan("main", &l.near), detached_scan(&l.head)]; - let picked = - nearest_ancestor_baseline(&scans, &mut ancestry, false, true).expect("a baseline"); + let picked = nearest_ancestor_baseline(&scans, &ancestry, false, true).expect("a baseline"); assert_eq!(picked.commits_back, Some(0)); } @@ -1513,11 +1517,11 @@ mod tests { #[test] fn no_ancestor_on_the_page_means_no_ancestry_baseline() { let l = lineage(); - let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); let scans = vec![scan("feature", &l.side), scan_without_git("no-git")]; assert_eq!( - nearest_ancestor_baseline(&scans, &mut ancestry, true, true), + nearest_ancestor_baseline(&scans, &ancestry, true, true), None ); } @@ -1525,14 +1529,14 @@ mod tests { #[test] fn between_scans_of_one_commit_the_one_with_checksums_wins() { let l = lineage(); - let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); let mut plain = detached_scan(&l.near); plain.id = "plain".to_string(); let mut hashed = with_checksums(detached_scan(&l.near)); hashed.id = "hashed".to_string(); - let picked = nearest_ancestor_baseline(&[plain, hashed], &mut ancestry, true, true) - .expect("a baseline"); + let picked = + nearest_ancestor_baseline(&[plain, hashed], &ancestry, true, true).expect("a baseline"); assert_eq!(picked.id, "hashed"); } @@ -1542,14 +1546,13 @@ mod tests { // A dirty tree without --ignore-dirty-worktree, or an --exclude run: // the nearer scan could only be diffed from its commit. let l = lineage(); - let mut ancestry = Ancestry::new(&l.repo, &l.head).expect("head in clone"); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); let scans = vec![ detached_scan(&l.near), with_checksums(detached_scan(&l.root)), ]; - let picked = - nearest_ancestor_baseline(&scans, &mut ancestry, true, false).expect("a baseline"); + let picked = nearest_ancestor_baseline(&scans, &ancestry, true, false).expect("a baseline"); assert_eq!(picked.sha.as_deref(), Some(l.root.as_str())); assert_eq!(picked.commits_back, Some(2)); @@ -1558,8 +1561,47 @@ mod tests { #[test] fn a_head_this_clone_does_not_hold_offers_no_ancestry() { let l = lineage(); - assert!(Ancestry::new(&l.repo, &"0".repeat(40)).is_none()); - assert!(Ancestry::new(&l.repo, "not-a-sha").is_none()); + assert!(Ancestry::walk(&l.repo, &"0".repeat(40)).is_none()); + assert!(Ancestry::walk(&l.repo, "not-a-sha").is_none()); + } + + #[test] + fn the_walk_follows_both_sides_of_a_merge_and_skips_other_branches() { + let l = lineage(); + let side_tip = commit_on(&l.repo, &[git2::Oid::from_str(&l.side).unwrap()], "tip.txt"); + let merge = commit_on( + &l.repo, + &[git2::Oid::from_str(&l.near).unwrap(), side_tip], + "merge.txt", + ); + + let ancestry = Ancestry::walk(&l.repo, &merge.to_string()).expect("head in clone"); + + let back = |sha: &str| ancestry.commits_back.get(sha).copied(); + assert_eq!(back(&l.near), Some(1)); + assert_eq!(back(&side_tip.to_string()), Some(1)); + assert_eq!(back(&l.side), Some(2)); + assert_eq!(back(&l.root), Some(2)); + assert_eq!(back(&l.head), None); + assert_eq!(ancestry.shas.first(), Some(&merge.to_string())); + } + + #[test] + fn the_walk_stops_at_what_one_request_can_ask_about() { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = Repository::init(dir.path()).expect("init"); + let mut tip = commit_on(&repo, &[], "0"); + for i in 1..=ANCESTOR_LOOKUP_COMMITS + 5 { + tip = commit_on(&repo, &[tip], &i.to_string()); + } + + let ancestry = Ancestry::walk(&repo, &tip.to_string()).expect("head in clone"); + + assert_eq!(ancestry.shas.len(), ANCESTOR_LOOKUP_COMMITS); + assert_eq!( + ancestry.commits_back.get(ancestry.shas.last().unwrap()), + Some(&(ANCESTOR_LOOKUP_COMMITS - 1)) + ); } #[test] diff --git a/src/utils/api.rs b/src/utils/api.rs index a31b547..01c94b0 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -1241,6 +1241,41 @@ pub fn query_baseline_scans( page: u16, page_size: u16, ) -> Result> { + let mut query_params = baseline_scan_params(project, engine, require_clean, page, page_size); + if let Some(branch) = branch { + query_params.push(("branch", branch.to_string())); + } + request_scan_list(url, query_params) +} + +/// One page of the project's scans of any of `shas` that could be diffed +/// against, on any branch, newest first. +/// +/// A backend that takes a single `sha` reads the list as one literal commit +/// and matches nothing. One predating the `sha` filter ignores it and answers +/// with scans at any commit, so callers must re-check `git_sha` on every scan +/// they act on. +pub fn query_baseline_scans_at_commits( + url: &str, + project: &str, + engine: &str, + shas: &[String], + require_clean: bool, + page: u16, + page_size: u16, +) -> Result> { + let mut query_params = baseline_scan_params(project, engine, require_clean, page, page_size); + query_params.push(("sha", shas.join(","))); + request_scan_list(url, query_params) +} + +fn baseline_scan_params( + project: &str, + engine: &str, + require_clean: bool, + page: u16, + page_size: u16, +) -> Vec<(&'static str, String)> { let mut query_params = vec![ ("page", page.to_string()), ("page_size", page_size.to_string()), @@ -1252,13 +1287,10 @@ pub fn query_baseline_scans( // so copying forward from one would drop everything else. ("full_project_state", "true".to_string()), ]; - if let Some(branch) = branch { - query_params.push(("branch", branch.to_string())); - } if require_clean { query_params.push(("worktree_dirty", "false".to_string())); } - request_scan_list(url, query_params) + query_params } /// One page of the project's scans at exactly `sha`, newest first. diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index b67b041..1639164 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -343,26 +343,87 @@ pub(crate) fn plan_step(plan: &[ExpectedRequest], label: &str) -> usize { /// Every baseline lookup a fixture repo makes when nothing turns up. /// -/// Two walks over the branch candidates: the one being scanned, whose last scan -/// is the nearest baseline there could be, then the two trunk names, since the -/// fixture records no origin/HEAD. The first walk takes a baseline of either -/// kind, so it cannot let the server drop scans that are not known-clean; the -/// second asks for exactly those once no checksums have been found, so a clean -/// scan behind a page budget's worth of dirty ones is still reachable. +/// The branch being scanned first, whose last scan is the nearest baseline +/// there could be; then the scans of this checkout's recent commits on any +/// branch; then the two trunk names, since the fixture records no origin/HEAD. +/// Each branch lookup is two walks. The first takes a baseline of either kind, +/// so it cannot let the server drop scans that are not known-clean; the second +/// asks for exactly those once no checksums have been found, so a clean scan +/// behind a page budget's worth of dirty ones is still reachable. pub(crate) fn baseline_lookups_finding_nothing(project: &'static str) -> Vec { - let mut lookups = Vec::new(); - for require_clean in [false, true] { - for branch in BASELINE_BRANCH_ORDER { - lookups.push(expected_request( - "look up a baseline scan to diff against", - move |request| { - assert_baseline_lookup_request(request, project, Some(branch), require_clean) - }, - json_response(scans_response(Vec::new())), - )); + let (own, trunks) = BASELINE_BRANCH_ORDER.split_at(1); + let branch_walks = |branches: &[&'static str]| -> Vec { + [false, true] + .into_iter() + .flat_map(|require_clean| { + branches.iter().map(move |&branch| { + expected_request( + "look up a baseline scan to diff against", + move |request| { + assert_baseline_lookup_request( + request, + project, + Some(branch), + require_clean, + ) + }, + json_response(scans_response(Vec::new())), + ) + }) + }) + .collect() + }; + let mut lookups = branch_walks(own); + lookups.push(expected_request( + "look up a baseline scan of a recent commit", + move |request| assert_ancestor_lookup_request(request, project, None, false), + json_response(scans_response(Vec::new())), + )); + lookups.extend(branch_walks(trunks)); + lookups +} + +/// The lookup by commit: the checkout's recent commits in one `sha` list, on +/// any branch. `shas`, when given, is the exact list expected, nearest first; +/// otherwise any list of full commit hashes passes. +pub(crate) fn assert_ancestor_lookup_request( + request: &CapturedRequest, + project: &str, + shas: Option<&[String]>, + require_clean: bool, +) -> Result<(), String> { + assert_authenticated_request(request, Method::GET, "/api/v1/scans")?; + assert_query(request, "page", "1")?; + assert_query(request, "page_size", "50")?; + assert_query(request, "project", project)?; + assert_query(request, "engine", "corgea-blast")?; + assert_query(request, "status", "complete")?; + assert_query(request, "exclude_pull_requests", "true")?; + assert_query(request, "full_project_state", "true")?; + if let Ok(branch) = query_value(request, "branch") { + return Err(format!( + "a lookup by commit must not ask for branch {branch}" + )); + } + match (require_clean, query_value(request, "worktree_dirty")) { + (true, Ok(value)) if value == "false" => {} + (false, Err(_)) => {} + (_, value) => return Err(format!("unexpected worktree_dirty filter {value:?}")), + } + let sent = query_value(request, "sha")?; + match shas { + Some(shas) if sent != shas.join(",") => { + Err(format!("expected sha={}, got {sent}", shas.join(","))) + } + Some(_) => Ok(()), + None if sent + .split(',') + .all(|sha| sha.len() == 40 && sha.bytes().all(|b| b.is_ascii_hexdigit())) => + { + Ok(()) } + None => Err(format!("expected a list of commit hashes, got sha={sent}")), } - lookups } /// One baseline lookup an incremental scan makes before uploading. diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 67beae1..cbe7ab6 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -115,6 +115,15 @@ fn clean_baseline_lookup(branch: &'static str, scans: Vec) -> ExpectedReq ) } +/// The lookup by commit, over this checkout's recent history on any branch. +fn ancestor_lookup(shas: Option>, scans: Vec) -> ExpectedRequest { + expected_request( + "look up a baseline scan of a recent commit", + move |request| assert_ancestor_lookup_request(request, PROJECT, shas.as_deref(), false), + json_response(scans_response(scans)), + ) +} + /// One page of the baseline lookup, for the walk an old backend forces. fn baseline_lookup_page( branch: &'static str, @@ -661,8 +670,8 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { assert_eq!(output.status.code(), Some(0), "{context}"); assert!( stdout.contains(&format!( - "has no completed scan on {FIXTURE_BRANCH}, main or master that could be diffed \ - against" + "has no completed scan on {FIXTURE_BRANCH}, main or master, nor of any of the 2 \ + most recent commits in this checkout's history that could be diffed against" )), "{context}" ); @@ -686,11 +695,14 @@ fn the_branch_being_scanned_is_preferred_over_trunk_and_falls_back_to_it() { let mut scan = baseline_scan(&base_sha); scan["branch"] = json!(baseline_branch); // The branch being scanned is asked about first either way. When the - // baseline is trunk's, that lookup comes back empty and the next one - // finds it -- so the plan is the assertion that the order is what it is. + // baseline is trunk's, that lookup and the one by commit come back + // empty and the next one finds it -- so the plan is the assertion that + // the order is what it is. let mut plan = vec![verify_request(), scan_settings_request(PROJECT)]; if baseline_branch != FIXTURE_BRANCH { plan.push(baseline_lookup(FIXTURE_BRANCH, vec![])); + plan.push(clean_baseline_lookup(FIXTURE_BRANCH, vec![])); + plan.push(ancestor_lookup(None, vec![])); } let expected_base = base_sha.clone(); plan.extend([ @@ -723,6 +735,77 @@ fn the_branch_being_scanned_is_preferred_over_trunk_and_falls_back_to_it() { } } +/// A branch with no scan of its own diffs against the scan of the commit it +/// forked from, whatever branch that scan names -- here a trunk called +/// `develop`, which the name lookup never asks about. A detached HEAD, which +/// names no branch to look up at all, gets the same answer. +#[test] +fn a_scan_of_a_recent_commit_is_the_baseline_when_the_branch_has_none() { + for detached in [false, true] { + let project = git_project(); + let base_sha = project.sha.clone(); + let head_sha = second_commit(&project); + if detached { + run_git(project.path(), &["checkout", "--detach"]); + } + + let (mut scan, manifest) = + baseline_scan_with_checksums(&base_sha, &[("main.py", SOURCE_BODY)]); + scan["branch"] = json!("develop"); + let mut plan = vec![verify_request(), scan_settings_request(PROJECT)]; + if !detached { + plan.push(baseline_lookup(FIXTURE_BRANCH, vec![])); + plan.push(clean_baseline_lookup(FIXTURE_BRANCH, vec![])); + } + plan.extend([ + ancestor_lookup(Some(vec![head_sha.clone(), base_sha.clone()]), vec![scan]), + checksum_download(manifest), + start_upload(), + expected_request( + "upload BLAST archive with the checksum diff", + |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field( + request, + "incremental_base_scan_id", + BASELINE_SCAN, + )?; + assert_multipart_text_field( + request, + "incremental_changed_files", + r#"["helper.py","main.py"]"#, + ) + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]); + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains(&format!( + "Incremental scan: 2 files changed since the scan of ancestor commit {} (1 \ + commit back).", + &base_sha[..7] + )), + "{context}" + ); + } +} + /// A backend predating the server-side filters returns scans of every kind, so /// a page can hold nothing usable. The walk is what stops that project from /// being permanently unable to find a baseline it has. @@ -941,8 +1024,6 @@ fn a_clean_baseline_is_found_even_when_dirty_scans_come_back_first() { verify_request(), scan_settings_request(PROJECT), baseline_lookup(FIXTURE_BRANCH, vec![dirty]), - baseline_lookup("main", vec![]), - baseline_lookup("master", vec![]), clean_baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), start_upload(), expected_request( From fb71e3bad41d741ac8a8c910fa29bff223338d81 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 29 Sep 2026 13:41:09 +0000 Subject: [PATCH 3/5] Share the git-diff refusal with the ancestor lookup; cover its failure and detached trunk fallback The ancestor lookup decides whether a commit-only baseline is usable from the same function plan_git_diff refuses with, so a change to when a git diff is allowed cannot leave the two disagreeing. E2E: a failed lookup by commit scans everything and says the lookup failed, and a detached HEAD with no scanned recent commit reaches trunk. Co-authored-by: ibrahim --- src/incremental.rs | 64 +++++++------- tests/cloud_commands_e2e/scan_incremental.rs | 91 ++++++++++++++++++-- 2 files changed, 117 insertions(+), 38 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 920256b..f8a12ec 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -229,7 +229,7 @@ fn plan_diff( project_name, ancestry, checksums_usable, - can_git_diff(sources), + git_diff_refusal(sources).is_none(), ); } if lookup == BaselineLookup::NotFound { @@ -313,33 +313,10 @@ fn plan_git_diff( repo: Option<&Repository>, sources: &DiffSources<'_>, ) -> Result<(IncrementalPlan, String), String> { - // git diffs the repository, and --exclude means the archive is not it. An - // excluded file git reports unchanged is left off the list, so the server - // copies its findings forward over a file this upload does not contain -- - // and no later run under the same --exclude will look at it either. First, - // because these runs report dirty whatever the worktree holds, so the check - // below would otherwise answer for a tree with nothing uncommitted in it. - if sources.exclude_narrowed { - return Err( - "--exclude held files back from this archive, so a git diff of the repository \ - would not describe it (its stored file checksums would, on the next run)" - .to_string(), - ); + if let Some(refusal) = git_diff_refusal(sources) { + return Err(refusal); } - - // A commit-to-commit diff cannot see uncommitted edits, so on a dirty tree - // it leaves modified files off the list and their old findings are copied - // forward as current. --ignore-dirty-worktree does not paper over that; it - // switches the diff to measure the working tree, so those files are named - // and rescanned like any other change. let covers_worktree = sources.worktree_dirty; - if sources.worktree_dirty && !sources.ignore_dirty_worktree { - return Err( - "this worktree has uncommitted changes that a commit-to-commit diff cannot \ - see. Pass --ignore-dirty-worktree to diff the working tree instead" - .to_string(), - ); - } // Nothing to diff from. Covers a non-git directory, a repo with no commit, // and a scan started below the repo root — none of which report RepoInfo @@ -695,10 +672,37 @@ fn branch_baseline( Some(BaselineScan::from_response(scan)) } -/// Whether this run could diff from a baseline's commit, as opposed to only -/// from its stored checksums. Mirrors the refusals in `plan_git_diff`. -fn can_git_diff(sources: &DiffSources<'_>) -> bool { - !sources.exclude_narrowed && (!sources.worktree_dirty || sources.ignore_dirty_worktree) +/// Why this run cannot diff from any baseline's commit, only from stored +/// checksums. One function for both `plan_git_diff` and the ancestor lookup, +/// which skips commit-only baselines when this says no. +fn git_diff_refusal(sources: &DiffSources<'_>) -> Option { + // git diffs the repository, and --exclude means the archive is not it. An + // excluded file git reports unchanged is left off the list, so the server + // copies its findings forward over a file this upload does not contain -- + // and no later run under the same --exclude will look at it either. First, + // because these runs report dirty whatever the worktree holds, so the check + // below would otherwise answer for a tree with nothing uncommitted in it. + if sources.exclude_narrowed { + return Some( + "--exclude held files back from this archive, so a git diff of the repository \ + would not describe it (its stored file checksums would, on the next run)" + .to_string(), + ); + } + + // A commit-to-commit diff cannot see uncommitted edits, so on a dirty tree + // it leaves modified files off the list and their old findings are copied + // forward as current. --ignore-dirty-worktree does not paper over that; it + // switches the diff to measure the working tree, so those files are named + // and rescanned like any other change. + if sources.worktree_dirty && !sources.ignore_dirty_worktree { + return Some( + "this worktree has uncommitted changes that a commit-to-commit diff cannot \ + see. Pass --ignore-dirty-worktree to diff the working tree instead" + .to_string(), + ); + } + None } /// HEAD and the commits nearest behind it in this clone, with how far back diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index cbe7ab6..35a45a4 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -806,6 +806,61 @@ fn a_scan_of_a_recent_commit_is_the_baseline_when_the_branch_has_none() { } } +/// A detached HEAD has no branch lookup to make, and when none of its recent +/// commits was scanned it still reaches trunk -- through a git diff here, which +/// needs the two commits and no branch. +#[test] +fn a_detached_head_with_no_scanned_recent_commit_falls_back_to_trunk() { + let project = git_project(); + let base_sha = project.sha.clone(); + let head_sha = second_commit(&project); + run_git(project.path(), &["checkout", "--detach"]); + + let mut scan = baseline_scan(&base_sha); + scan["branch"] = json!("main"); + let expected_base = base_sha.clone(); + let mut plan = vec![ + verify_request(), + scan_settings_request(PROJECT), + ancestor_lookup(Some(vec![head_sha, base_sha]), vec![]), + baseline_lookup("main", vec![scan]), + start_upload(), + expected_request( + "upload BLAST archive with the git diff", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field(request, "incremental_base_sha", &expected_base)?; + assert_multipart_text_field( + request, + "incremental_changed_files", + r#"["helper.py","main.py"]"#, + ) + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains("Incremental scan: 2 files changed since commit"), + "{context}" + ); +} + /// A backend predating the server-side filters returns scans of every kind, so /// a page can hold nothing usable. The walk is what stops that project from /// being permanently unable to find a baseline it has. @@ -853,20 +908,40 @@ fn a_baseline_on_a_later_page_is_still_found() { /// A lookup that failed says so. Reporting it as "no earlier scan" tells someone /// with years of scan history that they have none, and now that incremental is -/// the default, any network blip would say it. +/// the default, any network blip would say it. Nor does a failure fall through +/// to the next lookup: the endpoint that failed is the one it would ask. #[test] fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { + for fail_by_commit in [false, true] { + assert_failed_lookup_scans_everything(fail_by_commit); + } +} + +fn assert_failed_lookup_scans_everything(fail_by_commit: bool) { let project = git_project(); second_commit(&project); - let mut plan = vec![ - verify_request(), - scan_settings_request(PROJECT), - expected_request( + let failure = + || json_response_with_status(StatusCode::INTERNAL_SERVER_ERROR, json!({"error": "boom"})); + let mut plan = vec![verify_request(), scan_settings_request(PROJECT)]; + if fail_by_commit { + plan.extend([ + baseline_lookup(FIXTURE_BRANCH, vec![]), + clean_baseline_lookup(FIXTURE_BRANCH, vec![]), + expected_request( + "fail the baseline lookup by commit", + |request| assert_ancestor_lookup_request(request, PROJECT, None, false), + failure(), + ), + ]); + } else { + plan.push(expected_request( "fail the baseline lookup", |request| assert_baseline_lookup_request(request, PROJECT, Some(FIXTURE_BRANCH), false), - json_response_with_status(StatusCode::INTERNAL_SERVER_ERROR, json!({"error": "boom"})), - ), + failure(), + )); + } + plan.extend([ start_upload(), expected_request( "upload BLAST archive with no diff", @@ -881,7 +956,7 @@ fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { }, json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), ), - ]; + ]); plan.extend(scan_tail()); let api = ApiStub::start(plan); From 06609c844ecbe311d44117d7a7b32534501dec10 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 30 Sep 2026 07:49:59 +0000 Subject: [PATCH 4/5] Fall back to trunk when the baseline lookup by commit fails A backend that cannot answer a list of commits, or errors on it, must not cost the trunk baseline earlier releases found. The failure is treated as no match and the trunk lookup runs; if the endpoint itself is down, that lookup fails and the run says so. The refusal only claims no recent commit was scanned when the lookup by commit actually answered. Co-authored-by: ibrahim --- src/incremental.rs | 22 ++++++-- tests/cloud_commands_e2e/scan_incremental.rs | 57 ++++++++++---------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index f8a12ec..5b7072b 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -211,6 +211,9 @@ fn plan_diff( _ => None, }; + // Whether the lookup by commit answered, so the refusal below only claims + // none of those commits was scanned when the server actually said so. + let mut commits_searched = false; let lookup = match &candidates { Some(candidates) => { // `baseline_branches` puts the branch being scanned first. @@ -224,13 +227,22 @@ fn plan_diff( // master. It also finds the earlier runs of a detached CI pipeline, // which recorded no branch for a name to match. if let (BaselineLookup::NotFound, Some(ancestry)) = (&lookup, &ancestry) { - lookup = find_ancestor_baseline( + lookup = match find_ancestor_baseline( config, project_name, ancestry, checksums_usable, git_diff_refusal(sources).is_none(), - ); + ) { + // A backend that cannot answer a list of commits must not + // cost the trunk lookup every earlier release made. If the + // endpoint itself is down, that lookup fails and says so. + BaselineLookup::LookupFailed => BaselineLookup::NotFound, + answered => { + commits_searched = true; + answered + } + }; } if lookup == BaselineLookup::NotFound { lookup = find_baseline(config, project_name, Some(trunks), checksums_usable); @@ -245,7 +257,7 @@ fn plan_diff( return Err(format!( "project '{project_name}' has no completed scan {} that could be diffed \ against, so there is nothing to compare this one to", - match (&candidates, &ancestry) { + match (&candidates, ancestry.as_ref().filter(|_| commits_searched)) { (Some(candidates), Some(ancestry)) => format!( "on {}, nor of any of the {} most recent commits in this checkout's \ history", @@ -783,7 +795,9 @@ fn find_ancestor_baseline( ) { Ok(response) => response, Err(e) => { - crate::log::debug(&format!("Baseline scan lookup failed: {e}")); + crate::log::debug(&format!( + "Baseline lookup by commit failed, trying trunk instead: {e}" + )); return BaselineLookup::LookupFailed; } }; diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 35a45a4..0663911 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -809,8 +809,17 @@ fn a_scan_of_a_recent_commit_is_the_baseline_when_the_branch_has_none() { /// A detached HEAD has no branch lookup to make, and when none of its recent /// commits was scanned it still reaches trunk -- through a git diff here, which /// needs the two commits and no branch. +/// +/// So does a lookup by commit that fails. A backend that cannot answer a list +/// of commits must not cost the trunk baseline every earlier release found. #[test] fn a_detached_head_with_no_scanned_recent_commit_falls_back_to_trunk() { + for lookup_by_commit_fails in [false, true] { + assert_detached_head_falls_back_to_trunk(lookup_by_commit_fails); + } +} + +fn assert_detached_head_falls_back_to_trunk(lookup_by_commit_fails: bool) { let project = git_project(); let base_sha = project.sha.clone(); let head_sha = second_commit(&project); @@ -819,10 +828,20 @@ fn a_detached_head_with_no_scanned_recent_commit_falls_back_to_trunk() { let mut scan = baseline_scan(&base_sha); scan["branch"] = json!("main"); let expected_base = base_sha.clone(); + let shas = vec![head_sha, base_sha]; + let lookup_by_commit = if lookup_by_commit_fails { + expected_request( + "fail the baseline lookup by commit", + move |request| assert_ancestor_lookup_request(request, PROJECT, Some(&shas), false), + json_response_with_status(StatusCode::BAD_REQUEST, json!({"error": "bad sha"})), + ) + } else { + ancestor_lookup(Some(shas), vec![]) + }; let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), - ancestor_lookup(Some(vec![head_sha, base_sha]), vec![]), + lookup_by_commit, baseline_lookup("main", vec![scan]), start_upload(), expected_request( @@ -908,40 +927,20 @@ fn a_baseline_on_a_later_page_is_still_found() { /// A lookup that failed says so. Reporting it as "no earlier scan" tells someone /// with years of scan history that they have none, and now that incremental is -/// the default, any network blip would say it. Nor does a failure fall through -/// to the next lookup: the endpoint that failed is the one it would ask. +/// the default, any network blip would say it. #[test] fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { - for fail_by_commit in [false, true] { - assert_failed_lookup_scans_everything(fail_by_commit); - } -} - -fn assert_failed_lookup_scans_everything(fail_by_commit: bool) { let project = git_project(); second_commit(&project); - let failure = - || json_response_with_status(StatusCode::INTERNAL_SERVER_ERROR, json!({"error": "boom"})); - let mut plan = vec![verify_request(), scan_settings_request(PROJECT)]; - if fail_by_commit { - plan.extend([ - baseline_lookup(FIXTURE_BRANCH, vec![]), - clean_baseline_lookup(FIXTURE_BRANCH, vec![]), - expected_request( - "fail the baseline lookup by commit", - |request| assert_ancestor_lookup_request(request, PROJECT, None, false), - failure(), - ), - ]); - } else { - plan.push(expected_request( + let mut plan = vec![ + verify_request(), + scan_settings_request(PROJECT), + expected_request( "fail the baseline lookup", |request| assert_baseline_lookup_request(request, PROJECT, Some(FIXTURE_BRANCH), false), - failure(), - )); - } - plan.extend([ + json_response_with_status(StatusCode::INTERNAL_SERVER_ERROR, json!({"error": "boom"})), + ), start_upload(), expected_request( "upload BLAST archive with no diff", @@ -956,7 +955,7 @@ fn assert_failed_lookup_scans_everything(fail_by_commit: bool) { }, json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), ), - ]); + ]; plan.extend(scan_tail()); let api = ApiStub::start(plan); From 4378093f8d445aefef72faeb02af7dea7d7112ee Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 30 Sep 2026 08:39:57 +0000 Subject: [PATCH 5/5] Look for an ancestor baseline among the nearest 10 commits, not 50 Co-authored-by: ibrahim --- src/incremental.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/incremental.rs b/src/incremental.rs index 5b7072b..1a50137 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -56,9 +56,10 @@ use std::collections::{BTreeSet, HashMap, VecDeque}; const SCAN_LOOKUP_PAGE_SIZE: u16 = 30; /// How many of this checkout's nearest commits a baseline is looked for among. -/// The most the scan list's `sha` filter takes in one request, and the most -/// scans per page it returns. -const ANCESTOR_LOOKUP_COMMITS: usize = 50; +/// Must stay within the 50 the scan list's `sha` filter takes in one request. +const ANCESTOR_LOOKUP_COMMITS: usize = 10; +/// The most scans per page the scan list returns, so one page normally holds +/// every scan of those commits. const ANCESTOR_LOOKUP_PAGE_SIZE: u16 = 50; /// Backstop on pages walked looking for a baseline.