From 43b758063c2f9184811fb0e6b5862e303cb952ab Mon Sep 17 00:00:00 2001 From: Leen Kilani Date: Thu, 6 Aug 2026 14:44:28 +0300 Subject: [PATCH 1/5] COR-1766: report dirty worktree state with corgea scan uploads --- src/scanners/blast.rs | 18 ++++- src/utils/api.rs | 7 ++ src/utils/generic.rs | 92 ++++++++++++++++++++++++-- tests/cloud_commands_e2e/common/mod.rs | 20 ++++-- tests/cloud_commands_e2e/scan_list.rs | 27 ++++++++ 5 files changed, 151 insertions(+), 13 deletions(-) diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index 4923f05..4e41614 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -64,7 +64,6 @@ pub fn run( fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); let project_name = utils::generic::determine_project_name(project_name.as_deref()); let zip_path = format!("{}/{}.zip", temp_dir.display(), project_name); - let repo_info = utils::generic::get_repo_info("./").unwrap_or_default(); match utils::generic::create_path_if_not_exists(&temp_dir) { Ok(_) => (), Err(e) => { @@ -207,6 +206,23 @@ pub fn run( "\r{}Project packaged successfully.\n", utils::terminal::set_text_color("", utils::terminal::TerminalColor::Green) ); + // Read dirty/sha after packaging so the flag matches the uploaded archive. + let repo_info = utils::generic::get_repo_info("./").unwrap_or_default(); + if let Some(ref info) = repo_info { + if info.dirty { + match info.sha.as_deref() { + Some(sha) => { + let short_sha = &sha[..sha.len().min(7)]; + println!( + "Working tree has uncommitted changes - scanning your local files, not commit {short_sha}." + ); + } + None => { + println!("Working tree has uncommitted changes - scanning your local files.") + } + } + } + } println!("\n\nSubmitting scan to Corgea:"); let upload_result = match utils::api::upload_zip( &zip_path, diff --git a/src/utils/api.rs b/src/utils/api.rs index 6bfc784..2b1cba8 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -18,6 +18,8 @@ use std::path::Path; const CHUNK_SIZE: usize = 50 * 1024 * 1024; // 50 MB const API_BASE: &str = "/api/v1"; +const DIRTY_TRUE: &str = "true"; +const DIRTY_FALSE: &str = "false"; fn auth_headers(token: &str) -> HeaderMap { let mut headers = HeaderMap::new(); @@ -336,6 +338,11 @@ pub fn upload_zip( if let Some(sha) = &info.sha { form = form.part("sha", multipart::Part::text(sha.to_string())); } + // Always send dirty: omitted field = old CLI; "false" = clean tree. + form = form.part( + "dirty", + multipart::Part::text(if info.dirty { DIRTY_TRUE } else { DIRTY_FALSE }), + ); } if let Some(scan_type) = scan_type.clone() { let scan_type = if scan_type.contains("blast") { diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 3d28131..5e45e3d 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -1,5 +1,5 @@ use crate::utils::terminal::{set_text_color, TerminalColor}; -use git2::Repository; +use git2::{Repository, StatusOptions}; use globset::{Glob, GlobSetBuilder}; use ignore::WalkBuilder; use std::env; @@ -297,13 +297,33 @@ pub fn get_repo_info(dir: &str) -> Result, git2::Error> { .map(|commit| commit.id().to_string()) }); + let dirty = is_worktree_dirty(&repo); + Ok(Some(RepoInfo { branch, repo_url: origin_url(&repo), sha, + dirty, })) } +/// True when the worktree has modified, staged, or untracked files. +/// Gitignored paths alone do not count; submodules are excluded. +/// +/// Untracked paths that packaging would later drop via `DEFAULT_EXCLUDE_GLOBS` +/// still count as dirty (false-positive dirty costs a full scan, not a miss). +/// Status errors also treat the tree as dirty so we never claim clean HEAD. +fn is_worktree_dirty(repo: &Repository) -> bool { + let mut opts = StatusOptions::new(); + opts.include_untracked(true) + .recurse_untracked_dirs(true) + .include_ignored(false) + .exclude_submodules(true); + repo.statuses(Some(&mut opts)) + .map(|s| !s.is_empty()) + .unwrap_or(true) +} + /// `origin`'s URL, or None when the remote is missing or carries no URL. fn origin_url(repo: &Repository) -> Option { repo.find_remote("origin") @@ -412,6 +432,7 @@ pub struct RepoInfo { pub branch: Option, pub repo_url: Option, pub sha: Option, + pub dirty: bool, } #[cfg(test)] @@ -440,12 +461,7 @@ mod tests { fn get_repo_info_at_root_only_not_nested_cwd() { let dir = tempfile::tempdir().unwrap(); let root = dir.path(); - git(root, &["init"]); - git(root, &["config", "user.email", "test@example.com"]); - git(root, &["config", "user.name", "Test"]); - fs::write(root.join("README"), "hi").unwrap(); - git(root, &["add", "README"]); - git(root, &["commit", "-m", "init"]); + init_committed_repo(root); let root_s = root.to_str().unwrap(); let nested = root.join("pkg").join("inner"); @@ -456,6 +472,7 @@ mod tests { .unwrap() .expect("repo root should yield SHA metadata"); assert!(info.sha.is_some()); + assert!(!info.dirty, "clean commit should report dirty=false"); assert!(is_at_repo_root(root_s)); assert!( @@ -465,6 +482,67 @@ mod tests { assert!(!is_at_repo_root(nested_s)); } + fn init_committed_repo(root: &std::path::Path) { + git(root, &["init"]); + git(root, &["config", "user.email", "test@example.com"]); + git(root, &["config", "user.name", "Test"]); + fs::write(root.join("README"), "hi").unwrap(); + git(root, &["add", "README"]); + git(root, &["commit", "-m", "init"]); + } + + #[test] + fn get_repo_info_dirty_true_when_tracked_file_modified() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + fs::write(root.join("README"), "changed").unwrap(); + let info = get_repo_info(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(info.dirty); + } + + #[test] + fn get_repo_info_dirty_true_when_change_staged() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + fs::write(root.join("README"), "staged").unwrap(); + git(root, &["add", "README"]); + let info = get_repo_info(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(info.dirty); + } + + #[test] + fn get_repo_info_dirty_true_when_untracked_file() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + fs::write(root.join("new.py"), "print(1)").unwrap(); + let info = get_repo_info(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(info.dirty); + } + + #[test] + fn get_repo_info_dirty_false_when_only_gitignored_file() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + fs::write(root.join(".gitignore"), "ignored.txt\n").unwrap(); + git(root, &["add", ".gitignore"]); + git(root, &["commit", "-m", "ignore"]); + fs::write(root.join("ignored.txt"), "secret").unwrap(); + let info = get_repo_info(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(!info.dirty); + } + #[test] fn create_zip_from_target_excludes_default_globs() { let dir = tempfile::tempdir().unwrap(); diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index 3cd26a8..b475a28 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -767,12 +767,17 @@ pub(crate) fn assert_issue_summary(stdout: &str, context: &str) { } pub(crate) fn blast_plan(sha: &str) -> Vec { + blast_upload_plan(sha, false, true) +} + +/// 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 { 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 sca_path = "/api/v1/scan/blast-scan-123/issues/sca".to_string(); - vec![ + let mut plan = vec![ verify_request(), expected_request( "start BLAST upload", @@ -808,6 +813,7 @@ pub(crate) fn blast_plan(sha: &str) -> Vec { "https://github.com/corgea/cloud-e2e.git", )?; assert_multipart_text_field(request, "sha", &patch_sha)?; + assert_multipart_text_field(request, "dirty", &dirty_value)?; assert_body_contains(request, b"name=\"chunk_data\"") }, json_response(json!({ @@ -829,7 +835,10 @@ pub(crate) fn blast_plan(sha: &str) -> Vec { }, json_response(empty_issue_page()), ), - expected_request( + ]; + if include_sca { + let sca_path = "/api/v1/scan/blast-scan-123/issues/sca".to_string(); + plan.push(expected_request( "read malicious SCA issues", move |request| { assert_authenticated_request(request, Method::GET, &sca_path)?; @@ -837,6 +846,7 @@ pub(crate) fn blast_plan(sha: &str) -> Vec { assert_query(request, "page_size", "30") }, json_response(malicious_sca_issue_page()), - ), - ] + )); + } + plan } diff --git a/tests/cloud_commands_e2e/scan_list.rs b/tests/cloud_commands_e2e/scan_list.rs index 9632472..f210d48 100644 --- a/tests/cloud_commands_e2e/scan_list.rs +++ b/tests/cloud_commands_e2e/scan_list.rs @@ -26,6 +26,10 @@ fn scan_fail_on_malicious_sends_sha_and_list_renders_it() { scan_stdout.contains("matched --fail-on malicious"), "{scan_context}" ); + assert!( + !scan_stdout.contains("Working tree has uncommitted changes"), + "clean tree must not print dirty notice\n{scan_context}" + ); let list_response_sha = project.sha.clone(); let list_api = ApiStub::start(vec![ @@ -72,6 +76,29 @@ fn scan_fail_on_malicious_sends_sha_and_list_renders_it() { assert!(list_stdout.contains(&project.sha[..8]), "{list_context}"); } +#[test] +fn scan_dirty_worktree_sends_dirty_true_and_prints_notice() { + let project = git_project(); + std::fs::write(project.path().join("main.py"), "print('dirty')\n") + .expect("modify tracked file"); + let short_sha = &project.sha[..7]; + let scan_api = ApiStub::start(blast_upload_plan(&project.sha, true, false)); + let (mut scan_command, _scan_home) = cloud_command(&scan_api, project.path()); + scan_command.args(["scan", "blast", "--project-name", "cloud-e2e"]); + + let scan_output = run_with_timeout(scan_command, &scan_api); + let scan_transcript = scan_api.assert_finished(); + let scan_context = output_context(&scan_output, &scan_transcript); + assert_eq!(scan_output.status.code(), Some(0), "{scan_context}"); + let scan_stdout = String::from_utf8_lossy(&scan_output.stdout); + assert!( + scan_stdout.contains(&format!( + "Working tree has uncommitted changes - scanning your local files, not commit {short_sha}." + )), + "{scan_context}" + ); +} + #[test] fn list_json_returns_filtered_scan_contract() { let project = TempDir::new().expect("create list project"); From a848e8bff02820d2440cc9842da40568d1e7f3f7 Mon Sep 17 00:00:00 2001 From: Leen Kilani Date: Sun, 9 Aug 2026 13:50:42 +0300 Subject: [PATCH 2/5] COR-1764: address comments --- src/scanners/blast.rs | 44 +++++--- src/utils/api.rs | 2 +- src/utils/generic.rs | 142 +++++++++++++++++++++++++- tests/cloud_commands_e2e/scan_list.rs | 66 ++++++++++++ 4 files changed, 234 insertions(+), 20 deletions(-) diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index 4e41614..e63d432 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -90,6 +90,10 @@ pub fn run( target.as_deref() }; + // Sample before packaging so a mid-pack commit cannot advertise a new clean HEAD + // against an archive built from the previous (or mixed) tree. + let repo_before = utils::generic::get_repo_info("./").unwrap_or_default(); + if target_str.is_none() && exclude.is_some() { println!("Excluding files matching: {}", exclude.as_deref().unwrap()); } @@ -206,23 +210,35 @@ pub fn run( "\r{}Project packaged successfully.\n", utils::terminal::set_text_color("", utils::terminal::TerminalColor::Green) ); - // Read dirty/sha after packaging so the flag matches the uploaded archive. - let repo_info = utils::generic::get_repo_info("./").unwrap_or_default(); - if let Some(ref info) = repo_info { - if info.dirty { - match info.sha.as_deref() { - Some(sha) => { - let short_sha = &sha[..sha.len().min(7)]; - println!( - "Working tree has uncommitted changes - scanning your local files, not commit {short_sha}." - ); - } - None => { - println!("Working tree has uncommitted changes - scanning your local files.") - } + let repo_after = utils::generic::get_repo_info("./").unwrap_or_default(); + // User notice reflects actual worktree dirtiness, not upload fail-safes + // (--target/--exclude or before/after SHA drift). + let worktree_dirty = repo_before.as_ref().is_some_and(|i| i.dirty) + || repo_after.as_ref().is_some_and(|i| i.dirty); + if worktree_dirty { + let notice_sha = repo_after + .as_ref() + .and_then(|i| i.sha.as_deref()) + .or_else(|| repo_before.as_ref().and_then(|i| i.sha.as_deref())); + match notice_sha { + Some(sha) => { + let short_sha = &sha[..sha.len().min(7)]; + println!( + "Working tree has uncommitted changes - scanning your local files, not commit {short_sha}." + ); + } + None => { + println!("Working tree has uncommitted changes - scanning your local files.") } } } + let mut repo_info = utils::generic::reconcile_repo_info_for_upload(repo_before, repo_after); + // Partial archives are not an exact HEAD snapshot even on a clean tree. + if target_str.is_some() || exclude.is_some() { + if let Some(ref mut info) = repo_info { + info.dirty = true; + } + } println!("\n\nSubmitting scan to Corgea:"); let upload_result = match utils::api::upload_zip( &zip_path, diff --git a/src/utils/api.rs b/src/utils/api.rs index 2b1cba8..627e169 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -338,7 +338,7 @@ pub fn upload_zip( if let Some(sha) = &info.sha { form = form.part("sha", multipart::Part::text(sha.to_string())); } - // Always send dirty: omitted field = old CLI; "false" = clean tree. + // Always send dirty: omitted = old CLI; false = exact clean HEAD snapshot. form = form.part( "dirty", multipart::Part::text(if info.dirty { DIRTY_TRUE } else { DIRTY_FALSE }), diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 5e45e3d..387b982 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -307,23 +307,52 @@ pub fn get_repo_info(dir: &str) -> Result, git2::Error> { })) } -/// True when the worktree has modified, staged, or untracked files. -/// Gitignored paths alone do not count; submodules are excluded. +/// True when the worktree has modified, staged, or untracked files (including +/// dirty submodules). Gitignored paths alone do not count. /// /// Untracked paths that packaging would later drop via `DEFAULT_EXCLUDE_GLOBS` /// still count as dirty (false-positive dirty costs a full scan, not a miss). /// Status errors also treat the tree as dirty so we never claim clean HEAD. fn is_worktree_dirty(repo: &Repository) -> bool { let mut opts = StatusOptions::new(); + // Include submodule status: packaging walks into submodule dirs, so a + // modified checkout must not be advertised as clean parent HEAD. opts.include_untracked(true) .recurse_untracked_dirs(true) - .include_ignored(false) - .exclude_submodules(true); + .include_ignored(false); repo.statuses(Some(&mut opts)) .map(|s| !s.is_empty()) .unwrap_or(true) } +/// Merge before/after packaging samples into upload metadata. +/// +/// Only `dirty=false` when both samples exist, are clean, and share the same +/// SHA. Any missing sample, dirty sample, or SHA drift fails safe to dirty. +/// Prefer the post-packaging sample for branch/url/sha display. +pub fn reconcile_repo_info_for_upload( + before: Option, + after: Option, +) -> Option { + match (before, after) { + (None, None) => None, + (Some(sample), None) | (None, Some(sample)) => Some(RepoInfo { + dirty: true, + ..sample + }), + (Some(before), Some(after)) => { + let stable_clean = + !before.dirty && !after.dirty && before.sha.is_some() && before.sha == after.sha; + Some(RepoInfo { + branch: after.branch.or(before.branch), + repo_url: after.repo_url.or(before.repo_url), + sha: after.sha.or(before.sha), + dirty: !stable_clean, + }) + } + } +} + /// `origin`'s URL, or None when the remote is missing or carries no URL. fn origin_url(repo: &Repository) -> Option { repo.find_remote("origin") @@ -427,11 +456,12 @@ pub fn get_status(status: &str) -> &str { } } -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct RepoInfo { pub branch: Option, pub repo_url: Option, pub sha: Option, + /// True when the upload must not be treated as an exact clean HEAD snapshot. pub dirty: bool, } @@ -543,6 +573,108 @@ mod tests { assert!(!info.dirty); } + fn sample_info(sha: &str, dirty: bool) -> RepoInfo { + RepoInfo { + branch: Some("main".into()), + repo_url: Some("https://github.com/org/repo.git".into()), + sha: Some(sha.into()), + dirty, + } + } + + #[test] + fn reconcile_clean_same_sha_stays_clean() { + let before = sample_info("aaa", false); + let after = sample_info("aaa", false); + let out = reconcile_repo_info_for_upload(Some(before), Some(after)).unwrap(); + assert!(!out.dirty); + assert_eq!(out.sha.as_deref(), Some("aaa")); + } + + #[test] + fn reconcile_sha_drift_marks_dirty() { + let before = sample_info("aaa", false); + let after = sample_info("bbb", false); + let out = reconcile_repo_info_for_upload(Some(before), Some(after)).unwrap(); + assert!(out.dirty); + assert_eq!(out.sha.as_deref(), Some("bbb")); + } + + #[test] + fn reconcile_either_dirty_marks_dirty() { + let before = sample_info("aaa", true); + let after = sample_info("aaa", false); + let out = reconcile_repo_info_for_upload(Some(before), Some(after)).unwrap(); + assert!(out.dirty); + + let before = sample_info("aaa", false); + let after = sample_info("aaa", true); + let out = reconcile_repo_info_for_upload(Some(before), Some(after)).unwrap(); + assert!(out.dirty); + } + + #[test] + fn reconcile_missing_sample_marks_dirty() { + let only = sample_info("aaa", false); + assert!( + reconcile_repo_info_for_upload(Some(only.clone()), None) + .unwrap() + .dirty + ); + assert!( + reconcile_repo_info_for_upload(None, Some(only)) + .unwrap() + .dirty + ); + assert!(reconcile_repo_info_for_upload(None, None).is_none()); + } + + #[test] + fn get_repo_info_dirty_when_submodule_content_modified() { + let parent_dir = tempfile::tempdir().unwrap(); + let parent = parent_dir.path(); + init_committed_repo(parent); + + // Keep the submodule source outside the parent so it is not an + // untracked sibling that would itself mark the tree dirty. + let sub_dir = tempfile::tempdir().unwrap(); + let sub_src = sub_dir.path(); + git(sub_src, &["init"]); + git(sub_src, &["config", "user.email", "test@example.com"]); + git(sub_src, &["config", "user.name", "Test"]); + fs::write(sub_src.join("lib.py"), "v1\n").unwrap(); + git(sub_src, &["add", "lib.py"]); + git(sub_src, &["commit", "-m", "sub init"]); + + // Modern git blocks file:// clones unless explicitly allowed. + git( + parent, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + sub_src.to_str().unwrap(), + "vendor", + ], + ); + git(parent, &["commit", "-m", "add submodule"]); + + let clean = get_repo_info(parent.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(!clean.dirty, "committed submodule should be clean"); + + fs::write(parent.join("vendor").join("lib.py"), "v2\n").unwrap(); + let dirty = get_repo_info(parent.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!( + dirty.dirty, + "modified submodule checkout must mark parent dirty" + ); + } + #[test] fn create_zip_from_target_excludes_default_globs() { let dir = tempfile::tempdir().unwrap(); diff --git a/tests/cloud_commands_e2e/scan_list.rs b/tests/cloud_commands_e2e/scan_list.rs index f210d48..a0e179f 100644 --- a/tests/cloud_commands_e2e/scan_list.rs +++ b/tests/cloud_commands_e2e/scan_list.rs @@ -99,6 +99,72 @@ fn scan_dirty_worktree_sends_dirty_true_and_prints_notice() { ); } +#[test] +fn scan_clean_target_upload_sends_dirty_true_without_worktree_notice() { + let project = git_project(); + std::fs::write(project.path().join("other.py"), "print('other')\n").expect("write other"); + run_git(project.path(), &["add", "other.py"]); + run_git(project.path(), &["commit", "-m", "add other"]); + let sha = String::from_utf8(run_git(project.path(), &["rev-parse", "HEAD"]).stdout) + .expect("UTF-8 SHA") + .trim() + .to_string(); + + let scan_api = ApiStub::start(blast_upload_plan(&sha, true, false)); + let (mut scan_command, _scan_home) = cloud_command(&scan_api, project.path()); + scan_command.args([ + "scan", + "blast", + "--target", + "main.py", + "--project-name", + "cloud-e2e", + ]); + + let scan_output = run_with_timeout(scan_command, &scan_api); + let scan_transcript = scan_api.assert_finished(); + let scan_context = output_context(&scan_output, &scan_transcript); + assert_eq!(scan_output.status.code(), Some(0), "{scan_context}"); + let scan_stdout = String::from_utf8_lossy(&scan_output.stdout); + assert!( + !scan_stdout.contains("Working tree has uncommitted changes"), + "clean partial target must not print dirty worktree notice\n{scan_context}" + ); +} + +#[test] +fn scan_clean_exclude_upload_sends_dirty_true_without_worktree_notice() { + let project = git_project(); + std::fs::write(project.path().join("other.py"), "print('other')\n").expect("write other"); + run_git(project.path(), &["add", "other.py"]); + run_git(project.path(), &["commit", "-m", "add other"]); + let sha = String::from_utf8(run_git(project.path(), &["rev-parse", "HEAD"]).stdout) + .expect("UTF-8 SHA") + .trim() + .to_string(); + + let scan_api = ApiStub::start(blast_upload_plan(&sha, true, false)); + let (mut scan_command, _scan_home) = cloud_command(&scan_api, project.path()); + scan_command.args([ + "scan", + "blast", + "--exclude", + "other.py", + "--project-name", + "cloud-e2e", + ]); + + let scan_output = run_with_timeout(scan_command, &scan_api); + let scan_transcript = scan_api.assert_finished(); + let scan_context = output_context(&scan_output, &scan_transcript); + assert_eq!(scan_output.status.code(), Some(0), "{scan_context}"); + let scan_stdout = String::from_utf8_lossy(&scan_output.stdout); + assert!( + !scan_stdout.contains("Working tree has uncommitted changes"), + "clean exclude scan must not print dirty worktree notice\n{scan_context}" + ); +} + #[test] fn list_json_returns_filtered_scan_contract() { let project = TempDir::new().expect("create list project"); From 7cc993769a4167588802ca6cbe9fd9c15ba60f53 Mon Sep 17 00:00:00 2001 From: Leen Kilani Date: Mon, 10 Aug 2026 11:41:06 +0300 Subject: [PATCH 3/5] fix tests --- src/utils/generic.rs | 24 +++++++++++++++++++++++- tests/cloud_commands_e2e/scan_list.rs | 6 ++++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 387b982..b7ddaa9 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -109,7 +109,9 @@ pub fn create_zip_from_target>( let mut excluded_files = Vec::new(); for (path, relative_path) in files_to_zip { - let is_excluded = glob_set.is_match(&path); + // Match against the repo-relative path. Absolute paths (target mode) + // can live under `/tmp/...` on Linux and would falsely hit `**/tmp/**`. + let is_excluded = glob_set.is_match(&relative_path); if (path.is_file() || path.is_dir()) && !is_excluded { if path.is_file() { @@ -718,6 +720,26 @@ mod tests { ); } + #[test] + fn default_exclude_globs_match_abs_tmp_but_not_repo_relative_paths() { + // Linux CI tempdirs are `/tmp/...`. Matching DEFAULT_EXCLUDE_GLOBS on the + // absolute path made `--target` drop every file via `**/tmp/**`. + let mut builder = GlobSetBuilder::new(); + for &pattern in DEFAULT_EXCLUDE_GLOBS { + builder.add(Glob::new(pattern).unwrap()); + } + let set = builder.build().unwrap(); + assert!( + set.is_match(Path::new("/tmp/proj/app.py")), + "absolute /tmp paths hit **/tmp/** (the CI failure mode)" + ); + assert!( + !set.is_match(Path::new("app.py")), + "repo-relative paths must stay scannable" + ); + assert!(!set.is_match(Path::new("src/app.py"))); + } + #[test] fn extract_repo_path_handles_common_remote_forms() { for url in [ diff --git a/tests/cloud_commands_e2e/scan_list.rs b/tests/cloud_commands_e2e/scan_list.rs index a0e179f..d715a16 100644 --- a/tests/cloud_commands_e2e/scan_list.rs +++ b/tests/cloud_commands_e2e/scan_list.rs @@ -122,9 +122,10 @@ fn scan_clean_target_upload_sends_dirty_true_without_worktree_notice() { ]); let scan_output = run_with_timeout(scan_command, &scan_api); + let scan_context_early = output_context(&scan_output, &scan_api.transcript()); + assert_eq!(scan_output.status.code(), Some(0), "{scan_context_early}"); let scan_transcript = scan_api.assert_finished(); let scan_context = output_context(&scan_output, &scan_transcript); - assert_eq!(scan_output.status.code(), Some(0), "{scan_context}"); let scan_stdout = String::from_utf8_lossy(&scan_output.stdout); assert!( !scan_stdout.contains("Working tree has uncommitted changes"), @@ -155,9 +156,10 @@ fn scan_clean_exclude_upload_sends_dirty_true_without_worktree_notice() { ]); let scan_output = run_with_timeout(scan_command, &scan_api); + let scan_context_early = output_context(&scan_output, &scan_api.transcript()); + assert_eq!(scan_output.status.code(), Some(0), "{scan_context_early}"); let scan_transcript = scan_api.assert_finished(); let scan_context = output_context(&scan_output, &scan_transcript); - assert_eq!(scan_output.status.code(), Some(0), "{scan_context}"); let scan_stdout = String::from_utf8_lossy(&scan_output.stdout); assert!( !scan_stdout.contains("Working tree has uncommitted changes"), From bc9b8fa9011893df358b5229d256833ce0d74bb5 Mon Sep 17 00:00:00 2001 From: Leen Kilani Date: Wed, 12 Aug 2026 12:10:36 +0300 Subject: [PATCH 4/5] COR-1764: address comments --- src/scanners/blast.rs | 4 +-- src/utils/generic.rs | 62 ++++++++++++++++++++++++++++++++----------- 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index d1a3ebb..6628174 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -100,7 +100,7 @@ pub fn run( // Sample before packaging so a mid-pack commit cannot advertise a new clean HEAD // against an archive built from the previous (or mixed) tree. - let repo_before = utils::generic::get_repo_info("./").unwrap_or_default(); + let repo_before = utils::generic::get_repo_info_for_scan("./").unwrap_or_default(); if target_str.is_none() && exclude.is_some() { println!("Excluding files matching: {}", exclude.as_deref().unwrap()); @@ -218,7 +218,7 @@ pub fn run( "\r{}Project packaged successfully.\n", utils::terminal::set_text_color("", utils::terminal::TerminalColor::Green) ); - let repo_after = utils::generic::get_repo_info("./").unwrap_or_default(); + let repo_after = utils::generic::get_repo_info_for_scan("./").unwrap_or_default(); // User notice reflects actual worktree dirtiness, not upload fail-safes // (--target/--exclude or before/after SHA drift). let worktree_dirty = repo_before.as_ref().is_some_and(|i| i.dirty) diff --git a/src/utils/generic.rs b/src/utils/generic.rs index b7ddaa9..1af6bb1 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -269,7 +269,19 @@ pub fn get_env_var_if_exists(var_name: &str) -> Option { } } +/// Worktree identity (branch / url / sha) at the repo root. +/// Does not walk git status — `dirty` is always false. Use +/// [`get_repo_info_for_scan`] when building BLAST upload metadata. pub fn get_repo_info(dir: &str) -> Result, git2::Error> { + get_repo_info_inner(dir, false) +} + +/// Like [`get_repo_info`], plus a worktree dirty sample for scan uploads. +pub fn get_repo_info_for_scan(dir: &str) -> Result, git2::Error> { + get_repo_info_inner(dir, true) +} + +fn get_repo_info_inner(dir: &str, sample_dirty: bool) -> Result, git2::Error> { // discover (not open) so worktrees / .git-as-file roots still resolve. let repo = match Repository::discover(Path::new(dir)) { Ok(repo) => repo, @@ -299,13 +311,11 @@ pub fn get_repo_info(dir: &str) -> Result, git2::Error> { .map(|commit| commit.id().to_string()) }); - let dirty = is_worktree_dirty(&repo); - Ok(Some(RepoInfo { branch, repo_url: origin_url(&repo), sha, - dirty, + dirty: sample_dirty && is_worktree_dirty(&repo), })) } @@ -463,7 +473,8 @@ pub struct RepoInfo { pub branch: Option, pub repo_url: Option, pub sha: Option, - /// True when the upload must not be treated as an exact clean HEAD snapshot. + /// Upload must not be treated as an exact clean HEAD snapshot. + /// Always false from [`get_repo_info`] (unsampled); set by [`get_repo_info_for_scan`]. pub dirty: bool, } @@ -504,7 +515,6 @@ mod tests { .unwrap() .expect("repo root should yield SHA metadata"); assert!(info.sha.is_some()); - assert!(!info.dirty, "clean commit should report dirty=false"); assert!(is_at_repo_root(root_s)); assert!( @@ -524,44 +534,44 @@ mod tests { } #[test] - fn get_repo_info_dirty_true_when_tracked_file_modified() { + fn get_repo_info_for_scan_dirty_true_when_tracked_file_modified() { let dir = tempfile::tempdir().unwrap(); let root = dir.path(); init_committed_repo(root); fs::write(root.join("README"), "changed").unwrap(); - let info = get_repo_info(root.to_str().unwrap()) + let info = get_repo_info_for_scan(root.to_str().unwrap()) .unwrap() .expect("repo info"); assert!(info.dirty); } #[test] - fn get_repo_info_dirty_true_when_change_staged() { + fn get_repo_info_for_scan_dirty_true_when_change_staged() { let dir = tempfile::tempdir().unwrap(); let root = dir.path(); init_committed_repo(root); fs::write(root.join("README"), "staged").unwrap(); git(root, &["add", "README"]); - let info = get_repo_info(root.to_str().unwrap()) + let info = get_repo_info_for_scan(root.to_str().unwrap()) .unwrap() .expect("repo info"); assert!(info.dirty); } #[test] - fn get_repo_info_dirty_true_when_untracked_file() { + fn get_repo_info_for_scan_dirty_true_when_untracked_file() { let dir = tempfile::tempdir().unwrap(); let root = dir.path(); init_committed_repo(root); fs::write(root.join("new.py"), "print(1)").unwrap(); - let info = get_repo_info(root.to_str().unwrap()) + let info = get_repo_info_for_scan(root.to_str().unwrap()) .unwrap() .expect("repo info"); assert!(info.dirty); } #[test] - fn get_repo_info_dirty_false_when_only_gitignored_file() { + fn get_repo_info_for_scan_dirty_false_when_only_gitignored_file() { let dir = tempfile::tempdir().unwrap(); let root = dir.path(); init_committed_repo(root); @@ -569,12 +579,34 @@ mod tests { git(root, &["add", ".gitignore"]); git(root, &["commit", "-m", "ignore"]); fs::write(root.join("ignored.txt"), "secret").unwrap(); - let info = get_repo_info(root.to_str().unwrap()) + let info = get_repo_info_for_scan(root.to_str().unwrap()) .unwrap() .expect("repo info"); assert!(!info.dirty); } + #[test] + fn get_repo_info_skips_dirty_sampling() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + + let clean = get_repo_info_for_scan(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(!clean.dirty); + + fs::write(root.join("README"), "changed").unwrap(); + let identity = get_repo_info(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(!identity.dirty); + let scan = get_repo_info_for_scan(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(scan.dirty); + } + fn sample_info(sha: &str, dirty: bool) -> RepoInfo { RepoInfo { branch: Some("main".into()), @@ -662,13 +694,13 @@ mod tests { ); git(parent, &["commit", "-m", "add submodule"]); - let clean = get_repo_info(parent.to_str().unwrap()) + let clean = get_repo_info_for_scan(parent.to_str().unwrap()) .unwrap() .expect("repo info"); assert!(!clean.dirty, "committed submodule should be clean"); fs::write(parent.join("vendor").join("lib.py"), "v2\n").unwrap(); - let dirty = get_repo_info(parent.to_str().unwrap()) + let dirty = get_repo_info_for_scan(parent.to_str().unwrap()) .unwrap() .expect("repo info"); assert!( From 8ceb633faf3d5a33d34d69595e854306fc845b4d Mon Sep 17 00:00:00 2001 From: Leen Kilani Date: Wed, 12 Aug 2026 18:22:25 +0300 Subject: [PATCH 5/5] COR-1764: address comments --- src/scanners/blast.rs | 12 ++--- src/utils/api.rs | 2 +- src/utils/generic.rs | 121 +++++++++++++++++++++++++++++------------- 3 files changed, 90 insertions(+), 45 deletions(-) diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index 6628174..dd7f0a0 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -98,8 +98,7 @@ pub fn run( target.as_deref() }; - // Sample before packaging so a mid-pack commit cannot advertise a new clean HEAD - // against an archive built from the previous (or mixed) tree. + // Before packaging: mid-pack HEAD move must not look like a clean new SHA. let repo_before = utils::generic::get_repo_info_for_scan("./").unwrap_or_default(); if target_str.is_none() && exclude.is_some() { @@ -219,10 +218,9 @@ pub fn run( utils::terminal::set_text_color("", utils::terminal::TerminalColor::Green) ); let repo_after = utils::generic::get_repo_info_for_scan("./").unwrap_or_default(); - // User notice reflects actual worktree dirtiness, not upload fail-safes - // (--target/--exclude or before/after SHA drift). - let worktree_dirty = repo_before.as_ref().is_some_and(|i| i.dirty) - || repo_after.as_ref().is_some_and(|i| i.dirty); + // Notice = visible status only (not index hide-bits / --target / SHA drift). + let worktree_dirty = repo_before.as_ref().is_some_and(|i| i.status_dirty) + || repo_after.as_ref().is_some_and(|i| i.status_dirty); if worktree_dirty { let notice_sha = repo_after .as_ref() @@ -241,7 +239,7 @@ pub fn run( } } let mut repo_info = utils::generic::reconcile_repo_info_for_upload(repo_before, repo_after); - // Partial archives are not an exact HEAD snapshot even on a clean tree. + // --target/--exclude archives are never an exact HEAD snapshot. if target_str.is_some() || exclude.is_some() { if let Some(ref mut info) = repo_info { info.dirty = true; diff --git a/src/utils/api.rs b/src/utils/api.rs index 882a4e6..880fded 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -350,7 +350,7 @@ pub fn upload_zip( if let Some(sha) = &info.sha { form = form.part("sha", multipart::Part::text(sha.to_string())); } - // Always send dirty: omitted = old CLI; false = exact clean HEAD snapshot. + // Always send: omitted = old CLI; false = clean HEAD snapshot. form = form.part( "dirty", multipart::Part::text(if info.dirty { DIRTY_TRUE } else { DIRTY_FALSE }), diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 1af6bb1..f9300fe 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -1,5 +1,5 @@ use crate::utils::terminal::{set_text_color, TerminalColor}; -use git2::{Repository, StatusOptions}; +use git2::{IndexEntryExtendedFlag, IndexEntryFlag, Repository, StatusOptions}; use globset::{Glob, GlobSetBuilder}; use ignore::WalkBuilder; use std::env; @@ -109,8 +109,7 @@ pub fn create_zip_from_target>( let mut excluded_files = Vec::new(); for (path, relative_path) in files_to_zip { - // Match against the repo-relative path. Absolute paths (target mode) - // can live under `/tmp/...` on Linux and would falsely hit `**/tmp/**`. + // Match repo-relative paths so abs `/tmp/...` targets don't hit `**/tmp/**`. let is_excluded = glob_set.is_match(&relative_path); if (path.is_file() || path.is_dir()) && !is_excluded { @@ -269,14 +268,13 @@ pub fn get_env_var_if_exists(var_name: &str) -> Option { } } -/// Worktree identity (branch / url / sha) at the repo root. -/// Does not walk git status — `dirty` is always false. Use -/// [`get_repo_info_for_scan`] when building BLAST upload metadata. +/// Repo identity at the worktree root. Does not sample dirty state — use +/// [`get_repo_info_for_scan`] for BLAST uploads. pub fn get_repo_info(dir: &str) -> Result, git2::Error> { get_repo_info_inner(dir, false) } -/// Like [`get_repo_info`], plus a worktree dirty sample for scan uploads. +/// [`get_repo_info`] plus dirty sampling for scan uploads. pub fn get_repo_info_for_scan(dir: &str) -> Result, git2::Error> { get_repo_info_inner(dir, true) } @@ -311,24 +309,34 @@ fn get_repo_info_inner(dir: &str, sample_dirty: bool) -> Result .map(|commit| commit.id().to_string()) }); + let (dirty, status_dirty) = if sample_dirty { + worktree_dirty_flags(&repo) + } else { + (false, false) + }; + Ok(Some(RepoInfo { branch, repo_url: origin_url(&repo), sha, - dirty: sample_dirty && is_worktree_dirty(&repo), + dirty, + status_dirty, })) } -/// True when the worktree has modified, staged, or untracked files (including -/// dirty submodules). Gitignored paths alone do not count. -/// -/// Untracked paths that packaging would later drop via `DEFAULT_EXCLUDE_GLOBS` -/// still count as dirty (false-positive dirty costs a full scan, not a miss). -/// Status errors also treat the tree as dirty so we never claim clean HEAD. -fn is_worktree_dirty(repo: &Repository) -> bool { +/// `(upload_dirty, status_dirty)`. +/// `upload_dirty`: status changes, dirty submodules, or assume-unchanged / +/// skip-worktree (status hides those). Errors fail closed to dirty. +/// `status_dirty`: non-empty `statuses()` only (user notice). +fn worktree_dirty_flags(repo: &Repository) -> (bool, bool) { + let status_dirty = status_has_changes(repo); + let upload_dirty = index_hides_worktree(repo) || status_dirty; + (upload_dirty, status_dirty) +} + +fn status_has_changes(repo: &Repository) -> bool { let mut opts = StatusOptions::new(); - // Include submodule status: packaging walks into submodule dirs, so a - // modified checkout must not be advertised as clean parent HEAD. + // Submodules: packaging walks into them, so dirty checkouts must count. opts.include_untracked(true) .recurse_untracked_dirs(true) .include_ignored(false); @@ -337,11 +345,21 @@ fn is_worktree_dirty(repo: &Repository) -> bool { .unwrap_or(true) } -/// Merge before/after packaging samples into upload metadata. -/// -/// Only `dirty=false` when both samples exist, are clean, and share the same -/// SHA. Any missing sample, dirty sample, or SHA drift fails safe to dirty. -/// Prefer the post-packaging sample for branch/url/sha display. +/// assume-unchanged / skip-worktree are omitted from `statuses()`. +fn index_hides_worktree(repo: &Repository) -> bool { + repo.index() + .map(|index| { + index.iter().any(|entry| { + IndexEntryFlag::from_bits_truncate(entry.flags).is_valid() + || IndexEntryExtendedFlag::from_bits_truncate(entry.flags_extended) + .is_skip_worktree() + }) + }) + .unwrap_or(true) +} + +/// Merge before/after packaging samples. Clean only if both exist, both clean, +/// same SHA; otherwise dirty. Prefer post-packaging branch/url/sha. pub fn reconcile_repo_info_for_upload( before: Option, after: Option, @@ -360,6 +378,7 @@ pub fn reconcile_repo_info_for_upload( repo_url: after.repo_url.or(before.repo_url), sha: after.sha.or(before.sha), dirty: !stable_clean, + status_dirty: before.status_dirty || after.status_dirty, }) } } @@ -473,9 +492,10 @@ pub struct RepoInfo { pub branch: Option, pub repo_url: Option, pub sha: Option, - /// Upload must not be treated as an exact clean HEAD snapshot. - /// Always false from [`get_repo_info`] (unsampled); set by [`get_repo_info_for_scan`]. + /// Not an exact clean HEAD snapshot. Always false from [`get_repo_info`]. pub dirty: bool, + /// Non-empty git status (excludes index hide-bits). Drives user notice. + pub status_dirty: bool, } #[cfg(test)] @@ -543,6 +563,7 @@ mod tests { .unwrap() .expect("repo info"); assert!(info.dirty); + assert!(info.status_dirty); } #[test] @@ -556,6 +577,7 @@ mod tests { .unwrap() .expect("repo info"); assert!(info.dirty); + assert!(info.status_dirty); } #[test] @@ -568,6 +590,7 @@ mod tests { .unwrap() .expect("repo info"); assert!(info.dirty); + assert!(info.status_dirty); } #[test] @@ -583,6 +606,35 @@ mod tests { .unwrap() .expect("repo info"); assert!(!info.dirty); + assert!(!info.status_dirty); + } + + #[test] + fn get_repo_info_for_scan_dirty_when_assume_unchanged_hides_edit() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + fs::write(root.join("README"), "changed").unwrap(); + git(root, &["update-index", "--assume-unchanged", "README"]); + // status clean; zip would still include the edit + let info = get_repo_info_for_scan(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(info.dirty); + assert!(!info.status_dirty); + } + + #[test] + fn get_repo_info_for_scan_dirty_when_skip_worktree() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + git(root, &["update-index", "--skip-worktree", "README"]); + let info = get_repo_info_for_scan(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!(info.dirty); + assert!(!info.status_dirty); } #[test] @@ -595,16 +647,19 @@ mod tests { .unwrap() .expect("repo info"); assert!(!clean.dirty); + assert!(!clean.status_dirty); fs::write(root.join("README"), "changed").unwrap(); let identity = get_repo_info(root.to_str().unwrap()) .unwrap() .expect("repo info"); assert!(!identity.dirty); + assert!(!identity.status_dirty); let scan = get_repo_info_for_scan(root.to_str().unwrap()) .unwrap() .expect("repo info"); assert!(scan.dirty); + assert!(scan.status_dirty); } fn sample_info(sha: &str, dirty: bool) -> RepoInfo { @@ -613,6 +668,7 @@ mod tests { repo_url: Some("https://github.com/org/repo.git".into()), sha: Some(sha.into()), dirty, + status_dirty: false, } } @@ -669,8 +725,7 @@ mod tests { let parent = parent_dir.path(); init_committed_repo(parent); - // Keep the submodule source outside the parent so it is not an - // untracked sibling that would itself mark the tree dirty. + // Submodule source outside parent so it isn't an untracked sibling. let sub_dir = tempfile::tempdir().unwrap(); let sub_src = sub_dir.path(); git(sub_src, &["init"]); @@ -680,7 +735,6 @@ mod tests { git(sub_src, &["add", "lib.py"]); git(sub_src, &["commit", "-m", "sub init"]); - // Modern git blocks file:// clones unless explicitly allowed. git( parent, &[ @@ -754,21 +808,14 @@ mod tests { #[test] fn default_exclude_globs_match_abs_tmp_but_not_repo_relative_paths() { - // Linux CI tempdirs are `/tmp/...`. Matching DEFAULT_EXCLUDE_GLOBS on the - // absolute path made `--target` drop every file via `**/tmp/**`. + // Abs `/tmp/...` hits `**/tmp/**`; repo-relative paths must not. let mut builder = GlobSetBuilder::new(); for &pattern in DEFAULT_EXCLUDE_GLOBS { builder.add(Glob::new(pattern).unwrap()); } let set = builder.build().unwrap(); - assert!( - set.is_match(Path::new("/tmp/proj/app.py")), - "absolute /tmp paths hit **/tmp/** (the CI failure mode)" - ); - assert!( - !set.is_match(Path::new("app.py")), - "repo-relative paths must stay scannable" - ); + assert!(set.is_match(Path::new("/tmp/proj/app.py"))); + assert!(!set.is_match(Path::new("app.py"))); assert!(!set.is_match(Path::new("src/app.py"))); }