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

swarm-controller: fall back to the forge diff when a file read fails, and stop rendering at the cap

Browse source 
A failure reading the changed files (a forge error on a tree or blob, a
truncated tree, a non-base64 blob) or joining the diff task made the PR
read fail, so the review posted no verdict. It now falls back to the
forge's 3-line .diff with "reading the files failed: …" as the note.

render wrote each file's whole diff before wide compared it with
MAX_BYTES. It now writes through a writer that refuses once past the cap,
which stops similar's formatter mid-file, so one huge file costs about
MAX_BYTES of output, not its full diff.
This commit is contained in:
atlas 2026-10-15 07:54:08 +02:00 • committed by mara
commit f4b5e1445b
3 changed files with 82 additions and 31 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -293,8 +293,9 @@ 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.
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

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

@ -1248,7 +1248,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 from the merge base: [`pull_diff::wide`]'s, or the
/// forge's `.diff` when the wide one is not read or is too large.
/// forge's `.diff` when the wide one is not read, cannot be read, or is
/// too large.
///
/// # Errors
/// A forge failure, with the [`ForgejoError`] underneath; a 404 on the
@ -1299,12 +1300,15 @@ impl Client {
(Some(reason), _) => Err(reason),
(None, None) => Err("the forge names no merge base".to_owned()),
(None, Some(base)) => {
let files = self
match 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")?
{
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 {
@ -2622,6 +2626,16 @@ mod tests {
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.

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

@ -4,7 +4,7 @@
//! 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::fmt::{self, Write as _};
use std::time::Duration;
@ -47,33 +47,56 @@ pub fn too_many_files(count: usize) -> Option<String> {
/// `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)
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.
pub fn render(files: &[FileChange], context: usize) -> String {
let mut out = String::new();
///
/// # 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 {
section(&mut out, file, context);
if section(&mut out, file, context).is_err() {
return Err(out.out.len());
}
}
out
Ok(out.out)
}
fn section(out: &mut String, file: &FileChange, context: usize) {
/// 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);
let _ = writeln!(out, "diff --git a/{old_path} b/{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 {
let _ = writeln!(out, "rename from {old_path}\nrename to {new_path}");
writeln!(out, "rename from {old_path}\nrename to {new_path}")?;
}
let a = file
.old
@ -84,17 +107,16 @@ fn section(out: &mut String, file: &FileChange, context: usize) {
.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;
return writeln!(out, "Binary files {a} and {b} differ");
};
let diff = TextDiff::configure()
.timeout(DIFF_TIMEOUT)
.diff_lines(old, new);
let _ = write!(
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.
@ -175,7 +197,7 @@ mod tests {
#[test]
fn three_lines_of_context_leave_that_gate_out() {
let diff = render(&move_into_glue(), 3);
let diff = render(&move_into_glue(), 3, usize::MAX).unwrap();
assert!(!diff.contains(GATE), "{diff}");
}
@ -183,7 +205,7 @@ mod tests {
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 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"));
@ -205,7 +227,9 @@ mod tests {
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\
@ -227,6 +251,18 @@ mod tests {
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());