From 78f10aa260543a9b0aa74bc16c2927cc2492d142 Mon Sep 17 00:00:00 2001 From: Jernej Barbaric Date: Fri, 10 Jul 2026 09:58:27 +0200 Subject: [PATCH 1/3] fix(report)!: address CI files from the repository root When --root pointed at a package inside a larger repo, fallow's CI output was unusable in two ways that shared one cause: findings were keyed and emitted relative to the analysis root, while CI platforms and git address files from the repository root. Report paths. codeclimate, review-github, and review-gitlab emitted --root-relative paths, so GitLab's Code Quality widget matched nothing and every inline review discussion was rejected for naming a file absent from the MR diff. github-annotations already rebased onto the git toplevel; the others now do too, via the same PathRebase. The review and sticky-summary formats derive their paths from CodeClimate issues, so one seam covers them all. --annotations-path-prefix becomes --report-path-prefix (old name kept as an alias) and governs every CI format. Both names are now registered in global_value_options, fixing a latent misparse of the following subcommand. The generated capability manifest and CLI reference are regenerated for the rename. The prefix is presentation only. The review and sticky-summary filters used to key issues by their rendered path, so a custom or empty prefix silently stopped matching the diff and dropped every inline comment. They now key by the analysis-root-relative path plus the diff's own base. Diff filtering. --diff-file compared diff keys against finding paths already stripped to --root, so below the toplevel nothing matched: every source-anchored finding was dropped and the run reported a clean diff, exit 0, silently. A unified diff does not declare its own base, and both conventions occur in practice -- `git diff` writes toplevel-relative paths, `git diff --relative` writes root-relative ones. The base is therefore resolved from the diff's paths themselves: whichever candidate directory they actually name files under wins. The chosen base rides on DiffIndex, the type that defines the namespace, so every filter call site picks it up and callers that build their own index keep today's behavior. Existence cannot always place a diff. A path naming a real file under both candidates is ambiguous, and a path naming one under neither is foreign. Both are reported. Silence here would rebuild the very failure this change removes: a filter that matches nothing looks exactly like a clean diff. When --root is the repository toplevel the two candidates coincide and output is byte-identical, which the untouched snapshot suite pins. SARIF is deliberately excluded: four builders and its own URI encoding make it a separate change, so it is left out of the flag's allowlist rather than half-rebased. BREAKING CHANGE: codeclimate, review-github, and review-gitlab now emit repository-root-relative paths when --root is a subdirectory. Consumers that prepend the offset themselves should drop that step or pass --report-path-prefix '' to restore the previous output. --- CHANGELOG.md | 54 ++ crates/api/src/duplication_filters.rs | 4 +- crates/api/src/runtime/dead_code.rs | 7 +- crates/api/src/runtime/feature_flags.rs | 3 +- crates/cli/src/audit.rs | 4 +- crates/cli/src/check/filtering.rs | 21 +- crates/cli/src/cli_startup.rs | 61 +- crates/cli/src/combined/output.rs | 9 +- crates/cli/src/dupes.rs | 4 +- crates/cli/src/main.rs | 23 +- crates/cli/src/report/ci/diff_filter.rs | 184 +++- crates/cli/src/report/ci/review.rs | 2 + crates/cli/src/report/codeclimate.rs | 58 +- crates/cli/src/report/github.rs | 67 +- crates/cli/src/report/mod.rs | 5 +- .../cli/tests/monorepo_report_paths_tests.rs | 895 ++++++++++++++++++ crates/engine/src/health/filters.rs | 16 +- crates/output/src/ci_output.rs | 37 +- crates/output/src/diff.rs | 240 ++++- crates/output/src/lib.rs | 11 +- npm/fallow/capabilities.json | 4 +- .../skills/fallow/references/cli-reference.md | 2 +- 22 files changed, 1608 insertions(+), 103 deletions(-) create mode 100644 crates/cli/tests/monorepo_report_paths_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ceb313c43..74f7b2f0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,60 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 every `ModuleInfo`. Existing owned APIs remain compatible, and detector facts are prepared before modules become immutable. +- **BREAKING: CI-facing formats now emit repository-root-relative paths when + `--root` is a subdirectory.** `codeclimate`, `review-github`, and + `review-gitlab` addressed files relative to `--root`, while + `github-annotations` already rebased onto the git toplevel. CI platforms + address files from the repository root, so GitLab's Code Quality widget + matched nothing and every inline review discussion was rejected when the + analyzed project lived in a package subdirectory. All CI formats now share + one namespace, detected via the git toplevel. Consumers that post-process + these paths themselves (prepending the offset in a wrapper script) should + drop that step or pass `--report-path-prefix ''` to restore the old output. + Single-package repositories, where `--root` is the toplevel, are unaffected. + +- **`--annotations-path-prefix` is now `--report-path-prefix`.** It governs + every CI-facing format rather than only the GitHub-native ones. The old name + keeps working as an alias. An explicit empty value disables rebasing. + +### Fixed + +- **`--diff-file` no longer silently discards every source-anchored finding + when `--root` is a subdirectory.** `git diff` names paths relative to the + repository toplevel, but findings were keyed relative to `--root` before the + lookup, so in a monorepo package the two namespaces never met: every + source-anchored finding was dropped and the run reported a clean diff, exit + 0, with nothing on stderr. A unified diff does not declare its own base, and + both conventions are real (`git diff --relative` writes `--root`-relative + paths), so fallow now resolves the base from the diff's paths themselves: + whichever candidate directory they actually name files under wins. Both + conventions work; when `--root` is the repository toplevel they coincide and + output is byte-identical to before. + +- **`--diff-file` now scopes inline review comments, not only analysis results.** + That filter was gated on `FALLOW_DIFF_FILE`, so the flag rendered every comment + while appearing to have applied the diff. + +- **A `--diff-file` whose paths name no file under the repository toplevel or + the analysis root now warns.** This class of mismatch previously produced a + plausible-looking empty report rather than an error. A diff whose paths name + real files under *both* candidates is likewise reported as ambiguous rather + than silently guessed, since existence alone cannot place it. In both cases the + diff is discarded and findings are reported at full scope: a path that cannot be + expressed in the diff's namespace is retained, never silently dropped. + +- **Renamed files keep their `old_path` in `review-gitlab`.** The rename map is + keyed in the diff's namespace, but the lookup used the rendered path, so below + the repository toplevel (or under any `--report-path-prefix`) it missed and + `old_path` silently fell back to `new_path` -- telling GitLab a moved file had + not moved, and getting the discussion rejected. + +- **`--report-path-prefix` no longer decides which inline review comments + survive the diff filter.** The review and sticky-summary filters keyed issues + by their rendered path, so a custom or empty prefix silently stopped matching + the diff and dropped every comment. They now key by the analysis-root-relative + path and the diff's own base; the prefix only affects how paths are rendered. + ## [3.3.0] - 2026-07-09 ### Added diff --git a/crates/api/src/duplication_filters.rs b/crates/api/src/duplication_filters.rs index ddf60a396..92dc05bfc 100644 --- a/crates/api/src/duplication_filters.rs +++ b/crates/api/src/duplication_filters.rs @@ -2,12 +2,12 @@ use std::path::{Path, PathBuf}; -use fallow_output::{DiffIndex, relative_to_diff_path}; +use fallow_output::DiffIndex; use fallow_types::duplicates::{CloneInstance, DuplicationReport}; pub fn filter_by_diff(report: &mut DuplicationReport, diff_index: &DiffIndex, root: &Path) { let instance_overlaps = |instance: &CloneInstance| -> bool { - let Some(rel) = relative_to_diff_path(&instance.file, root) else { + let Some(rel) = diff_index.key_for(&instance.file, root) else { return true; }; let start = u64::try_from(instance.start_line).unwrap_or(u64::MAX); diff --git a/crates/api/src/runtime/dead_code.rs b/crates/api/src/runtime/dead_code.rs index 3f9c5a3f7..283472755 100644 --- a/crates/api/src/runtime/dead_code.rs +++ b/crates/api/src/runtime/dead_code.rs @@ -9,7 +9,7 @@ use fallow_engine::{ }; use fallow_output::{ CHECK_SCHEMA_VERSION, CheckOutputInput, DeadCodeNextStepsInput, DiffIndex, build_check_output, - build_dead_code_next_steps, check_meta, relative_to_diff_path, + build_dead_code_next_steps, check_meta, }; use fallow_types::output_format::OutputFormat; use fallow_types::path_util::is_absolute_path_any_platform; @@ -334,10 +334,11 @@ fn apply_dead_code_scope( fn filter_dead_code_by_diff(results: &mut AnalysisResults, diff: &DiffIndex, root: &Path) { let touches_file = |path: &Path| -> bool { - relative_to_diff_path(path, root).is_none_or(|rel| diff.touches_file(&rel)) + diff.key_for(path, root) + .is_none_or(|rel| diff.touches_file(&rel)) }; let line_in_diff = |path: &Path, line: u32| -> bool { - relative_to_diff_path(path, root) + diff.key_for(path, root) .is_none_or(|rel| diff.line_is_added(&rel, u64::from(line))) }; diff --git a/crates/api/src/runtime/feature_flags.rs b/crates/api/src/runtime/feature_flags.rs index 9a3681a0f..aecb74d25 100644 --- a/crates/api/src/runtime/feature_flags.rs +++ b/crates/api/src/runtime/feature_flags.rs @@ -3,7 +3,6 @@ use std::time::Instant; use fallow_engine::{project_config::ProjectConfig, session::AnalysisSession}; use fallow_output::{ CHECK_SCHEMA_VERSION, FeatureFlagsOutputInput, build_feature_flags_output, feature_flags_meta, - relative_to_diff_path, }; use fallow_types::output_format::OutputFormat; use fallow_types::results::FeatureFlag; @@ -115,7 +114,7 @@ fn apply_feature_flags_scope( } if let Some(diff) = resolved.diff.as_ref() { flags.retain(|flag| { - relative_to_diff_path(&flag.path, session.root()) + diff.key_for(&flag.path, session.root()) .is_none_or(|rel| diff.touches_file(&rel)) }); } diff --git a/crates/cli/src/audit.rs b/crates/cli/src/audit.rs index a523e6cac..8445dc286 100644 --- a/crates/cli/src/audit.rs +++ b/crates/cli/src/audit.rs @@ -1529,8 +1529,8 @@ fn compute_decision_surface( // or renames); lets each decision carry a rename-durable `previous_signal_id`. let rename_old_path = |rel: &str| -> Option { crate::report::ci::diff_filter::shared_diff_index() - .and_then(|idx| idx.old_path_for(rel)) - .map(str::to_string) + .and_then(|idx| idx.old_path_for_root_relative(rel)) + .map(std::borrow::Cow::into_owned) }; // Honest per-anchor consumer count, looked up from the map precomputed before diff --git a/crates/cli/src/check/filtering.rs b/crates/cli/src/check/filtering.rs index af08cbcb9..cf2714ced 100644 --- a/crates/cli/src/check/filtering.rs +++ b/crates/cli/src/check/filtering.rs @@ -39,26 +39,26 @@ pub use fallow_engine::changed_files::{ /// the PR caused. Mirrors the default `FALLOW_SUMMARY_SCOPE=all` behavior /// in typed PR-comment rendering. /// -/// `relative_to_diff_path` normalizes the finding's absolute path to the -/// forward-slashed key shape `git diff` writes (`+++ b/`). When the -/// path cannot be expressed relative to `root` (different drive, traversal -/// escape), the finding is RETAINED rather than silently dropped: an -/// unfilterable path is better surfaced than silently hidden. +/// `DiffIndex::key_for` normalizes the finding's absolute path to the +/// forward-slashed key shape `git diff` writes (`+++ b/`), relative to +/// the diff's own base rather than to `root` — the two differ whenever the +/// analysis root sits below the repository toplevel. When the path cannot be +/// expressed relative to that base (different drive, traversal escape), the +/// finding is RETAINED rather than silently dropped: an unfilterable path is +/// better surfaced than silently hidden. pub fn filter_results_by_diff( results: &mut fallow_types::results::AnalysisResults, diff_index: &crate::report::ci::diff_filter::DiffIndex, root: &Path, ) { - use crate::report::ci::diff_filter::relative_to_diff_path; - let touches_file = |path: &Path| -> bool { - match relative_to_diff_path(path, root) { + match diff_index.key_for(path, root) { Some(p) => diff_index.touches_file(&p), None => true, } }; let line_in_diff = |path: &Path, line: u32| -> bool { - match relative_to_diff_path(path, root) { + match diff_index.key_for(path, root) { Some(p) => diff_index .added_lines_in(&p) .is_some_and(|set| set.contains(&u64::from(line))), @@ -256,11 +256,10 @@ pub fn retain_gate_new( diff_index: &crate::report::ci::diff_filter::DiffIndex, root: &Path, ) { - use crate::report::ci::diff_filter::relative_to_diff_path; use fallow_types::results::TraceHopRole; let line_in_diff = |path: &Path, line: u32| -> bool { - match relative_to_diff_path(path, root) { + match diff_index.key_for(path, root) { Some(p) => diff_index .added_lines_in(&p) .is_some_and(|set| set.contains(&u64::from(line))), diff --git a/crates/cli/src/cli_startup.rs b/crates/cli/src/cli_startup.rs index 048a756bc..cdfc6bef9 100644 --- a/crates/cli/src/cli_startup.rs +++ b/crates/cli/src/cli_startup.rs @@ -210,6 +210,8 @@ fn global_value_options() -> &'static [&'static str] { "--group-by", "--file", "--sarif-file", + "--report-path-prefix", + "--annotations-path-prefix", "--only", "--skip", "--dupes-mode", @@ -318,22 +320,27 @@ pub fn run_pre_dispatch_checks( return Err(fail(code, telemetry::FailureReason::Validation)); } - if cli.annotations_path_prefix.is_some() + if cli.report_path_prefix.is_some() && !matches!( output, fallow_config::OutputFormat::GithubAnnotations | fallow_config::OutputFormat::GithubSummary + | fallow_config::OutputFormat::CodeClimate + | fallow_config::OutputFormat::ReviewGithub + | fallow_config::OutputFormat::ReviewGitlab ) { let code = emit_known_failure( - "--annotations-path-prefix is only valid with --format github-annotations or github-summary", + "--report-path-prefix is only valid with --format github-annotations, \ + github-summary, codeclimate, review-github, or review-gitlab", 2, output, telemetry::FailureReason::Validation, ); return Err(fail(code, telemetry::FailureReason::Validation)); } - report::github::set_annotations_path_prefix(cli.annotations_path_prefix.clone()); + report::github::set_report_path_prefix(cli.report_path_prefix.clone()); + report::github::init_report_prefix(root); parse_cli_tolerance(cli, output) .map_err(|code| fail(code, telemetry::FailureReason::Validation)) @@ -464,6 +471,47 @@ fn parse_cli_tolerance( }) } +/// Directories a supplied unified diff's paths might be relative to, most +/// preferred first. +/// +/// `git diff` writes paths relative to the repository toplevel, while +/// `git diff --relative` writes them relative to the invoking directory. Both +/// reach fallow through `--diff-file` / `--diff-stdin`, and a unified diff does +/// not say which one it is, so the caller offers both and the paths decide (see +/// `choose_diff_base`). The two coincide for a single-package repo, which is why +/// keying against `--root` alone went unnoticed until `--root` addressed a +/// package inside a monorepo. +/// +/// The toplevel is only used to measure how far `root` sits below it; the +/// returned base is that many components popped off `root` itself, so it keeps +/// `root`'s spelling. Finding paths are built from `root`, and a canonicalized +/// base would fail to prefix them wherever the two disagree (`/tmp` vs +/// `/private/tmp` on macOS). +fn diff_base_candidates(root: &Path) -> Vec { + let Some(toplevel) = git_toplevel_base(root) else { + return vec![root.to_path_buf()]; + }; + if toplevel == root { + return vec![root.to_path_buf()]; + } + vec![toplevel, root.to_path_buf()] +} + +/// `root` with its offset below the git toplevel popped off, preserving +/// `root`'s spelling. `None` outside a git repo. +fn git_toplevel_base(root: &Path) -> Option { + let toplevel = crate::base_worktree::git_toplevel(root)?; + let canonical_root = dunce::canonicalize(root).unwrap_or_else(|_| root.to_path_buf()); + let offset = canonical_root.strip_prefix(&toplevel).ok()?; + let mut base = root.to_path_buf(); + for _ in offset.components() { + if !base.pop() { + return None; + } + } + Some(base) +} + fn init_cli_diff_filter( cli: &Cli, root: &Path, @@ -488,7 +536,12 @@ fn init_cli_diff_filter( diff_source, Some(report::ci::diff_filter::DiffSource::EnvVar(_)) | None ); - let _ = report::ci::diff_filter::init_shared_diff(diff_source.as_ref(), suppress_warnings); + let _ = report::ci::diff_filter::init_shared_diff( + diff_source.as_ref(), + root, + &diff_base_candidates(root), + suppress_warnings, + ); Ok(()) } diff --git a/crates/cli/src/combined/output.rs b/crates/cli/src/combined/output.rs index ae303af8e..2431dd373 100644 --- a/crates/cli/src/combined/output.rs +++ b/crates/cli/src/combined/output.rs @@ -946,10 +946,17 @@ fn build_combined_codeclimate( dupes: Option<&DupesResult>, health: Option<&HealthResult>, ) -> serde_json::Value { - let all_issues = build_combined_codeclimate_issues(check, dupes, health); + let mut all_issues = build_combined_codeclimate_issues(check, dupes, health); + // Rebase at the wire boundary only: the same issues feed the sticky summary, + // whose diff filter matches on analysis-root-relative paths. + crate::report::codeclimate::rebase_codeclimate_paths(&mut all_issues); codeclimate_issues_to_value(&all_issues) } +/// Analysis-root-relative CodeClimate issues for every enabled analysis. +/// +/// Deliberately un-rebased: the sticky summary filters these against the diff, +/// and the presentation prefix belongs at the wire boundary. fn build_combined_codeclimate_issues( check: Option<&CheckResult>, dupes: Option<&DupesResult>, diff --git a/crates/cli/src/dupes.rs b/crates/cli/src/dupes.rs index 979335db1..b11f8d335 100644 --- a/crates/cli/src/dupes.rs +++ b/crates/cli/src/dupes.rs @@ -217,10 +217,8 @@ fn filter_by_diff( diff_index: &crate::report::ci::diff_filter::DiffIndex, root: &std::path::Path, ) { - use crate::report::ci::diff_filter::relative_to_diff_path; - let instance_overlaps = |instance: &fallow_types::duplicates::CloneInstance| -> bool { - let Some(rel) = relative_to_diff_path(&instance.file, root) else { + let Some(rel) = diff_index.key_for(&instance.file, root) else { return true; }; let start = u64::try_from(instance.start_line).unwrap_or(u64::MAX); diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 48ac5f945..5f62d7856 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -360,14 +360,21 @@ struct Cli { #[arg(short = 'o', long, global = true, value_name = "PATH")] output_file: Option, - /// Prefix prepended to every `file=` path in `--format github-annotations` - /// output. GitHub resolves annotation paths against the repository root, - /// so when the analyzed project lives in a subdirectory (e.g. - /// `packages/app/`), paths need that offset. fallow detects the offset via - /// the git toplevel automatically; this flag overrides the detection. - /// Valid only with the GitHub-native formats. - #[arg(long = "annotations-path-prefix", global = true, value_name = "PREFIX")] - annotations_path_prefix: Option, + /// Prefix prepended to every path in the CI-facing formats + /// (`github-annotations`, `github-summary`, `codeclimate`, + /// `review-github`, `review-gitlab`). CI platforms address files by + /// repository-root-relative path, so when the analyzed project lives in a + /// subdirectory (e.g. `packages/app/`), paths need that offset. fallow + /// detects the offset via the git toplevel automatically; this flag + /// overrides the detection. Pass an empty string to disable rebasing and + /// emit paths relative to `--root`. + #[arg( + long = "report-path-prefix", + visible_alias = "annotations-path-prefix", + global = true, + value_name = "PREFIX" + )] + report_path_prefix: Option, /// Fail if issue count increased beyond tolerance compared to a regression baseline. #[arg(long, global = true)] diff --git a/crates/cli/src/report/ci/diff_filter.rs b/crates/cli/src/report/ci/diff_filter.rs index 4c7ed5a0d..39c96b36b 100644 --- a/crates/cli/src/report/ci/diff_filter.rs +++ b/crates/cli/src/report/ci/diff_filter.rs @@ -2,7 +2,7 @@ use std::io::Read as _; use std::path::{Path, PathBuf}; use std::sync::OnceLock; -pub use fallow_output::{DiffIndex, MAX_DIFF_BYTES, parse_new_hunk_start, relative_to_diff_path}; +pub use fallow_output::{DiffIndex, MAX_DIFF_BYTES, parse_new_hunk_start}; use fallow_output::CiIssue; @@ -269,12 +269,152 @@ static SHARED_DIFF: OnceLock> = OnceLock::new(); /// Pass `None` to lock the cache to "no diff" without reading anything, /// so a subsequent errant load attempt cannot accidentally populate the /// cache later. -pub fn init_shared_diff(source: Option<&DiffSource>, quiet: bool) -> Option<&'static DiffIndex> { - let loaded = source.and_then(|src| load_diff_index_for_findings(src, quiet)); +pub fn init_shared_diff( + source: Option<&DiffSource>, + root: &Path, + candidate_bases: &[PathBuf], + quiet: bool, +) -> Option<&'static DiffIndex> { + let loaded = source + .and_then(|src| load_diff_index_for_findings(src, quiet)) + .and_then(|loaded| { + let label = source.map(DiffSource::label).unwrap_or_default(); + let chosen = choose_diff_base(&loaded.index, candidate_bases); + match chosen { + // The diff names nothing we can find, or names it in two places + // at once. Either way we cannot express findings in its + // namespace. `check::filtering` sets the convention for that: an + // unfilterable path is RETAINED, never silently dropped. So drop + // the diff instead of the findings and report at full scope. + None => { + if !quiet { + warn_on_foreign_diff_namespace(&loaded.index, candidate_bases, &label); + } + None + } + Some(chosen) if chosen.ambiguous => { + if !quiet { + warn_on_ambiguous_diff_base(&chosen.base, candidate_bases, &label); + } + None + } + Some(chosen) => { + let offset = root_offset_below(&chosen.base, root); + Some(LoadedDiff { + index: loaded.index.with_base(chosen.base).with_root_offset(offset), + raw: loaded.raw, + }) + } + } + }); let _ = SHARED_DIFF.set(loaded); shared_diff_index() } +/// Where the analysis root sits below `base`, forward-slashed, empty when they +/// are the same directory. +fn root_offset_below(base: &Path, root: &Path) -> String { + root.strip_prefix(base) + .map(|offset| offset.display().to_string().replace('\\', "/")) + .unwrap_or_default() +} + +/// The base a diff's paths were written relative to, plus whether the evidence +/// actually distinguished it from the runner-up. +struct ChosenBase { + base: PathBuf, + ambiguous: bool, +} + +/// Decide which directory the diff's paths are relative to. +/// +/// A unified diff carries no statement of its own base. `git diff` writes paths +/// relative to the repository toplevel, but `git diff --relative` writes them +/// relative to the invoking directory, and both reach fallow through +/// `--diff-file` / `--diff-stdin`. Assuming either one silently drops every +/// source-anchored finding for users of the other. +/// +/// The paths themselves settle it: they name files that exist on disk. Score +/// each candidate by how many of the diff's paths resolve under it and take the +/// best. `candidate_bases` is ordered most-preferred first, so an exact tie +/// keeps the caller's precedence. +/// +/// A tie is not a decision. A repo with both `/src/a.ts` and +/// `/src/a.ts` resolves the diff path `src/a.ts` under either candidate, +/// and existence alone cannot say which the diff meant. Picking the preferred +/// one and staying silent would reproduce the empty-report-looks-clean failure +/// this whole mechanism exists to prevent, so the tie is reported. +/// `None` means the diff names nothing under any candidate. +fn choose_diff_base(index: &DiffIndex, candidate_bases: &[PathBuf]) -> Option { + let mut scored: Vec<(usize, &PathBuf)> = candidate_bases + .iter() + .map(|base| { + let resolved = index + .touched_files() + .filter(|path| base.join(path).exists()) + .count(); + (resolved, base) + }) + .filter(|(resolved, _)| *resolved > 0) + .collect(); + + // Stable sort by score, descending: equal scores keep caller precedence. + scored.sort_by(|(a, _), (b, _)| b.cmp(a)); + let (best_score, best_base) = *scored.first()?; + let ambiguous = scored + .get(1) + .is_some_and(|(runner_up, _)| *runner_up == best_score); + + Some(ChosenBase { + base: best_base.clone(), + ambiguous, + }) +} + +/// The diff's paths resolve equally well under two different directories, so +/// the run is about to filter against a base it guessed. Whichever way it +/// guessed, a wrong guess drops every source-anchored finding and prints a +/// clean report, so name the ambiguity rather than let silence imply confidence. +fn warn_on_ambiguous_diff_base(chosen: &Path, candidate_bases: &[PathBuf], label: &str) { + let others = candidate_bases + .iter() + .filter(|base| base.as_path() != chosen) + .map(|base| base.display().to_string()) + .collect::>() + .join(", "); + eprintln!( + "fallow: warning [diff-file]: the paths in {label} name existing files under \ + both {} and {others}, so their base is ambiguous; filtering against {}. \ + If that is wrong, no source-anchored finding will match and the report will \ + look clean. Generate the diff from the repository root (plain `git diff`, \ + not `git diff --relative`) to remove the ambiguity.", + chosen.display(), + chosen.display() + ); +} + +/// A diff whose paths name no file under any candidate base was almost +/// certainly generated relative to some other directory. Every finding would +/// then miss every key and the run would report a clean diff. Say so, once, +/// rather than emitting a plausible-looking empty report. +fn warn_on_foreign_diff_namespace(index: &DiffIndex, candidate_bases: &[PathBuf], label: &str) { + let total = index.touched_files().count(); + if total == 0 { + return; + } + let bases = candidate_bases + .iter() + .map(|base| base.display().to_string()) + .collect::>() + .join(", "); + eprintln!( + "fallow: warning [diff-file]: none of the {total} file(s) named by {label} exist \ + under {bases}; the diff's paths look relative to a different directory. \ + Source-anchored findings will not match it and the report will look clean. \ + Regenerate the diff from one of those directories." + ); +} + /// Read the cached diff index populated by [`init_shared_diff`]. Returns /// `None` when the cache is empty (no diff was supplied, or /// `init_shared_diff` was never called). @@ -303,17 +443,31 @@ fn context_radius_from_env() -> u64 { .unwrap_or(3) } +/// Filter issues against this run's diff. +/// +/// Gated on the shared index, not on `$FALLOW_DIFF_FILE`: `--diff-file` takes +/// precedence when resolving that index, so gating on the env var would leave +/// `--diff-file --format review-gitlab` rendering unfiltered comments, and +/// would filter against the flag's diff while claiming to honour the env var's. +/// The shared index also carries the base its paths were written against; +/// re-parsing here would yield an unbased index whose every lookup misses for +/// an analysis root below that base. #[must_use] pub fn filter_issues_from_env(issues: Vec) -> Vec { + let mode = DiffFilterMode::from_env(); + let radius = context_radius_from_env(); + if let Some(index) = shared_diff_index() { + return issues + .into_iter() + .filter(|issue| diff_index_keeps_issue(index, issue, mode, radius)) + .collect(); + } + // No shared index: this is an embedder or a test, not a CLI run. Honour the + // env var directly so those callers keep working. let Some(raw_path) = std::env::var_os("FALLOW_DIFF_FILE") else { return issues; }; - filter_issues_from_path( - issues, - Path::new(&raw_path), - DiffFilterMode::from_env(), - context_radius_from_env(), - ) + filter_issues_from_path(issues, Path::new(&raw_path), mode, radius) } /// Filter for the typed PR-comment renderer (`print_pr_comment`). @@ -409,13 +563,14 @@ fn diff_index_keeps_issue( mode: DiffFilterMode, radius: u64, ) -> bool { + // `issue.path` is analysis-root-relative; the index's keys live in the + // diff's own namespace. Presentation prefixes are applied later, at render. + let key = index.key_for_root_relative(&issue.path); match mode { DiffFilterMode::NoFilter => true, - DiffFilterMode::File => index.touches_file(&issue.path), - DiffFilterMode::DiffContext => { - index.line_within_added_context(&issue.path, issue.line, radius) - } - DiffFilterMode::Added => index.line_is_added(&issue.path, issue.line), + DiffFilterMode::File => index.touches_file(&key), + DiffFilterMode::DiffContext => index.line_within_added_context(&key, issue.line, radius), + DiffFilterMode::Added => index.line_is_added(&key, issue.line), } } @@ -424,6 +579,7 @@ mod tests { use std::io::Write as _; use super::*; + use fallow_output::relative_to_diff_path; #[test] fn filter_issues_from_path_skips_oversize_diff() { diff --git a/crates/cli/src/report/ci/review.rs b/crates/cli/src/report/ci/review.rs index 2838dbbf2..c5fd10e41 100644 --- a/crates/cli/src/report/ci/review.rs +++ b/crates/cli/src/report/ci/review.rs @@ -50,6 +50,7 @@ pub fn render_review_envelope_with_diff( provider, issues, diff_index, + path_prefix: crate::report::github::report_prefix(), max_comments: max, gitlab_diff_refs: gitlab_diff_refs.as_ref(), include_guidance, @@ -173,6 +174,7 @@ fn render_merged_comment( group, gitlab_diff_refs, diff_index, + path_prefix: "", include_guidance, suggestion_block: &super::suggestion::suggestion_block, guidance_block: &review_guidance_block, diff --git a/crates/cli/src/report/codeclimate.rs b/crates/cli/src/report/codeclimate.rs index ddf94e4e5..bae677529 100644 --- a/crates/cli/src/report/codeclimate.rs +++ b/crates/cli/src/report/codeclimate.rs @@ -13,9 +13,40 @@ use fallow_output::{CodeClimateSeverity, codeclimate_fingerprint_hash}; use fallow_types::duplicates::DuplicationReport; use fallow_types::results::AnalysisResults; +use super::github::report_prefix; use super::grouping::{self, OwnershipResolver}; use super::{emit_json, normalize_uri, relative_path}; +/// Rebase every issue path onto the repository root. +/// +/// CI platforms address files by repo-root-relative path: GitLab's Code +/// Quality widget matches `location.path` against the MR diff, and the review +/// APIs reject a `new_path` that names no file in the diff. The CodeClimate +/// builders emit analysis-root-relative paths, which only coincide with the +/// repo root for a single-package repo. +/// +/// The review and sticky-summary formats derive their paths from these issues, +/// so rebasing here covers every CI surface but `github-annotations`, which +/// renders from JSON and applies the same rebase itself. +pub fn rebase_codeclimate_paths(issues: &mut [CodeClimateIssue]) { + let prefix = report_prefix(); + if prefix.is_empty() { + return; + } + for issue in issues { + issue.location.path = fallow_output::apply_path_prefix(prefix, &issue.location.path); + } +} + +/// Rebase, then serialize. The single point where CodeClimate issues leave as +/// the CodeClimate wire format; everything upstream keeps analysis-root-relative +/// paths so diff lookups and CODEOWNERS resolution stay in one namespace. +fn emit_codeclimate(mut issues: Vec) -> ExitCode { + rebase_codeclimate_paths(&mut issues); + let value = codeclimate_issues_to_value(&issues); + emit_json(&value, "CodeClimate") +} + /// Map fallow severity to CodeClimate severity. #[cfg(test)] fn severity_to_codeclimate(s: Severity) -> CodeClimateSeverity { @@ -134,9 +165,7 @@ pub(super) fn print_codeclimate( root: &Path, rules: &RulesConfig, ) -> ExitCode { - let issues = api_codeclimate_issues(results, root, rules); - let value = codeclimate_issues_to_value(&issues); - emit_json(&value, "CodeClimate") + emit_codeclimate(api_codeclimate_issues(results, root, rules)) } /// Print CodeClimate output with owner properties added to each issue. @@ -154,9 +183,7 @@ pub(super) fn print_grouped_codeclimate( annotate_codeclimate_issues(&mut issues, CodeClimateAnnotationField::Owner, |path| { grouping::resolve_owner(Path::new(path), Path::new(""), resolver) }); - let value = codeclimate_issues_to_value(&issues); - - emit_json(&value, "CodeClimate") + emit_codeclimate(issues) } /// Fetch CodeClimate issues from the API-owned health output builder. @@ -170,9 +197,7 @@ pub(super) fn api_health_codeclimate_issues( /// Print health analysis results in CodeClimate format. pub(super) fn print_health_codeclimate(report: &HealthReport, root: &Path) -> ExitCode { - let issues = api_health_codeclimate_issues(report, root); - let value = codeclimate_issues_to_value(&issues); - emit_json(&value, "CodeClimate") + emit_codeclimate(api_health_codeclimate_issues(report, root)) } /// Print health CodeClimate output with a per-issue `group` field. @@ -192,9 +217,7 @@ pub(super) fn print_grouped_health_codeclimate( annotate_codeclimate_issues(&mut issues, CodeClimateAnnotationField::Group, |path| { grouping::resolve_owner(Path::new(path), Path::new(""), resolver) }); - let value = codeclimate_issues_to_value(&issues); - - emit_json(&value, "CodeClimate") + emit_codeclimate(issues) } /// Fetch CodeClimate issues from the API-owned duplication output builder. @@ -208,9 +231,7 @@ pub(super) fn api_duplication_codeclimate_issues( /// Print duplication analysis results in CodeClimate format. pub(super) fn print_duplication_codeclimate(report: &DuplicationReport, root: &Path) -> ExitCode { - let issues = api_duplication_codeclimate_issues(report, root); - let value = codeclimate_issues_to_value(&issues); - emit_json(&value, "CodeClimate") + emit_codeclimate(api_duplication_codeclimate_issues(report, root)) } /// Print duplication CodeClimate output with a per-issue `group` field. @@ -233,8 +254,7 @@ pub(super) fn print_grouped_duplication_codeclimate( for group in &report.clone_groups { let owner = super::dupes_grouping::largest_owner(group, root, resolver); for instance in &group.instances { - let path = cc_path(&instance.file, root); - path_to_owner.insert(path, owner.clone()); + path_to_owner.insert(cc_path(&instance.file, root), owner.clone()); } } @@ -244,9 +264,7 @@ pub(super) fn print_grouped_duplication_codeclimate( .cloned() .unwrap_or_else(|| crate::codeowners::UNOWNED_LABEL.to_string()) }); - let value = codeclimate_issues_to_value(&issues); - - emit_json(&value, "CodeClimate") + emit_codeclimate(issues) } #[cfg(test)] diff --git a/crates/cli/src/report/github.rs b/crates/cli/src/report/github.rs index 5af547751..741cf68c2 100644 --- a/crates/cli/src/report/github.rs +++ b/crates/cli/src/report/github.rs @@ -195,8 +195,9 @@ pub fn resolve_package_manager(env_value: Option<&str>, root: &Path) -> PackageM } } -/// How `file=` paths are rebased onto the git repository root. GitHub -/// resolves annotation paths against the REPO root, while fallow emits +/// How report paths are rebased onto the git repository root. CI platforms +/// address files by repo-root-relative path (GitHub annotations, GitLab's +/// Code Quality widget, the review-discussion APIs), while fallow emits /// analysis-root-relative paths; when the analysis root is a subdirectory /// (e.g. `packages/app/`), every path needs the offset prefixed. #[derive(Clone, PartialEq, Eq, Debug, Default)] @@ -218,8 +219,22 @@ impl PathRebase { } } - /// Resolve the rebase: an explicit `--annotations-path-prefix` wins over + /// The prefix this rebase prepends, empty when it prepends nothing. + /// + /// Consumers that resolve ownership from an already-rebased path strip + /// this back off to recover the analysis-root-relative path CODEOWNERS + /// patterns are written against. + #[must_use] + pub fn prefix(&self) -> &str { + match self { + Self::None => "", + Self::Prefix(prefix) => prefix, + } + } + + /// Resolve the rebase: an explicit `--report-path-prefix` wins over /// git-toplevel detection; no git and no flag means paths pass through. + /// An explicit empty prefix disables rebasing. #[must_use] pub fn resolve(root: &Path, explicit: Option<&str>) -> Self { if let Some(prefix) = explicit { @@ -254,22 +269,52 @@ impl PathRebase { } } -/// Process-wide `--annotations-path-prefix` override, set once by `main` +/// Process-wide `--report-path-prefix` override, set once by `main` /// after parse (same ambient pattern as the report sink and the /// max-file-size override). -static ANNOTATIONS_PATH_PREFIX: OnceLock> = OnceLock::new(); +static REPORT_PATH_PREFIX: OnceLock> = OnceLock::new(); -/// Record the `--annotations-path-prefix` flag value. Call at most once. -pub fn set_annotations_path_prefix(prefix: Option) { - let _ = ANNOTATIONS_PATH_PREFIX.set(prefix); +/// Record the `--report-path-prefix` flag value. Call at most once. +pub fn set_report_path_prefix(prefix: Option) { + let _ = REPORT_PATH_PREFIX.set(prefix); } -fn annotations_path_prefix() -> Option<&'static str> { - ANNOTATIONS_PATH_PREFIX +fn report_path_prefix() -> Option<&'static str> { + REPORT_PATH_PREFIX .get() .and_then(|prefix| prefix.as_deref()) } +/// The presentation prefix for this run, resolved once. +/// +/// `report_rebase` shells out to `git rev-parse --show-toplevel`; the emitters +/// need the answer on every issue, and the review renderer has no `root` to +/// re-derive it from. +static RESOLVED_REPORT_PREFIX: OnceLock = OnceLock::new(); + +/// Resolve the presentation prefix once, after `--report-path-prefix` is +/// recorded. Call at most once. +pub fn init_report_prefix(root: &Path) { + let _ = RESOLVED_REPORT_PREFIX.set(report_rebase(root).prefix().to_owned()); +} + +/// The prefix prepended to every CI-facing path emitted this run. Empty when +/// the analysis root is the repository root, or when no CLI run resolved it. +#[must_use] +pub fn report_prefix() -> &'static str { + RESOLVED_REPORT_PREFIX.get().map_or("", String::as_str) +} + +/// Rebase every CI-facing report path emitted for this run onto the repo root. +/// +/// Shared by the GitHub renderers and the CodeClimate emitters (which the +/// review and sticky-summary formats derive their paths from), so all CI +/// surfaces address files the same way the platform does. +#[must_use] +pub fn report_rebase(root: &Path) -> PathRebase { + PathRebase::resolve(root, report_path_prefix()) +} + /// Ambient options for the GitHub renderers, resolved once per render at the /// CLI print boundary so the pure render functions stay deterministic. #[derive(Debug, Default)] @@ -283,7 +328,7 @@ pub struct RenderOptions { pub fn resolve_render_options(root: &Path) -> RenderOptions { let env_pm = std::env::var("PKG_MANAGER").ok(); RenderOptions { - rebase: PathRebase::resolve(root, annotations_path_prefix()), + rebase: report_rebase(root), pm: resolve_package_manager(env_pm.as_deref(), root), } } diff --git a/crates/cli/src/report/mod.rs b/crates/cli/src/report/mod.rs index ed6186a8f..80f9cd3a6 100644 --- a/crates/cli/src/report/mod.rs +++ b/crates/cli/src/report/mod.rs @@ -1,6 +1,6 @@ mod badge; pub mod ci; -mod codeclimate; +pub mod codeclimate; mod compact; pub mod dupes_grouping; pub mod github; @@ -363,6 +363,9 @@ fn print_results_ci_comment( ctx: &ReportContext<'_>, output: OutputFormat, ) -> ExitCode { + // Analysis-root-relative on purpose: the review renderer applies the + // presentation prefix after its diff lookups, and rebasing here would + // prefix twice and key the filter in the wrong namespace. let issues = codeclimate::api_codeclimate_issues(results, ctx.root, ctx.rules); let value = fallow_output::codeclimate_issues_to_value(&issues); print_ci_comment_format("dead-code", &value, output).unwrap_or_else(|| { diff --git a/crates/cli/tests/monorepo_report_paths_tests.rs b/crates/cli/tests/monorepo_report_paths_tests.rs new file mode 100644 index 000000000..f6e09ad7d --- /dev/null +++ b/crates/cli/tests/monorepo_report_paths_tests.rs @@ -0,0 +1,895 @@ +#![allow( + clippy::unwrap_used, + clippy::expect_used, + reason = "tests and benches use unwrap and expect to keep fixture setup concise" +)] +//! `--root` below the git toplevel: report paths and diff-filter keys. +//! +//! Both behaviors are invisible in the single-package case where `--root` IS +//! the repo toplevel, which is what every other fixture covers. + +#[path = "common/mod.rs"] +mod common; + +use std::path::Path; +use std::process::Command; + +use common::{fallow_bin, parse_json}; + +fn git(dir: &Path, args: &[&str]) { + let status = Command::new("git") + .args(args) + .current_dir(dir) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .env("GIT_AUTHOR_NAME", "test") + .env("GIT_AUTHOR_EMAIL", "test@test.com") + .env("GIT_COMMITTER_NAME", "test") + .env("GIT_COMMITTER_EMAIL", "test@test.com") + .status() + .expect("git command failed"); + assert!(status.success(), "git {args:?} failed"); +} + +/// A repo whose `packages/pkg` is the analysis root, holding one function that +/// trips a complexity rule. +fn monorepo() -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let pkg = root.join("packages/pkg/src"); + std::fs::create_dir_all(&pkg).expect("mkdir"); + + std::fs::write(root.join("package.json"), r#"{"name":"mono"}"#).expect("write"); + std::fs::write( + root.join("packages/pkg/package.json"), + r#"{"name":"pkg","version":"1.0.0","main":"src/index.js"}"#, + ) + .expect("write"); + std::fs::write( + pkg.join("index.js"), + "export function entry(a) {\n return a\n}\n", + ) + .expect("write"); + std::fs::write(pkg.join("complex.js"), COMPLEX_JS).expect("write"); + + git(root, &["init", "-q"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-qm", "init"]); + dir +} + +const COMPLEX_JS: &str = "export function complex(a, b, c, d, e) { + let r = 0 + if (a > 1) { r += 1 } else if (a < 0) { r -= 1 } + if (b > 1) { r += 2 } else if (b < 0) { r -= 2 } + if (c > 1) { r += 3 } else if (c < 0) { r -= 3 } + if (d > 1) { r += 4 } else if (d < 0) { r -= 4 } + if (e > 1) { r += 5 } else if (e < 0) { r -= 5 } + for (const x of [a, b, c]) { if (x) { r += x } } + while (r > 100) { r -= 10 } + return r +} +"; + +fn run(root: &Path, args: &[&str]) -> common::CommandOutput { + let mut cmd = Command::new(fallow_bin()); + cmd.current_dir(root) + .env("RUST_LOG", "") + .env("NO_COLOR", "1"); + for arg in args { + cmd.arg(arg); + } + let output = cmd.output().expect("failed to run fallow"); + common::CommandOutput { + stdout: String::from_utf8_lossy(&output.stdout).to_string(), + stderr: String::from_utf8_lossy(&output.stderr).to_string(), + code: output.status.code().unwrap_or(-1), + } +} + +fn run_with_env(root: &Path, args: &[&str], env: &[(&str, &str)]) -> common::CommandOutput { + let mut cmd = Command::new(fallow_bin()); + cmd.current_dir(root) + .env("RUST_LOG", "") + .env("NO_COLOR", "1"); + for (key, value) in env { + cmd.env(key, value); + } + for arg in args { + cmd.arg(arg); + } + let output = cmd.output().expect("failed to run fallow"); + common::CommandOutput { + stdout: String::from_utf8_lossy(&output.stdout).to_string(), + stderr: String::from_utf8_lossy(&output.stderr).to_string(), + code: output.status.code().unwrap_or(-1), + } +} + +fn review_comment_count(output: &common::CommandOutput) -> usize { + parse_json(output)["comments"] + .as_array() + .map_or(0, Vec::len) +} + +fn codeclimate_paths(output: &common::CommandOutput) -> Vec { + parse_json(output) + .as_array() + .expect("codeclimate output is an array") + .iter() + .map(|issue| issue["location"]["path"].as_str().expect("path").to_owned()) + .collect() +} + +/// Ordering shifted between releases (a project-level `package.json` finding +/// began sorting first), so assert on membership, never on index. +fn check_names(output: &common::CommandOutput) -> Vec { + parse_json(output) + .as_array() + .expect("codeclimate output is an array") + .iter() + .map(|issue| issue["check_name"].as_str().expect("check_name").to_owned()) + .collect() +} + +#[test] +fn codeclimate_paths_are_repo_root_relative_below_the_toplevel() { + let dir = monorepo(); + let out = run( + dir.path(), + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + ], + ); + + let paths = codeclimate_paths(&out); + assert!(!paths.is_empty(), "expected findings, got {}", out.stdout); + for path in &paths { + assert!( + path.starts_with("packages/pkg/"), + "CI consumers address files from the repo root; got {path}" + ); + } +} + +#[test] +fn report_path_prefix_overrides_the_detected_offset() { + let dir = monorepo(); + let out = run( + dir.path(), + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--report-path-prefix", + "custom/base", + ], + ); + + for path in codeclimate_paths(&out) { + assert!(path.starts_with("custom/base/"), "got {path}"); + } +} + +#[test] +fn empty_report_path_prefix_disables_rebasing() { + let dir = monorepo(); + let out = run( + dir.path(), + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--report-path-prefix", + "", + ], + ); + + for path in codeclimate_paths(&out) { + assert!( + !path.starts_with("packages/"), + "explicit empty prefix should emit --root-relative paths; got {path}" + ); + } +} + +#[test] +fn deprecated_annotations_path_prefix_alias_still_parses() { + let dir = monorepo(); + let out = run( + dir.path(), + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--annotations-path-prefix", + "custom/base", + ], + ); + + assert_eq!(out.code, 0, "alias should be accepted: {}", out.stderr); + for path in codeclimate_paths(&out) { + assert!(path.starts_with("custom/base/"), "got {path}"); + } +} + +/// `github-annotations` renders from JSON and applies its own rebase, so the +/// CodeClimate-side rebase must not double-prefix it. +#[test] +fn github_annotations_and_codeclimate_agree_on_path_shape() { + let dir = monorepo(); + let cc = run( + dir.path(), + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + ], + ); + let annotations = run( + dir.path(), + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "github-annotations", + ], + ); + + for path in codeclimate_paths(&cc) { + assert!(path.starts_with("packages/pkg/"), "got {path}"); + assert!( + !path.starts_with("packages/pkg/packages/"), + "double-prefixed" + ); + } + assert!( + annotations + .stdout + .contains("file=packages/pkg/src/complex.js"), + "annotations lost their rebase: {}", + annotations.stdout + ); + assert!( + !annotations.stdout.contains("packages/pkg/packages/"), + "annotations double-prefixed: {}", + annotations.stdout + ); +} + +/// The dangerous defect: a real `git diff` names paths from the repo toplevel. +/// Keyed against `--root` they matched nothing, and the run reported a clean +/// diff with no warning. +#[test] +fn repo_root_relative_diff_keeps_source_anchored_findings() { + let dir = monorepo(); + let root = dir.path(); + let complex = root.join("packages/pkg/src/complex.js"); + std::fs::write( + &complex, + COMPLEX_JS.replace( + " return r\n", + " const touched = 1\n return r + touched\n", + ), + ) + .expect("write"); + + let diff = Command::new("git") + .args(["diff"]) + .current_dir(root) + .output() + .expect("git diff"); + let diff_path = root.join("pr.diff"); + std::fs::write(&diff_path, &diff.stdout).expect("write diff"); + assert!( + String::from_utf8_lossy(&diff.stdout).contains("+++ b/packages/pkg/src/complex.js"), + "fixture diff should be repo-root-relative" + ); + + let unfiltered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + ], + ); + let filtered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + let names = check_names(&filtered); + assert!( + names.iter().any(|n| n == "fallow/high-crap-score"), + "the complexity finding sits on an added line and must survive the \ + diff filter; got {names:?}" + ); + assert_eq!( + check_names(&unfiltered).len(), + names.len(), + "every finding in this fixture is in the diff, so the filter should \ + drop nothing" + ); + assert!( + !filtered.stderr.contains("warning [diff-file]"), + "a matching diff must not warn: {}", + filtered.stderr + ); +} + +/// `git diff --relative`, run from the package directory, writes `--root`-relative +/// paths. That is a legitimate second convention, so the base is chosen by which +/// one the diff's paths actually name on disk rather than assumed. +#[test] +fn root_relative_diff_also_keeps_source_anchored_findings() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("relative.diff"); + std::fs::write( + &diff_path, + "diff --git a/src/complex.js b/src/complex.js\n\ + --- a/src/complex.js\n\ + +++ b/src/complex.js\n\ + @@ -9,0 +10,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let out = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + check_names(&out) + .iter() + .any(|n| n == "fallow/high-crap-score"), + "a --relative diff names real files under --root and must filter \ + against it; got {:?}", + check_names(&out) + ); + assert!( + !out.stderr.contains("warning [diff-file]"), + "a resolvable diff must not warn: {}", + out.stderr + ); +} + +/// A diff naming files that exist under neither the toplevel nor `--root`. +/// Silently reporting zero findings is what made the original defect look like +/// a clean diff, so this must warn. +#[test] +fn foreign_namespace_diff_warns_instead_of_reporting_clean() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("foreign.diff"); + std::fs::write( + &diff_path, + "diff --git a/nowhere/ghost.js b/nowhere/ghost.js\n\ + --- a/nowhere/ghost.js\n\ + +++ b/nowhere/ghost.js\n\ + @@ -0,0 +1,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let out = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + out.stderr.contains("warning [diff-file]") + && out.stderr.contains("relative to a different directory"), + "expected a foreign-namespace warning, got stderr: {}", + out.stderr + ); +} + +/// When `--root` IS the toplevel, nothing about either behavior may move. +#[test] +fn analysis_root_at_the_toplevel_is_unchanged() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::create_dir_all(root.join("src")).expect("mkdir"); + std::fs::write( + root.join("package.json"), + r#"{"name":"single","version":"1.0.0","main":"src/index.js"}"#, + ) + .expect("write"); + std::fs::write( + root.join("src/index.js"), + "export function entry(a) {\n return a\n}\n", + ) + .expect("write"); + std::fs::write(root.join("src/complex.js"), COMPLEX_JS).expect("write"); + git(root, &["init", "-q"]); + git(root, &["add", "-A"]); + git(root, &["commit", "-qm", "init"]); + + let out = run(root, &["--quiet", "--format", "codeclimate"]); + for path in codeclimate_paths(&out) { + assert!( + path.starts_with("src/"), + "root == toplevel must emit unprefixed paths; got {path}" + ); + } +} + +/// A diff path that names a real file under BOTH the toplevel and `--root` +/// cannot be placed by existence alone. Guessing and staying quiet would +/// reproduce the empty-report-looks-clean failure this mechanism exists to +/// prevent, so the ambiguity is reported. +#[test] +fn ambiguous_diff_base_warns_instead_of_guessing_silently() { + let dir = monorepo(); + let root = dir.path(); + // Now `src/complex.js` resolves under the toplevel AND under packages/pkg. + std::fs::create_dir_all(root.join("src")).expect("mkdir"); + std::fs::write(root.join("src/complex.js"), COMPLEX_JS).expect("write"); + git(root, &["add", "-A"]); + git(root, &["commit", "-qm", "top-level src"]); + + let diff_path = root.join("ambiguous.diff"); + std::fs::write( + &diff_path, + "diff --git a/src/complex.js b/src/complex.js\n\ + --- a/src/complex.js\n\ + +++ b/src/complex.js\n\ + @@ -9,0 +10,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let out = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + out.stderr.contains("warning [diff-file]") && out.stderr.contains("ambiguous"), + "an ambiguous diff base must be reported, not guessed silently; \ + stderr: {}", + out.stderr + ); +} + +/// An unambiguous diff must not acquire the ambiguity warning. +#[test] +fn unambiguous_diff_base_stays_quiet() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("clear.diff"); + std::fs::write( + &diff_path, + "diff --git a/packages/pkg/src/complex.js b/packages/pkg/src/complex.js\n\ + --- a/packages/pkg/src/complex.js\n\ + +++ b/packages/pkg/src/complex.js\n\ + @@ -9,0 +10,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let out = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + !out.stderr.contains("warning [diff-file]"), + "a diff that resolves under exactly one base must not warn: {}", + out.stderr + ); +} + +/// The review / sticky-summary filter matches issues against the diff. Its keys +/// must come from the analysis-root-relative path and the diff's own base, never +/// from the rendered path, or `--report-path-prefix` would silently decide which +/// inline comments survive. +#[test] +fn review_filter_is_independent_of_report_path_prefix() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("pr.diff"); + std::fs::write( + &diff_path, + "diff --git a/packages/pkg/src/complex.js b/packages/pkg/src/complex.js\n\ + --- a/packages/pkg/src/complex.js\n\ + +++ b/packages/pkg/src/complex.js\n\ + @@ -9,0 +10,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + let diff = diff_path.to_str().expect("utf8"); + let env = [("FALLOW_DIFF_FILE", diff), ("FALLOW_DIFF_FILTER", "file")]; + let base_args = [ + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + ]; + + let default = run_with_env(root, &base_args, &env); + let expected = review_comment_count(&default); + assert!( + expected > 0, + "fixture should produce an inline comment: {}", + default.stdout + ); + + for prefix in ["", "custom/base"] { + let mut args = base_args.to_vec(); + args.extend_from_slice(&["--report-path-prefix", prefix]); + let out = run_with_env(root, &args, &env); + assert_eq!( + review_comment_count(&out), + expected, + "--report-path-prefix {prefix:?} changed which comments survive the \ + diff filter; it must only change how paths are rendered" + ); + } +} + +/// ...while still rendering the prefix it was given. +#[test] +fn report_path_prefix_still_renders_on_review_paths() { + let dir = monorepo(); + let root = dir.path(); + let out = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + "--report-path-prefix", + "custom/base", + ], + ); + + let paths: Vec = parse_json(&out)["comments"] + .as_array() + .expect("comments") + .iter() + .map(|c| { + c["position"]["new_path"] + .as_str() + .expect("new_path") + .to_owned() + }) + .collect(); + assert!(!paths.is_empty(), "expected comments: {}", out.stdout); + for path in paths { + assert!(path.starts_with("custom/base/"), "got {path}"); + } +} + +/// Renamed files: GitLab needs `old_path` to place a discussion, and the diff's +/// rename pairs are keyed in the diff's namespace, not the rendered one. A +/// rendered-path lookup misses and `old_path` silently falls back to `new_path`, +/// telling GitLab a moved file never moved. +#[test] +fn renames_resolve_old_path_across_every_namespace() { + let dir = monorepo(); + let root = dir.path(); + git( + root, + &[ + "mv", + "packages/pkg/src/complex.js", + "packages/pkg/src/renamed.js", + ], + ); + let toplevel_diff = root.join("rename.diff"); + let out = Command::new("git") + .args(["diff", "-M", "HEAD"]) + .current_dir(root) + .output() + .expect("git diff"); + std::fs::write(&toplevel_diff, &out.stdout).expect("write diff"); + assert!( + String::from_utf8_lossy(&out.stdout).contains("rename to packages/pkg/src/renamed.js"), + "fixture diff should record the rename" + ); + + // The same rename, expressed the way `git diff --relative` would. + let relative_diff = root.join("rename_rel.diff"); + std::fs::write( + &relative_diff, + String::from_utf8_lossy(&out.stdout).replace("packages/pkg/", ""), + ) + .expect("write diff"); + + let old_path = |diff: &std::path::Path, extra: &[&str]| -> (String, String) { + let mut args = vec![ + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + "--diff-file", + diff.to_str().expect("utf8"), + ]; + args.extend_from_slice(extra); + let out = run_with_env(root, &args, &[("FALLOW_DIFF_FILTER", "file")]); + let position = &parse_json(&out)["comments"][0]["position"]; + ( + position["old_path"].as_str().unwrap_or_default().to_owned(), + position["new_path"].as_str().unwrap_or_default().to_owned(), + ) + }; + + for (label, diff, extra, want_old, want_new) in [ + ( + "toplevel-relative diff", + &toplevel_diff, + &[][..], + "packages/pkg/src/complex.js", + "packages/pkg/src/renamed.js", + ), + ( + "custom presentation prefix", + &toplevel_diff, + &["--report-path-prefix", "custom/base"][..], + "custom/base/src/complex.js", + "custom/base/src/renamed.js", + ), + ( + "root-relative diff", + &relative_diff, + &[][..], + "packages/pkg/src/complex.js", + "packages/pkg/src/renamed.js", + ), + ( + "rebasing disabled", + &toplevel_diff, + &["--report-path-prefix", ""][..], + "src/complex.js", + "src/renamed.js", + ), + ] { + let (old, new) = old_path(diff, extra); + assert_eq!(old, want_old, "{label}: wrong old_path"); + assert_eq!(new, want_new, "{label}: wrong new_path"); + assert_ne!( + old, new, + "{label}: old_path fell back to new_path, so the rename was lost" + ); + } +} + +/// The review renderer applies the presentation prefix itself. Any emitter that +/// also rebases before handing it CodeClimate issues prefixes twice AND keys the +/// diff filter in the wrong namespace. Combined mode and the subcommands reach +/// the renderer by different routes, so both are pinned. +#[test] +fn review_paths_are_prefixed_exactly_once_on_every_route() { + let dir = monorepo(); + let root = dir.path(); + let new_path = |args: &[&str]| -> String { + let out = run(root, args); + parse_json(&out)["comments"][0]["position"]["new_path"] + .as_str() + .unwrap_or_default() + .to_owned() + }; + + let combined = new_path(&[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + ]); + let dead_code = new_path(&[ + "dead-code", + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + ]); + let health = new_path(&[ + "health", + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + ]); + + for (route, path) in [ + ("combined", &combined), + ("dead-code", &dead_code), + ("health", &health), + ] { + assert!( + path.starts_with("packages/pkg/"), + "{route}: expected a repo-root-relative path, got {path}" + ); + assert!( + !path.contains("packages/pkg/packages/pkg/"), + "{route}: path was prefixed twice: {path}" + ); + } +} + +/// `--diff-file` must scope inline review comments, not just the analysis +/// results. Gating that filter on `$FALLOW_DIFF_FILE` left the flag rendering +/// every comment while claiming the diff had been applied. +#[test] +fn diff_file_flag_scopes_review_comments_without_the_env_var() { + let dir = monorepo(); + let root = dir.path(); + + let touching = root.join("touching.diff"); + std::fs::write( + &touching, + "diff --git a/packages/pkg/src/complex.js b/packages/pkg/src/complex.js\n\ + --- a/packages/pkg/src/complex.js\n\ + +++ b/packages/pkg/src/complex.js\n\ + @@ -9,0 +10,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let unrelated = root.join("unrelated.diff"); + std::fs::write( + &unrelated, + "diff --git a/packages/pkg/src/index.js b/packages/pkg/src/index.js\n\ + --- a/packages/pkg/src/index.js\n\ + +++ b/packages/pkg/src/index.js\n\ + @@ -1,0 +2,1 @@\n\ + +// unrelated\n", + ) + .expect("write diff"); + + let comments = |diff: &std::path::Path| -> usize { + let out = run_with_env( + root, + &[ + "dead-code", + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + "--diff-file", + diff.to_str().expect("utf8"), + ], + &[("FALLOW_DIFF_FILTER", "file")], + ); + review_comment_count(&out) + }; + + assert_eq!( + comments(&touching), + 1, + "a finding in a file the diff touches must survive" + ); + assert_eq!( + comments(&unrelated), + 0, + "--diff-file must scope review comments even with no FALLOW_DIFF_FILE" + ); +} + +/// An ambiguous base means fallow cannot express findings in the diff's +/// namespace. `check::filtering` sets the convention: retain what cannot be +/// filtered. Dropping every finding on a guess is the failure this whole change +/// removes, so the diff is discarded rather than the findings. +#[test] +fn ambiguous_diff_base_fails_open_and_retains_findings() { + let dir = monorepo(); + let root = dir.path(); + std::fs::create_dir_all(root.join("src")).expect("mkdir"); + std::fs::write(root.join("src/complex.js"), COMPLEX_JS).expect("write"); + git(root, &["add", "-A"]); + git(root, &["commit", "-qm", "top-level src"]); + + let diff_path = root.join("ambiguous.diff"); + std::fs::write( + &diff_path, + "diff --git a/src/complex.js b/src/complex.js\n\ + --- a/src/complex.js\n\ + +++ b/src/complex.js\n\ + @@ -9,0 +10,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let unfiltered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + ], + ); + let ambiguous = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + ambiguous.stderr.contains("ambiguous"), + "expected an ambiguity warning: {}", + ambiguous.stderr + ); + assert_eq!( + check_names(&ambiguous).len(), + check_names(&unfiltered).len(), + "an ambiguous base must retain findings, not filter them away" + ); +} diff --git a/crates/engine/src/health/filters.rs b/crates/engine/src/health/filters.rs index e530f1037..0c7814bd1 100644 --- a/crates/engine/src/health/filters.rs +++ b/crates/engine/src/health/filters.rs @@ -9,7 +9,6 @@ use fallow_output::{ use fallow_types::discover::DiscoveredFile; use rustc_hash::FxHashSet; -use super::runtime_filter::relative_to_root; use super::scoring; /// Drop complexity findings whose function body span does NOT overlap any @@ -19,15 +18,18 @@ use super::scoring; /// of zero collapses to `[line..=line]` so older fixture rows without /// extents do not silently match every diff. /// -/// Paths that cannot be expressed relative to `root` are retained rather than -/// silently dropped: surfacing an unfilterable path is better than hiding it. +/// Paths are keyed against the diff's own base (the repository toplevel for +/// `git diff` output), which differs from `root` whenever the analysis root +/// sits below it. Paths that cannot be expressed relative to that base are +/// retained rather than silently dropped: surfacing an unfilterable path is +/// better than hiding it. pub(super) fn filter_complexity_findings_by_diff( findings: &mut Vec, diff_index: &fallow_output::DiffIndex, root: &Path, ) { findings.retain(|finding| { - let Some(rel) = relative_to_root(&finding.path, root) else { + let Some(rel) = diff_index.key_for(&finding.path, root) else { return true; }; diff_index.range_overlaps_added( @@ -44,7 +46,7 @@ pub(super) fn filter_hotspots_by_diff( diff_index: &fallow_output::DiffIndex, root: &Path, ) { - hotspots.retain(|hotspot| match relative_to_root(&hotspot.path, root) { + hotspots.retain(|hotspot| match diff_index.key_for(&hotspot.path, root) { Some(rel) => diff_index.touches_file(&rel), None => true, }); @@ -56,7 +58,7 @@ pub(super) fn filter_refactoring_targets_by_diff( diff_index: &fallow_output::DiffIndex, root: &Path, ) { - targets.retain(|target| match relative_to_root(&target.path, root) { + targets.retain(|target| match diff_index.key_for(&target.path, root) { Some(rel) => diff_index.touches_file(&rel), None => true, }); @@ -70,7 +72,7 @@ pub(super) fn filter_large_functions_by_diff( root: &Path, ) { entries.retain(|entry| { - let Some(rel) = relative_to_root(&entry.path, root) else { + let Some(rel) = diff_index.key_for(&entry.path, root) else { return true; }; diff_index.range_overlaps_added( diff --git a/crates/output/src/ci_output.rs b/crates/output/src/ci_output.rs index 61e14634a..dbea9713b 100644 --- a/crates/output/src/ci_output.rs +++ b/crates/output/src/ci_output.rs @@ -1,5 +1,6 @@ //! Shared CI comment output contracts for CLI and programmatic consumers. +use std::borrow::Cow; use std::fmt::Write as _; use crate::{ @@ -28,6 +29,21 @@ impl CiProvider { } } +/// Prefix prepended to a rendered path so CI platforms, which address files +/// from the repository root, can find it. Empty when the analysis root already +/// is the repository root. +/// +/// This is presentation only. Nothing looks a path up in a diff after it has +/// been prefixed: matching happens on analysis-root-relative paths, which is +/// the namespace `DiffIndex::key_for_root_relative` translates from. +#[must_use] +pub fn apply_path_prefix(prefix: &str, path: &str) -> String { + if prefix.is_empty() { + return path.to_owned(); + } + format!("{prefix}/{path}") +} + /// Normalized CodeClimate issue used by CI comment renderers. #[derive(Clone, Debug, PartialEq, Eq)] pub struct CiIssue { @@ -77,6 +93,8 @@ pub struct ReviewEnvelopeRenderInput<'a> { pub provider: CiProvider, pub issues: &'a [CiIssue], pub diff_index: Option<&'a DiffIndex>, + /// Prepended to every emitted path after diff lookups have run. + pub path_prefix: &'a str, pub max_comments: usize, pub gitlab_diff_refs: Option<&'a ReviewGitlabDiffRefs>, pub include_guidance: bool, @@ -357,6 +375,7 @@ pub fn render_review_envelope(input: &ReviewEnvelopeRenderInput<'_>) -> ReviewEn group, gitlab_diff_refs: input.gitlab_diff_refs, diff_index: input.diff_index, + path_prefix: input.path_prefix, include_guidance: input.include_guidance, suggestion_block: input.suggestion_block, guidance_block: input.guidance_block, @@ -475,6 +494,8 @@ pub struct ReviewCommentRenderInput<'a, 'group> { pub group: &'a [&'group CiIssue], pub gitlab_diff_refs: Option<&'a ReviewGitlabDiffRefs>, pub diff_index: Option<&'a DiffIndex>, + /// Prepended to every emitted path after diff lookups have run. + pub path_prefix: &'a str, pub include_guidance: bool, pub suggestion_block: &'a dyn Fn(CiProvider, &CiIssue) -> Option, pub guidance_block: &'a dyn Fn(&CiIssue) -> Option, @@ -504,6 +525,7 @@ pub fn render_review_comment_for_group(input: &ReviewCommentRenderInput<'_, '_>) representative, gitlab_diff_refs: input.gitlab_diff_refs, diff_index: input.diff_index, + path_prefix: input.path_prefix, body, fingerprint, truncated, @@ -543,6 +565,7 @@ struct ReviewCommentInput<'a> { representative: &'a CiIssue, gitlab_diff_refs: Option<&'a ReviewGitlabDiffRefs>, diff_index: Option<&'a DiffIndex>, + path_prefix: &'a str, body: String, fingerprint: String, truncated: bool, @@ -554,13 +577,14 @@ fn build_review_comment(input: ReviewCommentInput<'_>) -> ReviewComment { representative, gitlab_diff_refs, diff_index, + path_prefix, body, fingerprint, truncated, } = input; match provider { CiProvider::Github => ReviewComment::GitHub(GitHubReviewComment { - path: representative.path.clone(), + path: apply_path_prefix(path_prefix, &representative.path), line: u32::try_from(representative.line).unwrap_or(u32::MAX), side: GitHubReviewSide::Right, body, @@ -568,10 +592,13 @@ fn build_review_comment(input: ReviewCommentInput<'_>) -> ReviewComment { truncated, }), CiProvider::Gitlab => { - let new_path = representative.path.clone(); - let old_path = diff_index - .and_then(|di| di.old_path_for(&new_path)) - .map_or_else(|| new_path.clone(), str::to_owned); + // Renames resolve on the analysis-root-relative path, before the + // presentation prefix goes on: the diff's keys never carry it. + let old_rel = diff_index + .and_then(|di| di.old_path_for_root_relative(&representative.path)) + .map_or_else(|| representative.path.clone(), Cow::into_owned); + let new_path = apply_path_prefix(path_prefix, &representative.path); + let old_path = apply_path_prefix(path_prefix, &old_rel); let position = GitLabReviewPosition { base_sha: gitlab_diff_refs.map(|r| r.base_sha.clone()), start_sha: gitlab_diff_refs.map(|r| r.start_sha.clone()), diff --git a/crates/output/src/diff.rs b/crates/output/src/diff.rs index 50cd26e92..a2dad9aad 100644 --- a/crates/output/src/diff.rs +++ b/crates/output/src/diff.rs @@ -1,4 +1,5 @@ -use std::path::Path; +use std::borrow::Cow; +use std::path::{Path, PathBuf}; use rustc_hash::{FxHashMap, FxHashSet}; @@ -9,12 +10,21 @@ pub const MAX_DIFF_BYTES: u64 = 10 * 1024 * 1024; pub const MAX_ADDED_LINES: usize = 1_000_000; /// Parsed, command-neutral index of files and added lines in a unified diff. +/// +/// Keys are exactly the paths the diff names in its `+++ b/` headers, +/// so they live in whatever namespace produced the diff — for `git diff`, +/// relative to the repository toplevel. [`DiffIndex::base`] records the +/// directory those keys are relative to, so a finding's absolute path can be +/// mapped into the same namespace before lookup. Without it, an analysis root +/// below the toplevel silently misses every key. #[derive(Debug, Default, Clone)] pub struct DiffIndex { added_lines: FxHashMap>, touched_files: FxHashSet, added_line_count: usize, rename_pairs: FxHashMap, + base: Option, + root_offset: String, } /// Mutable cursor state threaded through unified-diff parsing. @@ -138,6 +148,83 @@ impl DiffIndex { pub fn added_lines_in(&self, path: &str) -> Option<&FxHashSet> { self.added_lines.get(path) } + + /// Declare the directory this diff's paths are relative to (the git + /// toplevel for `git diff` output). + #[must_use] + pub fn with_base(mut self, base: impl Into) -> Self { + self.base = Some(base.into()); + self + } + + /// Declare where the analysis root sits below [`DiffIndex::base`], as a + /// forward-slashed relative path (empty when they are the same directory). + /// + /// Findings are addressed relative to the analysis root; this diff's keys + /// are relative to its base. Everything that looks a finding up in this + /// index has to cross that gap, so the index carries the offset rather than + /// making each caller rediscover it. + #[must_use] + pub fn with_root_offset(mut self, offset: impl Into) -> Self { + let mut offset = offset.into(); + // A trailing separator would make `strip_path_component_prefix` demand a + // second one and never match. Normalize rather than trust the caller. + offset.truncate(offset.trim_end_matches('/').len()); + self.root_offset = offset; + self + } + + #[must_use] + pub fn root_offset(&self) -> &str { + &self.root_offset + } + + /// Lift an analysis-root-relative path into this diff's key namespace. + #[must_use] + pub fn key_for_root_relative<'a>(&self, rel: &'a str) -> Cow<'a, str> { + if self.root_offset.is_empty() { + return Cow::Borrowed(rel); + } + Cow::Owned(format!("{}/{rel}", self.root_offset)) + } + + /// Lower one of this diff's keys back to an analysis-root-relative path. + /// `None` when the key names a file outside the analysis root. + #[must_use] + pub fn root_relative_from_key<'a>(&self, key: &'a str) -> Option> { + if self.root_offset.is_empty() { + return Some(Cow::Borrowed(key)); + } + strip_path_component_prefix(key, &self.root_offset).map(Cow::Borrowed) + } + + /// The pre-rename path of an analysis-root-relative path, itself + /// analysis-root-relative. Crosses into the diff's key namespace and back, + /// so a monorepo package below the diff's base resolves its renames. + #[must_use] + pub fn old_path_for_root_relative<'a>(&'a self, rel: &str) -> Option> { + let old = self.old_path_for(&self.key_for_root_relative(rel))?; + self.root_relative_from_key(old) + } + + #[must_use] + pub fn base(&self) -> Option<&Path> { + self.base.as_deref() + } + + pub fn touched_files(&self) -> impl Iterator { + self.touched_files.iter().map(String::as_str) + } + + /// Map a finding's path into this diff's key namespace. + /// + /// Relativizes against [`DiffIndex::base`] when one was declared, else + /// against `fallback_root`. When base == `fallback_root` (the analysis + /// root is the repository toplevel) both agree, so behavior is unchanged. + #[must_use] + pub fn key_for(&self, path: &Path, fallback_root: &Path) -> Option { + relative_to_diff_path(path, self.base.as_deref().unwrap_or(fallback_root)) + } } #[must_use] @@ -151,6 +238,14 @@ pub fn relative_to_diff_path(path: &Path, root: &Path) -> Option { Some(path.to_string_lossy().replace('\\', "/")) } +/// Strip `prefix` and its trailing separator, only on a path-component +/// boundary, so `packages/pkg-extra/a.ts` is never read as `packages/pkg` +/// plus `-extra/a.ts`. +#[must_use] +pub fn strip_path_component_prefix<'a>(path: &'a str, prefix: &str) -> Option<&'a str> { + path.strip_prefix(prefix)?.strip_prefix('/') +} + pub fn parse_new_hunk_start(header: &str) -> Option { let plus = header.find('+')?; let rest = &header[plus + 1..]; @@ -266,4 +361,147 @@ diff --git a/src/a.ts b/src/a.ts let path = Path::new("/elsewhere/src/a.ts"); assert!(relative_to_diff_path(path, root).is_none()); } + + #[test] + fn key_for_without_base_relativizes_against_the_fallback_root() { + let index = DiffIndex::default(); + assert_eq!( + index + .key_for(Path::new("/repo/pkg/src/a.ts"), Path::new("/repo/pkg")) + .as_deref(), + Some("src/a.ts") + ); + } + + #[test] + fn key_for_with_base_equal_to_root_is_unchanged() { + let index = DiffIndex::default().with_base("/repo"); + assert_eq!( + index + .key_for(Path::new("/repo/src/a.ts"), Path::new("/repo")) + .as_deref(), + Some("src/a.ts") + ); + } + + /// The regression: an analysis root below the repo toplevel must still + /// produce the toplevel-relative key `git diff` writes. + #[test] + fn key_for_with_base_above_root_yields_repo_root_relative_key() { + let index = DiffIndex::default().with_base("/repo"); + assert_eq!( + index + .key_for(Path::new("/repo/pkg/src/a.ts"), Path::new("/repo/pkg")) + .as_deref(), + Some("pkg/src/a.ts") + ); + } + + #[test] + fn key_for_with_base_above_root_matches_a_repo_root_relative_diff() { + let diff = "\ +diff --git a/pkg/src/a.ts b/pkg/src/a.ts +--- a/pkg/src/a.ts ++++ b/pkg/src/a.ts +@@ -1,0 +2,1 @@ ++added +"; + let index = DiffIndex::from_unified_diff(diff).with_base("/repo"); + let key = index + .key_for(Path::new("/repo/pkg/src/a.ts"), Path::new("/repo/pkg")) + .expect("finding path is under the base"); + + assert!(index.touches_file(&key)); + assert!(index.line_is_added(&key, 2)); + + // Without the base, the same finding keys as `src/a.ts` and misses. + let unbased = DiffIndex::from_unified_diff(diff); + let missed = unbased + .key_for(Path::new("/repo/pkg/src/a.ts"), Path::new("/repo/pkg")) + .expect("still relativizable"); + assert_eq!(missed, "src/a.ts"); + assert!(!unbased.touches_file(&missed)); + } + + #[test] + fn key_for_returns_none_for_path_outside_the_base() { + let index = DiffIndex::default().with_base("/repo"); + assert!( + index + .key_for(Path::new("/elsewhere/a.ts"), Path::new("/repo/pkg")) + .is_none() + ); + } + + #[test] + fn old_path_for_root_relative_crosses_the_namespace_and_back() { + let diff = "\ +diff --git a/pkg/src/old.ts b/pkg/src/new.ts +similarity index 90% +rename from pkg/src/old.ts +rename to pkg/src/new.ts +--- a/pkg/src/old.ts ++++ b/pkg/src/new.ts +@@ -1,1 +1,1 @@ +-old ++new +"; + let index = DiffIndex::from_unified_diff(diff) + .with_base("/repo") + .with_root_offset("pkg"); + + // The finding is addressed `src/new.ts`; the diff says `pkg/src/new.ts`. + assert_eq!( + index.old_path_for_root_relative("src/new.ts").as_deref(), + Some("src/old.ts") + ); + // The raw lookup, in the diff's own namespace, still works. + assert_eq!(index.old_path_for("pkg/src/new.ts"), Some("pkg/src/old.ts")); + assert_eq!(index.old_path_for_root_relative("src/absent.ts"), None); + } + + #[test] + fn root_relative_key_round_trips() { + let index = DiffIndex::default().with_root_offset("packages/pkg"); + assert_eq!( + index.key_for_root_relative("src/a.ts"), + "packages/pkg/src/a.ts" + ); + assert_eq!( + index + .root_relative_from_key("packages/pkg/src/a.ts") + .as_deref(), + Some("src/a.ts") + ); + // A key outside the analysis root has no root-relative form. + assert_eq!(index.root_relative_from_key("other/src/a.ts"), None); + // Sibling directory sharing a name prefix is not a match. + assert_eq!( + index.root_relative_from_key("packages/pkg-extra/a.ts"), + None + ); + } + + #[test] + fn empty_root_offset_is_identity() { + let index = DiffIndex::default(); + assert_eq!(index.key_for_root_relative("src/a.ts"), "src/a.ts"); + assert_eq!( + index.root_relative_from_key("src/a.ts").as_deref(), + Some("src/a.ts") + ); + } + + #[test] + fn touched_files_enumerates_diff_header_paths() { + let diff = "\ +diff --git a/pkg/a.ts b/pkg/a.ts +--- a/pkg/a.ts ++++ b/pkg/a.ts +@@ -0,0 +1,1 @@ ++x +"; + let index = DiffIndex::from_unified_diff(diff); + assert_eq!(index.touched_files().collect::>(), vec!["pkg/a.ts"]); + } } diff --git a/crates/output/src/lib.rs b/crates/output/src/lib.rs index be59e9205..6535fbdd0 100644 --- a/crates/output/src/lib.rs +++ b/crates/output/src/lib.rs @@ -108,11 +108,11 @@ pub use ci_output::{ CiIssue, CiProvider, GroupedReviewIssues, MARKER_PREFIX_V2, MARKER_SUFFIX_V2, MAX_COMMENT_BODY_BYTES, PROJECT_LEVEL_RULE_IDS, PrCommentRenderInput, ReviewCommentRenderInput, ReviewEnvelopeRenderInput, ReviewEnvelopeRenderResult, ReviewEnvelopeTruncation, - ReviewGitlabDiffRefs, cap_body_with_marker, command_title, composite_fingerprint, escape_md, - github_check_conclusion, group_review_issues_by_path_line, is_project_level_rule, - issues_from_codeclimate, issues_from_codeclimate_issues, render_pr_comment, - render_review_comment_for_group, render_review_envelope, review_label_from_codeclimate, - summary_fingerprint, summary_label, + ReviewGitlabDiffRefs, apply_path_prefix, cap_body_with_marker, command_title, + composite_fingerprint, escape_md, github_check_conclusion, group_review_issues_by_path_line, + is_project_level_rule, issues_from_codeclimate, issues_from_codeclimate_issues, + render_pr_comment, render_review_comment_for_group, render_review_envelope, + review_label_from_codeclimate, summary_fingerprint, summary_label, }; pub use codeclimate::{ CodeClimateAnnotationField, CodeClimateIssue, CodeClimateIssueInput, CodeClimateIssueKind, @@ -130,6 +130,7 @@ pub use coverage_envelopes::{ pub use dead_code_sarif::build_dead_code_sarif; pub use diff::{ DiffIndex, MAX_ADDED_LINES, MAX_DIFF_BYTES, parse_new_hunk_start, relative_to_diff_path, + strip_path_component_prefix, }; pub use dupes::{ CloneFamilyAction, CloneFamilyActionType, CloneGroupAction, CloneGroupActionType, diff --git a/npm/fallow/capabilities.json b/npm/fallow/capabilities.json index e9d3a91b8..2aafc6be2 100644 --- a/npm/fallow/capabilities.json +++ b/npm/fallow/capabilities.json @@ -279,10 +279,10 @@ "short": "-o" }, { - "name": "--annotations-path-prefix", + "name": "--report-path-prefix", "type": "string", "required": false, - "description": "Prefix prepended to every `file=` path in `--format github-annotations` output. GitHub resolves annotation paths against the repository root, so when the analyzed project lives in a subdirectory (e.g. `packages/app/`), paths need that offset. fallow detects the offset via the git toplevel automatically; this flag overrides the detection. Valid only with the GitHub-native formats" + "description": "Prefix prepended to every path in the CI-facing formats (`github-annotations`, `github-summary`, `codeclimate`, `review-github`, `review-gitlab`). CI platforms address files by repository-root-relative path, so when the analyzed project lives in a subdirectory (e.g. `packages/app/`), paths need that offset. fallow detects the offset via the git toplevel automatically; this flag overrides the detection. Pass an empty string to disable rebasing and emit paths relative to `--root`" }, { "name": "--fail-on-regression", diff --git a/npm/fallow/skills/fallow/references/cli-reference.md b/npm/fallow/skills/fallow/references/cli-reference.md index f7c1f548a..a6b67466e 100644 --- a/npm/fallow/skills/fallow/references/cli-reference.md +++ b/npm/fallow/skills/fallow/references/cli-reference.md @@ -1661,7 +1661,7 @@ Available on all commands: | `--fail-on-issues` | `bool` | `false` | Exit 1 if any issues found (promotes `warn` to `error`) | | `--sarif-file` | `string` | - | Write SARIF output to a file instead of stdout | | `-o, --output-file` | `string` | - | Write the report to a file instead of stdout, for any --format (no ANSI codes). Useful on large projects where the terminal scrollback truncates the top. Progress and the confirmation stay on stderr | -| `--annotations-path-prefix` | `string` | - | Prefix prepended to every `file=` path in `--format github-annotations` output. GitHub resolves annotation paths against the repository root, so when the analyzed project lives in a subdirectory (e.g. `packages/app/`), paths need that offset. fallow detects the offset via the git toplevel automatically; this flag overrides the detection. Valid only with the GitHub-native formats | +| `--report-path-prefix` | `string` | - | Prefix prepended to every path in the CI-facing formats (`github-annotations`, `github-summary`, `codeclimate`, `review-github`, `review-gitlab`). CI platforms address files by repository-root-relative path, so when the analyzed project lives in a subdirectory (e.g. `packages/app/`), paths need that offset. fallow detects the offset via the git toplevel automatically; this flag overrides the detection. Pass an empty string to disable rebasing and emit paths relative to `--root` | | `--fail-on-regression` | `bool` | `false` | Fail if issue count increased beyond tolerance vs a regression baseline | | `--tolerance` | `string` | `0` | Allowed increase: `"2%"` (percentage) or `"5"` (absolute). Default: `"0"` | | `--regression-baseline` | `string` | - | Path to regression baseline file (default: `.fallow/regression-baseline.json`) | From 95f9862cd391931ec93fcf340b9f265ac50510be Mon Sep 17 00:00:00 2001 From: Bart Waardenburg Date: Sun, 12 Jul 2026 23:22:53 +0200 Subject: [PATCH 2/3] fix(report): filter parsed diffs with no analyzable files to zero scope A diff that parses but names no analyzable head-side file (empty, deletion-only, or binary-only) is a real empty scope, not an unplaceable base. init_shared_diff conflated the two: choose_diff_base returned None for both, so such diffs were discarded and the run reported at full scope, flipping a change that touches nothing analyzable from clean to noisy (including when --root equals the git toplevel). Split the two: touched_files being empty keeps the empty index so every source-anchored finding filters out; only a diff whose paths resolve nowhere (foreign) or in two places at once (ambiguous) still falls open. Make the discard decision authoritative for the FALLOW_DIFF_FILE path too: filter_issues_from_env now distinguishes a resolved cache, a deliberately-discarded cache (full scope), and an unset cache (embedder fallback), so the env route no longer re-reads and re-filters against a diff base detection already rejected. Align the ambiguous- and foreign-base warnings with the fail-open behavior: they report at full scope, not a clean report. --- crates/cli/src/report/ci/diff_filter.rs | 100 +++++--- .../cli/tests/monorepo_report_paths_tests.rs | 229 ++++++++++++++++++ 2 files changed, 292 insertions(+), 37 deletions(-) diff --git a/crates/cli/src/report/ci/diff_filter.rs b/crates/cli/src/report/ci/diff_filter.rs index 39c96b36b..1184e0c46 100644 --- a/crates/cli/src/report/ci/diff_filter.rs +++ b/crates/cli/src/report/ci/diff_filter.rs @@ -278,14 +278,27 @@ pub fn init_shared_diff( let loaded = source .and_then(|src| load_diff_index_for_findings(src, quiet)) .and_then(|loaded| { + // A diff that parsed but names no analyzable head-side file (empty, + // deletion-only, or binary-only) changed nothing a finding can be + // attributed to. That is a real, EMPTY scope, not an unplaceable + // base: keep the empty index so every source-anchored finding + // filters out (report clean) rather than falling open to full scope. + // Only a diff we cannot place (foreign or ambiguous base) falls open. + // The empty index needs no base: with no keys every lookup misses, + // and `key_for` still yields a key for in-root paths, so findings are + // dropped rather than retained. + if loaded.index.touched_files().next().is_none() { + return Some(loaded); + } let label = source.map(DiffSource::label).unwrap_or_default(); let chosen = choose_diff_base(&loaded.index, candidate_bases); match chosen { - // The diff names nothing we can find, or names it in two places - // at once. Either way we cannot express findings in its - // namespace. `check::filtering` sets the convention for that: an - // unfilterable path is RETAINED, never silently dropped. So drop - // the diff instead of the findings and report at full scope. + // The diff names files, but none under any candidate base + // (foreign), or equally under two at once (ambiguous). Either way + // we cannot express findings in its namespace. `check::filtering` + // sets the convention for that: an unfilterable path is RETAINED, + // never silently dropped. So drop the diff instead of the findings + // and report at full scope. None => { if !quiet { warn_on_foreign_diff_namespace(&loaded.index, candidate_bases, &label); @@ -294,7 +307,7 @@ pub fn init_shared_diff( } Some(chosen) if chosen.ambiguous => { if !quiet { - warn_on_ambiguous_diff_base(&chosen.base, candidate_bases, &label); + warn_on_ambiguous_diff_base(candidate_bases, &label); } None } @@ -372,31 +385,29 @@ fn choose_diff_base(index: &DiffIndex, candidate_bases: &[PathBuf]) -> Option>() - .join(", "); + .join(" and "); eprintln!( "fallow: warning [diff-file]: the paths in {label} name existing files under \ - both {} and {others}, so their base is ambiguous; filtering against {}. \ - If that is wrong, no source-anchored finding will match and the report will \ - look clean. Generate the diff from the repository root (plain `git diff`, \ - not `git diff --relative`) to remove the ambiguity.", - chosen.display(), - chosen.display() + {bases}, so their base is ambiguous and fallow cannot tell which one the diff \ + is relative to. It will not filter against a guess: every finding is reported \ + (full scope, not scoped to the diff). Generate the diff from the repository \ + root (plain `git diff`, not `git diff --relative`) to scope the report." ); } /// A diff whose paths name no file under any candidate base was almost -/// certainly generated relative to some other directory. Every finding would -/// then miss every key and the run would report a clean diff. Say so, once, -/// rather than emitting a plausible-looking empty report. +/// certainly generated relative to some other directory. fallow cannot place it, +/// so it discards the diff and reports at full scope. Say so, once, rather than +/// let the unscoped report imply the diff was applied. fn warn_on_foreign_diff_namespace(index: &DiffIndex, candidate_bases: &[PathBuf], label: &str) { let total = index.touched_files().count(); if total == 0 { @@ -409,9 +420,9 @@ fn warn_on_foreign_diff_namespace(index: &DiffIndex, candidate_bases: &[PathBuf] .join(", "); eprintln!( "fallow: warning [diff-file]: none of the {total} file(s) named by {label} exist \ - under {bases}; the diff's paths look relative to a different directory. \ - Source-anchored findings will not match it and the report will look clean. \ - Regenerate the diff from one of those directories." + under {bases}; the diff's paths look relative to a different directory. fallow \ + cannot place the diff, so every finding is reported (full scope, not scoped to \ + the diff). Regenerate the diff from one of those directories to scope the report." ); } @@ -445,29 +456,44 @@ fn context_radius_from_env() -> u64 { /// Filter issues against this run's diff. /// -/// Gated on the shared index, not on `$FALLOW_DIFF_FILE`: `--diff-file` takes -/// precedence when resolving that index, so gating on the env var would leave +/// Gated on the shared cache, not on `$FALLOW_DIFF_FILE`: `--diff-file` takes +/// precedence when resolving that cache, so gating on the env var would leave /// `--diff-file --format review-gitlab` rendering unfiltered comments, and /// would filter against the flag's diff while claiming to honour the env var's. /// The shared index also carries the base its paths were written against; /// re-parsing here would yield an unbased index whose every lookup misses for /// an analysis root below that base. +/// +/// The three cache states are distinct and must stay so. When `init_shared_diff` +/// discarded the diff (unplaceable base), that full-scope decision is +/// authoritative here too: re-reading the env var would re-filter and contradict +/// it. The env-var fallback is only for the case where `init_shared_diff` never +/// ran (an embedder or a test), so those callers keep working. #[must_use] pub fn filter_issues_from_env(issues: Vec) -> Vec { let mode = DiffFilterMode::from_env(); let radius = context_radius_from_env(); - if let Some(index) = shared_diff_index() { - return issues + match SHARED_DIFF.get() { + // A diff was resolved for this run (a placed base, or a parsed-but-empty + // scope). Filter against it; an empty-scope index drops every + // source-anchored issue, matching the finding filter. + Some(Some(loaded)) => issues .into_iter() - .filter(|issue| diff_index_keeps_issue(index, issue, mode, radius)) - .collect(); + .filter(|issue| diff_index_keeps_issue(&loaded.index, issue, mode, radius)) + .collect(), + // `init_shared_diff` ran and deliberately discarded the diff (foreign or + // ambiguous base): report at full scope, the same decision the finding + // filter made. Re-reading FALLOW_DIFF_FILE here would contradict it. + Some(None) => issues, + // `init_shared_diff` never ran: an embedder or a test, not a CLI run. + // Honour the env var directly so those callers keep working. + None => { + let Some(raw_path) = std::env::var_os("FALLOW_DIFF_FILE") else { + return issues; + }; + filter_issues_from_path(issues, Path::new(&raw_path), mode, radius) + } } - // No shared index: this is an embedder or a test, not a CLI run. Honour the - // env var directly so those callers keep working. - let Some(raw_path) = std::env::var_os("FALLOW_DIFF_FILE") else { - return issues; - }; - filter_issues_from_path(issues, Path::new(&raw_path), mode, radius) } /// Filter for the typed PR-comment renderer (`print_pr_comment`). diff --git a/crates/cli/tests/monorepo_report_paths_tests.rs b/crates/cli/tests/monorepo_report_paths_tests.rs index f6e09ad7d..93c0a3f37 100644 --- a/crates/cli/tests/monorepo_report_paths_tests.rs +++ b/crates/cli/tests/monorepo_report_paths_tests.rs @@ -893,3 +893,232 @@ fn ambiguous_diff_base_fails_open_and_retains_findings() { "an ambiguous base must retain findings, not filter them away" ); } + +/// A diff that PARSED but names no analyzable head-side file (deletion-only) +/// changed nothing a finding can be attributed to. That is a real, EMPTY scope, +/// not an unplaceable base: it must filter to zero, never fall open to full +/// scope. The original defect reported such a run at full scope. +#[test] +fn deletion_only_diff_filters_to_zero_not_full_scope() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("deletion.diff"); + std::fs::write( + &diff_path, + "diff --git a/packages/pkg/src/removed.js b/packages/pkg/src/removed.js\n\ + deleted file mode 100644\n\ + --- a/packages/pkg/src/removed.js\n\ + +++ /dev/null\n\ + @@ -1,3 +0,0 @@\n\ + -one\n\ + -two\n\ + -three\n", + ) + .expect("write diff"); + + let unfiltered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + ], + ); + let filtered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + !check_names(&unfiltered).is_empty(), + "fixture must produce findings at full scope: {}", + unfiltered.stdout + ); + assert!( + check_names(&filtered).is_empty(), + "a deletion-only diff analyzes no head-side file, so every \ + source-anchored finding must filter out; got {:?}", + check_names(&filtered) + ); + assert!( + !filtered + .stderr + .contains("relative to a different directory") + && !filtered.stderr.contains("ambiguous"), + "an empty-scope diff is not foreign or ambiguous; got stderr: {}", + filtered.stderr + ); +} + +/// An empty diff (nothing staged) is the same empty scope: no head-side file +/// changed, so it filters to zero rather than falling open. +#[test] +fn empty_diff_filters_to_zero_not_full_scope() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("empty.diff"); + std::fs::write(&diff_path, "").expect("write diff"); + + let filtered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + check_names(&filtered).is_empty(), + "an empty diff analyzes nothing and must filter to zero; got {:?}", + check_names(&filtered) + ); +} + +/// A binary-only diff names no `+++ b/` head-side text file, so it is the +/// same empty scope and filters to zero. +#[test] +fn binary_only_diff_filters_to_zero_not_full_scope() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("binary.diff"); + std::fs::write( + &diff_path, + "diff --git a/packages/pkg/logo.png b/packages/pkg/logo.png\n\ + index 0000000..1111111 100644\n\ + Binary files a/packages/pkg/logo.png and b/packages/pkg/logo.png differ\n", + ) + .expect("write diff"); + + let filtered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert!( + check_names(&filtered).is_empty(), + "a binary-only diff touches no analyzable file and must filter to \ + zero; got {:?}", + check_names(&filtered) + ); +} + +/// The split's other half: a diff that names files but places them nowhere +/// (foreign base) is UNATTRIBUTABLE, not empty. It must fall open to full scope, +/// never collapse to the empty-scope zero. This is the case the empty-scope path +/// must not swallow. +#[test] +fn foreign_namespace_diff_fails_open_to_full_scope() { + let dir = monorepo(); + let root = dir.path(); + let diff_path = root.join("foreign.diff"); + std::fs::write( + &diff_path, + "diff --git a/nowhere/ghost.js b/nowhere/ghost.js\n\ + --- a/nowhere/ghost.js\n\ + +++ b/nowhere/ghost.js\n\ + @@ -0,0 +1,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let unfiltered = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + ], + ); + let foreign = run( + root, + &[ + "--root", + "packages/pkg", + "--quiet", + "--format", + "codeclimate", + "--diff-file", + diff_path.to_str().expect("utf8"), + ], + ); + + assert_eq!( + check_names(&foreign).len(), + check_names(&unfiltered).len(), + "a foreign (unattributable) diff must retain findings at full scope, \ + never collapse to the empty-scope zero" + ); +} + +/// The FALLOW_DIFF_FILE env path (the GitHub Action's standard route) must +/// honour the discard decision. When base detection discards an unplaceable +/// (foreign) diff and reports at full scope, the env-var comment filter must not +/// re-read the file and re-filter, which would drop every comment and disagree +/// with the full-scope decision the finding filter made. +#[test] +fn env_diff_file_honours_discard_and_keeps_comments_at_full_scope() { + let dir = monorepo(); + let root = dir.path(); + let foreign = root.join("foreign.diff"); + std::fs::write( + &foreign, + "diff --git a/nowhere/ghost.js b/nowhere/ghost.js\n\ + --- a/nowhere/ghost.js\n\ + +++ b/nowhere/ghost.js\n\ + @@ -0,0 +1,1 @@\n\ + + const touched = 1\n", + ) + .expect("write diff"); + + let base_args = [ + "--root", + "packages/pkg", + "--quiet", + "--format", + "review-gitlab", + ]; + let no_diff = run(root, &base_args); + let expected = review_comment_count(&no_diff); + assert!( + expected > 0, + "fixture should produce comments: {}", + no_diff.stdout + ); + + let with_env = run_with_env( + root, + &base_args, + &[("FALLOW_DIFF_FILE", foreign.to_str().expect("utf8"))], + ); + assert_eq!( + review_comment_count(&with_env), + expected, + "a discarded (foreign) diff reports at full scope; the FALLOW_DIFF_FILE \ + comment filter must not re-filter and drop comments" + ); +} From 4b6c53493467766ad8e4e01625fddd59739e2386 Mon Sep 17 00:00:00 2001 From: Bart Waardenburg Date: Sun, 12 Jul 2026 23:23:01 +0200 Subject: [PATCH 3/3] perf(report): resolve CI path prefix only for formats that use it init_report_prefix shells out to git rev-parse --show-toplevel on every run, but only codeclimate and review-{github,gitlab} read the resolved global (report_prefix()); the github-native formats compute their rebase from root directly. Gate the probe on those three formats so the other formats skip the subprocess. --- crates/cli/src/cli_startup.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/crates/cli/src/cli_startup.rs b/crates/cli/src/cli_startup.rs index cdfc6bef9..10e9ac0f3 100644 --- a/crates/cli/src/cli_startup.rs +++ b/crates/cli/src/cli_startup.rs @@ -340,7 +340,20 @@ pub fn run_pre_dispatch_checks( return Err(fail(code, telemetry::FailureReason::Validation)); } report::github::set_report_path_prefix(cli.report_path_prefix.clone()); - report::github::init_report_prefix(root); + // `init_report_prefix` shells out to `git rev-parse --show-toplevel`. Only + // the formats that read the resolved global (`report_prefix()`) need it: + // codeclimate applies it at the wire boundary, and review-{github,gitlab} + // apply it in the renderer, which has no `root` to re-derive it from. The + // github-native formats compute their rebase from `root` directly, so skip + // the probe for every other format. + if matches!( + output, + fallow_config::OutputFormat::CodeClimate + | fallow_config::OutputFormat::ReviewGithub + | fallow_config::OutputFormat::ReviewGitlab + ) { + report::github::init_report_prefix(root); + } parse_cli_tolerance(cli, output) .map_err(|code| fail(code, telemetry::FailureReason::Validation))