diff --git a/src/incremental.rs b/src/incremental.rs index 8a8523b..134e9be 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -68,21 +68,14 @@ pub fn resolve_incremental_plan( branch: Option<&str>, head_sha: Option<&str>, worktree_dirty: bool, - ignore_dirty_worktree: bool, ) -> Option { // A commit-to-commit diff cannot see uncommitted edits, so 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. + // it would leave modified files off the list and their old findings would be + // copied forward as current. Diff the working tree instead, which is what + // this run uploads: modified and untracked files are named and rescanned + // like any other change. Gitignored files are absent from both the diff and + // the archive, so nothing unnamed is left to go stale. let covers_worktree = worktree_dirty; - if worktree_dirty && !ignore_dirty_worktree { - explain_full_scan( - "this worktree has uncommitted changes, and a commit-to-commit diff cannot \ - see them. Pass --ignore-dirty-worktree to diff the working tree instead", - ); - return None; - } // Nothing to diff from. Covers a non-git directory, a repo with no commit, // a detached HEAD, and a scan started below the repo root — none of which @@ -325,9 +318,6 @@ fn is_usable_baseline(scan: &ScanResponse) -> bool { /// whose old path needs the same. `--target`'s `git:diff=` selector wants the /// opposite — paths still on disk, to archive — hence no reuse. /// -/// Untracked files are not a gap: they make the worktree dirty, already -/// refused above. -/// /// Submodules are the one thing this cannot describe. A committed pointer bump /// is one gitlink delta naming the submodule directory, while packaging walks /// into it and uploads the files inside, so those files would be missing from @@ -640,6 +630,21 @@ mod tests { assert_eq!(files, vec!["added.txt", "edit.txt", "gone.txt", "keep.txt"]); } + #[test] + fn a_worktree_diff_leaves_gitignored_files_out() { + // Build output is what makes a CI worktree dirty, and packaging applies + // the same ignore rules, so an unnamed ignored file is not in the + // archive either and has no findings to carry forward. + let (dir, repo, _base, head) = repo_with_history(); + fs::write(dir.path().join(".gitignore"), "build/\n").expect("ignore"); + fs::create_dir(dir.path().join("build")).expect("mkdir"); + fs::write(dir.path().join("build/out.js"), "generated").expect("build output"); + + let files = changed_files_since(&repo, &head, &head, true).expect("diff"); + + assert_eq!(files, vec![".gitignore"]); + } + /// 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"); diff --git a/src/main.rs b/src/main.rs index 9fb413e..61e74ca 100644 --- a/src/main.rs +++ b/src/main.rs @@ -196,7 +196,7 @@ enum Commands { #[arg( long = "ignore-dirty-worktree", - help = "Do not let uncommitted changes stop this run from taking a shortcut. For an incremental scan, the diff is measured against the working tree instead of the last commit, so edited and untracked files are analyzed rather than skipped. With --skip-if-commit-scanned-recently, a recent scan of this commit may be reused even though this worktree is dirty or the prior scan recorded worktree_dirty. A new scan still reports the real dirty status." + help = "With --skip-if-commit-scanned-recently, reuse a recent scan of this commit even though this worktree is dirty or the prior scan recorded worktree_dirty, which means reporting results that do not cover your uncommitted changes. A new scan still reports the real dirty status. Incremental scans no longer need this flag: they diff the working tree whenever it is dirty, so edited and untracked files are always analyzed." )] ignore_dirty_worktree: bool, }, @@ -896,7 +896,6 @@ fn main() { block_on, only_uncommitted, disable_incremental, - ignore_dirty_worktree, metadata_json, scan_type.clone(), policy.clone(), diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index ca813fe..35759c1 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -54,7 +54,6 @@ pub fn run( block_on: Option, only_uncommitted: &bool, disable_incremental: &bool, - ignore_dirty_worktree_for_run: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -92,6 +91,19 @@ pub fn run( let project_name = utils::generic::determine_project_name(project_name.as_deref()); + // 1.11.1 gave the flag a `requires` clause, 1.12.0 dropped it to let the + // flag also put incremental back on the table for a dirty tree, and nothing + // needs it for that any more. So a run can still pass it alone, where it now + // governs nothing -- and someone passing it to get an incremental scan would + // otherwise get full scans with no idea why. + if *ignore_dirty_worktree && skip_recent.is_none() { + log::warn!( + "--ignore-dirty-worktree has no effect without --skip-if-commit-scanned-recently. \ + Incremental scans do not need it: a dirty worktree is diffed against the working \ + tree, so uncommitted and untracked files are analyzed." + ); + } + // A reused scan stands in for the new one: everything below this point — // the results table, the blocking-rule gate, the report file — runs against // whichever scan id this resolves to. @@ -112,7 +124,6 @@ pub fn run( &project_name, only_uncommitted, disable_incremental, - ignore_dirty_worktree_for_run, metadata, scan_type, policy, @@ -286,7 +297,6 @@ fn start_new_scan( project_name: &str, only_uncommitted: &bool, disable_incremental: &bool, - ignore_dirty_worktree: &bool, metadata: Option, scan_type: Option, policy: Option, @@ -528,9 +538,9 @@ fn start_new_scan( let incremental_plan = if *disable_incremental || narrowed_archive { None } else { - // Reconciled repo info, so a tree that turned out dirty — or a HEAD - // that moved mid-packaging — refuses rather than diffing against a - // commit this upload is not a snapshot of. + // Reconciled repo info, so the flag here is the one the upload reports. + // They have to agree: the server refuses a dirty upload whose diff does + // not claim to cover the worktree. crate::incremental::resolve_incremental_plan( config, project_name, @@ -539,7 +549,6 @@ fn start_new_scan( // Missing repo info is not dirtiness; it is the missing // branch/commit the resolver reports next, by its real name. repo_info.as_ref().is_some_and(|info| info.dirty), - *ignore_dirty_worktree, ) }; println!("\n\nSubmitting scan to Corgea:"); diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index b9548d3..0de2dae 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -809,18 +809,34 @@ pub(crate) fn blast_plan(sha: &str) -> Vec { /// BLAST upload contract. `include_sca` covers `--fail-on malicious` (SCA fetch). pub(crate) fn blast_upload_plan(sha: &str, dirty: bool, include_sca: bool) -> Vec { + blast_upload_plan_inner(sha, dirty, include_sca, true) +} + +/// BLAST upload contract for a run that never looks for a baseline, because +/// `--target`/`--exclude` narrowed the archive and findings cannot be carried +/// forward for files it no longer holds. +pub(crate) fn narrowed_blast_upload_plan(sha: &str) -> Vec { + blast_upload_plan_inner(sha, true, false, false) +} + +fn blast_upload_plan_inner( + sha: &str, + dirty: bool, + include_sca: bool, + baseline_lookups: bool, +) -> Vec { let patch_sha = sha.to_string(); let dirty_value = if dirty { "true" } else { "false" }.to_string(); 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()]; - // Scans are incremental by default, so every clean-tree run looks for a - // baseline before uploading -- once per trunk branch, since the fixture - // records no origin/HEAD. Answering with no scans keeps this the full-scan - // contract: nothing to diff from, no incremental fields on the upload. A - // dirty tree never asks. - if !dirty { + // Scans are incremental by default, so a run looks for a baseline before + // uploading -- once per trunk branch, since the fixture records no + // origin/HEAD. A dirty tree asks too: its diff moves to the working tree + // rather than giving up. Answering with no scans keeps this the full-scan + // contract: nothing to diff from, no incremental fields on the upload. + if baseline_lookups { for branch in ["main", "master"] { 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 c9385ba..ea6f5fd 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -453,11 +453,34 @@ fn a_directory_that_is_not_a_git_repository_scans_everything() { ); } -/// `--ignore-dirty-worktree` does not pretend the tree is clean: it moves the -/// far side of the diff to the working tree, so the edited file is named and -/// rescanned rather than keeping findings nothing analyzed. +/// A dirty tree does not pretend to be clean, and does not give up on +/// incremental either: the far side of the diff moves to the working tree, so +/// the edited file is named and rescanned rather than keeping findings nothing +/// analyzed. No flag — CI worktrees are dirty far more often than not. #[test] -fn ignore_dirty_worktree_diffs_the_working_tree_instead_of_refusing() { +fn a_dirty_worktree_diffs_the_working_tree_rather_than_scanning_everything() { + let output = scan_a_dirty_worktree(&[]); + assert!(output.contains("and your uncommitted changes"), "{output}"); +} + +/// Pipelines pass --ignore-dirty-worktree to get exactly the behaviour above, +/// which it no longer governs. Same scan either way, plus a warning, because a +/// flag that silently governs nothing is how a pipeline ends up believing it +/// asked for something. +#[test] +fn ignore_dirty_worktree_alone_warns_and_scans_incrementally_anyway() { + let output = scan_a_dirty_worktree(&["--ignore-dirty-worktree"]); + assert!(output.contains("and your uncommitted changes"), "{output}"); + assert!( + output.contains("--ignore-dirty-worktree has no effect without"), + "{output}" + ); +} + +/// Scan a tree with one uncommitted edit against a baseline at HEAD, asserting +/// the upload carries a worktree-covering diff. Returns stdout, stderr and the +/// API transcript, so a caller can assert on whichever stream it cares about. +fn scan_a_dirty_worktree(extra_args: &[&str]) -> String { let project = git_project(); let base_sha = project.sha.clone(); std::fs::write(project.path().join("main.py"), "print('uncommitted')\n") @@ -492,69 +515,15 @@ fn ignore_dirty_worktree_diffs_the_working_tree_instead_of_refusing() { ]; plan.extend(scan_tail()); - let api = ApiStub::start(plan); - let (mut command, _home) = cloud_command(&api, project.path()); - command.args([ - "scan", - "blast", - "--ignore-dirty-worktree", - "--project-name", - PROJECT, - ]); - - let output = run_with_timeout(command, &api); - let transcript = api.assert_finished(); - let context = output_context(&output, &transcript); - let stdout = String::from_utf8_lossy(&output.stdout); - - assert_eq!(output.status.code(), Some(0), "{context}"); - assert!(stdout.contains("and your uncommitted changes"), "{context}"); -} - -/// The server refuses a dirty tree too, and the refusal must come before the -/// baseline lookup: a commit-to-commit diff cannot see uncommitted edits, so no -/// baseline makes the list correct. -#[test] -fn a_dirty_worktree_skips_the_baseline_lookup_and_scans_everything() { - let project = git_project(); - let head_sha = second_commit(&project); - std::fs::write(project.path().join("main.py"), "print('uncommitted')\n") - .expect("dirty the tree"); - - let patch_sha = head_sha.clone(); - let mut plan = vec![ - verify_request(), - start_upload(), - expected_request( - "upload BLAST archive with no diff", - move |request| { - assert_authenticated_request( - request, - Method::PATCH, - "/api/v1/start-scan/transfer-123/", - )?; - assert_multipart_text_field(request, "sha", &patch_sha)?; - assert_multipart_text_field(request, "dirty", "true")?; - assert_no_multipart_field(request, "incremental_base_sha")?; - assert_no_multipart_field(request, "incremental_changed_files") - }, - json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), - ), - ]; - plan.extend(scan_tail()); - let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path()); command.args(["scan", "blast", "--project-name", PROJECT]); + command.args(extra_args); 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}" - ); + context } diff --git a/tests/cloud_commands_e2e/scan_list.rs b/tests/cloud_commands_e2e/scan_list.rs index 714f7aa..a43b26e 100644 --- a/tests/cloud_commands_e2e/scan_list.rs +++ b/tests/cloud_commands_e2e/scan_list.rs @@ -169,7 +169,7 @@ fn scan_clean_target_upload_sends_dirty_true_without_worktree_notice() { .trim() .to_string(); - let scan_api = ApiStub::start(blast_upload_plan(&sha, true, false)); + let scan_api = ApiStub::start(narrowed_blast_upload_plan(&sha)); let (mut scan_command, _scan_home) = cloud_command(&scan_api, project.path()); scan_command.args([ "scan", @@ -203,7 +203,7 @@ fn scan_clean_exclude_upload_sends_dirty_true_without_worktree_notice() { .trim() .to_string(); - let scan_api = ApiStub::start(blast_upload_plan(&sha, true, false)); + let scan_api = ApiStub::start(narrowed_blast_upload_plan(&sha)); let (mut scan_command, _scan_home) = cloud_command(&scan_api, project.path()); scan_command.args([ "scan", diff --git a/tests/cloud_commands_e2e/scan_skip.rs b/tests/cloud_commands_e2e/scan_skip.rs index 6b073b1..a8459df 100644 --- a/tests/cloud_commands_e2e/scan_skip.rs +++ b/tests/cloud_commands_e2e/scan_skip.rs @@ -32,16 +32,6 @@ fn prior_scan(sha: &str, created_at: &str) -> Value { }) } -/// The trunk baseline lookup a dirty run makes once --ignore-dirty-worktree -/// puts incremental back on the table. -fn baseline_lookup_for_branch(branch: &'static str, scans: Vec) -> ExpectedRequest { - expected_request( - "look up a baseline scan to diff against", - move |request| assert_baseline_lookup_request(request, PROJECT, branch), - json_response(scans_response(scans)), - ) -} - fn commit_lookup(sha: &str, scans: Vec) -> ExpectedRequest { let sha = sha.to_string(); expected_request( @@ -439,16 +429,14 @@ fn ignore_dirty_worktree_reuses_a_prior_dirty_scan() { } /// When nothing can be reused, the new scan still sends the real dirty status. -/// The override does not launder it -- it only lets the diff measure the -/// working tree, which is why a baseline lookup follows the reuse lookup here. +/// The override does not launder it -- it only governs reuse, which is why the +/// reuse lookup precedes the baseline lookup the incremental diff makes anyway. #[test] fn ignore_dirty_worktree_still_uploads_dirty_when_nothing_is_reused() { let project = git_project(); std::fs::write(project.path().join("main.py"), "print('dirty')\n") .expect("modify tracked file"); let mut plan = blast_upload_plan(&project.sha, true, false); - plan.insert(1, baseline_lookup_for_branch("master", vec![])); - plan.insert(1, baseline_lookup_for_branch("main", vec![])); plan.insert(1, commit_lookup(&project.sha, vec![])); let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path());