diff --git a/Cargo.lock b/Cargo.lock index c735e9e0..592d785a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4669,12 +4669,6 @@ 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" @@ -4881,7 +4875,6 @@ 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 4d7819b2..ff3bc8bc 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -287,17 +287,6 @@ 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 25 lines of context around each change -rather than the 3 of the forge's `.diff`. An enclosing gate, such as a -`config = lib.mkIf …` or an option's `default`, is in view when it's within -25 lines of the change, so a move that restates it doesn't read as adding one. -A gate further away is still out of view. The -controller sends the forge's `.diff` instead when the PR changes more than -100 files, when the forge names no merge base, when reading a changed file -fails, 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 @@ -630,9 +619,8 @@ 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 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`. +- **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`. - **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 56e7eefa..8ae5ca1f 100644 --- a/swarm-controller/Cargo.toml +++ b/swarm-controller/Cargo.toml @@ -63,9 +63,6 @@ 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 e405c995..653033db 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -29,8 +29,7 @@ use forgejo_api::structs::{ CreateRepoOption, CreateStatusOption, CreateUserOption, EditUserOption, GetTreeQuery, GitEntry, IssueListIssuesQuery, IssueListIssuesQueryState, IssueListIssuesQueryType, RepoDownloadPullDiffOrPatchQuery, RepoGetContentsQuery, RepoGetPullRequestCommitsQuery, - RepoGetPullRequestFilesQuery, RepoListPullRequestsQuery, RepoListPullRequestsQueryState, - RepoSearchQuery, StateType, + RepoListPullRequestsQuery, RepoListPullRequestsQueryState, RepoSearchQuery, StateType, }; use forgejo_api::{ApiErrorKind, ForgejoError}; use futures_util::{StreamExt as _, TryStreamExt as _}; @@ -52,7 +51,6 @@ 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; @@ -116,9 +114,6 @@ 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 @@ -1247,9 +1242,7 @@ 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 from the merge base: [`pull_diff::wide`]'s, or the - /// forge's `.diff` when the wide one is not read, cannot be read, or is - /// too large. + /// its unified diff. /// /// # Errors /// A forge failure, with the [`ForgejoError`] underneath; a 404 on the @@ -1266,7 +1259,6 @@ 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) @@ -1295,43 +1287,8 @@ impl Client { message: c.commit.and_then(|m| m.message).unwrap_or_default(), }) .collect(); - 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)) => { - match self - .changed_files(owner, repo, number, &base, &head_sha) - .await - { - Ok(files) => tokio::task::spawn_blocking(move || pull_diff::wide(&files)) - .await - .unwrap_or_else(|e| Err(format!("building the wide diff failed: {e}"))), - Err(e) => Err(format!("reading the files failed: {e:#}")), - } - } - }; - 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 + let diff = self + .api .call(|api| { api.repo_download_pull_diff_or_patch( owner, @@ -1343,55 +1300,12 @@ impl Client { .into_future() }) .await - .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) + .with_context(|| format!("read the diff of PR {number} of {owner}/{repo}"))?; + Ok(PullForReview { + head_sha, + commits, + diff, + }) } /// The `paths` of `owner/repo` that exist on its default branch, all @@ -2523,121 +2437,6 @@ 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") - ); - - let unreadable = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h2"},"merge_base":"mb","changed_files":1}"#; - let (client, _) = pull_forge(unreadable, old, new).await; - let read = client.pull_for_review("o", "r", 7).await.unwrap(); - assert_eq!(read.diff, "forge diff\n"); - let reason = read.narrowed.expect("a failed file read falls back"); - assert!( - reason.starts_with("reading the files failed: read tree h2 of o/r"), - "{reason}" - ); - } - /// 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 deleted file mode 100644 index 79b397ea..00000000 --- a/swarm-controller/src/forge/pull_diff.rs +++ /dev/null @@ -1,272 +0,0 @@ -//! 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. [`CONTEXT_LINES`] of context shows an enclosing gate (a -//! `config = lib.mkIf …`, an option's `default`) only when it is within that -//! many lines of the change; a gate further away is still out of view. - -use std::fmt::{self, Write as _}; - -use std::time::Duration; - -use similar::TextDiff; - -/// Lines of context around each change. A block's enclosing -/// `config = lib.mkIf …` is in view when it opens within this many lines above -/// the block. -pub const CONTEXT_LINES: usize = 25; - -/// 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 { - render(files, CONTEXT_LINES, MAX_BYTES).map_err(|_| { - format!( - "the diff with {CONTEXT_LINES} lines of context is over the {MAX_BYTES} bytes a \ - review sends" - ) - }) -} - -/// `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. -/// -/// # Errors -/// The bytes rendered when the output passed `cap`; rendering stops there. -pub fn render(files: &[FileChange], context: usize, cap: usize) -> Result { - let mut out = Capped { - out: String::new(), - cap, - }; - for file in files { - if section(&mut out, file, context).is_err() { - return Err(out.out.len()); - } - } - Ok(out.out) -} - -/// A `String` whose writes fail once it holds more than `cap` bytes, which -/// stops a `Display` impl writing into it. -struct Capped { - out: String, - cap: usize, -} - -impl fmt::Write for Capped { - fn write_str(&mut self, s: &str) -> fmt::Result { - self.out.push_str(s); - if self.out.len() > self.cap { - Err(fmt::Error) - } else { - Ok(()) - } - } -} - -fn section(out: &mut Capped, file: &FileChange, context: usize) -> fmt::Result { - let (old_path, new_path) = (&file.old_path, &file.new_path); - writeln!(out, "diff --git a/{old_path} b/{new_path}")?; - if file.old.is_some() && file.new.is_some() && old_path != new_path { - 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 { - return writeln!(out, "Binary files {a} and {b} differ"); - }; - let diff = TextDiff::configure() - .timeout(DIFF_TIMEOUT) - .diff_lines(old, new); - 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(20, true)), - Some(&module(20, 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, usize::MAX).unwrap(); - 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, usize::MAX).unwrap(); - 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, - usize::MAX, - ) - .unwrap(); - 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 rendering_stops_once_past_the_limit() { - let huge = numbered(MAX_BYTES / 2, |i| format!("{i:07}")); - let files = [change("huge", None, Some(&huge))]; - let written = render(&files, CONTEXT_LINES, MAX_BYTES).expect_err("over the limit"); - assert!( - written <= MAX_BYTES + "0000000\n".len(), - "{written} bytes rendered of a {}-byte file", - huge.len() - ); - } - - #[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 2a429bbe..82ea7982 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -784,7 +784,6 @@ pub(crate) mod tests { pub(crate) sent: Arc>>, pub(crate) pushed: Arc>>, pub(crate) push_refusal: Option, - pub(crate) narrowed: Option, } impl Fake { @@ -796,7 +795,6 @@ pub(crate) mod tests { sent: Arc::default(), pushed: Arc::default(), push_refusal: None, - narrowed: None, } } } @@ -844,7 +842,6 @@ 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 5e698ec0..f84e6ef5 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -30,7 +30,6 @@ 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. @@ -451,15 +450,8 @@ async fn run( req: &Requested, ) -> RunRecord { let started_unix = unix_now(); - 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 (head, outcome) = classify(files, pulls, sink, req).await; + let ruling = review_for(&outcome); let posted_as = match ruling.event { Event::Approve => RunOutcome::Approved, Event::RequestChanges => RunOutcome::ChangesRequested, @@ -628,14 +620,13 @@ fn record( } } -/// 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. +/// The PR's head, when it was read, and how far the run got. async fn classify( files: &impl FileSource, pulls: &impl PullSource, sink: &(impl RunSink + SettingsSource), req: &Requested, -) -> (Option, Option, Outcome) { +) -> (Option, Outcome) { let repo = format!("{}/{}", req.owner, req.name); let pull = match forge::within_forge_deadline( "review classifier PR read", @@ -646,28 +637,24 @@ 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}")), ); } @@ -677,7 +664,6 @@ async fn classify( Err(e) => { return ( head, - narrowed, Outcome::NotRun(format!("reading the classifier's settings failed: {e:#}")), ); } @@ -687,7 +673,6 @@ async fn classify( Err(e) => { return ( head, - narrowed, Outcome::NotRun(format!("the classifier's settings: {e}")), ); } @@ -712,7 +697,7 @@ async fn classify( }, Err(e) => Outcome::NotRun(format!("encoding the run failed: {e}")), }; - (head, narrowed, outcome) + (head, outcome) } /// What a run posts: the event, the review body, and why it has no clean @@ -2008,43 +1993,4 @@ 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 25: \ - 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 7e4b9a94..de3b73b8 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 from its merge base to its head. + /// The PR's unified diff, as the forge serves it. pub diff: String, pub classifications: Vec, /// The model every classification runs on, as claude's `--model` takes it.