add review type (perf, bug, security, ...) + fix tests

crates/herald-server/src/bot_actions/review.rs

@@ -3,7 +3,7 @@ use std::str::FromStr;
 use tracing::{info, instrument, warn};
 
 use crate::{
-    bot::{ReviewResult, ReviewSide},
+    bot::{ReviewItem, ReviewResult, ReviewSeverity, ReviewSide},
     consts::{BOT_PROCESS_MSG, REVIEW_PROMPT, SANDBOX_SYSTEM_PROMPT},
     gitea::{GiteaAPI, PullRequestFile, ReviewPayload},
     metrics,
@@ -53,6 +53,7 @@ pub async fn exec_review(
         };
 
         let mut changed_lines = parse_changed_lines(&git_diff);
+        drop_generated_files(&mut changed_lines);
         resolve_filenames(&mut changed_lines, &files);
 
         let changes = format_changes(&files, &changed_lines);
@@ -126,11 +127,9 @@ async fn run_sandboxed_review(
     review_payload: &ReviewPayload,
     bot_request: &str,
 ) -> anyhow::Result<(ReviewResult, Option<f64>)> {
-    let repo_url = gitea_api.repo_clone_url(&review_payload.repository.full_name);
-
     let sandbox = Sandbox::create(
         &sandbox_config.runtime,
-        &repo_url,
+        &review_payload.repository.clone_url,
         gitea_api.token(),
         review_payload.pull_request.number,
     )
@@ -172,6 +171,12 @@ fn review_result_to_markdown(review_result: &ReviewResult) -> String {
         review_result.reviews.len()
     ));
 
+    let breakdown = severity_breakdown(&review_result.reviews);
+    if !breakdown.is_empty() {
+        md.push_str(&breakdown);
+        md.push('\n');
+    }
+
     if !review_result.comment.is_empty() {
         md.push_str("\n---\n\n");
         md.push_str("### Summary\n\n");
@@ -188,6 +193,30 @@ fn review_result_to_markdown(review_result: &ReviewResult) -> String {
     md
 }
 
+/// Counts the reviews per severity, most severe first.
+///
+/// A review whose severity the model omitted or wrote unreadably is counted as
+/// [`ReviewSeverity::Maintainability`], the same fallback used when posting it.
+fn severity_breakdown(reviews: &[ReviewItem]) -> String {
+    [
+        ReviewSeverity::Bug,
+        ReviewSeverity::Security,
+        ReviewSeverity::Performance,
+        ReviewSeverity::Maintainability,
+    ]
+    .into_iter()
+    .filter_map(|severity| {
+        let count = reviews
+            .iter()
+            .filter(|review| review.severity.unwrap_or(ReviewSeverity::Maintainability) == severity)
+            .count();
+
+        (count > 0).then(|| format!("- {} {}", count, severity.label()))
+    })
+    .collect::<Vec<_>>()
+    .join("\n")
+}
+
 /// The lines a pull request changed, per file.
 ///
 /// Line numbers are the ones Gitea expects to place a review comment:
@@ -386,18 +415,61 @@ fn resolve_filenames(changed_lines: &mut ChangedLines, files: &[PullRequestFile]
     }
 }
 
+/// Basenames of generated files that are never reviewed.
+///
+/// Their diffs are machine-written dependency churn: they run to thousands of
+/// lines, flood the prompt with line numbers and drown the code the model should
+/// look at. A lockfile is also never a place where a review comment belongs.
+const IGNORED_FILE_NAMES: [&str; 16] = [
+    "Cargo.lock",
+    "package-lock.json",
+    "npm-shrinkwrap.json",
+    "yarn.lock",
+    "pnpm-lock.yaml",
+    "bun.lock",
+    "bun.lockb",
+    "composer.lock",
+    "Gemfile.lock",
+    "poetry.lock",
+    "uv.lock",
+    "Pipfile.lock",
+    "go.sum",
+    "packages.lock.json",
+    "flake.lock",
+    "pubspec.lock",
+];
+
+/// Whether a changed path is a generated file that is never reviewed.
+///
+/// The comparison is on the basename, so a lockfile nested in a workspace (for
+/// example `crates/foo/Cargo.lock`) is matched too.
+fn is_ignored(filename: &str) -> bool {
+    let basename = filename.rsplit('/').next().unwrap_or(filename);
+
+    IGNORED_FILE_NAMES.contains(&basename)
+}
+
+/// Drops the generated files from the changed lines.
+///
+/// Removing them here keeps the prompt free of their line numbers, and also makes
+/// [`resolve_review_sides`] reject any review the model anchors on one of them.
+fn drop_generated_files(changed_lines: &mut ChangedLines) {
+    changed_lines.retain(|file| !is_ignored(&file.filename));
+}
+
 /// Renders the changes for the model: the files the pull request touches, then
 /// the lines to review per file.
 fn format_changes(files: &[PullRequestFile], changed_lines: &ChangedLines) -> String {
     let mut sections = Vec::new();
 
-    if !files.is_empty() {
-        let described = files
-            .iter()
-            .map(describe_file)
-            .collect::<Vec<_>>()
-            .join(", ");
+    let described = files
+        .iter()
+        .filter(|file| !is_ignored(&file.filename))
+        .map(describe_file)
+        .collect::<Vec<_>>()
+        .join(", ");
 
+    if !described.is_empty() {
         sections.push(format!("Files changed by the pull request: {described}"));
     }
 
@@ -666,6 +738,7 @@ mod tests {
             filename: String::from(filename),
             line,
             side,
+            severity: None,
             message: String::from("message"),
         }
     }
@@ -799,6 +872,56 @@ mod tests {
         assert_eq!(format_changes(&[], &changed_lines), expected);
     }
 
+    #[test]
+    fn generated_lockfiles_are_ignored() {
+        assert!(is_ignored("Cargo.lock"));
+        assert!(is_ignored("crates/herald-server/Cargo.lock"));
+        assert!(is_ignored("package-lock.json"));
+        assert!(is_ignored("web/yarn.lock"));
+        assert!(is_ignored("go.sum"));
+
+        assert!(!is_ignored("src/main.rs"));
+        assert!(!is_ignored("Cargo.toml"));
+        assert!(!is_ignored("docs/lockfiles.md"));
+    }
+
+    #[test]
+    fn generated_files_are_dropped_from_the_changed_lines() {
+        const DIFF_WITH_LOCKFILE: &str = concat!(
+            "diff --git a/Cargo.lock b/Cargo.lock\n",
+            "--- a/Cargo.lock\n",
+            "+++ b/Cargo.lock\n",
+            "@@ -1,1 +1,2 @@\n",
+            " [[package]]\n",
+            "+name = \"x\"\n",
+            "diff --git a/src/foo.rs b/src/foo.rs\n",
+            "--- a/src/foo.rs\n",
+            "+++ b/src/foo.rs\n",
+            "@@ -1,1 +1,2 @@\n",
+            " fn a() {}\n",
+            "+fn b() {}\n",
+        );
+
+        let mut changed_lines = parse_changed_lines(DIFF_WITH_LOCKFILE);
+        drop_generated_files(&mut changed_lines);
+
+        assert_eq!(format_changed_lines(&changed_lines), "src/foo.rs: added 2");
+    }
+
+    #[test]
+    fn a_generated_file_is_not_described_to_the_model() {
+        let files = vec![
+            pull_request_file("Cargo.lock", None, "modified"),
+            pull_request_file("src/foo.rs", None, "modified"),
+        ];
+        let changed_lines = parse_changed_lines(DIFF);
+
+        let changes = format_changes(&files, &changed_lines);
+
+        assert!(!changes.contains("Cargo.lock"));
+        assert!(changes.contains("src/foo.rs (modified)"));
+    }
+
     #[test]
     fn a_renamed_file_is_described_with_its_previous_path() {
         let file = pull_request_file("src/new.rs", Some("src/old.rs"), "renamed");
@@ -960,4 +1083,79 @@ mod tests {
             ]
         );
     }
+
+    #[test]
+    fn severities_are_read_from_the_model() {
+        let result: ReviewResult = serde_json::from_str(
+            r#"{
+                "reviews": [
+                    { "filename": "a", "line": 1, "severity": "Bug", "message": "x" },
+                    { "filename": "b", "line": 2, "severity": "SECURITY", "message": "x" },
+                    { "filename": "c", "line": 3, "severity": "perf", "message": "x" },
+                    { "filename": "d", "line": 4, "severity": "banana", "message": "x" }
+                ],
+                "comment": ""
+            }"#,
+        )
+        .unwrap();
+
+        let severities = result
+            .reviews
+            .iter()
+            .map(|review| review.severity)
+            .collect::<Vec<_>>();
+
+        assert_eq!(
+            severities,
+            vec![
+                Some(ReviewSeverity::Bug),
+                Some(ReviewSeverity::Security),
+                Some(ReviewSeverity::Performance),
+                None,
+            ]
+        );
+    }
+
+    #[test]
+    fn the_breakdown_counts_severities_most_severe_first() {
+        let reviews = vec![
+            ReviewItem {
+                severity: Some(ReviewSeverity::Maintainability),
+                ..review("a", Some(1), None)
+            },
+            ReviewItem {
+                severity: Some(ReviewSeverity::Bug),
+                ..review("b", Some(2), None)
+            },
+            ReviewItem {
+                severity: Some(ReviewSeverity::Bug),
+                ..review("c", Some(3), None)
+            },
+            // A severity the model left out counts as maintainability.
+            review("d", Some(4), None),
+        ];
+
+        assert_eq!(severity_breakdown(&reviews), "- 2 bug\n- 2 maintainability");
+    }
+
+    #[test]
+    fn the_markdown_breaks_the_issues_down_by_severity() {
+        let mut result = review_result(vec![
+            ReviewItem {
+                severity: Some(ReviewSeverity::Bug),
+                ..review("a", Some(1), None)
+            },
+            ReviewItem {
+                severity: Some(ReviewSeverity::Performance),
+                ..review("b", Some(2), None)
+            },
+        ]);
+        result.comment = String::from("Le découpage en crates est propre.");
+
+        let markdown = review_result_to_markdown(&result);
+
+        assert!(markdown.contains("- 1 bug"));
+        assert!(markdown.contains("- 1 performance"));
+        assert!(markdown.contains("Le découpage en crates est propre."));
+    }
 }