clean project structure

crates/steep-server/src/bot_actions/review/diff.rs

@@ -0,0 +1,389 @@
+//! Parsing of a pull request unified diff into the lines it changes.
+
+use tracing::warn;
+
+use crate::{domain::review::ReviewSide, gitea::PullRequestFile};
+
+/// The lines a pull request changed, per file.
+///
+/// Line numbers are the ones Gitea expects to place a review comment:
+/// [`ReviewSide::Added`] numbers refer to the new version of the file (sent as
+/// `new_position`), [`ReviewSide::Removed`] numbers to the old version (sent as
+/// `old_position`).
+pub(super) type ChangedLines = Vec<ChangedFile>;
+
+pub(super) struct ChangedFile {
+    pub(super) filename: String,
+    pub(super) added: Vec<u64>,
+    pub(super) removed: Vec<u64>,
+}
+
+impl ChangedFile {
+    fn new(filename: &str) -> Self {
+        Self {
+            filename: String::from(filename),
+            added: Vec::new(),
+            removed: Vec::new(),
+        }
+    }
+
+    /// Lines that can be commented on for the given side.
+    pub(super) fn lines(&self, side: ReviewSide) -> &[u64] {
+        match side {
+            ReviewSide::Added => &self.added,
+            ReviewSide::Removed => &self.removed,
+        }
+    }
+
+    /// Side a line belongs to, used when the model did not state one.
+    pub(super) fn side_of(&self, line: u64) -> Option<ReviewSide> {
+        if self.added.contains(&line) {
+            Some(ReviewSide::Added)
+        } else if self.removed.contains(&line) {
+            Some(ReviewSide::Removed)
+        } else {
+            None
+        }
+    }
+}
+
+/// Lists the lines changed by the diff, per file, on both sides.
+///
+/// Only the line numbers are kept: the model reads the code itself through the
+/// sandbox tools.
+pub(super) fn parse_changed_lines(git_diff: &str) -> ChangedLines {
+    let mut files = Vec::new();
+    let mut current_file: Option<String> = None;
+    let mut in_hunk = false;
+    let mut old_line: u64 = 0;
+    let mut new_line: u64 = 0;
+
+    for line in git_diff.lines() {
+        if line.starts_with("diff --git ") {
+            current_file = None;
+            in_hunk = false;
+            continue;
+        }
+
+        // `--- a/x` and `+++ b/x` only appear before the first hunk of a file.
+        // Inside a hunk, a line may legitimately start with them: removing
+        // `--x` gives `---x`, adding `++i;` gives `+++i;`.
+        if !in_hunk && (line.starts_with("--- ") || line.starts_with("+++ ")) {
+            // A header is never a content line: `+++ /dev/null` on a deleted
+            // file keeps the path of the other side in place.
+            if let Some(path) = header_file_path(line) {
+                current_file = Some(path);
+            }
+            continue;
+        }
+
+        if line.starts_with("@@") {
+            if let Some((old_start, new_start)) = parse_hunk_starts(line) {
+                old_line = old_start;
+                new_line = new_start;
+                in_hunk = true;
+            }
+            continue;
+        }
+
+        let Some(filename) = current_file.as_deref() else {
+            continue;
+        };
+
+        match line.as_bytes().first() {
+            Some(b' ') => {
+                old_line += 1;
+                new_line += 1;
+            }
+            Some(b'-') => {
+                changed_file(&mut files, filename).removed.push(old_line);
+                old_line += 1;
+            }
+            Some(b'+') => {
+                changed_file(&mut files, filename).added.push(new_line);
+                new_line += 1;
+            }
+            // `\ No newline at end of file`, and anything unexpected: a line
+            // that advances neither side.
+            _ => {}
+        }
+    }
+
+    files
+}
+
+/// Path of the file on one side of the diff, from a `--- a/<path>` or
+/// `+++ b/<path>` header line.
+///
+/// These lines are the only unambiguous source for the path: the `diff --git`
+/// line is cut at the first space, and it names the old path of a renamed file.
+/// `None` for `/dev/null`, which leaves the path of the other side in place.
+fn header_file_path(line: &str) -> Option<String> {
+    let (prefix, raw) = match line.strip_prefix("--- ") {
+        Some(raw) => ("a/", raw),
+        None => ("b/", line.strip_prefix("+++ ")?),
+    };
+
+    let path = decode_git_path(raw);
+
+    Some(String::from(path.strip_prefix(prefix)?))
+}
+
+/// Decodes a path as git writes it in a diff header: git wraps it in quotes and
+/// escapes the bytes that need it (`\303\251` for `é`) when the path contains
+/// non-printable or non-ASCII characters.
+fn decode_git_path(raw: &str) -> String {
+    let Some(quoted) = raw.strip_prefix('"').and_then(|raw| raw.strip_suffix('"')) else {
+        return String::from(raw);
+    };
+
+    let bytes = quoted.as_bytes();
+    let mut decoded = Vec::with_capacity(bytes.len());
+    let mut index = 0;
+
+    while let Some(byte) = bytes.get(index) {
+        index += 1;
+
+        if *byte != b'\\' {
+            decoded.push(*byte);
+            continue;
+        }
+
+        match bytes.get(index) {
+            // Octal escapes are the ones that matter for a path: git uses them
+            // for every non-ASCII byte.
+            Some(digit @ b'0'..=b'7') => {
+                let mut value = u32::from(digit - b'0');
+                let mut digits = 1;
+
+                while digits < 3 {
+                    match bytes.get(index + digits) {
+                        Some(next @ b'0'..=b'7') => {
+                            value = value * 8 + u32::from(next - b'0');
+                            digits += 1;
+                        }
+                        _ => break,
+                    }
+                }
+
+                decoded.push(u8::try_from(value).unwrap_or(b'?'));
+                index += digits;
+            }
+            Some(escaped) => {
+                decoded.push(match escaped {
+                    b't' => b'\t',
+                    b'n' => b'\n',
+                    b'r' => b'\r',
+                    other => *other,
+                });
+                index += 1;
+            }
+            None => decoded.push(b'\\'),
+        }
+    }
+
+    String::from_utf8_lossy(&decoded).into_owned()
+}
+
+/// Replaces the paths parsed from the diff with the exact paths reported by the
+/// API, which are the ones the model sees in the sandbox.
+pub(super) fn resolve_filenames(changed_lines: &mut ChangedLines, files: &[PullRequestFile]) {
+    for changed in changed_lines.iter_mut() {
+        let parsed = changed.filename.as_str();
+
+        let Some(file) = files.iter().find(|file| file.filename == parsed) else {
+            if !files.is_empty() {
+                warn!(path = %parsed, "Changed file is not in the pull request file list");
+            }
+            continue;
+        };
+
+        changed.filename = file.filename.clone();
+    }
+}
+
+/// 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.
+pub(super) 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.
+pub(super) fn drop_generated_files(changed_lines: &mut ChangedLines) {
+    changed_lines.retain(|file| !is_ignored(&file.filename));
+}
+
+/// Borrows the entry of `files` for a file, creating it on its first change.
+fn changed_file<'a>(files: &'a mut ChangedLines, filename: &str) -> &'a mut ChangedFile {
+    let index = match files.iter().position(|file| file.filename == filename) {
+        Some(index) => index,
+        None => {
+            files.push(ChangedFile::new(filename));
+            files.len() - 1
+        }
+    };
+
+    &mut files[index]
+}
+
+/// Extracts the old and new starting line numbers of a hunk header such as
+/// `@@ -12,3 +12,5 @@`. The counts are optional and git may append a section
+/// heading after the closing `@@`.
+fn parse_hunk_starts(hunk_header: &str) -> Option<(u64, u64)> {
+    let body = hunk_header.strip_prefix("@@ ")?;
+    let body = body.split(" @@").next()?;
+
+    let (old, new) = body.split_once(" +")?;
+    let old = old.strip_prefix('-')?.split(',').next()?;
+    let new = new.split(',').next()?;
+
+    Some((old.parse().ok()?, new.parse().ok()?))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::bot_actions::review::{fixtures::*, prompt::format_changed_lines};
+
+    #[test]
+    fn a_deletion_only_pull_request_lists_removed_lines() {
+        let expected = "src/old.rs: removed 1, 2, 3";
+
+        assert_eq!(
+            format_changed_lines(&parse_changed_lines(DELETION_ONLY)),
+            expected
+        );
+    }
+
+    #[test]
+    fn hunk_content_starting_with_plus_or_minus_is_counted() {
+        let expected = "src/tricky.js: added 2 / removed 2";
+
+        assert_eq!(
+            format_changed_lines(&parse_changed_lines(TRICKY_CONTENT)),
+            expected
+        );
+    }
+
+    #[test]
+    fn a_renamed_file_uses_its_new_path() {
+        let expected = "src/new.rs: added 1 / removed 1";
+
+        assert_eq!(
+            format_changed_lines(&parse_changed_lines(RENAMED)),
+            expected
+        );
+    }
+
+    #[test]
+    fn a_path_with_a_space_is_read_from_the_headers() {
+        let expected = "src/my file.rs: added 2";
+
+        assert_eq!(
+            format_changed_lines(&parse_changed_lines(PATH_WITH_SPACE)),
+            expected
+        );
+    }
+
+    #[test]
+    fn a_quoted_path_is_decoded() {
+        let expected = "docs/café.md: added 2";
+
+        assert_eq!(
+            format_changed_lines(&parse_changed_lines(QUOTED_PATH)),
+            expected
+        );
+    }
+
+    #[test]
+    fn filenames_are_resolved_against_the_api_list() {
+        let mut changed_lines = parse_changed_lines(DIFF);
+        let files = vec![
+            pull_request_file("src/bar.rs", None, "modified"),
+            pull_request_file("src/foo.rs", Some("src/renamed.rs"), "renamed"),
+        ];
+
+        resolve_filenames(&mut changed_lines, &files);
+
+        assert_eq!(changed_lines[0].filename, "src/foo.rs");
+        assert_eq!(changed_lines[1].filename, "src/bar.rs");
+    }
+
+    #[test]
+    fn a_file_absent_from_the_api_list_keeps_the_diff_path() {
+        let mut changed_lines = parse_changed_lines(DIFF);
+        let files = vec![pull_request_file("src/bar.rs", None, "modified")];
+
+        resolve_filenames(&mut changed_lines, &files);
+
+        assert_eq!(changed_lines[0].filename, "src/foo.rs");
+        assert_eq!(changed_lines[1].filename, "src/bar.rs");
+    }
+
+    #[test]
+    fn generated_lockfiles_are_ignored() {
+        assert!(is_ignored("Cargo.lock"));
+        assert!(is_ignored("crates/steep-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");
+    }
+}