swarm-controller: the review classifier reads a diff with 200 lines of context
The classifier read the forge's PR `.diff`, whose context is fixed at 3 lines. A block moved into a glue module lost its enclosing gate (a `config = lib.mkIf …` 13 to 191 lines up in the misfired moves #5199, #5200, #5201), so the glue restating that gate read as a new condition. The controller now builds the diff itself: it lists the PR's changed files, reads each at the merge base and at the head through the git tree and blob API with the token it already holds, and diffs them with 200 lines of context. It sends the forge's `.diff` instead when the PR changes more than 100 files, the forge names no merge base, or the wide diff is over 256 KiB, and the posted review then says so with the reason. Fixes #5227
This commit is contained in:
atlas
mara
parent
2ab8be61a2
commit
2b6d480d9c
8 changed files with 517 additions and 18 deletions
|
|
@ -29,7 +29,8 @@ use forgejo_api::structs::{
|
|||
CreateRepoOption, CreateStatusOption, CreateUserOption, EditUserOption, GetTreeQuery, GitEntry,
|
||||
IssueListIssuesQuery, IssueListIssuesQueryState, IssueListIssuesQueryType,
|
||||
RepoDownloadPullDiffOrPatchQuery, RepoGetContentsQuery, RepoGetPullRequestCommitsQuery,
|
||||
RepoListPullRequestsQuery, RepoListPullRequestsQueryState, RepoSearchQuery, StateType,
|
||||
RepoGetPullRequestFilesQuery, RepoListPullRequestsQuery, RepoListPullRequestsQueryState,
|
||||
RepoSearchQuery, StateType,
|
||||
};
|
||||
use forgejo_api::{ApiErrorKind, ForgejoError};
|
||||
use futures_util::{StreamExt as _, TryStreamExt as _};
|
||||
|
|
@ -51,6 +52,7 @@ pub mod docs_grant;
|
|||
pub mod hive_token;
|
||||
pub mod legacy_tokens;
|
||||
pub mod objects;
|
||||
pub mod pull_diff;
|
||||
pub mod runner_token;
|
||||
pub mod site_admin;
|
||||
|
||||
|
|
@ -114,6 +116,9 @@ pub struct PullForReview {
|
|||
pub head_sha: String,
|
||||
pub commits: Vec<Commit>,
|
||||
pub diff: String,
|
||||
/// Why `diff` is the forge's `.diff`, with 3 lines of context, rather
|
||||
/// than [`pull_diff::wide`]'s. `None` when it is the wide one.
|
||||
pub narrowed: Option<String>,
|
||||
}
|
||||
|
||||
/// Files on a repo's default branch, as [`Client::default_branch_files`] reads
|
||||
|
|
@ -1242,7 +1247,8 @@ impl Client {
|
|||
|
||||
/// What the review classifier reads of `owner/repo`'s PR `number`: its
|
||||
/// head commit, its commits oldest first with their full messages, and
|
||||
/// its unified diff.
|
||||
/// its unified diff from the merge base: [`pull_diff::wide`]'s, or the
|
||||
/// forge's `.diff` when the wide one is not read or is too large.
|
||||
///
|
||||
/// # Errors
|
||||
/// A forge failure, with the [`ForgejoError`] underneath; a 404 on the
|
||||
|
|
@ -1259,6 +1265,7 @@ impl Client {
|
|||
.call(|api| api.repo_get_pull_request(owner, repo, number).into_future())
|
||||
.await
|
||||
.with_context(|| format!("read PR {number} of {owner}/{repo}"))?;
|
||||
let (merge_base, changed_files) = (pull.merge_base, pull.changed_files);
|
||||
let head_sha = pull
|
||||
.head
|
||||
.and_then(|h| h.sha)
|
||||
|
|
@ -1287,8 +1294,40 @@ impl Client {
|
|||
message: c.commit.and_then(|m| m.message).unwrap_or_default(),
|
||||
})
|
||||
.collect();
|
||||
let diff = self
|
||||
.api
|
||||
let count = changed_files.map_or(0, |n| usize::try_from(n).unwrap_or(usize::MAX));
|
||||
let wide = match (pull_diff::too_many_files(count), merge_base) {
|
||||
(Some(reason), _) => Err(reason),
|
||||
(None, None) => Err("the forge names no merge base".to_owned()),
|
||||
(None, Some(base)) => {
|
||||
let files = self
|
||||
.changed_files(owner, repo, number, &base, &head_sha)
|
||||
.await?;
|
||||
tokio::task::spawn_blocking(move || pull_diff::wide(&files))
|
||||
.await
|
||||
.context("building the PR's wide diff")?
|
||||
}
|
||||
};
|
||||
let (diff, narrowed) = match wide {
|
||||
Ok(diff) => (diff, None),
|
||||
Err(reason) => {
|
||||
tracing::info!(
|
||||
%owner, %repo, pr = number, %reason,
|
||||
"the review classifier reads the forge's 3-line diff"
|
||||
);
|
||||
(self.forge_diff(owner, repo, number).await?, Some(reason))
|
||||
}
|
||||
};
|
||||
Ok(PullForReview {
|
||||
head_sha,
|
||||
commits,
|
||||
diff,
|
||||
narrowed,
|
||||
})
|
||||
}
|
||||
|
||||
/// The forge's own `.diff` of PR `number`, with 3 lines of context.
|
||||
async fn forge_diff(&self, owner: &str, repo: &str, number: i64) -> Result<String> {
|
||||
self.api
|
||||
.call(|api| {
|
||||
api.repo_download_pull_diff_or_patch(
|
||||
owner,
|
||||
|
|
@ -1300,12 +1339,55 @@ impl Client {
|
|||
.into_future()
|
||||
})
|
||||
.await
|
||||
.with_context(|| format!("read the diff of PR {number} of {owner}/{repo}"))?;
|
||||
Ok(PullForReview {
|
||||
head_sha,
|
||||
commits,
|
||||
diff,
|
||||
})
|
||||
.with_context(|| format!("read the diff of PR {number} of {owner}/{repo}"))
|
||||
}
|
||||
|
||||
/// Each file PR `number` changes, with its bytes at `base` and at `head`.
|
||||
/// A renamed file is read at its old path on the `base` side.
|
||||
async fn changed_files(
|
||||
&self,
|
||||
owner: &str,
|
||||
repo: &str,
|
||||
number: i64,
|
||||
base: &str,
|
||||
head: &str,
|
||||
) -> Result<Vec<pull_diff::FileChange>> {
|
||||
let listed = self
|
||||
.api
|
||||
.call(|api| {
|
||||
Box::pin(
|
||||
api.repo_get_pull_request_files(
|
||||
owner,
|
||||
repo,
|
||||
number,
|
||||
RepoGetPullRequestFilesQuery::default(),
|
||||
)
|
||||
.all(),
|
||||
)
|
||||
})
|
||||
.await
|
||||
.with_context(|| format!("list the files of PR {number} of {owner}/{repo}"))?;
|
||||
let mut trees = HashMap::new();
|
||||
let mut files = Vec::with_capacity(listed.len());
|
||||
for file in listed {
|
||||
let new_path = file
|
||||
.filename
|
||||
.with_context(|| format!("a file of PR {number} of {owner}/{repo} has no name"))?;
|
||||
let old_path = file.previous_filename.unwrap_or_else(|| new_path.clone());
|
||||
let old = self
|
||||
.file_at(owner, repo, base, &old_path, &mut trees)
|
||||
.await?;
|
||||
let new = self
|
||||
.file_at(owner, repo, head, &new_path, &mut trees)
|
||||
.await?;
|
||||
files.push(pull_diff::FileChange {
|
||||
old_path,
|
||||
new_path,
|
||||
old: old.map(|(_, bytes)| bytes),
|
||||
new: new.map(|(_, bytes)| bytes),
|
||||
});
|
||||
}
|
||||
Ok(files)
|
||||
}
|
||||
|
||||
/// The `paths` of `owner/repo` that exist on its default branch, all
|
||||
|
|
@ -2437,6 +2519,111 @@ mod tests {
|
|||
(Client { api, url }, seen)
|
||||
}
|
||||
|
||||
/// A stand-in forge holding PR 7 of `o/r`, head `h1`, which changes
|
||||
/// `m.nix` from `old` at merge base `mb` to `new`. `pull` is the PR's own
|
||||
/// JSON. Records every request's path.
|
||||
async fn pull_forge(
|
||||
pull: &'static str,
|
||||
old: &'static str,
|
||||
new: &'static str,
|
||||
) -> (Client, Arc<std::sync::Mutex<Vec<String>>>) {
|
||||
use axum::http::Uri;
|
||||
use base64::Engine as _;
|
||||
|
||||
let blob = |content: &str| {
|
||||
format!(
|
||||
r#"{{"encoding":"base64","content":"{}"}}"#,
|
||||
base64::engine::general_purpose::STANDARD.encode(content)
|
||||
)
|
||||
};
|
||||
let tree = |sha: &str, blob: &str| {
|
||||
format!(
|
||||
r#"{{"sha":"{sha}","truncated":false,"tree":[
|
||||
{{"path":"m.nix","type":"blob","sha":"{blob}"}}]}}"#
|
||||
)
|
||||
};
|
||||
let seen = Arc::new(std::sync::Mutex::new(Vec::new()));
|
||||
let recorded = Arc::clone(&seen);
|
||||
let app = axum::Router::new().fallback(move |uri: Uri| {
|
||||
let recorded = Arc::clone(&recorded);
|
||||
let body = match uri.path() {
|
||||
"/api/v1/repos/o/r/pulls/7" => Some(pull.to_owned()),
|
||||
"/api/v1/repos/o/r/pulls/7/commits" => Some("[]".to_owned()),
|
||||
"/api/v1/repos/o/r/pulls/7/files" => {
|
||||
Some(r#"[{"filename":"m.nix","status":"modified"}]"#.to_owned())
|
||||
}
|
||||
"/api/v1/repos/o/r/pulls/7.diff" => Some("forge diff\n".to_owned()),
|
||||
"/api/v1/repos/o/r/git/trees/mb" => Some(tree("mb", "bo")),
|
||||
"/api/v1/repos/o/r/git/trees/h1" => Some(tree("h1", "bn")),
|
||||
"/api/v1/repos/o/r/git/blobs/bo" => Some(blob(old)),
|
||||
"/api/v1/repos/o/r/git/blobs/bn" => Some(blob(new)),
|
||||
_ => None,
|
||||
};
|
||||
async move {
|
||||
recorded.lock().unwrap().push(uri.path().to_owned());
|
||||
let builder = axum::http::Response::builder();
|
||||
let (builder, body) = match body {
|
||||
Some(body) => {
|
||||
let count = if body == "[]" { "0" } else { "1" };
|
||||
(builder.status(200).header("x-total-count", count), body)
|
||||
}
|
||||
None => (builder.status(404), r#"{"message":"not found"}"#.to_owned()),
|
||||
};
|
||||
builder.body(axum::body::Body::from(body)).unwrap()
|
||||
}
|
||||
});
|
||||
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
|
||||
let url = url::Url::parse(&format!("http://{}/", listener.local_addr().unwrap())).unwrap();
|
||||
tokio::spawn(async move { axum::serve(listener, app).await });
|
||||
let api = crate::forge::api::tests::fixed(url.clone(), "stub-token");
|
||||
(Client { api, url }, seen)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn a_review_reads_the_wide_diff_from_the_merge_base_or_says_why_not() {
|
||||
let old = "{\n config = lib.mkIf gate {\n a = 1;\n b = 2;\n c = 3;\n d = 4;\n e = 5;\n };\n}\n";
|
||||
let new = "{\n config = lib.mkIf gate {\n a = 1;\n b = 2;\n c = 3;\n d = 4;\n };\n}\n";
|
||||
let wide = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"mb","changed_files":1}"#;
|
||||
let (client, _) = pull_forge(wide, old, new).await;
|
||||
let read = client.pull_for_review("o", "r", 7).await.unwrap();
|
||||
assert_eq!(read.narrowed, None);
|
||||
assert!(
|
||||
read.diff
|
||||
.starts_with("diff --git a/m.nix b/m.nix\n--- a/m.nix\n+++ b/m.nix\n"),
|
||||
"{}",
|
||||
read.diff
|
||||
);
|
||||
assert!(
|
||||
read.diff.contains("\n config = lib.mkIf gate {\n"),
|
||||
"{}",
|
||||
read.diff
|
||||
);
|
||||
assert!(read.diff.contains("\n- e = 5;\n"), "{}", read.diff);
|
||||
|
||||
let many = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"mb","changed_files":101}"#;
|
||||
let (client, seen) = pull_forge(many, old, new).await;
|
||||
let read = client.pull_for_review("o", "r", 7).await.unwrap();
|
||||
assert_eq!(read.diff, "forge diff\n");
|
||||
assert_eq!(
|
||||
read.narrowed.as_deref(),
|
||||
Some("the PR changes 101 files, more than the 100 a wide diff is read for")
|
||||
);
|
||||
assert!(
|
||||
!seen.lock().unwrap().iter().any(|p| p.ends_with("/files")),
|
||||
"no file is read for a PR past the limit"
|
||||
);
|
||||
|
||||
let no_base =
|
||||
r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"changed_files":1}"#;
|
||||
let (client, _) = pull_forge(no_base, old, new).await;
|
||||
let read = client.pull_for_review("o", "r", 7).await.unwrap();
|
||||
assert_eq!(read.diff, "forge diff\n");
|
||||
assert_eq!(
|
||||
read.narrowed.as_deref(),
|
||||
Some("the forge names no merge base")
|
||||
);
|
||||
}
|
||||
|
||||
/// A root tree with the `.hyperhive` tree and a blob beside it.
|
||||
const ROOT: &str = r#"{"sha":"def456","truncated":false,"tree":[
|
||||
{"path":"README.md","type":"blob","sha":"b0"},
|
||||
|
|
|
|||
235
swarm-controller/src/forge/pull_diff.rs
Normal file
235
swarm-controller/src/forge/pull_diff.rs
Normal file
|
|
@ -0,0 +1,235 @@
|
|||
//! The diff the review classifier reads, built from both sides of each
|
||||
//! changed file rather than taken from the forge's `.diff`, whose context is
|
||||
//! fixed at 3 lines. With [`CONTEXT_LINES`] of context a moved block keeps
|
||||
//! its enclosing gate (a `config = lib.mkIf …`, an option's `default`) in
|
||||
//! view, so a move that restates that gate does not read as adding one.
|
||||
|
||||
use std::fmt::Write as _;
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
use similar::TextDiff;
|
||||
|
||||
/// Lines of context around each change. In this repo's Nix modules a block's
|
||||
/// enclosing `config = lib.mkIf …` sits up to 191 lines above it.
|
||||
pub const CONTEXT_LINES: usize = 200;
|
||||
|
||||
/// The largest wide diff sent, in bytes, about 64k tokens of code. A larger
|
||||
/// one gives way to the forge's `.diff`.
|
||||
pub const MAX_BYTES: usize = 256 * 1024;
|
||||
|
||||
/// The most changed files a wide diff is read for. Each costs a few forge
|
||||
/// reads per side, all within the one deadline the PR read runs under.
|
||||
pub const MAX_FILES: usize = 100;
|
||||
|
||||
/// How long one file's diff may search for the smallest edit before settling
|
||||
/// for a larger one. Without it a rewritten file of tens of thousands of
|
||||
/// lines takes minutes.
|
||||
const DIFF_TIMEOUT: Duration = Duration::from_secs(1);
|
||||
|
||||
/// One changed file: its path and bytes at the merge base and at the head,
|
||||
/// `None` on a side where it does not exist.
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub struct FileChange {
|
||||
pub old_path: String,
|
||||
pub new_path: String,
|
||||
pub old: Option<Vec<u8>>,
|
||||
pub new: Option<Vec<u8>>,
|
||||
}
|
||||
|
||||
/// Why a PR of `count` changed files gets the forge's `.diff`, if it does.
|
||||
pub fn too_many_files(count: usize) -> Option<String> {
|
||||
(count > MAX_FILES).then(|| {
|
||||
format!("the PR changes {count} files, more than the {MAX_FILES} a wide diff is read for")
|
||||
})
|
||||
}
|
||||
|
||||
/// `files` as one diff with [`CONTEXT_LINES`] of context, or why the forge's
|
||||
/// `.diff` stands in: the result is over [`MAX_BYTES`].
|
||||
pub fn wide(files: &[FileChange]) -> Result<String, String> {
|
||||
let diff = render(files, CONTEXT_LINES);
|
||||
if diff.len() > MAX_BYTES {
|
||||
return Err(format!(
|
||||
"the diff with {CONTEXT_LINES} lines of context is {} bytes, over the {MAX_BYTES} \
|
||||
a review sends",
|
||||
diff.len()
|
||||
));
|
||||
}
|
||||
Ok(diff)
|
||||
}
|
||||
|
||||
/// `files` as one unified diff with `context` lines around each change, each
|
||||
/// file in `git diff`'s shape: a `diff --git` line, `---`/`+++`, then hunks.
|
||||
/// A file with a non-UTF-8 or NUL-bearing side is one `Binary files` line.
|
||||
pub fn render(files: &[FileChange], context: usize) -> String {
|
||||
let mut out = String::new();
|
||||
for file in files {
|
||||
section(&mut out, file, context);
|
||||
}
|
||||
out
|
||||
}
|
||||
|
||||
fn section(out: &mut String, file: &FileChange, context: usize) {
|
||||
let (old_path, new_path) = (&file.old_path, &file.new_path);
|
||||
let _ = writeln!(out, "diff --git a/{old_path} b/{new_path}");
|
||||
if file.old.is_some() && file.new.is_some() && old_path != new_path {
|
||||
let _ = writeln!(out, "rename from {old_path}\nrename to {new_path}");
|
||||
}
|
||||
let a = file
|
||||
.old
|
||||
.as_ref()
|
||||
.map_or_else(|| "/dev/null".to_owned(), |_| format!("a/{old_path}"));
|
||||
let b = file
|
||||
.new
|
||||
.as_ref()
|
||||
.map_or_else(|| "/dev/null".to_owned(), |_| format!("b/{new_path}"));
|
||||
let (Some(old), Some(new)) = (text(file.old.as_deref()), text(file.new.as_deref())) else {
|
||||
let _ = writeln!(out, "Binary files {a} and {b} differ");
|
||||
return;
|
||||
};
|
||||
let diff = TextDiff::configure()
|
||||
.timeout(DIFF_TIMEOUT)
|
||||
.diff_lines(old, new);
|
||||
let _ = write!(
|
||||
out,
|
||||
"{}",
|
||||
diff.unified_diff().context_radius(context).header(&a, &b)
|
||||
);
|
||||
}
|
||||
|
||||
/// A side's text: empty when the file is absent, `None` when it is binary.
|
||||
fn text(bytes: Option<&[u8]>) -> Option<&str> {
|
||||
let bytes = bytes.unwrap_or_default();
|
||||
if bytes.contains(&0) {
|
||||
return None;
|
||||
}
|
||||
std::str::from_utf8(bytes).ok()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn change(path: &str, old: Option<&str>, new: Option<&str>) -> FileChange {
|
||||
FileChange {
|
||||
old_path: path.to_owned(),
|
||||
new_path: path.to_owned(),
|
||||
old: old.map(|s| s.as_bytes().to_vec()),
|
||||
new: new.map(|s| s.as_bytes().to_vec()),
|
||||
}
|
||||
}
|
||||
|
||||
/// A Nix module whose `config = lib.mkIf …` block opens `gap` lines above
|
||||
/// a firewall rule on the wireguard interface.
|
||||
fn module(gap: usize, with_rule: bool) -> String {
|
||||
let mut s =
|
||||
String::from("{ lib, config, ... }:\n{\n config = lib.mkIf deployCfg.nats.enable {\n");
|
||||
for i in 0..gap {
|
||||
let _ = writeln!(s, " services.nats.settings.k{i} = {i};");
|
||||
}
|
||||
if with_rule {
|
||||
s.push_str(
|
||||
" networking.firewall.interfaces.wg0 = {\n allowedTCPPorts = lib.mkIf \
|
||||
deployCfg.wireguard.enable [ 4222 ];\n };\n",
|
||||
);
|
||||
}
|
||||
for i in 0..5 {
|
||||
let _ = writeln!(s, " systemd.services.nats.environment.E{i} = \"{i}\";");
|
||||
}
|
||||
s.push_str(" };\n}\n");
|
||||
s
|
||||
}
|
||||
|
||||
/// The rule moves out of the module into a glue module that restates
|
||||
/// both conditions.
|
||||
fn move_into_glue() -> Vec<FileChange> {
|
||||
let glue = "{ lib, config, ... }:\n{\n config = lib.mkIf (deployCfg.nats.enable && \
|
||||
deployCfg.wireguard.enable) {\n networking.firewall.interfaces.wg0.\
|
||||
allowedTCPPorts = [ 4222 ];\n };\n}\n";
|
||||
vec![
|
||||
change("nix/host-modules/glue-nats-wireguard.nix", None, Some(glue)),
|
||||
change(
|
||||
"nix/host-modules/swarm-nats.nix",
|
||||
Some(&module(47, true)),
|
||||
Some(&module(47, false)),
|
||||
),
|
||||
]
|
||||
}
|
||||
|
||||
/// `count` lines, line `i` reading `line(i)`.
|
||||
fn numbered(count: usize, line: impl Fn(usize) -> String) -> String {
|
||||
(0..count).fold(String::new(), |mut s, i| {
|
||||
let _ = writeln!(s, "{}", line(i));
|
||||
s
|
||||
})
|
||||
}
|
||||
|
||||
const GATE: &str = "config = lib.mkIf deployCfg.nats.enable {";
|
||||
|
||||
#[test]
|
||||
fn a_moved_block_keeps_its_enclosing_gate_in_view() {
|
||||
let diff = wide(&move_into_glue()).expect("fits");
|
||||
assert!(diff.contains(&format!("\n {GATE}\n")), "{diff}");
|
||||
assert!(diff.contains("\n- allowedTCPPorts = lib.mkIf deployCfg.wireguard.enable"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn three_lines_of_context_leave_that_gate_out() {
|
||||
let diff = render(&move_into_glue(), 3);
|
||||
assert!(!diff.contains(GATE), "{diff}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn context_is_as_wide_as_asked_and_no_wider() {
|
||||
let old = numbered(30, |i| format!("line {i}"));
|
||||
let new = old.replace("line 15\n", "line fifteen\n");
|
||||
let at = |n| render(&[change("f", Some(&old), Some(&new))], n);
|
||||
let diff = at(10);
|
||||
assert!(diff.contains("@@ -6,21 +6,21 @@"), "{diff}");
|
||||
assert!(diff.contains(" line 5\n") && !diff.contains("line 4\n"));
|
||||
assert!(at(CONTEXT_LINES).contains("@@ -1,30 +1,30 @@"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_headers_follow_git_diff() {
|
||||
let diff = render(
|
||||
&[
|
||||
change("added", None, Some("a\n")),
|
||||
change("gone", Some("a\n"), None),
|
||||
FileChange {
|
||||
old_path: "old".to_owned(),
|
||||
new_path: "new".to_owned(),
|
||||
old: Some(b"a\n".to_vec()),
|
||||
new: Some(b"b\n".to_vec()),
|
||||
},
|
||||
change("img", Some("a\n"), Some("\0")),
|
||||
],
|
||||
3,
|
||||
);
|
||||
assert_eq!(
|
||||
diff,
|
||||
"diff --git a/added b/added\n--- /dev/null\n+++ b/added\n@@ -0,0 +1 @@\n+a\n\
|
||||
diff --git a/gone b/gone\n--- a/gone\n+++ /dev/null\n@@ -1 +0,0 @@\n-a\n\
|
||||
diff --git a/old b/new\nrename from old\nrename to new\n--- a/old\n+++ b/new\n\
|
||||
@@ -1 +1 @@\n-a\n+b\n\
|
||||
diff --git a/img b/img\nBinary files a/img and b/img differ\n"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_wide_diff_over_the_limit_gives_way() {
|
||||
let big = numbered(MAX_BYTES / 8, |i| format!("{i:07}"));
|
||||
let edited = numbered(MAX_BYTES / 8, |i| format!("{i:07}x"));
|
||||
let files = [change("big", Some(&big), Some(&edited))];
|
||||
let reason = wide(&files).expect_err("over the limit");
|
||||
assert!(reason.contains("over the"), "{reason}");
|
||||
let small = [change("small", Some("a\n"), Some("b\n"))];
|
||||
assert!(wide(&small).is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn more_files_than_the_limit_give_way() {
|
||||
assert!(too_many_files(MAX_FILES).is_none());
|
||||
assert!(too_many_files(MAX_FILES + 1).is_some());
|
||||
}
|
||||
}
|
||||
|
|
@ -784,6 +784,7 @@ pub(crate) mod tests {
|
|||
pub(crate) sent: Arc<std::sync::Mutex<Vec<TryOut>>>,
|
||||
pub(crate) pushed: Arc<std::sync::Mutex<Vec<(String, BranchCommit)>>>,
|
||||
pub(crate) push_refusal: Option<StatusCode>,
|
||||
pub(crate) narrowed: Option<String>,
|
||||
}
|
||||
|
||||
impl Fake {
|
||||
|
|
@ -795,6 +796,7 @@ pub(crate) mod tests {
|
|||
sent: Arc::default(),
|
||||
pushed: Arc::default(),
|
||||
push_refusal: None,
|
||||
narrowed: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -842,6 +844,7 @@ pub(crate) mod tests {
|
|||
head_sha: "abc123".to_owned(),
|
||||
commits: Vec::new(),
|
||||
diff: String::new(),
|
||||
narrowed: self.narrowed.clone(),
|
||||
})
|
||||
} else {
|
||||
Err(ForgejoError::ApiError(ApiErrorKind::NotFound { errors: None }.into()).into())
|
||||
|
|
|
|||
|
|
@ -30,6 +30,7 @@ use super::settings::{Settings, SettingsSource};
|
|||
use super::{FileSource, PullSource, RunSink, split_repo};
|
||||
use crate::AppState;
|
||||
use crate::agent_renewal::unix_now;
|
||||
use crate::forge::pull_diff::CONTEXT_LINES;
|
||||
use crate::forge::{self, classifier_account::LOGIN, classifier_account::Review};
|
||||
|
||||
/// The most review runs waiting for a slot.
|
||||
|
|
@ -450,8 +451,15 @@ async fn run(
|
|||
req: &Requested,
|
||||
) -> RunRecord {
|
||||
let started_unix = unix_now();
|
||||
let (head, outcome) = classify(files, pulls, sink, req).await;
|
||||
let ruling = review_for(&outcome);
|
||||
let (head, narrowed, outcome) = classify(files, pulls, sink, req).await;
|
||||
let mut ruling = review_for(&outcome);
|
||||
if let (Some(reason), Outcome::Ran { .. }) = (narrowed, &outcome) {
|
||||
let _ = write!(
|
||||
ruling.body,
|
||||
"\nThe classifier read the forge's diff, with 3 lines of context, not \
|
||||
{CONTEXT_LINES}: {reason}.\n"
|
||||
);
|
||||
}
|
||||
let posted_as = match ruling.event {
|
||||
Event::Approve => RunOutcome::Approved,
|
||||
Event::RequestChanges => RunOutcome::ChangesRequested,
|
||||
|
|
@ -620,13 +628,14 @@ fn record(
|
|||
}
|
||||
}
|
||||
|
||||
/// The PR's head, when it was read, and how far the run got.
|
||||
/// The PR's head when it was read, why its diff is the forge's 3-line one
|
||||
/// when it is, and how far the run got.
|
||||
async fn classify(
|
||||
files: &impl FileSource,
|
||||
pulls: &impl PullSource,
|
||||
sink: &(impl RunSink + SettingsSource),
|
||||
req: &Requested,
|
||||
) -> (Option<String>, Outcome) {
|
||||
) -> (Option<String>, Option<String>, Outcome) {
|
||||
let repo = format!("{}/{}", req.owner, req.name);
|
||||
let pull = match forge::within_forge_deadline(
|
||||
"review classifier PR read",
|
||||
|
|
@ -637,24 +646,28 @@ async fn classify(
|
|||
Some(Ok(pull)) => pull,
|
||||
Some(Err(e)) => {
|
||||
return (
|
||||
None,
|
||||
None,
|
||||
Outcome::NotRun(format!("reading the PR failed: {e:#}")),
|
||||
);
|
||||
}
|
||||
None => {
|
||||
return (
|
||||
None,
|
||||
None,
|
||||
Outcome::NotRun("the forge did not answer within the deadline".to_owned()),
|
||||
);
|
||||
}
|
||||
};
|
||||
let head = Some(pull.head_sha.clone());
|
||||
let narrowed = pull.narrowed;
|
||||
let rules = match super::rules(files, &req.owner, &req.name).await {
|
||||
Ok(axum::Json(rules)) => rules,
|
||||
Err(problem) => {
|
||||
let detail = problem.detail.unwrap_or_default();
|
||||
return (
|
||||
head,
|
||||
narrowed,
|
||||
Outcome::NotRun(format!("the rules did not load: {detail}")),
|
||||
);
|
||||
}
|
||||
|
|
@ -664,6 +677,7 @@ async fn classify(
|
|||
Err(e) => {
|
||||
return (
|
||||
head,
|
||||
narrowed,
|
||||
Outcome::NotRun(format!("reading the classifier's settings failed: {e:#}")),
|
||||
);
|
||||
}
|
||||
|
|
@ -673,6 +687,7 @@ async fn classify(
|
|||
Err(e) => {
|
||||
return (
|
||||
head,
|
||||
narrowed,
|
||||
Outcome::NotRun(format!("the classifier's settings: {e}")),
|
||||
);
|
||||
}
|
||||
|
|
@ -697,7 +712,7 @@ async fn classify(
|
|||
},
|
||||
Err(e) => Outcome::NotRun(format!("encoding the run failed: {e}")),
|
||||
};
|
||||
(head, outcome)
|
||||
(head, narrowed, outcome)
|
||||
}
|
||||
|
||||
/// What a run posts: the event, the review body, and why it has no clean
|
||||
|
|
@ -1993,4 +2008,43 @@ mod tests {
|
|||
assert_eq!(Requested::parse(body.as_bytes()), None, "{body}");
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn a_run_on_the_forge_diff_says_so_in_its_review() {
|
||||
/// The body a run posts when the PR read was narrowed for `reason`,
|
||||
/// with the rules on the default branch or without them.
|
||||
async fn narrowed(reason: Option<String>, rules: bool) -> String {
|
||||
let reply = Reply::Done {
|
||||
rows: vec![row(None, true, true)],
|
||||
overall: Overall::Approve,
|
||||
};
|
||||
let files = if rules {
|
||||
vec![rules_json()]
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
let mut fake = Fake::new(reply, files);
|
||||
fake.narrowed = reason;
|
||||
let poster = Posted::default();
|
||||
run(&fake, &fake, &fake, &poster, &request("r")).await;
|
||||
poster.0.into_inner().expect("unpoisoned").remove(0).2.body
|
||||
}
|
||||
let reason = || Some("the PR changes 101 files".to_owned());
|
||||
let body = narrowed(reason(), true).await;
|
||||
assert!(
|
||||
body.ends_with(
|
||||
"\nThe classifier read the forge's diff, with 3 lines of context, not 200: \
|
||||
the PR changes 101 files.\n"
|
||||
),
|
||||
"{body}"
|
||||
);
|
||||
let body = narrowed(None, true).await;
|
||||
assert!(!body.contains("forge's diff"), "{body}");
|
||||
let body = narrowed(reason(), false).await;
|
||||
assert!(body.starts_with("**No verdict:**"), "{body}");
|
||||
assert!(
|
||||
!body.contains("forge's diff"),
|
||||
"a run that never ran read no diff: {body}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue