From f4b5e1445b797e80da58983ab2349cded56a1d02 Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 15 Oct 2026 07:54:08 +0200 Subject: [PATCH] swarm-controller: fall back to the forge diff when a file read fails, and stop rendering at the cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/swarm/review-classifier.md | 5 +- swarm-controller/src/forge.rs | 24 +++++-- swarm-controller/src/forge/pull_diff.rs | 84 ++++++++++++++++++------- 3 files changed, 82 insertions(+), 31 deletions(-) diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index f23f0c51..73054e05 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -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 diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index b9347970..e405c995 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -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. diff --git a/swarm-controller/src/forge/pull_diff.rs b/swarm-controller/src/forge/pull_diff.rs index 2972aa05..406929fe 100644 --- a/swarm-controller/src/forge/pull_diff.rs +++ b/swarm-controller/src/forge/pull_diff.rs @@ -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 { /// `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) + 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 { + 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());