From 463c5cbfcc0da8ecb6fad81bf9a3fbb2ffd0ca91 Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 24 Oct 2026 14:28:23 +0200 Subject: [PATCH] share the standing-review rule between hive-forge and the controller pr assign-reviewer behaves as before; the latest-review fold and the superseded check move to hive-types::review. --- Cargo.lock | 1 + hive-forge/Cargo.toml | 2 + hive-forge/src/verbs/mod.rs | 125 ++--------- hive-forge/src/verbs/pr_assign_reviewer.rs | 6 +- hive-types/README.md | 4 + hive-types/src/lib.rs | 5 +- hive-types/src/review.rs | 231 +++++++++++++++++++++ 7 files changed, 259 insertions(+), 115 deletions(-) create mode 100644 hive-types/src/review.rs diff --git a/Cargo.lock b/Cargo.lock index 2a6829f6..d18e0546 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1844,6 +1844,7 @@ dependencies = [ "clap", "clap-markdown", "forgejo-api", + "hive-types", "libc", "reqwest", "serde", diff --git a/hive-forge/Cargo.toml b/hive-forge/Cargo.toml index 1b6b6053..e264d25d 100644 --- a/hive-forge/Cargo.toml +++ b/hive-forge/Cargo.toml @@ -19,6 +19,8 @@ clap-markdown = "0.1" # not about the workspace. See the pin note on `forgejo-api` in the root # `Cargo.toml`. forgejo-api = { workspace = true, features = ["sync"] } +# The review rule `pr assign-reviewer` shares with swarm-controller. +hive-types.workspace = true url.workspace = true time.workspace = true reqwest = { workspace = true, features = [ diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index 61b993fa..c44017f6 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -346,66 +346,15 @@ fn edit_distance(a: &str, b: &str) -> usize { prev[b.len()] } -/// One reviewer's latest verdict on a PR, plus forgejo's `stale` / -/// `dismissed` bits. -/// -/// `stale` is set by forgejo when the PR head commit changed after this -/// review was submitted (branch protection then wants a fresh review); -/// `dismissed` is set when the review was explicitly dismissed. Either way -/// the review no longer applies to the current head even though its `state` -/// string still reads `APPROVED` / `REQUEST_CHANGES` — so surfacing them -/// stops the CLI from reporting a no-longer-valid review as still-good, and -/// [`ReviewInfo::superseded`] rolls both into one "doesn't count" check. -/// -/// ⚠️ **`stale` here is not forgejo's flag alone.** That flag is eventually -/// consistent: seconds after a push it still reports the pre-push answer, so -/// a verdict against the previous head reads as current in exactly the window -/// where someone runs the CLI right after pushing. [`latest_reviews`] therefore -/// ORs it with a direct comparison of the review's own `commit_id` against the -/// PR head — the flag is right *eventually*, the comparison is right -/// *immediately*, and either alone is worse than both. -pub(crate) struct ReviewInfo { - pub login: String, - pub state: String, - pub stale: bool, - pub dismissed: bool, -} - -impl ReviewInfo { - /// True when the review no longer applies to the current head — stale - /// (head moved) or dismissed. Such a verdict neither blocks a merge nor - /// counts as a fresh approval. - pub(crate) fn superseded(&self) -> bool { - self.stale || self.dismissed - } -} - -/// Whether this review was submitted against a commit that is no longer the -/// PR head — the race-free half of the staleness check. -/// -/// Fails **closed on unknowns**, i.e. "not stale": either side missing means -/// we cannot show the head moved, and the cost of guessing wrong in that -/// direction is one redundant re-review, where the other direction would -/// silently void every verdict on the PR (blocking nothing, but reporting a -/// ready PR as unreviewed and inviting a re-request that dismisses a real -/// approval). -fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bool { - let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else { - return false; - }; - // Both emptiness checks matter, and for the same reason: a blank string - // is a value the forge sent, not a sha it has. Treating one as real would - // make every review compare unequal and mark the whole PR stale — the - // direction this whole function exists to avoid. - !head.is_empty() && !reviewed.is_empty() && reviewed != head -} +pub(crate) use hive_types::review::ReviewInfo; /// Latest non-comment review per reviewer on a PR. Reviews come /// oldest-first, so a later verdict from the same user supersedes an /// earlier one; `COMMENT` / `PENDING` reviews carry no verdict and are -/// skipped. Shared by `pr status` (health view + readiness verdict) and -/// `pr merge` (pre-merge changes-requested gate) so the verdict semantics -/// stay in one place. +/// skipped. Shared by `pr status` (health view + readiness verdict), +/// `pr merge` (pre-merge changes-requested gate) and `pr assign-reviewer`, +/// and by swarm-controller's `request-reviewer` sink, through +/// [`hive_types::review`], so the verdict semantics stay in one place. /// /// # Errors /// @@ -439,32 +388,16 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result = Vec::new(); - for r in &reviews { - let Some(login) = r.user.as_ref().and_then(|u| u.login.as_deref()) else { - continue; - }; - let st = r.state.as_deref().unwrap_or(""); - if st == "COMMENT" || st == "PENDING" || st.is_empty() { - continue; - } - let stale = r.stale.unwrap_or(false) - || reviewed_older_head(r.commit_id.as_deref(), head_sha.as_deref()); - let dismissed = r.dismissed.unwrap_or(false); - if let Some(slot) = latest.iter_mut().find(|info| info.login == login) { - st.clone_into(&mut slot.state); - slot.stale = stale; - slot.dismissed = dismissed; - } else { - latest.push(ReviewInfo { - login: login.to_owned(), - state: st.to_owned(), - stale, - dismissed, - }); - } - } - Ok(latest) + Ok(hive_types::review::latest_reviews( + reviews.iter().map(|r| hive_types::review::Listed { + login: r.user.as_ref().and_then(|u| u.login.as_deref()), + state: r.state.as_deref(), + commit_id: r.commit_id.as_deref(), + stale: r.stale, + dismissed: r.dismissed, + }), + head_sha.as_deref(), + )) } /// The current dependency list for an issue or PR — each entry names @@ -684,7 +617,7 @@ pub(crate) fn attachment_json(assets: Option<&[Attachment]>) -> Vec { #[cfg(test)] mod tests { - use super::{NullableVec, pct_encode, refuse_json, reviewed_older_head}; + use super::{NullableVec, pct_encode, refuse_json}; #[test] fn refuse_json_bails_with_the_verb_and_reason() { @@ -715,32 +648,6 @@ mod tests { assert_eq!(v.0, vec![1, 2, 3]); } - #[test] - fn review_on_an_older_commit_is_stale() { - // The bug this exists for: forgejo still reports `stale: false` here - // in the seconds after a push, so the comparison has to catch it. - assert!(reviewed_older_head(Some("81292f14"), Some("f4c47088"))); - } - - #[test] - fn review_on_the_current_head_is_not_stale() { - assert!(!reviewed_older_head(Some("f4c47088"), Some("f4c47088"))); - } - - /// Every unknown fails *toward* keeping the verdict. Voiding every review - /// on a forge that stopped reporting one of these would be a far louder - /// wrong answer than one redundant re-review. - #[test] - fn unknown_commit_or_head_is_not_stale() { - assert!(!reviewed_older_head(None, Some("f4c47088"))); - assert!(!reviewed_older_head(Some("81292f14"), None)); - assert!(!reviewed_older_head(Some(""), Some("f4c47088"))); - // Absent and blank have to behave the same on BOTH sides — a blank - // head that counted as real would mark every review on the PR stale. - assert!(!reviewed_older_head(Some("81292f14"), Some(""))); - assert!(!reviewed_older_head(Some(""), Some(""))); - } - #[test] fn pct_encode_passes_unreserved_through() { // Plain artifact names round-trip verbatim — no performance diff --git a/hive-forge/src/verbs/pr_assign_reviewer.rs b/hive-forge/src/verbs/pr_assign_reviewer.rs index a36d941b..b36c8760 100644 --- a/hive-forge/src/verbs/pr_assign_reviewer.rs +++ b/hive-forge/src/verbs/pr_assign_reviewer.rs @@ -55,10 +55,8 @@ pub fn run(client: &Client, args: Args) -> Result<()> { // Guard the add path only — withdrawing a request is always safe. // Re-requesting from someone who already reviewed dismisses that review // instead of no-op'ing (see the module doc-comment above). - if let Some(existing) = super::latest_reviews(client, client.repo()?, args.number)? - .into_iter() - .find(|r| r.login == args.user && !r.superseded()) - { + let reviews = super::latest_reviews(client, client.repo()?, args.number)?; + if let Some(existing) = hive_types::review::standing(&reviews, &args.user) { println!( "not re-requesting: {} already has a standing {} review on #{} (re-requesting would dismiss it)", args.user, existing.state, args.number diff --git a/hive-types/README.md b/hive-types/README.md index 9a057917..34514019 100644 --- a/hive-types/README.md +++ b/hive-types/README.md @@ -17,6 +17,10 @@ Foundational shared **newtypes** for the hyperhive workspace. A zero-dependency - **`term_msg`** — the icons and summary prefixes of a terminal row that sent a message or started a turn from one, and the parsers that read the recipient or sender back out. +- **`review`** — which of a PR's reviews still stand (latest verdict per + reviewer, stale or dismissed ones superseded). `hive-forge pr assign-reviewer` + and the notification router's `request-reviewer` sink share it, so both skip a + request on the same grounds. The crate is deliberately a leaf with the smallest possible dependency footprint so anything in the workspace can depend on it. See `docs/process/conventions.md` diff --git a/hive-types/src/lib.rs b/hive-types/src/lib.rs index 0ef2e23d..84d834d4 100644 --- a/hive-types/src/lib.rs +++ b/hive-types/src/lib.rs @@ -1,5 +1,5 @@ -//! Foundational shared types for the hyperhive workspace: [`Ident`], [`Permission`] -//! and the [`term_msg`] row markers. +//! Foundational shared types for the hyperhive workspace: [`Ident`], [`Permission`], +//! the [`term_msg`] row markers and the [`review`] rule. //! //! A zero-dependency (bar `serde`) leaf crate so every wire-type crate //! (`hive-sh4re`, `hive-host-sock`, `hive-core-agent-sock`) and both binaries @@ -11,6 +11,7 @@ mod permission; pub use permission::Permission; +pub mod review; pub mod term_msg; /// The environment variable nix uses to hand this process the blacklist of diff --git a/hive-types/src/review.rs b/hive-types/src/review.rs new file mode 100644 index 00000000..d21360c6 --- /dev/null +++ b/hive-types/src/review.rs @@ -0,0 +1,231 @@ +//! Which of a PR's reviews still stand: the rule `hive-forge pr +//! assign-reviewer` and the notification router's `request-reviewer` sink +//! share, so a request is skipped or made on the same grounds in both. +//! +//! The forge keeps a review's `state` (`APPROVED`, `REQUEST_CHANGES`, …) after +//! the head moves or the review is dismissed, so reading the state alone +//! reports a review that no longer applies as still good. [`latest_reviews`] +//! folds a PR's reviews to one verdict per reviewer, and +//! [`ReviewInfo::superseded`] says whether it still counts. +//! +//! Re-requesting a review from a reviewer whose review stands makes the forge +//! dismiss it, approval included. [`standing`] is the check a caller makes +//! before it requests. + +/// One review as the forge lists it, reduced to what the rule reads. Every +/// field is optional because the forge's listing is. +#[derive(Clone, Copy, Debug, Default)] +pub struct Listed<'a> { + pub login: Option<&'a str>, + pub state: Option<&'a str>, + /// The commit the review was submitted against. + pub commit_id: Option<&'a str>, + /// The forge's own `stale` flag. + pub stale: Option, + pub dismissed: Option, +} + +/// One reviewer's latest verdict on a PR, plus the `stale` / `dismissed` bits. +/// +/// `stale` is set by the forge when the PR head commit changed after this +/// review was submitted; `dismissed` is set when the review was explicitly +/// dismissed. Either way the review no longer applies to the current head even +/// though its `state` string still reads `APPROVED` / `REQUEST_CHANGES`. +/// +/// ⚠️ **`stale` here is not the forge's flag alone.** That flag is eventually +/// consistent: seconds after a push it still reports the pre-push answer, so a +/// verdict against the previous head reads as current in exactly the window +/// where a push has just happened. [`latest_reviews`] therefore ORs it with a +/// direct comparison of the review's own `commit_id` against the PR head: the +/// flag is right *eventually*, the comparison is right *immediately*. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ReviewInfo { + pub login: String, + pub state: String, + pub stale: bool, + pub dismissed: bool, +} + +impl ReviewInfo { + /// True when the review no longer applies to the current head: stale + /// (head moved) or dismissed. Such a verdict neither blocks a merge nor + /// counts as a fresh approval. + #[must_use] + pub fn superseded(&self) -> bool { + self.stale || self.dismissed + } +} + +/// `login`'s review that still stands, if it has one. Its presence is the +/// reason not to request a review from `login` again. +#[must_use] +pub fn standing<'a>(reviews: &'a [ReviewInfo], login: &str) -> Option<&'a ReviewInfo> { + reviews.iter().find(|r| r.login == login && !r.superseded()) +} + +/// Whether this review was submitted against a commit that is no longer the PR +/// head: the race-free half of the staleness check. +/// +/// Fails **closed on unknowns**, i.e. "not stale": either side missing means +/// the head's move can't be shown, and guessing wrong in that direction costs +/// one redundant re-review, where the other direction would void every +/// verdict on the PR and invite a re-request that dismisses a real approval. +#[must_use] +pub fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bool { + let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else { + return false; + }; + // Both emptiness checks matter, and for the same reason: a blank string is + // a value the forge sent, not a sha it has. Treating one as real would make + // every review compare unequal and mark the whole PR stale. + !head.is_empty() && !reviewed.is_empty() && reviewed != head +} + +/// Latest non-comment review per reviewer. `reviews` come oldest first, so a +/// later verdict from the same reviewer supersedes an earlier one; `COMMENT` +/// and `PENDING` reviews carry no verdict and are skipped. `head_sha` is the +/// PR's current head, or `None` when it couldn't be read, which leaves the +/// forge's `stale` flag alone to decide. +#[must_use] +pub fn latest_reviews<'a>( + reviews: impl IntoIterator>, + head_sha: Option<&str>, +) -> Vec { + let mut latest: Vec = Vec::new(); + for r in reviews { + let Some(login) = r.login else { continue }; + let verdict = r.state.unwrap_or(""); + if verdict == "COMMENT" || verdict == "PENDING" || verdict.is_empty() { + continue; + } + let stale = r.stale.unwrap_or(false) || reviewed_older_head(r.commit_id, head_sha); + let dismissed = r.dismissed.unwrap_or(false); + if let Some(slot) = latest.iter_mut().find(|info| info.login == login) { + verdict.clone_into(&mut slot.state); + slot.stale = stale; + slot.dismissed = dismissed; + } else { + latest.push(ReviewInfo { + login: login.to_owned(), + state: verdict.to_owned(), + stale, + dismissed, + }); + } + } + latest +} + +#[cfg(test)] +mod tests { + use super::*; + + fn review<'a>(login: &'a str, state: &'a str, commit: &'a str) -> Listed<'a> { + Listed { + login: Some(login), + state: Some(state), + commit_id: Some(commit), + ..Listed::default() + } + } + + #[test] + fn review_on_an_older_commit_is_stale() { + // The forge still reports `stale: false` here in the seconds after a + // push, so the comparison has to catch it. + assert!(reviewed_older_head(Some("81292f14"), Some("f4c47088"))); + } + + #[test] + fn review_on_the_current_head_is_not_stale() { + assert!(!reviewed_older_head(Some("f4c47088"), Some("f4c47088"))); + } + + /// Every unknown fails *toward* keeping the verdict. Voiding every review + /// on a forge that stopped reporting one of these would be a far louder + /// wrong answer than one redundant re-review. + #[test] + fn unknown_commit_or_head_is_not_stale() { + assert!(!reviewed_older_head(None, Some("f4c47088"))); + assert!(!reviewed_older_head(Some("81292f14"), None)); + assert!(!reviewed_older_head(Some(""), Some("f4c47088"))); + // Absent and blank have to behave the same on BOTH sides: a blank head + // that counted as real would mark every review on the PR stale. + assert!(!reviewed_older_head(Some("81292f14"), Some(""))); + assert!(!reviewed_older_head(Some(""), Some(""))); + } + + #[test] + fn a_review_of_the_current_head_stands_and_one_of_an_older_head_does_not() { + let reviews = latest_reviews( + [ + review("review-classifier", "APPROVED", "new"), + review("argus", "APPROVED", "old"), + ], + Some("new"), + ); + assert_eq!( + standing(&reviews, "review-classifier").map(|r| r.state.as_str()), + Some("APPROVED") + ); + // The forge's flag is still false for argus: only the commit says so. + assert!(standing(&reviews, "argus").is_none()); + // Control: a reviewer who never reviewed has nothing standing. + assert!(standing(&reviews, "iris").is_none()); + } + + #[test] + fn a_dismissed_or_flagged_review_does_not_stand() { + let reviews = latest_reviews( + [ + Listed { + dismissed: Some(true), + ..review("a", "APPROVED", "new") + }, + Listed { + stale: Some(true), + ..review("b", "REQUEST_CHANGES", "new") + }, + review("c", "APPROVED", "new"), + ], + Some("new"), + ); + assert!(standing(&reviews, "a").is_none()); + assert!(standing(&reviews, "b").is_none()); + assert!(standing(&reviews, "c").is_some(), "control"); + } + + #[test] + fn the_latest_verdict_per_reviewer_wins_and_comments_carry_none() { + let reviews = latest_reviews( + [ + review("a", "REQUEST_CHANGES", "old"), + review("a", "APPROVED", "new"), + review("a", "COMMENT", "new"), + review("b", "PENDING", "new"), + Listed { + login: None, + ..review("x", "APPROVED", "new") + }, + ], + Some("new"), + ); + assert_eq!( + reviews, + [ReviewInfo { + login: "a".to_owned(), + state: "APPROVED".to_owned(), + stale: false, + dismissed: false, + }] + ); + } + + /// With the head unread, the forge's flag decides alone, so a review of an + /// unknown commit stands. + #[test] + fn an_unread_head_leaves_the_forges_flag_to_decide() { + let reviews = latest_reviews([review("a", "APPROVED", "old")], None); + assert!(standing(&reviews, "a").is_some()); + } +}