diff --git a/docs/architecture.md b/docs/architecture.md index 0218c84..b8830be 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -169,11 +169,16 @@ pub struct Repository { impl Repository { pub fn resolve_target(&self, config: &Config) -> Result; pub fn resolve_bases(&self, config: &Config) -> Result>; + pub fn resolve_diff_bases(&self, target: &ResolvedRef, bases: &[ResolvedRef], quiet: bool) -> Vec; pub fn generate_diffs(&self, target: &ResolvedRef, bases: &[ResolvedRef], quiet: bool) -> Result>; pub fn commit_patch(&self, commit_id: &str) -> Result; } ``` +Artifact diffs use `resolve_diff_bases()` before `generate_diffs()`, so the +review pack matches GitHub's three-dot "Files changed" model: branch names stay +displayable as bases, while each diff is anchored at the target/base merge-base. + Advantages of git2 over the `git` CLI: - much faster for batch operations - no subprocess spawning @@ -451,8 +456,8 @@ impl Cache { } // Key generation -pub fn rust_hash(repo_root: &PathBuf) -> String { - // git_head_short + cargo_files_hash + src_files_hash +pub fn rust_hash(root: &Path) -> String { + // Cargo.toml/Cargo.lock hash + Rust source hash, 16-byte digest segments } ``` diff --git a/src/artifacts/context.rs b/src/artifacts/context.rs index 428c0e5..fb6fe52 100644 --- a/src/artifacts/context.rs +++ b/src/artifacts/context.rs @@ -165,10 +165,14 @@ pub(crate) fn build_dashboard_context(input: DashboardContextInput<'_>) -> Dashb }; let blocking = config.policy.is_blocking(severity, status_class); if blocking { - blocking_issues.push(format!( - "Loctree heuristics (dead_exports={}, cycles={})", - dead, cycles - )); + record_blocking_issue( + &mut blocking_issues, + &mut worst_merge, + format!( + "Loctree heuristics (dead_exports={}, cycles={})", + dead, cycles + ), + ); } let name = if let Some(h) = heuristics { format!( @@ -190,12 +194,13 @@ pub(crate) fn build_dashboard_context(input: DashboardContextInput<'_>) -> Dashb // Inline findings blocking — THREAD 7: gate on introduced/unclassified // findings, not the raw error count, so the dashboard agrees with the merge // gate and a pre-existing-only scan does not block. - let inline_severity = config.policy.severity_for("inline_findings"); - let inline_class = effective_inline_gate_class(inline, &clean_comparison); - let inline_blocking = config.policy.is_blocking(inline_severity, inline_class); - if inline_blocking { - blocking_issues.push(format!("INLINE_FINDINGS ({})", inline.status)); - } + apply_inline_gate_outcome( + config, + inline, + &clean_comparison, + &mut blocking_issues, + &mut worst_merge, + ); let policy_allow_merge = blocking_issues.is_empty(); diff --git a/src/artifacts/findings.rs b/src/artifacts/findings.rs index b7d6a11..5c29f64 100644 --- a/src/artifacts/findings.rs +++ b/src/artifacts/findings.rs @@ -65,6 +65,40 @@ pub(super) fn effective_inline_gate_class( } } +pub(super) struct InlineGateOutcome { + pub severity: crate::policy::PolicySeverity, + pub blocking: bool, +} + +pub(super) fn record_blocking_issue( + blocking_issues: &mut Vec, + worst_merge: &mut crate::policy::engine::MergeRecommendation, + issue: impl Into, +) { + blocking_issues.push(issue.into()); + *worst_merge = crate::policy::engine::MergeRecommendation::Block; +} + +pub(super) fn apply_inline_gate_outcome( + config: &Config, + inline: &InlineFindingsSummary, + clean: &CleanComparison, + blocking_issues: &mut Vec, + worst_merge: &mut crate::policy::engine::MergeRecommendation, +) -> InlineGateOutcome { + let severity = config.policy.severity_for("inline_findings"); + let class = effective_inline_gate_class(inline, clean); + let blocking = config.policy.is_blocking(severity, class); + if blocking { + record_blocking_issue( + blocking_issues, + worst_merge, + format!("INLINE_FINDINGS ({})", inline.status), + ); + } + InlineGateOutcome { severity, blocking } +} + pub(super) fn gate_class_for_check(status: crate::checks::CheckStatus) -> GateClass { match status { crate::checks::CheckStatus::Passed => GateClass::Pass, diff --git a/src/artifacts/merge_gate.rs b/src/artifacts/merge_gate.rs index 3ddec96..55296ff 100644 --- a/src/artifacts/merge_gate.rs +++ b/src/artifacts/merge_gate.rs @@ -104,21 +104,22 @@ pub(super) fn generate_merge_gate(input: MergeGateInput<'_>) -> Result<()> { if !has_heuristics { let (heuristics_check, heuristics_issue) = build_heuristics_gate_check(config, heuristics); if let Some(issue) = heuristics_issue { - blocking_issues.push(issue); - worst_merge = MergeRecommendation::Block; + record_blocking_issue(&mut blocking_issues, &mut worst_merge, issue); } gate_checks.push(heuristics_check); } - let inline_severity = config.policy.severity_for("inline_findings"); // THREAD 7: gate on introduced/unclassified findings, not the raw error // count — a scan with only pre-existing errors must not block the merge. - let inline_class = effective_inline_gate_class(inline, &clean_comparison); - let inline_blocking = config.policy.is_blocking(inline_severity, inline_class); - if inline_blocking { - blocking_issues.push(format!("INLINE_FINDINGS ({})", inline.status)); - worst_merge = MergeRecommendation::Block; - } + let inline_gate = apply_inline_gate_outcome( + config, + inline, + &clean_comparison, + &mut blocking_issues, + &mut worst_merge, + ); + let inline_severity = inline_gate.severity; + let inline_blocking = inline_gate.blocking; let policy_allow_merge = blocking_issues.is_empty(); @@ -704,6 +705,75 @@ mod tests { .expect("gate check") } + #[test] + fn artifact_consistency_inline_blocking_verdict_matches_report_and_gate() { + use crate::policy::engine::MergeRecommendation; + + let tmp = tempfile::tempdir().expect("tempdir"); + let mut config = test_config(); + config.policy.checks.insert( + "inline_findings".to_string(), + crate::policy::PolicySeverity::Block, + ); + let inline = InlineFindingsSummary { + status: "failed".to_string(), + findings_count: 1, + dashboard_findings: vec![DashboardFinding { + level: "error", + check_name: "Semgrep scan".to_string(), + check_id: "semgrep_scan".to_string(), + message: "introduced finding".to_string(), + in_diff: Some(true), + }], + }; + let coverage = empty_coverage(); + let (resolved_target, resolved_bases) = resolved_refs(); + + generate_merge_gate(MergeGateInput { + dir: tmp.path(), + config: &config, + checks: &[], + heuristics: None, + inline: &inline, + breaking: &[], + coverage: &coverage, + diffs: &[], + skipped_checks: &[], + resolved_target: &resolved_target, + resolved_bases: &resolved_bases, + clean_comparison: CleanComparison::for_test(true, true), + }) + .expect("merge gate"); + + let raw = + std::fs::read_to_string(tmp.path().join("MERGE_GATE.json")).expect("read gate json"); + let gate: serde_json::Value = serde_json::from_str(&raw).expect("parse gate json"); + let dashboard = build_dashboard_context(DashboardContextInput { + config: &config, + checks: &[], + heuristics: None, + inline: &inline, + breaking: Vec::new(), + coverage, + diff_dir: tmp.path(), + skipped_checks: Vec::new(), + out_dir: tmp.path(), + diffs: &[], + ownership_map: Vec::new(), + clean_comparison: CleanComparison::for_test(true, true), + }); + + assert_eq!( + gate["decision"]["merge_recommendation"].as_str(), + Some("block") + ); + assert_eq!(dashboard.merge_recommendation, MergeRecommendation::Block); + assert_eq!( + dashboard.verdict, + gate["decision"]["verdict"].as_str().expect("gate verdict") + ); + } + #[test] fn effective_outcome_is_the_single_shared_verdict_source() { use crate::policy::engine::{MergeRecommendation, PolicyEngine}; diff --git a/src/artifacts/report.rs b/src/artifacts/report.rs index c9b71f7..9b38fcb 100644 --- a/src/artifacts/report.rs +++ b/src/artifacts/report.rs @@ -588,6 +588,11 @@ fn build_report(input: &ReportInput<'_>) -> Report { .. } = input; + let diff_merge_base = diffs + .first() + .map(|diff| diff.base_commit_id.clone()) + .or_else(|| resolved_bases.first().map(|base| base.commit_id.clone())); + // -- meta -- let meta = Meta { generated_at: run_started_at.to_string(), @@ -605,7 +610,7 @@ fn build_report(input: &ReportInput<'_>) -> Report { range: RangeMeta { base: resolved_bases.first().map(|b| b.name.clone()), head: resolved_target.name.clone(), - merge_base: resolved_bases.first().map(|b| b.commit_id.clone()), + merge_base: diff_merge_base, }, }; diff --git a/src/artifacts/signal/coverage.rs b/src/artifacts/signal/coverage.rs index dbc8852..3f8cc84 100644 --- a/src/artifacts/signal/coverage.rs +++ b/src/artifacts/signal/coverage.rs @@ -496,7 +496,7 @@ fn find_matching_test<'a>( .and_then(|p| p.to_str()) .unwrap_or(""); - // Strategy 1: Exact stem match (strip test prefix/suffix) → High + // Strategy 1: Exact stem match (strip test prefix/suffix) for test in test_files { let test_stem = Path::new(&test.path) .file_stem() @@ -511,11 +511,16 @@ fn find_matching_test<'a>( .unwrap_or(test_stem); if test_base == src_stem { - return Some((test, CoverageMatchTier::High)); + let tier = if same_coverage_module(&source.path, &test.path) { + CoverageMatchTier::High + } else { + CoverageMatchTier::Low + }; + return Some((test, tier)); } } - // Strategy 2: Path-mirrored (tests/foo/bar.rs <-> src/foo/bar.rs) → High + // Strategy 2: Path-mirrored (tests/foo/bar.rs <-> src/foo/bar.rs) for test in test_files { if test.path.contains("tests/") || test.path.contains("__tests__/") { let test_filename = Path::new(&test.path) @@ -523,7 +528,12 @@ fn find_matching_test<'a>( .and_then(|s| s.to_str()) .unwrap_or(""); if test_filename == src_stem { - return Some((test, CoverageMatchTier::High)); + let tier = if same_coverage_module(&source.path, &test.path) { + CoverageMatchTier::High + } else { + CoverageMatchTier::Low + }; + return Some((test, tier)); } } } @@ -601,6 +611,37 @@ fn find_matching_test<'a>( None } +fn same_coverage_module(source_path: &str, test_path: &str) -> bool { + coverage_module_key(source_path) == coverage_module_key(test_path) +} + +fn coverage_module_key(path: &str) -> Vec { + let mut components = Path::new(path) + .components() + .filter_map(|component| component.as_os_str().to_str()) + .filter(|component| !component.is_empty()) + .map(|component| component.to_string()) + .collect::>(); + + if components + .first() + .is_some_and(|component| matches!(component.as_str(), "src" | "tests" | "__tests__")) + { + components.remove(0); + } + + components.pop(); + + while components + .last() + .is_some_and(|component| matches!(component.as_str(), "tests" | "__tests__")) + { + components.pop(); + } + + components +} + /// Strategy 5 (import recovery): find a test file that imports the source module. /// /// Only called for source files that had no filename-heuristic match. @@ -823,6 +864,27 @@ mod tests { assert_eq!(signal.confidence, "high"); } + #[test] + fn artifact_consistency_stem_match_across_different_modules_is_low_confidence() { + let diff = mock_diff(vec![ + mock_file_change("src/a/util.rs", FileStatus::Modified, 10, 5), + mock_file_change("tests/b/util_test.rs", FileStatus::Modified, 5, 2), + ]); + + let signal = compute_coverage_signal(&[diff], None, None); + + assert_eq!(signal.covered_count, 1); + assert_eq!( + signal.covered_files[0], + ( + "src/a/util.rs".to_string(), + "tests/b/util_test.rs".to_string(), + CoverageMatchTier::Low, + ) + ); + assert_eq!(signal.confidence, "medium"); + } + #[test] fn coverage_delta_excludes_non_code_files_from_source_count() { let diff = mock_diff(vec![ diff --git a/src/artifacts/signal/semantic.rs b/src/artifacts/signal/semantic.rs index 6a2d986..63ea977 100644 --- a/src/artifacts/signal/semantic.rs +++ b/src/artifacts/signal/semantic.rs @@ -138,7 +138,7 @@ pub fn detect_orphaned_resource_delete(diffs: &[Diff]) -> Vec { // in its name, it's a candidate for orphaned resource detection if is_service_file { for indicator in DELETE_INDICATORS { - if lower.contains(indicator) { + if contains_identifier_token(&lower, indicator) { delete_evidence.push(( file.path.clone(), 0, @@ -207,6 +207,22 @@ pub fn detect_orphaned_resource_delete(diffs: &[Diff]) -> Vec { findings } +fn contains_identifier_token(haystack: &str, needle: &str) -> bool { + if needle.is_empty() { + return false; + } + + haystack.match_indices(needle).any(|(start, _)| { + let before = haystack[..start].chars().next_back(); + let after = haystack[start + needle.len()..].chars().next(); + is_identifier_token_boundary(before) && is_identifier_token_boundary(after) + }) +} + +fn is_identifier_token_boundary(ch: Option) -> bool { + ch.is_none_or(|ch| !ch.is_ascii_alphanumeric()) +} + #[cfg(test)] mod tests { use super::*; @@ -241,6 +257,23 @@ mod tests { assert!(!findings[0].evidence.is_empty()); } + #[test] + fn artifact_consistency_dropdown_filename_does_not_match_drop_indicator() { + let diff = mock_diff(vec![ + mock_file_change( + "src/services/dropdown_service.rs", + FileStatus::Modified, + 20, + 5, + ), + mock_file_change("src/models/menu_model.rs", FileStatus::Modified, 10, 2), + ]); + + let findings = detect_orphaned_resource_delete(&[diff]); + + assert!(findings.is_empty(), "dropdown must not trigger drop"); + } + #[test] fn no_finding_when_cleanup_present() { let diff = mock_diff(vec![ diff --git a/src/artifacts/tests.rs b/src/artifacts/tests.rs index 7f593f7..d57fa31 100644 --- a/src/artifacts/tests.rs +++ b/src/artifacts/tests.rs @@ -100,8 +100,10 @@ fn create_zip_rejects_pack_missing_metadata() { use crate::artifacts::signal::{BreakingKind, BreakingRisk}; use crate::checks::{CheckProvenance, CheckStatus}; use crate::cli::ExecutionMode; -use crate::config::{test_config_builder, test_js_profile, test_rust_profile}; -use crate::git::{CommitInfo, DiffStats, FileChange, FileStatus, ResolvedRef}; +use crate::config::{ + test_config_builder, test_generic_profile, test_js_profile, test_rust_profile, +}; +use crate::git::{CommitInfo, DiffStats, FileChange, FileStatus, Repository, ResolvedRef, git_cmd}; use crate::policy::{PolicyConfig, PolicyMode, PolicySeverity}; use std::time::Duration; @@ -159,6 +161,134 @@ fn create_test_config(policy: PolicyConfig) -> Config { .build() } +fn run_git_fixture(repo: &Path, args: &[&str]) { + let status = git_cmd() + .args(args) + .current_dir(repo) + .status() + .expect("git command"); + assert!(status.success(), "git {args:?} failed with {status}"); +} + +fn write_commit_fixture(repo: &Path, name: &str, body: &str) -> String { + fs::write(repo.join(name), body).expect("write fixture"); + run_git_fixture(repo, &["add", name]); + run_git_fixture( + repo, + &[ + "-c", + "user.name=prview test", + "-c", + "user.email=prview@example.test", + "commit", + "-m", + name, + ], + ); + let output = git_cmd() + .args(["rev-parse", "HEAD"]) + .current_dir(repo) + .output() + .expect("rev-parse"); + assert!(output.status.success()); + String::from_utf8(output.stdout) + .expect("utf8 rev-parse") + .trim() + .to_string() +} + +fn init_advanced_base_fixture() -> (tempfile::TempDir, String, String) { + let tmp = tempfile::tempdir().expect("tempdir"); + run_git_fixture(tmp.path(), &["init", "-q", "-b", "main"]); + let merge_base = write_commit_fixture(tmp.path(), "own.rs", "pub fn own() -> u8 { 1 }\n"); + run_git_fixture(tmp.path(), &["checkout", "-q", "-b", "feature"]); + let target = write_commit_fixture(tmp.path(), "own.rs", "pub fn own() -> u8 { 2 }\n"); + run_git_fixture(tmp.path(), &["checkout", "-q", "main"]); + let _advance_one = write_commit_fixture( + tmp.path(), + "unrelated.rs", + "pub fn unrelated_one() -> u8 { 1 }\n", + ); + let _advance_two = write_commit_fixture( + tmp.path(), + "unrelated.rs", + "pub fn unrelated_one() -> u8 { 1 }\npub fn unrelated_two() -> u8 { 2 }\n", + ); + run_git_fixture(tmp.path(), &["checkout", "-q", "feature"]); + (tmp, merge_base, target) +} + +#[tokio::test] +async fn artifact_pipeline_diffs_from_merge_base_when_base_advanced() { + let (repo_tmp, merge_base, _target) = init_advanced_base_fixture(); + let output_tmp = tempfile::tempdir().expect("output tempdir"); + let output_dir = output_tmp.path().join("pack"); + + let mut config = test_config_builder() + .repo_root(repo_tmp.path()) + .target(Some("feature")) + .bases(&["main"]) + .profile(test_generic_profile()) + .execution_mode(ExecutionMode::Standard) + .run_tests(false) + .run_lint(false) + .do_fetch(false) + .use_cache(false) + .create_zip(false) + .build(); + config.run_bundle = false; + config.run_security = false; + config.run_heuristics = false; + config.create_dashboard = false; + config.quiet = true; + config.output_dir = Some(output_dir.clone()); + + let app = crate::App::from_config(config).expect("app"); + let report = app.run().await.expect("run prview"); + assert_eq!(report.artifacts_dir, output_dir); + + let full_patch = + fs::read_to_string(output_dir.join("10_diff/full.patch")).expect("read full.patch"); + assert!( + full_patch.contains("own.rs"), + "target-owned change must be present:\n{full_patch}" + ); + assert!( + !full_patch.contains("unrelated.rs"), + "advanced base-only file must not leak into three-dot diff:\n{full_patch}" + ); + + let raw_report = fs::read_to_string(output_dir.join("report.json")).expect("read report.json"); + let report_json: serde_json::Value = + serde_json::from_str(&raw_report).expect("parse report.json"); + assert_eq!( + report_json["meta"]["range"]["merge_base"].as_str(), + Some(merge_base.as_str()) + ); + assert_eq!(report_json["meta"]["range"]["base"].as_str(), Some("main")); + assert_eq!( + report_json["meta"]["range"]["head"].as_str(), + Some("feature") + ); + assert_eq!( + report_json["diff"]["stats"]["files_changed"].as_u64(), + Some(1) + ); + + let repo = Repository::open(repo_tmp.path()).expect("open repo"); + let resolved_target = repo + .resolve_target(&app.config) + .expect("resolve target after run"); + let resolved_bases = repo + .resolve_bases(&app.config) + .expect("resolve bases after run"); + let diff_bases = repo.resolve_diff_bases(&resolved_target, &resolved_bases, true); + assert_eq!( + diff_bases.first().map(|base| base.commit_id.as_str()), + Some(merge_base.as_str()) + ); +} + fn sample_cargo_audit_output() -> String { r#"{ "vulnerabilities": { diff --git a/src/artifacts/verdict.rs b/src/artifacts/verdict.rs index 8c220f5..d58b34a 100644 --- a/src/artifacts/verdict.rs +++ b/src/artifacts/verdict.rs @@ -682,12 +682,16 @@ fn has_resolvable_base_diff( /// Whether a check materialises and scans an ephemeral snapshot of the analysed /// *target* when that target is a fetched remote ref not checked out locally. /// -/// Only `semgrep_scan` does this (R2-10): it builds a detached worktree at the -/// target commit and scans that clean tree, so its out-of-diff findings genuinely -/// predate the target diff. Every other baseline-signal check scans the local -/// checkout, which in a `--pr`/`--remote` run is a different tree than the target. +/// `semgrep_scan` builds its own detached worktree at the target commit (R2-10). +/// Since A2, the other file-scoped linters (`ruff`, `eslint`, `stylelint`) run +/// through `plan_check_run`, which materialises a worktree snapshot of the target +/// and scans there in `--pr`/`--remote` mode — so their out-of-diff findings also +/// genuinely predate the target diff and may be downgraded. `rustfmt` and +/// `cargo_audit` are deliberately excluded: they run at `cargo_cache_root` (the +/// local checkout), a *different* tree than the target, so their out-of-diff rows +/// prove nothing about the target diff and must NOT be downgraded (R3-16). fn check_scans_target_snapshot(check_id: &str) -> bool { - matches!(check_id, "semgrep_scan") + matches!(check_id, "semgrep_scan" | "ruff" | "eslint" | "stylelint") } /// Capture whether the working tree at `repo_root` is clean, for freezing the @@ -1212,21 +1216,36 @@ mod tests { #[test] fn remote_target_downgrades_only_snapshot_scanned_checks() { - // R3-16: on a remote/snapshot target (head != target) only semgrep scanned - // the target snapshot. rustfmt scanned the local checkout — a different - // tree — so its out-of-diff rows must NOT be downgraded to pre-existing, - // while semgrep's still are. + // R3-16: on a remote/snapshot target (head != target) the snapshot-backed + // checks (semgrep + the plan_check_run linters ruff/eslint/stylelint) + // scanned the target snapshot, so their out-of-diff rows may downgrade. + // rustfmt/cargo_audit scanned the local checkout — a different tree — so + // their out-of-diff rows must NOT be downgraded to pre-existing. let clean = CleanComparison::for_test(false, true); assert!( clean.applies_to("semgrep_scan"), "semgrep scans the target snapshot, downgrade applies" ); + assert!( + clean.applies_to("ruff"), + "ruff scans the target snapshot via plan_check_run, downgrade applies" + ); + assert!( + clean.applies_to("eslint"), + "eslint scans the target snapshot via plan_check_run, downgrade applies" + ); + assert!( + clean.applies_to("stylelint"), + "stylelint scans the target snapshot via plan_check_run, downgrade applies" + ); assert!( !clean.applies_to("rustfmt"), "rustfmt scanned the local checkout, downgrade must not apply" ); - assert!(!clean.applies_to("ruff")); - assert!(!clean.applies_to("eslint")); + assert!( + !clean.applies_to("cargo_audit"), + "cargo_audit scanned the local checkout, downgrade must not apply" + ); let findings = [out_of_diff_finding("rustfmt")]; let summary = build_quality_failure_summary( diff --git a/src/cache/mod.rs b/src/cache/mod.rs index 721f150..2090d8f 100644 --- a/src/cache/mod.rs +++ b/src/cache/mod.rs @@ -158,11 +158,19 @@ pub fn python_hash(repo_root: &Path) -> String { fn hash_files(repo_root: &Path, patterns: &[&str]) -> String { let mut hasher = Sha256::new(); + // Escape glob metacharacters in the repo root so a path like `repo[old]` is + // matched literally, not parsed as a glob pattern. `glob::Pattern::escape` + // brackets exactly the chars glob treats as special (`? * [ ]`); braces are + // literal to this crate (no brace expansion), so no extra handling is needed. + let escaped_root = glob::Pattern::escape(&repo_root.display().to_string()); for pattern in patterns { - if let Ok(entries) = glob::glob(&format!("{}/{}", repo_root.display(), pattern)) { - for entry in entries.filter_map(|e| e.ok()) { - if let Ok(content) = fs::read(&entry) { + if let Ok(entries) = glob::glob(&format!("{escaped_root}/{pattern}")) { + let mut paths: Vec<_> = entries.filter_map(|entry| entry.ok()).collect(); + paths.sort(); + + for path in paths { + if let Ok(content) = fs::read(&path) { hasher.update(&content); } } @@ -170,7 +178,7 @@ fn hash_files(repo_root: &Path, patterns: &[&str]) -> String { } let result = hasher.finalize(); - hex::encode(&result[..4]) // First 8 chars + hex::encode(&result[..16]) } #[cfg(test)] @@ -324,6 +332,18 @@ mod tests { assert_eq!(parts.len(), 2); } + #[test] + fn test_hash_functions_use_16_byte_digest_segments() { + let temp_dir = TempDir::new().unwrap(); + let ts_hash = ts_hash(temp_dir.path()); + assert_eq!(ts_hash.len(), 32); + + let rust_hash = rust_hash(temp_dir.path()); + let parts: Vec<_> = rust_hash.split('-').collect(); + assert_eq!(parts.len(), 2); + assert!(parts.iter().all(|part| part.len() == 32)); + } + #[test] fn test_python_hash_format() { let temp_dir = TempDir::new().unwrap(); @@ -394,6 +414,33 @@ mod tests { assert!(!hash2.is_empty()); } + #[test] + fn test_hash_files_escapes_repo_root_glob_metacharacters() { + let temp_dir = tempfile::Builder::new() + .prefix("repo[old]") + .tempdir() + .unwrap(); + + fs::write( + temp_dir.path().join("Cargo.toml"), + "[package]\nname = \"demo\"\n", + ) + .unwrap(); + let first = rust_hash(temp_dir.path()); + + fs::write( + temp_dir.path().join("Cargo.toml"), + "[package]\nname = \"demo\"\nversion = \"0.1.0\"\n", + ) + .unwrap(); + let second = rust_hash(temp_dir.path()); + + assert_ne!( + first, second, + "repo roots with glob metacharacters must still hash matched files" + ); + } + fn init_git_repo_with_commit() -> TempDir { let temp_dir = TempDir::new().unwrap(); git_cmd() diff --git a/src/checks/cargo.rs b/src/checks/cargo.rs index 841a2c5..9d75094 100644 --- a/src/checks/cargo.rs +++ b/src/checks/cargo.rs @@ -18,6 +18,37 @@ pub struct RustfmtCheck; pub struct CargoAuditCheck; pub struct CargoGeigerCheck; +fn cargo_cache_root(config: &Config) -> &Path { + config + .profile + .cargo_root + .as_deref() + .unwrap_or(config.repo_root.as_path()) +} + +/// Content hash for dependency-sensitive cargo checks (check/clippy/geiger). +/// +/// `rust_hash` keys on files under `cargo_cache_root`. When that root is a +/// workspace member distinct from the repo root, Cargo still resolves the +/// dependency set from the workspace-root `Cargo.lock` — and a member usually +/// has no lockfile of its own. Hashing only member files therefore lets a +/// root-lockfile-only dependency bump reuse a stale cached result. Fold the +/// repo-root lockfile in whenever the cargo root differs from the repo root so +/// such a bump invalidates the member key. +fn cargo_content_hash(config: &Config) -> String { + let cargo_root = cargo_cache_root(config); + let base = cache::rust_hash(cargo_root); + if cargo_root == config.repo_root.as_path() { + base + } else { + format!( + "{}-root:{}", + base, + cache::cargo_lock_hash(&config.repo_root) + ) + } +} + #[async_trait] impl Check for CargoCheck { fn name(&self) -> &str { @@ -35,18 +66,14 @@ impl Check for CargoCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(cache::rust_hash(&config.repo_root)) + Some(cargo_content_hash(config)) } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); - let cwd = config - .profile - .cargo_root - .as_ref() - .unwrap_or(&config.repo_root); + let cwd = cargo_cache_root(config); let args = &["check", "--message-format=short"]; let output = run_command("cargo", args, cwd).await?; @@ -111,18 +138,14 @@ impl Check for ClippyCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!("clippy-{}", cache::rust_hash(&config.repo_root))) + Some(format!("clippy-{}", cargo_content_hash(config))) } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); - let cwd = config - .profile - .cargo_root - .as_ref() - .unwrap_or(&config.repo_root); + let cwd = cargo_cache_root(config); let args = &["clippy", "--message-format=short", "--", "-D", "warnings"]; let output = run_command("cargo", args, cwd).await?; @@ -227,11 +250,7 @@ impl Check for CargoTestCheck { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); - let cwd = config - .profile - .cargo_root - .as_ref() - .unwrap_or(&config.repo_root); + let cwd = cargo_cache_root(config); let args = &["test", "--all-targets", "--no-fail-fast"]; let output = run_command_with_timeout("cargo", args, cwd, TEST_TIMEOUT_SECS).await?; @@ -296,18 +315,17 @@ impl Check for RustfmtCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!("rustfmt-{}", cache::rust_hash(&config.repo_root))) + Some(format!( + "rustfmt-{}", + cache::rust_hash(cargo_cache_root(config)) + )) } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); - let cwd = config - .profile - .cargo_root - .as_ref() - .unwrap_or(&config.repo_root); + let cwd = cargo_cache_root(config); let args = &["fmt", "--check"]; let output = run_command("cargo", args, cwd).await?; @@ -317,18 +335,22 @@ impl Check for RustfmtCheck { let stderr = String::from_utf8_lossy(&output.stderr); let combined = format!("{}\n{}", stdout, stderr); - let status = if output.status.success() { - CheckStatus::Passed + let status = classify_rustfmt_status(output.status.success(), &combined); + let result_output = if status == CheckStatus::Skipped { + format!( + "Rustfmt skipped: rustfmt component is not installed or cargo fmt is unavailable.\n{combined}" + ) + } else if status == CheckStatus::Error { + format!("Rustfmt error: cargo fmt failed unexpectedly.\n{combined}") } else { - // rustfmt --check exits non-zero if files need formatting - CheckStatus::Warnings + combined.clone() }; Ok(CheckResult { name: self.name().to_string(), status, duration: start.elapsed(), - output: combined.clone(), + output: result_output.clone(), cached: false, provenance: Some( ProvenanceBuilder { @@ -336,7 +358,7 @@ impl Check for RustfmtCheck { args, cwd, output: &output, - combined_output: &combined, + combined_output: &result_output, started_at: &started_at, finished_at: &finished_at, cache_key: self.cache_key(config), @@ -347,6 +369,31 @@ impl Check for RustfmtCheck { } } +fn classify_rustfmt_status(command_succeeded: bool, output: &str) -> CheckStatus { + if command_succeeded { + return CheckStatus::Passed; + } + + if rustfmt_tool_unavailable(output) { + return CheckStatus::Skipped; + } + + if has_tool_crash(output) { + return CheckStatus::Error; + } + + // `cargo fmt --check` exits non-zero when files need formatting. + CheckStatus::Warnings +} + +fn rustfmt_tool_unavailable(output: &str) -> bool { + let lower = output.to_ascii_lowercase(); + (lower.contains("rustfmt") || lower.contains("cargo-fmt")) + && (lower.contains("is not installed") + || lower.contains("component") && lower.contains("missing") + || lower.contains("no such command") && lower.contains("fmt")) +} + #[async_trait] impl Check for CargoAuditCheck { fn name(&self) -> &str { @@ -384,11 +431,7 @@ impl Check for CargoAuditCheck { // root. Keying on the root lock while executing in a member meant a // member Cargo.lock change never invalidated the cache and a stale audit // was served (PR #12 review #22). - let cargo_root = config - .profile - .cargo_root - .as_ref() - .unwrap_or(&config.repo_root); + let cargo_root = cargo_cache_root(config); Some(format!( "audit-{}-{}", cache::cargo_lock_hash(cargo_root), @@ -400,11 +443,7 @@ impl Check for CargoAuditCheck { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); - let cwd = config - .profile - .cargo_root - .as_ref() - .unwrap_or(&config.repo_root); + let cwd = cargo_cache_root(config); let args = &["audit", "--json"]; let output = run_command("cargo", args, cwd).await?; @@ -448,23 +487,23 @@ fn classify_cargo_audit_status( return CheckStatus::Failed; } - if command_succeeded { - return CheckStatus::Passed; + if cargo_audit_has_warnings(stdout, combined) { + return CheckStatus::Warnings; } - if cargo_audit_has_warnings(combined) { - return CheckStatus::Warnings; + if command_succeeded { + return CheckStatus::Passed; } return CheckStatus::Failed; } - if command_succeeded { - return CheckStatus::Passed; + if cargo_audit_has_warnings(stdout, combined) { + return CheckStatus::Warnings; } - if cargo_audit_has_warnings(combined) { - return CheckStatus::Warnings; + if command_succeeded { + return CheckStatus::Passed; } if combined.contains("RUSTSEC-") { @@ -495,8 +534,32 @@ fn cargo_audit_vulnerability_count(stdout: &str) -> Option { Some(0) } -fn cargo_audit_has_warnings(output: &str) -> bool { - output.to_ascii_lowercase().contains("warning") +fn cargo_audit_has_warnings(stdout: &str, output: &str) -> bool { + cargo_audit_warning_count(stdout).is_some_and(|count| count > 0) + || output + .lines() + .any(|line| line.to_ascii_lowercase().contains("warning:")) +} + +fn cargo_audit_warning_count(stdout: &str) -> Option { + let parsed = serde_json::from_str::(stdout).ok()?; + let warnings = parsed.get("warnings")?; + Some(count_cargo_audit_warning_items(warnings)) +} + +fn count_cargo_audit_warning_items(value: &serde_json::Value) -> usize { + match value { + serde_json::Value::Array(items) => items.len(), + serde_json::Value::Object(map) => { + if let Some(count) = map.get("count").and_then(|value| value.as_u64()) { + return count as usize; + } + + map.values().map(count_cargo_audit_warning_items).sum() + } + serde_json::Value::Bool(true) => 1, + _ => 0, + } } #[async_trait] @@ -533,18 +596,14 @@ impl Check for CargoGeigerCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!("geiger-{}", cache::rust_hash(&config.repo_root))) + Some(format!("geiger-{}", cargo_content_hash(config))) } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); - let cwd = config - .profile - .cargo_root - .as_ref() - .unwrap_or(&config.repo_root); + let cwd = cargo_cache_root(config); if cargo_metadata_is_virtual_manifest(cwd).await { return Ok(CheckResult { @@ -905,6 +964,109 @@ mod tests { assert!(key.unwrap().starts_with("clippy-")); } + fn config_with_cargo_root(repo_root: &Path, cargo_root: &Path, run_lint: bool) -> Config { + let mut profile = test_rust_profile(true); + profile.cargo_root = Some(cargo_root.to_path_buf()); + test_config_builder() + .repo_root(repo_root) + .profile(profile) + .run_lint(run_lint) + .build() + } + + fn assert_cache_key_changes_after_member_lock_bump(check: &dyn Check, run_lint: bool) { + let tmp = tempfile::tempdir().expect("tempdir"); + let root = tmp.path(); + let member = root.join("member"); + std::fs::create_dir_all(member.join("src")).unwrap(); + std::fs::write(root.join("Cargo.toml"), "[workspace]\n").unwrap(); + std::fs::write(root.join("Cargo.lock"), "# root lock\n").unwrap(); + std::fs::write(member.join("Cargo.toml"), "[package]\nname = \"m\"\n").unwrap(); + std::fs::write(member.join("Cargo.lock"), "# member lock v1\n").unwrap(); + std::fs::write(member.join("src/lib.rs"), "pub fn demo() {}\n").unwrap(); + + let config = config_with_cargo_root(root, &member, run_lint); + let before = check.cache_key(&config).expect("cache key before"); + std::fs::write(member.join("Cargo.lock"), "# member lock v2\n").unwrap(); + let after = check.cache_key(&config).expect("cache key after"); + assert_ne!( + before, + after, + "{} cache key must hash the configured cargo_root manifest set", + check.name() + ); + } + + /// A workspace member with no lockfile of its own must still invalidate its + /// cache key when the workspace-root `Cargo.lock` is bumped — Cargo resolves + /// deps from the root lock, so a root-only dependency change alters what is + /// compiled even though no member file moved. + fn assert_cache_key_changes_after_root_lock_bump(check: &dyn Check, run_lint: bool) { + let tmp = tempfile::tempdir().expect("tempdir"); + let root = tmp.path(); + let member = root.join("member"); + std::fs::create_dir_all(member.join("src")).unwrap(); + std::fs::write(root.join("Cargo.toml"), "[workspace]\n").unwrap(); + std::fs::write(root.join("Cargo.lock"), "# root lock v1\n").unwrap(); + // Member has NO Cargo.lock of its own (the realistic workspace shape). + std::fs::write(member.join("Cargo.toml"), "[package]\nname = \"m\"\n").unwrap(); + std::fs::write(member.join("src/lib.rs"), "pub fn demo() {}\n").unwrap(); + + let config = config_with_cargo_root(root, &member, run_lint); + let before = check.cache_key(&config).expect("cache key before"); + // Bump ONLY the workspace-root lockfile — no member file changes. + std::fs::write(root.join("Cargo.lock"), "# root lock v2 bumped dep\n").unwrap(); + let after = check.cache_key(&config).expect("cache key after"); + assert_ne!( + before, + after, + "{} cache key must fold the workspace-root lockfile for member cargo roots", + check.name() + ); + } + + #[test] + fn test_cargo_check_cache_key_reflects_workspace_root_lock_bump() { + assert_cache_key_changes_after_root_lock_bump(&CargoCheck, false); + } + + #[test] + fn test_clippy_cache_key_reflects_workspace_root_lock_bump() { + assert_cache_key_changes_after_root_lock_bump(&ClippyCheck, true); + } + + #[test] + fn test_geiger_cache_key_reflects_workspace_root_lock_bump() { + assert_cache_key_changes_after_root_lock_bump(&CargoGeigerCheck, false); + } + + #[test] + fn test_cargo_check_cache_key_follows_cargo_root_not_repo_root() { + assert_cache_key_changes_after_member_lock_bump(&CargoCheck, false); + } + + #[test] + fn test_clippy_check_cache_key_follows_cargo_root_not_repo_root() { + assert_cache_key_changes_after_member_lock_bump(&ClippyCheck, true); + } + + #[test] + fn test_rustfmt_cache_key_follows_cargo_root_not_repo_root() { + assert_cache_key_changes_after_member_lock_bump(&RustfmtCheck, true); + } + + #[test] + fn test_cargo_geiger_cache_key_follows_cargo_root_not_repo_root() { + assert_cache_key_changes_after_member_lock_bump(&CargoGeigerCheck, true); + } + + #[test] + fn test_rustfmt_missing_component_is_skipped_not_warnings() { + let output = + "error: 'rustfmt' is not installed for the toolchain 'stable-aarch64-apple-darwin'\n"; + assert_eq!(classify_rustfmt_status(false, output), CheckStatus::Skipped); + } + #[test] fn test_cargo_audit_cache_key_is_day_scoped() { // The audit key must carry the current day so a freshly published @@ -1008,6 +1170,27 @@ mod tests { assert_eq!(status, CheckStatus::Warnings); } + #[test] + fn test_cargo_audit_informational_warning_exit_zero_is_warnings() { + let stdout = r#"{ + "vulnerabilities": { + "found": false, + "count": 0, + "list": [] + }, + "warnings": { + "unmaintained": [ + {"advisory": {"id": "RUSTSEC-2024-0001"}} + ], + "yanked": [], + "notice": [] + } +}"#; + + let status = classify_cargo_audit_status(true, stdout, stdout); + assert_eq!(status, CheckStatus::Warnings); + } + #[test] fn test_cargo_audit_non_json_failure_is_failed() { let combined = "error: failed to fetch advisory db"; diff --git a/src/checks/mod.rs b/src/checks/mod.rs index f1abe15..7654dfa 100644 --- a/src/checks/mod.rs +++ b/src/checks/mod.rs @@ -222,10 +222,19 @@ pub async fn run_all(config: &Config) -> Result<(Vec, Vec = runnable_checks .into_iter() @@ -413,7 +422,11 @@ where }); } - let config = Arc::new(config.clone()); + // One shared target snapshot for the whole run (thread 1); see run_all. + let mut config = config.clone(); + let _shared_snapshot = share_target_snapshot(&mut config, &runnable_checks); + + let config = Arc::new(config); let cache = Arc::new(cache); // Cargo checks share one target/ build lock, so they serialize on a // single-permit semaphore while non-cargo checks stay parallel (PV-17). @@ -639,6 +652,42 @@ fn is_cargo_target_check(name: &str) -> bool { ) } +/// Checks that resolve their scan directory through [`plan_check_run`] and so +/// benefit from the run-wide shared target snapshot (thread 1). Cargo checks run +/// at `cargo_cache_root` and semgrep manages its own worktree, so neither is +/// listed here. +fn uses_shared_scan_dir(name: &str) -> bool { + matches!( + name, + "Ruff" | "Mypy" | "TypeScript" | "ESLint" | "Vitest" | "Stylelint" + ) +} + +/// Materialise ONE target snapshot for the whole run and point `config` at it, so +/// every snapshot-backed check reuses a single worktree instead of creating its +/// own (thread 1). Returns the snapshot handle for the caller to keep alive until +/// all checks finish; `None` (leaving `scan_dir_override` unset) when no runnable +/// check needs a snapshot, or when snapshot creation fails — in which case each +/// check falls back to resolving its own plan, the original per-check behaviour. +fn share_target_snapshot( + config: &mut Config, + runnable_checks: &[Box], +) -> Option { + if !runnable_checks + .iter() + .any(|c| uses_shared_scan_dir(c.name())) + { + return None; + } + match plan_check_run(config) { + Ok(plan) => { + config.scan_dir_override = Some(plan.scan_dir.clone()); + plan._snapshot + } + Err(_) => None, + } +} + /// Security checks stay loud: a spawn failure here is NOT downgraded to Skipped /// (PV-01), so a broken or half-installed security tool can't silently vanish /// from the gate. They pass which::which() at eligibility, but a runtime spawn @@ -827,6 +876,65 @@ pub fn js_tool_available(tool: &str, cwd: &Path) -> bool { local_js_bin(tool, cwd).is_some() } +/// A resolved plan for running a check. +pub struct CheckPlan { + /// Directory to run the check command in. + pub scan_dir: std::path::PathBuf, + /// Ephemeral worktree snapshot, kept alive until the check finishes. + pub _snapshot: Option, +} + +/// Plan check execution path: if we are in a remote/PR mode (meaning resolved target +/// commit is different from the checked-out HEAD commit), create an ephemeral worktree +/// snapshot of the target commit and run there. Otherwise, scan the working tree in place. +/// +/// When the dispatcher has already materialised ONE shared snapshot for the run +/// (`config.scan_dir_override`), reuse its directory instead of creating a +/// per-check worktree. The dispatcher owns and keeps that snapshot alive, so the +/// returned plan carries no snapshot of its own — avoiding N concurrent +/// `git worktree add/remove` calls (one per check) that contend on the git index +/// lock and re-check-out the whole tree repeatedly. +pub fn plan_check_run(config: &Config) -> Result { + if let Some(scan_dir) = &config.scan_dir_override { + return Ok(CheckPlan { + scan_dir: scan_dir.clone(), + _snapshot: None, + }); + } + + let repo_root = config.repo_root.clone(); + let repo = match crate::git::Repository::open(&repo_root) { + Ok(repo) => repo, + Err(_) => { + return Ok(CheckPlan { + scan_dir: repo_root, + _snapshot: None, + }); + } + }; + + let (Ok(target), Ok(head)) = (repo.resolve_target(config), repo.head_commit_id()) else { + return Ok(CheckPlan { + scan_dir: repo_root, + _snapshot: None, + }); + }; + + if head == target.commit_id { + return Ok(CheckPlan { + scan_dir: repo_root, + _snapshot: None, + }); + } + + // Ephemeral worktree + let snapshot = crate::git::create_worktree_snapshot(&repo_root, &target.commit_id)?; + Ok(CheckPlan { + scan_dir: snapshot.worktree_path.clone(), + _snapshot: Some(snapshot), + }) +} + impl CheckResult { pub fn is_failure(&self) -> bool { matches!(self.status, CheckStatus::Failed | CheckStatus::Error) @@ -853,6 +961,36 @@ mod tests { config } + #[test] + fn plan_check_run_reuses_shared_scan_dir_without_new_worktree() { + // Once the dispatcher has set a run-wide shared snapshot, every check's + // plan_check_run must reuse that directory and create NO worktree of its + // own — so N snapshot-backed checks add 0 extra worktrees (thread 1). + let shared = std::path::PathBuf::from("/tmp/prview-shared-snapshot"); + let mut config = test_config(); + config.scan_dir_override = Some(shared.clone()); + + for _ in 0..6 { + let plan = plan_check_run(&config).expect("override path never fails"); + assert_eq!(plan.scan_dir, shared); + assert!( + plan._snapshot.is_none(), + "a shared override must be reused, never re-materialised as a new worktree", + ); + } + } + + #[test] + fn share_target_snapshot_is_a_noop_without_snapshot_backed_checks() { + // Cargo-only / no snapshot-backed checks: no shared worktree is created + // and the override stays unset so nothing changes for those checks. + let mut config = rust_config(true, true, true); + let cargo_only: Vec> = vec![Box::new(crate::checks::cargo::CargoCheck)]; + let snapshot = share_target_snapshot(&mut config, &cargo_only); + assert!(snapshot.is_none()); + assert!(config.scan_dir_override.is_none()); + } + #[test] fn test_check_status_from_str_passed() { assert_eq!( @@ -1455,7 +1593,16 @@ test result: ok. 2 passed; 0 failed let bindir = tmp.path().join("node_modules/.bin"); std::fs::create_dir_all(&bindir).unwrap(); let toolpath = bindir.join("faketool"); - std::fs::write(&toolpath, "#!/bin/sh\necho LOCAL_BIN_RAN\n").unwrap(); + // Close and fsync the write fd in its own scope BEFORE chmod + spawn, so + // the file is fully flushed and no writable descriptor to it lingers in + // this thread when execve runs (first half of the ETXTBSY hardening). + { + use std::io::Write as _; + let mut f = std::fs::File::create(&toolpath).expect("create faketool"); + f.write_all(b"#!/bin/sh\necho LOCAL_BIN_RAN\n") + .expect("write faketool"); + f.sync_all().expect("sync faketool"); + } let mut perms = std::fs::metadata(&toolpath).unwrap().permissions(); perms.set_mode(0o755); std::fs::set_permissions(&toolpath, perms).unwrap(); @@ -1469,9 +1616,24 @@ test result: ok. 2 passed; 0 failed "absent tool must not resolve (caller falls back to npx)" ); - let output = run_js_command_with_timeout("faketool", &[], tmp.path(), 10) - .await - .expect("local bin should run"); + // On Linux a parallel test's fork can transiently inherit the write fd to + // this freshly written executable, so execve races with "Text file busy" + // (os error 26). Retry the spawn a few times; the racing child exec's and + // drops the inherited fd almost immediately. + let mut output = None; + for attempt in 0..8u32 { + match run_js_command_with_timeout("faketool", &[], tmp.path(), 10).await { + Ok(o) => { + output = Some(o); + break; + } + Err(e) if attempt < 7 && e.to_string().contains("os error 26") => { + tokio::time::sleep(Duration::from_millis(50)).await; + } + Err(e) => panic!("local bin should run: {e}"), + } + } + let output = output.expect("local bin should run within the ETXTBSY retry budget"); let stdout = String::from_utf8_lossy(&output.stdout); assert!( stdout.contains("LOCAL_BIN_RAN"), diff --git a/src/checks/python.rs b/src/checks/python.rs index 5e3e2c4..996494b 100644 --- a/src/checks/python.rs +++ b/src/checks/python.rs @@ -2,7 +2,7 @@ use super::{ Check, CheckProvenance, CheckResult, CheckStatus, TEST_TIMEOUT_SECS, find_hard_fail_signatures, - run_command, run_command_with_timeout, tool_spawn_failure_in_output, + plan_check_run, run_command, run_command_with_timeout, tool_spawn_failure_in_output, }; use crate::Config; use crate::cache; @@ -52,18 +52,28 @@ impl Check for RuffCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!("ruff-{}", cache::python_hash(&config.repo_root))) + let repo = crate::git::Repository::open(&config.repo_root).ok()?; + let target = repo.resolve_target(config).ok()?; + let head = repo.head_commit_id().ok()?; + if head == target.commit_id { + Some(format!("ruff-{}", cache::python_hash(&config.repo_root))) + } else { + Some(format!("ruff-{}", target.commit_id)) + } } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); + let plan = plan_check_run(config)?; + let run_dir = &plan.scan_dir; + let use_uv = which::which("uv").is_ok(); let output = if use_uv { - run_command("uv", &["run", "ruff", "check", "."], &config.repo_root).await? + run_command("uv", &["run", "ruff", "check", "."], run_dir).await? } else { - run_command("ruff", &["check", "."], &config.repo_root).await? + run_command("ruff", &["check", "."], run_dir).await? }; let finished_at = Local::now().to_rfc3339(); @@ -87,7 +97,7 @@ impl Check for RuffCheck { provenance: Some(CheckProvenance { command: cmd_str.to_string(), tool_version: None, - cwd: config.repo_root.display().to_string(), + cwd: run_dir.display().to_string(), exit_code: output.status.code(), started_at, finished_at, @@ -139,18 +149,28 @@ impl Check for MypyCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!("mypy-{}", cache::python_hash(&config.repo_root))) + let repo = crate::git::Repository::open(&config.repo_root).ok()?; + let target = repo.resolve_target(config).ok()?; + let head = repo.head_commit_id().ok()?; + if head == target.commit_id { + Some(format!("mypy-{}", cache::python_hash(&config.repo_root))) + } else { + Some(format!("mypy-{}", target.commit_id)) + } } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); + let plan = plan_check_run(config)?; + let run_dir = &plan.scan_dir; + let use_uv = which::which("uv").is_ok(); let output = if use_uv { - run_command("uv", &["run", "mypy", "."], &config.repo_root).await? + run_command("uv", &["run", "mypy", "."], run_dir).await? } else { - run_command("mypy", &["."], &config.repo_root).await? + run_command("mypy", &["."], run_dir).await? }; let finished_at = Local::now().to_rfc3339(); @@ -170,7 +190,7 @@ impl Check for MypyCheck { provenance: Some(CheckProvenance { command: cmd_str.to_string(), tool_version: None, - cwd: config.repo_root.display().to_string(), + cwd: run_dir.display().to_string(), exit_code: output.status.code(), started_at, finished_at, @@ -494,4 +514,105 @@ mod tests { CheckStatus::Passed ); } + + use std::path::Path; + + fn run_git(repo: &Path, args: &[&str]) { + let status = crate::git::git_cmd() + .args(args) + .current_dir(repo) + .status() + .expect("git command"); + assert!(status.success(), "git {args:?} failed with {status}"); + } + + fn write_commit(repo: &Path, name: &str, body: &str) -> String { + std::fs::write(repo.join(name), body).expect("write fixture"); + run_git(repo, &["add", name]); + run_git( + repo, + &[ + "-c", + "user.name=prview test", + "-c", + "user.email=prview@example.test", + "commit", + "-m", + name, + ], + ); + let output = crate::git::git_cmd() + .args(["rev-parse", "HEAD"]) + .current_dir(repo) + .output() + .expect("rev-parse"); + assert!(output.status.success()); + String::from_utf8(output.stdout).unwrap().trim().to_string() + } + + #[tokio::test] + async fn test_ruff_runs_on_fetched_target_in_remote_mode() { + if which::which("ruff").is_err() && which::which("uv").is_err() { + return; + } + + let tmp = tempfile::tempdir().expect("tempdir"); + let repo_path = tmp.path(); + run_git(repo_path, &["init", "-q", "-b", "main"]); + + // Write pyproject.toml so Ruff eligibility passes + std::fs::write( + repo_path.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n\n[tool.ruff]", + ) + .unwrap(); + run_git(repo_path, &["add", "pyproject.toml"]); + + // 1. Commit clean state + let clean_content = "def hello():\n print('hello')\n"; + let clean_commit = write_commit(repo_path, "main.py", clean_content); + + // 2. Commit dirty state with unused import + let dirty_content = "import os\n\ndef hello():\n print('hello')\n"; + let dirty_commit = write_commit(repo_path, "main.py", dirty_content); + + // Scenario A: HEAD is checked out at clean_commit (working tree clean), + // but target is dirty_commit. Ruff must analyze dirty_commit and report failure. + run_git(repo_path, &["checkout", "-q", "-f", &clean_commit]); + + let config_a = test_config_builder() + .profile(test_python_profile(true)) + .run_lint(true) + .target(Some(dirty_commit.as_str())) + .repo_root(repo_path.to_path_buf()) + .build(); + + let check = RuffCheck; + let result_a = check.run(&config_a).await.expect("ruff run scenario A"); + assert_eq!( + result_a.status, + CheckStatus::Failed, + "Ruff must fail because fetched target commit has an unused import. Output: {}", + result_a.output + ); + + // Scenario B: HEAD is checked out at dirty_commit (working tree dirty), + // but target is clean_commit. Ruff must analyze clean_commit and pass. + run_git(repo_path, &["checkout", "-q", "-f", &dirty_commit]); + + let config_b = test_config_builder() + .profile(test_python_profile(true)) + .run_lint(true) + .target(Some(clean_commit.as_str())) + .repo_root(repo_path.to_path_buf()) + .build(); + + let result_b = check.run(&config_b).await.expect("ruff run scenario B"); + assert_eq!( + result_b.status, + CheckStatus::Passed, + "Ruff must pass because fetched target commit is clean. Output: {}", + result_b.output + ); + } } diff --git a/src/checks/semgrep.rs b/src/checks/semgrep.rs index 8df5b7e..e368a2d 100644 --- a/src/checks/semgrep.rs +++ b/src/checks/semgrep.rs @@ -2,7 +2,7 @@ use super::{Check, CheckEligibility, CheckResult, CheckStatus, ProvenanceBuilder, run_command}; use crate::Config; -use crate::git::{Repository, ResolvedRef, git_cmd}; +use crate::git::{Repository, ResolvedRef, WorktreeSnapshot, create_worktree_snapshot}; use anyhow::Result; use async_trait::async_trait; use chrono::Local; @@ -319,61 +319,6 @@ fn merge_base_for_baseline( repo.merge_base(&base.commit_id, &target.commit_id).ok() } -/// An ephemeral detached `git worktree` checked out at a specific commit. Kept -/// alive for the duration of a scan; the worktree is deregistered and its files -/// removed on drop, on every path (scan success or error). -struct WorktreeSnapshot { - repo_root: PathBuf, - worktree_path: PathBuf, - // Owns the enclosing temp dir; dropped after the worktree is deregistered so - // the directory removal is the backstop for the `git worktree remove` call. - _tmp: tempfile::TempDir, -} - -impl Drop for WorktreeSnapshot { - fn drop(&mut self) { - // Deregister the worktree from the main repo, then prune bookkeeping. - // `--force` is required because the checkout is detached. Errors are - // swallowed: cleanup must be best-effort and never panic in a - // destructor (the temp-dir removal is the backstop). - let _ = git_cmd() - .args(["worktree", "remove", "--force"]) - .arg(&self.worktree_path) - .current_dir(&self.repo_root) - .output(); - let _ = git_cmd() - .args(["worktree", "prune"]) - .current_dir(&self.repo_root) - .output(); - } -} - -/// Create an ephemeral detached worktree of `commit` under a fresh temp dir. -fn create_worktree_snapshot(repo_root: &Path, commit: &str) -> Result { - let tmp = tempfile::tempdir()?; - // `git worktree add` wants a path it can create, so point it at a fresh - // subdirectory of the temp dir rather than the (already-created) temp root. - let worktree_path = tmp.path().join("snapshot"); - - let output = git_cmd() - .args(["worktree", "add", "--detach", "--force"]) - .arg(&worktree_path) - .arg(commit) - .current_dir(repo_root) - .output()?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - anyhow::bail!("git worktree add failed: {}", stderr.trim()); - } - - Ok(WorktreeSnapshot { - repo_root: repo_root.to_path_buf(), - worktree_path, - _tmp: tmp, - }) -} - /// Whether semgrep may run a diff-scoped `--baseline-commit` scan. /// /// Baseline mode diffs the *working tree* against the baseline commit, so it is @@ -418,6 +363,7 @@ fn worktree_has_uncommitted_changes(cwd: &Path) -> bool { mod tests { use super::*; use crate::config::test_config; + use crate::git::git_cmd; #[test] fn build_semgrep_args_excludes_vendored_and_generated_and_emits_json() { @@ -670,6 +616,50 @@ mod tests { ); } + #[test] + fn merge_base_baseline_matches_artifact_diff_base_after_base_advances() { + use crate::config::{test_config_builder, test_generic_profile}; + + let tmp = tempfile::tempdir().expect("tempdir"); + run_git(tmp.path(), &["init", "-q", "-b", "main"]); + let merge_base = write_commit(tmp.path(), "own.rs", "pub fn own() -> u8 { 1 }\n"); + run_git(tmp.path(), &["checkout", "-q", "-b", "feature"]); + let _target = write_commit(tmp.path(), "own.rs", "pub fn own() -> u8 { 2 }\n"); + run_git(tmp.path(), &["checkout", "-q", "main"]); + let _advance_one = write_commit( + tmp.path(), + "unrelated.rs", + "pub fn unrelated_one() -> u8 { 1 }\n", + ); + let _advance_two = write_commit( + tmp.path(), + "unrelated.rs", + "pub fn unrelated_one() -> u8 { 1 }\npub fn unrelated_two() -> u8 { 2 }\n", + ); + run_git(tmp.path(), &["checkout", "-q", "feature"]); + + let config = test_config_builder() + .repo_root(tmp.path()) + .target(Some("feature")) + .bases(&["main"]) + .profile(test_generic_profile()) + .build(); + + let repo = Repository::open(tmp.path()).expect("open repo"); + let resolved_target = repo.resolve_target(&config).expect("resolve target"); + let resolved_bases = repo.resolve_bases(&config).expect("resolve bases"); + let diff_bases = repo.resolve_diff_bases(&resolved_target, &resolved_bases, true); + + assert_eq!( + merge_base_for_baseline(&repo, &config, &resolved_target).as_deref(), + diff_bases.first().map(|base| base.commit_id.as_str()) + ); + assert_eq!( + diff_bases.first().map(|base| base.commit_id.as_str()), + Some(merge_base.as_str()) + ); + } + #[test] fn merge_base_falls_back_to_full_scan_with_multiple_bases() { use crate::config::{test_config_builder, test_generic_profile}; diff --git a/src/checks/typescript.rs b/src/checks/typescript.rs index 2cdcca6..aa452a6 100644 --- a/src/checks/typescript.rs +++ b/src/checks/typescript.rs @@ -2,7 +2,7 @@ use super::{ Check, CheckProvenance, CheckResult, CheckStatus, TEST_TIMEOUT_SECS, find_hard_fail_signatures, - js_tool_available, run_js_command, run_js_command_with_timeout, + js_tool_available, plan_check_run, run_js_command, run_js_command_with_timeout, }; use crate::Config; use crate::cache; @@ -228,14 +228,24 @@ impl Check for TypeScriptCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!("tsc-{}", cache::ts_hash(&config.repo_root))) + let repo = crate::git::Repository::open(&config.repo_root).ok()?; + let target = repo.resolve_target(config).ok()?; + let head = repo.head_commit_id().ok()?; + if head == target.commit_id { + Some(format!("tsc-{}", cache::ts_hash(&config.repo_root))) + } else { + Some(format!("tsc-{}", target.commit_id)) + } } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); - let output = run_js_command("tsc", &["--noEmit"], &config.repo_root).await?; + let plan = plan_check_run(config)?; + let run_dir = &plan.scan_dir; + + let output = run_js_command("tsc", &["--noEmit"], run_dir).await?; let finished_at = Local::now().to_rfc3339(); let stdout = String::from_utf8_lossy(&output.stdout); @@ -262,7 +272,7 @@ impl Check for TypeScriptCheck { provenance: Some(CheckProvenance { command: format!("{} tsc --noEmit", js_runner), tool_version: None, - cwd: config.repo_root.display().to_string(), + cwd: run_dir.display().to_string(), exit_code: output.status.code(), started_at, finished_at, @@ -301,16 +311,26 @@ impl Check for ESLintCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!("eslint-{}", cache::ts_hash(&config.repo_root))) + let repo = crate::git::Repository::open(&config.repo_root).ok()?; + let target = repo.resolve_target(config).ok()?; + let head = repo.head_commit_id().ok()?; + if head == target.commit_id { + Some(format!("eslint-{}", cache::ts_hash(&config.repo_root))) + } else { + Some(format!("eslint-{}", target.commit_id)) + } } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); + let plan = plan_check_run(config)?; + let run_dir = &plan.scan_dir; + let args = eslint_args(config); let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - let output = run_js_command("eslint", &args_ref, &config.repo_root).await?; + let output = run_js_command("eslint", &args_ref, run_dir).await?; let finished_at = Local::now().to_rfc3339(); let stdout = String::from_utf8_lossy(&output.stdout); @@ -334,7 +354,7 @@ impl Check for ESLintCheck { provenance: Some(CheckProvenance { command: format!("{} eslint {}", js_runner, args.join(" ")), tool_version: None, - cwd: config.repo_root.display().to_string(), + cwd: run_dir.display().to_string(), exit_code: output.status.code(), started_at, finished_at, @@ -433,6 +453,9 @@ impl Check for VitestCheck { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); + let plan = plan_check_run(config)?; + let run_dir = &plan.scan_dir; + // Build args let mut args = vec!["run"]; @@ -445,8 +468,7 @@ impl Check for VitestCheck { // Use longer timeout for tests let output = - run_js_command_with_timeout("vitest", &args, &config.repo_root, TEST_TIMEOUT_SECS) - .await?; + run_js_command_with_timeout("vitest", &args, run_dir, TEST_TIMEOUT_SECS).await?; let finished_at = Local::now().to_rfc3339(); let stdout = String::from_utf8_lossy(&output.stdout); @@ -474,7 +496,7 @@ impl Check for VitestCheck { provenance: Some(CheckProvenance { command: cmd_str, tool_version: None, - cwd: config.repo_root.display().to_string(), + cwd: run_dir.display().to_string(), exit_code: output.status.code(), started_at, finished_at, @@ -513,19 +535,29 @@ impl Check for StylelintCheck { } fn cache_key(&self, config: &Config) -> Option { - Some(format!( - "stylelint-{}", - cache::stylelint_hash(&config.repo_root) - )) + let repo = crate::git::Repository::open(&config.repo_root).ok()?; + let target = repo.resolve_target(config).ok()?; + let head = repo.head_commit_id().ok()?; + if head == target.commit_id { + Some(format!( + "stylelint-{}", + cache::stylelint_hash(&config.repo_root) + )) + } else { + Some(format!("stylelint-{}", target.commit_id)) + } } async fn run(&self, config: &Config) -> Result { let start = std::time::Instant::now(); let started_at = Local::now().to_rfc3339(); + let plan = plan_check_run(config)?; + let run_dir = &plan.scan_dir; + let args = stylelint_args(config); let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); - let output = run_js_command("stylelint", &args_ref, &config.repo_root).await?; + let output = run_js_command("stylelint", &args_ref, run_dir).await?; let finished_at = Local::now().to_rfc3339(); let stdout = String::from_utf8_lossy(&output.stdout); @@ -559,7 +591,7 @@ impl Check for StylelintCheck { provenance: Some(CheckProvenance { command: format!("{} stylelint {}", js_runner, args.join(" ")), tool_version: None, - cwd: config.repo_root.display().to_string(), + cwd: run_dir.display().to_string(), exit_code: output.status.code(), started_at, finished_at, diff --git a/src/config/mod.rs b/src/config/mod.rs index 932fd5a..c2c57a2 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -74,6 +74,13 @@ pub struct Config { // Bridge rollout metadata (bash->rust migration) pub bridge_stage: u8, pub lint_ignore_patterns: Vec, + + /// Directory the file-scoped checks should scan, when the dispatcher has + /// already materialised a shared target snapshot for the run. `None` = each + /// check resolves its own scan dir via `plan_check_run`. Set once per run by + /// `checks::run_all`/`run_all_with_events` so every snapshot-backed check + /// shares one ephemeral worktree instead of creating its own. + pub scan_dir_override: Option, } #[derive(Debug, Clone, Copy, Default)] @@ -630,6 +637,7 @@ impl Config { policy, bridge_stage: 0, lint_ignore_patterns, + scan_dir_override: None, } } diff --git a/src/git/mod.rs b/src/git/mod.rs index 5847fae..bddfeb5 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -8,6 +8,9 @@ pub use cmd::git_cmd; mod snapshot; pub use snapshot::AnalysisSnapshot; +mod worktree; +pub use worktree::{WorktreeSnapshot, create_worktree_snapshot}; + use crate::Config; use anyhow::{Context, Result}; use git2::{DiffOptions, Repository as Git2Repo}; @@ -290,6 +293,50 @@ impl Repository { Ok(diffs) } + /// Resolve the commits that should anchor target diffs. + /// + /// Base display metadata stays intact, but `commit_id` is moved from the + /// base tip to the merge-base with the target. When histories are unrelated, + /// fall back to the original base tip so callers still get the best local + /// comparison available. + pub fn resolve_diff_bases( + &self, + target: &ResolvedRef, + bases: &[ResolvedRef], + quiet: bool, + ) -> Vec { + use colored::Colorize; + + bases + .iter() + .map(|base| { + if base.commit_id == target.commit_id { + return base.clone(); + } + + match self.merge_base(&base.commit_id, &target.commit_id) { + Ok(merge_base) => { + let mut diff_base = base.clone(); + diff_base.commit_id = merge_base; + diff_base + } + Err(err) => { + if !quiet { + eprintln!( + " {} Could not resolve merge-base for '{}' and target {}; using base tip ({})", + "⚠".yellow(), + base.name, + short_sha(&target.commit_id), + err + ); + } + base.clone() + } + } + }) + .collect() + } + /// Generate diff between two refs pub(crate) fn diff_refs(&self, base: &ResolvedRef, target: &ResolvedRef) -> Result { let base_commit = self @@ -918,6 +965,27 @@ mod tests { (tmp, head_oid, github_base_oid) } + fn init_repo_with_advanced_base() -> (tempfile::TempDir, String, String, String) { + let tmp = tempfile::tempdir().expect("tempdir"); + run_git(tmp.path(), &["init", "-q", "-b", "main"]); + let merge_base = write_commit(tmp.path(), "own.rs", "pub fn own() -> u8 { 1 }\n"); + run_git(tmp.path(), &["checkout", "-q", "-b", "feature"]); + let target = write_commit(tmp.path(), "own.rs", "pub fn own() -> u8 { 2 }\n"); + run_git(tmp.path(), &["checkout", "-q", "main"]); + let _advance_one = write_commit( + tmp.path(), + "unrelated.rs", + "pub fn unrelated_one() -> u8 { 1 }\n", + ); + let base_tip = write_commit( + tmp.path(), + "unrelated.rs", + "pub fn unrelated_one() -> u8 { 1 }\npub fn unrelated_two() -> u8 { 2 }\n", + ); + run_git(tmp.path(), &["checkout", "-q", "feature"]); + (tmp, merge_base, target, base_tip) + } + fn commit(id_suffix: usize) -> CommitInfo { let full_id = format!("{id_suffix:040x}"); CommitInfo { @@ -942,6 +1010,35 @@ mod tests { assert!(!resolved.is_remote); } + #[test] + fn resolve_diff_bases_uses_merge_base_and_preserves_display_base() { + let (tmp, merge_base, _target, base_tip) = init_repo_with_advanced_base(); + let config = test_config_builder() + .repo_root(tmp.path()) + .target(Some("feature")) + .bases(&["main"]) + .profile(test_generic_profile()) + .build(); + + let repo = Repository::open(tmp.path()).expect("open repo"); + let resolved_target = repo.resolve_target(&config).expect("resolve target"); + let resolved_bases = repo.resolve_bases(&config).expect("resolve bases"); + assert_eq!(resolved_bases[0].commit_id, base_tip); + + let diff_bases = repo.resolve_diff_bases(&resolved_target, &resolved_bases, true); + + assert_eq!(diff_bases[0].name, "main"); + assert_eq!(diff_bases[0].commit_id, merge_base); + let diffs = repo + .generate_diffs(&resolved_target, &diff_bases, true) + .expect("generate diffs"); + let files: Vec<_> = diffs + .iter() + .flat_map(|diff| diff.files.iter().map(|file| file.path.as_str())) + .collect(); + assert_eq!(files, vec!["own.rs"]); + } + #[test] fn test_resolved_ref_remote() { let resolved = ResolvedRef { diff --git a/src/git/worktree.rs b/src/git/worktree.rs new file mode 100644 index 0000000..20331c0 --- /dev/null +++ b/src/git/worktree.rs @@ -0,0 +1,76 @@ +//! Ephemeral git worktree support for remote check verification +//! +//! Creates a detached git worktree at a specific commit, with +//! local dependencies (node_modules, .venv) symlinked to preserve local caches. + +use super::cmd::git_cmd; +use anyhow::Result; +use std::path::{Path, PathBuf}; + +/// An ephemeral detached `git worktree` checked out at a specific commit. Kept +/// alive for the duration of a scan; the worktree is deregistered and its files +/// removed on drop, on every path (scan success or error). +pub struct WorktreeSnapshot { + pub repo_root: PathBuf, + pub worktree_path: PathBuf, + // Owns the enclosing temp dir; dropped after the worktree is deregistered so + // the directory removal is the backstop for the `git worktree remove` call. + _tmp: tempfile::TempDir, +} + +impl Drop for WorktreeSnapshot { + fn drop(&mut self) { + // Deregister the worktree from the main repo, then prune bookkeeping. + // `--force` is required because the checkout is detached. Errors are + // swallowed: cleanup must be best-effort and never panic in a + // destructor (the temp-dir removal is the backstop). + let _ = git_cmd() + .args(["worktree", "remove", "--force"]) + .arg(&self.worktree_path) + .current_dir(&self.repo_root) + .output(); + let _ = git_cmd() + .args(["worktree", "prune"]) + .current_dir(&self.repo_root) + .output(); + } +} + +/// Create an ephemeral detached worktree of `commit` under a fresh temp dir. +pub fn create_worktree_snapshot(repo_root: &Path, commit: &str) -> Result { + let tmp = tempfile::tempdir()?; + // `git worktree add` wants a path it can create, so point it at a fresh + // subdirectory of the temp dir rather than the (already-created) temp root. + let worktree_path = tmp.path().join("snapshot"); + + let output = git_cmd() + .args(["worktree", "add", "--detach", "--force"]) + .arg(&worktree_path) + .arg(commit) + .current_dir(repo_root) + .output()?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("git worktree add failed: {}", stderr.trim()); + } + + // Symlink untracked dependencies (node_modules and .venv) to bypass reinstall overhead + #[cfg(unix)] + { + let nm = repo_root.join("node_modules"); + if nm.exists() { + let _ = std::os::unix::fs::symlink(&nm, worktree_path.join("node_modules")); + } + let venv = repo_root.join(".venv"); + if venv.exists() { + let _ = std::os::unix::fs::symlink(&venv, worktree_path.join(".venv")); + } + } + + Ok(WorktreeSnapshot { + repo_root: repo_root.to_path_buf(), + worktree_path, + _tmp: tmp, + }) +} diff --git a/src/lib.rs b/src/lib.rs index 69d7241..ec3725f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -143,9 +143,12 @@ impl App { } // 4. Generate diffs + let diff_bases = self + .repo + .resolve_diff_bases(&target, &bases, self.config.quiet); let diffs = self .repo - .generate_diffs(&target, &bases, self.config.quiet)?; + .generate_diffs(&target, &diff_bases, self.config.quiet)?; // 5. Run checks (reduced set in update mode) let (check_results, skipped_checks) = if self.config.update_mode { @@ -170,9 +173,15 @@ impl App { }; // 6. Run heuristics (loctree-suite) - // In remote/remote-only mode, use git snapshots for deterministic analysis + // In remote/remote-only mode, use git snapshots for deterministic analysis. + // Feed the SAME merge-base range the artifact diff uses (`diff_bases`), not + // the raw base tips: when the base branch has advanced with unrelated work, + // snapshotting the tip would compute the regression delta against base-only + // files the patch excludes, fabricating regressions/caveats. All signals + // must share one range. let heuristics_result = if self.config.remote_mode || self.config.remote_only { - self.run_heuristics_with_snapshots(&target, &bases).await? + self.run_heuristics_with_snapshots(&target, &diff_bases) + .await? } else { heuristics::run_all(&self.config, None).await? }; @@ -439,9 +448,12 @@ impl App { } else { self.repo.resolve_bases(&self.config)? }; + let diff_bases = self + .repo + .resolve_diff_bases(&target, &bases, self.config.quiet); let diffs = self .repo - .generate_diffs(&target, &bases, self.config.quiet)?; + .generate_diffs(&target, &diff_bases, self.config.quiet)?; // Skip checks and heuristics in quick mode let artifacts_dir = artifacts::generate(artifacts::GenerateInput { @@ -764,4 +776,109 @@ mod tests { assert!(commit_ids_match("abc1234def56789", "abc1234def56789")); assert!(!commit_ids_match("abc1234", "def5678")); } + + fn git_run(repo: &std::path::Path, args: &[&str]) { + let status = crate::git::git_cmd() + .args(args) + .current_dir(repo) + .status() + .unwrap(); + assert!(status.success(), "git {:?} failed", args); + } + + fn rev_parse(repo: &std::path::Path, rev: &str) -> String { + let out = crate::git::git_cmd() + .args(["rev-parse", rev]) + .current_dir(repo) + .output() + .unwrap(); + assert!(out.status.success(), "git rev-parse {} failed", rev); + String::from_utf8_lossy(&out.stdout).trim().to_string() + } + + fn resolved(sha: &str) -> crate::git::ResolvedRef { + crate::git::ResolvedRef { + name: sha[..7.min(sha.len())].to_string(), + commit_id: sha.to_string(), + is_remote: false, + } + } + + /// The snapshot-regression base must be exactly the base ref handed to + /// `run_heuristics_with_snapshots`. `run()` now feeds it the merge-base + /// (`diff_bases`), not the base tip, so that when the base branch advances + /// with unrelated work the regression is computed over the same range as the + /// artifact diff — not against base-only files the patch excludes. + #[tokio::test] + async fn snapshot_regression_is_anchored_to_the_base_ref_passed_in() { + let tmp = tempfile::tempdir().unwrap(); + let repo = tmp.path(); + git_run(repo, &["init", "-q", "-b", "main"]); + git_run(repo, &["config", "user.email", "t@t.t"]); + git_run(repo, &["config", "user.name", "T"]); + + // Merge-base commit M: a valid, loctree-analysable crate. + std::fs::write( + repo.join("Cargo.toml"), + "[package]\nname = \"t\"\nversion = \"0.1.0\"\nedition = \"2021\"\n", + ) + .unwrap(); + std::fs::create_dir_all(repo.join("src")).unwrap(); + std::fs::write(repo.join("src/lib.rs"), "pub fn keep() {}\n").unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "base"]); + let merge_base = rev_parse(repo, "HEAD"); + + // Target branch T forks from M with an unrelated source tweak. + git_run(repo, &["checkout", "-q", "-b", "target"]); + std::fs::write(repo.join("src/lib.rs"), "pub fn keep() {}\n// t\n").unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "target"]); + let target_sha = rev_parse(repo, "HEAD"); + + // Base branch B advances beyond the merge-base with its own unrelated file. + git_run(repo, &["checkout", "-q", "main"]); + git_run(repo, &["checkout", "-q", "-b", "advanced-base"]); + std::fs::write(repo.join("src/extra.rs"), "pub fn other() {}\n").unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "advance base"]); + let base_tip = rev_parse(repo, "HEAD"); + assert_ne!(merge_base, base_tip); + + let mut config = test_config(); + config.repo_root = repo.to_path_buf(); + config.run_heuristics = true; + config.execution_mode = ExecutionMode::Deep; + config.quiet = true; + let app = crate::App::from_config(config).unwrap(); + + let target_ref = resolved(&target_sha); + + // Handed the merge-base: regression anchors to it (what `run()` now does). + let via_merge_base = app + .run_heuristics_with_snapshots(&target_ref, &[resolved(&merge_base)]) + .await + .unwrap(); + let reg_mb = via_merge_base + .regression + .expect("loctree signal available on both merge-base and target snapshots"); + assert_eq!( + reg_mb.base_sha, merge_base, + "regression base snapshot must be the merge-base commit" + ); + + // Handed the base tip: it would anchor there instead — the pre-fix bug. + let via_tip = app + .run_heuristics_with_snapshots(&target_ref, &[resolved(&base_tip)]) + .await + .unwrap(); + let reg_tip = via_tip + .regression + .expect("loctree signal available on both base-tip and target snapshots"); + assert_eq!(reg_tip.base_sha, base_tip); + assert_ne!( + reg_mb.base_sha, reg_tip.base_sha, + "the chosen base ref changes which tree the regression is computed against" + ); + } } diff --git a/src/mcp/read.rs b/src/mcp/read.rs index de1e835..d954d52 100644 --- a/src/mcp/read.rs +++ b/src/mcp/read.rs @@ -302,12 +302,112 @@ fn same_run_path(left: &Path, right: &Path) -> bool { } } +/// Scan `runs///` for an in-flight run matching HEAD that the +/// index does not know about yet — a deep run is only registered on completion, +/// so it is invisible to the index-only lookup. Marker-bearing run dirs are +/// exactly the ones the index misses (completed runs are already registered). +/// Returns the most recently started match. Mirrors the `state` tool's +/// `running_run_summary` so `verdict` and `state` agree on "the current run". +fn find_on_disk_run_for_head(repo_name: &str, branch_key: &str, head: &str) -> Option { + let base = crate::config::prview_home() + .join("runs") + .join(repo_name) + .join(branch_key); + let mut best: Option<(String, ResolvedRun)> = None; + for entry in std::fs::read_dir(&base).ok()?.flatten() { + let run_dir = entry.path(); + if !run_dir.is_dir() { + continue; + } + let Some(marker) = read_running_marker(&run_dir) else { + continue; + }; + if !commit_matches(&marker.commit, head) { + continue; + } + let Some(run_id) = run_dir.file_name().and_then(|n| n.to_str()) else { + continue; + }; + let candidate = ResolvedRun { + run_dir: run_dir.clone(), + run_id: run_id.to_string(), + commit: marker.commit.clone(), + }; + let newer = best + .as_ref() + .map(|(started, _)| marker.started_at > *started) + .unwrap_or(true); + if newer { + best = Some((marker.started_at.clone(), candidate)); + } + } + best.map(|(_, run)| run) +} + +/// Newest LIVE in-flight run for HEAD (by `started_at`). +/// +/// Filters to `RunStatus::Running` (a live pid, no completion marker), exactly +/// what the `state` tool reports as "the current run", so `verdict` and `state` +/// agree. Unlike [`find_on_disk_run_for_head`], a stale marker or a +/// completed-but-marker-left run does not qualify here. +fn running_run_for_head(repo_name: &str, branch_key: &str, head: &str) -> Option { + let base = crate::config::prview_home() + .join("runs") + .join(repo_name) + .join(branch_key); + let mut best: Option<(String, ResolvedRun)> = None; + for entry in std::fs::read_dir(&base).ok()?.flatten() { + let run_dir = entry.path(); + if !run_dir.is_dir() || !matches!(run_status(&run_dir), RunStatus::Running { .. }) { + continue; + } + let Some(marker) = read_running_marker(&run_dir) else { + continue; + }; + if !commit_matches(&marker.commit, head) { + continue; + } + let Some(run_id) = run_dir.file_name().and_then(|n| n.to_str()) else { + continue; + }; + let candidate = ResolvedRun { + run_dir: run_dir.clone(), + run_id: run_id.to_string(), + commit: marker.commit.clone(), + }; + let newer = best + .as_ref() + .map(|(started, _)| marker.started_at > *started) + .unwrap_or(true); + if newer { + best = Some((marker.started_at.clone(), candidate)); + } + } + best.map(|(_, run)| run) +} + +/// Pick between the newest indexed COMPLETED run and the newest LIVE in-flight +/// run for HEAD. +/// +/// A live in-flight run wins whenever one exists: it is what `state` reports as +/// the current run, and `verdict` reports it as `in_progress` (so a poller keeps +/// polling) instead of stopping early on a stale completed pack from a prior run +/// on the same HEAD. With no live run, the indexed completed run is returned; +/// `None` only when neither exists, so the caller can fall back to any +/// unregistered on-disk run. +fn choose_head_run( + indexed: Option, + running: Option, +) -> Option { + running.or(indexed) +} + /// Resolve a run for `verdict`/`findings`/`read_artifact`. /// /// With `run_id`: look it up in the index (completed runs), else scan storage /// for an in-flight deep run. Without: the latest run for the current HEAD -/// (R3). A missing run is a fail-loud `run_not_found` — the agent then calls -/// `run_review`. +/// (R3), preferring a live in-flight run over a stale completed pack. A missing +/// run is a fail-loud `run_not_found` — the agent then calls `run_review`. pub fn resolve_run(root: &Path, run_id: Option<&str>) -> Result { let repo_name = crate::config::repo_name_from_root(root); let index = RunIndex::load(); @@ -368,16 +468,31 @@ pub fn resolve_run(root: &Path, run_id: Option<&str>) -> Result Ok(ResolvedRun { + // The index only knows COMPLETED, registered runs; `state` reports the + // live in-flight run. Prefer a live run for HEAD so a `verdict` poll + // without a run_id does not stop early on a stale completed pack while a + // fresh deep run on the same HEAD is still producing its own (A4). + let indexed = latest_for_head(&index, &repo_name, &branch_key, &state.head).map(|e| { + ResolvedRun { run_dir: e.path.clone(), run_id: e.id.clone(), commit: e.commit.clone(), - }), - None => Err(ToolError::new( - error_class::RUN_NOT_FOUND, - "no run for current HEAD; call run_review", - )), + } + }); + let running = running_run_for_head(&repo_name, &branch_key, &state.head); + match choose_head_run(indexed, running) { + Some(run) => Ok(run), + // Neither an indexed nor a live run matched HEAD. Before failing + // loud, scan the on-disk tree for a completed-but-not-yet-registered + // run for HEAD — otherwise a client gets a silent "no runs" while + // its just-finished pack is not yet in the index. + None => match find_on_disk_run_for_head(&repo_name, &branch_key, &state.head) { + Some(run) => Ok(run), + None => Err(ToolError::new( + error_class::RUN_NOT_FOUND, + "no run for current HEAD; call run_review", + )), + }, } } } @@ -959,6 +1074,20 @@ mod tests { assert_eq!(run_status(dir.path()), RunStatus::Failed); } + /// mcp-5 delivery-verifier (b): a `RUNNING.json` with pid 0 (the unknown-pid + /// sentinel) must never read as an immortal `Running`. `kill(0, 0)` targets + /// the caller's whole process group and always succeeds, so pid 0 has to be + /// special-cased as dead → the marker is `Stale`, not eternally alive. + #[test] + fn run_status_pid_zero_is_stale_not_running() { + let dir = tempfile::tempdir().unwrap(); + write_marker(dir.path(), 0); + match run_status(dir.path()) { + RunStatus::Stale { pid, .. } => assert_eq!(pid, 0), + other => panic!("pid 0 must be Stale, never Running; got {other:?}"), + } + } + #[test] fn validate_run_id_rejects_traversal() { assert!(validate_run_id("20260101-120000").is_ok()); @@ -1221,6 +1350,36 @@ mod tests { assert!(latest_for_head(&index, "demo", "other-branch", "aaaa111").is_none()); } + #[test] + fn choose_head_run_prefers_live_in_flight_over_indexed() { + let indexed = ResolvedRun { + run_dir: PathBuf::from("/runs/completed"), + run_id: "completed".to_string(), + commit: "aaaa111".to_string(), + }; + let running = ResolvedRun { + run_dir: PathBuf::from("/runs/in-flight"), + run_id: "in-flight".to_string(), + commit: "aaaa111".to_string(), + }; + + // A fresh in-flight run on the same HEAD wins over the stale completed + // pack — verdict then reports in_progress instead of stopping the poller. + let chosen = choose_head_run(Some(indexed.clone()), Some(running.clone())).unwrap(); + assert_eq!(chosen.run_id, "in-flight"); + + // With no live run, the indexed completed run is the answer. + let only_completed = choose_head_run(Some(indexed.clone()), None).unwrap(); + assert_eq!(only_completed.run_id, "completed"); + + // With only a live run, it is returned. + let only_running = choose_head_run(None, Some(running)).unwrap(); + assert_eq!(only_running.run_id, "in-flight"); + + // Neither → None, so the caller falls back to the on-disk scan. + assert!(choose_head_run(None, None).is_none()); + } + #[test] fn commit_matches_is_prefix_tolerant() { assert!(commit_matches("aaaa111", "aaaa111")); diff --git a/src/mcp/run.rs b/src/mcp/run.rs index c5b33d8..2703560 100644 --- a/src/mcp/run.rs +++ b/src/mcp/run.rs @@ -85,6 +85,29 @@ fn allocate_run_dir( } /// Detect a currently active run on this repo branch (live RUNNING marker). +/// Path to the per-branch activation lock that serializes concurrent `start`s. +/// A file (not a directory), so it is ignored by the run-dir scans in +/// `active_run`/`rebuild`, and lives alongside the branch's run directories. +fn branch_activation_lock_path(repo_name: &str, branch_key: &str) -> PathBuf { + crate::config::prview_home() + .join("runs") + .join(repo_name) + .join(branch_key) + .join(".active.lock") +} + +/// Build the R2b `storage_locked` error, surfacing the active run id when known. +fn locked(active_run_id: Option<&str>) -> ToolError { + ToolError::with_extra( + error_class::STORAGE_LOCKED, + "another review is already running for this repo branch", + serde_json::json!({ + "active_run_id": active_run_id, + "retry_after_ms": 5000, + }), + ) +} + fn active_run(repo_name: &str, branch_key: &str) -> Option { let base = crate::config::prview_home() .join("runs") @@ -287,16 +310,25 @@ pub async fn start( let repo_name = crate::config::repo_name_from_root(repo); let branch_key = crate::config::storage_branch_key(repo); - // R2b: one active run per repo branch. + // mcp-3/TOCTOU: the R2b "one active run" rule was a check-then-act — two + // concurrent starts could both see no active run and both proceed. Serialize + // activation for this repo branch behind an O_EXCL lock file (reusing the + // storage lock's atomic create-new + stale/PID-recycling handling). Held for + // the whole quick run and until a deep run's marker is on disk, so the + // window between "check" and "marker visible to `active_run`" is closed. + let _activation = match crate::storage::acquire_lock_at(&branch_activation_lock_path( + &repo_name, + &branch_key, + )) { + Ok(guard) => guard, + // Another activation is in flight; surface the current run id if its + // marker already landed, else a bare storage_locked. + Err(_) => return Err(locked(active_run(&repo_name, &branch_key).as_deref())), + }; + + // R2b: one active run per repo branch (now race-free under the lock above). if let Some(active_run_id) = active_run(&repo_name, &branch_key) { - return Err(ToolError::with_extra( - error_class::STORAGE_LOCKED, - "another review is already running for this repo branch", - serde_json::json!({ - "active_run_id": active_run_id, - "retry_after_ms": 5000, - }), - )); + return Err(locked(Some(&active_run_id))); } let commit = crate::config::short_head(repo); @@ -393,7 +425,7 @@ pub async fn start( // Completed: the child already registered the run; drop the // marker so status readers see a clean completion. let _ = std::fs::remove_file(read::running_marker_path(&run_dir)); - let mut body = completed_body(&run_dir, &run_id, &commit); + let mut body = completed_body(&run_dir, &run_id, &commit)?; add_base_metadata(&mut body, &selection); Ok(body) } @@ -458,26 +490,27 @@ fn spawn_detached( } /// Build the completed-run response body (quick sync path). -fn completed_body(run_dir: &Path, run_id: &str, commit: &str) -> serde_json::Value { - let decision = read::read_decision(run_dir); - let (verdict, merge_rec, allow_merge, base_used, blocking, caveats) = match &decision { - Ok(d) => ( - d.verdict.clone(), - d.merge_recommendation.clone(), - d.allow_merge, - d.base_used.clone(), - d.blocking_issues.clone(), - d.caveats.clone(), - ), - Err(_) => ( - "UNKNOWN".to_string(), - "review_required".to_string(), - false, - read::read_bases(run_dir), - Vec::new(), - Vec::new(), - ), - }; +/// +/// A completed pack with an unreadable/corrupt `MERGE_GATE.json` is a fail-loud +/// `storage_corrupt` — the SAME contract the `verdict` tool honours on the +/// identical state (mod.rs `verdict` returns `read_decision`'s error). The old +/// path silently substituted `verdict=UNKNOWN, blocking=[], caveats=[]` and +/// returned a `status=completed` success, an "empty success" the MCP contract +/// (types.rs) forbids and a signal the `verdict` tool would reject. +fn completed_body( + run_dir: &Path, + run_id: &str, + commit: &str, +) -> Result { + let d = read::read_decision(run_dir)?; + let (verdict, merge_rec, allow_merge, base_used, blocking, caveats) = ( + d.verdict.clone(), + d.merge_recommendation.clone(), + d.allow_merge, + d.base_used.clone(), + d.blocking_issues.clone(), + d.caveats.clone(), + ); let (checks_passed, checks_failed, files_changed) = run_stats(run_id, run_dir); @@ -499,7 +532,7 @@ fn completed_body(run_dir: &Path, run_id: &str, commit: &str) -> serde_json::Val artifact_paths["report"] = serde_json::json!("report.json"); } - serde_json::json!({ + Ok(serde_json::json!({ "run_id": run_id, "status": "completed", "commit": commit, @@ -516,7 +549,7 @@ fn completed_body(run_dir: &Path, run_id: &str, commit: &str) -> serde_json::Val "checks_failed": checks_failed, "files_changed": files_changed, }, - }) + })) } /// Pull run stats from the freshly-registered index entry (falls back to zeros). @@ -798,13 +831,30 @@ mod tests { // Root-level report.json — where the pack actually writes it. std::fs::write(run_dir.join("report.json"), "{}").unwrap(); - let body = completed_body(run_dir, "20260701-120000", "abc1234"); + let body = completed_body(run_dir, "20260701-120000", "abc1234").unwrap(); assert_eq!( body["artifact_paths"]["report"], serde_json::json!("report.json") ); } + /// mcp-2 delivery-verifier (c): a completed pack whose `MERGE_GATE.json` is + /// unreadable must fail loud (`storage_corrupt`) instead of returning a + /// `status=completed` body with `verdict=UNKNOWN, caveats=[]`. This mirrors + /// the `verdict` tool, which rejects the identical state. + #[test] + fn completed_body_fails_loud_on_corrupt_merge_gate() { + let tmp = tempfile::tempdir().unwrap(); + let run_dir = tmp.path(); + let summary = run_dir.join("00_summary"); + std::fs::create_dir_all(&summary).unwrap(); + // Present but not valid JSON — read_decision must reject it. + std::fs::write(summary.join("MERGE_GATE.json"), "{ not json ").unwrap(); + + let err = completed_body(run_dir, "20260701-120000", "abc1234").unwrap_err(); + assert_eq!(err.class, error_class::STORAGE_CORRUPT); + } + // The quick-timeout process-group kill uses crate::proc::sigkill_process_group, // proven canonically in crate::proc::tests (grandchild reap). } diff --git a/src/output/mod.rs b/src/output/mod.rs index 18d33cf..c0f6282 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -148,6 +148,27 @@ impl Report { } } +fn failure_summary_heading( + report: &Report, + gate: Option<&MergeGateSummary>, +) -> Option<&'static str> { + if !report.has_failures() { + return None; + } + if gate.is_some_and(failures_degraded_to_advisory) { + Some("Check failures downgraded to advisory/pre-existing:") + } else { + Some("Some checks failed:") + } +} + +fn failures_degraded_to_advisory(gate: &MergeGateSummary) -> bool { + gate.verdict == "PASS" + && gate.allow_merge + && gate.quality_pass + && gate.merge_recommendation == crate::policy::engine::MergeRecommendation::Approve +} + pub fn build_cli_json_summary(config: &Config, report: &Report) -> CliJsonSummary { let gate = read_merge_gate_summary(&report.artifacts_dir) .unwrap_or_else(|| fallback_merge_gate_summary(config, report)); @@ -311,6 +332,7 @@ fn read_merge_gate_summary(output_dir: &Path) -> Option { let analysis_status = match decision.get("analysis_status").and_then(Value::as_str) { Some("complete") => crate::policy::engine::AnalysisStatus::Complete, Some("degraded") => crate::policy::engine::AnalysisStatus::Degraded, + Some("incomplete") => crate::policy::engine::AnalysisStatus::Incomplete, _ if allow_merge && quality_pass => crate::policy::engine::AnalysisStatus::Complete, _ => crate::policy::engine::AnalysisStatus::Incomplete, }; @@ -835,9 +857,10 @@ pub fn print_summary(report: &Report) { } } - if report.has_failures() { + let gate = read_merge_gate_summary(&report.artifacts_dir); + if let Some(heading) = failure_summary_heading(report, gate.as_ref()) { println!(); - println!("{} Some checks failed:", "⚠".yellow()); + println!("{} {heading}", "⚠".yellow()); for check in &report.checks { if check.is_failure() { println!( @@ -852,7 +875,7 @@ pub fn print_summary(report: &Report) { // Final authoritative line: the merge-gate verdict, in the same vocabulary // as `--json` (PV-03). Never print a bare "all checks passed" that could // contradict a BLOCK/CONDITIONAL gate — the stdout summary must not lie. - if let Some(gate) = read_merge_gate_summary(&report.artifacts_dir) { + if let Some(gate) = gate { println!(); let (icon, label) = match gate.verdict.as_str() { "PASS" => ("✓".green(), "PASS".green().bold()), @@ -1566,6 +1589,32 @@ api-router/app/core/cache.py assert!(!failure.summary.contains("┌")); } + #[test] + fn artifact_consistency_explicit_incomplete_status_stays_incomplete() { + let temp = tempfile::tempdir().expect("tempdir"); + std::fs::create_dir_all(temp.path().join("00_summary")).expect("summary dir"); + std::fs::write( + temp.path().join("00_summary").join("MERGE_GATE.json"), + r#"{ + "decision": { + "verdict": "PASS", + "analysis_status": "incomplete", + "merge_recommendation": "approve", + "allow_merge": true, + "quality_pass": true + } +}"#, + ) + .expect("write gate"); + + let gate = read_merge_gate_summary(temp.path()).expect("gate"); + + assert_eq!( + gate.analysis_status, + crate::policy::engine::AnalysisStatus::Incomplete + ); + } + #[test] fn test_format_duration_zero() { assert_eq!(format_duration(Duration::from_secs(0)), "0s"); @@ -1576,6 +1625,40 @@ api-router/app/core/cache.py assert_eq!(format_duration(Duration::from_secs(3600)), "60m 0s"); } + #[test] + fn artifact_consistency_degraded_pass_summary_avoids_failed_heading() { + let report = Report { + target: "feature".to_string(), + bases: vec!["main".to_string()], + diffs: vec![], + checks: vec![CheckResult { + name: "Semgrep scan".to_string(), + status: CheckStatus::Failed, + duration: Duration::from_secs(1), + output: "{}".to_string(), + cached: false, + provenance: None, + }], + heuristics: None, + artifacts_dir: PathBuf::from("."), + duration: Duration::from_secs(1), + unchanged: false, + }; + let gate = MergeGateSummary { + verdict: "PASS".to_string(), + analysis_status: crate::policy::engine::AnalysisStatus::Complete, + merge_recommendation: crate::policy::engine::MergeRecommendation::Approve, + allow_merge: true, + quality_pass: true, + reason: Some("pre-existing findings outside the change".to_string()), + }; + + let heading = failure_summary_heading(&report, Some(&gate)).expect("heading"); + + assert!(!heading.contains("Some checks failed")); + assert!(heading.contains("advisory") || heading.contains("pre-existing")); + } + #[test] fn test_describe_run_mode_marks_fast_remote_only_preset() { let mut config = test_config(); diff --git a/src/storage/mod.rs b/src/storage/mod.rs index 1b92cc9..460c2fe 100644 --- a/src/storage/mod.rs +++ b/src/storage/mod.rs @@ -84,20 +84,44 @@ fn resolve_explicit_index_path(path: &Path) -> Result { crate::paths::resolve_file_name_within(parent, file_name) } +/// Parse index entries line-by-line, skipping (never truncating on) bad lines. +/// +/// `map_while(Result::ok)` used to stop at the first line `BufRead::lines` +/// returns an `Err` for (e.g. non-UTF-8): every later run vanished from the +/// view, and the next `register_and_prune` save persisted that loss — permanent +/// data loss from one bad byte. Here an unreadable line is skipped with a warn +/// and iteration continues; an invalid-JSON line is skipped silently as before. +fn read_entries_skipping_bad_lines(file: fs::File, path: &Path) -> Vec { + let reader = std::io::BufReader::new(file); + let mut entries = Vec::new(); + for (idx, line) in reader.lines().enumerate() { + let line = match line { + Ok(line) => line, + Err(err) => { + eprintln!( + "prview: skipping unreadable index line {} in {}: {err}", + idx + 1, + path.display() + ); + continue; + } + }; + if line.trim().is_empty() { + continue; + } + if let Ok(entry) = serde_json::from_str::(&line) { + entries.push(entry); + } + } + entries +} + impl RunIndex { /// Load index from `~/.prview/index.jsonl`. Missing/corrupt lines are skipped. pub fn load() -> Self { let path = index_path(); let entries = match fs::File::open(&path) { - Ok(file) => { - let reader = std::io::BufReader::new(file); - reader - .lines() - .map_while(Result::ok) - .filter(|line| !line.trim().is_empty()) - .filter_map(|line| serde_json::from_str::(&line).ok()) - .collect() - } + Ok(file) => read_entries_skipping_bad_lines(file, &path), Err(_) => Vec::new(), }; Self { entries } @@ -114,15 +138,7 @@ impl RunIndex { } }; let entries = match fs::File::open(&resolved) { - Ok(file) => { - let reader = std::io::BufReader::new(file); - reader - .lines() - .map_while(Result::ok) - .filter(|line| !line.trim().is_empty()) - .filter_map(|line| serde_json::from_str::(&line).ok()) - .collect() - } + Ok(file) => read_entries_skipping_bad_lines(file, &resolved), Err(_) => Vec::new(), }; Self { entries } @@ -144,8 +160,12 @@ impl RunIndex { writeln!(f, "{}", line)?; } f.flush()?; + // fsync the data before the rename publishes it: a bare buffered + // flush + rename can leave a renamed-but-empty file after power loss. + f.sync_all()?; } fs::rename(&tmp, &resolved)?; + fsync_parent_dir(&resolved); Ok(()) } @@ -165,8 +185,10 @@ impl RunIndex { writeln!(f, "{}", line)?; } f.flush()?; + f.sync_all()?; } fs::rename(&tmp, &path)?; + fsync_parent_dir(&path); Ok(()) } @@ -451,17 +473,40 @@ pub fn acquire_lock_at(path: &Path) -> Result { ); } +/// A lock older than any legitimate hold is treated as abandoned even if some +/// process now owns its recorded pid. One hour is far above the longest real +/// hold (a quick review's ~120s budget) yet catches a pid-recycling zombie. +const LOCK_STALE_MAX_AGE_NANOS: u128 = 3600 * 1_000_000_000; + fn lock_is_stale(content: &str) -> bool { - let pid = content - .trim() - .split(':') - .next() - .and_then(|part| part.parse::().ok()); + let mut parts = content.trim().split(':'); + let pid = parts.next().and_then(|part| part.parse::().ok()); + let created_nanos = parts.next().and_then(|part| part.parse::().ok()); + + // No parseable pid → ownership is unattributable → stale. + let Some(pid) = pid else { + return true; + }; + + // Primary signal: the owning process is gone. + if !is_process_alive(pid) { + return true; + } - match pid { - Some(pid) => !is_process_alive(pid), - None => true, + // Second signal (PID-recycling guard): pid liveness alone can be fooled by a + // *different* process that recycled the id. A lock older than any legitimate + // hold is stale regardless of who currently owns that pid. + if let Some(created) = created_nanos { + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + if now.saturating_sub(created) > LOCK_STALE_MAX_AGE_NANOS { + return true; + } } + + false } // --------------------------------------------------------------------------- @@ -473,6 +518,12 @@ fn lock_is_stale(content: &str) -> bool { /// Shared with the MCP run-liveness reader (`mcp::read::run_status`) which /// derives deep-run status deterministically from a pid marker. pub(crate) fn is_process_alive(pid: u32) -> bool { + // pid 0 is never a real owner: it is our unknown-pid sentinel, and + // `kill(0, 0)` targets the *caller's whole process group* — always + // succeeding, which would make a pid-0 marker an immortal "running". + if pid == 0 { + return false; + } // kill(pid, 0) checks if process exists without sending signal let rc = unsafe { libc::kill(pid as libc::pid_t, 0) }; if rc == 0 { @@ -485,6 +536,21 @@ pub(crate) fn is_process_alive(pid: u32) -> bool { ) } +/// Best-effort directory fsync so a just-published rename survives power loss. +/// On unix the parent directory entry must itself be synced for the rename to +/// be durable; elsewhere this is a no-op. Failures are non-fatal (the data is +/// already fsynced) so callers stay infallible. +fn fsync_parent_dir(path: &Path) { + #[cfg(unix)] + if let Some(dir) = path.parent() + && let Ok(dirf) = fs::File::open(dir) + { + let _ = dirf.sync_all(); + } + #[cfg(not(unix))] + let _ = path; +} + fn read_subdirs(dir: &Path) -> Vec { fs::read_dir(dir) .ok() @@ -1024,6 +1090,60 @@ mod tests { assert_eq!(loaded.entries()[1].id, "002"); } + #[test] + fn load_skips_corrupt_line_without_truncating_and_save_preserves_survivors() { + let tmp = tempfile::tempdir().unwrap(); + let idx_path = tmp.path().join("index.jsonl"); + + // Three JSONL records; the middle line is a non-UTF-8 byte sequence that + // `BufRead::lines` returns as `Err`. The old `map_while(Result::ok)` + // stopped there, dropping record 3 — and the next save persisted the loss. + let e1 = serde_json::to_string(&make_entry("001", "repo", "main", 1000)).unwrap(); + let e3 = serde_json::to_string(&make_entry("003", "repo", "main", 3000)).unwrap(); + let mut bytes: Vec = Vec::new(); + bytes.extend_from_slice(e1.as_bytes()); + bytes.push(b'\n'); + bytes.extend_from_slice(&[0xff, 0xfe, 0xfd]); // invalid UTF-8 line + bytes.push(b'\n'); + bytes.extend_from_slice(e3.as_bytes()); + bytes.push(b'\n'); + fs::write(&idx_path, &bytes).unwrap(); + + let loaded = RunIndex::load_from(&idx_path); + let ids: Vec<&str> = loaded.entries().iter().map(|e| e.id.as_str()).collect(); + assert_eq!( + ids, + vec!["001", "003"], + "a corrupt line must skip only itself, not truncate the rest" + ); + + // Re-save the survivors and reload: no silent loss on the round-trip. + let out_path = tmp.path().join("index2.jsonl"); + loaded.save_to(&out_path).unwrap(); + let reloaded = RunIndex::load_from(&out_path); + let ids2: Vec<&str> = reloaded.entries().iter().map(|e| e.id.as_str()).collect(); + assert_eq!(ids2, vec!["001", "003"]); + } + + #[test] + fn lock_is_stale_flags_ancient_lock_even_with_live_pid() { + // A live pid (our own) but an ancient creation timestamp reads as stale: + // the pid may have been recycled and a lock this old is abandoned. + let ancient = format!("{}:1", std::process::id()); + assert!(lock_is_stale(&ancient)); + + // A fresh lock owned by a live pid is NOT stale. + let now_nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + let fresh = format!("{}:{}", std::process::id(), now_nanos); + assert!(!lock_is_stale(&fresh)); + + // pid 0 is the unknown-pid sentinel: never a live owner. + assert!(lock_is_stale("0:1")); + } + #[test] fn test_list_for_repo_and_branch() { let mut index = RunIndex { entries: vec![] }; diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 382076a..65cf28c 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -278,7 +278,12 @@ pub async fn run_analysis(config: Config, tx: mpsc::UnboundedSender) - app.repo.prepare_refs(&app.config)?; let target = app.repo.resolve_target(&app.config)?; let bases = app.repo.resolve_bases(&app.config)?; - let diffs = app.repo.generate_diffs(&target, &bases, app.config.quiet)?; + let diff_bases = app + .repo + .resolve_diff_bases(&target, &bases, app.config.quiet); + let diffs = app + .repo + .generate_diffs(&target, &diff_bases, app.config.quiet)?; // Create snapshots synchronously (for remote/remote-only mode) let target_snap = if app.config.remote_mode || app.config.remote_only { diff --git a/tests/mcp_contract.rs b/tests/mcp_contract.rs index 9627be9..735bd55 100644 --- a/tests/mcp_contract.rs +++ b/tests/mcp_contract.rs @@ -727,6 +727,42 @@ fn polling_running_run_reports_in_progress_and_points_to_verdict() { ); } +/// mcp-1: `verdict` with NO run_id used to be index-only, so an in-flight deep +/// run (registered only on completion) was invisible — the client got a silent +/// `run_not_found` while a review was actively producing its pack. Resolution +/// now scans the on-disk tree for HEAD, matching the `state` tool. +#[test] +fn verdict_without_run_id_sees_in_flight_deep_run() { + let home = tempfile::tempdir().unwrap(); + let repo = fixture_repo(); // branch "feature" + let repo_name = repo_basename(repo.path()); + let commit = git_short_head(repo.path()); + // A live deep run for HEAD, present on disk but NOT in the index. + plant_running_run( + home.path(), + &repo_name, + "feature", + "20260101-020202", + &commit, + &["main"], + ); + + let mut s = McpSession::start(&[("PRVIEW_HOME", home.path().to_str().unwrap())]); + let repo_arg = repo.path().to_str().unwrap(); + + // No run_id: must resolve the on-disk in-flight run, never a silent no-run. + let result = s.call_tool("verdict", serde_json::json!({"repo": repo_arg})); + assert!( + !is_error(&result), + "verdict without run_id must not fail loud while a run is in flight: {result}" + ); + let body = tool_body(&result); + assert_eq!(body["status"], "in_progress"); + assert_eq!(body["run_status"], "running"); + assert_eq!(body["run_id"], "20260101-020202"); + assert_eq!(body["schema_version"], "prview.mcp.v1"); +} + #[test] fn findings_and_read_artifact_on_completed_run() { let home = tempfile::tempdir().unwrap();