diff --git a/Cargo.lock b/Cargo.lock index 592d785a..c735e9e0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4669,6 +4669,12 @@ version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" +[[package]] +name = "similar" +version = "2.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" + [[package]] name = "siphasher" version = "1.0.3" @@ -4875,6 +4881,7 @@ dependencies = [ "serde_json", "serde_norway", "sha2 0.11.0", + "similar", "strum", "swarm-authelia-bridge-sock", "swarm-matrix-client", diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index ff3bc8bc..f23f0c51 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -287,6 +287,15 @@ Only the last part differs per classification: that classification's description and the task to classify the PR against it. The identical prefix lets the runtime's prompt cache reuse it across questions. +The controller builds the diff itself, from each changed file at the PR's +merge base and at its head, with 200 lines of context around each change +rather than the 3 of the forge's `.diff`. That keeps a moved block's +enclosing gate in view, such as a `config = lib.mkIf …` or an option's +`default`, so a move that restates the gate doesn't read as adding one. The +controller sends the forge's `.diff` instead when the PR changes more than +100 files, when the forge names no merge base, or when the wide diff is over +256 KiB. The review then says so, with the reason. + On the API key runtime the commits and diff are one content block of the user message and the question another, with a cache breakpoint at the end of the first. The cache writes an entry only where a breakpoint is, and `claude` sets @@ -619,8 +628,9 @@ These get settled, and filled in here, as the work reaches them: - **Cache strategy.** Fully independent sessions per classification, or one warm session and then the rest in parallel. To measure through the cache token counts before choosing. -- **Oversized diffs.** What happens when the diff doesn't fit the model's - context. Lockfiles already collapse to one line each, as in `hive-forge diff`. +- **Oversized diffs.** What happens when even the forge's 3-line diff doesn't fit + the model's context. Lockfiles already collapse to one line each, as in + `hive-forge diff`. - **Budget.** Any per-run or per-day spend limit on top of the concurrency cap. - **Diff-leaves-swarm policy.** On the API key and ACP runtimes the diff diff --git a/swarm-controller/Cargo.toml b/swarm-controller/Cargo.toml index 8ae5ca1f..56e7eefa 100644 --- a/swarm-controller/Cargo.toml +++ b/swarm-controller/Cargo.toml @@ -63,6 +63,9 @@ opentelemetry-otlp.workspace = true # is departing, not arriving — see that module's docs. hmac.workspace = true sha2.workspace = true +# `forge/pull_diff.rs` diffs a PR's changed files itself, at a context width +# the forge's own `.diff` does not offer. +similar.workspace = true # Validates `POST /api/agents`' `name` before it becomes `agent`/`repo` # everywhere downstream — see `create_agent`'s doc comment for why this is # defense-in-depth, not the only gate (per an argus review finding). diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 653033db..b9347970 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -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, 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, } /// 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 { + 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> { + 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>>) { + 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"}, diff --git a/swarm-controller/src/forge/pull_diff.rs b/swarm-controller/src/forge/pull_diff.rs new file mode 100644 index 00000000..2972aa05 --- /dev/null +++ b/swarm-controller/src/forge/pull_diff.rs @@ -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>, + pub new: Option>, +} + +/// Why a PR of `count` changed files gets the forge's `.diff`, if it does. +pub fn too_many_files(count: usize) -> Option { + (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 { + 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 { + 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()); + } +} diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index 82ea7982..2a429bbe 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -784,6 +784,7 @@ pub(crate) mod tests { pub(crate) sent: Arc>>, pub(crate) pushed: Arc>>, pub(crate) push_refusal: Option, + pub(crate) narrowed: Option, } 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()) diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index f84e6ef5..a771befc 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -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, Outcome) { +) -> (Option, Option, 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, 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}" + ); + } } diff --git a/swarm-queue-client/src/review_classifier.rs b/swarm-queue-client/src/review_classifier.rs index de3b73b8..7e4b9a94 100644 --- a/swarm-queue-client/src/review_classifier.rs +++ b/swarm-queue-client/src/review_classifier.rs @@ -55,7 +55,7 @@ pub struct TryOut { pub head_sha: String, /// Oldest first. pub commits: Vec, - /// The PR's unified diff, as the forge serves it. + /// The PR's unified diff from its merge base to its head. pub diff: String, pub classifications: Vec, /// The model every classification runs on, as claude's `--model` takes it.