Scan incrementally by default: diff against the last scan locally and send the file list - #160
Open
Ibrahimrahhal wants to merge 7 commits into
Open
Scan incrementally by default: diff against the last scan locally and send the file list#160Ibrahimrahhal wants to merge 7 commits into
Ibrahimrahhal wants to merge 7 commits into
Conversation
…le 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 <ibrahim@corgea.com>
Ibrahimrahhal
marked this pull request as ready for review
August 23, 2026 10:33
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review risk: 4/5.
Incremental scans can incorrectly carry forward stale findings for changed container images and submodule contents. Both existing review comments are valid and require fixes.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: checks failed: rust-tests.
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 <ibrahim@corgea.com>
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 <ibrahim@corgea.com>
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 <ibrahim@corgea.com>
cursor
Bot
changed the title
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 <ibrahim@corgea.com>
Same content, fewer words. Also drops two stale --incremental references in test docs left by the flip to opt-out. Co-authored-by: ibrahim <ibrahim@corgea.com>
Ibrahimrahhal
requested review from
corgea-security,
juangaitanv,
leenk7991 and
yhoztak
August 23, 2026 11:49
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 <ibrahim@corgea.com>
There was a problem hiding this comment.
Automated review risk: 3/5.
No actionable critical or high-priority defects remain. The submodule issue was fixed, and the image concern was adequately rebutted by the documented server behavior. Risk is moderate because incremental behavior is now enabled by default.
No critical or high-priority changes were found.
Automatic approval was not submitted: automated risk 3/5 exceeds approval threshold 2.
leenk7991
reviewed
Aug 24, 2026
| /// Payload guard, not policy. The server applies the real ceiling | ||
| /// (`INCREMENTAL_SCAN_MAX_FILES`, 300) and falls back to a full scan above it. | ||
| /// This only avoids building a multi-megabyte form field to be refused. | ||
| const MAX_CHANGED_FILES: usize = 5_000; |
|
|
||
| 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}', \ |
Member
There was a problem hiding this comment.
when query_baseline_scans fails (auth, network, 5xx..), find_baseline_sha returns None and we print “no earlier completed scan of a clean worktree was found..” which is the same message as a project with no baseline
can we split lookup failure from “not found” so users see something more like “could not look up previous scans” instead of implying they have no scan history
juangaitanv
reviewed
Aug 24, 2026
Comment on lines
+159
to
+227
| for page in 1..=SCAN_LOOKUP_MAX_PAGES { | ||
| let response = match api::query_baseline_scans( | ||
| &url, | ||
| project_name, | ||
| BLAST_ENGINE, | ||
| page, | ||
| SCAN_LOOKUP_PAGE_SIZE, | ||
| ) { | ||
| Ok(response) => response, | ||
| Err(e) => { | ||
| // A failed lookup proves nothing about the project's history, | ||
| // so it means full scan, not error. | ||
| crate::log::debug(&format!("Baseline scan lookup failed: {e}")); | ||
| return any_branch_fallback; | ||
| } | ||
| }; | ||
|
|
||
| let scans = response.scans.unwrap_or_default(); | ||
| if scans.is_empty() { | ||
| break; | ||
| } | ||
|
|
||
| // Newest first, so the first same-branch match is the best available | ||
| // and no later page can improve on it. | ||
| if let Some(scan) = usable_baselines(&scans) | ||
| .find(|scan| scan.branch.as_deref().is_some_and(|b| b == branch)) | ||
| { | ||
| return scan.git_sha.clone(); | ||
| } | ||
| if any_branch_fallback.is_none() { | ||
| any_branch_fallback = usable_baselines(&scans) | ||
| .next() | ||
| .and_then(|s| s.git_sha.clone()); | ||
| } | ||
|
|
||
| if response | ||
| .total_pages | ||
| .is_some_and(|total| u32::from(page) >= total) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| any_branch_fallback | ||
| } | ||
|
|
||
| /// Scans on one page that can serve as a baseline, newest first. | ||
| fn usable_baselines(scans: &[ScanResponse]) -> impl Iterator<Item = &ScanResponse> { | ||
| scans.iter().filter(|scan| is_usable_baseline(scan)) | ||
| } | ||
|
|
||
| /// Whether `scan` may be diffed against. | ||
| /// | ||
| /// Client-side half of the filter doghouse applies picking a baseline itself: a | ||
| /// completed blast scan of a whole, clean, non-pull-request commit. | ||
| /// `worktree_dirty` must be an explicit `false` — `None` means never reported, | ||
| /// and unknown scope is not clean, so the server rejects it as a baseline too. | ||
| /// | ||
| /// `query_baseline_scans` asks the server for exactly these, which keeps the | ||
| /// page walk from iterating. This stays because a backend predating those | ||
| /// parameters ignores them, and a dirty or pull-request scan's commit would | ||
| /// diff against the wrong tree. | ||
| fn is_usable_baseline(scan: &ScanResponse) -> bool { | ||
| classify_scan_status(&scan.status) == ScanState::Completed | ||
| && scan.engine.eq_ignore_ascii_case(BLAST_ENGINE) | ||
| && scan.pull_request_id.is_none() | ||
| && scan.worktree_dirty == Some(false) | ||
| && scan.git_sha.as_deref().is_some_and(|sha| !sha.is_empty()) | ||
| } |
Contributor
There was a problem hiding this comment.
scan-list responses omit scan_errors, so a completed baseline can carry incomplete findings. could we fetch each candidate detail before selecting it, then fall back when it has warnings?
| #[arg(long, help = "Only scan uncommitted changes.")] | ||
| only_uncommitted: bool, | ||
|
|
||
| #[arg( |
Contributor
There was a problem hiding this comment.
the reuse path returns before start_new_scan, so this flag pair skips analysis. should these flags conflict?
Comment on lines
+188
to
+192
| if any_branch_fallback.is_none() { | ||
| any_branch_fallback = usable_baselines(&scans) | ||
| .next() | ||
| .and_then(|s| s.git_sha.clone()); | ||
| } |
Contributor
There was a problem hiding this comment.
cross-branch fallback and baseline pagination/error behavior remain untested. could we add coverage for those paths?
Contributor
|
Feedback from meeting. Whatever transunion is currently using, like --skip-duplicate --ignore-dirtyworktree, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Every
corgea scan blastrun now finds the project's newest completed scan of a clean worktree, diffs this commit against it withgit2, and sends both the base commit and the changed-file list with the upload. Corgea analyzes only those files and carries existing findings forward for everything else.--disable-incrementalforces every file to be analyzed.Needs the doghouse side to land first: Corgea/doghouse#1939. Against an older server the two fields are ignored and the run is a normal full scan.
Why
Corgea already runs incremental scans, but doghouse works the diff out server-side by asking the project's SCM integration to compare two commits. Projects it cannot answer for — zip-only projects, self-hosted hosts it cannot reach, commits that were never pushed — pay for a full analysis on every run. At one enterprise customer that is 86% of eligible full scans, and no server-side change can fix it, because the diff simply is not available to the server. It is available right here in the clone the scan is already reading from.
It is on by default because an optimization nobody opts into does not run, and the projects this exists for are the ones least likely to hear that a new flag shipped.
What it does not do
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 way it declines
Being the default means it has to be safe on a repository that was never set up for it. Each of these falls through to the full scan that run would have done anyway, and says why:
fetch-depth: 0--only-uncommitted,--targetand--excludedisable it silently rather than conflicting. 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 "scanning every file" would be a lie.The server-side
company.incremental_scan_enabledflag remains the central kill switch, so an org can turn this off for everyone without a CLI rollback.Design notes
The base commit travels with the list. The server carries findings forward for every file the list omits, so if it picked a different baseline than the one diffed here, findings in files that changed between the two would be reported as current when nothing had looked at them. Sending
incremental_base_shalets the server copy from exactly the scan this diff describes, or refuse. The two fields are attached to the form together or not at all.The diff is not
--target git:diff=.get_git_diff_filesrecords onlynew_file().path()and only for Added/Copied/Modified/Renamed, because it is picking files that still exist on disk to put in an archive. This needs the opposite: both sides of every delta, no status filtered out. 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. Two-dotdiff_tree_to_tree, not three-dot: the question is which files differ between the snapshot we are copying from and now, not what this branch introduced.A moved submodule refuses the diff. A committed pointer bump is a single 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 findings nothing re-examined. Diffing the two submodule commits would mean opening a repository that may not be checked out, so a gitlink on either side fails closed.
Baseline selection mirrors the server's. Completed
corgea-blastscan, not a pull request,worktree_dirty == Some(false)(Noneis unknown scope, not a clean tree, and the server would reject it anyway). Same branch preferred, falling back to the newest on any branch, so the first scan of a feature branch is incremental against the trunk rather than full.Client-side caps are payload guards, not policy. The server applies the real ceiling (
INCREMENTAL_SCAN_MAX_FILES, 300) so it can be tuned without a CLI release.upload_zipgrew a fourth optional setting, soscan_type/policy/metadata/incrementalmoved into anUploadOptionsstruct rather than adding an eighth argument and atoo_many_argumentssuppression.Cost
One extra
GET /api/v1/scansper clean-tree scan, before the upload. Runs that cannot use a baseline — no git, dirty tree, narrowed archive — skip the lookup entirely rather than making a call they cannot act on.Known limitation
Files git ignores are in the archive but invisible to the diff, so a changed ignored file would keep its old findings. Untracked files are not affected — they make the worktree dirty, which already refuses. This matches the existing server-side incremental path exactly;
repo.compare()only reports tracked changes too.Testing
./harness checkis green: clippy strict, format, 760 tests, no new suppressions../harness cipasses clippy, format and dep audit.Unit tests in
src/incremental.rscover baseline acceptance and each rejection reason (running, third-party engine, PR scan, dirty, dirty-signal-less, no commit), newest-first selection, and the diff itself against realgit2repositories — added, edited and deleted files named while untouched ones are not, a commit diffed against itself reporting nothing, an unknown base commit reported rather than panicked, and a moved submodule pointer refused.tests/cloud_commands_e2e/scan_incremental.rsasserts the wire contract end to end against the HTTP stub, one case per outcome: the upload carriesincremental_base_shaandincremental_changed_fileswith the exact JSON list; a project with no baseline, a directory that is not a git repository, and a dirty worktree each upload with neither field;--disable-incrementaland--targetdo not even look for a baseline. "Did not look" is proven by the stub's request sequence rather than by output.blast_upload_plan, the shared upload contract used by the other e2e suites, now expects the baseline lookup on clean-tree scans and answers with no scans — which is what makes it the full-scan contract.Unrelated fix included
cargo auditwas failing the CI gate onh2 0.4.12(RUSTSEC-2026-0258, published 2026-08-17).maincarries the identical lockfile entry, so this predates the branch. Bumped to 0.4.18 in its own lockfile-only commit.