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..10e9ac0f3 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,40 @@ 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()); + // `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)) @@ -464,6 +484,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 +549,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..1184e0c46 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,163 @@ 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| { + // 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 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); + } + None + } + Some(chosen) if chosen.ambiguous => { + if !quiet { + warn_on_ambiguous_diff_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 +/// existence alone cannot place its base. Rather than filter against a guess +/// (whose wrong half drops every source-anchored finding), the run discards the +/// diff and reports at full scope, so the message names the ambiguity and says +/// so rather than letting silence imply the report was scoped. +fn warn_on_ambiguous_diff_base(candidate_bases: &[PathBuf], label: &str) { + let bases = candidate_bases + .iter() + .map(|base| base.display().to_string()) + .collect::>() + .join(" and "); + eprintln!( + "fallow: warning [diff-file]: the paths in {label} name existing files under \ + {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. 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 { + 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. 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." + ); +} + /// 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 +454,46 @@ fn context_radius_from_env() -> u64 { .unwrap_or(3) } +/// Filter issues against this run's diff. +/// +/// 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 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(), - ) + let mode = DiffFilterMode::from_env(); + let radius = context_radius_from_env(); + 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(&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) + } + } } /// Filter for the typed PR-comment renderer (`print_pr_comment`). @@ -409,13 +589,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 +605,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..93c0a3f37 --- /dev/null +++ b/crates/cli/tests/monorepo_report_paths_tests.rs @@ -0,0 +1,1124 @@ +#![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" + ); +} + +/// 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" + ); +} 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`) |