From f0d93cd0fee936919d914e984120127504da2656 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 19 Oct 2026 19:34:39 +0200 Subject: [PATCH] the review classifier takes the merge base from the PR's commits, not the forge's stored merge_base --- docs/swarm/review-classifier.md | 13 ++++-- swarm-controller/src/forge.rs | 83 +++++++++++++++++++++++++++------ 2 files changed, 77 insertions(+), 19 deletions(-) diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index eb732898..edb83bea 100644 --- a/docs/swarm/review-classifier.md +++ b/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 -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. +A gate further away is still out of view. At review time the controller takes +the merge base from the PR's commits: the one parent that isn't a PR commit. +A rebased or force-pushed PR therefore gets the diff against the base it sits on +now, not the base it started from. The controller sends the forge's `.diff` +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 diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 162ffedf..bfdb3988 100644 --- a/swarm-controller/src/forge.rs +++ b/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 { + 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) -> Vec { let mut rows: Vec = 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>>) { @@ -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::>() + .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 = - r#"{"number":7,"requested_reviewers":[],"head":{"sha":"h1"},"changed_files":1}"#; - let (client, _) = pull_forge(no_base, old, new).await; + // The PR record still names the base it was opened on; the commits + // name the current one. + 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");