vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

Compare commits

base: vinzenz:8e98f3e1093e882220333c5223161516028c1beb
Branches Tags
vinzenz:main
..
compare: vinzenz:2ab8be61a2feeaa1c1a043d08bf5909efbf89458
Branches Tags
vinzenz:main
8 changed files with 18 additions and 570 deletions
Download patch file Download diff file Expand all files Collapse all files

7
Cargo.lock generated
View file

@ -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",

16
docs/swarm/review-classifier.md
View file

@ -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

3
swarm-controller/Cargo.toml
View file

@ -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).

221
swarm-controller/src/forge.rs
View file

@ -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<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
@ -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<String> {
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<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)
.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<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")
);
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"},

272
swarm-controller/src/forge/pull_diff.rs
View file

@ -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<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> {
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<String, usize> {
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<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(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());
}
}

3
swarm-controller/src/review_classifier.rs
View file

@ -784,7 +784,6 @@ 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 {
@ -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())

64
swarm-controller/src/review_classifier/review.rs
View file

@ -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<String>, Option<String>, Outcome) {
) -> (Option<String>, 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<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 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}"
);
}
}

2
swarm-queue-client/src/review_classifier.rs
View file

@ -55,7 +55,7 @@ pub struct TryOut {
pub head_sha: String,
/// Oldest first.
pub commits: Vec<Commit>,
/// 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<Classification>,
/// The model every classification runs on, as claude's `--model` takes it.