Replace openrouter-rs with in-tree client and require sandbox
Remove the openrouter-rs dependency in favor of a minimal in-tree OpenRouter chat-completions client, and drop the BOT_NAME and SANDBOX_ENABLED config options. Reviews now always run inside the sandbox, and the review prompt asks the model to read files with the available tools instead of embedding the diff.
This commit is contained in:
@@ -1,30 +1,18 @@
|
||||
use futures_util::stream::TryStreamExt;
|
||||
use openrouter_rs::types::Tool;
|
||||
use tokio::io::AsyncReadExt;
|
||||
use tokio_util::io::StreamReader;
|
||||
use tracing::{info, instrument, warn};
|
||||
|
||||
use crate::{
|
||||
bot::ReviewResult,
|
||||
consts::{BOT_PROCESS_MSG, MAX_DIFF_SIZE, REVIEW_PROMPT, SANDBOX_SYSTEM_PROMPT},
|
||||
gitea::{GiteaAPI, ReviewPayload},
|
||||
bot::{ReviewResult, ReviewSide},
|
||||
consts::{BOT_PROCESS_MSG, REVIEW_PROMPT, SANDBOX_SYSTEM_PROMPT},
|
||||
gitea::{GiteaAPI, PullRequestFile, ReviewPayload},
|
||||
metrics,
|
||||
open_router::OpenRouterClient,
|
||||
open_router::{OpenRouterClient, Tool},
|
||||
sandbox::{Sandbox, SandboxConfig, agent},
|
||||
};
|
||||
|
||||
#[instrument(skip(
|
||||
gitea_api,
|
||||
open_router_client,
|
||||
http_client,
|
||||
sandbox_config,
|
||||
tools,
|
||||
review_payload
|
||||
))]
|
||||
#[instrument(skip(gitea_api, open_router_client, sandbox_config, tools, review_payload))]
|
||||
pub async fn exec_review(
|
||||
gitea_api: &GiteaAPI,
|
||||
open_router_client: &OpenRouterClient,
|
||||
http_client: &reqwest::Client,
|
||||
model: &str,
|
||||
sandbox_config: &SandboxConfig,
|
||||
tools: Vec<Tool>,
|
||||
@@ -46,32 +34,43 @@ pub async fn exec_review(
|
||||
.await?;
|
||||
|
||||
let bot_result: Result<ReviewResult, anyhow::Error> = async {
|
||||
let git_diff =
|
||||
download_git_diff(http_client, &review_payload.pull_request.diff_url).await?;
|
||||
let full_name = &review_payload.repository.full_name;
|
||||
let index = review_payload.pull_request.number;
|
||||
|
||||
let diff_for_llm = format_diff_for_review(&git_diff);
|
||||
let git_diff = gitea_api.pull_request_diff(full_name, index).await?;
|
||||
|
||||
// The file list only refines the paths and describes the changes: a
|
||||
// failure is not fatal, the diff is the source of truth for the lines.
|
||||
let files = match gitea_api.pull_request_files(full_name, index).await {
|
||||
Ok(files) => files,
|
||||
Err(err) => {
|
||||
warn!(%err, "Failed to list the pull request files");
|
||||
Vec::new()
|
||||
}
|
||||
};
|
||||
|
||||
let mut changed_lines = parse_changed_lines(&git_diff);
|
||||
resolve_filenames(&mut changed_lines, &files);
|
||||
|
||||
let changes = format_changes(&files, &changed_lines);
|
||||
|
||||
let bot_request = REVIEW_PROMPT
|
||||
.replace("{subject}", &review_payload.pull_request.title)
|
||||
.replace("{comment}", &review_payload.comment.body)
|
||||
.replace("{diff}", &diff_for_llm);
|
||||
.replace("{changes}", &changes);
|
||||
|
||||
let (message, cost) = if sandbox_config.enabled {
|
||||
run_sandboxed_review(
|
||||
gitea_api,
|
||||
open_router_client,
|
||||
sandbox_config,
|
||||
tools,
|
||||
&review_payload,
|
||||
&bot_request,
|
||||
)
|
||||
.await?
|
||||
} else {
|
||||
let chat_result = open_router_client.chat(&bot_request).await?;
|
||||
(chat_result.message, chat_result.cost)
|
||||
};
|
||||
let (message, cost) = run_sandboxed_review(
|
||||
gitea_api,
|
||||
open_router_client,
|
||||
sandbox_config,
|
||||
tools,
|
||||
&review_payload,
|
||||
&bot_request,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let mut review_result = serde_json::from_str::<ReviewResult>(&message)?;
|
||||
resolve_review_sides(&mut review_result, &changed_lines);
|
||||
|
||||
review_result.cost = cost;
|
||||
if let Some(cost) = review_result.cost {
|
||||
@@ -183,83 +182,350 @@ fn review_result_to_markdown(review_result: &ReviewResult) -> String {
|
||||
md
|
||||
}
|
||||
|
||||
async fn download_git_diff(http_client: &reqwest::Client, url: &str) -> anyhow::Result<String> {
|
||||
let response = http_client.get(url).send().await?;
|
||||
let stream = response.bytes_stream().map_err(std::io::Error::other);
|
||||
/// 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`).
|
||||
type ChangedLines = Vec<ChangedFile>;
|
||||
|
||||
let mut buf = Vec::with_capacity(MAX_DIFF_SIZE);
|
||||
StreamReader::new(stream)
|
||||
.take((MAX_DIFF_SIZE + 1) as u64)
|
||||
.read_to_end(&mut buf)
|
||||
.await?;
|
||||
|
||||
if buf.len() > MAX_DIFF_SIZE {
|
||||
anyhow::bail!("Git diff exceeds the maximum allowed size of 1 Mo");
|
||||
}
|
||||
|
||||
Ok(String::from_utf8_lossy(&buf).into_owned())
|
||||
struct ChangedFile {
|
||||
filename: String,
|
||||
added: Vec<u64>,
|
||||
removed: Vec<u64>,
|
||||
}
|
||||
|
||||
fn format_diff_for_review(git_diff: &str) -> String {
|
||||
let mut output = String::new();
|
||||
let mut current_file: Option<&str> = None;
|
||||
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.
|
||||
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.
|
||||
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.
|
||||
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 let Some(rest) = line.strip_prefix("diff --git a/") {
|
||||
if let Some(end) = rest.find(' ') {
|
||||
current_file = Some(&rest[..end]);
|
||||
}
|
||||
new_line = 0;
|
||||
if line.starts_with("diff --git ") {
|
||||
current_file = None;
|
||||
in_hunk = false;
|
||||
continue;
|
||||
}
|
||||
|
||||
if line.starts_with("---") || line.starts_with("+++") {
|
||||
continue;
|
||||
}
|
||||
|
||||
if line.starts_with("@@") && line.contains('+') {
|
||||
if let Some(start) = parse_hunk_new_start(line) {
|
||||
new_line = start;
|
||||
// `--- 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;
|
||||
}
|
||||
|
||||
let Some(filename) = current_file else {
|
||||
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;
|
||||
};
|
||||
|
||||
if line.starts_with(' ') {
|
||||
new_line += 1;
|
||||
continue;
|
||||
}
|
||||
|
||||
if let Some(code) = line.strip_prefix('+') {
|
||||
use std::fmt::Write;
|
||||
let _ = writeln!(output, "{filename}:{new_line}:{code}");
|
||||
new_line += 1;
|
||||
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.
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
output
|
||||
files
|
||||
}
|
||||
|
||||
fn parse_hunk_new_start(hunk_header: &str) -> Option<u64> {
|
||||
let plus_part = hunk_header.split('+').nth(1)?;
|
||||
let num_str = plus_part.split(|c: char| !c.is_ascii_digit()).next()?;
|
||||
num_str.parse::<u64>().ok()
|
||||
/// 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.
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
/// 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(", ");
|
||||
|
||||
sections.push(format!("Files changed by the pull request: {described}"));
|
||||
}
|
||||
|
||||
sections.push(format!(
|
||||
"Lines to review, per file:\n{}",
|
||||
format_changed_lines(changed_lines)
|
||||
));
|
||||
|
||||
sections.join("\n\n")
|
||||
}
|
||||
|
||||
/// Describes a changed file for the model, including how it changed.
|
||||
fn describe_file(file: &PullRequestFile) -> String {
|
||||
match &file.previous_filename {
|
||||
Some(previous) => format!("{} ({} from {})", file.filename, file.status, previous),
|
||||
None => format!("{} ({})", file.filename, file.status),
|
||||
}
|
||||
}
|
||||
|
||||
/// 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]
|
||||
}
|
||||
|
||||
/// Renders the changed lines as `filename: added 1, 2 / removed 3`, one file per
|
||||
/// line, keeping only the sides the file actually has.
|
||||
fn format_changed_lines(changed_lines: &ChangedLines) -> String {
|
||||
changed_lines
|
||||
.iter()
|
||||
.map(|file| {
|
||||
let mut sides = Vec::new();
|
||||
|
||||
for (label, lines) in [("added", &file.added), ("removed", &file.removed)] {
|
||||
if !lines.is_empty() {
|
||||
sides.push(format!("{label} {}", format_line_numbers(lines)));
|
||||
}
|
||||
}
|
||||
|
||||
format!("{}: {}", file.filename, sides.join(" / "))
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
fn format_line_numbers(lines: &[u64]) -> String {
|
||||
lines
|
||||
.iter()
|
||||
.map(u64::to_string)
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ")
|
||||
}
|
||||
|
||||
/// Resolves the side each review is anchored on and drops the reviews that do
|
||||
/// not match a line the pull request changes.
|
||||
///
|
||||
/// The model is asked to pick a line and a side from the provided lists, but
|
||||
/// nothing forces it to, and Gitea accepts any position: a wrong one places the
|
||||
/// comment on an unrelated line of the file instead of failing. A review that
|
||||
/// omits its side is resolved from the lists, and one that matches no changed
|
||||
/// line is dropped.
|
||||
fn resolve_review_sides(review_result: &mut ReviewResult, changed_lines: &ChangedLines) {
|
||||
let mut dropped = Vec::new();
|
||||
|
||||
review_result.reviews.retain_mut(|review| {
|
||||
let side = review.line.and_then(|line| {
|
||||
let file = changed_lines
|
||||
.iter()
|
||||
.find(|file| file.filename == review.filename)?;
|
||||
|
||||
let side = review.side.or_else(|| file.side_of(line))?;
|
||||
|
||||
file.lines(side).contains(&line).then_some(side)
|
||||
});
|
||||
|
||||
match side {
|
||||
Some(side) => {
|
||||
review.side = Some(side);
|
||||
true
|
||||
}
|
||||
None => {
|
||||
dropped.push(match review.line {
|
||||
Some(line) => format!("{}:{line}", review.filename),
|
||||
None => format!("{}:no line", review.filename),
|
||||
});
|
||||
false
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
if !dropped.is_empty() {
|
||||
warn!(
|
||||
dropped = dropped.len(),
|
||||
reviews = %dropped.join(", "),
|
||||
"Dropped reviews that are not anchored on a changed line"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// 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)]
|
||||
#[test]
|
||||
fn test_format_diff_for_review() {
|
||||
let diff = concat!(
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::bot::ReviewItem;
|
||||
|
||||
/// Additions, a removal and a line changed on both sides.
|
||||
const DIFF: &str = concat!(
|
||||
"diff --git a/src/foo.rs b/src/foo.rs\n",
|
||||
"--- a/src/foo.rs\n",
|
||||
"+++ b/src/foo.rs\n",
|
||||
"@@ -1,3 +1,6 @@\n",
|
||||
"@@ -1,4 +1,6 @@\n",
|
||||
" fn main() {\n",
|
||||
"+ let x = 1;\n",
|
||||
"- let removed = 0;\n",
|
||||
" println!(\"hello\");\n",
|
||||
"+ let y = 2;\n",
|
||||
"+ let z = 3;\n",
|
||||
@@ -274,14 +540,324 @@ fn test_format_diff_for_review() {
|
||||
"+ let b = 20;\n",
|
||||
);
|
||||
|
||||
let result = format_diff_for_review(diff);
|
||||
let expected = concat!(
|
||||
"src/foo.rs:2: let x = 1;\n",
|
||||
"src/foo.rs:4: let y = 2;\n",
|
||||
"src/foo.rs:5: let z = 3;\n",
|
||||
"src/bar.rs:11: let a = 10;\n",
|
||||
"src/bar.rs:13: let b = 20;\n",
|
||||
/// A pull request that only deletes a file.
|
||||
const DELETION_ONLY: &str = concat!(
|
||||
"diff --git a/src/old.rs b/src/old.rs\n",
|
||||
"deleted file mode 100644\n",
|
||||
"--- a/src/old.rs\n",
|
||||
"+++ /dev/null\n",
|
||||
"@@ -1,3 +0,0 @@\n",
|
||||
"-fn a() {}\n",
|
||||
"-fn b() {}\n",
|
||||
"-fn c() {}\n",
|
||||
);
|
||||
|
||||
assert_eq!(result, expected);
|
||||
/// A hunk whose content starts with `+++` / `---`, which must not be taken
|
||||
/// for the file headers.
|
||||
const TRICKY_CONTENT: &str = concat!(
|
||||
"diff --git a/src/tricky.js b/src/tricky.js\n",
|
||||
"--- a/src/tricky.js\n",
|
||||
"+++ b/src/tricky.js\n",
|
||||
"@@ -1,4 +1,4 @@\n",
|
||||
" let i = 0;\n",
|
||||
"+++i;\n",
|
||||
"---x;\n",
|
||||
" console.log(i);\n",
|
||||
);
|
||||
|
||||
/// A pull request that renames a file.
|
||||
const RENAMED: &str = concat!(
|
||||
"diff --git a/src/old.rs b/src/new.rs\n",
|
||||
"similarity index 50%\n",
|
||||
"rename from src/old.rs\n",
|
||||
"rename to src/new.rs\n",
|
||||
"--- a/src/old.rs\n",
|
||||
"+++ b/src/new.rs\n",
|
||||
"@@ -1,1 +1,1 @@\n",
|
||||
"-fn old() {}\n",
|
||||
"+fn new() {}\n",
|
||||
);
|
||||
|
||||
/// A file whose path contains a space, which the `diff --git` line cannot
|
||||
/// express without ambiguity.
|
||||
const PATH_WITH_SPACE: &str = concat!(
|
||||
"diff --git a/src/my file.rs b/src/my file.rs\n",
|
||||
"--- a/src/my file.rs\n",
|
||||
"+++ b/src/my file.rs\n",
|
||||
"@@ -1,1 +1,2 @@\n",
|
||||
" fn a() {}\n",
|
||||
"+fn b() {}\n",
|
||||
);
|
||||
|
||||
/// A file whose path git quotes and escapes (`caf\\303\\251.md` is
|
||||
/// `caf\u{e9}.md`).
|
||||
const QUOTED_PATH: &str = concat!(
|
||||
"diff --git \"a/docs/caf\\303\\251.md\" \"b/docs/caf\\303\\251.md\"\n",
|
||||
"--- \"a/docs/caf\\303\\251.md\"\n",
|
||||
"+++ \"b/docs/caf\\303\\251.md\"\n",
|
||||
"@@ -1,1 +1,2 @@\n",
|
||||
" intro\n",
|
||||
"+ajout\n",
|
||||
);
|
||||
|
||||
fn review(filename: &str, line: Option<u64>, side: Option<ReviewSide>) -> ReviewItem {
|
||||
ReviewItem {
|
||||
filename: String::from(filename),
|
||||
line,
|
||||
side,
|
||||
message: String::from("message"),
|
||||
}
|
||||
}
|
||||
|
||||
fn review_result(reviews: Vec<ReviewItem>) -> ReviewResult {
|
||||
ReviewResult {
|
||||
reviews,
|
||||
comment: String::new(),
|
||||
cost: None,
|
||||
}
|
||||
}
|
||||
|
||||
fn pull_request_file(
|
||||
filename: &str,
|
||||
previous_filename: Option<&str>,
|
||||
status: &str,
|
||||
) -> PullRequestFile {
|
||||
PullRequestFile {
|
||||
filename: String::from(filename),
|
||||
previous_filename: previous_filename.map(String::from),
|
||||
status: String::from(status),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn changed_lines_are_listed_per_file_and_side() {
|
||||
let expected = concat!(
|
||||
"src/foo.rs: added 2, 4, 5 / removed 2\n",
|
||||
"src/bar.rs: added 11, 13"
|
||||
);
|
||||
|
||||
assert_eq!(format_changed_lines(&parse_changed_lines(DIFF)), expected);
|
||||
}
|
||||
|
||||
#[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 changes_describe_the_files_then_the_lines() {
|
||||
let changed_lines = parse_changed_lines(DELETION_ONLY);
|
||||
let files = vec![pull_request_file("src/old.rs", None, "deleted")];
|
||||
|
||||
let expected = concat!(
|
||||
"Files changed by the pull request: src/old.rs (deleted)\n",
|
||||
"\n",
|
||||
"Lines to review, per file:\n",
|
||||
"src/old.rs: removed 1, 2, 3"
|
||||
);
|
||||
|
||||
assert_eq!(format_changes(&files, &changed_lines), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn changes_without_the_api_list_only_hold_the_lines() {
|
||||
let changed_lines = parse_changed_lines(DELETION_ONLY);
|
||||
|
||||
let expected = "Lines to review, per file:\nsrc/old.rs: removed 1, 2, 3";
|
||||
|
||||
assert_eq!(format_changes(&[], &changed_lines), expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_renamed_file_is_described_with_its_previous_path() {
|
||||
let file = pull_request_file("src/new.rs", Some("src/old.rs"), "renamed");
|
||||
|
||||
assert_eq!(describe_file(&file), "src/new.rs (renamed from src/old.rs)");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn reviews_keep_their_changed_line_and_resolve_their_side() {
|
||||
let changed_lines = parse_changed_lines(DIFF);
|
||||
let mut review_result = review_result(vec![
|
||||
review("src/foo.rs", Some(4), Some(ReviewSide::Added)),
|
||||
review("src/foo.rs", Some(2), Some(ReviewSide::Removed)),
|
||||
review("src/bar.rs", Some(13), None),
|
||||
]);
|
||||
|
||||
resolve_review_sides(&mut review_result, &changed_lines);
|
||||
|
||||
let sides = review_result
|
||||
.reviews
|
||||
.iter()
|
||||
.map(|review| review.side)
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_eq!(
|
||||
sides,
|
||||
vec![
|
||||
Some(ReviewSide::Added),
|
||||
Some(ReviewSide::Removed),
|
||||
Some(ReviewSide::Added)
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_line_changed_on_both_sides_defaults_to_added() {
|
||||
let changed_lines = parse_changed_lines(DIFF);
|
||||
let mut review_result = review_result(vec![review("src/foo.rs", Some(2), None)]);
|
||||
|
||||
resolve_review_sides(&mut review_result, &changed_lines);
|
||||
|
||||
assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Added));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn reviews_outside_changed_lines_are_dropped() {
|
||||
let changed_lines = parse_changed_lines(DIFF);
|
||||
let mut review_result = review_result(vec![
|
||||
// valid
|
||||
review("src/foo.rs", Some(2), Some(ReviewSide::Added)),
|
||||
// a line that exists but is not part of the change
|
||||
review("src/foo.rs", Some(3), None),
|
||||
// a line beyond the change
|
||||
review("src/foo.rs", Some(999), None),
|
||||
// a changed line, but on the wrong side
|
||||
review("src/foo.rs", Some(4), Some(ReviewSide::Removed)),
|
||||
// a line changed in another file
|
||||
review("src/foo.rs", Some(11), None),
|
||||
// an unknown file
|
||||
review("src/baz.rs", Some(1), None),
|
||||
// no line at all
|
||||
review("src/bar.rs", None, None),
|
||||
]);
|
||||
|
||||
resolve_review_sides(&mut review_result, &changed_lines);
|
||||
|
||||
assert_eq!(review_result.reviews.len(), 1);
|
||||
assert_eq!(review_result.reviews[0].filename, "src/foo.rs");
|
||||
assert_eq!(review_result.reviews[0].line, Some(2));
|
||||
assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Added));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_deleted_line_is_kept_as_a_removed_anchor() {
|
||||
let changed_lines = parse_changed_lines(DELETION_ONLY);
|
||||
let mut review_result = review_result(vec![
|
||||
review("src/old.rs", Some(2), Some(ReviewSide::Removed)),
|
||||
review("src/old.rs", Some(2), Some(ReviewSide::Added)),
|
||||
]);
|
||||
|
||||
resolve_review_sides(&mut review_result, &changed_lines);
|
||||
|
||||
assert_eq!(review_result.reviews.len(), 1);
|
||||
assert_eq!(review_result.reviews[0].side, Some(ReviewSide::Removed));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn odd_sides_from_the_model_are_tolerated() {
|
||||
let changed_lines = parse_changed_lines(DIFF);
|
||||
let mut review_result: ReviewResult = serde_json::from_str(
|
||||
r#"{
|
||||
"reviews": [
|
||||
{ "filename": "src/foo.rs", "line": 4, "side": "Added", "message": "a" },
|
||||
{ "filename": "src/foo.rs", "line": 2, "side": "new", "message": "b" },
|
||||
{ "filename": "src/foo.rs", "line": 2, "side": "REMOVED", "message": "c" },
|
||||
{ "filename": "src/foo.rs", "line": 2, "side": "banana", "message": "d" },
|
||||
{ "filename": "src/foo.rs", "line": 5, "message": "e" }
|
||||
],
|
||||
"comment": ""
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
resolve_review_sides(&mut review_result, &changed_lines);
|
||||
|
||||
let sides = review_result
|
||||
.reviews
|
||||
.iter()
|
||||
.map(|review| review.side)
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_eq!(
|
||||
sides,
|
||||
vec![
|
||||
Some(ReviewSide::Added),
|
||||
Some(ReviewSide::Added),
|
||||
Some(ReviewSide::Removed),
|
||||
Some(ReviewSide::Added),
|
||||
Some(ReviewSide::Added)
|
||||
]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user