From 55245f4eb7dd434fde12dbc0dc2106fb7a54f6c2 Mon Sep 17 00:00:00 2001 From: Hesham Salman Date: Wed, 20 May 2026 16:14:50 -0700 Subject: [PATCH] fix: recompute filtered review summaries --- crates/inspect-cli/src/commands/diff.rs | 4 +- crates/inspect-cli/src/commands/file.rs | 104 +++++++++++- crates/inspect-cli/src/commands/pr.rs | 9 +- crates/inspect-core/src/analyze.rs | 214 ++++++++++++++++++++++++ crates/inspect-core/src/types.rs | 3 + 5 files changed, 320 insertions(+), 14 deletions(-) diff --git a/crates/inspect-cli/src/commands/diff.rs b/crates/inspect-cli/src/commands/diff.rs index 4054ee1..f66bdd7 100644 --- a/crates/inspect-cli/src/commands/diff.rs +++ b/crates/inspect-cli/src/commands/diff.rs @@ -5,7 +5,7 @@ use sem_core::git::types::DiffScope; use crate::formatters; use crate::OutputFormat; -use inspect_core::analyze::{analyze_with_options, AnalyzeOptions}; +use inspect_core::analyze::{analyze_with_options, retain_entity_reviews, AnalyzeOptions}; use inspect_core::types::RiskLevel; #[derive(Args)] @@ -48,7 +48,7 @@ pub fn run(args: DiffArgs) { // Filter by min risk if specified if let Some(ref min) = args.min_risk { let min_level = parse_risk_level(min); - result.entity_reviews.retain(|r| r.risk_level >= min_level); + retain_entity_reviews(&mut result, |r| r.risk_level >= min_level); } match args.format { diff --git a/crates/inspect-cli/src/commands/file.rs b/crates/inspect-cli/src/commands/file.rs index 936eaff..ec27831 100644 --- a/crates/inspect-cli/src/commands/file.rs +++ b/crates/inspect-cli/src/commands/file.rs @@ -1,11 +1,11 @@ -use std::path::PathBuf; +use std::path::{Component, Path, PathBuf}; use clap::Args; use sem_core::git::types::DiffScope; use crate::formatters; use crate::OutputFormat; -use inspect_core::analyze::analyze; +use inspect_core::analyze::{analyze, retain_entity_reviews}; use inspect_core::types::RiskLevel; #[derive(Args)] @@ -38,10 +38,12 @@ pub fn run(args: FileArgs) { match analyze(&repo, scope) { Ok(mut result) => { + let target_path = repo_relative_path(&repo, &args.path); + // Filter to only the specified file - result - .entity_reviews - .retain(|r| r.file_path.ends_with(&args.path)); + retain_entity_reviews(&mut result, |r| { + normalize_path(Path::new(&r.file_path)) == target_path + }); if let Some(ref min) = args.min_risk { let min_level = match min.to_lowercase().as_str() { @@ -50,7 +52,7 @@ pub fn run(args: FileArgs) { "medium" => RiskLevel::Medium, _ => RiskLevel::Low, }; - result.entity_reviews.retain(|r| r.risk_level >= min_level); + retain_entity_reviews(&mut result, |r| r.risk_level >= min_level); } match args.format { @@ -65,3 +67,93 @@ pub fn run(args: FileArgs) { } } } + +fn repo_relative_path(repo: &Path, input: &str) -> String { + let path = Path::new(input); + let absolute_path = if path.is_absolute() { + path.to_path_buf() + } else { + repo.join(path) + }; + let canonical_path = canonicalize_existing_path(&absolute_path); + + if let Ok(relative_path) = canonical_path.strip_prefix(repo) { + normalize_path(relative_path) + } else if let Ok(relative_path) = absolute_path.strip_prefix(repo) { + normalize_path(relative_path) + } else { + normalize_path(path) + } +} + +fn normalize_path(path: &Path) -> String { + let mut parts = Vec::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + if parts.last().is_some_and(|part| part != "..") { + parts.pop(); + } else { + parts.push("..".into()); + } + } + Component::Normal(part) => { + parts.push(part.to_string_lossy().into_owned()); + } + Component::Prefix(_) | Component::RootDir => {} + } + } + parts.join("/") +} + +fn canonicalize_existing_path(path: &Path) -> PathBuf { + if let Ok(canonical_path) = path.canonicalize() { + return canonical_path; + } + + let mut suffix = PathBuf::new(); + let mut cursor = path; + while let (Some(parent), Some(name)) = (cursor.parent(), cursor.file_name()) { + suffix = Path::new(name).join(suffix); + if let Ok(canonical_parent) = parent.canonicalize() { + return canonical_parent.join(suffix); + } + cursor = parent; + } + + path.to_path_buf() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn repo_relative_path_accepts_relative_paths() { + let repo = Path::new("/tmp/repo"); + + assert_eq!(repo_relative_path(repo, "src/../app.ts"), "app.ts"); + } + + #[test] + fn repo_relative_path_accepts_absolute_paths_under_repo() { + let repo = Path::new("/tmp/repo"); + + assert_eq!( + repo_relative_path(repo, "/tmp/repo/src/app.ts"), + "src/app.ts" + ); + } + + #[test] + fn normalize_path_preserves_leading_parent_dirs() { + assert_eq!(normalize_path(Path::new("../app.ts")), "../app.ts"); + } + + #[test] + fn normalize_path_collapses_nested_parent_dirs() { + assert_eq!(normalize_path(Path::new("src/../lib/../app.ts")), "app.ts"); + assert_eq!(normalize_path(Path::new("src/../../app.ts")), "../app.ts"); + } +} diff --git a/crates/inspect-cli/src/commands/pr.rs b/crates/inspect-cli/src/commands/pr.rs index 8dce62e..27a7870 100644 --- a/crates/inspect-cli/src/commands/pr.rs +++ b/crates/inspect-cli/src/commands/pr.rs @@ -6,7 +6,7 @@ use sem_core::git::types::DiffScope; use crate::formatters; use crate::OutputFormat; -use inspect_core::analyze::{analyze, analyze_remote}; +use inspect_core::analyze::{analyze, analyze_remote, retain_entity_reviews}; use inspect_core::github::GitHubClient; use inspect_core::noise::is_noise_file; use inspect_core::types::RiskLevel; @@ -146,10 +146,7 @@ async fn run_remote(args: &PrArgs, remote_repo: &str) { } } -fn apply_filters_and_print( - result: &mut inspect_core::types::ReviewResult, - args: &PrArgs, -) { +fn apply_filters_and_print(result: &mut inspect_core::types::ReviewResult, args: &PrArgs) { if let Some(ref min) = args.min_risk { let min_level = match min.to_lowercase().as_str() { "critical" => RiskLevel::Critical, @@ -157,7 +154,7 @@ fn apply_filters_and_print( "medium" => RiskLevel::Medium, _ => RiskLevel::Low, }; - result.entity_reviews.retain(|r| r.risk_level >= min_level); + retain_entity_reviews(result, |r| r.risk_level >= min_level); } match args.format { diff --git a/crates/inspect-core/src/analyze.rs b/crates/inspect-core/src/analyze.rs index 0e93525..b6ea09a 100644 --- a/crates/inspect-core/src/analyze.rs +++ b/crates/inspect-core/src/analyze.rs @@ -252,6 +252,7 @@ pub fn analyze_with_options( groups, stats, timing, + dependency_edges, changes, }) } @@ -372,6 +373,7 @@ pub fn analyze_remote(file_pairs: &[FilePair]) -> Result ReviewStats { } } +/// Retain entity reviews and keep derived result summaries in sync. +pub fn retain_entity_reviews(result: &mut ReviewResult, mut keep: F) +where + F: FnMut(&EntityReview) -> bool, +{ + result.entity_reviews.retain(|review| keep(review)); + refresh_result_summaries(result); +} + +fn refresh_result_summaries(result: &mut ReviewResult) { + let remaining_ids: HashSet = result + .entity_reviews + .iter() + .map(|review| review.entity_id.clone()) + .collect(); + + let dependency_edges: Vec<(String, String)> = result + .dependency_edges + .iter() + .filter(|(from, to)| remaining_ids.contains(from) && remaining_ids.contains(to)) + .cloned() + .collect(); + + let groups = untangle(&result.entity_reviews, &dependency_edges); + let entity_to_group: HashMap<&str, usize> = groups + .iter() + .flat_map(|g| g.entity_ids.iter().map(move |id| (id.as_str(), g.id))) + .collect(); + + for review in &mut result.entity_reviews { + if let Some(&gid) = entity_to_group.get(review.entity_id.as_str()) { + review.group_id = gid; + } + } + + result.groups = groups; + result.stats = compute_stats(&result.entity_reviews); + result.dependency_edges = dependency_edges; +} + /// Collect full source code of the top dependent entities for a changed entity. /// Uses the entity graph to get precise function boundaries via tree-sitter. fn collect_dependent_code( @@ -568,6 +610,7 @@ fn empty_result() -> ReviewResult { }, }, timing: Timing::default(), + dependency_edges: vec![], changes: vec![], } } @@ -584,6 +627,32 @@ mod tests { use std::process::Command; use tempfile::TempDir; + fn make_review(id: &str, name: &str, risk_level: RiskLevel) -> EntityReview { + EntityReview { + entity_id: id.into(), + entity_name: name.into(), + entity_type: "function".into(), + file_path: "main.rs".into(), + change_type: ChangeType::Modified, + classification: ChangeClassification::Functional, + risk_score: 0.5, + risk_level, + blast_radius: 0, + dependent_count: 0, + dependency_count: 0, + is_public_api: false, + structural_change: None, + group_id: 0, + start_line: 1, + end_line: 1, + before_content: None, + after_content: None, + dependent_names: vec![], + dependency_names: vec![], + dependent_entities: vec![], + } + } + fn init_repo(dir: &Path) { Command::new("git") .args(["init"]) @@ -759,4 +828,149 @@ mod tests { .iter() .all(|review| review.file_path == "main.rs")); } + + #[test] + fn retain_entity_reviews_recomputes_stats_and_groups() { + let mut result = ReviewResult { + entity_reviews: vec![ + make_review("main.rs::one", "one", RiskLevel::High), + make_review("main.rs::two", "two", RiskLevel::Medium), + ], + groups: vec![ChangeGroup { + id: 0, + label: "main.rs".into(), + entity_ids: vec!["main.rs::one".into(), "main.rs::two".into()], + }], + stats: compute_stats(&[]), + timing: Timing::default(), + dependency_edges: vec![("main.rs::one".into(), "main.rs::two".into())], + changes: vec![], + }; + + retain_entity_reviews(&mut result, |review| { + review.risk_level >= RiskLevel::Critical + }); + + assert!(result.entity_reviews.is_empty()); + assert!(result.groups.is_empty()); + assert_eq!(result.stats.total_entities, 0); + assert_eq!(result.stats.by_risk.high, 0); + } + + #[test] + fn retain_entity_reviews_updates_remaining_group_ids() { + let mut result = ReviewResult { + entity_reviews: vec![ + make_review("main.rs::one", "one", RiskLevel::High), + make_review("main.rs::two", "two", RiskLevel::Medium), + ], + groups: vec![ChangeGroup { + id: 0, + label: "main.rs".into(), + entity_ids: vec!["main.rs::one".into(), "main.rs::two".into()], + }], + stats: compute_stats(&[]), + timing: Timing::default(), + dependency_edges: vec![("main.rs::one".into(), "main.rs::two".into())], + changes: vec![], + }; + + retain_entity_reviews(&mut result, |review| review.entity_id == "main.rs::two"); + + assert_eq!(result.entity_reviews.len(), 1); + assert_eq!(result.entity_reviews[0].group_id, 0); + assert_eq!(result.groups.len(), 1); + assert_eq!(result.groups[0].entity_ids, vec!["main.rs::two"]); + assert_eq!(result.groups[0].label, "two"); + assert_eq!(result.stats.total_entities, 1); + assert_eq!(result.stats.by_risk.medium, 1); + } + + #[test] + fn retain_entity_reviews_drops_removed_bridge_edges() { + let mut result = ReviewResult { + entity_reviews: vec![ + make_review("main.rs::one", "one", RiskLevel::High), + make_review("main.rs::two", "two", RiskLevel::Medium), + make_review("main.rs::three", "three", RiskLevel::High), + ], + groups: vec![ChangeGroup { + id: 0, + label: "main.rs".into(), + entity_ids: vec![ + "main.rs::one".into(), + "main.rs::two".into(), + "main.rs::three".into(), + ], + }], + stats: compute_stats(&[]), + timing: Timing::default(), + dependency_edges: vec![ + ("main.rs::one".into(), "main.rs::two".into()), + ("main.rs::two".into(), "main.rs::three".into()), + ], + changes: vec![], + }; + + retain_entity_reviews(&mut result, |review| { + review.risk_level >= RiskLevel::High + }); + + assert_eq!(result.entity_reviews.len(), 2); + assert_eq!(result.groups.len(), 2); + assert!(result.groups.iter().all(|group| group.entity_ids.len() == 1)); + assert_ne!( + result.entity_reviews[0].group_id, + result.entity_reviews[1].group_id + ); + assert!(result.dependency_edges.is_empty()); + assert_eq!(result.stats.total_entities, 2); + assert_eq!(result.stats.by_risk.high, 2); + } + + #[test] + fn retain_entity_reviews_supports_sequential_filters() { + let mut result = ReviewResult { + entity_reviews: vec![ + make_review("main.rs::one", "one", RiskLevel::High), + make_review("main.rs::two", "two", RiskLevel::Medium), + make_review("main.rs::three", "three", RiskLevel::High), + make_review("main.rs::four", "four", RiskLevel::Low), + ], + groups: vec![ + ChangeGroup { + id: 0, + label: "main.rs".into(), + entity_ids: vec![ + "main.rs::one".into(), + "main.rs::two".into(), + "main.rs::three".into(), + ], + }, + ChangeGroup { + id: 1, + label: "four".into(), + entity_ids: vec!["main.rs::four".into()], + }, + ], + stats: compute_stats(&[]), + timing: Timing::default(), + dependency_edges: vec![ + ("main.rs::one".into(), "main.rs::two".into()), + ("main.rs::two".into(), "main.rs::three".into()), + ], + changes: vec![], + }; + + retain_entity_reviews(&mut result, |review| review.entity_id != "main.rs::four"); + assert_eq!(result.dependency_edges.len(), 2); + + retain_entity_reviews(&mut result, |review| { + review.risk_level >= RiskLevel::High + }); + + assert_eq!(result.entity_reviews.len(), 2); + assert_eq!(result.groups.len(), 2); + assert!(result.dependency_edges.is_empty()); + } } diff --git a/crates/inspect-core/src/types.rs b/crates/inspect-core/src/types.rs index c26bf70..4064aca 100644 --- a/crates/inspect-core/src/types.rs +++ b/crates/inspect-core/src/types.rs @@ -172,6 +172,9 @@ pub struct ReviewResult { pub groups: Vec, pub stats: ReviewStats, pub timing: Timing, + /// Dependency edges between changed entities, used to recompute groups after filtering. + #[serde(skip)] + pub dependency_edges: Vec<(String, String)>, /// The underlying semantic changes (for formatters that want raw data) #[serde(skip)] pub changes: Vec,