diff --git a/README.md b/README.md index 19c58bd..d075b7d 100644 --- a/README.md +++ b/README.md @@ -73,9 +73,8 @@ Only a scan that answers the same question is reused, which is stricter than branch rather than a pull request, from an explicitly clean worktree, with no scanner problems reported — and this run has to be a default whole-commit scan itself. Anything else runs a real scan: nothing inside the window, only a failed -or still-running scan, a worktree that does not match the commit (including -files the index hides from `git status`), or a lookup the platform could not -answer. +or still-running scan, a worktree `git status` reports changes in, or a lookup +the platform could not answer. Two things are hard errors instead. An unresolvable commit (not a git repository, or no commits yet) exits 1 rather than silently scanning. And a run diff --git a/skills/corgea/SKILL.md b/skills/corgea/SKILL.md index 0adffc1..d675396 100644 --- a/skills/corgea/SKILL.md +++ b/skills/corgea/SKILL.md @@ -86,7 +86,7 @@ An included image is enough on its own: when it is combined with `--only-uncommi `--skip-if-commit-scanned-recently` reuses the project's most recent reusable scan of the current commit instead of starting a duplicate, when one ran inside the `--scanned-within` window (default `24h`; accepts `90s`, `30m`, `4h`, `7d`, and a bare number as hours). The reused scan takes the new scan's place for the rest of the command — results table, `--block-on` gate and its exit code, `--out-file` report — so the pipeline behaves the same either way. It prints `CORGEA_SCAN_SKIPPED=true` plus `CORGEA_SCAN_ID=` on a reuse and `CORGEA_SCAN_SKIPPED=false` when a scan runs, so a later step can branch on it. -Reuse requires a candidate that answers the same question: a completed `corgea-blast` scan of that commit, on a branch rather than a pull request, from an explicitly clean worktree, reporting no scanner problems. Anything else runs a real scan (nothing in the window, a failed or still-running scan, a worktree that does not match the commit including files hidden from `git status`, or a failed lookup). `--ignore-dirty-worktree` (requires `--skip-if-commit-scanned-recently`) overrides the dirty-worktree half of that test: reuse proceeds even if this worktree is dirty or the prior scan recorded `worktree_dirty=true`. A prior scan that never reported the flag is still not reused. A new scan still reports the real dirty status. An unresolvable commit is a hard error (exit 1). Because the API exposes neither a scan's configured scan types and target policies nor whether it bundled a container image, a run that changes what gets scanned cannot be matched against a candidate, so the flag cannot be combined with `--scan-type`, `--policy`, `--include-image`, `--only-uncommitted`, or `--target`. `--exclude` is allowed but warns on a skip: what gets reused is a scan of the whole commit, so the results and the gate can cover files the run would have skipped (over-reporting, never under-reporting). +Reuse requires a candidate that answers the same question: a completed `corgea-blast` scan of that commit, on a branch rather than a pull request, from an explicitly clean worktree, reporting no scanner problems. Anything else runs a real scan (nothing in the window, a failed or still-running scan, a worktree `git status` reports changes in, or a failed lookup). `--ignore-dirty-worktree` (requires `--skip-if-commit-scanned-recently`) overrides the dirty-worktree half of that test: reuse proceeds even if this worktree is dirty or the prior scan recorded `worktree_dirty=true`. A prior scan that never reported the flag is still not reused. A new scan still reports the real dirty status. An unresolvable commit is a hard error (exit 1). Because the API exposes neither a scan's configured scan types and target policies nor whether it bundled a container image, a run that changes what gets scanned cannot be matched against a candidate, so the flag cannot be combined with `--scan-type`, `--policy`, `--include-image`, `--only-uncommitted`, or `--target`. `--exclude` is allowed but warns on a skip: what gets reused is a scan of the whole commit, so the results and the gate can cover files the run would have skipped (over-reporting, never under-reporting). ### Upload — `corgea upload [report]` diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index bbc5dc3..12421b7 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -486,9 +486,10 @@ fn start_new_scan( utils::terminal::set_text_color("", utils::terminal::TerminalColor::Green) ); let repo_after = utils::generic::get_repo_info_for_scan("./").unwrap_or_default(); - // 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); + // Notice = what `git status` shows, from the raw samples (so neither + // --target/--exclude nor SHA drift, which the upload flag also covers). + 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() diff --git a/src/skip_scan.rs b/src/skip_scan.rs index 7ec5ccd..1acc416 100644 --- a/src/skip_scan.rs +++ b/src/skip_scan.rs @@ -125,11 +125,9 @@ pub fn resolve_reusable_scan( exclude: Option<&str>, ignore_dirty_worktree: bool, ) -> Option { - // `dirty`, not `status_dirty`: this asks whether the run would upload an - // exact snapshot of the commit, and that is the flag the upload itself - // sends. `status_dirty` is narrower — it is the user notice, and it cannot - // see assume-unchanged/skip-worktree files, dirty submodules, or an index - // it failed to read, all of which change what gets packaged. + // Dirtiness here is what `git status` reports, the same signal the upload + // sends and the same one the user can check for themselves before asking + // why a scan ran. let commit = utils::generic::get_repo_info_for_scan("./") .ok() .flatten() @@ -151,7 +149,7 @@ pub fn resolve_reusable_scan( ); } else { println!( - "Working tree does not match commit {} exactly (uncommitted changes, or files the index hides from git status), so no scan of that commit describes what would be scanned here - running a new scan.", + "Working tree does not match commit {} exactly (git status reports uncommitted changes), so no scan of that commit describes what would be scanned here - running a new scan.", short ); print_skipped_marker(None); diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 82ab212..86e4c58 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -1,13 +1,38 @@ use crate::utils::terminal::{set_text_color, TerminalColor}; -use git2::{IndexEntryExtendedFlag, IndexEntryFlag, Repository, StatusOptions}; +use git2::{Repository, StatusOptions}; use globset::{Glob, GlobSetBuilder}; use ignore::WalkBuilder; use std::env; use std::fs::{self, File}; use std::io; use std::path::{Path, PathBuf}; +use std::process::Command; use zip::{write::FileOptions, ZipWriter}; +/// Environment variables through which git pins a subprocess to a specific +/// repository, index or config. Git exports them to hooks, so a `corgea` run +/// invoked from one would otherwise read the state the hook was handed instead +/// of the worktree it was pointed at. `deps::run` scrubs the same set for its +/// own git subprocesses; the library and binary crates share no module that +/// could hold one copy. +const GIT_LOCAL_ENV_VARS: &[&str] = &[ + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_CONFIG", + "GIT_CONFIG_PARAMETERS", + "GIT_CONFIG_COUNT", + "GIT_OBJECT_DIRECTORY", + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_IMPLICIT_WORK_TREE", + "GIT_GRAFT_FILE", + "GIT_INDEX_FILE", + "GIT_NO_REPLACE_OBJECTS", + "GIT_REPLACE_REF_BASE", + "GIT_PREFIX", + "GIT_SHALLOW_FILE", + "GIT_COMMON_DIR", +]; + // Global exclude globs used across multiple functions const DEFAULT_EXCLUDE_GLOBS: &[&str] = &[ "**/tests/**", @@ -353,29 +378,46 @@ 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) - }; + let dirty = sample_dirty && worktree_is_dirty(&repo, Path::new(dir)); Ok(Some(RepoInfo { branch, repo_url: origin_url(&repo), sha, dirty, - status_dirty, })) } -/// `(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) +/// True when the worktree holds changes `git status` reports. +/// +/// `git status` is what a user checks this answer against, so git itself is +/// asked and libgit2 only stands in when the git binary cannot answer. The two +/// disagree more often than it looks: libgit2 runs no clean filters (git-lfs +/// and friends), cannot read a sparse index, and knows nothing of a +/// `status.showUntrackedFiles` preference, and each disagreement surfaces as an +/// uncommitted change the user's own `git status` does not show. A status +/// nobody can produce still fails closed to dirty. +fn worktree_is_dirty(repo: &Repository, dir: &Path) -> bool { + git_status_has_changes(dir).unwrap_or_else(|| status_has_changes(repo)) +} + +/// Whether `git status` reports anything, or None when the git binary is +/// missing or the command failed - the only cases libgit2 answers instead. +fn git_status_has_changes(dir: &Path) -> Option { + let mut command = Command::new("git"); + for var in GIT_LOCAL_ENV_VARS { + command.env_remove(var); + } + let output = command + // `--no-optional-locks` keeps this read from writing the user's index. + .args(["--no-optional-locks", "status", "--porcelain"]) + .current_dir(dir) + .output() + .ok()?; + if !output.status.success() { + return None; + } + Some(output.stdout.iter().any(|b| !b.is_ascii_whitespace())) } fn status_has_changes(repo: &Repository) -> bool { @@ -389,19 +431,6 @@ fn status_has_changes(repo: &Repository) -> bool { .unwrap_or(true) } -/// 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( @@ -422,7 +451,6 @@ 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, }) } } @@ -538,8 +566,6 @@ pub struct RepoInfo { pub sha: Option, /// 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)] @@ -564,6 +590,21 @@ mod tests { ); } + /// Dates a file so its recorded stat data no longer matches the index. + #[cfg(unix)] + fn touch(path: &std::path::Path) { + assert!( + Command::new("touch") + .args(["-t", "203001010000"]) + .arg(path) + .status() + .unwrap() + .success(), + "touch {} failed", + path.display() + ); + } + #[test] fn get_repo_info_at_root_only_not_nested_cwd() { let dir = tempfile::tempdir().unwrap(); @@ -607,7 +648,6 @@ mod tests { .unwrap() .expect("repo info"); assert!(info.dirty); - assert!(info.status_dirty); } #[test] @@ -621,7 +661,6 @@ mod tests { .unwrap() .expect("repo info"); assert!(info.dirty); - assert!(info.status_dirty); } #[test] @@ -634,7 +673,6 @@ mod tests { .unwrap() .expect("repo info"); assert!(info.dirty); - assert!(info.status_dirty); } #[test] @@ -650,26 +688,27 @@ mod tests { .unwrap() .expect("repo info"); assert!(!info.dirty); - assert!(!info.status_dirty); } + /// `git status` shows nothing for an assume-unchanged edit, so neither does + /// the CLI: the reported state is the one the user can check. #[test] - fn get_repo_info_for_scan_dirty_when_assume_unchanged_hides_edit() { + fn get_repo_info_for_scan_clean_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); + assert!(!info.dirty); } + /// Same for skip-worktree, which sparse checkouts set on every file they + /// leave out - a whole clean repository would otherwise read as dirty. #[test] - fn get_repo_info_for_scan_dirty_when_skip_worktree() { + fn get_repo_info_for_scan_clean_when_skip_worktree() { let dir = tempfile::tempdir().unwrap(); let root = dir.path(); init_committed_repo(root); @@ -677,8 +716,42 @@ mod tests { let info = get_repo_info_for_scan(root.to_str().unwrap()) .unwrap() .expect("repo info"); - assert!(info.dirty); - assert!(!info.status_dirty); + assert!(!info.dirty); + } + + /// A clean filter (how git-lfs and similar tools store a file) makes the + /// worktree copy differ from the stored blob on purpose. `git status` + /// applies the filter and reports nothing; libgit2 cannot run it and reads + /// every such file as modified, which is why git is the one asked. + #[cfg(unix)] + #[test] + fn get_repo_info_for_scan_clean_when_a_clean_filter_rewrites_the_blob() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + git(root, &["config", "filter.upper.clean", "tr a-z A-Z"]); + git(root, &["config", "filter.upper.smudge", "cat"]); + fs::write(root.join(".gitattributes"), "*.txt filter=upper\n").unwrap(); + fs::write(root.join("payload.txt"), "lowercase\n").unwrap(); + git(root, &["add", ".gitattributes", "payload.txt"]); + git(root, &["commit", "-m", "filtered"]); + // Stale timestamps are how a checkout out of a CI cache looks. Both + // implementations stop trusting the index's stat cache and compare + // content; only git runs the filter while doing it. + touch(&root.join("payload.txt")); + + let repo = Repository::discover(root).unwrap(); + assert!( + status_has_changes(&repo), + "libgit2 runs no clean filter, so it should read the file as modified" + ); + let info = get_repo_info_for_scan(root.to_str().unwrap()) + .unwrap() + .expect("repo info"); + assert!( + !info.dirty, + "git status is empty, so the scan must not report a dirty worktree" + ); } #[test] @@ -691,19 +764,30 @@ 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); + } + + /// The libgit2 fallback answers only when the git binary cannot, so it is + /// exercised directly. + #[test] + fn libgit2_fallback_sees_a_modified_tracked_file() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + init_committed_repo(root); + let repo = Repository::discover(root).unwrap(); + assert!(!status_has_changes(&repo)); + + fs::write(root.join("README"), "changed").unwrap(); + assert!(status_has_changes(&repo)); } fn sample_info(sha: &str, dirty: bool) -> RepoInfo { @@ -712,7 +796,6 @@ mod tests { repo_url: Some("https://github.com/org/repo.git".into()), sha: Some(sha.into()), dirty, - status_dirty: false, } } diff --git a/tests/cloud_commands_e2e/scan_list.rs b/tests/cloud_commands_e2e/scan_list.rs index d715a16..714f7aa 100644 --- a/tests/cloud_commands_e2e/scan_list.rs +++ b/tests/cloud_commands_e2e/scan_list.rs @@ -99,6 +99,65 @@ fn scan_dirty_worktree_sends_dirty_true_and_prints_notice() { ); } +/// A worktree `git status` calls clean is scanned as clean, whatever libgit2 +/// makes of it. A clean filter (how git-lfs and similar tools store a file) is +/// one way a checkout gets there: the worktree bytes differ from the stored +/// blob by design, and libgit2 runs no filters, so it reads every such file as +/// modified. +#[cfg(unix)] +#[test] +fn scan_clean_by_git_status_sends_dirty_false_without_worktree_notice() { + let project = git_project(); + run_git( + project.path(), + &["config", "filter.upper.clean", "tr a-z A-Z"], + ); + run_git(project.path(), &["config", "filter.upper.smudge", "cat"]); + std::fs::write( + project.path().join(".gitattributes"), + "*.txt filter=upper\n", + ) + .expect("write attributes"); + std::fs::write(project.path().join("payload.txt"), "lowercase\n").expect("write payload"); + run_git(project.path(), &["add", ".gitattributes", "payload.txt"]); + run_git(project.path(), &["commit", "-m", "filtered"]); + // A checkout restored from a CI cache carries stale timestamps, so the + // index's stat cache stops answering and the content is compared instead. + assert!( + std::process::Command::new("touch") + .args(["-t", "203001010000"]) + .arg(project.path().join("payload.txt")) + .status() + .expect("run touch") + .success(), + "touch failed" + ); + let status = run_git(project.path(), &["status", "--porcelain"]); + assert!( + status.stdout.is_empty(), + "fixture must be clean to git: {}", + String::from_utf8_lossy(&status.stdout) + ); + let sha = String::from_utf8(run_git(project.path(), &["rev-parse", "HEAD"]).stdout) + .expect("UTF-8 Git SHA") + .trim() + .to_string(); + + let scan_api = ApiStub::start(blast_upload_plan(&sha, false, 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("Working tree has uncommitted changes"), + "a tree git status calls clean must not print the dirty notice\n{scan_context}" + ); +} + #[test] fn scan_clean_target_upload_sends_dirty_true_without_worktree_notice() { let project = git_project(); diff --git a/tests/cloud_commands_e2e/scan_skip.rs b/tests/cloud_commands_e2e/scan_skip.rs index 0335a51..f02183b 100644 --- a/tests/cloud_commands_e2e/scan_skip.rs +++ b/tests/cloud_commands_e2e/scan_skip.rs @@ -290,13 +290,13 @@ fn a_dirty_worktree_scans_instead_of_reusing_the_commits_scan() { assert!(stdout.contains("CORGEA_SCAN_SKIPPED=false"), "{context}"); } -/// The reuse decision has to read the same dirtiness signal the upload sends. -/// An assume-unchanged modified file is invisible to `git status` — so no -/// worktree notice is printed — but it still changes what gets packaged, and the -/// upload marks it dirty. Reading the narrower status signal here would reuse a -/// clean scan of the commit and gate on files this run does not contain. +/// Dirtiness is whatever `git status` reports, and nothing else: an +/// assume-unchanged edit is invisible to it (as are the sparse-checkout and +/// clean-filter setups that put whole repositories in this state), so the tree +/// counts as clean and the commit's scan is reused. Anything stricter here +/// scans on a dirtiness the user cannot see in their own `git status`. #[test] -fn a_file_hidden_from_git_status_scans_instead_of_reusing() { +fn a_file_hidden_from_git_status_reuses_the_commits_scan() { let project = git_project(); run_git( project.path(), @@ -304,12 +304,20 @@ fn a_file_hidden_from_git_status_scans_instead_of_reusing() { ); std::fs::write(project.path().join("main.py"), "print('hidden change')\n") .expect("modify assume-unchanged file"); - let api = ApiStub::start(blast_upload_plan(&project.sha, true, false)); + let api = ApiStub::start(vec![ + verify_request(), + commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), + clean_detail(&project.sha), + reused_scan_issues(), + reused_scan_blocking_rules(false), + ]); let (mut command, _home) = cloud_command(&api, project.path()); command.args([ "scan", "blast", "--skip-if-commit-scanned-recently", + "--block-on", + "criticals", "--project-name", PROJECT, ]); @@ -320,13 +328,12 @@ fn a_file_hidden_from_git_status_scans_instead_of_reusing() { let stdout = String::from_utf8_lossy(&output.stdout); assert_eq!(output.status.code(), Some(0), "{context}"); + assert!(stdout.contains("CORGEA_SCAN_SKIPPED=true"), "{context}"); + assert!(!stdout.contains("Scanning with BLAST"), "{context}"); assert!( - stdout.contains("Working tree does not match commit"), + !stdout.contains("Working tree does not match commit"), "{context}" ); - assert!(stdout.contains("CORGEA_SCAN_SKIPPED=false"), "{context}"); - // `git status` sees nothing, so the user-facing worktree notice stays quiet; - // only the reuse decision and the upload's dirty flag react. assert!( !stdout.contains("Working tree has uncommitted changes"), "{context}" @@ -377,49 +384,6 @@ fn ignore_dirty_worktree_reuses_a_scan_a_dirty_tree_would_otherwise_run() { ); } -/// Same override for index hide-bits: assume-unchanged is invisible to -/// `git status` but still blocks reuse unless ignored. -#[test] -fn ignore_dirty_worktree_reuses_when_a_file_is_hidden_from_git_status() { - let project = git_project(); - run_git( - project.path(), - &["update-index", "--assume-unchanged", "main.py"], - ); - std::fs::write(project.path().join("main.py"), "print('hidden change')\n") - .expect("modify assume-unchanged file"); - let api = ApiStub::start(vec![ - verify_request(), - commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), - clean_detail(&project.sha), - reused_scan_issues(), - reused_scan_blocking_rules(false), - ]); - let (mut command, _home) = cloud_command(&api, project.path()); - command.args([ - "scan", - "blast", - "--skip-if-commit-scanned-recently", - "--ignore-dirty-worktree", - "--block-on", - "criticals", - "--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("CORGEA_SCAN_SKIPPED=true"), "{context}"); - assert!( - stdout.contains("Ignoring dirty worktree (--ignore-dirty-worktree)"), - "{context}" - ); -} - /// A prior scan that itself recorded `worktree_dirty` is also reusable when /// the override is on — the customer case where the last scan of the commit /// was marked dirty even though they consider the tree clean.