the review classifier takes the merge base from the PR's commits, not the forge's stored merge_base

docs/swarm/review-classifier.md

@@ -308,11 +308,14 @@ 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
+A gate further away is still out of view. At review time the controller takes
-controller sends the forge's `.diff` instead when the PR changes more than
+the merge base from the PR's commits: the one parent that isn't a PR commit.
-100 files, when the forge names no merge base, when reading a changed file
+A rebased or force-pushed PR therefore gets the diff against the base it sits on
-fails, or when the wide diff is over 256 KiB. The review then says so, with
+now, not the base it started from. The controller sends the forge's `.diff`
-the reason.
+instead when the PR changes more than 100 files, when the commits don't name
+a single merge base (the head merges the base branch in), 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

swarm-controller/src/forge.rs

@@ -107,6 +107,29 @@ pub struct OpenPull {
 
 /// `pulls` as the dropdown's rows, newest (highest number) first. A PR
 /// without a usable number cannot be named, so it is left out.
+/// The merge base of a PR's head and base branch, read off its commits (the
+/// forge lists them from the current head and base): the one parent that is
+/// not itself a PR commit. `None` when the PR has no commits or its commits
+/// reach more than one such parent (a merge of the base branch into the head).
+///
+/// The PR's own `merge_base` field is not used: the forge stores it and
+/// refreshes it asynchronously, so right after a rebase or force-push it
+/// still names the old base.
+fn merge_base_of(commits: &[forgejo_api::structs::Commit]) -> Option<String> {
+    let own: HashSet<&str> = commits.iter().filter_map(|c| c.sha.as_deref()).collect();
+    let outside: HashSet<&str> = commits
+        .iter()
+        .flat_map(|c| c.parents.iter().flatten())
+        .filter_map(|p| p.sha.as_deref())
+        .filter(|sha| !own.contains(sha))
+        .collect();
+    let mut outside = outside.into_iter();
+    match (outside.next(), outside.next()) {
+        (Some(base), None) => Some(base.to_owned()),
+        _ => None,
+    }
+}
+
 fn open_pulls(pulls: Vec<forgejo_api::structs::PullRequest>) -> Vec<OpenPull> {
     let mut rows: Vec<OpenPull> = pulls
         .into_iter()
@@ -1253,7 +1276,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
+    /// its unified diff from the merge base ([`merge_base_of`]): [`pull_diff::wide`]'s, or the
     /// forge's `.diff` when the wide one is not read, cannot be read, or is
     /// too large.
     ///
@@ -1272,12 +1295,12 @@ 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 changed_files = pull.changed_files;
         let head_sha = pull
             .head
             .and_then(|h| h.sha)
             .with_context(|| format!("PR {number} of {owner}/{repo} names no head commit"))?;
-        let commits = self
+        let listed_commits = self
             .api
             .call(|api| {
                 Box::pin(
@@ -1294,7 +1317,9 @@ impl Client {
                 )
             })
             .await
-            .with_context(|| format!("list the commits of PR {number} of {owner}/{repo}"))?
+            .with_context(|| format!("list the commits of PR {number} of {owner}/{repo}"))?;
+        let merge_base = merge_base_of(&listed_commits);
+        let commits = listed_commits
             .into_iter()
             .map(|c| Commit {
                 sha: c.sha.unwrap_or_default(),
@@ -1304,7 +1329,7 @@ impl Client {
         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, None) => Err("the PR's commits do not name a single merge base".to_owned()),
             (None, Some(base)) => {
                 match self
                     .changed_files(owner, repo, number, &base, &head_sha)
@@ -2574,11 +2599,15 @@ mod tests {
         (Client { api, url }, seen)
     }
 
+    /// One PR commit whose parent is `mb`.
+    const ON_MB: &str = r#"[{"sha":"c1","parents":[{"sha":"mb"}]}]"#;
+
     /// 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.
+    /// JSON, `commits` its commit list. Records every request's path.
     async fn pull_forge(
         pull: &'static str,
+        commits: &'static str,
         old: &'static str,
         new: &'static str,
     ) -> (Client, Arc<std::sync::Mutex<Vec<String>>>) {
@@ -2603,7 +2632,7 @@ mod tests {
             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/commits" => Some(commits.to_owned()),
                 "/api/v1/repos/o/r/pulls/7/files" => {
                     Some(r#"[{"filename":"m.nix","status":"modified"}]"#.to_owned())
                 }
@@ -2634,12 +2663,31 @@ mod tests {
         (Client { api, url }, seen)
     }
 
+    fn commit(sha: &str, parents: &[&str]) -> forgejo_api::structs::Commit {
+        let parents = parents
+            .iter()
+            .map(|p| format!(r#"{{"sha":"{p}"}}"#))
+            .collect::<Vec<_>>()
+            .join(",");
+        serde_json::from_str(&format!(r#"{{"sha":"{sha}","parents":[{parents}]}}"#)).unwrap()
+    }
+
+    #[test]
+    fn the_merge_base_is_the_one_parent_outside_the_pr() {
+        let linear = [commit("c2", &["c1"]), commit("c1", &["mb"])];
+        assert_eq!(merge_base_of(&linear).as_deref(), Some("mb"));
+        // A merge of the base branch into the head reaches two outside parents.
+        let merged = [commit("c2", &["c1", "main-tip"]), commit("c1", &["mb"])];
+        assert_eq!(merge_base_of(&merged), None);
+        assert_eq!(merge_base_of(&[]), None);
+    }
+
     #[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 (client, _) = pull_forge(wide, ON_MB, old, new).await;
         let read = client.pull_for_review("o", "r", 7).await.unwrap();
         assert_eq!(read.narrowed, None);
         assert!(
@@ -2656,7 +2704,7 @@ mod tests {
         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 (client, seen) = pull_forge(many, ON_MB, old, new).await;
         let read = client.pull_for_review("o", "r", 7).await.unwrap();
         assert_eq!(read.diff, "forge diff\n");
         assert_eq!(
@@ -2668,18 +2716,25 @@ mod tests {
             "no file is read for a PR past the limit"
         );
 
-        let no_base =
+        // The PR record still names the base it was opened on; the commits
-            r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"changed_files":1}"#;
+        // name the current one.
-        let (client, _) = pull_forge(no_base, old, new).await;
+        let stale = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"old-base","changed_files":1}"#;
+        let (client, _) = pull_forge(stale, ON_MB, old, new).await;
+        let read = client.pull_for_review("o", "r", 7).await.unwrap();
+        assert_eq!(read.narrowed, None, "the diff is read from `mb`");
+        assert!(read.diff.contains("\n-    e = 5;\n"), "{}", read.diff);
+
+        let no_commits = r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"merge_base":"mb","changed_files":1}"#;
+        let (client, _) = pull_forge(no_commits, "[]", 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")
+            Some("the PR's commits do not name a single 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 (client, _) = pull_forge(unreadable, ON_MB, 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");